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

Arel/AR not replacing binds correct? #13887

Closed
arthurnn opened this issue Jan 30, 2014 · 7 comments
Closed

Arel/AR not replacing binds correct? #13887

arthurnn opened this issue Jan 30, 2014 · 7 comments

Comments

@arthurnn
Copy link
Member

Using this script:
https://gist.github.com/arthurnn/8702117

mysql output:

"SELECT `posts`.`title` FROM `posts`  WHERE `posts`.`author_id` = 1"

postgres output:

"SELECT \"posts\".\"title\" FROM \"posts\"  WHERE \"posts\".\"author_id\" = $1"

sqlite3 output:

"SELECT \"posts\".\"title\" FROM \"posts\"  WHERE \"posts\".\"author_id\" = ?"

Is this correct? (AFAIK, the ? and $1 should had been replaced)
author.posts.select(:title) returns a Relation, which calls to_sql https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation.rb#L510 . So that means that the binds are in place, so that might be an issue on AREL? (if it is a AREL thing, I can open a issue in there)

maybe related #12753 ?

thoughts @tenderlove @rafaelfranca @carlosantoniodasilva

@rafaelfranca
Copy link
Member

This is correct. Who finally replace the binds are the database adapter when doing the query.

@arthurnn
Copy link
Member Author

good point =)

@arthurnn
Copy link
Member Author

So, select_row should receive the binds, in case we have these queries. see https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L48

@rafaelfranca
Copy link
Member

See adequaterecord branch for adequate reason. If we replace the binds before the database connection call we would not have how to cache the prepared statements.

That branch also explain why the to_sql implementation of Relation can't use the database adapter version

@arthurnn
Copy link
Member Author

Got it... everything is clear now...
So select_row should receive the query(without replacing the binds), and the bind array?

@rafaelfranca
Copy link
Member

Yes, it should.

@arthurnn
Copy link
Member Author


will do that on #13886 ... And remote the to_sql refactoring.

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

No branches or pull requests

2 participants