diff --git a/docs/_guides/builder-dsl.adoc b/docs/_guides/builder-dsl.adoc new file mode 100644 index 00000000..fca2fdb9 --- /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. diff --git a/lib/lutaml/model/serialize/attribute_definition.rb b/lib/lutaml/model/serialize/attribute_definition.rb index daa116c0..c7281304 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 c03dbfed..1300d7bf 100644 --- a/lib/lutaml/model/serialize/builder.rb +++ b/lib/lutaml/model/serialize/builder.rb @@ -75,6 +75,61 @@ 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 + # 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. + # + # 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 value unless @__order_tracking__ + + track_order(attribute_name, value, nil) + value + 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). + # + # 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 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 # 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 c4aa7222..772e5e76 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 5e92866b..77303d49 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,106 @@ 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 (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 + # + # 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 + # @param emitted_counts [Hash] Coverage map + # @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) + + emitted = emitted_counts[rule] + if rule.collection? + value_length = value.respond_to?(:length) ? value.length : 0 + emitted >= value_length + else + emitted.positive? + 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. # diff --git a/spec/lutaml/model/ordered_content_spec.rb b/spec/lutaml/model/ordered_content_spec.rb index 43914aa9..ed7d083b 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 00000000..f9d17594 --- /dev/null +++ b/spec/lutaml/model/serialize/builder_spec.rb @@ -0,0 +1,358 @@ +# 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 "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| + 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