[LANG-1592] Correct implementation of RandomUtils.nextLong(long, long)

src/changes/changes.xml

@@ -48,6 +48,7 @@ The <action> type attribute can be add,update,fix,remove.
   <release version="3.12" date="2020-MM-DD" description="New features and bug fixes.">
     <action issue="LANG-1591" type="update" dev="kinow" due-to="bhawna94">Remove redundant argument from substring call.</action>
     <action                   type="update" dev="ggregory" due-to="Enable Dependabot #587">Enable Dependabot #587.</action>
+    <action issue="LANG-1592" type="fix" dev="aherbert" due-to="Huang Pingcai, Alex Herbert">Correct implementation of RandomUtils.nextLong(long, long)</action>
   </release>
   <release version="3.11" date="2020-07-12" description="New features and bug fixes.">
     <action                   type="update" dev="chtompki" due-to="Jin Xu">Refine test output for FastDateParserTest</action>

src/main/java/org/apache/commons/lang3/RandomUtils.java

@@ -145,7 +145,7 @@ public static long nextLong(final long startInclusive, final long endExclusive)
             return startInclusive;
         }
 
-        return (long) nextDouble(startInclusive, endExclusive);
+        return startInclusive + nextLong(endExclusive - startInclusive);
     }
 
     /**
@@ -156,7 +156,27 @@ public static long nextLong(final long startInclusive, final long endExclusive)
      * @since 3.5
      */
     public static long nextLong() {
-        return nextLong(0, Long.MAX_VALUE);
+        return nextLong(Long.MAX_VALUE);
+    }
+
+    /**
+     * Generates a {@code long} value between 0 (inclusive) and the specified
+     * value (exclusive).
+     *
+     * @param n Bound on the random number to be returned.  Must be positive.
+     * @return a random {@code long} value between 0 (inclusive) and {@code n}
+     * (exclusive).
+     */
+    private static long nextLong(long n) {
+        // Extracted from o.a.c.rng.core.BaseProvider.nextLong(long)
+        long bits;
+        long val;
+        do {
+            bits = RANDOM.nextLong() >>> 1;
+            val  = bits % n;
+        } while (bits - val + (n - 1) < 0);
+
+        return val;
     }
 
     /**

src/test/java/org/apache/commons/lang3/RandomUtilsTest.java

@@ -19,6 +19,7 @@
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -262,4 +263,28 @@ public void testExtremeRangeDouble() {
         final double result = RandomUtils.nextDouble(0, Double.MAX_VALUE);
         assertTrue(result >= 0 && result <= Double.MAX_VALUE);
     }
+
+    /**
+     * Test a large value for long. A previous implementation using
+     * {@link RandomUtils#nextDouble(double, double)} could generate a value equal
+     * to the upper limit.
+     *
+     * <pre>
+     * return (long) nextDouble(startInclusive, endExclusive);
+     * </pre>
+     *
+     * <p>See LANG-1592.</p>
+     */
+    @Test
+    public void testLargeValueRangeLong() {
+        final long startInclusive = 12900000000001L;
+        final long endExclusive = 12900000000016L;
+        // Note: The method using 'return (long) nextDouble(startInclusive, endExclusive)'
+        // takes thousands of calls to generate an error. This size loop fails most
+        // of the time with the previous method.
+        final int n = (int) (endExclusive - startInclusive) * 1000;
+        for (int i = 0; i < n; i++) {
+            assertNotEquals(endExclusive, RandomUtils.nextLong(startInclusive, endExclusive));
+        }
+    }
 }