OpenFeign / feign

Feign makes writing java http clients easier
Apache License 2.0
9.49k stars 1.93k forks source link

RequestTemplate.encodeIfNotVariable needs improvement. #675

Closed omi2012 closed 5 years ago

omi2012 commented 6 years ago

Hi,

Substitution of parameter doesn't work when the parameter is preceded by text.

@RequestLine("GET "+BASEURI+"endpoint?param=sometext{paramname}")
 Returntype myQuery(@Param("paramname") String paramname)

as { would not be the first char, see

 private static String encodeIfNotVariable(String in) {

    if (in == null || in.indexOf('{') == 0) {

      return in;

    }

    return urlEncode(in);

  }

The {paramname} would get encoded into %7Bparamname%7D, which would not get replaced further up in the code.

kdavisk6 commented 6 years ago

I wonder if this is an issue or something that should be documented more clearly.

rage-shadowman commented 6 years ago

Kinda ugly, but you could workaround it by removing the prefix from the request line and using an Expander to prepend some text to the string.

omi2012 commented 6 years ago

Another issue I've observed is some API has param~=value this would break too.

On Thu, Apr 19, 2018 at 8:41 PM, Shadow Man notifications@github.com wrote:

Kinda ugly, but you could workaround it by removing the prefix from the request line and using an Expander to prepend some text to the string.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/OpenFeign/feign/issues/675#issuecomment-382925882, or mute the thread https://github.com/notifications/unsubscribe-auth/ASzeuuLbTJTv7MI5fX3YBA_9sd1os82xks5tqS60gaJpZM4TKpAN .

-- Best regards,

Oleg Mitsura omitsura@gmail.com

kdavisk6 commented 6 years ago

I think this could be better solved by updating the documentation to indicate that our URI template handling is very simple and only supports simple string replacement and is not the same as doing a regular expression match and replace.

omi2012 commented 6 years ago

We use Infoblox. One of it's API requests for lookup of networks uses param~=value From cursory look at OpenFeign code, there is some tokenization happening on key=value which gets disassembled t hen reassembled. If that can be avoided, then such lookups would work.

On Wed, Jul 25, 2018 at 8:20 PM, Kevin Davis notifications@github.com wrote:

I think this could be better solved by updating the documentation to indicate that our URI template handling is very simple and only supports simple string replacement and is not the same as doing a regular expression match and replace.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/OpenFeign/feign/issues/675#issuecomment-407936767, or mute the thread https://github.com/notifications/unsubscribe-auth/ASzeutTtJJ2h7ActZ5fHSeBvbQiQhKPXks5uKQtbgaJpZM4TKpAN .

-- Best regards,

Oleg Mitsura omitsura@gmail.com

rage-shadowman commented 6 years ago

@omi2012 core feign works that way, but there are other flavors of the feign stuff that use jackson or spring annotations (etc) and they work in different ways using different Contracts if you wanted to give one of them a shot.

I think the original feign concept was just to be a very simple library with some simple assumptions that held true for the early users in their niche cases, but doesn't hold true for some of the newer users and their niche cases. This particular one doesn't seem like something that could be easily changed without breaking functionality (ex: some people expect query parameters specified in the interface to be removed from the URL if they are null).

kdavisk6 commented 6 years ago

Feign is not RFC 6570 compliant and it's support for parsing uri information is very simple. In lieu of making changes to core, a custom Contract is the preferred way to override the default uri, method, and parameter identification.

kdavisk6 commented 5 years ago

Closed via #778