swagger icon indicating copy to clipboard operation
swagger copied to clipboard

fix(transformer): prefer explicit config over auto-detected schema

Open tiholic opened this issue 3 years ago • 9 comments

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!

tiholic avatar Jul 15 '21 20:07 tiholic

Thanks for your contribution! Can you please update tests as well?

kamilmysliwiec avatar Jul 16 '21 06:07 kamilmysliwiec

@kamilmysliwiec updated tests, please review and release! Thanks :)

tiholic avatar Sep 07 '21 09:09 tiholic

@kamilmysliwiec please review and release changes in this PR. Thanks :)

tiholic avatar Oct 16 '21 12:10 tiholic

@kamilmysliwiec little attention here please. I'm desperate to get this released soon 😅

tiholic avatar Jan 04 '22 04:01 tiholic

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.

erikstoll avatar May 31 '22 07:05 erikstoll

nudge @kamilmysliwiec

tiholic avatar Jul 31 '22 15:07 tiholic

nudge @kamilmysliwiec again ;)

Sorry for pushing it this hard. I just think that the importance of this fix for some projects is underestimated.

eugenk avatar Aug 16 '22 09:08 eugenk

merge wanted. it is really small change

raphaelsoul avatar Oct 27 '22 09:10 raphaelsoul

@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.

tiholic avatar Oct 27 '22 17:10 tiholic