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

Basemodel.dict not converting std python dataclasses children #3764

Closed
giuliano-macedo opened this issue Feb 1, 2022 · 10 comments
Closed

Basemodel.dict not converting std python dataclasses children #3764

giuliano-macedo opened this issue Feb 1, 2022 · 10 comments
Assignees
Labels
bug V1 Bug related to Pydantic V1.X

Comments

@giuliano-macedo
Copy link
Contributor

giuliano-macedo commented Feb 1, 2022

Bug

The issue occurs with some combinations of a root and child types varying between pydantic's BaseModel, pydantic dataclass and builtin dataclass and then the method .dict() or asdict is called, the returning value does not convert the child dataclass as dict.

Output of executing the following script:

from dataclasses import dataclass
from pydantic import BaseModel

@dataclass
class _Child:
    n: int

class _Root(BaseModel):
    s:str
    c: Child

r = _Root(
    s="test",
    c=_Child(n=2)
)

print(r.dict())

Output:

{'s': 'test', 'c': _Child(n=2)}

Expected output:

{'s': 'test', 'c': {"n": 2}}

Extra info

Repo: https://github.com/giuliano-oliveira/pydantic-3764
I tested all combinations of using pydantic.dataclasses.dataclass, pydantic.BaseModel and dataclasses.dataclass as root and child types, and have noted that some combination doesn't return the expected output

Root type Child type method output returned expected output?
BaseModel BaseModel r.dict() {'s': 'test', 'c': {'n': 2}} Yes
BaseModel Pydantic dataclass r.dict() {'s': 'test', 'c': _Child(n=2)} No
BaseModel dataclass r.dict() {'s': 'test', 'c': _Child(n=2)} No
Pydantic dataclass BaseModel asdict(r) {'s': 'test', 'c': _Child(n=2)} No
Pydantic dataclass Pydantic dataclass asdict(r) {'s': 'test', 'c': {'n': 2}} Yes
Pydantic dataclass dataclass asdict(r) {'s': 'test', 'c': {'n': 2}} Yes
dataclass BaseModel asdict(r) {'s': 'test', 'c': _Child(n=2)} No
dataclass Pydantic dataclass asdict(r) {'s': 'test', 'c': {'n': 2}} Yes
dataclass dataclass asdict(r) {'s': 'test', 'c': {'n': 2}} Yes

Version info

Python: 3.9.13
pydantic: 1.10.4

@Zekii
Copy link

Zekii commented Dec 15, 2022

I reproduced the bug with that case:

Root type Child type method returned expected output?
BaseModel pydantic dataclass r.dict() No (at least in my case)
from pydantic.dataclasses import dataclass
from pydantic import BaseModel

@dataclass
class Child:
    n: int

class Root(BaseModel):
    s:str
    c: Child

r = Root(
    s="test",
    c=Child(n=2)
)

print(r.dict())

Output:

{'s': 'test', 'c': Child(n=2)}
  • Python: 3.10
  • pydantic: 1.9.0 & 1.10.2

@giuliano-macedo
Copy link
Contributor Author

giuliano-macedo commented Jan 22, 2023

@Zekii in your example your child class is using pydantic dataclass (It is the second row of my "Extra info" table).

If you change this line from pydantic.dataclasses import dataclass to dataclasses import dataclass you will get the same issue.

Edit:
You mean it didn't gave the desired output because it was outputting a Child instance instead of a dict?

If so, I guess It is another issue, that pydantic doesn't have a ".json_dict" method...

@giuliano-macedo
Copy link
Contributor Author

giuliano-macedo commented Jan 22, 2023

Closing the issue, it was solved in pydantic version 1.10.0, i don't know in which commit / PR, my guess is this one:

