Add XPath and JSON Pointer path formatters#550
Conversation
📝 WalkthroughWalkthroughIntroduces two new path formatters—XPath 3.1 (EQName-based) and RFC 6901 JSON Pointer—to augment path representation capabilities. Adds XPathFormatter and JsonPointerFormatter classes implementing IPathFormatter, removes JsonPathFormatter, updates CLI to support path format selection via a new PathFormatSelection enum, and integrates formatters into validation output handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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.19.0)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/PathFormatSelection.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. metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/AbstractValidateContentCommand.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. core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/IPathFormatter.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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
PRDs/20251215-path-formatters/plan.md (3)
88-92: Document performance considerations for SINGLETON_OR_LIST sibling counting.The specification for
SINGLETON_OR_LISTrequires counting siblings at runtime to determine if indexing is needed. This could be expensive if called frequently or on large structures. Document the expected performance characteristics and consider whether caching or optimization strategies should be mentioned.Consider adding a subsection under "Risks and Mitigations" or the implementation tasks to address this, such as:
- Whether traversal can be optimized (e.g., stop counting after finding 2 siblings)
- Whether results should be cached at the instance level
- Expected typical sibling counts in real Metaschema models
265-297: Expand test strategy to cover edge cases and error scenarios.The test strategy is comprehensive but lacks coverage for:
Missing XPath test cases:
- Empty namespace handling (local names without namespace)
- Very deeply nested paths (performance/stack concerns)
Missing JSON Pointer test cases:
- Position indexing: verify 0-based in JSON Pointer vs. 1-based in XPath (lines 230, 234)
- Complex property names with multiple special characters (e.g.,
a/b~c)- Error scenarios: missing key flags, malformed key values
Missing for both:
- Round-trip verification (if available)
- Performance tests for deep structures or large sibling counts
- Integration tests with actual Metaschema OSCAL modules (the test infrastructure mentions this but doesn't specify which)
Add subsections under "XPathFormatter Test Cases" and "JsonPointerFormatter Test Cases" for error/edge cases. Also specify which real Metaschema modules will be used for integration tests.
298-307: Provide more specific risk mitigation details.The risks table is good but mitigations are vague:
Line 302: Risk about
toEQName()format - mitigation should include:
- Reference to XPath 3.1 spec or EQName standard
- Specific assertion checks in unit tests (e.g., assert format matches regex
Q\{[^}]*\}[a-zA-Z])Line 304: Risk about
getJsonName()returning null - clarify:
- Under what conditions can this occur?
- Is fallback to
getEffectiveName()always safe?- Should this be guarded with a precondition check?
Line 305: Risk about JSON key flag not found - consider:
- Is the fallback to numeric index acceptable per RFC 6901 compliance?
- Should this emit a warning or error log?
Expand the risks section with specific preconditions, API assumptions, and logging strategies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)PRDs/20251215-path-formatters/plan.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
PRDs/20251215-path-formatters/plan.md
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (2)
.gitignore (1)
37-39: LGTM!Excluding Git worktrees is a best practice for managing local development artifacts and keeps the repository clean.
PRDs/20251215-path-formatters/plan.md (1)
163-168: ClarifyJsonPathFormatterremoval vs. replacement.Phase 3 task at line 164 states "Remove or replace commented
JsonPathFormatter.java". The PRD should specify:
- Is the file definitely being removed or is replacement still under consideration?
- If removing, confirm no active code depends on it
- If replacing, document the replacement strategy
- The AI summary indicates it was removed in this PR, so update task to "Remove obsolete
JsonPathFormatter.java"Update the task to be definitive based on PR status.
| private String getJsonKeyValue(IModelNodeItem<?, ?> item, INamedModelInstance instance) { | ||
| IFlagInstance keyFlag = instance.getEffectiveJsonKey(); | ||
| if (keyFlag != null) { | ||
| IFlagNodeItem flagItem = item.getFlagByName(keyFlag.getEffectiveName()); | ||
| if (flagItem != null) { | ||
| return flagItem.toAtomicItem().asString(); | ||
| } | ||
| } | ||
| return String.valueOf(item.getPosition() - 1); // fallback | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the relevant section
fd "plan.md" PRDs/20251215-path-formatters/Repository: metaschema-framework/metaschema-java
Length of output: 117
🏁 Script executed:
# Read the file to see lines 243-252 and surrounding context
wc -l PRDs/20251215-path-formatters/plan.mdRepository: metaschema-framework/metaschema-java
Length of output: 121
🏁 Script executed:
# Get lines around 243-252 with context
sed -n '220,270p' PRDs/20251215-path-formatters/plan.mdRepository: metaschema-framework/metaschema-java
Length of output: 1460
🏁 Script executed:
# Read more of the file to understand the full context and specification
sed -n '1,100p' PRDs/20251215-path-formatters/plan.mdRepository: metaschema-framework/metaschema-java
Length of output: 4233
🏁 Script executed:
# Check for test strategy and specification details
sed -n '260,314p' PRDs/20251215-path-formatters/plan.mdRepository: metaschema-framework/metaschema-java
Length of output: 2326
🏁 Script executed:
# Look for any existing implementations or related code
fd -e java | xargs grep -l "getJsonKeyValue\|JsonPointerFormatter" | head -10Repository: metaschema-framework/metaschema-java
Length of output: 62
🏁 Script executed:
# Search for any existing implementations of JsonPointerFormatter
fd -e java | xargs grep -l "JsonPointerFormatter" 2>/dev/null | head -10Repository: metaschema-framework/metaschema-java
Length of output: 62
🏁 Script executed:
# Look for warning/logging patterns in similar formatters
fd -e java | xargs grep -l "getJsonKeyValue\|JsonPointerFormatter\|XPathFormatter" 2>/dev/null | head -10Repository: metaschema-framework/metaschema-java
Length of output: 62
🏁 Script executed:
# Check if there are any existing test files for formatters
fd -e java path | xargs grep -l "PathFormatter" 2>/dev/null | head -10Repository: metaschema-framework/metaschema-java
Length of output: 525
🏁 Script executed:
# Check the IPathFormatter interface to understand the contract
cat -n core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/IPathFormatter.javaRepository: metaschema-framework/metaschema-java
Length of output: 5013
🏁 Script executed:
# Check the existing MetapathFormatter implementation for error handling patterns
cat -n core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/MetapathFormatter.javaRepository: metaschema-framework/metaschema-java
Length of output: 3239
🏁 Script executed:
# Check the deprecated JsonPathFormatter for reference
cat -n core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPathFormatter.javaRepository: metaschema-framework/metaschema-java
Length of output: 3353
🏁 Script executed:
# Look at the risk mitigation section more carefully
sed -n '275,285p' PRDs/20251215-path-formatters/plan.mdRepository: metaschema-framework/metaschema-java
Length of output: 595
🏁 Script executed:
# Check if there are any logging patterns in the codebase
rg "LOGGER|log\(" core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/ -A 2 | head -20Repository: metaschema-framework/metaschema-java
Length of output: 62
🏁 Script executed:
# Search for similar fallback patterns with warnings in the codebase
rg "fallback|warning" core/src/main/java/gov/nist/secauto/metaschema/core/ -B 2 -A 2 | head -30Repository: metaschema-framework/metaschema-java
Length of output: 3477
🏁 Script executed:
# Check the risk section more carefully to confirm the warning recommendation
sed -n '272,290p' PRDs/20251215-path-formatters/plan.mdRepository: metaschema-framework/metaschema-java
Length of output: 1054
🏁 Script executed:
# Look for patterns of how null safety is handled in similar code
rg "getEffectiveName\|getJsonName" core/src/main/java/gov/nist/secauto/metaschema/core/ -B 2 -A 2 | head -40Repository: metaschema-framework/metaschema-java
Length of output: 62
🏁 Script executed:
# Get the exact risk section from the PRD
sed -n '272,295p' PRDs/20251215-path-formatters/plan.mdRepository: metaschema-framework/metaschema-java
Length of output: 1197
🏁 Script executed:
# Check if there are any comments or TODOs about null handling in similar methods
rg "TODO|FIXME" core/src/main/java/gov/nist/secauto/metaschema/core/metapath/ -B 1 -A 1 | head -30Repository: metaschema-framework/metaschema-java
Length of output: 2925
🏁 Script executed:
# Search for the complete risk section
grep -n "Risks and Mitigations" PRDs/20251215-path-formatters/plan.mdRepository: metaschema-framework/metaschema-java
Length of output: 108
🏁 Script executed:
# Get lines around the risks section
sed -n '268,310p' PRDs/20251215-path-formatters/plan.mdRepository: metaschema-framework/metaschema-java
Length of output: 2003
Add warning/logging to the getJsonKeyValue() fallback as specified in the Risks and Mitigations section.
The code example (lines 243-252) falls back silently to numeric index when the key flag is not found, but the risk mitigation table (line 304) explicitly requires this fallback to include "with warning". The current implementation should emit a warning when falling back to numeric index instead of using the actual key value for KEYED collections, as callers rely on key-based addressing per the JSON Pointer specification.
The null safety checks are appropriate; ensure the warning is also issued if getEffectiveJsonKey() returns null.
private String getJsonKeyValue(IModelNodeItem<?, ?> item, INamedModelInstance instance) {
IFlagInstance keyFlag = instance.getEffectiveJsonKey();
if (keyFlag != null) {
IFlagNodeItem flagItem = item.getFlagByName(keyFlag.getEffectiveName());
if (flagItem != null) {
return flagItem.toAtomicItem().asString();
}
}
// Add warning here before fallback
return String.valueOf(item.getPosition() - 1); // fallback
}🤖 Prompt for AI Agents
In PRDs/20251215-path-formatters/plan.md around lines 243 to 252, the
getJsonKeyValue() method silently falls back to a numeric index when no json key
is available; add a warning log whenever the function returns the numeric
fallback (including when getEffectiveJsonKey() is null) so callers and operators
are alerted. Use the module’s standard logger to emit a clear warning that
includes the item identity (e.g., path or position) and that a numeric index is
being used as a fallback, then return the numeric index as before; keep the
existing null-safety checks and only add the warning at the fallback point.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
PRDs/20251215-path-formatters/plan.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
PRDs/20251215-path-formatters/plan.md
105-105: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
170-170: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
PRDs/20251215-path-formatters/plan.md (2)
105-117: Add language identifier to code fence.This code fence is missing a language identifier, which was flagged in a previous review. The block describes an integration flow diagram.
Apply this diff:
-``` +```text CLI Command ↓ parses --path-format option PathFormatSelection ↓ combined with Format (from document or --as option) ↓ resolved via IPathFormatter.resolveFormatter() ↓ passed to AbstractConstraintValidationHandler.setPathFormatter() ↓ used by Validation result messages -``` +```
170-175: Add language identifier to code fence.This code fence is missing a language identifier, which was flagged in a previous review. The block shows an architecture tree structure.
Apply this diff:
-``` +```tree IPathFormatter (interface) ├── MetapathFormatter (existing) ├── XPathFormatter (new) └── JsonPointerFormatter (new) -``` +```
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
PRDs/20251215-path-formatters/plan.md(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPointerFormatter.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.java: All code changes must follow the Javadoc style guide (docs/javadoc-style-guide.md). New code requires 100% Javadoc coverage on public/protected members. Modified code must add/update Javadoc on any members touched. All Javadoc must include @param, @return, @throws tags in the correct order (BLOCKING)
Java target version must be Java 11. Use SpotBugs annotations (@nonnull, @nullable) for null safety in code.
Follow package naming convention gov.nist.secauto.metaschema.* for all Java packages
Follow Test-Driven Development (TDD) principles: write tests first before implementing functionality, verify tests fail with current implementation, implement minimal code to pass tests, then refactor while keeping tests green
Files:
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPointerFormatter.java
**/metapath/**/*.java
📄 CodeRabbit inference engine (CLAUDE.md)
Metapath is an implementation of XPath 3.1. Use the XPath 3.1 specification (https://www.w3.org/TR/xpath-31/) and XPath Functions 3.1 (https://www.w3.org/TR/xpath-functions-31/) as authoritative reference when implementing new functions, fixing bugs, or understanding error handling. Raise clarification before making changes if implementation differs from spec.
Files:
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPointerFormatter.java
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: Applies to **/metapath/**/*.java : Metapath is an implementation of XPath 3.1. Use the XPath 3.1 specification (https://www.w3.org/TR/xpath-31/) and XPath Functions 3.1 (https://www.w3.org/TR/xpath-functions-31/) as authoritative reference when implementing new functions, fixing bugs, or understanding error handling. Raise clarification before making changes if implementation differs from spec.
📚 Learning: 2025-12-17T13:27:43.669Z
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: Applies to **/metapath/**/*.java : Metapath is an implementation of XPath 3.1. Use the XPath 3.1 specification (https://www.w3.org/TR/xpath-31/) and XPath Functions 3.1 (https://www.w3.org/TR/xpath-functions-31/) as authoritative reference when implementing new functions, fixing bugs, or understanding error handling. Raise clarification before making changes if implementation differs from spec.
Applied to files:
PRDs/20251215-path-formatters/plan.md
🧬 Code graph analysis (1)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPointerFormatter.java (1)
core/src/main/java/gov/nist/secauto/metaschema/core/util/ObjectUtils.java (1)
ObjectUtils(18-135)
🔇 Additional comments (5)
PRDs/20251215-path-formatters/plan.md (1)
84-85: Typo fixed.The previous typo
METAPATH_PATH_FORMATERhas been corrected toMETAPATH_PATH_FORMATTER. This resolves the naming inconsistency flagged in an earlier review.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPointerFormatter.java (4)
179-186: LGTM!The
countSiblingsimplementation correctly:
- Handles the edge case of no parent (returns 1)
- Uses
getModelItemsByName()to find all siblings sharing the same QName- Returns the total count including the item itself
This aligns with the SINGLETON_OR_LIST behavior requirement to check sibling count at runtime.
198-201: LGTM!The RFC 6901 escaping implementation is correct:
- Escapes
~to~0first (line 200)- Then escapes
/to~1- Order is critical and correctly implemented
ObjectUtils.notNull()ensures the@NonNullcontract is maintained
111-140: LGTM!The
formatModelItemimplementation correctly handles all JSON grouping behaviors:
- Proper null safety for instances without metadata (lines 112-116)
- KEYED: Extracts and escapes key values (lines 122-124)
- LIST: Uses correct 0-based indexing (line 127)
- SINGLETON_OR_LIST: Conditionally adds index only when multiple siblings exist (lines 129-135)
- NONE: Returns name without index (line 138)
All escaping is properly applied per RFC 6901, and position arithmetic correctly converts from 1-based to 0-based indexing.
152-170: Implementation correctly usesgetQName()for flag lookup; PRD example contains a bug.The implementation properly uses
keyFlag.getQName()to obtain anIEnhancedQNameparameter forgetFlagByName(). The method signature requires@NonNull IEnhancedQName name, which the code correctly provides. The PRD example in plan.md (lines 407-408) incorrectly usesgetEffectiveName(), which returns a String and is incompatible with the expected parameter type.The warning logging at lines 165-168 correctly addresses the previous review concern about alerting operators when falling back to numeric index.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPointerFormatter.java
Show resolved
Hide resolved
aj-stein
left a comment
aj-stein
left a comment
There was a problem hiding this comment.
Very useful, I look forward to using this feature.
Implement two new IPathFormatter implementations:
- XPathFormatter: Produces XPath 3.1 paths with EQName format
(e.g., /Q{http://example.com}catalog/Q{http://example.com}control[1])
Handles GROUPED/UNGROUPED XmlGroupAsBehavior for wrapper elements
- JsonPointerFormatter: Produces RFC 6901 compliant JSON Pointer paths
(e.g., /catalog/controls/0/id)
Handles all JsonGroupAsBehavior modes (NONE, LIST, KEYED, SINGLETON_OR_LIST)
Implements proper RFC 6901 escaping (~ -> ~0, / -> ~1)
Add constants to IPathFormatter:
- XPATH_PATH_FORMATTER
- JSON_POINTER_PATH_FORMATTER
Remove obsolete JsonPathFormatter (was commented out placeholder code).
Update package-info.java with documentation for new formatters.
Add design for: - PathFormatSelection enum (AUTO, METAPATH, XPATH, JSON_POINTER) - IPathFormatter.resolveFormatter() for format-based selection - CLI --path-format option for explicit override - Default behavior uses AUTO (format-based selection) Mark Phases 1-3 as completed, add Phase 4 implementation tasks and test cases.
Implement automatic path format selection based on document type: - JSON/YAML documents use JSON Pointer paths by default - XML documents use XPath paths by default Add --path-format CLI option to override the default: - auto (default): select based on document format - metapath: force Metapath format - xpath: force XPath 3.1 EQName format - jsonpointer: force RFC 6901 JSON Pointer format Changes: - Add PathFormatSelection enum in core module - Add Format.getPathFormatter() and Format.resolvePathFormatter() in databind - Add --path-format option to AbstractValidateContentCommand - Update LoggingValidationHandler to accept configurable path formatter - Add unit tests for Format path formatter resolution
- Fix PRD markdown code fence language identifiers (lines 105, 170) - Fix typo: METAPATH_PATH_FORMATER -> METAPATH_PATH_FORMATTER in PRD - Clarify PRD tasks for countSiblings() and getJsonKeyValue() helpers - Add warning logging when getJsonKeyValue() falls back to numeric index
Add .type(PathFormatSelection.class) to PATH_FORMAT_OPTION and register PathFormatSelection enum in CompletionTypeRegistry for shell completion support from PR 551.
david-waltermire
force-pushed
the
feature/xpath-jsonpointer-paths
branch
from
d0efab6 to
7cf592d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/AbstractValidateContentCommand.java (2)
103-112: The.type(PathFormatSelection.class)on the Option is ineffective since parsing is done manually.The Option is declared with
.type(PathFormatSelection.class), butcommandLine.getOptionValue()returns aStringwhich is then manually parsed inparsePathFormatSelection(). The.type()method on Apache Commons CLI Options doesn't automatically convert values to enum types. Consider removing the misleading.type()call or adding a clarifying comment.🔎 Suggested simplification
@NonNull private static final Option PATH_FORMAT_OPTION = ObjectUtils.notNull( Option.builder() .longOpt("path-format") .hasArg() .argName("FORMAT") - .type(PathFormatSelection.class) .desc("path format in validation output: auto (default, selects based on document format), " + "metapath, xpath, jsonpointer") .get());
366-390: Consider using a Map-based lookup for cleaner maintainability.The switch-based parsing works correctly with
Locale.ROOTfor case-insensitive comparison. The logging of unrecognized values with a fallback to AUTO is a good defensive pattern. However, a map-based approach could be more maintainable as formats are added.🔎 Alternative map-based approach (optional)
private static final Map<String, PathFormatSelection> PATH_FORMAT_MAP = Map.of( "auto", PathFormatSelection.AUTO, "metapath", PathFormatSelection.METAPATH, "xpath", PathFormatSelection.XPATH, "jsonpointer", PathFormatSelection.JSON_POINTER, "json-pointer", PathFormatSelection.JSON_POINTER ); @NonNull private PathFormatSelection parsePathFormatSelection(@NonNull String value) { PathFormatSelection selection = PATH_FORMAT_MAP.get(value.toLowerCase(Locale.ROOT)); if (selection == null) { LOGGER.warn("Unrecognized path format '{}', using auto", value); return PathFormatSelection.AUTO; } return selection; }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/XPathFormatter.java (2)
43-55: Consider adding brief Javadoc to these public override methods.Per coding guidelines, public members require Javadoc coverage. While these are
@Overridemethods with documentation in the interface, adding brief method-level Javadoc (or{@inheritDoc}) would ensure 100% coverage compliance. The implementation returning empty strings to produce leading "/" is correctly documented via inline comments.🔎 Example Javadoc addition
+ /** {@inheritDoc} */ @Override @NonNull public String formatMetaschema(IModuleNodeItem metaschema) { // Returns empty string to produce leading "/" via join in format method return ""; } + /** {@inheritDoc} */ @Override @NonNull public String formatDocument(IDocumentNodeItem document) { // Returns empty string to produce leading "/" via join in format method return ""; }
57-85: Missing Javadoc on public override methods.Per coding guidelines, all public members require Javadoc. The methods
formatRootAssembly,formatAssembly(both overloads),formatField, andformatFlaglack documentation. Consider adding{@inheritDoc}tags for interface contract documentation.🔎 Add Javadoc to public methods
+ /** {@inheritDoc} */ @Override @NonNull public String formatRootAssembly(IRootAssemblyNodeItem root) { return ObjectUtils.notNull(root.getQName().toEQName()); } + /** {@inheritDoc} */ @Override @NonNull public String formatAssembly(IAssemblyNodeItem assembly) { return formatModelItem(assembly); } + /** {@inheritDoc} */ @Override @NonNull public String formatAssembly(IAssemblyInstanceGroupedNodeItem assembly) { return formatModelItem(assembly); } + /** {@inheritDoc} */ @Override @NonNull public String formatField(IFieldNodeItem field) { return formatModelItem(field); } + /** {@inheritDoc} */ @Override @NonNull public String formatFlag(IFlagNodeItem flag) { return ObjectUtils.notNull("@" + flag.getQName().toEQName()); }As per coding guidelines, new code requires 100% Javadoc coverage on public/protected members.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
PRDs/20251215-path-formatters/plan.md(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/IPathFormatter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPathFormatter.java(0 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPointerFormatter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/PathFormatSelection.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/XPathFormatter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/package-info.java(4 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPointerFormatterTest.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/format/XPathFormatterTest.java(1 hunks)databind/src/main/java/gov/nist/secauto/metaschema/databind/io/Format.java(3 hunks)databind/src/test/java/gov/nist/secauto/metaschema/databind/io/FormatPathFormatterTest.java(1 hunks)metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/CLI.java(2 hunks)metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/AbstractValidateContentCommand.java(7 hunks)metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/util/LoggingValidationHandler.java(4 hunks)
💤 Files with no reviewable changes (1)
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPathFormatter.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPointerFormatter.java
- core/src/test/java/gov/nist/secauto/metaschema/core/metapath/format/XPathFormatterTest.java
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.java: All code changes must follow the Javadoc style guide (docs/javadoc-style-guide.md). New code requires 100% Javadoc coverage on public/protected members. Modified code must add/update Javadoc on any members touched. All Javadoc must include @param, @return, @throws tags in the correct order (BLOCKING)
Java target version must be Java 11. Use SpotBugs annotations (@nonnull, @nullable) for null safety in code.
Follow package naming convention gov.nist.secauto.metaschema.* for all Java packages
Follow Test-Driven Development (TDD) principles: write tests first before implementing functionality, verify tests fail with current implementation, implement minimal code to pass tests, then refactor while keeping tests green
Files:
metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/CLI.javacore/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/package-info.javacore/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/PathFormatSelection.javametaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/util/LoggingValidationHandler.javametaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/AbstractValidateContentCommand.javacore/src/test/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPointerFormatterTest.javadatabind/src/test/java/gov/nist/secauto/metaschema/databind/io/FormatPathFormatterTest.javacore/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/XPathFormatter.javacore/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/IPathFormatter.javadatabind/src/main/java/gov/nist/secauto/metaschema/databind/io/Format.java
**/metapath/**/*.java
📄 CodeRabbit inference engine (CLAUDE.md)
Metapath is an implementation of XPath 3.1. Use the XPath 3.1 specification (https://www.w3.org/TR/xpath-31/) and XPath Functions 3.1 (https://www.w3.org/TR/xpath-functions-31/) as authoritative reference when implementing new functions, fixing bugs, or understanding error handling. Raise clarification before making changes if implementation differs from spec.
Files:
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/package-info.javacore/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/PathFormatSelection.javacore/src/test/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPointerFormatterTest.javacore/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/XPathFormatter.javacore/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/IPathFormatter.java
**/*Test.java
📄 CodeRabbit inference engine (CLAUDE.md)
@test methods do not require Javadoc if they use descriptive method names. Use JUnit 5 for testing with parallel execution enabled.
Files:
core/src/test/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPointerFormatterTest.javadatabind/src/test/java/gov/nist/secauto/metaschema/databind/io/FormatPathFormatterTest.java
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: Applies to **/metapath/**/*.java : Metapath is an implementation of XPath 3.1. Use the XPath 3.1 specification (https://www.w3.org/TR/xpath-31/) and XPath Functions 3.1 (https://www.w3.org/TR/xpath-functions-31/) as authoritative reference when implementing new functions, fixing bugs, or understanding error handling. Raise clarification before making changes if implementation differs from spec.
📚 Learning: 2025-12-17T13:27:43.669Z
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: Applies to **/metapath/**/*.java : Metapath is an implementation of XPath 3.1. Use the XPath 3.1 specification (https://www.w3.org/TR/xpath-31/) and XPath Functions 3.1 (https://www.w3.org/TR/xpath-functions-31/) as authoritative reference when implementing new functions, fixing bugs, or understanding error handling. Raise clarification before making changes if implementation differs from spec.
Applied to files:
metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/CLI.javacore/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/package-info.javacore/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/PathFormatSelection.javaPRDs/20251215-path-formatters/plan.mdmetaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/util/LoggingValidationHandler.javametaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/AbstractValidateContentCommand.javadatabind/src/test/java/gov/nist/secauto/metaschema/databind/io/FormatPathFormatterTest.javacore/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/XPathFormatter.javacore/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/IPathFormatter.javadatabind/src/main/java/gov/nist/secauto/metaschema/databind/io/Format.java
📚 Learning: 2025-12-17T13:27:43.669Z
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: Applies to **/*.java : Follow package naming convention gov.nist.secauto.metaschema.* for all Java packages
Applied to files:
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/package-info.javadatabind/src/main/java/gov/nist/secauto/metaschema/databind/io/Format.java
📚 Learning: 2024-11-14T18:19:40.200Z
Learnt from: david-waltermire
Repo: metaschema-framework/metaschema-java PR: 245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBooleanItem.java:86-104
Timestamp: 2024-11-14T18:19:40.200Z
Learning: In the file `core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBooleanItem.java`, the 3-step approach in the `cast` method is consistent with the XPath 3.1 specification.
Applied to files:
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/XPathFormatter.java
📚 Learning: 2025-12-17T13:27:43.669Z
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: Applies to **/*.java : Java target version must be Java 11. Use SpotBugs annotations (NonNull, Nullable) for null safety in code.
Applied to files:
databind/src/main/java/gov/nist/secauto/metaschema/databind/io/Format.java
🧬 Code graph analysis (4)
metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/CLI.java (1)
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/completion/CompletionTypeRegistry.java (1)
CompletionTypeRegistry(30-154)
metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/util/LoggingValidationHandler.java (1)
metaschema-maven-plugin/src/main/java/gov/nist/secauto/metaschema/maven/plugin/AbstractMetaschemaMojo.java (1)
LoggingValidationHandler(575-701)
metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/AbstractValidateContentCommand.java (1)
metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/util/LoggingValidationHandler.java (1)
LoggingValidationHandler(37-248)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/XPathFormatter.java (1)
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 (19)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/IPathFormatter.java (2)
36-46: LGTM!The XPATH_PATH_FORMATTER constant is well-documented with clear Javadoc, an illustrative example, and proper @see reference to the implementation class.
48-60: LGTM!The JSON_POINTER_PATH_FORMATTER constant is properly documented with comprehensive Javadoc, RFC 6901 reference, and appropriate @see tags.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/package-info.java (1)
23-66: LGTM!The package documentation accurately reflects the expanded path formatter ecosystem with clear examples and usage patterns for all three formatters (Metapath, XPath, JSON Pointer).
PRDs/20251215-path-formatters/plan.md (1)
1-493: PRD documentation is informational.This is a planning document that provides design specifications and implementation guidance. Previous review comments have been addressed in commits, and no further review is needed for this planning artifact.
metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/CLI.java (1)
15-15: LGTM!The PathFormatSelection import and registration follow the established pattern for enum completion support, enabling shell autocomplete for the new path format options.
Also applies to: 74-74
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/PathFormatSelection.java (1)
1-54: LGTM!The PathFormatSelection enum is well-designed with comprehensive Javadoc coverage on all constants. The AUTO option's behavior is clearly documented, and examples for each format help users understand the differences.
databind/src/test/java/gov/nist/secauto/metaschema/databind/io/FormatPathFormatterTest.java (1)
1-125: LGTM!Comprehensive test coverage for path formatter resolution logic. Tests verify correct formatter selection for all Format types (XML, JSON, YAML) and all PathFormatSelection options (AUTO, METAPATH, XPATH, JSON_POINTER), including the null fallback case.
databind/src/main/java/gov/nist/secauto/metaschema/databind/io/Format.java (2)
93-109: LGTM!The getPathFormatter() method correctly maps each Format to its appropriate path formatter (XPath for XML, JSON Pointer for JSON/YAML). Javadoc is comprehensive and follows the style guide.
111-140: LGTM!The resolvePathFormatter() method correctly implements the selection logic for AUTO and explicit path format choices. The fallback to METAPATH_PATH_FORMATER when format is null is appropriate. Javadoc is comprehensive with proper @param and @return tags per coding guidelines.
core/src/test/java/gov/nist/secauto/metaschema/core/metapath/format/JsonPointerFormatterTest.java (1)
1-519: LGTM!Comprehensive test coverage for JsonPointerFormatter across all node types, JSON grouping behaviors (NONE, LIST, SINGLETON_OR_LIST, KEYED), and RFC 6901 escaping rules. Tests validate correct 0-based indexing, key extraction, sibling counting, and full path composition.
metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/util/LoggingValidationHandler.java (4)
47-48: LGTM!The
@NonNullannotation on thepathFormatterfield properly ensures null safety. The field declaration follows the SpotBugs annotation conventions.
77-104: LGTM!The two factory methods are well-documented with complete Javadoc including
@paramand@returntags. The method signatures properly use@NonNullannotations for both parameters and return types, following the null safety conventions. The design pattern of providing both a simple factory (withPathFormatter) and a full-featured factory (withSettings) offers good flexibility.
106-113: LGTM!The constructor chaining correctly delegates to the new overload with a default path formatter (
IPathFormatter.METAPATH_PATH_FORMATER), maintaining backward compatibility for the existing singleton instances.
163-184: The code change is correct and safe.The
toPath()method is called withpathFormatter, which is annotated as@NonNulland initialized in the constructor, guaranteeing a non-null value. No null-handling verification needed.metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/AbstractValidateContentCommand.java (2)
340-364: LGTM!The
resolvePathFormattermethod is well-documented with complete Javadoc. The logic correctly defaults toPathFormatSelection.AUTOand delegates toFormat.resolvePathFormatter()for format-specific resolution. The null check on line 358 properly guards against missing option values.
332-336: LGTM!The integration correctly resolves the path formatter before creating the handler, and the handler is properly instantiated using the new
withPathFormatter()factory method. This ensures validation findings are formatted according to user preference or document format default.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/format/XPathFormatter.java (3)
23-40: LGTM! Class Javadoc is comprehensive.The class-level documentation clearly explains the EQName format, provides an example path, and documents the XML grouping behavior. The
@seetag properly references theIEnhancedQName.toEQName()method.
87-120: LGTM! The private helper method is well-documented and correctly implements XML grouping logic.The
formatModelItemmethod properly handles the XmlGroupAsBehavior.GROUPED case by prepending the wrapper element with[1]position (since the wrapper appears exactly once). The EQName format with position predicates follows XPath 3.1 syntax. The null checks oninstanceandwrapperQNameare defensive and appropriate.Based on learnings, this implementation aligns with XPath 3.1 specification for path expressions with EQName notation and positional predicates.
81-85: Clarify the purpose of the@prefix notation in flag formatting.The
@prefix is standard XPath abbreviated syntax for the attribute axis, and EQName formatQ{namespace}localnameis correct per XPath 3.1 specification. However, this method formats flag nodes in the Metapath data model, not XML attribute nodes. Since Metapath uses a custom node model distinct from XPath (with flag, field, and assembly nodes rather than element/attribute nodes), verify that@Q{namespace}localnameis the intended representation for formatted flag references in this context. If this is a serialization format rather than an executable XPath expression, confirm the documentation or comments clarify the purpose.
david-waltermire
merged commit 1c8dd9f
into
metaschema-framework:develop
2 participants
Summary
This PR adds new path formatters and format-based selection to the Metaschema framework:
Addresses metaschema-framework/oscal-cli#32
Key Features
Path Formatters:
Q{namespace}element[position])~→~0,/→~1)Format-Based Selection:
--path-formatCLI option to override:auto,metapath,xpath,jsonpointerChanges
Core Module
XPathFormatter- XPath 3.1 paths with EQName-qualified namesJsonPointerFormatter- RFC 6901 compliant JSON Pointer pathsPathFormatSelectionenum for path format optionsXPATH_PATH_FORMATTERandJSON_POINTER_PATH_FORMATTERconstants toIPathFormatterJsonPathFormatterplaceholder codeDatabind Module
Format.getPathFormatter()- returns appropriate formatter for document formatFormat.resolvePathFormatter(PathFormatSelection, Format)- resolves formatter based on selectionCLI Module
--path-formatoption to validation commandsLoggingValidationHandlerto use configurable path formatterTest plan
mvn install -PCI -Prelease)Summary by CodeRabbit
New Features
--path-formatCLI option for selecting path output format (auto, metapath, xpath, jsonpointer)Removals
✏️ Tip: You can customize this high-level summary in your review settings.