Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix assumption of primary key name in PredicateBuilder subquery. #2602

Merged
merged 2 commits into from

2 participants

@ernie

In looking through the current implementation of PredicateBuilder, I caught an assumption that a model's primary key is "id" in the automatically-added select clause. This is just a quick fix to honor the primary_key if an alternate is specified.

Also, while looking at that first commit, I noticed we are clobbering select_values in the incoming relation, so that's fixed, too.

@tenderlove
Owner

Great! Thanks @ernie! :-)

@tenderlove tenderlove merged commit 257366d into rails:master
@tenderlove
Owner

@ernie would you mind filing backport pull requests for 3-1-stable and 3-0-stable? :heart:

@ernie

Sure thing! I can do 3-1-stable for sure, but I don't think 3-0-stable does subqueries, at all, right now:

          case value
          when Array, ActiveRecord::Associations::AssociationCollection, ActiveRecord::Relation
            values = value.to_a.map { |x|
              x.is_a?(ActiveRecord::Base) ? x.id : x
            }
            attribute.in(values)
@tenderlove
Owner

Ya, I just said 3-0-stable because I was too lazy to look if it needed it. ;-)

@tenderlove tenderlove referenced this pull request from a commit
@tenderlove tenderlove Merge branch '3-1-stable' into 3-1-0
* 3-1-stable: (21 commits)
  Added a test to check for correct behaviour with no options in add_index command recorder
  Using .try to test for the existence of a method option in a nil-resistent manner. Inlined the determination of the options hash for reversing using a ternary operator. Shortens the method in a way that keeps the code neat
  Neatened up the invert_add_index method as per suggeston
  Simple fix for correctly inverting an add_index migration when a name has been provided
  Merge pull request #2524 from JonathonMA/fix_ecd37084b28a05f05251
  Fix Sprockets rewrite_asset_path
  Merge pull request #2620 from cesario/3-1-0
  Fix sprockets warnings
  Use typewriter styling on url_for in documentation
  Merge pull request #2609 from guilleiguaran/bump-sprockets-beta14
  remove extra space since comment_if already returns the space. (cherry picked from commit 5e73a2f)
  Merge pull request #2596 from dharmatech/patch-1
  Merge pull request #2595 from smartinez87/assets-guide
  Merge pull request #2597 from dharmatech/patch-2
  Merge pull request #2604 from vijaydev/params_wrapper_docs
  Merge pull request #2602 from ernie/fix_predicate_builder_primary_key_assumption
  Merge pull request #2603 from vijaydev/guides
  Merge pull request #2581 from guilleiguaran/debug-assets-in-dev
  Revert "Deprecate the use of non-public methods by Module#delegate"
  mailer guide: fixes indentation, and use fixed width fonts wherever necessary
  ...
1f1d4aa
@ttosch ttosch referenced this pull request from a commit
@tenderlove tenderlove Merge branch '3-1-stable' into 3-1-0
* 3-1-stable: (21 commits)
  Added a test to check for correct behaviour with no options in add_index command recorder
  Using .try to test for the existence of a method option in a nil-resistent manner. Inlined the determination of the options hash for reversing using a ternary operator. Shortens the method in a way that keeps the code neat
  Neatened up the invert_add_index method as per suggeston
  Simple fix for correctly inverting an add_index migration when a name has been provided
  Merge pull request #2524 from JonathonMA/fix_ecd37084b28a05f05251
  Fix Sprockets rewrite_asset_path
  Merge pull request #2620 from cesario/3-1-0
  Fix sprockets warnings
  Use typewriter styling on url_for in documentation
  Merge pull request #2609 from guilleiguaran/bump-sprockets-beta14
  remove extra space since comment_if already returns the space. (cherry picked from commit 5e73a2f)
  Merge pull request #2596 from dharmatech/patch-1
  Merge pull request #2595 from smartinez87/assets-guide
  Merge pull request #2597 from dharmatech/patch-2
  Merge pull request #2604 from vijaydev/params_wrapper_docs
  Merge pull request #2602 from ernie/fix_predicate_builder_primary_key_assumption
  Merge pull request #2603 from vijaydev/guides
  Merge pull request #2581 from guilleiguaran/debug-assets-in-dev
  Revert "Deprecate the use of non-public methods by Module#delegate"
  mailer guide: fixes indentation, and use fixed width fonts wherever necessary
  ...
1683ba2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
2  activerecord/lib/active_record/relation/predicate_builder.rb
@@ -19,7 +19,7 @@ def self.build_from_hash(engine, attributes, default_table)
case value
when ActiveRecord::Relation
- value.select_values = [value.klass.arel_table['id']] if value.select_values.empty?
+ value = value.select(value.klass.arel_table[value.klass.primary_key]) if value.select_values.empty?
attribute.in(value.arel.ast)
when Array, ActiveRecord::Associations::CollectionProxy
values = value.to_a.map { |x|
View
23 activerecord/test/cases/relations_test.rb
@@ -540,6 +540,29 @@ def test_find_all_using_where_with_relation
}
end
+ def test_find_all_using_where_with_relation_and_alternate_primary_key
+ cool_first = minivans(:cool_first)
+ # switching the lines below would succeed in current rails
+ # assert_queries(2) {
+ assert_queries(1) {
+ relation = Minivan.where(:minivan_id => Minivan.where(:name => cool_first.name))
+ assert_equal [cool_first], relation.all
+ }
+ end
+
+ def test_find_all_using_where_with_relation_does_not_alter_select_values
+ david = authors(:david)
+
+ subquery = Author.where(:id => david.id)
+
+ assert_queries(1) {
+ relation = Author.where(:id => subquery)
+ assert_equal [david], relation.all
+ }
+
+ assert_equal 0, subquery.select_values.size
+ end
+
def test_find_all_using_where_with_relation_with_joins
david = authors(:david)
assert_queries(1) {
Something went wrong with that request. Please try again.