From 10f75af9330c0694a233b856057d0ee453f19e42 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Fri, 31 Oct 2014 09:43:38 -0600 Subject: [PATCH] Use bind values for joined tables in where statements In practical terms, this allows serialized columns and tz aware columns to be used in wheres that go through joins, where they previously would not behave correctly. Internally, this removes 1/3 of the cases where we rely on Arel to perform type casting for us. There were two non-obvious changes required for this. `update_all` on relation was merging its bind values with arel's in the wrong order. Additionally, through associations were assuming there would be no bind parameters in the preloader (presumably because the where would always be part of a join) [Melanie Gilman & Sean Griffin] --- activerecord/CHANGELOG.md | 5 +++++ .../preloader/through_association.rb | 1 + activerecord/lib/active_record/relation.rb | 2 +- .../lib/active_record/relation/query_methods.rb | 17 +++++++++++++++++ activerecord/test/cases/serialization_test.rb | 9 +++++++++ activerecord/test/models/author.rb | 1 + activerecord/test/models/post.rb | 4 ++++ activerecord/test/schema/schema.rb | 5 +++++ 8 files changed, 43 insertions(+), 1 deletion(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index ad4ee0f489..ab85f2a472 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,8 @@ +* Queries now properly type cast values that are part of a join statement, + even when using type decorators such as `serialize`. + + *Melanie Gilman & Sean Griffin* + * MySQL enum type lookups, with values matching another type, no longer result in an endless loop. diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb index d57da366bd..12bf3ef138 100644 --- a/activerecord/lib/active_record/associations/preloader/through_association.rb +++ b/activerecord/lib/active_record/associations/preloader/through_association.rb @@ -81,6 +81,7 @@ def through_scope unless reflection_scope.where_values.empty? scope.includes_values = Array(reflection_scope.values[:includes] || options[:source]) scope.where_values = reflection_scope.values[:where] + scope.bind_values = reflection_scope.bind_values end scope.references! reflection_scope.values[:references] diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 7d4a008f44..a25e6e321f 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -336,7 +336,7 @@ def update_all(updates) stmt.wheres = arel.constraints end - bvs = bind_values + arel.bind_values + bvs = arel.bind_values + bind_values @klass.connection.update stmt, 'SQL', bvs end diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index d8d416416e..619dfbcc9f 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -980,6 +980,10 @@ def create_binds(opts) end end + association_binds, non_binds = non_binds.partition do |column, value| + value.is_a?(Hash) && association_for_table(column) + end + new_opts = {} binds = [] @@ -988,11 +992,24 @@ def create_binds(opts) new_opts[column] = connection.substitute_at(column) end + association_binds.each do |(column, value)| + association_relation = association_for_table(column).klass.send(:relation) + association_new_opts, association_bind = association_relation.send(:create_binds, value) + new_opts[column] = association_new_opts + binds += association_bind + end + non_binds.each { |column,value| new_opts[column] = value } [new_opts, binds] end + def association_for_table(table_name) + table_name = table_name.to_s + @klass._reflect_on_association(table_name) || + @klass._reflect_on_association(table_name.singularize) + end + def build_from opts, name = from_value case opts diff --git a/activerecord/test/cases/serialization_test.rb b/activerecord/test/cases/serialization_test.rb index 3f52e80e11..35b13ea247 100644 --- a/activerecord/test/cases/serialization_test.rb +++ b/activerecord/test/cases/serialization_test.rb @@ -2,6 +2,8 @@ require 'models/contact' require 'models/topic' require 'models/book' +require 'models/author' +require 'models/post' class SerializationTest < ActiveRecord::TestCase fixtures :books @@ -92,4 +94,11 @@ def test_read_attribute_for_serialization_with_format_after_find book = klazz.find(books(:awdr).id) assert_equal 'paperback', book.read_attribute_for_serialization(:format) end + + def test_find_records_by_serialized_attributes_through_join + author = Author.create!(name: "David") + author.serialized_posts.create!(title: "Hello") + + assert_equal 1, Author.joins(:serialized_posts).where(name: "David", serialized_posts: { title: "Hello" }).length + end end diff --git a/activerecord/test/models/author.rb b/activerecord/test/models/author.rb index 3f34d09a04..3da3a1fd59 100644 --- a/activerecord/test/models/author.rb +++ b/activerecord/test/models/author.rb @@ -1,5 +1,6 @@ class Author < ActiveRecord::Base has_many :posts + has_many :serialized_posts has_one :post has_many :very_special_comments, :through => :posts has_many :posts_with_comments, -> { includes(:comments) }, :class_name => "Post" diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb index 67027cbc22..36cf221d45 100644 --- a/activerecord/test/models/post.rb +++ b/activerecord/test/models/post.rb @@ -233,3 +233,7 @@ class PostWithCommentWithDefaultScopeReferencesAssociation < ActiveRecord::Base has_many :comment_with_default_scope_references_associations, foreign_key: :post_id has_one :first_comment, class_name: "CommentWithDefaultScopeReferencesAssociation", foreign_key: :post_id end + +class SerializedPost < ActiveRecord::Base + serialize :title +end diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 2b68236256..539a0d2a49 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -585,6 +585,11 @@ def except(adapter_names_to_exclude) t.integer :tags_with_nullify_count, default: 0 end + create_table :serialized_posts, force: true do |t| + t.integer :author_id + t.string :title, null: false + end + create_table :price_estimates, force: true do |t| t.string :estimate_of_type t.integer :estimate_of_id -- GitLab