Simplify the implementation of custom argument serializers

We can speed up things for the supported types by keeping the code in the
way it was.

We can also avoid to loop trough all serializers in the deserialization by
trying to access the class already in the Hash.

We could also speed up the custom serialization if we define the class
that is going to be serialized when registering the serializers, but
that will remove the possibility of defining a serialzer for a
superclass and have the subclass serialized using it.

12 files changed, 192 insertions, 275 deletions

diff --git a/activejob/lib/active_job/arguments.rb b/activejob/lib/active_job/arguments.rb
index 9d47131864..e6ada163e8 100644
--- a/activejob/lib/active_job/arguments.rb
+++ b/activejob/lib/active_job/arguments.rb
@@ -3,6 +3,24 @@
 require "active_support/core_ext/hash"
 
 module ActiveJob
+  # Raised when an exception is raised during job arguments deserialization.
+  #
+  # Wraps the original exception raised as +cause+.
+  class DeserializationError < StandardError
+    def initialize #:nodoc:
+      super("Error while trying to deserialize arguments: #{$!.message}")
+      set_backtrace $!.backtrace
+    end
+  end
+
+  # Raised when an unsupported argument type is set as a job argument. We
+  # currently support NilClass, Integer, Fixnum, Float, String, TrueClass, FalseClass,
+  # Bignum, BigDecimal, and objects that can be represented as GlobalIDs (ex: Active Record).
+  # Raised if you set the key for a Hash something else than a string or
+  # a symbol. Also raised when trying to serialize an object which can't be
+  # identified with a Global ID - such as an unpersisted Active Record model.
+  class SerializationError < ArgumentError; end
+
   module Arguments
     extend self
     # :nodoc:
@@ -13,16 +31,135 @@ module ActiveJob
     # as-is. Arrays/Hashes are serialized element by element.
     # All other types are serialized using GlobalID.
     def serialize(arguments)
-      ActiveJob::Serializers.serialize(arguments)
+      arguments.map { |argument| serialize_argument(argument) }
     end
 
     # Deserializes a set of arguments. Whitelisted types are returned
     # as-is. Arrays/Hashes are deserialized element by element.
     # All other types are deserialized using GlobalID.
     def deserialize(arguments)
-      ActiveJob::Serializers.deserialize(arguments)
+      arguments.map { |argument| deserialize_argument(argument) }
     rescue
       raise DeserializationError
     end
+
+    private
+
+      # :nodoc:
+      GLOBALID_KEY = "_aj_globalid".freeze
+      # :nodoc:
+      SYMBOL_KEYS_KEY = "_aj_symbol_keys".freeze
+      # :nodoc:
+      WITH_INDIFFERENT_ACCESS_KEY = "_aj_hash_with_indifferent_access".freeze
+      # :nodoc:
+      OBJECT_SERIALIZER_KEY = "_aj_serialized"
+
+      # :nodoc:
+      RESERVED_KEYS = [
+        GLOBALID_KEY, GLOBALID_KEY.to_sym,
+        SYMBOL_KEYS_KEY, SYMBOL_KEYS_KEY.to_sym,
+        OBJECT_SERIALIZER_KEY, OBJECT_SERIALIZER_KEY.to_sym,
+        WITH_INDIFFERENT_ACCESS_KEY, WITH_INDIFFERENT_ACCESS_KEY.to_sym,
+      ]
+      private_constant :RESERVED_KEYS
+
+      def serialize_argument(argument)
+        case argument
+        when *TYPE_WHITELIST
+          argument
+        when GlobalID::Identification
+          convert_to_global_id_hash(argument)
+        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
+          symbol_keys = argument.each_key.grep(Symbol).map(&:to_s)
+          result = serialize_hash(argument)
+          result[SYMBOL_KEYS_KEY] = symbol_keys
+          result
+        else
+          Serializers.serialize(argument)
+        end
+      end
+
+      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
+          if serialized_global_id?(argument)
+            deserialize_global_id argument
+          elsif custom_serialized?(argument)
+            Serializers.deserialize(argument)
+          else
+            deserialize_hash(argument)
+          end
+        else
+          raise ArgumentError, "Can only deserialize primitive arguments: #{argument.inspect}"
+        end
+      end
+
+      def serialized_global_id?(hash)
+        hash.size == 1 && hash.include?(GLOBALID_KEY)
+      end
+
+      def deserialize_global_id(hash)
+        GlobalID::Locator.locate hash[GLOBALID_KEY]
+      end
+
+      def custom_serialized?(hash)
+        hash.key?(OBJECT_SERIALIZER_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)
+        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
+
+      def serialize_hash_key(key)
+        case key
+        when *RESERVED_KEYS
+          raise SerializationError.new("Can't serialize a Hash with reserved key #{key.inspect}")
+        when String, Symbol
+          key.to_s
+        else
+          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
+
+      def convert_to_global_id_hash(argument)
+        { GLOBALID_KEY => argument.to_global_id.to_s }
+      rescue URI::GID::MissingModelIdError
+        raise SerializationError, "Unable to serialize #{argument.class} " \
+          "without an id. (Maybe you forgot to call save?)"
+      end
   end
 end
