Fix unskipped defaults

[dmontagu]

Addresses #421

Currently, if you return a BaseModel instance in a route with a BaseModel response_type, the skip_defaults value does not influence the result. This PR fixes this.

---

Details

The source of the problem is that calling field.validate on a model results in a new model with the fields set. I dug in on the pydantic side and it looks like this is due to the recent change to re-parse the model with a non-subclass type when response_model is set. If you look in BaseModel.validate (which is ultimately called if you follow it deep enough), you can see the precise spot where defaults aren't being skipped:

@classmethod
    def validate(cls: Type['Model'], value: Any) -> 'Model':
        if isinstance(value, dict):
            return cls.parse_obj(value)
        elif isinstance(value, cls):
            return value.copy()
        elif cls.__config__.orm_mode:
            return cls.from_orm(value)
        else:
            with change_exception(DictError, TypeError, ValueError):
                return cls.parse_obj(value)

It calls into parse_obj on the last line, which subsequently calls dict on the object and then parses the dict. Unfortunately, this means defaults don't get skipped.

I don't think it makes sense to change this on the pydantic side because otherwise you'd run into problems parsing models with default values into non-subclass models with required values. So, if we want to stick with this approach of creating a new class to force a reparse, we probably need to handle the

---

That said, I would also be in favor of a larger refactor if we could come up with a way to get the desired field safely without another round of parsing/dumping 😬. But that does seem hard.

[codecov]

Codecov Report

Merging #422 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #422   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         240    241    +1     
  Lines        5626   5642   +16     
=====================================
+ Hits         5626   5642   +16

Impacted Files	Coverage Δ
fastapi/routing.py	100% <100%> (ø)	⬆️
tests/test_skip_defaults.py	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 033bc2a...0843c6b. Read the comment docs.

[dmontagu]

        if isinstance(response, BaseModel):
            value = response.dict(skip_defaults=skip_defaults)

might also work, but I'm not sure if that would play differently with include/exclude/jsonable_encoder. And given skip_defaults is probably usually false, I think the version above will be more performant in most cases.

[tiangolo]

Wow, you had to go quite deep into the rabbit hole to find that (field.validate and BaseModel.validate are quite deep inside the callback flow in Pydantic). Good job! Thanks! 👏 🚀

I updated the test to check for defaults in sub-models. And updated the implementation to use value = response.dict(skip_defaults=skip_defaults) to take into account those sub-models.

---

And yeah, it's unfortunate to have to do a second parse of the models and have to "clone" models.

That came after adding Pydantic ORM mode, as now the data is serialized by Pydantic directly (including DB models). And a sub-model including something sensitive (e.g. a hashed_password) still passes validation as-is for the parent model. So, a model class UserWithPassword(BaseUser) passes validation for a model class BaseUser(BaseModel), including the extra sensitive data.

To avoid that security issue, I had to "clone" the models declared in response_model, to make sure they are a "different" class, and sub-classes don't pass directly but have to be parsed and validated.