[Django] #27629: Inconsistent check of allow_relation in ForwardManyToOneDescriptor.__set__
Django
-------------------------------------+-------------------------------------
Reporter: Sven | Owner: nobody
Coenye |
Type: Bug | Status: new
Component: Database | Version: 1.10
layer (models, ORM) | Keywords: allow_relation
Severity: Normal | ForwardManyToOneDescriptor
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/27629>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: allow_relation | Triage Stage: Accepted
ForwardManyToOneDescriptor |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
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
[https://docs.djangoproject.com/en/stable/topics/db/multi-db/#cross-
database-relations 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27629#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: allow_relation | Triage Stage: Accepted
ForwardManyToOneDescriptor |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27629#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: allow_relation | Triage Stage: Accepted
ForwardManyToOneDescriptor |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/27629#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: allow_relation | Triage Stage: Accepted
ForwardManyToOneDescriptor |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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 it's starting to require significant
modifications.
--
Ticket URL: <https://code.djangoproject.com/ticket/27629#comment:4>
Django
-------------------------------------+-------------------------------------
| Filipek
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: allow_relation | Triage Stage: Accepted
ForwardManyToOneDescriptor |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Stefan R. Filipek
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/27629#comment:5>
Django
-------------------------------------+-------------------------------------
| Filipek
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
ForwardManyToOneDescriptor | checkin
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 1.10 => 1.11
* stage: Accepted => Ready for checkin
Comment:
Pull request against master: https://github.com/django/django/pull/9360
--
Ticket URL: <https://code.djangoproject.com/ticket/27629#comment:6>
Django
-------------------------------------+-------------------------------------
| Filipek
Type: Bug | Status: assigned
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
ForwardManyToOneDescriptor |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
"Ready for checkin" is set by a patch reviewer, not the patch author (see
[https://docs.djangoproject.com/en/dev/internals/contributing/triaging-
tickets/ Triaging tickets]).
--
Ticket URL: <https://code.djangoproject.com/ticket/27629#comment:7>
Django
-------------------------------------+-------------------------------------
| Filipek
Type: Bug | Status: assigned
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: allow_relation | Triage Stage: Accepted
ForwardManyToOneDescriptor |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27629#comment:8>
Django
-------------------------------------+-------------------------------------
| Filipek
Type: Bug | Status: assigned
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: allow_relation | Triage Stage: Accepted
ForwardManyToOneDescriptor |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_docs: 1 => 0
Comment:
Removing documentation tag as pull request has been updated. Release notes
added, targeted for 2.1.
--
Ticket URL: <https://code.djangoproject.com/ticket/27629#comment:9>
Django
-------------------------------------+-------------------------------------
| Filipek
Component: Database layer | Version: 1.11
(models, ORM) |
Keywords: allow_relation | Triage Stage: Accepted
ForwardManyToOneDescriptor |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"a5a2ceeb453b86caaf01687342984f372428beb8" a5a2cee]:
{{{
#!CommitTicketReference repository=""
revision="a5a2ceeb453b86caaf01687342984f372428beb8"
Fixed #27629 -- Added router.allow_relation() calls for assignments
between unsaved model instances.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27629#comment:10>