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

Enhance Operation Scanning Mechanism #1020

Closed
azige opened this issue Jan 15, 2021 · 7 comments
Closed

Enhance Operation Scanning Mechanism #1020

azige opened this issue Jan 15, 2021 · 7 comments
Labels
enhancement New feature or request

Comments

@azige
Copy link
Contributor

azige commented Jan 15, 2021

Is your feature request related to a problem? Please describe.

I installed the library and configure methods in a @Controller with @Operation. I was confused that some of my @Controller methods get picked and included in OpenAPI model, but others are not.

Later I learned the library is aimed to @RestController. @Controller should be configured through SpringDocUtils.getConfig().addRestControllers to be exposed.

But for version 1.5.2, any method with @ResponseBody in @Controller will be "leaked" to the OpenAPI model even that user does not configure SpringDocUtils.getConfig().addRestControllers.


Describe the solution you'd like

There are two ways.

1. Pick every @Operation in @Controller

Any @Operation should be picked no matter in @RestController or @Controller. Users expect a method get picked if they annotate @Operation on it. I think it makes more sense this way.

The problem is it may break the library's original purpose.

2. Strictly pick methods in @RestController only

this solution is dropped.

It means, any methods in @Controller won't be picked unless explicitly configured SpringDocUtils.getConfig().addRestControllers. Users won't get confused if their entire @Controller is missing from OpenAPI model and will search the document to find how to expose it.

The problem is it may cause compatibility issues.


The solution 1 is preferred. This test set should be passed later after new version released.

@bnasslahsen
Copy link
Contributor

@azige,

Note this feature is not necessary, but we can qualify it as a nice to have.
Projects that wants to scan for @Controller, use SpringDocUtils.getConfig().addRestControllers as explained in the documentation.

If you are willing to propose a PR for this feature we can keep it open.

@azige
Copy link
Contributor Author

azige commented Jan 16, 2021

Yes things can get worked currently. I don't think the "leaking" is a problem as user can easily find it out and add configuration to ignore.

It just confuses some users, especially users from SpringFox. SpringDoc stands as an alternative to SpringFox but it does have different behaviour to SpringFox even in such basic scanning strategy and have a poor document now (I think a good document should not explain many things in FAQ).

I'm trying to be familiar to the code and implement the solution 1.

@azige
Copy link
Contributor Author

azige commented Jan 16, 2021

azige@2684a92

I simplely modified OpenApiResource.isRestController to achieve solution 1. I added a new test and all other tests are passed.

I can't determine this behaviour should be default. Maybe a property to control it is need.

@bnasslahsen
Copy link
Contributor

bnasslahsen commented Jan 17, 2021

@azige,

Thank you for your first contribution to the project.

If the developer adds the @Operation annotation, we can assume that he wants his Rest API to be scanned.
In general, we make feature available for webmvc and weblfux.

You can propose your PR, by just adding the support for webflux as well.
I will merge it asap, for the upcoming release.

@azige
Copy link
Contributor Author

azige commented Jan 18, 2021

azige@c8ca4ad

I dig a bit further into the code and think the @Operation check should have a higher priority. Returning ModelAndView is a similar case to my case returning the view name directly. I can't figure out what operationPath.startsWith(DEFAULT_PATH_SEPARATOR) is for yet. Although the tests are all passed.

I have less knowledge about WebFlux and may need some help.

I will prepare a PR if the solution is acceptable.

@bnasslahsen
Copy link
Contributor

For the test against operationPath.startsWith(DEFAULT_PATH_SEPARATOR), i do not have in mind the reason; But i would guess if you remove it, you should see one of the tests failing.

The annotations for webflux with annotated controllers works are almost the same as the webmvc.

Anyway, once you submit your PR, i will review it.

@azige
Copy link
Contributor Author

azige commented Jan 18, 2021

I found the condition in webflux module https://github.com/azige/springdoc-openapi/blob/43efad00f11b9340c9ad702a29c649f79e7e5626/springdoc-openapi-webflux-core/src/main/java/org/springdoc/webflux/api/OpenApiResource.java#L189. But I'm not sure whether the presenting of @Operation should have higher priority than operationPath.startsWith(DEFAULT_PATH_SEPARATOR).

I will leave the work to you and open a PR. Thank you for all the great work for the project!

azige added a commit to azige/springdoc-openapi that referenced this issue Jan 18, 2021
This was referenced Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants