[Django] #25705: Parameters are not adapted or quoted in Query.__str_

Django

unread,

Nov 7, 2015, 7:00:05 PM11/7/15

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__
----------------------------------------------+--------------------
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

unread,

Nov 7, 2015, 7:02:54 PM11/7/15

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

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

unread,

Nov 7, 2015, 7:03:50 PM11/7/15

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

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):

* Attachment "ticket25705.patch" added.

Initial draft

Django

unread,

Nov 7, 2015, 7:07:05 PM11/7/15

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Keywords: | Triage Stage:
| Unreviewed

Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------

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

unread,

Nov 9, 2015, 10:11:16 AM11/9/15

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Keywords: | Triage Stage:
| Unreviewed

Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------

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

unread,

Nov 13, 2015, 12:38:59 PM11/13/15

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by timgraham):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:4>

Django

unread,

Jan 26, 2019, 3:14:09 AM1/26/19

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

-------------------------------------+-------------------------------------
Reporter: Dmitry Dygalo | Owner: Dmitry
Type: | Dygalo

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------

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

unread,

Dec 2, 2019, 7:59:24 AM12/2/19

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------

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

unread,

Apr 8, 2021, 2:44:37 AM4/8/21

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

-------------------------------------+-------------------------------------
Reporter: Dmitry Dygalo | Owner: Dmitry
Type: | Dygalo
Cleanup/optimization | Status: assigned

Component: Database layer | Version: dev

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Petr Přikryl):

* cc: Petr Přikryl (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:7>

Django

unread,

Dec 31, 2021, 5:33:50 AM12/31/21

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------

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

unread,

Dec 31, 2021, 5:36:41 AM12/31/21

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

-------------------------------------+-------------------------------------
Reporter: Dmitry Dygalo | Owner: (none)
Type: | Status: new
Cleanup/optimization |

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):

* owner: Dmitry Dygalo => (none)
* status: assigned => new

--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:9>

Django

unread,

Mar 18, 2024, 3:21:36 AM3/18/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Ülgen Sarıkavak):

* cc: Ülgen Sarıkavak (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:10>

Django

unread,

Jun 26, 2024, 1:27:19 PM6/26/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

-------------------------------------+-------------------------------------
Reporter: Dmitry Dygalo | Owner: Alex
Type: | Status: assigned

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Alex):

* owner: (none) => Alex

* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:11>

Django

unread,

Jul 5, 2024, 4:50:59 AM7/5/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
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>

Django

unread,

Jul 12, 2024, 10:18:34 AM7/12/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Simon Charette):

My concerns with fixing this issue related to comments such as comment:5

> 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)
> }}}

We absolutely don't want to support this pattern in a context where we
can't guarantee that the proper quoting is performed on all supported
backends as that might result in SQL injection problems. In this sense I
think that it's a good thing that `qs.query` doesn't even attempt to
perform the proper quoting to make it clear it should not be used for this
purpose.

