RockteMQ-AI commented on code in PR #535:
URL: https://github.com/apache/rocketmq-connect/pull/535#discussion_r4048278635
##########
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:
**[Critical — recurring issue]** `PATTERN.matcher(url).matches()` requires
the **entire** URL string to match `\{(\w+)\}`. This means only a URL that is
exactly `{word}` (nothing else) would pass this check. A realistic URL like
`http://example.com/{key}/path` would **never** match, so the placeholder
replacement would be silently skipped for all real-world use cases.
The fix is to use `.find()` instead of `.matches()`:
```java
if (!PATTERN.matcher(url).find()) {
return url;
}
```
This issue has been flagged in previous reviews but remains unresolved.
Please address it to ensure the feature works as intended.
##########
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
+ template = template.replace("{" + key + "}", value);
Review Comment:
**[Warning]** The replacement values from `extensions.getString(key)` are
not URL-encoded before substitution. If a value contains special URL characters
(e.g., spaces, `&`, `=`, `/`), the resulting URL could be malformed or lead to
unexpected behavior.
Consider using `URLEncoder.encode(value, StandardCharsets.UTF_8)` before
replacement, or document that values must be pre-encoded.
--
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]
