Tighten up AJ::Arguments and its tests

* Disallow deserialization of non-primitive objects
* Broaden coverage; remove superfluous tests

3 files changed, 90 insertions, 85 deletions

diff --git a/activejob/lib/active_job/arguments.rb b/activejob/lib/active_job/arguments.rb
index 175a2f0956..099ea4d169 100644
--- a/activejob/lib/active_job/arguments.rb
+++ b/activejob/lib/active_job/arguments.rb
@@ -37,14 +37,16 @@ module ActiveJob
     private
       def serialize_argument(argument)
         case argument
-        when GlobalID::Identification
-          argument.to_global_id.to_s
         when *TYPE_WHITELIST
           argument
+        when GlobalID::Identification
+          argument.to_global_id.to_s
         when Array
           argument.map { |arg| serialize_argument(arg) }
         when Hash
-          Hash[ argument.map { |key, value| [ serialize_hash_key(key), serialize_argument(value) ] } ]
+          argument.each_with_object({}) do |(key, value), hash|
+            hash[serialize_hash_key(key)] = serialize_argument(value)
+          end
         else
           raise SerializationError.new("Unsupported argument type: #{argument.class.name}")
         end
@@ -52,14 +54,18 @@ module ActiveJob
 
       def deserialize_argument(argument)
         case argument
+        when String
+          GlobalID::Locator.locate(argument) || argument
+        when *TYPE_WHITELIST
+          argument
         when Array
           argument.map { |arg| deserialize_argument(arg) }
         when Hash
-          Hash[ argument.map { |key, value| [ key, deserialize_argument(value) ] } ].with_indifferent_access
-        when String, GlobalID
-          GlobalID::Locator.locate(argument) || argument
+          argument.each_with_object({}.with_indifferent_access) do |(key, value), hash|
+            hash[key] = deserialize_argument(value)
+          end
         else
-          argument
+          raise ArgumentError, "Can only deserialize primitive arguments: #{argument.inspect}"
         end
       end
 
@@ -68,7 +74,7 @@ module ActiveJob
         when String, Symbol
           key.to_s
         else
-          raise SerializationError.new("Unsupported hash key type: #{key.class.name}")
+          raise SerializationError.new("Only string and symbol hash keys may be serialized as job arguments, but #{key.inspect} is a #{key.class}")
         end
       end
   end
