[Django] #25705: Parameters are not adapted or quoted in Query.__str__
Django
----------------------------------------------+--------------------
Reporter: Stranger6667 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
Now it is just string interpolation of the SQL template with parameters
and in most cases produces invalid queries for the following reasons:
- No quoting
- No adaptation. So, some python objects will be used as is, not like
their SQL equivalents
Yes, there are situations, when output of `Query.__str__` is equal to
actual query. But for debugging reasons, it will be better to see real
query here. Also it is logical and expected behavior of this method - to
show actual query.
--
Ticket URL: <https://code.djangoproject.com/ticket/25705>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Stranger6667 | Owner:
Type: | Stranger6667
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Stranger6667):
* status: new => assigned
* needs_better_patch: => 1
* needs_tests: => 1
* owner: nobody => Stranger6667
* needs_docs: => 0
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:1>
Django
Reporter: Stranger6667 | Owner:
Type: | Stranger6667
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by Stranger6667):
* Attachment "ticket25705.patch" added.
Initial draft
Django
Reporter: Stranger6667 | Owner:
Type: | Stranger6667
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by Stranger6667):
Commit:
https://github.com/Stranger6667/django/commit/b2e36668a6877fe29923493e5669402dd30d6421
This issue is more general case of #24991
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:2>
Django
Reporter: Stranger6667 | Owner:
Type: | Stranger6667
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by timgraham):
This looks like a duplicate of #17741 and #25092 which are "wontfix",
however, I'm not sure I see any harm in your proposal besides the question
of whether or not it can be implemented on other database backends.
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:3>
Django
Reporter: Stranger6667 | Owner:
Type: | Stranger6667
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:4>
Django
Reporter: Dmitry Dygalo | Owner: Dmitry
Type: | Dygalo
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by Bernd Wechner):
Replying to [ticket:25705 Dmitry Dygalo]:
> Now it is just string interpolation of the SQL template with parameters
and in most cases produces invalid queries for the following reasons:
>
> - No quoting
> - No adaptation. So, some python objects will be used as is, not like
their SQL equivalents
>
> Yes, there are situations, when output of `Query.__str__` is equal to
actual query. But for debugging reasons, it will be better to see real
query here. Also it is logical and expected behavior of this method - to
show actual query.
Fascinating. I inadvertantly filed a duplicate of this it seems:
https://code.djangoproject.com/ticket/30132#ticket
and I would dearly love this fixed. In essence `Query.__str__` should
return exactly what is logged as per here:
https://docs.djangoproject.com/en/2.1/faq/models/#how-can-i-see-the-raw-
sql-queries-django-is-running
so we can reliably see and extract teh SQL of a query in a production
environment in which DEBUG is not enabled!
I would gladly fix this and PR it, if I had the skills and I am close to
that, as I am coding and debugging python, but single stepping into
`Query.__str__` didn't find me a quick easy answer and I bailed for now.
So if you have any tips as to where the code is that `Query.__str__` uses
to generate SQL and where the code is that logs SQL to
`connection.queries` I canbegin to look at it consider how this might be
fixed.
It seems to me an experience Django coder could fix this in minutes and
that there should be a regression test for this kind of code:
{{{
qs=model.objects.somequeryset
sql=str(qs.query)
raw_qs=model.objects.raw(sql)
}}}
I have a test bed in which I confirmed this failure here:
https://github.com/bernd-
wechner/DjangoTutorial/blob/238787c83ef8515aeeb405577980e71ff35664e8/Library/views.py
Let me know how I might help get this fixed sooner, given ti was reported
3 years ago and is still not fixed!
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:5>
Django
Reporter: Dmitry Dygalo | Owner: Dmitry
Type: | Dygalo
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by GitHub <noreply@…>):
In [changeset:"845042b3d9faaefef8855c2bab48bd9532cd00ca" 845042b3]:
{{{
#!CommitTicketReference repository=""
revision="845042b3d9faaefef8855c2bab48bd9532cd00ca"
Refs #25705 -- Fixed invalid SQL generated by SQLFuncMixin.as_sql() in
custom_lookups tests.
Generated SQL was invalid because parameters are quoted by a driver.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:6>
Django
Reporter: Dmitry Dygalo | Owner: Dmitry
Type: | Dygalo
Cleanup/optimization | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by Petr Přikryl):
* cc: Petr Přikryl (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:7>
Django
Reporter: Dmitry Dygalo | Owner: Dmitry
Type: | Dygalo
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by Mariusz Felisiak):
IMO we should close all related tickets as duplicates:
- #24803 was marked as a duplicate (empty strings in parameters),
- #24991 was marked as a duplicate (range types in parameters).
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:8>
Django
Reporter: Dmitry Dygalo | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by Mariusz Felisiak):
* owner: Dmitry Dygalo => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:9>
Django
Reporter: Dmitry Dygalo | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:10>
Django
Reporter: Dmitry Dygalo | Owner: Alex
Type: | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by Alex):
* owner: (none) => Alex
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:11>
Django
Reporter: Dmitry Dygalo | Owner: Alex
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by Alex):
I've done some investigation.
The main issue comes to the Python DB API doesn't have a way to do this.
The only way to see the query with the parameters binded correctly is
after executing it.
As Mariusz commented
[https://github.com/django/django/pull/15951#issuecomment-1491424713
here], only Mysql/MariaDB and Postgres have a way to do it via a `mogrify`
function which is their own extension to the API.
Django is already using the postgres mogrify in its own compose_sql
function in the
[https://github.com/django/django/blob/f302343380c77e1eb5dab3b64dd70895a95926ca/django/contrib/postgres/search.py#L317
search backend],
[https://github.com/django/django/blob/f302343380c77e1eb5dab3b64dd70895a95926ca/django/db/backends/postgresql/schema.py#L46
schema queries] and
[https://github.com/django/django/blob/f302343380c77e1eb5dab3b64dd70895a95926ca/django/db/backends/postgresql/base.py#L98
ensuring the role of the connection]
In Oracle and SQLite, none of the extensions to the API they add allows do
this.
I see two approaches:
- We fix this issue for the first 3 backends and leave it as it is in
Oracle and SQLite.
- Use the mogrify function in the first three backends, and manually quote
the parameters in the other two. Something similar was already attempted
in [https://github.com/django/django/pull/10568 this PR] and it was
rejected. The amount of effort needed to implement and maintain this,
[https://github.com/python/cpython/blob/db39bc42f90c151b298f97b780e62703adbf1221/Modules/_sqlite/cursor.c#L532
example on how cPython does it for SQLite], would probably be too much
since this seems to be the only use case.
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:12>