Opened 6 years ago

Last modified 7 months ago

#23051 assigned Bug

QuerySet.only() fail to work with reverse o2o relationships

Reported by: Vladimir Dmitriev Owned by: Paulo
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: orm only reverse relationship OneToOneField
Cc: Dan Davis Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Specifying a field from reverse relationship model in the .only() queryset method have no effect on compiled query:

# sample models
class Person(models.Model):
    name = models.CharField(max_length=64)


class PersonExtra(models.Model):
    bio = models.TextField()
    information = models.TextField()
    person = models.OneToOneField(Person)

# manage.py shell
>>> from testapp.models import Person
>>> print Person.objects.all().only('name').query
SELECT "testapp_person"."id", "testapp_person"."name" FROM "testapp_person"
>>> print Person.objects.all().select_related('personextra').only('name', 'personextra__bio').query # expected table join and personextra__bio to be loaded
SELECT "testapp_person"."id", "testapp_person"."name" FROM "testapp_person"

defer() method works fine:

>>> print Person.objects.all().select_related('personextra').defer('personextra__information').query
SELECT "testapp_person"."id", "testapp_person"."name", "testapp_personextra"."id", "testapp_personextra"."bio", "testapp_personextra"."person_id" FROM "testapp_person" LEFT OUTER JOIN "testapp_personextra" ON ( "testapp_person"."id" = "testapp_personextra"."person_id" )

Change History (9)

comment:1 Changed 6 years ago by Baptiste Mispelon

Triage Stage: UnreviewedAccepted

Hi,

I can indeed reproduce this.

Currently, Person.objects.only('name', 'personextra__bio') is accepted by returns the wrong query. If supporting that use case is not possible, it should at least raise an error instead of silently doing the wrong thing.

Thanks.

comment:2 Changed 6 years ago by Niclas Olofsson

Owner: changed from nobody to Niclas Olofsson
Status: newassigned

Will try to at least make a test for this during EP14.

comment:3 Changed 6 years ago by Niclas Olofsson

Failing test:

  • tests/defer_regress/tests.py

       diff --git a/tests/defer_regress/tests.py b/tests/defer_regress/tests.py
    index 63f9e97..5d70da2 100644
    a b class DeferRegressionTest(TestCase): 
    144144            list(SimpleItem.objects.annotate(Count('feature')).only('name')),
    145145            list)
    146146
     147    def test_ticket_23051(self):
     148        item = Item.objects.create(value=1, name='item')
     149        OneToOneItem.objects.create(item=item, name='one_to_one_item')
     150
     151        obj = Item.objects.only('name', 'one_to_one_item__name').get(name='item')
     152
     153        with self.assertNumQueries(0):
     154            self.assertEqual(obj.name, 'item')
     155
     156        with self.assertNumQueries(0):
     157            self.assertEqual(obj.one_to_one_item.name, 'one_to_one_item')
     158
    147159    def test_only_and_defer_usage_on_proxy_models(self):
    148160        # Regression for #15790 - only() broken for proxy models
    149161        proxy = Proxy.objects.create(name="proxy", value=42)
Last edited 5 years ago by Tim Graham (previous) (diff)

comment:4 Changed 6 years ago by Niclas Olofsson

Owner: Niclas Olofsson deleted
Status: assignednew
Version: 1.6master

I looked in to this, and it seems that at least SQLCompiler.deferred_to_columns (and all related methods called before then) works as intended in this case. However, I will not be able to solve this, even if I believe that it should not be unsolvable.

comment:5 Changed 18 months ago by Dan Davis

Cc: Dan Davis added

comment:6 Changed 10 months ago by Paulo

Owner: set to Paulo
Status: newassigned

comment:7 Changed 10 months ago by Paulo

Has patch: set

The original reported issue was fixed already, calling:

Person.objects.all().select_related('personextra').only('name', 'personextra__bio')

Gives the expected result (on 2.3 master branch).
But as pointed out in the comments above, calling:

# removed select_related()
Person.objects.all().only('name', 'personextra__bio')

Will silently fail to prefetch the personextra relationship.

I've opened a pr https://github.com/django/django/pull/11981 that raises a ValueError when passing a reverse field to only(),
without prefetching the relationship first (as in the first example).

comment:8 Changed 8 months ago by Baptiste Mispelon

As noted in comment:7 above, the usecase is now supported when used in conjunction with select_related.

I did some bisecting to track down the commit involved and found out that two commits were actually involved:

1) With 3daa9d60be6672841ceed5bb812b6fb25950dc16, Django started raising an error: FieldError: Invalid field name(s) given in select_related: 'personextra'. Choices are: personextra
2) With 0456a8b9e66608e2e7b7e64035d477f44c259c94, the error message disappeared and the feature started working

For reference, here's the testcase that I used (using the models from the original ticket):

class TicketTestCase(TestCase):
    def test_ticket(self):
        p = Person.objects.create(name="Baptiste")
        PersonExtra.objects.create(person=p, bio="yes")

        qs = list(Person.objects.select_related('personextra').only('name', 'personextra__bio'))

        with self.assertNumQueries(0):
            self.assertEqual(len(qs), 1)
            self.assertEqual(qs[0].name, "Baptiste")
            self.assertEqual(qs[0].personextra.bio, "yes")

I haven't looked in details at the proposed pull request, but I do have some preliminary questions:

1) Is it fundamentally impossible to support using a reverse o2o field in calls to only()? The PR adds an error message but I think it would be better if we could fix the issue instead.
2) It seems that the problem exists for the case of a normal forward o2o (PersonExtra.objects.only('person__name') in our example) and also for reverse M2M (#30124). Does the PR address those too?

Thanks

comment:9 Changed 7 months ago by felixxm

Patch needs improvement: set

Paulo, please check Baptiste comments and questions.

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