Add IConstraintSetBuilder for programmatic constraint set construction#532
Conversation
📝 WalkthroughWalkthroughAdds test-support fluent builders and interfaces to programmatically construct metaschema constraint sets and contexts, integrates the builder via IModuleMockFactory, replaces XML-based loading in a post-processor test with programmatic construction, and adds unit tests for the new builder API. Test method signature no longer declares checked exceptions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java (1)
🔇 Additional comments (6)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/tests/ConstraintSetBuilderTest.java (1)
51-78: Consider adding assertions to verify constraints were added.The test only asserts
assertNotNull(constraintSet)but doesn't verify that the allowed-values constraint was actually added to the context. Adding assertions to check the constraint set's contexts or the specific constraint would strengthen this test.// Then assertNotNull(constraintSet, "Constraint set should not be null"); + // Optionally verify the context and constraint were added + // e.g., assertFalse(constraintSet.getTargetedConstraintsForScopedContext(...).isEmpty()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/test/java/gov/nist/secauto/metaschema/core/model/constraint/ExternalConstraintsModulePostProcessorTest.java(2 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ConstraintSetBuilder.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/IConstraintSetBuilder.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/IContextBuilder.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/IModuleMockFactory.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/tests/ConstraintSetBuilderTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-08T16:52:33.199Z
Learnt from: CR
Repo: metaschema-framework/metaschema PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-08T16:52:33.199Z
Learning: When adding new constraint types, update the constraint type lists for `define-flag`, `define-field`, and `define-assembly` in `schema/metaschema/metaschema-module-constraints.xml` and add a new section with syntax table and examples in `website/content/specification/syntax/constraints.md`.
Applied to files:
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/tests/ConstraintSetBuilderTest.javacore/src/test/java/gov/nist/secauto/metaschema/core/model/constraint/ExternalConstraintsModulePostProcessorTest.java
🧬 Code graph analysis (3)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/IContextBuilder.java (1)
core/src/main/java/gov/nist/secauto/metaschema/core/model/constraint/AbstractConstraintBuilder.java (1)
AbstractConstraintBuilder(36-310)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ConstraintSetBuilder.java (1)
core/src/main/java/gov/nist/secauto/metaschema/core/util/ObjectUtils.java (1)
ObjectUtils(18-135)
core/src/test/java/gov/nist/secauto/metaschema/core/model/constraint/ExternalConstraintsModulePostProcessorTest.java (3)
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupLine.java (1)
MarkupLine(32-114)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/MockedModelTestSupport.java (1)
MockedModelTestSupport(15-25)core/src/main/java/gov/nist/secauto/metaschema/core/util/ObjectUtils.java (1)
ObjectUtils(18-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Code
- GitHub Check: Website
🔇 Additional comments (12)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/IModuleMockFactory.java (1)
82-91: LGTM!The new
constraintSet()method follows the established pattern of other builder factory methods in this interface (flag(),field(),assembly(),module()). Clean delegation to the static builder factory.core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/IConstraintSetBuilder.java (1)
21-69: Well-designed fluent builder interface.The API design is clean and follows the established patterns in the codebase. The use of
Consumer<IContextBuilder>for context configuration enables flexible and readable constraint setup without exposing implementation details.core/src/test/java/gov/nist/secauto/metaschema/core/model/constraint/ExternalConstraintsModulePostProcessorTest.java (2)
31-53: Good migration from XML to programmatic construction.The inline comments documenting the original XML structure are helpful for understanding the intent. The test correctly builds a constraint set with an allowed-values constraint targeting
@valueand applies it to all nodes via the//*metapath.
55-80: Test verifies the integration correctly.The test validates that
ExternalConstraintsModulePostProcessorsuccessfully applies external constraints to a module's assembly definition. The assertion checkingdefinition.getConstraints().size()confirms the constraint was propagated.core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/tests/ConstraintSetBuilderTest.java (3)
34-49: LGTM - Good baseline test.The test validates basic builder functionality including source assignment and empty imports. The assertions appropriately verify the expected state.
107-132: Nested context test covers an important scenario.Testing hierarchical contexts (
//parentwith childchild) validates the builder's ability to handle complex constraint structures. The pattern is useful for real-world constraint definitions.
134-156: Good test for imports functionality.This test properly validates the import mechanism by creating an imported set, then a main set that references it, and verifying the relationship. The assertions are thorough.
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ConstraintSetBuilder.java (1)
61-73: LGTM - Build logic is straightforward.The build method properly validates that source is set (with a descriptive message), iterates through context builders to build them, and constructs the final
MetaConstraintSet. Thebuild(null)call for root-level contexts is appropriate.core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/IContextBuilder.java (1)
21-59: Well-designed fluent builder interface.The interface provides a clean API with proper nullability annotations and generics that align with the
AbstractConstraintBuilderpattern. TheConsumer<IContextBuilder>approach for child contexts enables flexible nested configuration.core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java (3)
31-73: Clean fluent builder implementation.The builder correctly implements the interface with proper fluent method chaining. The use of
IMetapathExpression.lazyCompilewith the source's static context and immediatebuild()on constraint builders integrates well with the existing constraint infrastructure.
75-107: Build method correctly constructs context hierarchy.The method properly creates the constraint context with parent-child relationships and recursively builds nested contexts. The
ObjectUtils.notNull(metapaths)wrapper on line 96 handles any nullability annotation requirements from theMetaConstraintSet.Contextconstructor.
109-122: Verify constraint type coverage against current framework constraint definitions.The
addConstraintmethod currently handles only 4 constraint types. Before extending support for additional types likeICardinalityConstraint,IIndexConstraint, orIUniqueConstraint, verify whether these types exist in the framework and are required for test scenarios by checking the constraint type registry inmetaschema-module-constraints.xml.
...src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ConstraintSetBuilder.java
Show resolved
Hide resolved
Introduces a fluent builder API for creating IConstraintSet instances programmatically in tests, eliminating the need for XMLBeans dependency when testing constraint-related functionality. New classes: - IConstraintSetBuilder: Main builder interface for constraint sets - IContextBuilder: Builder for constraint contexts with metapath targeting - ConstraintSetBuilder: Implementation that builds MetaConstraintSet - ContextBuilder: Implementation that builds MetaConstraintSet.Context The builder leverages existing constraint builders (IAllowedValuesConstraint, IMatchesConstraint, etc.) via a generic constraint() method that accepts any AbstractConstraintBuilder. Also migrates ExternalConstraintsModulePostProcessorTest to use the new programmatic builder instead of XmlMetaConstraintLoader.
- Add descriptive error message when source() not called before context() - Add Javadoc to addConstraint() private method
david-waltermire
force-pushed
the
feature/constraint-set-builder
branch
from
746fbaa to
346e3b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java (1)
1-135: Context building and constraint dispatch are sound; consider documenting supported constraint typesThe implementation correctly compiles metapaths against the source’s static context, constructs an
AssemblyConstraintSetper context, and recursively wires parent/childMetaConstraintSet.Contextinstances. TheaddConstrainthelper currently supports a specific subset of constraint interfaces (IAllowedValuesConstraint,IMatchesConstraint,IExpectConstraint,IIndexHasKeyConstraint) and throwsUnsupportedOperationExceptionfor anything else. That’s fine for the current test use, but it may surprise future callers who try to plug in additional constraint types.Consider either:
- Explicitly documenting this subset in the class-level Javadoc (and/or
constraint(...)Javadoc), and/or- Adding a short comment/TODO in
addConstraintreminding maintainers to extend the dispatch when new constraint types are introduced elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/test/java/gov/nist/secauto/metaschema/core/model/constraint/ExternalConstraintsModulePostProcessorTest.java(2 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ConstraintSetBuilder.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/IConstraintSetBuilder.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/IContextBuilder.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/IModuleMockFactory.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/tests/ConstraintSetBuilderTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/tests/ConstraintSetBuilderTest.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: metaschema-framework/metaschema PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-08T16:52:33.199Z
Learning: When adding new constraint types, update the constraint type lists for `define-flag`, `define-field`, and `define-assembly` in `schema/metaschema/metaschema-module-constraints.xml` and add a new section with syntax table and examples in `website/content/specification/syntax/constraints.md`.
📚 Learning: 2025-12-08T16:52:33.199Z
Learnt from: CR
Repo: metaschema-framework/metaschema PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-08T16:52:33.199Z
Learning: When adding new constraint types, update the constraint type lists for `define-flag`, `define-field`, and `define-assembly` in `schema/metaschema/metaschema-module-constraints.xml` and add a new section with syntax table and examples in `website/content/specification/syntax/constraints.md`.
Applied to files:
core/src/test/java/gov/nist/secauto/metaschema/core/model/constraint/ExternalConstraintsModulePostProcessorTest.java
🧬 Code graph analysis (4)
core/src/test/java/gov/nist/secauto/metaschema/core/model/constraint/ExternalConstraintsModulePostProcessorTest.java (3)
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupLine.java (1)
MarkupLine(32-114)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/MockedModelTestSupport.java (1)
MockedModelTestSupport(15-25)core/src/main/java/gov/nist/secauto/metaschema/core/util/ObjectUtils.java (1)
ObjectUtils(18-135)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/IContextBuilder.java (1)
core/src/main/java/gov/nist/secauto/metaschema/core/model/constraint/AbstractConstraintBuilder.java (1)
AbstractConstraintBuilder(36-310)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ConstraintSetBuilder.java (1)
core/src/main/java/gov/nist/secauto/metaschema/core/util/ObjectUtils.java (1)
ObjectUtils(18-135)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java (2)
core/src/main/java/gov/nist/secauto/metaschema/core/model/constraint/AbstractConstraintBuilder.java (1)
AbstractConstraintBuilder(36-310)core/src/main/java/gov/nist/secauto/metaschema/core/util/ObjectUtils.java (1)
ObjectUtils(18-135)
🔇 Additional comments (5)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/IConstraintSetBuilder.java (1)
1-70: Fluent constraint-set builder interface is clear and well-scopedThe interface cleanly exposes the minimal surface needed for test-time constraint-set construction (source, imports, contexts, build), and the static
builder()entry point keeps usage consistent with other test-support builders. No issues from an API or nullability standpoint.core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/IModuleMockFactory.java (1)
83-91:constraintSet()helper is consistent with existing factory methodsThe new default
constraintSet()method matches the established pattern for other builders on this factory and cleanly exposes the new test-support API. No further changes needed.core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ConstraintSetBuilder.java (1)
1-75: Constraint set builder implementation is correct; treat as single-useThe implementation correctly enforces that
sourceis set before adding contexts or building, and delegates context construction cleanly toContextBuilder. Given thatimportsandcontextsare accumulated and never cleared, this builder should be treated as single-use (per constraint set) in tests—which matches how it’s used now. No mandatory changes from a correctness standpoint.core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/IContextBuilder.java (1)
1-59: Context builder interface cleanly captures the intended usageThe API for setting metapaths, adding constraints via generic
AbstractConstraintBuilderinstances, and configuring child contexts is coherent and type-safe for the intended test-support use. It aligns well with the concreteContextBuilderimplementation.core/src/test/java/gov/nist/secauto/metaschema/core/model/constraint/ExternalConstraintsModulePostProcessorTest.java (1)
10-81: Programmatic constraint construction preserves original test semanticsThe refactored test builds the external
IConstraintSetand module entirely in code, closely mirroring the original XML context (//*) and allowed-values constraint on@value. UsingMockedModelTestSupport.constraintSet()plus the module builder keeps everything in one place and removes IO/XML concerns while still validating thatExternalConstraintsModulePostProcessorapplies a single constraint to assemblya. This looks correct and easier to maintain.
…traint types - Add class-level Javadoc to ContextBuilder listing supported constraint types (IAllowedValuesConstraint, IMatchesConstraint, IExpectConstraint, IIndexHasKeyConstraint) and noting which types will throw UnsupportedOperationException - Add source assertions to test methods and clarify that constraint application is tested in ExternalConstraintsModulePostProcessorTest
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java (3)
70-76: Consider defaulting constraintsourcefrom the context to avoid fragile usage
AbstractConstraintBuilder.validate()requires a non-nullsource, so callingconstraintBuilder.build()here will fail if the caller forgot to set.source(...)on the builder. That’s easy to miss in tests and leads to a runtime failure somewhat far from the call site.Since this builder already captures a context-level
source, consider setting it on the supplied builder before building, so the common case “just works”:public <B extends AbstractConstraintBuilder<B, C>, C extends IConstraint> IContextBuilder constraint( @NonNull AbstractConstraintBuilder<B, C> constraintBuilder) { - this.constraints.add(constraintBuilder.build()); + // Default the source for convenience in tests; callers that care about a + // different source can set it explicitly before passing the builder. + constraintBuilder.source(source); + this.constraints.add(constraintBuilder.build()); return this; }This does overwrite any previously configured
sourceon the builder, so if you rely on more granular sources, you may instead want to assert/document that a source must be set before callingconstraint(...).
95-109: Avoid potential aliasing ofmetapathsbetween builder and built context
MetaConstraintSet.Contextreceives themetapathslist directly (wrapped only byObjectUtils.notNull), so if the builder is mutated afterbuild(...)(e.g., reused for another configuration), those mutations could leak into an already-built context unlessMetaConstraintSet.Contextmakes its own defensive copy.If contexts are meant to be immutable post-build, consider copying here:
- MetaConstraintSet.Context context = new MetaConstraintSet.Context( - parent, - source, - ObjectUtils.notNull(metapaths), - modelConstrained); + MetaConstraintSet.Context context = new MetaConstraintSet.Context( + parent, + source, + new ArrayList<>(metapaths), + modelConstrained);This keeps the builder reusable without depending on the internals of
MetaConstraintSet.Context.
134-147:addConstraintdispatch is correct; remember to extend when new constraint types are addedThe
instanceofdispatch forIAllowedValuesConstraint,IMatchesConstraint,IExpectConstraint, andIIndexHasKeyConstraintmatches the documented “supported types” list, and throwingUnsupportedOperationExceptionfor others is a good fail-fast behavior in tests.As new constraint types are introduced (e.g., additional model constraints), this method will need to be extended to keep the test-support builder in sync with the core model. When you add new constraint types to the metaschema, that’s also the point where you typically update the constraint type lists in
metaschema-module-constraints.xmland the spec docs; this helper should be updated alongside those changes. Based on learnings, ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/tests/ConstraintSetBuilderTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/tests/ConstraintSetBuilderTest.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: metaschema-framework/metaschema PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-08T16:52:33.199Z
Learning: When adding new constraint types, update the constraint type lists for `define-flag`, `define-field`, and `define-assembly` in `schema/metaschema/metaschema-module-constraints.xml` and add a new section with syntax table and examples in `website/content/specification/syntax/constraints.md`.
🧬 Code graph analysis (1)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java (2)
core/src/main/java/gov/nist/secauto/metaschema/core/model/constraint/AbstractConstraintBuilder.java (1)
AbstractConstraintBuilder(36-310)core/src/main/java/gov/nist/secauto/metaschema/core/util/ObjectUtils.java (1)
ObjectUtils(18-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Website
- GitHub Check: Code
🔇 Additional comments (1)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java (1)
43-52: Overall builder structure looks solid for test supportThe field layout (single
ISource, plus lists for metapaths, constraints, and child contexts) provides a clear, minimal surface for constructing nested contexts, and the implementation stays cleanly in the test-support namespace without leaking into production code. No issues here.
Extend ContextBuilder to support the full set of constraint types: - IAllowedValuesConstraint (existing) - IMatchesConstraint (existing) - IExpectConstraint (existing) - IIndexHasKeyConstraint (existing) - ICardinalityConstraint (new) - IIndexConstraint (new) - IUniqueConstraint (new) Add comprehensive unit tests for each constraint type to verify the builder correctly constructs constraint sets with all supported constraint types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java (3)
73-79: Simplifyconstraint(...)generics for readabilityThe current signature
public <B extends AbstractConstraintBuilder<B, C>, C extends IConstraint> IContextBuilder constraint( @NonNull AbstractConstraintBuilder<B, C> constraintBuilder)is more complex than needed since you only call
build()and store the result asIConstraint. You could simplify to something like:- public <B extends AbstractConstraintBuilder<B, C>, C extends IConstraint> IContextBuilder constraint( - @NonNull AbstractConstraintBuilder<B, C> constraintBuilder) { + public IContextBuilder constraint( + @NonNull AbstractConstraintBuilder<?, ? extends IConstraint> constraintBuilder) { - this.constraints.add(constraintBuilder.build()); + this.constraints.add(constraintBuilder.build()); return this; }This keeps the API just as type-safe for callers while making it easier to read and maintain.
90-122: Consider defensively copyingmetapathswhen building the context
build(...)passes the mutablemetapathslist directly intoMetaConstraintSet.Context:MetaConstraintSet.Context context = new MetaConstraintSet.Context( parent, source, ObjectUtils.notNull(metapaths), modelConstrained);If the same
ContextBuilderwere reused or modified afterbuild, those changes would be visible through the already-built context. For a builder, it’s usually safer to decouple internal mutable state from the built object:- MetaConstraintSet.Context context = new MetaConstraintSet.Context( - parent, - source, - ObjectUtils.notNull(metapaths), - modelConstrained); + MetaConstraintSet.Context context = new MetaConstraintSet.Context( + parent, + source, + List.copyOf(ObjectUtils.notNull(metapaths)), + modelConstrained);This keeps the built context immutable with respect to further builder mutations, without changing the external API.
124-157: KeepaddConstraintand the documented supported constraint types in sync
addConstraintexplicitly dispatches on each known constraint interface and throwsUnsupportedOperationExceptionfor anything else. This matches the Javadoc list of supported types, but it also means:
- Any new
IConstraintsubtype added to the model will cause this builder to fail at runtime untiladdConstraintis updated.- The Javadoc list (lines 34–44) and this
if/elsechain must be updated together to avoid divergence.Given the project’s pattern of updating central constraint-type registries and docs when adding new constraint kinds, it would be good to treat this method as another place that must be kept in lockstep with the constraint model and documentation. You might consider adding a brief comment here to that effect to make future updates harder to miss. Based on learnings, this aligns with keeping constraint type lists and syntax docs updated when new constraints are introduced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/tests/ConstraintSetBuilderTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/tests/ConstraintSetBuilderTest.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: metaschema-framework/metaschema PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-08T16:52:33.199Z
Learning: When adding new constraint types, update the constraint type lists for `define-flag`, `define-field`, and `define-assembly` in `schema/metaschema/metaschema-module-constraints.xml` and add a new section with syntax table and examples in `website/content/specification/syntax/constraints.md`.
🧬 Code graph analysis (1)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java (2)
core/src/main/java/gov/nist/secauto/metaschema/core/model/constraint/AbstractConstraintBuilder.java (1)
AbstractConstraintBuilder(36-310)core/src/main/java/gov/nist/secauto/metaschema/core/util/ObjectUtils.java (1)
ObjectUtils(18-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Code
- GitHub Check: Website
🔇 Additional comments (1)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java (1)
31-55: ContextBuilder documentation and structure are clear and alignedThe class Javadoc clearly enumerates the supported constraint types, and the field layout (source, metapaths, constraints, childContexts) matches the intended builder responsibilities for test contexts. This is a solid, focused test utility.
…tainability note - Use List.copyOf() for defensive copy of metapaths when building context to ensure built context is immutable with respect to builder mutations - Add note to addConstraint() Javadoc reminding maintainers to update this method when new constraint types are added to the model - Remove unused ObjectUtils import
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java (3)
72-78: Consider simplifying generics onconstraint(...)for readabilityThe method’s type parameters
<B extends AbstractConstraintBuilder<B, C>, C extends IConstraint>don’t provide benefits beyond what the parameter type already ensures, and they slightly increase visual noise for a test helper.You could simplify the signature to reduce generics clutter while keeping type safety:
- public <B extends AbstractConstraintBuilder<B, C>, C extends IConstraint> IContextBuilder constraint( - @NonNull AbstractConstraintBuilder<B, C> constraintBuilder) { + public IContextBuilder constraint( + @NonNull AbstractConstraintBuilder<?, ? extends IConstraint> constraintBuilder) {This should still accept all existing constraint builders without impacting call sites.
80-87: Child context construction always reuses the sameISource
childContextcurrently always creates children with the samesourceas the parent, andbuild(...)wires them into a tree via theparentparameter. That’s perfectly fine for the current test scenarios where a single logical source is used.If you later need tests that combine constraints from multiple sources in a single tree, you might consider an overload like:
public IContextBuilder childContext(@NonNull ISource childSource, @NonNull Consumer<IContextBuilder> childConfigurer)so callers can explicitly vary the source per child when needed, while keeping the existing API as a convenience.
Also applies to: 96-121
123-160: Dispatcher inaddConstraintis clear but may become a maintenance hotspotThe
addConstraintmethod’s explicitinstanceofchain is clear and matches the class Javadoc listing supported constraint types. As more constraint types are added, this is another place that must be updated in lockstep with the core constraint model and associated documentation (on top of the schema and spec updates already required). Based on learnings, this aligns with the broader need to keep constraint type lists in sync across code and docs.If the core model ever gains either:
- a generic
addConstraint(IConstraint)onIModelConstrained, or- a visitor-style API on
IConstraint,you could centralize this dispatch there and have the builder delegate, avoiding another location that must be touched whenever a new constraint type is introduced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: metaschema-framework/metaschema PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-08T16:52:33.199Z
Learning: When adding new constraint types, update the constraint type lists for `define-flag`, `define-field`, and `define-assembly` in `schema/metaschema/metaschema-module-constraints.xml` and add a new section with syntax table and examples in `website/content/specification/syntax/constraints.md`.
🧬 Code graph analysis (1)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java (1)
core/src/main/java/gov/nist/secauto/metaschema/core/model/constraint/AbstractConstraintBuilder.java (1)
AbstractConstraintBuilder(36-310)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Website
- GitHub Check: Code
🔇 Additional comments (1)
core/src/test/java/gov/nist/secauto/metaschema/core/testsupport/builder/ContextBuilder.java (1)
45-70: ContextBuilder core state and metapath handling look goodConstructor, field initialization, and
metapath(String)are straightforward and appropriate for test support:sourceis final, collections are initialized eagerly, and usingIMetapathExpression.lazyCompilewithsource.getStaticContext()is a good fit for on-demand compilation in tests. No changes needed here.
Replace named type parameters with wildcards since the method only calls build() on the builder and doesn't need full type inference. Before: <B extends AbstractConstraintBuilder<B, C>, C extends IConstraint> After: AbstractConstraintBuilder<?, ? extends IConstraint>
david-waltermire
merged commit 226e13f
into
metaschema-framework:develop
1 participant
Summary
IConstraintSetinstances programmatically in testsExternalConstraintsModulePostProcessorTestto use the new programmatic builderNew Classes
IConstraintSetBuilder: Main builder interface for constraint setsIContextBuilder: Builder for constraint contexts with metapath targetingConstraintSetBuilder: Implementation that buildsMetaConstraintSetContextBuilder: Implementation that buildsMetaConstraintSet.ContextDesign
The builder leverages existing constraint builders (
IAllowedValuesConstraint.builder(),IMatchesConstraint.builder(), etc.) via a genericconstraint()method that accepts anyAbstractConstraintBuilder.Example usage:
Test plan
ConstraintSetBuilderTestverify builder functionalityExternalConstraintsModulePostProcessorTestmigrated and passingmvn install -PCI -Prelease)Summary by CodeRabbit
Tests
New Features
✏️ Tip: You can customize this high-level summary in your review settings.