V1.9 Name Collisions JSON model_as_dict

[zrothberg]

Checks

• I added a descriptive title to this issue
• I have searched (google, github) for similar issues and couldn't find anything
• I have read and followed the docs and still think this is a bug

Bug

In 1.9 Code changed in this merge #2650 The "models_as_dict" option for json has collision issues when the class names are the same but in multiple files. This is because of the change to name in custom_pydantic_encoder. In the following code this will result in an error as DBUser.User does not have the field Address. If we force the encoder to be defined after the class this issue does not happen. The other option would be modification of update_forward_refs but that seems a lot worse.

Output of python -c "import pydantic.utils; print(pydantic.utils.version_info())":

  pydantic version: 1.9.0a2
  pydantic compiled: True
  install path: /PycharmProjects/pythonProject/venv/lib/python3.8/site-packages/pydantic
  python version: 3.8.9 (default, Oct 26 2021, 07:25:53)  [Clang 13.0.0 (clang-1300.0.29.30)]
 
 Tried it in 3.10 as well. 

##DBusers.py
from pydantic import BaseModel
from uuid import UUID

class User(BaseModel):
    name: str
    user_id: UUID

##this one throws Exception
from typing import List, Optional

from pydantic import BaseModel
from DBUser import User as DBUser
from uuid import uuid4

class Address(BaseModel):
    city: str
    country: str


class User(BaseModel):
    name: str
    address: Address
    friends: Optional[List['User']] = None
    connections: Optional[List[DBUser]]=None

    class Config:
        json_encoders = {
            Address: lambda a: f'{a.city} ({a.country})',
            "User": lambda u: f'{u.name} in {u.address.city} '
                              f'({u.address.country[:2].upper()})',
        }


User.update_forward_refs()

wolfgang = User(
    name='Wolfgang',
    address=Address(city='Berlin', country='Deutschland'),
    friends=[
        User(name='Pierre', address=Address(city='Paris', country='France')),
        User(name='John', address=Address(city='London', country='UK')),
    ],
    connections=[
        DBUser(name="joe",user_id=uuid4()),
        DBUser(name="Jeff", user_id=uuid4()),
    ]
)
wolfgang.json(indent=2,models_as_dict=False) ##Raises AttributeError: 'User' object has no attribute 'address'

##this one does not throw Exception

from typing import List, Optional
from pydantic import BaseModel
from DBUser import User as DBUser
from uuid import uuid4

class Address(BaseModel):
    city: str
    country: str


class User(BaseModel):
    name: str
    address: Address
    friends: Optional[List["User"]] = None
    connections: Optional[List[DBUser]]=None

    class Config:
        json_encoders = {
            Address: lambda a: f'{a.city} ({a.country})',
        }


User.update_forward_refs()

User.Config.json_encoders[User] =lambda u: f'{u.name} in {u.address.city} ' f'({u.address.country[:2].upper()})'


wolfgang = User(
    name='Wolfgang',
    address=Address(city='Berlin', country='Deutschland'),
    friends=[
        User(name='Pierre', address=Address(city='Paris', country='France')),
        User(name='John', address=Address(city='London', country='UK')),
    ],
    connections=[
        DBUser(name="joe",user_id=uuid4()),
        DBUser(name="Jeff", user_id=uuid4()),
    ]
)
print(wolfgang.json(indent=2,models_as_dict=False)) ##Does not explode

[samuelcolvin]

I'm a bit unclear on what does "explode"?

I don't think this is a show stopper, but we should definitely look into it and at least add a warning to the docs.

[zrothberg]

Yep so what happens is DBUser gets encoded with the custom encoder for User. DBUser doesn't have the same fields and thus the .json throws an error. Neither object is a descendant of the other one they just share the same value for __name__.

This is the code causing the issue:
https://github.com/samuelcolvin/pydantic/blob/ef4678999f94625819ebad61b44ea264479aeb0a/pydantic/json.py#L100-L113

We are checking the field __name__ all classes get their name at class definition time. They are not guaranteed to be unique between files or modules.

As far as I can tell the only use of checking __name__ is so that a class's encoder can reference the class's type. Since this is also achievable by appending the encoder after class creation I don't think it is worth leaving the bug in.

@PrettyWood wrote the code so he will obviously know more then me. Is it needed for any other behavior?

Just to isolate the code a bit clearer

##Bad Code
from DBUser import User as DBUser
##DBUser.__name__ == "User"

class User(BaseModel):
    name: str
    address: Address
    friends: Optional[List['User']] = None
    connections: Optional[List[DBUser]]=None

    class Config:
        json_encoders = {
            Address: lambda a: f'{a.city} ({a.country})',
            "User": lambda u: f'{u.name} in {u.address.city} '
                              f'({u.address.country[:2].upper()})',
        }

User.update_forward_refs()

##Good Code
from DBUser import User as DBUser
##DBUser.__name__ == "User"

class User(BaseModel):
    name: str
    address: Address
    friends: Optional[List["User"]] = None
    connections: Optional[List[DBUser]]=None

    class Config:
        json_encoders = {
            Address: lambda a: f'{a.city} ({a.country})',
        }


User.update_forward_refs()

User.Config.json_encoders[User] =lambda u: f'{u.name} in {u.address.city} ' f'({u.address.country[:2].upper()})'

[samuelcolvin]

humm, having looked at this more, I still don't think it should block releasing v1.9.

@PrettyWood, what do you think?

I don't particularly want to remove support for json_encoders being strings at this late stage, and I don't see another route.

As you've said, the User.Config.json_encoders[User] = work around is available for those that do have this problem.

[PrettyWood]

@samuelcolvin I agree. I feel like the bug is quite rare and "expected". Since a workaround exists and is simple for this bug, I think it's fine.

[zrothberg]

So i have a working patch for the above done already. I am just building out some test cases to make sure inheritance is not affected. I put it into update_forward_refs so it runs automatically on class creation. And can be rerun later down the code.

I believe there is also another bug which caused the original need for a keyword. I should have both wrapped up with functional tests tonight. I was up late so just need to verify the behavior properly. Can yall give me like 12 hours to wrap it up? I can put them in separate pull requests incase you want to keep one but not the other.

[samuelcolvin]

Sorry, I didn't see the above two comments. I've created a fix for this #3595, should have the secondary effect of making validation a little bit faster by removing the need to check by name as well.

@zrothberg could you review the PR and check what you think.

If you have a better fix, feel free to create another PR. If you have some additional tests to add, add a link to them in the PR and I'll add them and credit you in the fix.

[zrothberg]

Yep so that clears up the top level bug the other one remaining from models_as_dict is it causes only shallow changes. Only the top level instances gets the effect. This seems straightforward to fix. We just need to walk through the class and check its variables recursively.

That would match the behavior from the non-Basemodel classes. I'll wrap that up tonight and add the tests so we check for these regressions.

[samuelcolvin]

sounds good, thanks.

[samuelcolvin]

@zrothberg I think this is the last thing we waiting for before releasing v1.9 🎉

If you have the logic and/or tests please could you either create a pull request against the forward-refs-json_encoders branch, or just provide a link so I can add to the pull request.