diff --git a/activejob/lib/active_job/serializers.rb b/activejob/lib/active_job/serializers.rb
index dfd654175d..d9a130fa73 100644
--- a/activejob/lib/active_job/serializers.rb
+++ b/activejob/lib/active_job/serializers.rb
@@ -3,37 +3,13 @@
 require "set"
 
 module ActiveJob
-  # Raised when an exception is raised during job arguments deserialization.
-  #
-  # Wraps the original exception raised as +cause+.
-  class DeserializationError < StandardError
-    def initialize #:nodoc:
-      super("Error while trying to deserialize arguments: #{$!.message}")
-      set_backtrace $!.backtrace
-    end
-  end
-
-  # Raised when an unsupported argument type is set as a job argument. We
-  # currently support NilClass, Integer, Fixnum, Float, String, TrueClass, FalseClass,
-  # Bignum, BigDecimal, and objects that can be represented as GlobalIDs (ex: Active Record).
-  # Raised if you set the key for a Hash something else than a string or
-  # a symbol. Also raised when trying to serialize an object which can't be
-  # identified with a Global ID - such as an unpersisted Active Record model.
-  class SerializationError < ArgumentError; end
-
   # The <tt>ActiveJob::Serializers</tt> module is used to store a list of known serializers
   # and to add new ones. It also has helpers to serialize/deserialize objects
   module Serializers
     extend ActiveSupport::Autoload
     extend ActiveSupport::Concern
 
-    autoload :ArraySerializer
-    autoload :BaseSerializer
-    autoload :GlobalIDSerializer
-    autoload :HashWithIndifferentAccessSerializer
-    autoload :HashSerializer
     autoload :ObjectSerializer
-    autoload :StandardTypeSerializer
     autoload :SymbolSerializer
     autoload :DurationSerializer
     autoload :DateSerializer
@@ -57,8 +33,12 @@ module ActiveJob
       # Will look up through all known serializers.
       # If no serializers found will raise `ArgumentError`
       def deserialize(argument)
-        serializer = serializers.detect { |s| s.deserialize?(argument) }
-        raise ArgumentError, "Can only deserialize primitive arguments: #{argument.inspect}" unless serializer
+        serializer_name = argument[Arguments::OBJECT_SERIALIZER_KEY]
+        raise ArgumentError, "Serializer name is not present in the argument: #{argument.inspect}" unless serializer_name
+
+        serializer = serializer_name.safe_constantize
+        raise ArgumentError, "Serializer #{serializer_name} is not know" unless serializer
+
         serializer.deserialize(argument)
       end
 
@@ -71,36 +51,9 @@ module ActiveJob
       def add_serializers(*new_serializers)
         self._additional_serializers += new_serializers
       end
-
-      # Returns a list of reserved keys, which cannot be used as keys for a hash
-      def reserved_serializers_keys
-        RESERVED_KEYS
-      end
     end
 
-    # :nodoc:
-    GLOBALID_KEY = "_aj_globalid".freeze
-    # :nodoc:
-    SYMBOL_KEYS_KEY = "_aj_symbol_keys".freeze
-    # :nodoc:
-    WITH_INDIFFERENT_ACCESS_KEY = "_aj_hash_with_indifferent_access".freeze
-    # :nodoc:
-    OBJECT_SERIALIZER_KEY = "_aj_serialized"
-
-    # :nodoc:
-    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
-
-    add_serializers GlobalIDSerializer,
-      StandardTypeSerializer,
-      HashWithIndifferentAccessSerializer,
-      HashSerializer,
-      ArraySerializer,
-      SymbolSerializer,
+    add_serializers SymbolSerializer,
       DurationSerializer,
       DateTimeSerializer,
       DateSerializer,
