Support a Metapath execution stack #336
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request introduces a comprehensive refactoring of the Metaschema Core Metapath expression handling system. The primary changes involve renaming methods from Changes
Sequence DiagramsequenceDiagram
participant DC as DynamicContext
participant E as Expression
participant CS as ChildExpression
DC->>E: pushExecutionStack(expression)
E->>CS: evaluate(dynamicContext, focus)
CS-->>E: return result
E->>DC: popExecutionStack(expression)
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/EmptySequence.java
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Range.java
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/UnaryLookup.java
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Union.java (1)
Line range hint
57-66: Consider pushing and popping the current expression to the DynamicContext’s execution stack for improved debugging.Since this PR aims to provide an execution stack for expression evaluation, it’s recommended to wrap child evaluations in push/pop calls for the current
Unionexpression. This ensures that all entered and exited expressions are tracked consistently, enhancing both debugging context and error reporting.Below is a potential approach:
@Override protected ISequence<?> evaluate(DynamicContext dynamicContext, ISequence<?> focus) { + dynamicContext.pushExecutionStack(this); + try { @NonNull Stream<? extends IItem> retval = ObjectUtils.notNull(getChildren().stream() .flatMap(child -> { ISequence<?> result = child.accept( dynamicContext, focus.reusable()); return result.stream(); }).distinct()); return ISequence.of(retval); + } finally { + dynamicContext.popExecutionStack(); + } }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/PostfixLookup.java (1)
Line range hint
69-79: Consider renaming.accept(...)to.evaluate(...)in the base expression call.
The newly introducedevaluate(...)method replacesaccept(...)for expression evaluation, yet the base expression call still uses.accept(...). If the base expression is also updated to support.evaluate(...), align these method names for consistency.- ISequence<?> base = getBase().accept(dynamicContext, focus); + ISequence<?> base = getBase().evaluate(dynamicContext, focus);
🧹 Nitpick comments (25)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/ValueComparison.java (1)
53-57: Method rename and visibility change aligns with broader refactoring patternThe change from
public accept()toprotected evaluate()is part of a broader refactoring pattern that:
- Better encapsulates the evaluation logic by making it protected
- Uses more descriptive method naming ('evaluate' vs 'accept')
- Supports the new execution stack feature for debugging and error reporting
The implementation maintains the existing functionality while improving the design.
This change follows good design principles by:
- Improving encapsulation through proper visibility modifiers
- Using more meaningful method names that better describe their purpose
- Maintaining consistency across the codebase as part of a larger refactoring effort
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/If.java (1)
89-89: Align sub-expression calls with the renamed method.
While thisevaluatemethod replaces the oldacceptlogic, it still invokesaccepton sub-expressions (getTestExpression().accept(...), etc.). If your intent is to rename all expression processing methods toevaluate, you should update these calls accordingly throughout the sub-expressions to prevent confusion and maintain consistent naming. Otherwise, confirm that it is intentional to retain theacceptmethod for sub-expressions.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/StringLiteral.java (1)
30-30: Validate string length before stripping quotes.Currently,
removeQuotes(value)assumes the string is at least two characters long and properly enclosed in quotes. Consider adding a defensive check to avoid potentialIndexOutOfBoundsExceptionfor malformed strings.private static String removeQuotes(@NonNull String value) { + if (value.length() < 2) { + throw new IllegalArgumentException("String not enclosed in quotes: " + value); + } return value.substring(1, value.length() - 1); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Intersect.java (1)
Line range hint
16-22: Correct the Javadoc to refer to “intersect” rather than “except.”The constructor Javadoc appears to be a copy-paste artifact referring to “Metapath except expression.” For clarity and consistency with the class name
Intersect, consider updating the doc comment as follows:- * Construct a new Metapath except expression CST node. + * Construct a new Metapath intersect expression CST node.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootSlashPath.java (1)
48-48: Consider adding a Javadoc comment for theevaluatemethod.The method’s purpose is clear from context, but providing a brief Javadoc block clarifying the evaluation approach and details of how the input
focusis transformed would enhance maintainability.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/Treat.java (1)
64-64: Use generics to ensure type safety.While switching from
public ISequence<? extends IItem>toprotected ISequence<?>provides flexibility, it may be beneficial to use generics or a more specific return type to avoid type erasure and maintain stricter compile-time checks. If the method usage must remain this generic, consider adding documentation explaining why.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/MapConstructor.java (2)
Line range hint
55-64: Ensure robust error reporting.The switch to
toCSTString()for the key expression in the error message is helpful for diagnosing parsing-level issues. However, consider also including context about the current evaluation stack if available, to pinpoint the failing expression more precisely during debugging.
132-132: Refine exception logic or message.Throwing
UnsupportedOperationExceptionindicates thatevaluateis intentionally delegated to the parent class. Consider adding a clarifying message or a Javadoc comment explaining that this method is deliberately unimplemented because the map entry logic is rolled up into the parent’sevaluatemethod.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/StringConcat.java (1)
71-71: Consider renaming the child calls fromaccepttoevaluatefor consistency.
Thisevaluatemethod relies on callingchild.accept(...)on each child expression. To enhance consistency and clarity throughout the codebase, consider refactoring these calls tochild.evaluate(...)if feasible.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/FunctionCallAccessor.java (1)
79-79: Maintain consistent naming across calls.
While the method is now namedevaluate, you still invokeaccept(dynamicContext, focus)on the base expression. This might introduce confusion or hamper discoverability. Consider renaming the child'sacceptmethod toevaluateto align with the broader refactoring.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/Let.java (1)
85-85: Promote internal consistency by renaming child calls toevaluate.
Similarly, thisevaluatemethod callsgetReturnExpression().accept(...). For clarity, consider renaming the child's method toevaluate(...), so that handling is consistent across the entire codebase.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/KindNodeTest.java (1)
58-58: Consider clarifying thetoCSTString()method name in comments or docstrings.Renaming to
toCSTString()is consistent with representing "Concrete Syntax Tree," but adding a short explanation may help future maintainers distinguish this method from anytoASTString()usage in other classes.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Except.java (1)
Line range hint
28-35: Consider optimizing the filter operation to avoid O(n²) complexity.Currently, using
items.contains(item)in a stream filter runs in O(n) per item. Ifitemsis large, this approach can significantly degrade performance. Convertingitemsto aSetbefore filtering can reduce the overall complexity to O(n).Below is one way to implement this change:
protected ISequence<?> applyFilterTo(@NonNull List<? extends IItem> source, @NonNull List<? extends IItem> items) { + Set<IItem> filterSet = new HashSet<>(items); return ISequence.of(ObjectUtils.notNull( source.stream() - .filter(item -> !items.contains(item)) + .filter(item -> !filterSet.contains(item)) )); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/ContextItem.java (1)
69-69: Handle null focus to avoid potential NPE.Currently, only an empty
focusis checked. While your higher-level code may guarantee thatfocusis nevernull, guarding against a null pointer would make this method more robust.One possible fix:
protected ISequence<?> evaluate(DynamicContext dynamicContext, ISequence<?> focus) { + if (focus == null) { + throw new DynamicMetapathException(DynamicMetapathException.DYNAMIC_CONTEXT_ABSENT, "The context is null"); + } if (focus.isEmpty()) { throw new DynamicMetapathException(DynamicMetapathException.DYNAMIC_CONTEXT_ABSENT, "The context is empty"); } return focus; }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/EmptySequence.java (1)
63-63: Consider adding debug instrumentation for consistency with the new execution stack feature.If other expressions in this PR push/pop from the
dynamicContextto track the current expression in the stack, you may want to do the same here, even though the sequence is empty. This ensures consistent behavior and debugging.@Override protected ISequence<RESULT_TYPE> evaluate(DynamicContext dynamicContext, ISequence<?> focus) { + // Optionally track expression for debugging and error reporting + dynamicContext.push(this); ISequence<RESULT_TYPE> retval = ISequence.empty(); + dynamicContext.pop(); return retval; }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/PredicateExpression.java (1)
112-112: Review type-safety and possible performance implications.Using
asInteger()on theIntegerLiteralfor indexing is acceptable; however, consider adding checks to ensure the integer value is within expected bounds. Large values could potentially degrade performance or lead to unexpected results.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Quantified.java (1)
Line range hint
136-196: Consider unifying naming and enhancing type safety.
Method Renaming Consistency
The class-level method changed fromaccept(...)toevaluate(...), but this block still callsaccept(...)on sub-expressions (e.g.,entry.getValue().accept(...)). If your intent is to standardize evaluation method names, consider renaming or delegating these calls through a similarly named method to maintain consistency.Return Type Specificity
Since this expression always produces a boolean result, changing the return type fromISequence<?>toISequence<IBooleanItem>could provide stronger type safety, facilitating clearer expectations when callingevaluate(...).Logic Review
- The “SOME”/“EVERY” evaluation logic is correct and short-circuits as expected on the first definitive result.
- The cartesian product iteration is suitably encapsulated in the inner class.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/AbstractLiteralExpression.java (1)
47-50: Concise evaluation method.
Theevaluateimplementation properly wraps the literal in anISequence.
Optional enhancement: If feasible in the overall design, consider returningISequence<RESULT_TYPE>instead ofISequence<?>for stricter typing.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/IExpression.java (1)
80-80: RenamingtoASTString()totoCSTString()
Renaming this method to “CST” terminology is consistent with other refactorings. Verify references in downstream classes or tests to ensure proper usage.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AnonymousFunctionCall.java (1)
87-87: PublictoCSTString()method
Renaming fromtoASTString()totoCSTString()aligns with the rest of the code’s naming convention. Adding public visibility is acceptable, but verify this method is not inadvertently exposed outside the intended scope.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/DynamicContext.java (1)
311-320: AddedpushExecutionStack().
Properly stores the current expression for debugging or introspection. Consider adding unit tests to confirm stack behavior under concurrent or nested expression scenarios.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractExpression.java (1)
53-65: Consider unifying generics for improved consistency.
acceptreturnsISequence<?>, whereasevaluatereturnsISequence<? extends IItem>. If feasible, unify these generics to maintain a consistent API boundary and avoid potential identity casting or typed usage confusion.Potential approach:
-protected abstract ISequence<? extends IItem> evaluate( +protected abstract <T extends IItem> ISequence<T> evaluate( @NonNull DynamicContext dynamicContext, @NonNull ISequence<?> focus);core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/PostfixLookup.java (1)
Line range hint
69-79: Replace or handleassert item != nullmore robustly.
Assertions may be disabled at runtime, so consider throwing a dedicated exception if anullitem truly indicates an error case.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootSlashOnlyPath.java (2)
63-63: Consider adding JavaDoc to clarify the evaluation approach.The new
evaluate()method introduces core logic for retrieving the root document node, but its behavior and potential edge cases (e.g., empty focus) could be more transparent if documented in JavaDoc, especially given this is a key method for resolving the root context in Metapath evaluation.
Line range hint
64-70: Refactor the chained stream operations for clarity.This stream chain is functional, but it can be harder to follow. Consider extracting sub-steps into local variables or a small helper method, which might improve readability without sacrificing performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (84)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/DynamicContext.java(14 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/IExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/MetapathExpression.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractBinaryExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractCSTVisitorBase.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractExpressionVisitor.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractNAryExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractUnaryExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AnonymousFunctionCall.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/BuildCSTVisitor.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/CSTPrinter.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/DynamicFunctionCall.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/ExpressionUtils.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/For.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/FunctionCallAccessor.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/ICstExpressionFactory.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/Let.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/NamedFunctionReference.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/StaticFunctionCall.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/VariableReference.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/AbstractFilterExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/AbstractLiteralExpression.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ArraySequenceConstructor.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ArraySquareConstructor.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/DecimalLiteral.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/EmptySequence.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Except.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ILiteralExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/IntegerLiteral.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Intersect.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/MapConstructor.java(4 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/PostfixLookup.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Quantified.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Range.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/SequenceExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/SimpleMap.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/StringConcat.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/StringLiteral.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/UnaryLookup.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Union.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractComparison.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/And.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/GeneralComparison.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/IBooleanLogicExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/If.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/Or.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/PredicateExpression.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/ValueComparison.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/AbstractArithmeticExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/AbstractBasicArithmeticExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Addition.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Division.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/IntegerDivision.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Modulo.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Multiplication.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Negate.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Subtraction.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractRelativePathExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractRootPathExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractSearchPathExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractStepExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/ContextItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/INodeTestExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/KindNodeTest.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/NameNodeTest.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RelativeDoubleSlashPath.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RelativeSlashPath.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootDoubleSlashPath.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootSlashOnlyPath.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootSlashPath.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/Step.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/WildcardNodeTest.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/AbstractCastingExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/Cast.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/Castable.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/InstanceOf.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/Treat.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/impl/AbstractKeySpecifier.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/ExpressionUtilsTest.java(0 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/BuildCstVisitorTest.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/CSTLogicalExpressionsTest.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/PredicateExpressionTest.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/ValueComparisonTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- core/src/test/java/gov/nist/secauto/metaschema/core/metapath/ExpressionUtilsTest.java
✅ Files skipped from review due to trivial changes (8)
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractCSTVisitorBase.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Addition.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractRelativePathExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Division.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractSearchPathExpression.java
- core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/PredicateExpressionTest.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/AbstractArithmeticExpression.java
- core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/ValueComparisonTest.java
🧰 Additional context used
📓 Learnings (13)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractBinaryExpression.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#315
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractFilterExpression.java:56-56
Timestamp: 2024-12-31T18:45:39.097Z
Learning: "ISequence" in the codebase is an implementation of the List interface, allowing direct usage where a List is expected.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Except.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#315
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractFilterExpression.java:56-56
Timestamp: 2024-12-31T18:45:39.097Z
Learning: "ISequence" in the codebase is an implementation of the List interface, allowing direct usage where a List is expected.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/IBooleanLogicExpression.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#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.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/CSTPrinter.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#315
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractFilterExpression.java:56-56
Timestamp: 2024-12-31T18:45:39.097Z
Learning: "ISequence" in the codebase is an implementation of the List interface, allowing direct usage where a List is expected.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Intersect.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#315
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractFilterExpression.java:56-56
Timestamp: 2024-12-31T18:45:39.097Z
Learning: "ISequence" in the codebase is an implementation of the List interface, allowing direct usage where a List is expected.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractNAryExpression.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#315
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractFilterExpression.java:56-56
Timestamp: 2024-12-31T18:45:39.097Z
Learning: "ISequence" in the codebase is an implementation of the List interface, allowing direct usage where a List is expected.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractExpressionVisitor.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#315
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractFilterExpression.java:56-56
Timestamp: 2024-12-31T18:45:39.097Z
Learning: "ISequence" in the codebase is an implementation of the List interface, allowing direct usage where a List is expected.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/ExpressionUtils.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#315
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractFilterExpression.java:56-56
Timestamp: 2024-12-31T18:45:39.097Z
Learning: "ISequence" in the codebase is an implementation of the List interface, allowing direct usage where a List is expected.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/AbstractCastingExpression.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#315
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractFilterExpression.java:56-56
Timestamp: 2024-12-31T18:45:39.097Z
Learning: "ISequence" in the codebase is an implementation of the List interface, allowing direct usage where a List is expected.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/FunctionCallAccessor.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#266
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/FunctionCallAccessor.java:82-89
Timestamp: 2024-11-28T04:47:39.173Z
Learning: In `FunctionCallAccessor.java`, the `accept` method must continue executing after handling `AnonymousFunctionCall` to properly process other collection types. Adding an `else` block that bypasses the remaining code is incorrect.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/AbstractFilterExpression.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#315
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractFilterExpression.java:56-56
Timestamp: 2024-12-31T18:45:39.097Z
Learning: "ISequence" in the codebase is an implementation of the List interface, allowing direct usage where a List is expected.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/AbstractLiteralExpression.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#315
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractFilterExpression.java:56-56
Timestamp: 2024-12-31T18:45:39.097Z
Learning: "ISequence" in the codebase is an implementation of the List interface, allowing direct usage where a List is expected.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractUnaryExpression.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#315
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractFilterExpression.java:56-56
Timestamp: 2024-12-31T18:45:39.097Z
Learning: "ISequence" in the codebase is an implementation of the List interface, allowing direct usage where a List is expected.
🔇 Additional comments (136)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/impl/AbstractKeySpecifier.java (1)
9-9: Consistent interface placement is beneficial.Switching to
gov.nist.secauto.metaschema.core.metapath.IExpressionhelps ensure that theIExpressioninterface is referenced from its more general location, promoting consistent package organization. Nice work!core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/WildcardNodeTest.java (2)
54-54: Maintain consistent method signature in refactored classes.The new
evaluatemethod aligns with the renaming convention fromaccepttoevaluateused elsewhere. Making itprotectedis consistent with limiting external access, and the usage of streams ensures efficient processing. Overall, this change appears structurally sound.
97-97: Renaming totoCSTString()is consistent with the shift from AST to CST.The updated method name clearly indicates that the returned string represents the concrete syntax tree, which improves clarity and consistency across the refactored codebase.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/ValueComparison.java (1)
9-9: Verify import statement consistency across codebaseThe import statement has been moved to a more general package, which aligns with the consolidation of the
IExpressioninterface imports.Let's verify this change is consistent across the codebase:
✅ Verification successful
Import change is consistent across codebase
The search results confirm that all files are using the new import path
gov.nist.secauto.metaschema.core.metapath.IExpressionconsistently. There are no remaining references to the old package-specific import path containing.cst.IExpression. This change aligns with the broader refactoring effort to consolidate theIExpressioninterface imports.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining imports from the old package rg "import.*\.cst\.IExpression;" -g '*.java' # Search for imports from the new package rg "import.*\.metapath\.IExpression;" -g '*.java'Length of output: 11246
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/DecimalLiteral.java (2)
22-22: Confirm the correctness of the generic type signature.
Switching from a multi-generic signature toAbstractLiteralExpression<IDecimalItem>is consistent with the refactor but ensure that any code relying on a second type parameter (e.g.,BigDecimal) no longer needs it.
33-33: Validate theIDecimalItem.valueOf(...)usage.
The call toIDecimalItem.valueOf(value)is appropriate and streamlines the instantiation. Just confirm thatvalueOf(...)handles large decimal inputs and edge cases (e.g., negative or extremely large values) as expected.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/Step.java (3)
9-9: Import references updated for clarity
The new import statement correctly referencesIExpressionin themetaschema.core.metapathpackage, which aligns with the broader refactoring changes and enhances consistency.
Line range hint
110-130: Visibility change and method name refactor
Changing the method from a publicacceptto a protectedevaluatealigns with the encapsulation of internal logic while retaining the existing functionality. The usage ofaxis.execute(item).distinct()is appropriate here, ensuring duplicates are removed, although you should confirm that discarding duplicates is the intended behavior for all use cases. Other than that, the method appears coherent and maintains readability.
131-131: Renamed 'toASTString()' to 'toCSTString()'
The rename and updated content of this method correctly follow the codebase transition from AST to CST terminology. The format string usage is clear and helps maintain consistent naming across the system.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/IntegerLiteral.java (2)
22-22: Good alignment with new generic type simplification.Changing the class to use
AbstractLiteralExpression<IIntegerItem>conforms nicely to the updated design, reducing complexity by removing the additional type parameter. This refactor increases consistency across literal expressions.
33-33: Properly convertingBigIntegertoIIntegerItem.Using
IIntegerItem.valueOf(value)at construction is correct and ensures that the literal expression consistently returns anIIntegerItemas required by its new parent class. No further changes needed.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractStepExpression.java (2)
66-66: Ensure downstream calls toevaluate()are properly adjusted.The method name and visibility have changed from
accept(public) toevaluate(protected). This may impact any classes or interfaces relying on the old signature. Verify that all subclasses override or call this method consistently.If references to the old
accept()exist, you might provide a short deprecation or bridging layer to avoid breakage. Do you want me to open a new GitHub issue to track this migration strategy?
9-9: Confirm alternative references toIExpressionhave been updated.This import update changes the package location of
IExpression. Ensure that other references to this interface across the codebase are also updated, to avoid any potential confusion or ambiguous imports.Run the following script to ensure there are no remaining imports from the old package:
✅ Verification successful
Let me search for actual references to
IExpressionto verify if there are any remaining imports from the old package.
All imports of
IExpressionare using the correct package pathBased on the search results, all imports of
IExpressionare consistently using the correct package pathgov.nist.secauto.metaschema.core.metapath.IExpression. There are no remaining imports from the old package path. The import change in the file under review is consistent with how the interface is being imported across the entire codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for references to the old package path for IExpression. rg "import gov\.nist\.secauto\.metaschema\.core\.metapath\.cst\.IExpression"Length of output: 7640
Script:
#!/bin/bash # Search for direct imports of IExpression rg "import .*\.IExpression;"Length of output: 11178
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Range.java (2)
9-9: The updated import maintains consistency with the new evaluation approach.
This change ensures thatIExpressionreferences align with the updated framework for Metapath expressions as described in the PR.
54-54: Rename fromaccepttoevaluateand shift toprotectedis well-aligned with the updated design.
Renaming the method toevaluateclarifies its role in the Metapath processing flow. Changing the scope frompublictoprotectedhelps encapsulate the evaluation logic, preventing unintended external access. This is consistent with good object-oriented design principles.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Subtraction.java (1)
8-8: Proper import usage.Switching the import of
IExpressionfrom thecstsubpackage tometapathis consistent with the broader refactoring efforts. This change improves clarity and aligns with the new package structure.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractExpressionVisitor.java (1)
8-8: Well-structured usage of the new import.
This addition clarifies references toIExpressionand aligns well with the visitor pattern usage throughout the class, improving overall type safety and readability.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/If.java (1)
9-9: Confirm correct import path change.
The package name forIExpressionhas changed fromcsttometapath. Make sure that other related imports and references follow this new package structure consistently to avoid any breakage.Run the following script to locate and confirm all
IExpressionimports across the codebase:✅ Verification successful
All imports of
IExpressionare consistently using the new package pathThe search results show that all imports of
IExpressionacross the codebase are consistently using the new package pathgov.nist.secauto.metaschema.core.metapath.IExpression. There are no remaining imports from the oldcstpackage path. The change appears to have been applied consistently throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify consistent usage of the new IExpression import path. rg -A 2 "import.*IExpression"Length of output: 48210
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/StringLiteral.java (1)
20-20: Simplified generics improves readability.Transitioning from
AbstractLiteralExpression<IStringItem, String>toAbstractLiteralExpression<IStringItem>makes the class signature more concise and improves maintainability. This change is well-aligned with the broader refactoring efforts.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Intersect.java (1)
8-8: Refactored import path aligns with the broader reorganization.The switch from
gov.nist.secauto.metaschema.core.metapath.cst.IExpressiontogov.nist.secauto.metaschema.core.metapath.IExpressionis consistent with the overall package refactoring and helps clarify whereIExpressionis defined. This practice improves code structure and organization.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/INodeTestExpression.java (1)
8-8: Ensure references toIExpressionare consistent across the codebase.The updated import path reflects the new package structure. Verify that no references to the old
cst.IExpressionremain in the repository to prevent compilation or functionality issues.Use the following script to locate any references to
cst.IExpression:core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/SequenceExpression.java (2)
9-9: Import usage looks goodThis new import for
IExpressionis necessary for referencing the interface within the constructor.
57-57: Consider integrating the new execution stack logicThe renamed and scoped-down
evaluatemethod is consistent with the refactoring. However, since the PR objective mentions adding an execution stack to track expressions for debugging and error reporting, ensure that push/pop calls are included here or in the invoked methods to record thisSequenceExpressionevaluation in the stack.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Negate.java (2)
9-9: Import usage looks consistent.
No concerns with introducingIExpressionin this file. The new import aligns well with the constructor usage and maintains a coherent design within the Metapath framework.
63-63: Evaluate method field stack usage check.
The newevaluate()method properly handles negation logic. However, ensure that the new execution stack is updated (pushed/popped) if it is relevant to capturing the current evaluation context. Verify that it aligns with the broader stack-based approach introduced in the PR.Would you like me to propose a code snippet showing potential stack push/pop calls to ensure full alignment with the DynamicContext execution stack?
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/Cast.java (2)
9-9: Import usage is consistent with the broader refactoring efforts.Switching from the
cst.IExpressionimport to themetapath.IExpressionimport aligns with the project's reorganized package structure. This helps maintain consistency across the codebase.
52-52: Method signature change: Evaluate potential impact of modified visibility and return type.Renaming
accepttoevaluateand reducing visibility frompublictoprotectedis logical if external consumers no longer require direct access. However, changing the return type fromISequence<? extends IItem>to a coarserISequence<?>may reduce type safety. Verify no external usage depends on the finer-grained type, ensuring this broader return type does not introduce runtime casting issues.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootSlashPath.java (1)
9-9: Confirm the updated import path forIExpression.Ensure that all references to
IExpressionin the codebase align with this new package path, and that no stale imports remain in other classes.✅ Verification successful
Based on the search results, I can see that
IExpressionis consistently imported fromgov.nist.secauto.metaschema.core.metapath.IExpressionacross all files in the codebase. The import statement in the file under review matches this pattern exactly. There are no stale or incorrect imports ofIExpressionfound in the codebase.The import path for
IExpressionis correctly updated and consistent throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify references to IExpression in the codebase still exist only in the new package. rg 'import.*IExpression' -A 3Length of output: 60240
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractComparison.java (2)
8-8: No issues with this import statement reordering.
The clarified import forIExpressionaligns with the reorganization for clarity.
62-62: Confirm all invocations and references to the oldtoASTString()have been updated.
Renaming totoCSTString()is consistent with the new terminology. Ensure that references totoASTString()in the rest of the codebase have been adjusted accordingly.Use this script to locate any lingering references to
toASTString():core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/And.java (2)
9-9: Confirm that this import is still needed.
It looks likeIExpressionwas already imported before, and removing the old import while adding a new one for the same class might be redundant. Verify that this import is the only reference in use and consider removing any unused imports if they exist.
57-57: Short-circuit evaluation appears correct.
Your approach to short-circuit the loop when one child returnsfalseis spot-on for AND logic, thus avoiding unnecessary processing. The use ofFnBoolean.fnBooleanAsPrimitive()for evaluating each child is consistent, but ensure that error cases (e.g., empty sequences) are handled gracefully.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Multiplication.java (1)
8-8: The updated import path looks consistent.This change moves
IExpressionto a higher-level package. Verify that all references toIExpressionin downstream files have also been updated to prevent compilation issues.Run the following script to confirm that no references to the old
cst.IExpressionpath remain:✅ Verification successful
Let me verify if there are any remaining references to the old path by searching for direct usage of
IExpressionfrom the old package.
Let me try one more verification to ensure we catch any potential references to the old path by searching for import statements.
All references to
IExpressionhave been properly updatedBased on the verification results, we can confirm that:
- There are no remaining references to
IExpressionfrom the oldcstpackage path- The search results show that files are consistently using
IExpressionVisitorfrom thecstpackage, butIExpressionitself is correctly imported from the new higher-level package location🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any references to the old import path. rg "metapath\.cst\.IExpression"Length of output: 7746
Script:
#!/bin/bash # Search for any direct usage of IExpression from the old cst package rg "cst\.IExpression[^V]" # Excluding IExpressionVisitor matchesLength of output: 27
Script:
#!/bin/bash # Search for any imports of IExpression from the old package rg "^import.*metapath\.cst\.IExpression;" --type javaLength of output: 55
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/UnaryLookup.java (2)
9-9: Confirm consistency of the import path.It’s good that you’ve switched from
cst.IExpressiontometapath.IExpression, ensuring proper alignment with the broader refactoring effort. Please verify other classes referencingIExpressionstill build correctly with this new import.
52-52: Method renaming and visibility look sound.Renaming
accepttoevaluateand changing visibility toprotectedalign with the newly established naming conventions and scoping. This helps better encapsulate the expression’s behavior. No further concerns here.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/Treat.java (2)
10-10: Ensuring correct import usage.The newly added import for
IExpressionis consistent with the usage in the file. No concerns here.
80-80: Method rename is consistent with refactoring strategy.Renaming
toASTStringtotoCSTStringaligns well with the broader renaming across the codebase, improving clarity and consistency. The usage of the method appears correct.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/Castable.java (2)
9-9: Confirm the intended interface usage.Importing
IExpressionfromgov.nist.secauto.metaschema.core.metapathinstead ofgov.nist.secauto.metaschema.core.metapath.cstis consistent with the broader refactoring across the codebase. Ensure that the rest of the code referencingIExpressionaligns with this new import path.
54-54: Verify handling ofdynamicContextwithinevaluate.The current implementation does not use any members or methods of
dynamicContextaside from callingcast(focus). Confirm if thecastmethod implicitly requires thedynamicContext, or remove it if unnecessary to reduce complexity. Additionally, consider adding tests for thisevaluatemethod to ensure the new casting logic behaves as expected.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ILiteralExpression.java (3)
8-8: Good import location adjustment.
This aligns with the refactoring away from thecstpackage.
31-31: Return type update is consistent with simplified typing.
Changing the returned value toRESULT_TYPEis consistent with the new interface definition.
24-24: Ensure all references to the removed generic type are updated.
Removing the second generic type parameter simplifies the interface. However, verify that there are no remaining references or usages of the removed type parameter in the codebase.✅ Verification successful
No remaining references to the removed generic type parameter found
Based on the comprehensive search of the codebase, all implementations and usages of
ILiteralExpressionare correctly using only a single generic type parameter that extendsIAnyAtomicItem. The interface is being used correctly in:
AbstractLiteralExpression<RESULT_TYPE extends IAnyAtomicItem>DecimalLiteral extends AbstractLiteralExpression<IDecimalItem>StringLiteral extends AbstractLiteralExpression<IStringItem>IntegerLiteral extends AbstractLiteralExpression<IIntegerItem>No instances of the old two-parameter generic syntax were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no references to the old generic type parameter exist. rg 'ILiteralExpression<[^>]+,' -A 3Length of output: 37
Script:
#!/bin/bash # Let's try a different approach to find any references to ILiteralExpression with generic parameters # Search for implementations and usages of ILiteralExpression rg "ILiteralExpression" -B 2 -A 2 # Also search for class/interface definitions that might implement/extend ILiteralExpression ast-grep --pattern 'class $_ implements $$$ILiteralExpression$$$' ast-grep --pattern 'class $_ extends $$$ILiteralExpression$$$'Length of output: 1544
Script:
#!/bin/bash # Let's search for concrete implementations of ILiteralExpression to ensure all are using the correct generic parameter # Find all files that might contain literal expression implementations fd -e java -x grep -l "LiteralExpression" {} \; | xargs cat # Also specifically look for any potential string patterns that might indicate old two-parameter usage rg "ILiteralExpression\s*<\s*\w+\s*,\s*\w+\s*>" --type javaLength of output: 6772
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/NameNodeTest.java (2)
64-64: Validate null/empty stream handling inevaluatemethod.While the usage of
ObjectUtils.notNull()ensures thatfocus.stream()isn't null, consider adding checks or documentation around empty streams. Iffocusis empty, this method would return an emptyISequence, which may be correct behavior but could also surprise callers.
78-78: RenamedtoCSTString()meets naming consistency.Renaming from
toASTString()totoCSTString()aligns with the broader rename of AST to CST. The implementation here remains concise and clear.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/MapConstructor.java (1)
9-9: LGTM on updated import location.Switching from the
cstpackage to the generalIExpressioninterface aligns with the broader refactoring efforts to consolidate expression handling in a single package.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/Or.java (1)
74-74: Rename fromaccepttoevaluateis consistent, but verify external referencesThe short-circuit logic within this method is correct and matches typical "or" behavior. However, ensure that no external code relies on the previous public
acceptmethod signature. This new protected method may not be accessible if there are external invocations.Run the following script to check for references to
Or.accept(...)that may be broken:✅ Verification successful
The rename from
accepttoevaluateis safeBased on the codebase analysis:
- The
Orclass is only instantiated in two places:
BuildCSTVisitor.javafor constructing the CSTCSTLogicalExpressionsTest.javafor unit testing- No direct calls to the
acceptmethod were found in the codebase- The test class shows that the class is used through the expression evaluation interface, not by directly calling the method
- The method is protected, limiting its visibility to the package and subclasses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find any references to the old method signature in the codebase rg -A 3 'Or\s*\(.*\)\s*{\s*.*accept\s*\('Length of output: 172
Script:
#!/bin/bash # Search for any references to 'accept' method in Or class or its usages rg -l "Or.*\.accept" # Search for any references to Or class rg "class.*extends.*Or|new\s+Or\s*\(" # Use AST to find method calls on Or instances ast-grep --pattern 'or.$accept($$$)'Length of output: 403
Script:
#!/bin/bash # Let's check the content of these files to understand the usage context cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/BuildCSTVisitor.java core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/CSTLogicalExpressionsTest.java # Also check for any inheritance hierarchy ast-grep --pattern 'class Or extends $_ { $$$ }'Length of output: 54580
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ArraySquareConstructor.java (2)
9-9: No issues with the new import.
This import correctly references theIExpressioninterface from the intended package, and there are no conflicts or unused imports detected in the provided snippet.
52-52: Confirm the impact of changing the method scope to protected.
Previously, a similar method was public. Changing it toprotectedlimits its visibility to subclasses and same-package classes. Ensure this reflects the intended encapsulation and won't break existing external usage.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/GeneralComparison.java (1)
9-9: Good alignment of package structure.Switching the import from the
cstpackage togov.nist.secauto.metaschema.core.metapath.IExpressionaligns with the broader refactoring goals. Ensure that other references are also updated to use the new import path.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/ExpressionUtils.java (1)
8-8: The import statement is well-aligned with the existing code usage.There's no immediate issue with bringing in
IExpressionfromgov.nist.secauto.metaschema.core.metapath, given it is used by the new method signature. If there are other classes that require the same interface, ensure they also use this single import source for consistency.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/StringConcat.java (1)
9-9: Import usage looks good.
No issues found with the new import ofIExpression.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/FunctionCallAccessor.java (1)
9-9: No concerns with the new import statement.
The addition of theIExpressionimport is consistent with the rest of the changes.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/Let.java (1)
9-9: Import modification accepted.
This change appears straightforward and aligns with the new usage ofIExpressionin place ofIItem.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RelativeSlashPath.java (3)
50-50: Double-check visibility impact of switching frompublictoprotected.
The method is now protected. Verify that no external callers beyond this class hierarchy need to invokeevaluate. If external usage is required, reverting topublicmay be necessary.
Line range hint
50-58: Consider renaming chained calls toevaluatefor consistency.
While this class usesevaluate(...)itself, it still calls.accept(...)on nested expressions. If all classes now supportevaluate(...)for dynamic evaluation, calling.accept(...)may be inconsistent or cause confusion. Confirm if nested expressions should also call.evaluate(...)for clarity.
9-9: Confirm consistency with broader import refactoring.
Switching togov.nist.secauto.metaschema.core.metapath.IExpressionis a viable move, but ensure that other classes in thecstpackage no longer rely on the previousIExpressionimport path.Run this script to identify any leftover references to the old package:
✅ Verification successful
Let me search for the actual
IExpressionusage to verify if the import change is consistent.
Import change is consistent with the codebase
Based on the search results, the change from
metapath.cst.IExpressiontogov.nist.secauto.metaschema.core.metapath.IExpressioninRelativeSlashPath.javais consistent with the rest of the codebase. All files are using the same import pathgov.nist.secauto.metaschema.core.metapath.IExpression, and there are no references to the old package path. The interface is defined in the core metapath package and is being correctly imported across all usages.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references to "metapath.cst.IExpression" rg "metapath\.cst\.IExpression"Length of output: 7746
Script:
#!/bin/bash # Search for IExpression usage in the cst package ast-grep --pattern 'IExpression'Length of output: 11614
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/IBooleanLogicExpression.java (1)
8-8: Looks good!This import path change aligns with the broader refactoring of
IExpressioninto a higher-level package.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/KindNodeTest.java (1)
51-56: Check for consistent usage of the newevaluatemethod across the codebase.Renaming from
accepttoevaluateis a clear improvement in readability, but ensure all references and overrides in subclasses or related classes are similarly updated for consistency. Additionally, confirm that downstream logic properly interprets the returned sequence since items are now passed throughcheckItemIsNodeItemForStep().✅ Verification successful
Let me try a different approach to search for the method references.
Based on the search results, I can now provide a final response:
The
evaluatemethod is consistently used across the codebase, replacingacceptThe codebase shows a clear pattern where:
- The
acceptmethod is defined in the interfaceIExpressionand implemented inAbstractExpressionas a public method- The protected
evaluatemethod is consistently used across all implementation classes (over 30 files found)- The
acceptmethod inAbstractExpressionserves as a wrapper that calls the protectedevaluatemethodThis architecture follows a proper template method pattern where:
- Public
acceptprovides the common execution context handling- Protected
evaluateallows subclasses to implement specific evaluation logicThe changes are consistent and no issues were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify references to the renamed/overridden `evaluate` method. ast-grep --pattern $'function_declaration(name: (_) @name) {}' --jsonLength of output: 73
Script:
#!/bin/bash # Search for files containing 'evaluate' method in Java files rg -t java "evaluate.*\(.*DynamicContext.*ISequence" -B1 -A1Length of output: 18844
Script:
#!/bin/bash # Search for files containing 'accept' method that might need to be renamed rg -t java "accept.*\(.*DynamicContext.*ISequence" -B1 -A1Length of output: 2589
Script:
#!/bin/bash # Find all Java files in the path hierarchy fd -e java . core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/Length of output: 2215
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/StaticFunctionCall.java (3)
9-9: Consistent Usage ofIExpressionInterfaceThe introduction of the
IExpressionimport aligns with the use ofList<IExpression>in the constructor arguments, improving consistency. Confirm that all relevant implementations ofIExpressionare properly imported and used across the codebase.
106-106: Scope Change frompublictoprotectedChanging the method from
publictoprotectedmay affect external access. Verify that any consumers requiring direct invocation have alternative interfaces. The rename fromaccept()toevaluate()clarifies the method’s intent for expression evaluation.
95-95: Rename totoCSTString()Renaming this method from
toASTString()totoCSTString()clarifies its purpose within a compact syntax tree (CST) context. Ensure that references to the old method name have been updated to avoid breaking existing functionality.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Except.java (1)
8-8: Import path update looks good.Using
gov.nist.secauto.metaschema.core.metapath.IExpressionaligns with the broader refactoring effort to consolidate expression-related classes.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootDoubleSlashPath.java (2)
9-9: Imports look consistent with refactoring.The switch from the CST package to the core Metapath package for
IExpressionappears consistent with the broader refactoring. Ensure all other references toIExpressionin the codebase are similarly updated to avoid confusion or compilation issues.
45-45: Consider verifying stack-based support for debugging and error handling.While this refactored
evaluatemethod is concise and follows a straightforward approach, ensure you incorporate the new stack-based context approach desired by the PR’s objectives. For instance, pushing onto the execution stack before callingsearch(), then popping after the method completes, can reveal deeper insights into the evaluation process for debugging.Execute this shell script to locate any usage of a push/pop stack mechanism in the
cstormetapathpackages that might need updating to ensureRootDoubleSlashPathis also integrated with the debugging context:core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/ContextItem.java (1)
10-10: Confirm single source of interface definitions.You've moved the
IExpressionimport from thecstpackage to the rootmetapathpackage. Ensure no residual references to the oldIExpressionexist in other classes and that this import aligns with the desired interface location.✅ Verification successful
Let me search for more specific information about the IExpression interface.
Let me try a different approach to find the IExpression interface.
The import change is correct and properly aligned
Based on the verification results, the
IExpressioninterface is indeed correctly defined in the rootmetapathpackage atgov/nist/secauto/metaschema/core/metapath/IExpression.java. The search results show that all files are now importingIExpressionVisitorfrom thecstpackage, butIExpressionis being imported from the rootmetapathpackage, which is the correct location. There are no residual references to the oldIExpressionimport path in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential references to the old `IExpression` import path within the codebase. rg $'import gov.nist.secauto.metaschema.core.metapath.cst.IExpression'Length of output: 7633
Script:
#!/bin/bash # Check for the actual IExpression interface definition ast-grep --pattern 'interface IExpression'Length of output: 44
Script:
#!/bin/bash # Find files containing IExpression definition and usage rg -l "IExpression" --type java # Show content of IExpression.java if it exists fd "IExpression.java" -x cat {}Length of output: 11080
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/InstanceOf.java (2)
9-9: Approved import usage.Your import reference to
IExpressionis appropriately placed and aligns with the revised structure of the code.
97-97: Renaming method for clarity.Renaming
toASTString()totoCSTString()clarifies that it returns the compact syntax tree representation. This is consistent with the broader refactoring across the codebase.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/AbstractBasicArithmeticExpression.java (2)
9-9: Verify import path consistencyThis import references the
IExpressioninterface from thegov.nist.secauto.metaschema.core.metapathpackage instead of the oldcstpackage. Confirm that no references to the old import remain and that all relevant interfaces are consistently referenced from this new location.
[approve]Use this script to search for any residual references to the old package:
✅ Verification successful
Let me search for any direct references to
cst.IExpressionto ensure we haven't missed anything.
Let me check the actual content of the IExpression interface to understand the relationship between the old and new locations.
Import change is correct and consistent
The import change in
AbstractBasicArithmeticExpression.javais correct. TheIExpressioninterface has been moved from thecstpackage to the parentmetapathpackage, and all references to this interface are now properly using the new location. The oldcst.IExpressionreferences found in the codebase are actually references toIExpressionVisitorfrom thecstpackage, which is a different interface that is still correctly located in thecstpackage and is used by theIExpressioninterface for the visitor pattern implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for cst.IExpression references across the codebase rg 'cst\.IExpression'Length of output: 7736
Script:
#!/bin/bash # Search for direct references to IExpression in the cst package ast-grep --pattern 'cst.IExpression' # Also search for any class that might implement the old interface ast-grep --pattern 'implements $_ cst.IExpression' # Let's also check for any references to the old package structure rg -l "gov\.nist\.secauto\.metaschema\.core\.metapath\.cst\.IExpression"Length of output: 4295
Script:
#!/bin/bash # Check the content of IExpression.java to understand the interface structure cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/IExpression.java # Also check if there's any old IExpression interface in the cst package fd IExpression.java -t f core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cstLength of output: 3347
57-57: Assess external exposure of the newevaluate()methodChanging the access modifier from
publictoprotectedmay break code outside this class hierarchy that previously relied on the publicaccept()method. Ensure that no external classes or callers depend on the earlier public access level. If necessary, provide a suitable public method or verify that method calls have been updated accordingly.✅ Verification successful
Let me analyze the code further to verify the impact of the access modifier change.
Let me try a different search pattern to understand the class hierarchy and method usage.
Based on the search results, I can now provide a final response:
The access modifier change from
publictoprotectedis safe and appropriateThe
evaluate()method inAbstractBasicArithmeticExpressionis only used within its class hierarchy by concrete implementations:
- Addition.java
- Subtraction.java
- Multiplication.java
- Division.java
The search results show no external direct calls to this
evaluate()method. The method is an internal implementation detail used by arithmetic operation classes that extendAbstractBasicArithmeticExpression. The change toprotectedactually improves encapsulation by properly restricting access to this implementation method within its inheritance hierarchy.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for direct calls to the old accept method in the repo. rg '\.accept\('Length of output: 20509
Script:
#!/bin/bash # Search for the class hierarchy and usage of AbstractBasicArithmeticExpression ast-grep --pattern 'class $_ extends AbstractBasicArithmeticExpression' # Search for the evaluate method in the class ast-grep --pattern 'protected ISequence<? extends IAnyAtomicItem> evaluate'Length of output: 151
Script:
#!/bin/bash # Search for the class and its evaluate method implementation rg -A 5 "class AbstractBasicArithmeticExpression" # Search for direct references to evaluate method rg "\.evaluate\(" # Search for files that might extend this class fd -e java | xargs grep -l "AbstractBasicArithmeticExpression"Length of output: 11976
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/EmptySequence.java (1)
9-9: No issues with the new import.The addition of
import gov.nist.secauto.metaschema.core.metapath.IExpression;aligns well with the usage ingetChildren(). No further changes are necessary here.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/AbstractCastingExpression.java (2)
8-8: The newIExpressionimport is consistent with the refactoring.This import aligns with the updated package structure. However, please confirm that all references to
IExpressionin related classes or tests have also been updated to avoid lingering import references to the old path.✅ Verification successful
Based on the search results, I can see that the
IExpressionimport is being used consistently across the codebase, with all imports referencing the same pathgov.nist.secauto.metaschema.core.metapath.IExpression. There are no references to any old or different path forIExpression, which confirms that the import is correctly aligned with the codebase's package structure.The import is correctly aligned with the codebase's package structure.
All references to
IExpressionin the codebase consistently use the same import pathgov.nist.secauto.metaschema.core.metapath.IExpression, indicating that the import statement is properly maintained across all dependent files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any references to the old `IExpression` import path in the entire repo rg -A 3 "import .*IExpression"Length of output: 60241
110-110: Ensure references to the oldtoASTString()are fully removed or updated totoCSTString().The method name change appears to align with the broader rename from AST to CST. Still, verify that no leftover references to
toASTString()remain in the codebase or documentation.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/PredicateExpression.java (2)
91-91: Check visibility change impact and naming consistency.Changing the method from public to protected may limit reusability if the method is accessed outside this class or package. Moreover, since other classes still appear to use
.accept(), you might consider whether shifting to.evaluate()aligns with the broader design.
9-9: Confirm that the importedIExpressionis fully aligned with the new refactoring.The import of
IExpressionfrom this package is consistent with the broader refactoring, but please ensure that all references toIExpressionin the codebase are still valid following the deprecation or removal of any old imports from other packages.Please run this script to check for any outdated references to
IExpression:✅ Verification successful
Based on the search results, I can see that the
IExpressioninterface is consistently imported from the same packagegov.nist.secauto.metaschema.core.metapathacross all files. Let me verify the actual interface definition and its usage.
Let me try a different approach to verify the interface definition and its usage.
The imported
IExpressioninterface is correctly aligned with the codebaseBased on the verification results, the import of
IExpressionfromgov.nist.secauto.metaschema.core.metapathis fully aligned with the codebase. The interface is defined in the same package and is consistently imported across all files. There are no alternative or deprecated versions ofIExpressionin the codebase, and the interface serves as the common base for all Metapath expression nodes as documented in its Javadoc.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references to removed or legacy IExpression imports rg -A 5 $'import .+IExpression'Length of output: 67766
Script:
#!/bin/bash # Check the IExpression interface definition ast-grep --pattern 'interface IExpression { $$$ }'Length of output: 54
Script:
#!/bin/bash # Check the content of IExpression.java cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/IExpression.java # Find any other potential IExpression interfaces in the codebase find . -type f -name "IExpression.java"Length of output: 3375
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/NamedFunctionReference.java (3)
9-9: Consistent adoption ofIExpressionfor CST nodes.
This import aligns with the class's usage ofIExpressionin thegetChildren()method. It ensures clarity and consistency with the larger Metapath architecture refactor.
74-74: Renaming method totoCSTString()improves clarity.
Changing fromtoASTString()totoCSTString()matches the newly adopted nomenclature for describing a "compact syntax tree." This uniformly reflects the shift in terminology throughout the codebase.
84-84: Handle possible null return fromdynamicContext.getFunction(...).
While the renamed and re-visibility-adjustedevaluatemethod is clear, ensure thatgetFunction(...)cannot returnnull. Otherwise,ISequence.of(...)may cause runtime issues. Consider adding a null-check or throwing an appropriate exception if the function is not found.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Modulo.java (2)
9-9: Import refactoring looks good.Switching to import
IExpressionfrom the reorganized package structure is consistent with the broader refactoring effort to streamline expression-related interfaces.
58-58: Confirm reduced visibility impact.Changing the method from public to protected may break references if external code relied on the old public signature. Verify that no external classes call this method.
✅ Verification successful
Let me search for more specific information about the
evaluatemethod in the Modulo class.
Let me check the inheritance hierarchy and usage pattern of the Modulo class.
The protected visibility change is safe to proceed with.
The
evaluatemethod inModuloclass is an implementation detail that is only used internally through the publicacceptmethod. This is evident from:
- The class extends
AbstractArithmeticExpressionand overrides the protectedevaluatemethod- External code interacts with the class through the public
acceptmethod which follows the visitor pattern- The
evaluatemethod is only called from within the class hierarchy through the template method pattern- No direct calls to
evaluatewere found in the codebase🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find external references to the old "accept" method signature # that might break due to it becoming protected. rg -A 5 "accept\\s*\\(.*DynamicContext"Length of output: 7573
Script:
#!/bin/bash # Search for references to the evaluate method in Modulo class ast-grep --pattern 'evaluate($_, $_) { $$$ }' # Also check for any direct usage of Modulo class rg -l "Modulo"Length of output: 496
Script:
#!/bin/bash # Check the Modulo class implementation and its parent class cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Modulo.java # Check how Modulo is used in visitor interfaces cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/IExpressionVisitor.javaLength of output: 24180
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ArraySequenceConstructor.java (1)
9-9: Confirm the import usage.While the import for
IExpressionis valid, please ensure that this interface is the correct one needed for constructing array sequences, especially given the broader refactoring efforts aroundaccept→evaluatein the codebase.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/VariableReference.java (3)
60-60: Method rename fromtoASTStringtotoCSTStringis consistent with naming conventions.This updated name aligns with the rest of the refactor across the codebase. It will improve clarity by referring to the correct abstraction level (CST vs AST).
9-9: Consolidate imports if needed.The addition of the
IExpressionimport is appropriate given we now return a list ofIExpressioningetChildren(). However, ensure that there are no unused imports left behind (e.g., ifIItemwas removed, verify it isn't still imported anywhere else in the codebase).
[approve]✅ Verification successful
Let me verify if there are any unused imports related to
IItemin the file under review.
Let me try a different approach to verify the usage of imports in the file.
No unused imports found - all imports are being used in the code.
Looking at the file content:
IExpressionis used in:
- The
getChildren()method's return type:List<? extends IExpression>- The method parameter in
accept():IExpressionVisitor<RESULT, CONTEXT>All other imports are also being used:
DynamicContextin theevaluate()methodISequencein multiple method signaturesIEnhancedQNamefor thenamefieldCollectionUtilingetChildren()ListingetChildren()NonNullannotations🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all references to IItem in the codebase, to ensure consistency. rg "IItem" -A 5Length of output: 100796
Script:
#!/bin/bash # Check if IItem is used in the file beyond the import statement ast-grep --pattern $'import gov.nist.secauto.metaschema.core.metapath.IExpression; $$$ class VariableReference { $$$ IExpression $$$ $$$ }' core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/VariableReference.javaLength of output: 234
Script:
#!/bin/bash # Show the content of the file to verify imports and their usage cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/VariableReference.javaLength of output: 1924
70-70: Visibility and naming change forevaluatelooks consistent.Previously
acceptwas public, now it’s protected with a more descriptive name:evaluate(). This is a good practice for encapsulating behavior while still allowing for extension if needed. Verify that existing references toaccepthave been updated accordingly.✅ Verification successful
Let me search for the old
acceptmethod implementation to verify the change.
Let me check for any references to this class and its methods.
Let me check the git history to see the changes made to this file.
Let me try a different approach to check the inheritance hierarchy and related classes.
Let me check the AbstractExpression class to understand the inheritance and method visibility.
Let me check the IExpression interface implementation to understand the method visibility requirements.
Based on the code analysis, I can now provide a final response:
The visibility and naming changes are consistent with the codebase design
The change from public
acceptto protectedevaluateinVariableReference.javais consistent with the codebase architecture because:
- The
evaluatemethod is an implementation detail that should be protected, as it's part of the internal evaluation mechanism- The public
acceptmethod is still maintained in the class for the visitor pattern implementation- The class properly extends
AbstractExpressionand follows its design patterns- No other parts of the codebase directly called the old public method, as all interactions are properly done through the visitor pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify references to the old `accept` method in the codebase. rg -A 5 $'\\.accept\\('Length of output: 65558
Script:
#!/bin/bash # Search for the old accept method implementation in VariableReference.java rg -A 10 "public.*accept.*" core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/VariableReference.javaLength of output: 445
Script:
#!/bin/bash # Search for references to VariableReference class and its usage rg -A 5 "VariableReference" --type javaLength of output: 6066
Script:
#!/bin/bash # Check git history for VariableReference.java git log -p core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/VariableReference.javaLength of output: 0
Script:
#!/bin/bash # Check AbstractExpression class which VariableReference extends rg -A 10 "class AbstractExpression" --type java # Also check for any test files related to VariableReference fd -e java -E 'test' -x rg -l "VariableReference"Length of output: 3200
Script:
#!/bin/bash # Check the AbstractExpression class implementation rg -A 20 "abstract class AbstractExpression" core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractExpression.java # Also check for the IExpression interface rg -A 10 "interface IExpression" --type javaLength of output: 2937
Script:
#!/bin/bash # Get more context from IExpression interface rg -A 30 "interface IExpression" core/src/main/java/gov/nist/secauto/metaschema/core/metapath/IExpression.java # Also check for any evaluate method declarations rg -A 5 "evaluate.*\(" core/src/main/java/gov/nist/secauto/metaschema/core/metapath/IExpression.javaLength of output: 1027
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Quantified.java (1)
9-9: Consistent usage ofIExpressionimport.
The new import forIExpressionis aligned with the rest of the code's references, and there appear to be no conflicts.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/AbstractLiteralExpression.java (6)
8-8: Good import usage.
ImportingDynamicContextis necessary for the newly introducedevaluatemethod.
10-10: Sensible addition of imports.
Imports forISequenceandIAnyAtomicItemare appropriate for the updated type parameter usage and returned sequence.
23-27: Nice simplification of generic parameters.
Reducing to a singleRESULT_TYPEimproves clarity and reduces complexity.
37-37: Constructor aligns well with the updated type.
Switching the constructor to acceptRESULT_TYPEdirectly is consistent with the simplified generics.
43-43: Getter refactor is consistent.
UsingRESULT_TYPEforgetValue()maintains type safety and clarity.
54-55: CST naming convention is clear.
RenamingtoASTString()totoCSTString()improves consistency with the broader codebase. The usage ofvalue.asString()is straightforward.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/IntegerDivision.java (2)
9-9: Use of updated import path looks good
This change to importIExpressionfrom its non-CST package aligns well with the broader refactoring. No issues here.
57-57: Confirm naming consistency and consider zero-divisor handling
Renaming the method toevaluatewhile retaining anotheraccept()method in this class is fine if those methods serve different use cases (visitor vs. direct evaluation). However, if unifying nomenclature is a goal, consider applying the same convention (e.g.,evaluate) for the visitor-based logic as well. Additionally, ensure that zero-divisor scenarios are handled according to the required XPath 3.1 or internal specification.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractNAryExpression.java (1)
8-9: Good use of theIExpressioninterfaceBy relying on
IExpressionas the type forchildren, the code clearly communicates that each sub-expression follows the same contract. This is a clean approach for representing n-ary expressions while ensuring type consistency.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractUnaryExpression.java (1)
8-9: Ensure single-child handling remains robustSwitching to
IExpressionfor the unary expression field improves consistency across all expression classes. The use ofList.of(expr)ingetChildren()is compact and thread-safe by design since the returned list is immutable.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractRootPathExpression.java (1)
8-8: Consistent import ofIExpressionThe updated import aligns this root path expression class with the rest of the package’s expression model. Returning a single-element list in
getChildren()allows uniform traversal of the expression tree.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractBinaryExpression.java (1)
8-9: Use of generics withIExpressionenhances type safetyDefining
AbstractBinaryExpression<L extends IExpression, R extends IExpression>makes the binary sub-expression contract explicit. The new import ensures that the left and right expressions remain consistent with the rest of the metapath framework.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RelativeDoubleSlashPath.java (2)
9-9: Consistent package refactoring forIExpressionimport
This import update aligns with the reorganized package structure, ensuring consistency across the codebase.
51-51: Method rename and visibility change
Renamingaccepttoevaluateand making itprotectedclarifies the method’s intent and scope. The implementation correctly usessearch(getRight(), ...)for further evaluation.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/AbstractFilterExpression.java (3)
6-6: Package reorganization noted
Switching fromcst.logictocst.itemsindicates a structural shift in how filter expressions are grouped. This matches the project's reorganized layout.
9-9: Import ofIExpression
AddingIExpressionhelps align with the new architecture for expression classes. This is consistent with the rest of the refactoring.
55-55: Evaluation method updated
Renamingaccepttoevaluateand reducing visibility toprotectedclarifies the intention to process the expression internally. Logic remains clear and consistent, leveragingapplyFilterTo(...).core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/For.java (3)
9-9: Import alignment
IncludingIExpressionfrom the updated package is part of the overall refactor. No issues noted.
84-84:evaluatemethod ensures sub-context usage
Changingaccepttoevaluateclarifies responsibility and properly encapsulates for-loop evaluation. The usage of a sub-context to bind each item is appropriate.
100-100: Renamed method totoCSTString
The updated naming is consistent with the shift from AST to CST terminology. This change improves clarity for debugging.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/DynamicFunctionCall.java (2)
9-9: Package-consistent import addition
The import now reflects the updated location ofIExpression. This is a straightforward, necessary change.
73-73: Method rename fromaccepttoevaluate
The new name andprotectedscope better convey that the function call is being evaluated rather than just visited. The logic for resolving and executing the function remains correct.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/IExpression.java (2)
6-6: Streamlined package structure alignment
Good job moving the interface to a more general package. This helps reduce complexity and clarifies the scope of this interface.
8-8: Import ofIExpressionVisitor
The added import matches the newly referenced visitor interface. Ensure that all references to this visitor are updated correctly in dependent classes.core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/CSTLogicalExpressionsTest.java (1)
12-12: Updated import location
Switching to the newIExpressionpackage is a sensible move that keeps the code aligned with the recent package refactoring inIExpression.java.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AnonymousFunctionCall.java (2)
9-9: Leverage the relocatedIExpression
The added import clarifies references to theIExpressioninterface. Make sure that the rest of thecstpackage references remain current.
81-81: 🛠️ Refactor suggestionTransition from
accepttoevaluate
Changing the method to protected and renaming it toevaluateclarifies its intent and limits external exposure. Verify that no external clients still rely on the old signature.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/ICstExpressionFactory.java (1)
8-8: Consistent usage ofIExpression
Adopting the new package path aligns with the rest of this PR’s refactoring. This improves consistency across the CST factories and related classes.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/MetapathExpression.java (3)
165-170: Renamed documentation from AST to CST is consistent and adds clarity.
The newly introducedgetCSTNode()method and its documentation clearly align with the CST-based approach.
181-181: Appropriate use ofCSTPrinterintoString().
Switching fromgetASTNode()togetCSTNode()here is consistent with the overall CST refactoring and ensures correct string representation.
190-190: Evaluation logic remains consistent with CST.
UsinggetCSTNode().accept(...)is a straightforward adaptation from the AST-based approach. No immediate issues found.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/DynamicContext.java (11)
20-33: New imports for Deque and related collections.
These imports correctly support stack-based operations. No issues found.
43-44: Documentation update for the dynamic context.
Minor doc clarifications. Looks good.
90-91: NewexecutionStackfield added.
ThisDeque<IExpression>is central to stack-based tracking of expressions. Properly declared as@NonNull.
113-115: Doc changes for subContext method.
The explanation of creating a new sub-context is still valid; no issues found.
155-155: Clarified documentation for document availability.
Retains effectively the same logic.
170-170: Document loader doc improvement.
Matches usage in this class.
194-198: Refined doc for deterministically cached function results.
No concerns with the new text.
207-210: Documentation forcacheResult().
Correctly clarifies the property-based caching approach.
Line range hint
220-234: Improved Javadoc coverage for predicate evaluation toggles.
The doc changes better explain disabling/enabling predicate evaluation.
321-331: AddedpopExecutionStack().
Ensures LIFO matching with the expected expression. The assertion is a good safeguard although it will be omitted in production if assertions are disabled.
332-341: AddedgetExecutionStack().
Returns an unmodifiable copy of the stack content, preserving immutability. This approach is correct for read-only inspection.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/CSTPrinter.java (2)
8-8: ImportingIExpressionto support CST usage.
No concerns about the added import.
129-129: RefactoredappendNodeto callexpr.toCSTString().
Consistent with the CST approach. No issues identified.core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/BuildCstVisitorTest.java (1)
26-26: AddedIExpressionimport in tests.
Provides more precise test references. No further refinements needed.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/BuildCSTVisitor.java (1)
8-8: Confirmed usage of addedIExpressionimport.
This updated import is required since the interface is referenced extensively throughout the class. No issues found.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/SimpleMap.java (2)
9-9: Confirm necessity of theIExpressionimport here.
Even though the import statement appears redundant given the same package references, it does look correct to ensure clarity and usage in this file.
45-45: Verify no external dependencies on the oldpublic accept(...).
Changing method visibility and renaming fromaccepttoevaluateacross code can break external code if any references toSimpleMap.accept(...)remain.Below is a script to search for any direct calls to
SimpleMap.accept(...)that might remain:✅ Verification successful
The previous search might have been too specific. Let's broaden our search to understand the usage of SimpleMap class and any accept method calls in the codebase. This will help us verify if there are any remaining dependencies on the old public accept method.
Let's verify if there was an old
acceptmethod in SimpleMap and check its usage in tests, as well as look at the interface contract to understand if this is a breaking change.
Let's examine the SimpleMap class implementation and its interfaces to understand the method visibility change and potential impact.
No breaking changes from renaming
accepttoevaluateLooking at the code, the change from
accepttoevaluateis safe because:
- The
evaluatemethod is a protected implementation detail, not part of the public API- The public visitor pattern method
accept(IExpressionVisitor<RESULT, CONTEXT>, CONTEXT)is still present and unchanged- The class properly implements the
IExpressionVisitorinterface contractThe change only affects the internal implementation method while preserving the public interface contract, so there are no external dependencies to worry about.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # This script searches the entire repository for code that calls SimpleMap.accept(...) rg 'SimpleMap\s*\([^)]*\)\.accept\s*\(' -A 5Length of output: 46
Script:
#!/bin/bash # Let's try a broader search for any references to SimpleMap class and its usage rg "SimpleMap" -l # Also search for any accept method calls in the codebase rg "\.accept\(" -A 2 # Let's also check if there are any test files that might be using SimpleMap fd "Test.*\.java" --exec grep -l "SimpleMap" {}Length of output: 49053
Script:
#!/bin/bash # Let's check the implementation of SimpleMap class and its accept method ast-grep --pattern $'class SimpleMap { $$$ accept($_, $_) { $$$ } $$$ }' # Also check for any references to SimpleMap.accept in test files rg -A 3 "SimpleMap.*\.accept" core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/BuildCstVisitorTest.java # Let's also check the interface that SimpleMap implements ast-grep --pattern $'interface $_ { $$$ accept($_, $_); $$$ }'Length of output: 277
Script:
#!/bin/bash # Let's check the interfaces implemented by SimpleMap rg "class SimpleMap\s+(\w+\s+)*implements" -A 2 # Check for any references to accept method in interfaces rg "interface.*\{" -A 10 core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/IExpressionVisitor.java # Check the SimpleMap class file directly cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/SimpleMap.javaLength of output: 2553
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractExpression.java (2)
44-51: Ensure proper error handling and concurrency.The
acceptmethod correctly uses a stack-based approach withtry/finallyto ensure expressions are always popped. This design provides clear traceability for debugging. Confirm thatDynamicContextis not shared across threads in ways that might lead to race conditions or concurrency issues.
8-12: Verify imports and usage consistency.The newly introduced imports align with the relocated
IExpressioninterface. Please ensure all references to the old package structure are updated throughout the codebase.✅ Verification successful
Let me generate additional scripts to verify the package structure changes more thoroughly.
The previous results showed references to
IExpressionVisitorstill in thecstpackage, but we need to verify the complete picture ofIExpressionusage and imports to ensure consistency with the new package structure.
All imports are correctly using the new package structure
Based on the verification results, all files are correctly importing
IExpressionfrom the new package locationgov.nist.secauto.metaschema.core.metapath.IExpression. The only references to thecstpackage are for theIExpressionVisitorinterface, which appears to be intentionally located in thecstpackage.The search results show:
- No references to the old package structure for
IExpression- All imports of
IExpressionare from the new package location- References to
cstpackage are only forIExpressionVisitor, which is correctly maintained in that package🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify references to old package paths for IExpression, IItem, and ISequence. # Expect: No references to old package paths remain. rg "gov\.nist\.secauto\.metaschema\.core\.metapath\.cst\.IExpression" rg "gov\.nist\.secauto\.metaschema\.core\.metapath\.cst\.item\.IItem" rg "gov\.nist\.secauto\.metaschema\.core\.metapath\.cst\.item\.ISequence"Length of output: 7932
Script:
#!/bin/bash # Check for any references to IExpression in import statements rg "^import.*IExpression" --type java # Check for any references to old package structure in javadoc or comments rg "@see.*metapath\.cst" --type javaLength of output: 18939
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/PostfixLookup.java (1)
9-9: Verify references toIExpressionin the oldcstpackage.
Please confirm that all references toIExpressionare consistently updated to the new package.✅ Verification successful
Let me search for any remaining references to the old package path.
Let me check the actual content of these files to verify the imports.
All references to
IExpressionare correctly using the new package pathThe verification shows that all files are using the correct import path
gov.nist.secauto.metaschema.core.metapath.IExpressionand not the old CST package path. The change is consistent across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Locate any remaining references to the old IExpression import in the codebase. rg -A 3 "import gov.nist.secauto.metaschema.core.metapath.cst.IExpression"Length of output: 29050
Script:
#!/bin/bash # Search for any references to IExpression in the old package path rg -l "gov\.nist\.secauto\.metaschema\.core\.metapath\.cst\.IExpression"Length of output: 4205
Script:
#!/bin/bash # Check the actual imports in files that reference IExpression for file in $(rg -l "gov\.nist\.secauto\.metaschema\.core\.metapath\.cst\.IExpression"); do echo "=== $file ===" rg "^import.*IExpression" "$file" doneLength of output: 24976
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootSlashOnlyPath.java (1)
Line range hint
71-79: Confirm exception strategy for non-document nodes.By immediately throwing an exception when the root node is not a document node, any partial hierarchy or non-document-based node trees will be disallowed. Confirm if this strict approach is desired, or if a more flexible handling is needed.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/GeneralComparison.java
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/InstanceOf.java
Show resolved
Hide resolved
.../main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ArraySequenceConstructor.java
Show resolved
Hide resolved
…s the expressions entered and exited during Metapath evaluation. This will be useful context for debugging and error reporting.
david-waltermire
force-pushed
the
feature/metapath-track-expression-context
branch
from
9281a76 to
8be41b0
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/Step.java (1)
Line range hint
110-129: Update step execution to use evaluateThe last line still uses
acceptinstead ofevaluate, which is inconsistent with the refactoring:return step == null ? axisResult : step.accept(dynamicContext, axisResult);This should be updated to use
evaluatefor consistency and proper execution stack tracking.- return step == null ? axisResult : step.accept(dynamicContext, axisResult); + return step == null ? axisResult : step.evaluate(dynamicContext, axisResult);core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/FunctionCallAccessor.java (2)
Line range hint
79-89: Updateacceptcall toevaluatefor consistency.The method has been renamed from
accepttoevaluate, but it still callsaccepton the base expression. This should be updated for consistency with the new naming convention.- ISequence<?> target = getBase().accept(dynamicContext, focus); + ISequence<?> target = getBase().evaluate(dynamicContext, focus);
Line range hint
79-89: Add execution stack tracking.As per the PR objectives, this method should track expression entry/exit in the DynamicContext's execution stack for debugging and error reporting purposes.
Consider wrapping the evaluation in try-finally blocks to ensure proper stack management:
protected ISequence<?> evaluate(DynamicContext dynamicContext, ISequence<?> focus) { + dynamicContext.pushExpression(this); + try { ISequence<?> target = getBase().evaluate(dynamicContext, focus); IItem collection = target.getFirstItem(true); if (!(collection instanceof IFunction)) { throw new InvalidTypeMetapathException(collection, "The base expression did not evaluate to a function."); } return ((IFunction) collection).execute(ObjectUtils.notNull(getArguments().stream() .map(expr -> expr.evaluate(dynamicContext, focus)) .collect(Collectors.toUnmodifiableList())), dynamicContext, focus); + } finally { + dynamicContext.popExpression(); + } }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/AbstractFilterExpression.java (1)
Line range hint
55-61: Method calls are inconsistent with new namingWhile the rename from
accepttoevaluatebetter describes the method's purpose, there's an inconsistency in the implementation. The method usesacceptcalls internally while being renamed toevaluate.Apply this diff to maintain consistency:
protected ISequence<?> evaluate( @NonNull DynamicContext dynamicContext, @NonNull ISequence<?> focus) { - ISequence<?> left = getLeft().accept(dynamicContext, focus); - ISequence<?> right = getRight().accept(dynamicContext, focus); + ISequence<?> left = getLeft().evaluate(dynamicContext, focus); + ISequence<?> right = getRight().evaluate(dynamicContext, focus); return applyFilterTo(left, right); }
🧹 Nitpick comments (15)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/Castable.java (1)
Line range hint
54-63: Method signature changes enhance encapsulation and type safety.The changes from:
publictoprotectedvisibilityaccepttoevaluatenaming- More specific return type
ISequence<IBooleanItem>align well with the PR's objectives. The implementation correctly handles casting attempts and error cases, providing clear boolean results for debugging.
Consider documenting these architectural decisions in the class-level JavaDoc to explain:
- Why
evaluateis protected (encapsulation strategy)- The relationship between
evaluateandacceptmethods- The error handling strategy for casting failures
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/EmptySequence.java (1)
63-63: Consider using RESULT_TYPE for better type safetyThe visibility change to protected and rename to
evaluatelook good and align with the refactoring goals. However, consider usingISequence<RESULT_TYPE>as the return type instead ofISequence<?>to maintain type safety, as RESULT_TYPE is already defined at the class level.- protected ISequence<?> evaluate(DynamicContext dynamicContext, ISequence<?> focus) { + protected ISequence<RESULT_TYPE> evaluate(DynamicContext dynamicContext, ISequence<?> focus) {core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/ValueComparison.java (1)
53-57: Consider leveraging the new execution stack for enhanced debugging.While the method rename and visibility change are good improvements, consider enhancing the implementation to utilize the new execution stack feature for better debugging and error reporting. This would align with the PR's objective of providing useful context during Metapath evaluation.
Example enhancement:
protected ISequence<? extends IBooleanItem> evaluate(DynamicContext dynamicContext, ISequence<?> focus) { + dynamicContext.pushExpression(this); // Track expression entry + try { IAnyAtomicItem left = ISequence.of(getLeft().accept(dynamicContext, focus).atomize()).getFirstItem(false); IAnyAtomicItem right = ISequence.of(getRight().accept(dynamicContext, focus).atomize()).getFirstItem(false); return resultOrEmpty(left, right); + } finally { + dynamicContext.popExpression(); // Ensure expression is popped even if evaluation fails + } }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractStepExpression.java (1)
66-69: LGTM! Method signature changes improve encapsulation and clarity.The changes from
public accept()toprotected evaluate()are well-aligned with:
- Encapsulation principles through protected access
- More accurate method naming that reflects its purpose
- The PR's goal of tracking expression evaluation
Consider adding a
@sinceJavadoc tag to document this API change, as it might affect downstream implementations:+ /** + * @since <version> Renamed from accept() to evaluate() and changed access to protected + */ protected ISequence<? extends RESULT_TYPE> evaluate(core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/DecimalLiteral.java (1)
33-33: Consider improving the constructor's parameter documentation.While the implementation change is good, the
@param valuedocumentation could be more specific about the expectedBigDecimaltype.Update the documentation to:
/** * Construct a new expression that always returns the same decimal value. * * @param text * the parsed text of the expression * @param value - * the literal value + * the literal value as a BigDecimal, which will be converted to an IDecimalItem */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/Step.java (1)
Line range hint
131-135: Consider updating method documentationThe rename from
toASTStringtotoCSTStringis appropriate, reflecting the shift from AST to CST terminology.Consider adding Javadoc to explain the CST (Concrete Syntax Tree) string representation format.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/NamedFunctionReference.java (1)
84-87: Consider enhancing error handling and stack tracking.While the method signature changes are appropriate, consider these improvements to support the PR's debugging and error reporting objectives:
- Add execution stack tracking before and after evaluation
- Enhance error handling to include stack context when function lookup fails
Example enhancement:
protected ISequence<?> evaluate(DynamicContext dynamicContext, ISequence<?> focus) { + dynamicContext.getExecutionStack().push(this); + try { IFunction function = dynamicContext.getFunction(name, arity); return ISequence.of(function); + } catch (Exception e) { + throw new MetapathException(String.format("Failed to evaluate function %s with arity %d", name, arity), e); + } finally { + dynamicContext.getExecutionStack().pop(); + } }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractUnaryExpression.java (2)
Line range hint
33-45: Remove unnecessary @SuppressWarnings("null").The
@SuppressWarnings("null")annotation ongetChildren()appears unnecessary since:
List.of()returns a non-null List- The
exprfield is already marked@NonNull- The wildcard return type doesn't affect nullability
- @SuppressWarnings("null") @Override public List<? extends IExpression> getChildren() { return List.of(expr); }
Line range hint
16-45: Well-designed for integration with execution stack.The immutable design and clear expression hierarchy make this class well-suited for the new execution stack feature:
- Immutability ensures thread-safety during stack operations
- Clear parent-child relationship enables proper stack traversal
- Robust null-safety supports reliable debugging and error reporting
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootSlashOnlyPath.java (2)
63-63: Add Javadoc for the evaluate method.Please add documentation explaining the method's purpose, parameters, return value, and potential exceptions.
Line range hint
63-77: Consider breaking down the stream operations for better readability.While the implementation is correct, the nested stream operations and method chaining make it harder to follow. Consider extracting the document node validation into a separate method.
Here's a suggested refactoring:
protected ISequence<? extends INodeItem> evaluate(DynamicContext dynamicContext, ISequence<?> focus) { + return ISequence.of(ObjectUtils.notNull(focus.stream() + .map(ItemUtils::checkItemIsNodeItemForStep) + .map(this::findAndValidateDocumentRoot))); + } + + private INodeItem findAndValidateDocumentRoot(INodeItem item) { + INodeItem root = Axis.ANCESTOR_OR_SELF.execute(ObjectUtils.notNull(item)) + .findFirst() + .orElseThrow(() -> new DynamicMetapathException( + DynamicMetapathException.TREAT_DOES_NOT_MATCH_TYPE, + "Root node not found")); + + if (!(root instanceof IDocumentNodeItem)) { + throw new DynamicMetapathException( + DynamicMetapathException.TREAT_DOES_NOT_MATCH_TYPE, + "The head of the tree is not a document node."); + } + return root; - return ISequence.of(ObjectUtils.notNull(focus.stream() - .map(ItemUtils::checkItemIsNodeItemForStep) - .map(item -> Axis.ANCESTOR_OR_SELF.execute(ObjectUtils.notNull(item)) - .findFirst() - .orElseThrow(() -> new DynamicMetapathException(DynamicMetapathException.TREAT_DOES_NOT_MATCH_TYPE, - "Root node not found"))) - .peek(item -> { - if (!(item instanceof IDocumentNodeItem)) { - throw new DynamicMetapathException(DynamicMetapathException.TREAT_DOES_NOT_MATCH_TYPE, - "The head of the tree is not a document node."); - } - }))); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/DynamicContext.java (2)
311-319: Consider adding validation in pushExecutionStack.While the implementation is correct, consider adding validation to ensure the expression is not null at runtime, rather than relying solely on the @nonnull annotation which might be ignored at runtime.
public void pushExecutionStack(@NonNull IExpression expression) { + Objects.requireNonNull(expression, "expression must not be null"); this.sharedState.executionStack.push(expression); }
Line range hint
90-340: Consider enhancing error reporting with execution stack information.The execution stack implementation could be further enhanced to provide rich debugging information. Consider:
- Adding a method to format the execution stack for error messages
- Integrating the stack information into existing exception handling
Example enhancement:
@NonNull public String formatExecutionStackTrace() { return getExecutionStack().stream() .map(IExpression::toString) .collect(Collectors.joining(" -> ")); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/VariableReference.java (1)
60-62: Consider adding Javadoc for the toCSTString method.The rename from
toASTStringtotoCSTStringbetter reflects the class's role in the Concrete Syntax Tree. Consider adding Javadoc to document the method's purpose and return format.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/KindNodeTest.java (1)
58-61: LGTM! Consider adding JavadocThe rename from
toASTString()totoCSTString()and visibility change to public is appropriate, reflecting the concrete syntax tree nature of this representation. This aligns well with the PR's goal of improved debugging support.Consider adding Javadoc to document the format and purpose of the CST string representation:
+ /** + * Returns a string representation of this node in the Concrete Syntax Tree. + * + * @return a string in the format "className[kind=typeSignature]" + */ public String toCSTString() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (85)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/DynamicContext.java(14 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/IExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/MetapathExpression.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractBinaryExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractCSTVisitorBase.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractExpressionVisitor.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractNAryExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractUnaryExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AnonymousFunctionCall.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/BuildCSTVisitor.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/CSTPrinter.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/DynamicFunctionCall.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/ExpressionUtils.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/For.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/FunctionCallAccessor.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/ICstExpressionFactory.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/Let.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/NamedFunctionReference.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/StaticFunctionCall.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/VariableReference.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/AbstractFilterExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/AbstractLiteralExpression.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ArraySequenceConstructor.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ArraySquareConstructor.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/DecimalLiteral.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/EmptySequence.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Except.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ILiteralExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/IntegerLiteral.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Intersect.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/MapConstructor.java(4 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/PostfixLookup.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Quantified.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Range.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/SequenceExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/SimpleMap.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/StringConcat.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/StringLiteral.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/UnaryLookup.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Union.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractComparison.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/And.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/GeneralComparison.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/IBooleanLogicExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/If.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/Or.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/PredicateExpression.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/ValueComparison.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/AbstractArithmeticExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/AbstractBasicArithmeticExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Addition.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Division.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/IntegerDivision.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Modulo.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Multiplication.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Negate.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Subtraction.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractRelativePathExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractRootPathExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractSearchPathExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractStepExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/ContextItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/INodeTestExpression.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/KindNodeTest.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/NameNodeTest.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RelativeDoubleSlashPath.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RelativeSlashPath.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootDoubleSlashPath.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootSlashOnlyPath.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootSlashPath.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/Step.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/WildcardNodeTest.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/AbstractCastingExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/Cast.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/Castable.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/InstanceOf.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/Treat.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/ComparisonFunctions.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/impl/AbstractKeySpecifier.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/ExpressionUtilsTest.java(0 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/BuildCstVisitorTest.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/CSTLogicalExpressionsTest.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/PredicateExpressionTest.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/ValueComparisonTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- core/src/test/java/gov/nist/secauto/metaschema/core/metapath/ExpressionUtilsTest.java
🚧 Files skipped from review as they are similar to previous changes (61)
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractExpressionVisitor.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractCSTVisitorBase.java
- core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/BuildCstVisitorTest.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Subtraction.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Intersect.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractNAryExpression.java
- core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/ValueComparisonTest.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/INodeTestExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/AbstractArithmeticExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractSearchPathExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/IBooleanLogicExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Addition.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Multiplication.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Except.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/SequenceExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/impl/AbstractKeySpecifier.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractRelativePathExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Division.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractBinaryExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/ExpressionUtils.java
- core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/CSTLogicalExpressionsTest.java
- core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/PredicateExpressionTest.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/CSTPrinter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/IExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractComparison.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/BuildCSTVisitor.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootDoubleSlashPath.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/AbstractCastingExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/StaticFunctionCall.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootSlashPath.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Quantified.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/NameNodeTest.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ArraySequenceConstructor.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/Or.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Negate.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/DynamicFunctionCall.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/For.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Range.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/UnaryLookup.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/SimpleMap.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/ContextItem.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/StringLiteral.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/StringConcat.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/If.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RelativeDoubleSlashPath.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RelativeSlashPath.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractRootPathExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/WildcardNodeTest.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/Cast.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/Treat.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/IntegerDivision.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/AbstractBasicArithmeticExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/PredicateExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Modulo.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/MetapathExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ILiteralExpression.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/ICstExpressionFactory.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ArraySquareConstructor.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AnonymousFunctionCall.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/MapConstructor.java
👮 Files not reviewed due to content moderation or server errors (2)
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/ComparisonFunctions.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/GeneralComparison.java
🧰 Additional context used
📓 Learnings (5)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/FunctionCallAccessor.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#266
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/FunctionCallAccessor.java:82-89
Timestamp: 2024-11-28T04:47:39.173Z
Learning: In `FunctionCallAccessor.java`, the `accept` method must continue executing after handling `AnonymousFunctionCall` to properly process other collection types. Adding an `else` block that bypasses the remaining code is incorrect.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractUnaryExpression.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#315
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractFilterExpression.java:56-56
Timestamp: 2024-12-31T18:45:39.097Z
Learning: "ISequence" in the codebase is an implementation of the List interface, allowing direct usage where a List is expected.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/AbstractFilterExpression.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#315
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractFilterExpression.java:56-56
Timestamp: 2024-12-31T18:45:39.097Z
Learning: "ISequence" in the codebase is an implementation of the List interface, allowing direct usage where a List is expected.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/AbstractLiteralExpression.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#315
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/AbstractFilterExpression.java:56-56
Timestamp: 2024-12-31T18:45:39.097Z
Learning: "ISequence" in the codebase is an implementation of the List interface, allowing direct usage where a List is expected.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/InstanceOf.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#336
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/InstanceOf.java:87-88
Timestamp: 2025-01-07T00:50:46.218Z
Learning: In the Metapath implementation, child expressions should be called using `accept()` rather than `evaluate()` to ensure proper management of the execution stack through push/pop operations. The `evaluate()` method is an internal implementation detail that performs the actual evaluation.
🔇 Additional comments (40)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/InstanceOf.java (3)
9-9: LGTM!The import consolidation to a more general package is appropriate.
87-88: LGTM! Implementation correctly uses accept() for child expressions.The implementation properly uses
accept()for child expression evaluation, which ensures proper management of the execution stack through push/pop operations, whileevaluate()handles the actual evaluation logic.
97-97: LGTM! Method rename reflects correct tree representation.The rename from
toASTStringtotoCSTStringaccurately reflects that this is working with a Concrete Syntax Tree (CST) rather than an Abstract Syntax Tree (AST).core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/type/Castable.java (1)
9-9: LGTM! Import change improves maintainability.Moving to a more general package for
IExpressionaligns with good architectural practices and the broader refactoring effort.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/EmptySequence.java (1)
9-9: LGTM! Import change aligns with refactoringThe consolidation of the
IExpressionimport aligns with the broader refactoring effort to improve organization.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/ValueComparison.java (1)
9-9: LGTM! Package restructuring improves organization.The move of
IExpressionto a more general package aligns with good architectural practices by consolidating core interfaces at the appropriate level.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/AbstractStepExpression.java (1)
9-9: LGTM! Package reorganization improves interface accessibility.The relocation of
IExpressionto a higher-level package improves its visibility and reusability across the codebase.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/DecimalLiteral.java (1)
22-22: LGTM! Good simplification of the type hierarchy.The removal of the
BigDecimaltype parameter streamlines the class hierarchy while maintaining type safety throughIDecimalItem.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/Step.java (1)
9-9: LGTM: Import consolidationThe import consolidation aligns with the broader refactoring goals.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/NamedFunctionReference.java (2)
9-9: LGTM! Import change aligns with framework reorganization.The import consolidation of
IExpressionto the core metapath package improves code organization.
74-76: LGTM! Method rename improves terminology consistency.The rename from
toASTStringtotoCSTStringbetter reflects the Compact Syntax Tree (CST) nature of this class and aligns with its package location.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/FunctionCallAccessor.java (1)
9-9: LGTM! Import change is appropriate.The added import for
IExpressionis necessary and properly consolidates the interface imports as part of the broader refactoring.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractUnaryExpression.java (2)
8-8: LGTM! Import change aligns with interface consolidation.The import statement change reflects the package restructuring of
IExpressioninterface, moving it to a more appropriate package level.
Line range hint
19-31: LGTM! Robust implementation with proper null-safety.The implementation follows best practices with:
- Proper encapsulation (private final field)
- Compile-time (@nonnull) and runtime (Objects.requireNonNull) null-safety checks
- Clear JavaDoc documentation
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/RootSlashOnlyPath.java (1)
10-10: LGTM! Good architectural improvement.Moving
IExpressionimport to the core package improves modularity by reducing coupling between CST-specific and general Metapath components.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/DynamicContext.java (3)
28-31: LGTM! Import changes are appropriate.The new imports for
ArrayDeque,ArrayList, andDequeare correctly added to support the execution stack functionality.
90-91: Consider thread-safety implications of the execution stack.While the field is correctly marked as final and initialized with ArrayDeque, consider whether concurrent access to the execution stack needs synchronization, especially since this is part of the SharedState class which might be accessed by multiple threads.
Consider either:
- Using a thread-safe deque implementation like
ConcurrentLinkedDeque- Documenting that the DynamicContext is not thread-safe and should be used in a single thread
332-340: LGTM! Defensive copy implementation.The getExecutionStack method correctly creates a defensive copy and returns an unmodifiable list, preventing modification of the internal state.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/PostfixLookup.java (2)
9-9: LGTM! Package restructuring looks good.The import statement change aligns with the broader refactoring effort to consolidate
IExpressioninterface imports.
69-69: Verify the impact of visibility change.The method signature changes look good and align with the broader refactoring effort. However, let's verify that all callers can still access this method through the new protected visibility.
✅ Verification successful
Protected visibility change is safe and aligned with the codebase design
The change to protected visibility is verified to be safe because:
- The method is only used through inheritance within the same package
- External interaction with PostfixLookup is properly handled through the visitor pattern
- No direct calls to the evaluate() method were found outside the inheritance hierarchy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all direct calls to evaluate() method on PostfixLookup instances # to ensure they are within the same package or subclasses # Search for PostfixLookup instantiations and method calls rg -l "PostfixLookup" | while read -r file; do if [[ "$file" =~ \.java$ ]]; then echo "=== $file ===" # Show the context of PostfixLookup usage rg -B 2 -A 2 "PostfixLookup" "$file" fi doneLength of output: 5657
Script:
#!/bin/bash # Search for direct calls to evaluate() method and check inheritance echo "=== Searching for evaluate() method calls ===" rg -p "\.evaluate\(" -B 3 -A 1 echo -e "\n=== Checking PostfixLookup inheritance ===" ast-grep --pattern 'class PostfixLookup extends $_' echo -e "\n=== Finding classes in the same package ===" fd -e java . "core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/"Length of output: 21851
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/AbstractLiteralExpression.java (5)
8-10: LGTM! Good design simplification.The removal of the redundant type parameter
VALUEand direct use ofRESULT_TYPEimproves the class design by reducing complexity while maintaining type safety. The new imports appropriately support the evaluation functionality.Also applies to: 23-25
27-27: LGTM! Consistent field and constructor changes.The field and constructor changes align well with the type parameter simplification while maintaining proper null-safety through
@NonNullannotations.Also applies to: 37-40
43-45: LGTM! Consistent method signature update.The
getValue()method signature change aligns with the type parameter simplification.
47-50: LGTM! Efficient evaluation implementation.The new
evaluatemethod provides a clean and efficient implementation usingISequence.of()for singleton sequence creation, which is an optimal approach for literal expressions.
54-56: Verify the naming convention change across the codebase.The method has been renamed from
toASTStringtotoCSTString, suggesting a shift from Abstract Syntax Tree to Concrete Syntax Tree terminology. Also, the use ofasString()instead oftoString()is more appropriate for domain objects.Let's verify this naming convention change is consistent across the codebase:
✅ Verification successful
Naming convention change is consistent across the codebase
The change from
toASTStringtotoCSTStringhas been consistently applied, with no remnants of the old naming. The use ofasString()is also consistently maintained across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining toASTString methods and verify consistent use of toCSTString # Expected: No occurrences of toASTString and consistent usage of toCSTString echo "Checking for any remaining toASTString methods:" rg "toASTString" echo -e "\nVerifying consistent usage of toCSTString:" rg "toCSTString" echo -e "\nChecking for potential missed renames in documentation or comments:" rg -i "abstract syntax tree|AST"Length of output: 67963
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/And.java (2)
9-9: LGTM! Import consolidation looks good.The import consolidation to use
IExpressionfrom the core package aligns with the broader refactoring effort.
Line range hint
57-67: Implementation changes look good and align with PR objectives.The changes improve encapsulation and clarity:
- Renamed method better reflects its purpose
- Protected visibility supports proper execution stack management
- Implementation maintains efficient short-circuit evaluation
Let's verify consistent method renaming across related classes:
✅ Verification successful
Method renaming is consistent across logical operation classes
The verification confirms:
- All logical operation classes use the protected 'evaluate' method
- No remaining 'accept' methods found
- Implementation patterns are consistent with the PR's objectives
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent method renaming pattern across related classes # Expected: All logical operation classes should follow the same pattern # Check for any remaining 'accept' methods in related logical operation classes ast-grep --pattern 'class $_ implements IBooleanLogicExpression { $$$ public ISequence<$_> accept($$$) { $$$ } }' # Check for consistent protected evaluate methods ast-grep --pattern 'class $_ implements IBooleanLogicExpression { $$$ protected ISequence<$_> evaluate($$$) { $$$ } }'Length of output: 256
Script:
#!/bin/bash # First, find relevant logical operation classes echo "=== Searching for logical operation classes ===" rg -l "class.*(?:And|Or|Not)" --type java echo -e "\n=== Checking for 'accept' methods ===" rg "(?:public|protected).*\baccess\b.*\{" --type java -A 2 echo -e "\n=== Checking for 'evaluate' methods ===" rg "(?:public|protected).*\bevaluate\b.*\{" --type java -A 2Length of output: 23203
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/IntegerLiteral.java (3)
33-33: Good use of factory method pattern!Using
IIntegerItem.valueOf()instead of direct instantiation is a good practice. It centralizes the conversion logic and allows for potential optimizations.
22-22: Verify consistent refactoring across literal expression classesThe simplified generic type parameter looks good, but let's ensure this pattern is consistently applied across other literal expression classes.
✅ Verification successful
Refactoring is consistently applied across literal expression classes
All literal expression classes (
StringLiteral,DecimalLiteral, andIntegerLiteral) follow the same pattern of extendingAbstractLiteralExpressionwith a single type parameter. The change is verified to be consistent throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other literal expression classes follow the same pattern # Expected: All literal expression classes should extend AbstractLiteralExpression with a single type parameter ast-grep --pattern 'class $_ extends AbstractLiteralExpression<$_>'Length of output: 69
Script:
#!/bin/bash # First, find all Literal Java files echo "=== Literal Java Files ===" fd "Literal.*\.java$" # Then check their content echo -e "\n=== AbstractLiteralExpression Usage ===" rg "extends\s+AbstractLiteralExpression" -A 1Length of output: 1417
Line range hint
22-45: Verify handling of removed accept methodThe removal of
accept(DynamicContext, ISequence<?>)aligns with the PR objectives. However, let's verify:
- The evaluation functionality is properly handled by the superclass
- No breaking changes for existing callers
✅ Verification successful
Removal of accept method is safe and consistent
The removal of
accept(DynamicContext, ISequence<?>)is verified to be safe. All literal implementations (Decimal, String, Integer) consistently use the visitor pattern throughaccept(IExpressionVisitor, CONTEXT)with no dependencies on the removed method.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining direct calls to the old accept method # Expected: No direct calls should remain # Search for potential calls to the old accept method rg -A 2 "IntegerLiteral.*\.accept\(.*DynamicContext.*ISequence" # Check if other similar classes still have the old accept method ast-grep --pattern 'class $_ { $$$ accept($_: DynamicContext, $_: ISequence) { $$$ } }'Length of output: 165
Script:
#!/bin/bash # Check superclass implementation and other literal classes ast-grep --pattern 'class AbstractLiteralExpression { $$$ }' # Look for any accept method calls on IntegerLiteral rg "IntegerLiteral.*\.accept\(" -A 3 # Check other literal implementations fd "Literal.java" --exec cat {}Length of output: 4063
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/AbstractFilterExpression.java (2)
55-55: LGTM: Method signature changes improve encapsulationThe changes to the method signature are well-considered:
- Protected visibility better encapsulates the template method pattern
- The method name
evaluatebetter describes its purpose- The more general return type
ISequence<?>maintains type safety while being less restrictive
6-6: Verify package move impact on dependent classesThe move from
cst.logictocst.itemspackage better reflects the class's responsibility of handling item filtering. However, we should ensure all dependent classes are updated.✅ Verification successful
Package move is complete and consistent ✅
The move of
AbstractFilterExpressionfromcst.logictocst.itemspackage is properly implemented. All dependent classes (IntersectandExcept) are correctly placed in the same package with proper inheritance and consistent implementation patterns. No references to the old package path remain in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any references to the old package path that need updating # Search for old package references rg -l "gov\.nist\.secauto\.metaschema\.core\.metapath\.cst\.logic\.AbstractFilterExpression" # Search for potential import statements that might need updating rg "import.*AbstractFilterExpression"Length of output: 134
Script:
#!/bin/bash # Search for any references to AbstractFilterExpression class rg -l "AbstractFilterExpression" # Check for any class definitions extending AbstractFilterExpression ast-grep --pattern 'class $_ extends AbstractFilterExpression' # Look for any potential usages ast-grep --pattern 'new AbstractFilterExpression'Length of output: 419
Script:
#!/bin/bash # Check the content of related files echo "=== Intersect.java ===" cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Intersect.java echo -e "\n=== Except.java ===" cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Except.javaLength of output: 3610
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/VariableReference.java (2)
9-9: LGTM! Import changes align with the class's dependencies.The addition of
IExpressionand removal ofIItemproperly reflect the class's current needs.
70-72: Verify integration with the execution stack.The method change from
accepttoevaluatewith protected visibility looks good. However, I don't see integration with the execution stack mentioned in the PR objectives. This method should potentially push/pop itself onto the stack for debugging purposes.Let's check if other expression classes implement stack tracking:
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/path/KindNodeTest.java (1)
51-54: Verify integration with execution stack trackingWhile the method signature change from
accepttoevaluatealigns with the broader refactoring, the implementation doesn't appear to interact with the new execution stack tracking mentioned in the PR objectives. Consider updating the implementation to leverage the execution stack for debugging and error reporting.Run this verification script to check for execution stack usage in other similar evaluate methods:
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/Let.java (2)
9-9: Updated import statement aligns with refactoringThe import of
IExpressionreplacesIItem, reflecting the shift to usingIExpressionwithin the class. This change is appropriate and maintains consistency with the refactored code.
85-85: Verify the impact of changing method visibility from public to protectedThe method
accepthas been renamed toevaluateand its visibility changed frompublictoprotected. This could affect external classes or interfaces that relied on theacceptmethod being publicly accessible. Ensure that this change does not introduce breaking changes in the codebase.Please run the following script to identify any external references to
Let.accept:✅ Verification successful
Protected visibility for
evaluatemethod is appropriateThe change from public
acceptto protectedevaluateis a safe refactoring that:
- Aligns with the evaluation pattern used consistently throughout the codebase
- Maintains proper encapsulation as the method is only used within the inheritance hierarchy
- Does not impact any external usage patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for external uses of the `accept` method in the `Let` class. # Expected result: No external references to `Let.accept` should be found. rg -tjava 'Let\.accept\('Length of output: 27
Script:
#!/bin/bash # Find the Let class definition and its hierarchy ast-grep --pattern 'class Let $_' # Find any references to Let class rg -tjava "class.*extends.*Let|new Let\(|Let\s+\w+\s*=" # Look for both accept and evaluate method calls rg -tjava "(accept|evaluate).*\(" -A 2 -B 2Length of output: 87501
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Union.java (3)
9-9: LGTM! Import cleanup looks good.Removing duplicate imports helps maintain clean and maintainable code.
Line range hint
58-68: Consider updating child node interaction pattern.While the implementation is functionally correct, it still uses
acceptfor child nodes while the parent class has moved toevaluate. This might need to be updated for consistency with the new pattern.Let's verify if other similar classes have updated their child node handling:
57-57: Method signature change aligns with architectural improvements.The change from public
acceptto protectedevaluateimproves encapsulation and supports the new execution stack feature. This is part of a consistent pattern across the codebase.Let's verify the consistency of this pattern across related files:
✅ Verification successful
Method signature change is consistent with codebase patterns
The change to protected
evaluatemethod is part of a consistent pattern across the codebase where:
- All related classes in the package use the same protected
evaluate(DynamicContext, ISequence<?>)signature- The visitor pattern integration is maintained through public
acceptmethods- The change properly encapsulates evaluation logic while preserving the architectural design
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent method signature changes across related files # Look for any remaining 'accept' methods or inconsistent 'evaluate' signatures # Check for any remaining 'accept' methods in related files ast-grep --pattern 'public $_ accept($_, $_)' # Check for consistent evaluate method signatures ast-grep --pattern 'protected ISequence<?> evaluate(DynamicContext $_,$_)'Length of output: 124
Script:
#!/bin/bash # First, let's understand the broader context echo "=== Searching for accept methods ===" rg "accept.*\(" --type java -A 2 echo -e "\n=== Searching for evaluate methods ===" rg "evaluate.*\(" --type java -A 2 echo -e "\n=== Looking for Union class usage ===" rg "class.*Union" --type java -A 2Length of output: 83135
Script:
#!/bin/bash # Look for Union class and related evaluate methods echo "=== Union class definition ===" rg "class.*Union" --type java -A 5 core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ echo -e "\n=== Evaluate methods in related classes ===" rg "protected.*evaluate.*\(" --type java -A 2 core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/ echo -e "\n=== Accept methods in the same package ===" rg "accept.*\(" --type java core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/Length of output: 14291
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/DynamicContext.java
Outdated
Show resolved
Hide resolved
david-waltermire
force-pushed
the
feature/metapath-track-expression-context
branch
from
b943cfe to
7c524da
Compare
|
@coderebbitai review |
david-waltermire
merged commit 555b495
into
metaschema-framework:develop
Committer Notes
Added support for an execution stack on the DynamicContext that tracks the expressions entered and exited during Metapath evaluation. This will be useful context for debugging and error reporting.
This approach adds minimal overhead to processing, due to the stack-based approach which guarantees amortized constant time for push and pop operations.
All Submissions:
By submitting a pull request, you are agreeing to provide this contribution under the CC0 1.0 Universal public domain dedication.
Changes to Core Features:
Summary by CodeRabbit
Release Notes
New Features
Improvements
evaluatemethod.IExpressioninterface imports.toASTString()methods totoCSTString().Changes
IExpressioninterface.Internal Refactoring