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

Numbers deserialization using gson #771

Open
patton73 opened this issue Mar 4, 2023 · 3 comments
Open

Numbers deserialization using gson #771

patton73 opened this issue Mar 4, 2023 · 3 comments

Comments

@patton73
Copy link
Contributor

patton73 commented Mar 4, 2023

Starting from this issue #616

I can say that :

this bug was closed in Gson library with this PR google/gson#1290 merged in version 2.9.0 of GSON library. Right now the right way to have numbers parsed correctly is to configure the gson() in this way.

Gson gson = new GsonBuilder().setObjectToNumberStrategy(ToNumberPolicy.LONG_OR_DOUBLE).create();

I suggest to make a new release that relay on latest gson release (right now is 2.10.1). And to close the bug adding this line to the gson builder.

Thanks.
Andrea.

@patton73 patton73 changed the title Number deserializations using gson Number deserialization using gson Mar 4, 2023
@patton73 patton73 changed the title Number deserialization using gson Numbers deserialization using gson Mar 4, 2023
@lhazlewood
Copy link
Contributor

lhazlewood commented Mar 6, 2023

Just a reminder for implementation:

The default Gson instance should be configured with a custom strategy that combines the logic in the code block at the end of #616 (comment), i.e.

JsonNumber number = parser.next();
String sval = number.stringValue();
if (sval.contains(".")) {
    return toDecimal(sval); //returns a Float, Double or a BigDecimal depending on value size
} else {
   return toInteger(sval); //returns an Integer, Long or BigInteger depending on value size
}

and maybe a variant of all of the logic in #616 (comment).

Ideally, the smallest Number implementation should be used based on value size and precision.

That is, a BigDecimal instance should be used only if Double isn't sufficient (can't be used without data loss). Double should be used only if Float isn't sufficient. Similarly, a BigInteger should only be used if Long isn't sufficient, and Long should only be used if Integer isn't sufficient. We probably don't need to worry about Short or Byte, as most app devs probably won't want that, and if they do, they can easily enable it on their own Gson instance as an alternative.

@patton73
Copy link
Contributor Author

patton73 commented Mar 6, 2023

They also have this option that at first sight should do the job.

ToNumberPolicy.LAZILY_PARSED_NUMBER that implements the default behavior of the Number type adapter.

Basically you get a LazilyParsedNumber and return the needed number type.
More if you want to write you own logic you need to implement a policy directly as in this example:

Gson gson = GsonBuilder().setObjectToNumberStrategy { reader ->
        // Read the JSON number as string to inspect its format
        String numberAsString = reader.nextString()

        if (numberAsString.contains('.')) 
                   //Pseudocode
                    numberAsString.toDouble()
            else 
                   //Pseudocode
                   numberAsString.toInt()
    }
    .create()

Regards.

@lhazlewood
Copy link
Contributor

lhazlewood commented Mar 6, 2023

I could be missing something, but I don't think that ToNumberPolicy.LAZILY_PARSED_NUMBER is desirable, at least definitely not as a JJWT default.

The code samples shown in the PR comment google/gson#1290 (comment) show that the values returned are instances of LazilyParsedNumber:

Gson gson = new GsonBuilder()
  .setObjectToNumberStrategy(ToNumberPolicy.LAZILY_PARSED_NUMBER)
  .create();
List<Object> actual = gson.fromJson("[null, 10, 10.0]", new TypeToken<List<Object>>() {}.getType());
List<LazilyParsedNumber> expected = Arrays.asList(null, new LazilyParsedNumber("10"), new LazilyParsedNumber("10.0"));
assertEquals(expected, actual);

I assume that the large majority of app developers do not want to deal with Gson-specific data types in their JSON data model. And at least from the JJWT perspective, the JSON parser should be totally transparent. If I switch from GsonDeserializer to JacksonDeserializer, I should never have to change my code that interacts with the JWT.

Based on that, I think JJWT's default Gson instance should enable a custom ToNumberStrategy that reflects the logic in my comment above. The Double or Integer logic isn't enough - values could be larger than what either type supports, so the step-down logic of BigDecimal to Decimal to Float and BigInteger to Long to Integer is necessary depending on the size/precision of the JSON value.

We could even provide an enum that indicates the 'level' of precision desired - everything as a BigDecimal or BigInteger or step-down to the 'smallest' data type that supports the value, and the app developer can choose their threshold as desired.

Just thinking out loud.

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

No branches or pull requests

2 participants