Schould BigInteger be always used for integral min/max

[kosty]

While this comment is absolutely true. In practice it mostly plays out when JsonNode tree was constructed manually and not through jackson's ObjectMapper

json-schema-validator/src/main/java/com/networknt/schema/MaximumValidator.java

Line 75 in 2449ee0

//node.isBigInteger is not trustable, the type BigInteger doesn't mean it is a big number.

Generally, given that isBigInteger could not be trusted, should the two cases be merged into one? Original split was based on thinking that ObjectMapper is always used to construct JsonNode tree and allowed to save some memory by not referencing additional BigInteger object. Since it is not longer the case, there is little benefit in keeping

case 2

json-schema-validator/src/main/java/com/networknt/schema/MaximumValidator.java

Line 89 in 2449ee0

} else {

separate from

case 1

json-schema-validator/src/main/java/com/networknt/schema/MaximumValidator.java

Line 67 in 2449ee0

} else if ( schemaNode.isLong() || schemaNode.isInt() ) {

And merging them will probably be more readable

[stevehu]

@BalloonWen @jiachen1120 I thought we have made some changes on this logic. Could you please comment on this? Thanks.

[BalloonWen]

While this comment is absolutely true. In practice it mostly plays out when JsonNode tree was constructed manually and not through jackson's ObjectMapper

json-schema-validator/src/main/java/com/networknt/schema/MaximumValidator.java

Line 75 in 2449ee0

//node.isBigInteger is not trustable, the type BigInteger doesn't mean it is a big number.

Generally, given that isBigInteger could not be trusted, should the two cases be merged into one? Original split was based on thinking that ObjectMapper is always used to construct JsonNode tree and allowed to save some memory by not referencing additional BigInteger object. Since it is not longer the case, there is little benefit in keeping

case 2

json-schema-validator/src/main/java/com/networknt/schema/MaximumValidator.java

Line 89 in 2449ee0

} else {

separate from

case 1

json-schema-validator/src/main/java/com/networknt/schema/MaximumValidator.java

Line 67 in 2449ee0

} else if ( schemaNode.isLong() || schemaNode.isInt() ) {

And merging them will probably be more readable

Hi @kosty , thank you for pointing this out, I found the same problem and made some changes recently,
we need to satisfy another case: the node type is an integer but the maximum in the schema is a float number, related issue: #160
right now the logic will be:

• only if the node type and the maximum in the schema are both integer/long type:
  • check if the node type is bigInteger, in the case that the node value may be bigger than the max value of long. (compare by BigInteger)
  • if not bigInteger, then compare by long value. ( as you said to save some memory by not referencing additional BigInteger object)
• if any of the node type and the maximum in the schema is a float type:
  • compare using BigDecimal (this should not be a big use case, using BigDecimal will reduce many defects such as the overflow of a Double value, the edge of the max of a double, compare with "Infinity" etc.). Also, I did a performance test, there are no obvious differences, I think the most time-consuming part is the node constructing instead of comparing.

I should have asked you for reviewing #164 since you did a lot of research and work on those parts. Please advise. 😄

[kosty]

Glad there is a resolution on this!

[kosty]

@BalloonWen was playing with that part of code a bit and decided to dive a bit into performance tradeoffs. see comments in #174

Hopefully you'll find those useful