I'd much rather see us document `sql.Query.sql_with_params(using: str =
DEFAULT_DB_ALIAS)` which could be used as

{{{#!python
sql, params = qs.query.sql_with_params()
model.objects.raw(sql, params)
}}}

over spending time trying to dangerously emulate parameters quoting.
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:13>

Django

unread,

Jul 15, 2024, 3:16:05 AM7/15/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):

@Simon do you think we should close this ticket as `wontfix`.

I can recall 2 or 3 (or 4?) occasions since say DjangoCon US 2018 where a
(new) contributor has tried to pick this up, only to hit the same wall.
The trail of breadcrumbs gets slightly clearer each time, but, is there a
realistic fix for this issue, and if not can we not say so?
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:14>

Django

unread,

Jul 15, 2024, 3:48:40 AM7/15/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Alex):

Replying to [comment:13 Simon Charette]:

> My concerns with fixing this issue related to comments such as comment:5
>
> > 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)
> > }}}
>
> We absolutely don't want to support this pattern in a context where we
can't guarantee that the proper quoting is performed on all supported
backends as that might result in SQL injection problems. In this sense I

think that it's a good thing that `sql.Query.__str__` doesn't attempt to

perform the proper quoting to make it clear it should not be used for this
purpose.

I'm not sure I understand your point. I think the original commenter was
suggesting that just as a simple way to do the test. I wouldn't consider
someone doing `str(qs.query)` to then pass in to `raw()` a real use case
that anyone would do.
Personally, the use case I've had with this issue is printing the query to
then try to format it and maybe execute it in a sql editor connected that
my database while testing stuff in local. For that case your suggestion of
sql_with_params wouldn't cover it. I'm not against having that method
either, but I don't see it as a replacement of a correct`str(qs.query)`.

Also my recommended choice was not trying to emulate parameter quoting,
but to use the database library `mogrify` method in 3 of the 5 supported
backends.
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:15>

Django

unread,

Jul 15, 2024, 1:28:08 PM7/15/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Simon Charette):

* cc: Simon Charette (added)

Comment:

@Carlton I think the request for this feature is a symptom of a different
problem so maybe we could re-purpose this ticket instead?

----

@Alex

> I wouldn't consider someone doing str(qs.query) to then pass in to raw()

(which the docs already warn of SQL injections) a real use case that
anyone would do.

I don't agree on that point. From helping users through various forums in
most cases the reason why they were bringing up the issue of improper
quoting was when they tried executing the query as is. The duplicates of
this ticket seem to support this position (see #17741 and #25092
[https://stackoverflow.com/questions/31287184/django-orm-magic-bug-
or-a-feature/31287932#31287932 SO]) and I don't think would be hard to
find other examples.

It might not be your personal use case as you are well versed into why
this a bad idea but I think it would be irresponsible to our user base
from an API design (newcomers footgun) and maintenance burden perspective
(think of the security and bug reports about `mogrify` not producing the
exact same SQL as backend escaping normally does).

> Also my recommended choice was not trying to emulate parameter quoting,
but to use the database library mogrify method in 3 of the 5 supported
backends.

You second alternative mentioned

> Use the mogrify function in the first three backends, and manually quote
the parameters in the other two.

I ''think'' that would qualify as a form of emulation or best-effort
wouldn't it? What guarantees do we have that the quoting is appropriate
for all datatypes that these backends support as parameters? Are we
feeling confident, knowing that users will attempt to pipe `str(qs.query)`
into `raw` and that `Query.__str__` always use `DEFAULT_DB_ALIAS` for
compilation (even if the proper backend can often only be determined as
execution time) that we are exposing the right tools to users and we can
commit to them being safe for the years to come?

IMO the usage of `raw(str(qs.query))`, and the main motive for this
ticket, is a symptom of a lack of documented way for safely building and
executing the SQL and parameters from a `QuerySet` object which makes me
believe the focus should be on documenting `queryset.qs.sql_with_params()`
first instead?

As for the printing and copy-pasting into a shell use case we've reached
an agreement that the only way to see the query with the parameters binded
correctly is after executing it. Knowing that, and the fact
`Query.__str__` is still pretty legible even without the exact mogrifying
that only leaves the copy-pasting into a shell use case. Should we make it
easier to retrieve the exact SQL associated with a particular queryset
instead? Something like `QuerySet.executed_query: str | None` that is only
present on ''evaluated'' querysets.

These appears like solutions that prevent misuse of `Query.__str__` and
avoid maintaining correct and safe x-backend mogrifying solutions?
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:16>

Django

unread,

Jul 15, 2024, 3:00:12 PM7/15/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Alex):

Replying to [comment:16 Simon Charette]:

> > I wouldn't consider someone doing str(qs.query) to then pass in to
raw() (which the docs already warn of SQL injections) a real use case that
anyone would do.
>
> I don't agree on that point. From helping users through various forums
in most cases the reason why they were bringing up the issue of improper
quoting was when they tried executing the query as is. The duplicates of
this ticket seem to support this position (see #17741 and #25092
[https://stackoverflow.com/questions/31287184/django-orm-magic-bug-
or-a-feature/31287932#31287932 SO]) and I don't think would be hard to
find other examples.

If the worry is `raw(str(qs.query))`, then not having the quoting is a
security issue.
In fact I can actually do an SQL injection abusing this lack of quoting.
{{{
qs = User.objects.filter(first_name='"test")--', is_staff=True).only('id')
>>> <QuerySet []>
list(User.objects.raw(str(qs.query)))
>>> [<User: test>]
list(User.objects.raw(str(qs.query)))
>>> 'SELECT "auth_user"."id" FROM "auth_user" WHERE
("auth_user"."first_name" = "test")-- AND "auth_user"."is_staff")'
}}}
With proper quoting, as the one done when executing normally, the query is
`SELECT "auth_user"."id" FROM "auth_user" WHERE ("auth_user"."first_name"
= \'"test")--\' AND "auth_user"."is_staff")`
So if `raw(str(qs.query))` is a risk, then quoting the parameters would
fix it.

> > Also my recommended choice was not trying to emulate parameter
quoting, but to use the database library mogrify method in 3 of the 5
supported backends.
>
> You second alternative mentioned
>
> > Use the mogrify function in the first three backends, and manually
quote the parameters in the other two.
>
> I ''think'' that would qualify as a form of emulation or best-effort
wouldn't it? What guarantees do we have that the quoting is appropriate
for all datatypes that these backends support as parameters? Are we
feeling confident, knowing that users will attempt to pipe `str(qs.query)`
into `raw` and that `Query.__str__` always use `DEFAULT_DB_ALIAS` for

compilation #25947 (even if the proper backend can often only be

determined as execution time) that we are exposing the right tools to
users and we can commit to them being safe for the years to come?

