Opened 4 years ago

Closed 2 years ago

Last modified 2 years ago

#14694 closed Bug (fixed)

defer() doesn't work with reverse relations

Reported by: sayane Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: defer only OneToOneField reverse relationship
Cc: sayane, anssi.kaariainen@…, real.human@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It's impossible to defer REVERSED relation. I'm not able to reproduce whole queryset, because it's generated automatically.

Traceback:
File "/home/sayane/Programowanie/virtenv/dj13/lib/python2.6/site-packages/django/core/handlers/base.py" in get_response
  109.                         response = callback(request, *callback_args, **callback_kwargs)
File "/home/sayane/Programowanie/virtenv/dj13/lib/python2.6/site-packages/django/views/generic/base.py" in view
  52.             return self.dispatch(request, *args, **kwargs)
File "/home/sayane/Programowanie/qcr2/unit/../qcr/core/ajax/views.py" in dispatch
  55.         resp = super(AjaxView, self).dispatch(request, *args, **kwargs)
File "/home/sayane/Programowanie/virtenv/dj13/lib/python2.6/site-packages/django/views/generic/base.py" in dispatch
  73.         return handler(request, *args, **kwargs)
File "/home/sayane/Programowanie/qcr2/unit/../qcr/core/ajax/store.py" in post
  101.         total = self.queryset.count()
File "/home/sayane/Programowanie/virtenv/dj13/lib/python2.6/site-packages/django/db/models/query.py" in count
  327.         return self.query.get_count(using=self.db)
File "/home/sayane/Programowanie/virtenv/dj13/lib/python2.6/site-packages/django/db/models/sql/query.py" in get_count
  391.             obj.add_subquery(subquery, using=using)
File "/home/sayane/Programowanie/virtenv/dj13/lib/python2.6/site-packages/django/db/models/sql/subqueries.py" in add_subquery
  222.         self.subquery, self.sub_params = query.get_compiler(using).as_sql(with_col_aliases=True)
File "/home/sayane/Programowanie/virtenv/dj13/lib/python2.6/site-packages/django/db/models/sql/compiler.py" in as_sql
  58.         self.pre_sql_setup()
File "/home/sayane/Programowanie/virtenv/dj13/lib/python2.6/site-packages/django/db/models/sql/compiler.py" in pre_sql_setup
  29.             self.fill_related_selections()
File "/home/sayane/Programowanie/virtenv/dj13/lib/python2.6/site-packages/django/db/models/sql/compiler.py" in fill_related_selections
  578.                     opts=f.rel.to._meta, as_pairs=True)
File "/home/sayane/Programowanie/virtenv/dj13/lib/python2.6/site-packages/django/db/models/sql/compiler.py" in get_default_columns
  237.         only_load = self.deferred_to_columns()
File "/home/sayane/Programowanie/virtenv/dj13/lib/python2.6/site-packages/django/db/models/sql/compiler.py" in deferred_to_columns
  666.         self.query.deferred_to_data(columns, self.query.deferred_to_columns_cb)
File "/home/sayane/Programowanie/virtenv/dj13/lib/python2.6/site-packages/django/db/models/sql/query.py" in deferred_to_data
  540.                 cur_model = opts.get_field_by_name(name)[0].rel.to

Exception Type: AttributeError at /a/case/stores/case/
Exception Value: 'RelatedObject' object has no attribute 'rel'

When trying with simple query, it doesn't return any results:

In [7]: Case.objects.select_related('info')
Out[7]: [<Case: test/01>]

In [8]: Case.objects.select_related('info').defer('info__requirements')
Out[8]: []

In [9]: Case.objects.select_related('info').defer('info__requirements').count()
Out[9]: 1

Attachments (4)

ticket14694.tgz (10.0 KB) - added by jordanreiter 4 years ago.
To save you time I've attached the sample app to recreate the error
14694-defer-reverse-relations-r17008.2.diff (4.0 KB) - added by mrmachine 3 years ago.
14694-defer-reverse-relations-r17008.diff (5.6 KB) - added by mrmachine 3 years ago.
fix and tests
14694-defer-reverse-relations-r17462.diff (6.0 KB) - added by mrmachine 3 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 4 years ago by russellm

  • milestone 1.3 deleted
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

