quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.64k stars 2.64k forks source link

PathParam injection doesn't work in subresources in Quarkus REST (formerly RESTEasy Reactive) #39800

Open tanadeau opened 6 months ago

tanadeau commented 6 months ago

Describe the bug

Per JAX-RS, request-scoped subresources have the ability to have parent path parameters, etc. injected. This works correctly for Quarkus RESTEasy and has for years.

We are looking at transitioning to Quarkus REST, and the transition was quite painless until we ran our tests and saw that almost all of our subresources were broken at runtime because none of the parent path parameters were injected.

See discussion in another ticket starting at https://github.com/quarkusio/quarkus/issues/37148#issuecomment-1826296595.

Expected behavior

@PathParam from a parent resource works and is properly injected in subresources in Quarkus REST as it is in Quarkus RESTEasy.

Actual behavior

Using Quarkus REST, @PathParam injection doesn't occur, and the fields are kept to their default value (usually null).

How to Reproduce?

Reproducer: code-with-quarkus-bug-reproducer.zip

Steps to reproduce:

  1. Run with Quarkus REST: ./mvnw compile quarkus:dev
  2. Use curl to request subresource endpoint: curl http://localhost:8080/hello/2/sub where "2" is the parent path param
  3. Expected response is "Hello from SubResource of Hello 2". Actual response is "Hello from SubResource of Hello null"

If you change quarkus-rest to quarkus-resteasy in pom.xml and re-run the steps above, the expected response is produced.

Output of uname -a or ver

Darwin QJRQ3G49V3-ML 23.4.0 Darwin Kernel Version 23.4.0: Fri Mar 15 00:12:49 PDT 2024; root:xnu-10063.101.17~1/RELEASE_ARM64_T6020 arm64

Output of java -version

openjdk version "21.0.2" 2024-01-16 OpenJDK Runtime Environment Homebrew (build 21.0.2) OpenJDK 64-Bit Server VM Homebrew (build 21.0.2, mixed mode, sharing)

Quarkus version or git rev

3.9.1

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

Apache Maven 3.9.6 (bc0240f3c744dd6b6ec2920b3cd08dcc295161ae) Maven home: /Users/nadeata1/.m2/wrapper/dists/apache-maven-3.9.6-bin/3311e1d4/apache-maven-3.9.6 Java version: 21.0.2, vendor: Homebrew, runtime: /opt/homebrew/Cellar/openjdk/21.0.2/libexec/openjdk.jdk/Contents/Home Default locale: en_US, platform encoding: UTF-8 OS name: "mac os x", version: "14.4.1", arch: "aarch64", family: "mac"

Additional information

No response

quarkus-bot[bot] commented 6 months ago

/cc @FroMage (resteasy-reactive), @Ladicek (arc), @geoand (resteasy-reactive), @manovotn (arc), @mkouba (arc), @stuartwdouglas (resteasy-reactive)

geoand commented 6 months ago

@FroMage looks like one for you 😉

FroMage commented 6 months ago

I've never touched any subresource-related code. I may be able to fix this, but I'm not sure when I'll have time for this, TBH. Does any other injection already work in subresources? Probably there's field zero injection ATM.

geoand commented 6 months ago

Yeah but you've done all the injection / transformation stuff 😉

geoand commented 6 months ago

Does any other injection already work in subresources

I don't recall

tanadeau commented 6 months ago

Other injections appear to work from what I can tell. Our code uses subresources pretty significantly, and fields injections for Request, SecurityIdentity, etc. work fine. As another FYI: I can get the parent path param as a parameter injection to endpoint methods; however, the field injection doesn't work.

tanadeau commented 5 months ago

Is there anything I can do to help fix or diagnose this? If there's a place in the code that you can point me to, I'd be happy to look.

FroMage commented 5 months ago

A contribution would be great indeed, let me help you help us :)

First, what does the method returning your locator look like? Is it returning an instance, or a Class? I'm not familiar with resource locators, last time I saw one was decades ago.

The magic method that injects stuff into injectable resources, such as endpoints or parameter container classes, is https://github.com/quarkusio/quarkus/blob/main/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/parameters/InjectParamExtractor.java#L26 (this is called for endpoint method parameters that are parameter containers) and https://github.com/quarkusio/quarkus/blob/main/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/handlers/PerRequestInstanceHandler.java#L35 (this is done for the endpoint instance)

So, we will need to call this method on resource locators too. This should be done in https://github.com/quarkusio/quarkus/blob/main/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/handlers/ResourceLocatorHandler.java#L32 though at what place I'm not sure, it needs to be figured out.

Also, and again this depends on what your locator looks like, it might not be transformed into an instance of ResteasyReactiveInjectionTarget yet, because we only pick it up for parameter container classes. Though, I'm pretty sure this is automatic and should already be the case (you will find this out by just trying), but only if your resource locator has any parameter injection points.

Let me rephrase that. This is described in https://github.com/quarkusio/quarkus/wiki/RESTEasy-Reactive-(Server). We will turn every endpoint class into a parameter container, and we will auto-detect any class with @*Param or @Rest* annotated field into a parameter container. If you resource locator has such fields, it should be automatically transformed into an instance of ResteasyReactiveInjectionTarget on which you can call the injection method. If not, we need to make sure we detect it. If a resource locator does not have such fields, it will not be transformed and will not be an instance of ResteasyReactiveInjectionTarget which is a valid use-case, so in the case of resource locators, they might or might not be injectable.

