Fix regression on .select method #13886
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -20,6 +20,14 @@ def to_sql(arel, binds = []) | |||
|
|
|||
| # Returns an ActiveRecord::Result instance. | |||
| def select_all(arel, name = nil, binds = []) | |||
| if arel.is_a?(ActiveRecord::AssociationRelation) | |||
There was a problem hiding this comment.
Should not we check for Relation only?
There was a problem hiding this comment.
good point... my tests cases were always returning a AssociationRelation. I can change that.. any test test case in mind that I can create a Relation and pass to this method?
|
Great work there @arthurnn! Could you please add a changelog entry and squash your commits ? |
|
Sure thing @robin850 ... 💚 |
| @@ -1,3 +1,11 @@ | |||
| * Fix regressions on `select_*` methods. | |||
| When `select_*` methods receive a `Relation` object, they should be able to get the arel/binds from it. | |||
| Also this fixes regressions on select_rows ignoring the binds. | |||
There was a problem hiding this comment.
To make it consistent with the first sentence, we could simply put Also fix regressions on ..., what do you think ? Also, I think that the indentation is weird on this line.
There was a problem hiding this comment.
make sense... will fix it...
emacs is messing my indentation =)
This was a common pattern: ``` query = author.posts.select(:title) connection.select_one(query) ``` However `.select` returns a ActiveRecord::AssociationRelation, which has the bind information, so we can use that to get the right sql query. Also fix select_rows on postgress and sqlite3 that were not using the binds [fixes rails#7538] [fixes rails#12017] [related rails#13731] [related rails#12056]
rafaelfranca
added a commit
that referenced
this pull request
Jan 30, 2014
Fix regression on .select method
rafaelfranca
added a commit
that referenced
this pull request
Jan 30, 2014
Fix regression on .select method Conflicts: activerecord/CHANGELOG.md activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb activerecord/test/cases/adapter_test.rb
yahonda
added a commit
to yahonda/oracle-enhanced
that referenced
this pull request
Feb 10, 2014
yahonda
added a commit
to rsim/oracle-enhanced
that referenced
this pull request
Feb 10, 2014
Support rails/rails#13886 by chainging select_rows arguments
tdooner
pushed a commit
to causes/rails
that referenced
this pull request
Feb 21, 2014
This fixes a regression that is causing a bug in our lib/advanced_batches.rb The original commit message: Fix regression on .select method
arthurnn
added a commit
to arthurnn/rails
that referenced
this pull request
Mar 13, 2014
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
That code wont work anymore on rails 4. And it used to work on rails 3.
Why
selectmethod, returns aActiveRecord::AssociationRelationand the connection is not expecting that object.Solution
If the argument is a
ActiveRecord::AssociationRelation, get the arel and binds from it.review @rafaelfranca @dmathieu
See this example that fails on AR 4 and not on 3:
https://gist.github.com/arthurnn/8701203
[fixes #7538]
[fixes #12017]
[related #13731]