Implement a Regex based Check on Expressions

[JKomoroski]

There are a few things that result in Json headers getting truncated. Firstly, headers appear to get processed more than one time during the RequestTemplate#resolve method. I reviewed that code and could not come up with simple way to address the issue without possibly breaking or making major changes to the external Request Template contract.

This fixes #1464

My solution was to simply validate that the string being passed to the expression code was actually an expression.

I don't know about code formatting or style on this. I'd like to see it back ported to the 11.x release if possible.

See my issue comment for more context.

[JKomoroski]

@velo You've reviewed some PRs that were seeking to fix this issue in the past. Are you the right person to review this?

• Attempt to fix truncated json literals in header values #1638
• Fix Truncated JSON literals in Headers Values #1766

@kdavisk6 You committed the change that introduced the regression. You may be well equipped to offer insight.

[JKomoroski]

Rebased after a dependabot bump

[kdavisk6]

While I don't have specific feedback on this change, I'd rather try to understand why the templates are being processed more than once, instead of attempting to make an educated guess at usage of the template.

[JKomoroski]

While I don't have specific feedback on this change, I'd rather try to understand why the templates are being processed more than once, instead of attempting to make an educated guess at usage of the template.

It basically comes down to the request storing a map of header templates and exposing that as a public api.

The code that resolves those templates by processing the header expressions turns the resolved espression strings back into templates to return them to the map, which triggers another pass through the code that checks for literals vs expressions.

Since inputs aren't really sanitized it processes the JSON as an expression.

An alternative would be to refactor header template with a processed flag or something, but I didn't want to modify any public interfaces -- that seemed far more invasive and less likely to be accepted.

Stick a breakpoint in the modified expression method and run the added unit tests in the request template resolve method. You'll hit the breakpoint at least twice. It's pretty easy to follow up the stack trace

[kdavisk6]

I ended up creating an alternative that does modify HeaderTemplate, but doesn't expose any new methods on RequestTemplate #1781

[kdavisk6]

An alternative would be to refactor header template with a processed flag or something, but I didn't want to modify any public interfaces -- that seemed far more invasive and less likely to be accepted.

We are pretty open to these types of changes, you should go with your gut on these and we'll work with you to get them merged.

[JKomoroski]

We are pretty open to these types of changes, you should go with your gut on these and we'll work with you to get them merged.

Formally defining what a legal Feign expression is, seemed like the place to start. It aligns with the goals on the readme as well.

If your PR is lower risk and fixes the issue that's great. Let's get it merged and released asap. I'd like to understand the concerns with using a regex to define the bounds of an expression as well.

I use feign everyday and I'd like to get context to contribute more.

[kdavisk6]

I'd like to understand the concerns with using a regex to define the bounds of an expression as well.

I don't have any concerns with using a regex here, however; by your own analysis, it doesn't correct the root cause, when appending already resolved headers, they are treated like expressions again.

For this minor nuance, I'd consider this PR an enhancement over a bug fix for #1464.

Regardless, it's a good change, and we'll take it.

[JKomoroski]

It sounds like the immediate problem #1464 (JSON headers being truncated) is fixed in #178.

Are you maintainer folks are on board with tightening down this expression validation, I'd be happy to take another pass at this.

There is quite a bit of the expression processing code that could be improved by it. And I think we could prep the code base to implement the rest of the modified RFC for Version 12.

Thoughts?

[kdavisk6]

I'll approve this and then feel free to look at #1019 for more information.

[Onarki]

Hi @kdavisk6, I don't know if this is the right place to ask, just wondering when this fix is going to be released ?
Thanks in advance