URL supports placeholders and can be dynamically replaced with msg ex… - #535
URL supports placeholders and can be dynamically replaced with msg ex…#535zhengchangqing wants to merge 1 commit into
Conversation
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review: Approved ✅
PR: #535 — URL supports placeholders and can be dynamically replaced with msg extensions
Type: Enhancement (1 file, +33/-1)
Assessment
Adds placeholder support to HTTP sink task URL, enabling dynamic replacement from message extensions. Useful for routing based on message metadata.
Verdict
✅ Clean enhancement with practical use case.
🤖 Automated review by oss-sentinel-ai
|
Issue Evaluation Category: Feature request to support URL placeholders that can be dynamically replaced with message extension values. This would enable more flexible routing in connectors. Feasibility: feasible Automated evaluation by github-manager-bot |
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
URL placeholder support for HttpSinkTask — allows dynamic URL construction using message extension values via {key} placeholders.
Findings
- [Critical]
formatUrl()line 68:PATTERN.matcher(url).matches()requires the entire string to match the regex\{(\w+)\}, not just contain a placeholder. A URL likehttp://api.example.com/{id}/datawill never match becausematches()tries to match the full string against the pattern. This should bePATTERN.matcher(url).find()instead. As written, the placeholder replacement will never execute for any real URL.
Suggestions
// Change from:
if (PATTERN.matcher(url).matches()) {
// To:
if (PATTERN.matcher(url).find()) {Automated review by github-manager-bot
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Review of PR #535: URL supports placeholders and can be dynamically replaced with msg ex…
Findings: 3 issue(s) identified (1 critical).
CLA: unknown
Please address the inline comments above.
Automated review by github-manager-bot
| @@ -140,6 +146,32 @@ public void put(List<ConnectRecord> records) throws ConnectException { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
formatUrl uses PATTERN.matcher(url).matches(), which requires the entire URL to match the placeholder pattern \\{(\\w+)\\}. A real URL such as http://host/api/{id} will not match, so the method almost always returns the original URL unchanged and placeholder replacement is effectively broken. Use find() to detect placeholders within the URL instead.
| * @return the formatted url | ||
| */ | ||
| private String formatUrl(String url, KeyValue extensions) { | ||
| if (!PATTERN.matcher(url).matches()) { |
There was a problem hiding this comment.
Extension values are inserted into the URL via raw string replacement without URL encoding. Values containing reserved characters (/, ?, &, =, #, spaces, etc.) can produce malformed URLs or alter request semantics. Encode values before substitution, e.g. with URLEncoder.encode(value, StandardCharsets.UTF_8).
| @@ -140,6 +146,32 @@ public void put(List<ConnectRecord> records) throws ConnectException { | |||
| } | |||
There was a problem hiding this comment.
The new formatUrl placeholder replacement logic has no test coverage in the diff. Add unit tests covering: successful replacement, missing extension key handling, null/empty extensions, special characters in values, and URLs with multiple placeholders.
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Re-review of PR #535 after new commits. The critical regex bug identified in the previous review remains unfixed.
The formatUrl() method uses PATTERN.matcher(url).matches() which will never match a real URL containing placeholders (e.g., http://api.example.com/{id}/data). This makes the entire placeholder feature non-functional.
Findings
- [Critical]
HttpSinkTask.java:155—matches()should befind()— placeholder replacement is currently dead code
Suggestions
// Change from:
if (!PATTERN.matcher(url).matches()) {
// To:
if (!PATTERN.matcher(url).find()) {Automated review by github-manager-bot
| * @param url the source url str | ||
| * @param extensions ConnectRecord Extension Values | ||
| * @return the formatted url | ||
| */ |
There was a problem hiding this comment.
[Critical] PATTERN.matcher(url).matches() requires the entire string to match the regex \{(\w+)\}. A real URL like http://api.example.com/{id}/data will never match because matches() anchors at both ends. This means the placeholder replacement will silently never execute for any practical URL.
Fix: Change to PATTERN.matcher(url).find() which checks if the pattern exists anywhere in the string.
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
The placeholder gate uses matches() so only a whole-URL single placeholder is ever replaced (embedded {key} templates are silently skipped), substitutions are unencoded untrusted data, unresolved placeholders are passed through silently, and the change ships with no tests.
Findings
- [CRITICAL]
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:156— PATTERN.matcher(url).matches() only returns true when the ENTIRE url is a single{placeholder}. For any realistic template likehttp://host/api/{tenant}/orders, matches() is false and formatUrl() returns the url unchanged — the feature silently no-ops for embedded placeholders, even though the replacement loop below is clearly written for them. Use find() (or drop the gate and just attempt replacement). Also note\w+excludes keys containing.or-, which are common in connect extension keys, so{connect.topic}would never be detected even after switching to find(). - [WARNING]
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:166— Extension values are untrusted record data inserted into the URL with no encoding. A value containing a space,&,#,/, or?yields a malformed URL or silently alters the path/query semantics. URL-encode values (e.g. URLEncoder.encode(value, StandardCharsets.UTF_8)). Additionally, sequential String.replace calls let a value that itself contains{otherKey}get re-replaced by a later iteration; a single-pass Matcher.appendReplacement (with Matcher.quoteReplacement) avoids both problems. - [WARNING]
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:169— If a placeholder has no matching extension key, or the value is null/empty (skipped at line 164), the raw{key}is left in the URL and the request is sent to an invalid endpoint — surfacing only as a confusing downstream 404/DNS error with nothing in the logs. Log a warning listing unresolved placeholders (or throw a ConnectException) so bad records are diagnosable and can be routed to dead-letter handling. - [WARNING]
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:155— The PR checklist claims >80% unit-test coverage, but no tests were added in this PR. formatUrl has several distinct branches worth covering: whole-URL placeholder, embedded placeholders, missing extension key, null/empty value, and values with special characters. Consider extracting the logic to a package-private static helper (or testing through put() with the HTTP call mocked) so it can be unit-tested. - [INFO]
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:61— PATTERN is declared protected but is only used inside this class, and the name is generic enough to collide with subclass fields. Preferprivate static final Pattern URL_PLACEHOLDER_PATTERN.
Automated review by github-manager-bot
| * @param extensions ConnectRecord Extension Values | ||
| * @return the formatted url | ||
| */ | ||
| private String formatUrl(String url, KeyValue extensions) { |
There was a problem hiding this comment.
PATTERN.matcher(url).matches() only returns true when the ENTIRE url is a single {placeholder}. For any realistic template like http://host/api/{tenant}/orders, matches() is false and formatUrl() returns the url unchanged — the feature silently no-ops for embedded placeholders, even though the replacement loop below is clearly written for them. Use find() (or drop the gate and just attempt replacement). Also note \w+ excludes keys containing . or -, which are common in connect extension keys, so {connect.topic} would never be detected even after switching to find().
| for (String key : keys) { | ||
| String value = extensions.getString(key); | ||
| if (StringUtils.isNotEmpty(value)) { | ||
| // simple replaced the placeholder |
There was a problem hiding this comment.
Extension values are untrusted record data inserted into the URL with no encoding. A value containing a space, &, #, /, or ? yields a malformed URL or silently alters the path/query semantics. URL-encode values (e.g. URLEncoder.encode(value, StandardCharsets.UTF_8)). Additionally, sequential String.replace calls let a value that itself contains {otherKey} get re-replaced by a later iteration; a single-pass Matcher.appendReplacement (with Matcher.quoteReplacement) avoids both problems.
| // simple replaced the placeholder | ||
| template = template.replace("{" + key + "}", value); | ||
| } | ||
| } |
There was a problem hiding this comment.
If a placeholder has no matching extension key, or the value is null/empty (skipped at line 164), the raw {key} is left in the URL and the request is sent to an invalid endpoint — surfacing only as a confusing downstream 404/DNS error with nothing in the logs. Log a warning listing unresolved placeholders (or throw a ConnectException) so bad records are diagnosable and can be routed to dead-letter handling.
| * @param url the source url str | ||
| * @param extensions ConnectRecord Extension Values | ||
| * @return the formatted url | ||
| */ |
There was a problem hiding this comment.
The PR checklist claims >80% unit-test coverage, but no tests were added in this PR. formatUrl has several distinct branches worth covering: whole-URL placeholder, embedded placeholders, missing extension key, null/empty value, and values with special characters. Consider extracting the logic to a package-private static helper (or testing through put() with the HTTP call mocked) so it can be unit-tested.
| public class HttpSinkTask extends SinkTask { | ||
| private static final Logger log = LoggerFactory.getLogger(HttpSinkTask.class); | ||
| // the Regex Pattern like '{number}' | ||
| protected static final Pattern PATTERN = Pattern.compile("\\{(\\w+)\\}"); |
There was a problem hiding this comment.
PATTERN is declared protected but is only used inside this class, and the name is generic enough to collide with subclass fields. Prefer private static final Pattern URL_PLACEHOLDER_PATTERN.
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
5 finding(s) to address.
Findings
- [CRITICAL]
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:157—matches()requires the entire input string to match the regex. A realistic URL likehttp://example.com/api/{id}/resourcewill never match\{(\w+)\}because the surrounding URL characters are not part of the pattern. This meansformatUrlwill always take the early-return path and never perform any substitution for real-world URLs. Usefind()instead, which scans for the pattern anywhere in the string. - [WARNING]
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:166— Extension values are substituted into the URL with a rawString.replace— no URL-encoding is applied. If an extension value contains characters like?,&,#,/, or spaces, the resulting URL will be structurally broken or could be exploited for path/query injection. Values should be encoded withURLEncoder.encode(value, StandardCharsets.UTF_8)(or at minimum, path-segment encoding) before substitution. - [WARNING]
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:166— If an extension key is missing or its value is empty, the corresponding{key}placeholder remains silently in the final URL. The downstream HTTP request will then target a malformed URL (e.g.http://host/{id}/path), likely producing a confusing 400/404 error. Consider logging a warning when unresolved placeholders remain, or throwing an exception to fail fast. - [INFO]
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:57— The comment says the pattern matches{number}, but the regex\{(\w+)\}actually matches any word-character sequence (letters, digits, underscore), not just digits. The comment is misleading — update it to say e.g.{placeholder}or{word}. - [INFO]
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:109— No unit tests are included in this diff. The PR checklist claims tests were written, but the diff contains no test file changes. Tests should cover: URL with single/multiple placeholders, missing extensions, empty extension values, values with special characters, and URLs with no placeholders.
Automated review by github-manager-bot
| * @return the formatted url | ||
| */ | ||
| private String formatUrl(String url, KeyValue extensions) { | ||
| if (!PATTERN.matcher(url).matches()) { |
There was a problem hiding this comment.
matches() requires the entire input string to match the regex. A realistic URL like http://example.com/api/{id}/resource will never match \{(\w+)\} because the surrounding URL characters are not part of the pattern. This means formatUrl will always take the early-return path and never perform any substitution for real-world URLs. Use find() instead, which scans for the pattern anywhere in the string.
| for (String key : keys) { | ||
| String value = extensions.getString(key); | ||
| if (StringUtils.isNotEmpty(value)) { | ||
| // simple replaced the placeholder |
There was a problem hiding this comment.
Extension values are substituted into the URL with a raw String.replace — no URL-encoding is applied. If an extension value contains characters like ?, &, #, /, or spaces, the resulting URL will be structurally broken or could be exploited for path/query injection. Values should be encoded with URLEncoder.encode(value, StandardCharsets.UTF_8) (or at minimum, path-segment encoding) before substitution.
| for (String key : keys) { | ||
| String value = extensions.getString(key); | ||
| if (StringUtils.isNotEmpty(value)) { | ||
| // simple replaced the placeholder |
There was a problem hiding this comment.
If an extension key is missing or its value is empty, the corresponding {key} placeholder remains silently in the final URL. The downstream HTTP request will then target a malformed URL (e.g. http://host/{id}/path), likely producing a confusing 400/404 error. Consider logging a warning when unresolved placeholders remain, or throwing an exception to fail fast.
| import java.util.concurrent.ScheduledExecutorService; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.regex.Pattern; | ||
|
|
There was a problem hiding this comment.
The comment says the pattern matches {number}, but the regex \{(\w+)\} actually matches any word-character sequence (letters, digits, underscore), not just digits. The comment is misleading — update it to say e.g. {placeholder} or {word}.
| @@ -105,8 +109,10 @@ public void put(List<ConnectRecord> records) throws ConnectException { | |||
| if (auth != null) { | |||
There was a problem hiding this comment.
No unit tests are included in this diff. The PR checklist claims tests were written, but the diff contains no test file changes. Tests should cover: URL with single/multiple placeholders, missing extensions, empty extension values, values with special characters, and URLs with no placeholders.
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Adds URL placeholder support to HttpSinkTask — placeholders like {key} in the URL are dynamically replaced with values from the record's extensions/properties.
Findings
-
[Critical]
PATTERN.matcher(url).matches()checks if the entire URL matches the pattern\{(\w+)\}. This will almost always returnfalsefor real URLs likehttp://api.example.com/{userId}/orders. The check should use.find()instead of.matches(). As written, the placeholder replacement will never trigger for typical URLs. -
[Warning] The
replaceAlllambda callsrecord.getExtensions().get(key)without null-checking. If a placeholder key doesn't exist in extensions, the replacement will be the literal stringnull. Consider keeping the original placeholder when the key is not found: -
[Info] The
PATTERNis compiled once as a static final field — good for performance.
Suggestions
- Fix
.matches()→.find()(critical — feature is non-functional otherwise) - Handle missing extension keys gracefully
- Add a unit test with a URL like
http://example.com/{topic}/{tag}to verify the replacement works
2 participants
What is the purpose of the change
[ISSUES #534 ]
Brief changelog
Modify
HttpSinkTasksupports placeholders and can be dynamically replaced with msg extendsionsVerifying this change
Follow this checklist to help us incorporate your contribution quickly and easily. Notice,
it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.[ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyleto make sure basic checks pass. Runmvn clean install -DskipITsto make sure unit-test pass. Runmvn clean test-compile failsafe:integration-testto make sure integration-test pass.