From 485a6c7a83b9e70c731930d8406925304efa04a8 Mon Sep 17 00:00:00 2001 From: Max VelDink Date: Mon, 8 Jul 2024 17:11:14 -0400 Subject: [PATCH] fix!: Fix mis-serializing hash keys that were suppose to be strings (#111) --- lib/sorbet-schema.rb | 3 +- lib/sorbet-schema/hash_transformer.rb | 47 ++++++--------------- lib/sorbet-schema/serialize_value.rb | 20 +++++++++ lib/typed/hash_serializer.rb | 4 +- lib/typed/json_serializer.rb | 2 +- lib/typed/serializer.rb | 6 ++- test/hash_transformer_test.rb | 60 +++++---------------------- test/serialize_value_test.rb | 25 +++++++++++ test/support/structs/city.rb | 2 + test/t/struct_test.rb | 3 +- test/typed/hash_serializer_test.rb | 7 ++++ 11 files changed, 88 insertions(+), 91 deletions(-) create mode 100644 lib/sorbet-schema/serialize_value.rb create mode 100644 test/serialize_value_test.rb diff --git a/lib/sorbet-schema.rb b/lib/sorbet-schema.rb index b9984c9..2cf937f 100644 --- a/lib/sorbet-schema.rb +++ b/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 diff --git a/lib/sorbet-schema/hash_transformer.rb b/lib/sorbet-schema/hash_transformer.rb index e18d36a..3b72b70 100644 --- a/lib/sorbet-schema/hash_transformer.rb +++ b/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 diff --git a/lib/sorbet-schema/serialize_value.rb b/lib/sorbet-schema/serialize_value.rb new file mode 100644 index 0000000..87afa71 --- /dev/null +++ b/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 diff --git a/lib/typed/hash_serializer.rb b/lib/typed/hash_serializer.rb index 63e1b91..207fb34 100644 --- a/lib/typed/hash_serializer.rb +++ b/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 diff --git a/lib/typed/json_serializer.rb b/lib/typed/json_serializer.rb index d03034e..2011d14 100644 --- a/lib/typed/json_serializer.rb +++ b/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 diff --git a/lib/typed/serializer.rb b/lib/typed/serializer.rb index b795317..5dec483 100644 --- a/lib/typed/serializer.rb +++ b/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 diff --git a/test/hash_transformer_test.rb b/test/hash_transformer_test.rb index a4093f6..022db07 100644 --- a/test/hash_transformer_test.rb +++ b/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 diff --git a/test/serialize_value_test.rb b/test/serialize_value_test.rb new file mode 100644 index 0000000..fdfcc9e --- /dev/null +++ b/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 diff --git a/test/support/structs/city.rb b/test/support/structs/city.rb index 0ce275a..eb9daea 100644 --- a/test/support/structs/city.rb +++ b/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}) diff --git a/test/t/struct_test.rb b/test/t/struct_test.rb index df63731..4e06568 100644 --- a/test/t/struct_test.rb +++ b/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 ) diff --git a/test/typed/hash_serializer_test.rb b/test/typed/hash_serializer_test.rb index 4e7bd67..2ebcb82 100644 --- a/test/typed/hash_serializer_test.rb +++ b/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