Opened 5 years ago

Closed 4 years ago

#29998 closed Bug (fixed)

pk setup for MTI to parent get confused by multiple OneToOne references.

Reported by: Mārtiņš Šulcs Owned by: Mariusz Felisiak
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Can Sarıgöl Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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?

Change History (12)

comment:1 by Simon Charette, 5 years ago

Triage Stage: UnreviewedAccepted

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 MTI documentation is not completely correcy, 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 parent links connection.

in reply to:  1 ; comment:2 by Mārtiņš Šulcs, 5 years ago

Replying to 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.

in reply to:  2 comment:3 by Mārtiņš Šulcs, 5 years ago

Replying to Mārtiņš Šulcs:

Replying to 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.

comment:4 by Ben Beecher, 5 years ago

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.

comment:5 by Can Sarıgöl, 5 years ago

Cc: Can Sarıgöl added

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()
Version 0, edited 5 years ago by Can Sarıgöl (next)

comment:6 by Can Sarıgöl, 5 years ago

Has patch: set
Owner: changed from nobody to Can Sarıgöl
Status: newassigned

comment:7 by Brian Maissy, 5 years ago

What's the status on this? I'm encountering the same issue.

comment:8 by Mariusz Felisiak, 4 years ago

Owner: changed from Can Sarıgöl to Mariusz Felisiak
Version: 2.1master

comment:9 by GitHub <noreply@…>, 4 years ago

In d202846c:

Refs #29998 -- Corrected auto-created OneToOneField parent_link in MTI docs.

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 8712027:

[3.0.x] Refs #29998 -- Corrected auto-created OneToOneField parent_link in MTI docs.

Backport of d202846ced2f58d7a34ad80bfe2bde8a542a70b9 from master

comment:11 by Carlton Gibson, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:12 by GitHub <noreply@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In bf776694:

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.

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