From 393c388e0d196e722e0e300e4d4401be76f90634 Mon Sep 17 00:00:00 2001
From: Shicong Huang <shicong@google.com>
Date: Tue, 1 Sep 2020 11:29:28 -0400
Subject: [PATCH] Consolidate conversion from Duration to Period in
 DurationConverter (#786)

* Consolidate conversion from Duration to Period in DurationConverter

* Resolve comment
---
 core/build.gradle                             |  2 --
 .../converter/DurationConverter.java          | 14 ++++++---
 .../converter/DurationConverterTest.java      | 31 ++++++++++++++-----
 3 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/core/build.gradle b/core/build.gradle
index 84459d8e2..a3bc9aa16 100644
--- a/core/build.gradle
+++ b/core/build.gradle
@@ -63,8 +63,6 @@ def dockerIncompatibleTestPatterns = [
     // methods, so we exclude the whole test class.
     "google/registry/tools/params/PathParameterTest.*",
     "google/registry/persistence/PersistenceModuleTest.*",
-    // This test is failing in docker when using Java 11. The cause is unclear.
-    "google/registry/tools/DomainLockUtilsTest.*",
 ]
 
 // Tests that conflict with members of both the main test suite and the
diff --git a/core/src/main/java/google/registry/persistence/converter/DurationConverter.java b/core/src/main/java/google/registry/persistence/converter/DurationConverter.java
index 35b85d67e..d45f25e7a 100644
--- a/core/src/main/java/google/registry/persistence/converter/DurationConverter.java
+++ b/core/src/main/java/google/registry/persistence/converter/DurationConverter.java
@@ -40,11 +40,17 @@ public class DurationConverter implements AttributeConverter<Duration, PGInterva
     if (duration == null) {
       return new PGInterval();
     }
+    // When the period is created from duration by calling duration.toPeriod(), only precise fields
+    // in the period type will be used. Thus, only the hour, minute, second and millisecond fields
+    // on the period will be used. The year, month, week and day fields will not be populated:
+    //   1. If the duration is small, less than one day, then this method will just set
+    //      hours/minutes/seconds correctly.
+    //   2. If the duration is larger than one day then all the remaining duration will
+    //      be stored in the largest available field, hours in this case.
+    // So, when we convert the period to a PGInterval instance, we set the days field by extracting
+    // it from period's hours field.
+    Period period = duration.toPeriod();
     PGInterval interval = new PGInterval();
-    Period period = new Period(duration);
-    // For some reason when the period is created from the duration, it does not set days, but
-    // instead just a total number of hours. Years and months are not created because those can
-    // differ in length of milliseconds.
     interval.setDays(period.getHours() / 24);
     interval.setHours(period.getHours() % 24);
     interval.setMinutes(period.getMinutes());
diff --git a/core/src/test/java/google/registry/persistence/converter/DurationConverterTest.java b/core/src/test/java/google/registry/persistence/converter/DurationConverterTest.java
index ef4dd101a..bfcd4e676 100644
--- a/core/src/test/java/google/registry/persistence/converter/DurationConverterTest.java
+++ b/core/src/test/java/google/registry/persistence/converter/DurationConverterTest.java
@@ -51,22 +51,37 @@ public class DurationConverterTest {
             .plus(Duration.standardMinutes(30))
             .plus(Duration.standardSeconds(15))
             .plus(Duration.millis(7));
-    DurationTestEntity entity = new DurationTestEntity(testDuration);
-    jpaTm().transact(() -> jpaTm().getEntityManager().persist(entity));
-    DurationTestEntity persisted =
-        jpaTm().transact(() -> jpaTm().getEntityManager().find(DurationTestEntity.class, "id"));
-    assertThat(persisted.duration.getMillis()).isEqualTo(testDuration.getMillis());
+    assertPersistedEntityHasSameDuration(testDuration);
   }
 
   @Test
   void testRoundTripLargeNumberOfDays() {
     Duration testDuration =
         Duration.standardDays(10001).plus(Duration.standardHours(100)).plus(Duration.millis(790));
-    DurationTestEntity entity = new DurationTestEntity(testDuration);
-    jpaTm().transact(() -> jpaTm().getEntityManager().persist(entity));
+    assertPersistedEntityHasSameDuration(testDuration);
+  }
+
+  @Test
+  void testRoundTripLessThanOneDay() {
+    Duration testDuration =
+        Duration.standardHours(15)
+            .plus(Duration.standardMinutes(40))
+            .plus(Duration.standardSeconds(50));
+    assertPersistedEntityHasSameDuration(testDuration);
+  }
+
+  @Test
+  void testRoundTripExactOneDay() {
+    Duration testDuration = Duration.standardDays(1);
+    assertPersistedEntityHasSameDuration(testDuration);
+  }
+
+  private void assertPersistedEntityHasSameDuration(Duration duration) {
+    DurationTestEntity entity = new DurationTestEntity(duration);
+    jpaTm().transact(() -> jpaTm().saveNew(entity));
     DurationTestEntity persisted =
         jpaTm().transact(() -> jpaTm().getEntityManager().find(DurationTestEntity.class, "id"));
-    assertThat(persisted.duration.getMillis()).isEqualTo(testDuration.getMillis());
+    assertThat(persisted.duration.getMillis()).isEqualTo(duration.getMillis());
   }
 
   @Entity(name = "TestEntity") // Override entity name to avoid the nested class reference.