From 895c3ddb4971fde955f6e49eed12738cb599b1a5 Mon Sep 17 00:00:00 2001 From: Eric Cochran Date: Wed, 12 Sep 2018 19:27:58 -0700 Subject: [PATCH] Track field names in the adapter lookup stack. (#616) * Track field names in the adapter lookup stack. This allows for an error message that includes field names to track down the cause of adapter creation failure for deeply nested structures. * Use a single stack in Moshi client. * Make optional fieldName parameter public API. * Simplify error message. --- .../com/squareup/moshi/ClassJsonAdapter.java | 5 +- .../main/java/com/squareup/moshi/Moshi.java | 56 +++++++++++++-- .../squareup/moshi/JsonQualifiersTest.java | 14 +++- .../java/com/squareup/moshi/MoshiTest.java | 69 +++++++++++++++++++ 4 files changed, 134 insertions(+), 10 deletions(-) diff --git a/moshi/src/main/java/com/squareup/moshi/ClassJsonAdapter.java b/moshi/src/main/java/com/squareup/moshi/ClassJsonAdapter.java index aee6d55..71d4534 100644 --- a/moshi/src/main/java/com/squareup/moshi/ClassJsonAdapter.java +++ b/moshi/src/main/java/com/squareup/moshi/ClassJsonAdapter.java @@ -95,14 +95,15 @@ final class ClassJsonAdapter extends JsonAdapter { // Look up a type adapter for this type. Type fieldType = resolve(type, rawType, field.getGenericType()); Set annotations = Util.jsonAnnotations(field); - JsonAdapter adapter = moshi.adapter(fieldType, annotations); + String fieldName = field.getName(); + JsonAdapter adapter = moshi.adapter(fieldType, annotations, fieldName); // Create the binding between field and JSON. field.setAccessible(true); // Store it using the field's name. If there was already a field with this name, fail! Json jsonAnnotation = field.getAnnotation(Json.class); - String name = jsonAnnotation != null ? jsonAnnotation.name() : field.getName(); + String name = jsonAnnotation != null ? jsonAnnotation.name() : fieldName; FieldBinding fieldBinding = new FieldBinding<>(name, field, adapter); FieldBinding replaced = fieldBindings.put(name, fieldBinding); if (replaced != null) { diff --git a/moshi/src/main/java/com/squareup/moshi/Moshi.java b/moshi/src/main/java/com/squareup/moshi/Moshi.java index 8d39c47..26db316 100644 --- a/moshi/src/main/java/com/squareup/moshi/Moshi.java +++ b/moshi/src/main/java/com/squareup/moshi/Moshi.java @@ -90,8 +90,18 @@ public final class Moshi { } @CheckReturnValue - @SuppressWarnings("unchecked") // Factories are required to return only matching JsonAdapters. public JsonAdapter adapter(Type type, Set annotations) { + return adapter(type, annotations, null); + } + + /** + * @param fieldName An optional field name associated with this type. The field name is used as a + * hint for better adapter lookup error messages for nested structures. + */ + @CheckReturnValue + @SuppressWarnings("unchecked") // Factories are required to return only matching JsonAdapters. + public JsonAdapter adapter(Type type, Set annotations, + @Nullable String fieldName) { if (type == null) { throw new NullPointerException("type == null"); } @@ -123,8 +133,10 @@ public final class Moshi { } // Prepare for re-entrant calls, then ask each factory to create a type adapter. - DeferredAdapter deferredAdapter = new DeferredAdapter<>(cacheKey); + DeferredAdapter deferredAdapter = new DeferredAdapter<>(type, fieldName, cacheKey); + deferredAdapters.add(deferredAdapter); + int lastIndex = deferredAdapters.size() - 1; try { for (int i = 0, size = factories.size(); i < size; i++) { JsonAdapter result = (JsonAdapter) factories.get(i).create(type, annotations, this); @@ -133,12 +145,19 @@ public final class Moshi { synchronized (adapterCache) { adapterCache.put(cacheKey, result); } + // Remove the type or field name only when we succeed in creating the adapter, + // so we have the full stack at the top level. + deferredAdapters.remove(lastIndex); return result; } } + } catch (IllegalArgumentException e) { + if (lastIndex == 0) { // Rewrite the exception at the top level. + e = errorWithFields(deferredAdapters, e); + } + throw e; } finally { - deferredAdapters.remove(deferredAdapters.size() - 1); - if (deferredAdapters.isEmpty()) { + if (lastIndex == 0) { reentrantCalls.remove(); } } @@ -181,6 +200,27 @@ public final class Moshi { return Arrays.asList(type, annotations); } + static IllegalArgumentException errorWithFields(List> typesAndFieldNames, + IllegalArgumentException e) { + int size = typesAndFieldNames.size(); + if (size == 1 && typesAndFieldNames.get(0).fieldName == null) { + return e; + } + StringBuilder errorMessageBuilder = new StringBuilder(e.getMessage()); + for (int i = size - 1; i >= 0; i--) { + DeferredAdapter deferredAdapter = typesAndFieldNames.get(i); + errorMessageBuilder + .append("\nfor ") + .append(deferredAdapter.type); + if (deferredAdapter.fieldName != null) { + errorMessageBuilder + .append(' ') + .append(deferredAdapter.fieldName); + } + } + return new IllegalArgumentException(errorMessageBuilder.toString(), e); + } + public static final class Builder { final List factories = new ArrayList<>(); @@ -250,11 +290,15 @@ public final class Moshi { *

Typically this is necessary in self-referential object models, such as an {@code Employee} * class that has a {@code List} field for an organization's management hierarchy. */ - private static class DeferredAdapter extends JsonAdapter { + private static final class DeferredAdapter extends JsonAdapter { + final Type type; + final @Nullable String fieldName; @Nullable Object cacheKey; private @Nullable JsonAdapter delegate; - DeferredAdapter(Object cacheKey) { + DeferredAdapter(Type type, @Nullable String fieldName, Object cacheKey) { + this.type = type; + this.fieldName = fieldName; this.cacheKey = cacheKey; } diff --git a/moshi/src/test/java/com/squareup/moshi/JsonQualifiersTest.java b/moshi/src/test/java/com/squareup/moshi/JsonQualifiersTest.java index 647df10..0c46e3a 100644 --- a/moshi/src/test/java/com/squareup/moshi/JsonQualifiersTest.java +++ b/moshi/src/test/java/com/squareup/moshi/JsonQualifiersTest.java @@ -332,7 +332,12 @@ public final class JsonQualifiersTest { moshi.adapter(StringAndFooString.class); fail(); } catch (IllegalArgumentException expected) { - assertThat(expected).hasMessage("No @FromJson adapter for class java.lang.String " + assertThat(expected).hasMessage("No @FromJson adapter for class java.lang.String annotated " + + "[@com.squareup.moshi.JsonQualifiersTest$FooPrefix()]" + + "\nfor class java.lang.String b" + + "\nfor class com.squareup.moshi.JsonQualifiersTest$StringAndFooString"); + assertThat(expected).hasCauseExactlyInstanceOf(IllegalArgumentException.class); + assertThat(expected.getCause()).hasMessage("No @FromJson adapter for class java.lang.String " + "annotated [@com.squareup.moshi.JsonQualifiersTest$FooPrefix()]"); assertThat(expected).hasCauseExactlyInstanceOf(IllegalArgumentException.class); assertThat(expected.getCause()).hasMessage("No next JsonAdapter for class " @@ -356,7 +361,12 @@ public final class JsonQualifiersTest { moshi.adapter(StringAndFooString.class); fail(); } catch (IllegalArgumentException expected) { - assertThat(expected).hasMessage("No @ToJson adapter for class java.lang.String " + assertThat(expected).hasMessage("No @ToJson adapter for class java.lang.String annotated " + + "[@com.squareup.moshi.JsonQualifiersTest$FooPrefix()]" + + "\nfor class java.lang.String b" + + "\nfor class com.squareup.moshi.JsonQualifiersTest$StringAndFooString"); + assertThat(expected).hasCauseExactlyInstanceOf(IllegalArgumentException.class); + assertThat(expected.getCause()).hasMessage("No @ToJson adapter for class java.lang.String " + "annotated [@com.squareup.moshi.JsonQualifiersTest$FooPrefix()]"); assertThat(expected).hasCauseExactlyInstanceOf(IllegalArgumentException.class); assertThat(expected.getCause()).hasMessage("No next JsonAdapter for class " diff --git a/moshi/src/test/java/com/squareup/moshi/MoshiTest.java b/moshi/src/test/java/com/squareup/moshi/MoshiTest.java index cefd67a..1303b63 100644 --- a/moshi/src/test/java/com/squareup/moshi/MoshiTest.java +++ b/moshi/src/test/java/com/squareup/moshi/MoshiTest.java @@ -45,6 +45,7 @@ import static java.lang.annotation.RetentionPolicy.RUNTIME; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.fail; +@SuppressWarnings("CheckReturnValue") public final class MoshiTest { @Test public void booleanAdapter() throws Exception { Moshi moshi = new Moshi.Builder().build(); @@ -955,6 +956,74 @@ public final class MoshiTest { } } + static final class HasPlatformType { + ArrayList strings; + + static final class Wrapper { + HasPlatformType hasPlatformType; + } + + static final class ListWrapper { + List platformTypes; + } + } + + @Test public void reentrantFieldErrorMessagesTopLevelMap() { + Moshi moshi = new Moshi.Builder().build(); + try { + moshi.adapter(Types.newParameterizedType(Map.class, String.class, HasPlatformType.class)); + fail(); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessage( + "Platform java.util.ArrayList (with no annotations) requires explicit " + + "JsonAdapter to be registered" + + "\nfor java.util.ArrayList strings" + + "\nfor class com.squareup.moshi.MoshiTest$HasPlatformType" + + "\nfor java.util.Map"); + assertThat(e).hasCauseExactlyInstanceOf(IllegalArgumentException.class); + assertThat(e.getCause()).hasMessage("Platform java.util.ArrayList " + + "(with no annotations) requires explicit JsonAdapter to be registered"); + } + } + + @Test public void reentrantFieldErrorMessagesWrapper() { + Moshi moshi = new Moshi.Builder().build(); + try { + moshi.adapter(HasPlatformType.Wrapper.class); + fail(); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessage( + "Platform java.util.ArrayList (with no annotations) requires explicit " + + "JsonAdapter to be registered" + + "\nfor java.util.ArrayList strings" + + "\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 java.util.ArrayList " + + "(with no annotations) requires explicit JsonAdapter to be registered"); + } + } + + @Test public void reentrantFieldErrorMessagesListWrapper() { + Moshi moshi = new Moshi.Builder().build(); + try { + moshi.adapter(HasPlatformType.ListWrapper.class); + fail(); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessage( + "Platform java.util.ArrayList (with no annotations) requires explicit " + + "JsonAdapter to be registered" + + "\nfor java.util.ArrayList strings" + + "\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 java.util.ArrayList " + + "(with no annotations) requires explicit JsonAdapter to be registered"); + } + } + @Test public void qualifierWithElementsMayNotBeDirectlyRegistered() throws IOException { try { new Moshi.Builder()