Attempt to fix truncated json literals in header values #1638
Conversation
| @@ -38,6 +38,11 @@ | |||
| <optional>true</optional> | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
| <groupId>org.json</groupId> | |||
| <artifactId>json</artifactId> | |||
There was a problem hiding this comment.
sorry mate, this is not going to happen.
feign core shall have no dependencies
There was a problem hiding this comment.
Alternate would be use regex to validate the json string without any external dependencies. I can already see a feign-json library which uses org.json to encode/decode json. I assume its the right place to add the json literal validation. But we would still need to add the feign-json library into feign-core to use the implementation.
There was a problem hiding this comment.
all modules have multiple dependecies.... feign-core does not
But we would still need to add the feign-json library into feign-core to use the implementation
sorry, that won't be approved.
| @@ -214,7 +216,7 @@ private void parseFragment(String fragment) { | |||
| /* check to see if we have an expression or a literal */ | |||
| String chunk = tokenizer.next(); | |||
|
|
|||
| if (chunk.startsWith("{")) { | |||
| if (chunk.startsWith("{") && !isValidJsonLiteral(fragment)) { | |||
There was a problem hiding this comment.
@velo This is the fix I think we need to resolve this issue. Check if the string is a json literal before parsing it as a template. So if you agree with this solution and I have to add this check here, what would be the best approach please? Open for suggestions.
There was a problem hiding this comment.
What about the regex option you proposed?
There was a problem hiding this comment.
I can try. The regex looks a bit complicated and yet to confirm if it breaks the template string. Let me add that with a few tests and update the PR. However I must add using a library is more elegant and remove complicated stuff under the hood which we don't want to implement ourself.
There was a problem hiding this comment.
What if you add a new boolean to @Header(expression = false) default it to true and then change this if to:
if (chunk.startsWith("{") && header.expression()) {
There was a problem hiding this comment.
feel free to pick any other name besides expression
There was a problem hiding this comment.
I prefer this approach. Let’s add a property to the header annotation indicating that it should be treated like a literal. What about naming the property “literal” and be a Boolean?
There was a problem hiding this comment.
It will avoid any regular expressions and simplify the logic to be if “literal” is true, just render otherwise, resolve the expression. There should be a “Literal” type I think in the template package. I think.
There was a problem hiding this comment.
Please remember annotation isn't the only way of setting the header
There was a problem hiding this comment.
yes. This will avoid any assumption about the expression/literal.
There was a problem hiding this comment.
Although adding a variable like literal in Headers annotation wouldn't always be right since headers values is a string array, it's possible one of header has an expression and one is a literal. A variable would only work out if we can tailor the new variable for every header added to the array. That will be an expensive change.
Fix for #1464
Attempted a simple fix for the problem where If the value of header, body, queryparam etc starts with {, check if its a json literal before attempting to consider it as an expression.
Assuming unit tests cover all the cases, this fix works as expected for json literals. Need help identifying if this will impact any other expressions.
(Too many file changes because of license header. Changes only in feign core pom, Template and HeaderTemplateTest)