Copilot commented on code in PR #4001:
URL: https://github.com/apache/hertzbeat/pull/4001#discussion_r2724258139
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/history/tsdb/greptime/GreptimeDbDataStorage.java:
##########
@@ -220,8 +222,8 @@ public Map<String, List<Value>> getHistoryMetricData(String
instance, String app
return getHistoryData(start, end, step, instance, app, metrics,
metric);
}
- private String getTableName(String metrics) {
- return metrics;
+ private String getTableName(String app, String metrics) {
+ return app + "_" + metrics;
Review Comment:
`getTableName` concatenates `app` and `metrics` directly and the result is
used both as a Greptime table name and (via `getHistoryIntervalMetricData`) as
a PromQL metric identifier. Because `app`/`metrics` values are not
validated/normalized when custom templates are applied (see
`AppServiceImpl#verifyDefineAppContent`, which doesn’t constrain `app` or
`metrics.name`), this can produce invalid PromQL selectors (e.g., containing
`-`, `.`, spaces) and break interval queries, or fail table creation. Consider
normalizing `app`/`metrics` to a safe identifier (e.g., replace non
`[A-Za-z0-9_]` with `_`, ensure a valid leading char), and/or build PromQL
selectors using `{__name__="..."}` rather than `name{...}` so metric-name
syntax rules don’t apply.
```suggestion
/**
* Normalize a raw identifier (e.g., app or metrics name) to a safe form
* suitable for use as a GreptimeDB table name and PromQL metric name.
* <p>
* Rules:
* <ul>
* <li>Null or blank values become an empty string.</li>
* <li>Characters outside {@code [A-Za-z0-9_]} are replaced with
'{@code _}'.</li>
* <li>If the first character is not a letter and not '{@code _}',
an '{@code _}' is prefixed.</li>
* </ul>
*/
private String normalizeIdentifier(String raw) {
if (!StringUtils.hasText(raw)) {
return "";
}
// Replace any character that is not A-Z, a-z, 0-9, or '_' with '_'
String normalized = raw.replaceAll("[^A-Za-z0-9_]", "_");
if (normalized.isEmpty()) {
return "";
}
char first = normalized.charAt(0);
if (!(Character.isLetter(first) || first == '_')) {
normalized = "_" + normalized;
}
return normalized;
}
private String getTableName(String app, String metrics) {
String safeApp = normalizeIdentifier(app);
String safeMetrics = normalizeIdentifier(metrics);
return safeApp + "_" + safeMetrics;
```
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/history/tsdb/greptime/GreptimeDbDataStorage.java:
##########
@@ -764,7 +766,7 @@ public boolean batchDeleteLogs(List<Long> timeUnixNanos) {
try {
StringBuilder sql = new StringBuilder("DELETE FROM
").append(LOG_TABLE_NAME).append(" WHERE time_unix_nano IN (");
sql.append(timeUnixNanos.stream()
- .filter(time -> time != null)
+ .filter(Objects::nonNull)
.map(String::valueOf)
.collect(Collectors.joining(", ")));
sql.append(")");
Review Comment:
After filtering out null timestamps, it’s still possible for the `IN (...)`
list to end up empty (e.g., input list contains only nulls), producing `... IN
()` which is invalid SQL. Consider collecting non-null values first and
returning `false` (or no-op) if the filtered list is empty; also log the
filtered count rather than `timeUnixNanos.size()` to avoid reporting deletions
for values that were skipped.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