diff --git a/activejob/lib/active_job/serializers/array_serializer.rb b/activejob/lib/active_job/serializers/array_serializer.rb
deleted file mode 100644
index 9db4edea99..0000000000
--- a/activejob/lib/active_job/serializers/array_serializer.rb
+++ /dev/null
@@ -1,24 +0,0 @@
-# frozen_string_literal: true
-
-module ActiveJob
-  module Serializers
-    # Provides methods to serialize and deserialize `Array`
-    class ArraySerializer < BaseSerializer # :nodoc:
-      alias_method :deserialize?, :serialize?
-
-      def serialize(array)
-        array.map { |arg| Serializers.serialize(arg) }
-      end
-
-      def deserialize(array)
-        array.map { |arg| Serializers.deserialize(arg) }
-      end
-
-      private
-
-        def klass
-          Array
-        end
-    end
-  end
-end
diff --git a/activejob/lib/active_job/serializers/base_serializer.rb b/activejob/lib/active_job/serializers/base_serializer.rb
deleted file mode 100644
index 155eeb29c3..0000000000
--- a/activejob/lib/active_job/serializers/base_serializer.rb
+++ /dev/null
@@ -1,41 +0,0 @@
-# frozen_string_literal: true
-
-module ActiveJob
-  module Serializers
-    # Implement the basic interface for Active Job arguments serializers.
-    class BaseSerializer
-      include Singleton
-
-      class << self
-        delegate :serialize?, :deserialize?, :serialize, :deserialize, to: :instance
-      end
-
-      # Determines if an argument should be serialized by a serializer.
-      def serialize?(argument)
-        argument.is_a?(klass)
-      end
-
-      # Determines if an argument should be deserialized by a serializer.
-      def deserialize?(_argument)
-        raise NotImplementedError
-      end
-
-      # Serializes an argument to a JSON primitive type.
-      def serialize(_argument)
-        raise NotImplementedError
-      end
-
-      # Deserilizes an argument form a JSON primiteve type.
-      def deserialize(_argument)
-        raise NotImplementedError
-      end
-
-      protected
-
-        # The class of the object that will be serialized.
-        def klass
-          raise NotImplementedError
-        end
-    end
-  end
-end
diff --git a/activejob/lib/active_job/serializers/duration_serializer.rb b/activejob/lib/active_job/serializers/duration_serializer.rb
index a3c4c5d1c2..715fe27a5c 100644
--- a/activejob/lib/active_job/serializers/duration_serializer.rb
+++ b/activejob/lib/active_job/serializers/duration_serializer.rb
@@ -4,12 +4,12 @@ module ActiveJob
   module Serializers
     class DurationSerializer < ObjectSerializer # :nodoc:
       def serialize(duration)
-        super("value" => duration.value, "parts" => Serializers.serialize(duration.parts))
+        super("value" => duration.value, "parts" => Arguments.serialize(duration.parts))
       end
 
       def deserialize(hash)
         value = hash["value"]
-        parts = Serializers.deserialize(hash["parts"])
+        parts = Arguments.deserialize(hash["parts"])
 
         klass.new(value, parts)
       end
