Merge pull request rails#46048 from rails/fix-hash-collisions

Dup and freeze complex types when making query attributes

Author: byroot

activemodel/lib/active_model/type/helpers/mutable.rb

@@ -4,6 +4,10 @@ module ActiveModel
   module Type
     module Helpers # :nodoc: all
       module Mutable
+        def immutable_value(value)
+          value.deep_dup.freeze
+        end
+
         def cast(value)
           deserialize(serialize(value))
         end

activemodel/lib/active_model/type/value.rb

@@ -129,6 +129,10 @@ def hash
       def assert_valid_value(_)
       end
 
+      def immutable_value(value) # :nodoc:
+        value
+      end
+
       private
         # Convenience method for types which do not need separate type casting
         # behavior for user and database inputs. Called by Value#cast for

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Fix a case where the query cache can return wrong values. See #46044
+
+    *Aaron Patterson*
+
 *   Support MySQL's ssl-mode option for MySQLDatabaseTasks.
 
     Verifying the identity of the database server requires setting the ssl-mode

activerecord/lib/active_record/relation/predicate_builder.rb

@@ -65,7 +65,8 @@ def build(attribute, value, operator = nil)
     end
 
     def build_bind_attribute(column_name, value)
-      Relation::QueryAttribute.new(column_name, value, table.type(column_name))
+      type = table.type(column_name)
+      Relation::QueryAttribute.new(column_name, type.immutable_value(value), type)
     end
 
     def resolve_arel_attribute(table_name, column_name, &block)

activerecord/test/cases/query_cache_test.rb

@@ -646,6 +646,51 @@ def assert_cache(state, connection = ActiveRecord::Base.connection)
     end
 end
 
+class QueryCacheMutableParamTest < ActiveRecord::TestCase
+  self.use_transactional_tests = false
+
+  class JsonObj < ActiveRecord::Base
+    self.table_name = "json_objs"
+
+    attribute :payload, :json
+  end
+
+  class HashWithFixedHash < Hash
+    # this isn't very realistic, but it is the worst case and therefore a good
+    # case to test
+    def hash
+      1
+    end
+  end
+
+  def setup
+    ActiveRecord::Base.connection.create_table("json_objs", force: true) do |t|
+      if current_adapter?(:PostgreSQLAdapter)
+        t.jsonb "payload"
+      else
+        t.json "payload"
+      end
+    end
+
+    ActiveRecord::Base.connection.enable_query_cache!
+  end
+
+  def test_query_cache_handles_mutated_binds
+    JsonObj.create(payload: { a: 1 })
+
+    search = HashWithFixedHash[a: 1]
+    JsonObj.where(payload: search).first # populate the cache
+
+    search.merge!(b: 2)
+    assert_nil JsonObj.where(payload: search).first, "cache returned a false positive"
+  end
+
+  def teardown
+    ActiveRecord::Base.connection.disable_query_cache!
+    ActiveRecord::Base.connection.drop_table("json_objs", if_exists: true)
+  end
+end
+
 class QueryCacheExpiryTest < ActiveRecord::TestCase
   fixtures :tasks, :posts, :categories, :categories_posts