From 990c7063e64ce1c0327256a20bced90a01ffbddc Mon Sep 17 00:00:00 2001 From: Ronald Tse Date: Mon, 27 Jul 2026 19:33:52 +0800 Subject: [PATCH] fix(element-builder): dispatch on value.class for unrelated Serializables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ElementBuilder#create_nested_model_element` reused the rule's cached `child_transformation` (compiled for the declared `attribute_type`) regardless of the value's actual class. When a caller assigned an unrelated `Lutaml::Model::Serializable` to a typed slot, the wrong transformation was applied silently: 1. The declared type's `compiled_rules` were iterated 2. `extract_rule_value` called `value.public_send(rule.attribute_name)` 3. If the value's class didn't have that attribute name, NoMethodError The cache itself was correct: `TransformationRegistry#transformation_key` includes `object_id`, so cache keys are unique per class. The bug was in the dispatch logic, not the cache. ## Fix Dispatch on `value.class` whenever value is a `Serializable` that differs from the declared `attribute_type`. This subsumes the existing `is_polymorphic_subtype` branch (subtypes have `value.class != rule.attribute_type` by definition). The fast path (`value.class == rule.attribute_type`) preserves the cached `rule.child_transformation` lookup, so no performance regression for the common case. ## Real-world trigger sts-ruby had two duplicate `` classes — `Sts::TbxIsoTml::Fn` (Ruby attr `:p`) and `Sts::NisoSts::Fn` (Ruby attr `:paragraph`). When a `TbxIsoTml::Fn` was nested inside a `NisoSts::Back` (whose `:fn_group` is typed `NisoSts::FnGroup`), the cached `NisoSts::Fn`-transformation was applied to the `TbxIsoTml::Fn` value, calling `fn.public_send(:paragraph)` and raising `NoMethodError`. Reverse direction also failed. sts-ruby worked around by deleting the duplicate classes (PR #47). The upstream fix here prevents the trap for any downstream consumer with similar duplicate-class patterns. ## Verification - New spec: `spec/lutaml/xml/cross_class_dispatch_spec.rb` (3 examples) - unrelated Serializable dispatch - declared-type fast path unchanged - XML round-trip preserved - Existing polymorphic specs (`spec/lutaml/model/polymorphic_spec.rb`) unchanged: 17 examples pass - Full suite: 5316 examples, 0 failures, 1 pending (pre-existing) ## Backwards compatibility Existing callers that rely on declared-type rules being applied to unrelated values will see different (correct) behaviour. This is the intentional fix — the prior behaviour was a silent wrong-rules application. See `BUGREPORT.element-builder-dispatch-on-value-class.md` for the original analysis and the downstream trigger. --- .../xml/transformation/element_builder.rb | 18 +++-- spec/lutaml/xml/cross_class_dispatch_spec.rb | 79 +++++++++++++++++++ 2 files changed, 92 insertions(+), 5 deletions(-) create mode 100644 spec/lutaml/xml/cross_class_dispatch_spec.rb diff --git a/lib/lutaml/xml/transformation/element_builder.rb b/lib/lutaml/xml/transformation/element_builder.rb index 8373ef20..064b7929 100644 --- a/lib/lutaml/xml/transformation/element_builder.rb +++ b/lib/lutaml/xml/transformation/element_builder.rb @@ -140,13 +140,21 @@ def create_nested_model_element(rule, value, options, register, actual_class = resolve_polymorphic_class(rule, value, is_polymorphic, is_polymorphic_subtype) + # Dispatch on value's actual class when it is a Serializable that + # differs from the declared attribute_type. This covers ad-hoc + # polymorphism (caller assigns an unrelated Serializable to a typed + # slot) and prevents the wrong transformation being applied + # silently. Subsumes is_polymorphic_subtype; preserved here for + # the resolve_polymorphic_class call above. + dispatch_on_value_class = union || is_polymorphic || + (value.is_a?(Lutaml::Model::Serialize) && + value.class != rule.attribute_type) + # Get transformation for the actual class. Unions resolve the member # from the value's own class, like polymorphism. - child_transformation = if union - value.class.transformation_for(:xml, register) - elsif is_polymorphic || is_polymorphic_subtype - actual_class.transformation_for(:xml, - register) + child_transformation = if dispatch_on_value_class + value.class.transformation_for(:xml, + register) else rule.child_transformation || actual_class.transformation_for( :xml, register diff --git a/spec/lutaml/xml/cross_class_dispatch_spec.rb b/spec/lutaml/xml/cross_class_dispatch_spec.rb new file mode 100644 index 00000000..2336bd2e --- /dev/null +++ b/spec/lutaml/xml/cross_class_dispatch_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_string: true + +require "spec_helper" + +# When a caller assigns an unrelated Lutaml::Model::Serializable (not a +# declared subtype) to a typed slot, serialisation must use the value's own +# class mapping rules. The prior behaviour reused the declared +# attribute_type's child_transformation regardless of value.class, which +# silently applied the wrong rules and raised NoMethodError on whichever +# Ruby attribute name differed. +# +# See BUGREPORT.element-builder-dispatch-on-value-class.md for the original +# analysis and sts-ruby PR #47 for the downstream trigger. +RSpec.describe "ElementBuilder cross-class dispatch" do + let(:declared_type) do + Class.new(Lutaml::Model::Serializable) do + attribute :name, :string + + xml do + root "declared" + map_element "name", to: :name + end + end + end + + let(:actual_type) do + Class.new(Lutaml::Model::Serializable) do + attribute :different_name, :string + + xml do + root "actual" + map_element "different-name", to: :different_name + end + end + end + + let(:holder_class) do + declared = declared_type + Class.new(Lutaml::Model::Serializable) do + attribute :child, declared + + xml do + root "holder" + map_element "child", to: :child + end + end + end + + it "serialises an unrelated Serializable using its own mapping" do + holder = holder_class.new + holder.child = actual_type.new(different_name: "x") + xml = holder.to_xml + + parsed = Nokogiri::XML(xml) + expect(parsed.at_css("different-name").content).to eq("x") + end + + it "preserves the declared-type fast path for matching classes" do + holder = holder_class.new + holder.child = declared_type.new(name: "y") + xml = holder.to_xml + + parsed = Nokogiri::XML(xml) + expect(parsed.at_css("name").content).to eq("y") + end + + it "round-trips the XML output (parsing yields declared type by design)" do + # Deserialisation cannot infer the runtime-assigned class without a + # discriminator; it instantiates the declared type. The XML round-trip + # itself must still be lossless. + holder = holder_class.new + holder.child = actual_type.new(different_name: "round-trip") + xml = holder.to_xml + + reparsed = holder_class.from_xml(xml) + expect(reparsed.child).to be_a(declared_type) + expect(xml).to include("round-trip") + end +end