Re: [Django] #10070: Named parameters not working on raw sql queries with sqlite
Django
-------------------------------------+-------------------------------------
Reporter: msurdi | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* ui_ux: => 0
* easy: => 0
* stage: Design decision needed => Accepted
Comment:
Based on Ramiro's and Ian's input in the first comments, there's no
fundamental reason not to support named parameters on Oracle and SQL, it's
just a matter of writing a solid implementation.
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:9>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: msurdi | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: shai@… (added)
* has_patch: 0 => 1
Comment:
Replying to [comment:2 ramiro]:
>
> In fact, Oracle used to have this and it was deleted in [9418] in
response to a ticket (#9478)
For future reference, the relevant ticket is #9408. The code that was
removed indeed did not really support named parameters (except by names
"arg0", "arg1" etc.).
Pull Request 411 supports named parameters for real. No tests provided (it
still isn't a documented feature as far as I know...). I can do the
documentation and add tests if this is deemed required.
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: msurdi | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by anonymous):
I think at very least this should be mentioned explicitly in the docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: msurdi | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: mike@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: msurdi | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by rupa108):
I started a related discussion here
https://groups.google.com/forum/#!topic/django-developers/ts14HX9Yz0I
named parameters do currently work with some db interfaces but the code
breaks on the __repr__() method of a RawQuerySet:
param = dict(lname = 'Doe')
qs = Person.objects.raw('SELECT * FROM myapp_person WHERE last_name =
%(lname)s', param)
repr(qs)
/home/user/vpy/dev/lib/python2.7/site-packages/django/db/models/query.pyc
in __repr__(self)
1530
1531 def __repr__(self):
-> 1532 return "<RawQuerySet: %r>" % (self.raw_query %
tuple(self.params))
1533
1534 def __getitem__(self, k):
TypeError: format requires a mapping
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: msurdi | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Shai Berger <shai@…>):
In [changeset:"d097417025e71286ad5bbde6e0a79caacabbbd64"]:
{{{
#!CommitTicketReference repository=""
revision="d097417025e71286ad5bbde6e0a79caacabbbd64"
Support 'pyformat' style parameters in raw queries, Refs #10070
Add support for Oracle, fix an issue with the repr of RawQuerySet,
add tests and documentations. Also added a 'supports_paramstyle_pyformat'
database feature, True by default, False for SQLite.
Thanks Donald Stufft for review of documentation.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: msurdi | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.0
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by JMGordon):
* cc: JMGordon (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:15>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
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: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 1.0 => master
Comment:
This still breaks on SQLite (3.27.2), although with a less helpful error
message:
{{{
Traceback (most recent call last):
File "<console>", line 1, in <module>
File "/home/rixx/.local/share/virtualenvs/demo/lib/python3.7/site-
packages/django/db/backends/utils.py", line 67, in execute
return self._execute_with_wrappers(sql, params, many=False,
executor=self._execute)
File "/home/rixx/.local/share/virtualenvs/demo/lib/python3.7/site-
packages/django/db/backends/utils.py", line 76, in _execute_with_wrappers
return executor(sql, params, many, context)
File "/home/rixx/.local/share/virtualenvs/demo/lib/python3.7/site-
packages/django/db/backends/utils.py", line 84, in _execute
return self.cursor.execute(sql, params)
File "/home/rixx/.local/share/virtualenvs/demo/lib/python3.7/site-
packages/django/db/utils.py", line 89, in __exit__
raise dj_exc_value.with_traceback(traceback) from exc_value
File "/home/rixx/.local/share/virtualenvs/demo/lib/python3.7/site-
packages/django/db/backends/utils.py", line 84, in _execute
return self.cursor.execute(sql, params)
File "/home/rixx/.local/share/virtualenvs/demo/lib/python3.7/site-
packages/django/db/backends/sqlite3/base.py", line 383, in execute
return Database.Cursor.execute(self, query, params)
django.db.utils.OperationalError: near "%": syntax error
}}}
This syntax isn't documented (as far as I can see, at least not in
`topics/db/sql`) – do we want to add documentation for this, and exclude
SQLite?
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: nobody
Type: Bug | Status: new
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: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Shai Berger):
Replying to [comment:16 Tobias Kunze]:
> This syntax isn't documented (as far as I can see, at least not in
`topics/db/sql`) – do we want to add documentation for this, and exclude
SQLite?
That much I did almost six years ago, as noted in comment:14 -- and it is
still [https://docs.djangoproject.com/en/2.2/topics/db/sql/#passing-
parameters-into-raw there].
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: nobody
Type: Bug | Status: new
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: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Baptiste Mispelon):
I don't know how recent it is, but sqlite does support named parameters,
albeit with a different syntax [1].
Both of these work on my machine:
{{{
#!python
c.execute("select name from inventory_host where id=:id", {'id': '1'})
Host.objects.raw("select * from inventory_host where id=:id", {'id': '1'})
}}}
Do we want to try and make the syntax consistent across all backends or
would it be enough to document the syntax for each one?
[1] https://docs.python.org/3/library/sqlite3.html#sqlite3.Cursor.execute
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: nobody
Type: Bug | Status: new
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: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
> Do we want to try and make the syntax consistent across all backends or
would it be enough to document the syntax for each one?
Given the most common use case of `raw` and `execute` is to perform non
database-agnostic SQL I think documenting the syntax would be fine and
easier to maintain. I ''think'' we already so format massaging for Oracle
though.
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: nobody
Type: Bug | Status: new
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: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Charlie DeTar):
I encountered a use case where I wanted to do an upsert query (`INSERT ...
ON CONFLICT DO UPDATE SET ...`). This is raw SQL that is not supported by
the ORM, but has consistent syntax between sqlite and postgresql. I ran
into this trouble with named parameters. The query syntax itself worked
for both backends, but the parameter naming syntax did not. I worked
around this by just using positional arguments instead -- but there are
valid needs for raw SQL targeting more than one database backend, and
consistent named parameter syntax would be useful.
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:20>
Django
-------------------------------------+-------------------------------------
| Cheley
Type: Bug | 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: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Ryan Cheley
* status: new => assigned
Comment:
I'm at DjangoCon US and I'm looking into this ticket
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: Ryan
| Cheley
Component: Database layer | Version: dev
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Ryan Cheley):
* status: assigned => closed
* resolution: => fixed
Comment:
In looking at the tests in `raw_query\tests` I see a few tests that
already exist:
- `test_params` (name refactored in [Fix camelCase test
names](https://code.djangoproject.com/ticket/22909), added in [Add a
method to the orm to create Model instances from raw sql
queries](https://code.djangoproject.com/ticket/11863))
- `test_params_none` (added [It should be possible to pass None as params
for Model.objects.raw](https://code.djangoproject.com/ticket/32231))
- `test_pyformat_params` (name refactored in [Fix camelCase test
names](https://code.djangoproject.com/ticket/22909), added in [PR
411](https://github.com/django/django/pull/411))
These are already available and **appear** to be testing the very thing
that the ticket is asking for.
Charlie DeTar, is there anything that is being missed here?
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: Ryan
| Cheley
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Matias Surdi):
Thanks for looking into this. I can get back to work now :)
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: Ryan
| Cheley
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
Sorry -- this is still broken on Sqlite. The additions and fixes mentioned
in comment:22 are mostly for Oracle and other backends. The Sqlite
backend, at the time I write this, still has
{{{#!python
supports_paramstyle_pyformat = False
}}}
and still borks with a syntax error if you try to replicate the session in
the ticket description.
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: Ryan
| Cheley
Type: Bug | Status: new
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: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ryan Cheley):
Shai Berger can you confirm which version of SQLite you are using?
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: Ryan
| Cheley
Type: Bug | Status: new
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: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Shai Berger):
Replying to [comment:25 Ryan Cheley]:
> Shai Berger can you confirm which version of SQLite you are using?
{{{#!python
Python 3.10.7 (main, Oct 1 2022, 04:31:04) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sqlite3
>>> sqlite3.sqlite_version
'3.39.4'
}}}
(in the env where I checked)
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: Ryan
| Cheley
Type: Bug | Status: new
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: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ryan Cheley):
I've done a bit of research and it looks like
`django.db.backends.sqlite3.base` has a class called
`SQLiteCursorWrapper`.
This in turn has a method `convert_query` which per the doc string of the
`SQLiteCursorWrapper ` method does:
Django uses "format" style placeholders, but pysqlite2 uses "qmark"
style.
This fixes it -- but note that if you want to use a literal "%s" in a
query,
you'll need to use "%%s".
I think the way to fix the issue is to update that method to include the
following logic:
{{{
query = re.sub("\)s", "", re.sub("%\(", ":", query))
}}}
this will convert the string passed using the expectations of the ORM API
to what SQLite is expecting for parameters, i.e. a passed in query will go
from this
{{{
'select * from content_injuredlist where id = %(id)s and app_name =
%(app_name)s'
}}}
to this
{{{
'select * from content_injuredlist where id = :id and app_name =
:app_name'
}}}
which will then work with SQLite
Still to do:
- Update the method
- Write any tests
- submit PR
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:27>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: Ryan
| Cheley
Type: Bug | Status: new
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: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Shai Berger):
Replying to [comment:27 Ryan Cheley]:
>
> I think the way to fix the issue is to update that method to include the
following logic:
>
>
> {{{
> query = re.sub("\)s", "", re.sub("%\(", ":", query))
> }}}
>
This looks quite fragile (consider, e.g. a query which, for whatever
reason, includes {{{)s}}} in a string constant).
A more robust approach is to create a naming-dict from all the param-
names, and use that to format the original text:
{{{#!python
naming_dict = { param: f":{param}" for param in param_names}
query = query % naming_dict
}}}
Where {{{param_names}}} can be collected either by parsing the query, or
from the parameters given.
(take a look at the Oracle backend -- I wrote that piece too long ago to
remember which it takes, but at the time I knew what I was doing there)
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:28>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: Ryan
| Cheley
Type: Bug | Status: new
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: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
I agree with Shai that we should avoid using regex here.
The most straightforward solution I can think of, I haven't looked at the
Oracle implementation in details, would be to simply implement
`__getitem__`
{{{#!python
class SQLiteParams:
def __getitem__(self, param):
return f":{param}"
sql = sql % SQLiteParams()
}}}
This will ensure the backend reports the low level ''missing param''
message in cases a parameter is missing instead of a `KeyError` during the
Django ''translation'' phase
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:29>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: Ryan
| Cheley
Type: Bug | Status: new
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: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ryan Cheley):
Thanks for the feed back Shai and Simon. I will take a look and see how I
can work that into the solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:30>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: Ryan
| Cheley
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:31>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: Ryan
| Cheley
Type: Bug | 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
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ryan Cheley):
I've implemented the solution as outlined by Shia.
I'll be making changes to the documentation as suggested by Simon here
https://github.com/django/django/pull/16243#pullrequestreview-1161175532
next
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:32>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: Ryan
| Cheley
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:33>
Django
-------------------------------------+-------------------------------------
Reporter: Matias Surdi | Owner: Ryan
| Cheley
Component: Database layer | Version: dev
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"8e6ea1d153a852b83eaa4807301b143df1647a44" 8e6ea1d]:
{{{
#!CommitTicketReference repository=""
revision="8e6ea1d153a852b83eaa4807301b143df1647a44"
Fixed #10070 -- Added support for pyformat style parameters on SQLite.
Co-authored-by: Nick Pope <ni...@nickpope.me.uk>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/10070#comment:34>