Skip to content
This repository was archived by the owner on May 14, 2025. It is now read-only.

feature: springdoc integration #4836

Closed

Conversation

klopfdreh
Copy link
Contributor

@klopfdreh klopfdreh commented Feb 19, 2022

This is a PR to implement springdoc. It relates to #4820.

I was not able to build the project completely at my local machine, maybe someone can help me with this. I get some errors related to sun jdk which makes sense, because I use OpenJDK.

The default behavior is that springdoc is deactivated by default. I had to implement a EnvironmentPostProcessor to achieve this. Only if you set the corresponding features to the value "true", the API and the UI will be available. See: https://springdoc.org/properties.html (springdoc.swagger-ui.enabled / springdoc.api-docs.enabled)

Note: I implemented a filter and didn't use a configurer in the mvc configuration, because this will lead to errors when opening task logs, because it is not wrapped into quotes.

I implemented it for 2.9.x but it can easily be cherrypicked to main.

I made the api-docs public without any login required.

Licenses:
Swagger: Version 2.0, January 2004
Springdoc: Version 2.0, January 2004

@klopfdreh
Copy link
Contributor Author

@ilayaperumalg build passed - the PR can be reviewed, now. 😁

@onobc onobc self-requested a review March 1, 2022 20:47
Copy link
Contributor

@onobc onobc left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @klopfdreh !

I have left some comments inline. Here are a couple of points of concern as well:

Testing

It is ok that there are no tests in it for this 1st round as we may morph direction a bit. However, we will want to have the following tests in before this gets merged:

  • Unit tests where applicable
  • IT for auto-config disabled by default
  • IT for auto-config enabled by setting enabled props
  • IT for any other OpenAPI configs that we add (customizers, etc...)
  • I am sure there are a few more we will want to add when we get to this stage

Custom Configuration

We will probably want to make sure we allow users to set properties to customize OpenAPI. We should make sure that we respect these properties. I would expect this to just work OOTB but would be great thing to either add an IT for that does some lightweight customization or at very least a manual test.

Doc updates

We should add some high level docs about this.

  • How to enable / disable
  • How to customize via openapi props (point to their doc)
  • How to view / use

Security

My biggest concern is what we want to do for security OOTB. OpenAPI is integrated in very well w/ Spring Boot. This is awesome and opens up some cool features. However, it brings to mind a spidey cliche quote "With great power comes great responsibility". We have to watch out what we allow OpenAPI to expose. One example is the ability to show the actuator endpoints via setting springdoc.show-actuator. Any other built-in features that we should be aware of?

We have an upcoming feature that leverages secured Actuator endpoints and we need to be sure that whatever we do here does not allow the user to subvert the security required by that feature. @dturanski your thoughts here would be greatly appreciated.

@klopfdreh
Copy link
Contributor Author

I renamed the classes into SpringDoc*, because it includes OpenApi and Swagger (the default integration) and I added a Unit-Test for the Filter in which you can see the behavior.

One little question to ensure this: Properties of ConfigurationProperties can be written in camel case and snake case right? So the configuration of swaggerUi can be written as swagger-ui or swaggerUi - I just want to ensure this.

@onobc can you try the branch and see if SpringDoc is working for you?

@klopfdreh klopfdreh requested a review from onobc March 2, 2022 17:23
@onobc
Copy link
Contributor

onobc commented Mar 2, 2022

@klopfdreh thanks for all of the updates. I will try to get back to this today (worst case w/in 24hrs).

Copy link
Contributor

@onobc onobc left a comment

Choose a reason for hiding this comment

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

@klopfdreh looking good. I did not get to the test but wanted to give you some feedback as I may not be able to get back to this for 1-2 days.

Thanks

@klopfdreh klopfdreh requested a review from onobc March 5, 2022 10:22
@klopfdreh
Copy link
Contributor Author

The build issue is because a SNAPSHOT dependency could not be resolved. I did not changed anything to the dependencies.

@onobc
Copy link
Contributor

onobc commented Mar 7, 2022

One little question to ensure this: Properties of ConfigurationProperties can be written in camel case and snake case right? So the configuration of swaggerUi can be written as swagger-ui or swaggerUi - I just want to ensure this.

Absolutely. What you are referring to is one of the beautiful features of config props, relaxed binding. Kebab case is the preferred choice when in yaml/properties. I believe you meant that by "snake case". Snake case is the python style underscores such as swagger_ui_ IIRC.

Copy link
Contributor

@onobc onobc left a comment