And as I said just after that, that alternative was rejected in the past,
would be too much effort and I was only mentioning it as the only way to
fix it in every backend. I should have been clearer that I wasn't pushing
for this option.

> IMO the usage of `raw(str(qs.query))`, and the main motive for this
ticket, is a symptom of a lack of documented way for safely building and
executing the SQL and parameters from a `QuerySet` object which makes me
believe the focus should be on documenting `queryset.qs.sql_with_params()`

first instead.

That idea was rejected [https://code.djangoproject.com/ticket/34636 14
months ago], but it could be reconsidered in light of this discussion.
Also, I didn't actually find `qs.query` being documented, mostly this
[https://docs.djangoproject.com/en/5.0/ref/models/querysets/#pickling-
querysets mention] about pickling querysets.

> As for the printing and copy-pasting into a shell use case we've reached
an agreement that the only way to see the query with the parameters binded
correctly is after executing it. Knowing that, and the fact
`Query.__str__` is still pretty legible even without the exact mogrifying
that only leaves the copy-pasting into a shell use case. Should we make it
easier to retrieve the exact SQL associated with a particular queryset
instead? Something like `QuerySet.executed_query: str | None` that is only
present on ''evaluated'' querysets.

I would be happy with this option. An alternative would be an error if the
queryset hasn't been evaluated instead of None. But I'm not that big of a
fan of the error idea.

> These appears like solutions that prevent misuse of `Query.__str__` and
avoid maintaining correct and safe x-backend mogrifying solutions?

However, my initial investigation was not deep enough and I found more
reasons to discard the mogrify option:
- In mysqlclient, mogrify was added in version
[https://github.com/PyMySQL/mysqlclient/blob/main/HISTORY.rst#whats-new-
in-220 2.2.0] and django supports
[https://docs.djangoproject.com/en/5.0/ref/databases/#mysqlclient 1.4.3]
onwards, so there would have to be an additional check and a difference in
behavior.
- In the MySQL Connector/Python driver I couldn't find a mogrify method.
Not the recommended driver, but still supported.
- In psicopg3, `mogrify` is only on the
[https://www.psycopg.org/psycopg3/docs/api/cursors.html#psycopg.ClientCursor.mogrify
ClientCursor] class. While that's the default in Django, server side is
also [https://docs.djangoproject.com/en/5.0/ref/databases/#server-side-
parameters-binding supported]. Again, an additional check. Also, I'm not
sure if this means that these usages of mogrify
[https://code.djangoproject.com/ticket/25705?replyto=16#comment:12 I
mentioned] in the postgres backend don't work with server side parameter
binding.

> 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]

My proposal is to close this as a wontfix and work on the
`QuerySet.executed_query` idea and maybe reconsider documenting
`sql_with_params`
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:17>

Django

unread,

Jul 15, 2024, 10:15:06 PM7/15/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Simon Charette):

* cc: Mariusz Felisiak (added)

Comment:

> So if raw(str(qs.query)) is a risk, then quoting the parameters would
fix it.

I was not arguing that it is not possible to cause SQL injection today by
using `raw(str(qs.query))` but that the moment we do ''fix it'' by quoting
parameters and we must ensure to safely support this anti-pattern.

> And as I said just after that, that alternative was rejected in the
past, would be too much effort and I was only mentioning it as the only
way to fix it in every backend. I should have been clearer that I wasn't
pushing for this option.

Sorry I missed that, I thought you were pushing for this option.

> That idea was rejected 14 months ago, but it could be reconsidered in
light of this discussion.

I wasn't aware that documenting `sql_with_params` was rejected in #34636.
I do think we should reconsider.

> I would be happy with this option. An alternative would be an error if
the queryset hasn't been evaluated instead of None. But I'm not that big
of a fan of the error idea.

I don't have a strong opinion on the subject, either works for me!

