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

Keep a reference to the Java method parameter for later use in OASFilter #1076

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

renegrob
Copy link

While OperationImpl has a methodRef property that can be used to match the method (as done in Quarkus AutoRolesAllowedFilter, AutoTagFilter etc.), ParameterImpl does not contain any information about the parameter's name or position. This PR introduces a paramRef for Parameter similar to methodRef.

@MikeEdgar
Copy link
Member

Thanks for the PR. I know this is still a work-in-progress, but just to mention it so you're aware - parameter handling can be a bit more complex than operation handling. There are some cases where parameters are derived without annotations whatsoever, and also cases where multiple annotations are set in different locations. For example an @Parameter in combination with a JAX-RS @QueryParam.

@phillip-kruger
Copy link
Member

@MikeEdgar would all other information not be available already on the Parameter (Impl) created. This is just a way to link back to a certain parameter in Quarkus... I think this should work (at least for now) @renegrob did you use this version in Quarkus to see if this works ?

@MikeEdgar
Copy link
Member

I think it will only work when there is a @Parameter. The cases where only JAXRS annotations are used won't have the ref attached. That may be good enough for what you need?

@phillip-kruger
Copy link
Member

Ok I thought we still create a parameter in the model in those cases ?

@renegrob
Copy link
Author

Thanks for the comments. Yes, indeed I need to test those cases. So far I couldn't find any cases where ParameterReader was not used. I think I'll add a test similar to ParameterScanTests to test those cases.

@MikeEdgar
Copy link
Member

@renegrob , the test ParameterScanTests.testJakartaGenericTypeVariableResource is an existing test that will not hit the new code.

@MikeEdgar
Copy link
Member

Ok I thought we still create a parameter in the model in those cases ?

That's right, but that model is populated with information from (potentially) multiple sources depending on where the annotations are placed.

@renegrob renegrob force-pushed the improvement-parameter-reference branch 2 times, most recently from 73b0834 to 8ef59ca Compare February 26, 2022 19:57
@phillip-kruger
Copy link
Member

@renegrob @MikeEdgar - is this ready for merge ?

@MikeEdgar MikeEdgar force-pushed the improvement-parameter-reference branch from 8ef59ca to 06ebe75 Compare March 3, 2022 01:57
@MikeEdgar
Copy link
Member

@phillip-kruger just rebased. I'll give this another look in my morning to double check all the necessary places are covered.

@phillip-kruger
Copy link
Member

Thanks @MikeEdgar !

@renegrob
Copy link
Author

renegrob commented Mar 3, 2022

Sorry, I was busy with another issue. I was able to filter parameters in my extension. I didn't test all cases but I think some of them are not even valid for Quarkus - or at least not for resteasy-reactive or for resteasy-classic with Singleton beans.
However, the tests in this repo should now cover most of the cases. I think now I should be able to map most of the OpenAPI objects back to their Java origins. If something is missing, I'll open another MR.
@MikeEdgar Thank you for rebasing and yes, it is ready for review.

@renegrob renegrob marked this pull request as ready for review March 3, 2022 07:13
@phillip-kruger
Copy link
Member

@MikeEdgar please merge when you are happy :)

@phillip-kruger phillip-kruger added this to the 2.1.21 milestone Mar 3, 2022
@MikeEdgar
Copy link
Member

@renegrob , do you have any requirement that the annotation used for the methodRef is the OpenAPI annotation versus the "framework" annotation? For example, in resource method below, do you expect the methodRef to be the sayHello method annotated with @Parameter, or the name method arg annotated with @QueryParam (or does it not matter)?

@Path("/")
class HelloResource {

    @GET
    @Parameter(name = "name", description = "The user's name")
    @Produces("text/plain")
    public String sayHello(@QueryParam("name") String name)) {
        return "Hello " + name;       
    }

}

@renegrob
Copy link
Author

renegrob commented Mar 3, 2022

@MikeEdgar My goal is to write a OASFilter that can associate the @MyAnnotation instance in the example below with the OpenAPI Parameter that was created from @QueryParam("name") String name in order to enrich the Paramters properties. (E.g. do parameter.setDescription("whatever") in that case.) I don't mind how or why the OpenAPI Parameter object was created.
I'm not sure if that answers your question.

@Path("/")
class HelloResource {

    @GET
    @Parameter(name = "name", description = "The user's name")
    @Produces("text/plain")
    public String sayHello(@MyAnnotation("whatever") @QueryParam("name") String name)) {
        return "Hello " + name;       
    }

}

@MikeEdgar
Copy link
Member

MikeEdgar commented Mar 3, 2022

Yes, that helps, thank you. So I think the answer is that you want methodRef to be generated from the String name and not from the sayHello method. I ask because both annotations (@Parameter and @QueryParam) contribute data to the ParameterImpl (including potentially their target). Annotations on method arguments and fields have higher priority than those on methods, so I think this is good then 👍

@MikeEdgar MikeEdgar merged commit ad3ce5e into smallrye:main Mar 3, 2022
@renegrob renegrob deleted the improvement-parameter-reference branch March 3, 2022 11:49
@MikeEdgar
Copy link
Member

@renegrob , was the functionality added in this PR ever used in a downstream project? I am trying to limit use of internal SmallRye classes, adding #2020 to support the OperationImpl#getMethodRef case, but this still remains. Thanks

@renegrob
Copy link
Author

@renegrob , was the functionality added in this PR ever used in a downstream project? I am trying to limit use of internal SmallRye classes, adding #2020 to support the OperationImpl#getMethodRef case, but this still remains. Thanks

Yes, it was used in a private project. I don't know if it is still in use, since I left that project. But I'm sure it can be changed the same way as Quarkus' AutoRolesAllowedFilter and AutoTagFilter. But as I see https://github.com/quarkusio/quarkus/blob/de2758317640649ae9d313e8208af0c41e347d46/extensions/smallrye-openapi/deployment/src/main/java/io/quarkus/smallrye/openapi/deployment/filter/AutoRolesAllowedFilter.java#L30
still uses MethodRef.

@MikeEdgar
Copy link
Member

Correct, for now until Quarkus updates to use the callback added in #2020 . I suppose we could add a ParameterHandler like the OperationHandler.

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

Successfully merging this pull request may close these issues.

3 participants