forms: recurse into nested-aggregate members' schemas to any depth - #35
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Yaraslaut
force-pushed
the
feature/25-nested-aggregate-forms
branch
from
August 4, 2026 16:38
9b16554 to
371386b
Compare
mergeSchemaExtras previously annotated (x-order, required, title, Quantity/ Choice/widget hints) only an action's own top-level members. A member that was itself a reflectable aggregate -- a nested struct, or std::vector<Sub> -- got none of that on its own sub-members, and its $defs entry got no required array at all, per docs/spec/forms/forms.md's documented "flat actions only" boundary. Domains that are naturally nested (a measurement with a repeated specimen sub-record) had to either flatten the action type (impossible for a repeated sub-record without a fixed max count) or hand-write the form, bypassing the schema-driven generator for exactly the screens that would benefit most from it. Recurse one level into a nested-aggregate member's own schema, applying the same annotation rules the top level already applies (factored into a shared annotateBasicMemberProperty helper). Two schema shapes exist for a nested aggregate, both handled: glaze deduplicates via a shared $defs entry referenced by $ref when the nested type is used two or more times anywhere in the schema, or inlines the object schema directly into the property when it is used exactly once -- annotateNestedAggregateRef resolves whichever form applies before recursing. Deliberately capped at exactly one level, matching the design doc's own suggested bound: a nested aggregate's own nested-aggregate members are left unannotated, and computed fields/formLayout/fieldSpans/formRules stay top-level-only. Purely additive -- an action with no nested-aggregate member has nothing to trigger on, so its schema is byte-for-byte unchanged (the full existing suite, including every prior schema-generation test, passes unmodified). Render-side add/remove affordances for a repeated-aggregate array are a separate, downstream renderer concern the issue explicitly called out as distinct from the annotation-reach gap this closes, and are out of scope here. Closes #25 Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
The nested-aggregate annotation pass (issue #25) shares annotateBasicMemberProperty with the top-level pass, so it applies the same FieldMeta/Quantity/Choice/widget/ranged-bounds rules to a nested member's own properties -- but every existing nested-forms fixture (Specimen, Attachment, Provenance) only has plain scalar members, so those branches were never exercised through the nested path. Add RichSub/RichRecord, a nested type whose own members carry a FieldMeta override, a Quantity, and a Choice, and assert the resolved nested schema carries the expected x-placeholder/ x-readonly/x-hidden/x-decimalPlaces/x-optionsAction annotations. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
…gaps Re-applies two fixes that were accidentally dropped by a stale-branch rebase (the local branch used for the rebase predated these commits on origin, so `git rebase origin/master` + force-push clobbered them). 1. UnitTraits<NestedFormUnit>::meta()'s switch had only a `kg` case plus `default`, tripping -Wswitch-enum (part of -Weverything, and not in apply_warnings()'s opt-out list) since it requires every enumerator named regardless of a default label. Add an explicit `case NestedFormUnit::scalar:` falling through to the existing default body. 2. codecov/patch still failed (79.41%, target 97.11%) after fix #1 because three annotation branches in the nested-aggregate path this PR adds -- FieldMeta::i18nKey, FieldMeta::widget, and Quantity::unitAlternatives() -- were structurally unreachable by the RichSub/RichRecord fixture. Declare a g<->kg unit relation and set i18nKey/widget on the fixture, and assert on x-i18nKey/x-widget/x-unitAlternatives at the nested level. Verified locally with llvm-cov: all three branches now execute at least once; full suite (826 cases) still passes. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Six new cases: a FieldMeta entry with no optional attributes set, a nested Quantity member with no unit alternatives, optionalFields applied one level down, and annotateNestedAggregateRef's three defensive fallbacks (non-string $ref, $ref outside #/$defs/, neither $ref nor properties) via direct detail:: calls, since glaze itself never produces those malformed shapes through the public schemaJson<A>() path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replaces the "one level" cap documented for mergeSchemaExtras with cycle-guarded recursion to arbitrary depth: a nested-aggregate member's own nested-aggregate members are now annotated too, however deep the type graph goes, stopping only (via a static_assert) when a member's type would repeat a type already on the current ancestor chain. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Three review passes: fixed a terminology wobble (the ancestor chain's first element is the action type, not itself a "nested-aggregate type"), tightened the cycle-detection paragraph's phrasing, and closed an ambiguity about when the static_assert actually fires (only when the offending type is nested under an action's schema, not for the type standing alone). Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Three tasks: extend test fixtures with a failing three-level-deep test (TDD red), implement the Ancestors-threaded cycle-guarded recursion in forms.hpp (TDD green), then add a standalone self-referential-type test plus a Doxygen docs-build verification pass. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Replaces the one-level "depth cap" test with a failing test proving recursion should continue past one level: DeepSpecimen -> Provenance -> Origin, three levels deep. Also adds a self-referential TreeNode fixture (never passed to schemaJson<A>() here -- that would trip the forthcoming cycle guard) proving such a type is unaffected on its own. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Threads an Ancestors... template parameter pack through annotateNestedAggregate/annotateNestedAggregateRef -- the chain of nested-aggregate types already being annotated on the current path, starting with the action type. Factors the "is this member itself a nested aggregate, and should I recurse into it" decision (previously duplicated inline in mergeSchemaExtras's loop, and entirely absent from annotateNestedAggregate since it never went past one level) into one shared recurseIntoNestedAggregateIfAny, used by both loops. A member whose type -- or, for std::vector<Sub>, Sub -- already appears on the ancestor chain would recurse into this same instantiation again, forever. Rather than let that happen, a static_assert (dependent on the member type and the chain, so it only fires for the actual cyclic instantiation) rejects a self- or mutually-referential nested-aggregate type graph at compile time instead. See docs/spec/forms/forms.md, "Nested aggregates (recursive, cycle-guarded)". Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Two doc-only fixes caught by task review: annotateNestedAggregate's @tparam Ancestors said the chain ends with Sub, when Sub is actually appended only when recursing one level deeper (contradicting the sibling comment on annotateNestedAggregateRef, which already had it right). The file's top-of-header public API doc still described the old one-level cap and pointed at a spec section title that no longer exists. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Proves TreeNode -- a tree-shaped, self-referential nested-aggregate type -- round-trips through glaze JSON encode/decode normally on its own. It is never passed to schemaJson<A>() in this file; doing so would trip forms.hpp's cycle-guard static_assert, which this test suite has no harness to exercise directly (see docs/spec/forms/forms.md, "Nested aggregates (recursive, cycle-guarded)"). Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Three Important findings from the final whole-branch review: mergeSchemaExtras duplicated annotateBasicMemberProperty's ~90-line body instead of calling it (the doc comment already falsely claimed this sharing existed); annotateNestedAggregateRef's $defs lookup inserted an empty entry on a miss, which the new recursion turned from dormant into a live use-after-free risk (glz::generic_u64's object storage reallocates on insert); and the spec's normative renderer-contract section still asserted nested $defs are never patched, which this branch's whole point contradicts. Also fixes a broken anchor and adds a one-sentence compatibility caveat the same review flagged as cheap Minor fixes. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Yaraslaut
force-pushed
the
feature/25-nested-aggregate-forms
branch
from
August 5, 2026 10:11
2a72ea9 to
820a993
Compare
Yaraslaut
changed the title
The plan document served its purpose during implementation; the spec (docs/spec/forms/forms.md) and git history are the lasting record. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
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.
Issue #25 asked whether flat-only form generation was a deliberate boundary or a gap worth closing.
mergeSchemaExtraspreviously annotated (x-order,required,title,Quantity/Choice/widget hints) only an action's own top-level members — a member that was itself a reflectable aggregate (a nested struct, orstd::vector<Sub>) got none of that on its own sub-members. This PR closes that gap fully:mergeSchemaExtrasnow recurses into a nested-aggregate member's own schema to whatever depth the type graph actually has, not just one level, so a naturally nested domain (a measurement with a repeated specimen sub-record, a document with a nested address, a category tree) gets consistent annotations at every level.Unbounded depth needs a way to terminate on a genuinely cyclic type graph: a self-referential type (
struct Node { std::vector<Node> children; };) or a mutual reference between two distinct types now fails to compile — astatic_assertdependent on the specific cyclic instantiation — instead of recursing forever. This is the one source-compatibility exception to an otherwise purely additive change; no such type exists in this repo today.Changes
annotateNestedAggregate/annotateNestedAggregateRefthread anAncestors...template parameter chain (the nested-aggregate types already being annotated on the current path) through a new sharedrecurseIntoNestedAggregateIfAny, replacing the old one-level cap with cycle-guarded recursion to any depth.mergeSchemaExtrasthat duplicatedannotateBasicMemberProperty's per-member annotation logic instead of calling it — both the top-level pass and the nested-recursion pass now share one implementation.$defslookup that the new recursion made reachable (glz::generic_u64's object storage reallocates on insert, which could dangle a reference an enclosing recursive frame still holds).docs/spec/forms/forms.md: replaces "Nested aggregates (one level)" with "Nested aggregates (recursive, cycle-guarded)", and reconciles the pre-existing normative renderer-contract section, which had assumed nested$defsentries are never patched.Closes #25