-
Notifications
You must be signed in to change notification settings - Fork 3k
Implement support for multiple openapi documents #51760
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
base: main
Are you sure you want to change the base?
Conversation
...yment/src/main/java/io/quarkus/smallrye/openapi/common/deployment/SmallRyeOpenApiConfig.java
Show resolved
Hide resolved
|
|
||
| routes.produce(RouteBuildItem.newManagementRoute(openApiConfig.path(), MANAGEMENT_ENABLED) | ||
| .withRouteCustomizer(corsFilter) | ||
| .withRoutePathConfigKey("quarkus.smallrye-openapi.path") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not understand what withRoutePathConfigKey is used for?
Since the path is already passed through the newManagementRoute anyway.
Is this used for runtime config path selection? That would not be needed here since this config is buildtime only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno. Maybe @MikeEdgar would know?
Would it be possible to keep it the way it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Would require me to manually build the config keys in this case.
But - I would still like to know what this does. :)
Apperantly, right now this does nothing anymore.
Was the only use. withRoutePathConfigKey gets mapped to ConfiguredPathInfo in io.quarkus.vertx.http.deployment.RouteBuildItem#getRouteConfigInfo
But getConfiguredPathInfo is unused.
Only user was the Devconsoleprocessor from old devui:
19aae16c113#diff-5d886c297bbf81799a7e65ae5aec5de0966e6a5dbe06641a569a7c5afa0cf5b1
No other users in quarkiverse / camel-quarkus.
I can add it back for now, and someone else does a complete cleanup of the associated build items later on?
| * The final OpenAPI Document as generated by the Extension. | ||
| */ | ||
| public final class OpenApiDocumentBuildItem extends SimpleBuildItem { | ||
| public final class OpenApiDocumentBuildItem extends MultiBuildItem { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since OpenApiDocumentBuildItem is part of an spi module, I guess it could also be consumed by other extensions?
Is it safe to change a build item from SimpleBuildItem to MultiBuildItem, or do the buildsteps that rely on this builditem just break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the buildsteps will break, since a SimpleBuildItem is usually injected without a Collection wrapper.. Might need to add a new builditem..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the changes from OpenApiDocumentBuildItem and created a new MultiBuildItem OpenApiNamedDocumentBuildItem, where one is created for each generated document.
However, @phillip-kruger is the OpenApiDocumentBuildItem even used anywhere? Not found any uses in quarkus.
You know of any extensions?
Lines 1021 to 1029 in 6ef44db
| * We need to use the deprecated OpenApiDocument as long as | |
| * OpenApiDocumentBuildItem needs to be produced. | |
| */ | |
| @SuppressWarnings("deprecation") | |
| OpenApiDocument toOpenApiDocument(SmallRyeOpenAPI finalOpenAPI) { | |
| OpenApiDocument output = OpenApiDocument.newInstance(); | |
| output.set(finalOpenAPI.model()); | |
| return output; | |
| } |
Makes me think that the builditem is not that useful anymore. Maybe deprecate it, and not provide an alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So usually, what I do in this case is look for usage in the Quarkiverse org and Camel Quarkus project.
You can see that the build item is used in the Backstage extension here: https://github.com/search?q=org%3Aquarkiverse+OpenApiDocumentBuildItem&type=code . This will break as we need to make it a list.
Now, we can adjust the project in a way that is backward compatible by just making it a list and it should work with other versions so I think I would favor this approach rather than introducing the new build item whose name looks a bit artificial.
Now I don't know if your changes to OpenApiDocumentBuildItem would break Backstage? I suppose you had maybe a isDefault() method? You won't be able to call it always but maybe we could have something that only calls it if there are multiple documents? That way, I think it would somehow work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I believe that could work.
We will only call .isDefault() when there is already a multipleOpenApiDocumentBuildItem given, which can only happen on 3.31.
All other documents than the default document will be ignored for a start. Support for multiple openapi documents can then be added later
I will prepare a PR for backstage. Lets hope they accept this idea. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
OpenApiDocumentBuildItem is now a MultiBuildItem.
backstage PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so backwards compatibility won't work (at least this way). Quarkus does not want to injects a SimpleBuildItem as a List.
https://github.com/quarkiverse/quarkus-backstage/pull/136/checks
...e-openapi/runtime/src/main/java/io/quarkus/smallrye/openapi/runtime/OpenApiConfigHelper.java
Show resolved
Hide resolved
|
The changes themselve work, i.e. I now have multiple openapi documents in my application. For now this pull request is a draft. |
|
🎊 PR Preview b4e83ad has been successfully built and deployed to https://quarkus-pr-main-51760-preview.surge.sh/version/main/guides/
|
c2ddb40 to
86fb0c7
Compare
|
Interesting work. Let’s have a proper discussion with the maintainer of SmallRye OpenApi when we’re all back. |
e5a0c58 to
ff049e6
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
gsmet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this work! I added a few comments here and there, mostly cosmetic.
|
|
||
| String DEFAULT_DOCUMENT_NAME = "<default>"; | ||
|
|
||
| String DEFAULT_PATH = "openapi"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if you should also use a <default> value here and resolve it later given it's not exactly always the default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could maybe use:
@ConfigDocDefault("""
Either
* openapi for the default document
* Or openapi-<document-name> for any named document
On OpenApiDocumentConfig path / storeSchemaDirectory.
That would make it extra clear for users how this value is determined - in addition to the description.
Alternatively, something like openapi-<document-name> as sole default value? But my fear is that users then expect the <document-name> pattern to work in other places of the openapi config.
...yment/src/main/java/io/quarkus/smallrye/openapi/common/deployment/SmallRyeOpenApiConfig.java
Show resolved
Hide resolved
|
|
||
| routes.produce(RouteBuildItem.newManagementRoute(openApiConfig.path(), MANAGEMENT_ENABLED) | ||
| .withRouteCustomizer(corsFilter) | ||
| .withRoutePathConfigKey("quarkus.smallrye-openapi.path") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno. Maybe @MikeEdgar would know?
Would it be possible to keep it the way it is?
| * The final OpenAPI Document as generated by the Extension. | ||
| */ | ||
| public final class OpenApiDocumentBuildItem extends SimpleBuildItem { | ||
| public final class OpenApiDocumentBuildItem extends MultiBuildItem { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So usually, what I do in this case is look for usage in the Quarkiverse org and Camel Quarkus project.
You can see that the build item is used in the Backstage extension here: https://github.com/search?q=org%3Aquarkiverse+OpenApiDocumentBuildItem&type=code . This will break as we need to make it a list.
Now, we can adjust the project in a way that is backward compatible by just making it a list and it should work with other versions so I think I would favor this approach rather than introducing the new build item whose name looks a bit artificial.
Now I don't know if your changes to OpenApiDocumentBuildItem would break Backstage? I suppose you had maybe a isDefault() method? You won't be able to call it always but maybe we could have something that only calls it if there are multiple documents? That way, I think it would somehow work?
ff049e6 to
8dc15de
Compare
This comment has been minimized.
This comment has been minimized.
8dc15de to
a134781
Compare
Status for workflow
|
Status for workflow
|
| Status | Name | Step | Failures | Logs | Raw logs | Build scan |
|---|---|---|---|---|---|---|
| ❌ | Native Tests - Windows support | Setup GraalVM |
Logs | Raw logs | 🚧 |
You can consult the Develocity build scans.

Implement support for multiple openapi documents.
Fixes #22355