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

Parameterization support #16

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

henadzit
Copy link

@henadzit henadzit commented Nov 18, 2024

This PR's main goal is to support query parametrization in tortoise-orm.

  • Wraps query's offset and limit with ValueWrapper so they can be parametrized
  • Refactors Parameter to be dialect-aware. The original version in Pypika requires the user to specify the correct placeholder for the dialect (PyformatParameter, QmarkParameter, etc.) which is contradictory to other features where pypika generates a correct SQL according to the chosen dialect. For tortoise purposes, it is much more convenient to delegate the responsibility of generating correct SQL to pypika as much as possible.
  • Changes the behavior of Parameter.get_sql to avoid doing character escaping, type conversion, etc. if parametrization is used because database drivers usually do those things themselves for parametrized queries. Doing it twice causes subtle errors and extra latency.
  • Introduces Parameterizer which replaces ListParameter, DictParameter, etc. This is mostly a refactoring change since the original code is quite convoluted and has questionable abstractions, e.g. ListParameter isn't really a parameter but a container for values, the parameter argument of get_sql isn't really a parameter but a container for values. This is change is making it harder to pull changes from pypika's upstream but I found it quite hard to use the original code for tortoise purposes.
  • Adds missing whitespace for MSSQL when pagination without ordering.

I'll rebase the branch when this is going to be merged tortoise/tortoise-orm#1776.

@abondar
Copy link
Member

abondar commented Nov 18, 2024

Have you seen conflicts here?
@henadzit

@henadzit
Copy link
Author

Have you seen conflicts here? @henadzit

Yes! I wanted to have it this way for now to be sure that the tortoise CI passes. I'll resolve them shortly!

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.

3 participants