fix: Added Non-Finite check #777
Conversation
|
@dubeyaditya29 Good start, but more work is needed. Please revert the change in pom.xml: Are there any other numeric types that should be checked? |
| @@ -2629,8 +2632,34 @@ public static Object wrap(Object object) { | |||
| return wrap(object, null); | |||
| } | |||
|
|
|||
| // Custom isFiniteNumber method | |||
| public static boolean isFiniteNumber(Object value) { | |||
There was a problem hiding this comment.
This method is sub-optimal, and redundant of testValidity and numberIsFinite methods that are already available. Please use the existing function(s)
pom.xml
Outdated
| @@ -95,8 +95,8 @@ | |||
| <artifactId>maven-compiler-plugin</artifactId> | |||
| <version>3.11.0</version> | |||
| <configuration> | |||
| <source>1.8</source> | |||
There was a problem hiding this comment.
As stated already, this needs to be reverted. Java 1.8 is our current target.
There was a problem hiding this comment.
I'll made the necessary changes here thanks.
| private static Object wrap(Object object, Set<Object> objectsRecord) { | ||
| try { | ||
| // Calling the isFiniteNumber(Object object) -> to check if the object is Non-finite. | ||
| if(!isFiniteNumber(object)){ |
There was a problem hiding this comment.
this check should happen after the NULL check. Also, you can change this to just use testValidity
| if (value instanceof Integer) { | ||
| doubleValue = ((Integer) value).doubleValue(); |
There was a problem hiding this comment.
Integers are always finite.
There was a problem hiding this comment.
agreed. Using the existing testValidity method would be the correct course and already covers what this PR is attempting.
There was a problem hiding this comment.
agree @johnjaylward the testValidity can alone verify this, am somehow new to this code base, can you help me understand how did you found this method.
|
hey @stleary Thankyou for reviewing this.
|
|
thanks to @johnjaylward suggestions I think there is no need for defining a new method here we already have |
| private static Object wrap(Object object, Set<Object> objectsRecord) { | ||
| try { | ||
| // Calling the isFiniteNumber(Object object) -> to check if the object is Non-finite. | ||
| if(!isFiniteNumber(object)){ | ||
| throw new RuntimeException("Non-Finite Number encountered"); |
There was a problem hiding this comment.
Throw JSONException, not a generic RuntimeException.
|
#779 is a better implementation. This should be rejected |
|
Thanks, but not accepting this PR. |
|
No problem @stleary thanks for reviewing. |
|
@dubeyaditya29 if you want to work on a different issue, I recommend #748, which I just reopened. Let me know if you have any questions. |
This PR fixes the missing check for non-finite values being added to the map object in JSONObject
Added a new method
isFiniteNumber(Object value)returns a boolean value false in case the value is non-finite.The wrap throws a runtime exception in case the value is non-finite.