Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not quote SQL params before passing them to mogrify #1832

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

tkoschnick
Copy link
Contributor

Description

SQL params should be adapted to the correct database representation by the corresponding driver's mogrify() for display in the SQL panel. Using the default Python representation works only for simple data types.

Fixes #1831

Checklist:

  • I have added no tests for this change - no new functionality, current tests pass.
  • I have added an item to the Pending section of docs/changes.rst.

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

I have to say I'm surprised that the tests all passed. it seems that the quoting really isn't necessary after all...?

docs/changes.rst Outdated Show resolved Hide resolved
Co-authored-by: Matthias Kestenholz <mk@feinheit.ch>
@tkoschnick
Copy link
Contributor Author

I have to say I'm surprised that the tests all passed. it seems that the quoting really isn't necessary after all...?

I would even argue that it's not only not necessary, but counterproductive. The job of mogrify() is to adapt/quote the query and params exactly as they would be by execute() before being passed to the databases' parser. If all parameters are quoted beforehand, they are presented as strings to mogrify(), which can't possibly adapt them correctly any more.

@tkoschnick
Copy link
Contributor Author

Regarding the tests passing - this is essentially only a presentational change, concerning how the query is displayed to the user in the SQL panel. So far, it has been represented differently from what is actually sent to the database; now it matches, since execute() and mogrify() receive the same parameters.

@tim-schilling
Copy link
Contributor

I don't think @matthiask is surprised that this works for postgres, but rather for mysql and sqlite. The process of getting a executable sql query is different between postgres, mysql and sqlite. I'm very skeptical of this change simply because our test suite doesn't cover every possibility of SQL access that the panel instruments in the wild.

Copy link
Contributor

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

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

I think I'm onboard with this. As far as I can tell the logic checks out and the original reason for the quoting is covered.

@living180
Copy link
Contributor

This change doesn't affect Django's MySQL backend, as it doesn't use the params argument for its last_executed_query() implementation. The sqlite backend on the otherhand does, but I haven't tried that backend with this change.

@tim-schilling
Copy link
Contributor

I ran this locally with sqlite and tested it with an int, string and datetime. It went fine which is why I approved it.

@living180
Copy link
Contributor

It looks like the code in question was originally introduced in #224 (merged January 2012), and appears to have been for MySQL, but Django 1.4 (May 2012) added a last_executed_query() implementation for the MySQL backend (django/django@5b0e4e4) which no longer used the params argument.

@matthiask matthiask merged commit 06c42d9 into jazzband:main Sep 26, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

list values incorrectly adapted in SQL panel
4 participants