Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#29891 closed Cleanup/optimization (duplicate)

Fix incorrect quoting in queryset.query

Reported by: Timothy Allen Owned by: Timothy Allen
Component: Database layer (models, ORM) Version: 2.1
Severity: Normal Keywords: queryset, orm
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

This old ticket has some recent activity I have also run into with fields not being properly quoted in queryset.query:

https://code.djangoproject.com/ticket/14091

For a minimum example, start an app and create a model with a DateField:

from django.db import models

class BadDateModel(models.Model):
    my_date = models.DateField()

Then fire up a shell:

>>> q = BadDateModel.objects.filter(my_date__gt='1990-01-01')
>>> print(q.query)
SELECT "bug_app_baddatemodel"."id", "bug_app_baddatemodel"."my_date" FROM "bug_app_baddatemodel" WHERE "bug_app_baddatemodel"."my_date" > 1990-01-01

As you can see, the generated query has a date value which is not quoted, which will cause an error when executed. This is causing me issues when trying to do an EXECUTE {queryset.query} to get an estimate of table size before performing certain DRF queries.

The error when it is executed against PostgreSQL:

operator does not exist: date < integer
LINE 1: ... FROM "blah"."blah1" WHERE ("blah"."blah1"."date" < 1990-01-...
                                                             ^
HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.

I discovered this in PostgreSQL, but it seems to affect all backends; the repro above was done with SQLite.

Change History (8)

comment:1 Changed 5 years ago by Tim Graham

QuerySet.__str__() isn't meant to give executable SQL, see #27587. That said, Aymeric's comment there suggests to improve the situation if possible. Do you want to try a patch?

comment:2 Changed 5 years ago by Timothy Allen

I'll take a look and see if I can implement the same logic Aymeric implemented for last_executed_query. Thanks for pointing me to the ticket!

comment:3 Changed 5 years ago by Timothy Allen

Owner: changed from nobody to Timothy Allen
Status: newassigned

comment:4 Changed 5 years ago by Dan Davis

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

I setup SQL logging via LOGGING dictConfig and verified that as suggested, this only affects print(queryset.query). Therefore, I'd describe this as cleanup/optimization rather than a bug.

comment:5 Changed 5 years ago by Timothy Allen

I've put in a work-in-progress PR: https://github.com/django/django/pull/10568

This is my first time diving into any part of the ORM source, so I'd love some feedback / suggestions. It currently causes a few tests to fail, I believe from the removal of the ESCAPE.

comment:6 Changed 5 years ago by Carlton Gibson

This is related to #27587, which concerns documenting Query.query.

Aymeric's comment there is worth having here:

Regarding the limitations, I don't think we should accept the current status quo as the best we can do:

  1. if it's reasonably easy to perform the escaping correctly, typically just by escaping arguments with a function provided by the database adapter prior to interpolation, then Django should do it.
  2. if there's no function for escaping arguments, but a well documented and not too complicated process do to so (replace " by "" then wrap in "...") then we should consider doing it as well
  3. if the escaping rules are unclear and there's no way to ask the database to do it, then we should return something that is *obviously* invalid, like QUERY = ... ; PARAMS = ...

I did that some time ago for last_executed_query. SQLite stayed at 3 for half a decade before moving to 2.

I believe the same logic should apply to QuerySet.query and similar methods.

comment:7 Changed 5 years ago by Carlton Gibson

Has patch: set
Needs tests: set
Patch needs improvement: set

comment:8 Changed 4 years ago by Mariusz Felisiak

Resolution: duplicate
Status: assignedclosed

Closing this as a duplicate of #25705.

Last edited 4 years ago by Mariusz Felisiak (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top