-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
feat(SwaggerUI): Add Swagger UI configuration parameters #6314
Conversation
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. I added a couple of suggestions and questions.
...ons/swagger-ui/deployment/src/main/java/io/quarkus/swaggerui/deployment/SwaggerUiConfig.java
Outdated
Show resolved
Hide resolved
* The default expansion depth for the model on the model-example section. | ||
*/ | ||
@ConfigItem | ||
OptionalInt defaultModelExpandDepth; |
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 find it a bit confusing to have an option with an s and one without. It's like that in Swagger UI?
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.
Me too, i just copied the original configuration properties names.
...ons/swagger-ui/deployment/src/main/java/io/quarkus/swaggerui/deployment/SwaggerUiConfig.java
Outdated
Show resolved
Hide resolved
...ons/swagger-ui/deployment/src/main/java/io/quarkus/swaggerui/deployment/SwaggerUiConfig.java
Outdated
Show resolved
Hide resolved
extensions/swagger-ui/runtime/src/main/java/io/quarkus/swaggerui/runtime/SwaggerUiRecorder.java
Outdated
Show resolved
Hide resolved
My opinion on this one is that adding things to the URL won't work very well. I would lean towards trying to find another solution. |
@aoudiamoncef Do you need any help on this pr? I kinda need this feature. |
I'm agree with @gsmet, we should be able to inject swagger ui configuration at build time. |
I think we are still able to do this, or at least something similar. Alternativly, we could also "just" generate a custom index.html, and configure the swagger-ui this way. |
@Postremus, we should look at how they inject the config file. |
Ah, I undertstand. The swagger.yml config is staticly included into "swagger-ui.js" during the compilation of the ui. Another option: WDYT? |
I think that keeping the config properties and inject them inside the html file could be the easiest way to do it. |
@aoudiamoncef any chance you could revisit this by pushing the configuration directly by adjusting the It would be similar to what we do here: https://github.com/quarkusio/quarkus/blob/master/extensions/swagger-ui/deployment/src/main/java/io/quarkus/swaggerui/deployment/SwaggerUiProcessor.java#L189 . Except we would push all the configuration properties there. IMHO, this would be far more future proof. |
@gsmet, I'll do it ASAP. |
Just a note that you will need to be a bit careful about escaping :). |
I'm working on pushing configuration to const ui = SwaggerUIBundle({ // start with this line
url: "https://petstore.swagger.io/v2/swagger.json",
dom_id: '#swagger-ui',
deepLinking: true,
presets: [
SwaggerUIBundle.presets.apis,
SwaggerUIStandalonePreset
],
plugins: [
SwaggerUIBundle.plugins.DownloadUrl
],
layout: "StandaloneLayout"
}) // and ends with this one Then i'll write a kind of string template and inject the configuration Map. Anyone have a better approach ? |
Yeah, I don't think it would be too hard. Just catch With this approach, we could also fix #4766 (see my last comment). |
Additional though: |
@aoudiamoncef are you making progress? Do you need any help? |
I'm still working on it but with another approach. It's not a big deal to create a string and replace the matched group. I'm trying to validate the configurations at build time. Handling authentication needs more work and we should create tests to avoid regressions in the future. |
Hi @gsmet, I'll push my work ASAP |
Cool! Thanks for the heads up. |
08c6fb1
to
2f6dc46
Compare
Hi @gsmet, |
2f6dc46
to
779bf81
Compare
779bf81
to
062bdbf
Compare
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.
Overall, very nice and thorough work, thanks a lot for that!
Could you have a look at my comments?
(sorry for the late review)
.disable(JsonWriteFeature.QUOTE_FIELD_NAMES) | ||
.enable(SerializationFeature.INDENT_OUTPUT) | ||
.addModule(new Jdk8Module()) | ||
.build(); |
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.
Could we initialize this one statically?
.../swagger-ui/deployment/src/main/java/io/quarkus/swaggerui/deployment/SwaggerUiProcessor.java
Outdated
Show resolved
Hide resolved
extensions/swagger-ui/deployment/src/main/resources/application-custom-config.properties
Outdated
Show resolved
Hide resolved
quarkus.swagger-ui.urls=/openapi?group=test, /openapi?group=lahzouz | ||
quarkus.swagger-ui.validator-url=https://validator.swagger.io/validator | ||
|
||
quarkus.swagger-ui.oauth=true |
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.
Same here?
extensions/swagger-ui/runtime/src/main/java/io/quarkus/swaggerui/runtime/SwaggerUiRecorder.java
Outdated
Show resolved
Hide resolved
...ons/swagger-ui/deployment/src/main/java/io/quarkus/swaggerui/deployment/SwaggerUiConfig.java
Outdated
Show resolved
Hide resolved
062bdbf
to
01e277b
Compare
@gsmet, i did the requested changes. I'm waiting for your feedbacks. |
01e277b
to
09a9498
Compare
09a9498
to
2700fe5
Compare
2700fe5
to
713898c
Compare
Hi Moncef, Sorry it took me so long to get back to you. The PR seems broken, I had to change this in the test:
but then the tests are failing anyway. I think it won't fly to generate the URIs in the config as you somehow need to use Then there are other minor things that could be improved but I think we need that to be addressed first. |
Hi @gsmet, Thanks |
When I suggested that we merge the two, there was concerns that maybe we want to add support for other UIs. I still think we can merge and add support for other UIs via config, as there will not be many (if ever more than one or two). This will make things easier to maintain I think. |
713898c
to
7877f20
Compare
7877f20
to
9fad489
Compare
Maybe other editors could be a separate issue once this one is merged? |
Hi guys, what is the state of this MR? We would love to get support for swagger-ui showing multiple openapi files (dropdown in header) for which we need to be able to specify
I understand that this MR would allow that among other things. |
Hi all. |
any solution that would allow customization of the swagger config options is fine. Right now there is no way how to customize it(I tried adding a webfilter that would modify the html response, but the swagger-ui is server via vertx...). Not sure how long the integration w smallrye would take. We will probably create a custom extension (based on this MR)in the meantime as we need it rather urgently. Thanks for the effort, will keep an eye on this. |
Hi all, |
@aoudiamoncef i have added this to SmallRye and will pull that into Quarkus soon. |
#12782 will supersede and close this. |
Hi @dmlloyd ,
This PR is related to #5639. I'm waiting for feedbacks
Moncef