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.

4 files changed, 82 insertions, 19 deletions

diff --git a/activejob/CHANGELOG.md b/activejob/CHANGELOG.md
index 794d05d1b4..09c8f0800d 100644
--- a/activejob/CHANGELOG.md
+++ b/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.
 
diff --git a/activejob/lib/active_job/arguments.rb b/activejob/lib/active_job/arguments.rb
index 752be6898e..622c37098e 100644
--- a/activejob/lib/active_job/arguments.rb
+++ b/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 @@ module ActiveJob
 
     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 @@ module ActiveJob
           { 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 @@ module ActiveJob
           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 @@ module ActiveJob
         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 @@ module ActiveJob
           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
diff --git a/activejob/test/cases/argument_serialization_test.rb b/activejob/test/cases/argument_serialization_test.rb
index dbe36fc572..8b9b62190f 100644
--- a/activejob/test/cases/argument_serialization_test.rb
+++ b/activejob/test/cases/argument_serialization_test.rb
@@ -2,6 +2,7 @@ require 'helper'
 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
diff --git a/activejob/test/jobs/kwargs_job.rb b/activejob/test/jobs/kwargs_job.rb
new file mode 100644
index 0000000000..2df17d15ae
--- /dev/null
+++ b/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