> My proposal is to close this as a wontfix and work on the
QuerySet.executed_query idea and maybe reconsider documenting

sql_with_params. I'm happy to do both as long as I don't get yelled for
reopenniing the wonfix sql_with_params ticket #34636.

I'm happy to support you towards re-opening #34636 but we should likely
follow the normal process of gathering a bit more consensus. It'd be great
to hear from Mariusz and others what their thoughts are on the matter.
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:18>

Django

unread,

Jul 16, 2024, 1:27:20 AM7/16/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):

FWIW I’ve wanted sql_with_params() to be public for ''other reasons'', so
I’d be keen there is there are no blockers.
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:19>

Django

unread,

Jul 18, 2024, 1:56:35 AM7/18/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):

Replying to [comment:19 Carlton Gibson]:

> FWIW I’ve wanted sql_with_params() to be public for ''other reasons'',
so I’d be keen there is there are no blockers

I don't mind documenting `sql_with_params()`, but would like to avoid
encouraging users to use raw SQL queries. Also, `sql_with_params()` is not
a panacea, it has it's limitation e.g. it always uses the default database
connection. For most users it may be tricky to include parameters into an
SQL string which can lead to SQL injection vectors. IMO,
`sql_with_params()` is only for advanced users, who know the risks.
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:20>

Django

unread,

Jul 18, 2024, 11:07:56 AM7/18/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Simon Charette):

Thank you for chiming in Mariusz.

> I don't mind documenting `sql_with_params()`, **but would like to avoid
encouraging users to use raw SQL queries.**. [snip]

> For most users it may be tricky to include parameters into an SQL string
which can lead to SQL injection vectors. IMO, sql_with_params() is only
for advanced users, who know the risks.

I share this sentiment but I believe that there will always be a need for
an escape hatch and if we don't provide a safe one users will follow the
path of least resistance that `sql.Query.__str__` provides. My hope is
that with the documentation of `sql_with_params` we could even consider
disallowing passing `str(qs.query)` to `raw` and `cursor.execute`
completely to prevent its misuse. Something like

