Stricter validation for min/max values for whole numbers#26137
Stricter validation for min/max values for whole numbers#26137jpountz merged 29 commits intoelastic:masterfrom
Conversation
…_out_of_range_validation
…_out_of_range_validation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
I'm wondering whether the fix should be upstream in jackson? |
|
@jpountz There are 2 overloaded NumberType.parse methods. I agree that fix for |
|
Maybe I'm missing something but the latter seems to already perform bound checks? |
|
@jpountz Incoming value in |
|
I am good with improving validation in that case. |
|
I'll update PR. I think same validation for long should be added to |
|
@jpountz PR updated. I looked at Jackson's parser.getLongValue() - it validates min/max values correctly. AbstractXContentParser was converting double to short/int/long without proper validation. |
jpountz
left a comment
There was a problem hiding this comment.
I left some questions/comments but I think this is going in the right direction.
| throw new IllegalArgumentException("Value [" + text() + "] is out of range for a short"); | ||
| } | ||
|
|
||
| return (short) doubleValue; |
There was a problem hiding this comment.
would it work if we only had checkCoerceString within this block? From what I read, doShortValue already has the ability to parse strings.
There was a problem hiding this comment.
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());
}| return result; | ||
| } | ||
|
|
||
| public static long preciseLongValue(String stringValue, boolean coerce) { |
There was a problem hiding this comment.
if you want to use it from different classes, please move it to the o.e.common.Numbers class
|
@jpountz PR updated. Also, when #25861 will be solved, we can futher simplify Long parse(Object value, boolean coerce) {
if (value instanceof Number) {
return Numbers.toExactLong(value);
}
String stringValue = (value instanceof BytesRef) ? ((BytesRef) value).utf8ToString() : value.toString();
return Numbers.toExactLong(stringValue);
} |
jpountz
left a comment
There was a problem hiding this comment.
Thanks, it looks good to me.
|
@elasticmachine please test it |
|
@jpountz PR updated |
|
@elasticmachine please test it |
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 elastic#26137 Closes elastic#40323
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
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
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
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
Currently we can overcame min/max validation by passing number as string or directly from plugin so its value will be rounded. Example:
@colings86 @jpountz Please take a look.