diff --git a/activejob/test/cases/argument_serialization_test.rb b/activejob/test/cases/argument_serialization_test.rb
new file mode 100644
index 0000000000..5a46c5cdef
--- /dev/null
+++ b/activejob/test/cases/argument_serialization_test.rb
@@ -0,0 +1,76 @@
+require 'helper'
+require 'active_job/arguments'
+require 'models/person'
+require 'active_support/core_ext/hash/indifferent_access'
+
+class ArgumentSerializationTest < ActiveSupport::TestCase
+  setup do
+    @person = Person.find('5')
+  end
+
+  [ nil, 1, 1.0, 1_000_000_000_000_000_000_000,
+    'a', true, false,
+    [ 1, 'a' ],
+    { 'a' => 1 }
+  ].each do |arg|
+    test "serializes #{arg.class} verbatim" do
+      assert_arguments_unchanged arg
+    end
+  end
+
+  [ :a, Object.new, self, Person.find('5').to_gid ].each do |arg|
+    test "does not serialize #{arg.class}" do
+      assert_raises ActiveJob::SerializationError do
+        ActiveJob::Arguments.serialize [ arg ]
+      end
+
+      assert_raises ActiveJob::DeserializationError do
+        ActiveJob::Arguments.deserialize [ arg ]
+      end
+    end
+  end
+
+  test 'should convert records to Global IDs' do
+    assert_arguments_roundtrip [@person], [@person.to_gid.to_s]
+  end
+
+  test 'should dive deep into arrays and hashes' do
+    assert_arguments_roundtrip [3, [@person]], [3, [@person.to_gid.to_s]]
+    assert_arguments_roundtrip [{ 'a' => @person }], [{ 'a' => @person.to_gid.to_s }.with_indifferent_access]
+  end
+
+  test 'should stringify symbol hash keys' do
+    assert_equal [ 'a' => 1 ], ActiveJob::Arguments.serialize([ a: 1 ])
+  end
+
+  test 'should disallow non-string/symbol hash keys' do
+    assert_raises ActiveJob::SerializationError do
+      ActiveJob::Arguments.serialize [ { 1 => 2 } ]
+    end
+
+    assert_raises ActiveJob::SerializationError do
+      ActiveJob::Arguments.serialize [ { :a => [{ 2 => 3 }] } ]
+    end
+  end
+
+  test 'should not allow non-primitive objects' do
+    assert_raises ActiveJob::SerializationError do
+      ActiveJob::Arguments.serialize [Object.new]
+    end
+
+    assert_raises ActiveJob::SerializationError do
+      ActiveJob::Arguments.serialize [1, [Object.new]]
+    end
+  end
+
+  private
+    def assert_arguments_unchanged(*args)
+      assert_arguments_roundtrip args, args
+    end
+
+    def assert_arguments_roundtrip(args, expected_serialized_args)
+      serialized = ActiveJob::Arguments.serialize(args)
+      assert_equal expected_serialized_args, serialized
+      assert_equal args, ActiveJob::Arguments.deserialize(serialized)
+    end
+end
diff --git a/activejob/test/cases/parameters_test.rb b/activejob/test/cases/parameters_test.rb
deleted file mode 100644
index 92f835af5d..0000000000
--- a/activejob/test/cases/parameters_test.rb
+++ /dev/null
@@ -1,77 +0,0 @@
-require 'helper'
-require 'active_job/arguments'
-require 'models/person'
-require 'active_support/core_ext/hash/indifferent_access'
-
-class ParameterSerializationTest < ActiveSupport::TestCase
-  test 'should make no change to regular values' do
-    assert_equal [ 1, "something" ], ActiveJob::Arguments.serialize([ 1, "something" ])
-  end
-
-  test 'should not allow complex objects' do
-    assert_equal [ nil ], ActiveJob::Arguments.serialize([ nil ])
-    assert_equal [ 1 ], ActiveJob::Arguments.serialize([ 1 ])
-    assert_equal [ 1.0 ], ActiveJob::Arguments.serialize([ 1.0 ])
-    assert_equal [ 'a' ], ActiveJob::Arguments.serialize([ 'a' ])
-    assert_equal [ true ], ActiveJob::Arguments.serialize([ true ])
-    assert_equal [ false ], ActiveJob::Arguments.serialize([ false ])
-    assert_equal [ { "a" => 1, "b" => 2 } ], ActiveJob::Arguments.serialize([ { a: 1, "b" => 2 } ])
-    assert_equal [ [ 1 ] ], ActiveJob::Arguments.serialize([ [ 1 ] ])
-    assert_equal [ 1_000_000_000_000_000_000_000 ], ActiveJob::Arguments.serialize([ 1_000_000_000_000_000_000_000 ])
-
-    err = assert_raises ActiveJob::SerializationError do
-      ActiveJob::Arguments.serialize([ 1, self ])
-    end
-    assert_equal "Unsupported argument type: #{self.class.name}", err.message
-  end
-
-  test 'should dive deep into arrays or hashes' do
-    assert_equal [ { "a" => Person.find(5).to_gid.to_s }.with_indifferent_access ], ActiveJob::Arguments.serialize([ { a: Person.find(5) } ])
-    assert_equal [ [ Person.find(5).to_gid.to_s ] ], ActiveJob::Arguments.serialize([ [ Person.find(5) ] ])
-  end
-
-  test 'should dive deep into arrays or hashes and raise exception on complex objects' do
-    err = assert_raises ActiveJob::SerializationError do
-      ActiveJob::Arguments.serialize([ 1, [self] ])
-    end
-    assert_equal "Unsupported argument type: #{self.class.name}", err.message
-  end
-
-  test 'shoud dive deep into hashes and allow raise exception on not string/symbol keys' do
-    err = assert_raises ActiveJob::SerializationError do
-      ActiveJob::Arguments.serialize([ [ { 1 => 2 } ] ])
-    end
-    assert_equal "Unsupported hash key type: Fixnum", err.message
-  end
-
-  test 'should serialize records with global id' do
-    assert_equal [ Person.find(5).to_gid.to_s ], ActiveJob::Arguments.serialize([ Person.find(5) ])
-  end
-
-  test 'should serialize values and records together' do
-    assert_equal [ 3, Person.find(5).to_gid.to_s ], ActiveJob::Arguments.serialize([ 3, Person.find(5) ])
-  end
-end
-
-class ParameterDeserializationTest < ActiveSupport::TestCase
-  test 'should make no change to regular values' do
-    assert_equal [ 1, "something" ], ActiveJob::Arguments.deserialize([ 1, "something" ])
-  end
-
-  test 'should deserialize records with global id' do
-    assert_equal [ Person.find(5) ], ActiveJob::Arguments.deserialize([ Person.find(5).to_gid ])
-  end
-
-  test 'should serialize values and records together' do
-    assert_equal [ 3, Person.find(5) ], ActiveJob::Arguments.deserialize([ 3, Person.find(5).to_gid ])
-  end
-
-  test 'should dive deep when deserialising arrays' do
-    assert_equal [ [ 3, Person.find(5) ] ], ActiveJob::Arguments.deserialize([ [ 3, Person.find(5).to_gid ] ])
-  end
-
-  test 'should dive deep when deserialising hashes' do
-    assert_equal [ { "5" => Person.find(5) } ], ActiveJob::Arguments.deserialize([ { "5" => Person.find(5).to_gid } ])
-  end
-
-end