Skip to content

🐛 Fix custom response input output schema bug#11517

Closed
slafs wants to merge 5 commits intofastapi:masterfrom
slafs:fix-custom-response-input-output-schema-bug
Closed

🐛 Fix custom response input output schema bug#11517
slafs wants to merge 5 commits intofastapi:masterfrom
slafs:fix-custom-response-input-output-schema-bug

Conversation

@slafs
Copy link
Copy Markdown
Contributor

@slafs slafs commented May 2, 2024

This PR addresses the issue (point 1.) from #10697

Add missing mode argument when calling create_model_field
for "custom" response models defined in the responses option of a route.
The same way we do that for the "main" response model (see the other call of create_model_field).

Otherwise that custom response model schema (if separate i/o models are enabled)
is generated with the "-Input" suffix (instead of -Output).

NOTE this PR doesn't address docs issues that are mentioned in #10697 (point 2.)

slafs added 2 commits May 2, 2024 15:07
Which should be "serialization"
also for custom response models defined in the `responses` option.

The same way we do that for the "main" response model on line 475.

This fixes issue 1. mentioned in
#10697
@alejsdev alejsdev added the bug Something isn't working label May 4, 2024
@alejsdev alejsdev changed the title Fix custom response input output schema bug 🐛 Fix custom response input output schema bug May 4, 2024
@slafs
Copy link
Copy Markdown
Contributor Author

slafs commented Jun 17, 2024

@alejsdev any chance on approving/merging this? 😬

@alejsdev
Copy link
Copy Markdown
Member

alejsdev commented Sep 3, 2024

Hi @slafs, thank you for your interest in contributing to FastAPI. We have a high volume of PRs, we're reviewing and classifying them. We'll come back to review your PR in detail, we appreciate your patience as we manage the queue. 🙇‍♀️

@flxdot
Copy link
Copy Markdown
Contributor

flxdot commented Sep 5, 2024

Seems like a duplicate of #10895. Would love to see any of PRs the merged. :)

@slafs
Copy link
Copy Markdown
Contributor Author

slafs commented Sep 5, 2024

Oh yeah. Cool. Although, I feel my test is more explicit about the custom responses bug.

@svlandeg svlandeg self-assigned this Sep 18, 2024
Copy link
Copy Markdown
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Hi @slafs,

Thanks for the contribution, and apologies for the delay in reviewing this!

Your unit tests & example code from #10697 clearly demonstrate the issue. I could reproduce the bug on master and confirm that the test fails on master and succeeds with this PR.

Because #10895 contains the same fix and was submitted earlier, I would suggest to merge that one and close this one. I have pushed your test to that PR as well though, as I agree that this test is slightly more clear.

I will leave this up to Tiangolo for a final review and then we'll hopefully be able to merge the fix soon 🙏

Thanks again!

@slafs
Copy link
Copy Markdown
Contributor Author

slafs commented Sep 18, 2024

Thanks @svlandeg!

Is there anything I can do regarding the documentation issue mentioned in #10697 (point 2.)?

@tiangolo
Copy link
Copy Markdown
Member

Thank you @slafs! 🚀

And thanks for the help @alejsdev, @svlandeg 🙇

This was fixed in #10895, it will be available in FastAPI 0.115.1 in the next hours. 🎉

I'll now close this one, but thanks for your effort! 🍰

@tiangolo tiangolo closed this Oct 12, 2024
@slafs slafs deleted the fix-custom-response-input-output-schema-bug branch October 12, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants