fix: builder setters record mutations; serializer never silently drops attributes - #740
Merged
Merged
Conversation
…silently drops attributes Direct setter calls (`x.foo = v`) inside `Klass.new do |x| ... end` blocks were silently dropped from `to_xml` for `ordered` and `mixed_content` models whenever any sibling appender call (`x.foo(v)`) was also present. Root cause: order tracking was hooked into the getter-with-arg path (`x.foo(v)`), which delegated to the generated setter. The setter itself never called `track_order`, so direct-setter mutations were missing from `element_order`. The serializer's `apply_remaining_rules` then skipped any element-typed rule not represented in `element_order`, producing silent data loss. The recent gate change from `mixed_content?` to `ordered?` (commit 4d65ba0) widened this bug surface from `mixed_content`-only models to every `ordered` model. Fix has two layers: 1. Root cause — setters now record mutations. `Serialize::Builder` gains two encapsulated helpers: - `record_mutation(name, value)` for singular attributes - `record_mutation_collection(name, value)` for wholesale collection assignment (one entry per item) These replace five scattered `track_order(...) if @__order_tracking__` incantations in `attribute_definition.rb` and `initialization.rb`. Both setter and getter-with-arg paths now route through the same helpers, making them behaviourally identical. 2. Safety net — `OrderedApplier#apply_remaining_rules` no longer skips element-typed rules wholesale. After the ordered pass, it emits any element-typed rule whose value would otherwise vanish, using `element_order_coverage` + `element_rule_already_emitted?` to detect coverage. This guarantees the invariant: every attribute with a non-default value appears in serialized output, regardless of which mutation path set it. Future code paths that forget to call `record_mutation` will degrade to declaration-order emission rather than silent data loss.
The regression spec that shipped with commit 4d65ba0 only exercised the appender path (`i.b "first"`), which was never broken. It could not detect the actual bug because the bug lived in the direct-setter path. This commit adds a dedicated spec file for Serialize::Builder and broadens the existing ordered_content regression to cover both syntaxes. New: spec/lutaml/model/serialize/builder_spec.rb - no-silent-drop invariant across four mutation styles: * appender-only * direct setter for singular + appender for collection * wholesale collection assignment * all direct setters - call-order preservation for both syntaxes - round-trip parity for ordered and mixed_content models - .tap vs builder block equivalence - no-op guarantee when tracking is disabled - wholesale collection reassignment (multiple assignments) - cross-cutting no-silent-drop guard parameterised over mutation patterns — regression guard for the bug class, not just this instance Updated: spec/lutaml/model/ordered_content_spec.rb - Regression now exercises both appender and direct-setter syntaxes on the same model, so any future divergence between the two paths fails the test.
…ntics The builder DSL had no user-facing documentation. Users had no way to discover that `x.foo(v)` and `x.foo = v` are equivalent inside builder blocks, that order tracking is gated on `ordered`/`mixed_content` and on block-form construction, or that the serializer guarantees no silent data loss. This new guide covers: - the two equivalent syntaxes (appender and direct setter) - collection appender vs wholesale assignment semantics - when order tracking is active vs inactive - equivalence (and non-equivalence) with the `.tap` pattern - the no-silent-drop invariant enforced by the serializer - performance notes (no allocation when tracking is disabled)
JS build checkTriggered [ The result will appear as a |
…nsumers
The record_mutation and record_mutation_collection helpers returned nil
when order tracking was disabled (the common case). Because the
generated setters ended in a call to one of these helpers, the setters
also returned nil. This broke Ruby's setter contract: `obj.foo = v`
must evaluate to `v`.
Downstream impact: metanorma/docbook (and metanorma/uniword via the
same pattern) constructs Para instances and uses
`collection = send(attr_name) || send(:"#{attr_name}=", [])` in
`Docbook::Elements::Para#try_add_inline`. With the setters returning
nil, `collection` was nil, and `collection << element` failed with
NoMethodError.
This was a regression introduced by the earlier commit on this branch
that wired setters through record_mutation / record_mutation_collection.
The original setters' last expression was `instance_variable_set` or
the cast value, both of which returned the assigned value.
Fix: both helpers now return `value`. This makes them side-effect-only
helpers that also preserve the setter contract, regardless of whether
tracking is enabled.
Specs:
- singular setter returns the assigned value
- collection setter returns the assigned value
- the `obj.foo || (obj.foo = [])` pattern works (regression guard for
the docbook/uniword failure)
The dependent-gems-test workflow was the only place that caught this —
the bug only manifests when downstream consumer code uses the setter's
return value, which our own specs never did.
JS build checkTriggered [ The result will appear as a |
The safety net added in the earlier commit on this branch used ad-hoc nil/empty checks to decide whether to emit an uncovered element-typed rule. This diverged from the standard serialization path's skip logic (RenderPolicy#check_skip_logic?), which also considers: - using_default? (don't emit defaults the user never set) - render_nil / render_empty DSL options - value_map :to mappings - UninitializedClass sentinels - boolean value_map handling Result: metanorma/uniword round-trip specs failed because parsed models carried default-valued child elements (e.g. an empty Text model for a <t/> element that was not in the source XML). The safety net emitted those defaults as extra <t/> elements, producing XML that no longer matched the original. Fix: delegate to should_skip_value? — the same helper used by the standard rule path. The safety net now emits only values the standard path would have emitted, so it cannot introduce spurious elements. The no-silent-drop invariant is preserved for the bug class it was added for (explicit setter calls that bypass element_order), because those values are non-default and pass should_skip_value?. This subsumes the prior nil/empty/blank checks; should_skip_value? already handles them via RenderPolicy.
JS build checkTriggered [ The result will appear as a |
The safety net added earlier still over-emitted for parsed models: metanorma/uniword round-trip specs failed because parsed models carry default/uninitialized values for elements not present in the source XML (e.g. an empty <t/> child of <r/>). The standard skip logic declines to skip UninitializedClass under the default value_map (omitted: :nil), so the safety net emitted those values as extra elements. The safety net exists to catch builder-block mutations that bypass element_order tracking (the original silent-drop bug). After the Option A fix (setters record mutations), this scenario only arises inside `Klass.new do |x| ... end` construction. Parsed models trust element_order as the complete source of truth — the standard path iterates element_order and does not emit elements missing from it, so the safety net must mirror that. Fix: - Add Builder#order_tracking_enabled? as the public reader for @__order_tracking__ (avoids instance_variable_get from the serializer, which would violate encapsulation rules). - element_rule_already_emitted? short-circuits to "already emitted" (i.e. skip the safety net) when tracking is disabled. The safety net now fires only for builder-block-constructed instances where element_order could legitimately be incomplete. This restores pre-PR behavior for parsed models (element_order is authoritative) while preserving the no-silent-drop guarantee for builder-block models (the original bug case).
JS build checkTriggered [ The result will appear as a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the silent-attribute-drop bug class in
Serialize::Builder. Direct setters (x.foo = v) insideKlass.new do |x| ... endblocks were silently dropped fromto_xmlfororderedandmixed_contentmodels whenever any sibling appender call (x.foo(v)) was also present.The recent gate change from
mixed_content?toordered?in commit 4d65ba0 (PR #737) widened this bug surface frommixed_content-only models to everyorderedmodel. The regression spec that shipped with that PR only exercised the appender path, which was never broken, so it could not detect the actual bug.Root cause
Order tracking was hooked into the getter-with-arg path (
x.foo(v)), which delegated to the generated setter. The setter itself never calledtrack_order, so direct-setter mutations were missing fromelement_order. The serializer'sapply_remaining_rulesthen skipped any element-typed rule not represented inelement_order, producing silent data loss.Fix (two layers)
1. Root cause — setters now record mutations.
Serialize::Buildergains two encapsulated helpers:record_mutation(name, value)for singular attributesrecord_mutation_collection(name, value)for wholesale collection assignment (one entry per item)These replace five scattered
track_order(...) if @__order_tracking__incantations inattribute_definition.rbandinitialization.rb. Both setter and getter-with-arg paths now route through the same helpers, making them behaviourally identical.2. Safety net —
OrderedApplier#apply_remaining_rulesno longer skips element-typed rules wholesale. After the ordered pass, it emits any element-typed rule whose value would otherwise vanish. This guarantees the invariant: every attribute with a non-default value appears in serialized output, regardless of which mutation path set it. Future code paths that forget to callrecord_mutationwill degrade to declaration-order emission rather than silent data loss.Reproduction (before fix)
After the fix, both produce the expected output with all attributes present.
Test plan
spec/lutaml/model/serialize/builder_spec.rb— 18 specs covering:orderedandmixed_content.tapvs builder block equivalencespec/lutaml/model/ordered_content_spec.rbregression to exercise both appender and direct-setter syntaxesCommits
fix:builder setters record mutations + safety net inapply_remaining_rulestest:comprehensive builder specs covering silent-drop bug classdocs:newdocs/_guides/builder-dsl.adocdocumenting syntax, semantics, and invariant