Opened 6 years ago

Last modified 3 days ago

#28944 new Bug

Chaining values()/values_list() after QuerySet.select_for_update(of=()) crashes

Reported by: Thierry Bastian Owned by:
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Ran Benita, Can Sarıgöl Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

With any of my models, if I do the following:

model.objects.select_for_update(of=('self',)).values_list('pk')

I'm getting the following exception:

Traceback (most recent call last):
  File "<console>", line 2, in <module>
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/query.py", line 272, in __iter__
    self._fetch_all()
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/query.py", line 1179, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/query.py", line 139, in __iter__
    return compiler.results_iter(tuple_expected=True, chunked_fetch=self.chunked_fetch, chunk_size=self.chunk_size)
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 1014, in results_iter
    results = self.execute_sql(MULTI, chunked_fetch=chunked_fetch, chunk_size=chunk_size)
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 1050, in execute_sql
    sql, params = self.as_sql()
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 507, in as_sql
    of=self.get_select_for_update_of_arguments(),
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 961, in get_select_for_update_of_arguments
    ', '.join(_get_field_choices()),
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 928, in _get_field_choices
    for klass_info in klass_info.get('related_klass_infos', [])
AttributeError: 'NoneType' object has no attribute 'get'

Change History (22)

comment:1 by Simon Charette, 6 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Marking as release blocker because it's a bug in a newly introduced feature. (selected_for_update(of)).

comment:2 by Tim Graham, 6 years ago

Cc: Ran Benita added
Summary: Queryset cannot handle select_for_update and values/values_list at the same timeChaining values()/values_list() after QuerySet.select_for_update(of=()) crashes

comment:3 by Ran Benita, 6 years ago

Has patch: set

comment:4 by Simon Charette, 6 years ago

Triage Stage: AcceptedReady for checkin

I think the patch is RFC except for a few cosmetic comments and a release note for 2.0.1.

comment:5 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: newclosed

In c21f1582:

Fixed #28944 -- Fixed crash when chaining values()/values_list() after QuerySet.select_for_update(of=()).

comment:6 by Tim Graham <timograham@…>, 6 years ago

In 4e4619a:

[2.0.x] Fixed #28944 -- Fixed crash when chaining values()/values_list() after QuerySet.select_for_update(of=()).

Backport of c21f158295d92e35caf96436bfdbbff554fc5569 from master

comment:7 by Thierry Bastian, 6 years ago

Resolution: fixed
Status: closednew

Ok so you fixed the simple case with self, but it still fails where there are joins.

So if you have 2 models:

class A(models.Model):
   foo = models.TextField()

class B(models.Model):
    bla = models.TextField()
    a = models.ForeignKey(a, on_delete=models.CASCADE)

and then if you want to do: B.objects.all().select_related('a').select_for_update(of=('self', 'a')), that works
but the same with a values_list: B.objects.all().select_related('a').select_for_update(of=('self', 'a')).values_list('pk', 'a__foo'), that does not work and results in something like (I have different class names in my code)

Traceback (most recent call last):
  File "<console>", line 2, in <module>
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/query.py", line 248, in __repr__
    data = list(self[:REPR_OUTPUT_SIZE + 1])
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/query.py", line 272, in __iter__
    self._fetch_all()
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/query.py", line 1179, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/query.py", line 139, in __iter__
    return compiler.results_iter(tuple_expected=True, chunked_fetch=self.chunked_fetch, chunk_size=self.chunk_size)
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 1015, in results_iter
    results = self.execute_sql(MULTI, chunked_fetch=chunked_fetch, chunk_size=chunk_size)
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 1051, in execute_sql
    sql, params = self.as_sql()
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 508, in as_sql
    of=self.get_select_for_update_of_arguments(),
  File "/usr/local/filewave/python/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 962, in get_select_for_update_of_arguments
    ', '.join(_get_field_choices()),
django.core.exceptions.FieldError: Invalid field name(s) given in select_for_update(of=(...)): mdm_client. Only relational fields followed in the query are allowed. Choices are: self.
Last edited 6 years ago by Tim Graham (previous) (diff)

comment:8 by Tim Graham, 6 years ago

Has patch: unset
Triage Stage: Ready for checkinAccepted

comment:9 by Ran Benita, 6 years ago

I'm afraid this case is harder. Essentially, the current implementation of select_for_update(of=(...)) is based on the select_related() infrastructure, but of course once .values()/.values_list() is invoked there is no more select_related.

I will try to dig deeper once I am able. Help is welcome.

comment:10 by Tim Graham, 6 years ago

It could be acceptable (at least as a temporary measure) to raise a message that it's not supported.

comment:11 by Thierry Bastian, 6 years ago

@Ran Benita: I was afraid you'd say something like that. My knowledge of django internals is not that great at the moment. So I will have to try and find a better way to do that on my own.

comment:12 by Tim Graham, 6 years ago

Severity: Release blockerNormal

comment:13 by Tim Graham, 6 years ago

Has patch: set
Patch needs improvement: set

Anssi started a PR but doesn't have time to finish it.

comment:14 by Can Sarıgöl, 5 years ago

PR

a different approach

comment:15 by Can Sarıgöl, 5 years ago

Cc: Can Sarıgöl added
Owner: changed from nobody to Can Sarıgöl
Patch needs improvement: unset
Status: newassigned

comment:16 by Mariusz Felisiak, 4 years ago

Needs tests: set
Patch needs improvement: set
Version: 2.0master

comment:17 by Chetan Khanna, 4 years ago

Needs tests: unset
Patch needs improvement: unset

https://github.com/django/django/pull/12258

I've added a PR based off the one Anssi started. I'll be glad if someone could take a look and let me know of any improvements needed. All sqlite and postgres tests were passing when I tested through django-docker-box.

comment:18 by Mariusz Felisiak, 4 years ago

Owner: changed from Can Sarıgöl to Chetan Khanna

comment:19 by Mariusz Felisiak, 4 years ago

Patch needs improvement: set

comment:20 by Chetan Khanna, 4 years ago

Patch needs improvement: unset

comment:21 by Mariusz Felisiak, 4 years ago

Patch needs improvement: set

comment:22 by Mariusz Felisiak, 3 days ago

Owner: Chetan Khanna removed
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top