Opened 8 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 , 8 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 8 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 , 8 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 , 7 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.
comment:5 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 7 years ago
Triage Stage: | Accepted → Ready for checkin |
---|---|
Version: | 1.10 → 1.11 |
Pull request against master: https://github.com/django/django/pull/9360
comment:7 by , 7 years ago
Has patch: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
"Ready for checkin" is set by a patch reviewer, not the patch author (see Triaging tickets).
comment:8 by , 7 years ago
Needs documentation: | set |
---|
comment:9 by , 7 years ago
Needs documentation: | unset |
---|
Removing documentation tag as pull request has been updated. Release notes added, targeted for 2.1.
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:
but I'm not positive about that.