Skip to content

Commit 1a3916a

Browse files
DaveCTurnerDaveCTurner
committed
Optimise rejection of out-of-range long values (#40325)
Today if you try and insert a very large number like `1e9999999` into a long field we first construct this number as a `BigDecimal`, convert this to a `BigInteger` and then reject it because it is out of range. Unfortunately making such a large `BigInteger` is rather expensive. We can avoid this expense by performing a (weaker) range check on the `BigDecimal` representation of incoming `long`s too. Relates #26137 Closes #40323
1 parent 6ef657c commit 1a3916a

File tree

4 files changed

+43
-9
lines changed

4 files changed

+43
-9
lines changed

‎libs/x-content/src/main/java/org/elasticsearch/common/xcontent/support/AbstractXContentParser.java

+13-3
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,12 @@ public int intValue(boolean coerce) throws IOException {
151151

152152
protected abstract int doIntValue() throws IOException;
153153

154+
private static BigInteger LONG_MAX_VALUE_AS_BIGINTEGER = BigInteger.valueOf(Long.MAX_VALUE);
155+
private static BigInteger LONG_MIN_VALUE_AS_BIGINTEGER = BigInteger.valueOf(Long.MIN_VALUE);
156+
// weak bounds on the BigDecimal representation to allow for coercion
157+
private static BigDecimal BIGDECIMAL_GREATER_THAN_LONG_MAX_VALUE = BigDecimal.valueOf(Long.MAX_VALUE).add(BigDecimal.ONE);
158+
private static BigDecimal BIGDECIMAL_LESS_THAN_LONG_MIN_VALUE = BigDecimal.valueOf(Long.MIN_VALUE).subtract(BigDecimal.ONE);
159+
154160
/** Return the long that {@code stringValue} stores or throws an exception if the
155161
* stored value cannot be converted to a long that stores the exact same
156162
* value and {@code coerce} is false. */
@@ -163,19 +169,23 @@ private static long toLong(String stringValue, boolean coerce) {
163169

164170
final BigInteger bigIntegerValue;
165171
try {
166-
BigDecimal bigDecimalValue = new BigDecimal(stringValue);
172+
final BigDecimal bigDecimalValue = new BigDecimal(stringValue);
173+
if (bigDecimalValue.compareTo(BIGDECIMAL_GREATER_THAN_LONG_MAX_VALUE) >= 0 ||
174+
bigDecimalValue.compareTo(BIGDECIMAL_LESS_THAN_LONG_MIN_VALUE) <= 0) {
175+
throw new IllegalArgumentException("Value [" + stringValue + "] is out of range for a long");
176+
}
167177
bigIntegerValue = coerce ? bigDecimalValue.toBigInteger() : bigDecimalValue.toBigIntegerExact();
168178
} catch (ArithmeticException e) {
169179
throw new IllegalArgumentException("Value [" + stringValue + "] has a decimal part");
170180
} catch (NumberFormatException e) {
171181
throw new IllegalArgumentException("For input string: \"" + stringValue + "\"");
172182
}
173183

174-
if (bigIntegerValue.compareTo(BigInteger.valueOf(Long.MAX_VALUE)) > 0 ||
175-
bigIntegerValue.compareTo(BigInteger.valueOf(Long.MIN_VALUE)) < 0) {
184+
if (bigIntegerValue.compareTo(LONG_MAX_VALUE_AS_BIGINTEGER) > 0 || bigIntegerValue.compareTo(LONG_MIN_VALUE_AS_BIGINTEGER) < 0) {
176185
throw new IllegalArgumentException("Value [" + stringValue + "] is out of range for a long");
177186
}
178187

188+
assert bigIntegerValue.longValueExact() <= Long.MAX_VALUE; // asserting that no ArithmeticException is thrown
179189
return bigIntegerValue.longValue();
180190
}
181191

‎server/src/main/java/org/elasticsearch/common/Numbers.java

+8
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,10 @@ public static long toLongExact(Number n) {
125125
}
126126
}
127127

128+
// weak bounds on the BigDecimal representation to allow for coercion
129+
private static BigDecimal BIGDECIMAL_GREATER_THAN_LONG_MAX_VALUE = BigDecimal.valueOf(Long.MAX_VALUE).add(BigDecimal.ONE);
130+
private static BigDecimal BIGDECIMAL_LESS_THAN_LONG_MIN_VALUE = BigDecimal.valueOf(Long.MIN_VALUE).subtract(BigDecimal.ONE);
131+
128132
/** Return the long that {@code stringValue} stores or throws an exception if the
129133
* stored value cannot be converted to a long that stores the exact same
130134
* value and {@code coerce} is false. */
@@ -138,6 +142,10 @@ public static long toLong(String stringValue, boolean coerce) {
138142
final BigInteger bigIntegerValue;
139143
try {
140144
BigDecimal bigDecimalValue = new BigDecimal(stringValue);
145+
if (bigDecimalValue.compareTo(BIGDECIMAL_GREATER_THAN_LONG_MAX_VALUE) >= 0 ||
146+
bigDecimalValue.compareTo(BIGDECIMAL_LESS_THAN_LONG_MIN_VALUE) <= 0) {
147+
throw new IllegalArgumentException("Value [" + stringValue + "] is out of range for a long");
148+
}
141149
bigIntegerValue = coerce ? bigDecimalValue.toBigInteger() : bigDecimalValue.toBigIntegerExact();
142150
} catch (ArithmeticException e) {
143151
throw new IllegalArgumentException("Value [" + stringValue + "] has a decimal part");

‎server/src/test/java/org/elasticsearch/common/NumbersTests.java

+14-6
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
package org.elasticsearch.common;
2121

22+
import com.carrotsearch.randomizedtesting.annotations.Timeout;
2223
import org.elasticsearch.test.ESTestCase;
2324

2425
import java.math.BigDecimal;
@@ -27,19 +28,26 @@
2728

2829
public class NumbersTests extends ESTestCase {
2930

31+
@Timeout(millis = 10000)
3032
public void testToLong() {
3133
assertEquals(3L, Numbers.toLong("3", false));
3234
assertEquals(3L, Numbers.toLong("3.1", true));
3335
assertEquals(9223372036854775807L, Numbers.toLong("9223372036854775807.00", false));
3436
assertEquals(-9223372036854775808L, Numbers.toLong("-9223372036854775808.00", false));
37+
assertEquals(9223372036854775807L, Numbers.toLong("9223372036854775807.00", true));
38+
assertEquals(-9223372036854775808L, Numbers.toLong("-9223372036854775808.00", true));
39+
assertEquals(9223372036854775807L, Numbers.toLong("9223372036854775807.99", true));
40+
assertEquals(-9223372036854775808L, Numbers.toLong("-9223372036854775808.99", true));
3541

36-
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
37-
() -> Numbers.toLong("9223372036854775808", false));
38-
assertEquals("Value [9223372036854775808] is out of range for a long", e.getMessage());
42+
assertEquals("Value [9223372036854775808] is out of range for a long", expectThrows(IllegalArgumentException.class,
43+
() -> Numbers.toLong("9223372036854775808", false)).getMessage());
44+
assertEquals("Value [-9223372036854775809] is out of range for a long", expectThrows(IllegalArgumentException.class,
45+
() -> Numbers.toLong("-9223372036854775809", false)).getMessage());
3946

40-
e = expectThrows(IllegalArgumentException.class,
41-
() -> Numbers.toLong("-9223372036854775809", false));
42-
assertEquals("Value [-9223372036854775809] is out of range for a long", e.getMessage());
47+
assertEquals("Value [1e99999999] is out of range for a long", expectThrows(IllegalArgumentException.class,
48+
() -> Numbers.toLong("1e99999999", false)).getMessage());
49+
assertEquals("Value [-1e99999999] is out of range for a long", expectThrows(IllegalArgumentException.class,
50+
() -> Numbers.toLong("-1e99999999", false)).getMessage());
4351
}
4452

4553
public void testToLongExact() {

‎server/src/test/java/org/elasticsearch/index/mapper/NumberFieldMapperTests.java

+8
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
package org.elasticsearch.index.mapper;
2121

22+
import com.carrotsearch.randomizedtesting.annotations.Timeout;
2223
import org.apache.lucene.index.DocValuesType;
2324
import org.apache.lucene.index.IndexableField;
2425
import org.elasticsearch.common.Strings;
@@ -367,17 +368,20 @@ public void testEmptyName() throws IOException {
367368
}
368369
}
369370

371+
@Timeout(millis = 30000)
370372
public void testOutOfRangeValues() throws IOException {
371373
final List<OutOfRangeSpec<Object>> inputs = Arrays.asList(
372374
OutOfRangeSpec.of(NumberType.BYTE, "128", "is out of range for a byte"),
373375
OutOfRangeSpec.of(NumberType.SHORT, "32768", "is out of range for a short"),
374376
OutOfRangeSpec.of(NumberType.INTEGER, "2147483648", "is out of range for an integer"),
375377
OutOfRangeSpec.of(NumberType.LONG, "9223372036854775808", "out of range for a long"),
378+
OutOfRangeSpec.of(NumberType.LONG, "1e999999999", "out of range for a long"),
376379

377380
OutOfRangeSpec.of(NumberType.BYTE, "-129", "is out of range for a byte"),
378381
OutOfRangeSpec.of(NumberType.SHORT, "-32769", "is out of range for a short"),
379382
OutOfRangeSpec.of(NumberType.INTEGER, "-2147483649", "is out of range for an integer"),
380383
OutOfRangeSpec.of(NumberType.LONG, "-9223372036854775809", "out of range for a long"),
384+
OutOfRangeSpec.of(NumberType.LONG, "-1e999999999", "out of range for a long"),
381385

382386
OutOfRangeSpec.of(NumberType.BYTE, 128, "is out of range for a byte"),
383387
OutOfRangeSpec.of(NumberType.SHORT, 32768, "out of range of Java short"),
@@ -419,6 +423,10 @@ public void testOutOfRangeValues() throws IOException {
419423
e.getCause().getMessage(), containsString(item.message));
420424
}
421425
}
426+
427+
// the following two strings are in-range for a long after coercion
428+
parseRequest(NumberType.LONG, createIndexRequest("9223372036854775807.9"));
429+
parseRequest(NumberType.LONG, createIndexRequest("-9223372036854775808.9"));
422430
}
423431

424432
private void parseRequest(NumberType type, BytesReference content) throws IOException {

0 commit comments

Comments
 (0)