Stricter validation for min/max values for whole numbers

[fred84]

Currently we can overcame min/max validation by passing number as string or directly from plugin so its value will be rounded. Example:

curl -XPUT 'localhost:9200/twitter?pretty' -H 'Content-Type: application/json' -d'
{
  "mappings": {
    "tweet": {
      "properties": {
        "size": {
          "type": "long"
        }
      }
    }
  }
}
'

curl -XPUT 'localhost:9200/twitter/tweet/1' -H 'Content-Type: application/json' -d'{ "size":  9223372036854775806 }'
# long max value
curl -XPUT 'localhost:9200/twitter/tweet/2' -H 'Content-Type: application/json' -d'{ "size":  9223372036854775807 }'
# wrapped number > long max value
curl -XPUT 'localhost:9200/twitter/tweet/3' -H 'Content-Type: application/json' -d'{ "size":  "9223372036854775808" }'

curl -XGET 'localhost:9200/twitter/tweet/_search?pretty' -H 'Content-Type: application/json' -d'
{
    "query": {
        "match" : {
            "size" : 9223372036854775807
        }
    }
}
'

{
  #...
  "hits" : {
    "total" : 2,
    "max_score" : 1.0,
    "hits" : [
      {
        "_index" : "twitter",
        "_type" : "tweet",
        "_id" : "2",
        "_score" : 1.0,
        "_source" : {
          "size" : 9223372036854775807
        }
      },
      {
        "_index" : "twitter",
        "_type" : "tweet",
        "_id" : "3",
        "_score" : 1.0,
        "_source" : {
          "size" : "9223372036854775808"
        }
      }
    ]
  }
}

@colings86 @jpountz Please take a look.

[elasticmachine]

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?

[jpountz]

I'm wondering whether the fix should be upstream in jackson?

[fred84]

@jpountz There are 2 overloaded NumberType.parse methods. I agree that fix for parse(XContentParser parser, boolean coerce) may be in jackson, but for java parse(Object value, boolean coerce) I think it should be in elastic.

[jpountz]

Maybe I'm missing something but the latter seems to already perform bound checks?

[fred84]

@jpountz Incoming value in parse(Object value, boolean coerce) is converted to double before comparison against min/max value. In case of NumberType.LONG there is not enough precision for double near min/max value.
Here is example demonstrating this:
assertFalse(Double.parseDouble("9223372036854776832") > Long.MAX_VALUE);

[jpountz]

I am good with improving validation in that case.

[fred84]

I'll update PR. I think same validation for long should be added to AbstractXContentParser.public long longValue(boolean coerce) when handling string values.

[fred84]

@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]

I left some questions/comments but I think this is going in the right direction.

[jpountz]

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

[fred84]

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

[jpountz]

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

[fred84]

Ok

[fred84]

@jpountz PR updated. Also, when #25861 will be solved, we can futher simplify NumericType.LONG.parse to

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]

Thanks, it looks good to me.

[jpountz]

@elasticmachine please test it

[fred84]

@jpountz PR updated

[jpountz]

@elasticmachine please test it