{{{#!diff
diff --git a/django/db/backends/utils.py b/django/db/backends/utils.py
index ab0ea8258b..9874b362b3 100644
--- a/django/db/backends/utils.py
+++ b/django/db/backends/utils.py
@@ -9,6 +9,7 @@

from django.apps import apps
from django.db import NotSupportedError
+from django.db.utils import QueryStr
from django.utils.dateparse import parse_time

logger = logging.getLogger("django.db.backends")
@@ -94,6 +95,8 @@ def _execute_with_wrappers(self, sql, params, many,
executor):
def _execute(self, sql, params, *ignored_wrapper_args):
# Raise a warning during app initialization (stored_app_configs
is only
# ever set during testing).
+ if isinstance(sql, QueryStr):
+ raise TypeError("str(qs.query) cannot be passed directly to
execute(sql).")
if not apps.ready and not apps.stored_app_configs:
warnings.warn(self.APPS_NOT_READY_WARNING_MSG,
category=RuntimeWarning)
self.db.validate_no_broken_transaction()
@@ -107,6 +110,8 @@ def _execute(self, sql, params,
*ignored_wrapper_args):
def _executemany(self, sql, param_list, *ignored_wrapper_args):
# Raise a warning during app initialization (stored_app_configs
is only
# ever set during testing).
+ if isinstance(sql, QueryStr):
+ raise TypeError("str(qs.query) cannot be passed directly to
execute(sql).")
if not apps.ready and not apps.stored_app_configs:
warnings.warn(self.APPS_NOT_READY_WARNING_MSG,
category=RuntimeWarning)
self.db.validate_no_broken_transaction()
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index f00eb1e5a5..9e7cc03527 100644
--- a/django/db/models/sql/query.py
+++ b/django/db/models/sql/query.py
@@ -18,6 +18,7 @@

from django.core.exceptions import FieldDoesNotExist, FieldError
from django.db import DEFAULT_DB_ALIAS, NotSupportedError, connections
+from django.db.utils import QueryStr
from django.db.models.aggregates import Count
from django.db.models.constants import LOOKUP_SEP
from django.db.models.expressions import (
@@ -145,6 +146,8 @@ class RawQuery:
"""A single raw SQL query."""

def __init__(self, sql, using, params=()):
+ if isinstance(sql, QueryStr):
+ raise TypeError("Query strings cannot be safely used by
RawQuery. Use sql_with_params().")
self.params = params
self.sql = sql
self.using = using
@@ -191,8 +194,8 @@ def params_type(self):

def __str__(self):
if self.params_type is None:
- return self.sql
- return self.sql % self.params_type(self.params)
+ return QueryStr(self.sql)
+ return QueryStr(self.sql % self.params_type(self.params))

def _execute_query(self):
connection = connections[self.using]
@@ -340,7 +343,7 @@ def __str__(self):
done by the database interface at execution time.
"""
sql, params = self.sql_with_params()
- return sql % params
+ return QueryStr(sql % params)

def sql_with_params(self):
"""
diff --git a/django/db/utils.py b/django/db/utils.py
index e45f1db249..a5f821cee7 100644
--- a/django/db/utils.py
+++ b/django/db/utils.py
@@ -276,3 +276,12 @@ def get_migratable_models(self, app_config, db,
include_auto_created=False):
"""Return app models allowed to be migrated on provided db."""
models =
app_config.get_models(include_auto_created=include_auto_created)
return [model for model in models if self.allow_migrate_model(db,
model)]
+
+
+class QueryStr(str):
+ """
+ String representation of a SQL query with interpolated params that is
+ unsafe to pass to the database in raw form.
+ """
+ def __str__(self):
+ return self

}}}

> Also, sql_with_params() is not a panacea, it has it's limitation e.g.

**it always uses the default database connection**.

I don't think this is a big issue, the above discussions suggest allowing
`alias` to be passed

{{{#!diff
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index 9e7cc03527..f952c1bb86 100644
--- a/django/db/models/sql/query.py
+++ b/django/db/models/sql/query.py
@@ -345,12 +345,12 @@ def __str__(self):
sql, params = self.sql_with_params()
return QueryStr(sql % params)

- def sql_with_params(self):
+ def sql_with_params(self, alias=DEFAULT_DB_ALIAS):
"""
Return the query as an SQL string and the parameters that will be
substituted into the query.
"""
- return self.get_compiler(DEFAULT_DB_ALIAS).as_sql()
+ return self.get_compiler(alias).as_sql()

def __deepcopy__(self, memo):
"""Limit the amount of work when a Query is deepcopied."""
}}}

With your approval I'll re-open #34636, Alex feel free to pick it up.
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:21>

Django

unread,

Aug 12, 2024, 5:27:45 AM8/12/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Alex):

Added a draft PR https://github.com/django/django/pull/18468 copying the
description for posterity.

No tests done yet. I'll like to know first if the approach I did is valid.

Since the DB connections are per-thread as far as I know, this approach
should not have any race condition issues. And since it's done before the
prefetch, it should pick the Queryset query. Also, since queries are only
logged if DEBUG is enabled, this approach requires that too.

My discarded approach was to to get it somehow in the return of
`self._result_cache = list(self._iterable_class(self))`, or stored in the
QuerySet.
However, it's much more complicated, because as far as I see the flow is
something like
QuerySet when
[https://github.com/django/django/blob/d5bebc1c26d4c0ec9eaa057aefc5b38649c0ba3b/django/db/models/query.py#L1909
evaluating] calls the
[https://github.com/django/django/blob/d5bebc1c26d4c0ec9eaa057aefc5b38649c0ba3b/django/db/models/query.py#L82
IterableClass] which then
[https://github.com/django/django/blob/d5bebc1c26d4c0ec9eaa057aefc5b38649c0ba3b/django/db/models/query.py#L91
executes the query] which then gets the
[https://github.com/django/django/blob/d5bebc1c26d4c0ec9eaa057aefc5b38649c0ba3b/django/db/models/sql/compiler.py#L1585
connection cursor], which is wrapped in the previous mentioned one that
can log the query.

So the a way would be to make all 4 `SqlCompiler.execute_sql` return the
result **and** the SQL query. Then you have to adapt all 6
`Iterable.__iter__` to also return the query. Then you can store it in the
Queryset. Since these methods are part of the public API, this would be a
breaking change in both methods. Also I'm not 100% sure how to get the
query in the wrapper since the `Cursor.execute` is wrapped in a context
manager which is the one logging the query.
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:22>

Django

unread,

Oct 26, 2024, 1:31:44 PM10/26/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Alex):

* needs_better_patch: 1 => 0
* needs_tests: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:23>

Django

unread,

Dec 17, 2024, 5:13:25 AM12/17/24

to django-...@googlegroups.com

#25705: Parameters are not adapted or quoted in Query.__str__

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:24>

Reply all

Reply to author

Forward

[Django] #25705: Parameters are not adapted or quoted in Query.__str__

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django

[Django] #25705: Parameters are not adapted or quoted in Query.str