Support additional URI Template Levels

[ddoubleday]

In versions of Feign before 10.x, if the URI template looked like http://base/action/{arg1}/{arg2}, and null was passed as the value to arg1, the URI

http://base/action/{arg1}/somevalue

would be tried, and rejected with IllegalArgumentException. That probably wasn't the right thing to do, but NOW, the null is actually resolved, and the situation is actually worse, resulting in the URI

http://base/action//someValue

resulting in, if you are using Spring Boot 2, a "not normalized URI exception" because they are doing security scans on URIs and disallowing consecutive slashes.

Even worse, if a value is passed to arg1 and null is passed to arg 2 and it is resolved, it can map to the entirely wrong controller method as:

http://base/action/someValue/

if these are GET operations, that might get the entire list associated with someValue, instead of the intended one value "someValue2".

Shouldn't feign refuse to resolve PATH variables if the supplied argument is null, and just throw some kind of NotNullable exception?

[kdavisk6]

@ddoubleday

Feign does it's best to adhere to RFC 6570 - Uri Templates which states:

2.3. Variables
An expression MAY reference variables that are unknown to the
template processor or whose value is set to a special "undefined"
value, such as undef or null. Such undefined variables are given
special treatment by the expansion process (Section 3.2.1).
A variable value that is a string of length zero is not considered
undefined; it has the defined value of an empty string.

3.2.1. Variable Expansion
A variable that is undefined (Section 2.3) has no value and is
ignored by the expansion process. If all of the variables in an
expression are undefined, then the expression's expansion is the
empty string.

Given this, Feign treats null values as undef values and will ignore them in the process. There is no expectation that this treatment changes based on where the expression is defined.

With that said, the specification does have support for your use case via Path Segment Expansion, which is consider a Level 3 expansion. Feign currently supports Level 1 or simple expansion at this time.

With all of that, the behavior you are seeing is expected.

[wilkystorm]

I second the concerns raised by @ddoubleday and am facing the same issues that he has enumerated above. Is it in the Feign roadmap to support these use cases and in effect correct this unintended behavior via support for level 3 path segment expansion or otherwise?

[kdavisk6]

I think that we should have support for all template levels. We are open contributions to this end if any one is interested and would welcome the help.

I have a working implementation in our feignx project that I am planning on back-porting, but I do not have a definitive timeframe on when that will be complete.

[kdavisk6]

I've marked this as needing votes. If we get enough interest, we can move this out of proposal state and into an enhancement state.

[ddoubleday]

All respect to you folks, but I don't think the renaming of this issue from "null should not be resolved in URITemplate" to "support additional URI Template Levels" actually fairly represents the problem.

It should NEVER be acceptable to replace path components with an empty string, whether you support Level 3 expansion or not. I guess another way to say this is that you have a serious bug in your code if you AREN'T supporting an expansion process that recognizes that path variables are a special case.

This issue as titled now seems like this is a "nice to have", but it really isn't.

[wilkystorm]

I agree with @ddoubleday. It should NEVER be acceptable to replace path components with an empty string, whether you support Level 3 expansion or not. Please let us know when this defect is resolved. Thank you!

[velo]

Please let us know when this defect is resolved. Thank you!

Waiting on your pull request.

[wilkystorm]

good point LOL. Maybe we should start with defining the behavior that should be expected in these cases and then go from there as to who/how will fix it. @ddoubleday thoughts?

[kdavisk6]

https://tools.ietf.org/html/rfc6570#section-3.2.6

This specification states that empty values, that is zero length strings, are not considered undefined. In Feign null=undefined. This means the following is valid:

var = value
who = fred

{/var,empty}       /value/
{/var,empty,who}   /value//fred
{/var,undef,who}   /value/fred

With regards to the original issue, since the template does not use the path syntax of {/arg1} and the simple version /{arg1}/{arg2} a null or empty for arg1 will result in the same result //arg2. Hence why I stated that if we were able to support the additional levels, we can recommend changing the template to a more appropriate form:

http://base/action{/arg1}{/arg2}

With the following results:

arg1 = undef
arg2 = somevalue

http://base/action/somevalue

All of this behavior is defined in the specification and there are some test case files created and maintained by the authors here:

https://github.com/uri-templates/uritemplate-test

I have a compliant implementation in feignx: https://github.com/OpenFeign/feignx/tree/master/core/src/main/java/feign/template

I just need find the time to port it over ⌚️

[bushwakko]

Is this the same issue I'm seeing when using JAXRSContract and a query-parameter "uri=http://test"

It ends up as "http%3A//test" instead of the expected: "http%3A%2F%2Ftest". This seems to be because encodeSlash is false.