Merge pull request #1121 from angusholder/improve-collections-errors

Improve error when incorrectly trying to use a collection class like ArrayList instead of List

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<T> extends JsonAdapter<T> {
       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<T> extends JsonAdapter<T> {
       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<String, FieldBinding<?>> fieldBindings) {

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<java.lang.String>, "
+          + "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<java.lang.String, java.lang.String>, "
+          + "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<? extends Annotation> annotations,
+              Moshi moshi) {
+            return new MapJsonAdapter<String, String>(moshi, String.class, String.class);
+          }
+        })
+        .build();
+
+    JsonAdapter<HashMap<String, String>> adapter = moshi.adapter(
+            Types.newParameterizedType(HashMap.class, String.class, String.class));
+    assertThat(adapter).isInstanceOf(MapJsonAdapter.class);
+  }
+
   static final class HasPlatformType {
-    ArrayList<String> 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<java.lang.String> requires explicit "
+          "Platform class java.util.UUID requires explicit "
               + "JsonAdapter to be registered"
-              + "\nfor java.util.ArrayList<java.lang.String> strings"
+              + "\nfor class java.util.UUID uuid"
               + "\nfor class com.squareup.moshi.MoshiTest$HasPlatformType"
               + "\nfor java.util.Map<java.lang.String, "
               + "com.squareup.moshi.MoshiTest$HasPlatformType>");
       assertThat(e).hasCauseExactlyInstanceOf(IllegalArgumentException.class);
-      assertThat(e.getCause()).hasMessage("Platform class java.util.ArrayList in java.util.ArrayList<java.lang.String> "
+      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<java.lang.String> requires explicit "
+          "Platform class java.util.UUID requires explicit "
               + "JsonAdapter to be registered"
-              + "\nfor java.util.ArrayList<java.lang.String> 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<java.lang.String> "
+      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<java.lang.String> requires explicit "
+          "Platform class java.util.UUID requires explicit "
               + "JsonAdapter to be registered"
-              + "\nfor java.util.ArrayList<java.lang.String> strings"
+              + "\nfor class java.util.UUID uuid"
               + "\nfor class com.squareup.moshi.MoshiTest$HasPlatformType"
               + "\nfor java.util.List<com.squareup.moshi.MoshiTest$HasPlatformType> 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<java.lang.String> "
+      assertThat(e.getCause()).hasMessage("Platform class java.util.UUID "
           + "requires explicit JsonAdapter to be registered");
     }
   }