WsdlValidator: new WSDL Parser#2863
Merged
Merged
Conversation
- Introduced comprehensive WSDL parsing utilities (`Definitions`, `Binding`, `Service`, etc.). - Added schema parsing components with support for `Include` and `Import`. - Implemented tests for schema validation, WSDL definitions, services, bindings, and operations. - Enhanced SOAP version detection through `WSDLSOAPVersionExtractor`. - Refactored `AuthorizationService` and `ResolverMap` for improved URI handling.
…rser fields - Deleted `WSDLSOAPVersionExtractor` as it is no longer in use. - Replaced redundant mutable fields in WSDL parser classes with `final` fields to ensure immutability. - Simplified WSDL parser constructors by removing unused context (`ctx`) parameters. - Enhanced code readability by improving method references and streamlining operations. - Updated comments and removed obsolete code for cleaner structure.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLarge refactor replacing the old soa-model WSDL handling with a new DOM-based WSDL parser and schema model (Definitions, Service, Port, PortType, Binding, etc.), migrating validators to Element/DOM-based schemas, updating resolver/URI APIs, removing legacy WSDL utilities, and adding many tests and WSDL/XSD fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ValidatorInterceptor
participant Definitions
participant SchemaValidator
participant Exchange
rect rgba(200,200,255,0.5)
Client->>ValidatorInterceptor: handleRequest(Exchange)
ValidatorInterceptor->>Definitions: parse(resolver, wsdlLocation)
Definitions-->>ValidatorInterceptor: Definitions
ValidatorInterceptor->>SchemaValidator: init WSDLValidator(defs,...)
SchemaValidator->>Definitions: getSchemas()
Definitions-->>SchemaValidator: List<Element>
SchemaValidator->>SchemaValidator: create Validators from DOMSource
SchemaValidator->>Exchange: read message body
SchemaValidator->>SchemaValidator: run DOM validation
alt valid
SchemaValidator-->>ValidatorInterceptor: CONTINUE
else invalid
SchemaValidator-->>ValidatorInterceptor: ABORT + exceptions
ValidatorInterceptor->>Exchange: setErrorResponse(...)
end
ValidatorInterceptor-->>Client: response
end
sequenceDiagram
participant Parser as WSDLParser
participant Definitions
participant WSDLSchemaExtractor
participant SchemaDoc
rect rgba(200,255,200,0.5)
Parser->>Definitions: parse(resolver, location)
Definitions->>WSDLSchemaExtractor: getSchemas(wsdlElement)
WSDLSchemaExtractor->>SchemaDoc: clone schema Element -> Document
WSDLSchemaExtractor->>SchemaDoc: add missing xmlns declarations
SchemaDoc-->>Definitions: List<Document> (embedded schemas)
Definitions-->>Parser: enriched Definitions (with schema elements)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (10)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java (1)
32-34: Potentially redundant import ofABORT.Line 32 imports
Outcome.*which already includesABORT, making the explicit import on line 33 redundant.🧹 Remove redundant import
import static com.predic8.membrane.core.interceptor.Interceptor.Flow.*; import static com.predic8.membrane.core.interceptor.Outcome.*; -import static com.predic8.membrane.core.interceptor.Outcome.ABORT; import static com.predic8.membrane.core.util.text.TextUtil.*;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java` around lines 32 - 34, Remove the redundant explicit import of ABORT in ValidatorInterceptor.java: since the file already has a wildcard static import import static com.predic8.membrane.core.interceptor.Outcome.*; you should delete the duplicate import line import static com.predic8.membrane.core.interceptor.Outcome.ABORT; so only the wildcard remains and imports for ABORT (and other Outcome members) resolve via Outcome.*.core/src/test/java/com/predic8/membrane/core/util/wsdl/parser/OperationTest.java (1)
11-15: Consider adding negative test cases for completeness.The test verifies positive matches but doesn't confirm that
INPUT.matches("output")returnsfalse. Adding negative assertions would strengthen the test coverage and prevent accidental over-matching.💡 Suggested addition
`@Test` void direction() { assertTrue(INPUT.matches("input")); assertTrue(OUTPUT.matches("OutPut")); + assertFalse(INPUT.matches("output")); + assertFalse(OUTPUT.matches("input")); + assertFalse(INPUT.matches("unknown")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/util/wsdl/parser/OperationTest.java` around lines 11 - 15, The test method direction() in OperationTest currently only asserts positive matches; add negative assertions to ensure enums don't over-match by verifying INPUT.matches("output") is false and OUTPUT.matches("input") is false (i.e., add assertions that INPUT does not match the opposite string and OUTPUT does not match the opposite string) so both positive and negative cases are covered for the INPUT and OUTPUT match logic.core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/AbstractXMLSchemaValidator.java (1)
108-117: Log message may be misleading when multiple schemas exist.The debug log at line 113 always prints the same
location(the WSDL/main schema location) for every schema in the iteration, which could be confusing when debugging multiple embedded schemas.💡 Consider including schema index or identification
- for (var schema : getSchemas()) { - log.debug("Creating validator for schema: {}", location); + var schemas = getSchemas(); + for (int i = 0; i < schemas.size(); i++) { + var schema = schemas.get(i); + log.debug("Creating validator {}/{} for schema at: {}", i + 1, schemas.size(), location); validators.add(createValidator(schema, sf)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/AbstractXMLSchemaValidator.java` around lines 108 - 117, The debug log in createValidators() prints the same `location` for every schema, which is misleading when getSchemas() returns multiple entries; update the loop in createValidators() to log a unique identifier for each schema (e.g., use the loop index and/or the schema object's location or toString()) instead of the shared `location` variable so each call to log.debug identifies which schema is being processed before calling createValidator(schema, sf).core/src/test/java/com/predic8/membrane/core/util/soap/WSDLParserTest.java (1)
57-84: Minor: Inconsistent indentation at line 57.The
@Testannotation has an extra leading space compared to other test methods. This is a cosmetic issue.🔧 Formatting fix
- `@Test` + `@Test` void includeImport() throws Exception {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/util/soap/WSDLParserTest.java` around lines 57 - 84, The `@Test` annotation for the includeImport test in WSDLParserTest has an extra leading space causing inconsistent indentation; open the WSDLParserTest class, locate the includeImport() test method and align the annotation with other tests by removing the extra leading space before "@Test" so the annotation and method signature use the same indentation style as the surrounding tests.core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPMessageValidatorInterceptorTest.java (2)
137-137: Drop the temporary stdout dumps.These prints make the test output noisy and are easy to forget once the assertions are stable.
Also applies to: 152-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPMessageValidatorInterceptorTest.java` at line 137, Remove the temporary stdout dumps in the SOAPMessageValidatorInterceptorTest test class: delete the System.out.println(body) call (and the other print at the nearby location) so tests no longer print to stdout; locate the prints inside the SOAPMessageValidatorInterceptorTest test methods and remove those lines, keeping only assertions and necessary logging.
154-156: Assert the QName, not the collection rendering.
"[{...}sayHello]"couples this test toCollection.toString()formatting. Checking for"{http://www.examples.com/wsdl/HelloService.wsdl}sayHello"keeps the intent while avoiding formatter-dependent failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPMessageValidatorInterceptorTest.java` around lines 154 - 156, The test in SOAPMessageValidatorInterceptorTest currently asserts on the collection's toString form ("[{http://www.examples.com/wsdl/HelloService.wsdl}sayHello]") which ties it to Collection.toString() formatting; change the assertion to check for the QName string itself by asserting body.contains("{http://www.examples.com/wsdl/HelloService.wsdl}sayHello") instead, leaving the other assertions (e.g., body.contains("ValidateEmailRequest") and body.contains("is not a valid request element")) intact so the test verifies the QName without relying on collection rendering.core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/XMLSchemaValidator.java (1)
52-53: Return an empty list here instead ofnull.The current override works only as long as no superclass path ever touches
getSchemas(). Returningnullturns that assumption into a latent NPE;Collections.emptyList()keeps the “unused” behavior without breaking the contract.Proposed fix
`@Override` protected List<Element> getSchemas() { - return null; // never gets called + return Collections.emptyList(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/XMLSchemaValidator.java` around lines 52 - 53, The getSchemas() override in XMLSchemaValidator currently returns null which can cause a latent NPE if a superclass or other code ever invokes it; change the implementation of XMLSchemaValidator.getSchemas() to return Collections.emptyList() instead of null and add/import java.util.Collections if necessary so the method honors the contract while preserving the “never used” behavior.core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractorTest.java (1)
63-67: Fail fast when the fixture is missing.
getResourceAsStream()can returnnull; letting that flow into the parser makes fixture-path mistakes harder to diagnose than they need to be.Proposed fix
private static Definitions parseWsdl(InputStream is, String location) { + assertNotNull(is, "Missing WSDL test resource: " + location); var ctx = new WSDLParserContext(null, new ResolverMap(), location); var definitions = new Definitions(ctx); definitions.parse(is, null); return definitions; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractorTest.java` around lines 63 - 67, The test helper parseWsdl should fail fast when the InputStream is null to make missing fixture errors clear: add a null check at the start of parseWsdl(InputStream is, String location) and throw an informative runtime exception (e.g. IllegalArgumentException or AssertionError) that includes the location string, so any call that passed a null from getResourceAsStream() immediately reports "WSDL fixture not found" with the given location instead of letting Definitions.parse receive a null stream.core/src/test/resources/ws/include/xsd/inc/MessageStructureTypes2.xsd (1)
5-12: Make the imported schema observable in this fixture.Line 5 imports
DataTypes.xsd, but nothing else in this schema references that namespace. A broken import resolver would still parse this file successfully, so tests using this fixture will not actually catch that regression. Please bindduto an imported type or element so the cross-schema path is exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/resources/ws/include/xsd/inc/MessageStructureTypes2.xsd` around lines 5 - 12, The imported schema is not exercised because the element "du" is unbound; modify the "du" declaration to reference a type or element from the imported namespace "http://example.com/test/data-types" (for example by adding an xmlns prefix bound to that URI and setting du to type="prefix:SomeType" or ref="prefix:SomeElement") so the cross-schema import is actually used and the resolver is exercised.core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java (1)
55-61: Keep RPC and document-style port types separate.Line 59 adds every binding's port type to
portTypesDocument, including the ones already classified as RPC on Lines 55-58. That makes the document-style element extraction run over RPC bindings too.portTypesDocumentshould only collect non-RPC bindings.♻️ Proposed refactor
for (var binding : getBindings(definitions, serviceName)) { if (binding.getStyle() == RPC) { portTypesRPC.add(binding.getPortType()); + } else { + portTypesDocument.add(binding.getPortType()); } - portTypesDocument.add(binding.getPortType()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java` around lines 55 - 61, The loop in WSDLMessageElementExtractor that builds PortTypesByStyle incorrectly adds every binding's port type to portTypesDocument even when binding.getStyle() == RPC; update the loop in the method that calls getBindings(definitions, serviceName) so that after checking if (binding.getStyle() == RPC) and adding to portTypesRPC, you only add binding.getPortType() to portTypesDocument when binding.getStyle() != RPC (i.e., else branch or explicit check), ensuring PortTypesByStyle contains mutually exclusive portTypesRPC and portTypesDocument.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java`:
- Around line 67-75: getBindings currently wraps a null Service into
List.of(null) when definitions.getService(serviceName) returns null, causing an
NPE later; change the lookup so that when serviceName is non-null you retrieve
the Service once (Service svc = definitions.getService(serviceName)) and if svc
is null throw a clear exception (e.g. IllegalArgumentException or a custom
exception) stating the service was not found, otherwise use List.of(svc); keep
the rest of the stream pipeline (Port::getPorts, Port::getBinding) unchanged so
callers get an explicit, informative failure instead of an NPE.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLSchemaExtractor.java`:
- Around line 40-43: The extraction currently only copies xmlns* attributes from
the root WSDL element via getNamespaceDeclarations(wsdl), which loses namespace
bindings declared on intermediate ancestors (e.g., wsdl:types) and breaks QName
refs; update the logic in WSDLSchemaExtractor.extractSchema (and the similar
blocks at the other occurrences around 51-60 and 78-84) to walk the ancestor
chain from the originalSchema element up to the document root and merge all
in-scope xmlns* declarations into the cloned schema; replace the
single-root-only call to getNamespaceDeclarations(wsdl) with a routine that
collects namespace attributes from each ancestor element (including
originalSchema) before cloning so the resulting schema preserves all prefix→URI
bindings.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLValidator.java`:
- Around line 76-80: The catch block in WSDLValidator that throws a new
ConfigurationException when parsing fails should preserve the original parser
exception as the cause; modify the throw in the catch(Exception e) branch so the
new ConfigurationException is constructed/passed with e as its cause (similar to
how ResourceRetrievalException is preserved), and include the same formatted
message with location and e.getMessage() while passing e into the
ConfigurationException constructor.
In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Address.java`:
- Around line 9-11: The Address constructor is calling super(node) but must pass
the WSDLParserContext; update the Address(WSDLParserContext ctx, Node node)
constructor to call super(ctx, node) so it matches the
WSDLElement(WSDLParserContext ctx, Node node) signature and ensure the rest of
the constructor continues to use the passed ctx and node correctly.
In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Binding.java`:
- Around line 62-71: In Binding.java inside the loop that checks child elements,
avoid calling ctx.getDefinitions().addSoapVersion(soapVersion) when the binding
element is not a SOAP binding: only set soapVersion and call addSoapVersion when
the child's namespace matches WSDL_SOAP11_NS or WSDL_SOAP12_NS (i.e. when you
actually set soapVersion to SOAPVersion.SOAP_11 or SOAPVersion.SOAP_12); add a
null-check or move the ctx.getDefinitions().addSoapVersion(...) call inside
those namespace branches so non-SOAP extensions (like http:binding) do not push
a null soapVersion into Definitions.
- Around line 96-105: The code currently matches a portType only by its local
name (portTypeQName.getLocalPart()) against portType.getAttribute("name"), which
ignores namespaces and imported WSDLs; change the lookup to match the full QName
by also comparing the portType's namespace (use portTypeQName.getNamespaceURI())
against the portType's definitions targetNamespace (e.g.
portType.getOwnerDocument().getDocumentElement().getAttribute("targetNamespace")),
and when scanning use all available definition documents (not only
bindingElement.getOwnerDocument()) so imports are considered; update the loop
that finds the Element to check both local part and namespace before returning
new PortType(ctx, portType).
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/BindingOperation.java`:
- Around line 9-12: The constructor BindingOperation(WSDLParserContext ctx, Node
node) currently reads soapAction directly from the outer binding operation node
which can be null; instead traverse the child nodes of the given Node in the
BindingOperation constructor, find the SOAP extensibility element (local name
"operation" in the SOAP or SOAP12 namespace or elements named
"soap:operation"/"soap12:operation"), and if found read its "soapAction"
attribute safely (checking getAttributes()/getNamedItem() for null) and assign
to the soapAction field; if no SOAP child or no attribute exists leave
soapAction null/empty rather than dereferencing and crashing.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.java`:
- Around line 59-63: The Definitions.parse method currently calls
WSDLParserUtil.parse(resolver.resolve(location)) without closing the InputStream
returned by Resolver.resolve; wrap the resolved stream in a try-with-resources
(or ensure finally-close) so the stream is closed after parsing. Locate
Definitions.parse and change it to obtain the InputStream from
resolver.resolve(location), pass that stream to WSDLParserUtil.parse inside the
try-with-resources block, and then set up defs.ctx = new WSDLParserContext(defs,
resolver, location) and defs.parse(...) as before to prevent leaking file
descriptors or HTTP connections.
In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Message.java`:
- Around line 10-33: The code only stores the first wsdl:part in the Message
class (field part, constructor Message(WSDLParserContext, Node), and
getPart(Node)), which silently drops additional parts; change the implementation
to collect all parts into a List<Part> (introduce a private List<Part> parts and
a public List<Part> getParts()), update the constructor to populate parts by
iterating children and adding new Part(ctx, child) for every wsdl:part element,
and keep getPart() for backward compatibility but make it fail-fast (e.g., throw
IllegalStateException if parts.size() > 1 or return the single element) so
callers are not silently misled.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Operation.java`:
- Around line 81-99: The findMessage method currently matches only the local
name against elements in the current Document, which ignores the namespace in
messageQName and breaks resolution for imported or same-local-name messages;
update findMessage(QName messageQName, Document document) (and callers that
compute messageQName) to perform a QName-aware lookup: match both
messageQName.getLocalPart() and messageQName.getNamespaceURI() against
message.getAttribute("name") and the element's namespace (or xmlns
declarations), and if not found in the given document iterate the Definitions
model (or all imported Documents/definitions available in ctx) to locate the
message element by its full QName before constructing new Message(ctx, message);
ensure you use WSDL11_NS only for element namespace checks and not as a
substitute for the message's targetNamespace so imported messages are correctly
resolved.
In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Part.java`:
- Around line 12-15: The Part constructor currently calls the wrong superclass
constructor; update the call in Part(WSDLParserContext ctx, Node node) from
super(node) to super(ctx, node) so it matches the WSDLElement constructor
signature (passing both ctx and node), leaving the rest of the constructor that
sets this.element and this.type intact.
In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Port.java`:
- Around line 52-59: The match currently compares only the local part of
bindingQName to binding.getAttribute("name"), which can incorrectly match same
local names in different namespaces; instead resolve the binding element's own
name to a QName and compare full QNames (namespace URI + local part). Update the
Port code (where bindingQName is computed) to call
WSDLParserUtil.resolveQName(binding.getAttribute("name"), binding) to get
bindingQName2 and then check equality (either QName.equals or both
getNamespaceURI() and getLocalPart()) before returning new Binding(ctx,
binding).
In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/PortType.java`:
- Around line 30-34: The port-type parser currently accepts any element named
"operation" regardless of namespace, causing non-WSDL extensibility elements to
be treated as WSDL operations; restore the namespace guard in the PortType
parsing loop by checking that child.getNamespaceURI() equals WSDL11_NS (in the
same conditional that tests child.getLocalName()), so only elements in the
WSDL11 namespace are passed to new Operation(ctx, child); ensure the comparison
is null-safe (use WSDL11_NS.equals(child.getNamespaceURI())) and update the
conditional around Operation construction accordingly.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.java`:
- Around line 21-23: The resolved InputStream from
ctx.getResolver().resolve(combined) is never closed; wrap the resolution and
parsing in a try-with-resources (or equivalent try/finally) so the stream
returned by ctx.getResolver().resolve(combined) is closed after
WSDLParserUtil.parse(is) completes, then construct and return the Schema using
ctx.basePath(combined) and the parsed result; modify the code around
ResolverMap.combine, ctx.getResolver().resolve, WSDLParserUtil.parse, and the
Schema(...) call in AbstractIncludeImport to ensure the stream is always closed.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.java`:
- Around line 14-17: The parsed import currently ignores the declared namespace
string stored in variable "namespace" and blindly registers whatever schema is
returned by schema=getSchema(ctx); update the Import class so that after calling
getSchema(ctx) you retrieve the schema's target namespace (e.g.
schema.getTargetNamespace() or equivalent) and compare it to the "namespace"
attribute; if they differ (taking into account empty/null cases per XSD rules),
do not silently register it—either throw a clear parsing exception or log and
skip adding it (i.e., prevent ctx.getDefinitions().addImportedSchema(schema)
from running) so downstream QName resolution isn’t corrupted.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Include.java`:
- Around line 9-13: The constructor Include(WSDLParserContext ctx, Node node,
Schema schema) currently merges included schema elements unconditionally via
schema.getSchemaElements().addAll(getSchema(ctx).getSchemaElements()), which
allows foreign namespaces to be mixed; change this to first obtain the included
Schema (via getSchema(ctx)), compare its target namespace to the parent Schema's
target namespace (e.g., schema.getTargetNamespace() vs
included.getTargetNamespace()), and only merge elements when they are equal or
the included namespace is null/empty per xs:include rules; if they differ,
reject the include by throwing a parsing exception or logging an error and
skipping the merge to prevent cross-namespace mixing.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserUtil.java`:
- Around line 11-14: The parse(InputStream) method in WSDLParserUtil currently
uses a default DocumentBuilderFactory which permits external entity/DTD
resolution; replace its use with the existing hardened XML parsing approach
(reuse HardenedXmlParser or replicate its safe factory configuration) so
external entities, DTDs and remote resource resolution are disabled and secure
EntityResolver/URI resolution is used; ensure the method still returns the
DocumentElement (same signature) but builds/parses the Document via
HardenedXmlParser (or by applying the same secure features on
DocumentBuilderFactory) to prevent XXE/SSRF/file disclosure.
- Around line 23-27: WSDLParserUtil currently creates QNames from strings
without checking that context.lookupNamespaceURI(prefix) returned a non-null
namespace, which lets unbound prefixes produce QNames with null namespaces and
causes incorrect matches (e.g., in Operation.findMessage()); update the code
paths that build new QName(...) (both the colon-split branch that uses p[0]/p[1]
and the default/no-colon branch) to validate the looked-up namespace URI and, if
null, throw a clear exception (e.g., IllegalArgumentException or a
SAXParseException) identifying the unbound prefix and the offending value so
callers can fail-fast instead of creating a QName with a null namespace. Ensure
the checks reference context.lookupNamespaceURI(...) and QName creation sites in
WSDLParserUtil so both branches behave consistently.
In `@core/src/test/resources/validation/inline-anytype.wsdl`:
- Around line 2-5: The WSDL uses unqualified QNames for wsdl:part/@type (SKU and
string) which require namespace prefixes; update the root <definitions> element
to declare the XML Schema and IPO namespaces (e.g.,
xmlns:xsd="http://www.w3.org/2001/XMLSchema" and
xmlns:ipo="http://www.example.com/IPO") and then qualify the part type
references so the wsdl:part types resolve (use ipo:SKU for the SKU type defined
in the embedded schema and xsd:string for the built-in string type), ensuring
the definitions element and the wsdl:part/@type usages (the parts referenced
around lines 27 and 31) are spec-compliant.
In `@core/src/test/resources/ws/include/include.wsdl`:
- Around line 33-46: The WSDL contains a duplicate portType declaration named
TestPortType (with operation test and messages tns:TestRequest/TestResponse);
remove the second <portType name="TestPortType"> block (or move it to a
dedicated invalid-case fixture) so only one TestPortType definition remains,
ensuring the parser sees a single contract for TestPortType.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/AbstractXMLSchemaValidator.java`:
- Around line 108-117: The debug log in createValidators() prints the same
`location` for every schema, which is misleading when getSchemas() returns
multiple entries; update the loop in createValidators() to log a unique
identifier for each schema (e.g., use the loop index and/or the schema object's
location or toString()) instead of the shared `location` variable so each call
to log.debug identifies which schema is being processed before calling
createValidator(schema, sf).
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java`:
- Around line 32-34: Remove the redundant explicit import of ABORT in
ValidatorInterceptor.java: since the file already has a wildcard static import
import static com.predic8.membrane.core.interceptor.Outcome.*; you should delete
the duplicate import line import static
com.predic8.membrane.core.interceptor.Outcome.ABORT; so only the wildcard
remains and imports for ABORT (and other Outcome members) resolve via Outcome.*.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java`:
- Around line 55-61: The loop in WSDLMessageElementExtractor that builds
PortTypesByStyle incorrectly adds every binding's port type to portTypesDocument
even when binding.getStyle() == RPC; update the loop in the method that calls
getBindings(definitions, serviceName) so that after checking if
(binding.getStyle() == RPC) and adding to portTypesRPC, you only add
binding.getPortType() to portTypesDocument when binding.getStyle() != RPC (i.e.,
else branch or explicit check), ensuring PortTypesByStyle contains mutually
exclusive portTypesRPC and portTypesDocument.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/XMLSchemaValidator.java`:
- Around line 52-53: The getSchemas() override in XMLSchemaValidator currently
returns null which can cause a latent NPE if a superclass or other code ever
invokes it; change the implementation of XMLSchemaValidator.getSchemas() to
return Collections.emptyList() instead of null and add/import
java.util.Collections if necessary so the method honors the contract while
preserving the “never used” behavior.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPMessageValidatorInterceptorTest.java`:
- Line 137: Remove the temporary stdout dumps in the
SOAPMessageValidatorInterceptorTest test class: delete the
System.out.println(body) call (and the other print at the nearby location) so
tests no longer print to stdout; locate the prints inside the
SOAPMessageValidatorInterceptorTest test methods and remove those lines, keeping
only assertions and necessary logging.
- Around line 154-156: The test in SOAPMessageValidatorInterceptorTest currently
asserts on the collection's toString form
("[{http://www.examples.com/wsdl/HelloService.wsdl}sayHello]") which ties it to
Collection.toString() formatting; change the assertion to check for the QName
string itself by asserting
body.contains("{http://www.examples.com/wsdl/HelloService.wsdl}sayHello")
instead, leaving the other assertions (e.g.,
body.contains("ValidateEmailRequest") and body.contains("is not a valid request
element")) intact so the test verifies the QName without relying on collection
rendering.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractorTest.java`:
- Around line 63-67: The test helper parseWsdl should fail fast when the
InputStream is null to make missing fixture errors clear: add a null check at
the start of parseWsdl(InputStream is, String location) and throw an informative
runtime exception (e.g. IllegalArgumentException or AssertionError) that
includes the location string, so any call that passed a null from
getResourceAsStream() immediately reports "WSDL fixture not found" with the
given location instead of letting Definitions.parse receive a null stream.
In `@core/src/test/java/com/predic8/membrane/core/util/soap/WSDLParserTest.java`:
- Around line 57-84: The `@Test` annotation for the includeImport test in
WSDLParserTest has an extra leading space causing inconsistent indentation; open
the WSDLParserTest class, locate the includeImport() test method and align the
annotation with other tests by removing the extra leading space before "@Test"
so the annotation and method signature use the same indentation style as the
surrounding tests.
In
`@core/src/test/java/com/predic8/membrane/core/util/wsdl/parser/OperationTest.java`:
- Around line 11-15: The test method direction() in OperationTest currently only
asserts positive matches; add negative assertions to ensure enums don't
over-match by verifying INPUT.matches("output") is false and
OUTPUT.matches("input") is false (i.e., add assertions that INPUT does not match
the opposite string and OUTPUT does not match the opposite string) so both
positive and negative cases are covered for the INPUT and OUTPUT match logic.
In `@core/src/test/resources/ws/include/xsd/inc/MessageStructureTypes2.xsd`:
- Around line 5-12: The imported schema is not exercised because the element
"du" is unbound; modify the "du" declaration to reference a type or element from
the imported namespace "http://example.com/test/data-types" (for example by
adding an xmlns prefix bound to that URI and setting du to
type="prefix:SomeType" or ref="prefix:SomeElement") so the cross-schema import
is actually used and the resolver is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0e429e8-b5de-4cf7-96c3-d18e9c126dfc
📒 Files selected for processing (41)
annot/src/main/java/com/predic8/membrane/annot/Constants.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/AbstractXMLSchemaValidator.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLSchemaExtractor.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLValidator.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/XMLSchemaValidator.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Address.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Binding.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/BindingOperation.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Message.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Operation.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Part.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Port.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/PortType.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Service.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Type.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLElement.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserContext.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserUtil.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Include.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Schema.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/SchemaElement.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPMessageValidatorInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLSchemaExtractorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLValidatorTest.javacore/src/test/java/com/predic8/membrane/core/resolver/ResolverTest.javacore/src/test/java/com/predic8/membrane/core/util/soap/WSDLParserTest.javacore/src/test/java/com/predic8/membrane/core/util/wsdl/parser/OperationTest.javacore/src/test/resources/validation/inline-anytype.wsdlcore/src/test/resources/ws/include/include.wsdlcore/src/test/resources/ws/include/include.xsdcore/src/test/resources/ws/include/xsd/inc/MessageStructureTypes2.xsdcore/src/test/resources/ws/include/xsd/inc/shared/xsd/DataTypes.xsdcore/src/test/resources/ws/include/xsd/inc/shared/xsd/inc/BaseTypes.xsdcore/src/test/resources/ws/include/xsd/messages.xsd
…, and improved validation tests - Added cyclic import handling for WSDL and XSD files, ensuring robust schema parsing in complex configurations. - Enhanced `Schema` class to include logic for merging elements from included schemas. - Streamlined `WSDLParserContext` to track and prevent duplicate resource loading. - Improved schema validation test coverage with new test cases for cyclic and imported elements. - Minor refactorings in WSDL and schema-related classes for better readability and maintainability.
- Replaced mutable fields with `final` to ensure immutability in `WSDLParserContext` and related classes. - Simplified test method `getDefinitions` by removing intermediate variable. - Removed redundant null check on the `type` attribute in `Binding`. - Enhanced code clarity in `PortType` by finalizing the `operations` list.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (4)
core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Include.java (1)
8-12:⚠️ Potential issue | 🟠 MajorReject foreign-namespace
xs:includes before merging.This still merges whatever
getSchema(ctx)returned into the current schema.xs:includeonly permits the same target namespace, so a mismatched include can silently pollute QName resolution later.Proposed change
public Include(WSDLParserContext ctx, Node node, Schema referensingSchema) { super(ctx,node,referensingSchema); - // Copy all elements from the imported Schema into the importing - referensingSchema.include(schema); + if (schema == null) + return; + + var includedNamespace = schema.getTargetNamespace(); + var parentNamespace = referensingSchema.getTargetNamespace(); + if (includedNamespace != null && !includedNamespace.isEmpty() + && !java.util.Objects.equals(parentNamespace, includedNamespace)) { + throw new WSDLParserException( + "xs:include namespace mismatch: expected %s but got %s" + .formatted(parentNamespace, includedNamespace)); + } + + referensingSchema.include(schema); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Include.java` around lines 8 - 12, In Include's constructor (Include(WSDLParserContext ctx, Node node, Schema referensingSchema)) reject merging when the included Schema's target namespace differs from the referensingSchema: obtain the included Schema (the variable/schema produced by getSchema(ctx) or the existing 'schema' variable), compare included.getTargetNamespace() with referensingSchema.getTargetNamespace() treating null/empty as equal where appropriate, and if they differ throw or raise a parsing error (or skip merge) instead of calling referensingSchema.include(schema); only call referensingSchema.include(schema) when the namespaces match.core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Message.java (1)
13-16:⚠️ Potential issue | 🟠 MajorThis model still drops every
wsdl:partafter the first one.The constructor adds exactly one result from
getPart(node), andgetPart(node)returns on the first match. Multipart/RPC messages are still parsed incorrectly, and a message with no parts now stores a null entry.Proposed change
public Message(WSDLParserContext ctx, Node node) { super(ctx,node); - this.parts.add(getPart(node)); + collectParts(node); } @@ - private Part getPart(Node node) { + private void collectParts(Node node) { NodeList children = node.getChildNodes(); for (int i = 0; i < children.getLength(); i++) { Node child = children.item(i); if (child.getNodeType() == Node.ELEMENT_NODE && "part".equals(child.getLocalName()) && WSDL11_NS.equals(child.getNamespaceURI())) { - return new Part(ctx, child); + parts.add(new Part(ctx, child)); } } - return null; }Also applies to: 30-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Message.java` around lines 13 - 16, The constructor Message(WSDLParserContext ctx, Node node) currently adds only a single part by calling getPart(node), which causes multipart messages to be truncated and can insert a null when no parts exist; update the constructor to iterate over the node's child elements (or use an existing helper to find all "wsdl:part" children) and call getPart for each child, adding each non-null result to this.parts so every wsdl:part is preserved and no null entry is added (ensure you reference the Message constructor and the getPart(Node) method while making this change).core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Binding.java (2)
95-103:⚠️ Potential issue | 🟠 MajorResolve
portTypeby full QName, not local name in the current document.This still ignores
portTypeQName.getNamespaceURI()and never looks beyondbindingElement.getOwnerDocument(). Same-local-name contracts or imported WSDLs can bind to the wrongportTypeor to none at all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Binding.java` around lines 95 - 103, The code currently matches a portType only by local name in the current document; change the lookup to match the full QName and to search imported WSDLs as well: use WSDLParserUtil.resolveQName(...) result (portTypeQName) and when iterating portType elements compare both portTypeQName.getLocalPart() and portTypeQName.getNamespaceURI() against the candidate element’s namespace (e.g., element.getAttribute("name") for local and element.getNamespaceURI() or its containing definitions’ targetNamespace for namespace), and if not found in bindingElement.getOwnerDocument() then search imported/parsed documents available in the parser context (e.g., via any existing registry or by iterating import/include definitions reachable from bindingElement) so that new PortType(ctx, portType) is only constructed for a true full-QName match.
62-70:⚠️ Potential issue | 🟠 MajorOnly read
stylefrom SOAP binding extensions.This branch still matches any
*:binding. If anhttp:bindingor another extensibility binding appears after the SOAP one, it can overwrite the SOAP style withDOCUMENTor an unrelated value.Proposed change
- if (child.getNodeType() == ELEMENT_NODE - && "binding".equals(child.getLocalName())) { + if (child.getNodeType() == ELEMENT_NODE + && "binding".equals(child.getLocalName()) + && (WSDL_SOAP11_NS.equals(child.getNamespaceURI()) + || WSDL_SOAP12_NS.equals(child.getNamespaceURI()))) { style = getStyle(child); if (WSDL_SOAP11_NS.equals(child.getNamespaceURI())) { soapVersion = SOAPVersion.SOAP_11; ctx.getDefinitions().addSoapVersion(soapVersion); } else if (WSDL_SOAP12_NS.equals(child.getNamespaceURI())) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Binding.java` around lines 62 - 70, The code reads the binding "style" for any *:binding node which lets non-SOAP extensions (e.g., http:binding) overwrite SOAP styles; change the logic in Binding.java so you only call getStyle(child) when the binding element is in a SOAP namespace. Concretely, check child.getNamespaceURI() against WSDL_SOAP11_NS and WSDL_SOAP12_NS first (as in the existing branches that set soapVersion and call ctx.getDefinitions().addSoapVersion), and move the getStyle(child) call inside those SOAP-specific branches so style is assigned only for SOAP binding extensions and not for other binding namespaces.
🧹 Nitpick comments (5)
core/src/main/java/com/predic8/membrane/core/util/URIUtil.java (1)
147-148: Consider pre-compiling the regex pattern for better performance.
String.matches()compiles the pattern on every invocation. If this method is called frequently during WSDL parsing, consider using a pre-compiledPatternconstant.♻️ Suggested refactor
public class URIUtil { private static final Pattern driveLetterPattern = Pattern.compile("^(\\w)[/:|].*"); + private static final Pattern URI_SCHEME_PATTERN = Pattern.compile("^[a-zA-Z][a-zA-Z0-9+.-]*:.*"); // ... other methods ... public static String normalize(String location) { // ... - if (location.matches("^[a-zA-Z][a-zA-Z0-9+.-]*:.*")) { + if (URI_SCHEME_PATTERN.matcher(location).matches()) { return URI.create(location).normalize().toString(); } // ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/URIUtil.java` around lines 147 - 148, The call to String.matches in URIUtil is compiling the regex on each invocation; define a private static final Pattern (e.g., SCHEME_PATTERN) using Pattern.compile("^[a-zA-Z][a-zA-Z0-9+.-]*:.*") and replace the current location.matches(...) check with SCHEME_PATTERN.matcher(location).matches(); also add the required import for java.util.regex.Pattern. This pre-compiles the regex and improves performance when methods like the one containing this check are called frequently during WSDL parsing.core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/AbstractXMLSchemaValidator.java (1)
109-109: TODO: Property naming concern flagged.The TODO indicates uncertainty about the usage of the
"error"property and whether it should be renamed.Would you like me to search for usages of this property across the codebase and help determine if renaming is needed? I can also open an issue to track this cleanup task.
#!/bin/bash # Search for usages of the "error" property on Exchange objects rg -n '"error"' --type java rg -n 'getProperty.*error' --type java rg -n 'setProperty.*error' --type java🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/AbstractXMLSchemaValidator.java` at line 109, The line exc.setProperty("error", errorMsg) in AbstractXMLSchemaValidator.java uses a generic "error" property name; search the codebase for all reads/writes of the "error" property (look for getProperty/setProperty and literal "error") to determine if it is relied upon elsewhere, and if it is not or if the name is ambiguous, rename the property to a clearer identifier (e.g. "schemaValidationError") by updating the setProperty call in AbstractXMLSchemaValidator and all corresponding getProperty/setProperty usages and tests to the new name; ensure any external consumers (logs, exchanges, handlers) are adjusted and run tests to verify no regressions.core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserException.java (1)
3-6: Add a cause-preserving constructor before this becomes the parser-wide error type.Several parsing paths already handle lower-level XML/IO failures. With only the message overload, they cannot rethrow
WSDLParserExceptionwithout discarding the original cause.Proposed change
public class WSDLParserException extends RuntimeException { public WSDLParserException(String message) { super(message); } + + public WSDLParserException(String message, Throwable cause) { + super(message, cause); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserException.java` around lines 3 - 6, Add a cause-preserving constructor to WSDLParserException so parsing code can rethrow while preserving the original Throwable; specifically, in class WSDLParserException add a constructor WSDLParserException(String message, Throwable cause) that calls super(message, cause) (optionally also add WSDLParserException(Throwable cause) that calls super(cause)) so existing catch blocks can wrap lower-level XML/IO exceptions without losing the original cause.core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.java (1)
66-71: Don't eraseWSDLParserExceptionbehind a genericRuntimeException.This catches the new parser-specific failures from schema imports and collapses them into “Could not parse WSDL”, which throws away the semantic error callers just gained.
Proposed change
public void parse(InputStream is, Resolver resolver) { try { ctx = ctx.resolver(resolver); parse(WSDLParserUtil.parse(is)); + } catch (WSDLParserException e) { + throw e; } catch (Exception e) { throw new RuntimeException("Could not parse WSDL", e); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.java` around lines 66 - 71, The current Definitions.parse(InputStream, Resolver) method catches all Exceptions and wraps them in a generic RuntimeException, which discards WSDLParserException details; change the exception handling in Definitions.parse so that WSDLParserException (the parser-specific exception) is caught separately and rethrown unchanged (or propagated) while other Exceptions can still be wrapped with the existing "Could not parse WSDL" RuntimeException; locate the parse method in class Definitions and add a catch (WSDLParserException e) { throw e; } (or remove WSDLParserException from the blanket catch) before the generic catch to preserve the original semantic error.core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Schema.java (1)
41-50: Consider returning read-only views from the public collection getters.These methods expose the live parser state. Since
Schemais public, callers can add/remove elements, imports, or includes after construction and change parser behavior from the outside.♻️ Possible tightening
public List<SchemaElement> getSchemaElements() { - return schemaElements; + return Collections.unmodifiableList(schemaElements); } public List<Import> getImports() { - return imports; + return Collections.unmodifiableList(imports); } public List<Include> getIncludes() { - return includes; + return Collections.unmodifiableList(includes); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Schema.java` around lines 41 - 50, The public getters in Schema (getSchemaElements, getImports, getIncludes) currently expose mutable internal lists; change them to return read-only views (e.g., wrap with an unmodifiable view or return an immutable copy) so external callers cannot mutate the parser's internal state while keeping the class's internal lists mutable for internal updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java`:
- Around line 27-30: In WSDLMessageElementExtractor, don't assume service-less
WSDLs are document style by setting portTypes = new
PortTypesByStyle(Collections.emptyList(), definitions.getPortTypes()); instead
preserve binding-style detection and serviceName validation by invoking
getPortTypesByStyle(definitions, serviceName) (the same path used when services
exist) so binding information is retained and getServices(...) validation is not
bypassed; update the branch that checks definitions.getServices().isEmpty() to
delegate to getPortTypesByStyle and ensure any serviceName validation still
runs.
- Around line 33-36: operationNamesRPC currently synthesizes QNames for every
RPC operation regardless of whether the operation has a message in the requested
direction, causing one-way operations to produce invalid request/response
QNames; to fix, filter the stream of operations returned by
portTypes.portTypesRPC() to include only operations that have a message in the
given direction before calling getElementNameRPC(op, direction) and constructing
the QName (e.g., check op.getInput()/op.getOutput() vs the direction or add a
helper like hasMessageInDirection(op, direction)), then map the remaining
operations to new QName(definitions.getTargetNamespace(), getElementNameRPC(op,
direction)) as before.
In `@core/src/main/java/com/predic8/membrane/core/util/URIUtil.java`:
- Around line 142-153: URIUtil.normalize misclassifies Windows absolute paths
like "C:\foo\bar" as URI schemes because the current scheme regex matches "C:";
before treating location as a URI in normalize, first detect Windows
drive-letter paths (e.g., match "^[a-zA-Z]:[\\/].*") and treat those as
filesystem paths by returning
Path.of(location).toAbsolutePath().normalize().toString(); only if the
Windows-drive check fails, proceed to the existing scheme regex and use
URI.create(location).normalize().toString() for true URIs.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.java`:
- Around line 21-24: The fields messages and bindings in Definitions are left
uninitialized and can be null; initialize them to empty lists to match the other
collections. Update the Definitions class to set List<Message> messages = new
ArrayList<>(); and List<Binding> bindings = new ArrayList<>(); so the
getMessages()/getBindings() callers never receive null; adjust any constructors
if they overwrite these fields to preserve non-null semantics.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.java`:
- Around line 46-47: The resolve(WSDLParserContext ctx) method currently calls
ResolverMap.combine(...) and URIUtil.normalize(...) even when schemaLocation is
blank (getAttribute() returns ""), causing a silent self-reference; modify
resolve to explicitly check for null/empty schemaLocation (and likewise any
similar logic around lines 54-57) and return null or throw a clear parsing error
before calling ResolverMap.combine/URIUtil.normalize so missing schemaLocation
is handled explicitly; reference the resolve(WSDLParserContext ctx) method and
the schemaLocation field to locate and change the guard.
- Around line 30-39: The current implemention in AbstractIncludeImport (around
resolve(), ctx.getVisitedLocations(), registerLocation(...) and the Schema
creation that calls new Schema(ctx.basePath(resolved),
WSDLParserUtil.parse(is))) treats visitedLocations as a global de-dup and
returns null for already-seen locations; change it so visitedLocations is used
only as a cycle guard (a per-resolution stack) or, alternatively, keep a cache
of already-parsed Schema objects and return the cached Schema instead of null.
Concretely: when resolving `resolved`, first check a parse-cache keyed by
`resolved` and return the cached Schema if present; otherwise push `resolved`
onto a per-resolution visited stack (or registerLocation) and ensure you
pop/unregister it in a finally block to avoid leaking state, then parse with
WSDLParserUtil.parse(is), store the created Schema in the cache, and return it
(not null). Ensure the code paths that previously short-circuited to null now
return the cached Schema or proceed after cycle-guard pop.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.java`:
- Around line 22-23: The exception message in Import.java uses mismatched
placeholders so schema.getTargetNamespace() is never interpolated; update the
WSDLParserException call in the block that checks schema.getTargetNamespace() to
use matching format placeholders (e.g., use two %s or String.format style) so
both namespace and schema.getTargetNamespace() are included in the message;
locate the throw statement that constructs the WSDLParserException and replace
the template so the second argument is actually rendered (reference: method/file
Import.java and symbol schema.getTargetNamespace()).
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserContext.java`:
- Around line 26-28: basePath(...) currently mutates the shared visitedLocations
before parsing, poisoning the set on failures; change WSDLParserContext.basePath
to not modify the existing list but instead create a new copy, add basePath to
that copy, and pass the copy into the constructor (i.e. create newVisited = new
ArrayList<>(visitedLocations); newVisited.add(basePath); return new
WSDLParserContext(definitions, resolver, basePath, newVisited)); this preserves
the original visitedLocations if parsing later fails.
In `@core/src/test/resources/ws/import/cyclic.wsdl`:
- Around line 1-63: Add a new unit test in WSDLParserTest (mirroring
includeImport()) called includeImportCyclic that parses
"classpath://ws/import/cyclic.wsdl" using the same parser helper used by
includeImport(), walks the returned AbstractIncludeImport/schema graph (calling
getSchema() where appropriate) and asserts the traversal terminates (no infinite
recursion) and that the expected imported schemas/namespaces are present (e.g.,
the schema for the cyclic import targets). This test should explicitly exercise
the cycle guard in AbstractIncludeImport.getSchema() so the import walk
completes and the import/schema relationships are asserted.
In `@core/src/test/resources/ws/include/multiple.wsdl`:
- Around line 8-19: The elements "test" and "testResponse" are currently untyped
so the WSDL does not actually constrain payloads against the included schema
multiple/a.xsd; update the xs:element declarations for test and testResponse to
consume types/elements defined in the included schema (for example by adding a
type attribute or using a ref to the element/type declared in multiple/a.xsd,
e.g. set test to refer to the included schema's element/type and likewise for
testResponse) so the fixture enforces the include chain; modify the xs:element
entries named "test" and "testResponse" to reference the included schema's
types/elements.
---
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Binding.java`:
- Around line 95-103: The code currently matches a portType only by local name
in the current document; change the lookup to match the full QName and to search
imported WSDLs as well: use WSDLParserUtil.resolveQName(...) result
(portTypeQName) and when iterating portType elements compare both
portTypeQName.getLocalPart() and portTypeQName.getNamespaceURI() against the
candidate element’s namespace (e.g., element.getAttribute("name") for local and
element.getNamespaceURI() or its containing definitions’ targetNamespace for
namespace), and if not found in bindingElement.getOwnerDocument() then search
imported/parsed documents available in the parser context (e.g., via any
existing registry or by iterating import/include definitions reachable from
bindingElement) so that new PortType(ctx, portType) is only constructed for a
true full-QName match.
- Around line 62-70: The code reads the binding "style" for any *:binding node
which lets non-SOAP extensions (e.g., http:binding) overwrite SOAP styles;
change the logic in Binding.java so you only call getStyle(child) when the
binding element is in a SOAP namespace. Concretely, check
child.getNamespaceURI() against WSDL_SOAP11_NS and WSDL_SOAP12_NS first (as in
the existing branches that set soapVersion and call
ctx.getDefinitions().addSoapVersion), and move the getStyle(child) call inside
those SOAP-specific branches so style is assigned only for SOAP binding
extensions and not for other binding namespaces.
In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Message.java`:
- Around line 13-16: The constructor Message(WSDLParserContext ctx, Node node)
currently adds only a single part by calling getPart(node), which causes
multipart messages to be truncated and can insert a null when no parts exist;
update the constructor to iterate over the node's child elements (or use an
existing helper to find all "wsdl:part" children) and call getPart for each
child, adding each non-null result to this.parts so every wsdl:part is preserved
and no null entry is added (ensure you reference the Message constructor and the
getPart(Node) method while making this change).
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Include.java`:
- Around line 8-12: In Include's constructor (Include(WSDLParserContext ctx,
Node node, Schema referensingSchema)) reject merging when the included Schema's
target namespace differs from the referensingSchema: obtain the included Schema
(the variable/schema produced by getSchema(ctx) or the existing 'schema'
variable), compare included.getTargetNamespace() with
referensingSchema.getTargetNamespace() treating null/empty as equal where
appropriate, and if they differ throw or raise a parsing error (or skip merge)
instead of calling referensingSchema.include(schema); only call
referensingSchema.include(schema) when the namespaces match.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/AbstractXMLSchemaValidator.java`:
- Line 109: The line exc.setProperty("error", errorMsg) in
AbstractXMLSchemaValidator.java uses a generic "error" property name; search the
codebase for all reads/writes of the "error" property (look for
getProperty/setProperty and literal "error") to determine if it is relied upon
elsewhere, and if it is not or if the name is ambiguous, rename the property to
a clearer identifier (e.g. "schemaValidationError") by updating the setProperty
call in AbstractXMLSchemaValidator and all corresponding getProperty/setProperty
usages and tests to the new name; ensure any external consumers (logs,
exchanges, handlers) are adjusted and run tests to verify no regressions.
In `@core/src/main/java/com/predic8/membrane/core/util/URIUtil.java`:
- Around line 147-148: The call to String.matches in URIUtil is compiling the
regex on each invocation; define a private static final Pattern (e.g.,
SCHEME_PATTERN) using Pattern.compile("^[a-zA-Z][a-zA-Z0-9+.-]*:.*") and replace
the current location.matches(...) check with
SCHEME_PATTERN.matcher(location).matches(); also add the required import for
java.util.regex.Pattern. This pre-compiles the regex and improves performance
when methods like the one containing this check are called frequently during
WSDL parsing.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.java`:
- Around line 66-71: The current Definitions.parse(InputStream, Resolver) method
catches all Exceptions and wraps them in a generic RuntimeException, which
discards WSDLParserException details; change the exception handling in
Definitions.parse so that WSDLParserException (the parser-specific exception) is
caught separately and rethrown unchanged (or propagated) while other Exceptions
can still be wrapped with the existing "Could not parse WSDL" RuntimeException;
locate the parse method in class Definitions and add a catch
(WSDLParserException e) { throw e; } (or remove WSDLParserException from the
blanket catch) before the generic catch to preserve the original semantic error.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Schema.java`:
- Around line 41-50: The public getters in Schema (getSchemaElements,
getImports, getIncludes) currently expose mutable internal lists; change them to
return read-only views (e.g., wrap with an unmodifiable view or return an
immutable copy) so external callers cannot mutate the parser's internal state
while keeping the class's internal lists mutable for internal updates.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserException.java`:
- Around line 3-6: Add a cause-preserving constructor to WSDLParserException so
parsing code can rethrow while preserving the original Throwable; specifically,
in class WSDLParserException add a constructor WSDLParserException(String
message, Throwable cause) that calls super(message, cause) (optionally also add
WSDLParserException(Throwable cause) that calls super(cause)) so existing catch
blocks can wrap lower-level XML/IO exceptions without losing the original cause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f694608-3d8d-4487-9fb8-500651f6e8b0
📒 Files selected for processing (39)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/AbstractXMLSchemaValidator.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.javacore/src/main/java/com/predic8/membrane/core/util/SOAPUtil.javacore/src/main/java/com/predic8/membrane/core/util/URIUtil.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Address.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Binding.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/BindingOperation.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Message.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Part.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/PortType.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Type.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserContext.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserException.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Include.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Schema.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/SchemaElement.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPMessageValidatorInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractorTest.javacore/src/test/java/com/predic8/membrane/core/util/soap/WSDLParserTest.javacore/src/test/resources/validation/inline-anytype.wsdlcore/src/test/resources/ws/import/cyclic.wsdlcore/src/test/resources/ws/import/cyclic/a.xsdcore/src/test/resources/ws/import/cyclic/b.xsdcore/src/test/resources/ws/import/message-references-import.wsdlcore/src/test/resources/ws/import/messages.xsdcore/src/test/resources/ws/include/cyclic.wsdlcore/src/test/resources/ws/include/cyclic/a.xsdcore/src/test/resources/ws/include/cyclic/b.xsdcore/src/test/resources/ws/include/include.wsdlcore/src/test/resources/ws/include/multiple.wsdlcore/src/test/resources/ws/include/multiple/a.xsdcore/src/test/resources/ws/include/multiple/b.xsdcore/src/test/resources/ws/include/multiple/c.xsdcore/src/test/resources/ws/include/xsd/inc/MessageStructureTypes2.xsdcore/src/test/resources/ws/include/xsd/inc/shared/xsd/inc/BaseTypes.xsdcore/src/test/resources/ws/include/xsd/messages2.xsd
✅ Files skipped from review due to trivial changes (5)
- core/src/test/resources/ws/include/multiple/c.xsd
- core/src/test/resources/ws/import/cyclic/a.xsd
- core/src/test/resources/ws/import/message-references-import.wsdl
- core/src/test/resources/ws/include/cyclic/a.xsd
- core/src/test/resources/ws/import/messages.xsd
🚧 Files skipped from review as they are similar to previous changes (10)
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/SchemaElement.java
- core/src/test/resources/ws/include/xsd/inc/MessageStructureTypes2.xsd
- core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractorTest.java
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Address.java
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/BindingOperation.java
- core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPMessageValidatorInterceptorTest.java
- core/src/test/resources/ws/include/xsd/inc/shared/xsd/inc/BaseTypes.xsd
- core/src/test/java/com/predic8/membrane/core/util/soap/WSDLParserTest.java
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Part.java
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/PortType.java
…ing, and message handling - Added robust path normalization in `URIUtil`, covering relative paths, absolute paths, and various URI schemes. - Enhanced `WSDLParser` to track and validate immutable definitions, bindings, and messages. - Introduced support for handling empty schema locations and improved error handling in cyclic imports. - Updated tests to cover added functionality, ensuring reliable parsing and normalization across platforms.
|
This pull request needs "/ok-to-test" from an authorized committer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.java (1)
140-148: Misleading variable name in generic helper method.The variable
serviceson line 141 is confusing since this is a generic method that handles any element name (e.g., "service", "portType", etc.). Consider renaming toelementsorresultfor clarity.private static List<Element> getElements(Element wsdl, String name) { - var services = new ArrayList<Element>(); + var elements = new ArrayList<Element>(); var list = wsdl.getElementsByTagNameNS(WSDL11_NS, name); for (int i = 0; i < list.getLength(); i++) { - services.add((Element) list.item(i)); + elements.add((Element) list.item(i)); } - return services; + return elements; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.java` around lines 140 - 148, In getElements(Element wsdl, String name) rename the misleading local variable services to a neutral name like elements or result to reflect that the method returns any element type (not just services); update all references in that method (the ArrayList<> instantiation and the return statement) so the new variable name replaces services while leaving method signature getElements and the use of WSDL11_NS and list unchanged.core/src/test/java/com/predic8/membrane/core/util/soap/WSDLParserTest.java (1)
63-65: Inconsistent classpath URI format.Line 65 uses
"classpath://ws/include/include.wsdl"(double slash), while lines 18 and 89 use single slash format"classpath:/ws/...". Both work due toClasspathSchemaResolver.normalize()stripping extra slashes, but standardizing on a single format would improve consistency and readability.- var definitions = Definitions.parse(new ResolverMap(), "classpath://ws/include/include.wsdl"); + var definitions = Definitions.parse(new ResolverMap(), "classpath:/ws/include/include.wsdl");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/util/soap/WSDLParserTest.java` around lines 63 - 65, Update the inconsistent classpath URI in the WSDLParserTest includeImport test: replace the double-slash form used in the Definitions.parse call ("classpath://ws/include/include.wsdl") with the single-slash standard ("classpath:/ws/include/include.wsdl") to match the other tests; locate the call in the includeImport method in WSDLParserTest where Definitions.parse(new ResolverMap(), ...) is invoked and make the URI change for consistency.core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.java (1)
18-18: Redundant assignment;referensingSchemais already set by the parent constructor.Looking at
AbstractIncludeImport's constructor,this.referensingSchemais assigned beforesuper()returns. This line can be safely removed.♻️ Proposed fix
if (schema == null) return; - this.referensingSchema = referensingSchema; - namespace = getNamespace(node);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.java` at line 18, The assignment this.referensingSchema = referensingSchema in the Import constructor is redundant because AbstractIncludeImport's constructor already sets referensingSchema; remove that assignment from the Import class (in the Import constructor) so referensingSchema is only initialized by the superclass constructor.core/src/main/java/com/predic8/membrane/core/util/URIUtil.java (1)
55-57: ReuseencodePathCharactersForUri()here.
convertPath2FilePathString()re-implements the same escaping logic with different string APIs. Keeping both versions aligned is easy to miss later; calling the helper afteraddFilePrefix()would remove that drift point.Proposed refactor
public static String convertPath2FilePathString(String path) { path = addFilePrefix(path); - return path.replaceAll(" ", "%20").replaceAll("\\\\", "/"); + return encodePathCharactersForUri(path); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/URIUtil.java` around lines 55 - 57, convertPath2FilePathString duplicates escaping logic; change it to call the existing helper encodePathCharactersForUri after addFilePrefix rather than re-implementing replacements. In convertPath2FilePathString, call addFilePrefix(path) then return encodePathCharactersForUri(result) (removing the current replaceAll calls), so the single encodePathCharactersForUri implementation is reused and drift is prevented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/util/URIUtil.java`:
- Around line 143-160: The normalize() method treats any non-scheme input as a
filesystem path; detect relative URI references (inputs containing '?' or '#' or
starting with '//' or containing '//' without a scheme) before falling back to
normalizeInternal() and handle them as URI references instead of paths. Update
URIUtil.normalize() to check for '?' or '#' or leading/standalone '//' and
return URI.create(location).normalize().toString() (preserving relative URI
semantics and letting URI normalization handle fragments/queries) rather than
calling normalizeInternal()/Path.of(), and keep normalizeSingleDot() behavior
intact. Add a regression test (e.g., in a new unit test for URIUtil) that
validates inputs like "schema.xsd?version=1" and "../types.xsd#frag" are
normalized as URI references (not converted to absolute filesystem paths or
throwing InvalidPathException).
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.java`:
- Around line 22-24: The null-unsafe comparison in Import (the check using
schema.getTargetNamespace().equals(namespace)) can NPE when
schema.getTargetNamespace() is null; replace that condition with a null-safe
comparison using Objects.equals(namespace, schema.getTargetNamespace()) and add
the import java.util.Objects at the top of the file so the WSDLParserException
logic remains the same when the namespaces differ.
---
Nitpick comments:
In `@core/src/main/java/com/predic8/membrane/core/util/URIUtil.java`:
- Around line 55-57: convertPath2FilePathString duplicates escaping logic;
change it to call the existing helper encodePathCharactersForUri after
addFilePrefix rather than re-implementing replacements. In
convertPath2FilePathString, call addFilePrefix(path) then return
encodePathCharactersForUri(result) (removing the current replaceAll calls), so
the single encodePathCharactersForUri implementation is reused and drift is
prevented.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.java`:
- Around line 140-148: In getElements(Element wsdl, String name) rename the
misleading local variable services to a neutral name like elements or result to
reflect that the method returns any element type (not just services); update all
references in that method (the ArrayList<> instantiation and the return
statement) so the new variable name replaces services while leaving method
signature getElements and the use of WSDL11_NS and list unchanged.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.java`:
- Line 18: The assignment this.referensingSchema = referensingSchema in the
Import constructor is redundant because AbstractIncludeImport's constructor
already sets referensingSchema; remove that assignment from the Import class (in
the Import constructor) so referensingSchema is only initialized by the
superclass constructor.
In `@core/src/test/java/com/predic8/membrane/core/util/soap/WSDLParserTest.java`:
- Around line 63-65: Update the inconsistent classpath URI in the WSDLParserTest
includeImport test: replace the double-slash form used in the Definitions.parse
call ("classpath://ws/include/include.wsdl") with the single-slash standard
("classpath:/ws/include/include.wsdl") to match the other tests; locate the call
in the includeImport method in WSDLParserTest where Definitions.parse(new
ResolverMap(), ...) is invoked and make the URI change for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d3d35cc0-f246-4ad1-8be8-45b268590c96
📒 Files selected for processing (11)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.javacore/src/main/java/com/predic8/membrane/core/util/URIUtil.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Binding.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Message.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserException.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.javacore/src/test/java/com/predic8/membrane/core/util/URIUtilTest.javacore/src/test/java/com/predic8/membrane/core/util/soap/WSDLParserTest.javacore/src/test/resources/ws/include/multiple.wsdl
🚧 Files skipped from review as they are similar to previous changes (5)
- core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Binding.java
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.java
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Message.java
- core/src/test/resources/ws/include/multiple.wsdl
…lization and schema imports - Included `embedded.wsdl` as a test resource for integration scenarios. - Improved `URIUtil` logic to handle query strings, fragments, and scheme-relative URIs during normalization. - Enhanced WSDL parser to handle embedded schemas and validate imports with improved error handling. - Updated tests to ensure accurate parsing and normalization in new and existing edge cases.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.java (3)
83-85: Consider returningOptional<Service>instead of null.
getServicereturnsnullwhen no matching service is found, which can lead to NPEs if callers don't anticipate this.♻️ Proposed change
- public Service getService(String name) { - return services.stream().filter(s -> s.getName().equals(name)).findFirst().orElse(null); + public Optional<Service> getService(String name) { + return services.stream().filter(s -> s.getName().equals(name)).findFirst(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.java` around lines 83 - 85, Change Definitions.getService to return Optional<Service> instead of possibly null: update the signature of Definitions.getService and its implementation (currently using services.stream().filter(...).findFirst().orElse(null)) to return the Optional from findFirst() directly; update any callers of Definitions.getService to handle Optional<Service> (use isPresent()/ifPresent(), orElseThrow, or map/flatMap as appropriate). Ensure imports for java.util.Optional are added and adjust any tests or usages that assumed a nullable Service return.
32-48: Side-effect constructors discard instances.Lines 40 and 46 instantiate
ServiceandPortTypebut discard the created objects, relying on constructor side effects to register them into the model. While this works, it's unconventional and can confuse readers.Consider returning the created instance and adding it explicitly, or documenting this pattern clearly.
♻️ Suggested alternative (explicit registration)
for (var e : getElements(element, "service")) { - new Service(ctx, e); + services.add(new Service(ctx, e)); } // Abstract WSDL without binding and service elements if (services.isEmpty()) { for (var e : getElements(element, "portType")) { - new PortType(ctx, e); + portTypes.add(new PortType(ctx, e)); } }This would require adjusting
ServiceandPortTypeconstructors to not self-register.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.java` around lines 32 - 48, The parse method in Definitions uses side-effect-only constructors for Service and PortType (new Service(ctx, e); new PortType(ctx, e);) which discards instances; change this to explicitly capture and register the created objects: call the constructors to get a Service/PortType instance and then add it to the Definitions' services (and portTypes) collection, or alternatively refactor Service and PortType constructors to stop self-registering and return instances so Definitions.parse can perform explicit registration; update the Service/PortType constructors and Definitions.parse accordingly to eliminate hidden side effects and make ownership explicit.
121-129:getElementsByTagNameNSretrieves all descendants.This helper returns elements at any depth. For standard WSDLs this is fine since
service/portTypeelements are always direct children ofdefinitions. However, if extensibility elements or imports ever nest these elements, unexpected results could occur.If you want stricter adherence to WSDL structure, consider filtering for direct children only (like you did in
getSchemaElements).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.java` around lines 121 - 129, The getElements helper currently uses getElementsByTagNameNS which returns descendant elements at any depth; change getElements(Element wsdl, String name) to only return direct child Elements by iterating wsdl.getChildNodes(), checking each Node is an Element, matching elem.getLocalName().equals(name) and elem.getNamespaceURI().equals(WSDL11_NS) (same approach as getSchemaElements), and add only those to the result list so service/portType lookups are limited to immediate children.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/util/URIUtil.java`:
- Around line 146-155: The guard in URIUtil that detects Windows drive-letter
paths treats only absolute forms (e.g., C:\ or C:/) and lets drive-relative
forms like "C:temp\schema.xsd" fall through to URI.create; update the
drive-letter branch that checks location.charAt(0) == letter and
location.charAt(1) == ':' so it does not require a separator at char 2 — i.e.
treat any non-empty remainder after the colon as a filesystem path and return
normalizeInternal(location) for those cases; keep URI_SCHEME_PATTERN and the
existing normalizeInternal(...) call for other cases, and add a regression test
asserting that inputs like "C:temp\\..\\data\\test.xsd" are normalized as
filesystem paths rather than opaque URIs.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.java`:
- Around line 27-37: getSchema(WSDLParserContext ctx) currently uses the
instance field namespace which may be null during superclass construction;
change the logic so when schemaLocation is null/empty you read the namespace
directly from the protected element (from WSDLElement) instead of using the
namespace field, and use Objects.equals(s.getTargetNamespace(), readNamespace)
for null-safe comparison; update the getSchema(WSDLParserContext ctx)
implementation (referencing getSchema, schemaLocation, namespace, element, and
WSDLElement) so embedded schema lookup works correctly during superclass
construction.
In `@core/src/test/java/com/predic8/membrane/core/util/URIUtilTest.java`:
- Around line 295-298: The test intended to cover the scheme-relative branch of
URIUtil.normalize(String) currently uses "/example.com/..." which is an
absolute-path and (with '?') hits the query/fragment branch; update the test
(keepsSchemeRelativeReference) to pass a scheme-relative input that starts with
"//" (e.g. "//example.com/schema.xsd") and assert the normalized result
preserves that scheme-relative form; ensure the test invokes
URIUtil.normalize(String) and compares to the exact expected
"//example.com/schema.xsd" (and rename the test if helpful to reflect
scheme-relative behavior).
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.java`:
- Around line 83-85: Change Definitions.getService to return Optional<Service>
instead of possibly null: update the signature of Definitions.getService and its
implementation (currently using
services.stream().filter(...).findFirst().orElse(null)) to return the Optional
from findFirst() directly; update any callers of Definitions.getService to
handle Optional<Service> (use isPresent()/ifPresent(), orElseThrow, or
map/flatMap as appropriate). Ensure imports for java.util.Optional are added and
adjust any tests or usages that assumed a nullable Service return.
- Around line 32-48: The parse method in Definitions uses side-effect-only
constructors for Service and PortType (new Service(ctx, e); new PortType(ctx,
e);) which discards instances; change this to explicitly capture and register
the created objects: call the constructors to get a Service/PortType instance
and then add it to the Definitions' services (and portTypes) collection, or
alternatively refactor Service and PortType constructors to stop
self-registering and return instances so Definitions.parse can perform explicit
registration; update the Service/PortType constructors and Definitions.parse
accordingly to eliminate hidden side effects and make ownership explicit.
- Around line 121-129: The getElements helper currently uses
getElementsByTagNameNS which returns descendant elements at any depth; change
getElements(Element wsdl, String name) to only return direct child Elements by
iterating wsdl.getChildNodes(), checking each Node is an Element, matching
elem.getLocalName().equals(name) and elem.getNamespaceURI().equals(WSDL11_NS)
(same approach as getSchemaElements), and add only those to the result list so
service/portType lookups are limited to immediate children.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b9ae083e-0e3d-46ef-ac0e-6c71945d4b31
📒 Files selected for processing (6)
core/src/main/java/com/predic8/membrane/core/util/URIUtil.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.javacore/src/test/java/com/predic8/membrane/core/util/URIUtilTest.javacore/src/test/resources/ws/import/embedded.wsdl
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.java
- Streamlined XML schema declarations in `embedded.wsdl`. - Enhanced `URIUtil` to simplify Windows drive path normalization. - Refactored WSDL parser for better error handling, schema imports, and embedded schema handling. - Modernized code by replacing specific types with `var` where applicable. - Improved `URIUtilTest` to cover Windows drive-relative paths and other edge cases.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java (1)
44-48:⚠️ Potential issue | 🟠 MajorService-less WSDLs are still forced into document style.
Lines 45-46 drop every
PortTypeinto the document bucket whenever<service>is absent. That skips binding-based style detection, so RPC WSDLs in this shape will miss their synthesized wrapper QNames, and a non-nullserviceNameis silently ignored instead of being rejected. This branch should preserve the same validation/style-resolution behavior as the serviced path rather than defaulting everything to document style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java` around lines 44 - 48, The current special-case in getTypesByStyle forces all PortTypes to document style when definitions.getServices().isEmpty(), losing binding-based style detection and ignoring a non-null serviceName; remove that branch and always call getPortTypesByStyle(definitions, serviceName) so binding-derived styles are preserved and serviceName is respected (leading to rejection earlier if an explicit serviceName cannot be resolved). Update getTypesByStyle to delegate unconditionally to getPortTypesByStyle and ensure downstream code handles the error/validation path when serviceName is provided but not found.
🧹 Nitpick comments (1)
core/src/test/java/com/predic8/membrane/core/util/URIUtilTest.java (1)
250-260: Consider clarifying the test comments.The comment "schema is the authority of the URI" on lines 252 and 258 is slightly confusing since
authorityis the actual authority in these URIs, notschema. The tests themselves are correct—verifying that..within the path normalizes but..at the path root (after authority) is preserved.📝 Suggested comment clarification
`@Test` void classpathUri() { - // schema is the authority of the URI so .. does not apply. + // The '..' within the path portion normalizes correctly. assertEquals("classpath://authority/xsd/test.xsd", normalize("classpath://authority/schema/../xsd/test.xsd")); } `@Test` void keepsClasspathUri() { - // schema is the authority of the URI so .. does not apply. + // The '..' at the start of the path (after authority) cannot traverse above the authority, so it's preserved. assertEquals("classpath://authority/../xsd/test.xsd", normalize("classpath://authority/../xsd/test.xsd")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/util/URIUtilTest.java` around lines 250 - 260, Update the confusing test comments in the classpathUri and keepsClasspathUri tests to correctly refer to the URI authority rather than "schema" and explain the behavior being asserted: in classpathUri and keepsClasspathUri (tests invoking normalize(...)) state that "authority" is the URI authority and that relative segments ("..") inside the path are normalized but a leading ".." immediately after the authority (i.e., at the path root) is preserved; adjust the inline comments accordingly to mention the normalize method and the intended preservation/normalization behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.java`:
- Around line 74-76: getEmbeddedSchema currently calls
s.getTargetNamespace().equals(namespace) which can NPE when getTargetNamespace()
is null; update the predicate to use null-safe comparison (e.g.
Objects.equals(namespace, s.getTargetNamespace()) or namespace == null ?
s.getTargetNamespace() == null : namespace.equals(s.getTargetNamespace())) in
the lambda inside Definitions.getEmbeddedSchema, and add the necessary import
for java.util.Objects if you choose Objects.equals; keep method semantics
(return first matching Schema or null).
- Around line 53-59: When services.isEmpty() the code currently only iterates
portType children and misses WSDLs that define binding but no service; update
the branch in Definitions (the block that iterates
getDirectChildElements(element, "portType")) to first parse direct "binding"
children (e.g., iterate getDirectChildElements(element, "binding") and
instantiate new Binding(ctx, e) for each) before falling back to creating
PortType instances, so bindings get registered in the WSDLParserContext and
getBindings() is populated.
---
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java`:
- Around line 44-48: The current special-case in getTypesByStyle forces all
PortTypes to document style when definitions.getServices().isEmpty(), losing
binding-based style detection and ignoring a non-null serviceName; remove that
branch and always call getPortTypesByStyle(definitions, serviceName) so
binding-derived styles are preserved and serviceName is respected (leading to
rejection earlier if an explicit serviceName cannot be resolved). Update
getTypesByStyle to delegate unconditionally to getPortTypesByStyle and ensure
downstream code handles the error/validation path when serviceName is provided
but not found.
---
Nitpick comments:
In `@core/src/test/java/com/predic8/membrane/core/util/URIUtilTest.java`:
- Around line 250-260: Update the confusing test comments in the classpathUri
and keepsClasspathUri tests to correctly refer to the URI authority rather than
"schema" and explain the behavior being asserted: in classpathUri and
keepsClasspathUri (tests invoking normalize(...)) state that "authority" is the
URI authority and that relative segments ("..") inside the path are normalized
but a leading ".." immediately after the authority (i.e., at the path root) is
preserved; adjust the inline comments accordingly to mention the normalize
method and the intended preservation/normalization behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e166f41-0b74-4de7-8234-65c880bfea3c
📒 Files selected for processing (6)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.javacore/src/main/java/com/predic8/membrane/core/util/URIUtil.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.javacore/src/test/java/com/predic8/membrane/core/util/URIUtilTest.javacore/src/test/resources/ws/import/embedded.wsdl
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.java
- core/src/test/resources/ws/import/embedded.wsdl
- core/src/main/java/com/predic8/membrane/core/util/URIUtil.java
…e handling - Added Apache 2.0 license headers to all modified files. - Enhanced SOAP analysis with additional debug logging in `SOAPUtil`. - Updated XPath expressions in YAML configuration to use explicit SOAP namespace references (`s11`). - Refactored several test files for consistency and improved readability.
…e handling - Added Apache 2.0 license headers to all modified files. - Enhanced SOAP analysis with additional debug logging in `SOAPUtil`. - Updated XPath expressions in YAML configuration to use explicit SOAP namespace references (`s11`). - Refactored several test files for consistency and improved readability.
- Deleted unused `registryWSDLRegisterURL` field and associated methods. - Removed redundant `HttpClient` logic. - Simplified `rewrite` method. - Streamlined imports and improved code readability.
- Removed unused methods and fields in `WSDLUtil`, `WSDLInterceptor`, and related classes for improved maintainability. - Deleted `WSDLUtilTest` and redundant SOAP operations tests. - Replaced specific types with `var` for modernized code. - Removed dependency on `soa-model-core` and updated `pom.xml` with refined dependencies for Groovy modules. - Simplified WSDL example handling by removing unused logic and streamlining `embedded.wsdl`.
- Deleted `WSDLUtil` and its unused methods for maintainability. - Replaced redundant logic in WSDL parser classes with utility methods. - Streamlined SOAP wsdl-related exemplars and refactored embedded schema handling. - Simplified field initializations and usage of `var` across classes. - Updated documentation to reflect these changes.
- Added new WSDL file (`calculator-fault.wsdl`) with fault definition for testing. - Updated WSDL parser to support fault direction (`FAULT`) in operations handling. - Enhanced `Relocator` and related classes by removing unnecessary fields and imports for better readability. - Added unit test to validate fault handling in operations.
- Deleted redundant `referencingSchema` field in `AbstractIncludeImport`. - Removed `hasLocalName` method from `WSDLElement`. - Simplified lambda usage in `Operation` for better readability.
Member
Author
|
@coderabbitai plz review |
…on and style detection, and add unit tests - Replaced direct field assignments with `getAttribute` for cleaner attribute handling in `BindingOperation` and `Address`. - Added `BindingStyle` class to improve SOAP version and style handling logic. - Streamlined `Node` processing in `Binding` and `WSDLElement`. - Introduced helper methods for SOAP namespace and element checks. - Added new unit tests (`WSDLParserUtilTest`) for utility methods and `rpcStyle` test case for style detection.
…Type` and `SOAPVersion` handling logic
…into wsdl-validator-new-wsdl-parser
…lity - Simplified object initialization and attribute handling across WSDL parser classes. - Removed redundant fields (`Type`, `Service`, `PortType`, etc.) and unified child node processing logic. - Introduced `instantiateChildren()` and related helper methods for cleaner element instantiation. - Enhanced `Binding`, `Operation`, `Message`, and related classes: reduced complexity by eliminating unnecessary lists and utility methods. - Added `ProtocolOperation` class to handle SOAP operation attributes (e.g., `soapAction`). - Updated and added unit tests to validate changes.
…handling, and enhance readability - Replaced `get(0)` with `getFirst()` for better semantics in child node initialization. - Made `imports` and `includes` lists final in `Schema` for immutability. - Simplified stream operations with method references in `Definitions`. - Removed unused `addSoapVersion` method and redundant test logic.
…ling, and optimize child element processing - Replaced direct field usage with `getAttribute` for cleaner logic (`getName()` in `WSDLElement` and `BindingOperation`). - Simplified and unified child node processing with `instantiateChildren()` and `instantiateChild()` methods across classes. - Added `Fault` handling for operations and updated `WebServiceExplorerInterceptor` to include fault details in table rendering. - Refactored `Operation` to improve input/output/fault message handling with streamlined classes (`Input`, `Output`, `Fault`). - Introduced `Types` class for better encapsulation of WSDL element processing. - Updated unit tests to validate refactored fault handling and schema updates.
- Replaced `findFirst().get()` with `orElseThrow()` for safer binding retrieval in `Port`. - Simplified `Definitions` parsing logic by removing unused method parameters and refactoring lambda expressions. - Updated unit test assertions for better accuracy (`assertNull` replaced with `assertEquals`).
Member
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
- Added missing `td()` element to ensure proper rendering of fault details in the web service explorer table.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
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)
53-67:⚠️ Potential issue | 🟠 MajorDo not silently blank out malformed relocation targets.
getNewLocation()returns""onMalformedURLException, and these branches write that empty string back intolocation/schemaLocation. That turns a bad input URL into corrupted output instead of preserving the original value or failing loudly.Suggested fix
public static String getNewLocation(String addr, String protocol, String host, int port, String contextPath) { try { var oldURL = new URL(addr); if (port == -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(); } catch (MalformedURLException e) { - log.error("", e); + log.warn("Skipping relocation for malformed URL '{}'.", addr, e); + return addr; } - return ""; }Also applies to: 136-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java` around lines 53 - 67, The catch block in getNewLocation currently swallows MalformedURLException and returns an empty string, corrupting relocation headers; change the error handling in getNewLocation to log a clear message with the exception and return the original addr instead of "" (or rethrow a checked/unchecked exception if you prefer failing loudly) so callers preserve the original Location value; apply the same fix to the other analogous method/branch mentioned (the similar code at lines 136-147) so both places return the original input on parse failure and include the exception in the log.distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java (1)
97-105:⚠️ Potential issue | 🟡 MinorSpecify explicit UTF-8 charset for the XML request with non-ASCII character.
Line 102 now contains the non-ASCII character
Düsseldorf, butcontentType(XML)defaults to ISO-8859-1 encoding in REST Assured, not UTF-8. This can cause encoding mismatches and make the test unreliable. Use.contentType(XML.withCharset("UTF-8"))or explicitly set the content type to"application/xml; charset=UTF-8"to ensure consistent encoding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java` around lines 97 - 105, The test sends XML with a non-ASCII character ("Düsseldorf") but uses contentType(XML) which defaults to ISO-8859-1 in REST Assured; update the request so the Content-Type explicitly specifies UTF-8 (e.g., replace the contentType(XML) usage in CustomErrorHandlingExampleTest (around the request builder that calls .contentType(XML) and .body(...)) with a content type that includes charset UTF-8 such as XML.withCharset("UTF-8") or the literal "application/xml; charset=UTF-8") so the request body is encoded consistently as UTF-8.
♻️ Duplicate comments (2)
core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.java (2)
42-47:⚠️ Potential issue | 🟠 MajorDo not treat
visitedLocationsas a global de-dup.Returning
nullhere means the second parent that imports the same shared XSD never gets that schema merged. This should be a cycle guard, or a cache lookup that returns the already parsedSchema, not a silent short-circuit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.java` around lines 42 - 47, The current check in AbstractIncludeImport that returns null when ctx.getVisitedLocations().contains(resolved) silently drops the schema and breaks merging for later parents; instead implement a proper cycle-guard + cache lookup: when a resolved location is seen, first try to return the already-parsed Schema from the parser context (e.g., a cached map on ctx keyed by resolved); only if it is currently "in-progress" (cycle) should you short-circuit to avoid recursion, otherwise return the cached Schema so subsequent parents get the merged schema; update AbstractIncludeImport to consult and populate the ctx schema-cache (and mark/unmark visited during parse) rather than returning null.
54-65:⚠️ Potential issue | 🟠 MajorHandle blank
schemaLocationbefore URI resolution.
Element.getAttribute()returns""when the attribute is missing, and Line 55 normalizes that into the current base URI. That turns a malformed include/import into a silent self-reference instead of a clear parser decision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.java` around lines 54 - 65, The resolver currently normalizes an empty schemaLocation into the base URI, causing silent self-references; update resolve(WSDLParserContext ctx) to first check if schemaLocation is null or blank and return it (or otherwise leave it unmodified) instead of calling URIUtil.normalize/ResolverMap.combine when schemaLocation.isBlank(), and also adjust the static getSchemaLocation(Node) to return null or a distinct empty marker (instead of the empty string) so callers can detect a missing attribute; refer to the resolve method and the static getSchemaLocation(Node) when making the change.
🧹 Nitpick comments (7)
distribution/examples/extending-membrane/error-handling/custom-error-messages/apis.yaml (1)
61-61: Consider adding/text()for consistency with fault extraction.Line 46 uses
${xpath('//faultstring/text()')}to extract text content, but this line uses${xpath('/failure/description')}which retrieves the element node rather than its text content. If the intent is to extract text, consider using/failure/description/text()for consistent behavior.✏️ Suggested change
- ${xpath('/failure/description')} + <description>${xpath('/failure/description/text()')}</description>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/examples/extending-membrane/error-handling/custom-error-messages/apis.yaml` at line 61, The xpath expression ${xpath('/failure/description')} returns the element node rather than its text; update it to extract the text content by using ${xpath('/failure/description/text()')} so it behaves consistently with the other usage ${xpath('//faultstring/text()')}; locate the occurrence of ${xpath('/failure/description')} in the APIs YAML and replace it with ${xpath('/failure/description/text()')}.core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/SchemaElement.java (1)
22-27: Remove the redundantctxfield.The
ctxfield is declared and assigned in the constructor but never used withinSchemaElement. Since the parent classWSDLElementalready stores and provides access toctxvia its protected field, this local copy is unnecessary.♻️ Suggested simplification
public class SchemaElement extends WSDLElement { - WSDLParserContext ctx; - public SchemaElement(WSDLParserContext ctx,Node node) { super(ctx,node); - this.ctx = ctx; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/SchemaElement.java` around lines 22 - 27, Remove the redundant WSDLParserContext ctx field from the SchemaElement class and its assignment in the SchemaElement(WSDLParserContext ctx, Node node) constructor; rely on the inherited protected ctx field in the parent class WSDLElement (leave the super(ctx, node) call intact) and delete the field declaration and the this.ctx = ctx; line so no local copy remains.core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserContext.java (1)
17-17: Remove unused import.The import
com.predic8.membrane.core.graphql.model.*appears unused in this file. No GraphQL types are referenced.🧹 Proposed fix
-import com.predic8.membrane.core.graphql.model.*; import com.predic8.membrane.core.resolver.*;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserContext.java` at line 17, Remove the unused wildcard import com.predic8.membrane.core.graphql.model.* from WSDLParserContext.java; locate the import statement in the top of the WSDLParserContext class and delete that import line so only actually used imports remain.core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLValidator.java (1)
141-143: PreferSet.contains()over streaming.
Setalready provides an O(1)contains()method. The stream-based check adds unnecessary overhead.♻️ Proposed simplification
private boolean isPossibleSOAPElement(Set<QName> elementNames, QName name) { - return elementNames.stream().anyMatch(qn -> qn.equals(name)); + return elementNames.contains(name); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLValidator.java` around lines 141 - 143, The method isPossibleSOAPElement currently checks membership by streaming elementNames; replace the stream-based anyMatch with the direct Set.contains call for O(1) lookup: use elementNames.contains(name) in isPossibleSOAPElement to simplify and improve performance (references: method isPossibleSOAPElement, parameter elementNames, QName name).core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLElement.java (1)
87-91: Consider preserving specific reflection exception details.The current catch wraps all exceptions in a generic
RuntimeException, which can obscure the actual failure reason (e.g.,NoSuchMethodExceptionvsInvocationTargetException). Consider logging or including more context.♻️ Optional enhancement
try { result.add(clazz.getConstructor(WSDLParserContext.class, Node.class).newInstance(ctx, child)); } catch (Exception e) { - throw new RuntimeException(e); + throw new RuntimeException("Failed to instantiate " + clazz.getSimpleName() + " for element: " + name, e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLElement.java` around lines 87 - 91, The catch-all wrapping of reflection exceptions in WSDLElement hides root causes; replace the blanket "catch (Exception e)" around the clazz.getConstructor(...).newInstance(...) call with specific handling for reflection exceptions (NoSuchMethodException, InstantiationException, IllegalAccessException, InvocationTargetException) and rethrow a RuntimeException that includes the class name (clazz), the Node (child) context, and the original exception (use e or e.getCause() for InvocationTargetException) so the actual failure reason is preserved and visible in logs/traces.core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java (1)
103-108: Remove unnecessary intermediate collection.The
.toList().stream()creates an intermediate list that is immediately streamed again. This can be simplified.♻️ Proposed simplification
private static `@NotNull` Stream<Part> getParts(Direction direction, List<PortType> result) { return result.stream().map(PortType::getOperations) .flatMap(Collection::stream) .map(op -> op.getMessagesByDirection(direction)) - .flatMap(Collection::stream).toList().stream().map(Message::getPart); + .flatMap(Collection::stream) + .map(Message::getPart); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java` around lines 103 - 108, The getParts method creates an unnecessary intermediate list by calling .toList().stream(); instead, keep the stream pipeline intact: start from result.stream(), flatMap PortType::getOperations, flatMap the operations' messages via op.getMessagesByDirection(direction), then map each Message to its Part using Message::getPart so the method returns a Stream<Part> without materializing a list; update the chain in getParts to remove the .toList().stream() step and directly continue the stream operations (symbols: getParts, Direction, PortType, getOperations, getMessagesByDirection, Message::getPart).core/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.java (1)
136-139: Consider creating a tracking issue for the TODO.The commented-out documentation display is marked with
@TODO. If this feature is planned for implementation, consider creating an issue to track it rather than leaving commented code.Would you like me to help create an issue to track the documentation display feature?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.java` around lines 136 - 139, The commented-out documentation display in WebServiceExplorerInterceptor (the block with Documentation d = pt.getDocumentation(); and the `@TODO`) should be tracked in the project issue tracker: create a short issue describing "Implement WSDL documentation display in WebServiceExplorerInterceptor" (include current commented code snippet and expected behavior), add the issue ID to the TODO comment in WebServiceExplorerInterceptor.java (replace `@TODO` with e.g. TODO: ISSUE-1234) and optionally add a one-line TODO that explains why it's commented out and what needs to be implemented (method: where pt.getDocumentation() was referenced). Ensure the comment references the new issue ID so future work is discoverable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/AbstractXMLSchemaValidator.java`:
- Around line 129-133: The DOMSource for each schema is being stamped with the
shared validator-wide variable location, causing relative xs:include/xs:import
to resolve incorrectly; in createValidator(Element schema, SchemaFactory sf)
replace source.setSystemId(location) with logic that uses the schema element's
own base/system URI (e.g. schema.getBaseURI() or
schema.getOwnerDocument().getDocumentURI()) and only fall back to the shared
location when the element-specific value is null/empty before calling
sf.newSchema(source).newValidator().
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.java`:
- Around line 172-179: In WebServiceExplorerInterceptor, the HTML row generation
for operation outputs/faults is missing a td() call before rendering faults,
which breaks the table structure; inside the method that builds the table rows
(the block that iterates over o.getOutputs() and o.getFaults(), referenced by
the variable o and class WebServiceExplorerInterceptor), add the missing td()
invocation immediately before the loop that writes fault parts so the fault
values are enclosed in their own <td> cell and then call end() after writing
faults.
- Around line 116-121: The code in WebServiceExplorerInterceptor using
Definitions.parse(...) must defensively handle empty services or ports to avoid
NoSuchElementException from service.getPorts().getFirst() and
definitions.getServices().getFirst(); update the logic around
Definitions.parse(router.getResolverMap(), wsdl) to check that
definitions.getServices() is not empty before calling getFirst(), and likewise
verify service.getPorts() contains elements before calling getFirst(); if either
list is empty, handle it gracefully (e.g., log a clear error via the
interceptor’s logger, return an appropriate empty response or error view, or
throw a checked/handled exception) instead of letting NoSuchElementException
propagate—adjust any existing catch blocks (in WebServiceExplorerInterceptor) to
catch NoSuchElementException if you prefer catching rather than pre-checking.
- Around line 169-177: In WebServiceExplorerInterceptor (the loop using
o.getInputs()/o.getOutputs()/o.getFaults() and iterating Part p), protect
against nulls by checking both that getMessage().getPart() actually returns a
part (avoid NoSuchElementException from calling getFirst()) and that
Part.getElementQName() is not null before calling toString(); skip or print a
safe placeholder when no part or no element QName is present. Locate the three
streams that map om -> om.getMessage().getPart() and replace the direct chaining
with a safe lookup (e.g., guard findFirst()/isPresent() or null-check) and a
null-check on p.getElementQName() (use a placeholder like "<no-element>" or
p.getName()) instead of calling toString() on a possibly null QName.
In `@core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java`:
- Around line 114-120: The current getService/getPort selection always picks the
first available port/service and doesn't honor portName or validate presence;
update the logic in getService(Definitions) and the corresponding port selection
(the code handling Service -> Port) to: if serviceName is set, look up that
service and throw a ConfigurationException with a clear message if missing; once
the Service is found, if portName is set, look up that Port by name and throw
ConfigurationException if not found; if portName is not set, ensure the service
has at least one Port and otherwise throw ConfigurationException (do not let raw
collection exceptions surface); ensure these checks replace any unconditional
getFirst() usage so setPortName() is honored and missing service/port produce
configuration errors.
In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Binding.java`:
- Around line 59-61: The Binding.getBindingStyle() currently calls
instantiateChildren("binding", BindingStyle.class).getFirst() which throws
NoSuchElementException for non-SOAP or malformed WSDLs; change it to obtain the
BindingStyle via instantiateChildAsOptional("binding", BindingStyle.class) (or
otherwise handle an empty result) and return null or an Optional so callers of
getStyle() and getSoapVersion() can handle missing bindings; update
getStyle()/getSoapVersion() to defensively handle a null/empty BindingStyle
(avoid assuming non-null) and propagate a safe default or throw a clearer
exception as appropriate.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/BindingStyle.java`:
- Around line 1-6: Add the Apache 2.0 license header to the top of
BindingStyle.java so it matches the other new files: insert the standard
multi-line Apache 2.0 header (including copyright and license URL/SPDX if used
in the repo) above the package declaration in
com.predic8.membrane.core.util.wsdl.parser.BindingStyle (file
BindingStyle.java).
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.java`:
- Around line 53-55: The isSchemaLocationMissing() method has inverted logic:
change its implementation in class Import so it returns true when schemaLocation
is null or empty (e.g., schemaLocation == null || schemaLocation.isEmpty()), not
when it exists; this will restore correct behavior for importEmbeddedSchema()
which expects a true result when the schema location is missing.
In
`@distribution/examples/extending-membrane/error-handling/custom-error-messages/apis.yaml`:
- Around line 51-54: The XPath condition uses the s11 prefix but lacks the
namespace binding, so update the if block containing test: not(/s11:Envelope) to
include the namespace reference by adding $ref: "#/components/ns" (or the
appropriate namespace component) so the s11 prefix is defined for XPath
evaluation; ensure the same change is applied to any other conditional blocks
using s11:Envelope.
---
Outside diff comments:
In `@core/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java`:
- Around line 53-67: The catch block in getNewLocation currently swallows
MalformedURLException and returns an empty string, corrupting relocation
headers; change the error handling in getNewLocation to log a clear message with
the exception and return the original addr instead of "" (or rethrow a
checked/unchecked exception if you prefer failing loudly) so callers preserve
the original Location value; apply the same fix to the other analogous
method/branch mentioned (the similar code at lines 136-147) so both places
return the original input on parse failure and include the exception in the log.
In
`@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.java`:
- Around line 97-105: The test sends XML with a non-ASCII character
("Düsseldorf") but uses contentType(XML) which defaults to ISO-8859-1 in REST
Assured; update the request so the Content-Type explicitly specifies UTF-8
(e.g., replace the contentType(XML) usage in CustomErrorHandlingExampleTest
(around the request builder that calls .contentType(XML) and .body(...)) with a
content type that includes charset UTF-8 such as XML.withCharset("UTF-8") or the
literal "application/xml; charset=UTF-8") so the request body is encoded
consistently as UTF-8.
---
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.java`:
- Around line 42-47: The current check in AbstractIncludeImport that returns
null when ctx.getVisitedLocations().contains(resolved) silently drops the schema
and breaks merging for later parents; instead implement a proper cycle-guard +
cache lookup: when a resolved location is seen, first try to return the
already-parsed Schema from the parser context (e.g., a cached map on ctx keyed
by resolved); only if it is currently "in-progress" (cycle) should you
short-circuit to avoid recursion, otherwise return the cached Schema so
subsequent parents get the merged schema; update AbstractIncludeImport to
consult and populate the ctx schema-cache (and mark/unmark visited during parse)
rather than returning null.
- Around line 54-65: The resolver currently normalizes an empty schemaLocation
into the base URI, causing silent self-references; update
resolve(WSDLParserContext ctx) to first check if schemaLocation is null or blank
and return it (or otherwise leave it unmodified) instead of calling
URIUtil.normalize/ResolverMap.combine when schemaLocation.isBlank(), and also
adjust the static getSchemaLocation(Node) to return null or a distinct empty
marker (instead of the empty string) so callers can detect a missing attribute;
refer to the resolve method and the static getSchemaLocation(Node) when making
the change.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java`:
- Around line 103-108: The getParts method creates an unnecessary intermediate
list by calling .toList().stream(); instead, keep the stream pipeline intact:
start from result.stream(), flatMap PortType::getOperations, flatMap the
operations' messages via op.getMessagesByDirection(direction), then map each
Message to its Part using Message::getPart so the method returns a Stream<Part>
without materializing a list; update the chain in getParts to remove the
.toList().stream() step and directly continue the stream operations (symbols:
getParts, Direction, PortType, getOperations, getMessagesByDirection,
Message::getPart).
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLValidator.java`:
- Around line 141-143: The method isPossibleSOAPElement currently checks
membership by streaming elementNames; replace the stream-based anyMatch with the
direct Set.contains call for O(1) lookup: use elementNames.contains(name) in
isPossibleSOAPElement to simplify and improve performance (references: method
isPossibleSOAPElement, parameter elementNames, QName name).
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.java`:
- Around line 136-139: The commented-out documentation display in
WebServiceExplorerInterceptor (the block with Documentation d =
pt.getDocumentation(); and the `@TODO`) should be tracked in the project issue
tracker: create a short issue describing "Implement WSDL documentation display
in WebServiceExplorerInterceptor" (include current commented code snippet and
expected behavior), add the issue ID to the TODO comment in
WebServiceExplorerInterceptor.java (replace `@TODO` with e.g. TODO: ISSUE-1234)
and optionally add a one-line TODO that explains why it's commented out and what
needs to be implemented (method: where pt.getDocumentation() was referenced).
Ensure the comment references the new issue ID so future work is discoverable.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/SchemaElement.java`:
- Around line 22-27: Remove the redundant WSDLParserContext ctx field from the
SchemaElement class and its assignment in the SchemaElement(WSDLParserContext
ctx, Node node) constructor; rely on the inherited protected ctx field in the
parent class WSDLElement (leave the super(ctx, node) call intact) and delete the
field declaration and the this.ctx = ctx; line so no local copy remains.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLElement.java`:
- Around line 87-91: The catch-all wrapping of reflection exceptions in
WSDLElement hides root causes; replace the blanket "catch (Exception e)" around
the clazz.getConstructor(...).newInstance(...) call with specific handling for
reflection exceptions (NoSuchMethodException, InstantiationException,
IllegalAccessException, InvocationTargetException) and rethrow a
RuntimeException that includes the class name (clazz), the Node (child) context,
and the original exception (use e or e.getCause() for InvocationTargetException)
so the actual failure reason is preserved and visible in logs/traces.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserContext.java`:
- Line 17: Remove the unused wildcard import
com.predic8.membrane.core.graphql.model.* from WSDLParserContext.java; locate
the import statement in the top of the WSDLParserContext class and delete that
import line so only actually used imports remain.
In
`@distribution/examples/extending-membrane/error-handling/custom-error-messages/apis.yaml`:
- Line 61: The xpath expression ${xpath('/failure/description')} returns the
element node rather than its text; update it to extract the text content by
using ${xpath('/failure/description/text()')} so it behaves consistently with
the other usage ${xpath('//faultstring/text()')}; locate the occurrence of
${xpath('/failure/description')} in the APIs YAML and replace it with
${xpath('/failure/description/text()')}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a806adb2-6d76-4c68-a500-0f9da2759689
📒 Files selected for processing (57)
core/pom.xmlcore/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/oauth2/authorizationservice/AuthorizationService.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/AbstractXMLSchemaValidator.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLValidator.javacore/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.javacore/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.javacore/src/main/java/com/predic8/membrane/core/resolver/ResolverMap.javacore/src/main/java/com/predic8/membrane/core/transport/ssl/TLSUnrecognizedNameException.javacore/src/main/java/com/predic8/membrane/core/util/SOAPUtil.javacore/src/main/java/com/predic8/membrane/core/util/WSDLUtil.javacore/src/main/java/com/predic8/membrane/core/util/soap/WSDLUtil.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Address.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Binding.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/BindingOperation.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/BindingStyle.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Definitions.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Message.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Operation.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Part.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Port.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/PortType.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/ProtocolOperation.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Service.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLElement.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserContext.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserException.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserUtil.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Include.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Schema.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/SchemaElement.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Types.javacore/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPFaultTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPUtilTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractorTest.javacore/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyTest.javacore/src/test/java/com/predic8/membrane/core/resolver/ResolverTest.javacore/src/test/java/com/predic8/membrane/core/util/URIUtilTest.javacore/src/test/java/com/predic8/membrane/core/util/WSDLUtilTest.javacore/src/test/java/com/predic8/membrane/core/util/soap/WSDLParserTest.javacore/src/test/java/com/predic8/membrane/core/util/wsdl/parser/OperationTest.javacore/src/test/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserUtilTest.javacore/src/test/java/com/predic8/membrane/integration/withinternet/LargeBodyTest.javacore/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/JwksRefreshTest.javacore/src/test/resources/ws/calculator-fault.wsdlcore/src/test/resources/ws/import/embedded.wsdldistribution/examples/extending-membrane/error-handling/custom-error-messages/apis.yamldistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/custom_error_messages/CustomErrorHandlingExampleTest.javadocs/ROADMAP.mdpom.xml
💤 Files with no reviewable changes (3)
- core/src/test/java/com/predic8/membrane/core/util/WSDLUtilTest.java
- core/src/main/java/com/predic8/membrane/core/util/WSDLUtil.java
- core/src/main/java/com/predic8/membrane/core/resolver/ResolverMap.java
✅ Files skipped from review due to trivial changes (3)
- core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPUtilTest.java
- docs/ROADMAP.md
- core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/JwksRefreshTest.java
🚧 Files skipped from review as they are similar to previous changes (9)
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Address.java
- core/src/test/java/com/predic8/membrane/core/util/wsdl/parser/OperationTest.java
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserUtil.java
- core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractorTest.java
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Port.java
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserException.java
- core/src/main/java/com/predic8/membrane/core/util/SOAPUtil.java
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Message.java
- core/src/test/resources/ws/import/embedded.wsdl
…mplify methods, and enhance exception handling - Replaced `WSDLParserContext` class with a `record` for better immutability and readability. - Updated method calls to use record getters (e.g., `ctx.definitions()` instead of `ctx.getDefinitions()`). - Enhanced exception messages in `WSDLElement` and `Binding` for better debugging. - Simplified stream and lambda usage across parser classes. - Added unit test for abstract WSDL service validation (`SOAPProxyTest`).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java (1)
58-63:⚠️ Potential issue | 🟠 MajorService-less WSDLs are still forced into document style.
When
<service>is absent, this path drops all RPC wrapper QNames by treating everyPortTypeas document-style, and it also skips the explicitserviceNamevalidation path. That will still reject valid service-less RPC WSDLs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java` around lines 58 - 63, The current getTypesByStyle method treats WSDLs with no <service> as document-style by returning new PortTypesByStyle(Collections.emptyList(), definitions.getPortTypes()), which drops RPC wrapper QNames and bypasses serviceName validation; instead, preserve RPC wrappers and the normal validation path by delegating to getPortTypesByStyle(definitions, serviceName) (or by implementing equivalent logic that returns the correct wrapper QNames and portTypes for service-less WSDLs). Update getTypesByStyle to avoid forcing document style when definitions.getServices().isEmpty() and ensure PortTypesByStyle contains the appropriate wrapper QNames and portTypes as computed by getPortTypesByStyle.core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserContext.java (1)
28-31:⚠️ Potential issue | 🟠 Major
basePath()mutates the sharedvisitedLocationslist before parsing succeeds.Line 29 modifies the original
visitedLocationslist in-place before returning a new context. If schema parsing subsequently fails, the location remains marked as "visited," causing later references to the same location to be incorrectly skipped. This also breaks the immutability semantics expected from a record.🛡️ Proposed fix: create a defensive copy
public WSDLParserContext basePath(String basePath) { - visitedLocations.add(basePath); - return new WSDLParserContext(definitions, resolver, basePath, visitedLocations); + var newVisited = new ArrayList<>(visitedLocations); + newVisited.add(basePath); + return new WSDLParserContext(definitions, resolver, basePath, newVisited); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserContext.java` around lines 28 - 31, WSDLParserContext.basePath currently mutates the shared visitedLocations list causing incorrect skipping on failures; change basePath to avoid in-place mutation by creating a defensive copy of visitedLocations (e.g., new ArrayList<>(visitedLocations) or equivalent) and add basePath to that copy, then construct and return the new WSDLParserContext with the copied list so the original visitedLocations remains unchanged if parsing fails.core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.java (1)
38-52:⚠️ Potential issue | 🟡 MinorHandle blank
schemaLocationexplicitly before resolving.When
getSchemaLocation(node)returns an empty string (line 65),resolve()will normalize the base path combined with an empty string, creating a self-reference. This causes thevisitedLocations.contains(resolved)check to potentially match the current document's location, resulting in a silentnullreturn instead of a clear parser decision.🛡️ Proposed fix
protected Schema getSchema(WSDLParserContext ctx) { + if (schemaLocation == null || schemaLocation.isEmpty()) { + return null; + } + try { var resolved = resolve(ctx); // Check if the schema has already been imported or included if (ctx.visitedLocations().contains(resolved)) return null; try (var is = ctx.resolver().resolve(resolved)) { return new Schema(ctx.basePath(resolved), WSDLParserUtil.parse(is)); } } catch (Exception e) { throw new RuntimeException(e); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.java` around lines 38 - 52, In getSchema(WSDLParserContext ctx) handle an empty schemaLocation before calling resolve: call getSchemaLocation(node) (or the equivalent caller logic) and if it returns an empty string, explicitly return null (or otherwise skip) instead of passing the empty string into resolve; update getSchema to check for an empty/blank location prior to calling resolve(ctx) and reference methods getSchemaLocation, resolve, getSchema, and ctx.visitedLocations to locate where to add this guard so the self-reference case is handled deterministically.core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java (1)
114-121:⚠️ Potential issue | 🔴 Critical
portNameis still ignored during routing.Service selection is validated now, but endpoint selection still hard-codes
service.getPorts().getFirst()ingetLocation(). On multi-port WSDLs that can route traffic to the wrong backend, and ports with no address/location still fail with raw collection/null exceptions instead of aConfigurationException.🐛 Suggested fix
private Service getService(Definitions defs) { - if (serviceName != null) + if (StringUtils.isNotEmpty(serviceName)) return defs.getService(serviceName).orElseThrow( () -> new ConfigurationException("No service with name '%s' found in WSDL %s".formatted(serviceName, wsdl)) ); if (defs.getServices().isEmpty()) throw new ConfigurationException("No service element found in WSDL %s".formatted(wsdl)); return defs.getServices().getFirst(); } -private `@NotNull` String getLocation(com.predic8.membrane.core.util.wsdl.parser.Service service) { - var location = service.getPorts().getFirst().getAddress().getLocation(); +private Port getPort(Service service) { + var ports = service.getPorts(); + if (StringUtils.isNotEmpty(portName)) { + return ports.stream() + .filter(port -> portName.equals(port.getName())) + .findFirst() + .orElseThrow(() -> new ConfigurationException( + "No port with name '%s' found in service '%s' of WSDL %s".formatted(portName, service.getName(), wsdl) + )); + } + return ports.stream().findFirst().orElseThrow( + () -> new ConfigurationException("Service '%s' in WSDL %s has no ports".formatted(service.getName(), wsdl)) + ); +} + +private `@NotNull` String getLocation(Service service) { + var port = getPort(service); + if (port.getAddress() == null || port.getAddress().getLocation() == null) { + throw new ConfigurationException("Port '%s' in WSDL %s has no SOAP address location.".formatted(port.getName(), wsdl)); + } + var location = port.getAddress().getLocation(); if (location == null) throw new ConfigurationException("In the WSDL %s, there is no `@location` defined on the port.".formatted(wsdl)); return location; }Also applies to: 172-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java` around lines 114 - 121, getLocation currently always uses service.getPorts().getFirst(), ignoring the configured portName and allowing null/collection exceptions; update getLocation (and related selection logic) to pick the Port from the Service by portName when portName != null (e.g. service.getPort(portName).orElseThrow(() -> new ConfigurationException(...))), otherwise fall back to the first port, and if the chosen Port has no address/location throw a ConfigurationException with a clear message; ensure the change covers the same logic paths referenced in getService/getLocation so multi-port WSDLs route correctly and missing locations produce ConfigurationException instead of raw NPEs.core/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.java (2)
118-120:⚠️ Potential issue | 🟠 MajorResolve the selected port instead of always falling back to
getFirst().
portNameis never consulted in this flow. On multi-port WSDLs, the explorer builds the operations table from the first port’s binding, andcreateEndpointTable(service.getPorts(), ports)currently marks every endpoint as “available through this proxy” because both arguments are the same list. The unconditionalgetFirst()calls also still turn missing services/ports into raw collection exceptions instead of a handled response.💡 Suggested shape
- var service = definitions.getServices().getFirst(); // TODO display all services - var port = service.getPorts().getFirst(); - var ports = service.getPorts(); + var service = definitions.getServices().stream().findFirst() + .orElseThrow(() -> new IllegalArgumentException("WSDL contains no services")); + var ports = service.getPorts(); + var port = (portName == null || portName.isEmpty() + ? ports.stream().findFirst() + : ports.stream().filter(p -> portName.equals(p.getName())).findFirst()) + .orElseThrow(() -> new IllegalArgumentException("No matching port found in WSDL")); var binding = port.getBinding(); createOperationsTable(definitions, binding, binding.getPortType()); ... - createEndpointTable(service.getPorts(), ports); + createEndpointTable(service.getPorts(), List.of(port));Also applies to: 142-143, 149-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.java` around lines 118 - 120, The code currently always picks the first service/port via definitions.getServices().getFirst() and service.getPorts().getFirst(), ignores portName, and passes identical lists into createEndpointTable(service.getPorts(), ports), causing wrong endpoint selection and unhandled collection exceptions; update the flow to (1) look up the requested service by name from definitions.getServices() (fall back to a clear handled response if not found) instead of getFirst(), (2) if a portName is provided, find that port in service.getPorts() and fall back to a sensible default or a handled "port not found" response instead of getFirst(), and (3) call createEndpointTable(allPorts, resolvedPortListOrSingle) with the real ports list and the resolved port(s) (not the same list twice) so availability calculation is correct; apply the same fix where getFirst() is used later (the blocks around createEndpointTable and any other service/port resolution code).
169-178:⚠️ Potential issue | 🟠 MajorDon’t assume every message part has an
elementQName.These loops still dereference
om.getMessage().getPart().getElementQName().toString()directly. That breaks on valid WSDLs with empty message-part lists and on RPC/type-based parts wheregetElementQName()is absent, so the explorer can still fail on schemas the new parser now accepts. Render a safe fallback instead of chaining straight throughgetPart()andgetElementQName().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.java` around lines 169 - 178, The code in WebServiceExplorerInterceptor iterates inputs/outputs/faults and calls getElementQName().toString() directly, which can NPE for empty or RPC/typed parts; in each loop (the three places iterating o.getInputs(), o.getOutputs(), o.getFaults()) ensure you null-check the Part returned from om.getMessage().getPart() and render a safe fallback: if part.getElementQName() != null use that, else if part.getTypeQName() != null use part.getTypeQName().toString(), else use part.getName() or a placeholder like "(anonymous)"; update the mapping/printing logic to skip null parts and use this fallback string instead of chaining toString() on a potentially null element QName.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/SchemaElement.java (1)
22-24: Minor formatting: add space after comma in parameter list.💅 Proposed fix
- public SchemaElement(WSDLParserContext ctx,Node node) { - super(ctx,node); + public SchemaElement(WSDLParserContext ctx, Node node) { + super(ctx, node); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/SchemaElement.java` around lines 22 - 24, The constructor signature for SchemaElement currently lacks a space after the comma; update the SchemaElement(WSDLParserContext ctx,Node node) constructor declaration to add a space after the comma so it reads SchemaElement(WSDLParserContext ctx, Node node) in the SchemaElement class (constructor method).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java`:
- Around line 96-100: The method getElementNameRPC in
WSDLMessageElementExtractor currently treats any non-INPUT as OUTPUT and
therefore misnames FAULTs; update getElementNameRPC(Operation operation,
Direction direction) to explicitly handle Direction.FAULT (do not silently treat
it as OUTPUT) by either returning the correct fault wrapper name (e.g.,
operation.getName() + "Fault") or failing fast—throw an
IllegalArgumentException/IllegalStateException with a clear message indicating
FAULT is unsupported—so that getPossibleElements(...) will no longer synthesize
incorrect RPC fault QNames.
- Around line 69-75: WSDLMessageElementExtractor currently classifies entire
bindings by binding.getStyle(), which misses operation-level overrides; instead,
iterate bindings from getBindings(definitions, serviceName) and for each binding
iterate its operations (e.g., binding.getOperations() or equivalent), determine
the effective style per operation (use an operation-level style if present,
otherwise fall back to binding.getStyle()), and place the operation's
portType/QName into portTypesRPC or portTypesDocument accordingly; update any
helper calls or the Binding parser as needed so an operation-level style can be
read (e.g., add/use binding.getOperationStyle(operationName) or inspect
operation.getStyle()) before bucketing.
---
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java`:
- Around line 58-63: The current getTypesByStyle method treats WSDLs with no
<service> as document-style by returning new
PortTypesByStyle(Collections.emptyList(), definitions.getPortTypes()), which
drops RPC wrapper QNames and bypasses serviceName validation; instead, preserve
RPC wrappers and the normal validation path by delegating to
getPortTypesByStyle(definitions, serviceName) (or by implementing equivalent
logic that returns the correct wrapper QNames and portTypes for service-less
WSDLs). Update getTypesByStyle to avoid forcing document style when
definitions.getServices().isEmpty() and ensure PortTypesByStyle contains the
appropriate wrapper QNames and portTypes as computed by getPortTypesByStyle.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.java`:
- Around line 118-120: The code currently always picks the first service/port
via definitions.getServices().getFirst() and service.getPorts().getFirst(),
ignores portName, and passes identical lists into
createEndpointTable(service.getPorts(), ports), causing wrong endpoint selection
and unhandled collection exceptions; update the flow to (1) look up the
requested service by name from definitions.getServices() (fall back to a clear
handled response if not found) instead of getFirst(), (2) if a portName is
provided, find that port in service.getPorts() and fall back to a sensible
default or a handled "port not found" response instead of getFirst(), and (3)
call createEndpointTable(allPorts, resolvedPortListOrSingle) with the real ports
list and the resolved port(s) (not the same list twice) so availability
calculation is correct; apply the same fix where getFirst() is used later (the
blocks around createEndpointTable and any other service/port resolution code).
- Around line 169-178: The code in WebServiceExplorerInterceptor iterates
inputs/outputs/faults and calls getElementQName().toString() directly, which can
NPE for empty or RPC/typed parts; in each loop (the three places iterating
o.getInputs(), o.getOutputs(), o.getFaults()) ensure you null-check the Part
returned from om.getMessage().getPart() and render a safe fallback: if
part.getElementQName() != null use that, else if part.getTypeQName() != null use
part.getTypeQName().toString(), else use part.getName() or a placeholder like
"(anonymous)"; update the mapping/printing logic to skip null parts and use this
fallback string instead of chaining toString() on a potentially null element
QName.
In `@core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java`:
- Around line 114-121: getLocation currently always uses
service.getPorts().getFirst(), ignoring the configured portName and allowing
null/collection exceptions; update getLocation (and related selection logic) to
pick the Port from the Service by portName when portName != null (e.g.
service.getPort(portName).orElseThrow(() -> new ConfigurationException(...))),
otherwise fall back to the first port, and if the chosen Port has no
address/location throw a ConfigurationException with a clear message; ensure the
change covers the same logic paths referenced in getService/getLocation so
multi-port WSDLs route correctly and missing locations produce
ConfigurationException instead of raw NPEs.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.java`:
- Around line 38-52: In getSchema(WSDLParserContext ctx) handle an empty
schemaLocation before calling resolve: call getSchemaLocation(node) (or the
equivalent caller logic) and if it returns an empty string, explicitly return
null (or otherwise skip) instead of passing the empty string into resolve;
update getSchema to check for an empty/blank location prior to calling
resolve(ctx) and reference methods getSchemaLocation, resolve, getSchema, and
ctx.visitedLocations to locate where to add this guard so the self-reference
case is handled deterministically.
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserContext.java`:
- Around line 28-31: WSDLParserContext.basePath currently mutates the shared
visitedLocations list causing incorrect skipping on failures; change basePath to
avoid in-place mutation by creating a defensive copy of visitedLocations (e.g.,
new ArrayList<>(visitedLocations) or equivalent) and add basePath to that copy,
then construct and return the new WSDLParserContext with the copied list so the
original visitedLocations remains unchanged if parsing fails.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/SchemaElement.java`:
- Around line 22-24: The constructor signature for SchemaElement currently lacks
a space after the comma; update the SchemaElement(WSDLParserContext ctx,Node
node) constructor declaration to add a space after the comma so it reads
SchemaElement(WSDLParserContext ctx, Node node) in the SchemaElement class
(constructor method).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22835ee6-7595-4c1f-929e-09a121376b3c
📒 Files selected for processing (13)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLValidator.javacore/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.javacore/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Binding.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Operation.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Port.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLElement.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLParserContext.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/AbstractIncludeImport.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/Import.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/SchemaElement.javacore/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/Port.java
- core/src/main/java/com/predic8/membrane/core/util/wsdl/parser/WSDLElement.java
…lidation - Introduced `hello-soap12.wsdl` in test resources, implementing a SOAP 1.2 example schema for validation purposes. - Added test cases in `WSDLParserTest` to verify the parsing and validation of SOAP 1.2 bindings (`HelloService`). - Created `WSDLIncludeImportTest` to test schema include/import scenarios, including cyclic and embedded cases. - Improved `RelativePathRewriter` in `WSDLPublisherInterceptor` by refining file existence checks and path resolution. - Minor formatting fixes and refinements in existing code and tests.
- Updated `WSDLMessageElementExtractor` to improve RPC element name handling, ensuring correct direction logic for `INPUT` and `OUTPUT`. - Fixed a formatting issue in `SchemaElement` constructor for better readability. - Simplified lambda usage in `getElementNames` method of `WSDLIncludeImportTest`. - Made `resource` field in `RelativePathRewriter` final for immutability.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java (1)
91-105:⚠️ Potential issue | 🔴 CriticalUse
ResolverMap.resolve()instead ofnew File(path).exists()—it breaks URL-based WSDL configurations.Lines 97–99 and 108 use
new File(path).exists(), which fails for non-file resources:
- The
setWsdl()javadoc (line 172) explicitly documents support for URLs- Test code confirms WSDL URLs like
http://localhost:2001?wsdlare passed to this method- The condition at line 93 allows URLs unchanged (paths containing
"://"are not combined), but thenFile.exists()incorrectly rejects them- Use
router.getResolverMap().resolve(path)instead, following the pattern used elsewhere in the codebase (e.g.,WebServerInterceptor.createResponse()at line 253)Additionally, line 109 contains the debug marker
"!!!!"in an error message, which should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java` around lines 91 - 105, The rewrite(String path) method currently checks resource existence with new File(path).exists(), which incorrectly rejects URL-based WSDLs; replace that check with router.getResolverMap().resolve(path) (or use ResolverMap.resolve(path)) to verify the resource can be resolved and throw ResourceRetrievalException when resolution fails, keeping the rest of the return logic that calls URLUtil.getNameComponent(...) and resolveToNumber(path); also remove the stray "!!!!" from the error/debug message to clean up logging.core/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java (1)
53-67:⚠️ Potential issue | 🟠 MajorDon't blank out relocated URLs on parse failure.
If URL reconstruction fails here, the method returns
"", andReplaceIterator.next()will emit that as the newlocation/schemaLocation. That turns one malformed input into a broken document and also drops the failing value from the log message. Keepaddrunchanged here, or throw with context instead.Proposed fix
public static String getNewLocation(String addr, String protocol, String host, int port, String contextPath) { try { var oldURL = new URL(addr); if (port == -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(); } catch (MalformedURLException e) { - log.error("", e); + log.warn("Failed to relocate URL '{}'. Leaving original value unchanged.", addr, e); + return addr; } - return ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java` around lines 53 - 67, Relocator.getNewLocation currently swallows MalformedURLException and returns an empty string, which causes ReplaceIterator.next() to emit a broken location; change the catch block to either rethrow a descriptive exception or return the original addr instead of "", and ensure the log includes the failing addr and exception (e.g., log.error("Failed to reconstruct URL for addr=" + addr, e)); keep the rest of the URL-building logic unchanged so callers receive the unchanged input when reconstruction fails.
🧹 Nitpick comments (6)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java (1)
49-55: Consider avoiding mutation of theCollectors.toSet()result.The JDK specification doesn't guarantee the mutability of the set returned by
Collectors.toSet(). While it works in practice, creating a newHashSetexplicitly makes the intent clearer.♻️ Suggested refactor
- var namesDocumentStyle = getParts(direction, portTypes.portTypesDocument()) + var result = new HashSet<>(getParts(direction, portTypes.portTypesDocument()) .map(Part::getElementQName) .filter(Objects::nonNull) - .collect(Collectors.toSet()); + .collect(Collectors.toSet())); - namesDocumentStyle.addAll(operationNamesRPC); - return namesDocumentStyle; + result.addAll(operationNamesRPC); + return result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java` around lines 49 - 55, The code mutates the Set returned by Collectors.toSet() (namesDocumentStyle) which may be unmodifiable; change getParts(...).map(...).filter(...).collect(Collectors.toSet()) to be wrapped in a new mutable set (e.g., new HashSet<>(...)) so you can safely call namesDocumentStyle.addAll(operationNamesRPC); locate the logic in WSDLMessageElementExtractor (the namesDocumentStyle variable built from getParts(direction, portTypes.portTypesDocument()) and Part::getElementQName) and replace the direct collect(...) result with a new HashSet copy before calling addAll.core/src/test/java/com/predic8/membrane/core/ws/WSDLIncludeImportTest.java (2)
102-108: Make validation failures fail the test.Right now this only checks that
init()returns. If the validator reports problems through the callback, the test still passes. Capture those messages and assert that none were emitted.Suggested fix
void validator() { + var failures = new ArrayList<String>(); var validator = new WSDLValidator(new ResolverMap(), WSDLIncludeImportTest.class.getResource("/ws/include/include.wsdl").toString(), "TestService", - (msg, exc) -> log.info("Validation failure: {}", msg), true); + (msg, exc) -> failures.add(msg), true); validator.init(); + assertTrue(failures.isEmpty(), () -> "Unexpected validation failures: " + failures); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/ws/WSDLIncludeImportTest.java` around lines 102 - 108, The validator callback currently just logs messages so validation problems don't fail the test; modify the validator() in WSDLIncludeImportTest to capture callback messages (e.g., add a thread-safe list or collection) when constructing the WSDLValidator (replace the lambda (msg, exc) -> log.info(...) with one that appends messages to the list), call validator.init(), and then assert that the collected message list is empty (or fail the test with the collected messages) so any validation failures reported by WSDLValidator cause the test to fail.
43-43: Remove the stray console print.
System.out.printlnwill clutter test output and makes failures harder to scan. The surrounding assertions already cover this case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/ws/WSDLIncludeImportTest.java` at line 43, Remove the stray System.out.println call in the WSDLIncludeImportTest test (the line that prints embedded.getImports()); delete that print statement so the test output isn't cluttered—rely on the existing assertions in WSDLIncludeImportTest to validate behavior instead of printing.core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java (2)
139-143: Field names use snake_case instead of Java camelCase convention.The fields
paths_reverseanddocuments_to_processdeviate from standard Java naming conventions. Consider renaming topathsReverseanddocumentsToProcessfor consistency with the rest of the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java` around lines 139 - 143, Rename the snake_case fields paths_reverse and documents_to_process to use Java camelCase (pathsReverse and documentsToProcess) and update all references accordingly; keep the `@GuardedBy`("paths") annotations unchanged, adjust the field declarations for Map<String,Integer> pathsReverse and Queue<String> documentsToProcess, and refactor every use in methods of the WSDLPublisherInterceptor class (and any inner classes) to the new names to ensure compilation.
145-165: Reentrant lock usage is safe but worth documenting.The
processDocumentsmethod holds thepathslock while callingwi.handleResponse(exc), which eventually triggerspathRewriter.rewrite()via the Relocator (see context snippet fromRelocator.java:141). Therewrite()method then callsresolveToNumber()which re-acquires the same lock.While Java's intrinsic locks are reentrant (so this works correctly), this nested locking pattern could be fragile if future refactoring changes the call chain. A brief comment documenting this intentional reentrant access would aid maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java` around lines 145 - 165, The code currently holds the intrinsic lock on the field paths inside processDocuments while creating a WSDLInterceptor and calling wi.handleResponse(exc), which ends up re-entering the same lock via RelativePathRewriter → Relocator.rewrite() → resolveToNumber(); add a concise inline comment in processDocuments (near the synchronized (paths) block) stating that reentrant acquisition of the paths lock is intentional and safe because Java intrinsic locks are reentrant, and reference the call chain (WSDLInterceptor, RelativePathRewriter, Relocator.rewrite(), resolveToNumber()) so future maintainers understand this design choice and avoid accidental non-reentrant refactors.core/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java (1)
85-94: Use theQNamemap directly in this hot path.
relocatingAttributesis already keyed by the start-elementQName, so scanning the full entry set on every event adds unnecessary iteration and allocations. A direct lookup is simpler and keeps this O(1).Proposed simplification
private XMLEvent process(XMLEventReader parser) throws XMLStreamException { var event = parser.nextEvent(); if (!event.isStartElement()) return event; - return relocatingAttributes.entrySet().stream() - .filter(e -> shouldProcess(e, event)) - .findFirst() - .map(e -> replace(event, e.getValue())) - .orElse(event); + var attribute = relocatingAttributes.get(getElementName(event)); + return attribute != null ? replace(event, attribute) : event; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java` around lines 85 - 94, The process(XMLEventReader) method currently iterates relocatingAttributes.entrySet() for each start element; instead obtain the start element QName via event.asStartElement().getName(), perform a direct lookup in relocatingAttributes (relocatingAttributes.get(qName)), and if the returned value is non-null and shouldProcess(entry, event) is true invoke replace(event, value) otherwise return event; keep using the existing shouldProcess(...) and replace(...) helpers and avoid creating streams/entrySets to eliminate the hot-path allocation and iteration overhead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java`:
- Around line 107-121: In resolveToNumber, remove the redundant File existence
check (the one duplicating rewrite()) and eliminate the debug marker from the
exception message; instead let the method assume rewrite() has validated
existence and, if you must keep a guard, throw a clear RuntimeException like
"Could not find resource: " + path (no multiple exclamation marks). Update
resolveToNumber (referenced variables: paths, paths_reverse,
documents_to_process) to only perform the synchronized lookup/insert logic and
consistent, non-debuggy error text; leave rewrite() validation intact.
In `@core/src/test/java/com/predic8/membrane/core/ws/WSDLIncludeImportTest.java`:
- Around line 91-94: Test currently fetches the first import twice: change the
second retrieval so it actually selects the second import from
first.getImports() (e.g. replace secondImport = first.getImports().getFirst()
with secondImport = first.getImports().get(1) or the equivalent API to access
the second element), then assert the expected element names from
secondImport.getSchema().getSchemaElements() via getElementNames(...); update
the expected List.of(...) to the correct values for the second imported schema
in the embeddedImports() test.
---
Outside diff comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java`:
- Around line 91-105: The rewrite(String path) method currently checks resource
existence with new File(path).exists(), which incorrectly rejects URL-based
WSDLs; replace that check with router.getResolverMap().resolve(path) (or use
ResolverMap.resolve(path)) to verify the resource can be resolved and throw
ResourceRetrievalException when resolution fails, keeping the rest of the return
logic that calls URLUtil.getNameComponent(...) and resolveToNumber(path); also
remove the stray "!!!!" from the error/debug message to clean up logging.
In `@core/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java`:
- Around line 53-67: Relocator.getNewLocation currently swallows
MalformedURLException and returns an empty string, which causes
ReplaceIterator.next() to emit a broken location; change the catch block to
either rethrow a descriptive exception or return the original addr instead of
"", and ensure the log includes the failing addr and exception (e.g.,
log.error("Failed to reconstruct URL for addr=" + addr, e)); keep the rest of
the URL-building logic unchanged so callers receive the unchanged input when
reconstruction fails.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java`:
- Around line 49-55: The code mutates the Set returned by Collectors.toSet()
(namesDocumentStyle) which may be unmodifiable; change
getParts(...).map(...).filter(...).collect(Collectors.toSet()) to be wrapped in
a new mutable set (e.g., new HashSet<>(...)) so you can safely call
namesDocumentStyle.addAll(operationNamesRPC); locate the logic in
WSDLMessageElementExtractor (the namesDocumentStyle variable built from
getParts(direction, portTypes.portTypesDocument()) and Part::getElementQName)
and replace the direct collect(...) result with a new HashSet copy before
calling addAll.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java`:
- Around line 139-143: Rename the snake_case fields paths_reverse and
documents_to_process to use Java camelCase (pathsReverse and documentsToProcess)
and update all references accordingly; keep the `@GuardedBy`("paths") annotations
unchanged, adjust the field declarations for Map<String,Integer> pathsReverse
and Queue<String> documentsToProcess, and refactor every use in methods of the
WSDLPublisherInterceptor class (and any inner classes) to the new names to
ensure compilation.
- Around line 145-165: The code currently holds the intrinsic lock on the field
paths inside processDocuments while creating a WSDLInterceptor and calling
wi.handleResponse(exc), which ends up re-entering the same lock via
RelativePathRewriter → Relocator.rewrite() → resolveToNumber(); add a concise
inline comment in processDocuments (near the synchronized (paths) block) stating
that reentrant acquisition of the paths lock is intentional and safe because
Java intrinsic locks are reentrant, and reference the call chain
(WSDLInterceptor, RelativePathRewriter, Relocator.rewrite(), resolveToNumber())
so future maintainers understand this design choice and avoid accidental
non-reentrant refactors.
In `@core/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java`:
- Around line 85-94: The process(XMLEventReader) method currently iterates
relocatingAttributes.entrySet() for each start element; instead obtain the start
element QName via event.asStartElement().getName(), perform a direct lookup in
relocatingAttributes (relocatingAttributes.get(qName)), and if the returned
value is non-null and shouldProcess(entry, event) is true invoke replace(event,
value) otherwise return event; keep using the existing shouldProcess(...) and
replace(...) helpers and avoid creating streams/entrySets to eliminate the
hot-path allocation and iteration overhead.
In `@core/src/test/java/com/predic8/membrane/core/ws/WSDLIncludeImportTest.java`:
- Around line 102-108: The validator callback currently just logs messages so
validation problems don't fail the test; modify the validator() in
WSDLIncludeImportTest to capture callback messages (e.g., add a thread-safe list
or collection) when constructing the WSDLValidator (replace the lambda (msg,
exc) -> log.info(...) with one that appends messages to the list), call
validator.init(), and then assert that the collected message list is empty (or
fail the test with the collected messages) so any validation failures reported
by WSDLValidator cause the test to fail.
- Line 43: Remove the stray System.out.println call in the WSDLIncludeImportTest
test (the line that prints embedded.getImports()); delete that print statement
so the test output isn't cluttered—rely on the existing assertions in
WSDLIncludeImportTest to validate behavior instead of printing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e4b7a9be-cd03-4f88-b665-4ba7b43b7c2f
📒 Files selected for processing (7)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.javacore/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.javacore/src/main/java/com/predic8/membrane/core/util/wsdl/parser/schema/SchemaElement.javacore/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.javacore/src/test/java/com/predic8/membrane/core/util/soap/WSDLParserTest.javacore/src/test/java/com/predic8/membrane/core/ws/WSDLIncludeImportTest.javacore/src/test/resources/ws/hello-soap12.wsdl
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/com/predic8/membrane/core/util/soap/WSDLParserTest.java
- Split `getPossibleElements` logic in `WSDLMessageElementExtractor` into separate methods for RPC and document styles for better clarity. - Simplified attribute relocation in `Relocator` by removing redundant `shouldProcess` method and streamlining logic. - Fixed test assertions in `WSDLIncludeImportTest` for correcting import validation (`get(1)` usage). - Updated `WSDLPublisherInterceptor` to improve path
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.java`:
- Around line 106-112: In getParts (WSDLMessageElementExtractor.getParts)
replace the final .map(Message::getPart) call with a flatMap that returns all
parts from each Message, e.g. .flatMap(message -> message.getParts().stream()),
so every <part> element is emitted instead of only the first; update imports if
needed and ensure the method still returns Stream<Part>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6dde70a-77c9-43c6-8eec-943adbaea37f
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLMessageElementExtractor.javacore/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.javacore/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.javacore/src/test/java/com/predic8/membrane/core/ws/WSDLIncludeImportTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java
- core/src/test/java/com/predic8/membrane/core/ws/WSDLIncludeImportTest.java
- core/src/main/java/com/predic8/membrane/core/ws/relocator/Relocator.java
rrayst
requested changes
Mar 11, 2026
- Added `normalize` method in `URIUtil` for standardizing paths and URIs. - Synchronized `resolveToNumber` method in `WSDLPublisherInterceptor` for thread safety. - Replaced hardcoded schema namespace with constants in `WSDLSchemaExtractor`. - Removed redundant imports and refactored commentary in multiple classes for clarity. - Added `XMLNS_NS` constant to `Constants`.
…into wsdl-validator-new-wsdl-parser
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.java (1)
146-149:⚠️ Potential issue | 🟠 Major
matchingPortsis no longer a filtered subset.On Line 149,
portsis the same fullservice.getPorts()list captured on Line 120, so every row satisfiesmatchingPorts.contains(p)and gets the* available through this proxymarker.💡 Suggested fix
- else - createEndpointTable(service.getPorts(), ports); + else + createEndpointTable(service.getPorts(), List.of(port));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.java` around lines 146 - 149, The code passes the full service.getPorts() list (ports) into createEndpointTable so matchingPorts.contains(p) is always true; change the call to pass the filtered subset (matchingPorts) or otherwise ensure createEndpointTable is invoked with the filtered list instead of the unfiltered ports list so only actual matching ports receive the "* available through this proxy" marker (look for variables/methods: matchingPorts, ports, service.getPorts(), createEndpointTable).
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLSchemaExtractor.java (1)
37-43:⚠️ Potential issue | 🟠 MajorPreserve the full in-scope namespace context when cloning the schema.
Line 43 only forwards
xmlns*declarations from<wsdl:definitions>. Any prefix or default namespace declared on an intermediate ancestor such as<wsdl:types>is lost, and shadowed root bindings can even be copied back with the wrong URI. That breaks QName resolution inside the extracted standalone schema.💡 Suggested fix
public static List<Document> getSchemas(Element wsdl) { try { var result = new ArrayList<Document>(); var schemas = wsdl.getElementsByTagNameNS(XSD_NS, "schema"); for (int i = 0; i < schemas.getLength(); i++) { - result.add(extractSchema((Element) schemas.item(i), - getNamespaceDeclarations(wsdl))); + var originalSchema = (Element) schemas.item(i); + result.add(extractSchema(originalSchema, + getInScopeNamespaceDeclarations(originalSchema))); } return result; } catch (Exception e) { throw new RuntimeException("Could not extract embedded schemas from WSDL.", e); } } -private static List<Attr> getNamespaceDeclarations(Element element) { - var namespaces = new ArrayList<Attr>(); - var attributes = element.getAttributes(); - for (int i = 0; i < attributes.getLength(); i++) { - var attr = (Attr) attributes.item(i); - if (XMLNS_NS.equals(attr.getNamespaceURI())) { - namespaces.add(attr); - } - } - return namespaces; +private static List<Attr> getInScopeNamespaceDeclarations(Element element) { + var namespaces = new LinkedHashMap<String, Attr>(); + for (Node current = element; current instanceof Element; current = current.getParentNode()) { + var attributes = ((Element) current).getAttributes(); + for (int i = 0; i < attributes.getLength(); i++) { + var attr = (Attr) attributes.item(i); + if (XMLNS_NS.equals(attr.getNamespaceURI())) { + namespaces.putIfAbsent(attr.getName(), attr); + } + } + } + return new ArrayList<>(namespaces.values()); }Also applies to: 51-60, 77-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLSchemaExtractor.java` around lines 37 - 43, The getSchemas method currently only passes namespace declarations from the wsdl root via getNamespaceDeclarations(wsdl) to extractSchema, losing xmlns* declared on intermediate ancestors (e.g., wsdl:types) and breaking QName resolution; modify the code so getSchemas collects the full in-scope namespace context for each schema element by walking that schema Element's ancestor chain up to the document root and merging all xmlns* attributes (closest ancestor/element wins for shadowing) and pass that merged map into extractSchema (replace the call that uses getNamespaceDeclarations(wsdl) with a per-schema ancestor-derived namespace collection), and ensure extractSchema uses that map when cloning/copying the schema so bindings are preserved correctly.
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/util/URIUtil.java (1)
18-20: Redundant explicit import.
import java.net.URI;on line 19 is redundant since line 18 already hasimport java.net.*;.Proposed fix
import java.net.*; -import java.net.URI; import java.nio.file.*;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/util/URIUtil.java` around lines 18 - 20, The import list in URIUtil.java contains a redundant explicit import: remove the duplicate import of java.net.URI since java.net.* already covers it; update the imports at the top of URIUtil.java by deleting the line "import java.net.URI;" so only the wildcard import (import java.net.*;) and necessary other imports remain.core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java (1)
140-151: Shrink thepathscritical section.This block keeps the
pathsmonitor while resolving the resource and rewriting/parsing it. Those calls are much more expensive than the queue mutation, so one slow import can block unrelated?wsdl/?xsdrequests on the same lock.♻️ Suggested refactor
- synchronized (paths) { - try { - while (true) { - var doc = documentsToProcess.poll(); - if (doc == null) - break; - log.debug("processing: {}", doc); - exc.setResponse(webServerInterceptor.createResponse(router.getResolverMap(), doc)); - var wi = new WSDLInterceptor(); - wi.setRewriteEndpoint(false); - wi.setPathRewriter(new RelativePathRewriter(exc, doc)); - wi.handleResponse(exc); - } - } catch (Exception e) { - log.error("Could not recursively load all documents referenced by {}.", wsdl, e); - } - } + try { + while (true) { + var doc; + synchronized (paths) { + doc = documentsToProcess.poll(); + if (doc == null) + break; + } + log.debug("processing: {}", doc); + exc.setResponse(webServerInterceptor.createResponse(router.getResolverMap(), doc)); + var wi = new WSDLInterceptor(); + wi.setRewriteEndpoint(false); + wi.setPathRewriter(new RelativePathRewriter(exc, doc)); + wi.handleResponse(exc); + } + } catch (Exception e) { + log.error("Could not recursively load all documents referenced by {}.", wsdl, e); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java` around lines 140 - 151, The synchronized(paths) block should be reduced to only the minimal queue mutation: inside the synchronized block poll documentsToProcess and assign the result to a local variable (e.g., var doc = documentsToProcess.poll()) and break if null; then perform heavy work (call webServerInterceptor.createResponse(router.getResolverMap(), doc), exc.setResponse(...), new WSDLInterceptor(), wi.setRewriteEndpoint(false), wi.setPathRewriter(new RelativePathRewriter(exc, doc)), wi.handleResponse(exc)) outside the synchronized block; ensure any shared mutable state touched by the moved code remains thread-safe or is copied into locals before leaving the synchronized scope (references: paths, documentsToProcess, exc, webServerInterceptor.createResponse, WSDLInterceptor, RelativePathRewriter, handleResponse).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java`:
- Around line 211-214: In WSDLPublisherInterceptor (around the exc handling that
calls getParams and parses params.get("xsd")), guard against non-numeric xsd
values by validating the string before Integer.parseInt or by catching
NumberFormatException and turning that path into an HTTP 400 Bad Request (set
the exchange/response status and return a short explanatory body) instead of
letting the exception bubble as a 500; apply the same change to the other
identical parse site later in the method so any malformed ?xsd= input is
consistently reported as 400.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.java`:
- Around line 118-140: The code always selects the first port
(service.getPorts().getFirst()) which ignores any configured portName; change
the logic in WebServiceExplorerInterceptor to look up the Port by the configured
portName (use the portName field / setPortName accessor) from service.getPorts()
and if found use that Port, otherwise fall back to getFirst(); ensure the local
variables ports, port and binding are derived from that selected Port and keep
the call createOperationsTable(definitions, binding, binding.getPortType())
using the resolved binding/portType so the explorer renders the correct port
when a portName is set.
In `@core/src/main/java/com/predic8/membrane/core/util/URIUtil.java`:
- Around line 152-175: Update the Javadoc for the method normalize in class
URIUtil to document that IllegalArgumentException can also be thrown for
malformed URIs produced by URI.create(), i.e., change the `@throws` text to
something like "IllegalArgumentException if the input location is null, empty,
or contains malformed URI syntax" so callers are informed that
URI.create(location).normalize() may raise the same exception.
---
Outside diff comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.java`:
- Around line 146-149: The code passes the full service.getPorts() list (ports)
into createEndpointTable so matchingPorts.contains(p) is always true; change the
call to pass the filtered subset (matchingPorts) or otherwise ensure
createEndpointTable is invoked with the filtered list instead of the unfiltered
ports list so only actual matching ports receive the "* available through this
proxy" marker (look for variables/methods: matchingPorts, ports,
service.getPorts(), createEndpointTable).
---
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLSchemaExtractor.java`:
- Around line 37-43: The getSchemas method currently only passes namespace
declarations from the wsdl root via getNamespaceDeclarations(wsdl) to
extractSchema, losing xmlns* declared on intermediate ancestors (e.g.,
wsdl:types) and breaking QName resolution; modify the code so getSchemas
collects the full in-scope namespace context for each schema element by walking
that schema Element's ancestor chain up to the document root and merging all
xmlns* attributes (closest ancestor/element wins for shadowing) and pass that
merged map into extractSchema (replace the call that uses
getNamespaceDeclarations(wsdl) with a per-schema ancestor-derived namespace
collection), and ensure extractSchema uses that map when cloning/copying the
schema so bindings are preserved correctly.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java`:
- Around line 140-151: The synchronized(paths) block should be reduced to only
the minimal queue mutation: inside the synchronized block poll
documentsToProcess and assign the result to a local variable (e.g., var doc =
documentsToProcess.poll()) and break if null; then perform heavy work (call
webServerInterceptor.createResponse(router.getResolverMap(), doc),
exc.setResponse(...), new WSDLInterceptor(), wi.setRewriteEndpoint(false),
wi.setPathRewriter(new RelativePathRewriter(exc, doc)), wi.handleResponse(exc))
outside the synchronized block; ensure any shared mutable state touched by the
moved code remains thread-safe or is copied into locals before leaving the
synchronized scope (references: paths, documentsToProcess, exc,
webServerInterceptor.createResponse, WSDLInterceptor, RelativePathRewriter,
handleResponse).
In `@core/src/main/java/com/predic8/membrane/core/util/URIUtil.java`:
- Around line 18-20: The import list in URIUtil.java contains a redundant
explicit import: remove the duplicate import of java.net.URI since java.net.*
already covers it; update the imports at the top of URIUtil.java by deleting the
line "import java.net.URI;" so only the wildcard import (import java.net.*;) and
necessary other imports remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb6d9ef8-48f7-427d-9eb1-129ecbb2f065
📒 Files selected for processing (6)
annot/src/main/java/com/predic8/membrane/annot/Constants.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/WSDLSchemaExtractor.javacore/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.javacore/src/main/java/com/predic8/membrane/core/util/URIUtil.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java
rrayst
previously approved these changes
Mar 12, 2026
…ion handling, and update references in tests and implementations
rrayst
approved these changes
Mar 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests