Opened 10 years ago

Last modified 17 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: dev
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 (10)

comment:1 by Baptiste Mispelon, 10 years ago

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 by Niclas Olofsson, 10 years ago

Owner: changed from nobody to Niclas Olofsson
Status: newassigned

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

comment:3 by Niclas Olofsson, 10 years ago

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 9 years ago by Tim Graham (previous) (diff)

comment:4 by Niclas Olofsson, 10 years ago

Owner: Niclas Olofsson removed
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 by Dan Davis, 6 years ago

Cc: Dan Davis added

comment:6 by Paulo, 5 years ago

Owner: set to Paulo
Status: newassigned

comment:7 by Paulo, 5 years ago

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 by Baptiste Mispelon, 5 years ago

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 by Mariusz Felisiak, 5 years ago

Patch needs improvement: set

Paulo, please check Baptiste comments and questions.

comment:10 by Simon Charette, 17 months ago

FWIW #34612 was a related regression of 4.2 refactored deferred field refactor. We still silence usage of only when it refers to related models that are not selected so this ticket is still relevant though.

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