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

Relation#where build BoundSqlLiteral rather than eagerly interpolate #51139

Merged
merged 1 commit into from Feb 21, 2024

Conversation

casperisfine
Copy link
Contributor

@casperisfine casperisfine commented Feb 20, 2024

Ref: #50793

To make not caching connection checkout viable, we need to reduced the amount of places where we need a connection.

Once big source of this is query/relation building, where in many cases it eagerly quote and interpolation bound values in SQL fragments.

Doing this requires an active connection because both MySQL and Postgres may quote values differently based on the connection settings.

Instead of eagerly doing all this, we can instead just insert these as bound values in the Arel AST. For adapters with prepared statements this is better anyway as it will avoid leaking statements, and for those that don't support it, it will simply delay the quoting to just before the query is executed.

However, the % API (where("title = %s", something)) can't realistically
be fixed this way, but I don't see much value in it and it probably should
be deprecated and removed.

Also CC @matthewd because I noticed #46600 so it look like you were headed in this same direction? Perhaps you have some insights to share.

casperisfine pushed a commit to Shopify/rails that referenced this pull request Feb 20, 2024
Ref: rails#51139

Makes it more clear that the expected behavior really is.
casperisfine pushed a commit to Shopify/rails that referenced this pull request Feb 20, 2024
Ref: rails#51139

Makes it more clear that the expected behavior really is.
@matthewd
Copy link
Member

Here's a quick & dirty rebase of my stash from when I was working on this: main...matthewd:rails:assemble-sql

I don't really remember what state it was in at the time, sorry, but I'm guessing it was something other than "usable" -- at most it might be interesting as a sketch of the approach that I had in my head.

@casperisfine casperisfine force-pushed the relation-bound-sql-literal branch 2 times, most recently from da28b5e to 27facb9 Compare February 20, 2024 15:10
@casperisfine
Copy link
Contributor Author

Here's a quick & dirty rebase of my stash from when I was working on this

Thanks for the link. There's definitely some resemblance with my PR, especially compile_sql_array, but the connection.to_sql(compile_sql_array(ary)) in sanitize_sql_array surprise me a bit. Perhaps this was a way to slowly start defering the compilation, and not the end goal, in which case, it's indeed the same idea.

@casperisfine casperisfine force-pushed the relation-bound-sql-literal branch 2 times, most recently from 1a144e3 to 03e8f44 Compare February 20, 2024 15:19
@matthewd
Copy link
Member

IIRC the intention was to move all internals to compile, and leave sanitize as existing public API (at least pending deprecation)

@casperisfine
Copy link
Contributor Author

IIRC the intention was to move all internals to compile, and leave sanitize as existing public API

I see. So just a cleanup / consolidation. Deferring the quoting wasn't a concern. That helps!

BTW, I'm on the fence between merging this pretty much as is, or trying to also support the named binds in the same PR.

@casperisfine casperisfine marked this pull request as ready for review February 20, 2024 15:43
@casperisfine
Copy link
Contributor Author

Alright, It now also handle :name interpolation, and fragments without interpolation at all.

The % interpolation still delegate to sanitize_sql, but unless someone has a creative idea how to handle it, I think it just can't be fixed. But it's also a very questionable API, which I think should probably be deprecated (here again, maybe I'm missing a use case?).

@casperisfine casperisfine force-pushed the relation-bound-sql-literal branch 2 times, most recently from dfcddbb to 679a6c6 Compare February 21, 2024 09:50
Ref: rails#50793

To make not caching connection checkout viable, we need to reduced
the amount of places where we need a connection.

Once big source of this is query/relation building, where in many
cases it eagerly quote and interpolation bound values in SQL fragments.

Doing this requires an active connection because both MySQL and Postgres
may quote values differently based on the connection settings.

Instead of eagerly doing all this, we can instead just insert these
as bound values in the Arel AST. For adapters with prepared statements
this is better anyway as it will avoid leaking statements, and for those
that don't support it, it will simply delay the quoting to just
before the query is executed.

However, the `%` API (`where("title = %s", something)`) can't realistically
be fixed this way, but I don't see much value in it and it probably should
be deprecated and removed.
@byroot byroot merged commit 684131a into rails:main Feb 21, 2024
4 checks passed
Ridhwana pushed a commit to Ridhwana/rails that referenced this pull request Mar 7, 2024
Ref: rails#51139

Makes it more clear that the expected behavior really is.
viralpraxis pushed a commit to viralpraxis/rails that referenced this pull request Mar 24, 2024
Ref: rails#51139

Makes it more clear that the expected behavior really is.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants