feat: add list-allowed-values command and visitor#666
feat: add list-allowed-values command and visitor#666david-waltermire wants to merge 3 commits intometaschema-framework:developfrom
Conversation
Migrate AllowedValueCollectingNodeItemVisitor from liboscal-java to the core module, making it available to all Metaschema users. The visitor traverses a module's node items to collect allowed-values constraints organized by target node. Add a new list-allowed-values CLI command that generates a YAML listing of all allowed-values constraints for a given Metaschema module. This command was previously only available in oscal-cli with OSCAL-specific module loading hardcoded; the generic version accepts any Metaschema module via a positional argument.
📝 WalkthroughWalkthroughAdds a visitor to collect allowed-values constraints across Metaschema modules, a CLI command that emits those constraints as YAML, tests for both visitor and CLI, and module descriptor updates for YAML support and command registration. Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Cmd as ListAllowedValuesCommand
participant Loader as Module Loader
participant Visitor as AllowedValueCollectingNodeItemVisitor
participant Module as IModule
participant YAML as YAML Generator
participant Output as File/Stdout
User->>Cmd: list-allowed-values module.xml [output.yaml]
Cmd->>Loader: resolve & load module
Loader->>Module: parse module
Cmd->>Visitor: create visitor with DynamicContext
Cmd->>Visitor: visit(module)
Visitor->>Module: traverse node items (flags/fields/assemblies)
Visitor->>Visitor: apply Let-bindings, evaluate targets
Visitor->>Visitor: collect AllowedValuesRecords into NodeItemRecords
Visitor-->>Cmd: return collected records
Cmd->>YAML: generate structured YAML grouped by target
YAML->>Output: write to file or stdout
Output-->>User: display/save results
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PMD (7.21.0)core/src/main/java/dev/metaschema/core/metapath/item/node/AllowedValueCollectingNodeItemVisitor.java[ERROR] Cannot load ruleset pmd/category/java/custom.xml: Cannot resolve rule/ruleset reference 'pmd/category/java/custom.xml'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath. Comment |
Verify the YAML output contains expected constraint locations, types, target paths, allowed values, and allow-other settings by writing to a file and checking the content.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/main/java/dev/metaschema/core/metapath/item/node/AllowedValueCollectingNodeItemVisitor.java (1)
188-196: Consider returning an unmodifiable list fromgetAllowedValues.
This prevents external mutation of the internal collection while preserving theaddAllowedValuesAPI.♻️ Suggested change
import java.util.Collection; +import java.util.Collections; import java.util.LinkedHashMap; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ `@NonNull` public List<AllowedValuesRecord> getAllowedValues() { - return allowedValues; + return Collections.unmodifiableList(allowedValues); }
Prevents external mutation of the internal allowed-values collection while preserving the addAllowedValues API for internal use.
|
@coderabbitai Addressed in commit ab75ed8. Returning |
|
Tip For best results, initiate chat on the files or code changes.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/src/main/java/dev/metaschema/core/metapath/item/node/AllowedValueCollectingNodeItemVisitor.java (2)
199-208: Consider reducing visibility ofaddAllowedValues.The PR commit message indicates the intent to "prevent external mutation of the internal allowed-values collection." While
getAllowedValues()correctly returns an unmodifiable list, the publicaddAllowedValues()method still allows external callers to add records directly. SinceNodeItemRecordinstances are only created internally (private constructor), consider makingaddAllowedValuespackage-private to fully enforce the immutability contract from the caller's perspective.Suggested change
- /** - * Add an allowed-values constraint record to this node item. - * - * `@param` record - * the allowed-values record to add - */ - public void addAllowedValues(`@NonNull` AllowedValuesRecord record) { + // Package-private: only used by the enclosing visitor + void addAllowedValues(`@NonNull` AllowedValuesRecord record) { this.allowedValues.add(record); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/dev/metaschema/core/metapath/item/node/AllowedValueCollectingNodeItemVisitor.java` around lines 199 - 208, The public method addAllowedValues(`@NonNull` AllowedValuesRecord record) on AllowedValueCollectingNodeItemVisitor allows external mutation despite getAllowedValues() returning an unmodifiable list; change the visibility of addAllowedValues from public to package-private (remove the public modifier) so only classes in the same package (and internal creation of NodeItemRecord) can add records, leaving getAllowedValues() intact to return the unmodifiable view.
52-55: Consider returning an unmodifiable view for consistency.The method returns
nodeItemAnalysis.values()directly, which is a live view backed by the map. While typical usage completes visitation before calling this method, wrapping inCollections.unmodifiableCollection()would align with the PR's intent (per commit message) of returning unmodifiable collections from public APIs.Suggested change
`@NonNull` public Collection<NodeItemRecord> getAllowedValueLocations() { - return nodeItemAnalysis.values(); + return Collections.unmodifiableCollection(nodeItemAnalysis.values()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/dev/metaschema/core/metapath/item/node/AllowedValueCollectingNodeItemVisitor.java` around lines 52 - 55, The method getAllowedValueLocations in AllowedValueCollectingNodeItemVisitor currently returns the live collection nodeItemAnalysis.values(); change it to return an unmodifiable view (wrap the collection with Collections.unmodifiableCollection(...)) so callers cannot mutate the returned collection while preserving the backing data; update imports if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@core/src/main/java/dev/metaschema/core/metapath/item/node/AllowedValueCollectingNodeItemVisitor.java`:
- Around line 199-208: The public method addAllowedValues(`@NonNull`
AllowedValuesRecord record) on AllowedValueCollectingNodeItemVisitor allows
external mutation despite getAllowedValues() returning an unmodifiable list;
change the visibility of addAllowedValues from public to package-private (remove
the public modifier) so only classes in the same package (and internal creation
of NodeItemRecord) can add records, leaving getAllowedValues() intact to return
the unmodifiable view.
- Around line 52-55: The method getAllowedValueLocations in
AllowedValueCollectingNodeItemVisitor currently returns the live collection
nodeItemAnalysis.values(); change it to return an unmodifiable view (wrap the
collection with Collections.unmodifiableCollection(...)) so callers cannot
mutate the returned collection while preserving the backing data; update imports
if necessary.
1 participant
Summary
AllowedValueCollectingNodeItemVisitorfrom liboscal-java tocoremodule atdev.metaschema.core.metapath.item.node, making it available to all Metaschema userslist-allowed-valuesCLI command that generates a YAML listing of allowed-values constraints for any Metaschema moduleMetaschemaCommandsand the module-info service providerMotivation
The visitor and command were previously in liboscal-java and oscal-cli respectively, but contain zero OSCAL-specific logic. All imports are
dev.metaschema.core.*types. Moving them to metaschema-java makes this functionality available to all Metaschema-based projects.Changes
core/.../AllowedValueCollectingNodeItemVisitor.javacore/.../AllowedValueCollectingNodeItemVisitorTest.javametaschema-cli/.../ListAllowedValuesCommand.javaGenerateDiagramCommandpatternmetaschema-cli/.../MetaschemaCommands.javametaschema-cli/module-info.javarequiresfor Jackson YAML andprovidesentrymetaschema-cli/.../CLITest.javaTest plan
AllowedValueCollectingNodeItemVisitorTest- 3 tests (finds constraints, empty for no constraints, handles multiple)CLITest-list-allowed-values --helpreturns OKCLITest-list-allowed-values <module>returns OK with YAML outputmvn clean install -PCI -PreleasepassesSummary by CodeRabbit
New Features
Tests