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.

Sign up for GitHub

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

Jump to bottom

Allow multiple levels of custom root models #4428

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Allow multiple levels of custom root models #4428

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6c10d7d
Add .pyton-version to gitignore for those who use pyenv
andyhasit Jan 15, 2022
e2e81fa
Enables nested models with custom roots
andyhasit Jan 15, 2022
fc2de5b
Merge branch 'master' into allow-multiple-levels-of-custom-root-models
andyhasit Oct 3, 2022
5dbd3fa
Merge branch 'master' into allow-multiple-levels-of-custom-root-models
tiangolo Nov 27, 2022
f9681ba
Merge branch 'master' into allow-multiple-levels-of-custom-root-models
tiangolo Nov 27, 2022
1f8c7f2
Merge branch 'master' into allow-multiple-levels-of-custom-root-models
tiangolo Dec 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
.idea
.ipynb_checkpoints
.mypy_cache
.python-version
.vscode
__pycache__
.pytest_cache
Expand Down
5 changes: 5 additions & 0 deletions fastapi/routing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,11 @@ async def serialize_response(
exclude_defaults=exclude_defaults,
exclude_none=exclude_none,
)
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason behind using try-except block here?

Copy link
Author

Choose a reason for hiding this comment

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

response_content is not necessarily a dictionary. If you remove the try catch some tests fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then why don't you check the variable data type instead?

Copy link
Author

@andyhasit andyhasit Jan 16, 2022 •
edited

Choose a reason for hiding this comment

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

For two reasons:

  1. Python advocates EAFP over type checking.
  2. For all I know it may not be a dict, it could be a dict like object (which partly explains the preference for point 1)

I didn't follow the code far enough to see what all could get passed, but we're dealing with Pydantic models upstream and it's perfectly possible to implement this dict functionality in a model (see my comment on #911).

Though to be consistent I note I'm breaking EAFP inside the try catch, so I could be doing:

try:
    response_content = response_content["__root__"]
except TypeErrror, KeyError:
    pass

But I think the intention is slightly less clear.

Copy link

Choose a reason for hiding this comment

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

Actually, there's already an if isinstance(res, BaseModel) check in _prepare_response_content that might be a better place for this code, because presumably it should apply if (and only if) it's dealing with a Pydantic BaseModel instance.

Somewhat relatedly, it looks like Pydantic itself does something similar in the BaseModel.json() method, where it notices that the model has a custom __root__ and then extracts the nested value:

    def json(
        self,
        ...
    ) -> str:
        ... # (trimmed for brevity)

        data = self.dict(
            include=include,
            exclude=exclude,
            by_alias=by_alias,
            exclude_unset=exclude_unset,
            exclude_defaults=exclude_defaults,
            exclude_none=exclude_none,
        )
        if self.__custom_root_type__:
            data = data[ROOT_KEY]

.. where ROOT_KEY == "__root__", and self.__custom_root_type__ is a property of all pydantic.BaseModels (possibly None, but defined either way).

So putting all of this together - combining the logic from BaseModel.json with the approach in this PR, applied to @cikay's feedback:

 def _prepare_response_content(
     res: Any,
     *,
     exclude_unset: bool,
     exclude_defaults: bool = False,
     exclude_none: bool = False,
 ) -> Any:
     if isinstance(res, BaseModel):
         read_with_orm_mode = getattr(res.__config__, "read_with_orm_mode", None)
         if read_with_orm_mode:
             # Let from_orm extract the data from this model instead of converting
             # it now to a dict.
             # Otherwise there's no way to extract lazy data that requires attribute
             # access instead of dict iteration, e.g. lazy relationships.
             return res
-        return res.dict(
+        res_dict = res.dict(
             by_alias=True,
             exclude_unset=exclude_unset,
             exclude_defaults=exclude_defaults,
             exclude_none=exclude_none,
         )
+        if res.__custom_root_type__:
+            res_dict = res_dict["__root__"]
+        return res_dict

... or something like that (I haven't been able to try running this yet).

The fact that pydantic.BaseModel does basically the same exact thing might be a sign that it's a valid, non-crazy approach here to have well-defined behavior around types with custom __root__.

if "__root__" in response_content:
response_content = response_content["__root__"]
except TypeError:
pass
if is_coroutine:
value, errors_ = field.validate(response_content, {}, loc=("response",))
else:
Expand Down
36 changes: 36 additions & 0 deletions tests/test_serialize_response_custom_root_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
from typing import Dict

from fastapi import FastAPI
from fastapi.testclient import TestClient
from pydantic import BaseModel

app = FastAPI()


class Level2(BaseModel):
__root__: str


class Level1(BaseModel):
__root__: Dict[str, Level2]


class Level0(BaseModel):
__root__: Dict[str, Level1]


@app.get("/items/valid", response_model=Level0)
def get_valid():
cat = Level2(__root__="cat")
mammal = Level1(__root__={"mammal": cat})
animal = Level0(__root__={"animal": mammal})
return animal


client = TestClient(app)


def test_valid():
response = client.get("/items/valid")
response.raise_for_status()
assert response.json() == {"animal": {"mammal": "cat"}}