Copilot commented on code in PR #12518:
URL: https://github.com/apache/cloudstack/pull/12518#discussion_r2731007502
##########
utils/src/main/java/org/apache/cloudstack/utils/usage/UsageUtils.java:
##########
@@ -19,6 +19,50 @@
package org.apache.cloudstack.utils.usage;
+import com.cloud.utils.DateUtil;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.Calendar;
+import java.util.Date;
+import java.util.TimeZone;
+
public class UsageUtils {
+ protected static Logger logger = LogManager.getLogger(UsageUtils.class);
+
public static final int USAGE_AGGREGATION_RANGE_MIN = 1;
+
+ public static Date getNextJobExecutionTime(TimeZone usageTimeZone, String
jobExecTimeConfig) {
+ return getJobExecutionTime(usageTimeZone, jobExecTimeConfig, true);
+ }
+
+ public static Date getPreviousJobExecutionTime(TimeZone usageTimeZone,
String jobExecTimeConfig) {
+ return getJobExecutionTime(usageTimeZone, jobExecTimeConfig, false);
+ }
+
+ protected static Date getJobExecutionTime(TimeZone usageTimeZone, String
jobExecTimeConfig, boolean next) {
+ String[] execTimeSegments = jobExecTimeConfig.split(":");
+ if (execTimeSegments.length != 2) {
+ logger.warn("Unable to parse configuration
'usage.stats.job.exec.time'.");
+ return null;
+ }
+ int hourOfDay = Integer.parseInt(execTimeSegments[0]);
+ int minutes = Integer.parseInt(execTimeSegments[1]);
+
+ Date currentDate = DateUtil.currentGMTTime();
+ Calendar jobExecTime = Calendar.getInstance(usageTimeZone);
+ jobExecTime.setTime(currentDate);
+ jobExecTime.set(Calendar.HOUR_OF_DAY, hourOfDay);
+ jobExecTime.set(Calendar.MINUTE, minutes);
+ jobExecTime.set(Calendar.SECOND, 0);
+ jobExecTime.set(Calendar.MILLISECOND, 0);
+
+ if (next && jobExecTime.getTime().before(currentDate)) {
+ jobExecTime.roll(Calendar.DAY_OF_YEAR, true);
+ } else if (!next && jobExecTime.getTime().after(currentDate)) {
+ jobExecTime.roll(Calendar.DAY_OF_YEAR, false);
Review Comment:
Using Calendar.roll() instead of Calendar.add() causes incorrect behavior at
year boundaries. When rolling DAY_OF_YEAR on December 31, it will wrap to
January 1 of the same year instead of incrementing to the next year. For
example, if the current date is Dec 31, 2026 at 23:50 and the job is scheduled
for 00:00, roll() would incorrectly calculate Jan 1, 2026 instead of Jan 1,
2027.
The codebase consistently uses Calendar.add() for this purpose (see
DateUtil.java:229 and ManagementServerImpl.java:4181). This should be changed
to: jobExecTime.add(Calendar.DAY_OF_YEAR, 1);
```suggestion
jobExecTime.add(Calendar.DAY_OF_YEAR, 1);
} else if (!next && jobExecTime.getTime().after(currentDate)) {
jobExecTime.add(Calendar.DAY_OF_YEAR, -1);
```
##########
utils/src/main/java/org/apache/cloudstack/utils/usage/UsageUtils.java:
##########
@@ -19,6 +19,50 @@
package org.apache.cloudstack.utils.usage;
+import com.cloud.utils.DateUtil;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.Calendar;
+import java.util.Date;
+import java.util.TimeZone;
+
public class UsageUtils {
+ protected static Logger logger = LogManager.getLogger(UsageUtils.class);
+
public static final int USAGE_AGGREGATION_RANGE_MIN = 1;
+
+ public static Date getNextJobExecutionTime(TimeZone usageTimeZone, String
jobExecTimeConfig) {
+ return getJobExecutionTime(usageTimeZone, jobExecTimeConfig, true);
+ }
+
+ public static Date getPreviousJobExecutionTime(TimeZone usageTimeZone,
String jobExecTimeConfig) {
+ return getJobExecutionTime(usageTimeZone, jobExecTimeConfig, false);
+ }
+
+ protected static Date getJobExecutionTime(TimeZone usageTimeZone, String
jobExecTimeConfig, boolean next) {
+ String[] execTimeSegments = jobExecTimeConfig.split(":");
+ if (execTimeSegments.length != 2) {
+ logger.warn("Unable to parse configuration
'usage.stats.job.exec.time'.");
+ return null;
+ }
+ int hourOfDay = Integer.parseInt(execTimeSegments[0]);
+ int minutes = Integer.parseInt(execTimeSegments[1]);
+
+ Date currentDate = DateUtil.currentGMTTime();
+ Calendar jobExecTime = Calendar.getInstance(usageTimeZone);
+ jobExecTime.setTime(currentDate);
+ jobExecTime.set(Calendar.HOUR_OF_DAY, hourOfDay);
+ jobExecTime.set(Calendar.MINUTE, minutes);
+ jobExecTime.set(Calendar.SECOND, 0);
+ jobExecTime.set(Calendar.MILLISECOND, 0);
+
+ if (next && jobExecTime.getTime().before(currentDate)) {
+ jobExecTime.roll(Calendar.DAY_OF_YEAR, true);
+ } else if (!next && jobExecTime.getTime().after(currentDate)) {
+ jobExecTime.roll(Calendar.DAY_OF_YEAR, false);
Review Comment:
Using Calendar.roll() instead of Calendar.add() causes incorrect behavior at
year boundaries. When rolling DAY_OF_YEAR backwards on January 1, it will wrap
to December 31 of the same year instead of decrementing to the previous year.
For example, if the current date is Jan 1, 2027 at 00:05 and the job is
scheduled for 00:00, roll() would incorrectly calculate Dec 31, 2027 instead of
Dec 31, 2026.
The codebase consistently uses Calendar.add() for this purpose (see
DateUtil.java:229). This should be changed to:
jobExecTime.add(Calendar.DAY_OF_YEAR, -1);
```suggestion
jobExecTime.add(Calendar.DAY_OF_YEAR, 1);
} else if (!next && jobExecTime.getTime().after(currentDate)) {
jobExecTime.add(Calendar.DAY_OF_YEAR, -1);
```
##########
utils/src/main/java/org/apache/cloudstack/utils/usage/UsageUtils.java:
##########
@@ -19,6 +19,50 @@
package org.apache.cloudstack.utils.usage;
+import com.cloud.utils.DateUtil;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.Calendar;
+import java.util.Date;
+import java.util.TimeZone;
+
public class UsageUtils {
+ protected static Logger logger = LogManager.getLogger(UsageUtils.class);
+
public static final int USAGE_AGGREGATION_RANGE_MIN = 1;
+
+ public static Date getNextJobExecutionTime(TimeZone usageTimeZone, String
jobExecTimeConfig) {
+ return getJobExecutionTime(usageTimeZone, jobExecTimeConfig, true);
+ }
+
+ public static Date getPreviousJobExecutionTime(TimeZone usageTimeZone,
String jobExecTimeConfig) {
+ return getJobExecutionTime(usageTimeZone, jobExecTimeConfig, false);
+ }
+
+ protected static Date getJobExecutionTime(TimeZone usageTimeZone, String
jobExecTimeConfig, boolean next) {
+ String[] execTimeSegments = jobExecTimeConfig.split(":");
+ if (execTimeSegments.length != 2) {
+ logger.warn("Unable to parse configuration
'usage.stats.job.exec.time'.");
+ return null;
+ }
+ int hourOfDay = Integer.parseInt(execTimeSegments[0]);
+ int minutes = Integer.parseInt(execTimeSegments[1]);
Review Comment:
The Integer.parseInt() calls can throw NumberFormatException if the time
segments contain non-numeric values. This is not handled and could cause the
method to throw an uncaught exception. While the configuration is validated in
the calling code (UsageManagerImpl catches NumberFormatException), this method
should either handle the exception or document that it can be thrown. Consider
adding a try-catch block and logging a more descriptive error message before
returning null, similar to the pattern used in the length validation above.
```suggestion
int hourOfDay;
int minutes;
try {
hourOfDay = Integer.parseInt(execTimeSegments[0]);
minutes = Integer.parseInt(execTimeSegments[1]);
} catch (NumberFormatException e) {
logger.warn("Unable to parse configuration
'usage.stats.job.exec.time' due to non-numeric values: " + jobExecTimeConfig,
e);
return null;
}
```
##########
utils/src/test/java/org/apache/cloudstack/utils/usage/UsageUtilsTest.java:
##########
@@ -0,0 +1,105 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package org.apache.cloudstack.utils.usage;
+
+import com.cloud.utils.DateUtil;
+import junit.framework.TestCase;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.util.Date;
+import java.util.TimeZone;
+
+@RunWith(MockitoJUnitRunner.class)
+public class UsageUtilsTest extends TestCase {
+
+ TimeZone usageTimeZone = TimeZone.getTimeZone("GMT-3");
+
+ @Test
+ public void
getJobExecutionTimeTestReturnsNullWhenConfigurationValueIsInvalid() {
+ Date result = UsageUtils.getNextJobExecutionTime(usageTimeZone,
"test");
+ assertNull(result);
+ }
+
+ @Test
+ public void
getJobExecutionTimeTestReturnsExpectedDateWhenNextIsTrueAndExecutionTimeHasNotPassed()
{
+ Date currentDate = new Date();
+ currentDate.setTime(1724296800000L);
+
+ try (MockedStatic<DateUtil> dateUtilMockedStatic =
Mockito.mockStatic(DateUtil.class)) {
+
dateUtilMockedStatic.when(DateUtil::currentGMTTime).thenReturn(currentDate);
+
+ Date result = UsageUtils.getJobExecutionTime(usageTimeZone,
"00:30", true);
+
+ Assert.assertNotNull(result);
+ Assert.assertEquals(1724297400000L, result.getTime());
+ }
+ }
+
+ @Test
+ public void
getJobExecutionTimeTestReturnsExpectedDateWhenNextIsTrueAndExecutionTimeHasPassed()
{
+ Date currentDate = new Date();
+ currentDate.setTime(1724297460000L);
+
+ try (MockedStatic<DateUtil> dateUtilMockedStatic =
Mockito.mockStatic(DateUtil.class)) {
+
dateUtilMockedStatic.when(DateUtil::currentGMTTime).thenReturn(currentDate);
+
+ Date result = UsageUtils.getJobExecutionTime(usageTimeZone,
"00:30", true);
+
+ Assert.assertNotNull(result);
+ Assert.assertEquals(1724383800000L, result.getTime());
+ }
+ }
+
+ @Test
+ public void
getJobExecutionTimeTestReturnsExpectedDateWhenNextIsFalseAndExecutionTimeHasNotPassed()
{
+ Date currentDate = new Date();
+ currentDate.setTime(1724296800000L);
+
+ try (MockedStatic<DateUtil> dateUtilMockedStatic =
Mockito.mockStatic(DateUtil.class)) {
+
dateUtilMockedStatic.when(DateUtil::currentGMTTime).thenReturn(currentDate);
+
+ Date result = UsageUtils.getJobExecutionTime(usageTimeZone,
"00:30", false);
+
+ Assert.assertNotNull(result);
+ Assert.assertEquals(1724211000000L, result.getTime());
+ }
+ }
+
+ @Test
+ public void
getJobExecutionTimeTestReturnsExpectedDateWhenNextIsFalseAndExecutionTimeHasPassed()
{
+ Date currentDate = new Date();
+ currentDate.setTime(1724297460000L);
+
+ try (MockedStatic<DateUtil> dateUtilMockedStatic =
Mockito.mockStatic(DateUtil.class)) {
+
dateUtilMockedStatic.when(DateUtil::currentGMTTime).thenReturn(currentDate);
+
+ Date result = UsageUtils.getJobExecutionTime(usageTimeZone,
"00:30", false);
+
+ Assert.assertNotNull(result);
+ Assert.assertEquals(1724297400000L, result.getTime());
+ }
+ }
+
+}
Review Comment:
The test cases do not cover the critical year boundary scenarios that this
PR is meant to fix. Consider adding test cases for:
1. When the current date is Dec 31 at 23:59 and the job is scheduled for
00:00 (should calculate Jan 1 of next year)
2. When the current date is Jan 1 at 00:01 and the job is scheduled for
00:00 (should calculate Dec 31 of previous year for getPreviousJobExecutionTime)
These scenarios are the edge cases most likely to expose bugs with
Calendar.roll() vs Calendar.add().
--
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]
