Code

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#12851 closed (fixed)

Queryset "defer" won't work properly with select_related

Reported by: ruosteinen Owned by: ruosteinen
Component: Database layer (models, ORM) Version: 1.2-beta
Severity: Keywords: select_related, defer, only, nesting, query
Cc: carljm, ruosteinen, phxx Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Select_related and defer don't seem to work properly together. Consider following models in APP buggy.

from django.db import models

class ProblemModel(models.Model):
    name = models.CharField(max_length=100)
    target = models.ForeignKey('Target', null=True)

class Target(models.Model):
    location = models.CharField(max_length=100)

Even though Queryset API document the use of underscore in deferring, the following ends up in assertion error.

from buggy.models import Target, ProblemModel
Target.objects.all().delete(); ProblemModel.objects.all().delete()

tgt = Target.objects.create(location='Earth')
pm = ProblemModel.objects.create(name='bug', target=tgt)

pm = ProblemModel.objects.select_related('target').defer('target__location').get(name='bug')

assert pm.target.location == 'bug'


At least according to Queryset API docs, defer() is supposed to allow access to deferred fields even if they're not loaded initially.

In addition pm.target is not of type Target_Deferred_location_deferred as it should be(?). There seem to be some other weirdness as well with select_related and defer but they'll probably be fixed once nesting works properly.

This in Django 1.2 beta 1 SVN-12416

I'll start working on a patch right away.

-Tommi Penttinen-

Attachments (2)

12851_inheritance_regression_test.patch (1.0 KB) - added by phxx 4 years ago.
The current fix for this bug does not work if you use model inheritance. Here is a failing testcase.
12851_inheritance_regression_test_2.patch (550 bytes) - added by phxx 4 years ago.
Here is another testcase using "only" instead of "defer" which fails with the current trunk.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 4 years ago by ruosteinen

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Seems to be related to #10733.

comment:2 Changed 4 years ago by carljm

  • Cc carljm added

comment:3 Changed 4 years ago by carljm

I haven't played with this yet, but your repro code doesn't look right. I would expect an assertion error with that code: pm.target.location should be 'Earth' not 'bug'.

comment:4 Changed 4 years ago by russellm

  • milestone set to 1.2
  • Triage Stage changed from Unreviewed to Accepted

@carljm - Agreed: "Earth" should be the expected result. Either way, it's not working - pm.target.location is returning an empty string.

@ruosteinen - Although #10733 also deals with select_related() + defer()/only() problems, I'm not immediately convinced that it is the same problem (at least, not enough to close this as a duplicate).#10733 deals with having multiple joins to the same table; this ticket is about traversing a join correctly. I suspect the problem here is something to do with populating the related object cache correctly - the query is doing the join as it should, but because the query defers the only data field on the related object, and it is being deferred, the deferred class isn't being instantiated correctly.

comment:5 Changed 4 years ago by ruosteinen

@carljm, sorry. Changed the code a bit on the fly. But as russellm said, it still doesn't work.

@russellm True. Although in the proposed patch to #10733 only_load=only_load get passed properly to django.db.models.query.get_cached_row while recursing and after that relationships with deferred fields work properly i.e. type(pm.target) == <class 'buggy.models.Target_Deferred_location'>.

comment:6 Changed 4 years ago by ruosteinen

  • Cc ruosteinen added
  • Has patch set
  • Owner changed from nobody to ruosteinen
  • Status changed from new to assigned

I'll mark this as "has patch" since one is available at #10733.

comment:7 Changed 4 years ago by russellm

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [12817]) Fixed #12851 -- Corrected the interaction of defer() with select_related(). Thanks to ruosteinen for the report.

comment:8 Changed 4 years ago by russellm

(In [12818]) [1.1.X] Fixed #12851 -- Corrected the interaction of defer() with select_related(). Thanks to ruosteinen for the report.

Backport of r12817 from trunk.

Changed 4 years ago by phxx

The current fix for this bug does not work if you use model inheritance. Here is a failing testcase.

comment:9 Changed 4 years ago by phxx

  • Cc phxx added
  • Keywords only, added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Unfortunatelly this bug is still valid in trunk if you use model inheritance.

My history with this bug was the following:

  • Discovered it with 1.2-beta-1
  • Saw that it was fixed in trunk and updated (and worked)
  • Changed my models to use inheritance which broke things again.

I added a testcase which fails for me with the following backtrace:

Failed example:
    markus.value
