Inconsistent check of allow_relation in ForwardManyToOneDescriptor.__set__

[Sven Coenye]

The method in django/db/models/fields/related_descriptors.py currently contains the following code:

if instance._state.db is None:
    instance._state.db = router.db_for_write(instance.__class__, instance=value)
elif value._state.db is None:
    value._state.db = router.db_for_write(value.__class__, instance=instance)
elif value._state.db is not None and instance._state.db is not None:
    if not router.allow_relation(value, instance):
        raise ValueError('Cannot assign "%r": the current database router prevents this relation.' % value)

When using an application containing the following model

from otherapp.models import AdProgram

class Order(models.Model):
    first_name   = models.CharField(max_length = 15)
    last_name    = models.CharField(max_length = 20)
    program      = models.ForeignKey(AdProgram, db_constraint=False)    # No cross database FK constraints under MSSQL

where the imported model belongs to a legacy database different from the application's own database, the code in question does not flag an inappropriate relationship. E.g. when submitting a ModelForm containing these three fields, the value for program will be saved to the application's database even if allow_relation would return False.

When the model definition is changed to

from otherapp.models import AdProgram, AdTerm

class Order(models.Model):
    first_name   = models.CharField(max_length = 15)
    last_name    = models.CharField(max_length = 20)
    program      = models.ForeignKey(AdProgram, db_constraint=False)    # No cross database FK constraints under MSSQL
    term         = models.ForeignKey(AdTerm, db_constraint=False)

"program" again gets a free pass, but "term" triggers the ValueError exception.

The reason is that instance._state.db is None for the "program" field but contains the value retrieved for "program" for any subsequent fields.

I believe those "elif" statements should be plain "if" statements so the uninitialized value on the first pass does not fall through?

[Tim Graham]

That code hasn't changed since multiple database support was introduced in Django 1.2 (1b3dc8ad9a28486542f766ff93318aa6b4f5999b) but you may be correct -- at least no tests are failing with the proposed change so that's a good sign.

I was going to make the claim that that code might be written based on ​this assumption in the documentation:

However, if you’re using SQLite or MySQL with MyISAM tables, there is no enforced referential integrity; as a result, you may be able to ‘fake’ cross database foreign keys. However, this configuration is not officially supported by Django.

but I'm not positive about that.

[Sven Coenye]

Perhaps a slightly overzealous optimization?

To test further, I changed the two ForeignKey fields to point to models in a second application residing in the same database, and simplified allow_relation in the database router to:

    def allow_relation(self, obj1, obj2, **hints):
        return False

The outcome remained the same. No errors are raised if only a single field links to an external model. If more fields are present, the first field gets a pass, the second field trips the ValueError exception.

[Ian R-P]

If all DATABASE_ROUTERS return None for db_for_write it takes the hints['instance']._state.db value (if one exists) and returns that for the parent model's own _state.db value.

...\django\db\utils.py:257 def_router_func(action):

267:                    chosen_db = method(model, **hints)
268:                    if chosen_db:
269:                        return chosen_db
270:            instance = hints.get('instance')
271:            if instance is not None and instance._state.db:
272:                return instance._state.db
273:            return DEFAULT_DB_ALIAS
274:        return _route_db

I was able to solve this by changing my database router from return None to 'default' (for the time being, as documentation says it should return None).

[Stefan R. Filipek]

I'm running into this issue as well on 1.11.7 and the latest master branch. There's a large amount of inconsistency with this behavior that I've run into.

General constructors, create(), get_or_create() and update_or_create() don't call allow_relation(). Assignment of a foreign key will perform the check, only once the object has been saved.

I've been working of a fix, but I need to make more test cases for it.

[Stefan R. Filipek]

Pull request against master: ​https://github.com/django/django/pull/9360

[Tim Graham]

"Ready for checkin" is set by a patch reviewer, not the patch author (see ​Triaging tickets).

[Stefan R. Filipek]

Removing documentation tag as pull request has been updated. Release notes added, targeted for 2.1.

[Tim Graham <timograham@…>]

In a5a2cee:

Fixed #27629 -- Added router.allow_relation() calls for assignments between unsaved model instances.