RockteMQ-AI commented on code in PR #535:
URL: https://github.com/apache/rocketmq-connect/pull/535#discussion_r3839478058
##########
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:
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)`.
##########
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 {
}
}
Review 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.
##########
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 {
}
Review 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.
--
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]