Closing invalid because of the complete lack of helpful information in narrowing down this problem. You've provided a stack trace... with no example code that generated it. You've provided three sample queries... without any details of the models that are involved.

Feel free to reopen if can provide a complete test case that would allow others to verify the bug without resorting to guesswork.

comment:2 Changed 4 years ago by jordanreiter

  • Easy pickings unset
  • Resolution invalid deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Type set to Bug
  • UI/UX unset
  • Version changed from SVN to 1.3

I can provide some example code, I've run into the exact same problem. It's actually a bit more complicated than that.

Here are some models:

class Paper(models.Model):
    title = models.CharField(max_length=200)
    abstract = models.TextField()

    def __unicode__(self):
        return self.title

class Presentation(models.Model):
    starts = models.DateTimeField()
    ends = models.DateTimeField()
    notes = models.TextField()
    paper = models.OneToOneField("Paper", related_name="presentation")

Then, in the shell:

>>> from papers.models import *
>>> for p in range(0,83):
...     paper = Paper.objects.create(title="Sample Title %d" % p, abstract="This is paper %d" % p)
...     if p / 6 == 0:
...         presentation = Presentation.objects.create(starts='2011-01-01 00:00:00', ends='2011-01-01 01:00:00', paper=paper, notes="This is the presentation for %s" % paper)
... 
>>> qs_no_related = Paper.objects.all().defer('abstract')
>>> qs_no_related
[<Paper_Deferred_abstract: Sample Title 0>, <Paper_Deferred_abstract: Sample Title 1>, <Paper_Deferred_abstract: Sample Title 2>, <Paper_Deferred_abstract: Sample Title 3>, <Paper_Deferred_abstract: Sample Title 4>, <Paper_Deferred_abstract: Sample Title 5>, <Paper_Deferred_abstract: Sample Title 6>, <Paper_Deferred_abstract: Sample Title 7>, <Paper_Deferred_abstract: Sample Title 8>, <Paper_Deferred_abstract: Sample Title 9>, <Paper_Deferred_abstract: Sample Title 10>, <Paper_Deferred_abstract: Sample Title 11>, <Paper_Deferred_abstract: Sample Title 12>, <Paper_Deferred_abstract: Sample Title 13>, <Paper_Deferred_abstract: Sample Title 14>, <Paper_Deferred_abstract: Sample Title 15>, <Paper_Deferred_abstract: Sample Title 16>, <Paper_Deferred_abstract: Sample Title 17>, <Paper_Deferred_abstract: Sample Title 18>, <Paper_Deferred_abstract: Sample Title 19>, '...(remaining elements truncated)...']
>>> print qs_no_related.query
SELECT "papers_paper"."id", "papers_paper"."title" FROM "papers_paper"
>>> len(qs_no_related)
83
>>> qs_no_defer = Paper.objects.all().select_related("presentation__pk")
>>> qs_no_defer
[<Paper: Sample Title 0>, <Paper: Sample Title 1>, <Paper: Sample Title 2>, <Paper: Sample Title 3>, <Paper: Sample Title 4>, <Paper: Sample Title 5>, <Paper: Sample Title 6>, <Paper: Sample Title 7>, <Paper: Sample Title 8>, <Paper: Sample Title 9>, <Paper: Sample Title 10>, <Paper: Sample Title 11>, <Paper: Sample Title 12>, <Paper: Sample Title 13>, <Paper: Sample Title 14>, <Paper: Sample Title 15>, <Paper: Sample Title 16>, <Paper: Sample Title 17>, <Paper: Sample Title 18>, <Paper: Sample Title 19>, '...(remaining elements truncated)...']
>>> print qs_no_defer.query
SELECT "papers_paper"."id", "papers_paper"."title", "papers_paper"."abstract", "papers_presentation"."id", "papers_presentation"."starts", "papers_presentation"."ends", "papers_presentation"."notes", "papers_presentation"."paper_id" FROM "papers_paper" LEFT OUTER JOIN "papers_presentation" ON ("papers_paper"."id" = "papers_presentation"."paper_id")
>>> len(qs_no_defer)
83
>>> qs_related_defer=Paper.objects.all().defer('abstract','presentation__notes').select_related("presentation__pk")
>>> qs_related_defer
[]
>>> print qs_related_defer.query
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/path/to/django/db/models/sql/query.py", line 162, in __str__
    sql, params = self.get_compiler(DEFAULT_DB_ALIAS).as_sql()
  File "/path/to/django/db/models/sql/compiler.py", line 58, in as_sql
    self.pre_sql_setup()
  File "/path/to/django/db/models/sql/compiler.py", line 29, in pre_sql_setup
    self.fill_related_selections()
  File "/path/to/django/db/models/sql/compiler.py", line 653, in fill_related_selections
    opts=model._meta, as_pairs=True, local_only=True)
  File "/path/to/django/db/models/sql/compiler.py", line 237, in get_default_columns
    only_load = self.deferred_to_columns()
  File "/path/to/django/db/models/sql/compiler.py", line 670, in deferred_to_columns
    self.query.deferred_to_data(columns, self.query.deferred_to_columns_cb)
  File "/path/to/django/db/models/sql/query.py", line 555, in deferred_to_data
    cur_model = opts.get_field_by_name(name)[0].rel.to
AttributeError: 'RelatedObject' object has no attribute 'rel'
>>> len(qs_related_defer)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/path/to/django/db/models/query.py", line 82, in __len__
    self._result_cache = list(self.iterator())
  File "/path/to/django/db/models/query.py", line 229, in iterator
    only_load = self.query.get_loaded_field_names()
  File "/path/to/django/db/models/sql/query.py", line 1761, in get_loaded_field_names
    self.deferred_to_data(collection, self.get_loaded_field_names_cb)
  File "/path/to/django/db/models/sql/query.py", line 555, in deferred_to_data
    cur_model = opts.get_field_by_name(name)[0].rel.to
AttributeError: 'RelatedObject' object has no attribute 'rel'
>>> 


Note that the reason I was using defer in this case is because I was also using a distinct() which can't be used on Text fields (at least, not on the DB server I'm using). However, I found that this error occurs whether or not you are using distinct().

This is 100% repeatable in Django 1.3. The example shell output came from a project I created from scratch for the purpose of this ticket. It uses the SQLite backend so it is not specific to a backend either.

Changed 4 years ago by jordanreiter

To save you time I've attached the sample app to recreate the error

comment:3 Changed 4 years ago by Alex

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 3 years ago by mrmachine

  • Has patch set
  • Keywords defer only OneToOneField reverse relationship added
  • Version changed from 1.3 to SVN

I think this is a simple fix, and have attached a patch with tests. Using get_field_by_name() with the name of a reverse relation returns a RelatedObject, which has a .model attribute, instead of rel.to, for the target model.

comment:5 Changed 3 years ago by mrmachine

  • Patch needs improvement set

On closer inspection, this fix doesn't fully resolve the problem. deferred_to_columns_cb() still tries to access field.column, which doesn't exist on RelatedObject. I tried adding the actual field instead of the RelatedObject to seen and must_include, but I still get problems. There are probably ORM complexities that I'm missing here. One thing that has probably helped this bug to slip under the radar is that exceptions raised in deferred_to_columns_cb() and deferred_to_data() appear to be silenced in some cases. Evaluating a queryset could return no results when .count() shows that there are items in the set, but converting querysey.query to str raises an exception.

Changed 3 years ago by mrmachine

comment:6 Changed 3 years ago by mrmachine

  • Patch needs improvement unset

Updated the patch to avoid adding RelatedObject fields (not actual DB fields), which fixes the error mentioned above (trying to access field.column).

Also tracked down the cause of problems when using select_related() with defer() for reverse relations. Line 1400 of query.py in get_cached_row() was trying to get the value of foreign key fields on the related object by looking up the attname of the foreign key field on the target object. Obviously this would never work. I'm not sure why it wasn't picked up until now. It seems to fail silently under normal circumstances, but the exception was raised only when running the test suite.

comment:7 Changed 3 years ago by mrmachine

  • Patch needs improvement set

