From cd3c276db54476cd255ff5e1e8fa5d329f8eecc5 Mon Sep 17 00:00:00 2001 From: Ronald Tse Date: Sun, 26 Jul 2026 12:34:52 +0800 Subject: [PATCH 1/6] fix: builder setters record mutations so ordered serialization never silently drops attributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 4d65ba01) 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. --- .../model/serialize/attribute_definition.rb | 18 ++--- lib/lutaml/model/serialize/builder.rb | 34 ++++++++ lib/lutaml/model/serialize/initialization.rb | 5 +- .../xml/transformation/ordered_applier.rb | 80 ++++++++++++++++++- 4 files changed, 122 insertions(+), 15 deletions(-) diff --git a/lib/lutaml/model/serialize/attribute_definition.rb b/lib/lutaml/model/serialize/attribute_definition.rb index daa116c0f..c72813042 100644 --- a/lib/lutaml/model/serialize/attribute_definition.rb +++ b/lib/lutaml/model/serialize/attribute_definition.rb @@ -108,24 +108,20 @@ def define_regular_attribute_methods(name, attr) current = instance_variable_get(:"@#{name}") || [] new_value = current.is_a?(Array) ? current + [value] : value instance_variable_set(:"@#{name}", new_value) - # Track order for mixed_content serialization - track_order(name, value, nil) if @__order_tracking__ + record_mutation(name, value) value end end else # For non-collection attributes, getter accepts optional argument - # for builder-style syntax: g.description(value) sets the value + # for builder-style syntax: g.description(value) sets the value. + # Tracking happens inside the setter, so no duplicate call here. define_method(name) do |*args| if args.empty? instance_variable_get(:"@#{name}") else - # Builder-style: g.description(value) sets the value - value = args.first - public_send(:"#{name}=", value) - # Track order for mixed_content serialization - track_order(name, value, nil) if @__order_tracking__ - value + public_send(:"#{name}=", args.first) + args.first end end end @@ -156,12 +152,16 @@ def define_regular_attribute_methods(name, attr) else instance_variable_set(:"@#{name}", value) end + # Track one entry per item so element_order reflects the + # number of elements that will be emitted. + record_mutation_collection(name, value) end else define_method(:"#{name}=") do |value| value_set_for(name) value = attr.cast_value(value, lutaml_register) instance_variable_set(:"@#{name}", value) + record_mutation(name, value) end end end diff --git a/lib/lutaml/model/serialize/builder.rb b/lib/lutaml/model/serialize/builder.rb index c03dbfed3..26bd77732 100644 --- a/lib/lutaml/model/serialize/builder.rb +++ b/lib/lutaml/model/serialize/builder.rb @@ -75,6 +75,40 @@ def ordered? mapping&.ordered? || false end + # Record a singular attribute mutation in element_order. + # + # No-op unless order tracking is enabled (i.e. the instance was + # constructed via a builder block on an ordered/mixed_content + # model). This is the single entry point for singular mutations + # and is called from generated setters and getter-with-arg paths + # so that direct setters (`x.foo = v`) and appender calls + # (`x.foo(v)`) behave identically. + # + # @param attribute_name [Symbol] The attribute being mutated + # @param value [Object, nil] The value being set; stored as text + # content for content-mapped attributes, ignored otherwise + def record_mutation(attribute_name, value = nil) + return unless @__order_tracking__ + + track_order(attribute_name, value, nil) + end + + # Record a collection-attribute mutation in element_order. + # + # Emits one entry per item so that element_order length matches + # the number of serialized child elements. No-op when tracking + # is disabled, when the value is nil/empty, or when the + # collection's frozen sentinel is preserved (no real data). + # + # @param attribute_name [Symbol] The collection attribute + # @param value [Object, nil] The value assigned to the collection + def record_mutation_collection(attribute_name, value) + return unless @__order_tracking__ + return if value.nil? || Lutaml::Model::Utils.uninitialized?(value) + + Array(value).each { |item| track_order(attribute_name, item, nil) } + end + private # Intercept method calls to track order for mixed_content diff --git a/lib/lutaml/model/serialize/initialization.rb b/lib/lutaml/model/serialize/initialization.rb index c4aa72225..772e5e76d 100644 --- a/lib/lutaml/model/serialize/initialization.rb +++ b/lib/lutaml/model/serialize/initialization.rb @@ -370,7 +370,6 @@ def define_scalar_register_methods(name) instance_variable_get(:"@#{name}") else public_send(:"#{name}=", args.first) - track_order(name, args.first, nil) if @__order_tracking__ args.first end end @@ -380,6 +379,7 @@ def define_scalar_register_methods(name) reg_attr = resolve_register_attr(name) value = reg_attr.cast_value(value, lutaml_register) instance_variable_set(:"@#{name}", value) + record_mutation(name, value) end end @@ -395,7 +395,7 @@ def define_collection_register_methods(name) current = [] if current.equal?(LAZY_EMPTY_COLLECTION) new_value = current.is_a?(Array) ? current + [value] : value instance_variable_set(:"@#{name}", new_value) - track_order(name, value, nil) if @__order_tracking__ + record_mutation(name, value) value end end @@ -411,6 +411,7 @@ def define_collection_register_methods(name) else instance_variable_set(:"@#{name}", value) end + record_mutation_collection(name, value) end end diff --git a/lib/lutaml/xml/transformation/ordered_applier.rb b/lib/lutaml/xml/transformation/ordered_applier.rb index 5e92866bb..60b6c1a3d 100644 --- a/lib/lutaml/xml/transformation/ordered_applier.rb +++ b/lib/lutaml/xml/transformation/ordered_applier.rb @@ -297,7 +297,16 @@ def process_collection_item(_root, rule, value, object, element_indices, end end - # Apply remaining rules (attributes and content/raw) + # Apply remaining rules (attributes, content/raw, and any element + # rules not represented in element_order). + # + # The element-type skip that used to live here caused silent data + # loss whenever an element-typed attribute was missing from + # element_order (e.g. when a direct setter forgot to call + # track_order). After the builder-side fix all mutation paths + # record into element_order, so this branch is a defense-in-depth + # safety net: emit any element-typed rule whose value would + # otherwise vanish. # # @param root [XmlElement] Root element # @param model_instance [Object] The model instance @@ -317,21 +326,84 @@ def apply_remaining_rules(_root, model_instance, options, compiled_rules end + emitted_counts = element_order_coverage(model_instance, compiled_rules) + rules_to_apply.each do |rule| - next if rule.option(:mapping_type) == :element + mapping_type = rule.option(:mapping_type) - # Skip content rules if we processed text nodes from element_order - if %i[content raw].include?(rule.option(:mapping_type)) && + # Skip content/raw if mixed or text nodes were processed + if %i[content raw].include?(mapping_type) && (mapping&.mixed_content? || processed_text_nodes) next end + if mapping_type == :element && + element_rule_already_emitted?(rule, model_instance, + emitted_counts) + next + end + next unless valid_mapping?(rule, options) yield(:apply_rule, rule, nil) if block_given? end end + # Count, per element-typed rule, how many entries in element_order + # already cover it. Returns a Hash keyed by CompiledRule identity. + # + # @param model_instance [Object] The model instance + # @param compiled_rules [Array] The compiled rules + # @return [Hash] Coverage counts + def element_order_coverage(model_instance, compiled_rules) + counts = ::Hash.new(0) + return counts unless model_instance.respond_to?(:element_order) + return counts unless (order = model_instance.element_order) + + element_rules = compiled_rules.select do |r| + r.is_a?(::Lutaml::Model::CompiledRule) && + r.option(:mapping_type) == :element + end + + order.each do |object| + next unless object.type == "Element" + + object_ns_uri = object.namespace_uri + matched = element_rules.find do |r| + matches_element_rule?(r, object.name, object_ns_uri) + end + counts[matched] += 1 if matched + end + + counts + end + + # Whether an element-typed rule has already been fully emitted + # via element_order. For singular rules, a single match covers it + # (or a nil/empty value, which emits nothing). For collection + # rules, coverage requires the entry count to meet the value's length. + # + # @param rule [CompiledRule] The element rule + # @param model_instance [Object] The model instance + # @param emitted_counts [Hash] Coverage map + # @return [Boolean] + def element_rule_already_emitted?(rule, model_instance, +emitted_counts) + emitted = emitted_counts[rule] + value = extract_ordered_rule_value(rule, model_instance) + + if rule.collection? + value_length = value.respond_to?(:length) ? value.length : 0 + emitted >= value_length + else + return true if emitted.positive? + return true if value.nil? + return true if value.respond_to?(:empty?) && value.empty? + + false + end + end + # Sort compiled rules so attribute rules follow the captured attribute_order. # Non-attribute rules (content, raw) maintain their original position. # From d0ac82ee9828dc73bbfad062ae8c9ab14651c3aa Mon Sep 17 00:00:00 2001 From: Ronald Tse Date: Sun, 26 Jul 2026 12:35:11 +0800 Subject: [PATCH 2/6] test: add comprehensive builder specs covering silent-drop bug class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The regression spec that shipped with commit 4d65ba01 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. --- spec/lutaml/model/ordered_content_spec.rb | 25 +- spec/lutaml/model/serialize/builder_spec.rb | 330 ++++++++++++++++++++ 2 files changed, 348 insertions(+), 7 deletions(-) create mode 100644 spec/lutaml/model/serialize/builder_spec.rb diff --git a/spec/lutaml/model/ordered_content_spec.rb b/spec/lutaml/model/ordered_content_spec.rb index 43914aa90..ed7d083b7 100644 --- a/spec/lutaml/model/ordered_content_spec.rb +++ b/spec/lutaml/model/ordered_content_spec.rb @@ -148,7 +148,9 @@ class Schema < Lutaml::Model::Serializable # Regression for issue #735: `ordered` builder must honour setter # call order, not declaration order. The @__order_tracking__ flag - # was gated on mixed_content? instead of ordered?. + # was gated on mixed_content? instead of ordered?. Comprehensive + # coverage of the setter/appender equivalence lives in + # spec/lutaml/model/serialize/builder_spec.rb. context "ordered builder honours setter call order" do it "emits elements in builder-call order, not declaration order" do klass = Class.new(Lutaml::Model::Serializable) do @@ -163,16 +165,25 @@ class Schema < Lutaml::Model::Serializable end end - item = klass.new do |i| + # Both the appender syntax and the direct setter syntax MUST + # honour call order and MUST NOT silently drop the other. + item_appender = klass.new do |i| i.b "first" i.a "second" end - xml = item.to_xml - b_pos = xml.index("first") - a_pos = xml.index("second") - expect(b_pos).to be < a_pos - expect(item.element_order.map(&:name)).to eq(%w[b a]) + item_setter = klass.new do |i| + i.b = "first" + i.a = "second" + end + + [item_appender, item_setter].each do |item| + xml = item.to_xml + b_pos = xml.index("first") + a_pos = xml.index("second") + expect(b_pos).to be < a_pos + expect(item.element_order.map(&:name)).to eq(%w[b a]) + end end end end diff --git a/spec/lutaml/model/serialize/builder_spec.rb b/spec/lutaml/model/serialize/builder_spec.rb new file mode 100644 index 000000000..be3bc9605 --- /dev/null +++ b/spec/lutaml/model/serialize/builder_spec.rb @@ -0,0 +1,330 @@ +# frozen_string_literal: true + +require "spec_helper" +require_relative "../../../../lib/lutaml/model" +require "lutaml/xml/adapter/nokogiri_adapter" + +# Specs for Lutaml::Model::Serialize::Builder — the module that gives +# Serializable the `Klass.new do |x| ... end` block syntax. +# +# These specs exist because of a regression where direct setters +# (`x.attr = v`) inside the builder block silently dropped attributes +# from serialized output while appender calls (`x.attr(v)`) worked. +# The root cause was that only getter-with-arg paths called track_order; +# generated setters did not. The fix centralises mutation recording via +# Builder#record_mutation / record_mutation_collection, and the +# serializer's apply_remaining_rules acts as a safety net so element +# values are never silently dropped. +# +# These specs cover the invariant ("no silent data loss"), call-order +# preservation, round-trip parity, equivalence between .tap and block, +# the no-op guarantee for non-ordered models, and the wholesale +# collection assignment case. +RSpec.describe "Lutaml::Model::Serialize::Builder" do + let(:ordered_klass) do + Class.new(Lutaml::Model::Serializable) do + attribute :singular, :string + attribute :items, :string, collection: true + + xml do + element "container" + ordered + map_element "singular", to: :singular + map_element "items", to: :items + end + end + end + + let(:mixed_klass) do + Class.new(Lutaml::Model::Serializable) do + attribute :singular, :string + attribute :items, :string, collection: true + + xml do + element "container" + mixed_content + map_element "singular", to: :singular + map_element "items", to: :items + end + end + end + + let(:plain_klass) do + Class.new(Lutaml::Model::Serializable) do + attribute :singular, :string + attribute :items, :string, collection: true + + xml do + element "container" + map_element "singular", to: :singular + map_element "items", to: :items + end + end + end + + shared_examples "no silent data loss across mutation styles" do + it "appender-only: emits every call in order" do + obj = ordered_klass.new do |x| + x.singular "first" + x.items "a" + x.items "b" + end + + xml = obj.to_xml + expect(xml).to be_xml_equivalent_to(<<~XML) + + first + a + b + + XML + expect(obj.element_order.map(&:name)).to eq(%w[singular items items]) + end + + it "direct setter for singular: still emits the element" do + obj = ordered_klass.new do |x| + x.singular = "first" + x.items "a" + end + + xml = obj.to_xml + expect(xml).to be_xml_equivalent_to(<<~XML) + + first + a + + XML + expect(obj.element_order.map(&:name)).to eq(%w[singular items]) + end + + it "wholesale collection assignment: emits one element per item" do + obj = ordered_klass.new do |x| + x.singular "first" + x.items = %w[a b c] + end + + xml = obj.to_xml + expect(xml).to be_xml_equivalent_to(<<~XML) + + first + a + b + c + + XML + expect(obj.element_order.map(&:name)).to eq(%w[singular items items items]) + end + + it "all direct setters: emits in call order" do + obj = ordered_klass.new do |x| + x.singular = "first" + x.items = %w[a b] + end + + xml = obj.to_xml + expect(xml).to be_xml_equivalent_to(<<~XML) + + first + a + b + + XML + end + + it "mixed style preserves call order across setters and appenders" do + obj = ordered_klass.new do |x| + x.items "a" + x.singular = "middle" + x.items "b" + end + + xml = obj.to_xml + expect(xml).to be_xml_equivalent_to(<<~XML) + + a + middle + b + + XML + end + end + + describe "no silent data loss" do + it_behaves_like "no silent data loss across mutation styles" + + it "mixed_content model emits setter-only attributes too" do + obj = mixed_klass.new do |x| + x.singular = "first" + x.items "a" + end + + xml = obj.to_xml + expect(xml).to include("first") + expect(xml).to include("a") + expect(obj.element_order.map(&:name)).to eq(%w[singular items]) + end + end + + describe "call order preservation" do + it "reverse declaration order is honoured via direct setters" do + obj = ordered_klass.new do |x| + x.items = %w[a b] + x.singular = "last" + end + + xml = obj.to_xml + items_pos = xml.index("") + singular_pos = xml.index("") + expect(items_pos).to be < singular_pos + expect(obj.element_order.map(&:name)).to eq(%w[items items singular]) + end + + it "reverse declaration order is honoured via appenders" do + obj = ordered_klass.new do |x| + x.items "a" + x.singular "last" + end + + xml = obj.to_xml + items_pos = xml.index("") + singular_pos = xml.index("") + expect(items_pos).to be < singular_pos + expect(obj.element_order.map(&:name)).to eq(%w[items singular]) + end + end + + describe "round-trip parity" do + it "ordered: parses and re-emits equivalent XML" do + original = <<~XML + + a + middle + b + + XML + + parsed = ordered_klass.from_xml(original) + expect(parsed.to_xml).to be_xml_equivalent_to(original) + end + + it "mixed_content: parses and re-emits equivalent XML" do + original = <<~XML + before a mid x end + XML + + parsed = mixed_klass.from_xml(original) + roundtrip = parsed.to_xml + expect(roundtrip).to include("a") + expect(roundtrip).to include("x") + end + end + + describe ".tap vs builder block equivalence" do + it "produces identical element_order presence and XML shape" do + via_tap = ordered_klass.new.tap do |x| + x.singular = "first" + x.items = %w[a b] + end + + via_block = ordered_klass.new do |x| + x.singular = "first" + x.items = %w[a b] + end + + # .tap path does not enable tracking; both must still emit the same XML. + expect(via_tap.element_order).to be_nil + expect(via_block.element_order).not_to be_nil + expect(via_tap.to_xml).to be_xml_equivalent_to(via_block.to_xml) + end + end + + describe "no-op guarantee when not tracking" do + it "plain (non-ordered) models do not allocate element_order entries" do + obj = plain_klass.new do |x| + x.singular = "first" + x.items "a" + end + + expect(obj.element_order).to be_nil + expect { obj.to_xml }.not_to raise_error + end + + it "ordered models constructed without a block do not track" do + obj = ordered_klass.new + obj.singular = "first" + obj.items = %w[a b] + + expect(obj.element_order).to be_nil + expect(obj.to_xml).to include("first") + end + end + + describe "wholesale collection reassignment" do + it "tracks one entry per item across multiple assignments" do + obj = ordered_klass.new do |x| + x.items = %w[a b] + x.items = %w[c d e] + end + + # element_order is a log of mutations, not a reflection of final state, + # so both assignments are recorded. The serializer emits the current + # value of the attribute via element_order in recorded order; the + # safety net guarantees no data loss. + expect(obj.element_order.map(&:name)).to eq(%w[items items items items items]) + xml = obj.to_xml + # The serialized output must contain all current items + expect(xml).to include("c") + expect(xml).to include("d") + expect(xml).to include("e") + end + end + + describe "no-silent-drop invariant (cross-cutting)" do + # The bug class: any attribute with a non-default value MUST appear in + # the serialized output, regardless of how it was set. This spec is the + # regression guard for the whole class of bug, not just one instance. + shared_examples "no silent drop" do |description, mutation_block| + it "does not silently drop any attribute set via: #{description}" do + obj = ordered_klass.new(&mutation_block) + xml = obj.to_xml + + if !obj.singular.nil? && !obj.singular.to_s.empty? + expect(xml).to include(""), "singular was dropped from output" + end + return if obj.items.nil? || obj.items.empty? + + obj.items.each do |item| + expect(xml).to include("#{item}"), + "items element for #{item.inspect} was dropped from output" + end + end + end + + it_behaves_like "no silent drop", + "singular setter + wholesale items setter", + lambda { |x| + x.singular = "v" + x.items = %w[a b] + } + it_behaves_like "no silent drop", + "all appenders", + lambda { |x| + x.singular "v" + x.items "a" + x.items "b" + } + it_behaves_like "no silent drop", + "singular setter + items appender (the original bug)", + lambda { |x| + x.singular = "v" + x.items "a" + x.items "b" + } + it_behaves_like "no silent drop", + "singular appender + wholesale items setter", + lambda { |x| + x.singular "v" + x.items = %w[a b] + } + end +end From 854953746511839679ddd431867ea7b3304bc4a9 Mon Sep 17 00:00:00 2001 From: Ronald Tse Date: Sun, 26 Jul 2026 12:35:25 +0800 Subject: [PATCH 3/6] docs: document the Serialize::Builder DSL and its order-tracking semantics 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) --- docs/_guides/builder-dsl.adoc | 127 ++++++++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) create mode 100644 docs/_guides/builder-dsl.adoc diff --git a/docs/_guides/builder-dsl.adoc b/docs/_guides/builder-dsl.adoc new file mode 100644 index 000000000..fca2fdb9a --- /dev/null +++ b/docs/_guides/builder-dsl.adoc @@ -0,0 +1,127 @@ +--- +title: Builder DSL +nav_order: 99 +--- + += Builder DSL +:toc: +:toclevels: 3 + +The Builder DSL gives any `Lutaml::Model::Serializable` class a block-based +construction syntax: + +[source,ruby] +---- +person = Person.new do |p| + p.name "Ada" + p.email "ada@example.com" +end +---- + +It works for every model. For models declared with `ordered` or +`mixed_content`, the builder also records the order in which attributes were +mutated so that `to_xml` can emit child elements in the same order. + +== Two equivalent syntaxes + +Every attribute supports both the *appender* form and the *direct setter* form +inside the block. They are interchangeable: + +[source,ruby] +---- +# Appender form: looks like a method call with the value as argument +person = Person.new do |p| + p.name "Ada" # equivalent to p.name = "Ada" + p.email "ada@x.org" # equivalent to p.email = "ada@x.org" +end + +# Direct setter form: looks like a normal assignment +person = Person.new do |p| + p.name = "Ada" + p.email = "ada@x.org" +end +---- + +Both snippets produce identical `to_xml`, `to_json`, etc. Use whichever reads +better in context. + +=== Collections + +Collection attributes accept both forms too: + +[source,ruby] +---- +group = Group.new do |g| + # Append one item at a time: + g.member(person1) + g.member(person2) + + # Or assign the whole collection at once: + g.members = [person1, person2, person3] +end +---- + +The wholesale assignment records one order-entry per item, so the resulting +`element_order` length matches the number of serialized child elements. + +== Order tracking and serialization + +The builder tracks call order only when *all* of the following are true: + +1. The instance was constructed via `Klass.new do |x| ... end` (a block was + passed). +2. The XML mapping is declared with `ordered` or `mixed_content`. + +When tracking is active, every mutation (via either syntax) is appended to the +instance's `element_order`. `to_xml` then iterates `element_order` to emit +child elements in the order they were set. + +When tracking is *not* active (no block, or the model is neither `ordered` nor +`mixed_content`), `element_order` stays `nil` and `to_xml` emits elements in +declaration order. + +=== Equivalence with `.tap` + +The block form is the only way to enable tracking. Constructing the instance +separately and mutating it afterwards does not enable tracking: + +[source,ruby] +---- +# Tracking active: +item = Item.new { |i| i.b = "first"; i.a = "second" } +item.element_order.map(&:name) # => ["b", "a"] + +# Tracking NOT active: +item = Item.new +item.b = "first" +item.a = "second" +item.element_order # => nil +# Falls back to declaration order for serialization. +---- + +If you want call-order preservation, use the block form. + +== No silent data loss (invariant) + +The serializer guarantees that every attribute with a non-default value appears +in the serialized output, regardless of which syntax was used to set it. + +This is enforced by `OrderedApplier#apply_remaining_rules`: any element-typed +attribute not represented in `element_order` is emitted in declaration order +after the tracked entries. This is a safety net on top of the builder's own +tracking, so that future code paths (new mutation helpers, attribute merge +operations, etc.) cannot silently drop user data. + +If you find an attribute missing from `to_xml`, the right answer is to fix the +mutation path so it records into `element_order`. The safety net guarantees +correctness; it does not guarantee order. + +== Performance notes + +For models that are neither `ordered` nor `mixed_content`, the builder does +not allocate any `Lutaml::Xml::Element` order-tracking objects. The check is +`@__order_tracking__` (a boolean), and the recording helpers are no-ops. + +The check is also a no-op for `ordered`/`mixed_content` models constructed +without a block. This keeps the common case (`Klass.from_xml(xml)` followed +by `to_xml`) free of tracking overhead. From aa58332cdf1e147286ff089a86ec9707a60b6954 Mon Sep 17 00:00:00 2001 From: Ronald Tse Date: Mon, 27 Jul 2026 09:21:18 +0800 Subject: [PATCH 4/6] fix(builder): preserve setter return value contract for downstream consumers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- lib/lutaml/model/serialize/builder.rb | 17 ++++++++++--- spec/lutaml/model/serialize/builder_spec.rb | 28 +++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/lib/lutaml/model/serialize/builder.rb b/lib/lutaml/model/serialize/builder.rb index 26bd77732..9b003f76c 100644 --- a/lib/lutaml/model/serialize/builder.rb +++ b/lib/lutaml/model/serialize/builder.rb @@ -84,13 +84,19 @@ def ordered? # so that direct setters (`x.foo = v`) and appender calls # (`x.foo(v)`) behave identically. # + # Returns `value` so generated setters preserve Ruby's setter + # contract: `obj.foo = v` evaluates to `v`. Callers like + # `obj.foo || (obj.foo = [])` depend on this. + # # @param attribute_name [Symbol] The attribute being mutated # @param value [Object, nil] The value being set; stored as text # content for content-mapped attributes, ignored otherwise + # @return [Object] the passed value def record_mutation(attribute_name, value = nil) - return unless @__order_tracking__ + return value unless @__order_tracking__ track_order(attribute_name, value, nil) + value end # Record a collection-attribute mutation in element_order. @@ -100,13 +106,18 @@ def record_mutation(attribute_name, value = nil) # is disabled, when the value is nil/empty, or when the # collection's frozen sentinel is preserved (no real data). # + # Returns `value` so generated setters preserve Ruby's setter + # contract (see {record_mutation}). + # # @param attribute_name [Symbol] The collection attribute # @param value [Object, nil] The value assigned to the collection + # @return [Object] the passed value def record_mutation_collection(attribute_name, value) - return unless @__order_tracking__ - return if value.nil? || Lutaml::Model::Utils.uninitialized?(value) + return value unless @__order_tracking__ + return value if value.nil? || Lutaml::Model::Utils.uninitialized?(value) Array(value).each { |item| track_order(attribute_name, item, nil) } + value end private diff --git a/spec/lutaml/model/serialize/builder_spec.rb b/spec/lutaml/model/serialize/builder_spec.rb index be3bc9605..f9d17594a 100644 --- a/spec/lutaml/model/serialize/builder_spec.rb +++ b/spec/lutaml/model/serialize/builder_spec.rb @@ -238,6 +238,34 @@ end end + describe "setter return value contract" do + # Ruby's setter contract: `obj.foo = v` evaluates to `v`. Callers + # like `obj.foo || (obj.foo = [])` depend on this. Generated + # setters must continue to honour this contract regardless of + # whether order tracking is enabled. + it "singular setter returns the assigned value" do + obj = plain_klass.new + expect((obj.singular = "v")).to eq("v") + end + + it "collection setter returns the assigned value" do + obj = plain_klass.new + expect((obj.items = %w[a b])).to eq(%w[a b]) + end + + it "supports the `obj.foo || (obj.foo = [])` pattern from downstream consumers" do + obj = plain_klass.new + # Mirror Docbook::Elements::Para#try_add_inline, which relies on + # the setter returning the array (not nil) when the getter is nil. + # The bug: when the setter returned nil (because record_mutation + # returned nil with tracking disabled), `collection` was nil and + # `collection << element` failed with NoMethodError. + collection = obj.items || (obj.items = []) + expect(collection).to eq([]) + expect { collection << "x" }.not_to raise_error + end + end + describe "no-op guarantee when not tracking" do it "plain (non-ordered) models do not allocate element_order entries" do obj = plain_klass.new do |x| From 4d574a6c18cecfffa4419a5ad80ddeb39be38191 Mon Sep 17 00:00:00 2001 From: Ronald Tse Date: Mon, 27 Jul 2026 11:09:13 +0800 Subject: [PATCH 5/6] fix(ordered-applier): safety net must respect standard skip logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 element that was not in the source XML). The safety net emitted those defaults as extra 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. --- .../xml/transformation/ordered_applier.rb | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/lutaml/xml/transformation/ordered_applier.rb b/lib/lutaml/xml/transformation/ordered_applier.rb index 60b6c1a3d..68e999b77 100644 --- a/lib/lutaml/xml/transformation/ordered_applier.rb +++ b/lib/lutaml/xml/transformation/ordered_applier.rb @@ -379,9 +379,15 @@ def element_order_coverage(model_instance, compiled_rules) end # Whether an element-typed rule has already been fully emitted - # via element_order. For singular rules, a single match covers it - # (or a nil/empty value, which emits nothing). For collection - # rules, coverage requires the entry count to meet the value's length. + # via element_order (or should not be emitted at all by the safety + # net). Returns true when: + # - the standard skip logic says the value should be skipped + # (handles defaults, render_nil/render_empty, value_map, etc.) + # - or the rule was fully covered by element_order entries + # Subsumes the prior nil/empty checks by delegating to the same + # skip logic used by the standard serialization path, so the + # safety net cannot emit a value the standard path would have + # skipped. # # @param rule [CompiledRule] The element rule # @param model_instance [Object] The model instance @@ -389,18 +395,15 @@ def element_order_coverage(model_instance, compiled_rules) # @return [Boolean] def element_rule_already_emitted?(rule, model_instance, emitted_counts) - emitted = emitted_counts[rule] value = extract_ordered_rule_value(rule, model_instance) + return true if should_skip_value?(value, rule, model_instance) + emitted = emitted_counts[rule] if rule.collection? value_length = value.respond_to?(:length) ? value.length : 0 emitted >= value_length else - return true if emitted.positive? - return true if value.nil? - return true if value.respond_to?(:empty?) && value.empty? - - false + emitted.positive? end end From 3fad6fdbc439cd01f38789dd1e133c9775c3606b Mon Sep 17 00:00:00 2001 From: Ronald Tse Date: Mon, 27 Jul 2026 12:30:54 +0800 Subject: [PATCH 6/6] fix(ordered-applier): restrict safety net to builder-block models MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 child of ). 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). --- lib/lutaml/model/serialize/builder.rb | 10 +++++++ .../xml/transformation/ordered_applier.rb | 27 ++++++++++++++++--- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/lib/lutaml/model/serialize/builder.rb b/lib/lutaml/model/serialize/builder.rb index 9b003f76c..1300d7bf4 100644 --- a/lib/lutaml/model/serialize/builder.rb +++ b/lib/lutaml/model/serialize/builder.rb @@ -75,6 +75,16 @@ def ordered? mapping&.ordered? || false end + # Whether this instance was constructed via a builder block and + # therefore records mutations into element_order. Parsed models + # and instances constructed without a block do not track; their + # element_order (if any) comes from the parser and is treated as + # the complete source of truth by the serializer. + # @return [Boolean] + def order_tracking_enabled? + @__order_tracking__ ? true : false + end + # Record a singular attribute mutation in element_order. # # No-op unless order tracking is enabled (i.e. the instance was diff --git a/lib/lutaml/xml/transformation/ordered_applier.rb b/lib/lutaml/xml/transformation/ordered_applier.rb index 68e999b77..77303d499 100644 --- a/lib/lutaml/xml/transformation/ordered_applier.rb +++ b/lib/lutaml/xml/transformation/ordered_applier.rb @@ -381,13 +381,21 @@ def element_order_coverage(model_instance, compiled_rules) # Whether an element-typed rule has already been fully emitted # via element_order (or should not be emitted at all by the safety # net). Returns true when: + # - the model was not constructed via builder block + # (`@__order_tracking__` is nil/false): parsed models trust + # element_order as the complete source of truth, so the safety + # net must not second-guess it by emitting defaults/uninitialized + # values that the standard path would otherwise have skipped. # - the standard skip logic says the value should be skipped # (handles defaults, render_nil/render_empty, value_map, etc.) # - or the rule was fully covered by element_order entries - # Subsumes the prior nil/empty checks by delegating to the same - # skip logic used by the standard serialization path, so the - # safety net cannot emit a value the standard path would have - # skipped. + # + # The safety net targets the bug class where a builder-block + # construction bypassed element_order tracking (e.g. via a + # mutation path that forgot to call record_mutation). After + # Option A, all setter/getter paths record into element_order, + # so this branch is defense-in-depth rather than the common + # path. # # @param rule [CompiledRule] The element rule # @param model_instance [Object] The model instance @@ -395,6 +403,8 @@ def element_order_coverage(model_instance, compiled_rules) # @return [Boolean] def element_rule_already_emitted?(rule, model_instance, emitted_counts) + return true unless model_order_tracking_enabled?(model_instance) + value = extract_ordered_rule_value(rule, model_instance) return true if should_skip_value?(value, rule, model_instance) @@ -407,6 +417,15 @@ def element_rule_already_emitted?(rule, model_instance, end end + # Whether the model was constructed via a builder block and thus + # has order tracking enabled. Only such models are candidates for + # the safety net; parsed models trust element_order as-is. + def model_order_tracking_enabled?(model_instance) + return false unless model_instance.respond_to?(:order_tracking_enabled?) + + model_instance.order_tracking_enabled? + end + # Sort compiled rules so attribute rules follow the captured attribute_order. # Non-attribute rules (content, raw) maintain their original position. #