From 2a9177168be58ee3b24aa85f8903d534a2a994d8 Mon Sep 17 00:00:00 2001 From: Angus Holder Date: Fri, 10 Apr 2020 00:19:32 +0100 Subject: [PATCH] Improved error when using collection classes by mistake A new user of Moshi might try to make a class like data class Response ( val users: ArrayList ) Then when trying to create a JsonAdapter for it, they get an unhelpful error message like Platform class java.util.ArrayList in java.util.ArrayList requires explicit JsonAdapter to be registered which doesn't explain what they should do to fix the problem. In fact what it hints towards is that the user should implement a JsonAdapter for ArrayList, when really they just need to change the type to List! Now the error looks like No JsonAdapter for java.util.ArrayList, you should use List instead of ArrayList (Moshi only supports the collection interfaces by default). --- .../com/squareup/moshi/ClassJsonAdapter.java | 22 +++++++ .../java/com/squareup/moshi/MoshiTest.java | 64 +++++++++++++++---- 2 files changed, 75 insertions(+), 11 deletions(-) diff --git a/moshi/src/main/java/com/squareup/moshi/ClassJsonAdapter.java b/moshi/src/main/java/com/squareup/moshi/ClassJsonAdapter.java index a149116..9d33553 100644 --- a/moshi/src/main/java/com/squareup/moshi/ClassJsonAdapter.java +++ b/moshi/src/main/java/com/squareup/moshi/ClassJsonAdapter.java @@ -23,6 +23,8 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Modifier; import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; +import java.util.Collection; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.TreeMap; @@ -58,6 +60,11 @@ final class ClassJsonAdapter extends JsonAdapter { if (rawType.isInterface() || rawType.isEnum()) return null; if (!annotations.isEmpty()) return null; if (Util.isPlatformType(rawType)) { + throwIfIsCollectionClass(type, List.class); + throwIfIsCollectionClass(type, Set.class); + throwIfIsCollectionClass(type, Map.class); + throwIfIsCollectionClass(type, Collection.class); + String messagePrefix = "Platform " + rawType; if (type instanceof ParameterizedType) { messagePrefix += " in " + type; @@ -94,6 +101,21 @@ final class ClassJsonAdapter extends JsonAdapter { return new ClassJsonAdapter<>(classFactory, fields).nullSafe(); } + /** + * Throw clear error messages for the common beginner mistake of using the concrete + * collection classes instead of the collection interfaces, eg: ArrayList instead of List. + */ + private void throwIfIsCollectionClass(Type type, Class collectionInterface) { + Class rawClass = Types.getRawType(type); + if (collectionInterface.isAssignableFrom(rawClass)) { + throw new IllegalArgumentException( + "No JsonAdapter for " + type + ", you should probably use " + + collectionInterface.getSimpleName() + " instead of " + rawClass.getSimpleName() + + " (Moshi only supports the collection interfaces by default)" + + " or else register a custom JsonAdapter."); + } + } + /** Creates a field binding for each of declared field of {@code type}. */ private void createFieldBindings( Moshi moshi, Type type, Map> fieldBindings) { diff --git a/moshi/src/test/java/com/squareup/moshi/MoshiTest.java b/moshi/src/test/java/com/squareup/moshi/MoshiTest.java index 1d51689..e6f2013 100644 --- a/moshi/src/test/java/com/squareup/moshi/MoshiTest.java +++ b/moshi/src/test/java/com/squareup/moshi/MoshiTest.java @@ -28,12 +28,14 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Set; +import java.util.UUID; import javax.annotation.Nullable; import javax.crypto.KeyGenerator; import okio.Buffer; @@ -45,7 +47,7 @@ import static java.lang.annotation.RetentionPolicy.RUNTIME; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.fail; -@SuppressWarnings("CheckReturnValue") +@SuppressWarnings({"CheckReturnValue", "ResultOfMethodCallIgnored"}) public final class MoshiTest { @Test public void booleanAdapter() throws Exception { Moshi moshi = new Moshi.Builder().build(); @@ -954,8 +956,48 @@ public final class MoshiTest { } } + @Test public void collectionClassesHaveClearErrorMessage() { + Moshi moshi = new Moshi.Builder().build(); + try { + moshi.adapter(Types.newParameterizedType(ArrayList.class, String.class)); + fail(); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessage("No JsonAdapter for " + + "java.util.ArrayList, " + + "you should probably use List instead of ArrayList " + + "(Moshi only supports the collection interfaces by default) " + + "or else register a custom JsonAdapter."); + } + + try { + moshi.adapter(Types.newParameterizedType(HashMap.class, String.class, String.class)); + fail(); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessage("No JsonAdapter for " + + "java.util.HashMap, " + + "you should probably use Map instead of HashMap " + + "(Moshi only supports the collection interfaces by default) " + + "or else register a custom JsonAdapter."); + } + } + + @Test public void noCollectionErrorIfAdapterExplicitlyProvided() { + Moshi moshi = new Moshi.Builder() + .add(new JsonAdapter.Factory() { + @Override public JsonAdapter create(Type type, Set annotations, + Moshi moshi) { + return new MapJsonAdapter(moshi, String.class, String.class); + } + }) + .build(); + + JsonAdapter> adapter = moshi.adapter( + Types.newParameterizedType(HashMap.class, String.class, String.class)); + assertThat(adapter).isInstanceOf(MapJsonAdapter.class); + } + static final class HasPlatformType { - ArrayList strings; + UUID uuid; static final class Wrapper { HasPlatformType hasPlatformType; @@ -973,14 +1015,14 @@ public final class MoshiTest { fail(); } catch (IllegalArgumentException e) { assertThat(e).hasMessage( - "Platform class java.util.ArrayList in java.util.ArrayList requires explicit " + "Platform class java.util.UUID requires explicit " + "JsonAdapter to be registered" - + "\nfor java.util.ArrayList strings" + + "\nfor class java.util.UUID uuid" + "\nfor class com.squareup.moshi.MoshiTest$HasPlatformType" + "\nfor java.util.Map"); assertThat(e).hasCauseExactlyInstanceOf(IllegalArgumentException.class); - assertThat(e.getCause()).hasMessage("Platform class java.util.ArrayList in java.util.ArrayList " + assertThat(e.getCause()).hasMessage("Platform class java.util.UUID " + "requires explicit JsonAdapter to be registered"); } } @@ -992,13 +1034,13 @@ public final class MoshiTest { fail(); } catch (IllegalArgumentException e) { assertThat(e).hasMessage( - "Platform class java.util.ArrayList in java.util.ArrayList requires explicit " + "Platform class java.util.UUID requires explicit " + "JsonAdapter to be registered" - + "\nfor java.util.ArrayList strings" + + "\nfor class java.util.UUID uuid" + "\nfor class com.squareup.moshi.MoshiTest$HasPlatformType hasPlatformType" + "\nfor class com.squareup.moshi.MoshiTest$HasPlatformType$Wrapper"); assertThat(e).hasCauseExactlyInstanceOf(IllegalArgumentException.class); - assertThat(e.getCause()).hasMessage("Platform class java.util.ArrayList in java.util.ArrayList " + assertThat(e.getCause()).hasMessage("Platform class java.util.UUID " + "requires explicit JsonAdapter to be registered"); } } @@ -1010,14 +1052,14 @@ public final class MoshiTest { fail(); } catch (IllegalArgumentException e) { assertThat(e).hasMessage( - "Platform class java.util.ArrayList in java.util.ArrayList requires explicit " + "Platform class java.util.UUID requires explicit " + "JsonAdapter to be registered" - + "\nfor java.util.ArrayList strings" + + "\nfor class java.util.UUID uuid" + "\nfor class com.squareup.moshi.MoshiTest$HasPlatformType" + "\nfor java.util.List platformTypes" + "\nfor class com.squareup.moshi.MoshiTest$HasPlatformType$ListWrapper"); assertThat(e).hasCauseExactlyInstanceOf(IllegalArgumentException.class); - assertThat(e.getCause()).hasMessage("Platform class java.util.ArrayList in java.util.ArrayList " + assertThat(e.getCause()).hasMessage("Platform class java.util.UUID " + "requires explicit JsonAdapter to be registered"); } }