swagger
swagger copied to clipboard
fix(transformer): prefer explicit config over auto-detected schema
PR Checklist
Please check if your PR fulfills the following requirements:
- [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
- [x] Tests for the changes have been added (for bug fixes / features)
- [x] Docs have been added / updated (for bug fixes / features)
PR Type
What kind of change does this PR introduce?
[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:
What is the current behavior?
@ApiOperation.responses was disregarded as the auto-detected responses were given preference. Auto-detection happens always, even if no return type is configured.
Issue Number: #1420
What is the new behavior?
Explicit config is given preference over auto-detected schema
Does this PR introduce a breaking change?
[ ] Yes
[x] No
Other information
This PR only fixes the issue on a smaller level, however there is a scope for improving how the actual merging should happen for the schema extracted from different areas.
Each schema extracted should be assigned a priority status (like constants in the library code). For example, body extracted from controller function argument should be given a priority of 0, one that is defined as @ApiParam should be given a priority of 10, and the one in @ApiOperation a priority of 50. When making the final schema, we should be able to order all the configured schemas by priority and pick first one.
This is a very vague suggestion, and not sure if there is a way to implement this as we'd be dealing with nested json structures, but may provide a solution for easily deciding what's important over what!
Thanks for your contribution! Can you please update tests as well?
@kamilmysliwiec updated tests, please review and release! Thanks :)
@kamilmysliwiec please review and release changes in this PR. Thanks :)
@kamilmysliwiec little attention here please. I'm desperate to get this released soon 😅
I have a similar use case and also noticed this bug. I looked into the PR and tested it with my use case. It fixes the bug for me and enables my use case.
So I’d like to ask what the status of this PR is? Can it be merged? Can something be contributed here?
I’m also desperate to have this fix released.
nudge @kamilmysliwiec
nudge @kamilmysliwiec again ;)
Sorry for pushing it this hard. I just think that the importance of this fix for some projects is underestimated.
merge wanted. it is really small change
@kamilmysliwiec rebased over all the latest changes. I believe you can prioritise releasing these changes considering that the changes involved in functional code is just one line.