Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 29 commits into from
Aug 21, 2017
Merged

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

Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
2d0e5c1
validate half float values
fred84 Jul 12, 2017
c0fff6f
Merge branch 'master' into 25534_reject_out_of_range_numbers
fred84 Jul 13, 2017
6e0a6ea
test upper bound for numeric mapper
fred84 Jul 17, 2017
7d4f315
merge master
fred84 Jul 21, 2017
d1ebd6f
test for upper bound for float, double and half_float
fred84 Jul 21, 2017
1358fed
Merge branch 'master' into 25533_reject_out_of_range_numbers
fred84 Jul 22, 2017
44983d8
more tests on NaN and Infinity for NumberFieldMapper
fred84 Jul 23, 2017
d072e69
Merge branch 'master' into 25534_reject_out_of_range_numbers
fred84 Jul 23, 2017
ecf3424
fix checkstyle errors
fred84 Jul 23, 2017
b5d231d
minor renaming
fred84 Jul 23, 2017
187982a
resolve merge conflict and cleanup NumberFieldMapper out of range tests
fred84 Jul 27, 2017
d236158
Merge remote-tracking branch 'upstream/master' into 25534_reject_out_…
fred84 Jul 27, 2017
7423c4d
comments for disabled test
fred84 Jul 27, 2017
bced91e
tests for out of range values for long
fred84 Jul 28, 2017
7e07a63
min/max validation for short and integer numbertypes
fred84 Jul 29, 2017
61d8585
Merge remote-tracking branch 'upstream/master' into number_field_type…
fred84 Jul 29, 2017
8e88c9d
Merge remote-tracking branch 'upstream/master' into 25534_reject_out_…
fred84 Jul 30, 2017
11ce7c8
tests for byte/short/integer/long removed and will be added in separa…
fred84 Jul 30, 2017
3902911
remove unused import
fred84 Jul 30, 2017
eba60b7
Merge remote-tracking branch 'upstream/master' into number_field_type…
fred84 Jul 30, 2017
4dd7a83
25534_reject_out_of_range_numbers merged
fred84 Jul 30, 2017
8901920
merge and resolve conflict
fred84 Aug 10, 2017
1a32b05
tests for negative values
fred84 Aug 10, 2017
efe2adf
min/max validation for short/int/long
fred84 Aug 12, 2017
0eb0c01
Merge remote-tracking branch 'upstream/master' into number_field_type…
fred84 Aug 12, 2017
d76e214
fix checkstyle errors & return more informative message for malformed…
fred84 Aug 12, 2017
376a934
Merge remote-tracking branch 'upstream/master' into number_field_type…
fred84 Aug 17, 2017
cfe9461
move toLong from AbstractXContentParser to Numbers
fred84 Aug 17, 2017
3d8521b
fix test
fred84 Aug 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions core/src/main/java/org/elasticsearch/common/xcontent/support/AbstractXContentParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import org.elasticsearch.common.xcontent.XContentParser;

import java.io.IOException;
import java.math.BigDecimal;
import java.math.BigInteger;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedHashMap;
Expand All @@ -42,6 +44,9 @@ public abstract class AbstractXContentParser implements XContentParser {
// and change any code that needs to apply an alternative policy.
public static final boolean DEFAULT_NUMBER_COERCE_POLICY = true;

private static final BigInteger MIN_LONG_VALUE = BigInteger.valueOf(Long.MIN_VALUE);
private static final BigInteger MAX_LONG_VALUE = BigInteger.valueOf(Long.MAX_VALUE);

private static void checkCoerceString(boolean coerce, Class<? extends Number> clazz) {
if (!coerce) {
//Need to throw type IllegalArgumentException as current catch logic in
Expand Down Expand Up @@ -133,7 +138,14 @@ public short shortValue(boolean coerce) throws IOException {
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it work if we only had checkCoerceString within this block? From what I read, doShortValue already has the ability to parse strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doShortValue calls parser.getShortValue() which throws exception on non-numeric json values. Even if it is number in wrapped in string:

    public void testGetShortValue() throws Exception {
        JsonFactory factory = new JsonFactory();

        JsonParser parser = factory.createParser("123");
        parser.nextToken();
        assertEquals(123, parser.getShortValue());

        final JsonParser parser2 = factory.createParser("\"123\"");
        parser2.nextToken();
        JsonParseException e = expectThrows(JsonParseException.class, () -> parser2.getShortValue());
        assertEquals("123", parser2.getValueAsString());
    }

}
short result = doShortValue();
ensureNumberConversion(coerce, result, Short.class);
Expand All @@ -152,7 +164,13 @@ public int intValue(boolean coerce) throws IOException {
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);
Expand All @@ -176,14 +194,31 @@ public long longValue(boolean coerce) throws IOException {
try {
return Long.parseLong(stringValue);
} catch (NumberFormatException e) {
return (long) Double.parseDouble(stringValue);
return preciseLongValue(stringValue, coerce);
}
}
long result = doLongValue();
ensureNumberConversion(coerce, result, Long.class);
return result;
}

public static long preciseLongValue(String stringValue, boolean coerce) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to use it from different classes, please move it to the o.e.common.Numbers class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

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();
}

protected abstract long doLongValue() throws IOException;

@Override
Expand Down
15 changes: 8 additions & 7 deletions core/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
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;
Expand Down Expand Up @@ -644,26 +645,27 @@ public List<Field> createFields(String name, Number value,
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");
}
if (!coerce && doubleValue % 1 != 0) {
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 AbstractXContentParser.preciseLongValue(stringValue, coerce);
}
}

Expand Down Expand Up @@ -836,7 +838,6 @@ private static double objectToDouble(Object value) {

return doubleValue;
}

}

public static final class NumberFieldType extends MappedFieldType {
Expand Down
32 changes: 31 additions & 1 deletion core/src/test/java/org/elasticsearch/index/mapper/NumberFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@
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;
Expand Down Expand Up @@ -234,6 +236,7 @@ private void doTestIgnoreMalformed(String type) throws IOException {
.bytes(),
XContentType.JSON));
MapperParsingException e = expectThrows(MapperParsingException.class, runnable);

assertThat(e.getCause().getMessage(), containsString("For input string: \"a\""));

mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
Expand Down Expand Up @@ -345,6 +348,26 @@ public void testEmptyName() throws IOException {

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"),
Expand Down Expand Up @@ -398,6 +421,13 @@ private DocumentMapper createDocumentMapper(NumberType type) throws IOException
}

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();
}
}
}
19 changes: 19 additions & 0 deletions core/src/test/java/org/elasticsearch/index/mapper/NumberFieldTypeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,12 @@

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;
Expand Down Expand Up @@ -389,6 +392,22 @@ public void doTestDocValueRangeQueries(NumberType type, Supplier<Number> valueSu

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"),
Expand Down