add language support for ReplaceInterceptor #2757
Conversation
📝 WalkthroughWalkthroughReplaceInterceptor was refactored to extend AbstractExchangeExpressionInterceptor, replacing jsonPath-specific API with a generic expression API supporting JSONPATH and XPATH. Request/response handling delegates to handleInternal(Exchange, Flow) which dispatches to handleJsonPath or handleXPath; initialization validates language and superclass init is called. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
This pull request needs "/ok-to-test" from an authorized committer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/interceptor/json/ReplaceInterceptor.java (1)
31-40:⚠️ Potential issue | 🔴 CriticalTest failure and breaking API change with outdated documentation.
The test calls
replaceInterceptor.replaceWithJsonPath(msg, jsonPath, replacement)on line 48, but this method doesn't exist in the ReplaceInterceptor class. Either implement this public method or rewrite the test to match the current implementation.Additionally, renaming the setter from
setJsonPathtosetExpressionchanges the XML/YAML attribute name fromjsonPathtoexpression. This is a breaking change for existing configurations:
- The YAML example in the class documentation (lines 31-40) still shows
jsonPath: $.person.name- Configuration examples in
distribution/examples/message-transformation/replace/still use the oldjsonPathattributeEither add a deprecated
setJsonPathmethod that delegates tosetExpressionfor backward compatibility, or update all documentation, examples, and configuration files to use the newexpressionattribute name.
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/ReplaceInterceptor.java`:
- Around line 71-73: The handleXPath method in ReplaceInterceptor is a no-op and
silently ignores XPath replacements; update the method (handleXPath(Message
message, String expr) in class ReplaceInterceptor) to immediately throw an
UnsupportedOperationException (including a clear message that XPath language is
not supported and echoing the expr) so misconfiguration is surfaced; ensure the
exception type and message are consistent with other interceptor errors in the
codebase and remove any silent returns or empty bodies.
- Around line 60-69: The switch in handleInternal currently handles only
JSONPATH and XPATH and silently continues for other values of language; add a
default branch to the switch that logs an error (including the invalid language
value and the evaluated expr via exchangeExpression) and throws an
IllegalArgumentException (or another appropriate runtime exception) so
unsupported languages are surfaced instead of returning CONTINUE; update
handleInternal to use this default and reference existing handlers
handleJsonPath and handleXPath and the CONTINUE return as needed.
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/json/ReplaceInterceptor.java (2)
75-80: No error handling for malformed JSON bodies.
defaultConfiguration().jsonProvider().parse(...)will throw a runtime exception (e.g.,InvalidJsonException) if the body is not valid JSON. Since the content-type check on line 76 only verifies the header (which can be wrong), consider wrapping the parse/set logic in a try-catch to return a meaningful error or log a warning rather than letting the exception propagate unhandled.Proposed fix
private void handleJsonPath(Message msg, String jsonPath) { if (!isJson(msg.getHeader().getContentType())) return; - Object document = defaultConfiguration().jsonProvider().parse(msg.getBodyAsStringDecoded()); - document = JsonPath.parse(document).set(jsonPath, with).json(); - msg.setBodyContent(defaultConfiguration().jsonProvider().toJson(document).getBytes(UTF_8)); + try { + Object document = defaultConfiguration().jsonProvider().parse(msg.getBodyAsStringDecoded()); + document = JsonPath.parse(document).set(jsonPath, with).json(); + msg.setBodyContent(defaultConfiguration().jsonProvider().toJson(document).getBytes(UTF_8)); + } catch (Exception e) { + log.warn("Failed to apply JSONPath replacement '{}': {}", jsonPath, e.getMessage()); + } }
82-90: Javadoc still references "JSONPath expression" — should be updated to be language-agnostic.The method now serves as a generic expression setter (JSONPath, XPath, etc.), so the Javadoc should reflect the broader purpose. The
@paramexample also only shows JSONPath syntax.Proposed fix
/** - * Sets the JSONPath expression to identify the target node in the JSON structure. + * Sets the expression to identify the target node (e.g., JSONPath or XPath). * - * `@param` expr the JSONPath expression (e.g., "$.person.name"). + * `@param` expr the expression (e.g., "$.person.name" for JSONPath). */
| private Outcome handleInternal(Exchange exc, Flow flow) { | ||
| Message msg = exc.getMessage(flow); | ||
| String expr = exchangeExpression.evaluate(exc, flow, String.class); | ||
|
|
||
| switch (language) { | ||
| case JSONPATH -> handleJsonPath(msg, expr); | ||
| case XPATH -> handleXPath(msg, expr); | ||
| } | ||
| return CONTINUE; | ||
| } |
There was a problem hiding this comment.
Switch on language silently ignores unsupported languages.
If language is set to a value other than JSONPATH or XPATH (e.g., SPEL, GROOVY), the switch falls through silently and returns CONTINUE — the message passes through unmodified with no warning. Consider adding a default branch that throws or logs a warning for unsupported languages.
Proposed fix
switch (language) {
case JSONPATH -> handleJsonPath(msg, expr);
case XPATH -> handleXPath(msg, expr);
+ default -> throw new UnsupportedOperationException("Language " + language + " is not supported by ReplaceInterceptor");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private Outcome handleInternal(Exchange exc, Flow flow) { | |
| Message msg = exc.getMessage(flow); | |
| String expr = exchangeExpression.evaluate(exc, flow, String.class); | |
| switch (language) { | |
| case JSONPATH -> handleJsonPath(msg, expr); | |
| case XPATH -> handleXPath(msg, expr); | |
| } | |
| return CONTINUE; | |
| } | |
| private Outcome handleInternal(Exchange exc, Flow flow) { | |
| Message msg = exc.getMessage(flow); | |
| String expr = exchangeExpression.evaluate(exc, flow, String.class); | |
| switch (language) { | |
| case JSONPATH -> handleJsonPath(msg, expr); | |
| case XPATH -> handleXPath(msg, expr); | |
| default -> throw new UnsupportedOperationException("Language " + language + " is not supported by ReplaceInterceptor"); | |
| } | |
| return CONTINUE; | |
| } |
🤖 Prompt for AI Agents
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/ReplaceInterceptor.java`
around lines 60 - 69, The switch in handleInternal currently handles only
JSONPATH and XPATH and silently continues for other values of language; add a
default branch to the switch that logs an error (including the invalid language
value and the evaluated expr via exchangeExpression) and throws an
IllegalArgumentException (or another appropriate runtime exception) so
unsupported languages are surfaced instead of returning CONTINUE; update
handleInternal to use this default and reference existing handlers
handleJsonPath and handleXPath and the CONTINUE return as needed.
| private void handleXPath(Message message, String expr) { | ||
|
|
||
| } |
There was a problem hiding this comment.
handleXPath is an empty no-op — silently drops the replacement.
If a user configures language="xpath", the interceptor will accept it, evaluate the expression, and then do absolutely nothing. At minimum, throw UnsupportedOperationException so the misconfiguration is surfaced immediately rather than silently swallowed.
Proposed fix
private void handleXPath(Message message, String expr) {
-
+ throw new UnsupportedOperationException("XPath is not yet supported by ReplaceInterceptor");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void handleXPath(Message message, String expr) { | |
| } | |
| private void handleXPath(Message message, String expr) { | |
| throw new UnsupportedOperationException("XPath is not yet supported by ReplaceInterceptor"); | |
| } |
🤖 Prompt for AI Agents
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/ReplaceInterceptor.java`
around lines 71 - 73, The handleXPath method in ReplaceInterceptor is a no-op
and silently ignores XPath replacements; update the method (handleXPath(Message
message, String expr) in class ReplaceInterceptor) to immediately throw an
UnsupportedOperationException (including a clear message that XPath language is
not supported and echoing the expr) so misconfiguration is surfaced; ensure the
exception type and message are consistent with other interceptor errors in the
codebase and remove any silent returns or empty bodies.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/ReplaceInterceptor.java`:
- Line 119: In ReplaceInterceptor replace the insecure per-call
TransformerFactory.newInstance() usage (the line creating Transformer t) with
the hardened factory from XMLUtil: obtain the thread-local/hardened factory via
XMLUtil (e.g. XMLUtil.newHardenedBestEffortTransformerFactory() or the
XMLUtil-provided thread-local factory) and call .newTransformer() on that
factory so you get XXE-safe settings and avoid recreating an expensive factory
each call; update the Transformer creation in ReplaceInterceptor accordingly to
use XMLUtil's factory.
🧹 Nitpick comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/json/ReplaceInterceptor.java (3)
126-128: Consider logging replacement errors atwarnlevel instead ofinfo.A failed replacement silently passes the original message through unmodified. Logging at
infomakes this easy to miss in production.warnwould be more appropriate for an operation that was expected to succeed but didn't.
54-56: Javadoc and class description still reference only JSON/JSONPath.The class
@description(Line 55) says "Replaces a JSON value at the configured JSONPath" andsetExpressionJavadoc (Line 146-148) mentions only JSONPath. These should be updated to reflect the dual-language (JSONPath + XPath) support.Also applies to: 145-153
75-79:super.init()creates an unusedExchangeExpression.
AbstractExchangeExpressionInterceptor.init()allocates anExchangeExpressionvianewInstance(this, language, expression, router), buthandleInternaluses the rawexpressionstring andlanguagefield directly—never referencingexchangeExpression. Consider overridinggetExchangeExpression()to returnnullto avoid the unnecessary allocation, or alternatively skip callingsuper.init()if it only serves to set this field.
| } | ||
| } | ||
|
|
||
| Transformer t = TransformerFactory.newInstance().newTransformer(); |
There was a problem hiding this comment.
Use the hardened TransformerFactory from XMLUtil instead of TransformerFactory.newInstance().
TransformerFactory.newInstance() creates an unhardened factory without the XXE protections (secure processing, disabled external DTD/stylesheet/schema access) that XMLUtil.newHardenedBestEffortTransformerFactory() provides. It's also more expensive to create a new factory on every call versus reusing the thread-local one.
Proposed fix
- Transformer t = TransformerFactory.newInstance().newTransformer();
+ Transformer t = XMLUtil.newHardenedBestEffortTransformerFactory().newTransformer();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Transformer t = TransformerFactory.newInstance().newTransformer(); | |
| Transformer t = XMLUtil.newHardenedBestEffortTransformerFactory().newTransformer(); |
🤖 Prompt for AI Agents
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/ReplaceInterceptor.java`
at line 119, In ReplaceInterceptor replace the insecure per-call
TransformerFactory.newInstance() usage (the line creating Transformer t) with
the hardened factory from XMLUtil: obtain the thread-local/hardened factory via
XMLUtil (e.g. XMLUtil.newHardenedBestEffortTransformerFactory() or the
XMLUtil-provided thread-local factory) and call .newTransformer() on that
factory so you get XXE-safe settings and avoid recreating an expensive factory
each call; update the Transformer creation in ReplaceInterceptor accordingly to
use XMLUtil's factory.
1 participant
Summary by CodeRabbit
New Features
Refactor