Refactor the whole pydantic dataclass decorator to really act like its standard lib equivalent.
It hence keeps eq, hash, ... and makes comparison with its non-validated version possible.
It also fixes usage of frozen dataclasses in fields and usage of default_factory in nested dataclasses.
The support of Config.extra has been added.
Finally, config customization directly via a dict is now possible, #2557 by @PrettyWood

For testing sake, I made a repo running the cases in the table with some pydantic versions here: https://github.com/giuliano-oliveira/pydantic-3764

FYI: @ypsah

@ypsah
Copy link

ypsah commented Jan 22, 2023

Thanks for the heads-up @giuliano-oliveira.

For clarity's sake, when you opened this issue, you reported that the output you expected of your reproducer was:

{'s': 'test', 'c': {"n": 2}}

But with the latest version of pydantic (1.10.4) it is currently:

{'s': 'test', 'c': Child(n=2)}

I agree this is an improvement over the old {'s': 'test', 'c': _Pydantic_Child_94747278016048(n=2)}, but since dataclass has an asdict() operator, it feels intuitive IMO that model.dict() would convert dataclasses into dicts as well.

The reason I'm pushing for this is that I can still reproduce tiangolo/fastapi#4402 (exact same stack trace). My understanding is that ModelField.validate() is not happy to see a dataclass instance... somehow...

Happy to work on a pydantic-specific reproducer if you want.

@ypsah
Copy link

ypsah commented Jan 22, 2023

That being said, I acknowledge that nested pydantic.dataclasses.dataclasses have never been converted into dicts. At least not since 1.7.0, included (1.6.2 failed to run my test in python3.10, I stopped there).

So maybe converting dataclasses into dicts is not the way to go.

Would like to see a real-world example of code relying on this behavior though to convince myself there is value in it.

@giuliano-macedo
Copy link
Contributor Author

I agree this is an improvement over the old {'s': 'test', 'c': _Pydantic_Child_94747278016048(n=2)}, but since dataclass has an asdict() operator, it feels intuitive IMO that model.dict() would convert dataclasses into dicts as well.

I agree with you, the problem still stands, just the mangled pydatnic dataclasses that is solved...
But I'm not sure if this is related to simplify .dict() kwarg idea or V2 plan for exports or if it is a different issue, what is your take on this?

TBH, I forgot what was my train of thought in this issue, so I assumed it was the mangled dataclass issue, I'm open to the idea of reopening the issue, or opening another one if needed.

Would like to see a real-world example of code relying on this behavior though to convince myself there is value in it.

The use case that brought me to this was that I was trying to .dict() a BaseModel directly as a return of an AWS Lambda handler function, and if a non json primitive object is returned it throws an unmarshal error, but this extends out of the dataclass -> dict issue since if a datetime or Decimal object is returned it gives the same error as well, it would be very convenient if .dict() or a kwarg or another method would return a jsonable dict ...

@samuelcolvin
Copy link
Member

I totally agree that this should be serialized to {'s': 'test', 'c': {"n": 2}}, this will be fixed in V2.

I doubt however we can change this in v1.10 as it would be a breaking chance for many.

@samuelcolvin samuelcolvin reopened this Jan 22, 2023
@samuelcolvin samuelcolvin added this to the Version 2 Issues milestone Jan 22, 2023
@giuliano-macedo
Copy link
Contributor Author

Great @samuelcolvin ! I will update the example repo and the issue :)

@giuliano-macedo
Copy link
Contributor Author

giuliano-macedo commented Jan 22, 2023

@Zekii I included your results in the issue and updated pydantic to 1.10.4 :)

@Kludex
Copy link
Member

Kludex commented Apr 29, 2023

I totally agree that this should be serialized to {'s': 'test', 'c': {"n": 2}}, this will be fixed in V2.

I doubt however we can change this in v1.10 as it would be a breaking chance for many.

I confirm this is the behavior on V2. 🙏

@Kludex Kludex closed this as completed Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug V1 Bug related to Pydantic V1.X
Projects
None yet
Development

No branches or pull requests

5 participants