Merge pull request #17888 from mrgilman/dot-notation

Refactor `build_from_hash` to convert dot notation to hash first

2 files changed, 40 insertions, 29 deletions

diff --git a/activerecord/lib/active_record/relation/predicate_builder.rb b/activerecord/lib/active_record/relation/predicate_builder.rb
index a6da60b21a..eb21d01465 100644
--- a/activerecord/lib/active_record/relation/predicate_builder.rb
+++ b/activerecord/lib/active_record/relation/predicate_builder.rb
@@ -21,35 +21,8 @@ module ActiveRecord
     end
 
     def build_from_hash(attributes)
-      queries = []
-      builder = self
-
-      attributes.each do |column, value|
-        if value.is_a?(Hash)
-          if value.empty?
-            queries << '1=0'
-          else
-            arel_table = Arel::Table.new(column)
-            association = klass._reflect_on_association(column)
-            builder = self.class.new(association && association.klass, arel_table)
-
-            value.each do |k, v|
-              queries.concat builder.expand(k, v)
-            end
-          end
-        else
-          column = column.to_s
-
-          if column.include?('.')
-            table_name, column = column.split('.', 2)
-            arel_table = Arel::Table.new(table_name)
-            builder = self.class.new(klass, arel_table)
-          end
-          queries.concat builder.expand(column, value)
-        end
-      end
-
-      queries
+      attributes = convert_dot_notation_to_hash(attributes.stringify_keys)
+      expand_from_hash(attributes)
     end
 
     def expand(column, value)
@@ -130,5 +103,37 @@ module ActiveRecord
     protected
 
     attr_reader :klass, :table
+
+    def expand_from_hash(attributes)
+      return ["1=0"] if attributes.empty?
+
+      attributes.flat_map do |key, value|
+        if value.is_a?(Hash)
+          arel_table = Arel::Table.new(key)
+          association = klass._reflect_on_association(key)
+          builder = self.class.new(association && association.klass, arel_table)
+
+          builder.expand_from_hash(value)
+        else
+          expand(key, value)
+        end
+      end
+    end
+
+    private
+
+    def convert_dot_notation_to_hash(attributes)
+      dot_notation = attributes.keys.select { |s| s.include?(".") }
+
+      dot_notation.each do |key|
+        table_name, column_name = key.split(".")
+        value = attributes.delete(key)
+        attributes[table_name] ||= {}
+
+        attributes[table_name] = attributes[table_name].merge(column_name => value)
+      end
+
+      attributes
+    end
   end
 end
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index a607c1319f..f24b30c685 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/activerecord/test/cases/finder_test.rb
@@ -489,6 +489,12 @@ class FinderTest < ActiveRecord::TestCase
     assert_raise(ActiveRecord::RecordNotFound) { Topic.where(topics: { approved: true }).find(1) }
   end
 
+  def test_find_on_combined_explicit_and_hashed_table_names
+    assert Topic.where('topics.approved' => false, topics: { author_name: "David" }).find(1)
+    assert_raise(ActiveRecord::RecordNotFound) { Topic.where('topics.approved' => true, topics: { author_name: "David" }).find(1) }
+    assert_raise(ActiveRecord::RecordNotFound) { Topic.where('topics.approved' => false, topics: { author_name: "Melanie" }).find(1) }
+  end
+
   def test_find_with_hash_conditions_on_joined_table
     firms = Firm.joins(:account).where(:accounts => { :credit_limit => 50 })
     assert_equal 1, firms.size