Still problems. I don't think my change to get_cached_row() is correct, but I don't think what's there is correct, either. At least, not for objects with deferred fields.

For objects with deferred fields, it seems that NO fields are local and _meta.get_fields_with_model() returns a model for what should be local fields.

This causes the rel_model is not None condition to fail and all attributes on rel_obj are set to the same value as the matching attribute on obj.

For example: if Profile has user = OneToOneField(User, reverse_name='profile'), and you try User.objects.select_related('profile').defer('profile__nickname'), then when get_cached_row() is called, rel_obj will be a Profile_Deferred_nickname object with no local fields and Profile as the model for all fields, and obj will be a User object. It will do setattr(rel_obj, rel_field.attname, getattr(obj, rel_field.attname)) for all fields including the auto PK field, which will end up setting the PK of your Profile to the same PK as your User. If Profile has any fields that don't exist, an exception is raised when running tests. The exception appears to be silenced and an empty queryset returned under normal circumstances.

Changed 3 years ago by mrmachine

fix and tests

comment:8 Changed 3 years ago by mrmachine

I think I've finally cracked it. When working with model instances that have deferred fields, you need to call get_fields_with_model() on the original model to get non-local fields correctly. You can access the opts for the original model with rel_obj._meta.proxy_for_model._opts. I have just updated the patch. The full test suite passes, so I think this is RFC now, but I will wait for review by more a set of eyes more experienced with the ORM.

Changed 3 years ago by mrmachine

comment:9 Changed 3 years ago by mrmachine

Updated and re-tested against trunk.

Fixed (and tested) one other case where passing in the related_name for a reverse OneToOneField to .only() (e.g. Item.objects.only('one_to_one_item')) would result in a silenced exception (triggered by attempting to access field.column when field is a RelatedObject) and an empty queryset when evaluated.

This problem wasn't obvious with .defer(), or when referencing fields on a reverse related model with .only() (e.g. Item.objects.only('one_to_one_item__name')).

I think this is a clear bug, and the patch RFC. Just awaiting review by another set of eyes more familiar with the ORM :)

comment:10 Changed 3 years ago by akaariai

  • Cc anssi.kaariainen@… added

I think this part could lead to some bugs:

if getattr(rel_obj, '_deferred'): 
    opts = opts.proxy_for_model._meta 

The reason is, proxy_for_model relates to the concrete model behind the proxy chain. So, if you have

class A:
    pass

class ProxyA(A):
    proxy = True

And you query ProxyA.objects.defer('some_f'), now you will be using the options of A instead of ProxyA. I don't know if this will cause any bugs but it is wrong in any case.

There are many places in the code assuming that proxy_for_model generates a chain. However, this is not true. I think I have seen issues related to this on at least couple of tickets. Fixing this would be nice, but I haven't gotten much feedback from the community in the tickets where I have tried to solve this (#16128 for example).

There are some isinstance() calls added to query.py. Is it at all possible to return something consistent from get_field_by_name(), so that query.py would not need this complication?

comment:11 Changed 3 years ago by mrmachine

I've updated this patch on a branch at GitHub to apply cleanly again.

https://github.com/thirstydigital/django/tree/tickets/14694-defer-reverse-relations

I don't think there is a problem with the opts = opts.proxy_for_model._meta line that you questioned.

In the case of an automatic proxy model with deferred fields, we are intentionally accessing the opts from the concrete model in order to get and populate non-local fields correctly. Those fields don't exist on the automatic proxy model. We can only get them from the concrete model.

I still think this patch is RFC, and that this is a clear bug. Maybe you can improve the test cases to confirm your suspicions (or confirm that the patch is good)?

comment:12 Changed 3 years ago by mrmachine

  • Cc real.human@… added

comment:13 Changed 3 years ago by akaariai

  • Patch needs improvement unset

The proxy_for_model actually works correctly now, it was fixed some time ago to actually be the next model in the chain, and there is a new concrete_model opts parameter which is the concrete model in the end of the chain.

So, I am pretty sure that part of the patch is correct.

comment:14 Changed 3 years ago by mrmachine

Great. If there's no other issues, can this be marked RFC now?

