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

Make default adapters stricter; improve exception messages #2000

Merged

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Oct 25, 2021

Makes the default type adapters stricter:

  • byte and short adapter detect numeric overflow (but both allow signed and unsigned value range, e.g. byte allows -128 to 255).
    For float no such check has been added even though conversion from small != 0 double can become 0 float and large finite double values can become Infinity float, because there the 'meaning' is preserved. I have also not adjusted the lenient check to detect float Infinity after conversion from finite double, because the JSON contains finite numbers and therefore should be allowed in non-lenient mode.
  • BitSet only allows 0 and 1 as numeric values

Improves exception messages:

  • Adds JsonReader.getPreviousPath(), this allows getting the JSON path after a value has been consumed. The existing getPath() would for array elements already point to the next element.
  • Adds descriptions to many of the JsonSyntaxExceptions thrown by adapters. Note that the implementation proposed here allows exception message injection (when the JSON comes from an untrusted source), but I am not sure if this is something which Gson should consider; the existing JsonReader implementation is already vulnerable to this anyway when it includes the JSON path in exception messages for malformed JSON.

Improves synchronization of classes using DateFormat. Reduces the synchronized blocks to the minimum, excluding JSON reading and writing calls to only hold the lock as short as possible.

@google-cla google-cla bot added the cla: yes label Oct 25, 2021
case STRING:
int intValue = in.nextInt();
Copy link
Collaborator Author

@Marcono1234 Marcono1234 Oct 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JsonReader.nextInt() can also parse JSON strings as int so I simplified this instead of handling JSON strings separately (as it was the case before). However, this has the following disadvantages:

  • The exception thrown for a JSON string which is not an int (e.g. "abc") is not as descriptive anymore because it will now be thrown by JsonReader
  • It now allows parsing JSON strings which have a floating point value (but whose value is integral), previously it only allowed ints due to usage of Integer.parseInt

So maybe it would still be worth handling JSON strings here manually, as it was done before?

Copy link
Member

@eamonnmcmanus eamonnmcmanus Oct 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think it's likely that people would be affected by these particular changes in behaviour. Only recognizing 0 or 1 seems more likely to affect people, but even that seems unlikely. (I'm not sure how often people are serializing BitSet instances anyway, especially given how verbose the encoding is for sparse sets.)

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Thanks, this looks great! Particularly the thorough tests.

case STRING:
int intValue = in.nextInt();
Copy link
Member

@eamonnmcmanus eamonnmcmanus Oct 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think it's likely that people would be affected by these particular changes in behaviour. Only recognizing 0 or 1 seems more likely to affect people, but even that seems unlikely. (I'm not sure how often people are serializing BitSet instances anyway, especially given how verbose the encoding is for sparse sets.)

gson/src/main/java/com/google/gson/stream/JsonReader.java Outdated Show resolved Hide resolved
@eamonnmcmanus eamonnmcmanus merged commit b4dab86 into google:master Nov 1, 2021
3 checks passed
@Marcono1234 Marcono1234 deleted the marcono1234/stricter-type-adapters branch Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants