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

LRA extension endpoints are populated to Swagger UI automatically #23586

Closed
lordofthejars opened this issue Feb 10, 2022 · 26 comments · Fixed by #40812
Closed

LRA extension endpoints are populated to Swagger UI automatically #23586

lordofthejars opened this issue Feb 10, 2022 · 26 comments · Fixed by #40812
Assignees
Labels
Milestone

Comments

@lordofthejars
Copy link
Contributor

Description

When using LRA and OpenAPI + Swagger UI, all LRA endpoints + business endpoints are shown in the UI. I think that by default it should show only business endpoints and if it is set, then show the LRA endpints

Screenshot 2022-02-10 at 16 47 10

endpoints

Implementation ideas

Remove none business endpoints from swagger UI and add a flag when you want to see them.

@lordofthejars lordofthejars added the kind/enhancement New feature or request label Feb 10, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 10, 2022

/cc @MikeEdgar, @phillip-kruger

@phillip-kruger
Copy link
Member

Thanks for this. This will have to be in the LRA Extension, but I can also have a look.
cc @xstefank

@xstefank
Copy link
Member

I can take a look, @phillip-kruger

@xstefank
Copy link
Member

xstefank commented Feb 14, 2022

I also wonder whether LRA endpoints should go under non-application path. @gsmet or @geoand maybe you can tell?

@geoand
Copy link
Contributor

geoand commented Feb 14, 2022

You mean under /q? If so, I think that makes sense

@xstefank
Copy link
Member

xstefank commented Feb 14, 2022

yes, under /q as coordinator and proxy paths aren't user paths.

@geoand
Copy link
Contributor

geoand commented Feb 14, 2022

Seems reasonable, albeit somewhat breaking

@xstefank
Copy link
Member

proxy calls should not be exposed to users, these are used if the user doesn't expose business REST methods for participant. But I understand. I'll mention it in the PR.

@phillip-kruger
Copy link
Member

@xstefank - any news on this ?

@xstefank
Copy link
Member

@phillip-kruger didn't have much time for this one. Will try to prioritize.

@xstefank
Copy link
Member

xstefank commented Feb 28, 2024

@phillip-kruger I actually got an external contributor willing to look at this :) So sorry that I don't have time for LRA work.

@phillip-kruger
Copy link
Member

No worries :)

@pcheucle
Copy link
Contributor

I started looking at this.

My first idea was to change the @Path value at build time to move LRA proxy endpoints under non-application path.
But I think that doing changes in the LRA extension processor will not have any effect since these endpoints, coming from Narayana dependency, are already scanned by the REST extension.

My second idea was to recreate proxy endpoints in LRA Extension.
Maybe by creating new JAX-RS resources and making them available to the scanning process using AdditionalResourceClassBuildItem, or by creating endpoints using RouteBuildItem.
But again, I don't see how to exclude JAX-RS resources coming from the Narayana library.
@xstefank : in this case, do you think that creating a proxy library without JAX-RS is a possible option ?

@pcheucle
Copy link
Contributor

@xstefank You will find a working solution there : pcheucle@82b9a22

Basically, I used shade plugin to exclude beans.xml from lra-proxy dependency, to remove original LRA proxy resources LRAParticipantResource and ParticipantProxyResource from the scan.
Then I created 2 new classes in the LRA extension that extend original JAX-RS resources and where I can add /q in the @Path.
Finally, I added a new config property to make LRA proxy endpoints visible or not in Swagger UI.

It's working but I am still looking for a cleaner approach.
Ideally, to be able to replace the @Path value of original resources but after the scanning process, but I didn't find any way to do that.

@geoand or @xstefank maybe you can help on this ?

@geoand
Copy link
Contributor

geoand commented Apr 25, 2024

@geoand or @xstefank maybe you can help on this ?

I am not familiar with LRA

@rmanibus
Copy link
Contributor

rmanibus commented Apr 26, 2024

@geoand I am looking into this with @pcheucle,
we are trying to modify some jaxRs annotations in the lra plugin processor.
The annotation transformer build item is not working with jaxRs resources.
Is there a way to transform the index that is fed to the jaxRs indexer ?

@geoand
Copy link
Contributor

geoand commented Apr 29, 2024

AFAIR, the OpenAPI extension does not take into account the annotation transformer stuff at all, but @phillip-kruger would know for sure

@phillip-kruger
Copy link
Member

Yea, the best way to achieve this is with filters

@phillip-kruger
Copy link
Member

@MikeEdgar do you have any suggestions ?

@xstefank
Copy link
Member

@pcheucle, sorry I'm late to the party; I was traveling. I think you're on the right path, but I think we should try a solution where we either transform Narayana classes or remove them and add a new override, as you did. I kind of don't like shading out beans.xml since this can become a problem down the line.

I'll cycle this back to @geoand :) https://stackoverflow.com/a/70847243/14040597. I think this should work for our use case. Maybe BytecodeTransformerBuildItem is what we are after.

Your solution as it stands now needs to at least not hardcode the /q path since this can be changed with config - https://quarkus.io/guides/all-config#quarkus-vertx-http_quarkus-http-non-application-root-path. We should plug these endpoints into the same non-application endpoint management.

Will you try this?

@MikeEdgar
Copy link
Contributor

If these endpoints are under the non-application path, they would still be picked up from the index by the OpenAPI scanner, right? Or would those end up in a separate index that we're not using?

Otherwise, using a filter or perhaps adding a new OpenAPI build item by which extensions can provide classes/packages to exclude from the OpenAPI would work.

@xstefank
Copy link
Member

xstefank commented May 2, 2024

@MikeEdgar similar case as /q/health or /q/metrics, do we pick those to OpenAPI?

@MikeEdgar
Copy link
Contributor

The Health extension can create a filter that adds those paths to the OpenAPI, but I don't believe metrics has that. If those are not using JAX-RS annotations they're invisible to the OpenAPI scanner by default.

@pcheucle
Copy link
Contributor

pcheucle commented May 2, 2024

@xstefank
I found that when using RemovedResourceBuildItem or ExcludeDependencyBuildItem, Narayana classes are removed from BeanArchiveIndexBuildItem but are still present in CombinedIndexBuildItem which is used by ReactiveCommonProcessor to scan resources.
I need a Quarkus way to exclude Narayana classes from the CombinedIndexBuildItem, so I can fully override them and replace the current solution with shade plugin.

I also tried BytecodeTransformerBuildItem to change the @Path value, but it has no effect.
When debugging, I found that this transformation is happening after Resteasy resources scanning.

And yes, I will not hardcode the /q in the path.

@xstefank
Copy link
Member

xstefank commented May 2, 2024

@pcheucle TBH, if there is no better way, then we can just "hide" the endpoints from user view for this issue. Just adjust it to take the config changes into account and open a PR please.

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

Successfully merging a pull request may close this issue.

7 participants