diff --git a/activejob/lib/active_job/serializers/global_id_serializer.rb b/activejob/lib/active_job/serializers/global_id_serializer.rb
deleted file mode 100644
index 2d8629ef02..0000000000
--- a/activejob/lib/active_job/serializers/global_id_serializer.rb
+++ /dev/null
@@ -1,30 +0,0 @@
-# frozen_string_literal: true
-
-module ActiveJob
-  module Serializers
-    # Provides methods to serialize and deserialize objects which mixes `GlobalID::Identification`,
-    # including `ActiveRecord::Base` models
-    class GlobalIDSerializer < BaseSerializer # :nodoc:
-      def serialize(object)
-        { GLOBALID_KEY => object.to_global_id.to_s }
-      rescue URI::GID::MissingModelIdError
-        raise SerializationError, "Unable to serialize #{object.class} " \
-          "without an id. (Maybe you forgot to call save?)"
-      end
-
-      def deserialize(hash)
-        GlobalID::Locator.locate(hash[GLOBALID_KEY])
-      end
-
-      def deserialize?(argument)
-        argument.is_a?(Hash) && argument[GLOBALID_KEY]
-      end
-
-      private
-
-        def klass
-          GlobalID::Identification
-        end
-    end
-  end
-end
diff --git a/activejob/lib/active_job/serializers/hash_serializer.rb b/activejob/lib/active_job/serializers/hash_serializer.rb
deleted file mode 100644
index e569fe7501..0000000000
--- a/activejob/lib/active_job/serializers/hash_serializer.rb
+++ /dev/null
@@ -1,60 +0,0 @@
-# frozen_string_literal: true
-
-module ActiveJob
-  module Serializers
-    # Provides methods to serialize and deserialize `Hash` (`{key: field, ...}`)
-    # Only `String` or `Symbol` can be used as a key. Values will be serialized by known serializers
-    class HashSerializer < BaseSerializer # :nodoc:
-      def serialize(hash)
-        symbol_keys = hash.each_key.grep(Symbol).map(&:to_s)
-        result = serialize_hash(hash)
-        result[key] = symbol_keys
-        result
-      end
-
-      def deserialize?(argument)
-        argument.is_a?(Hash) && argument[key]
-      end
-
-      def deserialize(hash)
-        result = hash.transform_values { |v| Serializers::deserialize(v) }
-        symbol_keys = result.delete(key)
-        transform_symbol_keys(result, symbol_keys)
-      end
-
-      private
-
-        def key
-          SYMBOL_KEYS_KEY
-        end
-
-        def serialize_hash(hash)
-          hash.each_with_object({}) do |(key, value), result|
-            result[serialize_hash_key(key)] = Serializers.serialize(value)
-          end
-        end
-
-        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}") unless [String, Symbol].include?(key.class)
-
-          raise SerializationError.new("Can't serialize a Hash with reserved key #{key.inspect}") if ActiveJob::Serializers.reserved_serializers_keys.include?(key.to_s)
-
-          key.to_s
-        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
-
-        def klass
-          Hash
-        end
-    end
-  end
-end
diff --git a/activejob/lib/active_job/serializers/hash_with_indifferent_access_serializer.rb b/activejob/lib/active_job/serializers/hash_with_indifferent_access_serializer.rb
deleted file mode 100644
index 3b812ba304..0000000000
--- a/activejob/lib/active_job/serializers/hash_with_indifferent_access_serializer.rb
+++ /dev/null
@@ -1,31 +0,0 @@
-# frozen_string_literal: true
-
-module ActiveJob
-  module Serializers
-    # Provides methods to serialize and deserialize `ActiveSupport::HashWithIndifferentAccess`
-    # Values will be serialized by known serializers
-    class HashWithIndifferentAccessSerializer < HashSerializer # :nodoc:
-      def serialize(hash)
-        result = serialize_hash(hash)
-        result[key] = Serializers.serialize(true)
-        result
-      end
-
-      def deserialize(hash)
-        result = hash.transform_values { |v| Serializers.deserialize(v) }
-        result.delete(key)
-        result.with_indifferent_access
-      end
-
-      private
-
-        def key
-          WITH_INDIFFERENT_ACCESS_KEY
-        end
-
-        def klass
-          ActiveSupport::HashWithIndifferentAccess
-        end
-    end
-  end
-end
diff --git a/activejob/lib/active_job/serializers/object_serializer.rb b/activejob/lib/active_job/serializers/object_serializer.rb
index 940b6ff95d..9f59e8236f 100644
--- a/activejob/lib/active_job/serializers/object_serializer.rb
+++ b/activejob/lib/active_job/serializers/object_serializer.rb
@@ -21,14 +21,34 @@ module ActiveJob
     #           Money
     #         end
     #     end
-    class ObjectSerializer < BaseSerializer
+    class ObjectSerializer
+      include Singleton
+
+      class << self
+        delegate :serialize?, :serialize, :deserialize, to: :instance
+      end
+
+      # Determines if an argument should be serialized by a serializer.
+      def serialize?(argument)
+        argument.is_a?(klass)
+      end
+
+      # Serializes an argument to a JSON primitive type.
       def serialize(hash)
-        { OBJECT_SERIALIZER_KEY => self.class.name }.merge!(hash)
+        { Arguments::OBJECT_SERIALIZER_KEY => self.class.name }.merge!(hash)
       end
 
