fix!: Fix mis-serializing hash keys that were suppose to be strings (#…

…111)

lib/sorbet-schema.rb

@@ -16,10 +16,11 @@
 )
 loader.setup
 
-# We don't want to place this in the `Typed` module.
+# We don't want to place these in the `Typed` module.
 # `sorbet-schema` is a directory that is not autoloaded
 # but contains extensions, so we need to manually require it.
 require_relative "sorbet-schema/hash_transformer"
+require_relative "sorbet-schema/serialize_value"
 
 # We want to add a default `schema` method to structs
 # that will guarentee a schema can be created for use

lib/sorbet-schema/hash_transformer.rb

@@ -4,44 +4,21 @@
 # This is a simplified version of ActiveSupport's Key Hash extension
 # https://github.com/rails/rails/blob/main/activesupport/lib/active_support/core_ext/hash/keys.rb
 class HashTransformer
-  extend T::Sig
+  class << self
+    extend T::Sig
 
-  sig { params(should_serialize_values: T::Boolean).void }
-  def initialize(should_serialize_values: false)
-    @should_serialize_values = should_serialize_values
-  end
-
-  sig { params(hash: T::Hash[T.untyped, T.untyped]).returns(T::Hash[Symbol, T.untyped]) }
-  def deep_symbolize_keys(hash)
-    hash.each_with_object({}) do |(key, value), result|
-      result[key.to_sym] = transform_value(value, hash_transformation_method: :deep_symbolize_keys)
-    end
-  end
-
-  sig { params(hash: T::Hash[T.untyped, T.untyped]).returns(T::Hash[String, T.untyped]) }
-  def deep_stringify_keys(hash)
-    hash.each_with_object({}) do |(key, value), result|
-      result[key.to_s] = transform_value(value, hash_transformation_method: :deep_stringify_keys)
+    sig { params(hash: T::Hash[T.untyped, T.untyped]).returns(T::Hash[Symbol, T.untyped]) }
+    def symbolize_keys(hash)
+      hash.each_with_object({}) do |(key, value), result|
+        result[key.to_sym] = value
+      end
     end
-  end
-
-  private
-
-  sig { returns(T::Boolean) }
-  attr_reader :should_serialize_values
 
-  sig { params(value: T.untyped, hash_transformation_method: Symbol).returns(T.untyped) }
-  def transform_value(value, hash_transformation_method:)
-    if value.is_a?(Hash)
-      send(hash_transformation_method, value)
-    elsif value.is_a?(Array)
-      value.map { |inner_val| transform_value(inner_val, hash_transformation_method: hash_transformation_method) }
-    elsif value.is_a?(T::Struct) && should_serialize_values
-      deep_symbolize_keys(value.serialize)
-    elsif value.respond_to?(:serialize) && should_serialize_values
-      value.serialize
-    else
-      value
+    sig { params(hash: T::Hash[T.untyped, T.untyped]).returns(T::Hash[T.untyped, T.untyped]) }
+    def serialize_values(hash)
+      hash.each_with_object({}) do |(key, value), result|
+        result[key] = SerializeValue.serialize(value)
+      end
     end
   end
 end

lib/sorbet-schema/serialize_value.rb

@@ -0,0 +1,20 @@
+# typed: strict
+
+class SerializeValue
+  extend T::Sig
+
+  sig { params(value: T.untyped).returns(T.untyped) }
+  def self.serialize(value)
+    if value.is_a?(Hash)
+      HashTransformer.serialize_values(value)
+    elsif value.is_a?(Array)
+      value.map { |item| serialize(item) }
+    elsif value.is_a?(T::Struct)
+      value.serialize_to(:hash).payload_or(value)
+    elsif value.respond_to?(:serialize)
+      value.serialize
+    else
+      value
+    end
+  end
+end

lib/typed/hash_serializer.rb

