Code

Opened 5 years ago

Closed 6 months ago

#11722 closed Bug (fixed)

Query lookups that reference an F() expression produce invalid sql

Reported by: plandry@… Owned by: Anssi Kääriäinen <akaariai@…>
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: F() expression query sql
Cc: hongshuning@…, mumino Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When using an iexact lookup, the sql generated is invalid. For example

MyObject.objects.filter(col1__iexact=F('col2'))

throws a syntax error. Here's an example of the SQL generated:

UPPER("app_myobject"."col1"::text) = UPPER() "app_myobject"."col2"

It's actually not working correctly for any lookup type that involves an expression, but only iexact produces sql that breaks because of the UPPER() function call. An exact lookup produces:

UPPER("app_myobject"."col1"::text) =  "app_myobject"."col2"

Notice the extra space between '=' and '"app_myobject"."col2".

I'm using Django trunk, and Postgresql 8.3.

Attachments (3)

incorrect_fix.diff (1.1 KB) - added by plandry@… 5 years ago.
A patch that illustrates a fix for this case, but is not a permanent fix
possible_fix.patch (600 bytes) - added by plandry@… 5 years ago.
iexact_regression.diff (700 bytes) - added by Ubercore 3 years ago.
Simple test illustrating the issue

Download all attachments as: .zip

Change History (26)

Changed 5 years ago by plandry@…

A patch that illustrates a fix for this case, but is not a permanent fix

comment:1 Changed 5 years ago by plandry@…

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

I don't have an Oracle installation handy, but this error also happens in sqlite3, but not in MySQL. Here is the query produced on sqlite3:

SELECT "app_myobject"."id", "app_myobject"."col1", "app_myobject"."col2" FROM "app_myobject" WHERE "app_myobject"."col1" LIKE  ESCAPE \'\\\' "app_myobject"."col2"

comment:2 Changed 5 years ago by anonymous

  • Has patch set
  • Patch needs improvement set

Changed 5 years ago by plandry@…

comment:3 Changed 5 years ago by plandry@…

I've confirmed that this issue still exists in r12085. I've attached another patch that I still assume isn't a proper solution, but it's simpler than my original wild guess (and trac parses it correctly now, as well). I will continue looking at this bug, but as I think my patch shows, I don't fully understand all the internals yet.

comment:4 Changed 5 years ago by russellm

  • Needs tests set

I fully encourage you to poke around and try and find a solution to this problem - but if you're going to provide a patch, you need to provide a test case as well. Test cases aren't optional - they're how you prove to me (and the rest of the triage and core team) that your patch actually fixes the problem you suggest.

comment:5 Changed 5 years ago by plandry@…

Right you are. Creating the regression test revealed some problems with my patch. I'll re-evaluate how I'm approaching this and get a patch + test up soon. Thanks for the gentle nudge.

comment:6 Changed 4 years ago by Ubercore

  • Owner changed from nobody to Ubercore
  • Status changed from new to assigned

comment:7 Changed 4 years ago by Ubercore

  • Triage Stage changed from Unreviewed to Accepted

comment:8 Changed 4 years ago by Ubercore

Closed #12545 as a duplicate.

comment:9 Changed 4 years ago by Ubercore

  • Owner Ubercore deleted
  • Status changed from assigned to new

Removing myself from this ticket due to lack of time/progress. I still plan on looking at it when time is available!

comment:10 Changed 4 years ago by hongshuning@…

  • Cc hongshuning@… added

I have the save problem when I use startswith lookup.
After appling the patch, the problem isn't gone.
I'm using V1.2.1

comment:11 Changed 3 years ago by baumer1122

  • Severity set to Normal
  • Type set to Bug

comment:12 Changed 3 years ago by aaugustin

  • Easy pickings unset

#15914 is a duplicate. A patch with a very simple test case exhibiting the problem is attached to that ticket.

comment:13 Changed 3 years ago by Ubercore

Thanks, I'm attaching the diff with the test from that ticket here.

Version 0, edited 3 years ago by Ubercore (next)

Changed 3 years ago by Ubercore

Simple test illustrating the issue

comment:14 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:15 Changed 18 months ago by anonymous

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

