Opened 7 years ago

Closed 6 years ago

#27629 closed Bug (fixed)

Inconsistent check of allow_relation in ForwardManyToOneDescriptor.__set__

Reported by: Sven Coenye Owned by: Stefan R. Filipek
Component: Database layer (models, ORM) Version: 1.11
Severity: Normal Keywords: allow_relation ForwardManyToOneDescriptor
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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?

Change History (10)

comment:1 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted

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.

comment:2 by Sven Coenye, 7 years ago

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.

comment:3 by Ian R-P, 7 years ago

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

comment:4 by Stefan R. Filipek, 6 years ago

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.

Last edited 6 years ago by Stefan R. Filipek (previous) (diff)

comment:5 by Stefan R. Filipek, 6 years ago

Owner: changed from nobody to Stefan R. Filipek
Status: newassigned

comment:6 by Stefan R. Filipek, 6 years ago

Triage Stage: AcceptedReady for checkin
Version: 1.101.11

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

comment:7 by Tim Graham, 6 years ago

Has patch: set
Triage Stage: Ready for checkinAccepted

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

comment:8 by Asif Saifuddin Auvi, 6 years ago

Needs documentation: set

comment:9 by Stefan R. Filipek, 6 years ago

Needs documentation: unset

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

comment:10 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: assignedclosed

In a5a2cee:

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

Note: See TracTickets for help on using tickets.
Back to Top