Added strict cardinality validation - #720
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses Issue #185 by enforcing strict cardinality for XML deserialization: attributes that are not declared as collections (collection: true / range) now error when multiple XML elements map to a singular attribute, aligning XML behavior with existing key/value (JSON) behavior.
Changes:
- Enforce parse-time cardinality errors for non-collection
map_elementmappings when multiple elements are present. - Add specs covering strict cardinality behavior for XML vs JSON and preserving “lazy” validation behavior for ranged collections and
map_content. - Update validation documentation to clarify
collectionsemantics as cardinality (0..1 vs 0..* vs bounded ranges).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| spec/lutaml/model/cdata_spec.rb | Marks element2 as a collection to match existing test data with repeated <element2> nodes under strict cardinality. |
| spec/lutaml/model/cardinality_strict_spec.rb | Adds regression coverage for strict non-collection over-count in XML parsing and verifies unchanged lazy behaviors. |
| lib/lutaml/xml/model_transform.rb | Preserves multi-occurrence arrays for singular attrs and raises CollectionTrueMissingError during XML parse for non-collection over-count. |
| docs/_pages/validation.adoc | Clarifies that collection defines attribute cardinality and documents the related error types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4a940c9 to
d688d1a
Compare
JS build checkTriggered [ The result will appear as a |
d688d1a to
dff2d3b
Compare
JS build checkTriggered [ The result will appear as a |
JS build checkTriggered [ The result will appear as a |
JS build checkTriggered [ The result will appear as a |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
lib/lutaml/model/attribute.rb:677
- When casting list input for a custom Collection whose element type is
Lutaml::Model::Type::Hash, this branch casts each element first and then passes the casted results into the Collection initializer.Type::Hash.castcan normalize{ "text" => "x" }into the scalar string "x"; the Collection initializer then casts again (callingType::Hash.cast("x")), which will raise because it falls through tovalue.to_hfor non-Hash/Array values.
To avoid this double-cast, skip the per-element cast(...) step for custom-collection attributes when the resolved element type is a Type::Value (e.g., Type::Hash) and let the Collection initializer perform the single cast.
if collection_instance?(value) || value.is_a?(Array)
merged_opts = options.merge(resolved_type: resolved_type,
converted: true)
return build_collection(value.map do |v|
cast(v, format, register, merged_opts)
f12d9aa to
d22dd39
Compare
JS build checkTriggered [ The result will appear as a |
JS build checkTriggered [ The result will appear as a |
545d315 to
2b664f7
Compare
JS build checkTriggered [ The result will appear as a |
JS build checkTriggered [ The result will appear as a |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (2)
lib/lutaml/model/validation.rb:58
- Nested model validation currently calls
item.validatewithout threading the active register (or the child’s ownlutaml_register). That can cause nested models to validate against the default register even when the parent is validating under a non-default register, producing incorrect attribute/type resolution and inconsistent behavior with serialization (which already prefersvalue.lutaml_register). Consider passingregister:when the child#validateaccepts it, falling back to a zero-arity call for downstream overrides that don’t declare parameters.
next if Validation.visiting?(item)
sub_errors = item.validate
errors.concat(sub_errors) if sub_errors.is_a?(Array)
lib/lutaml/xml/model_transform.rb:339
- For XML mapping onto a plain Ruby
model(non-Serialize instance), cardinality violations have no later#validatephase to be reported. The new eager check only runs for non-collection attributes (!attr.collection?), so a ranged collection attribute on a PORO (e.g.,collection: 0..2) can exceed its bounds without raising anywhere. It seems safer to runattr.valid_collection!for PORO mappings regardless of whether the attribute is a collection, while still skippingmap_contentrules.
if !instance_is_serialize && attr && !attr.collection? &&
!rule.content_mapping?
attr.valid_collection!(value, context)
end
Fixes #185.
Attributes now enforce their declared cardinality. No
collection:means onevalue; giving it several is a violation.
Where the error surfaces (deviation from #185)
#185 says "it should raise an error". This reports at
#validateinstead of atparse, and
#validate!raisesValidationError. The wording still holds, thetiming differs.
Parsing keeps every value it was given. Nothing is discarded, so the violation
stays visible and
#validatereports it. Raising at parse would throw thedocument away before anyone could inspect it, and it would break every caller
that parses first and checks later. Reporting at validate is non-breaking and
matches how every other rule in this library already works.
Flagging for the issue author to confirm.
Behavior changes
reporting.
#validate.value?shorthands answerfalsewhile an attribute is over-counted.collection:attribute wraps it in a collection.All four are in
docs/_pages/breaking-changes.adoc.The xmi benchmark gate was measuring the wrong thing
The gate flagged +7% allocations. There is no regression.
ratios 0.99988 to 1.00012. Two fixtures are bit-identical.
coerce_to_collection?fires0 times, the three model_transform guards 0 times.
bench_common.rbmeasuredObjectSpace.count_objects[:TOTAL], which countsheap slots, not allocations. It only moves when Ruby adds a page.
a 3.9x spread, under-counting the true 400k by 4x.
largefails its own 1.05gate against itself at 1.0562.
GC.stat[:total_allocated_objects]. Same six runs: 400003 then400001 five times. A 1.000005x spread.
The workflow runs the PR's copy of the harness for both sides, so both readings
change together. Stored baselines feed only the push-to-main job, not this gate.
All five downstream gates share this code. Thresholds tuned loose to absorb the
old noise stay satisfied. If a non-xmi gate goes red after this, it means a
pre-existing regression stopped being hidden — this PR did not cause it.