Stricter validation for min/max values for whole numbers (#26137)

core/src/main/java/org/elasticsearch/common/Numbers.java

@@ -29,6 +29,9 @@ import java.math.BigInteger;
  */
 public final class Numbers {
 
+    private static final BigInteger MAX_LONG_VALUE = BigInteger.valueOf(Long.MAX_VALUE);
+    private static final BigInteger MIN_LONG_VALUE = BigInteger.valueOf(Long.MIN_VALUE);
+
     private Numbers() {
 
     }
@@ -205,6 +208,33 @@ public final class Numbers {
         }
     }
 
+    /** Return the long that {@code stringValue} stores or throws an exception if the
+     *  stored value cannot be converted to a long that stores the exact same
+     *  value and {@code coerce} is false. */
+    public static long toLong(String stringValue, boolean coerce) {
+        try {
+            return Long.parseLong(stringValue);
+        } catch (NumberFormatException e) {
+            // we will try again with BigDecimal
+        }
+
+        final BigInteger bigIntegerValue;
+        try {
+            BigDecimal bigDecimalValue = new BigDecimal(stringValue);
+            bigIntegerValue = coerce ? bigDecimalValue.toBigInteger() : bigDecimalValue.toBigIntegerExact();
+        } catch (ArithmeticException e) {
+            throw new IllegalArgumentException("Value [" + stringValue + "] has a decimal part");
+        } catch (NumberFormatException e) {
+            throw new IllegalArgumentException("For input string: \"" + stringValue + "\"");
+        }
+
+        if (bigIntegerValue.compareTo(MAX_LONG_VALUE) > 0 || bigIntegerValue.compareTo(MIN_LONG_VALUE) < 0) {
+            throw new IllegalArgumentException("Value [" + stringValue + "] is out of range for a long");
+        }
+
+        return bigIntegerValue.longValue();
+    }
+
     /** Return the int that {@code n} stores, or throws an exception if the
      *  stored value cannot be converted to an int that stores the exact same
      *  value. */

core/src/main/java/org/elasticsearch/common/xcontent/support/AbstractXContentParser.java

@@ -22,6 +22,7 @@ package org.elasticsearch.common.xcontent.support;
 import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.Booleans;
+import org.elasticsearch.common.Numbers;
 import org.elasticsearch.common.xcontent.NamedXContentRegistry;
 import org.elasticsearch.common.xcontent.XContentParser;
 
@@ -133,7 +134,14 @@ public abstract class AbstractXContentParser implements XContentParser {
         Token token = currentToken();
         if (token == Token.VALUE_STRING) {
             checkCoerceString(coerce, Short.class);
-            return (short) Double.parseDouble(text());
+
+            double doubleValue = Double.parseDouble(text());
+
+            if (doubleValue < Short.MIN_VALUE || doubleValue > Short.MAX_VALUE) {
+                throw new IllegalArgumentException("Value [" + text() + "] is out of range for a short");
+            }
+
+            return (short) doubleValue;
         }
         short result = doShortValue();
         ensureNumberConversion(coerce, result, Short.class);
@@ -152,7 +160,13 @@ public abstract class AbstractXContentParser implements XContentParser {
         Token token = currentToken();
         if (token == Token.VALUE_STRING) {
             checkCoerceString(coerce, Integer.class);
-            return (int) Double.parseDouble(text());
+            double doubleValue = Double.parseDouble(text());
+
+            if (doubleValue < Integer.MIN_VALUE || doubleValue > Integer.MAX_VALUE) {
+                throw new IllegalArgumentException("Value [" + text() + "] is out of range for an integer");
+            }
+
+            return (int) doubleValue;
         }
         int result = doIntValue();
         ensureNumberConversion(coerce, result, Integer.class);
@@ -171,13 +185,7 @@ public abstract class AbstractXContentParser implements XContentParser {
         Token token = currentToken();
         if (token == Token.VALUE_STRING) {
             checkCoerceString(coerce, Long.class);
-            // longs need special handling so we don't lose precision while parsing
-            String stringValue = text();
-            try {
-                return Long.parseLong(stringValue);
-            } catch (NumberFormatException e) {
-                return (long) Double.parseDouble(stringValue);
-            }
+            return Numbers.toLong(text(), coerce);
         }
         long result = doLongValue();
         ensureNumberConversion(coerce, result, Long.class);

core/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java

@@ -36,6 +36,7 @@ import org.apache.lucene.search.Query;
 import org.apache.lucene.util.BytesRef;
 import org.apache.lucene.util.NumericUtils;
 import org.elasticsearch.common.Explicit;
+import org.elasticsearch.common.Numbers;
 import org.elasticsearch.common.lucene.search.Queries;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Setting.Property;
@@ -43,6 +44,7 @@ import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentParser.Token;
+import org.elasticsearch.common.xcontent.support.AbstractXContentParser;
 import org.elasticsearch.index.fielddata.IndexFieldData;
 import org.elasticsearch.index.fielddata.IndexNumericFieldData.NumericType;
 import org.elasticsearch.index.fielddata.plain.DocValuesIndexFieldData;
@@ -644,8 +646,13 @@ public class NumberFieldMapper extends FieldMapper {
         LONG("long", NumericType.LONG) {
             @Override
             Long parse(Object value, boolean coerce) {
-                double doubleValue = objectToDouble(value);
+                if (value instanceof Long) {
+                    return (Long)value;
+                }
 
+                double doubleValue = objectToDouble(value);
+                // this check does not guarantee that value is inside MIN_VALUE/MAX_VALUE because values up to 9223372036854776832 will
+                // be equal to Long.MAX_VALUE after conversion to double. More checks ahead.
                 if (doubleValue < Long.MIN_VALUE || doubleValue > Long.MAX_VALUE) {
                     throw new IllegalArgumentException("Value [" + value + "] is out of range for a long");
                 }
@@ -653,18 +660,9 @@ public class NumberFieldMapper extends FieldMapper {
                     throw new IllegalArgumentException("Value [" + value + "] has a decimal part");
                 }
 
-                if (value instanceof Number) {
-                    return ((Number) value).longValue();
-                }
-
                 // longs need special handling so we don't lose precision while parsing
                 String stringValue = (value instanceof BytesRef) ? ((BytesRef) value).utf8ToString() : value.toString();
-
-                try {
-                    return Long.parseLong(stringValue);
-                } catch (NumberFormatException e) {
-                    return (long) Double.parseDouble(stringValue);
-                }
+                return Numbers.toLong(stringValue, coerce);
             }
 
             @Override
@@ -836,7 +834,6 @@ public class NumberFieldMapper extends FieldMapper {
 
             return doubleValue;
         }
-
     }
 
     public static final class NumberFieldType extends MappedFieldType {

core/src/test/java/org/elasticsearch/common/NumbersTests.java

@@ -27,6 +27,21 @@ import java.util.concurrent.atomic.AtomicInteger;
 
 public class NumbersTests extends ESTestCase {
 
+    public void testToLong() {
+        assertEquals(3L, Numbers.toLong("3", false));
+        assertEquals(3L, Numbers.toLong("3.1", true));
+        assertEquals(9223372036854775807L, Numbers.toLong("9223372036854775807.00", false));
+        assertEquals(-9223372036854775808L, Numbers.toLong("-9223372036854775808.00", false));
+
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
+            () -> Numbers.toLong("9223372036854775808", false));
+        assertEquals("Value [9223372036854775808] is out of range for a long", e.getMessage());
+
+        e = expectThrows(IllegalArgumentException.class,
+            () -> Numbers.toLong("-9223372036854775809", false));
+        assertEquals("Value [-9223372036854775809] is out of range for a long", e.getMessage());
+    }
+
     public void testToLongExact() {
         assertEquals(3L, Numbers.toLongExact(Long.valueOf(3L)));
         assertEquals(3L, Numbers.toLongExact(Integer.valueOf(3)));

core/src/test/java/org/elasticsearch/index/mapper/NumberFieldMapperTests.java

@@ -28,7 +28,9 @@ import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.index.mapper.NumberFieldMapper.NumberType;
 import org.elasticsearch.index.mapper.NumberFieldTypeTests.OutOfRangeSpec;
 
+import java.io.ByteArrayInputStream;
 import java.io.IOException;
+import java.math.BigInteger;
 import java.util.List;
 import java.util.Arrays;
 import java.util.HashSet;
@@ -234,6 +236,7 @@ public class NumberFieldMapperTests extends AbstractNumericFieldMapperTestCase {
                 .bytes(),
                 XContentType.JSON));
         MapperParsingException e = expectThrows(MapperParsingException.class, runnable);
+
         assertThat(e.getCause().getMessage(), containsString("For input string: \"a\""));
 
         mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
@@ -345,6 +348,26 @@ public class NumberFieldMapperTests extends AbstractNumericFieldMapperTestCase {
 
     public void testOutOfRangeValues() throws IOException {
         final List<OutOfRangeSpec<Object>> inputs = Arrays.asList(
+            OutOfRangeSpec.of(NumberType.BYTE, "128", "is out of range for a byte"),
+            OutOfRangeSpec.of(NumberType.SHORT, "32768", "is out of range for a short"),
+            OutOfRangeSpec.of(NumberType.INTEGER, "2147483648", "is out of range for an integer"),
+            OutOfRangeSpec.of(NumberType.LONG, "9223372036854775808", "out of range for a long"),
+
+            OutOfRangeSpec.of(NumberType.BYTE, "-129", "is out of range for a byte"),
+            OutOfRangeSpec.of(NumberType.SHORT, "-32769", "is out of range for a short"),
+            OutOfRangeSpec.of(NumberType.INTEGER, "-2147483649", "is out of range for an integer"),
+            OutOfRangeSpec.of(NumberType.LONG, "-9223372036854775809", "out of range for a long"),
+
+            OutOfRangeSpec.of(NumberType.BYTE, 128, "is out of range for a byte"),
+            OutOfRangeSpec.of(NumberType.SHORT, 32768, "out of range of Java short"),
+            OutOfRangeSpec.of(NumberType.INTEGER, 2147483648L, " out of range of int"),
+            OutOfRangeSpec.of(NumberType.LONG, new BigInteger("9223372036854775808"), "out of range of long"),
+
+            OutOfRangeSpec.of(NumberType.BYTE, -129, "is out of range for a byte"),
+            OutOfRangeSpec.of(NumberType.SHORT, -32769, "out of range of Java short"),
+            OutOfRangeSpec.of(NumberType.INTEGER, -2147483649L, " out of range of int"),
+            OutOfRangeSpec.of(NumberType.LONG, new BigInteger("-9223372036854775809"), "out of range of long"),
+
             OutOfRangeSpec.of(NumberType.HALF_FLOAT, "65520", "[half_float] supports only finite values"),
             OutOfRangeSpec.of(NumberType.FLOAT, "3.4028235E39", "[float] supports only finite values"),
             OutOfRangeSpec.of(NumberType.DOUBLE, "1.7976931348623157E309", "[double] supports only finite values"),
@@ -398,6 +421,13 @@ public class NumberFieldMapperTests extends AbstractNumericFieldMapperTestCase {
     }
 
     private BytesReference createIndexRequest(Object value) throws IOException {
-        return XContentFactory.jsonBuilder().startObject().field("field", value).endObject().bytes();
+        if (value instanceof BigInteger) {
+            return XContentFactory.jsonBuilder()
+                .startObject()
+                    .rawField("field", new ByteArrayInputStream(value.toString().getBytes("UTF-8")), XContentType.JSON)
+                .endObject().bytes();
+        } else {
+            return XContentFactory.jsonBuilder().startObject().field("field", value).endObject().bytes();
+        }
     }
 }

core/src/test/java/org/elasticsearch/index/mapper/NumberFieldTypeTests.java

@@ -46,9 +46,12 @@ import org.junit.Before;
 
 import java.io.IOException;
 import java.math.BigDecimal;
+import java.math.BigInteger;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.function.Supplier;
 
 import static org.hamcrest.Matchers.containsString;
@@ -389,6 +392,22 @@ public class NumberFieldTypeTests extends FieldTypeTestCase {
 
     public void testParseOutOfRangeValues() throws IOException {
         final List<OutOfRangeSpec<Object>> inputs = Arrays.asList(
+            OutOfRangeSpec.of(NumberType.BYTE, "128", "out of range for a byte"),
+            OutOfRangeSpec.of(NumberType.BYTE, 128, "is out of range for a byte"),
+            OutOfRangeSpec.of(NumberType.BYTE, -129, "is out of range for a byte"),
+
+            OutOfRangeSpec.of(NumberType.SHORT, "32768", "out of range for a short"),
+            OutOfRangeSpec.of(NumberType.SHORT, 32768, "is out of range for a short"),
+            OutOfRangeSpec.of(NumberType.SHORT, -32769, "is out of range for a short"),
+
+            OutOfRangeSpec.of(NumberType.INTEGER, "2147483648", "out of range for an integer"),
+            OutOfRangeSpec.of(NumberType.INTEGER, 2147483648L, "is out of range for an integer"),
+            OutOfRangeSpec.of(NumberType.INTEGER, -2147483649L, "is out of range for an integer"),
+
+            OutOfRangeSpec.of(NumberType.LONG, "9223372036854775808", "out of range for a long"),
+            OutOfRangeSpec.of(NumberType.LONG, new BigInteger("9223372036854775808"), " is out of range for a long"),
+            OutOfRangeSpec.of(NumberType.LONG, new BigInteger("-9223372036854775809"), " is out of range for a long"),
+
             OutOfRangeSpec.of(NumberType.HALF_FLOAT, "65520", "[half_float] supports only finite values"),
             OutOfRangeSpec.of(NumberType.FLOAT, "3.4028235E39", "[float] supports only finite values"),
             OutOfRangeSpec.of(NumberType.DOUBLE, "1.7976931348623157E309", "[double] supports only finite values"),

modules/reindex/src/test/java/org/elasticsearch/index/reindex/ReindexFailureTests.java

@@ -63,7 +63,7 @@ public class ReindexFailureTests extends ReindexTestCase {
                 .batches(1)
                 .failures(both(greaterThan(0)).and(lessThanOrEqualTo(maximumNumberOfShards()))));
         for (Failure failure: response.getBulkFailures()) {
-            assertThat(failure.getMessage(), containsString("NumberFormatException[For input string: \"words words\"]"));
+            assertThat(failure.getMessage(), containsString("IllegalArgumentException[For input string: \"words words\"]"));
         }
     }