Open sguillope opened 4 years ago
@sguillope
Short Answer
What are experiencing is the result of some recent refactoring to achieve consistency across all of Feign. The most recent change that is responsible for the behavior you are seeing is #1156, where we were applying different uri encoding rules when expanding values provided within a @QueryMap
, where the decodeSlash
override was not being applied, when it should have.
One thing that is puzzling to me is that you are unable to provide the value encoded already. Feign should be able to recognized pct-encoded values and leave them as is. I'll do some research on our end to see if there is something amiss there. This should work:
Input | Expected |
---|---|
path%2Fvariable |
path%2Fvariable |
Long Answer
Feign treats all expressions as Level 1 templates as defined in Uri Template Specification RFC 6570, which declares the following:
If no operator is provided, the expression defaults to simple variable expansion of unreserved values.
So we encode all values that are not in the unreserved set. The only exception, which you've noticed is the slash /
character. Our documentation outlines this behavior:
What about slashes? /
@RequestLine
templates do not encode slash / characters by default. To change this behavior, set thedecodeSlash
property on the@RequestLine
to false.
This means that slash values are not encoded by default. To override this behavior, set the decodeSlash
property on the @RequestLine
annotation to false
.
I understand this is not ideal and how this is may be frustrating for you. We do have a more complete solution outlined in #1019; however, this is a non-backward compatible, breaking change that we have decided to move to our next major release.
Hi @kdavisk6, thanks for the thorough reply, much appreciated.
Unfortunately because we use Spring Cloud OpenFeign, I believe there isn't much control we have over the decodeSlash
parameter (arguably that's a Spring Cloud problem)
That said, I've added more test cases to my test project sguillope/openfeign-url-encoding-issue with using pre-encoded path variables.
Test Case | Input | Result | Expected |
---|---|---|---|
Encoded / |
path%2Fvariable |
path/variable |
path%2Fvariable |
Encoded : and / |
a%3Apath%2Fvariable |
a%3Apath/variable |
a%3Apath%2Fvariable |
Unencoded : and encoded / |
a:path%2Fvariable |
a%3Apath%252Fvariable |
a:path%2Fvariable |
Encoded : and unencoded / |
a%3Apath/variable |
a%253Apath/variable |
? |
For some reason, when Feign detects the variable as already being encoded, it ends up decoding %2F
back to /
.
As you can see without control over the decodeSlash
behaviour it's impossible to get the /
to be encoded.
Ok. Let me noodle on it a bit. I’ll see what I can think up.
So there is no way to specify that one does not want to encode :
in a path-param? 🤔
@Klaboe, @sguillope
There is, however it's a bit of a sledgehammer. You can provide a custom Param.Expander
instance on @Param
annotation, but again, that's only for vanilla Feign. The Spring annotations do not expose that option, as they are an extension to the contract and Spring has no equivalent, as its URI expansion logic is different from ours.
It is situations like this one that make #1019 so important. We, core Feign, cannot guarantee consistent behavior when custom Contract
extensions are used. However, if we were to support the URI template specification more completely, we will be able to ensure consistent expansion across all Contract
extensions as the expansion of the URI would no longer be coupled to the Contract
.
Again, I apologize for the inconsistencies. In the short term, I am considering adding a new method to the Feign.Builder#decodeSlash
to allow users to override decodeSlash
for an entire interface Target. This would allow users to disable this behavior completely, but, like the Param.Expander
options, it's a sledgehammer, all-or-nothing deal.
Feign.builder()
.decodeSlash(false)
.build("https://api.com", Interface.class);
For those using Spring, you will need to declare your Feign Clients manually for this to work:
Is this something that can work for either of you?
Thanks @kdavisk6 for taking the time to look into this and posting a reply. On our end for now we've blocked the upgrade of Spring Cloud OpenFeign in our services and will probably leave it as is until it's fixed. Or if not upgrading becomes an issue for us then we'll look into other options.
Cheers!
Probably; i use vanilla feign, so i will look into Param.Expander
and see if this helps. Thanks for the feedback!
Is there a better solution now?
Since we are using SpringMvcContract
now, and we have to override it.
@Override
protected void processAnnotationOnMethod(MethodMetadata data, Annotation methodAnnotation, Method method) {
// Note: append empty URI to avoid NPE of template()#uriTemplate.
data.template().uri("", true);
data.template().decodeSlash(false);
super.processAnnotationOnMethod(data, methodAnnotation, method);
}
BTW, RequestTemplate#decodeSlash()
should check uriTemplate
to avoid NPE.
I am using feignclient but encoding of url is not proper for sort object.
Ex: &page=0&size=2000&sort=ChangeDT: ASC&sort=RecordedDT: ASC producing in this way. but wanted it to be like -> &sort=ChangeDT,ASC&sort=RecordedDT,ASC
@Harishrv You will need to reach out to Spring Cloud OpenFeign for this one. They own that encoder.
Hello, can I ask you is here any progress?
I too have a similar issue. I'm using wiremock to get response mapping which fails as the encoding isn't as expected.
¶m=value1%2Cvalue2
instead of
¶m=value1,value1
I'm using spring-boot-starter-parent, version 2.3.0.RELEASE AND spring-cloud-dependencies, version 2020.0.0
Same issue here, in my case I was trying to integrate with Google Analitycs 4, they require parameters with ":" in the requests.
I solved it by changing this
@FeignClient
interface AwesomeClient {
@PostMapping("/awesome_request")
fun request(
@RequestParam("param_with_colon") param: String
)
}
To this
@FeignClient
interface AwesomeClient {
@PostMapping("/awesome_request")
fun request(
@SpringQueryMap(encoded = true) params: ParamWithColon
)
}
class ParamWithColon(
val param_with_colon: String
)
this behaviour is also true when you have either a path parameter or a query parameter that might be base64 encoded which usually contains a =
sign that is swapped by %3D
and may change the outcome.
for now, the solution for this is to have a request interceptor
kotlin
...
val URL_ENCODED_EQUAL_SIGN: String = "%3D"
...
class UrlEncodingFixInterceptor : RequestInterceptor {
override fun apply(template: RequestTemplate) {
template.uri(template.url().replace(URL_ENCODED_EQUAL_SIGN, "="))
}
}
val feignBuilder = HystrixFeign.builder().....requestInterceptor(UrlEncodingFixInterceptor())...
From a URI stand point, =
and %3D
are the same. That goes for ,
and %2C
, as both of those characters are in the reserved set per RFC 3986.
There have been a number of updates to Feign in this area since this issue was first reported. As of this comment, Feign adheres to RFC 6570 - Simple String Expansion or Level 1 URI templates with limited support for Path Style Expressions.
Additional changes are needed to support the other use cases mentioned:
As with all of the issues present here, we are open to Pull Requests and contributions on this topic.
Solution: To Turn off decodeSlash:
Thanks to @zqq90
@FeignClient(name = "yourClient", url= "xxxx", configuration = YourClient.ClientConf.class)
public interface YourClient {
class ClientConf{
@Bean
Contract contract(@Autowired(required = false) List<AnnotatedParameterProcessor> parameterProcessors,
ConversionService feignConversionService) {
if (parameterProcessors == null) {
parameterProcessors = new ArrayList<>();
}
return new SpringMvcContract(parameterProcessors, feignConversionService, false);
}
}
}
Or:
feign.client.decodeSlash=false
This issue is probably related to #1189 but is focused on the encoding of path variables instead. We've recently upgraded to Spring Cloud to
Hoxton.SR3
which includes a bump of Spring Cloud OpenFeign from2.2.1.RELEASE
to2.2.2.RELEASE
. This patch version bumps OpenFeign from 10.4.0 to 10.7.4 which seems to contain quite a few important changes.In particular, there is a change of behaviour around url encoding (#1138) which in our case has broken a couple of things with using path variables. I've created a sandbox project at https://github.com/sguillope/openfeign-url-encoding-issue to make it easier to reproduce the issue.
The 2 problems we've hit are as follows when using a path variable:
:
character now gets url-encoded to%3A
. While not really incorrect, it's not actually required as a colon is a valid character in a path segment (https://tools.ietf.org/html/rfc3986#section-3.3), except for one case which shouldn't apply here. This only broke our integration tests, not production code, so we can easily fix that./
character. With the latest version, it doesn't get url-encoded in path variables. Fortunately this was caught by our integration tests, otherwise it would have broken our production code. We can't pre-encode it because it will then get double-encoded as%252F
instead of%2F
.a:path/variable
a%3Apath/variable
a:path%2Fvariable
path/variable
path/variable
path%2Fvariable
a:path
a%3Apath
a:path
Let me know if you need more details. Thanks