Tests are available in extensions/resteasy-reactive/rest/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/beanparam so you can extend one to support resource locators.

Let me know if anything was unclear or if you need anything else :)

tanadeau commented 5 months ago

@FroMage: The resource locator in the reproducer attached to the ticket is the same as what we're using:

    @Path("{id}/sub")
    public SubResource subResource(@PathParam("id") Long id) {
        return resourceContext.getResource(SubResource.class);
    }

I'll start taking a look at the places you've described. Thanks for the help!

tanadeau commented 5 months ago

@FroMage: I think I found the underlying issue, but the fix looks to be pretty large.

First of all, based on my reproducer above, I created the following test class that recreated my issue:

package io.quarkus.resteasy.reactive.server.test.beanparam;

import static io.restassured.RestAssured.get;

import java.util.function.Consumer;

import jakarta.enterprise.context.RequestScoped;
import jakarta.inject.Inject;
import jakarta.ws.rs.GET;
import jakarta.ws.rs.Path;
import jakarta.ws.rs.PathParam;
import jakarta.ws.rs.container.ResourceContext;

import org.hamcrest.Matchers;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.builder.BuildChainBuilder;
import io.quarkus.builder.BuildContext;
import io.quarkus.builder.BuildStep;
import io.quarkus.resteasy.reactive.server.spi.SubResourcesAsBeansBuildItem;
import io.quarkus.test.QuarkusUnitTest;

/**
 * Related to the issue: https://github.com/quarkusio/quarkus/issues/39800
 */
public class SubResourceAsBeanParamTest {

    @RegisterExtension
    static QuarkusUnitTest test = new QuarkusUnitTest()
            .addBuildChainCustomizer(new Consumer<>() {
                @Override
                public void accept(BuildChainBuilder buildChainBuilder) {
                    buildChainBuilder.addBuildStep(new BuildStep() {
                        @Override
                        public void execute(BuildContext context) {
                            context.produce(new SubResourcesAsBeansBuildItem());
                        }
                    }).produces(SubResourcesAsBeansBuildItem.class).build();
                }
            })
            .withApplicationRoot((jar) -> jar.addClasses(SubResourceAsBeanParamTest.RestResource.class,
                    SubResourceAsBeanParamTest.SubResource.class));

    @Test
    public void testSubResource() {
        get("/5/sub")
                .then()
                .body(Matchers.equalTo("Hello from sub-resource of parent 5"))
                .statusCode(200);
    }

    @Path("/")
    public static class RestResource {
        @Inject
        ResourceContext resourceContext;

        @SuppressWarnings("unused")
        @Path("{id}/sub")
        public SubResourceAsBeanParamTest.SubResource subResource(@PathParam("id") Long id) {
            return resourceContext.getResource(SubResourceAsBeanParamTest.SubResource.class);
        }
    }

    @RequestScoped
    public static class SubResource {
        @PathParam("id")
        Long parentId;

        @GET
        public String getFromSubresource() {
            return "Hello from sub-resource of parent " + parentId;
        }
    }
}

After doing some debugging, it appears that the root cause is that for all REST request-based injections, everything is based off of method parameters. All of the injection points are found and set up via RuntimeResourceDeployment.java. This includes bean params and resource locators via classes such as LocatableResourcePathParamExtractor.java and the handlers that you mentioned. For bean params, these are found via the method parameters and then __quarkus_rest_inject is called.

From what I can tell, there is no infrastructure to find @PathParam or any other non-@Context, non-@Inject fields for injection. Everything stems from method parameters.

FroMage commented 5 months ago

Sorry about the delay. Did you check whether we already transform your subresource into an instance of ResteasyReactiveInjectionTarget?

Can you alter your test like this?

        @SuppressWarnings("unused")
        @Path("{id}/sub")
        public SubResourceAsBeanParamTest.SubResource subResource(@PathParam("id") Long id) {
SubResourceAsBeanParamTest.SubResource ret = resourceContext.getResource(SubResourceAsBeanParamTest.SubResource.class);
System.err.println("Is it already injectable? "+ (((Object)ret) instanceof ResteasyReactiveInjectionTarget));
            return ret;
        }
tanadeau commented 5 months ago

@FroMage: Yes. The subresource is an instance of ResteasyReactiveInjectionTarget.

Note that I did a refactor to remove our use of subresources from our codebase. After doing so, I found out that subresources were surfacing other bugs, including in OpenAPI YAML generation.

It may be worth a warning in the docs that while subresources are technically supported, there are known issues.

FroMage commented 5 months ago

@FroMage: Yes. The subresource is an instance of ResteasyReactiveInjectionTarget.

In that case, supporting injection should be trivial, you just have to call the relevant method in the handler I mentioned.

But if you're not using subresources anymore, perhaps you don't need this anymore? :-/

tanadeau commented 5 months ago

I don't believe that to be the case. If you see my last paragraphs in the comment above with the test code, there doesn't seem to be any infrastructure to fill in field values at all if there's no method parameter injection point. BeanParams are found via method parameter and then that code you mentioned is used. Unfortunately, there's no method parameter injection point for subresources; so a whole new injection infrastructure is needed.

However, it's correct that we don't need this anymore. I would recommend closing this via a documentation update as I mentioned earlier.

FroMage commented 4 months ago

Sorry, I don't think I understand your point. I'm pretty sure that if you call ((ResteasyReactiveInjectionTarget) locator).__quarkus_rest_inject(requestContext); in ResourceLocatorHandler.handle then your locator will have the proper values injected.