Merge pull request #45221 from jhawthorn/ac_params_eql_fix

Fix eql? of AC::Parameters to match hash

actionpack/lib/action_controller/metal/strong_parameters.rb

@@ -284,10 +284,15 @@ def ==(other)
         end
       end
     end
-    alias eql? ==
+
+    def eql?(other)
+      self.class == other.class &&
+        permitted? == other.permitted? &&
+        parameters.eql?(other.parameters)
+    end
 
     def hash
-      [@parameters.hash, @permitted].hash
+      [self.class, @parameters, @permitted].hash
     end
 
     # Returns a safe <tt>ActiveSupport::HashWithIndifferentAccess</tt>

actionpack/test/controller/parameters/accessors_test.rb

@@ -97,22 +97,6 @@ class ParametersAccessorsTest < ActiveSupport::TestCase
     assert_equal @params, ActionController::Parameters.new(@params.each_pair.to_h)
   end
 
-  test "deprecated comparison works" do
-    assert_kind_of Enumerator, @params.each_pair
-    assert_deprecated do
-      assert_equal @params, @params.each_pair.to_h
-    end
-  end
-
-  test "deprecated comparison disabled" do
-    without_deprecated_params_hash_equality do
-      assert_kind_of Enumerator, @params.each_pair
-      assert_not_deprecated do
-        assert_not_equal @params, @params.each_pair.to_h
-      end
-    end
-  end
-
   test "each_value carries permitted status" do
     @params.permit!
     @params.each_value do |value|
@@ -418,28 +402,4 @@ class ParametersAccessorsTest < ActiveSupport::TestCase
     @params.dig(:person, :addresses)[0] = { city: "Boston", state: "Massachusetts" }
     assert_equal "Boston", @params.dig(:person, :addresses, 0, :city)
   end
-
-  test "has_value? converts hashes to parameters" do
-    assert_not_deprecated do
-      params = ActionController::Parameters.new(foo: { bar: "baz" })
-      assert params.has_value?("bar" => "baz")
-      params[:foo] # converts value to AC::Params
-      assert params.has_value?("bar" => "baz")
-    end
-  end
-
-  test "has_value? works with parameters" do
-    without_deprecated_params_hash_equality do
-      params = ActionController::Parameters.new(foo: { bar: "baz" })
-      assert params.has_value?(ActionController::Parameters.new("bar" => "baz"))
-    end
-  end
-
-  private
-    def without_deprecated_params_hash_equality
-      ActionController::Parameters.allow_deprecated_parameters_hash_equality = false
-      yield
-    ensure
-      ActionController::Parameters.allow_deprecated_parameters_hash_equality = true
-    end
 end

actionpack/test/controller/parameters/equality_test.rb

@@ -0,0 +1,87 @@
+# frozen_string_literal: true
+
+require "abstract_unit"
+require "action_controller/metal/strong_parameters"
+
+class ParametersAccessorsTest < ActiveSupport::TestCase
+  setup do
+    ActionController::Parameters.permit_all_parameters = false
+
+    @params = ActionController::Parameters.new(
+      person: {
+        age: "32",
+        name: {
+          first: "David",
+          last: "Heinemeier Hansson"
+        },
+        addresses: [{ city: "Chicago", state: "Illinois" }]
+      }
+    )
+  end
+
+  test "deprecated comparison works" do
+    @hash = @params.each_pair.to_h
+    assert_deprecated do
+      assert_equal @params, @hash
+    end
+  end
+
+  test "deprecated comparison disabled" do
+    without_deprecated_params_hash_equality do
+      @hash = @params.each_pair.to_h
+      assert_not_deprecated do
+        assert_not_equal @params, @hash
+      end
+    end
+  end
+
+  test "not eql? to equivalent hash" do
+    @hash = {}
+    @params = ActionController::Parameters.new(@hash)
+    assert_not_deprecated do
+      assert_not @params.eql?(@hash)
+    end
+  end
+
+  test "not eql? to equivalent nested hash" do
+    @params1 = ActionController::Parameters.new({ foo: {} })
+    @params2 = ActionController::Parameters.new({ foo: ActionController::Parameters.new({}) })
+    assert_not_deprecated do
+      assert_not @params1.eql?(@params2)
+    end
+  end
+
+  test "not eql? when permitted is different" do
+    permitted = @params.permit(:person)
+    assert_not @params.eql?(permitted)
+  end
+
+  test "eql? when equivalent" do
+    permitted = @params.permit(:person)
+    assert @params.permit(:person).eql?(permitted)
+  end
+
+  test "has_value? converts hashes to parameters" do
+    assert_not_deprecated do
+      params = ActionController::Parameters.new(foo: { bar: "baz" })
+      assert params.has_value?("bar" => "baz")
+      params[:foo] # converts value to AC::Params
+      assert params.has_value?("bar" => "baz")
+    end
+  end
+
+  test "has_value? works with parameters" do
+    without_deprecated_params_hash_equality do
+      params = ActionController::Parameters.new(foo: { bar: "baz" })
+      assert params.has_value?(ActionController::Parameters.new("bar" => "baz"))
+    end
+  end
+
+  private
+    def without_deprecated_params_hash_equality
+      ActionController::Parameters.allow_deprecated_parameters_hash_equality = false
+      yield
+    ensure
+      ActionController::Parameters.allow_deprecated_parameters_hash_equality = true
+    end
+end

actionpack/test/controller/parameters_integration_test.rb

@@ -6,6 +6,14 @@ class IntegrationController < ActionController::Base
   def yaml_params
     render plain: params.to_yaml
   end
+
+  def permit_params
+    params.permit(
+      key1: {}
+    )
+
+    render plain: "Home"
+  end
 end
 
 class ActionControllerParametersIntegrationTest < ActionController::TestCase
@@ -24,4 +32,20 @@ class ActionControllerParametersIntegrationTest < ActionController::TestCase
     YAML
     assert_equal expected, response.body
   end
+
+  # Ensure no deprecation warning from comparing AC::Parameters against Hash
+  # See https://github.com/rails/rails/issues/44940
+  test "identical arrays can be permitted" do
+    params = {
+      key1: {
+        a: [{ same_key: { c: 1 } }],
+        b: [{ same_key: { c: 1 } }]
+      }
+    }
+
+    assert_not_deprecated do
+      post :permit_params, params: params
+    end
+    assert_response :ok
+  end
 end