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

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

Merged
merged 3 commits into from
Oct 28, 2024

Conversation

tiholic
Copy link
Contributor

@tiholic tiholic commented Jul 15, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

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!

@kamilmysliwiec
Copy link
Member

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

@tiholic
Copy link
Contributor Author

tiholic commented Sep 7, 2021

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

@tiholic
Copy link
Contributor Author

tiholic commented Oct 16, 2021

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

@tiholic
Copy link
Contributor Author

tiholic commented Jan 4, 2022

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

@erikstoll
Copy link

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.

@tiholic
Copy link
Contributor Author

tiholic commented Jul 31, 2022

nudge @kamilmysliwiec

@eugenk
Copy link

eugenk commented Aug 16, 2022

nudge @kamilmysliwiec again ;)

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

@raphaelsoul
Copy link

merge wanted. it is really small change

tiholic and others added 3 commits October 27, 2022 22:43
for example, responses confgured in @ApiOperation should be given
     preference over what's detected from return type of the controller function

fixes: nestjs#1420
… auto-detected schema"

- add test download api to e2e cats controller
- update api-spec.json
- add test in validate-schema.e2e-spec.ts to test the generated openapi schema response
@tiholic
Copy link
Contributor Author

tiholic commented Oct 27, 2022

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

@nestjs nestjs locked as too heated and limited conversation to collaborators Oct 28, 2022
@kamilmysliwiec kamilmysliwiec merged commit c951592 into nestjs:master Oct 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants