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

ReactiveClientHeadersFactory does not work with internal Uni #24153

Closed
SimonScholz opened this issue Mar 7, 2022 · 6 comments
Closed

ReactiveClientHeadersFactory does not work with internal Uni #24153

SimonScholz opened this issue Mar 7, 2022 · 6 comments
Labels
area/kotlin kind/bug Something isn't working triage/out-of-date This issue/PR is no longer valid or relevant

Comments

@SimonScholz
Copy link

Describe the bug

Hi,

I've already made some comments in the original pull request, which introduced the ReactiveClientHeadersFactory https://github.com/quarkusio/quarkus/pull/21807/files and @Sgitario asked me to open a bug for this issue, which also contains a reproducer.

The issue is that the following code for a ReactiveClientHeadersFactory works using the Uni.createFrom().item() method...

class AuthRequestHeaderFactory : ReactiveClientHeadersFactory() {

    override fun getHeaders(incomingHeaders: MultivaluedMap<String, String>?): Uni<MultivaluedMap<String, String>> {
        return Uni.createFrom().item(
            MultivaluedHashMap<String, String>().apply {
                add("Cache-Control", "no-cache")
                add("Accept-Encoding", "identity")
                add("Authorization", "Basic secret")
            }
        )
    }
}

... while this does not:

class BeverageRequestHeaderFactory(
    @RestClient private val authRestClient: AuthRestClient,
) : ReactiveClientHeadersFactory() {

    override fun getHeaders(incomingHeaders: MultivaluedMap<String, String>?): Uni<MultivaluedMap<String, String>> {
        return authRestClient.fetchClientToken().onItem().transform {
            MultivaluedHashMap<String, String>().apply {
                add(HttpHeaders.CACHE_CONTROL, "no-cache")
                add(HttpHeaders.AUTHORIZATION, "${it.tokenType} ${it.accessToken}")
            }
        }
    }
}

Actually the latter tries to use the auth token, which is received by the rest client, which uses the ReactiveClientHeadersFactory from the first snippet.

The code, which reproduces the issue can be found here:

https://github.com/SimonScholz/ReactiveClientHeadersFactory/blob/b1a7a07c13afa9168b8897653fa27ce0f4cf3be3/src/test/kotlin/io/github/simonscholz/ReactiveResourceTest.kt#L82

The test case shows that the auth rest client can be called in a proper manner by using the AuthRequestHeaderFactory from above, while BeverageRequestHeaderFactory does not work.

When printing all requests, which reached out towards Wiremock, which is used for testing, we can see that also for the BeverageRestClient the values from the AuthRequestHeaderFactory is being used, which is wrong.
It should've been using the BeverageRequestHeaderFactory, which is specified via @RegisterClientHeaders(BeverageRequestHeaderFactory::class) in the BeverageRestClient class.

Please let me know if something is still unclear or if you have questions...

Thanks a lot in advance for your support. :-)

Expected behavior

The expected behavior would be that the BeverageRequestHeaderFactory properly sets the header values for the BeverageRestClient in the reproducer example.

Actual behavior

Unfortunately it seems that the values from the AuthRequestHeaderFactory are being used for the BeverageRestClient as well, even though it should pick up the header value from the BeverageRequestHeaderFactory.

The weird thing also is that if I make the fetchClientToken call blocking it works....

How to Reproduce?

I've created this sample code in order to reproduce the issue we're facing:

https://github.com/SimonScholz/ReactiveClientHeadersFactory/blob/b1a7a07c13afa9168b8897653fa27ce0f4cf3be3/src/test/kotlin/io/github/simonscholz/ReactiveResourceTest.kt#L82

In this test case in line 82 I print all requests, which were made towards Wiremock.
With that in place I was able to see that the wrong header values are being used for the BeverageRestClient.

Feel free to clone the Github repository, I've created, and run the test.

Output of uname -a or ver

No response

Output of java -version

Java 17.0.1 Temurin

GraalVM version (if different from Java)

OpenJDK Runtime Environment GraalVM CE 21.3.0

Quarkus version or git rev

2.7.1

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

Gradle 7.3.3

Additional information

I've already made some comments in the original PR, which introduced the ReactiveClientHeadersFactory.
Also see https://github.com/quarkusio/quarkus/pull/21807/files

@SimonScholz SimonScholz added the kind/bug Something isn't working label Mar 7, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 7, 2022

/cc @evanchooly

@michalszynkiewicz
Copy link
Member

@SimonScholz can you try Quarkus from the current main branch? It should be fixed in #24099
I don't know yet if this will be backported to 2.7

@SimonScholz
Copy link
Author

SimonScholz commented Mar 7, 2022

@michalszynkiewicz Thanks for the quick response. Can I get the snapshot version of the main branch from a certain artifact repository?
Or do I have to build it locally and deploy it to my local maven repository?

@michalszynkiewicz
Copy link
Member

I have never tried it but guessing from Github Actions we have, this can be the repository to use: https://oss.sonatype.org/content/repositories/snapshots

@geoand
Copy link
Contributor

geoand commented Mar 8, 2022

I have never tried it but guessing from Github Actions we have, this can be the repository to use: https://oss.sonatype.org/content/repositories/snapshots

Yes, this is built nightly

@geoand
Copy link
Contributor

geoand commented Aug 30, 2022

I am going to assume this was fixed. If it's not, please comment and we can revisit the issue.

@geoand geoand closed this as not planned Won't fix, can't repro, duplicate, stale Aug 30, 2022
@geoand geoand added triage/invalid This doesn't seem right triage/out-of-date This issue/PR is no longer valid or relevant and removed triage/invalid This doesn't seem right labels Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kotlin kind/bug Something isn't working triage/out-of-date This issue/PR is no longer valid or relevant
Projects
None yet
Development

No branches or pull requests

3 participants