comment:16 Changed 18 months ago by aaugustin

  • Resolution fixed deleted
  • Status changed from closed to new

Could you describe how you confirmed this problem was fixed? Do you know which commit fixed it?

Last edited 18 months ago by aaugustin (previous) (diff)

comment:17 Changed 11 months ago by akaariai

I can confirm that this issue is still present in master.

comment:18 Changed 8 months ago by mumino

invalid sql generation happens for iendswith, istartswith, icontains, iexact for postgreql. similar problems arise in SQLite. I think other backends have same problem too.

this pull request has a fix. https://github.com/django/django/pull/1945

on the other hand, F objects has another issue related with here.
https://code.djangoproject.com/ticket/16731

because of it istartswith, icontains, iendswith still don't produce correct SQL. but they produce valid SQL now.

comment:19 Changed 8 months ago by mumino

  • Cc mumino added

comment:20 Changed 8 months ago by timo

  • Needs tests unset
  • Patch needs improvement unset

comment:21 Changed 7 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

Confirmed the regression test works as expected and passes on all backends.

comment:22 Changed 7 months ago by akaariai

This one will be fixed as a side effect of lookup refactor, see: https://github.com/akaariai/django/commit/193cd097ca8f2cc6a911e57b8e3fb726f96ee6a6

comment:23 Changed 6 months ago by Anssi Kääriäinen <akaariai@…>

  • Owner set to Anssi Kääriäinen <akaariai@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 20bab2cf9d02a5c6477d8aac066a635986e0d3f3:

Fixed #16187 -- refactored ORM lookup system

Allowed users to specify which lookups or transforms ("nested lookus")
are available for fields. The implementation is now class based.

Squashed commit of the following:

commit fa7a7195f1952a9c8dea7f6e89ee13f81757eda7
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Jan 18 10:53:24 2014 +0200

Added lookup registration API docs

commit eb1c8ce164325e0d8641f14202e12486c70efdb6
Author: Anssi Kääriäinen <akaariai@…>
Date: Tue Jan 14 18:59:36 2014 +0200

Release notes and other minor docs changes

commit 11501c29c9352d17f22f3a0f59d3b805913dedcc
Author: Anssi Kääriäinen <akaariai@…>
Date: Sun Jan 12 20:53:03 2014 +0200

Forgot to add custom_lookups tests in prev commit

commit 83173b960ea7eb2b24d573f326be59948df33536
Author: Anssi Kääriäinen <akaariai@…>
Date: Sun Jan 12 19:59:12 2014 +0200

Renamed Extract -> Transform

commit 3b18d9f3a1bcdd93280f79654eba0efa209377bd
Author: Anssi Kääriäinen <akaariai@…>
Date: Sun Jan 12 19:51:53 2014 +0200

Removed suggestion of temporary lookup registration from docs

commit 21d0c7631c161fc0c67911480be5d3f13f1afa68
Merge: 2509006 f2dc442
Author: Anssi Kääriäinen <akaariai@…>
Date: Sun Jan 12 09:38:23 2014 -0800

Merge pull request #2 from mjtamlyn/lookups_3

Reworked custom lookups docs.

commit f2dc4429a1da04c858364972eea57a35a868dab4
Author: Marc Tamlyn <marc.tamlyn@…>
Date: Sun Jan 12 13:15:05 2014 +0000

Reworked custom lookups docs.

Mostly just formatting and rewording, but also replaced the example
using YearExtract to use an example which is unlikely to ever be
possible directly in the ORM.

commit 250900650628d1f11beadb22814abd666029fb81
Author: Anssi Kääriäinen <akaariai@…>
Date: Sun Jan 12 13:19:13 2014 +0200

Removed unused import

commit 4fba5dfaa022653ffa72497258ffd8f8b7476f92
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Jan 11 22:34:41 2014 +0200

Added docs to index

commit 6d53963f375c77a1f287833b19b976d23f36c30b
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Jan 11 22:10:24 2014 +0200

Dead code removal

commit f9cc0390078e21f1ea5a7bc1f15b09f8f6b0904d
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Jan 11 19:00:43 2014 +0200

A new try for docs

commit 33aa18a6e3c831930bda0028222a26f9c1d96e66
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Jan 11 14:57:12 2014 +0200

