Xpath builtin function and readme yaml#2485
Conversation
…th support and tests - Removed `BuiltInFunctions` and integrated its logic into `CommonBuiltInFunctions` for better modularity. - Introduced support for XPath evaluation in `CommonBuiltInFunctions` using `javax.xml.xpath`. - Updated SpEL and Groovy built-in functions to include XPath support. - Added comprehensive unit tests for XPath functionality and updated README with usage examples.
…ns and optimize imports in SpEL test
WalkthroughAdds an XPath built-in (fn.xpath) implemented in CommonBuiltInFunctions with Groovy and SpEL bindings and tests; removes a legacy SpEL BuiltInFunctions class; improves expression error handling by producing structured ProblemDetails and replacing ExchangeExpressionException.stacktrace(...) with detail(...). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Groovy as Groovy script
participant SpEL as SpEL evaluator
participant Common as CommonBuiltInFunctions
participant Parser as HardenedXmlParser
participant XPath as XPathEvaluator
Groovy->>Common: xpath(expr, message)
SpEL->>Common: xpath(expr, ctx.getMessage())
Common->>Parser: parse(message.body)
Parser-->>Common: Document
Common->>XPath: evaluate(expr, Document)
alt success
XPath-->>Common: stringValue
Common-->>Groovy: result
Common-->>SpEL: result
else XPathExpressionException
XPath-->>Common: XPathExpressionException
Common-->>Groovy: null
Common-->>SpEL: null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java(4 hunks)core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java(3 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java(0 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctions.java(1 hunks)core/src/test/java/com/predic8/membrane/core/lang/CommonBuiltInFunctionsTest.java(3 hunks)core/src/test/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctionsTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctionsTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java
🧰 Additional context used
🧬 Code graph analysis (4)
core/src/test/java/com/predic8/membrane/core/lang/CommonBuiltInFunctionsTest.java (1)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
CommonBuiltInFunctions(43-174)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (3)
core/src/main/java/com/predic8/membrane/core/exchange/Exchange.java (1)
Exchange(31-220)core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedXmlParser.java (1)
HardenedXmlParser(30-99)core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java (1)
XMLUtil(30-111)
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (1)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
CommonBuiltInFunctions(43-174)
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctions.java (2)
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (1)
SuppressWarnings(29-103)core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
CommonBuiltInFunctions(43-174)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Automated tests
🔇 Additional comments (10)
core/src/test/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctionsTest.java (1)
16-24: LGTM!The test properly validates SpEL XPath extraction by creating an exchange with an XML payload and asserting the expected attribute value.
core/src/test/java/com/predic8/membrane/core/lang/CommonBuiltInFunctionsTest.java (1)
57-61: LGTM!The test validates XPath extraction from XML payloads, following the existing test patterns in this file.
README.md (1)
748-762: LGTM!The documentation clearly demonstrates XPath-based value extraction within templates using the new
fn.xpath()built-in function. The YAML example is concise and practical.core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (2)
29-29: LGTM!The
@SuppressWarnings("unused")annotation is appropriate since these methods are invoked via reflection from Groovy templates.
49-51: LGTM!The
xpathmethod correctly delegates toCommonBuiltInFunctions.xpathand follows the established pattern of other methods in this class.core/src/test/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctionsTest.java (1)
17-25: LGTM!The test properly validates Groovy XPath extraction by creating a GroovyBuiltInFunctions instance with an XML exchange and asserting the expected result.
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctions.java (2)
29-29: LGTM!The
@SuppressWarnings("unused")annotation is appropriate since these methods are registered and invoked by the SpEL BuiltInFunctionResolver.
36-38: LGTM!The
xpathmethod correctly follows the class constraints by being static and properly delegates toCommonBuiltInFunctions.xpath.core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (2)
49-50: LGTM!The static
xPathFactoryandparserfields are properly initialized. UsingHardenedXmlParserensures protection against XXE and other XML-based attacks.
60-74: LGTM!The
xpathimplementation is secure and follows existing patterns:
- Creates a new
XPathinstance per call (thread-safe approach)- Uses
HardenedXmlParserfor secure XML processing- Returns
nullon error, consistent withjsonPathbehavior- TODO comment appropriately notes future namespace support
…d enhance `ExchangeExpressionException` with detailed messages and stacktrace control - Moved redundant error handling logic in `AbstractSetterInterceptor` to a dedicated method for improved readability and maintainability. - Updated `ExchangeExpressionException` to support detailed error messages and enable/disable stacktraces. - Introduced `getBuiltInFunctionNames` in `SpELBuiltInFunctions` to provide a list of supported SpEL functions for use in error messages. - Enhanced logging clarity for expression evaluation errors.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpressionException.java (1)
42-55: Thestacktracefield value is immediately overwritten.Line 46 sets
pd.stacktrace(stacktrace)using the field value, but line 53 unconditionally setspd.stacktrace(false), making the field assignment on line 46 ineffective. This appears to be dead code or a logic error.If the intent is to always disable stacktrace output, consider removing line 46 and the
stacktracefield entirely. Otherwise, remove line 53 to respect the field value.🔎 Proposed fix (if intent is to always disable)
public ProblemDetails provideDetails(ProblemDetails pd) { if (detail != null) pd.detail(detail); pd.internal("expression", expression) - .stacktrace(stacktrace); + .stacktrace(false); for (Map.Entry<String, Object> entry : extensions.entrySet()) { pd.internal(entry.getKey(), entry.getValue()); } if (body != null) pd.internal("body", body.length() > 1024 ? body.substring(0, 1024) : body); pd.exception(this); - pd.stacktrace(false); return pd; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractSetterInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpressionException.java(4 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctions.java(3 hunks)core/src/test/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpressionTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractSetterInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java (1)
ExceptionUtil(16-39)
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctions.java (2)
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (1)
SuppressWarnings(29-103)core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
CommonBuiltInFunctions(43-174)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (14)
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java (2)
57-58: LGTM! Good centralization of the development mode warning.Extracting this message into a constant improves maintainability and enables consistent reuse across the codebase.
305-305: LGTM!Correctly uses the new constant.
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctions.java (2)
41-43: LGTM! Correct implementation of XPath delegation.The method follows the established pattern: static, takes SpELExchangeEvaluationContext as last parameter, and delegates to CommonBuiltInFunctions. This is consistent with the other built-in functions in this class.
97-111: LGTM! Clean reflection-based function discovery.The implementation correctly filters for public static methods with
SpELExchangeEvaluationContextas the last parameter, matching the constraints documented in the class Javadoc. Usingdistinct()appropriately handles overloaded methods likescopesandhasScope.core/src/test/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpressionTest.java (1)
212-225: LGTM! Good test coverage for unknown built-in function error handling.The test effectively validates that:
- An
ExchangeExpressionExceptionis thrown for unknown functionsProblemDetailsis properly populated with the expression- The error message includes helpful context (the unknown expression, "Method not found", and available built-ins like
isBearerAuthorization)This ensures users receive actionable feedback when they mistype or use non-existent functions.
core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpressionException.java (2)
24-26: LGTM! Good addition of thedetailfield.The new field enables richer error messages to be propagated through
ProblemDetails.
57-60: LGTM!Fluent setter follows the established pattern used by other methods in this class.
core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java (1)
86-99: LGTM! Excellent improvement to error messaging.The multi-branch error handling provides actionable user feedback:
- For unknown methods, it lists available Membrane built-in functions
- For type conversion failures, it includes source and target types
- All errors are logged as warnings with the detailed message
This significantly improves the developer experience when debugging SpEL expressions.
core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractSetterInterceptor.java (6)
18-18: LGTM! Necessary imports for enhanced error handling.The new imports support the structured error handling improvements with ProblemDetails and ExchangeExpressionException.
Also applies to: 21-21
56-56: LGTM! Improved error message clarity.The formatted error message provides clear context by including both the expression and field name.
64-67: LGTM! Proper root cause extraction and error response.The use of
ExceptionUtil.getRootCause(e)ensures the underlying cause is captured, and.stacktrace(false)appropriately prevents exposing internal implementation details in the error response.
70-70: LGTM! Consistent and informative logging.The log message appropriately uses the same formatted message for consistency, and the
infolevel is suitable for this non-failure scenario.
75-80: LGTM! Well-structured error details preparation.The
prepareProblemDetailsmethod effectively centralizes the creation of standardized ProblemDetails with appropriate context (field name and expression), enabling consistent error reporting across different exception types.
58-63: No issues found. TheprovideDetailsmethod onExchangeExpressionExceptioncorrectly accepts aProblemDetailsparameter and is used properly. Both exception handlers ensure.stacktrace(false)is called consistently—theExchangeExpressionExceptionbranch does so inside theprovideDetails()method (line 53 of ExchangeExpressionException.java), while the general handler calls it directly.
… `ExchangeExpressionException` - Eliminated unused `stacktrace` field and its associated methods. - Updated `ExchangeExpressionException` and related classes to streamline error reporting. - Fixed minor wording in README example configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
748-762: Consider adding example input XML for clarity.The YAML configuration demonstrates XPath extraction well, but users may benefit from seeing the expected XML input format. For consistency with other transformation examples (e.g., lines 693-697 show the curl command with JSON input), consider adding a sample curl command with XML input.
Example input to add
Call this API with: ```bash curl -d '<person firstname="Maria"/>' -H "Content-Type: application/xml" "http://localhost:2000"</details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between e1e4543d27b36caeb8c5d924faf32c1fdd9e8ad8 and fea0b91a7101090311ce94c31f1c88010d0fa169. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `README.md` (2 hunks) * `core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java` (2 hunks) * `core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpressionException.java` (3 hunks) * `core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java` (1 hunks) * `core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpressionException.java </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)</summary> * GitHub Check: Automated tests * GitHub Check: Analyze (java) </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java (2)</summary><blockquote> `57-59`: **LGTM! Good refactoring.** Extracting the development mode warning into a named constant improves maintainability and makes the message reusable across the codebase. --- `302-302`: **LGTM!** The constant is properly applied here, maintaining the same functionality with better code organization. </blockquote></details> <details> <summary>core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (1)</summary><blockquote> `65-68`: **Stacktrace handling is centralized in ExchangeExpressionException.provideDetails().** The concern about stacktrace inclusion is based on a misunderstanding. Stacktrace visibility is controlled by the `provideDetails()` method in ExchangeExpressionException, which always calls `pd.stacktrace(false)` before returning the error response. This applies to all expression evaluation errors regardless of whether `.stacktrace(false)` is called in individual catch blocks. The removal aligns XPath error handling with JsonpathExchangeExpression, SpELExchangeExpression, and other evaluators, none of which explicitly suppress stacktraces in their catch blocks. > Likely an incorrect or invalid review comment. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…handling for improved clarity - Used `METHOD_NOT_FOUND` constant instead of string matching for detecting method-not-found errors in SpEL expressions.
|
tests are failing? |
|
Yes. The tests should also fail on master. The reason is: OAuth2ExampleTest because of: |
|
@rrayst that's the for more than one router |
|
This pull request needs "/ok-to-test" from an authorized committer. |
3 participants
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / UX
Tests
Chore
✏️ Tip: You can customize this high-level summary in your review settings.