From ac0070e7100ceac90861b37dab0669e5b3e7aabb Mon Sep 17 00:00:00 2001
From: Margarit Hakobyan <margarit.hakobyan@improving.com>
Date: Tue, 3 Jan 2023 12:35:12 -0800
Subject: [PATCH] Fixes "`ResultSet.getTime()` returns `null`" bug (#1)

* Fixes ResultSet.getTime returns null bug

Signed-off-by: Margarit Hakobyan <margarit.hakobyan@improving.com>
---
 .../org/opensearch/jdbc/ResultSetImpl.java    | 30 +++++++---
 .../org/opensearch/jdbc/types/TimeType.java   | 58 ++++++++++++------
 .../opensearch/jdbc/types/TypeConverters.java |  2 +-
 .../opensearch/jdbc/types/TimeTypeTest.java   | 60 ++++++++++++++++++-
 4 files changed, 121 insertions(+), 29 deletions(-)

diff --git a/src/main/java/org/opensearch/jdbc/ResultSetImpl.java b/src/main/java/org/opensearch/jdbc/ResultSetImpl.java
index 3cace79..e34a42d 100644
--- a/src/main/java/org/opensearch/jdbc/ResultSetImpl.java
+++ b/src/main/java/org/opensearch/jdbc/ResultSetImpl.java
@@ -347,14 +347,18 @@ private Date getDateX(int columnIndex, Calendar calendar) throws SQLException {
     public Time getTime(int columnIndex) throws SQLException {
         log.debug(() -> logEntry("getTime (%d)", columnIndex));
         checkCursorOperationPossible();
-        Time value = getTimeX(columnIndex);
+        Time value = getTimeX(columnIndex, null);
         log.debug(() -> logExit("getTime", value));
         return value;
     }
 
-    private Time getTimeX(int columnIndex) throws SQLException {
-        // TODO - add/check support
-        return getObjectX(columnIndex, Time.class);
+    private Time getTimeX(int columnIndex, Calendar calendar) throws SQLException {
+        Map<String, Object> conversionParams = null;
+        if (calendar != null) {
+            conversionParams = new HashMap<>();
+            conversionParams.put("calendar", calendar);
+        }
+        return getObjectX(columnIndex, Time.class, conversionParams);
     }
 
     @Override
@@ -494,7 +498,7 @@ public Date getDate(String columnLabel) throws SQLException {
     public Time getTime(String columnLabel) throws SQLException {
         log.debug(() -> logEntry("getTime (%s)", columnLabel));
         checkCursorOperationPossible();
-        Time value = getTimeX(getColumnIndex(columnLabel));
+        Time value = getTimeX(getColumnIndex(columnLabel), null);
         log.debug(() -> logExit("getTime", value));
         return value;
     }
@@ -1071,14 +1075,22 @@ public Date getDate(String columnLabel, Calendar cal) throws SQLException {
 
     @Override
     public Time getTime(int columnIndex, Calendar cal) throws SQLException {
-        // TODO - implement?
-        return null;
+        log.debug(() -> logEntry("getTime (%d, %s)", columnIndex,
+                cal == null ? "null" : "Calendar TZ= " + cal.getTimeZone()));
+        checkCursorOperationPossible();
+        Time value = getTimeX(columnIndex, cal);
+        log.debug(() -> logExit("getTime", value));
+        return value;
     }
 
     @Override
     public Time getTime(String columnLabel, Calendar cal) throws SQLException {
-        // TODO - implement?
-        return null;
+        log.debug(() -> logEntry("getTime (%s, %s)", columnLabel,
+                cal == null ? "null" : "Calendar TZ= " + cal.getTimeZone()));
+        checkCursorOperationPossible();
+        Time value = getTimeX(getColumnIndex(columnLabel), cal);
+        log.debug(() -> logExit("getTime", value));
+        return value;
     }
 
     @Override
diff --git a/src/main/java/org/opensearch/jdbc/types/TimeType.java b/src/main/java/org/opensearch/jdbc/types/TimeType.java
index ad44c7f..73593c3 100644
--- a/src/main/java/org/opensearch/jdbc/types/TimeType.java
+++ b/src/main/java/org/opensearch/jdbc/types/TimeType.java
@@ -8,7 +8,14 @@
 
 import java.sql.SQLException;
 import java.sql.Time;
-import java.time.LocalTime;
+import java.sql.Timestamp;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.format.DateTimeFormatter;
+import java.time.format.DateTimeParseException;
+import java.time.temporal.TemporalAccessor;
+import java.util.Calendar;
+import java.util.Locale;
 import java.util.Map;
 
 public class TimeType implements TypeHelper<Time>{
@@ -24,41 +31,56 @@ public Time fromValue(Object value, Map<String, Object> conversionParams) throws
     if (value == null) {
       return null;
     }
+    Calendar calendar = conversionParams != null ? (Calendar) conversionParams.get("calendar") : null;
     if (value instanceof Time) {
-      return asTime((Time) value);
+      return (Time) value;
     } else if (value instanceof String) {
-      return asTime((String) value);
+      return asTime((String) value, calendar);
     } else if (value instanceof Number) {
-      return this.asTime((Number) value);
+      return asTime((Number) value);
     } else {
       throw objectConversionException(value);
     }
   }
 
-  public Time asTime(Time value) {
-    return localTimetoSqlTime(value.toLocalTime());
-  }
+  public Time asTime(String value, Calendar calendar) {
+    Time time;
+    LocalDateTime localDateTime;
 
-  public Time asTime(String value) throws SQLException {
-    return localTimetoSqlTime(toLocalTime(value));
-  }
+    try {
+      TemporalAccessor temporal = DateTimeFormatter
+              .ofPattern("yyyy-MM-dd HH:mm:ss", Locale.getDefault())
+              .parse(value);
+
+      localDateTime = LocalDateTime.from(temporal);
+      time = Time.valueOf(localDateTime.toLocalTime());
+    } catch (DateTimeParseException exception) {
+      time = Time.valueOf(value);
+    }
+
+    if (calendar == null) {
+      return time;
+    }
 
-  private Time localTimetoSqlTime(LocalTime localTime) {
-    return new Time(localTime.getHour(), localTime.getMinute(), localTime.getSecond());
+    localDateTime = time.toLocalTime().atDate(LocalDate.now());
+
+    return localDateTimeToTime(localDateTime, calendar);
   }
 
   public Time asTime(Number value) {
     return new Time(value.longValue());
   }
 
-  private LocalTime toLocalTime(String value) throws SQLException {
-    if (value == null)
-      throw stringConversionException(value, null);
-    return LocalTime.parse(value);
-  }
-
   @Override
   public String getTypeName() {
     return "Time";
   }
+
+  private Time localDateTimeToTime(LocalDateTime ldt, Calendar calendar) {
+    calendar.set(ldt.getYear(), ldt.getMonthValue()-1, ldt.getDayOfMonth(),
+            ldt.getHour(), ldt.getMinute(), ldt.getSecond());
+    calendar.set(Calendar.MILLISECOND, ldt.getNano()/1000000);
+
+    return new Time(new Timestamp(calendar.getTimeInMillis()).getTime());
+  }
 }
diff --git a/src/main/java/org/opensearch/jdbc/types/TypeConverters.java b/src/main/java/org/opensearch/jdbc/types/TypeConverters.java
index 2dce3e8..189e970 100644
--- a/src/main/java/org/opensearch/jdbc/types/TypeConverters.java
+++ b/src/main/java/org/opensearch/jdbc/types/TypeConverters.java
@@ -66,7 +66,7 @@ public static class TimestampTypeConverter extends BaseTypeConverter {
 
         private static final Set<Class> supportedJavaClasses = Collections.unmodifiableSet(
                 new HashSet<>(Arrays.asList(
-                        String.class, Timestamp.class
+                        String.class, Timestamp.class, Time.class
                 )));
 
         private TimestampTypeConverter() {
diff --git a/src/test/java/org/opensearch/jdbc/types/TimeTypeTest.java b/src/test/java/org/opensearch/jdbc/types/TimeTypeTest.java
index 263c1dd..bec269e 100644
--- a/src/test/java/org/opensearch/jdbc/types/TimeTypeTest.java
+++ b/src/test/java/org/opensearch/jdbc/types/TimeTypeTest.java
@@ -8,8 +8,13 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
+import com.google.common.collect.ImmutableMap;
+import org.junit.jupiter.api.Test;
 import org.opensearch.jdbc.test.UTCTimeZoneTestExtension;
 import java.sql.Time;
+import java.time.LocalTime;
+import java.util.Calendar;
+
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.params.ParameterizedTest;
@@ -22,11 +27,64 @@ public class TimeTypeTest {
   @CsvSource(value = {
       "00:00:00, 00:00:00",
       "01:01:01, 01:01:01",
-      "23:59:59, 23:59:59"
+      "23:59:59, 23:59:59",
+      "1880-12-22 00:00:00, 00:00:00",
+      "2000-01-10 01:01:01, 01:01:01",
+      "1998-08-17 23:59:59, 23:59:59"
   })
   void testTimeFromString(String inputString, String resultString) {
     Time time = Assertions.assertDoesNotThrow(
         () -> TimeType.INSTANCE.fromValue(inputString, null));
     assertEquals(resultString, time.toString());
   }
+
+  @ParameterizedTest
+  @CsvSource(value = {
+      "00:00:00, 00:00:00",
+      "01:01:01, 01:01:01",
+      "23:59:59, 23:59:59",
+      "1880-12-22 00:00:00, 00:00:00",
+      "2000-01-10 01:01:01, 01:01:01",
+      "1998-08-17 23:59:59, 23:59:59"
+  })
+  void testTimeFromStringWithCalendar(String inputString, String resultString) {
+    Time time = Assertions.assertDoesNotThrow(
+            () -> TimeType.INSTANCE.fromValue(
+                    inputString, ImmutableMap.of("calendar", Calendar.getInstance())));
+    assertEquals(resultString, time.toString());
+  }
+
+  @Test
+  void testTimeFromTime() {
+    Time timeNow = Time.valueOf(LocalTime.now());
+    Time time = Assertions.assertDoesNotThrow(
+            () -> TimeType.INSTANCE.fromValue(timeNow, null));
+    assertEquals(timeNow, time);
+  }
+
+  @Test
+  void testTimeFromTimeWithCalendar() {
+    Time timeNow = Time.valueOf(LocalTime.now());
+    Time time = Assertions.assertDoesNotThrow(
+            () -> TimeType.INSTANCE.fromValue(
+                    timeNow, ImmutableMap.of("calendar", Calendar.getInstance())));
+    assertEquals(timeNow, time);
+  }
+
+  @Test
+  void testTimeFromNumber() {
+    long currentTimestamp = System.currentTimeMillis();
+    Time time = Assertions.assertDoesNotThrow(
+            () -> TimeType.INSTANCE.fromValue(currentTimestamp, null));
+    assertEquals(currentTimestamp, time.getTime());
+  }
+
+  @Test
+  void testTimeFromNumberWithCalendar() {
+    long currentTimestamp = System.currentTimeMillis();
+    Time time = Assertions.assertDoesNotThrow(
+            () -> TimeType.INSTANCE.fromValue(
+                    currentTimestamp, ImmutableMap.of("calendar", Calendar.getInstance())));
+    assertEquals(currentTimestamp, time.getTime());
+  }
 }