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.

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

Allow multiple levels of custom root models #4428

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

Conversation

andyhasit
Copy link

This relates to #911 which has a fix, but that doesn't work for models with custom roots which nest models with custom roots.

class Level2(BaseModel):
    __root__: str


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


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

Note that I am new to pydantic, so let me know if the above is not something anyone should be doing.

@@ -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

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.

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__.

@kkroening
Copy link

kkroening commented Jun 2, 2022

This still seems to be an issue, and from what I can tell, this PR is the path of least resistance for fixing the issue.

While, it may seem strange to have to have a seemingly magical if "__root__" in response_content:, it's directly related to a longstanding issue with Pydantic's .dict() behavior that won't be fixed until Pydantic v2:

dict() behaviour will change in v2 to default to returning the root object, but that's backwards incompatible so can't happen now. It's also not a bug.

The fundamental issue seems to be that Pydantic's .dict() -> pydantic.parse_* does not round-trip correctly: .dict() wraps the result inside {"__root__": ...} but pydantic.parse_* does not accept such wrapping.

Oddly enough, .json() -> .parse_raw(...) does round-trip correctly, unlike .dict() -> .parse_obj(...); and unfortunately this .dict() asymmetry impacts FastAPI.

I realize that it's generally sub-optimal to design APIs around reliance of __root__ containers, but it's often the case that it's not an option to change existing API contracts, so there is a valid use case for this fix.

Apparently the .dict() behavior will be "fixed" in Pydantic v2 (as mentioned above) at which point this may become a non-issue, but there's the question of what to do in the meantime. From what I can tell, FastAPI works almost flawlessly with __root__ except for the quirky wrapping that Pydantic does with .dict(), so a small bandaid could go a long ways.

FWIW, here's a minimum-viable example that demonstrates the issue:

class Item(pydantic.BaseModel):
    name: str


class ItemMap(pydantic.BaseModel):
    __root__: dict[str, Item]


class Container(pydantic.BaseModel):
    __root__: dict[str, ItemMap]


sample_obj = Container(
    __root__={
        'group1': ItemMap(
            __root__={
                'id1': {'name': 'Item 1'},
                'id2': {'name': 'Item 2'},
            }
        ),
        'group2': ItemMap(
            __root__={
                'id3': {'name': 'Item 3'},
            }
        ),
    }
)


@app.get('/index', response_model=Container)
async def handle_index() -> Container:
    return sample_obj

Error message:

2 validation errors for Container
response -> __root__ -> __root__ -> __root__ -> group1 -> name
  field required (type=value_error.missing)
response -> __root__ -> __root__ -> __root__ -> group2 -> name
  field required (type=value_error.missing)

This PR seems to fix the issue, so it would be great if this could be finalized/merged/released.

In the meantime, this may be a viable workaround (at least in some cases) - manually apply .dict() and unpack __root__ before returning dict[str, Any]:

@app.get('/index', response_model=Container)
async def _handle_index() -> dict[str, Any]:
    return sample_obj.dict()['__root__']  # type: ignore

Note that the schema still shows up correctly thanks for response_model=Container, despite the -> dict[str, Any] type annotation.

The full example code, along with test cases that demonstrate the underlying Pydantic .dict() behavior issue is attached below.

(edit: I just saw that this PR already has a similar example test case, so I guess this is just an alternate example with a corresponding workaround)

(Click to expand)
import fastapi
import fastapi.testclient
import http
import json
import pydantic
import pytest
import textwrap
from typing import Any


class Item(pydantic.BaseModel):
    name: str


class ItemMap(pydantic.BaseModel):
    __root__: dict[str, Item]


class Container(pydantic.BaseModel):
    __root__: dict[str, ItemMap]


sample_obj = Container(
    __root__={
        'group1': ItemMap(
            __root__={
                'id1': {'name': 'Item 1'},
                'id2': {'name': 'Item 2'},
            }
        ),
        'group2': ItemMap(
            __root__={
                'id3': {'name': 'Item 3'},
            }
        ),
    }
)


def test_pydantic__serialization_quirks() -> None:
    rootless_dict = {
        'group1': {
            'id1': {'name': 'Item 1'},
            'id2': {'name': 'Item 2'},
        },
        'group2': {
            'id3': {'name': 'Item 3'},
        },
    }
    rootless_json = json.dumps(rootless_dict)
    rooted_dict = {'__root__': rootless_dict}
    rooted_json = json.dumps(rooted_dict)

    # Serialization quirks - `.dict()` emits `__root__` but `.json()` doesn't:
    assert sample_obj.json() == rootless_json  # (without __root__)
    assert sample_obj.dict() == rooted_dict  # (*with* __root__)

    # Parsing quirks - neither `parse_raw` nor `parse_obj` expect `__root__`.
    assert Container.parse_raw(rootless_json) == sample_obj
    assert Container.parse_obj(rootless_dict) == sample_obj
    with pytest.raises(pydantic.ValidationError):
        assert Container.parse_raw(rooted_json)
    with pytest.raises(pydantic.ValidationError):
        assert Container.parse_obj(rooted_dict)

    # In other words, dump->parse round-trips correctly for `.json()` but *not* `.dict()`:
    assert Container.parse_raw(sample_obj.json()) == sample_obj
    with pytest.raises(pydantic.ValidationError):
        assert Container.parse_obj(sample_obj.dict()) == sample_obj

    # Workaround:
    assert Container.parse_obj(sample_obj.dict()['__root__']) == sample_obj


def test__fastapi__validation_error() -> None:
    app = fastapi.FastAPI()

    @app.get('/index', response_model=Container)
    async def handle_index() -> Container:
        return sample_obj

    testclient = fastapi.testclient.TestClient(app)
    with pytest.raises(pydantic.ValidationError) as excinfo:
        testclient.get('/index')

    assert (
        str(excinfo.value)
        == textwrap.dedent(
            '''
            2 validation errors for Container
            response -> __root__ -> __root__ -> __root__ -> group1 -> name
              field required (type=value_error.missing)
            response -> __root__ -> __root__ -> __root__ -> group2 -> name
              field required (type=value_error.missing)
            '''
        ).strip()
    )


def test__fastapi__workaround() -> None:
    app = fastapi.FastAPI()

    @app.get('/index', response_model=Container)
    async def _handle_index() -> dict[str, Any]:
        return sample_obj.dict()['__root__']  # type: ignore

    testclient = fastapi.testclient.TestClient(app)
    response = testclient.get('/index')
    assert response.json() == json.loads(sample_obj.json())
    assert response.status_code == http.HTTPStatus.OK

Setup:

virtualenv venv
. venv/bin/activate
pip install fastapi mypy pytest requests types-requests
mypy --strict test_example.py && pytest test_example.py

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2022

📝 Docs preview for commit fc2de5b at: https://633b08f24f297c1662e3d281--fastapi.netlify.app

@codecov
Copy link

codecov bot commented Oct 3, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ Current head fc2de5b differs from pull request most recent head 1f8c7f2. Consider uploading reports for the commit 1f8c7f2 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##            master    #4428      +/-   ##
===========================================
- Coverage   100.00%   99.99%   -0.01%     
===========================================
  Files          540      541       +1     
  Lines        13969    13961       -8     
===========================================
- Hits         13969    13960       -9     
- Misses           0        1       +1     

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

@github-actions
Copy link
Contributor

📝 Docs preview for commit 5dbd3fa at: https://638367288cebdf76dd9a335a--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit f9681ba at: https://63836fccadf1a07bb1911271--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 1f8c7f2 at: https://639ce81d580637075d23446a--fastapi.netlify.app

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants