Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optLong vs getLong inconsitencies #653

Closed
ejoncas-rivalbet opened this issue Dec 9, 2021 · 17 comments
Closed

optLong vs getLong inconsitencies #653

ejoncas-rivalbet opened this issue Dec 9, 2021 · 17 comments

Comments

@ejoncas-rivalbet
Copy link

ejoncas-rivalbet commented Dec 9, 2021

Hi there,

We found a problem today where we stopped using getLong(String) and started using optLong(String) as it doesn't throw exception.

The problem is that there are inconsistencies between the two methods in regards on how they treat "numbers as strings". I know this is not the intended case but our mobile-app was sending strings when we expected numbers and everything was working well with getLong and suddenly some strange scenarios broke.

Here's is a sample scenario:

 @Test
    public void testJSONProblem() {
        JSONObject json = new JSONObject("{\"number_1\":\"01234\", \"number_2\": \"332211\"}");

        assertThat(json.getLong("number_1"), is(1234L));
        assertThat(json.optLong("number_1"), is(1234L)); //THIS FAILS it results 0
        assertThat(json.getLong("number_2"), is(332211L));
        assertThat(json.optLong("number_2"), is(332211L));
    }

As you can see optLong and getLong almost behave the same for numbers as strings but the problem arises when the number starts with 0.

The root of this problem is on how getLong parse numbers:
https://github.com/stleary/JSON-java/blob/master/src/main/java/org/json/JSONObject.java#L807
(it uses instanceof and Number#longValue()) where as optLong ends up calling this function

protected static Number stringToNumber(final String val) throws NumberFormatException {

@stleary
Copy link
Owner

stleary commented Dec 10, 2021

Thanks, @ejoncas-rivalbet for bringing up this issue, investigating.

@johnjaylward
Copy link
Contributor

@ejoncas-rivalbet , just to be sure I understand you correctly, you believe that optLong is correct, while getLong is wrong, correct?

To be honest, I'm a little surprised that your first test passes. I would have expected the parser to process that as octal, which would be 668 in decimal.

@johnjaylward
Copy link
Contributor

hmm, I'm reading the javadocs on the parsers, and I'm not seeing a reference to the "leading 0 means octal" as noted in our comments. From what I can tell, all the string -> number parsers seem to process as decimal unless using the overload with the radix.

@stleary , if we don't have it already, we should include tests which process various number formats (hex, scientific/exponent, floating point hex, octal, standard decimal) and make sure we have coverage.

@johnjaylward
Copy link
Contributor

The JSON spec does not allow leading zeros on a number:
https://www.ietf.org/rfc/rfc4627.html#section-2.4

so I'm going with optLong being "more correct" than getLong

@ejoncas-rivalbet
Copy link
Author

ejoncas-rivalbet commented Dec 12, 2021

@johnjaylward getLong is correct for me. We could argue wether this library is supposed to parse a string to a number when the developer is explicitly asking for a Long on a field that is type string.

But since it does it, I believe getLong does it well where as optLong just fails. As you can see optLong works in "some cases" where as getLong works in all cases.

The JSON spect does not allow leading zeros on numbers but given this is a string everything is possible.

Fixing optLong to behave the same way as getLong is a small change that will simply fix anyone running into this issue. If you change getLong to return 0 when anyone tries to parse a number as a string it's a massive change that will break possibly lots of integrations when upgrading to newer versions of this library.

On the project where I run onto this issue. I simply came back to use getLong surrounded by a try/catch.

@stleary
Copy link
Owner

stleary commented Dec 21, 2021

I think there are reasonable arguments for both sides. I don't think either JSON spec covers accessor functions, so this implementation is free to choose how to support get and opt functions. optLong() used to call getLong() in a try block, but recently it was changed to call stringToNumber() instead. Other numeric opt*() functions may have similar changed behavior.
Example of original behavior:
optLlong()

@stleary
Copy link
Owner

stleary commented Dec 31, 2021

This issue is available for someone to work on. Whatever solution is selected, optLong() and getlong() should parse input in the same way. Other opt*() and get*() methods should be checked and fixed as needed.

@abimarank
Copy link

@stleary I would like to provide the fix.

Was there any reason to change from calling getLong() to stringToNumber()?

@johnjaylward
Copy link
Contributor

stringToNumber choice was to make number parsing more consistent across different number types (int, long, float, double, etc).

Rereading the discussion here, I'm leaning towards modifying the stringToNumber implementation to properly parse numbers with leading 0's (trim the 0's off).

It also looks like stringToNumber may not handle numbers that looks like .1234. Seems that it expects numbers to start with either a digit or a - sign. We may want to expand the parse to support these too.

Possible values to use for new test cases:

  • "0987" -> 987 test both get/opt int, long, BigInteger, BigDecimal
  • "987" -> 987 test both get/opt int, long, BigInteger, BigDecimal
  • "0123" -> 123 test both get/opt int, long, BigInteger, BigDecimal
  • "123" -> 123 test both get/opt int, long, BigInteger, BigDecimal
  • ".123" -> 0.123 test both get/opt float, double, BigDecimal
  • "0.123" -> 0.123 test both get/opt float, double, BigDecimal

@johnjaylward
Copy link
Contributor

also to note, for android compatibility, the code changes for "stringToNumber" will need to be copied to the XML class

@stleary
Copy link
Owner

stleary commented Sep 30, 2023

@ejoncas-rivalbet Is this still a problem area for you? If not, I think it might be difficult to find a fix that doesn't break existing applications (see #661, which was eventually closed).

@rudrajyotib
Copy link
Contributor

I have identified the cause and fix for this. Can this issue be marked as Hacktoberfest please.
@johnjaylward @abimarank @ejoncas-rivalbet @stleary @bpieber

@stleary
Copy link
Owner

stleary commented Oct 4, 2023

Done.

rudrajyotib added a commit to rudrajyotib/JSON-java that referenced this issue Oct 6, 2023
For exponential decimal conversion, number is not touched.
Leading zeros removed from numeric number strings before converting to number.
@rudrajyotib
Copy link
Contributor

PR 783 raised. Please review.

@stleary
Copy link
Owner

stleary commented Oct 6, 2023

@johnjaylward is this comment still relevant and should it be included in the solution?
"also to note, for android compatibility, the code changes for "stringToNumber" will need to be copied to the XML class"

@stleary
Copy link
Owner

stleary commented Oct 7, 2023

Fixed with #783

@stleary stleary closed this as completed Oct 7, 2023
rudrajyotib added a commit to rudrajyotib/JSON-java that referenced this issue Oct 11, 2023
@johnjaylward
Copy link
Contributor

@johnjaylward is this comment still relevant and should it be included in the solution? "also to note, for android compatibility, the code changes for "stringToNumber" will need to be copied to the XML class"

Yes, it should be copied over

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants