RockteMQ-AI commented on code in PR #535:
URL: https://github.com/apache/rocketmq-connect/pull/535#discussion_r3909423225
##########
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:
##########
@@ -140,6 +146,32 @@ public void put(List<ConnectRecord> records) throws
ConnectException {
}
}
+ /**
+ * 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
+ */
+ private String formatUrl(String url, KeyValue extensions) {
+ if (!PATTERN.matcher(url).matches()) {
+ 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
Review 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.
##########
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:
##########
@@ -140,6 +146,32 @@ public void put(List<ConnectRecord> records) throws
ConnectException {
}
}
+ /**
+ * 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
+ */
+ private String formatUrl(String url, KeyValue extensions) {
+ if (!PATTERN.matcher(url).matches()) {
Review 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.
##########
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:
##########
@@ -140,6 +146,32 @@ public void put(List<ConnectRecord> records) throws
ConnectException {
}
}
+ /**
+ * 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
+ */
+ private String formatUrl(String url, KeyValue extensions) {
+ if (!PATTERN.matcher(url).matches()) {
+ 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
Review 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.
##########
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:
##########
@@ -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;
Review 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}`.
##########
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/HttpSinkTask.java:
##########
@@ -105,8 +109,10 @@ public void put(List<ConnectRecord> records) throws
ConnectException {
if (auth != null) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