@@ -15,14 +15,14 @@ def initialize(schema:, should_serialize_values: false)
 
     sig { override.params(source: Input).returns(Result[T::Struct, DeserializeError]) }
     def deserialize(source)
-      deserialize_from_creation_params(HashTransformer.new(should_serialize_values: should_serialize_values).deep_symbolize_keys(source))
+      deserialize_from_creation_params(HashTransformer.symbolize_keys(source))
     end
 
     sig { override.params(struct: T::Struct).returns(Result[Output, SerializeError]) }
     def serialize(struct)
       return Failure.new(SerializeError.new("'#{struct.class}' cannot be serialized to target type of '#{schema.target}'.")) if struct.class != schema.target
 
-      Success.new(serialize_from_struct(struct: struct, should_serialize_values: should_serialize_values))
+      Success.new(serialize_from_struct(struct:, should_serialize_values:))
     end
 
     private

lib/typed/json_serializer.rb

@@ -24,7 +24,7 @@ def deserialize(source)
     def serialize(struct)
       return Failure.new(SerializeError.new("'#{struct.class}' cannot be serialized to target type of '#{schema.target}'.")) if struct.class != schema.target
 
-      Success.new(JSON.generate(serialize_from_struct(struct: struct, should_serialize_values: true)))
+      Success.new(JSON.generate(serialize_from_struct(struct:, should_serialize_values: true)))
     end
   end
 end

lib/typed/serializer.rb

@@ -82,7 +82,11 @@ def deserialize_from_creation_params(creation_params)
     def serialize_from_struct(struct:, should_serialize_values: false)
       hsh = schema.fields.each_with_object({}) { |field, hsh| hsh[field.name] = field.serialize(struct.send(field.name)) }.compact
 
-      HashTransformer.new(should_serialize_values: should_serialize_values).deep_symbolize_keys(hsh)
+      if should_serialize_values
+        hsh = HashTransformer.serialize_values(hsh)
+      end
+
+      hsh
     end
   end
 end

test/hash_transformer_test.rb

@@ -1,5 +1,7 @@
 # typed: true
 
+require "test_helper"
+
 class HashTransformerTest < Minitest::Test
   def setup
     @test_hash = {
@@ -15,54 +17,14 @@ def setup
         }
       }
     }
-    # standard:enable Style/HashSyntax
-    @transformer = HashTransformer.new
   end
 
-  def test_deep_symbolize_keys_symbolizes_all_keys
+  def test_symbolize_keys_symbolizes_first_level_keys
     expected_hash = {
       test: TestEnums::EnumOne,
       another: 1,
       deeper: {
-        anotherhash: 2,
-        deeperagain: {
-          value: TestEnums::EnumThree,
-          boolean: false,
-          date: Date.new(1776, 7, 4),
-          array: [1, TestEnums::EnumOne, {verydeep: 1}]
-        }
-      }
-    }
-
-    assert_equal(expected_hash, @transformer.deep_symbolize_keys(@test_hash))
-  end
-
-  def test_deep_symbolize_keys_serialize_values_symbolizes_all_keys_and_serializes_values
-    transformer = HashTransformer.new(should_serialize_values: true)
-
-    expected_hash = {
-      test: "1",
-      another: 1,
-      deeper: {
-        anotherhash: 2,
-        deeperagain: {
-          value: "3",
-          boolean: false,
-          date: Date.new(1776, 7, 4),
-          array: [1, "1", {verydeep: 1}]
-        }
-      }
-    }
-
-    assert_equal(expected_hash, transformer.deep_symbolize_keys(@test_hash))
-  end
-
-  def test_deep_stringify_keys_stringifies_all_keys
-    expected_hash = {
-      "test" => TestEnums::EnumOne,
-      "another" => 1,
-      "deeper" => {
-        "anotherhash" => 2,
+        :anotherhash => 2,
         "deeperagain" => {
           "value" => TestEnums::EnumThree,
           "boolean" => false,
@@ -72,17 +34,15 @@ def test_deep_stringify_keys_stringifies_all_keys
       }
     }
 
-    assert_equal(expected_hash, @transformer.deep_stringify_keys(@test_hash))
+    assert_equal(expected_hash, HashTransformer.symbolize_keys(@test_hash))
   end
 
-  def test_deep_stringify_keys_serialize_values_stringifies_all_keys_and_serializes_values
-    transformer = HashTransformer.new(should_serialize_values: true)
-
+  def test_serialize_values_serializes_values
     expected_hash = {
       "test" => "1",
-      "another" => 1,
-      "deeper" => {
-        "anotherhash" => 2,
+      :another => 1,
+      :deeper => {
+        :anotherhash => 2,
         "deeperagain" => {
           "value" => "3",
           "boolean" => false,
@@ -92,6 +52,6 @@ def test_deep_stringify_keys_serialize_values_stringifies_all_keys_and_serialize
       }
     }
 
-    assert_equal(expected_hash, transformer.deep_stringify_keys(@test_hash))
+    assert_equal(expected_hash, HashTransformer.serialize_values(@test_hash))
   end
 end

test/serialize_value_test.rb

@@ -0,0 +1,25 @@
+# typed: true
+
+require "test_helper"
+
+class SerializeValueTest < Minitest::Test
+  def test_when_value_is_a_hash_returns_each_value_serialized
+    assert_equal({:one => "1", "two" => "2"}, SerializeValue.serialize({:one => TestEnums::EnumOne, "two" => TestEnums::EnumTwo}))
+  end
+
+  def test_when_value_is_an_array_returns_each_value_serialized
+    assert_equal(["1", "2"], SerializeValue.serialize([TestEnums::EnumOne, TestEnums::EnumTwo]))
+  end
+
+  def test_when_value_is_a_struct_returns_serialized_struct
+    assert_equal({name: "DC", capital: true}, SerializeValue.serialize(DC_CITY))
+  end
+
+  def test_when_value_implements_serialize_returns_serialized_value
+    assert_equal("1", SerializeValue.serialize(TestEnums::EnumOne))
+  end
+
+  def test_when_value_doesnt_serialize_returns_value
+    assert_equal(1, SerializeValue.serialize(1))
+  end
+end

test/support/structs/city.rb

@@ -5,7 +5,9 @@ class City < T::Struct
 
   const :name, String
   const :capital, T::Boolean
+  const :data, T.nilable(T::Hash[String, Integer])
 end
 
 NEW_YORK_CITY = City.new(name: "New York", capital: false)
 DC_CITY = City.new(name: "DC", capital: true)
+OVIEDO_CITY = City.new(name: "Oviedo", capital: false, data: {"how many maxes live here?" => 1})

test/t/struct_test.rb

@@ -5,7 +5,8 @@ def test_schema_can_be_derived_from_struct
     expected_schema = Typed::Schema.new(
       fields: [
         Typed::Field.new(name: :name, type: String),
-        Typed::Field.new(name: :capital, type: T::Utils.coerce(T::Boolean))
+        Typed::Field.new(name: :capital, type: T::Utils.coerce(T::Boolean)),
+        Typed::Field.new(name: :data, type: T::Utils.coerce(T::Hash[String, Integer]), optional: true)
       ],
       target: City
     )

test/typed/hash_serializer_test.rb

@@ -79,6 +79,13 @@ def test_will_use_inline_serializers
     assert_payload({title: "Software Developer", salary: 90_000_00, start_date: "061 March"}, result)
   end
 
+  def test_with_hash_field_with_string_keys_serializes
+    result = Typed::HashSerializer.new(schema: City.schema).serialize(OVIEDO_CITY)
+
+    assert_success(result)
+    assert_payload({name: "Oviedo", capital: false, data: {"how many maxes live here?" => 1}}, result)
+  end
+
   # Deserialize Tests
 
   def test_it_can_simple_deserialize