Better exception message on Duration overflow

neo4j · Sep 5, 2018 · 48956ec · 48956ec
1 parent 745e0af
commit 48956ec
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 48 deletions.
diff --git a/community/values/src/main/java/org/neo4j/values/storable/DurationValue.java b/community/values/src/main/java/org/neo4j/values/storable/DurationValue.java
@@ -180,7 +180,7 @@ public abstract static class Compiler<Input> extends DurationBuilder<Input,Metho
     private static final List<TemporalUnit> UNITS = unmodifiableList( asList( MONTHS, DAYS, SECONDS, NANOS ) );
     // This comparator is safe until 292,271,023,045 years. After that, we have an overflow.
     private static final Comparator<DurationValue> COMPARATOR =
-            Comparator.comparingLong( DurationValue::averageLengthInSeconds )
+            Comparator.comparingLong( DurationValue::getAverageLengthInSeconds )
                     // nanos are guaranteed to be smaller than NANOS_PER_SECOND
                     .thenComparingLong( d -> d.nanos )
                     // At this point, the durations have the same length and we compare by the individual fields.
@@ -200,7 +200,8 @@ private static DurationValue newDuration( long months, long days, long seconds,
 
     private DurationValue( long months, long days, long seconds, long nanos )
     {
-        seconds = safeAdd( seconds, nanos / NANOS_PER_SECOND );
+        assertNoOverflow( months, days, seconds, nanos );
+        seconds = secondsWithNanos( seconds, nanos );
         nanos %= NANOS_PER_SECOND;
         // normalize nanos to be between 0 and NANOS_PER_SECOND-1
         if ( nanos < 0 )
@@ -212,7 +213,6 @@ private DurationValue( long months, long days, long seconds, long nanos )
         this.days = days;
         this.seconds = seconds;
         this.nanos = (int) nanos;
-        assertNoOverflow();
     }
 
     @Override
@@ -227,42 +227,38 @@ int unsafeCompareTo( Value otherValue )
         return compareTo( (DurationValue) otherValue );
     }
 
-    private long averageLengthInSeconds()
+    private long getAverageLengthInSeconds()
     {
-        long daysInSeconds = safeMultiply( days, SECONDS_PER_DAY );
+        return calcAverageLengthInSeconds( this.months, this.days, this.seconds );
-        long monthsInSeconds = safeMultiply( months, AVG_SECONDS_PER_MONTH );
-        return safeAdd( seconds, safeAdd( daysInSeconds, monthsInSeconds ) );
     }
 
-    private static long safeAdd( long l1, long l2 )
+    private long calcAverageLengthInSeconds( long months, long days, long seconds )
     {
-        try
+        long daysInSeconds = Math.multiplyExact( days, SECONDS_PER_DAY );
-        {
+        long monthsInSeconds = Math.multiplyExact( months, AVG_SECONDS_PER_MONTH );
-            return Math.addExact( l1, l2 );
+        return Math.addExact( seconds, Math.addExact( daysInSeconds, monthsInSeconds ) );
-        }
-        catch ( ArithmeticException e )
-        {
-            throw new InvalidValuesArgumentException( "Invalid value for duration", e );
-        }
     }
 
-    private static long safeMultiply( long l1, long l2 )
+    private long secondsWithNanos( long seconds, long nanos )
+    {
+        return Math.addExact( seconds, nanos / NANOS_PER_SECOND );
+    }
+
+    private void assertNoOverflow( long months, long days, long seconds, long nanos )
     {
         try
         {
-            return Math.multiplyExact( l1, l2 );
+            calcAverageLengthInSeconds( months, days, seconds );
+            secondsWithNanos( seconds, nanos );
         }
         catch ( ArithmeticException e )
         {
-            throw new InvalidValuesArgumentException( "Invalid value for duration", e );
+            throw new InvalidValuesArgumentException(
+                    String.format( "Invalid value for duration, will cause overflow. Value was months=%d, days=%d, seconds=%d, nanos=%d",
+                            months, days, seconds, nanos ), e );
         }
     }
 
-    private void assertNoOverflow()
-    {
-        averageLengthInSeconds();
-    }
-
     long nanosOfDay()
     {
         return (seconds % SECONDS_PER_DAY) * NANOS_PER_SECOND + nanos;

diff --git a/community/values/src/test/java/org/neo4j/values/storable/DurationValueTest.java b/community/values/src/test/java/org/neo4j/values/storable/DurationValueTest.java
@@ -572,48 +572,45 @@ public void shouldNotThrowWhenInsideOverflowLimit()
     public void shouldThrowOnOverflowOnNanos()
     {
         // when
-        try
+        int nanos = 1_000_000_000;
-        {
+        long seconds = Long.MAX_VALUE;
-            DurationValue duration = duration( 0, 0, Long.MAX_VALUE, 1_000_000_000 );
+        assertConstructorThrows( 0, 0, seconds, nanos );
-            fail( "Should have failed" );
-        }
-        catch ( InvalidValuesArgumentException e )
-        {
-            assertThat( e.getMessage(), Matchers.containsString( "Invalid value for duration" ) );
-        }
     }
 
     @Test
     public void shouldThrowOnOverflowOnDays()
     {
         // when
-        try
+        long days = Long.MAX_VALUE / TemporalUtil.SECONDS_PER_DAY;
-        {
+        long seconds = Long.MAX_VALUE - days * TemporalUtil.SECONDS_PER_DAY;
-            long days = Long.MAX_VALUE / TemporalUtil.SECONDS_PER_DAY;
+        assertConstructorThrows( 0, days, seconds + 1, 0 );
-            long seconds = Long.MAX_VALUE - days * TemporalUtil.SECONDS_PER_DAY;
-            DurationValue duration = duration( 0, days, seconds + 1, 0 );
-            fail( "Should have failed" );
-        }
-        catch ( InvalidValuesArgumentException e )
-        {
-            assertThat( e.getMessage(), Matchers.containsString( "Invalid value for duration" ) );
-        }
     }
 
     @Test
     public void shouldThrowOnOverflowOnMonths()
     {
         // when
+        long months = Long.MAX_VALUE / TemporalUtil.AVG_SECONDS_PER_MONTH;
+        long seconds = Long.MAX_VALUE - months * TemporalUtil.AVG_SECONDS_PER_MONTH;
+        assertConstructorThrows( months, 0, seconds + 1, 0 );
+    }
+
+    private void assertConstructorThrows( long months, long days, long seconds, int nanos )
+    {
         try
         {
-            long months = Long.MAX_VALUE / TemporalUtil.AVG_SECONDS_PER_MONTH;
+            DurationValue duration = duration( months, days, seconds, nanos );
-            long seconds = Long.MAX_VALUE - months * TemporalUtil.AVG_SECONDS_PER_MONTH;
-            DurationValue duration = duration( months, 0, seconds + 1, 0 );
             fail( "Should have failed" );
         }
         catch ( InvalidValuesArgumentException e )
         {
-            assertThat( e.getMessage(), Matchers.containsString( "Invalid value for duration" ) );
+            assertThat( e.getMessage(), Matchers.allOf(
+                    Matchers.containsString( "Invalid value for duration" ),
+                    Matchers.containsString( "months=" + months ),
+                    Matchers.containsString( "days=" + days ),
+                    Matchers.containsString( "seconds=" + seconds ),
+                    Matchers.containsString( "nanos=" + nanos )
+            ) );
         }
     }
 }