XML, JSON, TEXT Serialization#2853
Merged
Merged
Conversation
…nhance XPath integration - Updated tests and examples to reflect new serialization logic. - Enhanced expression handling for structured data (e.g., XML, JSON) in `TemplateExchangeExpression` and interceptors. - Refactored `XPath` examples, aligning error handling and template usage patterns.
… improve test clarity
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRefactors escaping into a serialization model across core and tests: adds Changes
Sequence Diagram(s)sequenceDiagram
participant Interceptor as Interceptor
participant Parser as TemplateExpressionParser
participant Template as TemplateExchangeExpression
participant Scripting as ScriptingUtils
participant Serializer as SerializationFunction
Interceptor->>Template: newInstance(..., serialization=TEXT_SERIALIZATION)
Template->>Parser: parseTokens(language, expression)
Parser-->>Template: List<Token>
Interceptor->>Template: evaluate(exchange)
Template->>Scripting: evaluate token expressions -> Object results
Scripting-->>Template: Objects
Template->>Serializer: apply(Object) for non-text tokens
Serializer-->>Template: String
Template-->>Interceptor: concatenated result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
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/proxies/Target.java (1)
241-247:⚠️ Potential issue | 🟡 Minor
setEscapingJavadoc is stale after enum migration.Line 241 still documents
NONE, URL, SEGMENT, butSerializationnow includesTEXT, URL, SEGMENT, JSON, XML. This will mislead generated/config docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/proxies/Target.java` around lines 241 - 247, Update the stale Javadoc on Target.setEscaping to reflect the current Serialization enum values and default: replace the old "NONE, URL, SEGMENT" list with the full set "TEXT, URL, SEGMENT, JSON, XML" and ensure the `@description` and `@default` lines match the current behavior (e.g., default URL if that remains true); locate the setter method setEscaping(Serialization escaping) in class Target and adjust the comment text so generated/config docs show the correct options and default.
🧹 Nitpick comments (9)
core/src/test/java/com/predic8/membrane/core/interceptor/BodyDoesntThrowIOExceptionTest.java (1)
25-25: Move@SuppressWarningsfrom class level to the specific@ArchTestfield.The suppression at class level can mask unrelated future warnings. Since this test class contains only one
@ArchTestfield that triggers the "unused" warning, move the annotation directly to that field:`@ArchTest` `@SuppressWarnings`("unused") static final ArchRule bodyMethodsShouldNotThrowIOException = ...Alternatively, remove the suppression entirely to align with the pattern used in similar ArchUnit tests like
ThrowablePrintStackTraceTestandInterceptorInitADRTest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/BodyDoesntThrowIOExceptionTest.java` at line 25, Move the class-level `@SuppressWarnings`("unused") from BodyDoesntThrowIOExceptionTest to the specific ArchUnit test field: apply `@SuppressWarnings`("unused") directly to the `@ArchTest` field bodyMethodsShouldNotThrowIOException (or remove the suppression entirely to match other tests like ThrowablePrintStackTraceTest); ensure the `@ArchTest` annotation remains on the field and that only that field carries the suppression so unrelated warnings aren't masked.core/src/test/java/com/predic8/membrane/core/util/xml/XMLTestUtil.java (1)
28-30: Harden parser configuration to prevent XXE attacks and external entity resolution.The default
DocumentBuilderFactoryallows DTD and external entity processing, which can enable XXE vulnerabilities and cause flaky tests when external resources are unexpectedly fetched. Add explicit security features to disable DOCTYPE declarations, block external entities, and prevent external resource access.Proposed change
+import javax.xml.XMLConstants; import javax.xml.parsers.*; import java.io.*; @@ public static Document parse(String xml) throws Exception { - return DocumentBuilderFactory.newInstance().newDocumentBuilder() - .parse(new InputSource(new StringReader(xml))); + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); + factory.setXIncludeAware(false); + factory.setExpandEntityReferences(false); + + return factory.newDocumentBuilder() + .parse(new InputSource(new StringReader(xml))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/util/xml/XMLTestUtil.java` around lines 28 - 30, The parse(String xml) helper uses a raw DocumentBuilderFactory which permits DOCTYPE/external-entity processing; update XMLTestUtil.parse to harden the factory before creating the builder by enabling secure processing and disabling DOCTYPE/external entities: set XMLConstants.FEATURE_SECURE_PROCESSING=true, set "http://apache.org/xml/features/disallow-doctype-decl"=true, set "http://xml.org/sax/features/external-general-entities"=false and "http://xml.org/sax/features/external-parameter-entities"=false, and set the properties "http://javax.xml.XMLConstants/property/accessExternalDTD" and "http://javax.xml.XMLConstants/property/accessExternalSchema" to empty strings; additionally set a no-op EntityResolver on the DocumentBuilder to block any remaining external resolution before calling parse.distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java (1)
88-89: Assert the response media type for this JSON-path assertion.At Line [88]-Line [89], field-path checks are good, but adding
.contentType(JSON)will lock the error format contract and avoid silent format drift.Suggested test hardening
.then() .statusCode(500) + .contentType(JSON) .body("error.case", equalTo("d")) .body("error.description", equalTo("XML error at the backend!"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java` around lines 88 - 89, In CustomErrorHandlingExampleTest add an assertion to lock the response media type before the JSON-path body checks: call the response contentType(JSON) (or equivalent matcher) immediately before the .body("error.case", equalTo("d")) and .body("error.description", equalTo("XML error at the backend!")) assertions so the test verifies the response is JSON and prevents silent format drift.core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptorTest.java (1)
142-153: Use XML-oriented request construction in the XPath test input.At Line [142], using
.json(...)with an XML body weakens test intent and can hide content-type related regressions. Prefer constructing this input explicitly as XML.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptorTest.java` around lines 142 - 153, The test constructs an XML payload using post("/foo").json(...), which misrepresents the content type; update the test in SetBodyInterceptorTest to create the request explicitly as XML (e.g., replace json("""...""") with xml("""...""") or call json(...) plus setHeader("Content-Type","application/xml") if an xml(...) helper is unavailable) so the exchange built by post("/foo").xml(...).buildExchange() carries an application/xml content type and accurately exercises XML/XPath behavior.distribution/src/test/java/com/predic8/membrane/tutorials/xml/XPathTutorialTest.java (1)
51-52: Remove stale commented assertion.Leaving the commented header assertion in committed test code makes intent unclear.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/src/test/java/com/predic8/membrane/tutorials/xml/XPathTutorialTest.java` around lines 51 - 52, Remove the stale commented assertion in the test: delete the commented .header("X-Sunny","Sunny is a parrot") line inside XPathTutorialTest (the fluent assertion chain around .body(allOf(...))). Keep the active assertions intact so the test intent is clear.core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java (1)
94-96: Align fallback warning text with serialization terminology.The warning still says “Turning off escaping”, but this path now falls back to
TEXT_SERIALIZATION. Updating wording will reduce operator confusion in logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java` around lines 94 - 96, The log.warn message in SetBodyInterceptor where getSerializationFunction(contentType).orElseGet(...) falls back to TEXT_SERIALIZATION should be reworded to use serialization terminology: change the message to indicate a fallback to TEXT_SERIALIZATION (or to a plain-text serializer) rather than saying "Turning off escaping", and include contentType and guidance to set 'contentType' on setBody; update the log.warn call in the orElseGet block accordingly.core/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.java (1)
133-146: Add regression test for backslash-escaped single quotes.Current coverage misses cases like
${'it\\'s {fine}'}. Adding this test will protect the parser fix and prevent regressions.💡 Suggested test addition
+ `@Test` + void supportsSingleQuoteBackslashEscaping() { + var tokens = parseTokens(interceptor, GROOVY, "x ${'it\\'s {fine}'} y"); + + assertEquals(3, tokens.size()); + assertEquals("x ", tokens.get(0).getExpression()); + assertEquals("'it\\'s {fine}'", tokens.get(1).getExpression()); + assertEquals(" y", tokens.get(2).getExpression()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.java` around lines 133 - 146, Add a regression test covering backslash-escaped single quotes in SpEL by extending or adding a test near supportsSpelSingleQuoteEscapingWithDoubleSingleQuote: call parseTokens(interceptor, SPEL, "x ${'it\\'s {fine}'} y") and assert the tokens length and expressions match ("x ", "'it\\'s {fine}'", " y") to ensure parseTokens and the TemplateExchangeExpression.Token handling correctly accept backslash-escaped single quotes as literal characters.core/src/main/java/com/predic8/membrane/core/lang/TemplateExpressionParser.java (1)
25-26: Make utility class non-instantiable.
TemplateExpressionParseris static-only; add a private constructor to prevent accidental instantiation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/lang/TemplateExpressionParser.java` around lines 25 - 26, TemplateExpressionParser is a static-only utility class and must be made non-instantiable: add a private no-argument constructor to the TemplateExpressionParser class (e.g., private constructor that throws an AssertionError or IllegalStateException) so any attempt to instantiate TemplateExpressionParser fails at compile/runtime; update the class declaration containing static parsing methods to include this private constructor.core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java (1)
68-73: UseToken.getExpression()for error context.
Token.getExpression()was added, but exception wrapping still usestoString(). UsinggetExpression()keeps diagnostics stable and user-facing.Suggested fix
- throw new ExchangeExpressionException(tokens.getFirst().toString(), e); + throw new ExchangeExpressionException(tokens.getFirst().getExpression(), e); ... - throw new ExchangeExpressionException(token.toString(), e); + throw new ExchangeExpressionException(token.getExpression(), e);Also applies to: 93-95, 100-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java` around lines 68 - 73, Replace usages of tokens.getFirst().toString() when constructing ExchangeExpressionException with tokens.getFirst().getExpression() to provide stable user-facing diagnostics; update this in TemplateExchangeExpression (notably in the evaluateSingle method and the other catch blocks around the ranges indicated) so each catch wraps the original exception using tokens.getFirst().getExpression() as the message/context for ExchangeExpressionException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java`:
- Around line 74-76: The NodeList branch in CommonBuiltInFunctions currently
returns nodeListToString(nl, ",") which produces raw CSV-like text instead of
valid JSON; change this branch to serialize the NodeList as a JSON array string
by mapping each Node in the NodeList to its JSON-safe string representation
(escape/quote each value) and returning a JSON array (e.g. ["val1","val2",...])
so callers of toJSON always get valid JSON; use the existing node-to-string
helper the class provides (or add a small helper) to convert each Node, then
assemble and return the JSON array string instead of calling nodeListToString.
In
`@core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java`:
- Around line 89-92: The current check in TemplateExchangeExpression that
coerces a Node to its textContent before calling encoder.apply (the lines using
"if (value instanceof Node n) { value = n.getTextContent(); }" and then
"line.append(encoder.apply(value));") must be removed so we don't strip XML
structure; instead pass the original Node instance to encoder.apply and let the
SerializationFunction implementations handle Node-to-text conversion; if you
need text-only behavior implement it inside the text serializer (or a dedicated
SerializationFunction) rather than in TemplateExchangeExpression, and update any
tests that assumed text coercion accordingly.
In
`@core/src/main/java/com/predic8/membrane/core/lang/TemplateExpressionParser.java`:
- Around line 89-97: In TemplateExpressionParser.java, inside the inSingle
handling block of the parse method (the code that currently only handles
SpEL-style '' escapes), add a backslash-escape check (same approach used for
double-quoted strings) before treating a single quote as closing the string;
i.e., if the previous character is a backslash (or current char is backslash and
you handle escape consumption) treat the next quote as escaped and skip it
instead of ending the single-quoted literal, ensuring expressions like ${'it\'s
{ok}'} are parsed correctly and do not leave unpaired braces outside the string.
In
`@core/src/main/java/com/predic8/membrane/core/util/text/SerializationFunction.java`:
- Line 33: SerializationFunction URL_SERIALIZATION currently references
ToURLSerializer::toURL which can call o.toString() and throw NPE for null
inputs; change the binding to a null-safe implementation by replacing the method
reference with a lambda that checks for null (e.g. returns a safe representation
such as "null" or empty string) before delegating to ToURLSerializer.toURL,
referencing the symbols SerializationFunction, URL_SERIALIZATION and
ToURLSerializer::toURL so the serialization no longer throws on null.
In
`@core/src/main/java/com/predic8/membrane/core/util/text/SerializationUtil.java`:
- Around line 30-40: Update the Javadoc for the Serialization enum so the list
of strategies matches the current enum values: replace references to the removed
NONE constant and document the actual members TEXT, URL, SEGMENT, JSON, and XML;
for each entry describe its behavior (e.g., TEXT = no escaping / returned as-is,
URL = URL-encoding, SEGMENT = RFC 3986-safe path segment encoding, JSON = JSON
string escaping, XML = XML 1.1 serialization) and keep the existing style and
brief examples where present. Ensure you reference the enum name Serialization
and the exact constants TEXT, URL, SEGMENT, JSON, XML so the doc stays in sync
with the code.
- Around line 50-57: The method getSerializationFunction(String mimeType) only
maps JSON/XML; add a branch to detect text/* MIME types (use an existing
isText(mimeType) helper or implement one) and return
Optional.of(getSerializationFunction(TEXT)) so plain text responses map to the
TEXT serialization; update getSerializationFunction to call isText(mimeType)
before returning Optional.empty() and reference Serialization.TEXT (or
TEXT_SERIALIZATION) via getSerializationFunction(TEXT).
In `@core/src/main/java/com/predic8/membrane/core/util/text/ToURLSerializer.java`:
- Around line 28-35: The toURL method can NPE when o is null because the
fallback calls o.toString(); change the fallback to use
SerializationUtil.identity(o) so nulls are handled safely (as done for the
String case), i.e. update the return path in toURL to call
encode(SerializationUtil.identity(o), UTF_8) instead of encode(o.toString(),
UTF_8); keep existing handling for String and NodeList (NodeList nl and
XMLTextUtil.nodeListToString) and continue using encode(...) as before.
In `@core/src/main/java/com/predic8/membrane/core/util/text/ToXMLSerializer.java`:
- Around line 28-35: The current ToXMLSerializer.toXML method returns raw String
values unescaped (case String s -> s), allowing XML-breaking characters; change
the String branch to return an escaped XML-safe string (e.g., use a new or
existing utility method escapeXml(String) and call case String s ->
escapeXml(s)), and add an escapeXml helper in ToXMLSerializer that replaces & <
> " ' (or delegate to a shared XML-escaping utility if one exists). Keep the
existing nodeToString(Node) and nodeListToString(NodeList) branches unchanged.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptorSpELTest.java`:
- Around line 155-166: The tests in SetHeaderInterceptorSpELTest check the wrong
header key: both assertNull(super.getHeader("x-FoO")) calls in the
failOnErrorTrue and failOnErrorFalse test cases should assert the evaluated
header key "x-bar" instead; update those assertNull calls to use "x-bar" so the
assertions validate the actual header the interceptor would write.
In
`@core/src/test/java/com/predic8/membrane/core/lang/xpath/SetPropertyInterceptorXPathTest.java`:
- Line 19: Remove the unused wildcard import
com.predic8.membrane.core.interceptor.* from
SetPropertyInterceptorXPathTest.java; keep the existing specific imports (the
static import of Outcome and the SetPropertyInterceptor from the .lang
subpackage) so only necessary imports remain and the unused wildcard is deleted.
In
`@distribution/src/test/java/com/predic8/membrane/tutorials/xml/XPathTutorialTest.java`:
- Around line 35-64: The test XPathTutorialTest mutates global JVM state by
calling synchronized(System.out) and System.setOut to capture output, which can
cause flakiness with background Membrane processes or parallel tests; replace
this global stdout redirection with a scoped capture utility (e.g., a
JUnit/Testcontainers-friendly output capture or a dedicated helper) that
captures only the test-thread's output or the process's logs without calling
System.setOut; modify the test to call that scoped capture around the
request/response assertions and then assert on the captured string (replace
usages of synchronized(System.out), System.setOut, the ByteArrayOutputStream
variable and out.toString() checks with the capture utility), ensuring
compatibility with the Membrane process started in `@BeforeEach`.
---
Outside diff comments:
In `@core/src/main/java/com/predic8/membrane/core/proxies/Target.java`:
- Around line 241-247: Update the stale Javadoc on Target.setEscaping to reflect
the current Serialization enum values and default: replace the old "NONE, URL,
SEGMENT" list with the full set "TEXT, URL, SEGMENT, JSON, XML" and ensure the
`@description` and `@default` lines match the current behavior (e.g., default URL if
that remains true); locate the setter method setEscaping(Serialization escaping)
in class Target and adjust the comment text so generated/config docs show the
correct options and default.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java`:
- Around line 94-96: The log.warn message in SetBodyInterceptor where
getSerializationFunction(contentType).orElseGet(...) falls back to
TEXT_SERIALIZATION should be reworded to use serialization terminology: change
the message to indicate a fallback to TEXT_SERIALIZATION (or to a plain-text
serializer) rather than saying "Turning off escaping", and include contentType
and guidance to set 'contentType' on setBody; update the log.warn call in the
orElseGet block accordingly.
In
`@core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java`:
- Around line 68-73: Replace usages of tokens.getFirst().toString() when
constructing ExchangeExpressionException with tokens.getFirst().getExpression()
to provide stable user-facing diagnostics; update this in
TemplateExchangeExpression (notably in the evaluateSingle method and the other
catch blocks around the ranges indicated) so each catch wraps the original
exception using tokens.getFirst().getExpression() as the message/context for
ExchangeExpressionException.
In
`@core/src/main/java/com/predic8/membrane/core/lang/TemplateExpressionParser.java`:
- Around line 25-26: TemplateExpressionParser is a static-only utility class and
must be made non-instantiable: add a private no-argument constructor to the
TemplateExpressionParser class (e.g., private constructor that throws an
AssertionError or IllegalStateException) so any attempt to instantiate
TemplateExpressionParser fails at compile/runtime; update the class declaration
containing static parsing methods to include this private constructor.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/BodyDoesntThrowIOExceptionTest.java`:
- Line 25: Move the class-level `@SuppressWarnings`("unused") from
BodyDoesntThrowIOExceptionTest to the specific ArchUnit test field: apply
`@SuppressWarnings`("unused") directly to the `@ArchTest` field
bodyMethodsShouldNotThrowIOException (or remove the suppression entirely to
match other tests like ThrowablePrintStackTraceTest); ensure the `@ArchTest`
annotation remains on the field and that only that field carries the suppression
so unrelated warnings aren't masked.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptorTest.java`:
- Around line 142-153: The test constructs an XML payload using
post("/foo").json(...), which misrepresents the content type; update the test in
SetBodyInterceptorTest to create the request explicitly as XML (e.g., replace
json("""...""") with xml("""...""") or call json(...) plus
setHeader("Content-Type","application/xml") if an xml(...) helper is
unavailable) so the exchange built by post("/foo").xml(...).buildExchange()
carries an application/xml content type and accurately exercises XML/XPath
behavior.
In
`@core/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.java`:
- Around line 133-146: Add a regression test covering backslash-escaped single
quotes in SpEL by extending or adding a test near
supportsSpelSingleQuoteEscapingWithDoubleSingleQuote: call
parseTokens(interceptor, SPEL, "x ${'it\\'s {fine}'} y") and assert the tokens
length and expressions match ("x ", "'it\\'s {fine}'", " y") to ensure
parseTokens and the TemplateExchangeExpression.Token handling correctly accept
backslash-escaped single quotes as literal characters.
In `@core/src/test/java/com/predic8/membrane/core/util/xml/XMLTestUtil.java`:
- Around line 28-30: The parse(String xml) helper uses a raw
DocumentBuilderFactory which permits DOCTYPE/external-entity processing; update
XMLTestUtil.parse to harden the factory before creating the builder by enabling
secure processing and disabling DOCTYPE/external entities: set
XMLConstants.FEATURE_SECURE_PROCESSING=true, set
"http://apache.org/xml/features/disallow-doctype-decl"=true, set
"http://xml.org/sax/features/external-general-entities"=false and
"http://xml.org/sax/features/external-parameter-entities"=false, and set the
properties "http://javax.xml.XMLConstants/property/accessExternalDTD" and
"http://javax.xml.XMLConstants/property/accessExternalSchema" to empty strings;
additionally set a no-op EntityResolver on the DocumentBuilder to block any
remaining external resolution before calling parse.
In
`@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java`:
- Around line 88-89: In CustomErrorHandlingExampleTest add an assertion to lock
the response media type before the JSON-path body checks: call the response
contentType(JSON) (or equivalent matcher) immediately before the
.body("error.case", equalTo("d")) and .body("error.description", equalTo("XML
error at the backend!")) assertions so the test verifies the response is JSON
and prevents silent format drift.
In
`@distribution/src/test/java/com/predic8/membrane/tutorials/xml/XPathTutorialTest.java`:
- Around line 51-52: Remove the stale commented assertion in the test: delete
the commented .header("X-Sunny","Sunny is a parrot") line inside
XPathTutorialTest (the fluent assertion chain around .body(allOf(...))). Keep
the active assertions intact so the test intent is clear.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractExchangeExpressionInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/log/LogInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.javacore/src/main/java/com/predic8/membrane/core/lang/AbstractScriptInterceptor.javacore/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.javacore/src/main/java/com/predic8/membrane/core/lang/ExchangeExpressionException.javacore/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.javacore/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.javacore/src/main/java/com/predic8/membrane/core/lang/TemplateExpressionParser.javacore/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyExchangeExpression.javacore/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.javacore/src/main/java/com/predic8/membrane/core/proxies/Target.javacore/src/main/java/com/predic8/membrane/core/util/text/SerializationFunction.javacore/src/main/java/com/predic8/membrane/core/util/text/SerializationUtil.javacore/src/main/java/com/predic8/membrane/core/util/text/ToTextSerializer.javacore/src/main/java/com/predic8/membrane/core/util/text/ToURLSerializer.javacore/src/main/java/com/predic8/membrane/core/util/text/ToXMLSerializer.javacore/src/main/java/com/predic8/membrane/core/util/xml/XMLTextUtil.javacore/src/test/java/com/predic8/membrane/core/interceptor/BodyDoesntThrowIOExceptionTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptorSpELTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/templating/GroovyTemplateEscapingTest.javacore/src/test/java/com/predic8/membrane/core/lang/TemplateExchangeExpressionTest.javacore/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.javacore/src/test/java/com/predic8/membrane/core/lang/xpath/SetPropertyInterceptorXPathTest.javacore/src/test/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpressionTest.javacore/src/test/java/com/predic8/membrane/core/proxies/TargetTest.javacore/src/test/java/com/predic8/membrane/core/util/URLUtilTest.javacore/src/test/java/com/predic8/membrane/core/util/xml/XMLTestUtil.javadistribution/examples/extending-membrane/error-handling/custom-error-messages/apis.yamldistribution/examples/templating/json/apis.yamldistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/transformation/GetToPostTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/xml/XPathTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/xml/XmlToJsonTutorialTest.javadistribution/tutorials/transformation/20-GET-to-POST.yamldistribution/tutorials/xml/30-XPath.yamldistribution/tutorials/xml/animals.xml
- Unified serialization handling across interceptors using `SerializationUtil`. - Replaced legacy escaping functions with modern serialization functions. - Enhanced `TemplateExpressionParser` tests for clarity and coverage. - Introduced new unit tests for `ToURLSerializer` and `ToXMLSerializer`.
…andling - Adjusted test expectations to use XML content type instead of JSON in `CustomErrorHandlingExampleTest`. - Refined `SerializationUtil` by improving enum documentation and removing redundant methods. - Added copyright headers to serializer test files. - Simplified `SetHeaderInterceptorSpELTest` by removing unnecessary `Exception` declarations.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
distribution/src/test/java/com/predic8/membrane/tutorials/xml/XPathTutorialTest.java (1)
35-63:⚠️ Potential issue | 🟠 MajorAvoid JVM-global
System.outrebinding in this test.Line 35 through Line 63 still mutates global stdout (
System.setOut) during test execution, which can introduce flakiness with concurrent/background output and parallel execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/src/test/java/com/predic8/membrane/tutorials/xml/XPathTutorialTest.java` around lines 35 - 63, The test mutates JVM-global System.out (System.setOut) which is unsafe for parallel tests; remove the ByteArrayOutputStream/System.setOut block in XPathTutorialTest and instead capture what you need directly from the HTTP response or RestAssured logging: call Response response = given()...when().post("http://localhost:2000").then().extract().response() (or use a RestAssured logging filter) and assert on response.asString() (replace console.contains checks) or use a test-specific logger injection if you must inspect server logs; remove references to out, original, System.setOut, and console and assert against the response content or injected logger.core/src/main/java/com/predic8/membrane/core/lang/TemplateExpressionParser.java (1)
92-101:⚠️ Potential issue | 🟠 MajorHandle backslash escapes in single-quoted literals.
Line 93 currently treats
'as closing unless doubled (''), but does not support backslash-escaped characters. This can prematurely end single-quoted literals and break brace matching for expressions like${'it\'s {ok}'}.Proposed fix
if (inSingle) { + if (c == '\\' && i + 1 < s.length()) { + i++; // skip escaped char in single-quoted literal + continue; + } if (c == '\'') { // SpEL style escaping: '' inside single-quoted string if (i + 1 < s.length() && s.charAt(i + 1) == '\'') { i++; // skip the escaped quote } else {#!/bin/bash set -euo pipefail PARSER_FILE="core/src/main/java/com/predic8/membrane/core/lang/TemplateExpressionParser.java" TEST_FILE="core/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.java" echo "Inspecting single-quote parsing block (expected to include backslash handling):" nl -ba "$PARSER_FILE" | sed -n '88,112p' echo echo "Checking whether inSingle block contains backslash-escape logic..." python - <<'PY' from pathlib import Path import re p = Path("core/src/main/java/com/predic8/membrane/core/lang/TemplateExpressionParser.java") s = p.read_text() m = re.search(r'if \(inSingle\) \{(?P<body>.*?)\n\s*\}\n\s*continue;', s, re.S) if not m: print("inSingle block not found") raise SystemExit(1) body = m.group('body') print("contains `c == '\\\\'` check:", "c == '\\\\'" in body) PY echo echo "Checking test coverage for backslash-escaped single quote examples:" rg -n --fixed-strings "it\\'s" "$TEST_FILE" || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/lang/TemplateExpressionParser.java` around lines 92 - 101, The single-quote parsing in TemplateExpressionParser incorrectly ends single-quoted literals on a backslash-escaped quote; update the inSingle handling so that if c == '\\' and i+1 < s.length() you increment i to skip the escaped character (preserving the escape) before any quote-check, and only treat a single quote as closing when it's not escaped (still keep the existing SpEL double-single-quote handling). Locate the inSingle block in TemplateExpressionParser (variables s, i, c, inSingle) and add the backslash-skip branch before or as part of the c == '\'' logic; also ensure brace-matching tests in TemplateExpressionParserTest include a case like ${'it\\'s {ok}'} to verify the fix.
🧹 Nitpick comments (7)
core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptorSpELTest.java (2)
59-65: Fix test name typo (Singe→Single).Small readability cleanup in method names.
✏️ Proposed rename
- void computeSingeExpression() { + void computeSingleExpression() { assertEquals("24", setHeaderEvalGetValue("${1*2*3*4}")); } - void computeSingeExpressionWithCtx() { + void computeSingleExpressionWithCtx() { assertEquals("Panama", setHeaderEvalGetValue("${property.bar}")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptorSpELTest.java` around lines 59 - 65, Rename the misnamed test method computeSingeExpression to computeSingleExpression to fix the typo; update the method declaration in SetHeaderInterceptorSpELTest (and any references) so the test name is spelled "Single" consistently (leave the test body and assertion setHeaderEvalGetValue("${1*2*3*4}") unchanged).
151-166: Make failOnError tests independent of fixture defaults.Set the header field name explicitly in each test to avoid hidden coupling to superclass setup.
🧪 Proposed hardening
void failOnErrorTrue() { + interceptor.setFieldName("x-bar"); interceptor.setValue("42${wrong}"); interceptor.setFailOnError(true); interceptor.init(router); assertEquals(ABORT, interceptor.handleRequest(exchange)); assertNull(super.getHeader("x-bar")); } `@Test` void failOnErrorFalse() { + interceptor.setFieldName("x-bar"); interceptor.setValue("42${wrong}"); interceptor.setFailOnError(false); interceptor.init(router); assertEquals(CONTINUE, interceptor.handleRequest(exchange)); assertNull(super.getHeader("x-bar")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptorSpELTest.java` around lines 151 - 166, The tests failOnErrorTrue and failOnErrorFalse rely on a superclass fixture for the target header name; make them independent by explicitly setting the header name on the interceptor in each test (e.g., call interceptor.setHeaderName("x-bar") before interceptor.init(router) in both failOnErrorTrue and failOnErrorFalse). This ensures the assertions using super.getHeader("x-bar") don't depend on fixture defaults; update both test methods in SetHeaderInterceptorSpELTest accordingly.core/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.java (1)
120-168: Add a regression case for backslash-escaped single quotes.Please add a test for input like
x ${'it\\'s {ok}'} yto lock parser behavior for escaped quotes in single-quoted literals (Line 120 onward currently covers only doubled-single-quote escaping).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.java` around lines 120 - 168, Add a regression test in TemplateExpressionParserTest that verifies backslash-escaped single quotes inside a SpEL single-quoted literal are treated as part of the literal: create a new `@Test` (similar to supportsSpelSingleQuoteEscapingWithDoubleSingleQuote and doubleSingleQuoteEscaping) that calls parseTokens(interceptor, SPEL, "x ${'it\\'s {ok}'} y") and asserts tokens.size() == 3 and tokens.get(0).getExpression().equals("x "), tokens.get(1).getExpression().equals("'it\\'s {ok}'"), tokens.get(2).getExpression().equals(" y") to lock the parser behavior for backslash-escaped single quotes.core/src/test/java/com/predic8/membrane/core/util/text/ToURLSerializerTest.java (1)
55-58: Remove duplicated null-case assertion.Line [55]-Line [58] repeats the same behavior already covered by Line [25]-Line [27]. Keeping one null test is enough.
🧹 Proposed cleanup
- `@Test` - void encodesNullViaTextSerialization() { - // depends on TEXT_SERIALIZATION(null) → typically "null" - assertEquals("null", toURL(null)); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/util/text/ToURLSerializerTest.java` around lines 55 - 58, The test encodesNullViaTextSerialization in ToURLSerializerTest duplicates an existing null-case assertion for toURL; remove this duplicate test method (encodesNullViaTextSerialization) or its redundant assertEquals("null", toURL(null)) so only one null-case assertion remains, keeping the original null coverage in the other test and avoiding duplicate assertions.core/src/test/java/com/predic8/membrane/core/util/text/ToXMLSerializerTest.java (1)
84-88: Prefer reusing shared XML test parser utility.Line [84]-Line [88] duplicates parsing setup that already exists in
core/src/test/java/com/predic8/membrane/core/util/xml/XMLTestUtil.java. Reusing it would reduce local boilerplate and centralize parser behavior.♻️ Proposed refactor
private static Document parseXml(String xml) throws Exception { - var dbf = DocumentBuilderFactory.newInstance(); - dbf.setNamespaceAware(true); - return dbf.newDocumentBuilder().parse(new ByteArrayInputStream(xml.getBytes(UTF_8))); + return com.predic8.membrane.core.util.xml.XMLTestUtil.parse(xml); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/util/text/ToXMLSerializerTest.java` around lines 84 - 88, The private parseXml(...) helper in ToXMLSerializerTest duplicates existing functionality; remove this method and replace its call sites with the shared XMLTestUtil parser (e.g., call XMLTestUtil.parseXml(xmlString) or the appropriate static method on XMLTestUtil), add the necessary import for XMLTestUtil, and keep the same exception propagation (throws Exception) so tests compile; finally delete the now-unused private parseXml method from ToXMLSerializerTest.core/src/main/java/com/predic8/membrane/core/util/text/SerializationUtil.java (1)
68-69: Rename method parameter for terminology consistency.Consider renaming
escapingtoserializationingetSerialization(Serialization escaping)to match the new API vocabulary.Suggested rename
- public static SerializationFunction getSerialization(Serialization escaping) { - return switch (escaping) { + public static SerializationFunction getSerialization(Serialization serialization) { + return switch (serialization) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/text/SerializationUtil.java` around lines 68 - 69, Rename the parameter in the method signature getSerialization(Serialization escaping) from "escaping" to "serialization" and update all references inside the method body to use "serialization" (and any Javadoc or parameter tags) so the API vocabulary is consistent with the Serialization type; ensure any internal usages, switch on the parameter, and tests/ callers compiled against this signature are updated to reflect the new parameter name.core/src/main/java/com/predic8/membrane/core/util/text/ToJsonSerializer.java (1)
34-39: Remove redundantNodeListbranch after normalization.After
normalizeForJson(o), theNodeListcheck is redundant in this flow and adds confusion.Suggested simplification
public static String toJson(Object o) { try { if (o instanceof NodeList || o instanceof Node) { o = normalizeForJson(o); } - if (o instanceof NodeList nl) { - return nodeListToJson(nl); - } return objectMapper.writeValueAsString(o); } catch (Exception first) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/text/ToJsonSerializer.java` around lines 34 - 39, The second NodeList branch is redundant after calling normalizeForJson; remove the conditional "if (o instanceof NodeList nl) return nodeListToJson(nl);" from ToJsonSerializer so normalizeForJson fully handles Node/NodeList normalization and the subsequent serialization path no longer tries to handle NodeList separately; ensure normalizeForJson (and any callers) returns the final normalized type expected by the remaining serialization logic (update references to nodeListToJson only if still needed elsewhere).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/util/text/SerializationFunction.java`:
- Line 20: Fix the Javadoc typo in the SerializationFunction interface: update
the class/interface-level Javadoc sentence that currently reads "Represents a
specialized function that serializes an object into its string
representation.m." to "Represents a specialized function that serializes an
object into its string representation." by removing the stray ".m" in the
comment above the SerializationFunction declaration.
In
`@distribution/src/test/java/com/predic8/membrane/tutorials/xml/XPathTutorialTest.java`:
- Around line 33-54: The test method xpathExtractsPropertiesAndSetsHeader no
longer asserts the response header; restore a direct header assertion in the
RestAssured .then() chain to verify the controller set the header as intended.
Inside xpathExtractsPropertiesAndSetsHeader, after
.statusCode(200)/.contentType(TEXT) add a .header(...) (or .headers(...))
assertion referencing the expected header name and value extracted by the XPath
logic (e.g., the header that should contain the comma-separated Names or
Animals) so the test verifies both body content and the response header
contract.
---
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/lang/TemplateExpressionParser.java`:
- Around line 92-101: The single-quote parsing in TemplateExpressionParser
incorrectly ends single-quoted literals on a backslash-escaped quote; update the
inSingle handling so that if c == '\\' and i+1 < s.length() you increment i to
skip the escaped character (preserving the escape) before any quote-check, and
only treat a single quote as closing when it's not escaped (still keep the
existing SpEL double-single-quote handling). Locate the inSingle block in
TemplateExpressionParser (variables s, i, c, inSingle) and add the
backslash-skip branch before or as part of the c == '\'' logic; also ensure
brace-matching tests in TemplateExpressionParserTest include a case like
${'it\\'s {ok}'} to verify the fix.
In
`@distribution/src/test/java/com/predic8/membrane/tutorials/xml/XPathTutorialTest.java`:
- Around line 35-63: The test mutates JVM-global System.out (System.setOut)
which is unsafe for parallel tests; remove the
ByteArrayOutputStream/System.setOut block in XPathTutorialTest and instead
capture what you need directly from the HTTP response or RestAssured logging:
call Response response =
given()...when().post("http://localhost:2000").then().extract().response() (or
use a RestAssured logging filter) and assert on response.asString() (replace
console.contains checks) or use a test-specific logger injection if you must
inspect server logs; remove references to out, original, System.setOut, and
console and assert against the response content or injected logger.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/util/text/SerializationUtil.java`:
- Around line 68-69: Rename the parameter in the method signature
getSerialization(Serialization escaping) from "escaping" to "serialization" and
update all references inside the method body to use "serialization" (and any
Javadoc or parameter tags) so the API vocabulary is consistent with the
Serialization type; ensure any internal usages, switch on the parameter, and
tests/ callers compiled against this signature are updated to reflect the new
parameter name.
In
`@core/src/main/java/com/predic8/membrane/core/util/text/ToJsonSerializer.java`:
- Around line 34-39: The second NodeList branch is redundant after calling
normalizeForJson; remove the conditional "if (o instanceof NodeList nl) return
nodeListToJson(nl);" from ToJsonSerializer so normalizeForJson fully handles
Node/NodeList normalization and the subsequent serialization path no longer
tries to handle NodeList separately; ensure normalizeForJson (and any callers)
returns the final normalized type expected by the remaining serialization logic
(update references to nodeListToJson only if still needed elsewhere).
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptorSpELTest.java`:
- Around line 59-65: Rename the misnamed test method computeSingeExpression to
computeSingleExpression to fix the typo; update the method declaration in
SetHeaderInterceptorSpELTest (and any references) so the test name is spelled
"Single" consistently (leave the test body and assertion
setHeaderEvalGetValue("${1*2*3*4}") unchanged).
- Around line 151-166: The tests failOnErrorTrue and failOnErrorFalse rely on a
superclass fixture for the target header name; make them independent by
explicitly setting the header name on the interceptor in each test (e.g., call
interceptor.setHeaderName("x-bar") before interceptor.init(router) in both
failOnErrorTrue and failOnErrorFalse). This ensures the assertions using
super.getHeader("x-bar") don't depend on fixture defaults; update both test
methods in SetHeaderInterceptorSpELTest accordingly.
In
`@core/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.java`:
- Around line 120-168: Add a regression test in TemplateExpressionParserTest
that verifies backslash-escaped single quotes inside a SpEL single-quoted
literal are treated as part of the literal: create a new `@Test` (similar to
supportsSpelSingleQuoteEscapingWithDoubleSingleQuote and
doubleSingleQuoteEscaping) that calls parseTokens(interceptor, SPEL, "x
${'it\\'s {ok}'} y") and asserts tokens.size() == 3 and
tokens.get(0).getExpression().equals("x "),
tokens.get(1).getExpression().equals("'it\\'s {ok}'"),
tokens.get(2).getExpression().equals(" y") to lock the parser behavior for
backslash-escaped single quotes.
In
`@core/src/test/java/com/predic8/membrane/core/util/text/ToURLSerializerTest.java`:
- Around line 55-58: The test encodesNullViaTextSerialization in
ToURLSerializerTest duplicates an existing null-case assertion for toURL; remove
this duplicate test method (encodesNullViaTextSerialization) or its redundant
assertEquals("null", toURL(null)) so only one null-case assertion remains,
keeping the original null coverage in the other test and avoiding duplicate
assertions.
In
`@core/src/test/java/com/predic8/membrane/core/util/text/ToXMLSerializerTest.java`:
- Around line 84-88: The private parseXml(...) helper in ToXMLSerializerTest
duplicates existing functionality; remove this method and replace its call sites
with the shared XMLTestUtil parser (e.g., call XMLTestUtil.parseXml(xmlString)
or the appropriate static method on XMLTestUtil), add the necessary import for
XMLTestUtil, and keep the same exception propagation (throws Exception) so tests
compile; finally delete the now-unused private parseXml method from
ToXMLSerializerTest.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/log/LogInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.javacore/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.javacore/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.javacore/src/main/java/com/predic8/membrane/core/lang/TemplateExpressionParser.javacore/src/main/java/com/predic8/membrane/core/proxies/Target.javacore/src/main/java/com/predic8/membrane/core/util/text/SerializationFunction.javacore/src/main/java/com/predic8/membrane/core/util/text/SerializationUtil.javacore/src/main/java/com/predic8/membrane/core/util/text/ToJsonSerializer.javacore/src/main/java/com/predic8/membrane/core/util/text/ToURLSerializer.javacore/src/main/java/com/predic8/membrane/core/util/text/ToXMLSerializer.javacore/src/test/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptorSpELTest.javacore/src/test/java/com/predic8/membrane/core/lang/TemplateExchangeExpressionTest.javacore/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.javacore/src/test/java/com/predic8/membrane/core/util/text/ToTextSerializerTest.javacore/src/test/java/com/predic8/membrane/core/util/text/ToURLSerializerTest.javacore/src/test/java/com/predic8/membrane/core/util/text/ToXMLSerializerTest.javacore/src/test/java/com/predic8/membrane/core/util/xml/XMLTestUtil.javadistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/xml/XPathTutorialTest.java
🚧 Files skipped from review as they are similar to previous changes (6)
- core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java
- core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java
- core/src/main/java/com/predic8/membrane/core/interceptor/log/LogInterceptor.java
- core/src/test/java/com/predic8/membrane/core/util/xml/XMLTestUtil.java
- core/src/main/java/com/predic8/membrane/core/util/text/ToXMLSerializer.java
- core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java
…r typos - Renamed parameters in `SerializationUtil` for clarity. - Simplified XML parsing and removed redundant methods in `ToXMLSerializerTest`. - Adjusted test method names for consistency. - Removed unnecessary test cases in `ToURLSerializerTest` and `ToJsonSerializer`. - Fixed minor typo in `SerializationFunction` and `SetHeaderInterceptorSpELTest`.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/util/text/SerializationUtil.java (1)
34-44:⚠️ Potential issue | 🟡 MinorFix the broken enum Javadoc bullet structure.
The
SEGMENTbullet is interrupted by theTEXTbullet, which makes the docs confusing.📝 Proposed doc fix
- * - {`@code` SEGMENT}: Encodes as safe URI path segments, ensuring they do not introduce - * - {`@code` TEXT}: Serializes as plain text, without any encoding. - * path separators, query delimiters, or other unsafe characters, as per RFC 3986. + * - {`@code` SEGMENT}: Encodes as safe URI path segments, ensuring they do not introduce + * path separators, query delimiters, or other unsafe characters, as per RFC 3986. + * - {`@code` TEXT}: Serializes as plain text, without any encoding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/text/SerializationUtil.java` around lines 34 - 44, In SerializationUtil.java update the enum Javadoc so each serialization type has its own uninterrupted bullet: ensure the {`@code` SEGMENT} description ("Encodes as safe URI path segments, ensuring they do not introduce path separators, query delimiters, or other unsafe characters, as per RFC 3986.") is a single contiguous bullet and move the {`@code` TEXT} bullet ("Serializes as plain text, without any encoding.") to its own separate line; keep the other bullets for {`@code` URL}, {`@code` JSON}, and {`@code` XML} intact and consistently formatted so all enum values (URL, JSON, XML, SEGMENT, TEXT) are described cleanly in the doc block.
🧹 Nitpick comments (4)
core/src/main/java/com/predic8/membrane/core/util/text/ToJsonSerializer.java (1)
26-31: Make this utility class non-instantiable.This class only exposes static behavior; marking it
finaland adding a private constructor prevents accidental instantiation/subclassing.♻️ Proposed refactor
-public class ToJsonSerializer { +public final class ToJsonSerializer { private static final Logger log = LoggerFactory.getLogger(ToJsonSerializer.class); private static final ObjectMapper objectMapper = new ObjectMapper(); + + private ToJsonSerializer() { + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/text/ToJsonSerializer.java` around lines 26 - 31, The ToJsonSerializer utility class is instantiable and subclassable; make it non-instantiable by declaring the class final (final class ToJsonSerializer) and adding a private no-arg constructor (private ToJsonSerializer()) to prevent construction/subclassing, leaving static members like objectMapper and log unchanged so static behavior continues to work.core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptorSpELTest.java (2)
59-66: Inconsistent method naming: typo remains incomputeSingeExpressionWithCtx.Line 59 correctly fixes the typo (
computeSingeExpression→computeSingleExpression), but line 64 still has the same typo (computeSingeExpressionWithCtxinstead ofcomputeSingleExpressionWithCtx).✏️ Suggested fix
`@Test` -void computeSingeExpressionWithCtx() { +void computeSingleExpressionWithCtx() { assertEquals("Panama", setHeaderEvalGetValue("${property.bar}")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptorSpELTest.java` around lines 59 - 66, Rename the test method computeSingeExpressionWithCtx to computeSingleExpressionWithCtx to fix the typo; update the method declaration (the test annotated method) so its name matches computeSingleExpressionWithCtx while leaving the `@Test` annotation and the method body (assertEquals("Panama", setHeaderEvalGetValue("${property.bar}"))) unchanged.
84-91: Minor inconsistency:throws Exceptionretained on some methods.
jsonWrongAttribute()andjsonPath()still declarethrows Exceptionwhile other test methods in this class had it removed. Consider removing for consistency.✏️ Suggested fix
`@Test` -void jsonWrongAttribute() throws Exception { +void jsonWrongAttribute() { assertEquals("foo null baz", setHeaderEvalGetValue("foo ${json['b']} baz")); } `@Test` -void jsonPath() throws Exception { +void jsonPath() { assertEquals("Mango", setHeaderEvalGetValue("${jsonPath('$.name')}")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptorSpELTest.java` around lines 84 - 91, In SetHeaderInterceptorSpELTest, remove the unnecessary "throws Exception" declaration from the method signatures of jsonWrongAttribute() and jsonPath() to match the other tests; update the two methods (jsonWrongAttribute and jsonPath) to have no thrown exception in their signatures, run tests to confirm no compilation issues, and leave test logic and annotations intact.core/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.java (1)
135-142: Consider adding test for empty expression${}.The error handling test for unclosed expressions is good. Consider also testing the edge case of an empty expression
${}to verify whether the parser handles it gracefully or throws an appropriate error.💡 Optional: Add test for empty expression
`@Test` void handlesEmptyExpression() { // Verify behavior for empty expression - adjust assertion based on expected behavior var tokens = parseTokens(interceptor, SPEL, "x ${} y"); // Either expect it to parse with empty inner expression, or throw an exception }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.java` around lines 135 - 142, Add a unit test alongside throwsOnUnclosedExpression to cover the empty-expression edge case `${}`: create a new test method (e.g., handlesEmptyExpression) that calls parseTokens(interceptor, SPEL, "x ${} y") and assert the expected behavior (either successful tokenization producing a token sequence with an empty expression or assertThrows with the specific exception and message). Use the same helper parseTokens, interceptor and SPEL symbols so the test mirrors existing tests and assert either the returned tokens content or that ExchangeExpressionException is thrown with an appropriate message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/util/text/SerializationUtil.java`:
- Around line 34-44: In SerializationUtil.java update the enum Javadoc so each
serialization type has its own uninterrupted bullet: ensure the {`@code` SEGMENT}
description ("Encodes as safe URI path segments, ensuring they do not introduce
path separators, query delimiters, or other unsafe characters, as per RFC
3986.") is a single contiguous bullet and move the {`@code` TEXT} bullet
("Serializes as plain text, without any encoding.") to its own separate line;
keep the other bullets for {`@code` URL}, {`@code` JSON}, and {`@code` XML} intact and
consistently formatted so all enum values (URL, JSON, XML, SEGMENT, TEXT) are
described cleanly in the doc block.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/util/text/ToJsonSerializer.java`:
- Around line 26-31: The ToJsonSerializer utility class is instantiable and
subclassable; make it non-instantiable by declaring the class final (final class
ToJsonSerializer) and adding a private no-arg constructor (private
ToJsonSerializer()) to prevent construction/subclassing, leaving static members
like objectMapper and log unchanged so static behavior continues to work.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptorSpELTest.java`:
- Around line 59-66: Rename the test method computeSingeExpressionWithCtx to
computeSingleExpressionWithCtx to fix the typo; update the method declaration
(the test annotated method) so its name matches computeSingleExpressionWithCtx
while leaving the `@Test` annotation and the method body (assertEquals("Panama",
setHeaderEvalGetValue("${property.bar}"))) unchanged.
- Around line 84-91: In SetHeaderInterceptorSpELTest, remove the unnecessary
"throws Exception" declaration from the method signatures of
jsonWrongAttribute() and jsonPath() to match the other tests; update the two
methods (jsonWrongAttribute and jsonPath) to have no thrown exception in their
signatures, run tests to confirm no compilation issues, and leave test logic and
annotations intact.
In
`@core/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.java`:
- Around line 135-142: Add a unit test alongside throwsOnUnclosedExpression to
cover the empty-expression edge case `${}`: create a new test method (e.g.,
handlesEmptyExpression) that calls parseTokens(interceptor, SPEL, "x ${} y") and
assert the expected behavior (either successful tokenization producing a token
sequence with an empty expression or assertThrows with the specific exception
and message). Use the same helper parseTokens, interceptor and SPEL symbols so
the test mirrors existing tests and assert either the returned tokens content or
that ExchangeExpressionException is thrown with an appropriate message.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/main/java/com/predic8/membrane/core/util/text/SerializationFunction.javacore/src/main/java/com/predic8/membrane/core/util/text/SerializationUtil.javacore/src/main/java/com/predic8/membrane/core/util/text/ToJsonSerializer.javacore/src/test/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptorSpELTest.javacore/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.javacore/src/test/java/com/predic8/membrane/core/util/text/ToURLSerializerTest.javacore/src/test/java/com/predic8/membrane/core/util/text/ToXMLSerializerTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/xml/XPathTutorialTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/main/java/com/predic8/membrane/core/util/text/SerializationFunction.java
- core/src/test/java/com/predic8/membrane/core/util/text/ToURLSerializerTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/xml/XPathTutorialTest.java
- Added a test for handling empty expressions in `TemplateExpressionParserTest`. - Renamed methods in `SetHeaderInterceptorSpELTest` for consistency and removed unnecessary `Exception` declarations. - Added a private constructor to `ToJsonSerializer`.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.java (1)
121-133: Consolidate duplicate SpEL quote-escaping tests.Line [121]-[133] and Line [161]-[168] assert the same parser behavior (double single-quote escaping). Consider a parameterized test to reduce duplication and future drift.
♻️ Suggested refactor
+import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; @@ - `@Test` - void supportsSpelSingleQuoteEscapingWithDoubleSingleQuote() { - // SpEL: '' inside single-quoted string represents a literal ' - var tokens = parseTokens( - interceptor, - SPEL, - "x ${'it''s {fine}'} y" - ); - - assertEquals(3, tokens.size()); - assertEquals("x ", tokens.get(0).getExpression()); - assertEquals("'it''s {fine}'", tokens.get(1).getExpression()); - assertEquals(" y", tokens.get(2).getExpression()); - } + `@ParameterizedTest` + `@ValueSource`(strings = { + "x ${'it''s {fine}'} y", + "x ${'Ehm'' '} y" + }) + void supportsSpelSingleQuoteEscapingWithDoubleSingleQuote(String input) { + var tokens = parseTokens(interceptor, SPEL, input); + assertEquals(3, tokens.size()); + assertEquals("x ", tokens.get(0).getExpression()); + assertEquals(" y", tokens.get(2).getExpression()); + } @@ - `@Test` - void doubleSingleQuoteEscaping() { - var tokens = parseTokens(interceptor, SPEL, "x ${'Ehm'' '} y"); - assertEquals(3, tokens.size()); - assertEquals("x ", tokens.get(0).getExpression()); - assertEquals("'Ehm'' '", tokens.get(1).getExpression()); - assertEquals(" y", tokens.get(2).getExpression()); - }Also applies to: 161-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.java` around lines 121 - 133, Duplicate tests asserting SpEL single-quote escaping should be consolidated into a single parameterized test: replace the two methods (e.g., supportsSpelSingleQuoteEscapingWithDoubleSingleQuote and the similar test around lines 161-168) with a `@ParameterizedTest` that iterates over input strings and expected token lists, invoking parseTokens(interceptor, SPEL, input) and asserting token sizes and expressions; keep references to parseTokens, SPEL and interceptor so the unified test covers both cases (e.g., inputs like "x ${'it''s {fine}'} y") and asserts the same expected tokens to avoid duplication and future drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@core/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.java`:
- Around line 121-133: Duplicate tests asserting SpEL single-quote escaping
should be consolidated into a single parameterized test: replace the two methods
(e.g., supportsSpelSingleQuoteEscapingWithDoubleSingleQuote and the similar test
around lines 161-168) with a `@ParameterizedTest` that iterates over input strings
and expected token lists, invoking parseTokens(interceptor, SPEL, input) and
asserting token sizes and expressions; keep references to parseTokens, SPEL and
interceptor so the unified test covers both cases (e.g., inputs like "x ${'it''s
{fine}'} y") and asserts the same expected tokens to avoid duplication and
future drift.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/util/text/ToJsonSerializer.javacore/src/test/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptorSpELTest.javacore/src/test/java/com/predic8/membrane/core/lang/TemplateExpressionParserTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/util/text/ToJsonSerializer.java
rrayst
approved these changes
Mar 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation