Parametrize SELECT queries #1777
Conversation
|
|
||
| @classmethod | ||
| def parameterizer(cls) -> Parameterizer: | ||
| return Parameterizer(placeholder_factory=lambda _: "%s") |
There was a problem hiding this comment.
psycopg decided to use non-standard placeholder for Postgres, so we have to account for that.
henadzit
force-pushed
the
feat/parametrize-select-queries-2
branch
2 times, most recently
from
7ad8250
to
ca5e37d
Compare
Pull Request Test Coverage Report for Build 11923213266Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
tests/test_case_when.py
Outdated
| if self.dialect == "mysql": | ||
| expected_sql = "SELECT `intnum` `intnum`,CASE WHEN `intnum`>=%s THEN %s ELSE `intnum_null` END `category` FROM `intfields`" | ||
| elif self.dialect == "postgres": | ||
| if self.is_psycopg: | ||
| expected_sql = 'SELECT "intnum" "intnum",CASE WHEN "intnum">=%s THEN %s ELSE "intnum_null" END "category" FROM "intfields"' | ||
| else: | ||
| expected_sql = 'SELECT "intnum" "intnum",CASE WHEN "intnum">=$1 THEN $2 ELSE "intnum_null" END "category" FROM "intfields"' |
There was a problem hiding this comment.
Shouldn't we add parameters asserts, if we are not checking them inside query text anymore?
There was a problem hiding this comment.
Good point!
I did a bit of research. Django's equivalent of sql() method returns SQL with values in place but the SQL itself might be invalid. So it's basically just for reference when you want to have a quick check what SQL the query generates.
In Ruby on Rails they return SQL with values in place too. I read a few issues in their repo and looks like it's just for reference too.
In SQLAlchemy they return the parametrized query without parameters by default but you can pass an argument to the function to have values in place.
I'm a bit torn on this one. It feels to me that returning a not parametrized query with values in place means that we are returning a different query from the one actually being executed. So I'm inclined to introduce a function argument to sql() to configure whether to return parametrized query or a query with inlined values. WDYT?
There was a problem hiding this comment.
Yeah, I think doing it with parameter is okay, if someone needs it for debug reasons - shouldn't be problem to pass parameter
There was a problem hiding this comment.
Added params_inline arg to QuerySet.sql().
tortoise/queryset.py
Outdated
| parameterizer = pypika_kwargs.pop("parameterizer", self._db.executor_class.parameterizer()) | ||
| return ( | ||
| self.query.get_sql(parameterizer=parameterizer, **pypika_kwargs), | ||
| parameterizer.values, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Why you decided to implement logic of parametrisation in queryset, and not in executor?
If it is about reusing same logic between different backends - it seems to me that it should be no problem to store that logic in base executor
It just seem odd for queryset to get down in such details of query formatting as forming sql string and deriving params from it.
I would imagine queryset as quite abstract entity that doesn't go in such details in making query and pypika query seems to be okay mediator between queryset and executors
There was a problem hiding this comment.
Why you decided to implement logic of parametrisation in queryset, and not in executor?
Honestly, I haven't thought about it before you asked but it's a great question.
I tried to refactor the code to have parametrization as part of executors and realized a few things:
- The executor interface will need to be extended with a method to return SQL, parametrized or not
- When using
Subqueryand parametrizing the query, pypika's keywords are passed from the parent query, so they will need to be passed to the executor too, basically, making the the logic a bit more complicated - I tried to see how we can have better abstractions but honestly right now pypika is all over the place.
QuerySetmodifies private fields of pypika's query and executor builds its own query, etc. So this PR doesn't change the situation by much.
However, I moved the parametrization code to a private method which doesn't seem't seem to be complicated, it's 5 lines of code - please see _parametrize_query
I'm kinda inclined to think that it's Okay to keep parametrization as part of queryset but please let me know what you think!
henadzit
force-pushed
the
feat/parametrize-select-queries-2
branch
from
ca5e37d
to
0d2fe63
Compare
Description
This PR:
.count(),exists(),.values(),.values_list()). It does NOT implement parametrization of UPDATE and DELETE queries. I'll work on UPDATE and DELETE parametrization as part of a separate PR since this one is already quite big.AwaitableQuery._executeuses the same SQL as returned byAwaitableQuery.sql()by changing the signatures of_executeand_make_query.This PR is dependent on changes in pypika-tortoise tortoise/pypika-tortoise#16.⚠️ we should merge them first and release a new version of
pypika-tortoise!Motivation and Context
Parameterized queries are crucial for preventing SQL injection attacks and but also can improve performance of database operations.
How Has This Been Tested?
Checklist: