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

Add openapi support to SE #558

Merged
merged 27 commits into from
May 6, 2019
Merged

Add openapi support to SE #558

merged 27 commits into from
May 6, 2019

Conversation

tjquinno
Copy link
Member

Basic approach:

  1. Take advantage of smallrye/smallrye-open-api which implements MP OpenAPI support.
  2. Create OpenAPISupport class in SE which developers can use to explicitly and easily add OpenAPI support to their apps. Our SE support excludes annotation support. The most reasonable use of OpenAPI support in SE will be with a static OpenAPI document file built into the app. This fits well with the idea often described of writing the OpenAPI document first and using that to drive development of the endpoints described in the document.
  3. MP support will use a Java service that sets up OpenAPISupport with annotation handling (as well as the other mechanisms for providing OpenAPI model information).

Note that the smallrye implementation expects to find a jandex index to support its OpenAPI annotation handling. It does not even fail gracefully if the index is missing. The work savings to us in leveraging smallrye's impl is significant and so requiring the presence of the jandex index seems worth it. Early on, our MP support will check for the index and fail if it is absent. Later (presumably a short while later), our MP support can create the index just-in-time (in memory) if the developer did not build it into the application. I'd expect our first release that supports OpenAPI to build the index JIT if it is missing.

@tjquinno tjquinno added enhancement New feature or request open-api labels Apr 10, 2019
@tjquinno tjquinno added this to the 1.1 milestone Apr 10, 2019
@tjquinno tjquinno self-assigned this Apr 10, 2019
@tjquinno
Copy link
Member Author

The starting point of this PR contains only the SE changes. MP will come along shortly.

spericas
spericas previously approved these changes Apr 10, 2019
Copy link
Member

@spericas spericas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We should add some tests for the new service.

@tjquinno tjquinno changed the title WIP: Add openapi support to SE and MP Add openapi support to SE Apr 23, 2019
batsatt
batsatt previously approved these changes Apr 24, 2019
Copy link
Contributor

@batsatt batsatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tomas-langer
Copy link
Member

I have a problem with the dependency on small rye - mostly because it obviously requires Weld (as you note above - Jandex index is expected).
We do not have any explicit dependency on Weld in our project nor should we have. We actually plan to do a POC against other CDI implementation(s) in the future.
Is the dependency on Weld "hard", or can it use any CDI implementation?

@romain-grecourt
Copy link
Contributor

What are the required runtime dependencies for SE ? Does it drag weld and jandex even for SE ?

@tjquinno
Copy link
Member Author

tjquinno commented Apr 25, 2019

I have been digging into the dependency situation since Tomas noted his concern. As a brute force experiment I deleted the entire org/jboss/weld directory from my local .m2 repository. Building, testing, and packaging the smallrye openapi parent project (includes implementation and TCK) caused maven to download weld-parent-36 (a pom) but nothing else.

There seems to be no dependency on Weld introduced by using smallrye's OpenAPI impl but I'll check a little more to be sure.

@tjquinno tjquinno dismissed stale reviews from batsatt and spericas via 2161817 May 6, 2019 17:37
@tjquinno tjquinno merged commit 29e6893 into helidon-io:master May 6, 2019
@tjquinno tjquinno deleted the add-openapi branch May 24, 2019 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request open-api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants