Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feign HeaderTemplates adds an extra space to date headers after commas #1270

Closed
muhamadto opened this issue Sep 10, 2020 · 9 comments · Fixed by #1347
Closed

Feign HeaderTemplates adds an extra space to date headers after commas #1270

muhamadto opened this issue Sep 10, 2020 · 9 comments · Fixed by #1347
Labels
bug Unexpected or incorrect behavior

Comments

@muhamadto
Copy link

muhamadto commented Sep 10, 2020

HeaderTemplates#expand(Map<String, ?>) adds space after each comma in headers. This causes an issue when converting date that uses the EEE, d MMM yyyy HH:mm:ss Z format.

Example
Client code sends Wed, 4 Jul 2001 12:08:56 -0700
Provider expects Wed, 4 Jul 2001 12:08:56 -0700
Provider receives Wed, 4 Jul 2001 12:08:56 -0700 please note the extra space between the , and 4

@kdavisk6
Copy link
Member

@muhamadto Can you please provide a minimal working example that demonstrates this issue?

@kdavisk6 kdavisk6 added the bug Unexpected or incorrect behavior label Sep 18, 2020
@gtay003
Copy link

gtay003 commented Sep 24, 2020

We've just been hit by this issue too, it seems to affect any expanded header with a comma. Example code attached. The log output shows that the provided date value has an additional space inserted after the comma:

12:34:33.537 [main] DEBUG feign.Logger - [SampleApi#run] ---> GET https://reqres.in/api/users?page=2 HTTP/1.1
12:34:33.538 [main] DEBUG feign.Logger - [SampleApi#run] Date: Thu,  24 Sep 2020 10:34:09 GMT
12:34:33.538 [main] DEBUG feign.Logger - [SampleApi#run] ---> END HTTP (0-byte body)

The issue was likely caused by this change: #1203

Downgrading to 10.9 (last version before this change was made) has fixed for us for now.

feign-header-bug.zip

@jjz921024
Copy link

hi, i made pr #1295 for fix this issue, but CI build failed because of The command "./travis/no-git-changes.sh" exited with 1

i don't know how to make it build successfully

@gtay003
Copy link

gtay003 commented Oct 29, 2020

hi, i made pr #1295 for fix this issue, but CI build failed because of The command "./travis/no-git-changes.sh" exited with 1

i don't know how to make it build successfully

Looks like it's complaining about code formatting; did you see this in the build output?

Please run 'mvn clean install' locally to format files.

At a guess I'd say it's because you have two blank lines above your new unit test method, and the formatter expects only one. But I guess mvn clean install will fix this for you.

@jjz921024
Copy link

hi, i made pr #1295 for fix this issue, but CI build failed because of The command "./travis/no-git-changes.sh" exited with 1
i don't know how to make it build successfully

Looks like it's complaining about code formatting; did you see this in the build output?

Please run 'mvn clean install' locally to format files.

At a guess I'd say it's because you have two blank lines above your new unit test method, and the formatter expects only one. But I guess mvn clean install will fix this for you.

Thank for you reply. you are right, it is caused by formatting, but it's because there are blank lines in import statement, not unit test method

@kdavisk6
Copy link
Member

I'm beginning to see that we'll have to "bite the bullet" and upgrade the Template handling sooner than Feign 11. The root cause of all of these issues is that our Template Handling relies on String manipulation for templates that can support Collections of values. Single values work fine.

Since HTTP Headers are generally considered Collection based, this problem will continue to appear. I can see that in #1298 an attempt is make to clear out extra spaces, but that solution will work for the example provided only and not generally across all headers. I'm going to link this to the template update issue in #1019 and see if we can start by separating general URI templates with Headers first.

kdavisk6 added a commit to kdavisk6/feign that referenced this issue Dec 29, 2020
Fixes: OpenFeign#1270

`HeaderTemplate` was confusing iterable values with literal values due
to the presence of comma `,` characters in the result.  The result was
that, in certain cases like HTTP Dates, additional spaces were inserted
into the final expanded value.

The root cause of the issue is that `HeaderTemplate` combined all values
into a single `String` template, with each value separated by a comma.

This change refactors `HeaderTemplate` to treat all `values` as individual
`Templates`, removing the need to combine any provided values into a single
String.
kdavisk6 added a commit that referenced this issue Dec 29, 2020
Fixes: #1270

`HeaderTemplate` was confusing iterable values with literal values due
to the presence of comma `,` characters in the result.  The result was
that, in certain cases like HTTP Dates, additional spaces were inserted
into the final expanded value.

The root cause of the issue is that `HeaderTemplate` combined all values
into a single `String` template, with each value separated by a comma.

This change refactors `HeaderTemplate` to treat all `values` as individual
`Templates`, removing the need to combine any provided values into a single
String.

* Remove unnecessary string splits when expanding Headers in RequestTemplate
@superfive666
Copy link

May I know if currently there is a fix for spring-cloud-starter-openfeign?
Using spring-cloud.version = 2020.0.1;
My current workaround is headers.forEach(h -> h.replace(", ", ",");

@deniskichan
Copy link

Hello!
Where do i need to put this code to solve the problem?

@superfive666
Copy link

@deniskichan You just need to manipulate the headers you sent in each individual requests.
But have you tried upgrade to the newest version first?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants