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

Headers set with @HeaderParam ignored by DefaultClientHeadersFactoryImpl #25619

Closed
softmetz opened this issue May 17, 2022 · 8 comments · Fixed by #26389
Closed

Headers set with @HeaderParam ignored by DefaultClientHeadersFactoryImpl #25619

softmetz opened this issue May 17, 2022 · 8 comments · Fixed by #26389
Assignees
Labels
area/rest-client kind/bug Something isn't working
Milestone

Comments

@softmetz
Copy link

softmetz commented May 17, 2022

Describe the bug

If a header set via @HeaderParam has the same name as a propagated header coming via incoming request, the incoming value overwrites the value set via annotation.

I debugged into org.eclipse.microprofile.rest.client.ext.DefaultClientHeadersFactoryImpl and found out, that in method update the value of the annotated parameter is in clientOutgoingHeaders which is not used at all.

Expected behavior

The parameter bound via annotation @HeaderParam should have a higher priority that the incoming header

Actual behavior

The incoming header gets used instead of the one given via @HeaderParam

How to Reproduce?

  1. Create a Restclient with a parameter void something(@HeaderParam("authorization") String authorizationHeader);
  2. call the code via REST and give an authorization header
  3. The outgoing call contains the value of the incoming header

Output of uname -a or ver

Linux nb0mws009 5.10.0-14-amd64 #1 SMP Debian 5.10.113-1 (2022-04-29) x86_64 GNU/Linux

Output of java -version

openjdk version "17.0.3" 2022-04-19 OpenJDK Runtime Environment (build 17.0.3+7-Debian-1deb11u1) OpenJDK 64-Bit Server VM (build 17.0.3+7-Debian-1deb11u1, mixed mode, sharing)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.9.0

Build tool (ie. output of mvnw --version or gradlew --version)

Java version: 17.0.3, vendor: Debian, runtime: /usr/lib/jvm/java-17-openjdk-amd64 Default locale: de_DE, platform encoding: UTF-8 OS name: "linux", version: "5.10.0-14-amd64", arch: "amd64", family: "unix"

Additional information

No response

@softmetz softmetz added the kind/bug Something isn't working label May 17, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented May 17, 2022

/cc @michalszynkiewicz

@geoand
Copy link
Contributor

geoand commented May 17, 2022

Which rest client extension are you using?

@softmetz
Copy link
Author

Reactive (JSONB)

@michalszynkiewicz
Copy link
Member

If the issue is in the MicroProfile class, I think it's best to report it there.

Can you create a simple reproducer?

@softmetz
Copy link
Author

@michalszynkiewicz I am not sure it is. Is the DefaultClientHeadersFactoryImpl actually supposed to handle the clientOutgoingHeaders? Before I switched to REST reactive (from rest-easy) it worked as expected.

I just had a look at the history (https://github.com/eclipse/microprofile-rest-client/commits/master/api/src/main/java/org/eclipse/microprofile/rest/client/ext/DefaultClientHeadersFactoryImpl.java) of the class in MP and it never took on clientOutgoingHeaders.

@michalszynkiewicz
Copy link
Member

I'll dig into it, can you create a small reproducer?

@softmetz
Copy link
Author

I have created https://github.com/softmetz/quarkusio-25619-header-propagation

There are two test cases, one calling an public endpoint and one a secured one using a bearer token.

The latter one will fail, because the value of authorization gets propagated regardless of @HeaderParam. I hope that helps.

                                               Request was not matched
                                               =======================

-----------------------------------------------------------------------------------------------------------------------
| Closest stub                                             | Request                                                  |
-----------------------------------------------------------------------------------------------------------------------
                                                           |
GET                                                        | GET
/hello                                                     | /hello
                                                           |
authorization: Bearer fromSource                           | authorization: Bearer fromRequest                   <<<<< Header does not match
                                                           |
                                                           |
-----------------------------------------------------------------------------------------------------------------------

Please note, that I used 2.9.1, so the problem is still there.

@michalszynkiewicz michalszynkiewicz self-assigned this Jun 27, 2022
@michalszynkiewicz
Copy link
Member

The classic REST client behaves exactly the same as the reactive one in this scenario.

The difference in the behavior that you observe comes from the fact that REST Client Reactive (RCR) always registers the DefaultClientHeadersFactoryImpl while the classic client registers it only if the interfaces is annotated with @RegisterClientHeaders. The latter is how the spec describes it should work.

I'll create a pull request to fix RCR to follow the spec.

michalszynkiewicz added a commit to michalszynkiewicz/quarkus that referenced this issue Jun 27, 2022
michalszynkiewicz added a commit to michalszynkiewicz/quarkus that referenced this issue Jun 27, 2022
@quarkus-bot quarkus-bot bot added this to the 2.11 - main milestone Jun 28, 2022
@gsmet gsmet modified the milestones: 2.11 - main, 2.10.1.Final Jun 28, 2022
gsmet pushed a commit to gsmet/quarkus that referenced this issue Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest-client kind/bug Something isn't working
Projects
None yet
4 participants