Opened 8 months ago

Last modified 3 weeks ago

#29998 assigned Bug

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

Reported by: Mārtiņš Šulcs Owned by: Can Sarıgöl
Component: Database layer (models, ORM) Version: 2.1
Severity: Normal Keywords:
Cc: Can Sarıgöl Triage Stage: Accepted
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 (6)

comment:1 Changed 8 months ago by Simon Charette

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.

comment:2 in reply to:  1 ; Changed 8 months ago by 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.

comment:3 in reply to:  2 Changed 8 months ago by Mārtiņš Šulcs

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 Changed 4 months ago 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.

comment:5 Changed 2 months ago by Can Sarıgöl

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.

PR

+++ 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()
Last edited 2 months ago by Can Sarıgöl (previous) (diff)

comment:6 Changed 3 weeks ago by Can Sarıgöl

Has patch: set
Owner: changed from nobody to Can Sarıgöl
Status: newassigned
Note: See TracTickets for help on using tickets.
Back to Top