UriUtils.encode should not skip already encoded strings

[Zordid]

I ended up here using feign because of a seemingly simple task: encode a String using RFC 3986.

Against the odds, this turns out to be quite complicated. Java fails because the URLEncoder is not for URIs, due to the Space=+... Apache does not work, Guava does not have a decoder...

Now feign... I couldn't believe my eyes when my property based tests failed to properly encode the literal String "%07"... I look into the code and find stuff like "skips already encoded strings"... :(

This is a very bad design decision in itself, but even worse: how does one now encode such a literal string? I cannot find an option to disable this (for me) unwanted behaviour...

Thanks!

[kdavisk6]

Much of Feign's uri-template handling adheres to RFC 6570, where states Section 1.2

URI Templates are similar to a macro language with a fixed set of macro definitions: the expression type determines the expansion process. The default expression type is simple string expansion, wherein a single named variable is replaced by its value as a string after pct-encoding any characters not in the set of unreserved URI characters (Section 1.5).

In our library, we will ensure that all items are pct-encoded when present on a URI. This is different from URL encoding slightly, and as you are now aware, not consistently supported due to variances in server technology, age, and general misuse across applications. We have an entire section in our documentation dedicated to this issue, specifically around slashes and plus signs.

Feign makes a best effort to ensure that the resulting URI will pct-encode per RFC 6570. While we understand this may not work for every single scenario, we've found that it works for the vast majority of them. If you find that the results are not what you expect, you can override the handling by either implementing a custom Expander for the troublesome parameter, or implement a RequestInterceptor to change the entire URI before processing.

[jebeaudet]

I just hit the same problem as @Zordid today to find that a simple test test path parameter gets properly encoded to test%20test but if I pass test%20test, Feign will detect it as already encoded here and skip the encoding all together so I end up with the same test%20test in the end instead of the proper test%2520test.

I understand the "best effort" tentative here but with no easy way to disable this behavior, it's really unfortunate. With a RequestInterceptor, I have no way of determining if the test%20test in the template was already encoded by Feign or if it was determined to be already encoded. An Expander would work only on @Param. Is there any other option out there?

[jebeaudet]

OK I managed to get a Expander working, however, I still hit some issues with encoding path parameters myself for example, test = test within a path segment gets encoded to test%20=%20test but the method UriUtils::isEncoded detects this string as non encoded because of the = and double encode it. Having a = not encoded within a path segment is totally valid as there are different rules for path/query/fragment (I love this source for a easy to read reference).

I'm actually using org.springframework.web.util.UriUtils which correctly consider the differences in encoding depending if you're encoding a path segment, query, queryString, etc.

Feign cannot accurately guess if the passed string is encoded or not by just scanning it like it does right now. IMO, it should never consider them encoded (is there a use case where you get a method parameter that is already encoded?), or it should have a switch to tell feign that the parameter passed are already encoded, either at the parameter level of the client level.

WDYT?

Thanks