Allow keyword arguments to work with ActiveJob

Unfortunately, the HashWithIndifferent access approach is insufficient
for our needs. It's perfectly reasonable to want to use keyword
arguments with Active Job, which we will see as a symbol keyed hash. For
Ruby to convert this back to keyword arguments, it must deserialize to a
symbol keyed hash.

There are two primary changes to the serialization behavior. We first
treat a HWIA separately, and mark it as such so we can convert it back
into a HWIA during deserialization.

For normal hashes, we keep a list of all symbol keys, and convert them
back to symbol keys after deserialization.

Fixes #18741.

activejob/CHANGELOG.md

@@ -1,3 +1,9 @@
+*   Allow keyword arguments to be used with Active Job.
+
+    Fixes #18741.
+
+    *Sean Griffin*
+
 *   Add `:only` option to `assert_enqueued_jobs`, to check the number of times
     a specific kind of job is enqueued.
 

activejob/lib/active_job/arguments.rb

@@ -1,4 +1,4 @@
-require 'active_support/core_ext/hash/indifferent_access'
+require 'active_support/core_ext/hash'
 
 module ActiveJob
   # Raised when an exception is raised during job arguments deserialization.
@@ -44,7 +44,9 @@ def deserialize(arguments)
 
     private
       GLOBALID_KEY = '_aj_globalid'.freeze
-      private_constant :GLOBALID_KEY
+      SYMBOL_KEYS_KEY = '_aj_symbol_keys'.freeze
+      WITH_INDIFFERENT_ACCESS_KEY = '_aj_hash_with_indifferent_access'.freeze
+      private_constant :GLOBALID_KEY, :SYMBOL_KEYS_KEY, :WITH_INDIFFERENT_ACCESS_KEY
 
       def serialize_argument(argument)
         case argument
@@ -54,10 +56,15 @@ def serialize_argument(argument)
           { GLOBALID_KEY => argument.to_global_id.to_s }
         when Array
           argument.map { |arg| serialize_argument(arg) }
+        when ActiveSupport::HashWithIndifferentAccess
+          result = serialize_hash(argument)
+          result[WITH_INDIFFERENT_ACCESS_KEY] = serialize_argument(true)
+          result
         when Hash
-          argument.each_with_object({}) do |(key, value), hash|
-            hash[serialize_hash_key(key)] = serialize_argument(value)
-          end
+          symbol_keys = argument.each_key.grep(Symbol).map(&:to_s)
+          result = serialize_hash(argument)
+          result[SYMBOL_KEYS_KEY] = symbol_keys
+          result
         else
           raise SerializationError.new("Unsupported argument type: #{argument.class.name}")
         end
@@ -75,7 +82,7 @@ def deserialize_argument(argument)
           if serialized_global_id?(argument)
             deserialize_global_id argument
           else
-            deserialize_hash argument
+            deserialize_hash(argument)
           end
         else
           raise ArgumentError, "Can only deserialize primitive arguments: #{argument.inspect}"
@@ -90,13 +97,27 @@ def deserialize_global_id(hash)
         GlobalID::Locator.locate hash[GLOBALID_KEY]
       end
 
+      def serialize_hash(argument)
+        argument.each_with_object({}) do |(key, value), hash|
+          hash[serialize_hash_key(key)] = serialize_argument(value)
+        end
+      end
+
       def deserialize_hash(serialized_hash)
-        serialized_hash.each_with_object({}.with_indifferent_access) do |(key, value), hash|
-          hash[key] = deserialize_argument(value)
+        result = serialized_hash.transform_values { |v| deserialize_argument(v) }
+        if result.delete(WITH_INDIFFERENT_ACCESS_KEY)
+          result = result.with_indifferent_access
+        elsif symbol_keys = result.delete(SYMBOL_KEYS_KEY)
+          result = transform_symbol_keys(result, symbol_keys)
         end
+        result
       end
 
