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
V1.9 Name Collisions JSON model_as_dict #3583
Comments
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. |
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: 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()})' |
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 As you've said, the |
@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. |
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 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. |
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. |
Yep so that clears up the top level bug the other one remaining from 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. |
sounds good, thanks. |
@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 |
* apply update_forward_refs to json_encoders, fix #3583 * linting * mypy * avoid use of ForwardRef with python3.6 * fix ForwardRef usage, take 2 * coverage
* apply update_forward_refs to json_encoders, fix pydantic#3583 * linting * mypy * avoid use of ForwardRef with python3.6 * fix ForwardRef usage, take 2 * coverage
Checks
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())"
:The text was updated successfully, but these errors were encountered: