Skip to content

URL supports placeholders and can be dynamically replaced with msg ex… #535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zhengchangqing wants to merge 1 commit into
base: master
Choose a base branch
from
Open

URL supports placeholders and can be dynamically replaced with msg ex… #535

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion ...rs/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,17 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.regex.Pattern;

@RockteMQ-AI RockteMQ-AI Sep 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}.

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+)\\}");

@RockteMQ-AI RockteMQ-AI Sep 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

protected static final int DEFAULT_CONSUMER_TIMEOUT_SECONDS = 30;
protected static final String DEFAULT_REQUEST_TIMEOUT_MILL_SECONDS = "3000";
protected static final int DEFAULT_OAUTH_DELAY_SECONDS = 1;
Expand Down Expand Up @@ -105,8 +109,10 @@ public void put(List<ConnectRecord> records) throws ConnectException {
if (auth != null) {

@RockteMQ-AI RockteMQ-AI Sep 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

headerMap.putAll(auth.auth());
}
// replace the placeholder of url with extensions
String urlInsteadOfPlaceholder = formatUrl(url, connectRecord.getExtensions());
// render query to url
String urlWithQueryParameters = renderQueryParametersToUrl(url, queryParameters, fixedQueryParameters);
String urlWithQueryParameters = renderQueryParametersToUrl(urlInsteadOfPlaceholder, queryParameters, fixedQueryParameters);
HttpRequest httpRequest = new HttpRequest();
httpRequest.setUrl(urlWithQueryParameters);
httpRequest.setMethod(method);
Expand Down Expand Up @@ -140,6 +146,32 @@ public void put(List<ConnectRecord> records) throws ConnectException {
}

@RockteMQ-AI RockteMQ-AI Aug 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 RockteMQ-AI Aug 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/**
* Get a formatted url that will replace a placeholder with Extension Values
*
* @param url the source url str
* @param extensions ConnectRecord Extension Values
* @return the formatted url
*/

@RockteMQ-AI RockteMQ-AI Aug 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 RockteMQ-AI Sep 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private String formatUrl(String url, KeyValue extensions) {

@RockteMQ-AI RockteMQ-AI Sep 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

if (!PATTERN.matcher(url).matches()) {

@RockteMQ-AI RockteMQ-AI Aug 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@RockteMQ-AI RockteMQ-AI Sep 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return url;
}
if (extensions != null && extensions.keySet() != null) {
Set<String> keys = extensions.keySet();
String template = url;
for (String key : keys) {
String value = extensions.getString(key);
if (StringUtils.isNotEmpty(value)) {
// simple replaced the placeholder

@RockteMQ-AI RockteMQ-AI Sep 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RockteMQ-AI RockteMQ-AI Sep 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RockteMQ-AI RockteMQ-AI Sep 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

template = template.replace("{" + key + "}", value);
}
}

@RockteMQ-AI RockteMQ-AI Sep 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return template;
}
return url;
}

private Map<String, String> renderHeaderMap(String headerParameters, String fixedHeaderParameters, String token) {
Map<String, String> headerMap = new HashMap<>();
if (headerParameters != null) {
Expand Down