Use router.allow_relation in RelatedManager.add and GenericRelatedObjectManager.add

[Laurent Szyster]

In the create_reverse_many_to_one_manager function, the RelatedManager.add method will raise an ValueError if one of the related objects to add to the many-to-many relation set has not been loaded from the same database connection as the instance to which it is related.

            if bulk:
                pks = []
                for obj in objs:
                    check_and_update_obj(obj)
                    if obj._state.adding or obj._state.db != db:
                        raise ValueError(
                            "%r instance isn't saved. Use bulk=False or save "
                            "the object first." % obj
                        )
                    pks.append(obj.pk)
                self.model._base_manager.using(db).filter(pk__in=pks).update(
                    **{
                        self.field.name: self.instance,
                    }
                )

See: ​https://github.com/django/django/blob/39cf3c63f3228a04f101f3e62c75a6aae7c6ef0f/django/db/models/fields/related_descriptors.py#L767-L776

This implies that an object loaded from a read-only connection to a database cannot be related to an instance created using a distinct write-enabled connection to the same database.

For instance, assume the following Database Router:

class DatabaseRouter:
    def db_for_read(self, model, **hints):
        return "read-only"

    def db_for_write(self, model, **hints):
        return "default"

    def allow_relation(self, value, instance, **hints):
        if instance._state.db == 'default' and value._state.db in {'default', 'read-only'}:
            return True
        
        return None

    def allow_migrate(self, db, app_label, model_name=None, **hints):
        return True

This router will dispatch read queries to a "read-only" database connection and write queries to the "default" database connection. Such setup is intended to limit the load on the writer node of a database cluster.

But for such setup to work the router's allow_relation method should be used instead of comparing the database connection used to load the related value with the database to write the instance to which we want to add a relation.

Line 771 of related_descriptor.py:

                    if obj._state.adding or obj._state.db != db:

Should be replaced with:

                    if obj._state.adding or not (router.allow_relation(obj, self.instance) or obj._state.db == db):

See: ​https://docs.djangoproject.com/en/4.2/topics/db/multi-db/#allow_relation

[Laurent Szyster]

Note that the database router's allow_relation method is used elsewhere to test if two objects can be related:

• ​https://github.com/django/django/blob/39cf3c63f3228a04f101f3e62c75a6aae7c6ef0f/django/db/models/fields/related_descriptors.py#L284
• ​https://github.com/django/django/blob/39cf3c63f3228a04f101f3e62c75a6aae7c6ef0f/django/db/models/fields/related_descriptors.py#L547
• ​https://github.com/django/django/blob/39cf3c63f3228a04f101f3e62c75a6aae7c6ef0f/django/db/models/fields/related_descriptors.py#L1312

[Sarah Boyce]

Thank you for the report Laurent
Been looking into it and this is also the case for GenericRelatedObjectManager and I think the logic was added in #18556 (see also this comment).
This is not something I am very familiar with but what you're saying sounds reasonable and I would be happy to see a patch with tests - tentatively accepting

[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).

[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."
-        )