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

[resteasy-reactive] [3.8.6] Header not reset between request #45789

Open
ZetalyAlexandre opened this issue Jan 22, 2025 · 13 comments
Open

[resteasy-reactive] [3.8.6] Header not reset between request #45789

ZetalyAlexandre opened this issue Jan 22, 2025 · 13 comments
Labels
area/rest kind/bug Something isn't working

Comments

@ZetalyAlexandre
Copy link

Describe the bug

When a header is injected directy in a Endpoint class, this header is not clean between two request.

Exemple:

@Path("/hello")
public class GreetingResource {

    @HeaderParam("filter")
    String filter;

    @GET
    @Produces(MediaType.TEXT_PLAIN)
    public String hello() {
        return "Header: %s".formatted(filter);
    }
}

On first call without the header filter, the value of filter is null.
On second call with the header filter with the value 'my filter', the value of filter is 'my filter'.
On third call without the header filter, the value of filter is 'my filter'.

FI: I have the same issue with the 3.15.3

Expected behavior

On third call without the header filter, the value should be null.

Actual behavior

The third call keep the value of the second one

How to Reproduce?

repro-header.zip

Small reproducer. Use the /hello with a header header once with a value, and without a value.

Output of uname -a or ver

No response

Output of java -version

openjdk version "17.0.12" 2024-07-16

Quarkus version or git rev

3.8.6

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

apache-maven-3.9.0

Additional information

No response

@ZetalyAlexandre ZetalyAlexandre added the kind/bug Something isn't working label Jan 22, 2025
Copy link

quarkus-bot bot commented Jan 22, 2025

/cc @FroMage (rest), @geoand (rest), @stuartwdouglas (rest)

@geoand
Copy link
Contributor

geoand commented Jan 22, 2025

Does this problem also occur in Quarkus 3.15? Never mind, I just saw the answer in the description

@geoand geoand added triage/needs-feedback We are waiting for feedback. and removed triage/needs-feedback We are waiting for feedback. labels Jan 22, 2025
@sberyozkin
Copy link
Member

Probably a request scope is missing, for smallrye-jwt we started failing builds when non-proxiable types like String are injected into application scoped beans

@geoand
Copy link
Contributor

geoand commented Jan 22, 2025

Ah good point @sberyozkin , String is indeed not proxyable!

@geoand
Copy link
Contributor

geoand commented Jan 22, 2025

Everything works file if you make GreetingResource @RequestScoped.
@FroMage I wonder if we should be doing that automatically when field injection is being used like this... Or we should at least print a strong worded warning

@ZetalyAlexandre
Copy link
Author

The solution is to declared the class as @RequestScoped ? Or to inject the parameter to each function ?

@RequestScoped
@Path("/hello")
public class GreetingResource {

    @HeaderParam("filter")
    String filter;

    @GET
    @Produces(MediaType.TEXT_PLAIN)
    public String hello() {
        return "Header: %s".formatted(filter);
    }
}

Or

@Path("/hello")
public class GreetingResource {
    @GET
    @Produces(MediaType.TEXT_PLAIN)
    public String hello(@HeaderParam("filter") String filter) {
        return "Header: %s".formatted(filter);
    }
}

@geoand
Copy link
Contributor

geoand commented Jan 22, 2025

Or to inject the parameter to each function ?

This is always the better option

@FroMage
Copy link
Member

FroMage commented Jan 22, 2025

Everything works file if you make GreetingResource @RequestScoped. @FroMage I wonder if we should be doing that automatically when field injection is being used like this... Or we should at least print a strong worded warning

Wow that's scary. This should be an error. Any request-scoped value injected into fields in a non-request-scoped bean should be rejected at build time. All @Rest* and @*Param, as well as implicit @Context objects that aren't beans (I assume beans have an automatic proxy that supports request-scoped access) fields into a non-request-scoped bean.

Otherwise, given an application-scoped bean, we will inject parameter fields left and right on the same objects while requests are happening, leading to huge breakage, since we handle this sort of injection ourselves.

Pretty amazing this never manifested earlier 😱

@geoand
Copy link
Contributor

geoand commented Jan 22, 2025

Pretty amazing this never manifested earlier 😱

I agree

@geoand
Copy link
Contributor

geoand commented Jan 23, 2025

@FroMage do you want to make this a build failure? It goes without saying that we can't backport it as it's a breaking change.

@gastaldi
Copy link
Contributor

+1 to fail on build, as it's a programming error. Bonus points if the IDE plugin could detect that

@FroMage
Copy link
Member

FroMage commented Jan 23, 2025

Definitely a build failure. This is way too dangerous to be allowed. We should break older applications by backporting it. The risk is pretty huge.

But also, is it normal that we default to anything else besides @RequestScoped when we have any injection points in the endpoint?

@geoand
Copy link
Contributor

geoand commented Jan 23, 2025

But also, is it normal that we default to anything else besides @RequestScoped when we have any injection points in the endpoint?

That was one of my proposals for fixing this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants