Opened 2 years ago

Closed 12 days ago

#22288 closed Bug (fixed)

F() expression not compatible with __range field look up

Reported by: liushaohua86@… Owned by: MatthewWilkes
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: github@…, josh.smeaton@…, matthew@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by timgraham)

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'

Change History (15)

comment:2 Changed 2 years ago by bmispelon

  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.5 to master

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.

comment:3 Changed 2 years ago by timmartin

  • Has patch set

I've had a go at fixing this, the commit is here: https://github.com/django/django/compare/master...timmartin:bug22288

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.

Last edited 6 months ago by timgraham (previous) (diff)

comment:4 Changed 2 years ago by akaariai

  • Patch needs improvement set

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.

comment:5 Changed 13 months ago by Naddiseo

  • Cc github@… added

Any chance that this can be looked into now?

comment:6 Changed 13 months ago 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

In fact, I'll incorporate the test case above into this patch to check.

Last edited 13 months ago by jarshwah (previous) (diff)

comment:7 Changed 13 months ago by jarshwah

Nope, the linked patch doesn't alter the path at all.

comment:8 Changed 10 months ago by timgraham

  • Description modified (diff)

comment:9 Changed 6 months ago by MatthewWilkes

  • Needs documentation unset
  • Needs tests unset

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.

EDIT: On a practical note, I didn't intentionally unset the docs and tests parameters, they were unset on load and checking them changed the message to say that I'd changed it to checked. I guess they are newish fields?

Last edited 6 months ago by MatthewWilkes (previous) (diff)

comment:10 Changed 6 months ago by jarshwah

  • Cc josh.smeaton@… added

comment:11 Changed 6 months ago by MatthewWilkes

  • Cc matthew@… added

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.

comment:12 Changed 6 months ago by MatthewWilkes

#16487 is another instance of this

comment:13 Changed 3 months ago 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?

comment:14 Changed 4 weeks ago by MatthewWilkes

  • Owner changed from nobody to MatthewWilkes
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Accepted to Ready for checkin

The PR is against 1.11 and ready for final review. Setting to ready for check-in following jarshwah's instructions on GitHub.

comment:15 Changed 2 weeks ago by timgraham

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

I left a few comments for improvement.

comment:16 Changed 12 days ago by Tim Graham <timograham@…>

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

In 4f138fe5:

Fixed #22288 -- Fixed F() expressions with the range lookup.

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