Skip to content

fix(element-builder): dispatch on value.class for unrelated Serializables - #741

Open
ronaldtse wants to merge 1 commit into
mainfrom
fix/element-builder-dispatch-on-value-class
Open

fix(element-builder): dispatch on value.class for unrelated Serializables#741
ronaldtse wants to merge 1 commit into
mainfrom
fix/element-builder-dispatch-on-value-class

Conversation

@ronaldtse

Copy link
Copy Markdown
Contributor

Summary

Fixes the dispatch bug documented in BUGREPORT.element-builder-dispatch-on-value-class.md.

ElementBuilder#create_nested_model_element reused the rule's cached child_transformation (compiled for the declared attribute_type) regardless of value.class. When a caller assigned an unrelated Lutaml::Model::Serializable to a typed slot, the wrong transformation was applied silently — eventually raising NoMethodError on whichever Ruby attribute name differed between the declared type and the value's class.

The cache itself was correct (TransformationRegistry#transformation_key includes object_id); the bug was in the dispatch logic.

Fix

Dispatch on value.class whenever value is a Serializable that differs from the declared attribute_type:

dispatch_on_value_class = union || is_polymorphic ||
  (value.is_a?(Lutaml::Model::Serialize) &&
   value.class != rule.attribute_type)

child_transformation = if dispatch_on_value_class
                         value.class.transformation_for(:xml, register)
                       else
                         rule.child_transformation ||
                           rule.attribute_type.transformation_for(:xml, register)
                       end

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 <fn> 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 (regression guard)
    • XML round-trip preserved
  • Existing 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 relied on declared-type rules being applied to unrelated values will see different (correct) behaviour. This is the intentional fix — the prior behaviour silently applied wrong rules.

Test plan

  • bundle exec rspec — 5316 examples, 0 failures
  • bundle exec rubocop — clean
  • CI green

…bles

`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 `<fn>` 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.
@github-actions

Copy link
Copy Markdown

JS build check

Triggered [js-pr-check]
(https://github.com/lutaml/lutaml-model-js/actions/workflows/pr-check.yml)
against this PR's head (990c706).

The result will appear as a lutaml-model-js / pr-check status check
on this PR once the workflow run completes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant