-
Notifications
You must be signed in to change notification settings - Fork 1.1k
do not quote SQL params before passing them to mogrify #1832
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
Conversation
There was a problem hiding this 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...?
Co-authored-by: Matthias Kestenholz <[email protected]>
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. |
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. |
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. |
There was a problem hiding this 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.
This change doesn't affect Django's MySQL backend, as it doesn't use the |
I ran this locally with sqlite and tested it with an int, string and datetime. It went fine which is why I approved it. |
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 |
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:
docs/changes.rst
.