Fix response_model_exclude_none having problems with uncommon defaults and required fields

[ddanier]

To output responses including a response_model FastAPI does something like (from my understanding):

# Thats what we get from the endpoint function
response_obj = SomethingModel(...)
# See https://github.com/tiangolo/fastapi/blob/master/fastapi/routing.py#L121 and https://github.com/tiangolo/fastapi/blob/master/fastapi/routing.py#L76
response_data = response_obj.dict()
# See https://github.com/tiangolo/fastapi/blob/master/fastapi/routing.py#L127, the field validation does just that
revalidated_obj = response_model(**response_data)
# See https://github.com/tiangolo/fastapi/blob/master/fastapi/routing.py#L139, jsonable_encoder will call obj.dict() again (https://github.com/tiangolo/fastapi/blob/master/fastapi/encoders.py#L52)
return jsonable_encoder(revalidated_obj.dict())

Now if we add response_model_exclude_none FastAPI will change this to:

response_obj = SomethingModel(...)
response_data = response_obj.dict(exclude_none=True)
revalidated_obj = response_model(**response_data)
return jsonable_encoder(revalidated_obj.dict(exclude_none=True))

The issue now is that the response_data already did exclude all those None values. This means that when the response_model is then instantiated again the dictionary you use here (in my code named response_data) already lost those values which may cause problems.

Now if your response model is something like:

class ModelNoneWithUncommonDefaults(BaseModel):
    x: Optional[str] = ...
    y: Optional[str] = "y"

...we have two issues with None values (think of ModelNoneWithUncommonDefaults(x=None, y=None)):

• x is required, but may be None. As response_data does not include x any more when x == None...the validation when instantiating response_model will fail with a ValidationError. The field has to be set.
• y has a default value of "y" but may be None. As response_data does not include y any more when y == None...the response will actually contain y = "y" as the validation will add its default value again. This will result in a false response.

The problem overall is the duplicate exclude_none=True and thus the missing values we already had when doing the validation.

The PR will skip exclude_none on the first conversion to dict and thus fix the issue. Not sure if my change was done at the best place to fix this, but I hope it is. Also the PR includes a new test to show the issue and ensure it does not come back later.

I believe exclude_unset and exclude_defaults should not have the same issue as those always have the same default values and the validation would not break/change anything. Anyways it would be cleaner to also not use those on the first conversion to dict I think. This has not be changed by me.

[ddanier]

I just created an issue for this, as my feeling was the PR is not enough. Please correct me if this was a dumb idea ;-)

#4648

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cf73051) 100.00% compared to head (ba8e87e) 100.00%.
Report is 1038 commits behind head on master.

❗ Current head ba8e87e differs from pull request most recent head 0eebcb9. Consider uploading reports for the commit 0eebcb9 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##            master     #4647    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          540       529    -11     
  Lines        13969     13519   -450     
==========================================
- Hits         13969     13519   -450     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

📝 Docs preview for commit ba8e87e at: https://625334f5d7935435d5b7a7d8--fastapi.netlify.app

[github-actions]

📝 Docs preview for commit 270f5f7 at: https://639de2d618290c3257ff4888--fastapi.netlify.app

[ddanier]

@tiangolo As you updated the MR just 3 days ago. Can I hope this will get fixed soon?

Thanks for your feedback!

[github-actions]

📝 Docs preview for commit a19a00c at: https://63fe6d18dce37c2ebcba0629--fastapi.netlify.app

[github-actions]

📝 Docs preview for commit 65458a5 at: https://64318b3e7e2e523d650bbe16--fastapi.netlify.app

[github-actions]

📝 Docs preview for commit 0eebcb9 at: https://647b4ea856364a0db7bbf9ed--fastapi.netlify.app

[YuriiMotov]

This issue isn't relevant to Pydantic v.2, but it's still relevant to Pydantic v.1 (tested on FastAPI 0.110.0 and Pydantic 1.10.14).

New added test fails before changes are made (on Pydantic v.1) and passes after.

Regarding the code. I would just remove the line with exclude_none. Comment there is unnecessary.

[YuriiMotov]

I Think this comment is unnecessary. I would just remove the line with exclude_none=