-      def deserialize?(argument)
-        argument.is_a?(Hash) && argument[OBJECT_SERIALIZER_KEY] == self.class.name
+      # Deserilizes an argument form a JSON primiteve type.
+      def deserialize(_argument)
+        raise NotImplementedError
       end
+
+      protected
+
+        # The class of the object that will be serialized.
+        def klass
+          raise NotImplementedError
+        end
     end
   end
 end
diff --git a/activejob/lib/active_job/serializers/standard_type_serializer.rb b/activejob/lib/active_job/serializers/standard_type_serializer.rb
deleted file mode 100644
index 1db4f3937d..0000000000
--- a/activejob/lib/active_job/serializers/standard_type_serializer.rb
+++ /dev/null
@@ -1,24 +0,0 @@
-# frozen_string_literal: true
-
-module ActiveJob
-  module Serializers
-    # Provides methods to serialize and deserialize standard types
-    # (`NilClass`, `String`, `Integer`, `Fixnum`, `Bignum`, `Float`, `BigDecimal`, `TrueClass`, `FalseClass`)
-    class StandardTypeSerializer < BaseSerializer # :nodoc:
-      def serialize?(argument)
-        Arguments::TYPE_WHITELIST.include? argument.class
-      end
-
-      def serialize(argument)
-        argument
-      end
-
-      alias_method :deserialize?, :serialize?
-
-      def deserialize(argument)
-        object = GlobalID::Locator.locate(argument) if argument.is_a? String
-        object || argument
-      end
-    end
-  end
-end
diff --git a/activejob/test/cases/argument_serialization_test.rb b/activejob/test/cases/argument_serialization_test.rb
index 8f51a2d238..4e26b9a178 100644
--- a/activejob/test/cases/argument_serialization_test.rb
+++ b/activejob/test/cases/argument_serialization_test.rb
@@ -14,7 +14,7 @@ class ArgumentSerializationTest < ActiveSupport::TestCase
   [ nil, 1, 1.0, 1_000_000_000_000_000_000_000,
     "a", true, false, BigDecimal.new(5),
     :a, 1.day, Date.new(2001, 2, 3), Time.new(2002, 10, 31, 2, 2, 2, "+02:00"),
-    DateTime.new(2001, 2, 3, 4, 5, 6, '+03:00'),
+    DateTime.new(2001, 2, 3, 4, 5, 6, "+03:00"),
     ActiveSupport::TimeWithZone.new(Time.utc(1999, 12, 31, 23, 59, 59), ActiveSupport::TimeZone["UTC"]),
     [ 1, "a" ],
     { "a" => 1 }
diff --git a/activejob/test/cases/serializers_test.rb b/activejob/test/cases/serializers_test.rb
index 207ae55b49..a86f168d03 100644
--- a/activejob/test/cases/serializers_test.rb
+++ b/activejob/test/cases/serializers_test.rb
@@ -58,7 +58,24 @@ class SerializersTest < ActiveSupport::TestCase
 
   test "won't deserialize unknown hash" do
     hash = { "_dummy_serializer" => 123, "_aj_symbol_keys" => [] }
-    assert_equal({ "_dummy_serializer" => 123 }, ActiveJob::Serializers.deserialize(hash))
+    error = assert_raises(ArgumentError) do
+      ActiveJob::Serializers.deserialize(hash)
+    end
+    assert_equal(
+      'Serializer name is not present in the argument: {"_dummy_serializer"=>123, "_aj_symbol_keys"=>[]}',
+      error.message
+    )
+  end
+
+  test "won't deserialize unknown serializer" do
+    hash = { "_aj_serialized" => "DoNotExist", "value" => 123 }
+    error = assert_raises(ArgumentError) do
+      ActiveJob::Serializers.deserialize(hash)
+    end
+    assert_equal(
+      "Serializer DoNotExist is not know",
+      error.message
+    )
   end
 
   test "will deserialize know serialized objects" do
@@ -74,7 +91,7 @@ class SerializersTest < ActiveSupport::TestCase
 
   test "can't add serializer with the same key twice" do
     ActiveJob::Serializers.add_serializers DummySerializer
-    assert_no_difference(-> { ActiveJob::Serializers.serializers.size } ) do
+    assert_no_difference(-> { ActiveJob::Serializers.serializers.size }) do
       ActiveJob::Serializers.add_serializers DummySerializer
     end
   end