square / moshi

A modern JSON library for Kotlin and Java.
https://square.github.io/moshi/1.x/
Apache License 2.0
9.73k stars 758 forks source link

Allow PolymorphicJsonAdapterFactory's fallback adapter to handle case when expected `labelKey` is missing from JSON object. #1512

Open hossain-khan opened 2 years ago

hossain-khan commented 2 years ago

Start by telling us what problem you’re trying to solve. Often a solution already exists!

Summary

I was trying to use the PolymorphicJsonAdapterFactory, and the usage is very straightforward, as seen in test:

PolymorphicJsonAdapterFactory.of(Message.class, "type" /* labelKey */)
            .withSubtype(Success.class, "success")
            .withSubtype(Error.class, "error")

However, in non-ideal world, the backend server may not always send the expected labelKey. In that case, instead of failing, I wanted to leverage the withFallbackJsonAdapter API to return a different subtype.

However, when the labelKey is missing, it throws JsonDataException: Missing label for type, which would result in failing the API request.

Provided a unit-test below that explains this further. I was wondering if allowing the provided withFallbackJsonAdapter to handle such case would be an acceptable use case?

Test that reflects the ask

I have added a test in PolymorphicJsonAdapterFactoryTest to showcase what is expected

/**
 * When {@link PolymorphicJsonAdapterFactory#withFallbackJsonAdapter(JsonAdapter)} is used,
 * it should allow the adapter to handle missing `labelKey` from the source JSON object,
 * so that custom handling can be done using the provided fallback JSON adapter.
 *
 * In this unit-test, when type is missing, a special implementation {@link UnknownMessage} is provided,
 * so that the application can render something else based on business-logic/requirement.
 *
 * <p>---</p>
 *
 * Currently, for following JSON input (that has "type" labelKey missing in JSON object):
 * <code>{"faulty_state":"Unknown Message Type"}</code>
 *
 * It fails with {@code "JsonDataException: Missing label for type"} error.
 *
 * <p>
 *
 * The new {@link UnknownMessage} class of type {@link Message} is defined as:
 * <pre>
 * static final class UnknownMessage implements Message {
 *   &#64;Override
 *   public String toString() {
 *     return "Unknown";
 *   }
 * }
 * </pre>
 */
@Test
public void missingLabelKeyShouldFallBackToJsonAdapter() throws IOException {
  Moshi moshi =
    new Moshi.Builder()
      .add(
        PolymorphicJsonAdapterFactory.of(Message.class, "type")
          .withSubtype(Success.class, "success")
          .withSubtype(Error.class, "error")
          .withFallbackJsonAdapter(
            new JsonAdapter<Object>() {
              @Override
              public Object fromJson(JsonReader reader) throws IOException {
                reader.beginObject();
                // This is just test, the actual implementation can vary based on JSON data.
                assertThat(reader.nextName()).isEqualTo("faulty_state");
                assertThat(reader.nextString()).isEqualTo("Unknown Message Type");
                reader.endObject();
                return new UnknownMessage();
              }

              @Override
              public void toJson(JsonWriter writer, @Nullable Object value) {
                throw new AssertionError();
              }
            }))
      .build();
  JsonAdapter<Message> adapter = moshi.adapter(Message.class);

  JsonReader reader =
    JsonReader.of(new Buffer().writeUtf8("{\"faulty_state\":\"Unknown Message Type\"}"));

  Message message = adapter.fromJson(reader);
  assertThat(message).isInstanceOf(UnknownMessage.class);
  assertThat(reader.peek()).isEqualTo(JsonReader.Token.END_DOCUMENT);
}
ZacSweers commented 2 years ago

I think using the default adapter in that case seems reasonable. @swankjesse @JakeWharton thoughts?

hossain-khan commented 2 years ago

I have talked to server-side engineers to make sure the labelKey is always present for polymorphic type responses.

In case of UnknownMessage, server-side could likely implement a specific typed object to represent it.

{
  "type": "unknown"
}

I am going to close this request for now until a new use-case comes in from different individual.

consp1racy commented 2 years ago

@hossain-khan Hi, do you want to reopen this, so we don't have multiple Issues tracking the same thing? I'm interested.

Three years ago I did something along the lines of

/**
 * <p>Eugen changed: Support {@code null} label and absent label key.
 */
public final class PolymorphicJsonAdapterFactory<T> implements JsonAdapter.Factory {

  private static final String FAKE_NULL_LABEL = "__FAKE_NULL_LABEL__";

  // ...
}
PolymorphicJsonAdapterFactory.of(JsonBasicPlan::class.java, "version")
    .withSubtype(JsonBasicPlan1::class.java, null)
    .withSubtype(JsonBasicPlan2::class.java, "2")
ZacSweers commented 1 year ago

Reopening this as I think this is a valid use case

jsilva05 commented 1 month ago

Hi all! Is there any workaround or plan to achieve this? I'm facing this issue and we do not control what the backend sends us. Sometimes, they send an empty object without the key and our decode fails.

I am wondering if this is still something we'd like to support in the lib