Improve date/timestamp handling performance in GenericDaoBase and DateUtil

engine/schema/src/main/java/org/apache/cloudstack/backup/BackupVO.java

@@ -60,7 +60,7 @@ public class BackupVO implements Backup {
     private String backupType;
 
     @Column(name = "date")
-    @Temporal(value = TemporalType.DATE)
+    @Temporal(value = TemporalType.TIMESTAMP)
     private Date date;
 
     @Column(name = GenericDao.REMOVED_COLUMN)

framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java

@@ -34,6 +34,9 @@
 import java.sql.ResultSetMetaData;
 import java.sql.SQLException;
 import java.sql.Statement;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.sql.Types;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Calendar;
@@ -67,7 +70,6 @@
 import org.apache.commons.lang3.exception.ExceptionUtils;
 
 import com.amazonaws.util.CollectionUtils;
-import com.cloud.utils.DateUtil;
 import com.cloud.utils.NumbersUtil;
 import com.cloud.utils.Pair;
 import com.cloud.utils.Ternary;
@@ -129,6 +131,28 @@ public abstract class GenericDaoBase<T, ID extends Serializable> extends Compone
 
     protected final static TimeZone s_gmtTimeZone = TimeZone.getTimeZone("GMT");
 
+    /**
+     * Returns a fresh GMT {@link Calendar} for a single JDBC get/set timestamp call. Calendar is
+     * mutable and JDBC drivers may mutate the instance passed to them, so a new one is used per call
+     * rather than sharing a single instance across concurrent DAO operations.
+     */
+    protected static Calendar gmtCalendar() {
+        return Calendar.getInstance(s_gmtTimeZone);
+    }
+
+    /**
+     * Returns the SQL type ({@link Types}) matching the temporal flag of the given attribute, so a
+     * null date/time/timestamp column is bound with the correct type instead of always TIMESTAMP.
+     */
+    protected static int temporalSqlType(Attribute attr) {
+        if (attr.is(Attribute.Flag.Date)) {
+            return Types.DATE;
+        } else if (attr.is(Attribute.Flag.Time)) {
+            return Types.TIME;
+        }
+        return Types.TIMESTAMP;
+    }
+
     protected final static Map<Class<?>, GenericDao<?, ? extends Serializable>> s_daoMaps = new ConcurrentHashMap<Class<?>, GenericDao<?, ? extends Serializable>>(71);
     private final ConversionSupport _conversionSupport;
 
@@ -598,20 +622,16 @@ protected void setField(Object entity, Field field, ResultSet rs, int index) thr
                     field.set(entity, rs.getInt(index));
                 }
             } else if (type == Date.class) {
-                final Object data = rs.getDate(index);
-                if (data == null) {
-                    field.set(entity, null);
-                    return;
-                }
-                field.set(entity, DateUtil.parseDateString(s_gmtTimeZone, rs.getString(index)));
+                final Timestamp ts = rs.getTimestamp(index, gmtCalendar());
+                field.set(entity, ts == null ? null : new Date(ts.getTime()));
             } else if (type == Calendar.class) {
-                final Object data = rs.getDate(index);
+                final Timestamp data = rs.getTimestamp(index, gmtCalendar());
                 if (data == null) {
                     field.set(entity, null);
                     return;
                 }
-                final Calendar cal = Calendar.getInstance();
-                cal.setTime(DateUtil.parseDateString(s_gmtTimeZone, rs.getString(index)));
+                final Calendar cal = Calendar.getInstance(s_gmtTimeZone);
+                cal.setTime(data);
                 field.set(entity, cal);
             } else if (type == boolean.class) {
                 field.setBoolean(entity, rs.getBoolean(index));
@@ -732,11 +752,11 @@ protected static <M> M getObject(Class<M> type, ResultSet rs, int index) throws
                 return (M) (Long) rs.getLong(index);
             }
         } else if (type == Date.class) {
-            final Object data = rs.getDate(index);
-            if (data == null) {
+            final Timestamp ts = rs.getTimestamp(index, gmtCalendar());
+            if (ts == null) {
                 return null;
             } else {
-                return (M)DateUtil.parseDateString(s_gmtTimeZone, rs.getString(index));
+                return (M) new Date(ts.getTime());
             }
         } else if (type == short.class) {
             return (M) (Short) rs.getShort(index);
@@ -779,12 +799,12 @@ protected static <M> M getObject(Class<M> type, ResultSet rs, int index) throws
                 return (M) (Byte) rs.getByte(index);
             }
         } else if (type == Calendar.class) {
-            final Object data = rs.getDate(index);
+            final Timestamp data = rs.getTimestamp(index, gmtCalendar());
             if (data == null) {
                 return null;
             } else {
-                final Calendar cal = Calendar.getInstance();
-                cal.setTime(DateUtil.parseDateString(s_gmtTimeZone, rs.getString(index)));
+                final Calendar cal = Calendar.getInstance(s_gmtTimeZone);
+                cal.setTime(data);
                 return (M)cal;
             }
         } else if (type == byte[].class) {
@@ -1696,7 +1716,12 @@ protected void insertElementCollection(T entity, Attribute idAttribute, ID id, M
             while (en.hasMoreElements()) {
                 pstmt = txn.prepareAutoCloseStatement(ec.insertSql);
                 if (ec.targetClass == Date.class) {
-                    pstmt.setString(1, DateUtil.getDateDisplayString(s_gmtTimeZone, (Date)en.nextElement()));
+                    Date d = (Date) en.nextElement();
+                    if (d == null) {
+                        pstmt.setNull(1, Types.TIMESTAMP);
+                    } else {
+                        pstmt.setTimestamp(1, new Timestamp(d.getTime()), gmtCalendar());
+                    }
                 } else {
                     pstmt.setObject(1, en.nextElement());
                 }
@@ -1800,28 +1825,28 @@ protected void prepareAttribute(final int j, final PreparedStatement pstmt, fina
         } else if (attr.field.getType() == Date.class) {
             final Date date = (Date)value;
             if (date == null || date.equals(DATE_TO_NULL)) {
-                pstmt.setObject(j, null);
+                pstmt.setNull(j, temporalSqlType(attr));
                 return;
             }
             if (attr.is(Attribute.Flag.Date)) {
-                pstmt.setString(j, DateUtil.getDateDisplayString(s_gmtTimeZone, date));
+                pstmt.setDate(j, new java.sql.Date(date.getTime()), gmtCalendar());
             } else if (attr.is(Attribute.Flag.TimeStamp)) {
-                pstmt.setString(j, DateUtil.getDateDisplayString(s_gmtTimeZone, date));
+                pstmt.setTimestamp(j, new Timestamp(date.getTime()), gmtCalendar());
             } else if (attr.is(Attribute.Flag.Time)) {
-                pstmt.setString(j, DateUtil.getDateDisplayString(s_gmtTimeZone, date));
+                pstmt.setTime(j, new java.sql.Time(date.getTime()), gmtCalendar());
             }
         } else if (attr.field.getType() == Calendar.class) {
             final Calendar cal = (Calendar)value;
             if (cal == null) {
-                pstmt.setObject(j, null);
+                pstmt.setNull(j, temporalSqlType(attr));
                 return;
             }
             if (attr.is(Attribute.Flag.Date)) {
-                pstmt.setString(j, DateUtil.getDateDisplayString(s_gmtTimeZone, cal.getTime()));
+                pstmt.setDate(j, new java.sql.Date(cal.getTimeInMillis()), gmtCalendar());
             } else if (attr.is(Attribute.Flag.TimeStamp)) {
-                pstmt.setString(j, DateUtil.getDateDisplayString(s_gmtTimeZone, cal.getTime()));
+                pstmt.setTimestamp(j, new Timestamp(cal.getTimeInMillis()), gmtCalendar());
             } else if (attr.is(Attribute.Flag.Time)) {
-                pstmt.setString(j, DateUtil.getDateDisplayString(s_gmtTimeZone, cal.getTime()));
+                pstmt.setTime(j, new Time(cal.getTimeInMillis()), gmtCalendar());
             }
         } else if (attr.field.getType().isEnum()) {
             final Enumerated enumerated = attr.field.getAnnotation(Enumerated.class);
@@ -1955,7 +1980,8 @@ protected void loadCollection(T entity, Attribute attr) {
                     }
                 } else if (ec.targetClass == Date.class) {
                     while (rs.next()) {
-                        lst.add(DateUtil.parseDateString(s_gmtTimeZone, rs.getString(1)));
+                        final Timestamp ts = rs.getTimestamp(1, gmtCalendar());
+                        lst.add(ts == null ? null : new Date(ts.getTime()));
                     }
                 } else if (ec.targetClass == Boolean.class) {
                     while (rs.next()) {

framework/db/src/test/java/com/cloud/utils/db/GenericDaoBaseTest.java

@@ -18,9 +18,14 @@
 
 import java.sql.ResultSet;
 import java.sql.SQLException;
+import java.sql.Timestamp;
+import java.sql.Types;
 import java.util.ArrayList;
+import java.util.Calendar;
 import java.util.Collection;
+import java.util.Date;
 import java.util.List;
+import java.util.TimeZone;
 
 import org.junit.Assert;
 import org.junit.Before;
@@ -331,4 +336,74 @@ public void testLockOneRandomRowReturnsFirstElement() {
         Assert.assertNotNull(result);
         Assert.assertEquals(expectedResult, result);
     }
+
+    @Test
+    public void gmtCalendarUsesGmtTimeZone() {
+        Calendar calendar = GenericDaoBase.gmtCalendar();
+
+        Assert.assertEquals(TimeZone.getTimeZone("GMT"), calendar.getTimeZone());
+    }
+
+    @Test
+    public void gmtCalendarReturnsFreshInstancePerCall() {
+        Assert.assertNotSame(GenericDaoBase.gmtCalendar(), GenericDaoBase.gmtCalendar());
+    }
+
+    @Test
+    public void temporalSqlTypeDate() {
+        Attribute attr = new Attribute("table", "column");
+        attr.flags = Attribute.Flag.Date.setTrue(attr.flags);
+
+        Assert.assertEquals(Types.DATE, GenericDaoBase.temporalSqlType(attr));
+    }
+
+    @Test
+    public void temporalSqlTypeTime() {
+        Attribute attr = new Attribute("table", "column");
+        attr.flags = Attribute.Flag.Time.setTrue(attr.flags);
+
+        Assert.assertEquals(Types.TIME, GenericDaoBase.temporalSqlType(attr));
+    }
+
+    @Test
+    public void temporalSqlTypeDefaultsToTimestamp() {
+        Attribute attr = new Attribute("table", "column");
+        attr.flags = Attribute.Flag.TimeStamp.setTrue(attr.flags);
+
+        Assert.assertEquals(Types.TIMESTAMP, GenericDaoBase.temporalSqlType(attr));
+    }
+
+    @Test
+    public void getObjectDateReadsViaGmtTimestamp() throws SQLException {
+        Timestamp ts = new Timestamp(1_700_000_000_000L);
+        Mockito.when(resultSet.getTimestamp(Mockito.eq(2), Mockito.any(Calendar.class))).thenReturn(ts);
+
+        Date result = GenericDaoBase.getObject(Date.class, resultSet, 2);
+
+        Assert.assertEquals(ts.getTime(), result.getTime());
+    }
+
+    @Test
+    public void getObjectDateNullTimestampReturnsNull() throws SQLException {
+        Mockito.when(resultSet.getTimestamp(Mockito.eq(3), Mockito.any(Calendar.class))).thenReturn(null);
+
+        Assert.assertNull(GenericDaoBase.getObject(Date.class, resultSet, 3));
+    }
+
+    @Test
+    public void getObjectCalendarReadsViaGmtTimestamp() throws SQLException {
+        Timestamp ts = new Timestamp(1_700_000_000_000L);
+        Mockito.when(resultSet.getTimestamp(Mockito.eq(4), Mockito.any(Calendar.class))).thenReturn(ts);
+
+        Calendar result = GenericDaoBase.getObject(Calendar.class, resultSet, 4);
+
+        Assert.assertEquals(ts.getTime(), result.getTimeInMillis());
+    }
+
+    @Test
+    public void getObjectCalendarNullTimestampReturnsNull() throws SQLException {
+        Mockito.when(resultSet.getTimestamp(Mockito.eq(5), Mockito.any(Calendar.class))).thenReturn(null);
+
+        Assert.assertNull(GenericDaoBase.getObject(Calendar.class, resultSet, 5));
+    }
 }

server/src/main/java/com/cloud/api/ApiServer.java

@@ -192,6 +192,7 @@
 
 import static com.cloud.user.AccountManagerImpl.apiKeyAccess;
 import static org.apache.cloudstack.user.UserPasswordResetManager.UserPasswordResetEnabled;
+import static org.apache.commons.lang3.StringUtils.deleteWhitespace;
 
 @Component
 public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiServerService, Configurable {
@@ -1372,19 +1373,17 @@ private void checkCommandAvailable(final User user, final String commandName, fi
             throw new PermissionDeniedException("User is null for role based API access check for command" + commandName);
         }
 
-        final Account account = accountMgr.getAccount(user.getAccountId());
-        final String accessAllowedCidrs = ApiServiceConfiguration.ApiAllowedSourceCidrList.valueIn(account.getId()).replaceAll("\\s","");
         final Boolean apiSourceCidrChecksEnabled = ApiServiceConfiguration.ApiSourceCidrChecksEnabled.value();
-
         if (apiSourceCidrChecksEnabled) {
+            final Account account = accountMgr.getAccount(user.getAccountId());
+            final String accessAllowedCidrs = deleteWhitespace(ApiServiceConfiguration.ApiAllowedSourceCidrList.valueIn(account.getId()));
             logger.debug("CIDRs from which account '" + account.toString() + "' is allowed to perform API calls: " + accessAllowedCidrs);
             if (!NetUtils.isIpInCidrList(remoteAddress, accessAllowedCidrs.split(","))) {
                 logger.warn("Request by account '" + account.toString() + "' was denied since " + remoteAddress + " does not match " + accessAllowedCidrs);
                 throw new OriginDeniedException("Calls from disallowed origin", account, remoteAddress);
                 }
         }
 
-
         for (final APIChecker apiChecker : apiAccessCheckers) {
             apiChecker.checkAccess(user, commandName);
         }