XmlBeautifier: Fix Yaml Indentation, soapBody Format Fix#2714
Conversation
…OAP mock tutorial - Corrected typos in class names for XML beautifiers. - Enhanced whitespace handling for XML formatting in `XMLBeautifier`. - Added `50-Simple-SOAP-Mock.yaml` tutorial for creating a static SOAP mock API.
📝 WalkthroughWalkthroughPropagates Router context through expression and templating subsystems; converts many SpEL/Groovy built-ins to router-bound instances; adds indentation unification for templating/YAML; adjusts XML prettifier/beautifier charset and whitespace handling; introduces content-type defaults and SetBodyInterceptor contentType; adds tutorials and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Interceptor as TemplateInterceptor
participant TemplateExpr as TemplateExchangeExpression
participant SpEL as SpELBuiltInFunctions
participant Router as Router/Registry
participant XPathUtil as XPathUtil
Client->>Interceptor: request -> render template
Interceptor->>TemplateExpr: create/evaluate (passes Router)
TemplateExpr->>SpEL: evaluate expression (instance bound, uses Router)
SpEL->>Router: lookup XmlConfig / registry data
Router-->>SpEL: return config/data
alt XPath needed
SpEL->>XPathUtil: newXPath(XmlConfig)
XPathUtil-->>SpEL: XPath instance
end
SpEL-->>TemplateExpr: evaluation result
TemplateExpr-->>Interceptor: rendered body
Interceptor-->>Client: response (with Content-Type)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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}
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/templating/AbstractTemplateInterceptor.java`:
- Around line 104-108: The code uses a hardcoded UTF_8 in the call to
unifyIndent when setting the message body, which can corrupt output if the
class-level charset is configured to something else; update the call site in
AbstractTemplateInterceptor where
msg.setBodyContent(unifyIndent(prettify(getContent(exchange, flow)), UTF_8)) is
called to use the instance's configured charset (the charset field used by
prettify) instead of UTF_8 so unifyIndent interprets bytes with the same
encoding used by prettify (ensure you reference the charset field and the
methods prettify(...) and unifyIndent(...) and keep msg.setBodyContent(...) and
msg.getHeader().setContentType(...) behavior unchanged).
🧹 Nitpick comments (3)
core/src/main/java/com/predic8/membrane/core/util/text/TextUtil.java (1)
208-210: Guard against null charset in the new byte[] overload.
Using the same defaulting logic avoids surprising NPEs.Proposed fix
public static byte[] unifyIndent(byte[] data, Charset charset) { - return unifyIndent(new String(data, charset)).getBytes(charset); + Charset cs = getCharsetOrDefault(charset); + return unifyIndent(new String(data, cs)).getBytes(cs); }core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java (2)
32-32: Redundant import.
ABORTis already imported via the wildcard import on line 31 (import static com.predic8.membrane.core.interceptor.Outcome.*;).🧹 Proposed fix
import static com.predic8.membrane.core.interceptor.Outcome.*; -import static com.predic8.membrane.core.interceptor.Outcome.ABORT;
46-46: Calling overridable method from field initializer is fragile.
getDefaultContentType()is a protected method that subclasses can override (per the AI summary,SoapBodyTemplateInterceptordoes override it). Calling overridable methods during field initialization can lead to subtle bugs: when a subclass instance is created, the superclass field initializers run first, triggering dynamic dispatch to the subclass override before the subclass is fully initialized.This works as long as the override only returns a constant, but it's a known anti-pattern that can break if an override ever accesses subclass state.
Consider initializing
contentTypein the constructor or using a factory pattern instead.♻️ Proposed fix using constructor initialization
- protected String contentType = getDefaultContentType(); + protected String contentType; + + protected AbstractTemplateInterceptor() { + this.contentType = getDefaultContentType(); + }Note: While this doesn't eliminate the virtual call during construction, it makes the intent clearer and is the more conventional pattern. A safer alternative would be to make
getDefaultContentType()finaland use a class-level constant, or pass the default content type as a constructor parameter from subclasses.
…add indentation handling tests - Simplified `prettify` methods by eliminating duplicated logic, improving maintainability. - Introduced unified indentation handling in `TextUtil`. - Added new tests in `StaticInterceptorTest` to validate minimum indentation behavior. - Enhanced XML encoding fallback mechanism in `XMLPrettifier`.
|
This pull request needs "/ok-to-test" from an authorized committer. |
…t types - Added unit tests to verify default content type behavior and updates based on SOAP version changes. - Updated `AbstractTemplateInterceptor` to use dynamic `getContentType()` instead of a fixed value.
…on' into xmlbeautifier-fix-yaml-indentation
…and SOAP body templates - Introduced `50-SOAP-Array-from-JSON.yaml` for transforming JSON arrays into SOAP responses. - Added `40-REST-GET-to-SOAP.yaml` to demonstrate exposing SOAP services via REST endpoints. - Created `60-SOAP-Body-Template.yaml` for static SOAP response testing. - Included sample data `fruits.json` for JSON-to-SOAP transformations. - Enhanced error logging in `TemplateInterceptor` for better debugging.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@distribution/tutorials/transformation/50-SOAP-Array-from-JSON.yaml`:
- Around line 19-27: The example XML has mismatched element tags: opening tags
use <fruits> but the closers use </fruit>, which will confuse readers; update
the example so each opening tag matches its closing tag consistently (either
change all <fruits> to <fruit> or change all </fruit> to </fruits>) for the
repeated elements inside the getFruitsResponse block so the fragment is
well-formed and consistent.
… and built-in function handling - Added `Router` dependencies to SpEL functions, XPath, and JSONPath evaluations for enhanced configurability. - Updated `SpELBuiltInFunctions`, `TemplateExchangeExpression`, and related classes to inject `Router` during initialization. - Refactored tests to accommodate non-static functions and router requirements. - Simplified `ReflectiveMethodHandler` to remove reliance on static method invocations. - Adjusted tutorials and examples to reflect XPath improvements and consistent JSON/SOAP mappings.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (2)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
54-54:⚠️ Potential issue | 🔴 CriticalShared
XPathFactoryinstance is not thread-safe.
XPathFactoryis explicitly documented in JAXP as not thread-safe: the application must ensure at most one thread uses a givenXPathFactoryinstance at a time. The static shared instance at line 54 is used to createXPathobjects in bothxpath()method overloads (lines 79 and 120), which will cause race conditions when handling concurrent requests in the gateway.Consider creating a new factory per call or using
ThreadLocal:Suggested fix using ThreadLocal
- private static final XPathFactory xPathFactory = XPathFactory.newInstance(); + private static final ThreadLocal<XPathFactory> xPathFactory = + ThreadLocal.withInitial(XPathFactory::newInstance);Then update usages:
- var xPath = xPathFactory.newXPath(); + var xPath = xPathFactory.get().newXPath();core/src/main/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctions.java (1)
26-33:⚠️ Potential issue | 🟡 MinorUpdate the class Javadoc — “must be static” is no longer true.
The built-in functions are now instance methods, so the constraints should reflect that.📝 Suggested doc fix
- * 1. They must be static. + * 1. They are instance methods registered on a SpELBuiltInFunctions instance.
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java`:
- Around line 110-118: The Javadoc claims a default Content-Type but the
contentType field in SetBodyInterceptor is null and only applied when non-null;
fix by implementing the documented default: initialize the contentType field to
"text/plain" (and keep the existing setContentType(String) setter to override
it) so the header-setting logic in the interceptor (the code that checks
contentType before setting the header) will apply the documented default;
alternatively, if you prefer to remove the default, update the Javadoc to remove
the `@default` annotation and ensure setContentType remains nullable—pick one of
these two options and apply consistently.
In
`@core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyExchangeExpression.java`:
- Around line 38-40: Constructor GroovyExchangeExpression currently ignores the
router parameter by assigning this.router = interceptor.getRouter(); change the
constructor to consistently use the passed router parameter (assign this.router
= router) so the value passed into GroovyExchangeExpression(Interceptor
interceptor, String source, Router router) is honored; if a fallback is needed,
use interceptor.getRouter() only when the router parameter is null, and update
any references to the router field accordingly.
🧹 Nitpick comments (5)
distribution/tutorials/transformation/60-SOAP-Array-to-JSON.yaml (1)
7-14: Minor documentation inconsistency with sample data.The example SOAP structure in the comments shows
<name>and<price>without namespace prefixes, but the actualfruits.soap.xmlfile uses<f:name>and<f:price>. Consider updating the example to match:# <s11:Envelope xmlns:s11="http://schemas.xmlsoap.org/soap/envelope/"> # <s11:Body> # <f:getFruitsResponse xmlns:f="https://predic8.de/fruits"> -# <fruit><name>Apricot</name><price>3.8</price></fruit> -# <fruit><name>Date</name><price>2.3</price></fruit> +# <f:fruit><f:name>Apricot</f:name><f:price>3.8</f:price></f:fruit> +# <f:fruit><f:name>Date</f:name><f:price>2.3</f:price></f:fruit> # </f:getFruitsResponse> # </s11:Body> # </s11:Envelope>core/src/test/java/com/predic8/membrane/core/lang/TemplateExchangeExpressionTest.java (1)
31-41: Inconsistent router usage between test setup and eval() method.The test creates a
DefaultRouterinsetUp()(line 40) but theeval()method (line 69) creates a newDummyTestRouterinstance instead of using the existingrouterfield. This inconsistency could mask issues if the router state matters for expression evaluation.Consider using the same router instance consistently:
♻️ Suggested fix
private String eval(String expr) { - return new TemplateExchangeExpression(new InterceptorAdapter(router), language, expr, new DummyTestRouter()).evaluate(exc, REQUEST,String.class); + return new TemplateExchangeExpression(new InterceptorAdapter(router), language, expr, router).evaluate(exc, REQUEST, String.class); }Also applies to: 69-70
core/src/main/java/com/predic8/membrane/core/lang/AbstractExchangeExpression.java (1)
23-24: Consider makingrouterfield final for consistency and immutability.The
expressionfield is declaredfinalbutrouteris not. If the router should not change after construction (which appears to be the case based on the constructor-only assignment), marking itfinalwould enforce immutability and align with the existing pattern.♻️ Suggested fix
protected final String expression; - protected Router router; + protected final Router router;core/src/main/java/com/predic8/membrane/core/lang/spel/functions/ReflectiveMethodHandler.java (1)
66-76: Double-brace initialization creates anonymous inner classes.The double-brace initialization pattern at lines 67-69 and 73-75 creates anonymous inner classes, which has minor memory overhead. Consider using standard collection operations for cleaner code:
♻️ Optional refactor
private static ArrayList<TypeDescriptor> getParameterTypeDescriptors(List<TypeDescriptor> types) { - return new ArrayList<>(types) {{ - add(getTypeDescriptor(SpELExchangeEvaluationContext.class)); - }}; + var result = new ArrayList<>(types); + result.add(getTypeDescriptor(SpELExchangeEvaluationContext.class)); + return result; } private static Object[] getParameters(EvaluationContext ctx, Object[] args) { - return new ArrayList<>(of(args)) {{ - add(ctx); - }}.toArray(); + var result = new ArrayList<>(of(args)); + result.add(ctx); + return result.toArray(); }core/src/test/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctionsTest.java (1)
34-35: Use a shared DummyTestRouter for closer parity with production wiring.
This keeps ctx and functions aligned on the same router state/registry.♻️ Suggested tweak
- ctx = new SpELExchangeEvaluationContext(post("/foo").xml("<person name='Fritz'/>").buildExchange(), REQUEST, new DummyTestRouter()); - functions = new SpELBuiltInFunctions(new DummyTestRouter()); + Router router = new DummyTestRouter(); + ctx = new SpELExchangeEvaluationContext(post("/foo").xml("<person name='Fritz'/>").buildExchange(), REQUEST, router); + functions = new SpELBuiltInFunctions(router);
…` instances, and add copyright headers - Centralized `XPath` creation into `XPathUtil` for thread safety and consistent namespace handling. - Removed redundant `XPathFactory` usage across classes. - Updated copyright headers in classes and tests. - Simplified test initialization by improving `DummyTestRouter` handling.
There was a problem hiding this comment.
Actionable comments posted: 1
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/xmlcontentfilter/XMLContentFilter.java (1)
124-132:⚠️ Potential issue | 🔴 CriticalXPathUtil class is missing from the codebase - this refactoring is incomplete.
The code references
com.predic8.membrane.core.util.xml.XPathUtil.newXPath()but the XPathUtil class does not exist in the repository. This will cause compilation errors.The utility class needs to be created in
core/src/main/java/com/predic8/membrane/core/util/xml/XPathUtil.javawith thread-safenewXPath()method implementation (supporting both no-argument and XmlConfig-argument variants to match all call sites).
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java`:
- Around line 100-101: Implement the missing utility class XPathUtil with two
public static factory methods: newXPath() and newXPath(XmlConfig xmlConfig);
newXPath() should create and return a thread-safe XPath instance (e.g., via
XPathFactory.newInstance().newXPath()) and newXPath(XmlConfig) should delegate
to newXPath() when xmlConfig is null, otherwise configure namespace support
using xmlConfig (e.g., setNamespaceContext based on xmlConfig) before returning
the XPath; ensure method signatures match exactly (public static XPath
newXPath(), public static XPath newXPath(XmlConfig xmlConfig)) and place the
class named XPathUtil in the util.xml package so imports of
com.predic8.membrane.core.util.xml.XPathUtil compile.
🧹 Nitpick comments (3)
annot/src/main/java/com/predic8/membrane/annot/yaml/spel/HashTemplateParserContext.java (1)
21-21: Rename constant for clarity:DOLLAR_TEMPLATE_PARSER_CONTEXT→HASH_TEMPLATE_PARSER_CONTEXTThe class uses hash delimiters
#{...}but the constant nameDOLLAR_TEMPLATE_PARSER_CONTEXTsuggests dollar delimiters${...}. This is confusing, especially sinceDollarTemplateParserContextexists as a separate class for dollar delimiters. Rename toHASH_TEMPLATE_PARSER_CONTEXTto match the actual delimiter style.This requires updating the import in
annot/src/main/java/com/predic8/membrane/annot/yaml/spel/SpELEngine.java(line 22).Changes needed
HashTemplateParserContext.java:
- public static final HashTemplateParserContext DOLLAR_TEMPLATE_PARSER_CONTEXT = new HashTemplateParserContext(); + public static final HashTemplateParserContext HASH_TEMPLATE_PARSER_CONTEXT = new HashTemplateParserContext();SpELEngine.java:
-import static com.predic8.membrane.annot.yaml.spel.HashTemplateParserContext.DOLLAR_TEMPLATE_PARSER_CONTEXT; +import static com.predic8.membrane.annot.yaml.spel.HashTemplateParserContext.HASH_TEMPLATE_PARSER_CONTEXT;core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
28-29: Redundant import.Line 29 imports
org.slf4j.Loggerexplicitly, but it's already covered by the wildcard import on line 28 (org.slf4j.*).🧹 Remove redundant import
import org.slf4j.*; -import org.slf4j.Logger;core/src/main/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctions.java (1)
36-44: Consider makingxmlConfigfinal.The
xmlConfigfield is only assigned in the constructor. Making itfinalwould better convey immutability and align withrouter.♻️ Make xmlConfig final
public class SpELBuiltInFunctions { private final Router router; - private XmlConfig xmlConfig; + private final XmlConfig xmlConfig; public SpELBuiltInFunctions(Router router) { this.router = router; if (router != null && router.getRegistry() != null) { this.xmlConfig = router.getRegistry().getBean(XmlConfig.class).orElse(null); + } else { + this.xmlConfig = null; } }
- Centralized `XPath` management with utility methods for consistent namespace handling and improved thread safety. - Introduced `newXPath` methods with support for `NamespaceContext` and `XmlConfig`.
… and unify template parser context - Made `XmlConfig` initialization in `SpELBuiltInFunctions` lazy and thread-safe. - Renamed `DOLLAR_TEMPLATE_PARSER_CONTEXT` to `HASH_TEMPLATE_PARSER_CONTEXT` for consistency.
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/util/xml/XPathUtil.java`:
- Around line 25-28: The Javadoc comment for the ThreadLocal XPathFactory in
class XPathUtil contains a typo ("!1"); update the comment above the FACTORY
field to read "XPathFactory is not thread-safe!" (remove the extra "1") so the
documentation is correct; locate the FACTORY declaration (private static final
ThreadLocal<XPathFactory> FACTORY =
ThreadLocal.withInitial(XPathFactory::newInstance);) and fix the comment text
accordingly.
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctions.java (2)
36-42: Consider removing unusedrouterfield.The
routerfield is stored but never accessed after the constructor completes—it is only used to derivexmlConfig. If there's no planned future use, you could passrouterdirectly togetXmlConfig()without storing it.That said, this mirrors the pattern in
GroovyBuiltInFunctions, so keeping it for consistency is also reasonable.♻️ Optional: Remove unused field
- private final Router router; private final XmlConfig xmlConfig; public SpELBuiltInFunctions(Router router) { - this.router = router; xmlConfig = getXmlConfig(router); }
56-58: Nit: Extra whitespace.Line 57 has a double space before
context.✏️ Fix formatting
public Object xpath(String expression, Object context, SpELExchangeEvaluationContext ctx) { - return CommonBuiltInFunctions.xpath(expression, context, xmlConfig); + return CommonBuiltInFunctions.xpath(expression, context, xmlConfig); }
…tions` and fix comment typo in `XPathUtil`
|
/ok-to-test |
…ion, and security tutorials - Added test classes to validate orchestration scenarios, such as authentication calls and for-loop operations. - Included tests for transformations like JSON-to-SOAP, SOAP-to-JSON, and REST-to-SOAP mappings. - Introduced security-related tests such as TLS termination and centralized SSL configuration. - Verified dynamic responses for SOAP mocks and templates with corresponding input data.
…ndant timeout argument - Updated orchestration tests to use the refactored `waitForOrFail` method with a default timeout. - Replaced individual timeout arguments with hardcoded defaults for improved readability and consistency.
|
/ok-to-test |
1 similar comment
|
/ok-to-test |
…type constant for consistency
|
/ok-to-test |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Docs / Examples
Tests
✏️ Tip: You can customize this high-level summary in your review settings.