Skip to content

Fixed #35974 -- Added router.allow_relation() check in RelatedManager.add() and GenericRelatedObjectManager.add().#20897

Open
AbhigyaShridhar wants to merge 1 commit intodjango:mainfrom
AbhigyaShridhar:ticket_35974
Open

Fixed #35974 -- Added router.allow_relation() check in RelatedManager.add() and GenericRelatedObjectManager.add().#20897
AbhigyaShridhar wants to merge 1 commit intodjango:mainfrom
AbhigyaShridhar:ticket_35974

Conversation

@AbhigyaShridhar
Copy link
Copy Markdown

@AbhigyaShridhar AbhigyaShridhar commented Mar 12, 2026

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)

  • No AI tools were used in preparing this PR.
  • If AI tools were used, I have disclosed which ones, and fully reviewed and verified their output.
    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

  • This PR follows the contribution guidelines.
  • This PR does not disclose a security vulnerability (see vulnerability reporting).
  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period (see guidelines).
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

@github-actions github-actions bot added the no ticket Based on PR title, no linked Trac ticket label Mar 12, 2026
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ⛵️!

@tim-schilling
Copy link
Copy Markdown
Member

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.

@AbhigyaShridhar AbhigyaShridhar force-pushed the ticket_35974 branch 2 times, most recently from f8d1278 to 9599f34 Compare March 12, 2026 20:28
@AbhigyaShridhar AbhigyaShridhar changed the title Ticket 35974 Fixed #35974 -- Added router.allow_relation() check in RelatedManager.add() and GenericRelatedObjectManager.add(). Mar 12, 2026
@github-actions github-actions bot removed the no ticket Based on PR title, no linked Trac ticket label Mar 12, 2026
@AbhigyaShridhar
Copy link
Copy Markdown
Author

Hi @tim-schilling! Thank you for pointing me to the right process. I've updated my PR.

Copy link
Copy Markdown

@Credok12 Credok12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@AbhigyaShridhar
Copy link
Copy Markdown
Author

Update: Added tests for the case where allow_relation() returns None, verifying that Django's default documented behaviour is still valid.

@tim-schilling
Copy link
Copy Markdown
Member

@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.

@jacobtylerwalls jacobtylerwalls added the coverage Generate a comment with a coverage diff label Mar 16, 2026
….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.
@github-actions
Copy link
Copy Markdown

Uncovered lines in changed files

-------------
django/contrib/contenttypes/fields.py (100%)
django/db/models/fields/related_descriptors.py (100%)
-------------
Total:   4 lines
Missing: 0 lines
Coverage: 100%
-------------

Note: Missing lines are warnings only. Some database-specific lines may not be measured. More information

@AbhigyaShridhar
Copy link
Copy Markdown
Author

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. RelatedManager.add() calls check_and_update_obj(), which calls setattr() on the FK field. The FK descriptor's __set__ already calls router.allow_relation(), so a denied relation is caught there before reaching the if statement I had added. For RelatedManager.add(), all that was needed was to remove the redundant _state.db equality check that raised the misleading exception. The GenericRelatedObjectManager.add() change remains as-is since generic relations don't go through the FK descriptor. The test cases remain unchanged.

@tim-schilling @jacobtylerwalls Is there anything else needed on this PR?

@jacobtylerwalls
Copy link
Copy Markdown
Member

Thanks for updates 👍

There are ~100 PRs waiting for a review, so we can't promise any specific timelines. We ask for your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coverage Generate a comment with a coverage diff

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants