Skip to content

Allow multiple levels of custom root models#4428

Closed
andyhasit wants to merge 6 commits intofastapi:masterfrom
andyhasit:allow-multiple-levels-of-custom-root-models
Closed

Allow multiple levels of custom root models#4428
andyhasit wants to merge 6 commits intofastapi:masterfrom
andyhasit:allow-multiple-levels-of-custom-root-models

Conversation

@andyhasit
Copy link
Copy Markdown

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.

exclude_defaults=exclude_defaults,
exclude_none=exclude_none,
)
try:
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown
Contributor

github-actions bot commented Oct 3, 2022

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

@codecov
Copy link
Copy Markdown

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
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

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

@tiangolo
Copy link
Copy Markdown
Member

Thanks! The recommended approach for dealing with types that would go in root models in Pydantic is to use the type annotations directly in FastAPI, as each parameter is not a model but a model field, we can have more flexibility. So, instead of `

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

You can use:

Dict[str, Level2]

Also, have in mind that some other PRs related to this were probably merged, so the original problem might be solved. And also with Pydatnic v2 you could use TypeAdapter, that would probably be much better than the root model.

Although if you have another use case, please create a Discussion with a minimal self contained example that I can copy paste to understand your use case better. 🤓

For now, I'll close this one, but thanks for the effort! 🍰

@tiangolo tiangolo closed this Aug 17, 2024
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.

4 participants