[Django] #29998: pk setup for MTI to parent get confused by multiple OneToOne references.
Django
-------------------------------------+-------------------------------------
Reporter: shulcsm | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 2.1
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
{{{
class Document(models.Model):
pass
class Picking(Document):
document_ptr = models.OneToOneField(Document,
on_delete=models.CASCADE, parent_link=True, related_name='+')
origin = models.OneToOneField(Document, related_name='picking',
on_delete=models.PROTECT)
}}}
produces django.core.exceptions.ImproperlyConfigured: Add parent_link=True
to appname.Picking.origin.
{{{
class Picking(Document):
origin = models.OneToOneField(Document, related_name='picking',
on_delete=models.PROTECT)
document_ptr = models.OneToOneField(Document,
on_delete=models.CASCADE, parent_link=True, related_name='+')
}}}
Works
First issue is that order seems to matter?
Even if ordering is required "by design"(It shouldn't be we have explicit
parent_link marker) shouldn't it look from top to bottom like it does with
managers and other things?
--
Ticket URL: <https://code.djangoproject.com/ticket/29998>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
That seems to be a bug, managed to reproduce.
Does the error go away if you add `primary_key=True` to `document_ptr`
like I assume you wanted to do? This makes me realized that the
[https://docs.djangoproject.com/en/2.1/topics/db/models/#multi-table-
inheritance MTI documentation] is not completely correcy,
[https://github.com/django/django/blob/7d1123e5ada60963ba3c708a8932e57342278706/django/db/models/options.py#L225-L244
the automatically added `place_ptr` field end up with `primary_key=True`].
Not sure why we're not checking `and field.remote_field.parent_link` on
[https://github.com/django/django/blob/7d1123e5ada60963ba3c708a8932e57342278706/django/db/models/base.py#L183
parent links connection].
--
Ticket URL: <https://code.djangoproject.com/ticket/29998#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mārtiņš Šulcs):
Replying to [comment:1 Simon Charette]:
It does go away primary_key.
Why is parent_link even necessary in that case? Having pk OneToOne with to
MTI child should imply it's parent link.
--
Ticket URL: <https://code.djangoproject.com/ticket/29998#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mārtiņš Šulcs):
Replying to [comment:2 Mārtiņš Šulcs]:
> Replying to [comment:1 Simon Charette]:
>
> It does go away primary_key.
> Why is parent_link even necessary in that case? Having pk OneToOne with
to MTI child should imply it's parent link.
I have an update. The warning goes away with primary_key, but model itself
is still broken(complains about document_ptr_id not populated) unless
field order is correct.
--
Ticket URL: <https://code.djangoproject.com/ticket/29998#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ben Beecher):
Been able to replicate this bug with a simpler case:
{{{
class Document(models.Model):
pass
class Picking(Document):
some_unrelated_document = models.OneToOneField(Document,
related_name='something', on_delete=models.PROTECT)
}}}
Produces the same error against some_unrelated_document.
--
Ticket URL: <https://code.djangoproject.com/ticket/29998#comment:4>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Can Sarıgöl (added)
Comment:
Hi, Could this approach be appropriate? {{{parent_links}}}'s params can be
base and related class instance therefore just last sample columns are
added into parent_links.
We can extend key logic with related_name, e.g ('app', 'document',
'picking').
Or using this method, we can always ensure that the {{{parent_link=True}}}
field is guaranteed into {{{self.parents}}}.
{{{
+++ b/django/db/models/base.py
@@ -196,10 +196,11 @@ class ModelBase(type):
if base != new_class and not base._meta.abstract:
continue
# Locate OneToOneField instances.
- for field in base._meta.local_fields:
- if isinstance(field, OneToOneField):
- related = resolve_relation(new_class,
field.remote_field.model)
- parent_links[make_model_tuple(related)] = field
+ fields = [field for field in base._meta.local_fields if
isinstance(field, OneToOneField)]
+ for field in sorted(fields, key=lambda x:
x.remote_field.parent_link, reverse=True):
+ related_key =
make_model_tuple(resolve_relation(new_class, field.remote_field.model))
+ if related_key not in parent_links:
+ parent_links[related_key] = field
# Track fields inherited from base models.
inherited_attributes = set()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29998#comment:5>
Django
-------------------------------------+-------------------------------------
| Sarıgöl
Type: Bug | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Can Sarıgöl
* status: new => assigned
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/29998#comment:6>
Django
-------------------------------------+-------------------------------------
| Sarıgöl
Type: Bug | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by brianmaissy):
What's the status on this? I'm encountering the same issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/29998#comment:7>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: Can Sarıgöl => felixxm
* version: 2.1 => master
Comment:
[https://github.com/django/django/pull/12325 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29998#comment:8>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"d202846ced2f58d7a34ad80bfe2bde8a542a70b9" d202846c]:
{{{
#!CommitTicketReference repository=""
revision="d202846ced2f58d7a34ad80bfe2bde8a542a70b9"
Refs #29998 -- Corrected auto-created OneToOneField parent_link in MTI
docs.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29998#comment:9>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"8712027b226f1400ea31f9c0500fbfe359dd9d07" 8712027]:
{{{
#!CommitTicketReference repository=""
revision="8712027b226f1400ea31f9c0500fbfe359dd9d07"
[3.0.x] Refs #29998 -- Corrected auto-created OneToOneField parent_link in
MTI docs.
Backport of d202846ced2f58d7a34ad80bfe2bde8a542a70b9 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29998#comment:10>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29998#comment:11>
Django
-------------------------------------+-------------------------------------
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"bf77669453b9e9f64291da6701fe06fd5ba47e29" bf776694]:
{{{
#!CommitTicketReference repository=""
revision="bf77669453b9e9f64291da6701fe06fd5ba47e29"
Fixed #29998 -- Allowed multiple OneToOneFields to the parent model.
We assumed that any OneToOneField's in a child model must be the
parent link and raised an error when parent_link=True was not
specified. This patch allows to specify multiple OneToOneField's to
the parent model.
OneToOneField's without a custom related_name will raise fields.E304
and fields.E305 so this should warn users when they try to override
the auto-created OneToOneField.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29998#comment:12>