Implements possibility to disable @Param url encoding by parkee · Pull Request #465 · OpenFeign/feign

parkee · 2016-09-26T14:02:08Z

Now it is possible with @Param(encoded = true) to disable hardcoded call of urlEncode method on every @Param
This helps to implement matrix params or send unencoded path variables or query parameters which is required by some APIs

codefromthecrypt

Mind adding some tests to exemplify this? Also, can you note which api this helps with? (being concrete is always better)

codefromthecrypt · 2016-09-26T15:32:40Z

    protected RequestTemplate resolve(Object[] argv, RequestTemplate mutable,
                                      Map<String, Object> variables) {
-      return mutable.resolve(variables);
+      final HashMap<String, Boolean> variableToEncoded = new HashMap<String, Boolean>();


LinkedHashMap pls

parkee · 2016-09-26T16:13:30Z

Added one test to exemplify this behaviour and changed HashMap to LinkedHashMap
We are using matrix params in one of out internal API
Also wheel-size api will not accept encoded parameter: i.e. trim should be "5.2FSi+", not "5.2FSi%2b":
So this request won't work
https://api.wheel-size.com/v1/vehicles/?user_key=USER_KEY&make=audi&model=r8&year=2015&trim=5.2FSi%2b
But not encoded will:
https://api.wheel-size.com/v1/vehicles/?user_key=USER_KEY&make=audi&model=r8&year=2015&trim=5.2FSi+
In some cases it would be path variable, not query parameter

codefromthecrypt

some changes, also need to update CHANGES. very close!

ps thanks for the help

codefromthecrypt · 2016-09-29T00:36:10Z

-          String name = ((Param) annotation).value();
-          checkState(emptyToNull(name) != null, "Param annotation was empty on param %s.",
-              paramIndex);
+          final Param paramAnnotation = (Param) annotation;


nit we don't usually mark locals final unless doing so is notable (ex used in an anonymous type)

codefromthecrypt · 2016-09-29T00:37:30Z

+          String name = paramAnnotation.value();
+          checkState(emptyToNull(name) != null, "Param annotation was empty on param %s.", paramIndex);
          nameParam(data, name, paramIndex);
-          if (annotationType == Param.class) {


weird.. we actually checked twice :P

codefromthecrypt · 2016-09-29T00:38:59Z

+          data.indexToEncoded().put(paramIndex, paramAnnotation.encoded());
          isHttpAnnotation = true;
          String varName = '{' + name + '}';
-          if (data.template().url().indexOf(varName) == -1 &&


fair enough!

codefromthecrypt · 2016-09-29T00:39:38Z


+  /**
+   * Specifies whether argument is already encoded
+   * Value be ignored for headers (headers are never encoded)


good clarification

probably should also put @see QueryMap#encoded as well putting a see link there back to here

codefromthecrypt · 2016-09-29T00:41:32Z

    protected RequestTemplate resolve(Object[] argv, RequestTemplate mutable,
                                      Map<String, Object> variables) {
-      return mutable.resolve(variables);
+      final Map<String, Boolean> variableToEncoded = new LinkedHashMap<String, Boolean>();


add a comment of what's going on here. (also pls unfinal the locals)

codefromthecrypt · 2016-09-29T00:42:43Z

    map.put(key, values);
  }

+  public RequestTemplate resolve(Map<String, ?> unencoded) {


add see link

ex.

/** like {@link #resolve(Map, Map)}, which assumes no parameter is encoded */

codefromthecrypt · 2016-09-29T00:43:33Z

   * Replaces query values which are templated with corresponding values from the {@code unencoded}
   * map. Any unresolved queries are removed.
   */
-  public void replaceQueryValues(Map<String, ?> unencoded) {


does this break api? if so, please add an overload

codefromthecrypt · 2016-09-29T00:44:01Z

    void queryMapWithQueryParams(@Param("name") String name, @QueryMap Map<String, Object> queryMap);

+    @RequestLine("GET /?trim={trim}")
+    void encodedQueryParam(@Param(value = "trim", encoded = true) String trim);


codefromthecrypt · 2016-10-01T01:23:32Z

   * Replaces query values which are templated with corresponding values from the {@code unencoded}
   * map. Any unresolved queries are removed.
   */
-  public void replaceQueryValues(Map<String, ?> unencoded) {


please overload with the prior signature so that we don't break other people's code.

? It's already done.

diff problem no worries

codefromthecrypt · 2016-10-01T01:23:49Z

   * map. Any unresolved queries are removed.
   */
-  public void replaceQueryValues(Map<String, ?> unencoded) {
+  public void replaceQueryValues(Map<String, ?> unencoded, Map<String, Boolean> alreadyEncoded) {


does this need to be public (ex are you using this directly?)

I see no reason in original method being public either, so I decided to leave overloaded one public too to be consistent.
Should I make new method private instead of overloading public method?

package private por favor

codefromthecrypt · 2016-10-01T01:24:44Z

@@ -1,5 +1,6 @@
 ### Version 9.3
 * Adds `FallbackFactory`, allowing access to the cause of a Hystrix fallback
+* Adds support for encoded parameters in @Param via @Param(encoded = true)


simplify and format? Ex.

Adds support for encoded parameters via @Param(encoded = true)

codefromthecrypt

k just some nits, then lets merge this

codefromthecrypt · 2016-10-01T15:38:22Z

@@ -1,5 +1,6 @@
 ### Version 9.3
 * Adds `FallbackFactory`, allowing access to the cause of a Hystrix fallback
+* Adds support for encoded parameters via @Param(encoded = true)


nit, if you get used to doing backticks, not only will the format look nicer, but you won't accidentally mention someone named Param :P

codefromthecrypt · 2016-10-01T15:39:39Z

   */
-  public RequestTemplate resolve(Map<String, ?> unencoded) {
-    replaceQueryValues(unencoded);
+  public RequestTemplate resolve(Map<String, ?> unencoded, Map<String, Boolean> alreadyEncoded) {


for this and below, package private is fine (as opposed to private). these are new methods, so we can hide them until someone asks. package private is better as it can be tested directly (plus the bytecode is smaller)

codefromthecrypt · 2016-10-01T15:40:28Z

   * Replaces query values which are templated with corresponding values from the {@code unencoded}
   * map. Any unresolved queries are removed.
   */
-  public void replaceQueryValues(Map<String, ?> unencoded) {


diff problem no worries

codefromthecrypt · 2016-10-01T15:40:38Z

   * map. Any unresolved queries are removed.
   */
-  public void replaceQueryValues(Map<String, ?> unencoded) {
+  public void replaceQueryValues(Map<String, ?> unencoded, Map<String, Boolean> alreadyEncoded) {


package private por favor

Implements possibility to disable @Param url encoding

4fbdb8e

codefromthecrypt reviewed Sep 26, 2016

View reviewed changes

Changes HashMap to LinkedHashMap and adds test for encoded param usage

1fc2892

codefromthecrypt reviewed Sep 29, 2016

View reviewed changes

Pavlo Klivak added 2 commits September 30, 2016 12:06

Adds some javadocs, unfinals local variables + minor cleanups

121f20c

Updates CHANGELOG

db9dd36

codefromthecrypt reviewed Oct 1, 2016

View reviewed changes

Simplifies changelog statement

752f0e6

codefromthecrypt approved these changes Oct 1, 2016

View reviewed changes

Changes new methods visibility from public to package-private

7d6a63c

codefromthecrypt merged commit deda96a into OpenFeign:master Oct 1, 2016

codefromthecrypt mentioned this pull request Oct 4, 2016

Issue on QueryMap when the value start with '{' #470

Closed

velo pushed a commit that referenced this pull request Oct 7, 2024

Implements possibility to disable @Param url encoding (#465)

e2d8bd3

velo pushed a commit that referenced this pull request Oct 8, 2024

Implements possibility to disable @Param url encoding (#465)

3bf889e

Uh oh!

Conversation

parkee commented Sep 26, 2016

Uh oh!

codefromthecrypt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parkee commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codefromthecrypt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

parkee commented Sep 26, 2016 •

edited

Loading