fix: compute URL expressions in the target after request flow#2790
Merged
rrayst merged 19 commits intoFeb 20, 2026
Conversation
…http client interceptor
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:
📝 WalkthroughWalkthroughThis PR refactors URI handling and expression evaluation in the framework. It introduces runtime URL template evaluation with customizable escaping strategies, replaces compile-time Target URL computation with runtime Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DispatchingInterceptor
participant Target
participant URIFactory
participant TemplateExpressionEvaluator
Client->>DispatchingInterceptor: handleRequest(Exchange)
DispatchingInterceptor->>Target: applyModifications(Exchange, Router)
Target->>TemplateExpressionEvaluator: evaluateTemplate(targetURL, Exchange)
TemplateExpressionEvaluator->>URIFactory: create(evaluatedURL)
URIFactory-->>TemplateExpressionEvaluator: URI with safe characters
TemplateExpressionEvaluator-->>Target: resolved destination
Target->>Target: applyEscaping(result)
Target->>Target: applyMethodOverride(Exchange)
Target-->>DispatchingInterceptor: modified Exchange
DispatchingInterceptor-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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/DispatchingInterceptor.java (1)
110-122:⚠️ Potential issue | 🟡 MinorUse lenient URI parsing consistently or skip resolution for expression URLs with root basePath.
Lines 26 and 28 use incompatible URI parsers:
UriUtil.getPathFromURL(new URIFactory(true), targetURL)on line 26 tolerates expression syntax (${}), but the standardjava.net.URI(targetURL)constructor on line 28 does not. When a target URL contains expressions in the authority (e.g.,http://${host}/) and evaluates to a root basePath, line 28 throwsURISyntaxExceptiondespite the intent (expressed in the comment) to support such expressions.The proposed check for
"${"is a pragmatic defensive fix:if (basePath == null || basePath.isEmpty() || "/".equals(basePath)) { + if (targetURL.contains("${")) { + return targetURL; + } URI base = new URI(targetURL); // Resolve and normalize slashes consistently with the branch below. return base.resolve(getUri(exc)).toString(); }
🤖 Fix all issues with AI agents
In `@core/src/main/java/com/predic8/membrane/core/http/Message.java`:
- Around line 89-101: Replace the empty warn log and add removal of
TRANSFER_ENCODING in Message.emptyBody: instead of log.warn("", e) use the same
logging style as discardBody (log.debug with a short descriptive message and the
exception) to handle the IOException, and after setting body = new EmptyBody()
remove the TRANSFER_ENCODING header (call
header.removeFields(TRANSFER_ENCODING)) similar to what setBodyContent() does;
update Message.emptyBody to mirror discardBody's logging level/format and to
clear the transfer-encoding header.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptor.java`:
- Line 43: The constant PROXIES_HINT in HTTPClientInterceptor currently
references the wrong config filename; update its string from "conf/apies.xml" to
"conf/proxies.xml" so the hint points to the correct configuration file (edit
the PROXIES_HINT field in class HTTPClientInterceptor to replace "apies.xml"
with "proxies.xml").
- Around line 70-73: The call to applyTargetModifications in
HTTPClientInterceptor.handleRequest is executed before the surrounding try block
so any runtime exceptions from expression evaluation (Target.applyModifications
→ computeDestinationExpressions, e.g., SpelEvaluationException or NPE) escape
the existing handlers; move the applyTargetModifications(exc) invocation inside
the existing try block (or wrap it in its own try-catch) and on caught
exceptions produce a controlled error handling path (set an appropriate error
response on the Exchange and return the correct Outcome) so
expression-evaluation failures are logged and return a formatted 500-style
response instead of letting exceptions propagate.
In
`@core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java`:
- Around line 258-260: The urlEncode method currently calls URLEncoder.encode(s,
UTF_8) which throws NullPointerException for null inputs; update
CommonBuiltInFunctions.urlEncode to guard against null (e.g., if s is null
return "" like pathSeg does) before calling URLEncoder.encode using the UTF_8
constant so scripts won’t crash on null values.
In `@core/src/main/java/com/predic8/membrane/core/util/URLUtil.java`:
- Around line 24-30: The method getHost currently returns the authority
(host:port) rather than just the hostname; rename the method to getAuthority or
getHostPort (or update its javadoc) to reflect that behavior, update all call
sites to the new name, and if you need a pure hostname add a new method (e.g.,
getHostname) that strips any ":port" from the returned value; reference the
existing getHost method when making these changes so callers and documentation
stay consistent.
🧹 Nitpick comments (6)
core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
172-183: Consider clearing body for all body-less methods, not just GET.
changeMethodonly clears the body when switching toGET, butmethodsWithoutBody(line 51) also includesHEADandCONNECT. If the intent is to enforce body semantics on method change, the check could use the existing set:if (methodsWithoutBody.contains(newMethod.toUpperCase())) emptyBody();If only
GETis intentional for the current use case, this is fine as-is — just flagging for consideration.core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
262-273: Minor Javadoc inconsistency:@param segment ... must not be nullbutURLUtil.pathSegaccepts null.The underlying
URLUtil.pathSeg(Object)gracefully handles null by returning"". The Javadoc here says "must not be null", which could confuse callers. Consider aligning the contract — either document that null returns empty, or add an explicit null check here.core/src/main/java/com/predic8/membrane/core/interceptor/DispatchingInterceptor.java (1)
24-25: Redundant explicitURIimport.Line 24 imports
java.net.*which already coversjava.net.URI. Line 25's explicitimport java.net.URI;is redundant.Remove the redundant import
import java.net.*; -import java.net.URI;core/src/main/java/com/predic8/membrane/core/proxies/Target.java (2)
60-67: Minor:asp.getTarget()isthis— consider using direct field access for clarity.
applyModificationsis an instance method called onasp.getTarget(), soasp.getTarget().getMethod()on Line 64 is equivalent tothis.method. Usingthis.method != nullandexc.getRequest().changeMethod(this.method)would be more direct and avoid the indirection.Simplify self-reference
public void applyModifications(Exchange exc, AbstractServiceProxy asp, Router router) { computeDestinationExpressions(exc, asp, router); // Changing the method must be the last step cause it can empty the body! - if (asp.getTarget().getMethod() != null) { - exc.getRequest().changeMethod(asp.getTarget().getMethod()); + if (method != null) { + exc.getRequest().changeMethod(method); } }
69-78: Expression parsing happens per-request for each destination.
TemplateExchangeExpression.newInstance()creates a new instance for each destination URL on every request. For SPEL (the default language), this triggersSpelExpressionParser.parseExpression(), and for other languages, it parses tokens via regex matching. This occurs even for plain URLs containing no${...}expressions.Consider caching compiled expressions keyed by the raw URL string, or fast-pathing plain URLs that contain no expression markers.
core/src/test/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptorTest.java (1)
54-65: Minor inconsistency: these tests still instantiatenew DefaultRouter()instead of using the sharedrouterfield.The shared
routerfield was introduced and used inprotocolUpgradeRejectedandcomputeTargetUrl, but these two tests still create their own instances. Consider reusing the shared field for consistency.♻️ Proposed fix
`@Test` void passFailOverOn500Default() { - hci.init(new DefaultRouter()); + hci.init(router); assertFalse(hci.getHttpClientConfig().getRetryHandler().isFailOverOn5XX()); } `@Test` void passFailOverOn500() { hci.setFailOverOn5XX(true); - hci.init(new DefaultRouter()); + hci.init(router); assertTrue(hci.getHttpClientConfig().getRetryHandler().isFailOverOn5XX()); }
…URL handling and modify target configurations - Introduced `computeDestinationExpressions` to streamline target URL modification logic. - Improved `DispatchingInterceptor` handling of base path resolution with illegal characters. - Enhanced `urlEncode` and `pathSeg` utility functions for safety and clarity. - Refactored `HTTPClientInterceptor` to adjust the target flow and error messages. - Added extensive URI tests, including custom-parsed and relative path resolution scenarios. - Updated `applyTargetModifications` for streamlined behavior during request processing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@core/src/main/java/com/predic8/membrane/core/http/Request.java`:
- Around line 172-181: The changeMethod method can NPE when this.method or
newMethod is null; update changeMethod to defensively handle nulls by first
validating newMethod (either return or throw IllegalArgumentException if
newMethod is null), then compare using a null-safe check (e.g. if (this.method
!= null && this.method.equalsIgnoreCase(newMethod)) return) or invert to compare
newMethod.equalsIgnoreCase(this.method) after validating newMethod, and when
checking methodsWithoutBody avoid calling newMethod.toUpperCase() on a null (use
newMethod.toUpperCase(Locale.ROOT) only after null check or use
methodsWithoutBody.contains(newMethod.toUpperCase(Locale.ROOT)) guarded by
non-null); keep references to changeMethod, methodsWithoutBody, and emptyBody
when applying the fix.
In `@core/src/main/java/com/predic8/membrane/core/util/URI.java`:
- Around line 285-300: The custom-parsed resolve in URI.resolve can produce
"scheme://null..." when authority is null and also mishandles relative paths
that don't start with '/'; update the resolve(URI relative) method so that when
authority==null it emits scheme:... (no "://null") and so that relative paths
not starting with '/' are merged per RFC 3986 §5.2/5.3: compute basePath =
this.getRawPath(), strip everything after the last '/' (if any) and append the
relative raw path, ensuring the merged path has the correct leading '/'; then
append query if present (relative.getRawQuery()). Keep the existing behavior for
absolute relative paths (starting with '/') and reuse getRawPath/getRawQuery and
scheme/authority fields to construct the result for both cases. Ensure you
reference URI.resolve and the authority/scheme/getRawPath/getRawQuery fields
when making the change.
In `@core/src/main/java/com/predic8/membrane/core/util/URLUtil.java`:
- Around line 24-30: getAuthority currently assumes uri has characters past the
scheme colon and will call uri.charAt(i) without bounds checks, which can throw
StringIndexOutOfBoundsException for inputs like "" or ":" or "http://"; update
getAuthority to defensively validate uri and index i before accessing chars:
check for null/empty uri, ensure i < uri.length() before entering the while loop
and when calling uri.charAt(i), and handle the case where no authority exists
(e.g., return empty string or null) before computing j; reference getAuthority,
variables i and j, and the uri.charAt(i) loop when making the change.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/DispatchingInterceptorTest.java`:
- Around line 152-165: The test getAddressFromTargetElement currently only
prints the computed URL and has no assertion; update it to assert the expected
resolved address (for the Target URL "https://${property.host}:8080" with
exc.setProperty("host","predic8.de")) by calling
dispatcher.getAddressFromTargetElement(exc) and asserting it equals
"https://predic8.de:8080" (use the test framework's assertion method),
referencing the APIProxy/Target setup and the
dispatcher.getAddressFromTargetElement call so the test fails if resolution is
incorrect.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptorTest.java`:
- Around line 67-82: The DispatchingInterceptor in the test is used without
initialization which can NPE in some cases; create the interceptor instance,
call init(router) on it before invoking handleRequest(exc) (e.g., var di = new
DispatchingInterceptor(); di.init(router); di.handleRequest(exc)), so the
interceptor has a router set just like hci.init(router) is done for
HTTPClientInterceptor; keep the rest of the test intact and ensure you call
di.init(router) prior to di.handleRequest.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/proxies/Target.java (1)
63-79: Clean separation of concerns — runtime expression evaluation.The
applyModificationsmethod cleanly centralizes destination resolution and method override. The comment about method change ordering (Line 66) is helpful.One consideration:
TemplateExchangeExpression.newInstanceis invoked per destination per request, which involves regex parsing and (for SpEL) expression compilation. For high-throughput proxies with plain target URLs (no${}), this adds overhead on every request. If this becomes a bottleneck, consider short-circuiting when the URL contains no${}placeholders.Optional: skip template evaluation for plain URLs
private String evaluateTemplate(Exchange exc, Router router, String url, InterceptorAdapter adapter) { + if (!url.contains("${")) { + return url; + } return TemplateExchangeExpression.newInstance(adapter, language, url, router).evaluate(exc, REQUEST, String.class); }
…t URLs - Introduced `TemplateUtil` with a method to check for template markers. - Enhanced `TemplateExchangeExpression` to support URL encoding for dynamic templates. - Updated `Target` to skip unnecessary evaluation for URLs without template markers. - Added extensive tests to verify correct URL encoding across various languages (Groovy, SpEL, XPath, JSONPath).
… target URLs
- Documented the change to URL encode expressions inside `${}` in target URLs.
- Updated tests to improve validation coverage and logging for clearer output.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java`:
- Around line 79-95: In evaluateToString, guard against encoder.apply(null)
causing an NPE: after var value = token.eval(exchange, flow, String.class) check
if token is an Expression and value==null, and in that case substitute a safe
string (e.g., "" or "null" per project convention) before calling encoder.apply;
update the branch that currently does line.append(encoder.apply(value)) to
compute a safeValue = (value == null ? "" : value) and call
encoder.apply(safeValue) so null evaluations no longer cause a
NullPointerException.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptorTest.java`:
- Around line 101-112: The test method computeTargetUrlWithEncodingXPath uses
post("/foo").json(...) while supplying XML; change the request builder call to
post("/foo").xml(...) so the Content-Type matches the XML body—update the
builder invocation in the test (computeTargetUrlWithEncodingXPath / variable
exc) to use .xml(...) instead of .json(...) to match other XPath tests and
maintain semantic consistency.
🧹 Nitpick comments (5)
core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetHeaderInterceptorJsonpathTest.java (1)
25-26: Redundant import:assertTrueis already covered by the wildcard import on line 25.Line 26 can be removed since
import static org.junit.jupiter.api.Assertions.*already importsassertTrue.Suggested fix
import static org.junit.jupiter.api.Assertions.*; -import static org.junit.jupiter.api.Assertions.assertTrue;core/src/main/java/com/predic8/membrane/core/util/TemplateUtil.java (1)
25-32: Null input will throw an unguarded NPE.If
nullis ever passed (e.g., a destination URL that isnull),s.length()will throw aNullPointerExceptionwith no helpful message. Consider adding an early null check or documenting the precondition.Proposed defensive guard
public static boolean containsTemplateMarker(String s) { + if (s == null) return false; for (int i = 0, len = s.length() - 1; i < len; i++) {core/src/main/java/com/predic8/membrane/core/proxies/Target.java (1)
75-90: Template expressions are parsed and compiled on every request.
evaluateTemplatecreates a newTemplateExchangeExpression(regex match + token compilation) for each destination URL on every invocation. For targets with template markers, this work is repeated per-request.If target URLs with expressions are common or the gateway handles high throughput, consider caching the compiled
TemplateExchangeExpressionper unique URL string. That said, for typical use cases this is likely acceptable.core/src/test/java/com/predic8/membrane/core/util/TemplateUtilTest.java (1)
24-30: Consider adding a few more edge cases and splitting into named tests.The current assertions cover the core scenarios well. A couple of additional boundary cases would strengthen confidence, especially a marker at the very start or end of the string, and a single
$character.Suggested additional assertions
`@Test` void test() { assertFalse(containsTemplateMarker("")); assertFalse(containsTemplateMarker("foo")); assertTrue(containsTemplateMarker("foo${bar")); assertFalse(containsTemplateMarker("foo$x{bar")); + assertTrue(containsTemplateMarker("${")); + assertFalse(containsTemplateMarker("$")); + assertTrue(containsTemplateMarker("end${")); }core/src/test/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptorTest.java (1)
114-127: Rename auto-generated helper methodextracted.The name
extractedis an IDE refactoring artifact and doesn't convey the method's purpose. A name likeassertTargetUrlEncodingorverifyEncodedDestinationwould improve readability.Suggested rename
- private void extracted(Language language, Exchange exc, String url, String expected) { + private void assertTargetUrlEncoding(Language language, Exchange exc, String url, String expected) {
- Introduced a `templateExpressionCache` in `Target` to avoid redundant parsing and compilation of frequently used expressions. - Improved token parsing and evaluation logic in `TemplateExchangeExpression`, enhancing clarity and performance. - Renamed test helper method `extracted` to `testExpression` for better readability. - Updated test cases across multiple classes to cover new enhancements and ensure consistent behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/src/main/java/com/predic8/membrane/core/proxies/Target.java`:
- Around line 83-86: computeDestinationExpressions currently returns
Stream.toList() which produces an unmodifiable list that downstream code
mutates; change the terminal operation to use .collect(Collectors.toList()) so
the returned list is mutable. In computeDestinationExpressions, keep the
InterceptorAdapter(router, xmlConfig) and the evaluateTemplate(exc, router, url,
adapter) mapping but replace .toList() with .collect(Collectors.toList()) and
add the necessary import for java.util.stream.Collectors so
exc.setDestinations(...) and downstream interceptors (e.g., OpenAPIInterceptor,
InternalRoutingInterceptor, DestinationInterceptor, REST2SOAPInterceptor,
RewriteInterceptor) can safely modify the list.
- Line 60: The field templateExpressionCache in class Target is using a
non-thread-safe HashMap which can corrupt state when computeIfAbsent is invoked
concurrently; replace the HashMap with a java.util.concurrent.ConcurrentHashMap
(i.e., change the declaration of templateExpressionCache to new
ConcurrentHashMap<>()) and ensure any code referring to templateExpressionCache
(e.g., the computeIfAbsent usage inside Target) continues to work with the
concurrent map; also update imports as needed to use ConcurrentHashMap.
🧹 Nitpick comments (1)
core/src/test/java/com/predic8/membrane/core/util/TemplateUtilTest.java (1)
24-33: Consider more descriptive test method name and a few additional edge cases.
test()is quite generic. Consider splitting into or renaming to something likecontainsTemplateMarker()or using@ParameterizedTestfor clarity.A couple of additional edge cases worth covering:
- A complete template expression:
"${foo}"→true- Null input: the current
containsTemplateMarkerimplementation will throw an NPE onnull. If that's intentional, a test documenting that (e.g.,assertThrows(NullPointerException.class, ...)) would make the contract explicit.
…simplify logic - Eliminated `templateExpressionCache` to reduce complexity and improve maintainability. - Updated `computeDestinationExpressions` to use `Collectors.toList()` for a mutable list. - Simplified `evaluateTemplate` by removing cache-related logic and comments on minor performance differences.
…n `Target` - Added `Escaping` enum with options `NONE`, `URL`, and `SEGMENT` to enhance placeholder escaping in URLs. - Updated `Target` to include configurable escaping behavior and a helper method `getEscapingFunction()`. - Extended test cases to validate new escaping options in various scenarios.
rrayst
requested changes
Feb 16, 2026
rrayst
requested changes
Feb 16, 2026
- Updated `URI` to leverage `var` for better readability and added JavaDoc for `resolve` method. - Improved `ResolverMap` handling of URI schemes, including `classpath:` and `internal:`. - Added new tests in `HTTPClientInterceptorTest` for complete path computation, including URL encoding.
|
This pull request needs "/ok-to-test" from an authorized committer. |
Member
Author
|
/ok-to-test |
…e template evaluation - Added `UriIllegalCharacterDetector` for validating URI components with RFC 3986 compliance and optional extensions. - Updated `Target` to skip template evaluation for URLs without placeholders and log warnings for insecure configurations. - Enhanced `TemplateUtil.containsTemplateMarker` with null safety. - Improved `RewriteInterceptor` to handle query parsing errors with more informative logging. - Refactored tests and added new ones to validate changes.
- Refactored `removeLastSegment` method for concise logic using `Math.max()` to avoid redundant checks. - Replaced `assertEquals(false, ...)` with `assertFalse(...)` for improved readability in tests. - Cleaned up unused imports in `ResolverMapCombineTest`.
- Replaced `pathSeg` method with `pathEncode` for improved naming clarity and functionality in line with RFC 3986. - Introduced `IPv6Util` for robust validation of IPv6 address text, ensuring only allowed characters are accepted. - Refactored `Target` to include URL encoding strategy through a dedicated `escapingFunction` and removed redundant logic. - Streamlined URI combination logic in `ResolverMap` with utility methods for handling slashes and file URIs. - Added extensive tests covering URL path encoding, IPv6 address validation, and robustification of URI logic for edge cases.
… and utilities - Adjusted visibility modifiers of constants to `final` for better immutability (`URITest`). - Reordered equality assertion arguments for better readability (`TargetTest`). - Replaced redundant comments with concise `<p/>` tags in JavaDocs for uniformity. - Refactored tests to leverage `assertNull` for null comparisons (`FileUtilTest`). - Removed unused method `stripIpv6BracketsIfNeeded` and redundant imports for cleaner code. - Enhanced URI combination logic and streamlined utility imports (`DispatchingInterceptor`).
Member
Author
|
@coderabbitai plz review |
Contributor
|
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/com/predic8/membrane/core/util/URI.java (1)
192-198:⚠️ Potential issue | 🔴 Critical
getHost()andgetPort()will throwNullPointerExceptionfor URIs without an authority.When
authorityisnull(e.g., relative path"/foo"),processAuthority()returns early at line 93 without initializinghostPort. CallinggetHost()(line 193) orgetPort()(line 197) then dereferencesnull, causing a NullPointerException.This path is exercised in tests (URITest.java line 615 creates
URI("/foo", true)) and production code—for example, AbstractExchange.java line 314 callsnew URI(getOriginalRequestUri()).getHost()without checking if authority exists.Proposed null guard
public String getHost() { + if (hostPort == null) return null; return hostPort.host; } public int getPort() { + if (hostPort == null) return -1; return hostPort.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/util/URI.java` around lines 192 - 198, The getters URI.getHost() and URI.getPort() can NPE because hostPort may remain null when processAuthority() returns early for URIs without an authority; update getHost() and getPort() to check if hostPort is null and return safe defaults (null for host, -1 for port) instead of dereferencing hostPort, so callers like new URI(...).getHost() won't throw; adjust any related javadoc/comments on getPort()/getHost() to document these return values and ensure processAuthority()/hostPort initialization need not change.core/src/main/java/com/predic8/membrane/core/resolver/ResolverMap.java (1)
99-111:⚠️ Potential issue | 🟠 MajorBug:
URIFactoryis lost during recursivecombinewith >2 locations.When
locations.length > 2, line 107 callscombine(combine(l), locations[...])— the 2-argumentcombine(String...)overload on line 95, which hardcodesDEFAULT_URI_FACTORY. Thefactoryparameter originally passed by the caller is silently discarded.Although this code path is never triggered in the current codebase (all multi-location calls use the no-factory variant), the public API contract is violated. Any external caller invoking
combine(customFactory, location1, location2, location3)would experience silent incorrect behavior.Proposed fix — propagate factory through recursion
private static String combineInternal(URIFactory factory,String... locations) { if (locations.length < 2) throw new InvalidParameterException(); if (locations.length > 2) { // lfold String[] l = new String[locations.length - 1]; System.arraycopy(locations, 0, l, 0, locations.length - 1); - return combine(combine(l), locations[locations.length - 1]); + return combine(factory, combine(factory, l), locations[locations.length - 1]); } return combineInternal2( factory, locations,locations[1], locations[0]); }🤖 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/resolver/ResolverMap.java` around lines 99 - 111, The recursive branch in combineInternal(URIFactory factory,String... locations) loses the provided URIFactory by calling the no-factory overload combine(combine(l), locations[last]) which uses DEFAULT_URI_FACTORY; change the recursive call to propagate the original factory, e.g. call the overload that accepts a URIFactory so the call becomes combine(factory, combine(l), locations[locations.length-1]) (or equivalently recurse via combineInternal with the factory), ensuring URIFactory is not discarded; adjust references to combineInternal2 only where needed and keep combineInternal, combine, combineInternal2 and URIFactory semantics intact.
🧹 Nitpick comments (12)
core/src/main/java/com/predic8/membrane/core/util/TemplateUtil.java (2)
17-17: Add a private no-arg constructor to prevent instantiation.
TemplateUtilis a pure-static utility class. Without an explicit private constructor, the compiler synthesises apublicdefault one, allowingnew TemplateUtil()at call sites.♻️ Proposed fix
public class TemplateUtil { + + private TemplateUtil() {} +🤖 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/TemplateUtil.java` at line 17, TemplateUtil is a pure-static utility class and needs a private no-arg constructor to prevent instantiation; add a private TemplateUtil() { } (optionally throw new AssertionError("No instances")); to the TemplateUtil class to stop the compiler from generating a public default constructor and ensure callers cannot do new TemplateUtil().
25-33: Replace the manual loop withString.contains("${").The comment claims "Fast implementation", but HotSpot's
String.contains→String.indexOfpath uses native SIMD intrinsics and is at least as fast as the hand-rolled loop. Usingcontainsis also clearer and eliminates the opportunity for an off-by-one mistake in future edits.♻️ Proposed refactor
public static boolean containsTemplateMarker(String s) { - if (s == null) return false; - for (int i = 0, len = s.length() - 1; i < len; i++) { - if (s.charAt(i) == '$' && s.charAt(i + 1) == '{') { - return true; - } - } - return false; + return s != null && s.contains("${"); }🤖 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/TemplateUtil.java` around lines 25 - 33, The containsTemplateMarker method currently uses a manual char-by-char loop which is unnecessary and error-prone; replace its body to use String.contains("${") (or s.indexOf("${") >= 0) to detect the marker, retaining the null check and method name containsTemplateMarker so the change is localized and clearer.core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeFalseArrayQueryParameterTest.java (1)
113-117:valuesUTF8is misplaced in theValidnested class.The test now asserts an error condition (1 error with "Invalid query string" for raw non-ASCII characters), making it semantically identical to
wrongCharacterin theInvalidclass. It should be moved there and renamed to make its intent clear — e.g.,rawUTF8InQueryStringIsInvalid. The companion testvaluesAreDecodedalready covers the valid (percent-encoded) UTF-8 case inValid.♻️ Proposed refactor: move and rename the test to `Invalid`
Remove from
Valid:- `@Test` - void valuesUTF8() { - var err = validator.validate(get().path("/array?const=foo,äöü,baz")); - assertEquals(1, err.size()); - assertTrue(err.get(0).getMessage().contains("Invalid query string")); - }Add to the
Invalidnested class:+ `@Test` + void rawUTF8InQueryStringIsInvalid() { + var err = validator.validate(get().path("/array?const=foo,äöü,baz")); + assertEquals(1, err.size()); + assertTrue(err.get(0).getMessage().contains("Invalid query string")); + }🤖 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/openapi/oas31/parameters/ExplodeFalseArrayQueryParameterTest.java` around lines 113 - 117, The test method valuesUTF8 currently lives inside the Valid nested class but asserts an error, so move it into the Invalid nested class and rename it to rawUTF8InQueryStringIsInvalid (or similar) to make intent clear; ensure its body remains the same (calling validator.validate(get().path("/array?const=foo,äöü,baz")) and asserting a single error containing "Invalid query string"), and keep the existing valuesAreDecoded test in Valid unchanged to cover the percent-encoded UTF‑8 case; also check for any duplicate semantics with wrongCharacter and consolidate if necessary.core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java (1)
124-127: Exact error count and message string coupling may become fragile maintenance points.Two minor concerns:
assertEquals(1, err.size())assumes theUriIllegalCharacterDetectoremits exactly one error for the valueäöü(3 non-ASCII codepoints). If the detector is ever changed to report per-character (producing 3 errors), this assertion silently becomes a regression rather than a spec.
err.get(0).getMessage().contains("Illegal character")hard-couples the test to a specific English error message literal from the new detector. A message wording change or future i18n layer would silently break this test.Consider extracting the expected message into a named constant shared with
UriIllegalCharacterDetector, or at minimum using a less fragile substring:💡 Suggested approach
+ // Pin to at least 1 error; exact count depends on per-value vs per-char reporting - assertEquals(1, err.size()); + assertFalse(err.isEmpty()); assertTrue(err.get(0).getMessage().contains("Illegal character"));Or, if "exactly 1 per invalid parameter value" is an intentional contract, add a comment documenting that invariant so future authors know not to change the detector's reporting granularity without updating this test.
Also,
var errhere is the only use ofvarin the file; all other tests declareValidationErrors errexplicitly. Minor inconsistency, but worth aligning.🤖 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/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java` around lines 124 - 127, The test couples to implementation details: in ExplodedArrayQueryParameterTest change the fragile assertions around validator.validate(...) — replace the exact count assertion (assertEquals(1, err.size())) with a less brittle check (e.g., assertTrue(err.size() >= 1) or assertTrue(err.stream().anyMatch(...)) ) unless the contract of UriIllegalCharacterDetector explicitly guarantees one error per parameter value (in which case add a comment stating that invariant near the test); also avoid hard‑coding the English message by comparing against a shared constant from UriIllegalCharacterDetector or by using a language-agnostic check such as matching a small stable substring or checking for the detector’s error code; finally, replace the lone usage of var with the explicit type ValidationErrors for consistency with other tests.core/src/main/java/com/predic8/membrane/core/util/URIValidationUtil.java (2)
60-75:validateHost— the explicit'.'check on line 67 is redundant.
'.'is already covered byisUnreserved(c)(line 32:c == '.'), so thec == '.'on line 67 is always true before reaching line 70. Not a bug — just a minor readability nit. Keeping it explicit is fine if the intent is documentation.🤖 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/URIValidationUtil.java` around lines 60 - 75, The check for '.' in validateHost is redundant because isUnreserved(c) already returns true for '.', so remove the explicit c == '.' branch in the character whitelist inside the validateHost method; update the condition that currently reads if (c == '.' || c == '%' || c == ':' || c == '[' || c == ']') to drop the '.' entry and keep the others, referencing the validateHost method and isUnreserved helper to locate and verify the change.
17-17: Inconsistent utility class pattern —URIValidationUtilis notfinaland lacks a private constructor.
UriIllegalCharacterDetectoris declaredpublic final classwith a private constructor to prevent instantiation. This class should follow the same pattern for consistency, since all methods are static.Proposed fix
-public class URIValidationUtil { +public final class URIValidationUtil { + + private URIValidationUtil() { + }🤖 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/URIValidationUtil.java` at line 17, The utility class URIValidationUtil is mutable and instantiable but only contains static methods; make it a proper utility by declaring the class final and adding a private no-arg constructor (e.g., private URIValidationUtil() { } ) to prevent instantiation, leaving all existing static methods untouched; mirror the pattern used in UriIllegalCharacterDetector to ensure consistency across utility classes.core/src/main/java/com/predic8/membrane/core/util/UriIllegalCharacterDetector.java (2)
17-19: Null safety:uriparameter is not null-checked, unlikeoptions.
validateAll(URI uri, Options options)will throw an unguardedNullPointerExceptionaturi.getScheme()ifuriis null, whereasoptionsis explicitly validated withObjects.requireNonNullin the overload at line 28. Consider adding a null check for consistency and clearer error messages.Proposed fix
public static void validateAll(URI uri, Options options) { + Objects.requireNonNull(uri, "uri"); validateAll(uri.getScheme(), uri.getAuthority(), uri.getUserInfo(), uri.getRawPath(), uri.getRawQuery(), uri.getRawFragment(), options); }🤖 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/UriIllegalCharacterDetector.java` around lines 17 - 19, The validateAll(URI uri, Options options) overload lacks a null check for uri and will NPE at uri.getScheme(); add an explicit null guard (e.g., Objects.requireNonNull(uri, "uri must not be null")) at the start of the method before calling uri.getScheme(), mirroring the options validation used in the other validateAll overload to provide a clear error message and consistent behavior.
58-84: Options class uses bare public mutable fields.This works but makes it easy to accidentally mutate shared instances. If an
Optionsobject is ever cached or reused across threads, this could introduce subtle bugs. A builder or factory methods for common presets (e.g.,Options.strict(),Options.relaxed()) would improve usability and safety.Not urgent — fine as-is for an internal utility.
🤖 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/UriIllegalCharacterDetector.java` around lines 58 - 84, The Options class exposes mutable public fields (skipAllValidation, strictRfc3986, allowBracesInPath, allowBracesInQueryAndFragment, allowBracesInUserInfo) which can cause accidental shared-state bugs; make Options immutable and provide a builder or static factory presets instead: convert fields to private final, add a nested Options.Builder with fluent setters and a build() method, and add static factory methods like Options.strict() and Options.relaxed() that return preconfigured immutable instances; update any callers that construct Options to use the builder or factory methods.core/src/test/java/com/predic8/membrane/core/interceptor/rewrite/RewriteInterceptorTest.java (1)
102-103: UseJsonNode.path()to avoid NPE on absent fields.
JsonNode.get()returnsnullwhen the field is absent; chaining.asText().contains(…)on it will throwNullPointerExceptioninstead of a meaningful assertion failure.JsonNode.path()returns aMissingNodethat safely returns an empty string from.asText().♻️ Proposed fix
- assertTrue(json.get("path").asText().contains("/buy/banana/%")); - assertTrue(json.get("component").asText().contains("rewrite")); + assertTrue(json.path("path").asText().contains("/buy/banana/%")); + assertTrue(json.path("component").asText().contains("rewrite"));🤖 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/rewrite/RewriteInterceptorTest.java` around lines 102 - 103, Replace usages of JsonNode.get(...) with JsonNode.path(...) in the assertions inside RewriteInterceptorTest (the two assertions that check json.get("path").asText().contains(...) and json.get("component").asText().contains(...)) so that missing fields produce a safe MissingNode and yield empty strings instead of throwing NPE; update the two expressions to use json.path("path").asText().contains(...) and json.path("component").asText().contains(...) respectively to ensure safe assertions.core/src/main/java/com/predic8/membrane/core/proxies/Target.java (1)
116-139: Consider cachingInterceptorAdapterif throughput profiling identifies this as a hot path.The
InterceptorAdapterconstructor is lightweight (only field assignments), but a new instance is created per request per destination URL incomputeDestinationExpressions(). While the inline comment correctly notes that caching showed negligible benefit, if high-throughput profiling later reveals this as a bottleneck, the adapter could be pre-built once in theinit()method and stored as a field, assuming the same router andxmlConfiginstances remain stable throughout the application lifecycle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/proxies/Target.java` around lines 116 - 139, computeDestinationExpressions currently instantiates a new InterceptorAdapter for every call which may be a hot path; to mitigate this, create and reuse a single InterceptorAdapter instance by initializing it once (e.g. in init() or as a lazy final field) instead of inside computeDestinationExpressions, ensuring it is constructed with the same router and xmlConfig used today; update computeDestinationExpressions to use the cached InterceptorAdapter and keep the existing behavior of evaluateTemplate and evaluateExpressions unchanged, and ensure thread-safety if fields may be accessed concurrently.core/src/test/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptorTest.java (1)
152-182:deactivateEvaluationOfURLTemplatesWhenIllegalCharactersAreAllowedanduriTemplateAndIllegalCharactersappear to test the same code path.Both tests:
- Call
allowIllegalURICharacters()- Use the same target URL
"https://${'hostname'}"(template expression)- Assert
ConfigurationExceptionThe only difference is the request URI (
"/foo/${555}"vs"/foo"). IfConfigurationExceptionoriginates fromtarget.init(router)(before dispatching even inspects the request URI), the request URI content is irrelevant and both tests exercise the same branch.Either add a comment clarifying what distinct scenario the
${555}-URI variant tests, or consolidate the two into one.🤖 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/HTTPClientInterceptorTest.java` around lines 152 - 182, These two tests (deactivateEvaluationOfURLTemplatesWhenIllegalCharactersAreAllowed and uriTemplateAndIllegalCharacters) exercise the same failure path because both call allowIllegalURICharacters() and use the template target "https://${'hostname'}" which fails at target.init(router) before the request URI matters; either consolidate them into a single test that asserts ConfigurationException for invokeDispatching(SPEL, ..., "https://${'hostname'}", Escaping.URL) and remove the duplicate, or change uriTemplateAndIllegalCharacters to a different distinct scenario (e.g., use a non-template target or assert different post-init behavior) so it no longer duplicates deactivateEvaluationOfURLTemplatesWhenIllegalCharactersAreAllowed; update or add a brief comment in the remaining test to explain why the request URI does or does not affect the failure and reference allowIllegalURICharacters(), invokeDispatching(...), and the two test method names to locate the code.core/src/main/java/com/predic8/membrane/core/resolver/ResolverMap.java (1)
53-55:countfield has package-private visibility — consider making itprivate.
count,schemas, andresolversare internal state ofResolverMap.countin particular lacks theprivatemodifier (lines 53), which exposes it to other classes in the same package.🤖 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/resolver/ResolverMap.java` around lines 53 - 55, The field count in class ResolverMap is package-private; make it private to encapsulate internal state along with schemas and resolvers; update the declaration of count to private int count and ensure any direct accesses from ResolverMap methods (or other classes) use existing ResolverMap methods (or add getters/setters if needed) so external code no longer relies on the package-visible field.
🤖 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/URI.java`:
- Around line 272-279: getWithoutPath() blindly concatenates getScheme() + "://"
+ getAuthority(), producing "null://null" for relative or authority-less URIs;
to fix, check getScheme() and getAuthority() at the start of getWithoutPath():
if either is null throw an IllegalStateException (or return a documented
alternative like the original URI string) with a clear message; update the
javadoc to state the method only applies to absolute hierarchical URIs. Ensure
references to getScheme(), getAuthority(), and getWithoutPath() remain in the
message so callers can locate the contract.
In `@core/src/main/java/com/predic8/membrane/core/util/uri/EscapingUtil.java`:
- Around line 30-32: The Javadoc for the URL escaping strategy is wrong:
URLEncoder.encode (used in EscapingUtil's URL strategy) produces '+' for spaces,
not '%20'; update the implementation in EscapingUtil's URL strategy to
post-process the result of URLEncoder.encode(...) by replacing '+' with "%20"
(and leave other percent-encodings intact) so spaces are percent-encoded for
URI/path contexts, and also update the Javadoc text for the URL strategy to
reflect that spaces become "%20" (or, if you prefer not to change behavior,
instead update the Javadoc to state that URLEncoder yields '+'); reference the
URL strategy, the use of URLEncoder.encode, and EscapingUtil when making the
change.
- Around line 69-73: Update the Javadoc examples in EscapingUtil to reference
the correct method name pathEncode instead of the stale pathSeg; specifically
replace occurrences like pathSeg("a/b & c") and pathSeg("ä") and pathSeg(123)
with pathEncode("a/b & c"), pathEncode("ä"), and pathEncode(123) in the Javadoc
block so the examples match the actual method name.
In `@core/src/main/java/com/predic8/membrane/core/util/URIFactory.java`:
- Around line 27-28: The public static singletons DEFAULT_URI_FACTORY and
ALLOW_ILLEGAL_CHARACTORY_FACTORY expose mutable URIFactory state; prevent global
mutation by making those instances immutable: create an unmodifiable subclass
(e.g., new URIFactory(true) { `@Override` setAllowIllegalCharacters(...){ throw
new UnsupportedOperationException(); } `@Override` setAutoEscapeBackslashes(...){
throw new UnsupportedOperationException(); } }) for
ALLOW_ILLEGAL_CHARACTERS_URI_FACTORY (and a similar immutable instance for
DEFAULT_URI_FACTORY), or alternatively stop exposing mutable instances and
instead expose factory methods that return new URIFactory copies; update
references to use the new immutable instances or callers that obtain copies and
remove/disable setter behavior on the shared instances (target methods:
setAllowIllegalCharacters, setAutoEscapeBackslashes, any other URIFactory
setters).
In `@core/src/main/java/com/predic8/membrane/core/util/URIValidationUtil.java`:
- Around line 19-24: validateDigits currently throws a NullPointerException when
port is null; update validateDigits in URIValidationUtil to guard against null
(either return/ignore null inputs or throw a clear IllegalArgumentException)
before calling port.length(), consistent with how UriIllegalCharacterDetector
handles nulls; ensure the chosen behavior is documented in the method javadoc
and use the method name validateDigits as the spot to add the null check and
error handling so callers get a clear, intentional outcome instead of an NPE.
In `@core/src/main/java/com/predic8/membrane/core/util/URLUtil.java`:
- Around line 46-50: The getNameComponent method may call getPath() on an opaque
URI and get null; update getNameComponent(URIFactory uriFactory, String uri) to
capture the result of uriFactory.create(uri).getPath() into a local variable,
check for null, and handle it (e.g., return an empty string or throw a
descriptive URISyntaxException) before computing lastIndexOf('/') and substring;
ensure references to getNameComponent and uriFactory.create(uri).getPath() are
used so reviewers can find the change.
In `@core/src/test/java/com/predic8/membrane/core/openapi/util/UriUtilTest.java`:
- Around line 86-95: Rename the misleading test methods to reflect they start
from a fully-qualified http URL: change the method rewriteWithoutHttp() to
rewriteFromHttpToHttp() and rewriteWithoutHttps() to rewriteFromHttpToHttps()
(keep the `@Test` annotation and method signatures the same except for the names)
and update any references or imports that call these method names so test
discovery and naming match the intent; verify the assertions inside
doRewrite(...) remain unchanged.
In `@core/src/test/java/com/predic8/membrane/core/util/FileUtilTest.java`:
- Around line 54-61: The Linux-only assertions in the FileUtilTest.resolveFile
test hard-code POSIX paths; update the expectations to be OS-aware by wrapping
the expected URI strings with OSUtil.wl so they match Windows drive-letter
absolute paths too. Add a static import for wl (e.g. import static
com.predic8.membrane.core.util.OSUtil.*) in the FileUtilTest and replace the
literal expected values in the resolveFile assertions (which reference
FileUtil.resolve) with wl("file:/a/b/c/d"), wl("file:/a/b/c/d/"), etc., so the
test passes on all platforms.
In
`@core/src/test/java/com/predic8/membrane/core/util/uri/URIVsJavaNetURITest.java`:
- Line 65: The test's assertion message is misleading: update the assertion in
URIVsJavaNetURITest (the assertEquals comparing j.getHost() and custom.getHost()
where variables j and custom are java.net.URI and custom URI) to reflect that
java.net.URI#getHost() returns IPv6 hosts with brackets; replace the message
"host (java host is bracket-free)" with a correct message such as "host (java
host includes brackets)" or remove the parenthetical entirely so the assertion
message is accurate.
In `@core/src/test/java/com/predic8/membrane/core/util/URITest.java`:
- Around line 649-653: In the URITest class update the inline comment that
currently reads "getRessource() can deal with that" to the correct spelling
"getResource() can deal with that" (edit the comment near the assertEquals in
URITest where base.resolve(relative) is asserted) so the method name is
correctly spelled.
- Around line 139-141: The three identical assertions in URITest all call
uf.create("http://localhost/foo").getWithoutPath(); locate the test method in
URITest and remove the duplicate assertions or replace two of them with
meaningful variations (e.g., use uf.create with a query string like
"http://localhost/foo?bar=1" and with a fragment like
"http://localhost/foo#frag") so that tests still assert getWithoutPath() returns
"http://localhost" for different input forms; update or delete the extra
assertEquals calls accordingly.
---
Outside diff comments:
In `@core/src/main/java/com/predic8/membrane/core/resolver/ResolverMap.java`:
- Around line 99-111: The recursive branch in combineInternal(URIFactory
factory,String... locations) loses the provided URIFactory by calling the
no-factory overload combine(combine(l), locations[last]) which uses
DEFAULT_URI_FACTORY; change the recursive call to propagate the original
factory, e.g. call the overload that accepts a URIFactory so the call becomes
combine(factory, combine(l), locations[locations.length-1]) (or equivalently
recurse via combineInternal with the factory), ensuring URIFactory is not
discarded; adjust references to combineInternal2 only where needed and keep
combineInternal, combine, combineInternal2 and URIFactory semantics intact.
In `@core/src/main/java/com/predic8/membrane/core/util/URI.java`:
- Around line 192-198: The getters URI.getHost() and URI.getPort() can NPE
because hostPort may remain null when processAuthority() returns early for URIs
without an authority; update getHost() and getPort() to check if hostPort is
null and return safe defaults (null for host, -1 for port) instead of
dereferencing hostPort, so callers like new URI(...).getHost() won't throw;
adjust any related javadoc/comments on getPort()/getHost() to document these
return values and ensure processAuthority()/hostPort initialization need not
change.
---
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java`:
- Around line 91-94: In TemplateExchangeExpression (class
TemplateExchangeExpression) remove the fallback that appends the literal "null"
when a resolved expression value is null and instead throw an
ExchangeExpressionException indicating the specific expression that evaluated to
null; update the null branch inside the loop (the block handling value == null)
to throw new ExchangeExpressionException with a clear message referencing the
expression (e.g., the template token or expression string) so callers fail fast
and can handle the error.
In `@core/src/main/java/com/predic8/membrane/core/util/URLUtil.java`:
- Around line 21-27: The getAuthority method can index past the string end;
before using uri.charAt(i) add defensive checks: first verify colon exists
(uri.indexOf(":") >= 0) and that i < uri.length() after computing i, and change
the while to require i < uri.length() && uri.charAt(i) == '/'; also handle cases
where i reaches uri.length() (e.g. return "" or null) and keep the existing j =
uri.indexOf("/", i) logic but ensure j is computed only when i < uri.length();
update getAuthority to return a safe empty string for malformed inputs instead
of allowing StringIndexOutOfBoundsException.
---
Nitpick comments:
In `@core/src/main/java/com/predic8/membrane/core/proxies/Target.java`:
- Around line 116-139: computeDestinationExpressions currently instantiates a
new InterceptorAdapter for every call which may be a hot path; to mitigate this,
create and reuse a single InterceptorAdapter instance by initializing it once
(e.g. in init() or as a lazy final field) instead of inside
computeDestinationExpressions, ensuring it is constructed with the same router
and xmlConfig used today; update computeDestinationExpressions to use the cached
InterceptorAdapter and keep the existing behavior of evaluateTemplate and
evaluateExpressions unchanged, and ensure thread-safety if fields may be
accessed concurrently.
In `@core/src/main/java/com/predic8/membrane/core/resolver/ResolverMap.java`:
- Around line 53-55: The field count in class ResolverMap is package-private;
make it private to encapsulate internal state along with schemas and resolvers;
update the declaration of count to private int count and ensure any direct
accesses from ResolverMap methods (or other classes) use existing ResolverMap
methods (or add getters/setters if needed) so external code no longer relies on
the package-visible field.
In `@core/src/main/java/com/predic8/membrane/core/util/TemplateUtil.java`:
- Line 17: TemplateUtil is a pure-static utility class and needs a private
no-arg constructor to prevent instantiation; add a private TemplateUtil() { }
(optionally throw new AssertionError("No instances")); to the TemplateUtil class
to stop the compiler from generating a public default constructor and ensure
callers cannot do new TemplateUtil().
- Around line 25-33: The containsTemplateMarker method currently uses a manual
char-by-char loop which is unnecessary and error-prone; replace its body to use
String.contains("${") (or s.indexOf("${") >= 0) to detect the marker, retaining
the null check and method name containsTemplateMarker so the change is localized
and clearer.
In
`@core/src/main/java/com/predic8/membrane/core/util/UriIllegalCharacterDetector.java`:
- Around line 17-19: The validateAll(URI uri, Options options) overload lacks a
null check for uri and will NPE at uri.getScheme(); add an explicit null guard
(e.g., Objects.requireNonNull(uri, "uri must not be null")) at the start of the
method before calling uri.getScheme(), mirroring the options validation used in
the other validateAll overload to provide a clear error message and consistent
behavior.
- Around line 58-84: The Options class exposes mutable public fields
(skipAllValidation, strictRfc3986, allowBracesInPath,
allowBracesInQueryAndFragment, allowBracesInUserInfo) which can cause accidental
shared-state bugs; make Options immutable and provide a builder or static
factory presets instead: convert fields to private final, add a nested
Options.Builder with fluent setters and a build() method, and add static factory
methods like Options.strict() and Options.relaxed() that return preconfigured
immutable instances; update any callers that construct Options to use the
builder or factory methods.
In `@core/src/main/java/com/predic8/membrane/core/util/URIValidationUtil.java`:
- Around line 60-75: The check for '.' in validateHost is redundant because
isUnreserved(c) already returns true for '.', so remove the explicit c == '.'
branch in the character whitelist inside the validateHost method; update the
condition that currently reads if (c == '.' || c == '%' || c == ':' || c == '['
|| c == ']') to drop the '.' entry and keep the others, referencing the
validateHost method and isUnreserved helper to locate and verify the change.
- Line 17: The utility class URIValidationUtil is mutable and instantiable but
only contains static methods; make it a proper utility by declaring the class
final and adding a private no-arg constructor (e.g., private URIValidationUtil()
{ } ) to prevent instantiation, leaving all existing static methods untouched;
mirror the pattern used in UriIllegalCharacterDetector to ensure consistency
across utility classes.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptorTest.java`:
- Around line 152-182: These two tests
(deactivateEvaluationOfURLTemplatesWhenIllegalCharactersAreAllowed and
uriTemplateAndIllegalCharacters) exercise the same failure path because both
call allowIllegalURICharacters() and use the template target
"https://${'hostname'}" which fails at target.init(router) before the request
URI matters; either consolidate them into a single test that asserts
ConfigurationException for invokeDispatching(SPEL, ..., "https://${'hostname'}",
Escaping.URL) and remove the duplicate, or change
uriTemplateAndIllegalCharacters to a different distinct scenario (e.g., use a
non-template target or assert different post-init behavior) so it no longer
duplicates deactivateEvaluationOfURLTemplatesWhenIllegalCharactersAreAllowed;
update or add a brief comment in the remaining test to explain why the request
URI does or does not affect the failure and reference
allowIllegalURICharacters(), invokeDispatching(...), and the two test method
names to locate the code.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/rewrite/RewriteInterceptorTest.java`:
- Around line 102-103: Replace usages of JsonNode.get(...) with
JsonNode.path(...) in the assertions inside RewriteInterceptorTest (the two
assertions that check json.get("path").asText().contains(...) and
json.get("component").asText().contains(...)) so that missing fields produce a
safe MissingNode and yield empty strings instead of throwing NPE; update the two
expressions to use json.path("path").asText().contains(...) and
json.path("component").asText().contains(...) respectively to ensure safe
assertions.
In
`@core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java`:
- Around line 124-127: The test couples to implementation details: in
ExplodedArrayQueryParameterTest change the fragile assertions around
validator.validate(...) — replace the exact count assertion (assertEquals(1,
err.size())) with a less brittle check (e.g., assertTrue(err.size() >= 1) or
assertTrue(err.stream().anyMatch(...)) ) unless the contract of
UriIllegalCharacterDetector explicitly guarantees one error per parameter value
(in which case add a comment stating that invariant near the test); also avoid
hard‑coding the English message by comparing against a shared constant from
UriIllegalCharacterDetector or by using a language-agnostic check such as
matching a small stable substring or checking for the detector’s error code;
finally, replace the lone usage of var with the explicit type ValidationErrors
for consistency with other tests.
In
`@core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeFalseArrayQueryParameterTest.java`:
- Around line 113-117: The test method valuesUTF8 currently lives inside the
Valid nested class but asserts an error, so move it into the Invalid nested
class and rename it to rawUTF8InQueryStringIsInvalid (or similar) to make intent
clear; ensure its body remains the same (calling
validator.validate(get().path("/array?const=foo,äöü,baz")) and asserting a
single error containing "Invalid query string"), and keep the existing
valuesAreDecoded test in Valid unchanged to cover the percent-encoded UTF‑8
case; also check for any duplicate semantics with wrongCharacter and consolidate
if necessary.
…comprehensive RFC 3986 compliance tests - Added `URIFactory` to support flexible URI creation with options for illegal character handling and backslash escaping. - Implemented extensive
… parsing - Added edge case assertions for parsing IPv6 with empty ports. - Introduced additional checks for IPv4 host and port parsing scenarios. - Renamed test methods to reflect broader case coverage.
rrayst
reviewed
Feb 19, 2026
…Target` and `CallInterceptor` - Replaced `evaluateExpressions` with `urlIsTemplate` for better semantics and clarity. - Improved `Target` and `CallInterceptor` to skip URL evaluation when no template markers are present. - Introduced stricter validation for configurations involving template markers and illegal characters. - Updated relevant tests to validate changes and added new test cases for URL template evaluation.
rrayst
approved these changes
Feb 20, 2026
This was referenced Mar 11, 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.
…http client interceptor
Summary by CodeRabbit
New Features
Bug Fixes
Improvements