Skip to content

Fixed #10070 -- Added support for pyformat style parameters on SQLite.#16243

Merged
felixxm merged 1 commit intodjango:mainfrom
ryancheley:ticket_10070
Nov 8, 2022
Merged

Fixed #10070 -- Added support for pyformat style parameters on SQLite.#16243
felixxm merged 1 commit intodjango:mainfrom
ryancheley:ticket_10070

Conversation

@ryancheley
Copy link
Copy Markdown
Member

@ryancheley ryancheley commented Oct 30, 2022

Fix to allow named parameters on raw sql queries with sqlite

charettes
charettes reviewed Oct 30, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@charettes charettes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for patch Ryan!

We'll want to adjust the docs added in d097417 to remove the mention of SQLite exception

django/docs/ref/databases.txt

Lines 822 to 828 in 970f61f

"pyformat" parameter style in raw queries not supported
-------------------------------------------------------
For most backends, raw queries (``Manager.raw()`` or ``cursor.execute()``)
can use the "pyformat" parameter style, where placeholders in the query
are given as ``'%(name)s'`` and the parameters are passed as a dictionary
rather than a list. SQLite does not support this.

@ryancheley ryancheley mentioned this pull request Nov 2, 2022
Closed
@ryancheley
Copy link
Copy Markdown
Member Author

ryancheley commented Nov 2, 2022

Looks like some tests for pyformat are failing and perhaps the initial change doesn't indicate an actual support of pyformat. I'll be looking into that next

@felixxm
Copy link
Copy Markdown
Member

felixxm commented Nov 2, 2022

Looks like some tests for pyformat are failing and perhaps the initial change doesn't indicate an actual support of pyformat. I'll be looking into that next

It works, but the same is needed for executemany().

@ryancheley ryancheley marked this pull request as draft November 3, 2022 13:19
@ryancheley
Copy link
Copy Markdown
Member Author

ryancheley commented Nov 3, 2022

@charettes @felixxm I’m not sure why the tests are failing here.

When I run it locally the tests pass (macOS 12.6 Monterey virtual environment with Python 3.10.2) so I’m kind of at a loss as to what to do ( I did forget to run the linters on my code locally but have done that now …. but I don’t want to push up another change until I can try and figure out why the tests aren’t passing in CI)

Also, the code as submitted work (locally at least) but I think it can be refactored though I’m not really sure how.

Any input you have on it would be welcome!

@charettes
Copy link
Copy Markdown
Member

charettes commented Nov 3, 2022

@ryancheley the postgres test failure is a fluke that isn't related to your changes. It's safe to ignore it and focus on the linting failures.

felixxm
felixxm reviewed Nov 3, 2022
View reviewed changes
ngnpope
ngnpope suggested changes Nov 3, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ngnpope ngnpope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of issues with the approach:

  • There is duplication which should be pushed down into .convert_query().
  • Converting %(value)s to :value in this way will also convert %%s to %s which will then be incorrectly converted to ? in .convert_query(). We should not perform both conversions1 and a test should be added to ensure this doesn't happen.
  • Converting param_list to a list in .executemany() will saddle us with poor performance. We many have been passed a generator and the current approach will force all of the parameters to be materialized. The approach needed requires that we can peek at the first item in the generator without consuming the lot.

I'll provide a PR to your branch so that you can see what I mean.

Footnotes

  1. We're either converting format paramstyle to qmark or the pyformat paramstyle to named. Note that the sqlite3 documentation only mentions qmark and numeric, but the SQLite documentation indicates that named is also supported. See also the relevant section in PEP 249. I'll try to get the sqlite3 documentation updated. ↩

@ngnpope ngnpope mentioned this pull request Nov 3, 2022
Merged
@ngnpope
Copy link
Copy Markdown
Member

ngnpope commented Nov 3, 2022 •
edited
Loading

I'll provide a PR to your branch so that you can see what I mean.

See ryancheley#1.

I'll try to get the sqlite3 documentation updated.

See python/cpython#99078.

@ryancheley ryancheley marked this pull request as ready for review November 4, 2022 10:41
@ryancheley ryancheley mentioned this pull request Nov 5, 2022
Merged
@ryancheley
Copy link
Copy Markdown
Member Author

ryancheley commented Nov 8, 2022

@felixxm is there anything left for me to do with this PR? If so, please let me know. 😁

@felixxm felixxm changed the title Fixed #10070 Fixed #10070 -- Added support for pyformat style parameters on SQLite. Nov 8, 2022
@felixxm
Copy link
Copy Markdown
Member

felixxm commented Nov 8, 2022

@ryancheley Thanks 👍

I simplified comments.

@felixxm felixxm force-pushed the ticket_10070 branch 3 times, most recently from 5200b94 to 54ea9fd Compare November 8, 2022 09:51
@ngnpope ngnpope mentioned this pull request Nov 8, 2022
Merged
@felixxm felixxm merged commit 8e6ea1d into django:main Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixxm felixxm felixxm left review comments

+2 more reviewers

@charettes charettes charettes left review comments

@ngnpope ngnpope ngnpope requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ryancheley @felixxm @charettes @ngnpope