Feature/anonymous functions #266
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 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe recent changes involve various modifications across multiple Java files within the project. Key updates include the introduction of new methods and classes, such as Changes
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 (
|
…vice as the source of data types. This provides for long-term scalability as new data types are added.
0804a78 to
945e9d4
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (40)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/node/AbstractNodeItem.java (1)
Line range hint
10-24: Consider adding documentation for the signature format.The
toSignature()method uses a specific format with special Unicode characters (⪻, ⪼) to construct the signature. Consider adding Javadoc to:
- Document the signature format:
type⪻metapath⪼(value)- Explain the purpose of the special Unicode characters
- Describe when the optional value part is included
This would improve maintainability and help developers understand the signature structure.
Here's a suggested documentation:
+ /** + * Generates a string signature for this node item in the format: + * {@code type⪻metapath⪼} or {@code type⪻metapath⪼(value)} + * where: + * <ul> + * <li>type: The node type signature</li> + * <li>metapath: The node's metapath expression</li> + * <li>value: Optional value signature if present</li> + * </ul> + * The special characters ⪻ and ⪼ are used as delimiters to clearly separate + * the type from the metapath expression. + * + * @return the string signature of this node item + */ @Override public final String toSignature() {core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/AnonymousFunctionCallTest.java (2)
15-17: Add class-level documentation.Consider adding Javadoc to describe:
- The purpose of this test class
- The relationship with the
AnonymousFunctionCallfunctionality being tested- The significance of the
NSconstant+/** + * Unit tests for anonymous function calls in Metapath expressions. + * These tests validate the compilation and execution of anonymous functions + * as defined in the Metaschema specification. + */ class AnonymousFunctionCallTest { private static final String NS = "http://example.com/ns";
24-24: Remove unused variable binding.The variable
var1is bound to the dynamic context but is never used in the test. Consider removing this line if it's not needed for the test case.core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/ArrowExpressionTest.java (2)
33-36: Remove commented test case if no longer neededThe commented test case should be removed if it's obsolete to maintain clean test code.
- // Arguments.of(ISequence.of(string("true")), "true() => string()"),Consider adding more test cases
While the current test cases cover basic string operations and variable access, consider adding tests for:
- Error cases (invalid function names, wrong argument types)
- Chaining multiple anonymous functions
- Complex expressions with nested arrow operations
42-54: LGTM: Well-structured context setup and test executionThe context setup and test execution are well-implemented. However, consider adding negative test cases to verify proper error handling:
- Invalid namespace bindings
- Undefined variables
- Type mismatches in function calls
Example test case to add:
@Test void testArrowExpressionWithUndefinedVariable() { StaticContext staticContext = StaticContext.builder() .namespace("ex", NS) .build(); DynamicContext dynamicContext = new DynamicContext(staticContext); assertThrows( MetapathException.class, () -> MetapathExpression.compile("() => $ex:undefined()", staticContext) .evaluateAs(null, MetapathExpression.ResultType.SEQUENCE, dynamicContext)); }core/src/test/java/gov/nist/secauto/metaschema/core/metapath/function/library/CastFunctionTest.java (1)
41-48: Add error handling tests and improve documentation.While the test method is well-structured, consider:
- Adding separate test methods for error scenarios using
assertThrows- Documenting the purpose of
newDynamicContext()and its role in the testAdd a test method for error cases:
@Test void testInvalidCasts() { assertThrows(MetapathException.class, () -> { MetapathExpression.compile(IIntegerItem.type().getQName().toEQName() + "('invalid')") .evaluateAs(null, MetapathExpression.ResultType.ITEM, newDynamicContext()); }); assertThrows(MetapathException.class, () -> { MetapathExpression.compile(IBooleanItem.type().getQName().toEQName() + "('invalid')") .evaluateAs(null, MetapathExpression.ResultType.ITEM, newDynamicContext()); }); }Add documentation to explain the test setup:
+ /** + * Tests the casting functionality using various input strings and target types. + * The dynamic context is created fresh for each test case to ensure isolation. + * + * @param text The input string to cast + * @param type The target type to cast to + * @param expected The expected result after casting + */ @ParameterizedTest @MethodSource("provideValues") void testExpression(@NonNull IStringItem text, @NonNull IAtomicOrUnionType<?> type,core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/CallingContext.java (4)
13-20: Consider defensive copying for the arguments list.While the class is properly immutable with final fields, the
List<ISequence<?>>arguments field could still be modified externally if the passed list is mutable. Consider making a defensive copy in the constructor and returning an unmodifiable view in the getter.public CallingContext( @NonNull IFunction function, @NonNull List<ISequence<?>> arguments, @Nullable IItem contextItem) { this.function = function; this.contextItem = contextItem; - this.arguments = arguments; + this.arguments = List.copyOf(arguments); // Creates an unmodifiable copy }
21-38: Enhance constructor documentation.While the documentation is present, it could be more descriptive about the role and requirements of each parameter. Consider adding details about:
- What constitutes a valid function
- Expected format/constraints for arguments
- Purpose and lifecycle of the contextItem
/** - * Set up the execution context for this function. + * Creates an immutable execution context for a function call. * * @param function - * the function + * the function to be executed, must implement IFunction interface * @param arguments - * the function arguments + * the list of evaluated arguments as sequences, must match function's arity * @param contextItem - * the current node context + * the optional context item representing the current node in scope */
50-58: Fix incorrect documentation in getContextItem method.The @return documentation incorrectly states it returns "the function instance" instead of "the context item".
/** * Get the node item focus associated with the calling context. * * @return - * the function instance + * the context item, or null if no context is set */
13-13: Add class-level documentation explaining the role in anonymous functions feature.Consider adding comprehensive class-level documentation that explains:
- The class's role in supporting anonymous functions
- Thread-safety guarantees through immutability
- Usage examples in the context of Metapath expressions
+/** + * Represents an immutable execution context for function calls in Metapath expressions. + * This class is designed to support both named and anonymous functions by maintaining + * the function instance, its arguments, and the current context item. It ensures + * thread-safety through immutability and is primarily used during the evaluation + * of Metapath expressions. + * + * <p>Example usage: + * <pre> + * IFunction function = ...; + * List<ISequence<?>> args = ...; + * IItem contextItem = ...; + * CallingContext context = new CallingContext(function, args, contextItem); + * // Use context for function evaluation + * </pre> + */ public final class CallingContext {core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/StaticFunctionCall.java (2)
22-26: Enhance class documentation to provide more contextWhile the documentation explains the basic purpose, consider adding:
- Distinction between static vs dynamic/anonymous function calls
- Relationship with the new anonymous functions feature
- Contract and expectations of the class
/** * Executes a function call based on the provided function and multiple argument * expressions that are used to determine the function arguments. + * + * This class handles static (pre-defined) function calls, as opposed to dynamic or + * anonymous function calls. Static functions are resolved during the parsing phase + * and must exist in the function registry. + * + * @see DynamicFunctionCall + * @see AnonymousFunctionCall */
37-37: Move implementation comment to JavadocThe comment explaining lazy loading behavior is important and should be in the constructor's Javadoc for better visibility.
/** * Construct a new function call expression. * * @param functionSupplier * the function supplier + * <p> + * Note: The function is fetched lazily to allow Metapaths to parse + * even if a function does not exist during the parsing phase. + * </p> * @param arguments * the expressions used to provide arguments to the function call */ public StaticFunctionCall(@NonNull Supplier<IFunction> functionSupplier, @NonNull List<IExpression> arguments) { - // lazy fetches the function so that Metapaths can parse even if a function does - // not exist this.functionSupplier = ObjectUtils.notNull(Lazy.lazy(functionSupplier)); this.arguments = arguments; }Also applies to: 42-45
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/DefaultFunction.java (2)
77-92: Consider enhancing documentation for focus parameterWhile the documentation is generally thorough, consider clarifying:
- The purpose and significance of the
focusparameter- How this method supports anonymous function execution
- When a null focus is acceptable vs. required
This would help developers better understand the method's role in the anonymous functions feature.
* Execute the provided function using the provided arguments, dynamic context, * and focus. * * @param function * the signature of the function * @param arguments * the function arguments * @param dynamicContext * the dynamic evaluation context * @param focus - * the current focus + * the current focus item in the evaluation context. This represents + * the context item for anonymous function evaluation. May be null + * for functions that don't require context item access. * @return a sequence containing the result of the execution * @throws MetapathException * if an error occurred while executing the function
Line range hint
1-99: Consider documenting the architectural changesThe simplified execution flow and delegation pattern are good architectural choices. Consider:
- Adding a class-level documentation section explaining how
DefaultFunctionfits into the anonymous functions feature- Documenting the relationship between
DefaultFunction,IFunctionExecutor, and the newCallingContextclass- Adding examples of how anonymous functions are handled through this execution path
This would help maintain the codebase's long-term maintainability as the anonymous functions feature evolves.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/CastFunction.java (1)
37-43: Fix typo in parameter nameresulingAtomicTypeThe method implementation is well-structured and provides a valuable convenience overload. However, there's a typo in the parameter name that should be fixed for better code readability.
Apply this diff to fix the typo:
static <ITEM extends IAnyAtomicItem> IFunction signature( @NonNull IEnhancedQName name, - @NonNull IAtomicOrUnionType<?> resulingAtomicType, + @NonNull IAtomicOrUnionType<?> resultingAtomicType, @NonNull IAtomicOrUnionType.ICastExecutor<ITEM> executor) { - return signature(name.getNamespace(), name.getLocalName(), resulingAtomicType, executor); + return signature(name.getNamespace(), name.getLocalName(), resultingAtomicType, executor); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IAnyAtomicItem.java (1)
58-61: Add Javadoc for the new method.While the implementation is correct and follows the interface's patterns, please add Javadoc to maintain consistency with other methods in the interface. The documentation should describe the method's purpose, return value, and any relevant details about the string conversion.
Here's a suggested addition:
+ /** + * Converts this atomic item to a string item representation. + * + * @return a new {@link IStringItem} containing the string representation of this item + * @see #asString() + */ @NonNull default IStringItem asStringItem() { return IStringItem.valueOf(asString()); }core/src/main/java/gov/nist/secauto/metaschema/core/datatype/DataTypeService.java (1)
220-228: Enhance JavaDoc with more context and usage examples.The implementation looks good and follows best practices for null safety and immutability. However, the JavaDoc could be more descriptive by:
- Explaining the relationship with anonymous functions feature
- Clarifying that the returned collection is unmodifiable
- Adding usage examples for dynamic function registration
Here's a suggested JavaDoc enhancement:
/** * Get the collection of data type adapters provided by this service. * + * This method provides access to all registered data type adapters, which is particularly + * useful for dynamic function registration and type casting operations. The returned + * collection is unmodifiable. * + * Example usage: + * <pre> + * Collection<? extends IDataTypeAdapter<?>> dataTypes = dataTypeService.getDataTypes(); + * for (IDataTypeAdapter<?> adapter : dataTypes) { + * // Register casting functions for each data type + * registerCastingFunction(adapter); + * } + * </pre> * * @return the data type adapters */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractExpressionVisitor.java (1)
217-220: LGTM! Consider documenting the dynamic function call behaviorThe new method follows the established visitor pattern and is implemented consistently. Consider adding Javadoc to explain:
- The purpose of dynamic function calls
- How they differ from static function calls
- Expected usage patterns
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/CSTPrinter.java (1)
195-206: Consider adding documentation for the new function typesWhile the implementation is solid, consider adding Javadoc comments to describe the different types of function calls (static, dynamic, and anonymous) and their use cases. This would help developers understand when to use each type.
Example documentation:
+ /** + * Visits a static function call node in the CST. + * Static function calls are resolved at compile time and have fixed function references. + * + * @param expr the static function call expression + * @param context the current visitor context + * @return the string representation of the node + */ public String visitStaticFunctionCall(StaticFunctionCall expr, State context) { + /** + * Visits a dynamic function call node in the CST. + * Dynamic function calls are resolved at runtime based on variable references. + * + * @param expr the dynamic function call expression + * @param context the current visitor context + * @return the string representation of the node + */ public String visitDynamicFunctionCall(DynamicFunctionCall expr, State context) { + /** + * Visits an anonymous function call node in the CST. + * Anonymous functions are inline function expressions that can be assigned to variables. + * + * @param expr the anonymous function call expression + * @param context the current visitor context + * @return the string representation of the node + */ public String visitAnonymousFunctionCall(AnonymousFunctionCall expr, State context) {core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/ComparisonFunctions.java (2)
9-9: Consider breaking down this class into smaller, more focused components.While the PMD suppressions for
GodClassandCyclomaticComplexityare understandable given the nature of comparison operations, the class could benefit from being split into smaller, type-specific comparison classes to improve maintainability and testability.Consider creating separate classes for each type of comparison:
StringComparisonNumericComparisonDateTimeComparison- etc.
This would:
- Reduce the complexity of individual classes
- Make the code more maintainable
- Make testing more focused
- Allow for better separation of concerns
Also applies to: 31-31
Line range hint
196-218: Remove commented-out code in stringCompare method.The method contains commented-out code showing a previous implementation using
OperationFunctions. This should be removed to maintain code cleanliness.Apply this diff to remove the commented-out code:
switch (operator) { case EQ: - // retval = OperationFunctions.opNumericEqual(left.compare(right), - // IIntegerItem.ZERO); retval = result == 0; break; case GE: - // retval = OperationFunctions.opNumericGreaterThan(left.compare(right), - // IIntegerItem.NEGATIVE_ONE); retval = result >= 0; break; case GT: - // retval = OperationFunctions.opNumericGreaterThan(left.compare(right), - // IIntegerItem.ZERO); retval = result > 0; break;pom.xml (1)
633-639: LGTM! Consider additional release plugin configurations.The addition of SemVerVersionPolicy is a good practice that will enforce semantic versioning rules during releases.
Consider adding these beneficial configurations to the release plugin:
<plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-release-plugin</artifactId> <configuration> <projectVersionPolicyId>SemVerVersionPolicy</projectVersionPolicyId> + <tagNameFormat>v@{project.version}</tagNameFormat> + <preparationGoals>clean verify</preparationGoals> + <autoVersionSubmodules>true</autoVersionSubmodules> </configuration> </plugin>These additions would:
- Standardize release tag format with a 'v' prefix
- Ensure clean builds during release preparation
- Automatically version submodules to match the parent version
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/impl/OperationFunctions.java (2)
Line range hint
392-396: Fix incorrect duration operation in opSubtractDayTimeDurationFromDateTime.The method is using
plusinstead ofminuswhich contradicts its intended subtraction operation.Apply this fix:
@NonNull public static IDateTimeItem opSubtractDayTimeDurationFromDateTime( @NonNull IDateTimeItem moment, @NonNull IDayTimeDurationItem duration) { return IDateTimeWithTimeZoneItem.valueOf( - ObjectUtils.notNull(moment.asZonedDateTime().plus(duration.asDuration()))); + ObjectUtils.notNull(moment.asZonedDateTime().minus(duration.asDuration()))); }
Line range hint
449-463: Consider improving duration comparison precision.The yearMonthDuration comparison using
toTotalMonthsis marked as an approximation. Consider implementing a more precise comparison that takes into account the varying lengths of months.Consider implementing a more precise comparison:
@NonNull public static IBooleanItem opYearMonthDurationGreaterThan( @NonNull IYearMonthDurationItem arg1, @NonNull IYearMonthDurationItem arg2) { Period p1 = arg1.asPeriod(); Period p2 = arg2.asPeriod(); - // this is only an approximation - return IBooleanItem.valueOf(p1.toTotalMonths() > p2.toTotalMonths()); + // Compare years first, then months if years are equal + if (p1.getYears() != p2.getYears()) { + return IBooleanItem.valueOf(p1.getYears() > p2.getYears()); + } + return IBooleanItem.valueOf(p1.getMonths() > p2.getMonths()); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/AbstractAstVisitor.java (1)
268-281: Add missing @return description in JavaDocThe JavaDoc for
handleInlinefunctionexpris missing the description for the @return parameter. Please add it to maintain consistency with other method documentation in the class.Apply this change:
/** * Handle the provided expression. * * @param ctx * the provided expression context - * @return the result + * @return the result of evaluating the inline function expression */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/DynamicFunctionCall.java (2)
22-25: Fix the typo in the class documentation comment.There's a misspelling of "determine" on line 23. Correcting this improves readability and professionalism.
Apply this diff to fix the typo:
/** * Executes a function call based on a specifier expression that is used to - * dtermine the function and multiple argument expressions that are used to + * determine the function and multiple argument expressions that are used to * determine the function arguments. */
78-80: Correct the exception message for clarity.The exception message contains a typo: "The error specifier is an empty sequence." It should be "The function specifier is an empty sequence." This clarification enhances understanding when the exception is thrown.
Apply this diff to correct the exception message:
throw new StaticMetapathException( StaticMetapathException.NO_FUNCTION_MATCH, - "Unable to get function name. The error specifier is an empty sequence."); + "Unable to get function name. The function specifier is an empty sequence.");core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AnonymousFunctionCall.java (1)
28-29: Fix typographical error in class documentationThere's a typo in the class comment: "dtermine" should be "determine".
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/FunctionCallAccessor.java (3)
33-33: Update documentation to reflect multiple argumentsThe field
argumentshas been changed to a list to support multiple arguments, but the Javadoc comments still refer to a singularkeyOrIndexand "the argument". Please update the documentation to accurately describe the parameter and its usage.
45-47: Constructor parameter name mismatch in documentationThe constructor's Javadoc parameter
@param keyOrIndexdoes not match the updated parameterarguments. Please update the parameter name in the Javadoc toargumentsand provide an appropriate description.
66-67: Correct method documentation to reflect multiple argumentsThe method
getArguments()returns a list of arguments, but the Javadoc refers to a singular "the argument". Update the documentation to reflect that multiple arguments are now returned.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/impl/AbstractFunction.java (3)
119-119: Remove unnecessary@SuppressWarnings("unused")annotationThe
@SuppressWarnings("unused")annotation on theconvertArgument()method is unnecessary because the method is actively used inconvertArguments(). Removing this annotation will improve code clarity.Apply this diff to remove the annotation:
- @SuppressWarnings("unused")
175-177: Address TODO or remove commented codeThere is a commented-out block with a
TODOnote indicating that a cast to atomic type should be applied. Please consider implementing the required functionality or removing the commented code to maintain code cleanliness.- // if (IUntypedAtomicItem.class.isInstance(item)) { // NOPMD - // // TODO: apply cast to atomic type - // }If the casting is necessary for correct functionality, please implement it accordingly.
239-242: Remove commented-out logging code or consider re-enabling itThe commented-out logging code might be useful for debugging or monitoring. If it's no longer needed, consider removing it. Otherwise, re-enable it to take advantage of the logging capabilities.
- // logger.info(String.format("Executed function '%s' with arguments '%s' producing result '%s'", - // toSignature(), convertedArguments.toString(), result.asList().toString())); + logger.info(String.format("Executed function '%s' with arguments '%s' producing result '%s'", + toSignature(), convertedArguments.toString(), result.asList().toString()));Ensure that the logger is properly configured before re-enabling this code.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractCSTVisitorBase.java (1)
138-155: Unused Generic Type ParameterSinnairyToListMethodThe generic type parameter
S extends List<R>in thenairyToListmethod is declared but not utilized within the method signature or body. Removing unused type parameters can improve code readability and maintainability.Apply this diff to remove the unused type parameter:
-protected <CONTEXT extends ParserRuleContext, T, R, S extends List<R>> +protected <CONTEXT extends ParserRuleContext, T, R>core/src/main/java/gov/nist/secauto/metaschema/core/metapath/StaticContext.java (3)
42-42: Consider refactoring well-known namespaces into a separate classThe
// FIXMEcomment indicates an intention to refactor the well-known namespaces into a new class. Organizing these namespaces into their own class can enhance code clarity and maintainability.Would you like assistance in refactoring the well-known namespaces into a separate class?
Line range hint
48-48: UseHashMapinstead ofConcurrentHashMapin static initializationThe
knownNamespacesmap in the static initializer is only used during class loading, which is inherently single-threaded. Using aConcurrentHashMaphere is unnecessary and may introduce overhead. Consider replacing it with aHashMapfor better performance.[performance]
Apply this diff:
-static { - Map<String, String> knownNamespaces = new ConcurrentHashMap<>(); +static { + Map<String, String> knownNamespaces = new HashMap<>();
Line range hint
90-92: Simplify redundant null check inlookupNamespaceForPrefixThe return statement is unnecessarily verbose. Since
resultis eithernullor a valid value, you can return it directly without the conditional check.Apply this diff:
-public String lookupNamespaceForPrefix(@NonNull String prefix) { - String result = lookupNamespaceURIForPrefix(prefix); - return result == null ? null : result; +public String lookupNamespaceForPrefix(@NonNull String prefix) { + return lookupNamespaceURIForPrefix(prefix); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/BuildCSTVisitor.java (2)
394-397: Improve exception message invisitNamedfunctionrefmethodCurrently, the method
visitNamedfunctionrefthrows anUnsupportedOperationExceptionwith a generic message. Providing a more descriptive error message that includes the unsupported expression or context can aid in debugging and maintenance.Consider updating the exception message as follows:
public IExpression visitNamedfunctionref(Metapath10.NamedfunctionrefContext ctx) { - throw new UnsupportedOperationException("expression not supported"); + throw new UnsupportedOperationException("Named function references are not supported at this time."); }
502-503: Remove redundant null check after collecting argumentsIn the
handlePostfixexprmethod, the result ofcollect(Collectors.toUnmodifiableList())is wrapped withObjectUtils.notNull. SinceCollectors.toUnmodifiableList()does not returnnull, this null check is unnecessary and can be removed.Consider simplifying the code as follows:
-ObjectUtils.notNull(parseArgumentList((Metapath10.ArgumentlistContext) tree) - .collect(Collectors.toUnmodifiableList())) +parseArgumentList((Metapath10.ArgumentlistContext) tree) + .collect(Collectors.toUnmodifiableList())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (41)
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/DataTypeService.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/DynamicContext.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/StaticContext.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/AbstractAstVisitor.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/AbstractExpressionVisitor.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AnonymousFunctionCall.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/BuildCSTVisitor.java(6 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/CSTPrinter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/DynamicFunctionCall.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/IExpressionVisitor.java(1 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/items/FunctionCallAccessor.java(4 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/Negate.java(1 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(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/Modulo.java(1 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/Subtraction.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/AbstractFunction.java(0 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/ArgumentImpl.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/CallingContext.java(1 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/function/DefaultFunction.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/IArgument.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/impl/AbstractFunction.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/impl/OperationFunctions.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/CastFunction.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnAvg.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnSum.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/AbstractAtomicItemBase.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IAnyAtomicItem.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/node/AbstractNodeItem.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/OperationFunctionsTest.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/AnonymousFunctionCallTest.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/ArrowExpressionTest.java(2 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/function/OperationFunctionsTest.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/function/library/CastFunctionTest.java(1 hunks)pom.xml(1 hunks)
💤 Files with no reviewable changes (1)
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/AbstractFunction.java
✅ Files skipped from review due to trivial changes (12)
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/DynamicContext.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/Negate.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/Division.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/Modulo.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/math/Subtraction.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnAvg.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnSum.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/AbstractAtomicItemBase.java
- core/src/test/java/gov/nist/secauto/metaschema/core/metapath/OperationFunctionsTest.java
🔇 Additional comments (42)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/node/AbstractNodeItem.java (1)
14-14: LGTM! Good performance optimization.
The change from string literals to character literals for single characters is a good optimization that avoids unnecessary string object creation.
Also applies to: 16-16, 19-21
core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/AnonymousFunctionCallTest.java (1)
1-14: LGTM! Well-organized imports.
The package structure and imports are clean, well-organized, and all imports are being utilized.
core/src/test/java/gov/nist/secauto/metaschema/core/metapath/function/OperationFunctionsTest.java (2)
12-12: LGTM! Import statement updated correctly.
The import statement has been properly updated to reflect the new package structure, moving OperationFunctions to the impl package which better separates interface from implementation.
12-12: Verify package restructuring consistency.
Let's ensure this package restructuring is consistent across the codebase.
✅ Verification successful
Let me verify the content of these files to ensure the package restructuring is consistent.
Package restructuring is consistent across the codebase
All references to OperationFunctions are using the new package path gov.nist.secauto.metaschema.core.metapath.function.impl.OperationFunctions consistently across the codebase, including:
- Test files in
core/src/test/java/gov/nist/secauto/metaschema/core/metapath/ - Math operation classes in
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/ - Logic operations in
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/logic/ - Function implementations in
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/
No instances of the old package path were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old package path and verify all imports are updated
# Check for any remaining references to the old package path
echo "Checking for old package references..."
rg -l "gov\.nist\.secauto\.metaschema\.core\.metapath\.function\.OperationFunctions" --type java
# Check for references to the new package path to confirm consistent usage
echo "Checking new package references..."
rg -l "gov\.nist\.secauto\.metaschema\.core\.metapath\.function\.impl\.OperationFunctions" --type java
# Look for any potential missed imports of OperationFunctions
echo "Checking for any potential missed imports..."
ast-grep --pattern 'import $$$OperationFunctions;'
Length of output: 1582
Script:
#!/bin/bash
# Check the actual content of files to verify the imports and usages
echo "Checking content of test files..."
rg -A 2 "import.*OperationFunctions" core/src/test/java/gov/nist/secauto/metaschema/core/metapath/OperationFunctionsTest.java core/src/test/java/gov/nist/secauto/metaschema/core/metapath/function/OperationFunctionsTest.java
echo "Checking content of main source files..."
rg -A 2 "import.*OperationFunctions" 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/Negate.java core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/math/*.java core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/Fn*.java
Length of output: 11425
core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/ArrowExpressionTest.java (2)
9-9: LGTM: Import changes align with new functionality
The added imports support the enhanced context management and string manipulation features needed for anonymous function testing.
Also applies to: 12-12, 16-17
29-30: LGTM: Well-defined test namespace constant
The namespace constant follows good practices with a clear purpose and standard URI format.
core/src/test/java/gov/nist/secauto/metaschema/core/metapath/function/library/CastFunctionTest.java (2)
1-30: LGTM! Well-structured test class with appropriate imports.
The file is well-organized with proper license headers, specific imports, and appropriate test base class extension.
1-50: Verify integration with anonymous functions feature.
While this test file effectively covers casting functionality, it's not immediately clear how it supports the PR's main objective of adding anonymous functions support. Consider adding test cases that demonstrate casting within anonymous function contexts.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/CallingContext.java (2)
1-12: LGTM! Well-organized imports with proper null safety annotations.
The imports are minimal, necessary, and the use of FindBugs annotations shows good attention to null safety in the public API.
70-91: LGTM! Well-implemented hashCode and equals methods.
The implementation follows best practices:
- Proper use of prime numbers in hashCode
- Complete null checks and type comparison in equals
- Consistent with equals/hashCode contract
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/StaticFunctionCall.java (3)
30-31: LGTM! Well-structured field declarations
The fields are properly declared with immutability, null-safety annotations, and efficient lazy loading for the function supplier.
73-74: LGTM! Improved AST string representation
Good addition of function arity information to the AST string, which will help with debugging and logging.
79-79: Verify visitor interface changes
The change to visitStaticFunctionCall suggests a hierarchy of function call types. Let's verify the visitor interface is properly updated.
Also applies to: 84-84
✅ Verification successful
Visitor interface properly implements function call hierarchy
The visitor interface and its implementations are correctly structured to support the function call hierarchy:
IExpressionVisitorinterface defines all three function call visitor methods:visitStaticFunctionCallvisitDynamicFunctionCallvisitAnonymousFunctionCall
AbstractExpressionVisitorimplements these methods- Concrete visitor
CSTPrinterproperly overrides all methods - Each function call type (
StaticFunctionCall,DynamicFunctionCall,AnonymousFunctionCall) correctly uses its corresponding visitor method
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the visitor interface definition and its implementations
ast-grep --pattern 'interface IExpressionVisitor {
$$$
visitStaticFunctionCall($$$) {
$$$
}
$$$
}'
# Check for other function call visitor methods
ast-grep --pattern 'visitDynamicFunctionCall($$$) {
$$$
}'
ast-grep --pattern 'visitAnonymousFunctionCall($$$) {
$$$
}'
Length of output: 240
Script:
#!/bin/bash
# Let's try a different approach to find the visitor interface and its methods
# First find the interface file
fd "IExpressionVisitor.java"
# Search for visitor method patterns in the codebase
rg "visitStaticFunctionCall|visitDynamicFunctionCall|visitAnonymousFunctionCall" -A 2
# Look for any class implementing IExpressionVisitor
rg "implements.*IExpressionVisitor"
Length of output: 5381
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/DefaultFunction.java (2)
11-11: LGTM: Import statement is correctly placed
The import for AbstractFunction is necessary and properly organized.
93-98: Verify error handling in IFunctionExecutor implementations
The simplified implementation looks clean and follows good practices. However, let's verify that all IFunctionExecutor implementations properly handle errors and maintain consistent behavior.
✅ Verification successful
Let me search for the IFunctionExecutor implementations to verify error handling.
Let me try a different search pattern to find the implementations and error handling.
Let me check the specific implementations found to verify their error handling.
Based on the review of the implementations and earlier error handling patterns found, I can now provide a final response:
Error handling is properly implemented throughout the function execution chain
The simplified implementation in DefaultFunction is safe because:
-
The IFunctionExecutor implementations (NumericFunction and CastFunction) properly handle:
- Null checks using ObjectUtils.requireNonNull/notNull
- Empty sequences with proper empty result returns
- Type safety through FunctionUtils.asType() which throws TypeMetapathException
-
The execution chain has comprehensive error handling:
- MetapathException and its subtypes (TypeMetapathException, StaticMetapathException, etc.) are properly thrown and propagated
- Input validation occurs at multiple levels (function arguments, type checking, value conversion)
- Empty/null cases are explicitly handled with appropriate responses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check IFunctionExecutor implementations for error handling
# Find all implementations of IFunctionExecutor
echo "Searching for IFunctionExecutor implementations..."
ast-grep --pattern 'class $_ implements $$$IFunctionExecutor$$$'
# Look for error handling patterns in these implementations
echo "Checking error handling patterns..."
rg -A 5 "throw.*MetapathException"
rg -A 5 "catch.*MetapathException"
Length of output: 106535
Script:
#!/bin/bash
# Search for IFunctionExecutor implementations and their error handling patterns
echo "Searching for IFunctionExecutor implementations..."
ast-grep --pattern 'class $_ implements $$$IFunctionExecutor$$$' -f
echo "Checking execute method implementations..."
ast-grep --pattern 'execute($$$) {
$$$
}'
Length of output: 499
Script:
#!/bin/bash
# Search for IFunctionExecutor implementations and their error handling
echo "Searching for IFunctionExecutor implementations..."
rg -l "implements.*IFunctionExecutor"
echo "Checking execute method implementations in these files..."
rg -A 10 "execute\(.*IFunction.*ISequence.*DynamicContext.*IItem.*\)"
Length of output: 542
Script:
#!/bin/bash
# Check error handling in the identified function executor implementations
echo "Checking NumericFunction implementation..."
cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/NumericFunction.java
echo -e "\nChecking CastFunction implementation..."
cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/CastFunction.java
Length of output: 7146
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/CastFunction.java (1)
18-18: LGTM!
The import statement for IEnhancedQName is appropriately placed and necessary for the new functionality.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IAnyAtomicItem.java (1)
58-61: LGTM! Implementation follows good practices.
The implementation:
- Properly uses @nonnull annotation
- Follows immutable design by returning new instance
- Leverages existing asString() method
- Maintains consistency with interface's conversion pattern
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/DataTypeService.java (2)
21-21: LGTM!
The Collection import is appropriately placed and necessary for the new method's return type.
226-227: Verify data type availability for anonymous functions.
Since this method is crucial for supporting anonymous functions, let's verify that all necessary data types are available.
✅ Verification successful
Data type availability for anonymous functions is properly supported
The method getDataTypes() is correctly integrated with the function system as evidenced by:
- It's used by DefaultFunctionLibrary to register casting functions for all data types:
DataTypeService.instance().getDataTypes().stream()
.map(IDataTypeAdapter::getItemType)
...
registerFunction(CastFunction.signature(type.getQName(), type, type::cast));- The extensive function registration in DefaultFunctionLibrary shows proper integration with all necessary data type operations including:
- Array operations (get, size, put, append, etc.)
- Map operations (merge, size, keys, contains, etc.)
- String operations (length, substring, etc.)
- Numeric operations (abs, round, floor, etc.)
- Boolean operations (true, false, not)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for data type implementations that might be used in anonymous functions
# Look for data type implementations
echo "Searching for data type implementations..."
rg -l "implements.*IDataTypeAdapter"
# Look for function registration patterns
echo "Checking function registration patterns..."
rg -A 5 "registerFunction|registerCastingFunction"
# Look for anonymous function related code
echo "Checking anonymous function usage..."
ast-grep --pattern 'class $_ implements IDataTypeAdapter {
$$$
}'
Length of output: 38355
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractExpressionVisitor.java (2)
222-223: LGTM! Good grouping of function call handlers
The repositioning of this method improves code organization by grouping all function call-related methods together.
213-215: Verify the impact of renaming visitFunctionCall to visitStaticFunctionCall
The rename better reflects the method's purpose for handling static function calls. However, we should verify that all implementing classes have been updated accordingly.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java (2)
8-9: LGTM: Import statements are appropriate
The new imports for DataTypeService and IDataTypeAdapter are necessary for the dynamic casting function registration implementation.
249-253: Verify complete type coverage in the dynamic registration
The new dynamic casting function registration is a good improvement that reduces code duplication and improves maintainability. However, we should verify that all previously supported data types are still covered by this change.
Let's verify the registered types:
✅ Verification successful
Let me try a different approach to gather the necessary information about data types and casting functions.
Let me try another approach to find the data types and their registration:
Let me try one more search to find the concrete data type implementations and their registration:
Based on the evidence gathered, I can now provide a final response:
Dynamic casting function registration is safe and complete
The implementation correctly handles all data types through the DataTypeService's ServiceLoader pattern. The registration:
- Uses Java's ServiceLoader to dynamically load all IDataTypeAdapter implementations
- Maintains type safety through the IDataTypeAdapter interface
- Preserves order through forEachOrdered() when registering cast functions
- Automatically includes any new data types added to the service provider configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare the available data types to ensure complete coverage
# Search for previously registered cast functions in git history
echo "Previously registered cast functions:"
git log -p --full-history -S "registerFunction.*Cast" -- "$(git rev-parse --show-toplevel)/core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java" | grep -B 1 "registerFunction.*Cast"
# List current data types
echo -e "\nCurrently available data types:"
ast-grep --pattern 'class $_ implements IDataTypeAdapter'
Length of output: 223
Script:
#!/bin/bash
# Search for data type implementations and casting related code
# Find data type adapter implementations
echo "Data Type Adapter Implementations:"
rg "class.*implements.*IDataTypeAdapter" -A 2
# Find DataTypeService usage and initialization
echo -e "\nDataTypeService Registration:"
rg "DataTypeService" -A 3
# Look for cast function related code
echo -e "\nCast Function Related Code:"
rg "CastFunction" -A 2
Length of output: 65844
Script:
#!/bin/bash
# Find data type implementations and their registration
# Look for data type adapter implementations
echo "Data Type Adapter Implementations:"
fd -e java -x rg -l "implements.*IDataTypeAdapter" {} \;
# Look for built-in data types initialization
echo -e "\nBuilt-in Data Types:"
rg "BUILTIN_ITEM_TYPES" -A 10
# Look for DataTypeService initialization
echo -e "\nDataTypeService Initialization:"
rg "DataTypeService.*private.*DataTypeService" -A 10
Length of output: 4694
Script:
#!/bin/bash
# Find concrete data type implementations and service provider registration
# Look for concrete data type adapter classes
echo "Concrete Data Type Adapters:"
rg "class.*extends.*AbstractDataTypeAdapter" -A 2
# Look for service provider configuration
echo -e "\nService Provider Configuration:"
fd "META-INF/services" --type f -x cat {}
# Look for data type registration in DataTypeService
echo -e "\nData Type Registration:"
rg "registerDataType|getDataTypes" -A 3
Length of output: 1461
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/CSTPrinter.java (3)
195-197: LGTM: Method rename maintains visitor pattern consistency
The rename from visitFunctionCall to visitStaticFunctionCall properly differentiates static function calls while maintaining consistency with the visitor pattern implementation.
204-206: Verify anonymous function implementation completeness
The implementation aligns with the PR objective to support anonymous functions. Let's verify the complete implementation chain.
✅ Verification successful
Let me gather more information about the anonymous function implementation.
Based on the gathered information, I can now generate the final response:
Anonymous function implementation is complete and well-structured
The implementation chain for anonymous functions is properly implemented with:
- Complete
AnonymousFunctionCallclass extendingAbstractFunction - Proper visitor pattern integration through
IExpressionVisitorinterface - Test coverage with function binding and evaluation tests
- Consistent implementation in the CST printer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the complete anonymous function implementation chain
# Check for AnonymousFunctionCall class and related components
echo "Checking AnonymousFunctionCall implementation..."
ast-grep --pattern 'class AnonymousFunctionCall {
$$$
}'
# Look for let expression handling with anonymous functions
echo "Checking let expression handling..."
rg -A 5 "let.*=.*function"
# Check for test coverage
echo "Checking test coverage..."
fd -e java -e groovy ".*Test.*" | rg -l "AnonymousFunction"
Length of output: 1234
Script:
#!/bin/bash
# Check for the complete anonymous function implementation chain
# Look for AnonymousFunctionCall class definition and usage
echo "Checking AnonymousFunctionCall class definition..."
rg -A 10 "class AnonymousFunctionCall"
# Check for function expression handling
echo "Checking function expression handling..."
rg -A 5 "function\s*\(\s*\$\w+\s*\)"
# Check parser implementation for anonymous functions
echo "Checking parser implementation..."
ast-grep --pattern 'function($$$) {
$$$
}'
# Check visitor pattern implementation
echo "Looking for visitor pattern implementation..."
rg -A 3 "visit.*AnonymousFunction"
Length of output: 5474
199-202: Verify dynamic function call implementation completeness
The implementation looks correct, but let's verify the supporting classes and error handling for dynamic function calls.
✅ Verification successful
Based on the search results, I can now provide a final response as we have sufficient information about the DynamicFunctionCall implementation and its supporting infrastructure:
Dynamic function call implementation is complete and well-integrated
The implementation is verified to be complete with proper integration:
- The
DynamicFunctionCallclass is properly implemented with required fields for function identifier and arguments - It's integrated into the visitor pattern through
IExpressionVisitorinterface - The class implements
IExpressioninterface and includes proper error handling through the dynamic context - The implementation is consistent with other function call types (StaticFunctionCall, AnonymousFunctionCall)
- The function evaluation is handled through the dynamic context, allowing runtime resolution of function calls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper implementation of dynamic function calls
# Look for DynamicFunctionCall class definition and error handling
# Check for DynamicFunctionCall class definition
echo "Checking DynamicFunctionCall class definition..."
ast-grep --pattern 'class DynamicFunctionCall {
$$$
}'
# Check for error handling in dynamic function resolution
echo "Checking error handling in dynamic function resolution..."
rg -l "DynamicFunctionCall.*throws"
Length of output: 328
Script:
#!/bin/bash
# Let's try a different approach to find DynamicFunctionCall implementation and usage
# Search for DynamicFunctionCall in Java files
echo "Searching for DynamicFunctionCall class and its usage..."
rg -t java "DynamicFunctionCall" -A 5
# Search for function call related interfaces and classes
echo "Searching for related function call interfaces..."
rg -t java "interface.*Function.*Call|class.*Function.*Call" -A 3
# Look for dynamic function evaluation related code
echo "Searching for dynamic function evaluation..."
rg -t java "evaluate.*dynamic|resolve.*dynamic" -A 3
Length of output: 27441
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/IExpressionVisitor.java (2)
200-200: Breaking Change: Function call visitor method renamed
The rename from visitFunctionCall to visitStaticFunctionCall better reflects its purpose but requires updates in all implementing classes.
Let's verify the impact:
✅ Verification successful
Let me gather more information about the implementation to ensure we have a complete picture.
Method rename is correctly implemented across the codebase
The search results show that:
- The
visitStaticFunctionCallmethod is properly implemented inAbstractExpressionVisitor - The
StaticFunctionCallclass correctly uses the new method name in its accept method - The occurrences of "visitFunctionCall" found are for a different method
visitFunctionCallAccessorwhich is unrelated to this change
The breaking change has been properly handled across all implementing classes, and there are no lingering instances of the old method name that need to be updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of IExpressionVisitor to ensure they're updated
# with the renamed method.
# Find all Java files that might implement IExpressionVisitor
rg -l "implements.*IExpressionVisitor"
# Search for any remaining uses of the old method name
rg "visitFunctionCall"
Length of output: 1026
Script:
#!/bin/bash
# Check the implementation in AbstractExpressionVisitor
ast-grep --pattern 'class AbstractExpressionVisitor implements IExpressionVisitor {
$$$
}'
# Look for any static function call related code
rg "StaticFunctionCall" -A 3
Length of output: 4352
202-222: LGTM: New function call visitor methods
The new visitDynamicFunctionCall and visitAnonymousFunctionCall methods:
- Follow the established visitor pattern
- Maintain consistent documentation and parameter annotations
- Support the PR's objective of adding anonymous functions per W3C XPath 3.1
Let's verify the implementations:
✅ Verification successful
Verified: New visitor methods are properly implemented across the codebase
The new visitor methods are correctly integrated into the visitor pattern implementation:
AbstractExpressionVisitorprovides default implementations that visit childrenCSTPrinterextends the base implementation for both methodsDynamicFunctionCallandAnonymousFunctionCallclasses properly implement the accept methods- The implementations maintain consistent parameter types and annotations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that concrete visitor implementations handle the new methods
# Find implementations of the new methods
ast-grep --pattern 'class $_ implements $_ {
$$$
visitDynamicFunctionCall($_, $_) {
$$$
}
$$$
}'
ast-grep --pattern 'class $_ implements $_ {
$$$
visitAnonymousFunctionCall($_, $_) {
$$$
}
$$$
}'
Length of output: 223
Script:
#!/bin/bash
# Let's try a different approach to find implementations using ripgrep
# Search for class files that implement IExpressionVisitor
rg -l "implements.*IExpressionVisitor" --type java
# Then look for the new visitor methods in those files
rg "visitDynamicFunctionCall|visitAnonymousFunctionCall" --type java -C 2
Length of output: 5319
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/ComparisonFunctions.java (2)
Line range hint 73-74: LGTM: Method name typos fixed correctly.
The method names have been corrected from valueCompairison to valueComparison and generalCompairison to generalComparison, improving code clarity and consistency.
Also applies to: 89-91
Line range hint 196-218: Verify consistency in comparison implementations.
The string comparison uses direct comparisons while other methods (numeric, boolean, dateTime, etc.) use OperationFunctions. This inconsistency in implementation approach should be reviewed.
Consider standardizing the comparison approach across all data types:
- Either use direct comparisons consistently
- Or use
OperationFunctionsconsistently
This would make the code more maintainable and easier to understand.
Also applies to: 240-260, 284-304, 328-348, 372-392, 416-436, 460-480
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/impl/OperationFunctions.java (2)
6-6: LGTM! Package reorganization and enhanced error handling.
The move to the 'impl' package and the addition of specific exception types improve the separation of concerns and error handling capabilities.
Also applies to: 8-10
Line range hint 31-39: LGTM! Well-structured utility class.
The class follows best practices for utility classes:
- Marked as final
- Private constructor
- Clear documentation with spec references
- Appropriate use of constants
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/AbstractAstVisitor.java (1)
258-266: LGTM: Function item expression implementation follows visitor pattern
The implementation correctly delegates to child nodes and follows the established visitor pattern in the class.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/ArgumentImpl.java (2)
42-42: Update Signature Representation with toEQName()
Using getName().toEQName() in the toSignature() method ensures that the name is properly formatted as an EQName, enhancing the readability and accuracy of the function signature.
9-9: Enhance Type Safety by Using IEnhancedQName for name
Changing the name field and related methods from String to IEnhancedQName improves type safety and allows for richer manipulation of qualified names. Please ensure that all instances where ArgumentImpl is instantiated or used are updated accordingly to prevent potential type mismatches.
Run the following script to identify affected code:
Also applies to: 17-17, 21-21, 27-27
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/items/FunctionCallAccessor.java (1)
72-74: Ensure getChildren() handles empty arguments list
In the getChildren() method, you're concatenating the base expression with the list of arguments. If arguments is an empty list, this should still function correctly. However, ensure that arguments is never null to avoid potential NullPointerException.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/IArgument.java (2)
27-30: Addition of static of method improves API usability
The new static method of provides a convenient and concise way to create IArgument instances, enhancing the usability of the interface.
38-38: Change of getName return type to IEnhancedQName enhances type safety
Updating the return type of getName from String to IEnhancedQName improves type safety and aligns with the use of qualified names in the codebase.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AbstractCSTVisitorBase.java (1)
157-179: nairyToCollection Method Implementation is Clear and Correct
The nairyToCollection method is well-defined, and the logic is consistent with its intended functionality. The use of generic types and functional interfaces enhances flexibility and readability.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/StaticContext.java (2)
Line range hint 98-100: Verify the handling of default namespace prefix
In lookupPrefixForNamespace, if result is null, the method returns XMLConstants.DEFAULT_NS_PREFIX. Ensure this behavior aligns with your intended logic, especially if an unbound namespace URI should map to the default namespace prefix.
Please confirm that returning XMLConstants.DEFAULT_NS_PREFIX when no prefix is found is the desired behavior.
Line range hint 257-260: Good practice: Preventing redefinition of reserved prefixes
The check to prevent redefinition of the reserved prefix 'meta' ensures compliance with the XPath 3.1 specification and avoids potential conflicts.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/BuildCSTVisitor.java (1)
403-433: Well-implemented handleInlinefunctionexpr method
The handleInlinefunctionexpr method effectively parses inline function expressions, including parameter lists, result types, and function bodies. The use of default sequence types and null checks enhances robustness and aligns with best practices.
core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/AnonymousFunctionCallTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/gov/nist/secauto/metaschema/core/metapath/function/library/CastFunctionTest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/StaticFunctionCall.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/CastFunction.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/AbstractAstVisitor.java
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/IArgument.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/IArgument.java
Outdated
Show resolved
Hide resolved
.../src/main/java/gov/nist/secauto/metaschema/core/metapath/function/impl/AbstractFunction.java
Outdated
Show resolved
Hide resolved
.../src/main/java/gov/nist/secauto/metaschema/core/metapath/function/impl/AbstractFunction.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/BuildCSTVisitor.java
Show resolved
Hide resolved
…lementation for returned instances. Updated all uses to use IMetaschemaExpression. Changed uses of evaluateAs using the ResultType.SEQUENCE to just use evaluate. Removed ResultType.SEQUENCE.
|
@coderabbitai Please update the summary. |
|
@david-waltermire: I will update the summary as per your request. ✅ Actions performedSummary regeneration triggered. |
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/IMetapathExpression.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/cst/AnonymousFunctionCall.java
Outdated
Show resolved
Hide resolved
core/src/test/java/gov/nist/secauto/metaschema/core/metapath/cst/AnonymousFunctionCallTest.java
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/IPrintable.java
Outdated
Show resolved
Hide resolved
dff71d3
into
metaschema-framework:develop
* Feature/anonymous functions (#266) * Hooked up the data type specific cast methods using the data type service as the source of data types. This provides for long-term scalability as new data types are added. * Completed testing eqname and varref cases for the arrow operator. Resolves #68. * Inline functions are now working. Resolves #242. * Refactored Metapath expression to use an interface instead of the implementation for returned instances. Updated all uses to use IMetaschemaExpression. Changed uses of evaluateAs using the ResultType.SEQUENCE to just use evaluate. Removed ResultType.SEQUENCE. * Applied suggestions from code review * Refactored coded exception to store the prefix. Created sub-exceptions for array handling. * Added absent dynamic context sub-exception. * Added sub-exceptions for the DynamicMetapathException. Fixed a bug where root paths were not raising the correct MPDY0050 error when a node was used in the context that did not have a document node ancestor. * Started refactoring the base of the MetapathException tree to have two branches: StaticMetapathError and DynamicMetapathError. StaticMetapathErrors should be limited to compilation, while DynamicMetapathErrors should be limited to execution. * Cleaned up Metapath compilation in constraint implementations. Metapaths are now directly compiled at parse time. * Added support for lazy Metapath compilation. Cleaned up well-known namespace and prefix support, moving this functionality to a new WellKnown class. * Enabled tests that now work with lazy Metapath compilation. * Continued to refactor the Metapath runtime exception hierarchy. Created a RuntimeMetapathError exception class that all runtime exceptions are now children of. Moved function-related exceptions to their own branch in the Metapath runtime error tree. * Package reorganization to align all item related classes in the same package hierarchy. Similar with XML and JSON related classes. * Added coderabbit config.
Committer Notes
Add support for anonymous functions.
This involves:
This approach will support #242 by allowing inline functions to be assigned to variables using the
letsyntax that is supported in Metaschema modules and Meta Constraints.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
New Features
getDataTypes()in the DataTypeService for fetching data type adapters.AnonymousFunctionCallto represent anonymous function calls.asStringItem()inIAnyAtomicIteminterface for converting atomic items to string items.signatureinCastFunctionfor enhanced function signature creation.Improvements
StaticContextwith new lookup methods.DefaultFunctionLibrary.OperationFunctions.Bug Fixes
ComparisonFunctions.Tests
AnonymousFunctionCallandCastFunction.Chores
OperationFunctionsacross multiple classes to reflect new package structure.pom.xml.