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

Fix add_field_serializer_for_reverse_relations clearing validators #1302

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

camillol
Copy link

When a model has a reverse relation, Ormar synthesizes a field for it. In order to get Pydantic to see the new field, it calls DecoratorInfos.build (in add_field_serializer_for_reverse_relations) to pick up the new serializer, and then model_rebuild. Unfortunately, DecoratorInfos.build is not idempotent. When it processes a model class, it collects the information in __pydantic_decorators__, and it strips out the decorators from the class members, to save time in case it has to process the class again (as the base class of a different class). If it is called again, it starts collecting the decorators from scratch, ignoring the existing __pydantic_decorators__. Since the class members have already had their decorators stripped, they are not collected again. The net result is that any pydantic decorators that had already been processed are lost.

This has the result of ignoring @field_validator etc. if a model has a reverse relation that is processed after the model.

This PR fixes the issue by storing the preexisting __pydantic_decorators__ and merging the new ones into it. This fix should keep working even if Pydantic changes DecoratorInfos.build to be idempotent.

@collerek
Copy link
Owner

sorry for the late answer, can you add a test that would fail before and this change fixes?

Copy link

codspeed-hq bot commented May 26, 2024

CodSpeed Performance Report

Merging #1302 will degrade performances by 12.51%

Comparing camillol:fixdescriptors (56a9d00) with master (7f3149d)

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 82 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark master camillol:fixdescriptors Change
test_min[250] 3.6 ms 2.7 ms +31.57%
test_deleting_individually[10] 8.6 ms 9.8 ms -12.51%

@camillol
Copy link
Author

sorry for the late answer, can you add a test that would fail before and this change fixes?

Done!

@collerek
Copy link
Owner

Hi, you didn't allow me to push to your fork, so can you please apply b1d803a on your pr (fixes test with default value)

@camillol
Copy link
Author

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants