CAY-2804 LocalTimeValueType potential loss of precision

Signed-off-by: Nikita Timofeev <[email protected]>

cayenne/src/main/java/org/apache/cayenne/access/types/LocalTimeValueType.java

@@ -20,7 +20,12 @@
 package org.apache.cayenne.access.types;
 
 import java.sql.Time;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
 import java.time.LocalTime;
+import java.time.ZoneId;
+import java.time.ZonedDateTime;
 
 /**
  * @since 4.0
@@ -39,12 +44,16 @@ public Class<LocalTime> getValueType() {
 
  @Override
  public LocalTime toJavaObject(Time value) {
- return value.toLocalTime();
+ Instant instant = Instant.ofEpochMilli(value.getTime());
+ return LocalTime.ofInstant(instant, ZoneId.systemDefault());
  }
 
  @Override
  public Time fromJavaObject(LocalTime object) {
- return Time.valueOf(object);
+ LocalDateTime epochDateTime = object.atDate(LocalDate.EPOCH);
+ ZonedDateTime zonedDateTime = epochDateTime.atZone(ZoneId.systemDefault());
+ long epochMillis = zonedDateTime.toInstant().toEpochMilli();
+ return new Time(epochMillis);
  }
 
  @Override

cayenne/src/main/java/org/apache/cayenne/dba/AutoAdapter.java

@@ -153,6 +153,14 @@ public boolean typeSupportsLength(int type) {
  return getAdapter().typeSupportsLength(type);
  }
 
+ /**
+ * @since 5.0
+ */
+ @Override
+ public boolean typeSupportsScale(int type) {
+ return getAdapter().typeSupportsScale(type);
+ }
+
  @Override
  public Collection<String> dropTableStatements(DbEntity table) {
  return getAdapter().dropTableStatements(table);

cayenne/src/main/java/org/apache/cayenne/dba/DbAdapter.java

@@ -110,6 +110,16 @@ default boolean supportsGeneratedKeysForBatchInserts() {
 
  boolean typeSupportsLength(int type);
 
+ /**
+ * Returns true if supplied type can have a scale attribute as a part of column definition.
+ *
+ * @param type sql type code
+ * @return <code>true</code> if a given type supports scale
+ *
+ * @since 5.0
+ */
+ boolean typeSupportsScale(int type);
+
  /**
  * Returns a collection of SQL statements needed to drop a database table.
  *

cayenne/src/main/java/org/apache/cayenne/dba/JdbcAdapter.java

@@ -32,7 +32,6 @@
 import org.apache.cayenne.access.DataNode;
 import org.apache.cayenne.access.sqlbuilder.sqltree.SQLTreeProcessor;
 import org.apache.cayenne.access.translator.ParameterBinding;
-import org.apache.cayenne.access.translator.batch.BatchTranslatorFactory;
 import org.apache.cayenne.access.translator.ejbql.EJBQLTranslatorFactory;
 import org.apache.cayenne.access.translator.ejbql.JdbcEJBQLTranslatorFactory;
 import org.apache.cayenne.access.translator.select.DefaultSelectTranslator;
@@ -247,12 +246,27 @@ public void setSupportsUniqueConstraints(boolean flag) {
  *
  * @since 4.0
  */
+ @Override
  public boolean typeSupportsLength(int type) {
  return type == Types.BINARY || type == Types.CHAR || type == Types.NCHAR || type == Types.NVARCHAR
  || type == Types.LONGNVARCHAR || type == Types.DECIMAL || type == Types.DOUBLE || type == Types.FLOAT
  || type == Types.NUMERIC || type == Types.REAL || type == Types.VARBINARY || type == Types.VARCHAR;
  }
 
+ /**
+ * Returns true if supplied type can have a scale attribute as a part of column definition.
+ *
+ * @param type sql type code
+ * @return <code>true</code> if a given type supports scale
+ *
+ * @since 5.0
+ */
+ @Override
+ public boolean typeSupportsScale(int type) {
+ return type == Types.DECIMAL || type == Types.DOUBLE || type == Types.REAL || type == Types.NUMERIC
+ || type == Types.TIME || type == Types.TIMESTAMP;
+ }
+
  /**
  * @since 3.0
  */
@@ -342,21 +356,21 @@ public void createTableAppendColumn(StringBuffer sqlBuffer, DbAttribute column)
  }
 
  public static String sizeAndPrecision(DbAdapter adapter, DbAttribute column) {
- if (!adapter.typeSupportsLength(column.getType())) {
+ if (!adapter.typeSupportsLength(column.getType()) && !adapter.typeSupportsScale(column.getType())) {
  return "";
  }
 
  int len = column.getMaxLength();
- int scale = TypesMapping.isDecimal(column.getType()) && column.getType() != Types.FLOAT ? column.getScale()
- : -1;
+ int scale = TypesMapping.isDateTime(column.getType())
+ || TypesMapping.isDecimal(column.getType()) && column.getType() != Types.FLOAT
+ ? column.getScale() : -1;
 
- // sanity check
- if (scale > len) {
- scale = -1;
+ if (len > 0) {
+ return "(" + len + (scale >= 0 && len > scale ? ", " + scale : "") + ")";
  }
 
- if (len > 0) {
- return "(" + len + (scale >= 0 ? ", " + scale : "") + ")";
+ if (scale >= 0 && TypesMapping.isDateTime(column.getType())) {
+ return "(" + scale + ")";
  }
 
  return "";

cayenne/src/main/java/org/apache/cayenne/dba/TypesMapping.java

@@ -310,6 +310,13 @@ public static boolean isDecimal(int type) {
  return type == DECIMAL || type == DOUBLE || type == FLOAT || type == REAL || type == NUMERIC;
  }
 
+ /**
+ * @since 5.0
+ */
+ public static boolean isDateTime(int type) {
+ return type == TIME || type == TIMESTAMP;
+ }
+
  /**
  * Returns an array of string names of the default JDBC data types.
  */

cayenne/src/main/java/org/apache/cayenne/dba/db2/DB2Adapter.java

@@ -133,6 +133,14 @@ public boolean typeSupportsLength(int type) {
  return type == Types.LONGVARCHAR || type == Types.LONGVARBINARY || super.typeSupportsLength(type);
  }
 
+ /**
+ * @since 5.0
+ */
+ @Override
+ public boolean typeSupportsScale(int type) {
+ return type != Types.TIME && super.typeSupportsScale(type);
+ }
+
  /**
  * @since 4.2
  */

cayenne/src/main/java/org/apache/cayenne/dba/derby/DerbyAdapter.java

@@ -165,6 +165,14 @@ public boolean typeSupportsLength(int type) {
  }
  }
 
+ /**
+ * @since 5.0
+ */
+ @Override
+ public boolean typeSupportsScale(int type) {
+ return type != Types.TIME && super.typeSupportsScale(type);
+ }
+
  /**
  * @since 4.2
  */

cayenne/src/main/java/org/apache/cayenne/dba/oracle/Oracle8Adapter.java

@@ -32,6 +32,7 @@
 
 import java.lang.reflect.Method;
 import java.net.URL;
+import java.sql.Types;
 import java.util.List;
 
 /**
@@ -96,4 +97,11 @@ protected URL findResource(String name) {
  return super.findResource(name);
  }
 
+ /**
+ * @since 5.0
+ */
+ @Override
+ public boolean typeSupportsScale(int type) {
+ return type != Types.TIMESTAMP && super.typeSupportsScale(type);
+ }
 }

cayenne/src/main/java/org/apache/cayenne/dba/oracle/OracleAdapter.java

@@ -300,6 +300,14 @@ public List<String> getSystemSchemas() {
  return SYSTEM_SCHEMAS;
  }
 
+ /**
+ * @since 5.0
+ */
+ @Override
+ public boolean typeSupportsScale(int type) {
+ return type != Types.TIME && super.typeSupportsScale(type);
+ }
+
  /**
  * @since 3.0
  */

cayenne/src/test/java/org/apache/cayenne/access/types/Java8TimeIT.java

@@ -26,6 +26,7 @@
 import java.time.LocalTime;
 import java.time.Period;
 import java.time.temporal.ChronoField;
+import java.time.temporal.TemporalField;
 
 import org.apache.cayenne.access.DataContext;
 import org.apache.cayenne.di.Inject;
@@ -36,6 +37,7 @@
 import org.apache.cayenne.testdo.java8.LocalDateTimeTestEntity;
 import org.apache.cayenne.testdo.java8.LocalTimeTestEntity;
 import org.apache.cayenne.testdo.java8.PeriodTestEntity;
+import org.apache.cayenne.unit.UnitDbAdapter;
 import org.apache.cayenne.unit.di.runtime.CayenneProjects;
 import org.apache.cayenne.unit.di.runtime.RuntimeCase;
 import org.apache.cayenne.unit.di.runtime.UseCayenneRuntime;
@@ -52,6 +54,9 @@ public class Java8TimeIT extends RuntimeCase {
  @Inject
  private DataContext context;
 
+ @Inject
+ private UnitDbAdapter unitDbAdapter;
+
  @Inject
  private DBHelper dbHelper;
 
@@ -101,10 +106,14 @@ public void testJava8LocalTime() {
 
  LocalTimeTestEntity testRead = ObjectSelect.query(LocalTimeTestEntity.class).selectOne(context);
 
+ TemporalField testValue = unitDbAdapter.supportsPreciseTime()
+ ? ChronoField.MILLI_OF_DAY
+ : ChronoField.SECOND_OF_DAY;
+
  assertNotNull(testRead.getTime());
  assertEquals(LocalTime.class, testRead.getTime().getClass());
  assertEquals(localTime.toSecondOfDay(), testRead.getTime().toSecondOfDay());
-
+ assertEquals(localTime.get(testValue), testRead.getTime().get(testValue));
  }
 
  @Test

cayenne/src/test/java/org/apache/cayenne/access/types/LocalTimeValueTypeTest.java

@@ -0,0 +1,137 @@
+/*****************************************************************
+ * 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
+ *
+ * https://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.cayenne.access.types;
+
+import org.junit.Test;
+
+import java.sql.Time;
+import java.time.Instant;
+import java.time.LocalTime;
+import java.time.ZoneId;
+import java.time.temporal.ChronoField;
+import java.util.Arrays;
+import java.util.List;
+import java.util.TimeZone;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+
+public class LocalTimeValueTypeTest {
+
+ private static final LocalTimeValueType valueType = new LocalTimeValueType();
+
+ @Test
+ public void testToJavaObject() {
+ // UTC 17:50:23.123
+ long utcMillis = 64223123;
+ long systemMillis = Instant.ofEpochMilli(utcMillis)
+ .atZone(ZoneId.systemDefault())
+ .get(ChronoField.MILLI_OF_DAY);
+
+ Time time = new Time(utcMillis);
+ LocalTime localTime = valueType.toJavaObject(time);
+
+ assertEquals(systemMillis, localTime.get(ChronoField.MILLI_OF_DAY));
+ }
+
+ @Test
+ public void testFromJavaObject() {
+ // UTC 17:50:23.123
+ long utcMillis = 64223123;
+ long systemMillis = Instant.ofEpochMilli(utcMillis)
+ .atZone(ZoneId.systemDefault())
+ .getLong(ChronoField.MILLI_OF_DAY);
+
+ LocalTime localTime = LocalTime.ofNanoOfDay(TimeUnit.MILLISECONDS.toNanos(systemMillis));
+ Time time = valueType.fromJavaObject(localTime);
+
+ assertEquals(utcMillis, time.getTime());
+ }
+
+ @Test
+ public void testToJavaObjectFromJavaObject() {
+ // UTC 17:50:23.123
+ long utcMillis = 64223123;
+
+ Time time = new Time(utcMillis);
+ LocalTime localTime = valueType.toJavaObject(time);
+ Time newTime = valueType.fromJavaObject(localTime);
+
+ assertEquals(time, newTime);
+ }
+
+ @Test
+ public void testToJavaObject_isBackwardCompatible() {
+ // UTC 17:50:23.123
+ long utcMillis = 64223123;
+
+ Time time = new Time(utcMillis);
+ LocalTime impreciseLocalTime = time.toLocalTime();
+ LocalTime localTime = valueType.toJavaObject(time);
+
+ assertEquals(impreciseLocalTime.toSecondOfDay(), localTime.get(ChronoField.SECOND_OF_DAY));
+ }
+
+ @Test
+ public void testFromJavaObject_isBackwardCompatible() {
+ // UTC 17:50:23.123
+ long utcMillis = 64223123;
+ long systemMillis = Instant.ofEpochMilli(utcMillis)
+ .atZone(ZoneId.systemDefault())
+ .getLong(ChronoField.MILLI_OF_DAY);
+
+ LocalTime localTime = LocalTime.ofNanoOfDay(TimeUnit.MILLISECONDS.toNanos(systemMillis));
+ Time impreciseTime = Time.valueOf(localTime);
+ Time time = valueType.fromJavaObject(localTime);
+
+ assertEquals(TimeUnit.MILLISECONDS.toSeconds(impreciseTime.getTime()),
+ TimeUnit.MILLISECONDS.toSeconds(time.getTime()));
+ }
+
+ @Test
+ public void testToJavaObjectFromJavaObject_changeTimeZone() {
+ TimeZone originalTimeZone = TimeZone.getDefault();
+
+ try {
+ // UTC 17:50:23.123
+ long utcMillis = 64223123;
+
+ Time time = new Time(utcMillis);
+ LocalTime localTime = valueType.toJavaObject(time);
+ TimeZone.setDefault(getOtherTimeZone(originalTimeZone));
+ Time newTime = valueType.fromJavaObject(localTime);
+
+ assertNotEquals(time, newTime);
+ } finally {
+ TimeZone.setDefault(originalTimeZone);
+ }
+ }
+
+ private static TimeZone getOtherTimeZone(TimeZone timeZone) {
+ List<TimeZone> timeZones = Arrays.stream(TimeZone.getAvailableIDs())
+ .map(TimeZone::getTimeZone)
+ .distinct()
+ .filter(tz -> tz.getRawOffset() != timeZone.getRawOffset())
+ .collect(Collectors.toList());
+ return timeZones.get(0);
+ }
+}

cayenne/src/test/java/org/apache/cayenne/unit/DB2UnitDbAdapter.java

@@ -73,4 +73,9 @@ public boolean supportsExpressionInHaving() {
  public boolean supportsSelectBooleanExpression() {
  return false;
  }
+
+ @Override
+ public boolean supportsPreciseTime() {
+ return false;
+ }
 }