Renamed get_cols to get_group_by_cols

commit c7d5f8661b7d364962bed2e6f81161c1b4f1bcc3
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Jan 11 14:45:53 2014 +0200

Altered query string customization for backends vendors

The new way is trying to call first method 'as_' + connection.vendor.
If that doesn't exist, then call as_sql().

Also altered how lookup registration is done. There is now
RegisterLookupMixin class that is used by Field, Extract and
sql.Aggregate. This allows one to register lookups for extracts and
aggregates in the same way lookup registration is done for fields.

commit 90e7004ec14e15503f828cc9bde2a7dab593814d
Merge: 66649ff f7c2c0a
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Jan 11 13:21:01 2014 +0200

Merge branch 'master' into lookups_3

commit 66649ff891c7c73c7eecf6038c9a6802611b5d8a
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Jan 11 13:16:01 2014 +0200

Some rewording in docs

commit 31b8faa62714b4b6b6057a9f5cc106c4dd73caab
Author: Anssi Kääriäinen <akaariai@…>
Date: Sun Dec 29 15:52:29 2013 +0200

Cleanup based on review comments

commit 1016159f34674c0df871ed891cde72be8340bb5d
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Dec 28 18:37:04 2013 +0200

Proof-of-concept fix for #16731

Implemented only for SQLite and PostgreSQL, and only for startswith
and istartswith lookups.

commit 193cd097ca8f2cc6a911e57b8e3fb726f96ee6a6
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Dec 28 17:57:58 2013 +0200

Fixed #11722 -- iexact=F() produced invalid SQL

commit 08ed3c3b49e100ed9019831e770c25c8f61b70f9
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Dec 21 23:59:52 2013 +0200

Made Lookup and Extract available from django.db.models

commit b99c8d83c972786c6fcd0e84c9e5cb08c1368300
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Dec 21 23:06:29 2013 +0200

Fixed review notes by Loic

commit 049eebc0703c151127f4f0265beceea7b8b39e72
Merge: ed8fab7 b80a835
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Dec 21 22:53:10 2013 +0200

Merge branch 'master' into lookups_3

Conflicts:

django/db/models/fields/init.py
django/db/models/sql/compiler.py
django/db/models/sql/query.py
tests/null_queries/tests.py

commit ed8fab7fe8867ff3eb801c3697a426478387bb2f
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Dec 21 22:47:23 2013 +0200

Made Extracts aware of full lookup path

commit 27a57b7aed91b2f346abc4a77da838bffa17c727
Author: Anssi Kääriäinen <akaariai@…>
Date: Sun Dec 1 21:10:11 2013 +0200

Removed debugger import

commit 074e0f5aca0572e368c11e6d2c73c9026e7d63d7
Author: Anssi Kääriäinen <akaariai@…>
Date: Sun Dec 1 21:02:16 2013 +0200

GIS lookup support added

commit 760e28e72bae475b442b026650969b0d182dbe53
Author: Anssi Kääriäinen <akaariai@…>
Date: Sun Dec 1 20:04:31 2013 +0200

Removed usage of Constraint, used Lookup instead

commit eac47766844b90e7d3269e7a8c012eee34ec0093
Author: Anssi Kääriäinen <akaariai@…>
Date: Sun Dec 1 02:22:30 2013 +0200

Minor cleanup of Lookup API

commit 2adf50428d59a783078b0da3d5d035106640c899
Author: Anssi Kääriäinen <akaariai@…>
Date: Sun Dec 1 02:14:19 2013 +0200

Added documentation, polished implementation

commit 32c04357a87e3727a34f8c5e6ec0114d1fbbb303
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Nov 30 23:10:15 2013 +0200

Avoid OrderedDict creation on lookup aggregate check

commit 7c8b3a32cc17b4dbca160921d48125f1631e0df4
Author: Anssi Kääriäinen <akaariai@…>
Date: Sat Nov 30 23:04:34 2013 +0200

Implemented nested lookups

But there is no support of using lookups outside filtering yet.

commit 4d219d4cdef21d9c14e5d6b9299d583d1975fcba
Author: Anssi Kääriäinen <akaariai@…>
Date: Wed Nov 27 22:07:30 2013 +0200

Initial implementation of custom lookups

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.