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

OpenAPI enhancement #25800

Merged
merged 2 commits into from
May 30, 2022
Merged

OpenAPI enhancement #25800

merged 2 commits into from
May 30, 2022

Conversation

phillip-kruger
Copy link
Member

Small PR that adds the following to the OpenAPI Extension:

  1. BuildItem in SPI that contain the OpenAPI Schema. Useful to be consumed by other extensions that wants to generate clients from the OpenAPI Schema.
  2. Auto server if no server is set. This will use the default (config quarkus.http.host and quarkus.http.port.

I am working on an extension (still early days) that will generate JS from the Schema, and these features originates from there.

…tion by other extensions

Signed-off-by: Phillip Kruger <[email protected]>
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

I think we should explicitly have serverHost and serverPort here defaulting to ${quarkus.http.host} and ${quarkus.http.port}.

The reason is because the host and port are runtime configs (meaning that the built binary will have the host and port set during the build) and also because an application running in OpenShift won't set these parameters, so it will expose localhost and 8080 through a route (eg.https://registry.quarkus.io)

@gastaldi
Copy link
Contributor

Since the config expects an URL maybe have an Optional<URL> would make sense

@phillip-kruger
Copy link
Member Author

I think we should explicitly have serverHost and serverPort here defaulting to ${quarkus.http.host} and ${quarkus.http.port}.

You mean have new configs that does this ?

@gastaldi
Copy link
Contributor

You mean have new configs that does this ?

Yes, sorry for not being clear

@phillip-kruger
Copy link
Member Author

The auto server feature happens at build time, so if the values are changes in runtime, the user will have to set this the non auto way, for now

@gastaldi
Copy link
Contributor

Yeah, that would have to be a build time config.

I personally think that using quarkus.http.host and quarkus.http.port would work for cases where the app is published in a non-cloud environment. In other cases you'll specify the published URL (which does not match these properties)

@phillip-kruger
Copy link
Member Author

I can open another issue that allows config values in the schema, that we replace in runtime, but that is a separate issue. For now then I'll create new configs that default to quarkus.http.host and quarkus.http.port. Right ?

@phillip-kruger
Copy link
Member Author

phillip-kruger commented May 26, 2022

@gastaldi actually thinking about it some more, it does not make sense to introduce those two new config options. The whole point of auto server is to add the server entry in the schema if the user did not. The user can already add the server in the schema with config or annotations. So if they are going to go through the trouble to add config, the auto filter is not needed anymore. Make sense ?

@gastaldi
Copy link
Contributor

gastaldi commented May 26, 2022

@phillip-kruger right, that makes sense to me.

On a separate topic, can we use interpolated config values in the @Server annotation?

UPDATE: Just saw it does in the Javadoc: Variable substitutions will be made when a variable is named in {brackets}.

@phillip-kruger
Copy link
Member Author

Mmm, let me check that out tomorrow

@phillip-kruger
Copy link
Member Author

/cc @MikeEdgar

@phillip-kruger phillip-kruger force-pushed the openapi-js branch 2 times, most recently from b4854bc to 34a7a6a Compare May 30, 2022 01:11
@phillip-kruger
Copy link
Member Author

Ok @gastaldi . During runtime servers can be added to the scheme using the spec's defined mp.openapi.servers, example: mp.openapi.servers=https://xyz.com/v1,https://abc.com/v1.

I have changed this feature to not be always enabled, because when using swagger-ui, these can be some CORS issues between 0.0.0.0 and localhost. So this is really in the case where you want to export the openapi schema document to use with code generation etc. So now this will be added in the export case by default, but need to explicitly set to add to runtime. Hope this makes sense.

@quarkus-bot

This comment has been minimized.

Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

LGTM

@phillip-kruger phillip-kruger merged commit 1584b8c into quarkusio:main May 30, 2022
@quarkus-bot quarkus-bot bot added this to the 2.10 - main milestone May 30, 2022
@phillip-kruger
Copy link
Member Author

I added the breaking change label due to the fact that if you are using your own filter (for OpenAPI) and that filter can not run during build time, then you app will now break. This is due to the fact that we are now having a BuildItem that contains the generated OpenAPI document. So Filters needs to handle the case of creating the OpenAPI document during build.

@phillip-kruger phillip-kruger deleted the openapi-js branch June 12, 2022 04:16
@Denistr
Copy link

Denistr commented Jun 16, 2022

Hi @phillip-kruger ,
When do you expect release these changes?
As far as I see, there are no these changes in 2.9.2.Final

@phillip-kruger
Copy link
Member Author

They will be in 2.10 in a week or so

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

Successfully merging this pull request may close these issues.

3 participants