Merge pull request #1121 from angusholder/improve-collections-errors
Improve error when incorrectly trying to use a collection class like ArrayList instead of List
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.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 @@
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 @@
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) {
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.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 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 @@
}
}
+ @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 @@
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 @@
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 @@
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");
}
}