comment:15 Changed 3 years ago by akaariai

There is one other issue - I think the patch will allow silent .only()/.defer() calls to reverse foreign keys, while before that should have errored out in .iterator(). This is hard to see as the error is silenced and an empty list is returned in the erroneous case.

As in the patch, a .defer/.only call to reverse FK is silently discarded. At least I think that is the case...

The correct approach would be to do a check on the .only/.defer values when called, not when iterating the queryset. Another ticket's problem.

So, I do think the patch is RFC, although the logic can be a little bit complex to follow in the deferred_to_data.

I have one idea for making the hidden exceptions bug a little less annoying. I will see if I can make it work. If yes, it should make seeing the above error easier.

comment:16 Changed 3 years ago by DrMeers

akaariai do you want to create another ticket before we close/commit this one so it doesn't get forgotten?

comment:17 Changed 3 years ago by akaariai

The idea about iterator ended up not working without altering the queryset iterator behavior. See #18702.

I will try to find some time to get this one committed.

comment:18 Changed 3 years ago by glarrain

Please forgive the lack of quality of this report (it's my first) but I want to comment that this issue affects me too. The following piece of code is where the exception 'RelatedObject' object has no attribute 'rel' is raised.

values = [
	o.caracteristicastelefono.resolucion_camara_trasera.get('total')
	for o in self.con_caracteristicas().\
		only('caracteristicastelefono__resolucion_total_camara_trasera',
			'caracteristicastelefono__resolucion_x_camara_trasera',
			'caracteristicastelefono__resolucion_y_camara_trasera')
	]

where models.py contains

class Telefono(mo.Model):
	# some irrelevant fields
	pass

class CaracteristicasTelefono(mo.Model):
	telefono = mo.OneToOneField(Telefono)

#
# more fields
#

	resolucion_total_camara_trasera = mo.FloatField('Resolución total (MP)',
		blank=True, null=True,
		help_text='(e.g. 1.5) Definir SÓLO si están vacías res. horizontal y vertical')
	resolucion_x_camara_trasera = mo.IntegerField('Resolución horizontal (px)',
		blank=True, null=True, help_text='(e.g. 1920)')
	resolucion_y_camara_trasera = mo.IntegerField('Resolución vertical (px)',
		blank=True, null=True, help_text='(e.g. 1080)')

#
# more fields and methods
#

	@property
	def resolucion_camara_trasera(self):
		total = self.resolucion_total_camara_trasera
		x = self.resolucion_x_camara_trasera
		y = self.resolucion_y_camara_trasera
		return get_resolucion(total, x, y)

and managers.py contains

class TelefonoQuerySet(models.query.QuerySet):

	def con_caracteristicas(self):
		return self.exclude(caracteristicastelefono__exact=None)

	def activos(self):
		return self.filter(status=STATUS_TELEFONOS_ACTIVOS)

class TelefonoManager(models.Manager):

	def get_query_set(self):
		model = models.get_model('telefonos', 'Telefono')
		return TelefonoQuerySet(model)

The class CaracteristicasTelefono has lots of fields. Since I only need to get the result of resolucion_camara_trasera and that method only uses 3 fields, it made sense to use Queryset's only() method.

I hope this info helps to diagnose/fix the problem. Thanks

PS: I'm using Django==1.4.1 / virtualenv==1.7.1.2 / Python 2.7.3 / Ubuntu 12.04 amd64

comment:19 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

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

In 6ebf115206289bce8f3d86318871faac13d6e835:

Fixed #14694 -- Made defer() work with reverse relations

Reverse o2o fields are now usable with defer.

comment:20 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

In a21e8dee7682abdba646c5b72b6371d392e44dd1:

[1.5.x] Fixed #14694 -- Made defer() work with reverse relations

Reverse o2o fields are now usable with defer.

Backpatch of [6ebf115206289bce8f3d86318871faac13d6e835]

comment:21 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

In e3ea668b47f6b7d63abd93a62b89f809cd3d5146:

[1.5.x] Fixed #14694 again -- Made defer() works with reverse relations

Master and stable/1.5.x had diverged in models/query.py.

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