search

smallrye / smallrye-open-api

SmallRye implementation of Eclipse MicroProfile OpenAPI
Apache License 2.0
116 stars 89 forks source link

Allow spring-web extension REST endpoints to be published in openapi #217

Closed ebullient closed 4 years ago

ebullient commented 4 years ago

Reported in quarkusio/quarkus#4693

Quarkus uses SmallRye OpenAPI to render API endpoints. As reported, if you create a Spring Web REST endpoint and add openapi, the Spring endpoint does not show up.

The OpenApiAnnotationScanner (or an extension) will need to discover Spring Rest endpoints.

ebullient commented 4 years ago

I am willing to work on this (per discussion with @kenfinnigan and @geoand)

geoand commented 4 years ago

Cool! Anything I can do to help, just let me know

MikeEdgar commented 4 years ago

It might make sense to split the JAX-RS and Spring scanning out from OpenApiAnnotationScanner into subclasses and keep the common logic in the super class. I assume the Spring endpoints should be able to utilize the Microprofile annotations also.

ebullient commented 4 years ago

I was thinking along those lines, yes.

phillip-kruger commented 4 years ago

Hi @geoand and @ebullient . Has work on this started ? I had a quick look at what the options are, I just want to double check that someone else is not already busy on this ?

I also want to check the Quarkus requirement. If we enhance SmallRye to take Spring annotation into account while scanning, would that solve the Quarkus case ? Or is Quarkus converting the annotations to JaxRS/OpenApi already ? And if so, is it then not working at the moment because the model has already been created ?

If we need to add support for Spring annotation in SmallRye, I was thinking to approach this as follow:

1) Separate OpenAPI and JAX-RS code, so that JAX-RS becomes an extension/specialization. 2) Add Spring (simular to JAX-RS) code as an extension/specialization.

Let me know. Thanks

geoand commented 4 years ago

Hey @phillip-kruger

Has work on this started ? I had a quick look at what the options are, I just want to double check that someone else is not already busy on this ? AFAIK, no one has started working on this

I am also ccing @FroMage @gytis because they also have a use case requires generating the OpenAPI specs when no JAX-RS annotations are present on a class.

Hi @geoand and @ebullient .

I also want to check the Quarkus requirement. If we enhance SmallRye to take Spring annotation into account while scanning, would that solve the Quarkus case ? Or is Quarkus converting the annotations to JaxRS/OpenApi already ? And if so, is it then not working at the moment because the model has already been created ?

Yes, that would be my prefered approach, because Quarkus does not convert Spring Web annotations into JAX-RS ones.

If we need to add support for Spring annotation in SmallRye, I was thinking to approach this as follow:

  1. Separate OpenAPI and JAX-RS code, so that JAX-RS becomes an extension/specialization.
  2. Add Spring (simular to JAX-RS) code as an extension/specialization.

I really really like this approach since it would both to support the Spring use case and hopefully be pluggable enough to support the use case that @gytis and @FroMage have in mind

phillip-kruger commented 4 years ago

Thanks @geoand . Waiting on the other's response. I'll start looking at separating the OpenAPI and JAX-RS so long.

ebullient commented 4 years ago

I tried... and it just felt very hacky to me!! Splitting JAX-RS and OpenAPI would be a great first step. I am happy to help adding Spring support.. but my attempts at splitting the JAX-RS stuff out were just.. messy.

phillip-kruger commented 4 years ago

Hi @ebullient - thanks for the feedback. Yes seems like it's going to be quit an effort to split them. I am still playing around, but it might be a big piece of rewrite to get this done cleanly.

EricWittmann commented 4 years ago

I can confirm - it will be non-trivial. The original implementation pretty much directly uses the jandex index as the source of annotation information for generating the OAI. We have some options, including:

1) Create an interface that abstracts away the jandex index 2) Add another processing phase

Maybe both things are needed? Right now there are several existing phases, including the phase that generates OAI from JAX-RS+OpenAPI annotations. But also including the static file processor and the ModelReader. The results of all phases are merged together. No reason we can't add a Spring processing phase as well.

phillip-kruger commented 4 years ago

Hi @EricWittmann, thanks for the feedback. If we add another Processing phase for Spring, that would still be Spring+OpenAPI right ? Can we discuss this Thursday ?

EricWittmann commented 4 years ago

Yes - for Spring+OpenAPI. I was imagining this extra phase to be a SmallRye (impl) specific feature, but maybe it's something that should be added to the spec. So yeah, let's chat about it during Thursday's spec meeting.

phillip-kruger commented 4 years ago

Ok yes I guess that will be SmallRye specific, so maybe can can chat after the meeting. So we still need to refactor the OpenApi out of JaxRs so it can be reused in Spring ?

phillip-kruger commented 4 years ago

For those who did not see the post, we discussed this in the last meeting, and I am busy with a restructure of the code to allow for this feature. see https://groups.google.com/forum/#!topic/smallrye/OC-_oOHtVIM.

ebullient commented 4 years ago

Jandex didn't bother me.. It just scans for annotations, it doesn't care what they are.

What I couldn't do was indicate "here is the top of the tree, look for this set of annotations.. " because so much was oriented around the assumption of JAX-RS.

phillip-kruger commented 4 years ago

Thanks @ebullient . Yes Jandex was more discussed in terms of decoupling the annotation scanner from the code, so that other implementations can use their own annotation scanner. This is on ice at the moment.

At the moment the effort is in changing the structure as in the sketch, so that we can decouple OpenAPI from JAX-RS and then we should be able to plug in Spring. I should be done with this soon, then I would like to get everyone's feedback.

ebullient commented 4 years ago

I am happy to help with the "adding Spring" part, which I realize I said before! but sentiment stands. ;) Let me know when you're ready, and I'll see how far I can get.

phillip-kruger commented 4 years ago

Hi @ebullient @geoand .

The first part of this has been done. I have separated JAX-RS and OpenAPI and I am now busy adding a Spring Module. Have a look at the latest master and let me know if you have any concerns.

I am planning to add just a basic Spring endpoint test and support that, and then I might need some help with some complex Spring examples. I'll let you know once that is done.

geoand commented 4 years ago

That's awesome news @phillip-kruger !

phillip-kruger commented 4 years ago

Hi @ebullient and @geoand

I have merged the first (very basic) Spring use case. (see https://github.com/smallrye/smallrye-open-api/blob/master/extension-spring/src/test/java/test/io/smallrye/openapi/runtime/scanner/resources/GreetingController.java)

I need help with adding more test cases. Most of the code is there (copied from the JAX-RS module) but it needs testing and possible fixes.

If you are still keen to help, please let me know :)

Thanks

geoand commented 4 years ago

Cool stuff!

@ebullient would you like to help drive this one home? It would be great to have this one done for Quarkus 1.4

phillip-kruger commented 4 years ago

Hi @ebullient . Anything you need from me ? Are you ok to add more test cases ? We wanted to include this in Quarkus 1.4 but I am not sure if we will make the dates...

ebullient commented 4 years ago

Y.. Attention has been divided. I will add more test cases.

ebullient commented 4 years ago

This is not ready for integration yet, as the result is missing all kinds of information (and it does a few things slightly differently than other tools, which isn't a problem, but will require a custom "good" result json for testing once it all works.

https://github.com/ebullient/smallrye-open-api/tree/spring

phillip-kruger commented 4 years ago

Closing this. We can create smaller Spring related issues as basic Spring support is added.