Fixed #35974 -- Added router.allow_relation() check in RelatedManager.add() and GenericRelatedObjectManager.add().#20897
Conversation
There was a problem hiding this comment.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
7e67c95 to
6cccb8e
Compare
|
Hi @AbhigyaShridhar! Can you edit the title of this PR to follow the format of other PRs? Please also run pre-commit and update your commit with those changes. |
f8d1278 to
9599f34
Compare
|
Hi @tim-schilling! Thank you for pointing me to the right process. I've updated my PR. |
9599f34 to
d4140f0
Compare
|
Update: Added tests for the case where |
|
@Credok12 I noticed you've left similar brief reviews on a number of other repos recently. Thanks for taking the time to look at these! Unfortunately, since you're new to the project, a review that says "ok" doesn't provide us with enough context to be actionable. We're not sure what you specifically reviewed, what you consider to be okay, or where your expertise lies. We absolutely welcome you as a reviewer, but to make the most of your contributions, we need more detail about what you've reviewed and any concerns or observations. If you'd like to know what we value in a code review, please review our docs. |
….add() and GenericRelatedObjectManager.add(). RelatedManager.add() and GenericRelatedObjectManager.add() previously combined obj._state.adding (unsaved-object check) with a _state.db equality check on both objects, raising a misleading "instance isn't saved" error for objects loaded from a different database configuration. The _state.adding check remains for unsaved objects, while router.allow_relation() is now used to validate cross-database relations in GenericRelatedObjectManager. In RelatedManager, router.allow_relation check is already handeled by __set__ method of the field descriptor.
d4140f0 to
a775930
Compare
Uncovered lines in changed filesNote: Missing lines are warnings only. Some database-specific lines may not be measured. More information |
|
Update: The previous missing coverage concerned me since the change is entirely composed of adding and removing conditions. On tracing back, I found it was actually unreachable code. @tim-schilling @jacobtylerwalls Is there anything else needed on this PR? |
|
Thanks for updates 👍 There are ~100 PRs waiting for a review, so we can't promise any specific timelines. We ask for your patience. |
Trac ticket number
ticket-35974
Branch description
Added router.allow_relation check in RelatedManager.add and GenericRelatedObjectManager.add
RelatedManager.add() and GenericRelatedObjectManager.add() previously combined
obj._state.adding (unsaved-object check) with a _state.db equality check on
both objects, raising a misleading "instance isn't saved" error for objects
loaded from a different database configuration. The _state.adding check
remains for unsaved objects, while router.allow_relation() is now used to
validate cross-database relations in GenericRelatedObjectManager. In RelatedManager,
router.allow_relation check is already handeled by set method of the
field descriptor.
Builds on the work in #18896 by @lszrn. The key difference from that PR is that it separates the obj._state.adding check and the router.allow_relation check instead of removing the previous check entirely.
AI Assistance Disclosure (REQUIRED)
AI tools used: Claude code was used to ensure that the tests cover all possible edge cases for the changes. All the code has been written manually.
Checklist
mainbranch.