Copilot commented on code in PR #18026:
URL:
https://github.com/apache/dolphinscheduler/pull/18026#discussion_r2937849945
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/parser/TimePlaceholderUtils.java:
##########
@@ -649,19 +649,43 @@ public static Map.Entry<Date, String> calcMinutes(String
expression, Date date)
if (expression.contains("+")) {
int index = expression.lastIndexOf('+');
- if (Character.isDigit(expression.charAt(index + 1))) {
- String addMinuteExpr = expression.substring(index + 1);
- Date targetDate = addMinutes(date, calcMinutes(addMinuteExpr));
+ if (index + 1 < expression.length() &&
Character.isDigit(expression.charAt(index + 1))) {
+ String addDayExpr = expression.substring(index + 1);
+ int days;
+ try {
+ days = Integer.parseInt(addDayExpr);
+ } catch (NumberFormatException e) {
+ return null;
+ }
+ LocalDate localDate = date.toInstant()
+ .atZone(ZoneId.systemDefault())
+ .toLocalDate()
+ .plusDays(days);
+
+ Date targetDate = Date.from(
+
localDate.atStartOfDay(ZoneId.systemDefault()).toInstant());
Review Comment:
On parse failure this method returns `null`, but `calcTimeExpression`
immediately dereferences `resultEntry.getKey()`/`getValue()`, causing an NPE
and treating valid expressions as "unsupported" under
`ignoreInvalidExpression`. Prefer returning a non-null entry (e.g., `(date,
expression)`) or throwing a clear exception so invalid placeholders fail
deterministically without an intermediate NPE.
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/parser/TimePlaceholderUtils.java:
##########
@@ -649,19 +649,43 @@ public static Map.Entry<Date, String> calcMinutes(String
expression, Date date)
if (expression.contains("+")) {
int index = expression.lastIndexOf('+');
- if (Character.isDigit(expression.charAt(index + 1))) {
- String addMinuteExpr = expression.substring(index + 1);
- Date targetDate = addMinutes(date, calcMinutes(addMinuteExpr));
+ if (index + 1 < expression.length() &&
Character.isDigit(expression.charAt(index + 1))) {
+ String addDayExpr = expression.substring(index + 1);
+ int days;
+ try {
+ days = Integer.parseInt(addDayExpr);
+ } catch (NumberFormatException e) {
+ return null;
+ }
Review Comment:
`calcMinutes(String expression, Date date)` now assumes the offset after
`+`/`-` is a plain integer and uses `Integer.parseInt`. This breaks existing
supported placeholders like `$[yyyyMMdd+7*2]` and `$[HHmmss+2/24]` (see
`ParameterUtilsTest`), which rely on arithmetic/fraction parsing via the
existing `calcMinutes(String minuteExpression)` helper.
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/parser/TimePlaceholderUtils.java:
##########
@@ -18,8 +18,6 @@
package org.apache.dolphinscheduler.plugin.task.api.parser;
import static org.apache.commons.lang3.time.DateUtils.addWeeks;
Review Comment:
The static import for `addDays` was removed, but this class still calls
`addDays(...)` in multiple methods (e.g., `calcMonthBegin`, `calcMonthEnd`,
`calcCustomDay`, `calcWeekStart/End`). As-is, this file will not compile;
either restore the static import or qualify the calls (e.g.,
`DateUtils.addDays`).
--
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]