Choose a reason for hiding this comment

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

@klopfdreh
I believe we are very close on this one . Nice work!

This current round of review is mostly of the nit variety. The one bigger comment is around the auto-config and conditional loading.

Also, I think we should only put this in main (2.10 release). I was going to bring this up earlier but it we were in middle of back/forth and it would only be distracting at that point. As you alluded to in your comment from the beginning - cherrypicking to main would not be a problem. Lets go ahead and move this to main.


I will go back through my initial list of things we will want to add/cover next:

Testing

  • Unit tests where applicable
  • IT for auto-config disabled by default
  • IT for auto-config enabled by setting enabled props
  • IT for auto-config one prop enabled, one prop disabled
  • IT for custom configuration using the built-in Springdoc properties path changes

For the auto-config tests, the ApplicationContextRunner is a beautiful tool. If you have not seen it, I would be happy to handle the start of these tests and let you flesh the others out. It will give a base of where to start w/ the ApplicationContextRunner. Just lmk.

Docs

  • We should add some high level docs about this.
    • How to enable / disable
    • How to customize via openapi props (point to their doc)
    • How to view / use

@klopfdreh
Copy link
Contributor Author

The tests and the docs have to wait until I have more time at weekend - have to do my daily work now. 👍

@klopfdreh klopfdreh requested a review from onobc March 7, 2022 06:15
@onobc
Copy link
Contributor

onobc commented Mar 7, 2022

The tests and the docs have to wait until I have more time at weekend - have to do my daily work now. 👍

Sounds good @klopfdreh . I will also pull down and take a test drive and also try to push the start of the auto-config tests up before you get back to it.

@onobc
Copy link
Contributor

onobc commented Mar 11, 2022

Hi @klopfdreh, I have not forgotten about my next steps and should get to it sometime today (week got busy).

@klopfdreh
Copy link
Contributor Author

Hey @onobc no hurry 😃

@onobc
Copy link
Contributor

onobc commented Mar 12, 2022

Hi @klopfdreh
So I pulled down and started writing the auto-config test and noticed something strange.

There is something going on w/ the Springdoc config props - as in they are coming to us w/o any defaults in them. The docs suggest various defaults but these defaults are not in the actual config props but rather in scattered locations that uses the config props (afaict).

This results in our customizer adding a bunch of null patterns as well as NPE in our filter registration as it relies on config-url.

Did you encounter this?

One workaround is to set the advertised defaults in our defaults file as such:


springdoc:
  api-docs:
    enabled: false
    path: /v3/api-docs
  swagger-ui:
    enabled: false
    path: /swagger-ui.html
    config-url: /v3/api-docs/swagger-config
    validator-url: https://validator.swagger.io/validator
    oauth2-redirect-url: /swagger-ui/oauth2-redirect.html

However, I would like to be able to explain whats going on before we go that route.

@klopfdreh
Copy link
Contributor Author

klopfdreh commented Mar 13, 2022

Hey @onobc - previously I set the default values accordingly to the documentation of springdoc. I would suggest to do the same with the settings we are going to use for our purpose. https://springdoc.org/properties.html

Yeh this is what I ended up doing in the snippet above w/ the defaults and that is where I found them as well.

One big issue I have: I use an Apple Silicon and was not able to build / run the server completely.

Yeh that is a bummer. What do you get when you run ./mvnw spring-boot:run in spring-cloud-dataflow-server ?

I am not totally sure how springdoc initializes the default values but I found all Constants in this class here: https://github.com/springdoc/springdoc-openapi/blob/master/springdoc-openapi-common/src/main/java/org/springdoc/core/Constants.java

Yeh I found those as well. I dug a bit and the defaults are determined where they are used rather than directly in the config props. In general, the springdoc config props are done in a non-standard fashion as they wire them in as auto-configuration rather than have them enabled from an auto-configuration. This makes them problematic to test w/ ApplicationContextRunner. Because if you list them as auto-configs (via withConfigurations) they do not get properties. bound on them. To get properties bound you have to use an @EnableCongiurationProperties on a test config.

I will go ahead w/ setting the defaults in the file and wrap up the couple of ITs I have in progress. I will push a commit sometime tomorrow and it will be your turn to review my commit (you can pay me back for any nitpicking I have been doing up to now LOL ;) )

@onobc onobc mentioned this pull request Mar 15, 2022
3 tasks
@onobc
Copy link
Contributor

onobc commented Mar 15, 2022

Closed in favor of #4855

@onobc onobc closed this Mar 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants