From 4e7d675a8fe508bdffe3d8b808de581d08d7d6be Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Sat, 25 Jan 2025 22:20:58 -0500 Subject: [PATCH] `Coder#load`: Conditionally call `Formats.remove_root` Follow-up to [#420][] Problem --- While the `Coder` class and `Serialization` module aim to comply with Active Record, there's still potential for that compatibility to drift of break without sufficient test coverage. The `Coder#load` implementation does not support inferring the correct `@persisted` value from payloads with root-level keys. For example, `{ "person": { "id": 1 } }` will not be inferred as persisted in the same way that `{ "id": 1 }` will. Proposal --- To resolve the `@persisted` value, call `Format.remove_root` to ensure that the payload consists of only attributes. [#420]: https://github.com/rails/activeresource/pull/420 --- lib/active_resource/coder.rb | 2 ++ test/cases/base/serialization_test.rb | 49 +++++++++++++++++---------- 2 files changed, 34 insertions(+), 17 deletions(-) diff --git a/lib/active_resource/coder.rb b/lib/active_resource/coder.rb index 720ad8c66e..f002d292da 100644 --- a/lib/active_resource/coder.rb +++ b/lib/active_resource/coder.rb @@ -70,6 +70,8 @@ def load(value) return if value.nil? value = resource_class.format.decode(value) if value.is_a?(String) raise ArgumentError.new("expected value to be Hash, but was #{value.class}") unless value.is_a?(Hash) + value = Formats.remove_root(value) if value.keys.first.to_s == resource_class.element_name + resource_class.new(value, value[resource_class.primary_key]) end end diff --git a/test/cases/base/serialization_test.rb b/test/cases/base/serialization_test.rb index d60adf1565..0556ece034 100644 --- a/test/cases/base/serialization_test.rb +++ b/test/cases/base/serialization_test.rb @@ -100,7 +100,7 @@ class SerializationTest < ActiveSupport::TestCase test "#load decodes a Hash into an instance" do resource = Person.new(id: 1, name: "Matz") - decoded = Person.coder.load(resource.serializable_hash) + decoded = Person.coder.load(JSON.parse(resource.encode)) assert_equal resource.id, decoded.id assert_equal resource.name, decoded.name @@ -110,21 +110,25 @@ class SerializationTest < ActiveSupport::TestCase test "#load builds the instance as persisted when the default primary key is present" do resource = Person.new(id: 1, name: "Matz") - decoded = Person.coder.load(resource.encode) + [ resource.encode, JSON.parse(resource.encode) ].each do |encoded| + decoded = Person.coder.load(encoded) - assert_predicate decoded, :persisted? - assert_not_predicate decoded, :new_record? + assert_predicate decoded, :persisted? + assert_not_predicate decoded, :new_record? + end end test "#load builds the instance as persisted when the configured primary key is present" do previous_value, Person.primary_key = Person.primary_key, "pk" resource = Person.new(pk: 1, name: "Matz") - decoded = Person.coder.load(resource.encode) + [ resource.encode, JSON.parse(resource.encode) ].each do |encoded| + decoded = Person.coder.load(encoded) - assert_equal 1, decoded.id - assert_predicate decoded, :persisted? - assert_not_predicate decoded, :new_record? + assert_equal 1, decoded.id + assert_predicate decoded, :persisted? + assert_not_predicate decoded, :new_record? + end ensure Person.primary_key = previous_value end @@ -132,23 +136,26 @@ class SerializationTest < ActiveSupport::TestCase test "#load builds the instance as a new record when the default primary key is absent" do resource = Person.new(name: "Matz") - decoded = Person.coder.load(resource.encode) + [ resource.encode, JSON.parse(resource.encode) ].each do |encoded| + decoded = Person.coder.load(encoded) - assert_nil decoded.id - assert_not_predicate decoded, :persisted? - assert_predicate decoded, :new_record? + assert_nil decoded.id + assert_not_predicate decoded, :persisted? + assert_predicate decoded, :new_record? + end end test "#load builds the instance as a new record when the configured primary key is absent" do previous_value, Person.primary_key = Person.primary_key, "pk" - resource = Person.new(name: "Matz") - decoded = Person.coder.load(resource.encode) + [ resource.encode, JSON.parse(resource.encode) ].each do |encoded| + decoded = Person.coder.load(encoded) - assert_nil decoded.id - assert_not_predicate decoded, :persisted? - assert_predicate decoded, :new_record? + assert_nil decoded.id + assert_not_predicate decoded, :persisted? + assert_predicate decoded, :new_record? + end ensure Person.primary_key = previous_value end @@ -163,6 +170,14 @@ class SerializationTest < ActiveSupport::TestCase assert_raises(ArgumentError, match: "expected value to be Hash, but was Integer") { Person.coder.load(1) } end + test "#load does not remove a non-root key from a single-key Hash" do + payload = { "friends" => [ { "id" => 1 } ] } + + decoded = Person.coder.load(payload) + + assert_equal [ 1 ], decoded.friends.map(&:id) + end + test "#dump encodes resources" do resource = Person.new(id: 1, name: "Matz")