[Django] #22288: F() expression not compatible with __range field look up
Django
----------------------------------------------+--------------------
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
class TestModel(models.Model):
a = models.SmallIntegerField()
b = models.SmallIntegerField()
TestModel.objects.filter(a__range=(F('b')-1, F('b')+1)
TypeError: int() argument must be a string or a number, not
'ExpressionNode'
--
Ticket URL: <https://code.djangoproject.com/ticket/22288>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by liushaohua86@…):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
class TestModel(models.Model):
a = models.SmallIntegerField()
b = models.SmallIntegerField()
TestModel.objects.filter(a_ _range=(F('b')-1, F('b')+1))
TypeError: int() argument must be a string or a number, not
'ExpressionNode'
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:1>
Django
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by bmispelon):
* version: 1.5 => master
* stage: Unreviewed => Accepted
Comment:
Hi,
I can reproduce this issue on latest master as well as on 1.6 so it
doesn't appear to be related to the new custom lookup feature.
Regarding your formatting issues, you can make code blocks on trac using
`{{{ code goes here }}}`, as explained on the WikiFormatting page.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:2>
Django
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
I've had a go at fixing this, the commit is here:
https://github.com/timmartin/django/commit/9ea32634efda8f4a74ef9ee9a742a1437bc27ca0
I'd like feedback on this fix, since I'm not sure I fully understand the
potential wider impact of the changes I've made (though it passes all
regression tests). It also looks like the changes in the pull request for
#14030 will interact with this.
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:3>
Django
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
I checked the patch. It seems we need changes to too many places just to
support `something__range=(F('foo'), F('bar'))`. The main issue is that we
have many rounds of value preparation.
The main issue here is that we just don't handle the case here value is an
iterable correctly for value preparation. While the above mentioned commit
touches the area around range lookups, we should likely fix all the
lookups to lists of F() objects properly. Even if the only other example
in core is `somefield__in=[F()]`.
It might be better to wait for #14030 before applying this one.
In addition the tests of the above patch aren't correct, the code checks
generated SQL, but that SQL differs between backends (quote character is
not always `"`, and some backends IIRC use upper case table names).
Sorry for delay in commenting this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:4>
Django
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Naddiseo):
* cc: github@… (added)
Comment:
Any chance that this can be looked into now?
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:5>
Django
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by jarshwah):
I think a side effect of https://github.com/django/django/pull/5090/ is
that range and in lookups will now understand expressions properly. I'm
not certain of this though, it just looks like similar code paths.
https://github.com/django/django/pull/5090/files#diff-
b6b218ec29b7fb6a7d89868a94bfc73eR61
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:6>
Django
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by jarshwah):
Nope, the linked patch doesn't alter the path at all.
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:7>
Django
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Description changed by timgraham:
Old description:
> class TestModel(models.Model):
> a = models.SmallIntegerField()
> b = models.SmallIntegerField()
>
> TestModel.objects.filter(a__range=(F('b')-1, F('b')+1)
>
> TypeError: int() argument must be a string or a number, not
> 'ExpressionNode'
New description:
{{{
class TestModel(models.Model):
a = models.SmallIntegerField()
b = models.SmallIntegerField()
TestModel.objects.filter(a__range=(F('b')-1, F('b')+1)
TypeError: int() argument must be a string or a number, not
'ExpressionNode'
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:8>
Django
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by MatthewWilkes):
* needs_docs: => 0
* needs_tests: => 0
Comment:
I have had the same problem, albeit with date objects. I've begun working
on a patch, WIP is at
https://github.com/django/django/compare/master...MatthewWilkes:22288
-iterables-and-F-objects
Current state is 1 failing test on sqlite (not tested on other backend) so
certainly not ready for review at the moment. Posting here for visibility,
as I've spoken to many of the people involved on IRC.
While working on this I've started to think that checking for expected
parameter types in lookups being done by comparing their names is a bad
idea, given the work done on custom lookups. Perhaps it would make sense
for some of the value preparation to be offloaded onto the lookup, or at
least the parsing of value data structures, to allow for richer lookups to
use expressions too.
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:9>
Django
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by jarshwah):
* cc: josh.smeaton@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:10>
Django
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by MatthewWilkes):
* cc: matthew@… (added)
Comment:
I've added a WIP pull request at
https://github.com/django/django/pull/6216
Any feedback on what additional testing would be required as well as on my
general approach would be very much appreciated.
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:11>
Django
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by MatthewWilkes):
#16487 is another instance of this
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:12>
Django
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by MatthewWilkes):
I have updated the PR at https://github.com/django/django/pull/6216 - I
believe this is in a good state but will need re-reviewing by ORM experts.
Is this something that should be against 1.10 or 1.11 at this stage?
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:13>
Django
Reporter: liushaohua86@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:14>
Django
Reporter: liushaohua86@… | Owner:
| MatthewWilkes
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MatthewWilkes):
* owner: nobody => MatthewWilkes
* needs_better_patch: 1 => 0
* status: new => assigned
* stage: Accepted => Ready for checkin
Comment:
The PR is against 1.11 and ready for final review. Setting to ready for
check-in following jarshwah's instructions on GitHub.
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:14>
Django
Reporter: liushaohua86@… | Owner:
| MatthewWilkes
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
I left a few comments for improvement.
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:15>
Django
Reporter: liushaohua86@… | Owner:
| MatthewWilkes
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"4f138fe5a496a81115c4fba6615a517fc62c3b17" 4f138fe5]:
{{{
#!CommitTicketReference repository=""
revision="4f138fe5a496a81115c4fba6615a517fc62c3b17"
Fixed #22288 -- Fixed F() expressions with the __range lookup.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:16>
Django
Reporter: liushaohua86@… | Owner: Matthew
| Wilkes
Type: Bug | Status: closed
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"561f77041542f9081288af2af97a289544a57e2a" 561f770]:
{{{
#!CommitTicketReference repository=""
revision="561f77041542f9081288af2af97a289544a57e2a"
Refs #22288 -- Corrected __range lookup test names.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22288#comment:17>