Exception raised:
    Traceback (most recent call last):
      File "C:\Python26\lib\site-packages\django\test\_doctest.py", line 1267, in __run
        compileflags, 1) in test.globs
      File "<doctest issue12851.models.__test__.API_TESTS[45]>", line 1, in <module>
        markus.value
      File "C:\Python26\lib\site-packages\django\db\models\query_utils.py", line 208, in __get__
        cls._base_manager.filter(pk=instance.pk).only(name).using(
      File "C:\Python26\lib\site-packages\django\db\models\manager.py", line 141, in filter
        return self.get_query_set().filter(*args, **kwargs)
      File "C:\Python26\lib\site-packages\django\db\models\query.py", line 550, in filter
        return self._filter_or_exclude(False, *args, **kwargs)
      File "C:\Python26\lib\site-packages\django\db\models\query.py", line 568, in _filter_or_exclude
        clone.query.add_q(Q(*args, **kwargs))
      File "C:\Python26\lib\site-packages\django\db\models\sql\query.py", line 1131, in add_q
        can_reuse=used_aliases)
      File "C:\Python26\lib\site-packages\django\db\models\sql\query.py", line 1071, in add_filter
        connector)
      File "C:\Python26\lib\site-packages\django\db\models\sql\where.py", line 66, in add
        value = obj.prepare(lookup_type, value)
      File "C:\Python26\lib\site-packages\django\db\models\sql\where.py", line 299, in prepare
        return self.field.get_prep_lookup(lookup_type, value)
      File "C:\Python26\lib\site-packages\django\db\models\fields\related.py", line 134, in get_prep_lookup
        return self._pk_trace(value, 'get_prep_lookup', lookup_type)
      File "C:\Python26\lib\site-packages\django\db\models\fields\related.py", line 196, in _pk_trace
        v = getattr(field, prep_func)(lookup_type, v, **kwargs)
      File "C:\Python26\lib\site-packages\django\db\models\fields\__init__.py", line 292, in get_prep_lookup
        return self.get_prep_value(value)
      File "C:\Python26\lib\site-packages\django\db\models\fields\__init__.py", line 476, in get_prep_value
        return int(value)
    ValueError: invalid literal for int() with base 10: 'Markus McDonalds'

IMO the SQL query is correct and retrieves the correct fields from the database, but the fields are populated in the wrong way.

comment:10 Changed 4 years ago by russellm

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [13055]) [1.1.X] Fixed #12851 -- Corrected the loading of values when select_related() is used on inherited models. Thanks to phxx for the report and test case.

Backport of r13054 from trunk.

Changed 4 years ago by phxx

Here is another testcase using "only" instead of "defer" which fails with the current trunk.

comment:11 Changed 4 years ago by phxx

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm very sorry for reopening the ticket *again*.

But the issue is still there if you use the "only" method instead of "defer". I've attached a new testcase which fails with the current trunk.

Here is the traceback:

Failed example:
    troy.state.name
Exception raised:
    Traceback (most recent call last):
      File "C:\Python26\lib\site-packages\django\test\_doctest.py", line 1267, in __run
        compileflags, 1) in test.globs
      File "<doctest issue12851.models.__test__.API_TESTS[56]>", line 1, in <module>
        troy.state.name
      File "C:\Python26\lib\site-packages\django\db\models\query_utils.py", line 208, in __get__
        cls._base_manager.filter(pk=instance.pk).only(name).using(
      File "C:\Python26\lib\site-packages\django\db\models\manager.py", line 141, in filter
        return self.get_query_set().filter(*args, **kwargs)
      File "C:\Python26\lib\site-packages\django\db\models\query.py", line 550, in filter
        return self._filter_or_exclude(False, *args, **kwargs)
      File "C:\Python26\lib\site-packages\django\db\models\query.py", line 568, in _filter_or_exclude
        clone.query.add_q(Q(*args, **kwargs))
      File "C:\Python26\lib\site-packages\django\db\models\sql\query.py", line 1131, in add_q
        can_reuse=used_aliases)
      File "C:\Python26\lib\site-packages\django\db\models\sql\query.py", line 1071, in add_filter
        connector)
      File "C:\Python26\lib\site-packages\django\db\models\sql\where.py", line 66, in add
        value = obj.prepare(lookup_type, value)
      File "C:\Python26\lib\site-packages\django\db\models\sql\where.py", line 299, in prepare
        return self.field.get_prep_lookup(lookup_type, value)
      File "C:\Python26\lib\site-packages\django\db\models\fields\related.py", line 134, in get_prep_lookup
        return self._pk_trace(value, 'get_prep_lookup', lookup_type)
      File "C:\Python26\lib\site-packages\django\db\models\fields\related.py", line 196, in _pk_trace
        v = getattr(field, prep_func)(lookup_type, v, **kwargs)
      File "C:\Python26\lib\site-packages\django\db\models\fields\__init__.py", line 292, in get_prep_lookup
        return self.get_prep_value(value)
      File "C:\Python26\lib\site-packages\django\db\models\fields\__init__.py", line 476, in get_prep_value
        return int(value)
    ValueError: invalid literal for int() with base 10: 'Troy Buswell'

comment:12 Changed 4 years ago by russellm

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [13059]) Fixed #12851 -- Another attempt at fixing select_related() with inherited models, this time with only(). Thanks to phxx for the test case.

comment:13 Changed 4 years ago by russellm

(In [13060]) [1.1.X] Fixed #12851 -- Another attempt at fixing select_related() with inherited models, this time with only(). Thanks to phxx for the test case.

Backport of r13059 from trunk.

comment:14 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.