refactor(wsdl): streamline WSDL handling, improve logging, and adjust…#2571
Conversation
… interceptor flow - Removed unused fields `blockRequest` and `blockResponse` in `AbstractProxy`. - Simplified `log.debug` messages in `WSDLPublisherInterceptor`. - Adjusted interceptor execution order in `SOAPProxy` by adding `WSDLPublisherInterceptor` to the end. - Changed `@MCElement` for `WSDLInterceptor` to include it in the flow. - Improved readability with `formatted` and repositioned imports for `notFound`.
📝 WalkthroughWalkthroughThis PR introduces WSDL path rewriting capabilities to the Membrane API Gateway. It adds path-based rewriting in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java (1)
114-118: Update the comment to reflect the current interceptor insertion logic.The comment states "add interceptors (in reverse order) to position 0", but
addWSDLPublisherInterceptor()now usesaddLast()at line 332, placing it at the end of the chain rather than position 0.📝 Proposed comment update
- // add interceptors (in reverse order) to position 0. + // add interceptors to configure WSDL handling addWebServiceExplorer(); addWSDLPublisherInterceptor(); addWSDLInterceptor();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.javacore/src/main/java/com/predic8/membrane/core/proxies/AbstractProxy.javacore/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java
💤 Files with no reviewable changes (1)
- core/src/main/java/com/predic8/membrane/core/proxies/AbstractProxy.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T10:09:12.663Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/interceptor/ws_addressing/WsaEndpointRewriter.java:47-81
Timestamp: 2025-08-22T10:09:12.663Z
Learning: In WS-A endpoint rewriter and similar HTTP processing components, avoid closing XMLEventReader/XMLEventWriter when they wrap HTTP streams from keep-alive sessions, as this can interfere with HTTP connection management. Only flush the writer to ensure output is written.
Applied to files:
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (5)
core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java (3)
32-32: LGTM! Static import improves readability.Adding the static import for
notFoundmakes line 222 more concise while maintaining clarity.
113-113: LGTM! Log message simplification is appropriate.Removing the "WSDLPublisherInterceptor:" prefix is appropriate since the logger already includes the class name in its output format.
231-231: LGTM! Modern string formatting improves readability.Using
formatted()is more readable and idiomatic than string concatenation.core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java (1)
325-333: Verify that moving WSDLPublisherInterceptor to the end of the chain preserves WSDL serving functionality.Changing from
addFirst()toaddLast()significantly alters the interceptor execution order. The WSDLPublisherInterceptor will now run after all other interceptors in the request flow, which may affect when and how WSDL documents are served to clients.Combined with the change to
WSDLInterceptor.excludeFromFlow(nowfalse), this represents a coordinated flow restructuring. Ensure this doesn't break:
- WSDL document retrieval via
?wsdlparameter- XSD document retrieval via
?xsd=parameter- Path rewriting in served WSDL documents
- Integration with ValidatorInterceptor and other interceptors
#!/bin/bash # Description: Find test files that verify WSDL serving functionality to ensure coverage # Search for test files related to SOAP proxy and WSDL publisher rg -l "WSDLPublisher|SOAPProxy" --type=java -g "*Test*.java"core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (1)
39-39: The XML content-type filtering in the parent class provides adequate safeguards; the flow participation change is safe.WSDLInterceptor's parent class
RelocatingInterceptor.handleResponse()enforces strict filtering that prevents processing non-XML responses (line 56-59 checks!exc.getResponse().isXML()). Additionally,Relocatoronly setswsdlFound=truewhen WSDL SOAP namespace elements are detected. The interceptor execution order is intentional:WSDLInterceptor.addFirst()(line 277) runs early in the chain, whileWSDLPublisherInterceptor.addLast()(line 332) runs at the end. The change is aligned with the WSDL handling refactor and existing test coverage validates the rewriting behavior.
…test - Introduced `20-SOAPProxy.yaml` for configuring a SOAP proxy example. - Added `SOAPProxyTutorialTest` and `AbstractSOAPTutorialTest` for integration testing.
|
This pull request needs "/ok-to-test" from an authorized committer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@distribution/src/test/java/com/predic8/membrane/tutorials/soap/SOAPProxyTutorialTest.java:
- Line 1: Add the standard Apache License 2.0 copyright header used in this
package to the top of SOAPProxyTutorialTest.java; locate the file by the package
declaration "package com.predic8.membrane.tutorials.soap;" and the class
"SOAPProxyTutorialTest" and prepend the identical header block used in
AbstractSOAPTutorialTest.java so the file matches licensing/formatting across
the package.
🧹 Nitpick comments (1)
distribution/src/test/java/com/predic8/membrane/tutorials/soap/SOAPProxyTutorialTest.java (1)
15-26: Consider renaming the test method to reflect its purpose.The test method name
adminConsole()doesn't clearly describe what it's testing. The test actually verifies WSDL retrieval from the SOAP proxy endpoint. A more descriptive name would improve readability and maintainability.♻️ Suggested method name alternatives
@Test - void adminConsole() { + void shouldReturnWsdlDefinitions() { // @formatter:off given() .when()or
@Test - void adminConsole() { + void testWsdlEndpointReturnsDefinitions() { // @formatter:off given() .when()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
distribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractSOAPTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/SOAPProxyTutorialTest.javadistribution/tutorials/soap/20-SOAPProxy.yaml
✅ Files skipped from review due to trivial changes (1)
- distribution/tutorials/soap/20-SOAPProxy.yaml
⏰ 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 (1)
distribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractSOAPTutorialTest.java (1)
1-26: LGTM!Clean test base class implementation. The structure follows good testing patterns by providing a reusable base for SOAP tutorial tests.
- Introduced `30-WSDL-Rewriter.yaml` for demonstrating WSDL rewriting functionality. - Added `WSDLRewriterTutorialTest` and expanded `AbstractCityServiceTest` for reusable SOAP test logic. - Rewrote `membrane.sh` and added `membrane.cmd` for improved script execution and structure detection. - Removed `invoke-sample-soap-service.sh` and integrated SOAP request example directly into YAML tutorials. - Enhanced SOAP proxy examples in `20-SOAPProxy.yaml` and `10-Sample-SOAP-Service.yaml`. - Updated `README.md` with new tutorial topics and streamlined documentation.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java:
- Around line 203-205: The new setPort(int) in WSDLInterceptor breaks existing
tests that call setPort(String); add a backward-compatible overload public void
setPort(String port) in WSDLInterceptor that delegates to super.setPort(port) so
existing calls in WSDLInterceptorTest (e.g., setPort("") and setPort("2000"))
keep compiling, while retaining the new setPort(int) behavior.
In
@distribution/src/test/java/com/predic8/membrane/tutorials/soap/WSDLRewriterTutorialTest.java:
- Around line 62-74: The test method webServiceExplorer sets a request body for
a GET which is unnecessary; remove the .body("Invalid") call from the
RestAssured given() chain in webServiceExplorer so the request is a plain GET
(keep the URL, .get("http://localhost:2000/my-service"), and the subsequent
assertions on statusCode, contentType(HTML), and body(containsString("Membrane
API Gateway: CityService"))).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.javacore/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractCityServiceTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/SOAPProxyTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/SampleSOAPServiceTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/WSDLRewriterTutorialTest.javadistribution/tutorials/data/city-request.soap.xmldistribution/tutorials/soap/10-Sample-SOAP-Service.yamldistribution/tutorials/soap/20-SOAPProxy.yamldistribution/tutorials/soap/30-WSDL-Rewriter.yamldistribution/tutorials/soap/README.mddistribution/tutorials/soap/invoke-sample-soap-service.shdistribution/tutorials/soap/membrane.cmddistribution/tutorials/soap/membrane.sh
💤 Files with no reviewable changes (1)
- distribution/tutorials/soap/invoke-sample-soap-service.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- distribution/src/test/java/com/predic8/membrane/tutorials/soap/SOAPProxyTutorialTest.java
- distribution/tutorials/soap/20-SOAPProxy.yaml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-02T18:05:47.059Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1947
File: distribution/tutorials/soap/membrane.sh:17-22
Timestamp: 2025-07-02T18:05:47.059Z
Learning: When using environment variable shortcuts like `$MEMBRANE_HOME` in shell scripts that look for installation directories, validate that required files (like `starter.jar` for Membrane) exist in the candidate directory before returning it. This prevents hard-to-diagnose runtime errors when the environment variable points to an invalid or stale location.
Applied to files:
distribution/tutorials/soap/membrane.shdistribution/tutorials/soap/membrane.cmd
📚 Learning: 2025-11-23T15:59:51.742Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2339
File: distribution/examples/web-services-soap/rest2soap-template/apis.yaml:17-32
Timestamp: 2025-11-23T15:59:51.742Z
Learning: In Membrane API Gateway YAML configuration files, response flows execute in reverse order. Steps that appear later in the response flow list actually execute first. For example, if a template references properties and setProperty steps appear after the template in the YAML, those setProperty steps will execute before the template renders.
Applied to files:
distribution/tutorials/soap/30-WSDL-Rewriter.yaml
📚 Learning: 2025-08-22T10:09:12.663Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/interceptor/ws_addressing/WsaEndpointRewriter.java:47-81
Timestamp: 2025-08-22T10:09:12.663Z
Learning: In WS-A endpoint rewriter and similar HTTP processing components, avoid closing XMLEventReader/XMLEventWriter when they wrap HTTP streams from keep-alive sessions, as this can interfere with HTTP connection management. Only flush the writer to ensure output is written.
Applied to files:
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java
🧬 Code graph analysis (2)
distribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractCityServiceTest.java (1)
core/src/main/java/com/predic8/membrane/core/Constants.java (1)
Constants(24-103)
distribution/src/test/java/com/predic8/membrane/tutorials/soap/WSDLRewriterTutorialTest.java (1)
core/src/main/java/com/predic8/membrane/core/Constants.java (1)
Constants(24-103)
🪛 Shellcheck (0.11.0)
distribution/tutorials/soap/membrane.sh
[warning] 8-8: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
⏰ 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 (20)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)
59-59: LGTM! Logging improvements enhance debuggability.The additions improve observability and error messaging:
- Startup args logging aids troubleshooting
- Formatted error messages provide clearer feedback
- Enhanced path resolution warnings include context
Also applies to: 253-253, 343-343
distribution/tutorials/soap/membrane.sh (2)
8-8: Shellcheck warning is a false positive.The syntax
CDPATH= cdis correct—it temporarily clearsCDPATHfor that command only to ensure reliable directory resolution. The space after=is intentional and necessary.
10-18: LGTM! Directory-walking launcher logic is sound.The script correctly walks up the directory tree to locate the Membrane root and delegates to the main runner script. The validation of
LICENSE.txtandscripts/run-membrane.shprovides a reasonable sanity check.distribution/tutorials/soap/membrane.cmd (1)
9-20: LGTM! Windows launcher mirrors the shell script logic.The directory-walking approach to locate Membrane root is consistent with
membrane.sh, and the delegation torun-membrane.cmdmaintains proper separation of concerns.core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (1)
39-39: No duplicate execution risk from excludeFromFlow change.The change from
excludeFromFlow = truetofalseenables automatic flow participation. However, existing code inSOAPProxy.addWSDLInterceptor()already contains deduplication logic (getFirstInterceptorOfType(WSDLInterceptor.class).isEmpty()) that prevents duplicate instances even when both automatic inclusion and explicit configuration occur.distribution/tutorials/data/city-request.soap.xml (1)
1-7: LGTM! Clean SOAP 1.1 request structure.The SOAP envelope structure is correct with proper namespace declarations. The request targets the
getCityoperation with a hardcoded city name "Delhi", which aligns with the test expectations inAbstractCityServiceTest.javawhere the response population is asserted to be "34665600".distribution/src/test/java/com/predic8/membrane/tutorials/soap/SampleSOAPServiceTutorialTest.java (1)
1-9: LGTM! Follows the tutorial test pattern correctly.The test class correctly extends
AbstractCityServiceTestand inherits thewsdl()andsoapCall()test methods. ThegetTutorialYaml()override properly references the corresponding tutorial configuration file.distribution/tutorials/soap/30-WSDL-Rewriter.yaml (2)
1-19: LGTM! Clear and helpful tutorial documentation.The documentation clearly explains the use case for WSDL rewriting behind reverse proxies or load balancers, and provides actionable instructions for testing the configuration.
20-29: Configuration is correct and properly supports all parameters in flow.The wsdlRewriter configuration with
host,protocol, andportparameters is valid. All three parameters are properly exposed via @MCAttribute annotations in the WSDLInterceptor class, confirming they are supported in flow configuration. The setPort API correctly accepts an int parameter as expected.distribution/tutorials/soap/10-Sample-SOAP-Service.yaml (2)
3-17: LGTM! Improved tutorial structure and clarity.The restructured documentation is clearer with distinct Run, WSDL, and Invoke sections. The curl command correctly references the SOAP request file, and the endpoint URL is consistent with the configuration and tests.
19-24: LGTM! Configuration aligns with test expectations.The API configuration with port 2000 and path
/city-servicematches whatAbstractCityServiceTestexpects. ThesampleSoapServiceflow step is properly configured.distribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractCityServiceTest.java (4)
1-12: LGTM! Proper imports and constant usage.The imports are well-organized, using static imports for readability. Good use of the
WSDL_SOAP11_NSconstant from theConstantsclass.
13-13: LGTM! Appropriate abstract class declaration.The class correctly extends
AbstractSOAPTutorialTest, allowing concrete test classes to specify their tutorial YAML configurations.
32-42: Verify that Delhi is a supported city in the sample SOAP service.The
readFileFromBaseDirmethod is correctly available fromDistributionExtractingTestcasethrough the parent class hierarchy, and the file../data/city-request.soap.xmlexists and loads correctly. However, the sample SOAP service documentation lists supported cities as: Bonn, Bielefeld, Manila, Da Nang, London, and New York—Delhi is not mentioned. Ensure Delhi is actually supported and returns population"34665600"before relying on this assertion.
15-30: The GPath expression is correct and matches the WSDL structure.The
definitions.service.port.address.@locationexpression correctly navigates the WSDL hierarchy. Rest-Assured's GPath automatically handles element namespace prefixes (ignoring them in path expressions), and the unprefixedlocationattribute is accessible directly via@locationwithout namespace configuration. This expression is used identically inWSDLRewriterTutorialTest.javaand aligns with the actual WSDL structure incity.wsdl, where the service has a single port with an address element containing the location attribute.distribution/src/test/java/com/predic8/membrane/tutorials/soap/WSDLRewriterTutorialTest.java (5)
1-25: LGTM: Imports are clean and well-organized.The static imports from Constants and RestAssured improve test readability, and all imports are utilized in the test methods.
26-26: LGTM: Class declaration follows the tutorial test pattern.
33-47: LGTM: WSDL rewriting test is well-structured.The test correctly validates:
- WSDL accessibility
- SOAP 1.1 namespace presence (using
WSDL_SOAP11_NSconstant)- URL rewriting from localhost to the public endpoint
The GPath expression for accessing the WSDL location attribute is appropriate.
28-31: Tutorial YAML file exists and is properly configured.The method correctly returns the tutorial YAML filename. The file
distribution/tutorials/soap/30-WSDL-Rewriter.yamlexists and contains proper WSDL rewriter configuration with host, protocol, and port overrides.
49-60: Test setup is correctly configured.The SOAP request test data file
city-request.soap.xmlexists atdistribution/tutorials/data/city-request.soap.xml, and thereadFileFromBaseDir()method is available in the parent classDistributionExtractingTestcase. The test correctly validates SOAP call processing and response assertions.
- Removed unused `.body()` setups in `WSDLRewriterTutorialTest`. - Simplified test method names in `WSDLInterceptorTest` for clarity and consistency.
…ix' into soapproxy-wsdlinterceptor-yaml-fix
- Introduced `40-WSDL-Message-Validation.yaml` for demonstrating WSDL validation in SOAP proxies. - Added `invalid-city.soap.xml` as a sample invalid SOAP request payload for tutorial testing.
…ainst WSDL constraints - Added a new test class extending `AbstractCityServiceTest` for validating WSDL message conformance with SOAP requests. - Utilizes `40-WSDL-Message-Validation.yaml` and `invalid-city.soap.xml` for testing invalid scenarios.
…e`, and improve test coverage - Introduced `getProxyEndpointName` for extracting endpoint names from proxy paths. - Added `TestableSOAPProxy` and new test cases to validate WSDL path rewriting logic. - Simplified `addWSDLInterceptor` and `setPathRewriter` methods for improved readability. - Streamlined formatting and code consistency in `SOAPProxyTest`.
…ix' into soapproxy-wsdlinterceptor-yaml-fix
…ve test coverage - Extracted WSDL-related logic into `WSDLUtil` and introduced new helper methods like `rewriteRelativeWsdlPath` for better modularity. - Enhanced `SOAPProxy` initialization to streamline and centralize WSDL interceptor setup. - Added `WSDLUtilTest` for unit testing path rewriting logic. - Improved test coverage across WSDL-related test classes by adding and modifying cases in `SOAPProxyTest` and creating additional tests for `WSDLInterceptor`. - Simplified and refactored existing methods to improve readability and maintainability.
…d, and improve test readability - Removed redundant `hasContent` method from `RelocatingInterceptor`. - Simplified `@MCElement` annotation in `WSDLInterceptor` by removing `excludeFromFlow` attribute. - Improved readability and structure of assertions in `URLUtilTest`.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@core/src/main/java/com/predic8/membrane/core/interceptor/RelocatingInterceptor.java:
- Around line 46-49: RelocatingInterceptor currently calls
exc.getRequest().isGETRequest() without guarding against a null Request; update
the check in the method where this appears to first ensure exc.getRequest() !=
null before calling isGETRequest() (i.e., change the if to check
exc.getRequest() != null && !exc.getRequest().isGETRequest()), keep the same
debug log and return CONTINUE behavior when the condition matches, and ensure
the null-safe check follows the pattern used in
AdminRESTInterceptor/ReverseProxyingInterceptor/WebSocketExchangeWatcher/AdminApiInterceptor.
In
@core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java:
- Around line 226-228: WSDLInterceptor removed the setPort(String) API causing
test compilation failures; restore compatibility by adding a setPort(String)
overload in WSDLInterceptor that delegates to super.setPort(port) (e.g., public
void setPort(String port) { super.setPort(port); }) so existing callers in
WSDLInterceptorTest, ServiceProxyWSDLInterceptorsTest, and
SoapAndInternalProxyTest keep working without changing tests.
In @core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java:
- Around line 113-118: Fix the typo in the inline comment near the interceptor
setup: change "WebServiceExploroer" to "WebServiceExplorer" in the comment that
documents addWebServiceExplorer(), addWSDLPublisherInterceptor(),
addAndGetWSDLInterceptor(), and
setPathRewriterOnWSDLInterceptor(wsdlInterceptor) so the comment correctly reads
"Will be before WebServiceExplorer".
In @core/src/main/java/com/predic8/membrane/core/util/soap/WSDLUtil.java:
- Around line 17-21: The regex and replacement in rewriteRelativeWsdlPath are
unsafe: change the Pattern relativePathPattern to anchor a literal dot (e.g.,
"^\\./[^/?]*\\?") so it only matches "./..." prefixes, and when building the
replacement use Matcher.quoteReplacement on replacementName (e.g., construct
"./" + Matcher.quoteReplacement(replacementName) + "?") before calling
relativePathPattern.matcher(path).replaceAll(...) so any '$' or '\' in
replacementName are treated literally.
🧹 Nitpick comments (10)
distribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractCityServiceTest.java (1)
17-17: Consider using explicit imports instead of wildcards.While wildcard imports are functional, explicit imports improve code clarity and help avoid potential naming conflicts. However, this is a minor style preference and common in test code.
Example of explicit imports
-import org.junit.jupiter.api.*; +import org.junit.jupiter.api.Test; -import java.io.*; +import java.io.IOException;Also applies to: 19-19
core/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyWSDLPublisherInterceptorTest.java (2)
20-20: Consider using a specific import instead of wildcard.While wildcard static imports are functionally correct, using a specific import for
getPathFromResourcewould make dependencies more explicit and avoid potential naming conflicts.Suggested change
-import static com.predic8.membrane.test.TestUtil.*; +import static com.predic8.membrane.test.TestUtil.getPathFromResource;
21-21: Consider pinning the Rest Assured version.The library context indicates that Rest Assured's version is not specified in the pom.xml, which may lead to non-reproducible builds and potential compatibility issues as newer versions are released.
Based on learnings, ensure dependency versions are explicitly defined for build reproducibility.
core/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyTest.java (1)
21-21: Consider avoiding wildcard import in tests if it risks ambiguity.
import com.predic8.membrane.core.util.*;can accidentally introduce type/staticmethod ambiguity; if it’s only needed for one symbol, a specific import is clearer.core/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java (1)
43-43:finalmap prevents reassignment, but external callers can still mutate it viagetRelocatingAttributes().If the intent is to reduce mutability/misuse, consider returning an unmodifiable view (or exposing explicit methods for allowed mutations).
Proposed adjustment (only if callers don’t rely on mutating the returned map)
public Map<QName, String> getRelocatingAttributes() { - return relocatingAttributes; + return Collections.unmodifiableMap(relocatingAttributes); }Also applies to: 125-127
core/src/test/java/com/predic8/membrane/core/util/URLUtilTest.java (1)
74-81: Nice coverage forgetNameComponent(...)path variants.Optional: consider adding
"/"(and maybe"") expectations to pin root/empty-path behavior explicitly.core/src/main/java/com/predic8/membrane/core/util/soap/WSDLUtil.java (2)
31-38:getPort(List<Port>): consider null/empty handling (fail fast with a clearer message).Right now
ports == nullwill NPE;ports.isEmpty()falls through to the generic exception.Possible tweak
public static Port getPort(List<Port> ports) { + if (ports == null || ports.isEmpty()) + throw new IllegalArgumentException("No ports provided in WSDL."); Port port = getPortByNamespace(ports, WSDL_SOAP11_NS); if (port == null) port = getPortByNamespace(ports, WSDL_SOAP12_NS); if (port != null) return port; throw new IllegalArgumentException("No SOAP/1.1 or SOAP/1.2 ports found in WSDL."); }
40-58: Port inspection loop is solid; logging could include port identity for faster debugging.If
getPortByNamespacehits malformed bindings, including something like port name / binding QName in the warn would make triage easier.core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (1)
66-84: Consider including exception details in error log.The error log on line 80 doesn't include the exception message, which could make debugging harder when URL parsing fails.
Suggested improvement
} catch (URISyntaxException | MalformedURLException e) { - log.error("Cannot parse URL {}", path); + log.error("Cannot parse URL {}: {}", path, e.getMessage()); }core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java (1)
253-268: Duplicate path rewriting logic withWSDLInterceptor.generatePathRewriter.The lambda defined here is identical to the one in
WSDLInterceptor.generatePathRewriter(String keypath)(lines 72-84 ofWSDLInterceptor.java). Consider using the existing method to avoid duplication.Proposed simplification
private void setPathRewriterOnWSDLInterceptor(WSDLInterceptor wsdlInterceptor) { if (key.getPath() == null) return; - final String keyPath = key.getPath(); - wsdlInterceptor.setPathRewriter(path -> { - try { - if (path.contains("://")) { - return new URL(new URL(path), keyPath).toString(); - } - return rewriteRelativeWsdlPath(path, URLUtil.getNameComponent(router.getConfiguration().getUriFactory(), keyPath)); - } catch (URISyntaxException | MalformedURLException e) { - log.error("Cannot parse URL {}",path); - } - return path; - }); + wsdlInterceptor.setPathRewriterOnWSDLInterceptor(key.getPath()); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
core/src/main/java/com/predic8/membrane/core/http/Request.javacore/src/main/java/com/predic8/membrane/core/interceptor/RelocatingInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.javacore/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.javacore/src/main/java/com/predic8/membrane/core/util/URLUtil.javacore/src/main/java/com/predic8/membrane/core/util/soap/WSDLUtil.javacore/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.javacore/src/test/java/com/predic8/membrane/core/interceptor/WSDLInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyTest.javacore/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyWSDLPublisherInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/util/URLUtilTest.javacore/src/test/java/com/predic8/membrane/core/util/soap/WSDLUtilTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractCityServiceTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/SampleSOAPServiceTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/WSDLMessageValidationTutorialTest.java
✅ Files skipped from review due to trivial changes (2)
- core/src/test/java/com/predic8/membrane/core/util/soap/WSDLUtilTest.java
- core/src/main/java/com/predic8/membrane/core/http/Request.java
🚧 Files skipped from review as they are similar to previous changes (2)
- distribution/src/test/java/com/predic8/membrane/tutorials/soap/WSDLMessageValidationTutorialTest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-01-07T12:43:52.805Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2529
File: annot/src/main/java/com/predic8/membrane/annot/beanregistry/SpringContextAdapter.java:50-53
Timestamp: 2026-01-07T12:43:52.805Z
Learning: SpringContextAdapter in annot/src/main/java/com/predic8/membrane/annot/beanregistry/SpringContextAdapter.java is only used during tests, not in production code, so stub implementations throwing UnsupportedOperationException are acceptable.
Applied to files:
core/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyTest.java
📚 Learning: 2025-09-19T11:12:26.787Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.787Z
Learning: Request.get().path() in com.predic8.membrane.core.openapi.model.Request preserves the query string portion of the path, so URIFactory.createWithoutException(request.getPath()).getQuery() correctly extracts query parameters.
Applied to files:
core/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyWSDLPublisherInterceptorTest.javacore/src/main/java/com/predic8/membrane/core/util/URLUtil.java
📚 Learning: 2025-08-22T10:09:12.663Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/interceptor/ws_addressing/WsaEndpointRewriter.java:47-81
Timestamp: 2025-08-22T10:09:12.663Z
Learning: In WS-A endpoint rewriter and similar HTTP processing components, avoid closing XMLEventReader/XMLEventWriter when they wrap HTTP streams from keep-alive sessions, as this can interfere with HTTP connection management. Only flush the writer to ensure output is written.
Applied to files:
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.javacore/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java
📚 Learning: 2025-09-10T06:50:06.396Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2132
File: core/src/test/java/com/predic8/membrane/core/util/URITest.java:0-0
Timestamp: 2025-09-10T06:50:06.396Z
Learning: java.net.URI#getHost() returns IPv6 addresses WITH brackets included, not without brackets. For example, new URI("http://[2001:db8::1]:8080/foo").getHost() returns "[2001:db8::1]", not "2001:db8::1".
Applied to files:
core/src/test/java/com/predic8/membrane/core/util/URLUtilTest.java
📚 Learning: 2025-09-10T06:50:06.396Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2132
File: core/src/test/java/com/predic8/membrane/core/util/URITest.java:0-0
Timestamp: 2025-09-10T06:50:06.396Z
Learning: java.net.URI#getHost() returns IPv6 addresses WITH brackets included, not without brackets. According to Java documentation, IPv6 addresses in URIs "must be enclosed in square brackets" and getHost() returns the host component with brackets for IPv6. For example, new URI("http://[2001:db8::1]:8080/foo").getHost() returns "[2001:db8::1]", not "2001:db8::1".
Applied to files:
core/src/test/java/com/predic8/membrane/core/util/URLUtilTest.java
🧬 Code graph analysis (5)
core/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyWSDLPublisherInterceptorTest.java (1)
core/src/test/java/com/predic8/membrane/test/TestUtil.java (1)
TestUtil(29-61)
distribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractCityServiceTest.java (1)
core/src/main/java/com/predic8/membrane/core/Constants.java (1)
Constants(24-103)
core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java (2)
core/src/main/java/com/predic8/membrane/core/util/soap/WSDLUtil.java (1)
WSDLUtil(13-59)core/src/main/java/com/predic8/membrane/core/util/URLUtil.java (1)
URLUtil(19-55)
core/src/test/java/com/predic8/membrane/core/util/URLUtilTest.java (1)
core/src/main/java/com/predic8/membrane/core/util/URLUtil.java (1)
URLUtil(19-55)
⏰ 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 (18)
distribution/src/test/java/com/predic8/membrane/tutorials/soap/SampleSOAPServiceTutorialTest.java (1)
17-22: LGTM!Clean test class that follows the established tutorial test pattern. The implementation correctly extends the base test class and configures the specific tutorial YAML file.
distribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractCityServiceTest.java (2)
29-44: Well-structured WSDL endpoint test.The test properly validates the WSDL retrieval, content type, namespace presence, and importantly verifies that the service location is correctly rewritten to the proxy endpoint. The REST-assured pattern and XPath expression are appropriate for WSDL structure validation.
46-56: SOAP call test looks good.The test correctly reads the SOAP request body from the filesystem and validates the response structure. The relative path usage with
readFileFromBaseDir()should handle the file location properly based on the parent class infrastructure.core/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyWSDLPublisherInterceptorTest.java (1)
57-57: No action needed. The project requires Java 21 (configured inpom.xmland all CI/CD workflows), which fully supports theString.formatted()method introduced in Java 15. There is no compatibility issue.Likely an incorrect or invalid review comment.
core/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyTest.java (2)
96-103: RestAssured chain reformat looks good.Readability improved; semantics unchanged.
Also applies to: 97-102
111-114: ExpandedassertThrowslambda is clearer.This formatting makes the failing operations obvious and keeps the assertion scoped correctly.
core/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java (1)
57-72:getNewLocation(...)change is formatting-only.No concerns from this diff.
core/src/test/java/com/predic8/membrane/core/util/URLUtilTest.java (1)
31-36: Assertion argument order fix is good.Using
assertEquals(expected, actual)improves failure messages.core/src/main/java/com/predic8/membrane/core/util/URLUtil.java (1)
36-50: LGTM!The method is well-documented and correctly handles the edge cases (no slash returns entire path, trailing slash returns empty string). The implementation is concise and aligns with the test coverage mentioned in the AI summary.
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (1)
42-42: Verify the impact of removingexcludeFromFlow = true.The
@MCElementannotation change now includesWSDLInterceptorin the interceptor flow. Confirm that this behavioral change is intentional and that existing configurations won't be affected.core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java (4)
21-39: LGTM!Import changes align with the refactoring to use centralized
WSDLUtilfor port resolution and path rewriting.
89-95: LGTM!Good use of pattern matching for instanceof, improving readability while maintaining the same functionality.
244-251: LGTM!Clean implementation using
Optional.orElseGetto lazily create and register the interceptor only when needed.
241-241: LGTM!Good use of the centralized
WSDLUtil.getPortto handle SOAP 1.1/1.2 port selection consistently.core/src/test/java/com/predic8/membrane/core/interceptor/WSDLInterceptorTest.java (4)
41-53: LGTM!Using
DummyTestRouterprovides proper router context needed for the interceptor initialization. The visibility change to package-private aligns with JUnit 5 conventions.
63-125: LGTM!Test method renaming follows JUnit 5 conventions where the
testprefix is no longer required. Method names remain descriptive.
102-108: LGTM!Test correctly updated to use
intparameter matching the newsetPort(int)signature.
171-177: Verify intended behavior: query strings dropped for absolute URLs.Line 175 shows that
"http://api.predic8.de/service-b?WSDL"becomes"http://api.predic8.de/service-a"(query string dropped), while line 176 preserves the query for relative paths ("./service-b?WSDL"→"./service-a?WSDL"). This asymmetry is due tonew URL(context, spec)behavior.If WSDL location rewriting should preserve query strings for absolute URLs, the
generatePathRewriterlogic would need adjustment.
…terceptor setup - Deleted the redundant `setPathRewriterOnWSDLInterceptor` method from `SOAPProxy`. - Corrected a typo in method comments and variable usage (`WebServiceExploroer` -> `WebServiceExplorer`). - Enhanced `setPathRewriter` call to streamline WSDL interceptor setup. - Updated `URLUtilTest` to expand and clarify `getNameComponent` behavior. - Refined `WSDLUtil` regex usage for better consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
core/src/test/java/com/predic8/membrane/core/util/URLUtilTest.java (3)
22-26: Redundant static import (URLUtil.*already coversgetNameComponent).
The explicitimport static ...URLUtil.getNameComponent;is unnecessary givenimport static ...URLUtil.*;.Proposed diff
import static com.predic8.membrane.core.util.URLParamUtil.DuplicateKeyOrInvalidFormStrategy.*; import static com.predic8.membrane.core.util.URLParamUtil.*; import static com.predic8.membrane.core.util.URLUtil.*; -import static com.predic8.membrane.core.util.URLUtil.getNameComponent; import static org.junit.jupiter.api.Assertions.*;
30-38: Collapse repeatedhost()assertions into a small table/loop.
Same expected value across multiple inputs; a loop makes future edits less error-prone.Proposed diff
@Test void host() { - assertEquals("a", getHost("internal:a")); - assertEquals("a", getHost("internal://a")); - assertEquals("a", getHost("a")); - assertEquals("a", getHost("a/b")); - assertEquals("a", getHost("internal:a/b")); - assertEquals("a", getHost("internal://a/b")); + for (String in : new String[] { + "internal:a", + "internal://a", + "a", + "a/b", + "internal:a/b", + "internal://a/b" + }) { + assertEquals("a", getHost(in)); + } }
75-84:testGetNameComponent()coverage looks solid (empty, root, nested, trailing-slash).Optional: reuse a single URIFactory instance
@Test void testGetNameComponent() throws Exception { - assertEquals("", getNameComponent(new URIFactory(), "")); - assertEquals("", getNameComponent(new URIFactory(), "/")); - assertEquals("foo", getNameComponent(new URIFactory(), "foo")); - assertEquals("foo", getNameComponent(new URIFactory(), "/foo")); - assertEquals("bar", getNameComponent(new URIFactory(), "/foo/bar")); - assertEquals("bar", getNameComponent(new URIFactory(), "foo/bar")); - assertEquals("", getNameComponent(new URIFactory(), "foo/bar/")); + URIFactory uf = new URIFactory(); + assertEquals("", getNameComponent(uf, "")); + assertEquals("", getNameComponent(uf, "/")); + assertEquals("foo", getNameComponent(uf, "foo")); + assertEquals("foo", getNameComponent(uf, "/foo")); + assertEquals("bar", getNameComponent(uf, "/foo/bar")); + assertEquals("bar", getNameComponent(uf, "foo/bar")); + assertEquals("", getNameComponent(uf, "foo/bar/")); }core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (1)
29-29: Remove unused regex import.The
java.util.regex.*import appears to be unused in this file. The regex logic is contained withinWSDLUtil.rewriteRelativeWsdlPath, which is statically imported and called, butPatternorMatcherare not directly referenced here.♻️ Proposed fix
-import java.util.regex.*;core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java (1)
21-21: Consider specific import over wildcard.The import was changed from a specific
ValidatorInterceptorimport to a wildcardschemavalidation.*import. Unless multiple classes from this package are used, prefer specific imports for clarity.♻️ Proposed fix
-import com.predic8.membrane.core.interceptor.schemavalidation.*; +import com.predic8.membrane.core.interceptor.schemavalidation.ValidatorInterceptor;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.javacore/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.javacore/src/main/java/com/predic8/membrane/core/util/soap/WSDLUtil.javacore/src/test/java/com/predic8/membrane/core/util/URLUtilTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/util/soap/WSDLUtil.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T10:09:12.663Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/interceptor/ws_addressing/WsaEndpointRewriter.java:47-81
Timestamp: 2025-08-22T10:09:12.663Z
Learning: In WS-A endpoint rewriter and similar HTTP processing components, avoid closing XMLEventReader/XMLEventWriter when they wrap HTTP streams from keep-alive sessions, as this can interfere with HTTP connection management. Only flush the writer to ensure output is written.
Applied to files:
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java
🧬 Code graph analysis (3)
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (2)
core/src/main/java/com/predic8/membrane/core/util/soap/WSDLUtil.java (1)
WSDLUtil(14-62)core/src/main/java/com/predic8/membrane/core/util/URLUtil.java (1)
URLUtil(19-55)
core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java (1)
core/src/main/java/com/predic8/membrane/core/util/soap/WSDLUtil.java (1)
WSDLUtil(14-62)
core/src/test/java/com/predic8/membrane/core/util/URLUtilTest.java (1)
core/src/main/java/com/predic8/membrane/core/util/URLUtil.java (1)
URLUtil(19-55)
⏰ 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 (7)
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (2)
42-42: LGTM! Annotation change enables flow participation.Removing
excludeFromFlow = trueallows the WSDLInterceptor to participate in the interceptor chain. This aligns with the PR objective to integrate the interceptor into the flow, and is consistent with the new path rewriting functionality and theRESPONSE_FLOWsetting in the constructor.
66-84: URL rewriting logic is correct and thoroughly tested.The
generatePathRewriter()method at lines 72-84 correctly handles both absolute and relative path rewriting. The URL construction usingnew URL(new URL(path), keypath)properly rewrites URLs by replacing the path component while preserving protocol and host. This is validated by existing test coverage inWSDLInterceptorTest.generatePathRewriter():
- Absolute URL:
http://api.predic8.de/service-b→http://api.predic8.de/service-a- Absolute URL with query params:
http://api.predic8.de/service-b?WSDL→http://api.predic8.de/service-a- Relative path:
./service-b?WSDL→./service-a?WSDLThe
keypathparameter fromSOAPProxy.key.getPath()provides path values that work correctly with this logic.Error handling at line 80 appropriately logs at ERROR level for URL parsing failures.
core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java (5)
87-93: LGTM! Modernized loop structure.The refactored loop uses pattern matching with
instanceof, which is more concise and readable than the previous implementation. No functional changes.
111-116: LGTM! Clear interceptor initialization with helpful comments.The refactored interceptor initialization clearly documents the reverse-order insertion behavior (using
addFirstresults in last-added being first in chain). The final execution order will be: WSDLInterceptor → WSDLPublisherInterceptor → WebServiceExplorer → (existing validator).The addition of path rewriting configuration (line 116) integrates well with the new
WSDLInterceptorcapabilities.
239-239: LGTM! Centralized port selection logic.Delegating to
WSDLUtil.getPort(ports)eliminates code duplication and centralizes SOAP port selection logic. The utility method handles both SOAP 1.1 and 1.2 namespaces as the previous local implementation did.
242-249: LGTM! Well-structured lazy initialization.The
addAndGetWSDLInterceptormethod elegantly handles both cases (existing or new interceptor) usingOptional.orElseGet. This pattern ensures a singleWSDLInterceptorinstance exists and returns it for further configuration.
252-252: LGTM! Consistent use of type inference.Using
varfor local variables where the type is obvious from the initializer (constructor call) reduces verbosity and improves readability. This is consistent with other var usage in the file.Also applies to: 262-262
…ng, and improve test coverage - Introduced `90-Manual-SOAPProxy.yaml` for setting up and testing manual SOAP proxies. - Added `ManualSOAPProxyTutorialTest` to validate SOAP proxy setup and ensure WSDL compliance. - Enhanced `moveToFirstPosition` utility for efficient WSDL interceptor addition and management. - Updated multiple YAML tutorials and tests to align with new SOAP proxy configuration. - Added `normalizeCRLF` utility in `StringTestUtil` for handling CRLF line endings in HTTP messages.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
@core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java:
- Around line 225-227: WSDLInterceptor's setter changed signature to
setPort(int) which breaks callers passing Strings; add a backward-compatible
overload public void setPort(String port) in WSDLInterceptor that parses the
String and delegates to the int setter (e.g., parse with Integer.parseInt and
call the existing setPort(int)) so existing tests like SoapAndInternalProxyTest
and ServiceProxyWSDLInterceptorsTest continue to compile; keep the existing
setPort(int) implementation and ensure the new overload handles null/invalid
input consistently (e.g., throw NumberFormatException) to match previous
behavior.
In @core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java:
- Around line 113-118: The call to
wsdlInterceptor.setPathRewriterOnWSDLInterceptor(key.getPath()) silently skips
path rewriting when key.getPath() is null; update SOAPProxy (around the
wsdlInterceptor setup) to explicitly document this behavior and, to aid
debugging, perform a null check on key.getPath() and emit a debug/info log
stating that path rewriting is skipped when no path is configured (or leave a
clear inline comment if logging is undesired). Reference the SOAPProxy setup
block where addAndGetWSDLInterceptor() is called and the
wsdlInterceptor.setPathRewriterOnWSDLInterceptor(...) invocation, and mention
WSDLInterceptor's null-safe behavior in the comment or log message.
In @core/src/test/java/com/predic8/membrane/core/util/StringTestUtil.java:
- Around line 25-29: Update the JavaDoc on the StringTestUtil method that
normalizes test strings to CRLF so it accurately describes behavior: note that
the input parameter "s" may contain LF, CR, CRLF, or mixed line endings and the
method will normalize all of them to CRLF-terminated lines; keep the existing
@param and @return tags but change the description to reflect the broader
capability (accepts any line ending format and returns a CRLF-terminated HTTP
message).
In
@distribution/src/test/java/com/predic8/membrane/tutorials/soap/ManualSOAPProxyTutorialTest.java:
- Around line 32-45: In the soapCallInvalid test, replace the unconditional
.log().body() call with .log().ifValidationFails() to avoid noisy output, and
make the SOAP fault assertion more resilient by removing the fragile
body("Envelope.Body.Fault.faultstring", equalTo(...)) XPath check and instead
assert the response contains a fault indicator and the error token (e.g., assert
response body contains "faultstring" and still contains "INVALID"), keeping the
existing containsString("INVALID") check.
🧹 Nitpick comments (8)
core/src/test/java/com/predic8/membrane/core/http/ResponseTest.java (1)
304-329: LGTM! Solid test coverage for chunked responses with charset.The new nested test class correctly validates parsing of a chunked HTTP response with charset in the Content-Type header. The chunked encoding format is accurate (chunk size 0x17 = 23 bytes matches the body content), and the use of
normalizeCRLFensures proper CRLF line endings.Optional: Consider a more descriptive class name
While "RealResponses" works, a more specific name like
ChunkedResponseParsingorRealWorldChunkedResponsesmight better convey the test's focus. This is purely a readability nitpick and not essential.core/src/test/java/com/predic8/membrane/core/util/StringTestUtil.java (1)
30-37: LGTM with optional suggestions.The normalization logic is correct and handles all line ending variants (LF, CR, CRLF, and mixed) properly. The three-step approach is sound.
Optional improvements:
- Null handling: Consider adding a null check or documenting that null input is not supported.
🛡️ Optional null-safe implementation
public static String normalizeCRLF(String s) { + if (s == null) { + return null; + } // First normalize all possible line endings to LF String lf = s.replace("\r\n", "\n") .replace("\r", "\n");
- Test coverage: Since this is a utility method with non-trivial logic, consider adding unit tests to verify it correctly handles edge cases (empty strings, mixed line endings, consecutive line breaks, etc.).
Would you like me to generate test cases for this method?
core/src/main/java/com/predic8/membrane/core/interceptor/InterceptorUtil.java (1)
48-62: Consider edge case: null element in interceptors list.Line 51 checks
current != null, but the subsequent class equality check at line 51 assumes non-null. While the null check protects against NPE, if the list contains null elements, they will be silently skipped. Consider whether this is the intended behavior or if null elements should trigger an error.core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (2)
65-83: Silent error recovery in path rewriting may mask issues.The
generatePathRewritermethod catchesURISyntaxException | MalformedURLExceptionand logs an error, but then returns the original path. This silent recovery could mask configuration issues. Consider whether failing fast (throwing an exception) would be more appropriate during initialization, or at minimum, use a higher log level (warn/error).Also, line 79 logs the error but the message format has both
pathande.getMessage()- consider including the exception itself for full stack trace:log.error("Cannot parse URL {}", path, e);📝 Improved error handling
return path -> { try { if (path.contains("://")) { return new URL(new URL(path), keypath).toString(); } return rewriteRelativeWsdlPath(path, URLUtil.getNameComponent(router.getConfiguration().getUriFactory(), keypath)); } catch (URISyntaxException | MalformedURLException e) { - log.error("Cannot parse URL {} - {}", path,e.getMessage()); + log.error("Cannot parse URL {}", path, e); } return path; };
71-83: Package-private method with unclear visibility intention.The
generatePathRewritermethod is package-private but returns a public interface type (PathRewriter). This visibility seems intentional for testing, but consider adding a comment explaining why it's package-private or making it private if tests can work through the public API.core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java (2)
21-21: Avoid wildcard imports.The change from importing
ValidatorInterceptorspecifically to using a wildcard import (com.predic8.membrane.core.interceptor.schemavalidation.*) reduces code clarity. Explicit imports make dependencies clearer and avoid potential naming conflicts.♻️ Use explicit import
-import com.predic8.membrane.core.interceptor.schemavalidation.*; +import com.predic8.membrane.core.interceptor.schemavalidation.ValidatorInterceptor;
113-118: Verify interceptor ordering comment accuracy.The comment states interceptors are added "in reverse order" because they call
addFirst, suggesting the final order is: WSDLInterceptor (first), WSDLPublisher, WebServiceExplorer, Validator (last). However, the comment also mentions "there might be already a validator interceptor that must go last."If a validator already exists in the list when these are added, using
addFirstwill place all three new interceptors before the existing validator, which appears correct. But the phrasing "in reverse order" is slightly confusing—it might be clearer to state: "Added with addFirst to position them before any existing validator."📝 Clarified comment
- // Add interceptors (in reverse order) cause each one calls List.addFirst. - // This is needed because there might be already a validator interceptor that must go last + // Add interceptors using addFirst to position them before any existing validator. + // Final order: WSDLInterceptor, WSDLPublisher, WebServiceExplorer, [existing interceptors like Validator] addWebServiceExplorer(); // Will be last to validator addWSDLPublisherInterceptor(); // Will be before WebServiceExplorer var wsdlInterceptor = addAndGetWSDLInterceptor(); // WSDLInterceptor will be firstdistribution/tutorials/soap/90-Manual-SOAPProxy.yaml (1)
6-17: Optional: dedupe repeatedwsdlRewriterconfig to reduce drift.The same
wsdlRewriter { host, protocol, port }block appears twice; if YAML anchors are supported in this config format, using them would reduce copy/paste drift.Also applies to: 21-32
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
core/src/main/java/com/predic8/membrane/core/interceptor/InterceptorUtil.javacore/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.javacore/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.javacore/src/test/java/com/predic8/membrane/core/http/ResponseTest.javacore/src/test/java/com/predic8/membrane/core/util/StringTestUtil.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractCityServiceTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/ManualSOAPProxyTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/WSDLRewriterTutorialTest.javadistribution/tutorials/data/city.soap.xmldistribution/tutorials/soap/10-Sample-SOAP-Service.yamldistribution/tutorials/soap/20-SOAPProxy.yamldistribution/tutorials/soap/30-WSDL-Rewriter.yamldistribution/tutorials/soap/40-WSDL-Message-Validation.yamldistribution/tutorials/soap/90-Manual-SOAPProxy.yaml
✅ Files skipped from review due to trivial changes (1)
- distribution/tutorials/data/city.soap.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- distribution/tutorials/soap/20-SOAPProxy.yaml
- distribution/tutorials/soap/30-WSDL-Rewriter.yaml
- distribution/src/test/java/com/predic8/membrane/tutorials/soap/WSDLRewriterTutorialTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T10:09:12.663Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/interceptor/ws_addressing/WsaEndpointRewriter.java:47-81
Timestamp: 2025-08-22T10:09:12.663Z
Learning: In WS-A endpoint rewriter and similar HTTP processing components, avoid closing XMLEventReader/XMLEventWriter when they wrap HTTP streams from keep-alive sessions, as this can interfere with HTTP connection management. Only flush the writer to ensure output is written.
Applied to files:
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java
🧬 Code graph analysis (4)
core/src/test/java/com/predic8/membrane/core/http/ResponseTest.java (1)
core/src/test/java/com/predic8/membrane/core/util/StringTestUtil.java (1)
StringTestUtil(19-39)
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (2)
core/src/main/java/com/predic8/membrane/core/util/soap/WSDLUtil.java (1)
WSDLUtil(14-62)core/src/main/java/com/predic8/membrane/core/util/URLUtil.java (1)
URLUtil(19-55)
distribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractCityServiceTest.java (1)
core/src/main/java/com/predic8/membrane/core/Constants.java (1)
Constants(24-103)
core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java (2)
core/src/main/java/com/predic8/membrane/core/util/soap/WSDLUtil.java (1)
WSDLUtil(14-62)core/src/main/java/com/predic8/membrane/core/interceptor/InterceptorUtil.java (1)
InterceptorUtil(19-73)
⏰ 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 (13)
distribution/tutorials/soap/10-Sample-SOAP-Service.yaml (3)
1-5: LGTM: Documentation header simplified.The title and description changes make the tutorial more concise and clearly state its purpose as a minimal SOAP endpoint for development and debugging.
7-12: LGTM: Startup instructions are clear.The reformatted startup instructions maintain clarity while being more concise.
19-24: No changes needed. ThesampleSoapServiceis a valid, documented Membrane component (SampleSoapServiceInterceptor), and the configuration is already clearly documented with usage instructions at the top of the file.distribution/tutorials/soap/40-WSDL-Message-Validation.yaml (3)
17-18: Verify that the referenced data files exist at the specified paths.The tutorial instructions reference
../data/city.soap.xmland../data/invalid-city.soap.xml. Ensure these files exist relative to this YAML file's location.
20-24: Backend routing is correctly configured via WSDL.The
soapProxyautomatically derives the backend service endpoint from the<soap:address location="..."/>element in the WSDL document. The configuration requires only thewsdlparameter; thetargetendpoint is extracted during initialization by parsing the WSDL's service definitions, ensuring proper request routing without explicit backend configuration.
22-22: The external WSDL athttps://www.predic8.de/city-service?wsdlis the intended tutorial service. This pattern is consistent across all SOAP tutorials in the distribution, and the endpoint is stable and accessible. No changes are needed.Likely an incorrect or invalid review comment.
core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java (2)
241-241: LGTM: Port selection centralized in WSDLUtil.Delegating port selection to
WSDLUtil.getPort(ports)improves code maintainability by centralizing the SOAP port discovery logic. The method handles both SOAP 1.1 and 1.2 namespaces and provides clear error messages.
244-246: LGTM: Clean use of moveToFirstPosition utility.The
addAndGetWSDLInterceptor()method effectively usesmoveToFirstPositionto ensure the WSDLInterceptor is at the front of the chain, creating one if needed. The use oforElseThrow()is appropriate here since the supplier (WSDLInterceptor::new) will never return null.core/src/main/java/com/predic8/membrane/core/interceptor/InterceptorUtil.java (1)
43-71: No action required. The project is configured for Java 21 (javac.source=21, javac.target=21), making the use ofaddFirst()fully compatible and appropriate.core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (1)
41-41: Behavioral change: interceptor now configurable in standard flow.Removing
excludeFromFlow = truemeansWSDLInterceptorwill now be configurable as part of the standard interceptor flow (instead of being reserved for utility purposes). This is a significant architectural change and should align with the new path rewriting functionality. Verify this change is intentional and document the implications in deployment guides if needed.distribution/src/test/java/com/predic8/membrane/tutorials/soap/ManualSOAPProxyTutorialTest.java (1)
27-30: YAML selection override looks good.distribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractCityServiceTest.java (2)
29-44: WSDL rewrite assertion is clear and high-signal.
46-56: Set SOAP request Content-Type explicitly to avoid parsing/CI drift.
soapCall()posts XML without specifying a content-type; some stacks behave differently if it isn'ttext/xml; charset=utf-8(SOAP 1.1) /application/soap+xml(SOAP 1.2).Proposed change
@@ import java.io.*; import static com.predic8.membrane.core.Constants.WSDL_SOAP11_NS; +import static com.predic8.membrane.core.http.MimeType.TEXT_XML_UTF8; import static io.restassured.RestAssured.given; import static io.restassured.http.ContentType.XML; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ void soapCall() throws IOException { given() // File is read from FS use the same file as the user .body(readFileFromBaseDir("../data/city.soap.xml")) + .contentType(TEXT_XML_UTF8) .when() .post("http://localhost:2000/city-service") .then() .log().body() .body("Envelope.Body.getCityResponse.population", equalTo("34665600")); }
- Added `path` attribute to `WSDLInterceptor` for configurable WSDL rewrite paths. - Improved error logging and exception handling in `WSDLInterceptor`. - Updated `90-Manual-SOAPProxy.yaml` with refined tutorial structure and enhanced example configuration. - Clarified `normalizeCRLF` utility documentation. - Added new roadmap items for stream tracing improvements.
- Removed unnecessary `@Test` annotations from test methods. - Streamlined method definitions and improved readability.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @core/src/test/java/com/predic8/membrane/core/util/StringTestUtil.java:
- Around line 25-37: The normalizeCRLF method should validate its input and
ensure the returned message is CRLF-terminated: add a null check (e.g.,
Objects.requireNonNull(s, "s must not be null")) at the start of
StringTestUtil.normalizeCRLF and after normalizing line endings, ensure the
result ends with a trailing CRLF (append "\r\n" if it doesn't) so callers that
expect a CRLF-terminated HTTP message always get one; update the Javadoc to
state the parameter is non-null and that the return value is guaranteed to end
with CRLF.
🧹 Nitpick comments (4)
core/src/test/java/com/predic8/membrane/core/util/StringTestUtil.java (1)
21-23: MakeinputStreamFromcharset explicit to avoid platform-dependent tests.
string.getBytes()uses the platform default charset, which can make tests flaky across environments. ConsiderStandardCharsets.UTF_8(or the charset you want to model for HTTP bytes).Proposed change
import java.io.*; +import java.nio.charset.StandardCharsets; ... public static InputStream inputStreamFrom(String string) { - return new ByteArrayInputStream(string.getBytes()); + return new ByteArrayInputStream(string.getBytes(StandardCharsets.UTF_8)); }core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (2)
233-235: Type safety improvement with potential breaking change.Changing the parameter type from
Stringtointimproves type safety. Since this is annotated with@MCAttribute, it's primarily used by the configuration framework, which should handle the conversion correctly. However, if any code directly calls this method with aString, it will break at compile time.
237-251: LGTM: Standard getter/setter with good documentation.The path accessor methods are well-implemented. The javadoc on
setPathclearly explains the SOAPProxy integration context.📝 Optional: Add @return javadoc to getPath for completeness
public String getPath() { + /** @return The configured path for service location rewriting */ return path; }distribution/tutorials/soap/90-Manual-SOAPProxy.yaml (1)
22-22: Clarify the comment about interceptor ordering.The comment states "The wsdlRewriter must be the first of the following interceptors," but there's a
rewriterinterceptor before it on line 23. Consider rephrasing to "The wsdlRewriter must be the first WSDL-related interceptor" or "The wsdlRewriter must come before wsdlPublisher, webServiceExplorer, and validator" to avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.javacore/src/test/java/com/predic8/membrane/core/http/ResponseTest.javacore/src/test/java/com/predic8/membrane/core/util/StringTestUtil.javadistribution/tutorials/soap/90-Manual-SOAPProxy.yamldocs/ROADMAP.md
✅ Files skipped from review due to trivial changes (1)
- docs/ROADMAP.md
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/com/predic8/membrane/core/http/ResponseTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T10:09:12.663Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/interceptor/ws_addressing/WsaEndpointRewriter.java:47-81
Timestamp: 2025-08-22T10:09:12.663Z
Learning: In WS-A endpoint rewriter and similar HTTP processing components, avoid closing XMLEventReader/XMLEventWriter when they wrap HTTP streams from keep-alive sessions, as this can interfere with HTTP connection management. Only flush the writer to ensure output is written.
Applied to files:
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (2)
core/src/main/java/com/predic8/membrane/core/util/soap/WSDLUtil.java (1)
WSDLUtil(14-62)core/src/main/java/com/predic8/membrane/core/util/URLUtil.java (1)
URLUtil(19-55)
⏰ 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 (7)
core/src/main/java/com/predic8/membrane/core/interceptor/WSDLInterceptor.java (6)
21-21: LGTM: Imports support new path rewriting functionality.The new imports enable the path rewriting capabilities added in this PR.
Also applies to: 34-34
41-41: LGTM: Annotation change aligns with PR objectives.Removing
excludeFromFlow = trueenables this interceptor to participate in the normal processing flow, as intended.
54-57: LGTM: Field addition is well-documented.The new
pathfield is properly documented and follows standard patterns.
68-71: LGTM: Initialization logic is correctly placed.The conditional initialization in
init()ensures path rewriting is only configured when needed.
73-77: LGTM: Delegation method with proper null handling.The method correctly guards against null input and delegates to the path rewriter generator.
79-91: Remove the URL manipulation concern; verify exception handling design intent.The test at
WSDLInterceptorTest.java:172-177confirms the URL combination logic on lines 82-83 is correct. The test verifies thatnew URL(new URL("http://api.predic8.de/service-b"), "service-a")correctly produces"http://api.predic8.de/service-a", which shows thatkeypathproperly replaces the last path component when the input is an absolute URL.Regarding exception handling on line 88: The method wraps checked exceptions in
RuntimeExceptionafter logging the error. This is a fail-fast design choice appropriate for WSDL URL rewriting, where invalid URLs should not be silently ignored. However, no error case tests exist to verify whether this is the intended behavior for production scenarios or if error recovery would be beneficial.distribution/tutorials/soap/90-Manual-SOAPProxy.yaml (1)
50-56: Clarify that customflowdoes not fully override auto-addition of interceptors insoapProxy.The tutorial comments (lines 5-9) suggest that providing a custom
flowallows precise control over interceptors, but this is misleading. ThesoapProxyautomatically adds interceptors even when a custom flow is provided:
wsdlRewriter(WSDLInterceptor): Deduplicated—if present in custom flow, it's moved to the first position; otherwise a new one is createdwsdlPublisher(WSDLPublisherInterceptor): Added only if not already in custom flowwebServiceExplorer(WebServiceExplorerInterceptor): Always added, creating a new instance even if one exists in custom flowFor the custom flow shown here (lines 50-56), the resulting interceptor chain will be:
wsdlRewriter,wsdlPublisher,webServiceExplorer, thenvalidator—not just the two interceptors defined in the flow.To achieve precise control over the interceptor chain, the tutorial recommends (line 12-13) using an
apiinstead ofsoapProxy, as demonstrated in the earlier example. Consider adding a clarifying comment or updating the explanation to make this behavior explicit.
- Updated `StringTestUtil` to ensure consistent UTF-8 encoding in `inputStreamFrom`. - Refactored `WSDLRewriterTutorialTest` and related test URLs to dynamically use ports via `getPort()`. - Introduced two new test classes (`APIManualSOAPProxyTutorialTest` and `SoapProxyManualSOAPProxyTutorialTest`) for enhanced modularity and clarity. - Converted `ManualSOAPProxyTutorialTest` into an abstract class for better reusability.
- Removed redundant `getPort` method from `APIManualSOAPProxyTutorialTest` by using defaults from superclass. - Reordered assertion parameters in `ResponseTest` for consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
distribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractManualSOAPProxyTutorialTest.java (1)
32-46: Minor formatting inconsistency on line 44.There's an extra space before the closing parenthesis:
.body(containsString("INVALID") );Suggested fix
- .body(containsString("INVALID") ); + .body(containsString("INVALID"));core/src/test/java/com/predic8/membrane/core/http/ResponseTest.java (1)
78-83: Naming inconsistency:parseStartLine1vstestParseStartLine2/testParseStartLine3This method was renamed to
parseStartLine1, but the other two related methods retain thetestprefix (testParseStartLine2,testParseStartLine3). Consider using consistent naming across these related test methods.♻️ Suggested fix for consistent naming
Either keep the
testprefix:@Test -void parseStartLine1() throws IOException { +void testParseStartLine1() throws IOException {Or remove it from all three:
@Test -void testParseStartLine2() throws IOException { +void parseStartLine2() throws IOException { ... @Test -void testParseStartLine3() throws IOException { +void parseStartLine3() throws IOException {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/test/java/com/predic8/membrane/core/http/ResponseTest.javacore/src/test/java/com/predic8/membrane/core/util/StringTestUtil.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/APIManualSOAPProxyTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractManualSOAPProxyTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/SoapProxyManualSOAPProxyTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/soap/WSDLRewriterTutorialTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- distribution/src/test/java/com/predic8/membrane/tutorials/soap/WSDLRewriterTutorialTest.java
🧰 Additional context used
🧬 Code graph analysis (2)
distribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractManualSOAPProxyTutorialTest.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/http/ResponseTest.java (1)
core/src/test/java/com/predic8/membrane/core/util/StringTestUtil.java (1)
StringTestUtil(22-42)
🔇 Additional comments (8)
core/src/test/java/com/predic8/membrane/core/util/StringTestUtil.java (2)
18-20: LGTM! Explicit UTF-8 encoding is a good improvement.Using
UTF_8explicitly instead of the platform default charset ensures consistent behavior across different environments and prevents subtle encoding bugs in tests.Also applies to: 24-26
28-40: LGTM! Clean and correct line-ending normalization.The two-step approach (normalize to LF first, then convert to CRLF) correctly handles all line ending variants including mixed endings. The Javadoc clearly explains the purpose for HTTP message testing.
distribution/src/test/java/com/predic8/membrane/tutorials/soap/AbstractManualSOAPProxyTutorialTest.java (1)
25-30: LGTM!Good use of template method pattern with the abstract class. The
getTutorialYaml()override allows concrete subclasses to reuse the test logic while specifying different configurations.distribution/src/test/java/com/predic8/membrane/tutorials/soap/APIManualSOAPProxyTutorialTest.java (1)
17-21: LGTM!Simple concrete test class that inherits all behavior from the abstract parent. The JavaDoc clearly documents the port configuration.
distribution/src/test/java/com/predic8/membrane/tutorials/soap/SoapProxyManualSOAPProxyTutorialTest.java (1)
17-23: LGTM!Clean implementation that overrides only what's needed. Per the AI summary, port 2001 aligns with the YAML configuration in
90-Manual-SOAPProxy.yaml.core/src/test/java/com/predic8/membrane/core/http/ResponseTest.java (3)
78-203: Good practice: Package-private visibility for JUnit 5 tests.The visibility changes from
publicto package-private for all test methods align with JUnit 5 best practices, which no longer require test methods to be public.Note: The AI summary incorrectly states that
@Testannotations were stripped. All@Testannotations remain intact (visible on lines 78, 85, 92, 99, 110, 127, 139, 158, 168, 189, 195, 200), and these tests will continue to execute normally.
185-186: LGTM: Correct assertion parameter order.The assertion now correctly places the expected value (
0) before the actual value (res3.getBody().getContent().length), following JUnit conventions.
299-325: Well-structured test for chunked response with charset handling.The new
RealResponsesnested class with thechunkedCharsettest is well-implemented:
- Uses
StringTestUtil.normalizeCRLFto properly handle line endings for HTTP message parsing- Tests a realistic 404 chunked response with
charset=us-ascii- Validates status code, content-type header, and decoded body content
One minor consideration: the test data includes a
set-cookieheader with a date in January 2026. This is fine for test purposes but consider using a more generic/past date if this might cause confusion during code archaeology.
2 participants
… interceptor flow
blockRequestandblockResponseinAbstractProxy.log.debugmessages inWSDLPublisherInterceptor.SOAPProxyby addingWSDLPublisherInterceptorto the end.@MCElementforWSDLInterceptorto include it in the flow.formattedand repositioned imports fornotFound.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.