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

Wrong parent class for pydantic V2 root models #1435

Closed
andreyLetenkovWefox opened this issue Jul 19, 2023 · 9 comments · Fixed by #1448 or #1477
Closed

Wrong parent class for pydantic V2 root models #1435

andreyLetenkovWefox opened this issue Jul 19, 2023 · 9 comments · Fixed by #1448 or #1477

Comments

@andreyLetenkovWefox
Copy link

andreyLetenkovWefox commented Jul 19, 2023

Describe the bug
Generator uses pydantic.BaseModel as parent class for root model instead of pydantic.RootModel

Example schema (custom_id.yaml):

openapi: 3.0.0
components:
  schemas:
    CustomId:
      description: My custom ID
      type: string
      format: uuid

Used commandline:

$ datamodel-codegen --input custom_id.yaml --output-model-type pydantic_v2.BaseModel --output model.py

Contents of model.py:

from __future__ import annotations

from uuid import UUID

from pydantic import BaseModel, Field


class CustomId(BaseModel):
    root: UUID = Field(..., description='My custom ID')

Expected behavior

from __future__ import annotations

from uuid import UUID

from pydantic import RootModel, Field


class CustomId(RootModel):
    root: UUID = Field(..., description='My custom ID')

Version:

  • OS: [e.g. iOS]
  • Python version: 3.10.8
  • datamodel-code-generator version: 0.21.1
@koxudaxi
Copy link
Owner

@andreyLetenkovWefox
Thank you for creating the issue.
I have tested your schema. But I can't reproduce the issue.
Do you use a custom template for root model?

@andreyLetenkovWefox
Copy link
Author

@andreyLetenkovWefox Thank you for creating the issue. I have tested your schema. But I can't reproduce the issue. Do you use a custom template for root model?

Sorry, I thought the problem could be reproduced via a CLI generator call, but it really only appears when using the python package:

from pathlib import Path

from datamodel_code_generator import DataModelType, generate

generate(
    input_=Path("custom_id.yaml"),
    output=Path("model.py"),
    output_model_type=DataModelType.PydanticV2BaseModel,
)

airwoodix added a commit to airwoodix/datamodel-code-generator that referenced this issue Jul 24, 2023
airwoodix added a commit to airwoodix/datamodel-code-generator that referenced this issue Jul 24, 2023
airwoodix added a commit to airwoodix/datamodel-code-generator that referenced this issue Jul 24, 2023
Signed-off-by: Etienne Wodey <etienne.wodey@aqt.eu>
@airwoodix
Copy link
Contributor

We hit this issue in a slightly different setting: if output_data_model = "pydantic_v2.BaseModel" is read from pyproject.toml and not passed on the command line, root models have the wrong base class. However, if --output_data_model "pydantic_v2.BaseModel" is passed on the command line, the root model base class is correct.

I drafted a fix in #1448 but it still needs quite some work (mostly because it only works with Pydantic v2 at the moment, and relies on using a Config instance as input to generate, so it won't fix @andreyLetenkovWefox 's example in #1435 (comment)).

@koxudaxi would you like to see further work in the direction of #1448? or do you have a simpler idea for the fix? It seems that the handling of base_class could be simplified in the parser stack, but this looks quite intrusive to me.

@barreeeiroo
Copy link
Contributor

TL;DR: The issue comes from base_model incorrectly overriding RootModel.

I was also running into this issue, but not in the same way as @andreyLetenkovWefox. Apparently, if you set the base_class parameter and use Pydantic V2, it will incorrectly set the Base Model for Root Models to your custom Base Class.

See the following example:

openapi: 3.0.0
components:
  schemas:
    CustomId:
      description: My custom ID
      type: string
      format: uuid
datamodel-codegen --input custom_id.yaml --base-class "utils.dto_class.BaseDtoModel" --output-model-type pydantic_v2.BaseModel

This will output:

from __future__ import annotations

from uuid import UUID

from pydantic import Field
from utils.dto_class import BaseDtoModel


class CustomId(BaseDtoModel):
    root: UUID = Field(..., description='My custom ID')

This, for Pydantic V2, is wrong as it will be interpreted as a normal model with a literal root field (and not a Root Model). In Pydantic V1 this was not an issue because it was being populated through the __root__ field which uses BaseModel as the upper class (just like a normal model).

But here arises another question: Should base_class actually override RootModel as well? In Pydantic V1 this made sense, but for Pydantic V2 probably RootModel should never be overwritten.
IMO, RootModel should not be overwritten even with a custom base_class as it implements several methods which are not available in normal models. However, a developer may want to define their custom RootModel as well, so not sure what's the correct procedure for this...

@adaamz
Copy link
Contributor

adaamz commented Aug 8, 2023

Also noticed the difference like @barreeeiroo mentioned. I think it's a problem for us now.
Any ETA for this? :)
Just wanted to upgrade this lib so we can swiftly start upgrade to Pydantic V2 yet, but this is probably no-go for us... but no rush, our upgrade process is still far away 😅

@koxudaxi
Copy link
Owner

koxudaxi commented Aug 8, 2023

Hey everyone, I'm sorry that the v2 root models have problems.
Tomorrow, I will get a half-off day for OSS tasks. And I will release the fixed version.

@koxudaxi
Copy link
Owner

koxudaxi commented Aug 8, 2023

a developer may want to define their custom RootModel as well, so not sure what's the correct procedure for this...

Should we add the base_class option for the RootModel in v2? 🤔

@barreeeiroo
Copy link
Contributor

barreeeiroo commented Aug 8, 2023

a developer may want to define their custom RootModel as well, so not sure what's the correct procedure for this...

Should we add the base_class option for the RootModel in v2? 🤔

That's the big question... In my opinion, we shouldn't. Think about RootModel in a similar way as an Enum: they are "technically" models but with a fixed set of properties (root models are basically single types with some constraints, and enums are single values from a closed list set). In contrast, normal models are open models in a way that new properties could be added or their serialization changed (this is not the case for either root models or enums as they are already primitive types).
If we were to allow overriding the base class for root models, we should also allow overriding it for enums as developers may want to customize their base class too.

@koxudaxi
Copy link
Owner

koxudaxi commented Aug 8, 2023

I agree with you.
We should consider passing custom_root_model_base_class and custom_enum_base_class as options to rootModel and enum in the future, but I don't think it is necessary now.

koxudaxi added a commit that referenced this issue Aug 9, 2023
* main: fix #1435 for pydantic v2 only

Signed-off-by: Etienne Wodey <etienne.wodey@aqt.eu>

* Fix for v1

* Remove unused code

* Add unittest

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Fix unittest

---------

Signed-off-by: Etienne Wodey <etienne.wodey@aqt.eu>
Co-authored-by: Koudai Aono <koxudaxi@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants