Change of URL encoding behaviour doesn't seem right

[sguillope]

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 from 2.2.1.RELEASE to 2.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:

• The colon : 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.
• The most problematic one is the handling of the forward slash / 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.

To summarise:

Input	Result	Expected
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

[kdavisk6]

@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 the decodeSlash 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.

[sguillope]

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.

[kdavisk6]

Ok. Let me noodle on it a bit. I’ll see what I can think up.

[Klaboe]

So there is no way to specify that one does not want to encode : in a path-param? 🤔

[kdavisk6]

@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:

https://cloud.spring.io/spring-cloud-static/spring-cloud-openfeign/2.2.2.RELEASE/reference/html/#creating-feign-clients-manually

Is this something that can work for either of you?

[sguillope]

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!

[Klaboe]

Probably; i use vanilla feign, so i will look into Param.Expander and see if this helps. Thanks for the feedback!

[zqq90]

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.

[Harishrv]

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

[kdavisk6]

@Harishrv You will need to reach out to Spring Cloud OpenFeign for this one. They own that encoder.