Serialize derived attributes during xml and key_value serialization#660
Serialize derived attributes during xml and key_value serialization#660HassanAkbar merged 4 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes #639 by ensuring derived (method:) attributes are not incorrectly treated as “defaulted” after XML deserialization, so they continue to serialize in downstream formats.
Changes:
- Add derived-attribute detection to bypass “skip default” logic in
RenderPolicy. - Ensure
MappingRule#render?renders derived attributes even whenusing_default?is true. - Add an RSpec covering derived attribute serialization after XML deserialization (YAML + JSON/key-value).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/lutaml/model/render_policy.rb |
Avoids skipping derived attributes due to default-render policy. |
lib/lutaml/model/mapping/mapping_rule.rb |
Forces derived attributes to render even when instance reports “using default”. |
spec/lutaml/model/derived_attribute_serialization_spec.rb |
Adds regression coverage for YAML + JSON after XML deserialization. |
Comments suppressed due to low confidence (1)
lib/lutaml/model/render_policy.rb:100
should_skip_default?now callsderived_attribute?unconditionally, which does an attribute lookup on every non-nil/non-empty value even whenusing_default?is false (the common case for instances created via.new). To avoid a potential serialization hot-path regression, consider only checkingderived_attribute?inside the existingusing_default? && !render_defaultbranch (i.e., only when the rule would otherwise be skipped due to defaults).
def should_skip_default?(value, rule, context_obj, attr_name)
return false if derived_attribute?(context_obj, attr_name)
# Skip if context object is using default and render_default is false
# But for collections, check if they were mutated (non-empty)
if context_obj.respond_to?(:using_default?) &&
context_obj.using_default?(attr_name) &&
!extract_option(rule, :render_default)
# For collections: if mutated to non-empty, serialize them
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # frozen_string_literal: true | ||
|
|
||
| require "spec_helper" | ||
| class DerivedAttributeParent < Lutaml::Model::Serializable |
There was a problem hiding this comment.
For custom classes defined in specs, we should wrap this in a module just for this spec to prevent pollution. We should add this rule to CLAUDE.md or AGENT.md. Or, use anonymous classes.
There was a problem hiding this comment.
@ronaldtse wrapped the class inside a module to prevent pollution. Let me know if you want me to change anything else. Thanks.
|
@HassanAkbar LGTM feel free to merge when ready. |
|
@ronaldtse Sure, I’ll merge it once the CI is green. |
fixes #639