Opened 7 months ago

Closed 7 months ago

Last modified 5 months ago

#35356 closed Bug (fixed)

Issue with OneToOneField and recursive relationships in select_related() and only().

Reported by: Joshua van Besouw Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 4.2
Severity: Normal Keywords:
Cc: Joshua van Besouw 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

In Django a 4.2 project, if you have a model with a recursive relationship OneToOneField like this:

class Example(models.Model):
    name = models.CharField(max_length=32)
    source = models.OneToOneField(
        'self', related_name='destination', on_delete=models.CASCADE
    )

And then query this model using:

Example.objects.select_related(
    "source",
    "destination",
).only(
    "name",
    "source__name",
    "destination__name",
).all()

It throws the following error:

django.core.exceptions.FieldError: Field Example.source cannot be both deferred and traversed using select_related at the same time.

Expected behavior:

The queryset should apply the select_related() and only() without an exception occurring as the only() is specifying sub fields of the fields in the select_related(). Or at least this is how it used to behave.

Interestingly, if you change the queryset to the following, it works without any issues:

Example.objects.select_related("source").only("name", "source__name").all()

And vice versa also works:

Example.objects.select_related("destination").only("name", "destination__name").all()

Effected versions:

This error occurs in version 4.2+ of Django.

This worked as expected in all versions of Django 4.1.

As far as I can tell, this is a regression as a brief search doesn't indicate this functionality was explicitly changed at any point. I have not tested whether this is only an issue with recursive relationships or a general issues with reverse relationships.

Change History (10)

comment:1 by Simon Charette, 7 months ago

Likely related to #21204 (b3db6c8dcb5145f7d45eff517bcd96460475c879) which was merged in 4.2.

comment:2 by Simon Charette, 7 months ago

Triage Stage: UnreviewedAccepted

{{{!diff
diff --git a/tests/defer_regress/models.py b/tests/defer_regress/models.py
index dd492993b7..38ba4a622f 100644
--- a/tests/defer_regress/models.py
+++ b/tests/defer_regress/models.py
@@ -10,6 +10,12 @@ class Item(models.Model):

text = models.TextField(default="xyzzy")
value = models.IntegerField()
other_value = models.IntegerField(default=0)

+ source = models.OneToOneField(
+ "self",
+ related_name="destination",
+ on_delete=models.CASCADE,
+ null=True,
+ )

class RelatedItem(models.Model):

diff --git a/tests/defer_regress/tests.py b/tests/defer_regress/tests.py
index 10100e348d..d8b2186f0a 100644
--- a/tests/defer_regress/tests.py
+++ b/tests/defer_regress/tests.py
@@ -309,6 +309,21 @@ def test_only_reverse_many_to_many_ignored(self):

with self.assertNumQueries(1):

self.assertEqual(Item.objects.only("request").get(), item)

+ def test_self_referential(self):
+ first = Item.objects.create(name="first", value=1)
+ second = Item.objects.create(name="second", value=2, source=first)
+ with self.assertNumQueries(1):
+ deferred_first, deferred_second = (
+ Item.objects.select_related("source", "destination")
+ .only("name", "sourcename", "destinationvalue")
+ .order_by("pk")
+ )
+ with self.assertNumQueries(0):
+ self.assertEqual(deferred_first.name, first.name)
+ self.assertEqual(deferred_second.name, second.name)
+ self.assertEqual(deferred_second.source.name, first.name)
+ self.assertEqual(deferred_first.destination.name, second.name)
+

class DeferDeletionSignalsTests(TestCase):

senders = [Item, Proxy]

}}}

Version 0, edited 7 months ago by Simon Charette (next)

comment:3 by Simon Charette, 7 months ago

#34612 could have fixed it but it took a different approach.

comment:4 by Simon Charette, 7 months ago

Has patch: set

comment:5 by Joshua van Besouw, 7 months ago

Thanks for the quick patch on this!

I can confirm, form my end, that this patch fixes the issue I was encountering. The FieldError was not raised and the following SQL was used in the resulting query:

SELECT
    "test_project_example"."id",
    "test_project_example"."name",
    "test_project_example"."source_id",
    T2."id",
    T2."name",
    T3."id",
    T3."name"
FROM
    "test_project_example"
    LEFT OUTER JOIN "test_project_example" T2 ON (
        "test_project_example"."source_id" = T2."id"
    )
    LEFT OUTER JOIN "test_project_example" T3 ON (
        "test_project_example"."id" = T3."source_id"
    )
ORDER BY
    "test_project_example"."id" ASC

comment:6 by Mariusz Felisiak, 7 months ago

Owner: changed from nobody to Simon Charette
Status: newassigned
Triage Stage: AcceptedReady for checkin

comment:7 by nessita <124304+nessita@…>, 7 months ago

Resolution: fixed
Status: assignedclosed

In 83f54782:

Fixed #35356 -- Deferred self-referential foreign key fields adequately.

While refs #34612 surfaced issues with reverse one-to-one fields
deferrals, it missed that switching to storing remote fields would break
self-referential relationships.

This change switches to storing related objects in the select mask
instead of remote fields to prevent collisions when dealing with
self-referential relationships that might have a different directional
mask.

Despite fixing #21204 introduced a crash under some self-referential
deferral conditions, it was simply not working even before that as it
aggregated the sets of deferred fields by model.

Thanks Joshua van Besouw for the report and Mariusz Felisiak for the
review.

comment:8 by nessita <124304+nessita@…>, 7 months ago

In 195d885:

Refs #35356 -- Clarified select related with masked field logic.

By always including related objects in the select mask via adjusting the
defer logic (_get_defer_select_mask()), it becomes possible for
select_related_descend() to treat forward and reverse relationships
indistinctively.

This work also simplifies and adds comments to
select_related_descend() to make it easier to understand.

comment:9 by Joshua van Besouw, 5 months ago

Will this fix be released on the version 4.2 series? Or should I expect to only see it getting applied to version 5.0+ ?

comment:10 by Simon Charette, 5 months ago

It will not be backported to 4.2 as it's long term support at this point and only receive security and data loss fixing patches at this point.

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