Remove query template empty argument filtering

[finnetrolle]

To allow empty values appear in request.
I've found only one way to change it. I don't like that both query string and arguments parsed via one class. Also removed code was added to create filter I have removed. It's strange. But test works fine after removing filter.

[kdavisk6]

@finnetrolle

The spec states that we should ignore any undefined values. While your changes appear to fix the issue, it's only masking it. The real fix belongs in the expand functions. In particular, we need to return a special value when an expression is unresolved and not an empty string.

I suggest that, if you are still interested, look at Template, specifically lines 88 - 91. We should probably return a special value when resolvedExpression is null. Take a look and let me know. If you are unsure, I can help

[finnetrolle]

@kdavisk6 roger that. I'll take a look

[finnetrolle]

After we expand vars map in Template we have fine result @ feign/RequestTemplate.java:182
QueryStiring built perfectly for my test and looks like people=first&people=&people=third
But when I'm on feign/RequestTemplate.java:196 I can see that resolved.queries is empty. Then I run 196 line and voila - on the line feign/RequestTemplate.java:199 I have resolved.queries fill with people -> people=first&people=third

Bad moment is when we goes to feign.RequestTemplate#extractQueryTemplates. We provide fine data and then RequestTemplate ruins out work with his filter. I don't know, may be I can add some specific value such as "EMPTY" (this is not null, it's like empty) on preparing stage and parse it replacing EMPTY with "", but this is a string received from string and there is a chance user make same...

[kdavisk6]

@finnetrolle

You are on the right track. We need to explicitly call out when a value is undefined and then update our filtering logic to excluded those undefined values. One way is to assume that null is undefined. Another is to use a constant, like your suggestion, but that does have some risks.

[finnetrolle]

Hi @kdavisk6
I've spent some more time on debugging and see next things:

1. We can't operate with null. Inside feign.RequestTemplate#resolve(java.util.Map<java.lang.String,?>) we're creating a String from given values and that's all - we'll never see values later in feign.RequestTemplate#uri(java.lang.String, boolean) method. To get values we parse uri and then process Iterable<String>. We can substitute "" to null in some method close to QueryTemplate call, but in this case looks like we need to know which method called this stack - processAnnotationOnMethod or feign.ReflectiveFeign.BuildTemplateByResolvingArgs#resolve. Due to fine atomicity of the code it will became a several changes in method's parameter lists. And even if we good - filter inside QueryTemplate will work because ::isNotBlank clear nulls too.

2. Special string value to substitute empty string. This case is risky because client can use combination we use to define that case. We can substitute empty string in many places. Even in the places we never get while in processAnnotationOnMethod. For example your proposal - feign/template/Template.java:88 or feign.template.Template#resolveExpression. Or even inside feign.template.Expressions.SimpleExpression#expand. We can use space as a symbol we'll never see in result uri string (because of encoding), but we can see it in headers or body.

Looks like we have the two inceptions - for annotation and for reflective clients, that call one method - uri() and then must call different methods in QueryTemplate - one with filtering and other without. Or risk with substituted String value... I really need your advice at that point.

[kdavisk6]

This has been superseded by #1138