-      RESERVED_KEYS = [GLOBALID_KEY, GLOBALID_KEY.to_sym]
+      RESERVED_KEYS = [
+        GLOBALID_KEY, GLOBALID_KEY.to_sym,
+        SYMBOL_KEYS_KEY, SYMBOL_KEYS_KEY.to_sym,
+        WITH_INDIFFERENT_ACCESS_KEY, WITH_INDIFFERENT_ACCESS_KEY.to_sym,
+      ]
       private_constant :RESERVED_KEYS
 
       def serialize_hash_key(key)
@@ -109,5 +130,15 @@ def serialize_hash_key(key)
           raise SerializationError.new("Only string and symbol hash keys may be serialized as job arguments, but #{key.inspect} is a #{key.class}")
         end
       end
+
+      def transform_symbol_keys(hash, symbol_keys)
+        hash.transform_keys do |key|
+          if symbol_keys.include?(key)
+            key.to_sym
+          else
+            key
+          end
+        end
+      end
   end
 end

activejob/test/cases/argument_serialization_test.rb

@@ -2,6 +2,7 @@
 require 'active_job/arguments'
 require 'models/person'
 require 'active_support/core_ext/hash/indifferent_access'
+require 'jobs/kwargs_job'
 
 class ArgumentSerializationTest < ActiveSupport::TestCase
   setup do
@@ -31,16 +32,26 @@ class ArgumentSerializationTest < ActiveSupport::TestCase
   end
 
   test 'should convert records to Global IDs' do
-    assert_arguments_roundtrip [@person], ['_aj_globalid' => @person.to_gid.to_s]
+    assert_arguments_roundtrip [@person]
   end
 
   test 'should dive deep into arrays and hashes' do
-    assert_arguments_roundtrip [3, [@person]], [3, ['_aj_globalid' => @person.to_gid.to_s]]
-    assert_arguments_roundtrip [{ 'a' => @person }], [{ 'a' => { '_aj_globalid' => @person.to_gid.to_s }}.with_indifferent_access]
+    assert_arguments_roundtrip [3, [@person]]
+    assert_arguments_roundtrip [{ 'a' => @person }]
   end
 
-  test 'should stringify symbol hash keys' do
-    assert_equal [ 'a' => 1 ], ActiveJob::Arguments.serialize([ a: 1 ])
+  test 'should maintain string and symbol keys' do
+    assert_arguments_roundtrip([a: 1, "b" => 2])
+  end
+
+  test 'should maintain hash with indifferent access' do
+    symbol_key = { a: 1 }
+    string_key = { 'a' => 1 }
+    indifferent_access = { a: 1 }.with_indifferent_access
+
+    assert_not_instance_of ActiveSupport::HashWithIndifferentAccess, perform_round_trip([symbol_key]).first
+    assert_not_instance_of ActiveSupport::HashWithIndifferentAccess, perform_round_trip([string_key]).first
+    assert_instance_of ActiveSupport::HashWithIndifferentAccess, perform_round_trip([indifferent_access]).first
   end
 
   test 'should disallow non-string/symbol hash keys' do
@@ -71,14 +82,22 @@ class ArgumentSerializationTest < ActiveSupport::TestCase
     end
   end
 
+  test 'allows for keyword arguments' do
+    KwargsJob.perform_later(argument: 2)
+
+    assert_equal "Job with argument: 2", JobBuffer.last_value
+  end
+
   private
     def assert_arguments_unchanged(*args)
-      assert_arguments_roundtrip args, args
+      assert_arguments_roundtrip args
+    end
+
+    def assert_arguments_roundtrip(args)
+      assert_equal args, perform_round_trip(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)
+    def perform_round_trip(args)
+      ActiveJob::Arguments.deserialize(ActiveJob::Arguments.serialize(args))
     end
 end

activejob/test/jobs/kwargs_job.rb

@@ -0,0 +1,7 @@
+require_relative '../support/job_buffer'
+
+class KwargsJob < ActiveJob::Base
+  def perform(argument: 1)
+    JobBuffer.add("Job with argument: #{argument}")
+  end
+end