google / gson

A Java serialization/deserialization library to convert Java Objects into JSON and back
Apache License 2.0
23.13k stars 4.27k forks source link

`JsonReader#beginObject` should throw checked exceptions, instead throws runtime `IllegalStateException`. #2663

Closed grantamos closed 2 months ago

grantamos commented 3 months ago

Gson version

2.9.0

Java / Android version

Unsure - if this is needed please let me know & I can dig this up, though it should be of no consequence.

Used tools

Nothing of consequence to this bug.

Description

When parsing an JSON object that fails due to the value being an incorrect type, an unchecked exception is thrown.

beginObject specifies a checked exception of type IOException here: https://github.com/google/gson/blob/18b08922537c16b0de1d503c82307d613aae0138/gson/src/main/java/com/google/gson/stream/JsonReader.java#L454

When it fails to find an object, however, it throws an IllegalStateException here: https://github.com/google/gson/blob/18b08922537c16b0de1d503c82307d613aae0138/gson/src/main/java/com/google/gson/stream/JsonReader.java#L463 and here: https://github.com/google/gson/blob/18b08922537c16b0de1d503c82307d613aae0138/gson/src/main/java/com/google/gson/stream/JsonReader.java#L1744

This isn't caught at compile time because IllegalStateException is a RuntimeException. Note that unexpectedTokenError says it throws IOException but the IDE warns this throws is unused.

Expected behavior

We expected an IOException to be thrown. Else, the beginObject method should check any other exceptions.

Actual behavior

An unchecked runtime exception was thrown.

Reproduction steps

Attempt to parse a JSON payload expecting an object but finding another type.

e.g.

JSON PAYLOAD:
{
  "this_key_is_expected_to_be_an_object": false
}

Java class:
class Payload {
   public NestedObject this_key_is_expected_to_be_an_object;
}

class NestedObject {
  public boolean inner_field;
}

Exception stack trace

java.lang.IllegalStateException: Expected BEGIN_OBJECT but was BOOLEAN at line 1 column 2691 path _REDACTED_
        at com.google.gson.stream.JsonReader.beginObject(JsonReader.java:393)
        at com.squareup.wire.MessageTypeAdapter.read(MessageTypeAdapter.kt:59)
        at com.squareup.wire.MessageTypeAdapter.read(MessageTypeAdapter.kt:25)
        at com.google.gson.TypeAdapter$1.read(TypeAdapter.java:204)
        at com.google.gson.TypeAdapter$1.read(TypeAdapter.java:204)
        at com.squareup.wire.MessageTypeAdapter.read(MessageTypeAdapter.kt:68)
        at com.squareup.wire.MessageTypeAdapter.read(MessageTypeAdapter.kt:25)
        at com.google.gson.TypeAdapter$1.read(TypeAdapter.java:204)
        at com.google.gson.TypeAdapter$1.read(TypeAdapter.java:204)
        at com.squareup.wire.MessageTypeAdapter.read(MessageTypeAdapter.kt:68)
        at com.squareup.wire.MessageTypeAdapter.read(MessageTypeAdapter.kt:25)
        at com.google.gson.TypeAdapter$1.read(TypeAdapter.java:204)
        at retrofit2.converter.gson.GsonResponseBodyConverter.convert(GsonResponseBodyConverter.java:39)
        at retrofit2.converter.gson.GsonResponseBodyConverter.convert(GsonResponseBodyConverter.java:27)
        at retrofit2.OkHttpCall.parseResponse(OkHttpCall.java:227)
        at retrofit2.OkHttpCall$1.onResponse(OkHttpCall.java:142)
        at okhttp3.internal.connection.RealCall$AsyncCall.run(RealCall.kt:519)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1162)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:636)
        at java.lang.Thread.run(Thread.java:764)
Marcono1234 commented 3 months ago

beginObject specifies a checked exception of type IOException here [...] Note that unexpectedTokenError says it throws IOException but the IDE warns this throws is unused.

They have throws IOException because reading (which occurs when doPeek() / peek() is called) can throw an IOException.

To me it seems it is intentional that an IllegalStateException is thrown. Whether it would have been more consistent to throw an IOException or a custom subclass since a JSON syntax error causes an IOException (more specifically MalformedJsonException) is a different question. But this can probably not be changed easily anymore either since it would be backward incompatible for users who expect an IllegalStateException respectively don't expect an IOException in that case.

For some of the JsonReader methods the documentation explicitly mentions the IllegalStateException: nextString(), nextBoolean(), nextNull(), nextDouble(), nextLong(), nextInt()

So to me the issue here seems to rather be that the other JsonReader methods (such as beginObject you mentioned) don't have the IllegalStateException documented.

Would you like to create a pull request for this (see also the contributing guide)? That would probably involve:

Marcono1234 commented 2 months ago

Have created #2670 now to improve the JsonReader documentation.