Phase-3 groundwork: IAssertionContainer, a structural schema traversal contract - #685
Draft
ewoutkramer wants to merge 2 commits into
Draft
Phase-3 groundwork: IAssertionContainer, a structural schema traversal contract#685ewoutkramer wants to merge 2 commits into
ewoutkramer wants to merge 2 commits into
Conversation
Adds an internal contract for assertions that hold nested assertions, so a schema tree can be walked and rewritten without a hardcoded type switch: * IAssertionContainer.WithChildren(rewrite) - one member, so a container cannot have a child set that disagrees with what it rewrites; returns the container itself when every child came back unchanged, so unchanged subtrees keep their identity. * AssertionStep - the label on the edge to a child (member / child element / slice / subschema / reference target), rendered like the PathStack events so rewriter-side paths read like the definition paths on issues. * Implemented on ElementSchema (through WithMembers, so schema subclasses come along), ChildrenValidator, SliceValidator, AllValidator, AnyValidator, DefinitionsAssertion, ReferencedInstanceValidator, KeyedObjectValidator and PathSelectorValidator. Slice discriminators and AnyValidator's summary error are not children: they are not assertions the container validates against. * AssertionContainerTests guards the set: any assertion with [DataMember] state holding assertions must implement the interface, so a future container breaks the build instead of being silently skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an internal schema traversal and rewriting contract for future enterprise rewriters.
Changes:
- Introduces assertion-container traversal and edge metadata.
- Implements immutable child rewriting across assertion containers.
- Adds reflection guards and behavioral tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
test/Firely.Fhir.Validation.Tests/Schema/AssertionContainerTests.cs |
Tests traversal and copy behavior. |
src/Firely.Fhir.Validation/Schema/IAssertionContainer.cs |
Defines traversal contract and steps. |
src/Firely.Fhir.Validation/Impl/SliceValidator.cs |
Rewrites slice assertions. |
src/Firely.Fhir.Validation/Impl/ReferencedInstanceValidator.cs |
Rewrites reference targets. |
src/Firely.Fhir.Validation/Impl/PathSelectorValidator.cs |
Rewrites selected assertions. |
src/Firely.Fhir.Validation/Impl/KeyedObjectValidator.cs |
Rewrites entry assertions. |
src/Firely.Fhir.Validation/Impl/ElementSchema.cs |
Rewrites schema members. |
src/Firely.Fhir.Validation/Impl/DefinitionsAssertion.cs |
Rewrites anchored subschemas. |
src/Firely.Fhir.Validation/Impl/ChildrenValidator.cs |
Rewrites named children. |
src/Firely.Fhir.Validation/Impl/AnyValidator.cs |
Rewrites alternative members. |
src/Firely.Fhir.Validation/Impl/AllValidator.cs |
Rewrites required members. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+83
to
+84
| var members = Members.TryRewriteMembers(AssertionStep.Member, rewrite); | ||
| return members is null ? this : WithMembers(members); |
Comment on lines
+80
to
+82
| if (rewritten is not ElementSchema rewrittenSchema) | ||
| throw new InvalidOperationException( | ||
| $"A rewrite of subschema '{schema.Id}' must return an {nameof(ElementSchema)}, but it returned a {rewritten.GetType().Name}."); |
| if (!ReferenceEquals(rewritten, child)) | ||
| { | ||
| // Only start copying once we actually have a change to record. | ||
| updated ??= new Dictionary<string, IAssertion>(_childList); |
ewoutkramer
marked this pull request as draft
August 26, 2026 09:31
ewoutkramer
commented
Aug 26, 2026
| /// <summary> | ||
| /// A step to a member that does not change the position within the instance. | ||
| /// </summary> | ||
| public static AssertionStep Member { get; } = new(AssertionStepKind.Member); |
Member
Author
There was a problem hiding this comment.
I think static properties that have a side effect (creating a new instance) are an anti-pattern. Turn this into a static function, like you did with the other options.
| /// The type of the reference target, when this is a <see cref="AssertionStepKind.ReferenceTarget"/> | ||
| /// step. <c>null</c> means targets of any type. | ||
| /// </summary> | ||
| public string? TargetType => Kind == AssertionStepKind.ReferenceTarget ? Name : null; |
Member
Author
There was a problem hiding this comment.
OMG. If only we had discriminated unions. Will we rework in C# 15?
Member
Author
There was a problem hiding this comment.
Discriminated unions in C# have been "basically shipping next release" since roughly the Obama administration, so I wouldn't hold AssertionStep hostage waiting for it. Until then we've built the artisanal, hand-rolled version: one enum to say what we are, and an overloaded string? Name doing five different jobs depending who's asking — basically a Ouija board that only understands Kind. If C# 15 ever ships real sum types I'll happily let the compiler do the lying instead of the doc comments.
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
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.
Phase-3 groundwork (OSS side). Replaces the schema rewriter's hardcoded switch over container types with an internal structural contract, so the enterprise rewriter in phase 3 can walk and rewrite a compiled schema without knowing the concrete assertion types.
Note this is a plan amendment: the approved plan had phase 3 as enterprise-only. Doing this bit in OSS is what lets the enterprise rewriter stop hardcoding OSS internals.
What's in it
Schema/IAssertionContainer.cs, all internal:IAssertionContainerwith a single member,IAssertion WithChildren(Func<AssertionStep, IAssertion, IAssertion> rewrite). A delegate rather than a member list, so keyed children (children, slices, subschemas, target cases) keep their structure instead of being flattened into a positional sequence. Implementations returnthiswhen every rewrite returned what it was given, so unchanged subtrees keep identity.AssertionStep— areadonly record struct (AssertionStepKind Kind, string? Name)labelling the edge from a container to a nested assertion, with kindsMember/Child/Slice/Subschema/ReferenceTarget. The kinds enumerate what the slot does to the walk context (does the instance position move? does the definition path gain a qualifier?), not what kind of container it is — which is why the set is closed: a consumer has to know how to react to every kind.ToString()renders steps the wayPathStackrenders its navigation events, so paths built while rewriting read like the definition paths reported on issues.ElementSchema(viaWithMembers, so its subclasses come along for free),ChildrenValidator,SliceValidator,AllValidator,AnyValidator,DefinitionsAssertion,ReferencedInstanceValidator,KeyedObjectValidator,PathSelectorValidator.AnyValidator.SummaryError— structural machinery rather than validation members, and rewriting them would be meaningless.AssertionContainerTests, 17 tests): reflection over[DataMember]assertion-typed state, so any future assertion that holds nested assertions and forgets to implement the interface fails the build. Keyed onDataMemberspecifically soIssueAssertion.Assertion(an internal back-pointer, not a child) isn't flagged.Design notes
Slicecovers the pseudo-slicing a choice element compiles into (fhir-type-labeldiscriminators). That's deliberate and it's the same criterion phase 2 applied when it removed the reference-target pseudo-slicing: a slice is real when the discriminator interrogates the node at the slot's own position. Choice types pass that test (and FHIR itself defines type slicing with atypediscriminator); reference targets failed it, which is why they got their own kind. TheSlicedoc comment records the one place the analogy leaks — slice names come from two vocabularies, authored slice names and FHIR type codes, so a consumer must not assume the name matches the:sliceNameinElementDefinition.id.Known follow-up for phase 3
ReferencedInstanceValidator's single-schema (untyped) constructor can't setChecks— only the target-cases constructor takes it. Reference rules on untyped RIVs will need that; this draft works around it with a private copy constructor used only for rewriting.Verification
Core project builds clean (0 warnings);
AssertionContainerTests17/17 green. No consumer yet — the rewriter that uses this lands in phase 3, so this PR adds contract + guard only and changes no validation behaviour.🤖 Generated with Claude Code