Re: [Django] #35974: Use router.allow_relation in RelatedManager.add and GenericRelatedObjectManager.add

[Django]

#35974: Use router.allow_relation in RelatedManager.add and
GenericRelatedObjectManager.add
-------------------------------------+-------------------------------------
Reporter: Laurent Szyster | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Laurent Szyster):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35974#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35974: Use router.allow_relation in RelatedManager.add and
GenericRelatedObjectManager.add
-------------------------------------+-------------------------------------
Reporter: Laurent Szyster | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Laurent Szyster):

* Attachment "ticket_35974.patch" added.

--
Ticket URL: <https://code.djangoproject.com/ticket/35974>

[Django]

#35974: Use router.allow_relation in RelatedManager.add and
GenericRelatedObjectManager.add
-------------------------------------+-------------------------------------
Reporter: Laurent Szyster | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Laurent Szyster):

The fix is simpler than it seemed at first.

When asserting that an object to be added by the RelatedManager or
GenericRelatedObjectManager is saved, testing the following expression is
enough:

{{{
obj._state.adding
}}}

In the attached patch I added a `ReadWriteClusterTests` regression test
case with eight tests, namely:
- test_reverse_many_to_one_add
- test_reverse_many_to_one_set
- test_reverse_many_to_one_add_unsaved
- test_reverse_many_to_one_set_unsaved
- test_generic_related_add
- test_generic_related_set
- test_generic_related_add_unsaved
- test_generic_related_set_unsaved

The attached patch also modifies the
`RouterTestCase.test_invalid_set_foreign_key_assignment` method to test
the assignment of an unsaved object using the related manager `set`
method, renaming it `test_invalid_set_unsaved_assignment`.

The previous version of that test was introduced when addressing #18556
but contradicted the `TestRouter.allow_relation`:

{{{
def allow_relation(self, obj1, obj2, **hints):
return obj1._state.db in ("default", "other") and obj2._state.db
in (
"default",
"other",
)
}}}

To cite the discussion at the time:
https://code.djangoproject.com/ticket/18556#comment:8 by Anssi Kääriäinen:

> Just checking the pk is set isn't enough, consulting the model._state.db
+ _state.adding would likely yield the correct result.

And:

> This seems to also raise a possible race condition. Assume thread T1
fetches the object from DB, then T2 deletes it, and then T1 issues add().
The result was that the object was resaved to DB, after patch it is that
it remains deleted. In the add() case the correct behavior is resave so
that after add you can trust that relation actually contains all the
objects in the DB.

In practice, in a setup with multiple database connections to the same
cluster, foreign key integrity constraint will prevent a relation to a
non-existing entity (ie: an object fetched from the reader node but that
no longer exists in the writer node).
--
Ticket URL: <https://code.djangoproject.com/ticket/35974#comment:4>

[Django]

#35974: Use router.allow_relation in RelatedManager.add and
GenericRelatedObjectManager.add
-------------------------------------+-------------------------------------
Reporter: Laurent Szyster | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Laurent Szyster):

Also, note that in the multiple database
`QueryTestCase.test_generic_key_cross_database_protection`, the same error
condition - a forbidden relation between distinct databases - now raises
the same exception with the same message about a prevented relation:

{{{
# Set a foreign key with an object from a different database
msg = (
'Cannot assign "<ContentType: Multiple_Database | book>": the
'
"current database router prevents this relation."
)
with self.assertRaisesMessage(ValueError, msg):
review1.content_object = dive

# Add to a foreign key set with an object from a different
database
with self.assertRaisesMessage(ValueError, msg):
with transaction.atomic(using="other"):
dive.reviews.add(review1)
}}}

Previously we would have an erroneous messages about an unsaved instance
instead:

{{{
- msg = (
- "<Book: Dive into Python> instance isn't saved. Use
bulk=False or save the "
- "object first."
- )
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35974#comment:5>

[Django]

#35974: Use router.allow_relation in RelatedManager.add and
GenericRelatedObjectManager.add
-------------------------------------+-------------------------------------

Reporter: Laurent Szyster | Owner: Laurent
| Szyster
Type: Bug | Status: assigned

Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* owner: (none) => Laurent Szyster
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/35974#comment:6>