Template/Static: Refactoring and text Prettifier #2068
Conversation
|
Warning Rate limit exceeded@predic8 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughLarge refactor introducing a Prettifier subsystem (JSON/XML/Text/Null), XML beautifier framework and thread-local XML factories, a new AbstractTemplateInterceptor byte[] flow with templating refactors, charset/stream modernizations across Message/Header/requests, SoapVersion/FileUtil helpers, many test/resource additions, and removal of legacy helpers (Magic, certain ByteUtil APIs). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Interceptor as ConcreteInterceptor
participant Template as AbstractTemplateInterceptor
participant Prett as Prettifier
participant Message as HTTPMessage
Client->>Interceptor: request/response arrives
Interceptor->>Template: handleRequest/handleResponse(exc)
Template->>Template: getContent(exc, flow) (abstract)
Template->>Prett: Prettifier.getInstance(contentType)
Prett-->>Template: instance
Template->>Prett: prettify(bytes, charset?)
Prett-->>Template: formatted bytes (or original)
Template->>Message: set body + Content-Type
Template-->>Interceptor: Outcome.CONTINUE
sequenceDiagram
autonumber
participant Service
participant RewriterInt as WsaEndpointRewriterInterceptor
participant Rewriter as WsaEndpointRewriter
participant Parser as StAX Parser
participant Client
Service-->>RewriterInt: SOAP envelope (contains ReplyTo/Address)
RewriterInt->>Rewriter: rewriteEndpoint(inputStream, output, Location)
Rewriter->>Parser: parse events (XMLInputFactoryFactory)
Parser-->>Rewriter: StartElement(Address)
Rewriter->>Rewriter: compute new URL from Location + path
Rewriter-->>Parser: write rewritten Address element
Rewriter-->>RewriterInt: rewritten SOAP output
RewriterInt-->>Client: forwarded message (CONTINUE)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai plz review |
|
This pull request needs "/ok-to-test" from an authorized committer. |
|
@predic8: I'll review the changes in this PR for you. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🔭 Outside diff range comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/FlowContext.java (1)
85-88: Potential NPE when encoding triggerFlow; make URL param building null-safe.
urlencode(fc.triggerFlow)may throw iftriggerFlowis null. This happens when it’s absent in session or state.Apply a null-safe construction:
- return fc != null ? "%26fcd%3D" + urlencode(fc.defaultFlow) + "%26fct%3D" + urlencode(fc.triggerFlow) : ""; + if (fc == null) return ""; + StringBuilder sb = new StringBuilder(); + sb.append("%26fcd%3D").append(urlencode(fc.defaultFlow)); + if (fc.triggerFlow != null) { + sb.append("%26fct%3D").append(urlencode(fc.triggerFlow)); + } + return sb.toString();Follow-up: If
OAuth2Util.urlencodealready tolerates null by returning an empty string, feel free to ignore this change, but that contract should be documented.
🧹 Nitpick comments (60)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/FlowContext.java (3)
1-14: Optional: Add SPDX identifier for tooling friendliness.Consider adding an SPDX tag to ease license scanning.
Apply this minimal change at the top of the header:
-/* Copyright 2025 predic8 GmbH, www.predic8.com +/* Copyright 2025 predic8 GmbH, www.predic8.com + * SPDX-License-Identifier: Apache-2.0
81-83: Guard against storing null values in session (if unsupported).
flowContext.triggerFlowmay be null (e.g., from session or URL parsing). IfSession.putdisallows nulls, this will throw or store an unexpected string. Prefer conditional put/remove.If nulls are not allowed, consider:
- session.put("flowContextDefault", flowContext.defaultFlow); - session.put("flowContextTrigger", flowContext.triggerFlow); + session.put("flowContextDefault", flowContext.defaultFlow); + if (flowContext.triggerFlow != null) { + session.put("flowContextTrigger", flowContext.triggerFlow); + } else { + session.remove("flowContextTrigger"); + }Please confirm whether
Session.putaccepts null and howSession.removebehaves for missing keys.
85-88: Clarify percent-encoding layer to avoid double-encoding bugs.The returned string starts with encoded “&” and “=“ sequences (e.g., “%26fcd%3D…”). That’s correct if this fragment is injected into an already-encoded parameter (e.g., nested state). Document this layering to prevent future misuse, or consider a helper that builds nested query fragments safely.
Optionally add a short Javadoc note to
toUrlParamexplaining that it returns an already percent-encoded fragment intended for inclusion in an encoded context (like OAuth2 state).core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/Rewrite.java (1)
86-88: Nit: Prefer “URI” and standard phrasing in @throws.URISyntaxException pertains to URIs; also Javadoc typically uses “if …” phrasing.
Apply this diff to improve clarity:
- * @throws URISyntaxException syntax error in URL + * @throws URISyntaxException if the URI has invalid syntaxdistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/SampleSoapServiceExampleTest.java (1)
59-67: Avoid name shadowing and consider deterministic iteration order.
- The field and the provider method share the same name
cityMap, which is confusing. Rename the provider method (e.g.,cities) to improve readability.- If the execution order of parameterized tests should be stable, switch to
LinkedHashMapto preserve insertion order.Apply within this block:
- private final HashMap<String, String> cityMap = new HashMap<>() {{ + private final java.util.LinkedHashMap<String, String> cityMap = new java.util.LinkedHashMap<>() {{ @@ - final List<String> cityMap() { + final List<String> cities() { return cityMap.keySet().stream().toList(); }And update the method source annotation accordingly (outside the selected range):
- @MethodSource("cityMap") + @MethodSource("cities")distribution/examples/web-services-soap/rest2soap-template/proxies.xml (1)
24-24: Unquoted JSON number: verify population is always numeric and presentDropping the quotes makes correctness depend on
${property.population}always resolving to a valid JSON number. If the XPath yields empty, NaN, or any non-numeric text, the template will emit invalid JSON.
- Verify the SOAP response always contains a numeric population.
- Consider normalizing at extraction to a numeric-safe value to guard against missing values.
If feasible with your templating, prefer extracting via XPath to a numeric with a safe default (XPath 1.0 tip: sum() returns 0 on empty node-sets):
Example (outside this hunk):
<setProperty name="population" value="${sum(//population)}" language="xpath"/>distribution/examples/web-services-soap/rest2soap/README.md (1)
1-6: Header level adjustments look good; minor consistency nitH1 and H2 upgrades improve readability. For consistency, consider aligning “HOW IT IS DONE” to the same level as “RUNNING THE EXAMPLE” (currently H4 vs H2).
distribution/src/test/java/com/predic8/membrane/examples/withinternet/rest2soap/Rest2SOAPTemplateExampleTest.java (1)
40-40: Assert numeric population: LGTM; consider asserting content type as wellNumeric assertion matches the updated template behavior. As a small enhancement, also assert the response content type to catch regressions in media type.
.then() .log().ifValidationFails(ALL) .statusCode(200) + .contentType("application/json") .body("population", equalTo(333000));distribution/examples/web-services-soap/rest2soap-template/README.md (1)
27-27: Wording nit: “Take a look”Minor style improvement for American English consistency.
-5. Have a look at `proxies.xml` to see how it works. +5. Take a look at `proxies.xml` to see how it works.core/src/test/resources/charsets/iso-8859-1.txt (1)
1-1: Confirmed ISO-8859-1 encoding forcore/src/test/resources/charsets/iso-8859-1.txt
Latin-1 decoding succeeds; UTF-8 decoding fails.Optional: to prevent Git/IDE from re-encoding these fixtures, add the following to your
.gitattributes:# Pin test‐fixture encodings core/src/test/resources/charsets/iso-8859-1.txt -text working-tree-encoding=ISO-8859-1 core/src/test/resources/charsets/utf-8.txt -text working-tree-encoding=UTF-8If your Git version doesn’t support
working-tree-encoding, mark them as binary instead:core/src/test/resources/charsets/iso-8859-1.txt binary core/src/test/resources/charsets/utf-8.txt binarypom.xml (1)
75-76: Switch to UTF-8 encodings: LGTM.Aligning source/report encodings with UTF-8 matches the added non-ASCII content across the codebase and tests.
To keep test behavior deterministic across platforms, consider enforcing the JVM default encoding during tests:
<!-- in the surefire configuration (either here or module POMs) --> <plugin> <artifactId>maven-surefire-plugin</artifactId> <configuration> <argLine>-Dfile.encoding=UTF-8</argLine> </configuration> </plugin>core/src/main/java/com/predic8/membrane/core/Constants.java (1)
95-95: Consider symmetry or clearer intent for boolean-as-String constant.Using a String constant named TRUE can be confusing alongside boolean TRUE. If this is intended for templating/tests, either:
- add the counterpart FALSE for symmetry, or
- rename to clarify intent (e.g., STRING_TRUE), or
- avoid the constant and use Boolean.TRUE.toString() at call sites.
Example (add symmetric constant):
public static final String USERAGENT = "Membrane " + VERSION; public static final String TRUE = "true"; + public static final String FALSE = "false";distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/VersioningSoapXsltExampleTest.java (1)
22-23: Star static import is fine, but be mindful of potential name clashes.Static-importing all Hamcrest matchers can occasionally introduce ambiguities in large test classes. If clashes arise, prefer importing only the used matchers.
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2RedirectTest.java (1)
28-31: Import consolidation is OK; consider consistent Hamcrest usage.
- Broadening to wildcard imports reduces boilerplate and is acceptable if it aligns with project style.
- For consistency with other updated tests in this PR, consider statically importing Hamcrest matchers and using is(...)/equalTo(...) without the Matchers. qualifier.
Within this file (optional consistency tweak):
- .body(Matchers.is("Missing session.")); + .body(is("Missing session."));And adjust imports accordingly (outside the lines above):
// Replace: import org.hamcrest.*; // With: import static org.hamcrest.Matchers.*;Also applies to: 34-40
core/src/test/java/com/predic8/membrane/core/interceptor/AdjustContentLengthTest.java (1)
62-77: Prefer assertNotEquals over inequality in assertTrue.Using assertNotEquals makes the intention explicit and improves assertion messages.
- assertTrue(directRequest.getResponseContentLength() != monitoredRequest - .getResponseContentLength()); + assertNotEquals(directRequest.getResponseContentLength(), + monitoredRequest.getResponseContentLength());core/src/main/java/com/predic8/membrane/core/util/FileUtil.java (3)
45-51: Make isXml/isJson null-safe and case-insensitive.FilenameUtils.getExtension(location) returns null for null input and can be mixed-case; equals(...) risks NPE and false negatives. Recommend guarding null and using equalsIgnoreCase (by comparing constant to variable to avoid NPE). Optionally strip query/fragment for URL-like inputs.
- public static boolean isXml(String location) { - return FilenameUtils.getExtension(location).equals(XML); - } + public static boolean isXml(String location) { + if (location == null) return false; + String path = location; + int q = path.indexOf('?'); if (q >= 0) path = path.substring(0, q); + int h = path.indexOf('#'); if (h >= 0) path = path.substring(0, h); + String ext = FilenameUtils.getExtension(path); + return XML.equalsIgnoreCase(ext); + } - public static boolean isJson(String location) { - return FilenameUtils.getExtension(location).equals(JSON); - } + public static boolean isJson(String location) { + if (location == null) return false; + String path = location; + int q = path.indexOf('?'); if (q >= 0) path = path.substring(0, q); + int h = path.indexOf('#'); if (h >= 0) path = path.substring(0, h); + String ext = FilenameUtils.getExtension(path); + return JSON.equalsIgnoreCase(ext); + }
17-18: Avoid wildcard import from commons-io.Import the specific type used to keep the namespace clean and aid readability.
-import org.apache.commons.io.*; +import org.apache.commons.io.FilenameUtils;
26-28: Optional: clarify constant names.If these are strictly file extensions, consider renaming to XML_EXTENSION/JSON_EXTENSION for self-documenting code.
docs/ROADMAP.md (2)
37-41: Align terminology with codebase and clarify encoding item
- Suggest renaming “Beautifier” to “Prettifier” to match the new API introduced in this PR.
- Slightly rephrase the UTF-8 item for clarity.
Apply:
-- Convert to UTF-8 source and outputEncoding to UTF-8 (TB) - TemplateInterceptor Refactoring (TB) - Refactor: Beautifier to use the Code from above +- Convert sourceEncoding and reporting/outputEncoding to UTF-8 (TB) +- Template Interceptor refactoring (TB) +- Refactor: Prettifier to reuse the code from above
44-44: Resolve status inconsistency (“TB” vs “done”)This line says both “(TB)” and “done”. Please pick one (e.g., drop “(TB)” if it’s completed).
Apply:
- - Cook Book: outgoing-api-gateway (TB) done + - Cook Book: outgoing-api-gateway (Done)core/src/test/java/com/predic8/membrane/core/RouterTest.java (4)
50-61: Remove redundant .extract()The extracted value is unused. Removing it simplifies the test without changing behavior.
Apply:
.body("$",aMapWithSize(3)) - .extract(); + ;
66-79: LGTM: simplified signature and improved logging
- Dropping
throws Exceptionis appropriate for the test method.- Using
.log().ifValidationFails(ALL)increases diagnostics on failures.Also, consider removing the trailing unused
.extract():.body("error.message", Matchers.not(containsString(INTERNAL_SECRET))) - .extract(); + ;
85-96: Add failure logging and remove unused .extract()
- For consistency with XML tests, consider adding
.log().ifValidationFails(ALL).- The trailing
.extract()is unused.Apply:
given() .get("http://localhost:2001/") .then() + .log().ifValidationFails(ALL) .statusCode(500) .contentType(APPLICATION_PROBLEM_JSON) .body("title", equalTo("Internal server error.")) .body("type",equalTo("https://membrane-api.io/problems/internal")) .body("$",hasKey("attention")) .body("attention", Matchers.containsString("development mode")) .body("$",not(hasKey("stacktrace"))) - .extract(); + ;
100-116: LGTM: consistent static DSL; remove unused .extract()Test reads well. Consider removing the unused extraction.
Apply:
.body("problem-details.message", Matchers.containsString("supersecret")) .body("problem-details.stacktrace", Matchers.not(containsString("HttpServerHandler"))) - .extract(); + ;core/src/test/resources/json/unformatted.json (1)
1-1: LGTM: suitable unformatted JSON fixtureFits the purpose for prettifier tests. Optional: add a trailing newline to appease some linters and avoid “No newline at end of file” diffs.
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/CertsEndpointProcessor.java (1)
33-34: Rename field for consistency: Beautifier → PrettifierThe field is a JSONPrettifier; naming it
jsonPrettifieravoids confusion with the old “Beautifier” API.Apply:
- private final JSONPrettifier jsonBeautifier = new JSONPrettifier(); + private final JSONPrettifier jsonPrettifier = new JSONPrettifier();And update usage:
- exc.setResponse(ok().contentType(APPLICATION_JSON_UTF8).body(jsonBeautifier.prettify(jwks.getBytes(UTF_8))).build()); + exc.setResponse(ok().contentType(APPLICATION_JSON_UTF8).body(jsonPrettifier.prettify(jwks.getBytes(UTF_8))).build());Also applies to: 54-54
core/src/test/java/com/predic8/membrane/core/interceptor/templating/SoapBodyTemplateInterceptorTest.java (1)
26-39: LGTM: canonicalization coverage via parameterized testGood addition—covers both dotted and condensed SOAP version tokens and asserts canonical values.
Optional: The simple
setSOAPVersion()test duplicates the “valid tokens” cases now covered here. Consider removing it to avoid redundancy and keep the suite lean.core/src/main/java/com/predic8/membrane/core/beautifier/NullPrettifier.java (1)
19-22: NullPrettifier returns the original byte array; consider documenting or defensively copying.Returning the same reference is fine for a no-op, but it can surprise callers expecting immutability (other prettifiers likely return new arrays). At minimum, add a brief Javadoc stating that the input reference is returned unchanged. Optionally, return a copy for consistency.
If you prefer defensive copying:
- public byte[] prettify(byte[] c) throws Exception { - return c; - } + public byte[] prettify(byte[] c) throws Exception { + return c == null ? null : java.util.Arrays.copyOf(c, c.length); + }core/src/test/java/com/predic8/membrane/core/util/soap/SoapVersionTest.java (1)
34-37: Add negative tests for null/empty and whitespace inputs.Parsing edge cases like null, empty string, and strings with whitespace would harden the contract of SoapVersion.parse and prevent regressions.
Example additions:
@Test void parseInvalid() { assertThrows(ConfigurationException.class, () -> parse("invalid")); + assertThrows(ConfigurationException.class, () -> parse("")); + assertThrows(ConfigurationException.class, () -> parse(" ")); + assertThrows(ConfigurationException.class, () -> parse(null)); }core/src/test/java/com/predic8/membrane/core/beautifier/JSONBeautifierTest.java (1)
23-23: Rename class to match subject under test.The class name still references “Beautifier” while testing JSONPrettifier. Rename to JSONPrettifierTest for clarity and consistency.
-class JSONBeautifierTest { +class JSONPrettifierTest {core/src/main/java/com/predic8/membrane/core/beautifier/XMLPrettifier.java (1)
25-25: Remove unused logger or add log usage.The
logfield is currently unused in this class. Either remove it or add relevant debug logs.- protected static final Logger log = LoggerFactory.getLogger(XMLPrettifier.class); + // protected static final Logger log = LoggerFactory.getLogger(XMLPrettifier.class);core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/WellknownEndpointProcessor.java (2)
34-34: Prefer factory-based selection to reduce coupling.To align with the new Prettifier framework, consider using the factory based on content type instead of hardcoding JSONPrettifier. This keeps the processor decoupled from a specific prettifier.
- private final JSONPrettifier prettifier = new JSONPrettifier(); + private final Prettifier prettifier = Prettifier.getInstance(APPLICATION_JSON_UTF8);Note: Adjust imports accordingly if Prettifier.getInstance is available.
50-51: Nit: log message wording.Consider “well-known” instead of “wellknown” for readability.
- log.error("While constructing the wellknown response.", e); + log.error("While constructing the well-known response.", e);core/src/test/java/com/predic8/membrane/core/beautifier/TextPrettifierTest.java (3)
31-36: Method name mismatch with lifecycle annotation (nit)The method annotated with @beforeeach is named beforeAll, which can be confusing when scanning tests. Consider renaming to setUp or beforeEach for clarity.
96-98: Avoid platform-default charset in tests to prevent flakinessassertPretty currently round-trips bytes via the platform default charset. Make the charset explicit to ensure deterministic behavior across environments.
Apply this diff:
- private void assertPretty(String exp, String actual) throws Exception { - assertEquals(exp, new String(tp.prettify(actual.getBytes()))); - } + private void assertPretty(String exp, String actual) throws Exception { + assertEquals(exp, + new String( + tp.prettify(actual.getBytes(java.nio.charset.StandardCharsets.UTF_8)), + java.nio.charset.StandardCharsets.UTF_8 + ) + ); + }If you prefer, add a normal import at the top of the file:
- import java.nio.charset.StandardCharsets;
36-66: Consider a few additional edge-case tests
- Only blanks/newlines: normalizeMultiline(" \n\t\n") should return "".
- CRLF handling: "a\r\n\r\nb\r\n" should normalize to "a\n\nb".
- Mixed indent where the first content line has less indent than following lines (already covered, but a dedicated case with tabs+spaces mixed across lines can lock behavior in).
I can draft these if helpful.
Also applies to: 68-95
core/src/main/java/com/predic8/membrane/core/beautifier/Prettifier.java (2)
17-18: Locale import required by updated factoryIf you apply the robust factory above, add the missing import.
import static com.predic8.membrane.core.http.MimeType.*; + +import java.util.Locale;
19-22: Optional: reuse singletons to reduce allocationsSince prettifiers are stateless, you can cache instances to avoid per-call allocations.
Example:
// inside interface Prettifier JSON = new JSONPrettifier(); Prettifier XML = new XMLPrettifier(); Prettifier TEXT = new TextPrettifier(); // then return JSON/XML/TEXT in getInstance(...)core/src/main/java/com/predic8/membrane/core/util/soap/SoapVersion.java (1)
43-49: Trim and null-check version input in parse()Defensively handle nulls and surrounding whitespace to avoid surprising ConfigurationException messages.
- public static SoapVersion parse(String s) { - return switch (s) { + public static SoapVersion parse(String s) { + if (s == null) + throw new ConfigurationException("SOAP version must not be null."); + String t = s.trim(); + return switch (t) { case "1.1","11" -> SOAP_11; case "1.2","12" -> SOAP_12; - default -> throw new ConfigurationException("SOAP version %s is not supported.".formatted(s)); + default -> throw new ConfigurationException("SOAP version %s is not supported.".formatted(s)); }; }core/src/main/java/com/predic8/membrane/core/beautifier/TextPrettifier.java (2)
17-18: Remove stale TODO: tests existTextPrettifierTest covers this class now. The TODO comment is outdated.
-// TODO Tests
26-75: Algorithm looks good; be aware of mixed tabs/spaces semanticsCurrent dedent counts leading whitespace in code points, not “indent levels.” That’s fine and matches tests, but mixed tabs/spaces across lines can lead to non-uniform visual alignment after normalization. If stricter behavior is desired, we can normalize tabs to spaces first or compute indent width in columns.
Would you like me to propose a version that first expands tabs (e.g., to 4 spaces) before computing minIndent?
core/src/test/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptorTest.java (2)
45-55: Assert content intent rather than length threshold (optional)Length > 30 is a weak proxy for “pretty JSON.” Consider asserting structural formatting traits (e.g., presence of newlines/spaces) or even exact pretty result to reduce false positives.
Example:
- assertTrue(body.contains("\n") && body.contains(" "))
- Or compare against a known pretty-printed string if stable.
34-35: Use classpath resources instead of hard-coded file pathsHard-coded relative paths can break under different working directories or IDEs. Loading from the classpath makes tests more robust.
Example replacement:
URL json = getClass().getResource("/json/unformatted.json"); i.setLocation(new File(json.toURI()).getAbsolutePath());And for charsets:
URL res = getClass().getResource("/charsets/%s.txt".formatted(charset)); i.setLocation(new File(res.toURI()).getAbsolutePath());Also applies to: 77-83
core/src/main/java/com/predic8/membrane/core/beautifier/JSONPrettifier.java (1)
23-28: Avoid per-instance ObjectMapper allocation.Make the mapper static to reduce allocations and allow future global configuration (features, modules) in one place.
Apply:
- private final ObjectMapper om = new ObjectMapper(); + private static final ObjectMapper OM = new ObjectMapper(); @@ - public byte[] prettify(byte[] content) throws IOException { - return om.writerWithDefaultPrettyPrinter().writeValueAsBytes(om.readTree(content)); + public byte[] prettify(byte[] content) throws IOException { + return OM.writerWithDefaultPrettyPrinter().writeValueAsBytes(OM.readTree(content)); }core/src/main/java/com/predic8/membrane/core/interceptor/templating/SoapBodyTemplateInterceptor.java (2)
53-55: Minor formatting: whitespace around concatenations.Readability and consistency with the SOAP 1.1 branch.
Apply:
- case SOAP_11 -> SOAP11_PREFIX + asString( super.getContent(exc, flow)) + SOAP11_POSTFIX; - case SOAP_12 -> SOAP12_PREFIX +asString( super.getContent(exc, flow)) + SOAP12_POSTFIX; + case SOAP_11 -> SOAP11_PREFIX + asString(super.getContent(exc, flow)) + SOAP11_POSTFIX; + case SOAP_12 -> SOAP12_PREFIX + asString(super.getContent(exc, flow)) + SOAP12_POSTFIX;
22-25: Javadoc typo: "intergration" → "integration".Fix the typo in the description.
Apply:
- * @description Renders a SOAP body for legacy intergration + * @description Renders a SOAP body for legacy integrationcore/src/main/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptor.java (2)
44-49: Cache invalidation on re-init.If this interceptor is re-initialized (tests or dynamic reload), the cached pretty output may be stale. Clear it in init().
Apply:
@Override public void init() { - super.init(); + super.init(); + cache = null; }
27-49: Prettify caching is safe but can compute twice under race.The volatile guard is fine; at worst, two threads compute once. If you want to avoid that tiny window, consider double-checked locking; otherwise, this is acceptable for a chill review.
No change required. Documented for awareness.
core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java (2)
154-155: Remove console output in tests.Tests should not print to stdout; it adds noise to CI logs.
Apply:
- System.out.println("body = " + body);
75-78: Prefer deleteIfExists to avoid teardown exceptions.Makes teardown more robust if prior steps failed.
Apply:
- Files.delete(copiedXml); - Files.delete(copiedJson); + Files.deleteIfExists(copiedXml); + Files.deleteIfExists(copiedJson);core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java (2)
61-63: Heuristic for detecting json access could be more robust.textTemplate.contains("json.") may false-positive/negative. Consider a simple regex like "\bjson\." to reduce accidental matches.
Example:
private static final Pattern JSON_ACCESS = Pattern.compile("\\bjson\\."); scriptAccessesJson = JSON_ACCESS.matcher(textTemplate).find();
112-118: Clarify error message when location is null.If using an inline template, the current message includes "null". Improve diagnostics.
Apply:
- } catch (Exception e) { - throw new ConfigurationException("Could not create template from " + location, e); + } catch (Exception e) { + String src = location != null ? location : "inline template"; + throw new ConfigurationException("Could not create template from " + src, e); }core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java (8)
62-64: Prettifier is only initialized during init; later contentType/pretty changes won’t take effect.If contentType or pretty is modified post-init (e.g., via dynamic reconfiguration), the Prettifier remains stale. Consider updating the Prettifier in the respective setters as well (see concrete diffs below for setContentType and setPretty).
106-114: Use configured charset when logging prettify failures to avoid mojibake.The trace log decodes bytes with UTF-8 regardless of the configured charset. Use the configured charset to keep diagnostics readable.
Apply this diff:
- log.trace("Content: {}", truncateAfter(new String(bytes,UTF_8), 100), e); + log.trace("Content: {}", truncateAfter(new String(bytes, charset), 100), e);
118-120: Honor charset in asString().Decoding with the platform default can corrupt non-UTF-8 content. Use the configured charset.
Apply this diff:
- protected String asString(byte[] bytes) { - return new String(bytes); // Encoding - } + protected String asString(byte[] bytes) { + return new String(bytes, charset); + }
122-129: Avoid direct use of router field; use getRouter() consistently.Keeps access uniform and protects against potential shadowing or lifecycle issues.
Apply this diff:
- try (InputStream is = getRouter().getResolverMap().resolve(combine(router.getBaseLocation(), location))) { + try (InputStream is = getRouter().getResolverMap().resolve(combine(getRouter().getBaseLocation(), location))) {
171-174: Update Prettifier when contentType changes while pretty is enabled.Keeps formatting behavior aligned with the configured content type even after init.
Apply this diff:
@MCAttribute public void setContentType(String contentType) { - this.contentType = contentType; + this.contentType = contentType; + if (Boolean.TRUE.equals(pretty)) { + this.prettifier = Prettifier.getInstance(contentType); + } }
186-188: Recalculate Prettifier when toggling pretty.Ensures consistent behavior when pretty is switched on/off dynamically.
Apply this diff:
@MCAttribute public void setPretty(String pretty) { - this.pretty = Boolean.valueOf(pretty); + this.pretty = Boolean.valueOf(pretty); + if (this.pretty) { + this.prettifier = Prettifier.getInstance(contentType); + } else { + this.prettifier = new NullPrettifier(); + } }
76-81: Javadoc nit: wording/grammar and clarity.Minor doc polish for readability.
Apply this diff:
/** - * To handle errors flexible this method can be overwritten - * @param exchange - * @param flow - * @return + * Subclasses may override this method to customize error handling. + * @param exchange the current Exchange + * @param flow the message flow (REQUEST or RESPONSE) + * @return the outcome (CONTINUE or ABORT) */
158-160: Optional: Guard getName() against missing @mcelement for future-proofingAll current subclasses of AbstractTemplateInterceptor (StaticInterceptor.java and TemplateInterceptor.java) declare @mcelement, so there’s no immediate risk of NPE. However, to avoid potential null-pointer issues if a future subclass omits the annotation, you can introduce a safe fallback:
--- a/core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java +++ b/core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java @@ -155,7 +155,11 @@ public abstract class AbstractTemplateInterceptor implements Interceptor { /** * Name of the template element, as declared in the @MCElement annotation. */ - protected String getName() { - return getClass().getAnnotation(MCElement.class).name(); + protected String getName() { + MCElement ann = getClass().getAnnotation(MCElement.class); + return ann != null + ? ann.name() + : getClass().getSimpleName(); }No other changes are required—this is purely an optional refactor to guard against future omissions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (45)
core/src/main/java/com/predic8/membrane/core/Constants.java(1 hunks)core/src/main/java/com/predic8/membrane/core/RuleManager.java(0 hunks)core/src/main/java/com/predic8/membrane/core/beautifier/JSONPrettifier.java(1 hunks)core/src/main/java/com/predic8/membrane/core/beautifier/NullPrettifier.java(1 hunks)core/src/main/java/com/predic8/membrane/core/beautifier/Prettifier.java(1 hunks)core/src/main/java/com/predic8/membrane/core/beautifier/TextPrettifier.java(1 hunks)core/src/main/java/com/predic8/membrane/core/beautifier/XMLPrettifier.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/FlowContext.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/CertsEndpointProcessor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/WellknownEndpointProcessor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/registration/ErrorMessages.java(0 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/registration/SecurityUtils.java(0 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/registration/entity/User.java(0 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/soap/SampleSoapServiceInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/templating/SoapBodyTemplateInterceptor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/Rewrite.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/FileUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/soap/SoapVersion.java(1 hunks)core/src/test/java/com/predic8/membrane/core/RouterTest.java(5 hunks)core/src/test/java/com/predic8/membrane/core/beautifier/JSONBeautifierTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/beautifier/TextPrettifierTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/AdjustContentLengthTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2RedirectTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/templating/SoapBodyTemplateInterceptorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java(8 hunks)core/src/test/java/com/predic8/membrane/core/transport/http/MassivelyParallelTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/soap/SoapVersionTest.java(1 hunks)core/src/test/resources/charsets/iso-8859-1.txt(1 hunks)core/src/test/resources/charsets/utf-8.txt(1 hunks)core/src/test/resources/json/unformatted.json(1 hunks)distribution/examples/web-services-soap/rest2soap-template/README.md(1 hunks)distribution/examples/web-services-soap/rest2soap-template/proxies.xml(1 hunks)distribution/examples/web-services-soap/rest2soap/README.md(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/rest2soap/Rest2SOAPTemplateExampleTest.java(2 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/ConfigurationPropertiesTest.java(2 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/OutgoingAPIGatewayExampleTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/VersioningSoapXsltExampleTest.java(2 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/SampleSoapServiceExampleTest.java(1 hunks)docs/ROADMAP.md(1 hunks)pom.xml(1 hunks)test/src/main/java/com/predic8/membrane/test/OAuth2AuthFlowClient.java(2 hunks)
💤 Files with no reviewable changes (4)
- core/src/main/java/com/predic8/membrane/core/interceptor/registration/entity/User.java
- core/src/main/java/com/predic8/membrane/core/interceptor/registration/ErrorMessages.java
- core/src/main/java/com/predic8/membrane/core/interceptor/registration/SecurityUtils.java
- core/src/main/java/com/predic8/membrane/core/RuleManager.java
🧰 Additional context used
🧬 Code Graph Analysis (9)
core/src/main/java/com/predic8/membrane/core/beautifier/Prettifier.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
core/src/test/java/com/predic8/membrane/core/util/soap/SoapVersionTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
core/src/main/java/com/predic8/membrane/core/beautifier/XMLPrettifier.java (1)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (1)
TextUtil(30-254)
core/src/test/java/com/predic8/membrane/core/beautifier/TextPrettifierTest.java (1)
core/src/main/java/com/predic8/membrane/core/beautifier/TextPrettifier.java (1)
TextPrettifier(18-78)
core/src/main/java/com/predic8/membrane/core/util/soap/SoapVersion.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/WellknownEndpointProcessor.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java (3)
core/src/main/java/com/predic8/membrane/core/Constants.java (1)
Constants(24-96)core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
Request(32-354)
core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java (2)
core/src/main/java/com/predic8/membrane/core/util/FileUtil.java (1)
FileUtil(24-52)core/src/main/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptor.java (1)
MCElement(22-50)
🪛 LanguageTool
distribution/examples/web-services-soap/rest2soap-template/README.md
[locale-violation] ~27-~27: In American English, “take a look” is more commonly used.
Context: ...population": "11346800" } ``` 5. Have a look at proxies.xml to see how it works. ...
(HAVE_A_LOOK)
docs/ROADMAP.md
[grammar] ~37-~37: There might be a mistake here.
Context: ... source and outputEncoding to UTF-8 (TB) - TemplateInterceptor Refactoring (TB) - T...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...) - TemplateInterceptor Refactoring (TB) - Template/Static Interceptor: Pretty for ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ... Pretty for text/* (Refactor first) (TB) - Pretty on text should trim whitespace in...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...ode from above - Describe RPM Setup (TP) - examples/routing-traffic/outgoing-api-ga...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...ook Book: outgoing-api-gateway (TB) done - READMEs in example folders listing the e...
(QB_NEW_EN)
🔇 Additional comments (32)
distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/OutgoingAPIGatewayExampleTest.java (1)
1-14: License header addition looks good.Standard Apache 2.0 header added correctly; no functional impact. Spacing before the package declaration is clean.
distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/ConfigurationPropertiesTest.java (2)
1-14: License header addition looks good.Standard Apache 2.0 header added correctly; no functional impact.
32-32: Good use ofignoredfor unused resource variable.Renaming the try-with-resources variable to
ignoredclearly communicates it’s intentionally unused and avoids warnings, with no behavioral change.core/src/test/java/com/predic8/membrane/core/transport/http/MassivelyParallelTest.java (1)
1-14: License header correctly added and positioned.Header text and placement before the package declaration look good and consistent with Apache 2.0 requirements.
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/FlowContext.java (1)
1-14: License header addition looks good.Apache 2.0 header is correct and non-invasive to behavior.
core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/Rewrite.java (1)
86-86: Typo fix in Javadoc looks good.Thanks for correcting the mojibake in the @throws description.
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/SampleSoapServiceExampleTest.java (1)
62-63: Test data update to "Stebbach" looks correct.Using an unknown city to assert the 404 fault payload aligns with the intent of the test.
distribution/src/test/java/com/predic8/membrane/examples/withinternet/rest2soap/Rest2SOAPTemplateExampleTest.java (1)
21-21: Static Hamcrest imports: LGTMSwitching to
static org.hamcrest.Matchers.*is cleaner and consistent with the rest of the tests.core/src/test/resources/charsets/utf-8.txt (1)
1-1: Confirmed: UTF-8 sample is correct, no BOM detected
Verified that core/src/test/resources/charsets/utf-8.txt decodes to “äöüÄÖÜßéèê” and contains no Byte Order Mark.core/src/main/java/com/predic8/membrane/core/interceptor/soap/SampleSoapServiceInterceptor.java (1)
114-124: City data corrections/additions look good (Berlin added; São Paulo normalized).Non-ASCII in identifiers and XML content relies on consistent UTF-8 handling, which the POM changes support. No functional or threading concerns introduced here.
test/src/main/java/com/predic8/membrane/test/OAuth2AuthFlowClient.java (2)
177-180: Assertion/log refactor: LGTM.Switching to static imports for LogDetail.ALL and is(...) is clean and preserves behavior.
16-31: Verify wildcard/static imports comply with project style rulesI didn’t find any Checkstyle or Spotless configuration in the repo forbidding star imports. Please confirm whether wildcard or static imports are allowed for tests. If your CI enforces an import‐ordering or “no star import” rule (via Checkstyle, Spotless, IDE settings, etc.), convert these to explicit imports to prevent potential build failures.
• File to check:
test/src/main/java/com/predic8/membrane/test/OAuth2AuthFlowClient.java (lines 16–31)distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/VersioningSoapXsltExampleTest.java (1)
50-51: Hamcrest static import migration looks good.Switching to equalTo(...) with static imports improves readability and matches the broader test style migration in this PR. No behavior change.
Also applies to: 57-57
core/src/test/java/com/predic8/membrane/core/interceptor/AdjustContentLengthTest.java (2)
62-62: JUnit 5 visibility change is correct.Making the test method package-private is fine under JUnit 5; no need for public.
80-85: Minor style cleanup is fine.The one-liner GetMethod constructors improve readability without behavior change.
core/src/test/java/com/predic8/membrane/core/RouterTest.java (1)
25-27: LGTM: switch to static RestAssured and LogDetail importsThe static imports improve readability of the given/when/then DSL.
core/src/test/java/com/predic8/membrane/core/util/soap/SoapVersionTest.java (1)
45-49: LGTM: content-type mapping assertions are correct and valuable.The mapping of SOAP_11 -> text/xml and SOAP_12 -> application/soap+xml is validated clearly.
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/WellknownEndpointProcessor.java (1)
48-49: No changes needed—ResponseBuilder.body(byte[])exists.
TheResponseclass defines abody(byte[])overload (see Response.java:93–96), so passing abyte[]compiles correctly.core/src/test/java/com/predic8/membrane/core/beautifier/TextPrettifierTest.java (1)
22-99: Solid coverage of normalization and indentation casesGood breadth across trimming, dedentation, tab handling, and internal blank lines. This is a solid baseline for the new TextPrettifier behavior.
core/src/test/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptorTest.java (1)
46-55: Add a test for content type with charset to guard factory behaviorGiven real-world headers like "application/json; charset=UTF-8", add a test ensuring pretty mode still kicks in. This will prevent regressions in Prettifier.getInstance.
I can add a test variant:
- i.setPretty("true");
- i.setContentType("application/json; charset=UTF-8");
- assertTrue(pretty-printed length > original length).
core/src/main/java/com/predic8/membrane/core/beautifier/JSONPrettifier.java (1)
21-28: Byte-oriented prettifier implementation looks correct.Conforms to the new Prettifier API and uses Jackson's pretty printer properly. Thread-safe use of ObjectMapper is fine in this context.
core/src/main/java/com/predic8/membrane/core/interceptor/templating/SoapBodyTemplateInterceptor.java (2)
58-61: Delegating content-type to SoapVersion is clean.Good use of SoapVersion.getContentType() to centralize MIME handling.
67-70: Version parsing via SoapVersion.parse(version) is a nice simplification.Externalizing version parsing removes duplication here.
core/src/main/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptor.java (1)
38-41: Explicit UTF-8 in getContent is correct.Avoids platform-default charset pitfalls; aligns with the new byte-oriented flow.
core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java (3)
229-233: Content-Type assertion for JSON is precise.Good to assert the exact content-type after rendering from a .json template.
258-269: Invalid JSON prettify fallback test is valuable.Confirms graceful degradation when prettification fails.
251-256: No router needed for init() in TemplateInterceptorTest
The no-arg init() in AbstractInterceptor is a no-op and router injection only occurs in init(Router), so calling init() in this test is safe. No changes required.core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java (4)
119-130: Engine selection and content-type setting looks good.XML location → XmlTemplateEngine with APPLICATION_XML; JSON location → StreamingTemplateEngine with APPLICATION_JSON.
55-63: Template creation lifecycle is sound.Creating the template once in init() and caching it is appropriate; the JSON-access flag is a pragmatic optimization.
64-95: Error handling maps nicely to Problem Details.Good separation between Groovy template errors and runtime exceptions with proper responses and ABORT outcome.
55-57: super.init() is safe when router is unsetAbstractTemplateInterceptor.init() only calls getRouter() inside readFromLocation(), and that method is invoked only if getLocation() is non-null. When no location is set (the common test scenario), init() does not dereference the router—no changes needed.
core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java (1)
55-60: Good: Enforces mutual exclusivity between location and inline template.Throwing a ConfigurationException when both are set is the right call and prevents ambiguous configuration.
core/src/main/java/com/predic8/membrane/core/beautifier/Prettifier.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/beautifier/TextPrettifier.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/beautifier/XMLPrettifier.java
Outdated
Show resolved
Hide resolved
...ain/java/com/predic8/membrane/core/interceptor/oauth2/processors/CertsEndpointProcessor.java
Outdated
Show resolved
Hide resolved
.../main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/beautifier/JSONBeautifierTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/beautifier/JSONBeautifierTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/beautifier/JSONBeautifierTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/beautifier/JSONBeautifierTest.java
Outdated
Show resolved
Hide resolved
|
/ok-to-test |
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/http/Request.java (1)
304-308: Ensure JSON builder uses UTF-8 and advertise itBuilder.json(String) still uses platform-default
.getBytes()and setsapplication/jsonwithout charset. For consistency and predictability, encode in UTF-8 and consider sendingapplication/json;charset=utf-8.You can update it like this (outside the changed hunk):
public Builder json(String body) { req.setBodyContent(body.getBytes(StandardCharsets.UTF_8)); req.header.setContentType(MimeType.APPLICATION_JSON_UTF8); return this; }If changing the content-type might be risky for some consumers, at least switch to
getBytes(UTF_8)and keep the current content-type.
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java (1)
98-101: UTF-8 byte conversion applied as previously suggested.The prior review asked to avoid platform defaults when converting to bytes. Using getBytes(UTF_8) addresses that.
🧹 Nitpick comments (29)
core/src/main/java/com/predic8/membrane/core/openapi/model/StringBody.java (2)
23-24: Avoid wildcard static import; import only UTF_8 (also fixes inconsistency with summary).Only UTF_8 is used; narrowing the import matches typical style and the PR summary.
-import static java.nio.charset.StandardCharsets.*; +import static java.nio.charset.StandardCharsets.UTF_8;
42-45: Use ObjectMapper.readTree(String) to avoid unnecessary byte[] allocation; optionally guard against null payload.The switch to explicit UTF-8 is good. Since you already have a String, feeding bytes is unnecessary and allocates an extra array. Using readTree(String) is simpler and faster. Optionally, fail fast with a clear message if payload is null to avoid an NPE in getBytes.
- return om.readValue(payload.getBytes(UTF_8), JsonNode.class); + if (payload == null) + throw new IllegalStateException("StringBody payload must not be null"); + return om.readTree(payload);If readTree(String) is not available in your Jackson version, fallback to:
return om.readValue(payload, JsonNode.class);core/src/main/java/com/predic8/membrane/core/http/cookie/Cookies.java (1)
19-21: Avoid wildcard imports; prefer explicit types for clarity and to satisfy common Checkstyle rules.Using
org.slf4j.*andjava.io.*can introduce ambiguity and often violates "AvoidStarImport" rules. Import only what’s used.Apply this diff:
-import org.slf4j.*; - -import java.io.*; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.PrintWriter; +import java.io.StringWriter;core/src/test/java/com/predic8/membrane/core/RouterTest.java (4)
50-61: Make Content-Type assertion resilient to charsetGiven the PR’s shift to explicit UTF-8 handling, servers often append charset to Content-Type. RestAssured’s
contentType(String)is strict and may fail if the header isapplication/problem+json; charset=UTF-8. Prefer a matcher.Apply this diff:
- .contentType(APPLICATION_PROBLEM_JSON) + .contentType(containsString(APPLICATION_PROBLEM_JSON))
65-77: prodXML: Robust Content-Type check; also verify XML root element naming
- Similar to JSON, make the Content-Type check tolerant to an appended charset.
- The prod XML assertions use
error.*while dev usesproblem-details.*. If this is intentional (different formats per mode), all good. If not, it’s worth aligning the element names to avoid confusion.Apply this diff to harden the Content-Type check:
- .contentType(APPLICATION_XML) + .contentType(containsString(APPLICATION_XML))
83-93: devJson: Harden assertions and align with charset-tolerant Content-Type
- Content-Type assertion: make it tolerant to charset.
- Consider asserting presence (and possibly contents) of
messagesimilar to devXML to prevent regressions. If JSON intentionally omitsstacktracewhile XML includes it, please confirm this divergence is by design.- Nit: Tests mix GET (here) and POST (others) even though the interceptor throws for any method; consider using a single method across tests for consistency.
Proposed diff:
- .get("http://localhost:2001/") + .post("http://localhost:2001/") .then() .statusCode(500) - .contentType(APPLICATION_PROBLEM_JSON) + .contentType(containsString(APPLICATION_PROBLEM_JSON)) .body("title", equalTo("Internal server error.")) .body("type",equalTo("https://membrane-api.io/problems/internal")) .body("$",hasKey("attention")) .body("attention", Matchers.containsString("development mode")) + .body("$",hasKey("message")) .body("$",not(hasKey("stacktrace")));
97-111: devXML: Use constant for the secret; make Content-Type tolerant to charset
- Replace the hard-coded
"supersecret"with theINTERNAL_SECRETconstant for consistency and maintainability.- Make the Content-Type check tolerant to charset.
Apply these diffs:
- .contentType(APPLICATION_XML) + .contentType(containsString(APPLICATION_XML))- .body("problem-details.message", Matchers.containsString("supersecret")) + .body("problem-details.message", Matchers.containsString(INTERNAL_SECRET))core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java (2)
33-33: Avoid wildcard static import; import only UTF_8Keeps imports tight and avoids pulling unused constants.
-import static java.nio.charset.StandardCharsets.*; +import static java.nio.charset.StandardCharsets.UTF_8;
46-49: Sweep for default-charsetgetBytes()usages
I ran the suggested search and found numerous calls toString.getBytes()(without charset) across the repo—including in production code and utilities:
• core/src/main/java/com/predic8/membrane/core/transport/ws/WebSocketConnection.java:145
• core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java:429
• core/src/main/java/com/predic8/membrane/core/transport/ssl/StaticSSLContext.java:432
• core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java:239
• core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java:134
• core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtSignInterceptor.java:89
• core/src/main/java/com/predic8/membrane/core/http/Request.java:305Consider updating these (and any others) to use an explicit charset—for example:
- msg.getBytes() + msg.getBytes(StandardCharsets.UTF_8)This is an optional cleanup to avoid platform-dependent charset issues.
pom.xml (1)
559-561: Also propagate file.encoding to Failsafe (ITs)Integration tests may still run with platform defaults. Mirroring the surefire argLine in failsafe keeps behavior consistent.
Add this configuration to the maven-failsafe-plugin (within pluginManagement alongside the existing plugin):
<plugin> <artifactId>maven-failsafe-plugin</artifactId> <version>3.1.2</version> <configuration> <argLine>-Dfile.encoding=UTF-8</argLine> </configuration> </plugin>core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java (1)
317-318: Use UTF-8-aware MimeType in fallback response
The fallback path is explicitly encoding the body as UTF-8 bytes, butTEXT_PLAINis defined as"text/plain"with no charset. To ensure clients decode correctly, switch to the UTF-8 variant:• In
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java(around line 318), change:- builder.contentType(TEXT_PLAIN); + builder.contentType(TEXT_PLAIN_UTF8);• Ensure you have a static import for
TEXT_PLAIN_UTF8:import static com.predic8.membrane.core.http.MimeType.TEXT_PLAIN_UTF8;core/src/main/java/com/predic8/membrane/core/resolver/HTTPSchemaResolver.java (3)
49-49: Make NO_HASH a static constantReads better as a class-level constant and avoids per-instance allocation.
- final byte[] NO_HASH = "NO_HASH".getBytes(UTF_8); + private static final byte[] NO_HASH = "NO_HASH".getBytes(UTF_8);
17-17: Prefer specific import over wildcard (Guava collect)Keeps imports minimal and clearer.
-import com.google.common.collect.*; +import com.google.common.collect.Lists;
38-38: Avoid wildcard static import; import only UTF_8Same rationale as elsewhere in the PR.
-import static java.nio.charset.StandardCharsets.*; +import static java.nio.charset.StandardCharsets.UTF_8;core/src/main/java/com/predic8/membrane/core/interceptor/json/ReplaceInterceptor.java (1)
16-26: Prefer explicit imports over wildcardsWildcard imports reduce clarity and can cause accidental symbol shadowing, especially in a large codebase.
Consider replacing
*with explicit imports for com.jayway.jsonpath and internal packages.core/src/main/java/com/predic8/membrane/core/prettifier/NullPrettifier.java (1)
17-19: Typo in Javadoc (“coping” → “copying”) and clarify aliasingMinor typo. Also worth clarifying that this returns the same reference (zero-copy), so callers must not mutate the returned array if they don't own it.
Apply:
-/** - * Returns the reference to the provided byte array without coping! - */ +/** + * Returns the reference to the provided byte array without copying. + * Note: zero-copy aliasing — callers must not mutate the returned array unless they own it. + */core/src/test/java/com/predic8/membrane/core/prettifier/JSONPrettifierTest.java (1)
37-47: Strengthen assertions beyond whitespace presenceCurrent checks for " " and "\n" are weak indicators of pretty printing. Consider asserting structural formatting (e.g., indentation length) or round-tripping through a JSON parser to verify semantic equivalence. Also add a non-ASCII sample to guard against encoding regressions.
Example additional test (outside the changed hunk):
@Test void prettify_preservesNonAsciiAndStructure() throws Exception { String in = "{\"msg\":\"Hällo Müße – 你好\"}"; byte[] pretty = beautifier.prettify(in.getBytes(StandardCharsets.UTF_8)); String out = new String(pretty, StandardCharsets.UTF_8); assertTrue(out.contains("Hällo Müße – 你好")); // Simple structural signal: key on its own line after prettify assertTrue(out.contains("\n \"msg\"")); }core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/CertsEndpointProcessor.java (1)
45-64: Consider caching headers for JWKS endpointJWKS responses are typically cacheable and change infrequently. Consider adding Cache-Control/ETag to reduce load and improve client performance.
Example:
- Add
Cache-Control: public, max-age=300and anETagderived from the JWKS content.core/src/main/java/com/predic8/membrane/core/prettifier/JSONPrettifier.java (1)
23-28: Optionally enable tolerant JSON parsing for robustness.If inputs can contain “JSON5-like” variants (comments, trailing commas, single quotes), consider enabling Jackson read features to prettify without failing hard.
Apply this diff if you want a more permissive reader:
-import com.fasterxml.jackson.databind.*; +import com.fasterxml.jackson.databind.*; +import com.fasterxml.jackson.databind.json.JsonMapper; +import com.fasterxml.jackson.core.json.JsonReadFeature; @@ - private static final ObjectMapper om = new ObjectMapper(); + private static final ObjectMapper om = JsonMapper.builder() + .enable(JsonReadFeature.ALLOW_JAVA_COMMENTS) + .enable(JsonReadFeature.ALLOW_TRAILING_COMMA) + .enable(JsonReadFeature.ALLOW_SINGLE_QUOTES) + .enable(JsonReadFeature.ALLOW_UNQUOTED_FIELD_NAMES) + .build();core/src/main/java/com/predic8/membrane/core/prettifier/XMLPrettifier.java (1)
25-27: Detect and Preserve XML Encoding Instead of Hard-coding UTF-8
Hard-coding UTF-8 in
new InputStreamReader(new ByteArrayInputStream(c), UTF_8)
will break documents that declare a different charset in their XML prolog. SinceTextUtil.formatXML(...)only accepts aReader, you have two main options:• Extend
TextUtil.formatXMLwith a new overload that takes anInputStream(orbyte[]) so the XML parser can auto-detect the encoding from the prolog.
• Or add lightweight prolog sniffing inXMLPrettifier.prettify(...):
- Peek at the first few bytes of the input to extract
encoding="…"from the XML declaration.- Instantiate the
InputStreamReaderwith the detected charset instead of hard-codingUTF_8.File to update:
• core/src/main/java/com/predic8/membrane/core/prettifier/XMLPrettifier.java (around lines 25–27)core/src/test/java/com/predic8/membrane/core/prettifier/PrettifierTest.java (1)
23-46: Add coverage for SOAP, JOSE+JSON, HTML, and GraphQL mappings.These MIME types are common in gateways; adding assertions locks in expected behavior and prevents regressions.
Apply this diff to extend coverage (matches current implementation that maps HTML to XML):
class PrettifierTest { @@ void json() { assertEquals(JSON, getInstance(APPLICATION_JSON)); assertEquals(JSON, getInstance(APPLICATION_JSON_UTF8)); assertEquals(JSON, getInstance(APPLICATION_PROBLEM_JSON)); + assertEquals(JSON, getInstance(APPLICATION_JOSE_JSON)); } @@ void xml() { assertEquals(XML, getInstance(APPLICATION_XML)); assertEquals(XML, getInstance(TEXT_XML)); assertEquals(XML, getInstance(TEXT_XML_UTF8)); + assertEquals(XML, getInstance(APPLICATION_SOAP)); } @@ void text() { assertEquals(TEXT, getInstance(TEXT_PLAIN)); assertEquals(TEXT, getInstance(TEXT_PLAIN_UTF8)); assertEquals(TEXT, getInstance("trash")); + assertEquals(TEXT, getInstance(APPLICATION_GRAPHQL)); } + + @Test + void html() { + assertEquals(XML, getInstance(TEXT_HTML)); + assertEquals(XML, getInstance(TEXT_HTML_UTF8)); + }core/src/test/java/com/predic8/membrane/core/prettifier/TextPrettifierTest.java (1)
94-101: Add a quick null/blank input test.TextPrettifier.normalizeMultiline explicitly handles null/blank. Lock this behavior in with a small test.
Apply this diff:
@Test void trimTrailingWhitespace() throws Exception { assertPretty("a\nb", "a \nb\t \t"); } + @Test + void nullAndBlank() { + assertEquals("", normalizeMultiline(null)); + assertEquals("", normalizeMultiline(" \n\t \n")); + } + private void assertPretty(String exp, String actual) throws Exception { assertEquals(exp, new String(tp.prettify(actual.getBytes(UTF_8)), UTF_8)); }core/src/main/java/com/predic8/membrane/core/prettifier/Prettifier.java (2)
31-33: Lowercasing the entire Content-Type is unnecessary and may alter parameter values; trim is sufficient.MimeType checks are case-insensitive for type/subtype. Keeping parameters intact is cleaner.
Apply this diff:
- String ct = contentType.toLowerCase(java.util.Locale.ROOT).trim(); + String ct = contentType.trim();
33-41: Reconsider HTML→XML mapping in Prettifier
TextUtil.formatXML(reader) (used by XMLPrettifier) always uses the plain XML beautifier (asHTML=false), so feeding it text/html will either error or produce mangled output. Until we add a true HTML formatter, it’s safer to treat HTML as plain text.Affected location:
- core/src/main/java/com/predic8/membrane/core/prettifier/Prettifier.java (around lines 33–41)
Proposed change:
- // XML/HTML family: text/xml, application/xml, text/html (and charset variants) - if (isXML(ct) || isHtml(ct)) + // XML family only; HTML may not be well-formed XML and should be treated as TEXT for now + if (isXML(ct)) return XML;core/src/main/java/com/predic8/membrane/core/prettifier/TextPrettifier.java (1)
26-75: Multiline normalization logic is sound; minor robustness nits only.
- The de-indentation via indexOf(line.trim()) is fine and efficient for typical whitespace. If you need to recognize json[...] access patterns in future (tabs vs spaces), this is already safe since you compute by character count, not visual columns.
- Consider clarifying the comment on Line 32: you keep trailing empty lines during split, but intentionally strip them a few lines later. A small wording tweak could reduce confusion.
No change required; just a note.
core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java (2)
60-63: Broaden json-variable detection to catch bracket access and whitespace.textTemplate.contains("json.") misses patterns like json['x'] or json [ "x" ]. A lightweight regex improves coverage without adding heavy dependencies.
Apply:
- // If the template accesses somewhere the json variable make sure it is there - // You can even access json in an XML or Text Template. See tests. - scriptAccessesJson = textTemplate.contains("json."); + // Enable JSON binding if the template accesses the `json` variable (json.<...> or json[...]). + // Keep the check cheap but a bit broader than a plain contains("json."). + scriptAccessesJson = java.util.regex.Pattern.compile("(?m)(^|\\W)json\\s*(\\.|\\[)") + .matcher(textTemplate) + .find();
124-135: Use the existing static import consistently.You already statically import FileUtil.* (Line 32). Prefer isJson(location) for consistency with isXml(location).
- if (FileUtil.isJson(location)) { + if (isJson(location)) { setContentType(APPLICATION_JSON); }core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java (1)
250-255: Avoid platform-default charset in byte[] assertions.Make the test deterministic across platforms by using UTF-8 explicitly.
- assertArrayEquals(expectedPrettyJson.getBytes(), ti.prettify(inputJson.getBytes())); + assertArrayEquals( + expectedPrettyJson.getBytes(java.nio.charset.StandardCharsets.UTF_8), + ti.prettify(inputJson.getBytes(java.nio.charset.StandardCharsets.UTF_8)) + );core/src/main/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptor.java (1)
45-50: Cache strategy is thread-safe enough and effective; minor nit about unused logger.
- The volatile cache with benign race on first computation is acceptable here and avoids repeated prettify work.
- Logger is currently unused; consider removing it to reduce noise, or use it for cache debug/trace.
- protected static final Logger log = LoggerFactory.getLogger(StaticInterceptor.class); + // (Optional) Re-introduce when needed for diagnostics. + // protected static final Logger log = LoggerFactory.getLogger(StaticInterceptor.class);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (40)
.gitattributes(1 hunks)core/src/main/java/com/predic8/membrane/core/Constants.java(0 hunks)core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java(2 hunks)core/src/main/java/com/predic8/membrane/core/http/Request.java(3 hunks)core/src/main/java/com/predic8/membrane/core/http/cookie/Cookies.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/json/ReplaceInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/CertsEndpointProcessor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/WellknownEndpointProcessor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/soap/SampleSoapServiceInterceptor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/templating/SoapBodyTemplateInterceptor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/model/StringBody.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/util/Utils.java(0 hunks)core/src/main/java/com/predic8/membrane/core/prettifier/JSONPrettifier.java(1 hunks)core/src/main/java/com/predic8/membrane/core/prettifier/NullPrettifier.java(1 hunks)core/src/main/java/com/predic8/membrane/core/prettifier/Prettifier.java(1 hunks)core/src/main/java/com/predic8/membrane/core/prettifier/TextPrettifier.java(1 hunks)core/src/main/java/com/predic8/membrane/core/prettifier/XMLPrettifier.java(1 hunks)core/src/main/java/com/predic8/membrane/core/resolver/HTTPSchemaResolver.java(3 hunks)core/src/main/java/com/predic8/membrane/core/transport/ws/WebSocketConnection.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/FileUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/soap/SoapVersion.java(1 hunks)core/src/test/java/com/predic8/membrane/core/RouterTest.java(6 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/AdjustContentLengthTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2RedirectTest.java(5 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/templating/SoapBodyTemplateInterceptorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java(8 hunks)core/src/test/java/com/predic8/membrane/core/prettifier/JSONPrettifierTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/prettifier/PrettifierTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/prettifier/TextPrettifierTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/soap/SoapVersionTest.java(1 hunks)distribution/examples/web-services-soap/rest2soap-template/README.md(1 hunks)distribution/examples/web-services-soap/rest2soap/README.md(2 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/rest2soap/Rest2SOAPTemplateExampleTest.java(2 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/SampleSoapServiceExampleTest.java(1 hunks)pom.xml(2 hunks)
💤 Files with no reviewable changes (2)
- core/src/main/java/com/predic8/membrane/core/openapi/util/Utils.java
- core/src/main/java/com/predic8/membrane/core/Constants.java
✅ Files skipped from review due to trivial changes (1)
- core/src/main/java/com/predic8/membrane/core/transport/ws/WebSocketConnection.java
🚧 Files skipped from review as they are similar to previous changes (15)
- distribution/examples/web-services-soap/rest2soap/README.md
- distribution/examples/web-services-soap/rest2soap-template/README.md
- core/src/main/java/com/predic8/membrane/core/interceptor/templating/SoapBodyTemplateInterceptor.java
- core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2RedirectTest.java
- core/src/test/java/com/predic8/membrane/core/interceptor/AdjustContentLengthTest.java
- core/src/main/java/com/predic8/membrane/core/util/soap/SoapVersion.java
- core/src/main/java/com/predic8/membrane/core/interceptor/soap/SampleSoapServiceInterceptor.java
- distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/SampleSoapServiceExampleTest.java
- core/src/main/java/com/predic8/membrane/core/util/FileUtil.java
- core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/WellknownEndpointProcessor.java
- distribution/src/test/java/com/predic8/membrane/examples/withinternet/rest2soap/Rest2SOAPTemplateExampleTest.java
- core/src/test/java/com/predic8/membrane/core/interceptor/templating/SoapBodyTemplateInterceptorTest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java
- core/src/test/java/com/predic8/membrane/core/util/soap/SoapVersionTest.java
- core/src/test/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptorTest.java
🧰 Additional context used
🧬 Code Graph Analysis (5)
core/src/main/java/com/predic8/membrane/core/interceptor/json/ReplaceInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
core/src/main/java/com/predic8/membrane/core/prettifier/Prettifier.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
core/src/test/java/com/predic8/membrane/core/prettifier/TextPrettifierTest.java (1)
core/src/main/java/com/predic8/membrane/core/prettifier/TextPrettifier.java (1)
TextPrettifier(19-78)
core/src/test/java/com/predic8/membrane/core/prettifier/PrettifierTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
Request(32-354)
🔇 Additional comments (17)
core/src/test/java/com/predic8/membrane/core/RouterTest.java (1)
25-25: Static RestAssured imports improve readabilityGood move to static imports; the fluent chains are cleaner and easier to scan.
core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java (1)
48-48: Explicit UTF-8 for JsonPath syntax check — LGTMGood move making the charset explicit here; removes platform-default ambiguity.
.gitattributes (1)
3-6: Pinning test fixture encodings — LGTMExplicit working-tree encodings for charset fixtures will make tests deterministic across platforms.
pom.xml (1)
75-76: Standardizing build/report encodings to UTF-8 — LGTMConsistent with the code changes and helps avoid platform-dependent behavior.
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java (1)
32-35: Static imports and explicit UTF-8 usage — LGTMConsistent with the PR’s UTF-8 standardization; no functional risks introduced.
core/src/main/java/com/predic8/membrane/core/http/Request.java (2)
194-201: LGTM: STOMP header encoding explicitly set to UTF-8Using UTF_8 here removes platform-default ambiguity. Good change.
280-283: LGTM: Builder.body(String) now uses UTF-8This aligns with the repo-wide explicit UTF-8 posture.
core/src/test/java/com/predic8/membrane/core/prettifier/JSONPrettifierTest.java (1)
50-55: LGTM: Idempotency test is valuableByte-for-byte idempotency is a solid property for a prettifier. Good test.
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/CertsEndpointProcessor.java (2)
34-35: Migration to JSONPrettifier and explicit UTF-8 looks goodSwitching to the new prettifier API and using UTF-8 is consistent with the PR’s direction. Response uses APPLICATION_JSON_UTF8 — good.
54-56: CatchingIOExceptionis correct here; just remove the stray spaceThe concrete type
JSONPrettifierdeclaresthrows IOException, so there’s no need to catchException. You can safely keep the existingcatch (IOException e)and apply only the minor whitespace fix:Locations to update:
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/CertsEndpointProcessor.java:55Suggested diff:
- exc.setResponse(ok().contentType(APPLICATION_JSON_UTF8) - .body(prettifier.prettify( jwks.getBytes(UTF_8))).build()); + exc.setResponse(ok().contentType(APPLICATION_JSON_UTF8) + .body(prettifier.prettify(jwks.getBytes(UTF_8))).build());Likely an incorrect or invalid review comment.
core/src/main/java/com/predic8/membrane/core/prettifier/JSONPrettifier.java (1)
21-28: Solid, minimal, and thread-safe JSON prettifier.Uses a shared ObjectMapper and the default pretty printer correctly; exception type is narrower than the interface and fine. Good UTF-8 default handling via Jackson’s byte[] path.
core/src/test/java/com/predic8/membrane/core/prettifier/TextPrettifierTest.java (1)
23-102: Comprehensive whitespace normalization coverage.Good breadth across indentation, trailing whitespace, blank line handling, CRLFs, and tabs.
core/src/main/java/com/predic8/membrane/core/prettifier/TextPrettifier.java (1)
21-24: Explicit UTF-8 handling in prettify() is correct and consistent.Using UTF_8 for both decoding and encoding makes the output deterministic across platforms.
core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java (1)
65-96: Error handling is clear and user-friendly.Mapping Groovy/templating exceptions to ProblemDetails with production-aware stack traces is solid.
core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java (2)
95-108: Nice coverage of JSON access from a TEXT template.This validates the new scriptAccessesJson gating and content-type separation. Looks good.
190-196: Mutually exclusive configuration check validated.Asserting a ConfigurationException when both location and inline template are set helps prevent ambiguous setups.
core/src/main/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptor.java (1)
39-42: Explicit UTF-8 for static content is correct.Returning textTemplate.getBytes(UTF_8) avoids platform-specific behavior.
core/src/main/java/com/predic8/membrane/core/interceptor/json/ReplaceInterceptor.java
Outdated
Show resolved
Hide resolved
|
@rrayst Wait to review that PR - Need to fix more |
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/RequestParameterizedTest.java (1)
77-85: Use literal replacement for regex; optional: simplify anonymous Callable to a lambdaYour pattern is properly quoted with
Pattern.quote(value), but the replacement string is still interpreted as a regex replacement and may treat$and\specially. Quote the replacement too to avoid surprises.Minimal fix:
- exc.getRequest().setUri(exc.getRequest().getUri().replaceFirst(Pattern.quote(value), replacement)); + exc.getRequest().setUri( + exc.getRequest() + .getUri() + .replaceFirst(Pattern.quote(value), java.util.regex.Matcher.quoteReplacement(replacement)) + );Optional cleanup: since this override no longer throws, you can use a lambda to reduce noise (returning
nullis fine as the return value is unused).- return new Callable<>() { - @Override - public Object call() { - exc.getRequest().setUri(exc.getRequest().getUri().replaceFirst(Pattern.quote(value), replacement)); - return this; - } - }; + return () -> { + exc.getRequest().setUri( + exc.getRequest() + .getUri() + .replaceFirst(Pattern.quote(value), java.util.regex.Matcher.quoteReplacement(replacement)) + ); + return null; + };core/src/main/java/com/predic8/membrane/core/interceptor/administration/AdminRESTInterceptor.java (1)
181-189: Guard against null content type when echoing the bodyexc.get(Response|Request)ContentType may be null. Passing null into ResponseBuilder.contentType(...) likely triggers a NullPointerException in Header.setContentType. Add a null check and only set it when non-null; otherwise, omit or default.
Apply this diff:
- ResponseBuilder rb = ok().contentType(ct).body(msg.getBodyAsStream(), false); + ResponseBuilder rb = ok(); + if (ct != null) { + rb = rb.contentType(ct); + } + rb = rb.body(msg.getBodyAsStream(), false); String contentEncoding = msg.getHeader().getContentEncoding(); if (contentEncoding != null) rb.header(CONTENT_ENCODING, contentEncoding); return rb.build();core/src/test/java/com/predic8/membrane/core/http/HeaderTest.java (1)
120-123: Fix duplicated assertion: actually test single-header normalizationBoth tests currently assert on h1 (multiple headers). Make this one verify normalization from a single combined header value like the h2 example.
- void getNormalizedValueFromOneHeader() { - assertEquals("192.3.14.1,10.0.0.1,2001:db8:85a3:8d3:1319:8a2e:370:7348", h1.getNormalizedValue(X_FORWARDED_FOR)); - } + void getNormalizedValueFromOneHeader() { + Header h2 = new Header(); + h2.add(X_FORWARDED_FOR, " 192.3.14.1 , \t10.0.0.1,2001:db8:85a3:8d3:1319:8a2e:370:7348\t "); + assertEquals("192.3.14.1,10.0.0.1,2001:db8:85a3:8d3:1319:8a2e:370:7348", h2.getNormalizedValue(X_FORWARDED_FOR)); + }
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java (1)
204-215: Prevent NPEs in getShortDescription() and handle all newline variants in formatAsHtml().formatAsHtml(null) will NPE, and splitting only on "\n" misses other line breaks. This was raised previously and is still applicable.
@Override public String getShortDescription() { String s = "Pretty print: %s<br/>".formatted(pretty); if (contentType != null) { s += "Content-Type: %s<br/>".formatted(contentType); } - return s + formatAsHtml(textTemplate); + return s + (textTemplate != null ? formatAsHtml(textTemplate) : ""); } protected String formatAsHtml(String plaintext) { - return String.join("<br/>", escapeHtml4(plaintext).split("\n")); + if (plaintext == null) return ""; + return String.join("<br/>", escapeHtml4(plaintext).split("\\R")); }
🧹 Nitpick comments (70)
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java (3)
32-34: Prefer explicit static imports over wildcards to avoid accidental symbol captureUsing wildcard static imports for ExceptionUtil, StandardCharsets, and UUID broadens the import surface and risks future naming collisions. Import the exact members used.
Apply:
-import static com.predic8.membrane.core.util.ExceptionUtil.*; -import static java.nio.charset.StandardCharsets.*; -import static java.util.UUID.*; +import static com.predic8.membrane.core.util.ExceptionUtil.concatMessageAndCauseMessages; +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.UUID.randomUUID;
264-266: Avoid adding a trailing slash when seeSuffix is emptyWhen seeSuffix is empty, this unconditionally appends "/". Gate on non-empty seeSuffix instead.
- if (!see.isEmpty()) { - see += "/" + seeSuffix; - } + if (!seeSuffix.isEmpty()) { + see += "/" + seeSuffix; + }
242-244: Locale-stable lowercasingFor identifiers/URLs, prefer Locale.ROOT for deterministic behavior across locales.
- private String normalizeForType(String s) { - return s.replace(" ", "-").toLowerCase(); - } + private String normalizeForType(String s) { + return s.replace(" ", "-").toLowerCase(java.util.Locale.ROOT); + }core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java (1)
130-132: Ensure null-safety and fix signature spacing in base64Encode
base64Encode(String s, SpELExchangeEvaluationContext ctx)is only used inBuiltInFunctionsand its unit test—no other usages rely on the old platform‐default encoding.- Optional: guard against null input to avoid unexpected NPEs when invoked from SpEL; returning
nullonnullinput is typically more ergonomic.- Nitpick: add a space after the comma in the method signature for consistency.
Apply this patch:
- public static String base64Encode(String s,SpELExchangeEvaluationContext ctx) { - return java.util.Base64.getEncoder().encodeToString(s.getBytes(UTF_8)); + public static String base64Encode(String s, SpELExchangeEvaluationContext ctx) { + if (s == null) { + return null; + } + return java.util.Base64.getEncoder().encodeToString(s.getBytes(UTF_8)); }You may also want to extend the existing test (
BuiltInFunctionsTest.testBase64Encode) to cover:
nullinput, asserting it returnsnull- non-ASCII inputs (e.g.
"äöü €") to guarantee cross-platform determinismcore/src/test/java/com/predic8/membrane/core/interceptor/websocket/WebSocketFrameTest.java (2)
30-30: Prefer narrowing the throws clause to IOException (or remove it if unnecessary).Using
throws Exceptionis broader than needed here. Narrowing improves clarity and prevents masking unrelated failures.- void testDynamicBuffer() throws Exception { + void testDynamicBuffer() throws IOException {
40-52: InputStream.read() should return 0..255; mask the byte to avoid negative ints.Returning
bytedirectly may yield negative values for bytes ≥ 128, violatingInputStream.read()’s contract. Mask to ensure the 0..255 range.- return frame[my]; + return frame[my] & 0xFF;Optional: For performance, consider overriding
read(byte[], int, int)to bulk-copy fromframerather than relying on the default per-byte loop.core/src/test/java/com/predic8/membrane/core/RouterTest.java (4)
50-61: Prefer Accept over Content-Type for response negotiation + avoid brittle map size assertion
- Using Content-Type on the request to influence response format can be misleading; Accept is the canonical header for content negotiation. Add an Accept header to make intent explicit and avoid accidental behavior changes in the server.
- Asserting an exact object size (aMapWithSize(3)) is brittle. Prefer asserting the required keys exist.
Suggested diff:
given() + .accept(APPLICATION_PROBLEM_JSON) .contentType(APPLICATION_JSON) .post("http://localhost:2000/") .then() - .log().ifValidationFails() + .log().ifValidationFails(ALL) .statusCode(500) .contentType( containsString(APPLICATION_PROBLEM_JSON)) .body("title", equalTo("Internal server error.")) .body("type",equalTo("https://membrane-api.io/problems/internal")) .body("message", Matchers.not(containsString(INTERNAL_SECRET))) - .body("$",aMapWithSize(3)); + .body("$", allOf(hasKey("title"), hasKey("type"), hasKey("message")));
65-78: Explicit Accept for XML + verify differing XML schemas between prod/dev are intentional
- Add Accept(APPLICATION_XML) to clearly request XML; relying on request Content-Type to steer the response is fragile.
- The prod XML root uses error.* while dev uses problem-details.*. If the intent is a unified prettifier, please confirm the differing element names are by design.
Suggested diff:
void prodXML() { // @formatter:off given() + .accept(APPLICATION_XML) .contentType(APPLICATION_XML) .post("http://localhost:2000/") .then() .log().ifValidationFails(ALL) .statusCode(500) .contentType(containsString(APPLICATION_XML)) .body("error.title", equalTo("Internal server error.")) .body("error.type",equalTo("https://membrane-api.io/problems/internal")) .body("error.message", Matchers.not(containsString(INTERNAL_SECRET))); // @formatter:on
83-95: Make negotiation explicit with Accept + align dev JSON vs dev XML expectations
- Add Accept(APPLICATION_PROBLEM_JSON) to request problem+json explicitly.
- Dev XML asserts that the message contains the secret, while dev JSON only asserts message exists. Please align expectations (either include or exclude the secret consistently across formats), or document why they differ.
Suggested diff:
given() + .accept(APPLICATION_PROBLEM_JSON) .get("http://localhost:2001/") .then() .statusCode(500) .contentType(containsString(APPLICATION_PROBLEM_JSON)) .body("title", equalTo("Internal server error.")) .body("type",equalTo("https://membrane-api.io/problems/internal")) .body("$",hasKey("attention")) .body("attention", containsString("development mode")) .body("$",hasKey("message")) .body("$",not(hasKey("stacktrace")));If the intended dev behavior is to expose the exception message, consider also asserting:
- .body("message", containsString(INTERNAL_SECRET))
Otherwise, mirror the dev XML test to not expose secrets.
98-113: Add Accept(APPLICATION_XML) and reduce brittleness of stacktrace assertion
- Add Accept(APPLICATION_XML) to unambiguously request XML.
- Asserting the absence of a specific class name in the stacktrace is brittle. Prefer checking that internal package names are not leaked, or align with dev JSON and omit stacktrace entirely if that’s the policy.
Suggested diff:
given() + .accept(APPLICATION_XML) .contentType(APPLICATION_XML) .post("http://localhost:2001/") .then() .log().ifValidationFails(ALL) .statusCode(500) .contentType(containsString(APPLICATION_XML)) .body("problem-details.title", equalTo("Internal server error.")) .body("problem-details.type",equalTo("https://membrane-api.io/problems/internal")) .body("problem-details.attention", containsString("development mode")) .body("problem-details.message", containsString("supersecret")) - .body("problem-details.stacktrace", Matchers.not(containsString("HttpServerHandler"))); + .body("problem-details.stacktrace", not(containsString("com.predic8.membrane")));Alternatively, if stacktraces should not be present in dev at all, assert the element is absent/null rather than checking contents.
core/src/main/java/com/predic8/membrane/core/interceptor/kubernetes/KubernetesValidationInterceptor.java (5)
31-33: Remove redundant static import and avoid wildcard import.
Outcome.ABORTis imported twice (once explicitly and once viaOutcome.*). Also, prefer importing onlyREQUESTinstead ofFlow.*to keep the namespace tight.Apply this diff to clean up imports:
-import static com.predic8.membrane.core.interceptor.Interceptor.Flow.*; -import static com.predic8.membrane.core.interceptor.Outcome.ABORT; +import static com.predic8.membrane.core.interceptor.Interceptor.Flow.REQUEST; import static com.predic8.membrane.core.interceptor.Outcome.*;
170-172: Reuse a single ObjectMapper instance to reduce per-request overhead.
ObjectMapperis thread-safe for reads after configuration. Constructing one per request is wasteful.Apply this diff to use a shared mapper:
- ObjectMapper mapper = new ObjectMapper(); - AdmissionReview review = mapper.readValue(exc.getRequest().getBodyAsStreamDecoded(), AdmissionReview.class); + AdmissionReview review = MAPPER.readValue(exc.getRequest().getBodyAsStreamDecoded(), AdmissionReview.class);Add this field near the logger (class scope):
private static final ObjectMapper MAPPER = new ObjectMapper();
173-179: Harden against missing/invalidkindand gate validation by configured resources.Avoid NPE when
kindis absent and skip validation for kinds not listed inresources(as per the interceptor’s configuration/JavaDoc).Apply this diff:
- Map<String, Object> object = review.getRequest().getObject(); - if (object != null) { // DELETE requests do not carry an object - String requestKind = (String) object.get("kind"); - - MessageValidator validator = validators.computeIfAbsent(requestKind.toLowerCase(), schema -> new JSONSchemaValidator(resourceResolver, "classpath:/com/predic8/membrane/core/config/kubernetes/" + schema + ".schema.json", null)); - validator.validateMessage(exc, REQUEST); - } + Map<String, Object> object = review.getRequest().getObject(); + if (object != null) { // DELETE requests do not carry an object + Object kindObj = object.get("kind"); + if (!(kindObj instanceof String) || ((String) kindObj).isBlank()) { + log.warn("AdmissionReview.request.object.kind is missing or blank - accepting request."); + setExchangeResponse(exc, MAPPER, review); + return RETURN; + } + String requestKind = ((String) kindObj).trim().toLowerCase(Locale.ROOT); + // If resources are configured, validate only those kinds. + if (resources != null && !resources.isEmpty() + && resources.stream().noneMatch(r -> r.equalsIgnoreCase(requestKind))) { + setExchangeResponse(exc, MAPPER, review); + return RETURN; + } + + MessageValidator validator = validators.computeIfAbsent( + requestKind, + schema -> new JSONSchemaValidator( + resourceResolver, + "classpath:/com/predic8/membrane/core/config/kubernetes/" + schema + ".schema.json", + null + ) + ); + validator.validateMessage(exc, REQUEST); + }Note:
Locale.ROOTis already available viajava.util.*.
180-183: Return path and response construction are correct; align with shared mapper.Continue returning the AdmissionReview immediately. If you adopt the shared mapper, update the call accordingly.
Apply this diff if adopting the shared mapper:
- setExchangeResponse(exc, mapper, review); + setExchangeResponse(exc, MAPPER, review);
184-191: Log a meaningful error message.
log.error("", e)loses context. Provide a concise message for diagnostics.Apply this diff:
- log.error("", e); + log.error("KubernetesValidationInterceptor: failed to handle AdmissionReview.", e);core/src/main/java/com/predic8/membrane/core/http/Message.java (1)
140-144: Prefer Charset over charset String to avoid UnsupportedEncodingException pathsMinor improvement: resolve the charset once into a Charset, defaulting to StandardCharsets.UTF_8. This avoids String-based charset lookups and reduces exceptions to unchecked UnsupportedCharsetException if misconfigured.
- String charset = getCharset(); - if (charset == null) - charset = "UTF-8"; - return new String(MessageUtil.getContent(this), charset); + String charsetName = getCharset(); + Charset charset = charsetName != null ? Charset.forName(charsetName) : StandardCharsets.UTF_8; + return new String(MessageUtil.getContent(this), charset);core/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java (1)
33-33: Remove or clarify TODOThe bare TODO at class scope provides no actionable guidance. Either remove it or expand it to a concrete task.
core/src/main/java/com/predic8/membrane/core/interceptor/WADLInterceptor.java (1)
60-64: Close the response body stream and set a consistent Content-Type
- The body InputStream should be closed or guaranteed to be closed by Relocator to avoid potential resource leaks. A local try-with-resources is simple and safe.
- After rewriting, Content-Type should reflect the (possibly defaulted) charset used to encode the body. You already left a TODO; suggest setting it when absent.
Apply this diff:
- relocator.relocate(exc.getResponse().getBodyAsStream()); - - exc.getResponse().setBodyContent(stream.toByteArray()); - // TODO setContentType - } + try (InputStream in = exc.getResponse().getBodyAsStream()) { + relocator.relocate(in); + } + + exc.getResponse().setBodyContent(stream.toByteArray()); + // Preserve existing Content-Type if present; otherwise set a sane default including charset. + if (exc.getResponse().getHeader().getContentType() == null) { + exc.getResponse().getHeader().setContentType("application/xml; charset=" + getCharset(exc)); + } + }core/src/test/java/com/predic8/membrane/core/ws/relocator/RelocatorTest.java (2)
41-45: Prefer assertions over println for test verificationThe test currently only asserts wsdlFound and prints the content. Consider asserting expected relocations or properties of the output to make the test meaningful and non-noisy.
Would you like me to draft assertions targeting known rewritten URLs or schemaLocation attributes?
54-56: Assert test resources exist to avoid NullPointerExceptionsgetResourceAsStream can return null. Add an assertion to fail fast with a helpful message.
Apply this diff:
- private InputStream getFile(String filename) { - return this.getClass().getResourceAsStream(filename); - } + private InputStream getFile(String filename) { + InputStream in = this.getClass().getResourceAsStream(filename); + assertNotNull(in, "Missing test resource: " + filename); + return in; + }core/src/main/java/com/predic8/membrane/core/interceptor/administration/AdminRESTInterceptor.java (1)
147-150: Use decoded body for raw exchange viewReplace the current
msg.toString()call withmsg.getBodyAsStringDecoded()so that the raw endpoint returns just the decoded payload (honoring the message’s charset) rather than the fulltoString()output.• File:
core/src/main/java/com/predic8/membrane/core/interceptor/administration/AdminRESTInterceptor.java
Method:getRaw(...)at thereturn ok()…lineProposed diff:
- return ok().contentType(TEXT_PLAIN_UTF8).body(msg.toString()).build(); // TODO: uses toString(), ignores encodings + return ok().contentType(TEXT_PLAIN_UTF8).body(msg.getBodyAsStringDecoded()).build();core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (2)
70-76: Close the response body stream and set a consistent Content-TypeMirror the WADL change: ensure the InputStream is closed and Content-Type set when missing, so downstream sees the correct charset.
Apply this diff:
- relocator.relocate(exc.getResponse().getBodyAsStream()); + try (InputStream in = exc.getResponse().getBodyAsStream()) { + relocator.relocate(in); + } if (relocator.isWsdlFound()) { registerWSDL(exc); } exc.getResponse().setBodyContent(stream.toByteArray()); - // TODO setContentType + // Preserve existing Content-Type if present; otherwise set a sane default including charset. + if (exc.getResponse().getHeader().getContentType() == null) { + exc.getResponse().getHeader().setContentType("application/xml; charset=" + + requireNonNullElseGet(exc.getResponse().getCharset(), UTF_8::name)); + }
139-146: Optional: use default ports per scheme when building the public WSDL URLCurrently, the port omission logic only handles http:80. Consider also skipping :443 for https to avoid aesthetic mismatches in generated registry URLs.
Proposed adjustment:
- if (getLocationPort(exc) != 80) { + boolean isHttps = "https".equalsIgnoreCase(getLocationProtocol()); + int port = getLocationPort(exc); + if (!(port == 80 && !isHttps) && !(port == 443 && isHttps)) { buf.append(":"); - buf.append(getLocationPort(exc)); + buf.append(port); }core/src/test/java/com/predic8/membrane/core/util/xml/XMLEncodingTestUtil.java (2)
35-35: Fix typo in error message"expectd" → "expected".
Apply this diff:
- System.err.printf("%s\n does not match expectd %s\n",actual, expected); + System.err.printf("%s\n does not match expected %s\n", actual, expected);
29-38: Optional: include failure context in the assertionPrinting to stderr can be swallowed; embedding the missing token in the assertion message improves diagnostics.
Alternative implementation:
for (String expected : List.of(UMLAUTE, ACCENTS, ESPANOL, NORDIC, ENTITIES)) { assertTrue(actual.contains(expected), "Missing expected substring: " + expected + "\nActual:\n" + actual); }core/src/main/java/com/predic8/membrane/core/interceptor/balancer/XMLElementSessionIdExtractor.java (3)
39-41: Fix log message grammarMinor but user-facing in logs.
- log.debug("Didn't search a XML element in none XML message."); + log.debug("Did not search for an XML element in a non-XML message.");
95-99: Namespace is marked @required but treated as optional in codeCode allows namespace == null (and configures the factory accordingly). Either remove @required or enforce non-null at parse time. I'd align the annotations with behavior.
- @Required @MCAttribute public void setNamespace(String namespace) { this.namespace = namespace; }
34-34: XMLInputFactory reuse may be unsafe under concurrencyInterceptors are typically singletons. XMLInputFactory is not guaranteed to be thread-safe. Consider per-call factory creation or a ThreadLocal.
Example:
- private final XMLInputFactory fac = XMLInputFactory.newInstance(); + private static final ThreadLocal<XMLInputFactory> FAC = + ThreadLocal.withInitial(XMLInputFactory::newInstance);and use
FAC.get()wherefacis used.core/src/main/java/com/predic8/membrane/core/prettifier/Prettifier.java (3)
49-60: Avoid lowercasing the entire Content-Type; rely on MimeType helpers for case-insensitivityLowercasing alters parameter values (e.g., boundary) unnecessarily. Since MimeType predicates handle case, trimming is sufficient.
- String ct = contentType.toLowerCase(java.util.Locale.ROOT).trim(); + String ct = contentType.trim();
28-31: Consider exposing a NULL prettifier to avoid altering unknown/binary payloadsFor non-JSON/XML/HTML and non-text types, returning TEXT can normalize whitespace unintentionally. Providing a NullPrettifier fallback is safer.
Prettifier JSON = new JSONPrettifier(); Prettifier XML = new XMLPrettifier(); Prettifier TEXT = new TextPrettifier(); + Prettifier NULL = new NullPrettifier();
55-60: Return TEXT only for text/*; use NULL for unknown/binary typesPrevents whitespace normalization on binary/unknown media types while preserving behavior for text/plain.
- // XML/HTML family: text/xml, application/xml, text/html (and charset variants) + // XML/HTML family: text/xml, application/xml, text/html (and charset variants) if (isXML(ct) || isHtml(ct)) return XML; - return TEXT; + if (isText(ct)) + return TEXT; + + return NULL;core/src/main/java/com/predic8/membrane/core/prettifier/NullPrettifier.java (1)
23-29: Make NullPrettifier a singleton to avoid needless allocationsSmall, but consistent with other static instances in Prettifier.
-public class NullPrettifier implements Prettifier { +public final class NullPrettifier implements Prettifier { + public static final NullPrettifier INSTANCE = new NullPrettifier(); + private NullPrettifier() {} @Override public byte[] prettify(byte[] c, Charset charset) { return c; // Ignore charset } }core/src/main/java/com/predic8/membrane/core/prettifier/TextPrettifier.java (2)
26-28: Defensive: handle null charset in prettifyEven if the default method passes UTF-8, callers may invoke prettify(byte[], null) directly. Make it robust.
- public byte[] prettify(byte[] c, Charset charset) { - return normalizeMultiline(new String(c, charset)).getBytes(charset); - } + public byte[] prettify(byte[] c, Charset charset) { + Charset cs = (charset != null) ? charset : java.nio.charset.StandardCharsets.UTF_8; + return normalizeMultiline(new String(c, cs)).getBytes(cs); + }
94-99: Comment “Keep trailing empty line info” is misleadingTrailing blank lines are preserved by split but removed later in getStartAndEnd(). Either adjust the comment or the logic.
Possible fix (adjust the comment):
- /** - * Keep trailing empty line info - */ + /** + * Split into lines (preserving trailing empty tokens); later logic may trim leading/trailing blank lines. + */core/src/test/java/com/predic8/membrane/core/http/HeaderTest.java (2)
46-48: Remove unused h2 from setup
h2is created and populated but never used; move its usage into the relevant test to avoid dead setup.- Header h2 = new Header(); - h2.add(X_FORWARDED_FOR, " 192.3.14.1 , 10.0.0.1,2001:db8:85a3:8d3:1319:8a2e:370:7348 ");
29-33: Avoid static mutable fixtures to improve test isolationUsing static fields for mutable test state can cause interference under parallel execution. Prefer instance fields initialized in @beforeeach.
- private static final Header header = new Header(); + private final Header header = new Header(); - private static Header h1; + private Header h1;core/src/main/java/com/predic8/membrane/core/interceptor/RegExReplaceInterceptor.java (1)
94-94: Minor: prefer Charset over charset-name string for getBytesAvoid a charset lookup by name on every call; compute a Charset once and use getBytes(Charset). This keeps the code type-safe and a bit clearer.
Apply this diff:
- res.setBodyContent(res.getBodyAsStringDecoded().replaceAll(regex, replace).getBytes(requireNonNullElseGet(res.getCharset(), UTF_8::name))); + var cs = java.nio.charset.Charset.forName(requireNonNullElseGet(res.getCharset(), UTF_8::name)); + res.setBodyContent(res.getBodyAsStringDecoded().replaceAll(regex, replace).getBytes(cs));core/src/main/java/com/predic8/membrane/core/http/xml/PlainBody.java (1)
31-33: Optional: store Charset instead of StringIf you touch this again, consider storing a Charset rather than a String for charset to use the new String(byte[], Charset) overload and avoid repeated lookups. Not critical.
core/src/test/java/com/predic8/membrane/test/TestUtil.java (1)
37-39: Harden getResourceAsBytes: guard missing resources and fix @NotNull placement
- Current code NPEs if the resource is missing; fail early with a clear message.
- @NotNull currently annotates the array component type. Annotate the return value instead.
- Reading via URL.openStream() also works when resources are packaged in a JAR.
Apply this diff:
- public static byte @NotNull [] getResourceAsBytes(String path) throws Exception { - return readAllBytes(Paths.get(TestUtil.class.getResource(path).toURI())); - } + public static @NotNull byte[] getResourceAsBytes(String path) throws IOException { + var url = java.util.Objects.requireNonNull( + TestUtil.class.getResource(path), + "Resource not found: " + path + ); + try (InputStream in = url.openStream()) { + return in.readAllBytes(); + } + }core/src/main/java/com/predic8/xml/beautifier/XMLInputFactoryFactory.java (1)
22-33: Optional hardening: prevent instantiation and disable external entities defensively
- Add a private constructor to prevent accidental instantiation of this utility class.
- Some StAX impls support toggling external entities separately; defensively turn them off and set a no-op XMLResolver. Wrap in try/catch to avoid IllegalArgumentException on impls that don’t support the property.
Apply this diff:
public final class XMLInputFactoryFactory { + private XMLInputFactoryFactory() {} + private static final ThreadLocal<XMLInputFactory> TL = ThreadLocal.withInitial(() -> { XMLInputFactory f = XMLInputFactory.newInstance(); f.setProperty(IS_COALESCING, TRUE); f.setProperty(SUPPORT_DTD, FALSE); f.setProperty(IS_NAMESPACE_AWARE, TRUE); + // Defensive hardening: disable external entities if supported and use a no-op resolver. + try { + f.setProperty("javax.xml.stream.isSupportingExternalEntities", FALSE); + } catch (IllegalArgumentException ignore) { + // property not supported by this implementation + } + f.setXMLResolver((publicId, systemId, baseURI, namespace) -> new java.io.ByteArrayInputStream(new byte[0])); return f; }); public static XMLInputFactory inputFactory() { return TL.get(); } }core/src/test/java/com/predic8/xml/beautifier/StandardXMLBeautifierFormatterTest.java (1)
133-135: CRLF expectations are hard-coded; verify portability or factor into a constantTests assert explicit "\r\n". If StandardXMLBeautifierFormatter is intentionally emitting CRLF (not platform-dependent), fine. If not, consider normalizing or factoring the newline as a constant to avoid brittle assertions on different platforms.
Also applies to: 142-144, 160-163, 190-192
core/src/test/java/com/predic8/membrane/core/prettifier/XMLPrettifierTest.java (1)
47-51: Misleading comment vs. code in utf16be testComment says “Let the beautifier detect the encoding” but the test passes UTF_16BE explicitly. Either adjust the comment or drop the charset argument to actually test detection.
Apply one of the following:
- // Let the beautifier detect the encoding - assertChars(new String(prettifier.prettify(getResourceAsBytes("/charsets/utf-16be-unformatted.xml"), UTF_16BE), UTF_16BE)); + // Parse with explicit UTF-16BE + assertChars(new String(prettifier.prettify(getResourceAsBytes("/charsets/utf-16be-unformatted.xml"), UTF_16BE), UTF_16BE));or
- // Let the beautifier detect the encoding - assertChars(new String(prettifier.prettify(getResourceAsBytes("/charsets/utf-16be-unformatted.xml"), UTF_16BE), UTF_16BE)); + // Let the beautifier detect the encoding + assertChars(new String(prettifier.prettify(getResourceAsBytes("/charsets/utf-16be-unformatted.xml")), UTF_16BE));core/src/main/java/com/predic8/membrane/core/http/xml/JSONBody.java (2)
35-41: Close JsonParser/InputStream via try-with-resourcesThe parser (and its underlying decoded stream) isn’t closed. Wrap in try-with-resources to avoid leaks and ensure AUTO_CLOSE_SOURCE kicks in.
Apply:
- try { - final JsonParser jp = new JsonFactory().createParser(msg.getBodyAsStreamDecoded()); + try (JsonParser jp = new JsonFactory().createParser(msg.getBodyAsStreamDecoded())) { final List<String> stack = new ArrayList<>(); String name = "root"; - OUTER: - while (jp.nextToken() != null) { + OUTER: + while (jp.nextToken() != null) { switch (jp.getCurrentToken()) {
37-70: Optional: use Deque for stack semanticsArrayDeque better conveys LIFO intent and avoids index math.
Example changes:
- final List<String> stack = new ArrayList<>(); + final Deque<String> stack = new ArrayDeque<>(); ... - stack.add(name); + stack.push(name); ... - name = stack.remove(stack.size()-1); + name = stack.pop(); ... - stack.add(name); + stack.push(name); ... - name = stack.remove(stack.size()-1); + name = stack.pop();Also add:
-import java.util.*; +import java.util.*; +import java.util.Deque; +import java.util.ArrayDeque;core/src/test/java/com/predic8/xml/beautifier/XMLBeautifierTest.java (1)
133-146: Encoding detection/prolog propagation tests read well; minor clarity nitSolid assertions. Minor nit: in encodingFromInputStream_isPropagatedToProlog, the comment “Use bytes in the actual encoding of this file” may confuse—bytes come from UTF-8, which matches enc. Consider clarifying to “Use bytes encoded as UTF-8 to match the declared prolog”.
Also applies to: 148-170
core/src/main/java/com/predic8/membrane/core/interceptor/beautifier/BeautifierInterceptor.java (3)
54-61: Guard against null MessageDefensive: handleResponse can be invoked when no response exists yet. A quick null-check avoids NPEs.
Apply:
- private Outcome handleInternal(Message msg) { + private Outcome handleInternal(Message msg) { + if (msg == null) + return CONTINUE; try { if (msg.isBodyEmpty()) return CONTINUE; } catch (IOException e) { log.error("", e); return CONTINUE; }
72-80: Charset resolution: handle invalid/unsupported charsets gracefullyCharset.forName can throw IllegalCharsetNameException/UnsupportedCharsetException. Consider a safe fallback (e.g., UTF-8) or log-and-continue for robustness.
Example:
- if (msg.getCharset() != null) - return Charset.forName(msg.getCharset()); + if (msg.getCharset() != null) { + try { + return Charset.forName(msg.getCharset()); + } catch (Exception ignored) { + // fall through to defaults + } + }
88-89: Short description mentions JSON, XML, text but omits JSON5Docs above cite JSON5 as supported. Align the short description for consistency.
Apply:
- return "Pretty printing of message body. Applies, if the body is JSON, XML or text."; + return "Pretty printing of message body. Applies if the body is JSON, JSON5, XML, or text.";core/src/test/java/com/predic8/membrane/core/interceptor/beautifier/BeautifierInterceptorTest.java (3)
59-63: Name the test to describe behavior.empty() is vague. Consider renaming to express the expectation, e.g., passesThroughOnGetWithoutBody().
- void empty() throws URISyntaxException { + void passesThroughOnGetWithoutBody() throws URISyntaxException {
66-77: Avoid comparing byte length with String char length.assertEquals(bodyLength, req.getBodyAsStringDecoded().length()) can break for non-ASCII payloads since String#length() counts UTF-16 code units, not bytes. If you intend to verify Content-Length consistency after prettification, compare against the UTF-8 byte length of the decoded string (or just the Body length).
- assertEquals(bodyLength, req.getBodyAsStringDecoded().length()); + assertEquals(bodyLength, req.getBodyAsStringDecoded().getBytes(UTF_8).length);Alternatively, drop the second assertion: the first one (header vs body length) is sufficient for this test.
136-139: UTF-16BE test decodes with UTF-16; verify BOM or decode explicitly with UTF-16BE.The test utf16BEWithoutEncodingInfoInTheHeader decodes using UTF_16 while the file name suggests BE. Using UTF_16 expects a BOM; if the resource lacks a BOM, decoding may be ambiguous.
Options:
- If the resource includes a BOM, keep UTF_16 but rename the test to reflect BOM presence.
- If there’s no BOM, decode explicitly with UTF_16BE to match the file’s encoding.
- void utf16BEWithoutEncodingInfoInTheHeader() throws Exception { - checkForFile("/charsets/utf-16be-unformatted.xml", UTF_16,null); + void utf16BEWithoutEncodingInfoInTheHeader() throws Exception { + checkForFile("/charsets/utf-16be-unformatted.xml", UTF_16BE, null);If you want me to, I can add a small verification that inspects the test resource for a BOM and adjusts expectations accordingly.
core/src/main/java/com/predic8/membrane/core/prettifier/JSONPrettifier.java (1)
41-48: Clarify charset handling and improve error logging.
- The charset parameter is ignored. That’s fine (JSON is UTF-8 by spec in HTTP), but it would be good to make this explicit at runtime and in logs to avoid confusion.
- log.debug("", e) loses context.
@Override public byte[] prettify(byte[] c, Charset charset) { try { - return om.writerWithDefaultPrettyPrinter().writeValueAsBytes(om.readTree(c)); + byte[] pretty = om.writerWithDefaultPrettyPrinter().writeValueAsBytes(om.readTree(c)); + // JSON should be UTF-8 on the wire; ignore non-UTF-8 requests but keep a breadcrumb. + if (charset != null && !StandardCharsets.UTF_8.equals(charset)) { + LoggerFactory.getLogger(JSONPrettifier.class) + .trace("Ignoring requested charset {} for JSON; emitting UTF-8.", charset); + } + return pretty; } catch (IOException e) { - log.debug("", e); + log.debug("Failed to prettify JSON. Returning input unmodified.", e); return c; } }Optionally, add an explicit override for prettify(byte[]) to mirror XMLPrettifier and make intent obvious:
@Override public byte[] prettify(byte[] c) { return prettify(c, StandardCharsets.UTF_8); }core/src/main/java/com/predic8/membrane/core/prettifier/XMLPrettifier.java (1)
50-63: Be defensive against unknown/invalid detected encodings and improve logging.
- Charset.forName(xb.getDetectedEncoding()) can throw IllegalArgumentException if a non-canonical or unknown name is returned. Fall back to the requested charset or UTF-8.
- log.debug("", e) should carry context.
- if (xb.getDetectedEncoding() != null) { - charset = Charset.forName(xb.getDetectedEncoding()); - } + if (xb.getDetectedEncoding() != null) { + try { + charset = Charset.forName(xb.getDetectedEncoding()); + } catch (IllegalArgumentException iae) { + log.debug("Unknown detected XML encoding '{}'. Falling back to requested charset {}.", + xb.getDetectedEncoding(), charset); + } + } // If you want a BOM for UTF-16 output, use "UTF-16" (not BE/LE) Charset out = ("UTF-16BE".equalsIgnoreCase(charset.name()) || "UTF-16LE".equalsIgnoreCase(charset.name())) ? java.nio.charset.StandardCharsets.UTF_16 : charset; return sw.toString().getBytes(out); } catch (Exception e) { - log.debug("", e); + log.debug("Failed to prettify XML. Returning input unmodified.", e); return c; }Consider adding a test case for an XML without an encoding declaration (no BOM), ensuring prettify(byte[]) defaults to UTF-8 and still formats.
core/src/test/java/com/predic8/membrane/core/prettifier/JSONPrettifierTest.java (2)
35-46: Stabilize “beautified contains space/newline” assertions or add a semantic assertion.Presence of " " and "\n" is a bit brittle across pretty printer configurations. Consider asserting structural properties (e.g., number of object fields, array sizes) or normalize whitespace before comparison. Not mandatory.
47-54: Add a negative test to assert graceful fallback on invalid JSON.Ensure prettifier returns the input unchanged on parse errors (as implemented). For example:
@Test void invalidJson_returnsInput() { byte[] invalid = "{ foo: [1, 2,".getBytes(UTF_8); // truncated byte[] pretty = beautifier.prettify(invalid); assertArrayEquals(invalid, pretty); }core/src/main/java/com/predic8/xml/beautifier/XMLBeautifier.java (4)
137-141: Remove stdout logging from library code; use formatter/logging or expose via API.
System.out.println("Detected encoding: ...")is noisy and not suitable for core libs. You already exposegetDetectedEncoding().Apply this diff:
case START_DOCUMENT -> { detectedEncoding = reader.getEncoding(); - System.out.println("Detected encoding: " + detectedEncoding); writeStartDocument(reader); }
85-90: Typo and naming: writeAtributes → writeAttributes.Consistent naming improves readability and discoverability.
Apply this diff:
- writeAtributes(reader, indent); + writeAttributes(reader, indent);And rename the method:
-private void writeAtributes(XMLStreamReader parser, int indent) throws IOException { +private void writeAttributes(XMLStreamReader parser, int indent) throws IOException {Also applies to: 166-179
131-132: Handle CDATA events (even as text) to avoid content loss.Currently CDATA is ignored. A minimal, safe default is to treat CDATA like text (escape where appropriate in formatter).
Apply this diff:
- case CDATA -> { // TODO - } + case CDATA -> { + empty = false; + if (!startTagClosed) { + formatter.closeTag(); + startTagClosed = true; + } + charContent = containsNonWhitespaceCharacters(reader.getText()); + formatter.writeText(reader.getText()); + }
151-154: Don't indent the XML declaration (minor).Indenting before the XML prolog is unusual. The formatter already prints a newline after the declaration; skipping the indent yields a cleaner start.
Apply this diff:
private void writeStartDocument(XMLStreamReader reader) throws IOException { - indent(); formatter.writeVersionAndEncoding(reader.getVersion(), reader.getEncoding()); }core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (2)
33-37: Clarify the TODO and consider centralizing XMLInputFactory usage.
- The TODO “make it thread safe! See” is incomplete. Either complete the reference or drop it.
- Consider reusing the thread-local XMLInputFactory via XMLInputFactoryFactory (as in XMLBeautifier) for consistency and to avoid drift in parser settings.
Proposed comment fix:
- // TODO make it thread safe! See + // TODO: Consider replacing with the shared, thread-local XMLInputFactory for consistency (see XMLInputFactoryFactory).
73-82: Align exception handling with the InputStream variant and avoid overbroad logging.Catching raw Exception logs all failures as “Error parsing XML”, which may hide non-XML issues. Prefer handling XMLStreamException specifically (then optionally rethrow others without logging).
Apply this diff:
- } - catch (Exception e){ - log.info("Error parsing XML: {}", e.getMessage()); - throw e; - } + } catch (XMLStreamException e) { + log.info("Error parsing XML: {}", e.getMessage()); + throw e; + }Note: The method signature still declares
throws Exception, so other exceptions will propagate without being logged here.core/src/main/java/com/predic8/xml/beautifier/HtmlBeautifierFormatter.java (2)
25-34: Optionally encode element names for defense-in-depth.While XML names should not contain HTML-breaking characters, encoding
prefixandlocalNamekeeps output robust.Apply this diff:
if (prefix != null && !prefix.isEmpty()) { - writer.write("<span class='p'>" + prefix + "</span>:<span class='e'>" + localName); + writer.write("<span class='p'>" + HTMLEntityEncode(prefix) + "</span>:<span class='e'>" + HTMLEntityEncode(localName)); } else { - writer.write("<span class='e'>" + localName); + writer.write("<span class='e'>" + HTMLEntityEncode(localName)); }
99-101: Document no-op version/encoding behavior.Intentional no-op is fine for HTML rendering, but add a short comment for future readers.
Proposed inline comment:
// Intentionally omitted: XML declaration is not rendered in HTML prettified output.core/src/main/java/com/predic8/xml/beautifier/StandardXMLBeautifierFormatter.java (2)
75-90: Tighten XML declaration formatting (remove extra space before '?>').Current output is
?>. The common form is?>without the leading space.Apply this diff:
- writer.write( " ?>"); + writer.write("?>"); printNewLine();
67-69: Consider using a platform-agnostic newline.Hardcoding CRLF may be undesirable across platforms. If portability matters, prefer
\norSystem.lineSeparator().Apply this diff:
- writer.write("\r\n"); + writer.write(System.lineSeparator());core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java (3)
47-49: Make JSON-variable detection robust (support bracket access, ${json}, and whitespace).textTemplate.contains("json.") will miss usages like json["city"], ${json}, or <%= json %>. Prefer a regex that covers common access patterns.
Apply:
@@ - private boolean scriptAccessesJson = false; + private boolean scriptAccessesJson = false; + private static final java.util.regex.Pattern JSON_VAR_PATTERN = + java.util.regex.Pattern.compile("(?s)(\\$\\{\\s*json\\b)|(<%=?\\s*json\\b)|\\bjson\\s*\\[|\\bjson\\s*\\."); @@ - // If the template accesses somewhere the json variable make sure it is there - // You can even access json in an XML or Text Template. See tests. - scriptAccessesJson = textTemplate.contains("json."); + // If the template accesses the 'json' variable, ensure it's available. + // Bracket, dot, and direct usages are supported: json[...], json., ${json}, <%= json %> + scriptAccessesJson = JSON_VAR_PATTERN.matcher(textTemplate).find();Also applies to: 60-63
114-125: Consistency: use the static import for isJson() like isXml().Minor style nit: you already statically import FileUtil.*; keep usage consistent.
- if (FileUtil.isJson(location)) { + if (isJson(location)) { setContentType(APPLICATION_JSON); }
77-79: Improve log message for generic exception.Empty string as a log message is unhelpful. Provide context.
- log.warn("", e); + log.warn("Error executing template.", e);core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java (1)
56-72: Optional: simplify file path assembly in setupFiles().Using Paths.get("project_template.xml") with REPLACE_EXISTING and working dir is sufficient; concatenating System.getProperty("user.dir") + separator is redundant and harder to read.
core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java (1)
62-64: Fail fast when neither location nor inline text is provided.If both are absent, subclasses like TemplateInterceptor will NPE at createTemplate(). Enforce a clear configuration error during init.
if (pretty) prettifier = Prettifier.getInstance(contentType); + + // Ensure there is a template source + if ((location == null || location.isBlank()) && (textTemplate == null || textTemplate.isBlank())) { + throw new ConfigurationException("On <%s>, either inline text or @location must be set.".formatted(getName())); + }
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/predic8/membrane/core/interceptor/balancer/XMLElementSessionIdExtractor.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/predic8/membrane/core/interceptor/balancer/XMLElementSessionIdExtractor.java
Show resolved
Hide resolved
...c/main/java/com/predic8/membrane/core/interceptor/balancer/XMLElementSessionIdExtractor.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/xml/beautifier/HtmlBeautifierFormatter.java
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/RequestParameterizedTest.java
Show resolved
Hide resolved
.../src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 15
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/ws/relocator/Relocator.java (1)
1-1: Secure all XMLInputFactory instantiations and close readers
The repo-wide search still uncovered numerous direct calls toXMLInputFactory.newInstance()(andnewFactory()) without disabling external entities or DTD support—leaving us vulnerable to XXE attacks—and without closing the created readers. Please update the following locations to use the safe factory (XMLInputFactoryFactory.inputFactory()) or explicitly setfactory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); factory.setProperty(XMLInputFactory.SUPPORT_DTD, false);and wrap all
XMLEventReader/XMLStreamReaderusages in try-with-resources:
- core/src/main/java/com/predic8/membrane/core/ws/magic/Magic.java (scan): lines 36–39
- core/src/main/java/com/predic8/membrane/core/util/SOAPUtil.java (isSOAP): line 44
- core/src/main/java/com/predic8/membrane/core/interceptor/acl/AccessControlInterceptor.java (parse): lines 131–133
- core/src/main/java/com/predic8/membrane/core/interceptor/soap/SampleSoapServiceInterceptor.java: lines 156–158, 173–175
- core/src/main/java/com/predic8/membrane/core/interceptor/rest/XML2HTTP.java: factory init at lines 32–33; getXmlEventReader at 146–149
- core/src/main/java/com/predic8/membrane/core/interceptor/xmlcontentfilter/XMLElementFinder.java: init at 37–39; reader creation at 63–66
- core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java: init at 43–45; parser at 73–77
- core/src/main/java/com/predic8/membrane/core/interceptor/ws_addressing/WsaEndpointRewriter.java (rewriteEndpoint): lines 48–50
- core/src/main/java/com/predic8/membrane/core/interceptor/rest/REST2SOAPInterceptor.java (getRootElementNamespace): lines 278–280
(You may also want to review other direct
newInstance()calls in tests and examples.)core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (1)
147-155: Close XMLEventReader in isValidXMLSnippet to avoid leaks- try { - XMLEventReader parser = XMLInputFactoryFactory.inputFactory().createXMLEventReader(new StringReader("<a>" + s + "</a>")); - XMLEvent event = null; - while (parser.hasNext()) { - event = (XMLEvent) parser.next(); - } - return event != null && event.isEndDocument(); - } catch (Exception e) { + try { + XMLEventReader parser = XMLInputFactoryFactory.inputFactory() + .createXMLEventReader(new StringReader("<a>" + s + "</a>")); + XMLEvent event = null; + try { + while (parser.hasNext()) { + event = (XMLEvent) parser.next(); + } + return event != null && event.isEndDocument(); + } finally { + try { parser.close(); } catch (Exception ignore) {} + } + } catch (Exception e) { log.error("", e); return false; }
♻️ Duplicate comments (11)
core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java (1)
88-92: Explicit UTF-8 encoding on rendered template bytes — good fix.This addresses the earlier feedback about platform-dependent String.getBytes() behavior and ensures deterministic output.
core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java (1)
251-256: UTF‑8 explicit in assertions — good stabilization.Using getBytes(UTF_8) on both sides prevents platform-default charset flakes.
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/RequestParameterizedTest.java (3)
81-85: Quote replacement literally to avoid regex group/backreference surprisesReplacement strings containing $ or \ are treated specially by replaceFirst. Wrap with Matcher.quoteReplacement(...) to make tests robust.
- exc.getRequest().setUri(exc.getRequest().getUri().replaceFirst(Pattern.quote(value), replacement)); + exc.getRequest().setUri( + exc.getRequest().getUri() + .replaceFirst(Pattern.quote(value), Matcher.quoteReplacement(replacement)) + );
89-96: Same here: quote replacement in request body mutationApply literal replacement to avoid regex backreference interpretation.
- exc.getRequest().setBodyContent(exc.getRequest().getBodyAsStringDecoded().replaceFirst(Pattern.quote(value), replacement).getBytes(UTF_8)); + exc.getRequest().setBodyContent( + exc.getRequest() + .getBodyAsStringDecoded() + .replaceFirst(Pattern.quote(value), Matcher.quoteReplacement(replacement)) + .getBytes(UTF_8) + );
101-105: Lazy variant: also quote the computed replacementMirror the literal replacement handling in the lazy branch.
- exc.getRequest().setBodyContent(exc.getRequest().getBodyAsStringDecoded().replaceFirst(Pattern.quote(value.call()), replacement.call()).getBytes(UTF_8)); + exc.getRequest().setBodyContent( + exc.getRequest() + .getBodyAsStringDecoded() + .replaceFirst(Pattern.quote(value.call()), Matcher.quoteReplacement(replacement.call())) + .getBytes(UTF_8) + );core/src/main/java/com/predic8/membrane/core/interceptor/balancer/XMLElementSessionIdExtractor.java (1)
77-81: Guard against null localName to avoid NPE on misconfigurationAdd a defensive check so misconfigured instances don’t throw.
Apply this diff:
- return reader.isStartElement() && - localName.equals(reader.getLocalName()) && - (namespace == null || namespace.equals(reader.getNamespaceURI())); + return reader.isStartElement() && + localName != null && + localName.equals(reader.getLocalName()) && + (namespace == null || namespace.equals(reader.getNamespaceURI()));core/src/main/java/com/predic8/membrane/core/interceptor/rest/XML2HTTP.java (1)
144-149: Good fix: always create reader over decoded stream; resolves compressed-body failure.Switching to getBodyAsStreamDecoded() regardless of charset avoids feeding compressed bytes to the XML parser when Content-Encoding is present. This addresses the earlier regression note.
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/CertsEndpointProcessor.java (1)
40-42: Explicit UTF-8 and unified Prettifier usage: resolved as discussed previouslyThis addresses the earlier remark about
.getBytes()without charset and aligns with the new Prettifier API.core/src/main/java/com/predic8/membrane/core/prettifier/Prettifier.java (1)
29-33: Javadoc contract corrected — goodThe method now clearly states it may return the same or a new array, which aligns with JSON/XML re-encoding behavior.
core/src/main/java/com/predic8/membrane/core/interceptor/beautifier/BeautifierInterceptor.java (1)
55-75: Prettifier is cached and decoded bytes are used — nice cleanupSwapping to msg.getBodyAsStreamDecoded() and caching the Prettifier/Charset addresses prior concerns. Exception handling is also right-sized (info-level, non-fatal).
core/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java (1)
113-115: Harden XMLInputFactory and close XMLEventReader (security + leak) — prior feedback still appliesCurrent code uses
XMLInputFactory.newInstance()and never closes the parser. This reintroduces XXE/DTD risks and leaks. You already use the hardened, thread-localXMLInputFactoryFactory.inputFactory()elsewhere.Apply:
public void relocate(InputStream is) throws Exception { - relocate(XMLInputFactory.newInstance().createXMLEventReader(is)); + relocate(XMLInputFactoryFactory.inputFactory().createXMLEventReader(is)); } -private void relocate(XMLEventReader parser) throws XMLStreamException { - while (parser.hasNext()) { - writer.add(process(parser)); - } - writer.flush(); -} +private void relocate(XMLEventReader parser) throws XMLStreamException { + try { + while (parser.hasNext()) { + writer.add(process(parser)); + } + writer.flush(); + } finally { + try { parser.close(); } catch (Exception ignore) {} + } +}This also ensures the deprecated
relocate(InputStreamReader)path closes the parser via the shared private method.Also applies to: 117-122
🧹 Nitpick comments (73)
core/src/test/java/com/predic8/membrane/core/interceptor/balancer/ClusterBalancerTest.java (4)
36-44: Minor: make extractor final and lift magic stringsInitialization and wiring look correct. Two small polish items:
- Declare
extractorasfinal(it’s assigned once in@BeforeAll).- Extract
"session"and the namespace URI into constants to avoid typos across tests.- extractor = new XMLElementSessionIdExtractor(); + extractor = new XMLElementSessionIdExtractor(); extractor.setLocalName("session"); extractor.setNamespace("http://predic8.com/session/");Potential follow-up:
private static final String SESSION_LOCAL_NAME = "session"; private static final String SESSION_NS = "http://predic8.com/session/";and then use those constants here.
120-124: Close InputStreams and fail fast when resources are missingUsing
getResourceAsStream(...).readAllBytes()without closing the stream can leak file descriptors (low risk in tests, but easy to avoid). Also, a missing resource will yield a null stream and an NPE at read time. Prefer try-with-resources and an explicit null check.Apply this diff and a small helper (see below) to DRY it up:
- Response res = ok().build(); - res.setHeader(getHeader()); - res.setBodyContent(getClass().getResourceAsStream( - "/getBankResponsewithSession.xml").readAllBytes()); - return res; + byte[] body = readResource("/getBankResponsewithSession.xml"); + Response res = ok().build(); + res.setHeader(getHeader()); + res.setBodyContent(body); + return res;Add this helper method and import once in the class (outside this hunk):
import java.util.Objects; private byte[] readResource(String path) throws IOException { try (InputStream is = Objects.requireNonNull( getClass().getResourceAsStream(path), "Missing test resource: " + path)) { return is.readAllBytes(); } }
131-133: Same resource-handling improvement hereRefactor to use the shared helper to close the stream and get a clearer error if the resource is missing.
- res.setBodyContent(getClass().getResourceAsStream( - "/getBankwithSession555555.xml").readAllBytes()); + res.setBodyContent(readResource("/getBankwithSession555555.xml"));
142-144: And here as wellUse the helper for consistency and safety.
- res.setBodyContent(getClass().getResourceAsStream( - "/getBank.xml").readAllBytes()); + res.setBodyContent(readResource("/getBank.xml"));core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java (5)
60-63: Make JSON-access detection null-safe and recognize more Groovy access patterns (json?. / json[ ]).Current check only matches "json." and will miss safe-navigation (json?.foo) and bracket access (json['foo']). It also risks an NPE if textTemplate were ever null before this point.
Suggested change:
- scriptAccessesJson = textTemplate.contains("json."); + // recognize: json.foo, json?.foo, json['foo'] + scriptAccessesJson = textTemplate != null + && java.util.regex.Pattern.compile("\\bjson(?:\\.|\\?\\.|\\[)") + .matcher(textTemplate) + .find();This keeps the “parse JSON only when needed” optimization while covering common Groovy forms and avoiding a null dereference.
94-96: Return an interface type (Map) instead of a concrete HashMap.Returning the interface improves flexibility and testability; callers shouldn’t rely on a specific Map implementation.
- private @NotNull HashMap<String, Object> getVariableBinding(Exchange exc, Flow flow) { + private @NotNull Map<String, Object> getVariableBinding(Exchange exc, Flow flow) { return createParameterBindings(router, exc, flow, scriptAccessesJson && isJsonMessage(exc, flow)); }
114-126: Use consistent static import style for FileUtil helpers.You already statically import FileUtil.* and call isXml(location) directly. Do the same for isJson for consistency.
- if (FileUtil.isJson(location)) { + if (isJson(location)) { setContentType(APPLICATION_JSON); }
65-86: Enrich error details with template location to speed up diagnostics.Including the template’s logical location (“inline template” vs file path) makes production incidents far easier to triage.
- internal(router.isProduction(), getDisplayName()) + internal(router.isProduction(), getDisplayName()) .addSubSee("groovy") - .detail("Groovy error during template rendering.") + .detail("Groovy error during template rendering. Template: " + getTemplateLocation()) .exception(e) .stacktrace(false) .buildAndSetResponse(exc);- internal(router.isProduction(), getDisplayName()) + internal(router.isProduction(), getDisplayName()) .addSubSee("template") + .detail("Error during template rendering. Template: " + getTemplateLocation()) .exception(e) .buildAndSetResponse(exc);
102-109: Fail early with a clearer message if no template text is available.If textTemplate is unexpectedly null at this point, the StringReader(...) path will NPE and be wrapped, but the root cause (“no template provided”) is obscured.
private Template createTemplate() { try { + if (textTemplate == null) + throw new ConfigurationException("No template content provided via 'location' or inline text (" + getTemplateLocation() + ")."); return createTemplateEngine().createTemplate(new StringReader(textTemplate)); } catch (Exception e) { throw new ConfigurationException("Could not create template from " + getTemplateLocation(), e); } }core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java (4)
88-94: Remove duplicate assignment to the same Exchange property."title" is set twice to the same value; keep one.
exc.setProperty("title", "minister"); List<String> lst = new ArrayList<>(); lst.add("food1"); lst.add("food2"); exc.setProperty("items", lst); - exc.setProperty("title", "minister");
58-69: Avoid writing into user.dir; switch to JUnit 5 @tempdir for filesystem isolation.Writing to the working directory can fail on CI (read-only, parallel runs). Prefer a temp directory managed by JUnit.
Example refactor (outside the selected lines):
@TempDir static Path tmpDir; @BeforeAll static void setupFiles() throws IOException { copyFiles(Paths.get("src/test/resources/xml/project_template.xml"), tmpDir.resolve("project_template.xml")); copyFiles(Paths.get("src/test/resources/json/template_test.json"), tmpDir.resolve("template_test.json")); copiedXml = tmpDir.resolve("project_template.xml"); copiedJson = tmpDir.resolve("template_test.json"); router = mock(Router.class); map = new ResolverMap(); when(router.getResolverMap()).thenReturn(map); when(router.getUriFactory()).thenReturn(new URIFactory()); }This removes dependence on System.getProperty("user.dir") and manual separator handling.
259-266: Use a consistent init(...) flavor across tests.Most tests use init(router), but this one constructs a new Router, which can hide configuration mismatches.
- ti.init(new Router()); + ti.init(router);
96-109: Add coverage for json access variants (safe-navigation and bracket).To validate the improved detection and binding behavior, add a test that exercises json?.field and json['field']:
@Test void accessJsonWithSafeNavAndBracket() throws Exception { Exchange exchange = post("/cities").contentType(APPLICATION_JSON).body(""" { "city": "Da Nang", "pop": 1000000 } """).buildExchange(); invokeInterceptor(exchange, """ City: <%= json?.city %>, Pop: <%= json['pop'] %> """, TEXT_PLAIN); String body = exchange.getRequest().getBodyAsStringDecoded(); assertTrue(body.contains("City: Da Nang, Pop: 1000000")); }This will guard against regressions if the detection logic evolves.
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java (3)
32-34: Prefer explicit static imports to reduce namespace bleed and avoid accidental ambiguitiesWildcard static imports make it harder to track where symbols come from and risk clashes as utilities grow. Import only what’s used here.
Apply:
-import static com.predic8.membrane.core.util.ExceptionUtil.*; -import static java.nio.charset.StandardCharsets.*; -import static java.util.UUID.*; +import static com.predic8.membrane.core.util.ExceptionUtil.concatMessageAndCauseMessages; +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.UUID.randomUUID;(Optional: For consistency, consider also replacing the MimeType star import on Line 31, if that aligns with project style.)
261-263: Make flow name lower-casing locale-safe as wellMirror the
Locale.ROOTfix here to keep URL construction deterministic under non-English locales.Apply:
- if (flow != null) { - see += "/" + flow.name().toLowerCase(); - } + if (flow != null) { + see += "/" + flow.name().toLowerCase(Locale.ROOT); + }
313-316: No existingAPPLICATION_PROBLEM_XMLconstant—add one or keepAPPLICATION_XMLIt looks like we already have
public static final String APPLICATION_PROBLEM_JSON = "application/problem+json";in
MimeType.java, but there is no correspondingAPPLICATION_PROBLEM_XML. You have two options:• Keep the current code using
APPLICATION_XML
• Introduce a new constant and then switch to it:
- In
core/src/main/java/com/predic8/membrane/core/http/MimeType.java, add:public static final String APPLICATION_PROBLEM_XML = "application/problem+xml";- Update the snippet in
ProblemDetails.java:- builder.contentType(APPLICATION_XML); + builder.contentType(APPLICATION_PROBLEM_XML);Optional: This brings parity with
application/problem+jsonand aligns with RFC 7807.core/src/main/java/com/predic8/membrane/core/util/MessageUtil.java (1)
56-57: Switch to readAllBytes() is fine; consider guarding very large payloads.readAllBytes() simplifies the gzip path and is consistent with the broader refactor. For extremely large responses, this fully materializes the payload and can spike memory. If this method may see untrusted/large bodies, consider a capped copy or streaming approach; otherwise, leave as-is.
core/src/main/java/com/predic8/membrane/core/util/ByteUtil.java (2)
28-39: Optionally fail fast on short reads when a fixed length is required.If fewer than length bytes are available, the current implementation returns a partially filled array. Consider throwing EOFException to make truncation explicit.
while (offset < length && (count = in.read(content, offset, length - offset)) >= 0) { offset += count; } - return content; + if (offset < length) { + throw new EOFException("Expected " + length + " bytes, got " + offset); + } + return content;
75-92: Minor clarity improvement looks good.Switching to !chunks.isEmpty() and simplifying the close-catch are fine; behavior remains unchanged. Note: ByteArrayOutputStream.close() is a no-op, so the try/catch is optional.
core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectorTest.java (5)
34-47: Close the stream and guard against missing resources; also flush the writer before reading bytes
getResourceAsStreammay return null (NPE risk withreadAllBytes()), and the stream is never closed. Additionally,OutputStreamWriterbuffers data; flush before callingbaos.toByteArray()to avoid truncated output on some JDKs. This keeps the test robust and aligns with the PR’s UTF-8 explicitness theme.Apply this diff:
- XMLProtector xmlProtector = new XMLProtector(new OutputStreamWriter(baos, UTF_8), removeDTD, 1000, 1000); - input = this.getClass().getResourceAsStream(resource).readAllBytes(); + Writer w = new OutputStreamWriter(baos, UTF_8); + XMLProtector xmlProtector = new XMLProtector(w, removeDTD, 1000, 1000); + InputStream is = this.getClass().getResourceAsStream(resource); + assertNotNull(is, "Test resource not found: " + resource); + try (is) { + input = is.readAllBytes(); + } @@ - boolean valid = xmlProtector.protect(new InputStreamReader(new ByteArrayInputStream(input), UTF_8)); + boolean valid = xmlProtector.protect(new InputStreamReader(new ByteArrayInputStream(input), UTF_8)); + w.flush();
51-63: Reverse bytes directly to avoid charset-dependent round-trips and inefficiencyThe current implementation converts bytes to a String using the platform default charset, reverses characters, then converts back using the platform default charset again. This is fragile and may break on non-UTF-8 platforms or with non-ASCII test data. Reverse the byte array in-place instead.
Apply this diff:
- private void reverse() { - // To reverse the input Stream - ByteArrayOutputStream bos = new ByteArrayOutputStream(); - try { - bos.write(input); - } catch (IOException e) { - LOG.error(e.getMessage(), e); - } - StringBuilder sb = new StringBuilder(); - sb.append(bos); - sb.reverse(); - input = String.valueOf(sb).getBytes(); - } + private void reverse() { + for (int i = 0, j = input.length - 1; i < j; i++, j--) { + byte tmp = input[i]; + input[i] = input[j]; + input[j] = tmp; + } + }
79-79: Use explicit UTF-8 when converting bytes to String in assertionsAvoid platform defaults; the PR moves toward explicit UTF-8 everywhere.
Apply this diff:
- assertFalse(new String(output).contains("ENTITY")); + assertFalse(new String(output, UTF_8).contains("ENTITY")); @@ - assertTrue(new String(output).contains("ENTITY")); + assertTrue(new String(output, UTF_8).contains("ENTITY"));Also applies to: 87-87
38-38: Match the extension preciselyMinor: ensure we only treat files with the “.lmx” extension specially.
Apply this diff:
- if (resource.endsWith("lmx")) { + if (resource.endsWith(".lmx")) {
71-73: Follow consistent test naming (lowerCamelCase) for readabilityJUnit 5 doesn’t care, but consistent naming helps readability and IDE navigation.
Apply this diff:
- void NotWellformed() throws Exception { + void notWellFormed() throws Exception {core/src/test/java/com/predic8/membrane/core/UnitTests.java (1)
26-41: Escape and anchor exclusion regexes for determinismThe eight tests you intend to exclude do exist under
core/src/test/javaand will currently be excluded because JUnit’s@ExcludeClassNamePatternsapplies each regex against the full FQCN, excluding any class whose name “matches” the pattern (junit.org). Java’sPattern.matches(...)enforces a full‐string match, so explicit^…$anchors aren’t strictly required—but escaping the literal dots (\.) and anchoring the pattern prevents any future accidental wildcard matches and makes your intent crystal-clear (docs.oracle.com).Files to update:
• core/src/test/java/com/predic8/membrane/core/UnitTests.java (lines 26–41)Illustrative (optional) diff:
@ExcludeClassNamePatterns({ - "com.predic8.membrane.core.interceptor.opentelemetry.OpenTelemetryInterceptorTest", + "^com\\.predic8\\.membrane\\.core\\.interceptor\\.opentelemetry\\.OpenTelemetryInterceptorTest$", - "com.predic8.membrane.core.interceptor.balancer.NodeOnlineCheckerTest", + "^com\\.predic8\\.membrane\\.core\\.interceptor\\.balancer\\.NodeOnlineCheckerTest$", - "com.predic8.membrane.core.interceptor.tunnel.WebsocketStompTest", + "^com\\.predic8\\.membrane\\.core\\.interceptor\\.tunnel\\.WebsocketStompTest$", - "com.predic8.membrane.core.interceptor.oauth2client.OAuth2Resource2InterceptorTest", + "^com\\.predic8\\.membrane\\.core\\.interceptor\\.oauth2client\\.OAuth2Resource2InterceptorTest$", - "com.predic8.membrane.core.interceptor.shadowing.ShadowingInterceptorTest", + "^com\\.predic8\\.membrane\\.core\\.interceptor\\.shadowing\\.ShadowingInterceptorTest$", - "com.predic8.membrane.core.interceptor.rewrite.RewriteInterceptorIntegrationTest", + "^com\\.predic8\\.membrane\\.core\\.interceptor\\.rewrite\\.RewriteInterceptorIntegrationTest$", - "com.predic8.membrane.core.transport.http.ConnectionTest", + "^com\\.predic8\\.membrane\\.core\\.transport\\.http\\.ConnectionTest$", - "com.predic8.membrane.core.util.MemcachedConnectorTest" + "^com\\.predic8\\.membrane\\.core\\.util\\.MemcachedConnectorTest$" })pom.xml (1)
553-557: Enforce UTF-8 in your failsafe executionsThe root POM already sets
<argLine>-Dfile.encoding=UTF-8</argLine>for the Surefire plugin (pom.xml lines 552–556), but the Failsafe plugin in the same file (lines 558–562) — and in your submodules — doesn’t yet mirror it. To ensure integration tests run with the same encoding, add the same<argLine>block to eachmaven-failsafe-plugindeclaration:• pom.xml (root), lines 558–562
• war/pom.xml, lines 133–136
• core/pom.xml, lines 420–424
• distribution/pom.xml, lines 188–192<plugin> <artifactId>maven-failsafe-plugin</artifactId> <version>3.1.2</version> + <configuration> + <argLine>-Dfile.encoding=UTF-8</argLine> + </configuration> <executions> … </executions> </plugin>Also keep in mind that when you introduce JaCoCo (prepare-agent) later, it will rewrite
argLineby default. Be sure to leverage property expansion (e.g.<argLine>${argLine} -javaagent:…</argLine>) so coverage tooling appends rather than overwrites your encoding setting.core/src/main/java/com/predic8/membrane/core/http/Message.java (1)
139-144: Harden charset resolution & confirm removal of hasMsgReleased
- In
core/src/main/java/com/predic8/membrane/core/http/Message.java(lines 139–144), the current implementationwill throw if the header’s charset is invalid. It’s safer to fall back to UTF-8 with logging, for example:String charsetName = getCharset(); Charset charset = charsetName != null ? Charset.forName(charsetName) : UTF_8; return new String(MessageUtil.getContent(this), charset);String charsetName = getCharset();- Charset charset = charsetName != null ? Charset.forName(charsetName) : UTF_8;
- return new String(MessageUtil.getContent(this), charset);
- Charset charset = UTF_8;
- if (charsetName != null) {
try {charset = Charset.forName(charsetName);} catch (IllegalArgumentException ex) {// covers UnsupportedCharsetException and IllegalCharsetNameExceptionlog.debug("Unsupported/invalid charset '{}', defaulting to UTF-8.", charsetName, ex);}- }
- return new String(MessageUtil.getContent(this), charset);
- Verified that
hasMsgReleased()has been fully removed—no remaining call sites or definitions were found in the Java sources.core/src/test/java/com/predic8/membrane/interceptor/ws_addressing/WsaEndpointRewriterTest.java (2)
17-21: Avoid platform-default charset and prefer JDK ByteArrayOutputStream
- The test currently depends on commons-io ByteArrayOutputStream and uses default charset in getBytes()/toString(). Prefer JDK’s ByteArrayOutputStream and explicit UTF-8 for deterministic behavior across environments.
-import org.apache.commons.io.output.ByteArrayOutputStream; +// use JDK ByteArrayOutputStream (already covered by java.io.*) @@ - is = new ByteArrayInputStream(SOAP_WITH_ADDRESSING.getBytes()); + is = new ByteArrayInputStream(SOAP_WITH_ADDRESSING.getBytes(java.nio.charset.StandardCharsets.UTF_8)); @@ - ByteArrayOutputStream output = new ByteArrayOutputStream(); + ByteArrayOutputStream output = new ByteArrayOutputStream(); @@ - String xml = output.toString(); + String xml = new String(output.toByteArray(), java.nio.charset.StandardCharsets.UTF_8);Also applies to: 49-50, 54-60
45-50: Optional: localize stream lifetime to the test and auto-closeField-level InputStream is not closed. For a self-contained test, consider creating the stream in the test method with try-with-resources.
-InputStream is; - -@BeforeEach -public void setUp() { - is = new ByteArrayInputStream(SOAP_WITH_ADDRESSING.getBytes(java.nio.charset.StandardCharsets.UTF_8)); -} +// no field neededAnd in the test:
try (InputStream is = new ByteArrayInputStream(SOAP_WITH_ADDRESSING.getBytes(StandardCharsets.UTF_8)); ByteArrayOutputStream output = new ByteArrayOutputStream()) { // ... same body ... }core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/RequestParameterizedTest.java (1)
30-41: No parallel test execution is configuredI inspected your build settings and found that:
- The Maven Surefire plugin (version 3.1.2) is declared in your root pom.xml without any
<parallel>configuration.- There are no JUnit Jupiter properties (e.g.
junit.jupiter.execution.parallel.enabled) defined in your codebase.By default, JUnit Jupiter runs tests sequentially, so the current use of static fields in RequestParameterizedTest does not introduce data races under today’s configuration.
To guard against future changes—should parallel execution ever be enabled—consider one of the following optional refactorings:
- Annotate the test class with JUnit’s
@Execution(ExecutionMode.SAME_THREAD)to force single-threaded execution regardless of global settings.- Refactor the shared static fields (
oasit,exc) into instance-scoped fixtures so each test method gets its own state.These measures aren’t strictly necessary today but will prevent subtle concurrency bugs if parallelism is turned on later.
core/src/main/java/com/predic8/membrane/core/interceptor/balancer/XMLElementSessionIdExtractor.java (1)
105-109: Namespace should be optional (aligns with No-NS test)The setter is annotated @required, but the No-NS test configures no namespace. Make namespace optional in configuration.
Apply this diff:
- @Required @MCAttribute public void setNamespace(String namespace) { this.namespace = namespace; }core/src/test/java/com/predic8/membrane/core/interceptor/balancer/XMLSessionIdExtractorTest.java (1)
54-56: Close resource when reading test fixtureWrap the resource stream in try-with-resources to avoid warnings and leaks in tools that track open streams.
Apply this diff:
- private byte[] getBodyContent() throws IOException { - return getClass().getResourceAsStream("/getBankwithSession555555.xml").readAllBytes(); - } + private byte[] getBodyContent() throws IOException { + try (InputStream is = getClass().getResourceAsStream("/getBankwithSession555555.xml")) { + return is.readAllBytes(); + } + }core/src/test/java/com/predic8/membrane/core/http/HeaderTest.java (2)
117-120: Test name/content mismatch: this duplicates the multi-header caseThis test claims “one header” but asserts the multi-value result against h1. Either rename or make it truly single-value.
Apply this diff:
- void getNormalizedValueFromOneHeader() { - assertEquals("192.3.14.1,10.0.0.1,2001:db8:85a3:8d3:1319:8a2e:370:7348", h1.getNormalizedValue(X_FORWARDED_FOR)); - } + void getNormalizedValueFromOneHeader() { + Header h = new Header(); + h.add(X_FORWARDED_FOR, "192.3.14.1"); + assertEquals("192.3.14.1", h.getNormalizedValue(X_FORWARDED_FOR)); + }
68-73: Minor: avoid shadowing field ‘header’ with a local var of the same nameRenaming the local variable reduces cognitive load while reading tests.
Apply this diff:
- var header = new Header(); - header.setValue("X-Foo", "123"); + var h = new Header(); + h.setValue("X-Foo", "123"); - assertTrue(header.contains("x-foo")); - assertFalse(header.contains("x-bar")); + assertTrue(h.contains("x-foo")); + assertFalse(h.contains("x-bar"));core/src/main/java/com/predic8/membrane/core/prettifier/AbstractPrettifier.java (1)
11-13: Annotate parameter nullability for clarityThe method accepts null and returns non-null. Make that explicit on the parameter.
Apply this diff:
- protected static @NotNull Charset getCharset(Charset charset) { + protected static @NotNull Charset getCharset(@Nullable Charset charset) { return (charset != null) ? charset : UTF_8; }core/src/test/java/com/predic8/membrane/core/prettifier/XMLPrettifierTest.java (1)
45-48: UTF-16BE expectation vs output charset selectionXMLPrettifier maps UTF-16BE/LE requests to UTF-16 (with BOM). Decoding the result using UTF_16BE may work but can be brittle across JVMs due to BOM handling. Consider asserting with UTF_16 to match the intended output or assert both.
Option A (align to emitter):
- assertChars(new String(prettifier.prettify(getResourceAsBytes("/charsets/utf-16be-unformatted.xml"), UTF_16BE), UTF_16BE)); + assertChars(new String(prettifier.prettify(getResourceAsBytes("/charsets/utf-16be-unformatted.xml"), UTF_16BE), UTF_16));Option B (dual check to be safe across JDKs):
var out = prettifier.prettify(getResourceAsBytes("/charsets/utf-16be-unformatted.xml"), UTF_16BE); assertDoesNotThrow(() -> assertChars(new String(out, UTF_16))); assertDoesNotThrow(() -> assertChars(new String(out, UTF_16BE)));core/src/test/java/com/predic8/membrane/core/prettifier/PrettifierTest.java (4)
25-30: Prefer identity assertions for singletons (use assertSame instead of assertEquals).Since the factory returns singleton instances, asserting identity communicates intent more clearly and avoids relying on equals(). Recommend switching to assertSame in these assertions.
Apply this diff:
- assertEquals(JSONPrettifier.INSTANCE, getInstance(APPLICATION_JSON)); - assertEquals(JSONPrettifier.INSTANCE, getInstance(APPLICATION_JSON_UTF8)); - assertEquals(JSONPrettifier.INSTANCE, getInstance(APPLICATION_PROBLEM_JSON)); + assertSame(JSONPrettifier.INSTANCE, getInstance(APPLICATION_JSON)); + assertSame(JSONPrettifier.INSTANCE, getInstance(APPLICATION_JSON_UTF8)); + assertSame(JSONPrettifier.INSTANCE, getInstance(APPLICATION_PROBLEM_JSON));
32-37: Consider expanding XML mappings coverage (optional).If intended, add cases for additional XML-ish media types (e.g., application/soap+xml). This protects the factory mapping from regressions as more types are recognized as XML.
39-43: Prefer assertSame for singleton instances here as well.Same rationale as the JSON case.
Apply this diff:
- assertEquals(TextPrettifier.INSTANCE, getInstance(TEXT_PLAIN)); - assertEquals(TextPrettifier.INSTANCE, getInstance(TEXT_PLAIN_UTF8)); + assertSame(TextPrettifier.INSTANCE, getInstance(TEXT_PLAIN)); + assertSame(TextPrettifier.INSTANCE, getInstance(TEXT_PLAIN_UTF8));
45-48: Add a null-content-type case to document default behavior.A quick test for getInstance(null) → NullPrettifier.INSTANCE helps future readers and guards against NPEs/regressions.
Example addition:
@Test void unknown() { - assertEquals(NullPrettifier.INSTANCE, getInstance("unknown")); + assertSame(NullPrettifier.INSTANCE, getInstance("unknown")); + assertSame(NullPrettifier.INSTANCE, getInstance(null)); }core/src/test/java/com/predic8/xml/beautifier/XMLBeautifierTest.java (3)
148-169: Prolog encoding propagation test is valuable; minor naming nit.The test intent is clear and assertions are precise. Consider making the DisplayName explicitly say “UTF-8” to tie to the constant used, but optional.
171-187: Display name contradicts assertions for CDATA; clarify intent.The test asserts that CDATA wrappers are not preserved (content is escaped, and “CDATA” absent). Rename to avoid confusion and tweak the failure message.
Apply this diff:
- @DisplayName("CDATA sections are preserved verbatim in output") + @DisplayName("CDATA wrappers are normalized: content escaped, no CDATA in output") @@ - assertFalse(result.contains("CDATA"),"CDATA sections are removed"); + assertFalse(result.contains("CDATA"), "Expected no CDATA sections in output");
189-205: Method name/display name suggest “remain split” but you assert concatenated characters.The behavior verified is correct (character data “foo]]>bar” in the result). Rename for clarity.
Apply this diff:
- @DisplayName("Adjacent/split CDATA sections remain split") - void adjacentCdata_sectionsRemainSplit() throws Exception { + @DisplayName("Adjacent CDATA sections yield the same character data (foo]]>bar)") + void adjacentCdata_yieldsSameCharacters() throws Exception {core/src/main/java/com/predic8/membrane/core/interceptor/rest/XML2HTTP.java (4)
52-52: Broaden XML detection beyond exact TEXT_XML_UTF8 match (optional).The current gate only unwraps when Content-Type equals “text/xml;charset=UTF-8”. Consider using MimeType.isXML(contentType) (or at least accepting application/xml, text/xml with other charsets, and application/soap+xml) to avoid missing valid XML envelopes.
Apply this diff:
- if (MimeType.TEXT_XML_UTF8.equals(message.getHeader().getContentType())) { + if (MimeType.isXML(message.getHeader().getContentType())) {
139-139: Improve log message context.log.error("", e) emits a stack trace without context. Add a short message to aid debugging.
Apply this diff:
- log.error("", e); + log.error("Failed to unwrap XML-HTTP document for {}", message.getUri(), e);
57-61: Close XMLEventReader to free parser resources (optional).After parsing completes, calling parser.close() is cheap and avoids retaining buffers in some StAX implementations.
Example:
- synchronized(xmlInputFactory) { - parser = getXmlEventReader(message); - } + synchronized (xmlInputFactory) { + parser = getXmlEventReader(message); + } /* States: ... */ int state = 0; ... - while (parser.hasNext()) { + try { + while (parser.hasNext()) { // loop - } + } + } finally { + try { parser.close(); } catch (Exception ignore) {} + }
213-219: Exception prefix references REST2SOAP — likely a copy/paste.Consider changing the prefix to “XML2HTTP” for accurate diagnostics.
Apply this diff:
- super("REST2SOAP: " + message); + super("XML2HTTP: " + message);core/src/test/java/com/predic8/membrane/core/interceptor/beautifier/BeautifierInterceptorTest.java (3)
81-89: Make jsonResponse test consistent with request test and intent.
- The response currently lacks Content-Type: application/json, which weakens the “jsonResponse” premise.
- Comparing byte length to String.length() (characters) is brittle; prefer bytes with a specific charset as you did in jsonRequest.
Apply this diff:
- Exchange exc = Response.ok().body(jsonString()).buildExchange(); + Exchange exc = Response.ok() + .contentType(APPLICATION_JSON) + .body(jsonString()) + .buildExchange(); @@ - assertEquals(bodyLength, res.getBodyAsStringDecoded().length()); + assertEquals(bodyLength, res.getBodyAsStringDecoded().getBytes(UTF_8).length);
146-157: Defensive null check for test resources (optional).getResourceAsStream may return null, yielding a late NPE. Fail fast with a clear message to simplify debugging.
Example change:
- Exchange exc = post("/foo") - .contentType(getContentType(contentTypeCharset)) - .body(getClass().getResourceAsStream(file)) - .buildExchange(); + var is = java.util.Objects.requireNonNull( + getClass().getResourceAsStream(file), + "Missing test resource: " + file + ); + Exchange exc = post("/foo") + .contentType(getContentType(contentTypeCharset)) + .body(is) + .buildExchange();
135-140: Fix typo in test name (“unknow” → “unknown”).Minor readability polish.
Apply this diff:
- @DisplayName("Fallback for unknown charsets") - void unknowEncodingInfoInTheHeader() throws Exception { + @DisplayName("Fallback for unknown charsets") + void unknownEncodingInfoInTheHeader() throws Exception {core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/WellknownEndpointProcessor.java (1)
39-41: Guard against potential null well-known contentIf
authServer.getWellknownFile()or itsgetWellknown()returns null,.getBytes(UTF_8)will NPE. JSONPrettifier safely degrades on parse errors, but not on null input.Apply this diff to fail soft with an empty object when the well-known JSON is unexpectedly null:
- exc.setResponse(ok().contentType(APPLICATION_JSON_UTF8).body(JSONPrettifier.INSTANCE.prettify(authServer.getWellknownFile().getWellknown().getBytes(UTF_8), UTF_8)).build()); + String wellknown = java.util.Objects.requireNonNullElse(authServer.getWellknownFile().getWellknown(), "{}"); + exc.setResponse( + ok() + .contentType(APPLICATION_JSON_UTF8) + .body(JSONPrettifier.INSTANCE.prettify(wellknown.getBytes(UTF_8), UTF_8)) + .build() + );If you prefer, statically import
requireNonNullElsefor brevity:// add near other static imports import static java.util.Objects.requireNonNullElse;core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (2)
97-101: Prefer Charset over charset name for OutputStreamWriterPassing a name defers failure to runtime and is slightly noisier. Using
Charsetis cleaner and avoidsUnsupportedEncodingException.Apply:
- return new Relocator(new OutputStreamWriter(stream, - requireNonNullElseGet(exc.getResponse().getCharset(), UTF_8::name)), getLocationProtocol(), getLocationHost(exc), + java.nio.charset.Charset cs = java.util.Optional.ofNullable(exc.getResponse().getCharset()) + .map(java.nio.charset.Charset::forName).orElse(UTF_8); + return new Relocator(new OutputStreamWriter(stream, cs), getLocationProtocol(), getLocationHost(exc), getLocationPort(exc), exc.getHandler().getContextPath(exc), pathRewriter);
70-71: The currentRelocatoronly delegates to anXMLEventReaderand does not close the underlying stream or reader. I recommend still wrapping the call in try-with-resources to ensure the response body is always closed:• Wrap
exc.getResponse().getBodyAsStream()in try-with-resources around the relocate call.
• No changes toRelocatorare necessary.--- a/core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java +++ b/core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java @@ -67,7 +67,11 @@ public class WSDLInterceptor implements Interceptor { - relocator.relocate(exc.getResponse().getBodyAsStream()); + try (InputStream in = exc.getResponse().getBodyAsStream()) { + relocator.relocate(in); + } + + // If you’re inspecting isWsdlFound() afterward, the stream is safely closed. if (relocator.isWsdlFound()) {core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/CertsEndpointProcessor.java (1)
44-51: Avoid manual JSON concatenation for JWKSString concatenation is fragile for JSON (e.g., spacing, accidental commas). Either build via Jackson or, minimally, use a StringBuilder. Since you already prettify downstream, a small cleanup improves readability.
Apply:
- private @NotNull String getJwks() { + private @NotNull String getJwks() { String accessTokenJWKIfAvailable = authServer.getTokenGenerator().getJwkIfAvailable(); String idTokenJWK = authServer.getJwtGenerator().getJwk(); - - return "{\"keys\": [ " + idTokenJWK + - (accessTokenJWKIfAvailable != null ? "," + accessTokenJWKIfAvailable : "") + - "]}"; + StringBuilder sb = new StringBuilder(64 + + idTokenJWK.length() + + (accessTokenJWKIfAvailable != null ? accessTokenJWKIfAvailable.length() + 1 : 0)); + sb.append("{\"keys\": [ ").append(idTokenJWK); + if (accessTokenJWKIfAvailable != null) sb.append(",").append(accessTokenJWKIfAvailable); + sb.append("]}"); + return sb.toString(); }If there’s any chance
idTokenJWKcan be null, add a precondition or fallback to avoid producing invalid JSON.Would you like a Jackson-based version that avoids parse+prettify altogether?
core/src/main/java/com/predic8/membrane/core/exchangestore/FileExchangeStore.java (1)
149-151: Favor InputStream-based formatter and rely on XML prolog for charset detectionYou currently decode using the header charset (or UTF-8) and then re-encode as UTF-8. Using the
formatXML(InputStream, boolean)overload allows the parser to honor the XML declaration, which is more robust when headers are wrong or missing.Apply:
- os.write(formatXML( - new InputStreamReader(body.getContentAsStream(), requireNonNullElseGet(msg.getHeader().getCharset(), UTF_8::name))) - .getBytes(UTF_8)); + try (InputStream is = body.getContentAsStream()) { + os.write(formatXML(is, /*asHTML*/ false).getBytes(UTF_8)); + }This still guarantees UTF-8 on disk (as per the class javadoc) while improving input decoding fidelity and ensuring the stream is closed.
core/src/main/java/com/predic8/xml/beautifier/XMLInputFactoryFactory.java (1)
44-46: Consider adding IS_VALIDATING=false and documenting ThreadLocal lifecycleTwo minor nits:
- Explicitly set
XMLInputFactory.IS_VALIDATINGtofalsefor completeness (some impls use it).- Document that the ThreadLocal is intentionally never cleared; in environments with thread pools this is fine, but a note helps future maintainers.
Apply:
// Disable entity resolving f.setXMLResolver((publicId, systemId, baseURI, namespace) -> new ByteArrayInputStream(new byte[0])); + // Ensure validation is disabled across implementations + try { f.setProperty(IS_VALIDATING, Boolean.FALSE); } catch (IllegalArgumentException ignore) {}And add a short comment above
TLabout the intended lifetime.core/src/test/java/com/predic8/membrane/interceptor/ws_addressing/WsaEndpointRewriterInterceptorTest.java (2)
58-61: Set Content-Type to SOAP to make the test deterministicIf the interceptor gates on MIME type, having no Content-Type may make the rewrite path depend on defaults. Setting a SOAP content type removes ambiguity.
- Exchange exc = Request.post("/foo").body(body).buildExchange(); + Exchange exc = Request.post("/foo").body(body).buildExchange(); + exc.getRequest().getHeader().setContentType("application/soap+xml");
60-62: Strengthen assertions to ensure the original host was actually replacedAsserting the absence of the old host guards against false positives (e.g., if the host only got appended).
assertEquals(CONTINUE, rewriter.handleRequest(exc)); - assertTrue(exc.getRequest().getBodyAsStringDecoded().contains("membrane-api.io")); + String rewritten = exc.getRequest().getBodyAsStringDecoded(); + assertTrue(rewritten.contains("membrane-api.io")); + assertFalse(rewritten.contains("api.predic8.de"));core/src/main/java/com/predic8/membrane/core/prettifier/Prettifier.java (2)
27-28: Interface-level logger is unused — remove for cleanlinessKeeping a logger on the interface is unusual and unused here. Prefer loggers in implementations.
- Logger log = LoggerFactory.getLogger(Prettifier.class); + // Intentionally no logger on the interface; implementations log as needed.
54-61: Route text/html to TextPrettifier instead of XMLPrettifierReal-world HTML is often not well-formed XML. Sending it to XMLPrettifier increases failure likelihood; TextPrettifier yields predictable formatting without parse errors.
- // XML/HTML family: text/xml, application/xml, text/html (and charset variants) - if (isXML(ct) || isHtml(ct)) - return XMLPrettifier.INSTANCE; + // XML family + if (isXML(ct)) + return XMLPrettifier.INSTANCE; + + // HTML (not guaranteed well-formed XML) + if (isHtml(ct)) + return TextPrettifier.INSTANCE;core/src/main/java/com/predic8/membrane/core/interceptor/beautifier/BeautifierInterceptor.java (2)
63-68: Skip work for unknown content types and inline the temp variableAvoid re-reading and re-setting the body when no prettifier applies; also the local variable is not necessary.
- byte[] prettified; try { - Prettifier prettifier = getPrettifier(msg); - prettified = prettifier.prettify(msg.getBodyAsStreamDecoded(), getCharset(msg, prettifier)); - msg.setBodyContent(prettified); + Prettifier prettifier = getPrettifier(msg); + if (prettifier instanceof NullPrettifier) { + return CONTINUE; + } + msg.setBodyContent(prettifier.prettify(msg.getBodyAsStreamDecoded(), getCharset(msg, prettifier))); } catch (IOException e) {
77-90: Charset nullability: document or avoid passing nullgetCharset returns null for XML. Today, XMLPrettifier ignores the charset argument, so this is safe. Consider documenting in Prettifier that charset may be null when the format self-declares encoding, or supply UTF-8 as a neutral default here.
core/src/main/java/com/predic8/membrane/core/interceptor/ws_addressing/WsaEndpointRewriter.java (1)
92-95: Preserve query/fragment and avoid always emitting a portCurrent rewrite drops query/fragment and always prints “:port”. Preserve URI components and omit the port if not explicitly set.
- private static @NotNull String rewrite(String address, WsaEndpointRewriterInterceptor.Location location) throws URISyntaxException { - URI orig = new URIFactory(false).create(address); - return location.protocol() + "://" + location.host() + ":" + location.port() + orig.getPath(); - } + private static @NotNull String rewrite(String address, WsaEndpointRewriterInterceptor.Location location) throws URISyntaxException { + URI orig = new URIFactory(false).create(address); + StringBuilder sb = new StringBuilder(); + sb.append(location.protocol()).append("://").append(location.host()); + if (location.port() > 0) { + sb.append(':').append(location.port()); + } + if (orig.getPath() != null) sb.append(orig.getPath()); + if (orig.getQuery() != null) sb.append('?').append(orig.getQuery()); + if (orig.getFragment() != null) sb.append('#').append(orig.getFragment()); + return sb.toString(); + }core/src/main/java/com/predic8/membrane/core/prettifier/XMLPrettifier.java (1)
47-55: Prefer a meaningful debug message on failure pathEmpty log messages make troubleshooting harder; include a short context string.
- } catch (Exception e) { - log.debug("", e); + } catch (Exception e) { + log.debug("Failed to prettify XML input (byte[]). Returning original bytes.", e); return c; }core/src/main/java/com/predic8/membrane/core/interceptor/ws_addressing/WsaEndpointRewriterInterceptor.java (3)
40-51: Broaden exception handling or rethrow with contextCatching only
XMLStreamExceptionrisks leaking other IO/runtime errors (e.g., malformed input streams) and returning a broken request downstream.- } catch (XMLStreamException e) { + } catch (Exception e) { log.error("",e); internal(router.isProduction(),getDisplayName()) .detail("Could not rewrite endpoint!") .exception(e) .buildAndSetResponse(exc); return ABORT; }If
rewriteEndpointhas a tighter throws clause, keep the catch list aligned but ensure other exceptions are not silently ignored upstream.
52-55: Remove stale Content-Encoding after modifying decoded bodyYou read a decoded stream and then set raw bytes. If the request had a
Content-Encoding(gzip, deflate, ...), it should be removed to prevent downstream double-decompression assumptions.exc.getRequest().setBodyContent(output.toByteArray()); + // Body has been rewritten from a decoded stream -> drop Content-Encoding + exc.getRequest().getHeader().removeFields(com.predic8.membrane.core.http.Header.CONTENT_ENCODING);
41-41: Nit: remove outdated commentThe "Why is port 2020 hard coded?" comment is obsolete with the new configurable target. Please delete to avoid confusion.
core/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java (2)
15-27: Optional: consolidate XMLOutputFactory creation if you introduce an output factory laterNot urgent, but for symmetry and future hardening, consider a small utility similar to
XMLInputFactoryFactoryfor output factory creation/config (even if only default for now).
160-176: Do not reassign method parameter for clarity
portis reassigned withingetNewLocation. Favor a local variable to avoid confusion in debugging.- if (port == -1) { + int effectivePort = port; + if (effectivePort == -1) { return new URL(protocol, host, contextPath + oldURL.getFile()).toString(); } - if ("http".equals(protocol) && port == 80) - port = -1; - if ("https".equals(protocol) && port == 443) - port = -1; - return new URL(protocol, host, port, contextPath + oldURL.getFile()).toString(); + if ("http".equals(protocol) && effectivePort == 80) + effectivePort = -1; + if ("https".equals(protocol) && effectivePort == 443) + effectivePort = -1; + return new URL(protocol, host, effectivePort, contextPath + oldURL.getFile()).toString();core/src/main/java/com/predic8/membrane/core/prettifier/JSONPrettifier.java (2)
42-53: Charset parameter is ignored; clarify contract or respect caller-provided charsetBoth methods assume UTF-8. That’s sane for JSON, but the API exposes a
charsetwhich is misleading if callers expect it to be honored or if upstream sets a non-UTF-8 charset in headers.Options:
- Document explicitly that output is UTF-8 regardless of
charset, and ensure callers setContent-Type; charset=UTF-8.- Or, emit a String and re-encode with the negotiated charset:
- public byte[] prettify(byte[] c, Charset charset) { - try { - return om.writerWithDefaultPrettyPrinter().writeValueAsBytes(om.readTree(c)); - } catch (IOException e) { + public byte[] prettify(byte[] c, Charset charset) { + try { + String pretty = om.writerWithDefaultPrettyPrinter().writeValueAsString(om.readTree(c)); + return pretty.getBytes(charset != null ? charset : StandardCharsets.UTF_8); + } catch (IOException e) { log.debug("Failed to prettify JSON. Returning input unmodified.", e); return c; } }Pick one approach and keep it consistent across call sites.
55-58: Inconsistent error behavior between overloadsThe byte[] overload swallows exceptions and returns input; the InputStream variant propagates. Consider aligning behavior to avoid surprising callers.
core/src/main/java/com/predic8/membrane/core/prettifier/TextPrettifier.java (2)
90-101: Indent detection is OK; consider reusing TextUtil to avoid duplicationYou already have similar helpers in
TextUtil(getCurrentIndent,getMinIndent). Consider delegating to avoid divergence and future maintenance.
35-37: Large inputs: consider streaming instead of readAllBytesFor very large payloads,
readAllBytes()is memory-heavy. Not critical for typical UI/console text, but worth noting.
...c/main/java/com/predic8/membrane/core/interceptor/balancer/XMLElementSessionIdExtractor.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/predic8/membrane/core/interceptor/balancer/XMLElementSessionIdExtractor.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/cache/PositiveNode.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/rest/XML2HTTP.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/prettifier/XMLPrettifier.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/interceptor/balancer/ClusterBalancerTest.java
Outdated
Show resolved
Hide resolved
...t/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptorTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (1)
65-74: Treat whitespace-only encodings as blank; trim before resolutionTrim the input and use the trimmed value for resolution and logging. This avoids surprises with headers like " UTF-8 " and aligns with prior guidance.
- public static Charset getCharset(String encoding, Charset defaultCharset) { - if (encoding == null || encoding.isEmpty()) - return defaultCharset; - try { - return Optional.of(encoding).map(Charset::forName).orElse(defaultCharset); - } catch (Exception e) { - log.info("Encoding '{}' caused error: {}, Use default: {}", encoding, e.getMessage(), defaultCharset); - return defaultCharset; - } - } + public static Charset getCharset(String encoding, Charset defaultCharset) { + if (encoding == null) return defaultCharset; + String enc = encoding.trim(); + if (enc.isEmpty()) return defaultCharset; + try { + return Charset.forName(enc); + } catch (Exception e) { + log.info("Encoding '{}' caused error: {}, Use default: {}", enc, e.getMessage(), defaultCharset); + return defaultCharset; + } + }core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java (2)
92-137: Always close XMLEventReader and clear ThreadLocal via finally to avoid leaks.Early returns (limits, DTD handling) and exceptions bypass parser.close() and ThreadLocal cleanup, which can pin factories on pooled threads and leak resources. Move cleanup to finally.
Apply:
- public boolean protect(InputStreamReader isr) throws XMLProtectionException { - try { - XMLEventReader parser = xmlInputFactoryFactory.get().createXMLEventReader(isr); + public boolean protect(InputStreamReader isr) throws XMLProtectionException { + XMLEventReader parser = null; + try { + parser = xmlInputFactoryFactory.get().createXMLEventReader(isr); while (parser.hasNext()) { XMLEvent event = parser.nextEvent(); if (event.isStartElement()) { StartElement startElement = event.asStartElement(); if (maxElementNameLength != -1 && startElement.getName().getLocalPart().length() > maxElementNameLength) { log.warn("Element name length: Limit exceeded."); return false; } if (maxAttributeCount != -1) { Iterator<?> attrs = startElement.getAttributes(); int attributeCount = 0; while (attrs.hasNext()) { attrs.next(); attributeCount++; if (attributeCount > maxAttributeCount) { log.warn("Number of attributes per element: Limit exceeded."); return false; } } } } if (event instanceof javax.xml.stream.events.DTD dtd) { checkExternalEntities(dtd); if (removeDTD) { log.debug("removed DTD."); continue; } } writer.add(event); } writer.flush(); } catch (XMLStreamException e) { Location loc = e.getLocation(); log.warn("Received not well-formed XML at line {}, column {}: {}", loc != null ? loc.getLineNumber() : -1, loc != null ? loc.getColumnNumber() : -1, e.getMessage()); return false; - } + } finally { + if (parser != null) { + try { parser.close(); } catch (XMLStreamException ignore) {} + } + // Avoid retaining per-thread factories across pooled threads. + xmlInputFactoryFactory.remove(); + } return true; }To verify, run tests or produce a small XML exceeding the attribute/name limits and confirm the finally block still executes (e.g., via a breakpoint or temporary log).
139-159: Missing no‑op XMLResolver permits pre‑check external entity resolution on some StAX impls.Even with javax.xml.stream.isSupportingExternalEntities=false, some implementations resolve external subsets before emitting DTD. Without a no‑op XMLResolver, that can still hit the filesystem/network.
Apply:
private XMLInputFactory getXmlInputFactory() { // Return a new, fully hardened XMLInputFactory instance. That is not shared XMLInputFactory f = XMLInputFactory.newInstance(); // hardening f.setProperty(IS_COALESCING, FALSE); // Support DTDs on purpose to detect them in the StAX loop! f.setProperty(SUPPORT_DTD, TRUE); f.setProperty(IS_NAMESPACE_AWARE, TRUE); f.setProperty(IS_REPLACING_ENTITY_REFERENCES, FALSE); try { f.setProperty(JAVAX_XML_STREAM_IS_SUPPORTING_EXTERNAL_ENTITIES, FALSE); } catch (IllegalArgumentException ignored) { } + // Guarantee that no external resolution happens regardless of implementation quirks. + f.setXMLResolver((publicId, systemId, baseURI, namespace) -> new ByteArrayInputStream(new byte[0])); try { f.setProperty(IS_VALIDATING, FALSE); } catch (IllegalArgumentException ignored) { } return f; }This keeps DTD events visible for removal/detection while preventing any external entity fetches.
🧹 Nitpick comments (7)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (4)
102-108: Javadoc nit: parameter description contradicts behavior“asHTML Should output formatted as XML” is misleading. When true, output is HTML-formatted. Update the doc to avoid confusion.
- * @param asHTML Should output formatted as XML + * @param asHTML If true, format as HTML; otherwise, as XML.
109-118: Catch the actually thrown type for Reader-parse to ensure loggingXMLBeautifier.parse(Reader) does not declare XMLStreamException specifically. Catch a broader type so parse failures are logged consistently (you already rethrow).
- public static String formatXML(Reader reader, boolean asHTML) throws Exception { - try { + public static String formatXML(Reader reader, boolean asHTML) throws Exception { + try { StringWriter out = new StringWriter(STRING_BUFFER_INITIAL_CAPACITY_FOR_XML); new XMLBeautifier(getXmlBeautifierFormatter(asHTML, out)).parse(reader); return out.toString(); - } catch (XMLStreamException e) { + } catch (Exception e) { log.info("Error parsing XML: {}", e.getMessage()); throw e; } }
217-222: removeFinalChar: null handling changed; confirm intended API contractReturning "" for null may surprise callers; previous utilities usually round-trip null. Suggest returning null unchanged while keeping code-point correctness.
- public static String removeFinalChar(String s) { - if (s == null || s.isEmpty()) - return ""; + public static String removeFinalChar(String s) { + if (s == null || s.isEmpty()) + return s; int end = s.offsetByCodePoints(s.length(), -1); return s.substring(0, end); }
245-267: unifyIndent/trimLines removes trailing spaces and converts tabs → spaces; optional refinementIf preserving intra-line whitespace matters (e.g., code blocks), avoid line.trim() and subtract exactly minIndent from leading whitespace, retaining trailing spaces and tabs. Current behavior is fine for most text; consider this if formatting fidelity is important.
- public static StringBuilder trimLines(String[] lines, int minIndent) { + public static StringBuilder trimLines(String[] lines, int minIndent) { StringBuilder result = new StringBuilder(); for (String line : lines) { - if (!line.trim().isEmpty()) { - result.append(" ".repeat(Math.max(getCurrentIndent(line) - minIndent, 0))).append(line.trim()).append("\n"); - } else { + if (!line.trim().isEmpty()) { + int curIndent = getCurrentIndent(line); + int cut = Math.min(minIndent, curIndent); + result.append(line.substring(cut)).append("\n"); + } else { result.append("\n"); } } return result; }core/src/main/java/com/predic8/membrane/core/interceptor/rewrite/RewriteInterceptor.java (1)
257-265: Description formatting is good, but will be affected by TextUtil.capitalizeFirstCharacter bugThe phrase construction reads well (“Permanently or temporarily redirects, or rewrites URLs.”). However, TextUtil.capitalizeFirstCharacter currently uppercases the entire string; once fixed (see comment in TextUtil.java), this will render as intended.
core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java (2)
26-28: Static wildcard imports from java.lang.Boolean and internal factory are a minor readability hit.Optional: replace with explicit references (Boolean.FALSE/TRUE or static import only the constant you use). This avoids surprises from broad static imports.
139-159: Consolidate XMLInputFactory hardening into a single freshFactory()I’ve identified that we have at least nine direct calls to
XMLInputFactory.newInstance()across production code—each re-implementing ad-hoc hardening (coalescing, entity-replacement, external entities, validation, etc.) and only our centralXMLInputFactoryFactorysets a safeXMLResolver. To avoid drift and reduce maintenance burden, let’s:
Add a new helper in
com.predic8.xml.beautifier.XMLInputFactoryFactory:public static XMLInputFactory freshFactory() { XMLInputFactory f = XMLInputFactory.newInstance(); f.setProperty(XMLInputFactory.IS_COALESCING, Boolean.FALSE); f.setProperty(XMLInputFactory.IS_NAMESPACE_AWARE, Boolean.TRUE); f.setProperty(XMLInputFactory.IS_REPLACING_ENTITY_REFERENCES, Boolean.FALSE); f.setProperty(XMLInputFactory.SUPPORT_DTD, Boolean.FALSE); try { f.setProperty(JAVAX_XML_STREAM_IS_SUPPORTING_EXTERNAL_ENTITIES, Boolean.FALSE); } catch (IllegalArgumentException ignored) {} f.setXMLResolver((publicId, systemId, baseURI, namespace) -> new ByteArrayInputStream(new byte[0])); try { f.setProperty(XMLInputFactory.IS_VALIDATING, Boolean.FALSE); } catch (IllegalArgumentException ignored) {} return f; }Update each ad-hoc call to use
freshFactory(), then selectively overrideSUPPORT_DTDwhere needed:• XMLProtector (
core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java):- XMLInputFactory f = XMLInputFactory.newInstance(); + XMLInputFactory f = XMLInputFactoryFactory.freshFactory(); + // enable DTDs only for detection in the StAX loop + f.setProperty(XMLInputFactory.SUPPORT_DTD, Boolean.TRUE);• SOAPUtil (
core/src/main/java/com/predic8/membrane/core/util/SOAPUtil.java)• ContentTypeDetector (
core/src/main/java/com/predic8/membrane/core/util/ContentTypeDetector.java)• XOPReconstitutor (
core/src/main/java/com/predic8/membrane/core/multipart/XOPReconstitutor.java)• XMLBody (
core/src/main/java/com/predic8/membrane/core/http/xml/XMLBody.java)• SoapOperationExtractor (
core/src/main/java/com/predic8/membrane/core/interceptor/soap/SoapOperationExtractor.java)• SchematronValidator (
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/SchematronValidator.java)• MessageAnalyser (
core/src/main/java/com/predic8/membrane/core/interceptor/MessageAnalyser.java)(Optional) Update example code and tests in
distribution/examples/...andcore/src/test/...if you want tests/examples to mirror hardened behavior.This refactor centralizes security hardening, ensures every factory uses the same safe defaults (including an empty‐stream resolver), and makes per-use overrides (e.g., DTD support) explicit and minimal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/interceptor/rewrite/RewriteInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/TextUtil.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-22T15:02:24.785Z
Learnt from: predic8
PR: membrane/api-gateway#2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.785Z
Learning: XMLBeautifier.parse(InputStream) throws Exception, not XMLStreamException specifically, so catching IOException in TextUtil.formatXML is appropriate for handling IO errors during XML parsing.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
📚 Learning: 2025-08-22T15:02:24.785Z
Learnt from: predic8
PR: membrane/api-gateway#2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.785Z
Learning: XMLBeautifier.parse(InputStream) throws IOException specifically, not Exception. It internally wraps any XMLStreamException or other parsing exceptions in an IOException. Therefore, catching IOException in TextUtil.formatXML is the correct approach for handling XML parsing errors from InputStream.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
📚 Learning: 2025-08-22T15:02:24.785Z
Learnt from: predic8
PR: membrane/api-gateway#2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.785Z
Learning: XMLBeautifier.parse(InputStream) and XMLBeautifier.parse(Reader) both declare "throws Exception" in their signatures, not XMLStreamException specifically. Catching IOException is appropriate for handling IO errors during XML parsing with XMLBeautifier.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
🧬 Code graph analysis (2)
core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java (1)
core/src/main/java/com/predic8/xml/beautifier/XMLInputFactoryFactory.java (1)
XMLInputFactoryFactory(24-60)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (1)
core/src/main/java/com/predic8/xml/beautifier/XMLInputFactoryFactory.java (1)
XMLInputFactoryFactory(24-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (5)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (4)
120-129: LGTM: correct exception handling for InputStream variantCatching IOException here matches XMLBeautifier.parse(InputStream)’s contract (it wraps internal parsing errors into IOException). Good consistency with logging + rethrow.
131-133: Formatter selection is clear and cohesiveClean separation of HTML vs XML formatters with sensible defaults (0/4 indent). Nice.
187-207: Robust and hardened XML snippet checkThread-local, hardened XMLInputFactory with entity resolution disabled is a good move. Close in finally is correct.
84-91: camelToKebab: simple and effectiveSolid approach for common camel-case patterns. No action needed.
core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java (1)
100-116: Off‑by‑one fix for attribute counting looks correct.The loop now allows exactly maxAttributeCount attributes and rejects only when the count exceeds the limit. This also correctly enforces the limit when maxAttributeCount == 0.
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (3)
65-74: Treat whitespace-only encodings as blank; trim before resolution (repeat of earlier suggestion).Handles " utf-8 " and " " correctly; also simplifies Optional usage.
Apply this diff:
- public static Charset getCharset(String encoding, Charset defaultCharset) { - if (encoding == null || encoding.isEmpty()) - return defaultCharset; - try { - return Optional.of(encoding).map(Charset::forName).orElse(defaultCharset); - } catch (Exception e) { - log.info("Encoding '{}' caused error: {}, Use default: {}", encoding, e.getMessage(), defaultCharset); - return defaultCharset; - } - } + public static Charset getCharset(String encoding, Charset defaultCharset) { + if (encoding == null) return defaultCharset; + String enc = encoding.trim(); + if (enc.isEmpty()) return defaultCharset; + try { + return Charset.forName(enc); + } catch (Exception e) { + log.info("Encoding '{}' caused error: {}, Use default: {}", enc, e.getMessage(), defaultCharset); + return defaultCharset; + } + }
217-222: Preserve null input; avoid turning null into empty string.Returning "" for null can mask bugs and change API semantics.
Apply this diff:
- public static String removeFinalChar(String s) { - if (s == null || s.isEmpty()) - return ""; - int end = s.offsetByCodePoints(s.length(), -1); - return s.substring(0, end); - } + public static String removeFinalChar(String s) { + if (s == null || s.isEmpty()) return s; + int end = s.offsetByCodePoints(s.length(), -1); + return s.substring(0, end); + }
178-182: capitalizeFirstCharacter uppercases the entire string; limit to the first Unicode code point and add null guard.Current implementation effectively returns english.toUpperCase().
Apply this diff:
- public static String capitalizeFirstCharacter(String english) { - if (english.isEmpty()) - return ""; - return (english.charAt(0) + english.substring(1)).toUpperCase(); - } + public static String capitalizeFirstCharacter(String english) { + if (english == null || english.isEmpty()) return english == null ? null : ""; + int firstCp = english.codePointAt(0); + int firstLen = Character.charCount(firstCp); + String head = new String(Character.toChars(Character.toTitleCase(firstCp))); + String tail = english.substring(firstLen); + return head + tail; + }
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (1)
316-318: Indent calculation counts characters, not columns; mismatches tabs=4 assumption used in trimming.Make getCurrentIndent tab-aware to align with charsToRemoveForColumns.
Apply this diff:
- public static int getCurrentIndent(String line) { - return line.length() - line.replaceFirst("^\\s+", "").length(); - } + public static int getCurrentIndent(String line) { + int cols = 0; + for (int i = 0; i < line.length(); i++) { + char c = line.charAt(i); + if (c == ' ') cols += 1; + else if (c == '\t') cols += 4; + else break; + } + return cols; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/TextUtilTest.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/com/predic8/membrane/core/util/TextUtilTest.java
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-22T15:02:24.785Z
Learnt from: predic8
PR: membrane/api-gateway#2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.785Z
Learning: XMLBeautifier.parse(InputStream) throws Exception, not XMLStreamException specifically, so catching IOException in TextUtil.formatXML is appropriate for handling IO errors during XML parsing.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
📚 Learning: 2025-08-22T15:02:24.785Z
Learnt from: predic8
PR: membrane/api-gateway#2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.785Z
Learning: XMLBeautifier.parse(InputStream) and XMLBeautifier.parse(Reader) both declare "throws Exception" in their signatures, not XMLStreamException specifically. Catching IOException is appropriate for handling IO errors during XML parsing with XMLBeautifier.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
📚 Learning: 2025-08-22T15:02:24.785Z
Learnt from: predic8
PR: membrane/api-gateway#2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.785Z
Learning: XMLBeautifier.parse(InputStream) throws IOException specifically, not Exception. It internally wraps any XMLStreamException or other parsing exceptions in an IOException. Therefore, catching IOException in TextUtil.formatXML is the correct approach for handling XML parsing errors from InputStream.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (1)
core/src/main/java/com/predic8/xml/beautifier/XMLInputFactoryFactory.java (1)
XMLInputFactoryFactory(24-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (5)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (5)
120-129: LGTM: InputStream variant correctly logs and rethrows IOException, without closing caller-owned stream.Matches XMLBeautifier.parse(InputStream) behavior (wraps parsing errors in IOException).
131-133: LGTM: Formatter factory cleanly separates HTML vs XML with explicit indentation.
187-207: LGTM: isValidXMLSnippet now uses hardened, thread-local XMLInputFactory and always closes the parser.Good defensive defaults (no external entities, validation off) and safe close in finally.
231-235: LGTM: Correct regex and bounds-check for line extraction.Using "\r?\n" and guarding the index prevents AIOOBE.
139-147: LGTM: glob-to-regex translation is straightforward and covers common glob metacharacters.Static lookup tables keep the loop simple and fast.
Also applies to: 149-157, 41-44
core/src/main/java/com/predic8/membrane/core/exchangestore/FileExchangeStore.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (4)
67-76: Treat whitespace-only encodings as blank; trim before resolutionCurrent logic treats only empty strings as blank and logs the untrimmed value. Trim first and resolve using the trimmed value. Also,
Optional.of(...).map(...)is unnecessary here.Apply this diff:
- public static Charset getCharset(String encoding, Charset defaultCharset) { - if (encoding == null || encoding.isEmpty()) - return defaultCharset; - try { - return Optional.of(encoding).map(Charset::forName).orElse(defaultCharset); - } catch (Exception e) { - log.info("Encoding '{}' caused error: {}, Use default: {}", encoding, e.getMessage(), defaultCharset); - return defaultCharset; - } - } + public static Charset getCharset(String encoding, Charset defaultCharset) { + if (encoding == null) return defaultCharset; + String enc = encoding.trim(); + if (enc.isEmpty()) return defaultCharset; + try { + return Charset.forName(enc); + } catch (Exception e) { + log.info("Encoding '{}' caused error: {}, Use default: {}", enc, e.getMessage(), defaultCharset); + return defaultCharset; + } + }
219-224: Preserve null semantics and avoid surprising empty-string substitutionFor
removeFinalChar, returning""whensisnullhides the nullness and may cause subtle NPEs later. Returnsunchanged fornull/empty and keep the code point–aware removal.Apply this diff:
- public static String removeFinalChar(String s) { - if (s == null || s.isEmpty()) - return ""; - int end = s.offsetByCodePoints(s.length(), -1); - return s.substring(0, end); - } + public static String removeFinalChar(String s) { + if (s == null || s.isEmpty()) return s; + int end = s.offsetByCodePoints(s.length(), -1); + return s.substring(0, end); + }
111-120: Catch the correct exception type for Reader-based parsing
XMLBeautifier.parse(Reader)declaresthrows Exception(per current implementation), notXMLStreamExceptionspecifically. Restricting the catch toXMLStreamExceptionbypasses logging for other failures. CatchExceptioninstead, log, and rethrow.Apply this diff:
public static String formatXML(Reader reader, boolean asHTML) throws Exception { try { StringWriter out = new StringWriter(STRING_BUFFER_INITIAL_CAPACITY_FOR_XML); new XMLBeautifier(getXmlBeautifierFormatter(asHTML, out)).parse(reader); return out.toString(); - } catch (XMLStreamException e) { + } catch (Exception e) { log.info("Error parsing XML: {}", e.getMessage()); throw e; } }
180-184: capitalizeFirstCharacter uppercases entire string; make it first-letter only and Unicode-awareThe method currently returns
english.toUpperCase()effectively. Capitalize only the first Unicode code point; add a null guard.Apply this diff:
- public static String capitalizeFirstCharacter(String english) { - if (english.isEmpty()) - return ""; - return (english.charAt(0) + english.substring(1)).toUpperCase(); - } + public static String capitalizeFirstCharacter(String english) { + if (english == null || english.isEmpty()) return english == null ? null : ""; + int firstCp = english.codePointAt(0); + int firstLen = Character.charCount(firstCp); + String head = new String(Character.toChars(Character.toTitleCase(firstCp))); + String tail = english.substring(firstLen); + return head + tail; + }
🧹 Nitpick comments (10)
core/src/test/java/com/predic8/membrane/core/util/URITest.java (1)
27-43: Avoid shadowing JUnit’s assertSame; consider a more specific helper nameUsing a helper named assertSame can be confused with Assertions.assertSame. Rename the helper to avoid ambiguity and improve readability.
Apply these diffs within the changed regions:
@@ - assertSame("http://predic8.de/?a=query"); - assertSame("http://predic8.de/#foo"); - assertSame("http://predic8.de/path/file"); - assertSame("http://predic8.de/path/file?a=query"); - assertSame("http://predic8.de/path/file#foo"); - assertSame("http://predic8.de/path/file?a=query#foo"); - assertSame("http://foo:bar@predic8.de/path/file?a=query#foo"); - assertSame("//predic8.de/path/file?a=query#foo"); - assertSame("/path/file?a=query#foo"); - assertSame("scheme:/path/file?a=query#foo"); - assertSame("path/file?a=query#foo"); - assertSame("scheme:path/file?a=query#foo", true); // considered 'opaque' by java.net.URI - we don't support that - assertSame("file?a=query#foo", true); // opaque - assertSame("scheme:file?a=query#foo", true); // opaque - assertSame("?a=query#foo"); - assertSame("scheme:?a=query#foo", true); // opaque + assertUrisEqual("http://predic8.de/?a=query"); + assertUrisEqual("http://predic8.de/#foo"); + assertUrisEqual("http://predic8.de/path/file"); + assertUrisEqual("http://predic8.de/path/file?a=query"); + assertUrisEqual("http://predic8.de/path/file#foo"); + assertUrisEqual("http://predic8.de/path/file?a=query#foo"); + assertUrisEqual("http://foo:bar@predic8.de/path/file?a=query#foo"); + assertUrisEqual("//predic8.de/path/file?a=query#foo"); + assertUrisEqual("/path/file?a=query#foo"); + assertUrisEqual("scheme:/path/file?a=query#foo"); + assertUrisEqual("path/file?a=query#foo"); + assertUrisEqual("scheme:path/file?a=query#foo", true); // considered 'opaque' by java.net.URI - we don't support that + assertUrisEqual("file?a=query#foo", true); // opaque + assertUrisEqual("scheme:file?a=query#foo", true); // opaque + assertUrisEqual("?a=query#foo"); + assertUrisEqual("scheme:?a=query#foo", true); // opaque@@ - private void assertSame(String uri) { - assertSame(uri, false); - } + private void assertUrisEqual(String uri) { + assertUrisEqual(uri, false); + } @@ - private void assertSame(String uri, boolean mayDiffer) { + private void assertUrisEqual(String uri, boolean mayDiffer) { try { URI u1 = new URI(uri, false); URI u2 = new URI(uri, true); if (!mayDiffer) { assertEquals(u1.getPath(), u2.getPath()); assertEquals(u1.getQuery(), u2.getQuery()); assertEquals(u1.getRawQuery(), u2.getRawQuery()); assertEquals(u1.getFragment(), u2.getFragment()); } assertEquals(u1.toString(), u2.toString()); } catch (URISyntaxException e) { throw new RuntimeException(e); } }Also applies to: 127-146
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (2)
249-252: Use an explicit escaped regex for line splitting (and consider CR-only)
split("\r?\n")embeds control characters. While it works,split("\\r?\\n")is clearer and consistent with other methods. You might also handle legacy CR-only with\\r?\\n|\\r.Apply this diff:
- String[] lines = multilineString.split("\r?\n"); + String[] lines = multilineString.split("\\r?\\n|\\r");
261-274: Indent calculation mixes “columns” (tabs=4) with char-count leading whitespace
charsToRemoveForColumnstreats a tab as 4 columns, butgetCurrentIndentcounts raw characters (tabs=1). As a result,minIndent/curIndentcan be inconsistent on mixed tab/space input and remove too little/much.Consider computing indentation in “columns” consistently:
- Compute per-line indent columns using the same logic as
charsToRemoveForColumns(scan leading whitespace accumulating 1 for space and 4 for tab).- Use that value for both
getMinIndentandtrimLinescomparisons.I can provide a concrete patch if you want this standardized across the helpers.
Also applies to: 318-321, 328-339
core/src/main/java/com/predic8/membrane/core/interceptor/administration/AdminRESTInterceptor.java (1)
181-185: Verify Response.body(InputStream, boolean) auto-close semanticsYou pass
falsetobody(InputStream, false). If that flag means “the framework will NOT close the stream,” this can leak resources. If it means “do not buffer; stream to client and close when done,” then it’s fine. Please confirm the contract and adjust if needed (e.g., passtrueto auto-close or wrap in a resource-managing wrapper).If the flag indeed controls auto-close, apply:
- ResponseBuilder rb = ok().contentType(ct).body(msg.getBodyAsStream(), false); + ResponseBuilder rb = ok().contentType(ct).body(msg.getBodyAsStream(), true);Otherwise, keep as-is but ensure the response-writing layer reliably closes the stream on completion/error.
docs/ROADMAP.md (6)
39-39: Normalize status/tone and past-tense for shipped work (UTF‑8 encoding).This looks completed in this PR. Use past tense and remove “(TB)”.
- - Convert to UTF-8 source and outputEncoding to UTF-8 (TB) + - Converted source and default outputEncoding to UTF-8. (Done)
40-40: Mark TemplateInterceptor refactor as done and use consistent casing.The refactor is part of this PR; reflect that here and prefer sentence case.
- - TemplateInterceptor Refactoring (TB) + - TemplateInterceptor refactoring. (Done)
43-43: Clarify terminology: Prettifier vs. Beautifier, and make the action explicit.This PR introduces a Prettifier abstraction and also reuses Beautifier components. Make the item concrete and status-aligned.
- - Refactor: Beautifier to use the Code from above + - Unified Prettifier/Beautifier pipeline across JSON/XML/Text; reused shared formatting code. (Done)If you prefer to keep “Beautifier” only, please align class names in the doc with the public types introduced by this PR (Prettifier, JSONPrettifier, XMLPrettifier, TextPrettifier).
44-44: Expand “TP/TB” abbreviations and use an action verb.“TP” is unclear to readers. Use “Planned/Done” or omit status, and use an imperative/past-tense phrasing.
- - Describe RPM Setup (TP) + - Document RPM setup. (Planned)
45-45: Make the example entry descriptive and apply consistent status notation.Add a short description and normalize status terms.
- - examples/routing-traffic/outgoing-api-gateway (TB) + - Add example “routing-traffic/outgoing-api-gateway”. (Planned)
39-46: Optional: establish a status legend and consistent style across roadmap sections.Readers won’t know what “TB/TP” mean. Consider adding a short legend at the top and using a consistent tense and punctuation throughout the document.
For example, add near the top of the file:
- Status legend: Planned, In Progress, Done.
- Conventions: Use sentence case; past tense for shipped items under released versions; avoid abbreviations like TB/TP; one sentence per bullet; end with a period.
I can generate a concise pass to harmonize phrasing across 6.3.0–7.0.0 if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
core/src/main/java/com/predic8/membrane/core/http/Message.java(3 hunks)core/src/main/java/com/predic8/membrane/core/http/Request.java(4 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/administration/AdminRESTInterceptor.java(5 hunks)core/src/main/java/com/predic8/membrane/core/util/TextUtil.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/URI.java(3 hunks)core/src/test/java/com/predic8/membrane/core/util/URITest.java(2 hunks)docs/ROADMAP.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/main/java/com/predic8/membrane/core/util/URI.java
- core/src/main/java/com/predic8/membrane/core/http/Message.java
- core/src/main/java/com/predic8/membrane/core/http/Request.java
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-22T15:02:24.785Z
Learnt from: predic8
PR: membrane/api-gateway#2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.785Z
Learning: XMLBeautifier.parse(InputStream) throws Exception, not XMLStreamException specifically, so catching IOException in TextUtil.formatXML is appropriate for handling IO errors during XML parsing.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
📚 Learning: 2025-08-22T15:02:24.785Z
Learnt from: predic8
PR: membrane/api-gateway#2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.785Z
Learning: XMLBeautifier.parse(InputStream) and XMLBeautifier.parse(Reader) both declare "throws Exception" in their signatures, not XMLStreamException specifically. Catching IOException is appropriate for handling IO errors during XML parsing with XMLBeautifier.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
📚 Learning: 2025-08-22T15:02:24.785Z
Learnt from: predic8
PR: membrane/api-gateway#2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.785Z
Learning: XMLBeautifier.parse(InputStream) throws IOException specifically, not Exception. It internally wraps any XMLStreamException or other parsing exceptions in an IOException. Therefore, catching IOException in TextUtil.formatXML is the correct approach for handling XML parsing errors from InputStream.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
📚 Learning: 2025-08-24T17:55:06.080Z
Learnt from: rrayst
PR: membrane/api-gateway#2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:178-183
Timestamp: 2025-08-24T17:55:06.080Z
Learning: The capitalizeFirstCharacter method in TextUtil.java incorrectly uppercases the entire string instead of just the first character due to calling .toUpperCase() on the concatenated result. The method should only capitalize the first Unicode code point and leave the rest unchanged.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (1)
core/src/main/java/com/predic8/xml/beautifier/XMLInputFactoryFactory.java (1)
XMLInputFactoryFactory(24-60)
🪛 LanguageTool
docs/ROADMAP.md
[grammar] ~39-~39: There might be a mistake here.
Context: ... source and outputEncoding to UTF-8 (TB) - TemplateInterceptor Refactoring (TB) - T...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...) - TemplateInterceptor Refactoring (TB) - Template/Static Interceptor: Pretty for ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ... Pretty for text/* (Refactor first) (TB) - Pretty on text should trim whitespace in...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...ode from above - Describe RPM Setup (TP) - examples/routing-traffic/outgoing-api-ga...
(QB_NEW_EN)
[grammar] ~46-~46: There might be a mistake here.
Context: ...ook Book: outgoing-api-gateway (TB) done - READMEs in example folders listing the e...
(QB_NEW_EN)
🔇 Additional comments (6)
core/src/test/java/com/predic8/membrane/core/util/URITest.java (3)
45-65: Encoding and illegal-character coverage looks solidGood variety of encoded vs raw umlauts and illegal character placements. The contrast between strict and custom parsing paths is clear.
67-125: Scheme/host/port assertions are precise and mirror java.net.URI behavior where intended
- getScheme/getHost/getPort parity checks between custom=false/true are clear.
- urlStandardBehaviour correctly documents java.net.URI returning -1 for default ports.
148-163: Negative parsing tests read wellThe strict-vs-custom expectations are explicit, and the additional u.getRawQuery() call ensures the raw accessor is safe to invoke post-parse.
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (1)
122-131: LGTM: InputStream overload uses correct exception and does not close caller-owned streamCatching
IOExceptionaligns withXMLBeautifier.parse(InputStream)wrapping and throwing IO-related errors, and you correctly avoid closing the caller-provided stream here.core/src/main/java/com/predic8/membrane/core/interceptor/administration/AdminRESTInterceptor.java (2)
147-150: LGTM: Return 204 when message is absentUsing
noContent()for missing payloads is a clean, semantically correct response.
201-203: LGTM: Consistent 204 for missing headers
noContent()for absent message headers keeps the API consistent across endpoints.
...src/main/java/com/predic8/membrane/core/interceptor/administration/AdminRESTInterceptor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java (1)
26-29: Missing configuration files in the custom-error-messages exampleI’ve confirmed that the override in CustomErrorHandlingExampleTest points at
distribution/examples/extending-membrane/error-handling/custom-error-messages/, and that this directory exists. However, it contains no typical configuration files (e.g., proxies.xml, any *.xml, *.yml, or *.yaml), which the base test class (AbstractSampleMembraneStartStopTestcase) will attempt to load on startup. Without these, the example won’t initialize correctly.Please add the required config artifacts under that directory, for example:
proxies.xml(or another rules file)- Any supporting XML/YAML rule definitions referenced by the example
Once those files are in place, rerun the test to ensure the example starts up as expected.
♻️ Duplicate comments (3)
core/src/main/java/com/predic8/membrane/core/exchangestore/FileExchangeStore.java (1)
173-190: Close InputStreams returned by body.getContentAsStream() to prevent leaks.formatXML(InputStream, ...) intentionally does not close the stream; all branches must manage it. Wrap each use in try-with-resources.
@@ - if (raw) { - IOUtils.copy(body.getContentAsStream(), os); - } else { - if (msg.isXML()) { - os.write(formatXML(body.getContentAsStream(), /*asHTML*/ false).getBytes(UTF_8)); - return; - } - IOUtils.copy(body.getContentAsStream(), os); - } + if (raw) { + try (InputStream in = body.getContentAsStream()) { + IOUtils.copy(in, os); + } + } else { + if (msg.isXML()) { + try (InputStream in = body.getContentAsStream()) { + os.write(formatXML(in, /*asHTML*/ false).getBytes(UTF_8)); + } + return; // os is closed by outer TWR + } + try (InputStream in = body.getContentAsStream()) { + IOUtils.copy(in, os); + } + }Run to locate other sites that may need the same fix:
#!/bin/bash # Find all usages of getContentAsStream() with context rg -nC3 --type java '\bgetContentAsStream\s*\(' # Heuristic: show uses not obviously wrapped in try-with-resources on the same line rg -nP --type java '(?<!try\s*\()\s*body\.getContentAsStream\s*\(' -g '!**/target/**'core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java (2)
92-124: Always close the parser and clear the ThreadLocal (prevents leaks on pooled threads).Early returns bypass cleanup. Move resource cleanup to finally and call
xmlInputFactoryFactory.remove(). This was raised earlier and still applies.Apply:
- public boolean protect(InputStreamReader isr) throws XMLProtectionException { - try { - XMLEventReader parser = xmlInputFactoryFactory.get().createXMLEventReader(isr); + public boolean protect(InputStreamReader isr) throws XMLProtectionException { + XMLEventReader parser = null; + try { + parser = xmlInputFactoryFactory.get().createXMLEventReader(isr); while (parser.hasNext()) { XMLEvent event = parser.nextEvent(); if (event.isStartElement()) { StartElement startElement = event.asStartElement(); if (!checkElementNameLength(startElement)) return false; if (!checkNumberOfAttributes(startElement)) return false; } if (event instanceof javax.xml.stream.events.DTD dtd) { checkExternalEntities(dtd); if (removeDTD) { log.debug("removed DTD."); continue; } } writer.add(event); } writer.flush(); } catch (XMLStreamException e) { Location loc = e.getLocation(); log.warn("Received not well-formed XML at line {}, column {}: {}", loc != null ? loc.getLineNumber() : -1, loc != null ? loc.getColumnNumber() : -1, e.getMessage()); return false; - } + } finally { + if (parser != null) { + try { parser.close(); } catch (XMLStreamException ignore) {} + } + // Avoid leaking factory instances across tasks on container thread pools. + xmlInputFactoryFactory.remove(); + } return true; }
149-169: Add a no-op XMLResolver to prevent pre-check external entity resolution (TOCTOU).Some StAX impls may resolve the external subset before emitting the DTD event despite
JAVAX_XML_STREAM_IS_SUPPORTING_EXTERNAL_ENTITIES=FALSE. Set a no-op resolver to guarantee no fetch occurs. Previously flagged; still missing here.Apply:
private XMLInputFactory getXmlInputFactory() { // Return a new, fully hardened XMLInputFactory instance. That is not shared XMLInputFactory f = XMLInputFactory.newInstance(); // hardening f.setProperty(IS_COALESCING, FALSE); // Support DTDs on purpose to detect them in the StAX loop! f.setProperty(SUPPORT_DTD, TRUE); f.setProperty(IS_NAMESPACE_AWARE, TRUE); f.setProperty(IS_REPLACING_ENTITY_REFERENCES, FALSE); try { f.setProperty(JAVAX_XML_STREAM_IS_SUPPORTING_EXTERNAL_ENTITIES, FALSE); } catch (IllegalArgumentException ignored) { } + // Prevent any external entity resolution regardless of implementation quirks. + f.setXMLResolver((publicId, systemId, baseURI, namespace) -> new ByteArrayInputStream(new byte[0])); try { f.setProperty(IS_VALIDATING, FALSE); } catch (IllegalArgumentException ignored) { } return f; }
🧹 Nitpick comments (26)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/MassivelyParallelTest.java (2)
73-73: Consider setting an explicit Content-Type for deterministic responses.
ok().body(...)does not set a Content-Type. For better interoperability (and to avoid client assumptions), set it explicitly to text/plain UTF‑8.Apply this diff:
- exc.setResponse(ok().body(exc.getRequest().getUri()).build()); + exc.setResponse(ok() + .contentType("text/plain; charset=UTF-8") + .body(exc.getRequest().getUri()) + .build());
90-90: Optional: pre-size the concurrent set to reduce resizing churn.You expect ~1000 entries; pre-sizing avoids internal growth steps. Low impact, but cheap to do.
- Set<String> paths = newKeySet(); + Set<String> paths = newKeySet(1024);distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java (5)
16-23: Avoid wildcard imports and standardize Content-Type sources
- Wildcard imports make symbol provenance ambiguous and increase collision risk (you’re importing both MimeType.* strings and RestAssured ContentType.* enums).
- In this file you mix string-based APPLICATION_XML (from MimeType) with enum-based XML (from RestAssured). Pick one approach consistently; enum-based is usually clearer in RestAssured tests.
Suggested minimal cleanup:
-import com.predic8.membrane.examples.util.*; -import org.junit.jupiter.api.*; - -import static com.predic8.membrane.core.http.MimeType.*; -import static io.restassured.RestAssured.*; -import static io.restassured.http.ContentType.*; -import static org.hamcrest.CoreMatchers.*; +import com.predic8.membrane.examples.util.AbstractSampleMembraneStartStopTestcase; +import org.junit.jupiter.api.Test; + +import static io.restassured.RestAssured.given; +import static io.restassured.http.ContentType.XML; +import static org.hamcrest.CoreMatchers.containsString;This pairs well with the content-type change proposed below on Line 36. If the repo’s style explicitly prefers wildcards, feel free to ignore the first two lines above and keep only the static imports narrowed.
36-36: Make Content-Type usage consistent (use RestAssured’s enum)Use RestAssured’s ContentType.XML (enum) rather than a string literal from MimeType for consistency with the rest of the class.
- .contentType(APPLICATION_XML) + .contentType(XML)
31-31: Fix formatter pragma mismatchYou use “@Formatter:off” at Line 31 but “formatter:on” at Line 137. IntelliJ recognizes “@Formatter:on”. Minor, but keeps IDE formatting predictable.
- // formatter:on + // @formatter:onAlso applies to: 137-137
42-46: Consider asserting response Content-Type alongside bodySince these tests validate error handling and prettified responses, asserting the response media type will catch regressions in the prettifier/templating chain.
For example:
.then() .statusCode(500) + .header("Content-Type", containsString("xml")) .body( containsString("<case>a</case>"), containsString("XML Protection: Invalid XML!") );Repeat similarly in other cases where an XML body is expected.
40-41: Optional: deduplicate base URL usageThe service URL repeats in each test. If the base class doesn’t already set RestAssured defaults, consider centralizing:
- Set RestAssured.baseURI and port in a @BeforeAll (or the base class), and then use relative paths (e.g., .post("/service")).
- Alternatively, introduce a private static final String SERVICE = "http://localhost:2000/service".
This reduces repetition and eases port changes in the future.
Also applies to: 56-56, 70-71, 84-85, 106-107, 128-129
core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptor.java (2)
64-65: Nit: add a space after the comma in the debug callMinor formatting to match SLF4J style used elsewhere.
- log.debug("Inspecting XML of content type: {}",exc.getRequest().getHeader().getContentType()); + log.debug("Inspecting XML of content type: {}", exc.getRequest().getHeader().getContentType());
71-79: Consider adding X-Protection response header and 415 status for non-XML requestsYou already set X-Protection on the XML-violation path. For consistency and easier troubleshooting on clients, set it also on the non-XML path. Additionally, a 415 Unsupported Media Type would better communicate the reason than a generic error (if ProblemDetails builder allows it).
if (!exc.getRequest().isXML()) { String msg = "Content-Type %s is not XML.".formatted(exc.getRequest().getHeader().getContentType()); log.warn(msg); user(router.isProduction(), getDisplayName()) .title("Request discarded by xmlProtection") .detail(msg) .buildAndSetResponse(exc); + // Mirror the X-Protection header used in the XML-violation path for consistency. + exc.getResponse().getHeader().add(X_PROTECTION, "Unsupported media type: request is not XML"); return ABORT; }If there’s a ProblemDetails helper for 415, consider switching to it; otherwise ignore.
core/src/main/java/com/predic8/membrane/core/exchangestore/FileExchangeStore.java (4)
146-156: Use zero-padded month/day to stabilize directory ordering.Improves lexicographic sort and consistency.
@@ - buf.append((time.get(Calendar.MONTH) + 1)); + buf.append(String.format("%02d", (time.get(Calendar.MONTH) + 1))); @@ - buf.append(time.get(DAY_OF_MONTH)); + buf.append(String.format("%02d", time.get(DAY_OF_MONTH)));
49-50: Prefer ThreadLocal.withInitial for simplicity.Removes branching in getDateFormat().
@@ - private static final ThreadLocal<DateFormat> dateFormat = new ThreadLocal<>(); + private static final ThreadLocal<DateFormat> dateFormat = + ThreadLocal.withInitial(() -> new SimpleDateFormat(DATE_FORMAT));Then simplify getDateFormat() to just return dateFormat.get(), or inline uses.
81-95: Optional: keep 03:14 local wall-clock time across DST.scheduleAtFixedRate(24h) will shift relative to wall-clock on DST changes. If you need “always 03:14 local time,” compute the next execution in each run and reschedule instead of using a fixed period. Otherwise, ignore.
100-103: Track TODOs for multi-snap and header snapshotting.Convert these TODOs to issues or to @SuppressWarnings with a rationale; otherwise they tend to linger.
Do you want me to open an issue with acceptance criteria for multi-snap support and header snapshot timing?
core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
52-65: Clarify iterator consumption, add null-check, and tighten API with @NotNull.The method works but silently consumes the iterator and NPEs on null. Make this explicit and fail fast.
Apply:
- /** - * @param it Iterator - * @return Number of items that Iterator provides - */ - public static int count(Iterator<?> it) { - int cnt = 0; - while (it.hasNext()) { - it.next(); - cnt++; - } - return cnt; - } + /** + * Counts the remaining elements in the given iterator. + * Note: This method consumes the iterator to its end. + * + * @param iterator the iterator to traverse (must not be null) + * @return number of remaining elements + * @throws NullPointerException if {@code iterator} is null + */ + public static int count(@NotNull Iterator<?> iterator) { + Objects.requireNonNull(iterator, "iterator"); + int count = 0; + while (iterator.hasNext()) { + iterator.next(); + count++; + } + return count; + }core/src/test/java/com/predic8/membrane/core/util/CollectionsUtilTest.java (1)
51-55: Solid coverage; add assertions for iterator exhaustion and null path (if you add the null-check).Two quick additions make behavior explicit and guard the null path.
Apply:
@Test void count_iterator() { assertEquals(0, count(emptyList().iterator())); assertEquals(3, count(List.of("a", "b", "c").iterator())); } + + @Test + void count_consumes_iterator() { + Iterator<String> it = List.of("x", "y").iterator(); + assertEquals(2, count(it)); + assertFalse(it.hasNext(), "Iterator should be exhausted after count()"); + } + + @Test + void count_null_iterator_throws() { + assertThrows(NullPointerException.class, () -> count((Iterator<?>) null)); + }core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java (5)
25-28: Prefer narrow static imports to avoid namespace pollution and shadowing.Only
countand the external-entities property are used; wildcard imports make the code harder to scan alongside a field namedxmlInputFactoryFactory.-import static com.predic8.membrane.core.util.CollectionsUtil.*; -import static com.predic8.xml.beautifier.XMLInputFactoryFactory.*; +import static com.predic8.membrane.core.util.CollectionsUtil.count; +import static com.predic8.xml.beautifier.XMLInputFactoryFactory.JAVAX_XML_STREAM_IS_SUPPORTING_EXTERNAL_ENTITIES;
55-63: ThreadLocal naming and lifecycle.The field name
xmlInputFactoryFactoryis easy to confuse with the classXMLInputFactoryFactory(also statically imported). Consider a clearer name likethreadLocalFactory. More importantly, ensureremove()is called after use to prevent thread-local retention on pooled threads (see change proposed in protect()).
126-137: Fix log placeholders and avoid O(n) counting when early exit is possible.The warning logs
maxAttributeCounttwice; it should log the actual count. Also, iterating up to the limit and bailing early prevents traversing all attributes unnecessarily.Apply:
- private boolean checkNumberOfAttributes(StartElement startElement) { + private boolean checkNumberOfAttributes(StartElement startElement) { if (maxAttributeCount < 0) return true; - var numberOfAttributes = count(startElement.getAttributes()); - if (numberOfAttributes > maxAttributeCount) { - log.warn("Element {} has {} attributes. Exceeding limit of {}", startElement.getName(), maxAttributeCount, maxAttributeCount); - return false; - } - - return true; + var attrs = startElement.getAttributes(); + int numberOfAttributes = 0; + while (attrs.hasNext()) { + attrs.next(); + numberOfAttributes++; + if (numberOfAttributes > maxAttributeCount) { + log.warn("Element {} has {} attributes. Exceeding limit of {}", + startElement.getName(), numberOfAttributes, maxAttributeCount); + return false; + } + } + return true; }
139-147: Typo: elementLenght → elementLength; enrich log with element name.Minor readability improvement; message becomes more actionable.
- private boolean checkElementNameLength(StartElement startElement) { - var elementLenght = startElement.getName().getLocalPart().length(); - if (maxElementNameLength != -1 && - elementLenght > maxElementNameLength) { - log.warn("Element with {} characters exceeds limit of {}", elementLenght, maxElementNameLength); - return false; - } - return true; - } + private boolean checkElementNameLength(StartElement startElement) { + var elementLength = startElement.getName().getLocalPart().length(); + if (maxElementNameLength != -1 && + elementLength > maxElementNameLength) { + log.warn("Element '{}' has {} characters and exceeds limit of {}", + startElement.getName(), elementLength, maxElementNameLength); + return false; + } + return true; + }
149-169: Consider centralizing factory hardening to avoid drift across call sites.You already have XMLInputFactoryFactory with a hardened baseline. Either:
- add and use a
freshFactory()there that returns a new fully hardened instance (no-op resolver included), then setSUPPORT_DTDas needed here; or- reuse
XMLInputFactoryFactory.inputFactory()and explicitly set/resetSUPPORT_DTDper call (ensuring no sticky state).This reduces duplication and prevents future divergence of security hardening.
core/src/main/java/com/predic8/membrane/core/interceptor/balancer/faultmonitoring/FaultMonitoringStrategy.java (1)
113-116: Misnamed time fields (…Seconds) actually hold milliseconds.Both clearFaultyProfilesByTimerAfterLastFailureSeconds and clearFaultyTimerIntervalSeconds are expressed in milliseconds. Rename or adjust Javadoc to prevent confusion. Low priority since it’s pre-existing.
core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java (5)
124-129: Teardown order is correct.Shutting down node routers first, then the balancer, prevents dangling connections. Consider null checks if future tests vary setup, but fine as-is.
171-178: Fix assertion message mismatch and grammar; keep the thresholds.Message says “less then 900” while the threshold is 700; also “then” -> “than”. Keep thresholds but correct the messages for clarity.
Apply this diff:
- assertTrue(ctx.successCounter.get() >= 700, "ctx.successCounter.get() is %d, less then 900".formatted(ctx.successCounter.get())); - assertTrue(ctx.successCounter.get() < 1000, "ctx.successCounter.get() is %d, greater then 1000".formatted(ctx.successCounter.get())); + assertTrue(ctx.successCounter.get() >= 700, + "ctx.successCounter.get() is %d, less than 700".formatted(ctx.successCounter.get())); + assertTrue(ctx.successCounter.get() < 1000, + "ctx.successCounter.get() is %d, greater than or equal to 1000".formatted(ctx.successCounter.get()));Also applies to: 181-183
400-406: Replace println with logging; keep result handling consistent.Use the class logger for non-200 statuses to keep output uniform.
Apply this diff:
- var statusCode = client.executeMethod(getPostMethod()); - if (statusCode == 200) { + var statusCode = client.executeMethod(getPostMethod()); + if (statusCode == 200) { ctx.successCounter.incrementAndGet(); } else { - System.out.println("Status code: " + statusCode); + log.debug("Non-200 response: {}", statusCode); ctx.exceptionCounter.incrementAndGet(); }
418-426: Remove unused totalInterceptorCount or assert on it.totalInterceptorCount is computed but unused. Either remove it or assert a minimal bound (e.g., >= numRequests) if it’s meaningful for the test.
Apply this diff to remove the dead code:
- var totalInterceptorCount = randomFailingInterceptors.stream() - .mapToLong(RandomlyFailingDummyWebServiceInterceptor::getCount) - .sum(); - assertEquals(ctx.numRequests, ctx.exceptionCounter.get() + ctx.successCounter.get(), "Total = success + exception counts");Alternatively, if you want a sanity check on retries:
// Optional: due to retries, totalInterceptorCount should be >= numRequests. assertTrue(totalInterceptorCount >= ctx.numRequests, "Expected at least one server invocation per request");
118-121: Portability: replace List.getLast() in enableFailOverOn5XX if targeting < Java 21.If the build targets Java 17/11, getLast() won’t compile. Replace with index-based access:
List<Interceptor> l = balancer.getTransport().getInterceptors(); ((HTTPClientInterceptor) l.get(l.size() - 1)).setFailOverOn5XX(true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
core/src/main/java/com/predic8/membrane/core/exchangestore/FileExchangeStore.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/balancer/faultmonitoring/FaultMonitoringStrategy.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java(1 hunks)core/src/main/java/com/predic8/membrane/core/transport/http/MessageTracer.java(1 hunks)core/src/main/java/com/predic8/membrane/core/transport/http/client/RetryHandler.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/TextUtil.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/URI.java(3 hunks)core/src/test/java/com/predic8/membrane/core/UnitTests.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptorTest.java(3 hunks)core/src/test/java/com/predic8/membrane/core/services/RandomlyFailingDummyWebServiceInterceptor.java(2 hunks)core/src/test/java/com/predic8/membrane/core/util/CollectionsUtilTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/util/URITest.java(2 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/MassivelyParallelTest.java(3 hunks)core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java(12 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- core/src/main/java/com/predic8/membrane/core/transport/http/client/RetryHandler.java
- core/src/main/java/com/predic8/membrane/core/transport/http/MessageTracer.java
🚧 Files skipped from review as they are similar to previous changes (5)
- core/src/main/java/com/predic8/membrane/core/util/URI.java
- core/src/main/java/com/predic8/membrane/core/util/TextUtil.java
- core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptorTest.java
- core/src/test/java/com/predic8/membrane/core/UnitTests.java
- core/src/test/java/com/predic8/membrane/core/util/URITest.java
🧰 Additional context used
🧬 Code graph analysis (7)
core/src/test/java/com/predic8/membrane/core/util/CollectionsUtilTest.java (1)
core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
CollectionsUtil(23-66)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/MassivelyParallelTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/Response.java (1)
Response(38-500)
core/src/test/java/com/predic8/membrane/core/services/RandomlyFailingDummyWebServiceInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java (2)
core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
CollectionsUtil(23-66)core/src/main/java/com/predic8/xml/beautifier/XMLInputFactoryFactory.java (1)
XMLInputFactoryFactory(24-60)
core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java (3)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)core/src/main/java/com/predic8/membrane/core/interceptor/balancer/BalancerUtil.java (1)
BalancerUtil(23-131)core/src/test/java/com/predic8/membrane/core/services/RandomlyFailingDummyWebServiceInterceptor.java (1)
RandomlyFailingDummyWebServiceInterceptor(30-70)
core/src/main/java/com/predic8/membrane/core/exchangestore/FileExchangeStore.java (1)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (1)
TextUtil(33-340)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Automated tests
🔇 Additional comments (20)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/MassivelyParallelTest.java (2)
30-30: Static import for Response helpers is fine and consistent.Using
import static ...Response.*;keeps the fluent builder readable (ok().body(...).build()) and matches the style elsewhere in the file.
33-33: Static import of ConcurrentHashMap utilities looks good.Brings
newKeySet()into scope cleanly for the concurrent set used in the test.core/src/test/java/com/predic8/membrane/core/services/RandomlyFailingDummyWebServiceInterceptor.java (5)
16-18: Imports switch to wildcards is acceptable hereGiven this is test code and the symbols used are clear (Exchange, AbstractInterceptor, Logger/Factory), this change improves brevity without harming readability. No action needed.
20-21: Concurrency imports via wildcards look fineOnly ThreadLocalRandom and AtomicLong are used; wildcard keeps the block compact in test scope. No action needed.
23-25: Static imports improve readabilityUsing MimeType and Response static members reduces verbosity and matches patterns elsewhere in the PR. The static Outcome import also reads clearly.
52-52: Good: parameterized loggingReplaces string concatenation with template logging; avoids unnecessary allocations when debug is disabled.
63-63: Static import of RETURN reads clearlyKeeps the control flow concise; no functional change.
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java (1)
58-58: Double-check intended HTTP status codes for error scenariosThe tests for caseB and caseE expect 200 even though they validate error conditions. That might be intentional (SOAP faults in-body, or upstream validation reported in-band), but it’s worth reconfirming to avoid masking real server error conditions in future changes. If 200 is the intended contract, consider a short comment explaining why 200 is expected.
Also applies to: 108-108, 130-130
core/src/main/java/com/predic8/membrane/core/exchangestore/FileExchangeStore.java (1)
137-139: I've set up scripts to confirm the file’s existence, locate the catch block, and inspect howexcandexc.getRequest()are used in context. Once you provide the output, we can determine if the suggested NPE-safe logging fallback is necessary or ifgetRequest()is guaranteed non-null.core/src/test/java/com/predic8/membrane/core/util/CollectionsUtilTest.java (1)
20-21: LGTM: focused static imports improve readability.core/src/main/java/com/predic8/membrane/core/interceptor/balancer/faultmonitoring/FaultMonitoringStrategy.java (2)
165-170: Control-flow tweak is fine; no semantic change.Splitting the size==1 branch into a separate if after the empty-check keeps behavior identical and reads clearer. No concerns here.
168-169: Ignore thegetFirst()compatibility concern—this project targets Java 21
The rootpom.xmlexplicitly sets<javac.source>21</javac.source> <javac.target>21</javac.target>and configures the Maven Compiler Plugin accordingly, so
List.getFirst()(a Java 21 API) is fully supported.
– No change needed inFaultMonitoringStrategy.javaor elsewhere forgetFirst()
– The original suggestion to revert toget(0)is unnecessary and can be discarded.Likely an incorrect or invalid review comment.
core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java (8)
54-54: Reducing test class visibility is fine.Package-private scope is appropriate for JUnit 5 tests; keeps API surface minimal.
61-64: Node-centric setup is a good simplification.Storing Routers in nodes and keeping RandomlyFailingDummyWebServiceInterceptor references aids teardown and introspection. Lookup via BalancerUtil is the right integration point.
Also applies to: 70-75
78-93: LB factory looks solid; minor portability note on List.getLast().Enabling 5xx failover and configuring retries are appropriate for these tests. One portability caveat: elsewhere in this class you use List.getLast() (Java 21+). If the repo targets <21, replace with index-based access in enableFailOverOn5XX (see separate note below).
95-101: Service node factory is clean and deterministic for tests.
- Forcing "Connection: close" avoids keep-alive reuse side-effects under failure—good for deterministic behavior.
- Capturing the RandomlyFailingDummyWebServiceInterceptor instances enables post-run accounting.
No changes requested.
Also applies to: 102-115
136-147: All-fail test assertions look correct.With successChance=0 and 100 requests, expecting 100 exceptions and 0 successes is appropriate.
385-386: Nice: switched noisy println to structured logging.Logging total time and slowest run via SLF4J is preferable in tests that may be run in CI.
446-448: Headers via constants are good; keep UTF‑8 explicit.Using TEXT_XML_UTF8 and an empty SOAP_ACTION is consistent with the test harness.
224-224: Replace getFirst()/get(n) mix with index-based access only.To avoid relying on Java 21 List.getFirst(), use get(0) consistently.
Apply this diff:
- void test_5destinations_6threads_100calls_4shutdown() throws Exception { + void test_5destinations_6threads_100calls_4shutdown() throws Exception { @@ - if (i == 10) { - nodes.getFirst().shutdown(); + if (i == 10) { + nodes.get(0).shutdown(); } else if (i == 20) { nodes.get(1).shutdown(); } else if (i == 30) { nodes.get(2).shutdown(); } else if (i == 40) { nodes.get(3).shutdown(); }Also applies to: 230-239
⛔ Skipped due to learnings
Learnt from: predic8 PR: membrane/api-gateway#2068 File: core/src/test/java/com/predic8/membrane/core/interceptor/balancer/ClusterBalancerTest.java:73-90 Timestamp: 2025-08-22T13:18:09.421Z Learning: The Membrane API Gateway project uses Java 21, so modern Java 21 features like List.getFirst() are valid and should be used instead of the older get(0) approach.
core/src/main/java/com/predic8/membrane/core/exchangestore/FileExchangeStore.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/exchangestore/FileExchangeStore.java
Show resolved
Hide resolved
.../main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptor.java
Outdated
Show resolved
Hide resolved
.../test/java/com/predic8/membrane/core/services/RandomlyFailingDummyWebServiceInterceptor.java
Show resolved
Hide resolved
...va/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java (2)
200-203: Keep List.getFirst(); project targets Java 21.Per the retrieved learnings for this repo, Java 21 is the baseline, so
List.getFirst()is preferred over index-based access. The prior bot comment suggestingget(0)can be ignored.We used your saved learning: “The project uses Java 21, so List.getFirst() is valid.”
230-239: List.getFirst()/get(n) usage here is fine with Java 21.The mix of
getFirst()andget(i)is clear and intentional for readability.
🧹 Nitpick comments (15)
distribution/examples/extending-membrane/error-handling/custom-error-messages/requests.http (3)
6-6: Case A payload diverges from test intent; align example with tests or clarify scenario.The test for Case A uses a duplicate attribute (invalid XML) while this example adds an extra distinct attribute k, likely exercising a “too many attributes” protection. If both are meant to produce the same “XML Protection: Invalid XML!” behavior, consider unifying to one mechanism across docs/tests to reduce confusion.
Two options:
- Option A (align example to tests; invalid XML via duplicate attribute):
-<foo a="1" b="2" c="3" d="4" e="5" f="6" g="7" h="8" i="9" j="10" k="to much"/> +<foo a="1" b="2" c="3" d="4" e="5" f="6" g="7" h="8" i="9" j="10" j="to much"/>
- Option B (align tests to example; exceed attribute count via new k attribute): see companion comment in the test file for a matching change.
6-6: Nit: “to much” → “too much” (if not intentional).If this string is user-facing and not intentional for test semantics, consider fixing the grammar.
-... k="to much"/> +... k="too much"/>
6-6: Consider using application/xml for consistency with tests.Case A’s test uses APPLICATION_XML, while this example uses text/xml. If project-wide guidance prefers application/xml, update the header here as well (not part of the changed lines, shown for clarity):
-Content-Type: text/xml +Content-Type: application/xmldistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java (4)
16-23: Avoid wildcard static imports to reduce namespace pollution and accidental collisions.Prefer explicit imports for the few members used.
-import com.predic8.membrane.examples.util.*; -import org.junit.jupiter.api.*; - -import static com.predic8.membrane.core.http.MimeType.*; -import static io.restassured.RestAssured.*; -import static io.restassured.http.ContentType.*; -import static org.hamcrest.CoreMatchers.*; +import com.predic8.membrane.examples.util.AbstractSampleMembraneStartStopTestcase; +import org.junit.jupiter.api.Test; + +import static com.predic8.membrane.core.http.MimeType.APPLICATION_XML; +import static io.restassured.RestAssured.given; +import static io.restassured.http.ContentType.XML; +import static org.hamcrest.CoreMatchers.containsString;
38-38: Intentionally malformed XML via duplicate attribute — add a short comment for future readers.This is fine to trigger “invalid XML,” but the intent isn’t obvious. A brief inline comment improves maintainability.
- <foo a="1" b="2" c="3" d="4" e="5" f="6" g="7" h="8" i="9" j="10" j="to much"/>""") + // Deliberately invalid: duplicate attribute 'j' to trigger XML Protection. + <foo a="1" b="2" c="3" d="4" e="5" f="6" g="7" h="8" i="9" j="10" j="to much"/>""")
16-23: Consistency: unify content-type usage across tests (MimeType vs RestAssured ContentType).Case A uses APPLICATION_XML; other cases use ContentType.XML. Consider standardizing to one style (prefer MimeType constants for cross-project consistency), or add a comment explaining the mix if there’s a reason.
Option (unify to MimeType constants):
- .contentType(XML) + .contentType(APPLICATION_XML)Apply to caseB, caseE, and caseF.
Also applies to: 36-36
36-36: Doc/test alignment: example uses extra attribute ‘k’, test uses duplicate ‘j’. Choose one pattern.If you prefer the “too many attributes” protection scenario (valid XML but blocked), adjust the test to mirror the example:
- <foo a="1" b="2" c="3" d="4" e="5" f="6" g="7" h="8" i="9" j="10" j="to much"/>""") + <foo a="1" b="2" c="3" d="4" e="5" f="6" g="7" h="8" i="9" j="10" k="to much"/>""")Otherwise, update the example as shown in the requests.http comment. Ensure the error message asserted (“XML Protection: Invalid XML!”) still matches the actual response in either case.
Also applies to: 38-38
core/src/test/java/com/predic8/membrane/core/UnitTests.java (2)
19-25: Convert inline TODOs to tracked issues or @Disabled-with-reasonThe TODO list in the Javadoc will drift. Prefer linking to GitHub issues (one per item) or temporarily annotating the affected tests with @disabled("link-to-issue") to keep status actionable and discoverable from the test runner.
If helpful, I can open issues for each bullet and push a follow-up that adds @disabled(reason) to the referenced tests.
29-41: Prefer tag-based filtering over brittle class-name regex; if keeping regex, anchor and escapeClass-name based excludes are hard to maintain at scale. Consider:
- Tagging long-running/infra-dependent tests (e.g., @tag("integration"), @tag("requires-internet"), @tag("flaky")) and using @ExcludeTags at the suite level.
- If you keep regex excludes, anchor and escape them to avoid accidental matches and to document intent.
Apply this diff to anchor/escape the current patterns:
@ExcludeClassNamePatterns({ - "com.predic8.membrane.AllTests", - "com.predic8.membrane.core.interceptor.opentelemetry.OpenTelemetryInterceptorTest", - "com.predic8.membrane.core.interceptor.balancer.NodeOnlineCheckerTest", - "com.predic8.membrane.core.interceptor.tunnel.WebsocketStompTest", - "com.predic8.membrane.core.interceptor.oauth2client.OAuth2Resource2InterceptorTest", - "com.predic8.membrane.core.interceptor.shadowing.ShadowingInterceptorTest", - "com.predic8.membrane.core.interceptor.rewrite.RewriteInterceptorIntegrationTest", + "^com\\.predic8\\.membrane\\.AllTests$", + "^com\\.predic8\\.membrane\\.core\\.interceptor\\.opentelemetry\\.OpenTelemetryInterceptorTest$", + "^com\\.predic8\\.membrane\\.core\\.interceptor\\.balancer\\.NodeOnlineCheckerTest$", + "^com\\.predic8\\.membrane\\.core\\.interceptor\\.tunnel\\.WebsocketStompTest$", + "^com\\.predic8\\.membrane\\.core\\.interceptor\\.oauth2client\\.OAuth2Resource2InterceptorTest$", + "^com\\.predic8\\.membrane\\.core\\.interceptor\\.shadowing\\.ShadowingInterceptorTest$", + "^com\\.predic8\\.membrane\\.core\\.interceptor\\.rewrite\\.RewriteInterceptorIntegrationTest$", // // From includes, weren't run before! // TODO: Fix or delete - "com.predic8.membrane.core.transport.http.ConnectionTest", - "com.predic8.membrane.core.util.MemcachedConnectorTest" + "^com\\.predic8\\.membrane\\.core\\.transport\\.http\\.ConnectionTest$", + "^com\\.predic8\\.membrane\\.core\\.util\\.MemcachedConnectorTest$" })If you want to move to tags later, the suite would look like:
@Suite @SelectPackages({"com.predic8"}) @ExcludeTags({"integration", "requires-internet", "flaky"}) public class UnitTests {}…and the excluded tests would gain @tag annotations accordingly.
core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java (6)
36-43: Prefer a single HTTP client family for params (optional: switch to HttpClient 3.x param constant).You’re mixing HttpComponents 4.x (
CoreProtocolPNames.PROTOCOL_VERSION) with Commons HttpClient 3.x (HttpClient,HttpVersion). While it works because the param key string matches, sticking to one family reduces cognitive load. Optional tweak below keeps everything in Commons HttpClient 3.x.- import static org.apache.http.params.CoreProtocolPNames.*; + import static org.apache.commons.httpclient.params.HttpMethodParams.PROTOCOL_VERSION;
61-64: Remove unused test-only accumulator.
randomFailingInterceptorsis populated but never read, which adds noise. If you don’t plan to assert per-node failure behavior, drop the field and the add.- private final List<RandomlyFailingDummyWebServiceInterceptor> randomFailingInterceptors = new ArrayList<>(); ... - RandomlyFailingDummyWebServiceInterceptor rfi = new RandomlyFailingDummyWebServiceInterceptor(ctx.successChance); - randomFailingInterceptors.add(rfi); + RandomlyFailingDummyWebServiceInterceptor rfi = new RandomlyFailingDummyWebServiceInterceptor(ctx.successChance);Also applies to: 112-113
118-121: Avoid fragile “last interceptor is HTTPClientInterceptor” assumption.If transport interceptor order changes, this will break at runtime. Resolve by selecting the last HTTPClientInterceptor by type.
- List<Interceptor> l = balancer.getTransport().getInterceptors(); - ((HTTPClientInterceptor) l.getLast()).setFailOverOn5XX(true); + List<Interceptor> l = balancer.getTransport().getInterceptors(); + HTTPClientInterceptor httpClient = + (HTTPClientInterceptor) l.stream() + .filter(i -> i instanceof HTTPClientInterceptor) + .reduce((first, second) -> second) // last of type + .orElseThrow(() -> new IllegalStateException("HTTPClientInterceptor not found in transport chain")); + httpClient.setFailOverOn5XX(true);
124-129: Make teardown best-effort to not mask test failures.If any shutdown throws, you’ll hide the original assertion failure. Wrap shutdowns.
- void tearDown() { - for (Router httpRouter : nodes) { - httpRouter.shutdown(); - } - balancer.shutdown(); - } + void tearDown() { + for (Router httpRouter : nodes) { + try { + httpRouter.shutdown(); + } catch (Exception e) { + log.warn("Node shutdown failed.", e); + } + } + try { + balancer.shutdown(); + } catch (Exception e) { + log.warn("Balancer shutdown failed.", e); + } + }
171-183: Fix assertion messages; consider tightening lower bound (optional).
- Grammar: “then” -> “than”.
- Message mismatch: the first assertion uses 700 but the message says 900.
Minimal fix below. Optionally, given retries=5 the theoretical success ≈ 98% with p=0.5, you could tighten the lower bound to reduce false negatives while staying non-flaky (e.g., ≥ 900). Up to you.
- assertTrue(ctx.successCounter.get() >= 700, "ctx.successCounter.get() is %d, less then 900".formatted(ctx.successCounter.get())); - assertTrue(ctx.successCounter.get() < 1000, "ctx.successCounter.get() is %d, greater then 1000".formatted(ctx.successCounter.get())); + assertTrue(ctx.successCounter.get() >= 700, "ctx.successCounter.get() is %d, less than 700".formatted(ctx.successCounter.get())); + assertTrue(ctx.successCounter.get() < 1000, "ctx.successCounter.get() is %d, greater than or equal to 1000".formatted(ctx.successCounter.get()));If you want the tighter bound:
- assertTrue(ctx.successCounter.get() >= 700, "ctx.successCounter.get() is %d, less than 700".formatted(ctx.successCounter.get())); + assertTrue(ctx.successCounter.get() >= 900, "ctx.successCounter.get() is %d, less than 900".formatted(ctx.successCounter.get()));
418-421: Minor: stray double-space in method signature; message already improved above.Not a blocker, but easy to tidy.
- private void standardExpectations(TestingContext ctx) { + private void standardExpectations(TestingContext ctx) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
core/src/main/java/com/predic8/membrane/core/transport/http/MessageTracer.java(1 hunks)core/src/test/java/com/predic8/membrane/core/UnitTests.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/CollectionsUtilTest.java(2 hunks)core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java(12 hunks)distribution/examples/extending-membrane/error-handling/custom-error-messages/requests.http(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/test/java/com/predic8/membrane/core/util/CollectionsUtilTest.java
- core/src/main/java/com/predic8/membrane/core/transport/http/MessageTracer.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T13:18:09.421Z
Learnt from: predic8
PR: membrane/api-gateway#2068
File: core/src/test/java/com/predic8/membrane/core/interceptor/balancer/ClusterBalancerTest.java:73-90
Timestamp: 2025-08-22T13:18:09.421Z
Learning: The Membrane API Gateway project uses Java 21, so modern Java 21 features like List.getFirst() are valid and should be used instead of the older get(0) approach.
Applied to files:
core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java
🧬 Code graph analysis (3)
core/src/test/java/com/predic8/membrane/core/UnitTests.java (3)
distribution/src/test/java/com/predic8/membrane/examples/ExampleTests.java (1)
Suite(25-31)distribution/src/test/java/com/predic8/membrane/examples/ExampleTestsWithInternet.java (1)
Suite(20-23)distribution/src/test/java/com/predic8/membrane/examples/ExampleTestsWithoutInternet.java (1)
Suite(20-23)
core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java (2)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)core/src/main/java/com/predic8/membrane/core/interceptor/balancer/BalancerUtil.java (1)
BalancerUtil(23-131)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (11)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java (3)
36-36: LGTM: Using APPLICATION_XML is consistent with project MimeType constants.No issues; aligns with MimeType.APPLICATION_XML usage.
42-42: LGTM: Conditional logging on validation failures helps triage flaky failures.Good addition for diagnosability without noisy logs on success.
26-29: Verification complete: example directory and resources are present
- Confirmed
distribution/examples/extending-membrane/error-handling/custom-error-messagesexists- Confirmed
requests.httpis present in that directoryNo further changes needed.
core/src/test/java/com/predic8/membrane/core/UnitTests.java (2)
29-29: Good addition: excluding com.predic8.membrane.AllTests avoids accidental recursive/duplicate suite executionThis prevents aggregator suites from triggering or being triggered here. Nice catch.
26-28: Scope broadened to com.predic8: please review newly-included test classesThe change from
@SelectPackages({"com.predic8.membrane.core"})to
@SelectPackages({"com.predic8"})in
core/src/test/java/com/predic8/membrane/core/UnitTests.javanow picks up all tests undercom.predic8.*. The verification script identified the following classes that would not have been included previously:• Abstract utility and aggregator tests
– com.predic8.membrane.AbstractTestWithRouter
– com.predic8.membrane.AllTests
– com.predic8.membrane.test.TestUtil• Performance and evaluation tests
– com.predic8.membrane.evaluation.CompareNewStringWithStringBuffer
– com.predic8.membrane.evaluation.JDBCApiKeyStorePerformanceTest
– com.predic8.membrane.evaluation.RequestPerformanceTest
– com.predic8.membrane.evaluation.RequestPerformanceVersion1
– com.predic8.membrane.evaluation.RequestPerformanceVersion2
– com.predic8.membrane.evaluation.RequestPerformanceVersion3
– com.predic8.membrane.evaluation.RequestPerformanceVersion4• Integration tests (with/without internet)
– com.predic8.membrane.integration.IntegrationTestsWithInternet
– com.predic8.membrane.integration.IntegrationTestsWithoutInternet
– com.predic8.membrane.integration.Util
– com.predic8.membrane.integration.withinternet.LargeBodyTest
– com.predic8.membrane.integration.withinternet.OpenAPIRecordFactoryIntegrationTest
– com.predic8.membrane.integration.withinternet.ProxySSLConnectionMethodTest
– com.predic8.membrane.integration.withinternet.ViaProxyTest
– com.predic8.membrane.integration.withoutinternet.LimitedMemoryExchangeStoreIntegrationTest
– com.predic8.membrane.integration.withoutinternet.MassivelyParallelTest
– com.predic8.membrane.integration.withoutinternet.SessionManagerTest• Interceptor coverage tests
– com.predic8.membrane.interceptor.LimitInterceptorTest
– com.predic8.membrane.interceptor.LoadBalancingInterceptorTest
– com.predic8.membrane.interceptor.LoadBalancingInterceptorFaultMonitoringStrategyTest
– com.predic8.membrane.interceptor.MultipleLoadBalancersTest
– com.predic8.membrane.interceptor.ws_addressing.WsaEndpointRewriterInterceptorTest
– com.predic8.membrane.interceptor.ws_addressing.WsaEndpointRewriterTest• XML beautifier tests
– com.predic8.xml.beautifier.StandardXMLBeautifierFormatterTest
– com.predic8.xml.beautifier.XMLBeautifierTestPlease confirm whether all of these were intended to run as part of the core module’s suite. If certain categories (e.g., internet-dependent integration tests or long-running performance tests) should remain excluded, consider restoring a narrower
@SelectPackagesscope or adding appropriate@ExcludeClassNamePatterns.core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java (6)
54-54: Package-private JUnit 5 test class is fine.JUnit 5 does not require public test classes or methods. Good tightening of visibility.
70-75: Balancer wiring via BalancerUtil.up(...) looks correct.Destinations registered against the "Default" balancer/cluster with the expected localhost ports. Matches the refactor toward centralized balancer lookup.
87-93: Retry/backoff config aligns with failure simulation.Setting retries/delay/backoff on the balancer’s HTTP client matches the stochastic failure tests. Good.
136-147: All-fail scenario assertion is consistent with successChance=0.The expectations match the setup and the configured retries/failover.
385-385: Good switch to structured logging for totals.Consistent with the rest of the file.
442-444: Static header constants and charset usage look good.Using
CONTENT_TYPE,TEXT_XML_UTF8, andSOAP_ACTIONvia static imports improves clarity and avoids typos.
...va/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptor.java (1)
96-108: Adopting try-with-resources + explicit flush resolves truncation risks.This addresses the earlier concern about encoder buffers and resource hygiene. Good fix.
core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java (2)
200-203: Keep List.getFirst() (Java 21) — good use given project standards.Using getFirst() is correct per the project’s Java 21 baseline. No change needed.
398-410: Good: release HTTP resources and use logging instead of System.out.The try/finally with method.releaseConnection() prevents socket leaks, and replacing println with SLF4J is appropriate.
🧹 Nitpick comments (15)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/MassivelyParallelTest.java (5)
46-46: Nit: grammar in comment.“to high” → “too high”.
Apply:
- // Should not be to high to keep tests stable + // Should not be too high to keep tests stable
47-47: Make concurrency a constant (and optionally configurable).Use final for immutability; optionally allow overriding via system property for CI tuning.
Minimal:
- private static int CONCURRENT_THREADS = 500; + private static final int CONCURRENT_THREADS = 500;Optional (configurable via -Dmp.threads=...):
- private static int CONCURRENT_THREADS = 500; + private static final int CONCURRENT_THREADS = + Integer.getInteger("mp.threads", 500);
57-59: Reduce risk of ephemeral accept-queue pressure by giving backlog headroom.Matching backlog to the exact concurrency can cause occasional connection refusals under burst conditions. A small multiple tends to be more robust across OSes.
- server.getTransport().setConcurrentConnectionLimitPerIp(CONCURRENT_THREADS); - server.getTransport().setBacklog(CONCURRENT_THREADS); + server.getTransport().setConcurrentConnectionLimitPerIp(CONCURRENT_THREADS); + server.getTransport().setBacklog(CONCURRENT_THREADS * 2);
76-76: Be explicit about content type to avoid charset heuristics.
ok().body(String)encodes UTF‑8 but doesn’t set Content-Type. Since the client decodes the response body, be explicit to avoid surprises if defaults change.- exc.setResponse(ok().body(exc.getRequest().getUri()).build()); + exc.setResponse(ok() + .contentType("text/plain; charset=UTF-8") + .body(exc.getRequest().getUri()) + .build());
98-116: Surface task failures explicitly (don’t rely only on final size mismatch).Exceptions in tasks submitted via
submitare captured in Futures and won’t fail the test unless inspected. You do assert onpaths.size(), which will usually catch failures indirectly, but explicit propagation gives clearer diagnostics.Option A — collect and join Futures:
- try { - for (int i = 0; i < threadCount; i++) { - es.submit(() -> job.accept(cdl)); - } - es.shutdown(); - if (!es.awaitTermination(30, TimeUnit.SECONDS)) { - es.shutdownNow(); - fail("Tasks did not complete within timeout"); - } - } catch (InterruptedException e) { + try { + List<Future<?>> futures = new ArrayList<>(threadCount); + for (int i = 0; i < threadCount; i++) { + futures.add(es.submit(() -> job.accept(cdl))); + } + es.shutdown(); + if (!es.awaitTermination(30, TimeUnit.SECONDS)) { + es.shutdownNow(); + fail("Tasks did not complete within timeout"); + } + // Propagate any task failure with full stack trace + for (Future<?> f : futures) { + try { + f.get(); + } catch (ExecutionException ee) { + fail("Task failed", ee.getCause()); + } + } + } catch (InterruptedException e) { Thread.currentThread().interrupt(); es.shutdownNow(); fail("Interrupted while waiting for tasks to complete"); }Option B — use Structured Concurrency (JDK 21):
private void runInParallel(Consumer<CountDownLatch> job, int threadCount) { CountDownLatch cdl = new CountDownLatch(threadCount); try (var scope = new StructuredTaskScope.ShutdownOnFailure()) { for (int i = 0; i < threadCount; i++) { scope.fork(() -> { job.accept(cdl); return null; }); } scope.join(); // waits for all scope.throwIfFailed(); // throws first failure cause } catch (InterruptedException e) { Thread.currentThread().interrupt(); fail("Interrupted while waiting for tasks to complete"); } }core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptor.java (4)
64-65: Useful debug trace; consider adding correlation context.The parameterized debug log is cheap and handy. If you have a request/exchange id (or remote IP), consider appending it for easier correlation during incident triage.
71-79: Also set X-Protection on non-XML rejections for consistency.You add X-Protection only when XML policy is violated, not when the content type is non-XML. Setting it in both paths simplifies client-side diagnostics and monitoring.
Apply this diff within the same block after buildAndSetResponse(exc) and before returning:
user(router.isProduction(), getDisplayName()) .title("Request discarded by xmlProtection") .detail(msg) .buildAndSetResponse(exc); + // Mark rejection reason consistently + exc.getResponse().getHeader().add(X_PROTECTION, + ("Non-XML content type: " + exc.getRequest().getHeader().getContentType())); return ABORT;
98-101: Fix stale variable name in comment (“msg”).The inline comment still references msg.getBodyAsStreamDecoded(), but the code uses exc.getRequest().getBodyAsStreamDecoded(). Update to avoid confusion for future readers.
- // msg.getBodyAsStreamDecoded() delivers an InputStream from bytes (Chunks) -> close should not be an issue + // exc.getRequest().getBodyAsStreamDecoded() delivers an InputStream from bytes (chunks) -> closing should not be an issue
99-101: Pre-size ByteArrayOutputStream when Content-Length is known to reduce reallocations.If the request advertises a positive Content-Length, use it as the initial BAOS capacity. This avoids buffer growth and copies for large messages.
- try (ByteArrayOutputStream stream = new ByteArrayOutputStream(); + // Pre-size buffer when possible to reduce reallocations + long cl = exc.getRequest().getHeader().getContentLength(); + int initialSize = (cl > 0 && cl < Integer.MAX_VALUE) ? (int) cl : 32_768; // keep a sane default + try (ByteArrayOutputStream stream = new ByteArrayOutputStream(initialSize); OutputStreamWriter out = new OutputStreamWriter(stream, charset); InputStreamReader in = new InputStreamReader(exc.getRequest().getBodyAsStreamDecoded(), charset)) {Note: adjust the header accessor if Content-Length is exposed elsewhere in your HTTP API.
core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java (6)
123-129: Make tearDown resilient to partial initialization and clear lists.Guard against NPEs when setup fails mid-way and ensure lists are cleared so tests remain independent.
@AfterEach -void tearDown() { - for (Router httpRouter : nodes) { - httpRouter.shutdown(); - } - balancer.shutdown(); -} +void tearDown() { + for (Router httpRouter : nodes) { + try { + httpRouter.shutdown(); + } catch (Exception e) { + log.warn("Error during node shutdown", e); + } + } + nodes.clear(); + randomFailingInterceptors.clear(); + if (balancer != null) { + try { + balancer.shutdown(); + } catch (Exception e) { + log.warn("Error during balancer shutdown", e); + } + } +}
154-164: Unify JUnit 5 test visibility (remove redundant public).Other test methods are package-private. JUnit 5 does not require public visibility.
- public void test_2destinations_6threads_100calls_allSucceed() throws Exception { + void test_2destinations_6threads_100calls_allSucceed() throws Exception {
171-183: Fix assertion messages (threshold mismatch and grammar).Messages say “less then 900”/“greater then 1000”, but the code asserts >=700 and <1000. Align the text and fix “than”.
- assertTrue(ctx.successCounter.get() >= 700, "ctx.successCounter.get() is %d, less then 900".formatted(ctx.successCounter.get())); - assertTrue(ctx.successCounter.get() < 1000, "ctx.successCounter.get() is %d, greater then 1000".formatted(ctx.successCounter.get())); + assertTrue(ctx.successCounter.get() >= 700, "ctx.successCounter.get() is %d, less than 700".formatted(ctx.successCounter.get())); + assertTrue(ctx.successCounter.get() < 1000, "ctx.successCounter.get() is %d, less than 1000".formatted(ctx.successCounter.get()));
422-426: Avoid mixing HttpClient 3.x with HttpComponents 4.x constants for protocol version.getHttpClient() uses org.apache.commons.httpclient.HttpClient (3.x) but imports PROTOCOL_VERSION from org.apache.http.params.CoreProtocolPNames (4.x). Prefer HttpClient 3.x’s constant to prevent accidental cross-lib coupling.
-import static org.apache.http.params.CoreProtocolPNames.*; +import static org.apache.commons.httpclient.params.HttpMethodParams.PROTOCOL_VERSION;No code change needed below; client.getParams().setParameter(PROTOCOL_VERSION, HTTP_1_1); will continue to work with the 3.x constant.
449-456: Close the resource stream or use a repeatable entity to avoid leaking the classpath InputStream.InputStreamRequestEntity typically does not close the provided InputStream for you. For a self-contained test, load the bytes and use ByteArrayRequestEntity.
private PostMethod getPostMethod() { PostMethod post = new PostMethod( "http://localhost:3054/axis2/services/BLZService"); - post.setRequestEntity(new InputStreamRequestEntity(this.getClass().getResourceAsStream("/getBank.xml"))); - post.setRequestHeader(CONTENT_TYPE, TEXT_XML_UTF8); + try (var is = Objects.requireNonNull(this.getClass().getResourceAsStream("/getBank.xml"), "getBank.xml not found")) { + var body = is.readAllBytes(); + post.setRequestEntity(new ByteArrayRequestEntity(body, TEXT_XML_UTF8)); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + post.setRequestHeader(CONTENT_TYPE, TEXT_XML_UTF8); post.setRequestHeader(SOAP_ACTION, ""); return post; }Note: Requires imports for java.io.IOException and java.io.UncheckedIOException, and ByteArrayRequestEntity (org.apache.commons.httpclient.methods).
251-321: Use Consumer (or IntConsumer) instead of Function<Integer, Void> for preSubmitCallback.Returning Void forces “return null;” noise and makes intent less clear.
- private Function<Integer, Void> preSubmitCallback = null; + private java.util.function.Consumer<Integer> preSubmitCallback = null; ... - public Builder preSubmitCallback(Function<Integer, Void> preSubmitCallback) { + public Builder preSubmitCallback(java.util.function.Consumer<Integer> preSubmitCallback) { this.preSubmitCallback = preSubmitCallback; return this; } ... - private final Function<Integer, Void> preSubmitCallback; + private final java.util.function.Consumer<Integer> preSubmitCallback;And at submission:
- if (ctx.preSubmitCallback != null) { - ctx.preSubmitCallback.apply(i); - } + if (ctx.preSubmitCallback != null) { + ctx.preSubmitCallback.accept(i); + }Also applies to: 389-394
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptor.java(2 hunks)core/src/test/java/com/predic8/membrane/core/services/RandomlyFailingDummyWebServiceInterceptor.java(2 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/MassivelyParallelTest.java(5 hunks)core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/com/predic8/membrane/core/services/RandomlyFailingDummyWebServiceInterceptor.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T13:18:09.421Z
Learnt from: predic8
PR: membrane/api-gateway#2068
File: core/src/test/java/com/predic8/membrane/core/interceptor/balancer/ClusterBalancerTest.java:73-90
Timestamp: 2025-08-22T13:18:09.421Z
Learning: The Membrane API Gateway project uses Java 21, so modern Java 21 features like List.getFirst() are valid and should be used instead of the older get(0) approach.
Applied to files:
core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java
📚 Learning: 2025-08-22T13:18:09.421Z
Learnt from: predic8
PR: membrane/api-gateway#2068
File: core/src/test/java/com/predic8/membrane/core/interceptor/balancer/ClusterBalancerTest.java:73-90
Timestamp: 2025-08-22T13:18:09.421Z
Learning: The Membrane API Gateway project has mixed Java version configurations in different modules, but the team uses Java 21 in practice. Modern Java 21 features like List.getFirst() are appropriate and should be used instead of older alternatives like List.get(0).
Applied to files:
core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java
🧬 Code graph analysis (2)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/MassivelyParallelTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/Response.java (1)
Response(38-500)
core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java (2)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)core/src/main/java/com/predic8/membrane/core/interceptor/balancer/BalancerUtil.java (1)
BalancerUtil(23-131)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (6)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/MassivelyParallelTest.java (3)
30-34: Static imports are concise and consistent with the new usage.The static imports of Response.* (for ok()) and ConcurrentHashMap.* (for newKeySet()) improve readability in this test context. No issues spotted.
93-96: Good: concurrent-safe set and parameterized assertions.Using
newKeySet()for concurrent writes and assertingpaths.size()againstCONCURRENT_THREADSkeeps the test self-validating for partial failures.
44-44: The locate script is running; I’ll review the path once it’s available.core/src/test/java/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java (3)
231-239: Node shutdown sequence looks correct and readable with getFirst()/get(i).The staged shutdowns are clear. Once the nodes list reset is added in setUp, these will target the intended routers for this test run.
36-41: LGTM: static imports, HTTP 5xx failover enablement, and invariant assertion.
- Static imports for headers/MIME/request/Outcome and BalancerUtil increase readability.
- Enabling failover on 5xx via the transport’s HTTPClientInterceptor is concise.
- The invariant “total = success + exception” is correctly asserted.
Also applies to: 118-121, 385-387, 429-432
27-27: No action needed: JetBrains annotations already declaredThe
org.jetbrains:annotationsartifact is present in the POMs:
- core module (
core/pom.xmllines 363–364)- test module (
test/pom.xmllines 44–45)- root POM (
pom.xmllines 354–355)Since the dependency is already declared, the imports in your test code (
@org.jetbrains.annotations.NotNull) will resolve correctly—no changes required.
.../main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptor.java
Show resolved
Hide resolved
...va/com/predic8/membrane/interceptor/LoadBalancingInterceptorFaultMonitoringStrategyTest.java
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores