@RequestHeader with null value set templated name as value

[tjuchniewicz]

Continuation of https://github.com/spring-cloud/spring-cloud-netflix/issues/1594

Looks like bug or feature in RequestTemplate:

 Object value = variables.get(var.toString());
 if (value != null) {
     builder.append(value);
} else {
    builder.append('{').append(key).append('}');
}

[fmcejudo]

what do you think it should be instead, when it tries to replace with a null value? it might throw an exception with illegal state if it is part of the context path and removing them from query and headers

[tjuchniewicz]

I'm not sure if null is valid value. In my workaround (RequestInterceptor) I remove such header from request.

[kdavisk6]

I did some detective work, the bug goes all the way back to the initial import from Netflix.
It looks like there have been a attempts in the past to address other issues related in #326 and #348 and had a very specific use case it was trying to solve. The idea is to prevent accidental parameter expansion.

In my opinion, the current implementation feels fragile and my need some refactoring. However, the code is making an assumption that if there is no value for the parameter to be expanded, it assumes you want the literal. It also does not differentiate between variables within the body, or header, which I feel is where the issue truly lies.

One possible approach could be to introduce an VariableExpanderStrategy concept, with one implementation that does not replace the token if a value is missing and another that does.

public class Expander {
   public String expand(String template, Map<String, ?> uriVariables, VariableExpanderStrategy strategy) {
      ...
   }
}

public class RemoveMissingExpanderStrategy implements VariableExpanderStrategy {
   public String expand(String template, Map<String, ?> uriVariables) {
      // if no variable exists for the token, remove it
   }
}

public class TokenLiteralExpanderStrategy implements VariableExpanderStrategy {
   public String expand(String template, Map<String, ?> uriVariables) {
      // if no variable exists for the token, treat the token as a literal string
   }
}

Thoughts?

[fmcejudo]

well, I think it might work. Based on the @kdavisk6 idea, it might be as an annotation like @FeignExpanderStrategy with default TokenLiteralExpanderStrategy and implementing some different choices. So, when it does the expand then it might take them in account.

At the moment not for @RequestHeaders, but for @Headers if they are null, I have a pull request for that what mitigates the problem avoiding headers like {headerName}

#724

[kdavisk6]

#724 has been accepted and removed headers that do not have a value. This should address your issue in the next release.

[kdavisk6]

#724 has been merged. This will go out with the next release. I'm going to close this issue for now.