-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
✨ Don't revalidate the response content if it is of the same type as response_model
#4416
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
base: master
Are you sure you want to change the base?
Conversation
5489df0 to
28c24dc
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
📝 Docs preview for commit a9b3e8c at: https://639cd42098600317dc000812--fastapi.netlify.app |
|
@tiangolo any chance this PR will be reviewed? |
|
Rebuilding a response model is a real problem and cannot actually be done correctly because of aliases and other settings (I guess I've already left a comment about this). In normal code your create it by your own from the other sources that could not be converted automatically so it is actually not needed. Can we expect this to be reviewed and fixed? |
|
@tiangolo hi! Can you answer on this comment? |
|
Hi! Thank you! |
|
@tiangolo are there any plans on merging or reimplementing this? |
|
@tiangolo is there any way this PR could be merged? |
response_modelresponse_model
patrick91
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I rebased it and update the tests for Pydantic 2 :)
d2946c3 to
b582069
Compare
|
Updated codes and tests. When users are using Pydantic v2 (fortunately, most applications should be using v2 by now), this issue doesn’t need much attention. In v2, [This is part of the validator schema for the Although there are some additional function calls, it won’t trigger the validator in a way that would modify the original Therefore, this fix is only relevant for applications that are still using Pydantic v1. |
b582069 to
b5fcef9
Compare
b5fcef9 to
ec08e23
Compare
| def test_response_model_should_not_revalidate_response_content_if_they_had_same_type(): | ||
| response = client.post("/increment") | ||
| response.raise_for_status() | ||
| assert response.json() == {"count": 1} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm: on master and with Pydantic 1.10.22, this fails with
AssertionError: assert {'count': 2} == {'count': 1}
This comment was marked as resolved.
This comment was marked as resolved.
|
The changes from #14099 are interfering with this PR... 🕵️♀️ [UPDATE]: solved ✔️ |
There's no need to use
secure_cloned_response_fieldto revalidate theresponse_contentif the response object type is exactly the same asresponse_modelrelated issues:
response_modelseem to__init__the same object twice? #2359Updated comment: #4416 (comment)