From 7b73ce04ec6dcf329a42999a953f51db54ce3f92 Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sun, 13 Oct 2019 16:20:29 -0400 Subject: [PATCH 1/6] Port Gson Types recursion fix for subtypeOf/supertypeOf From https://github.com/google/gson/commit/a300148003e3a067875b1444e8268b6e0f0e0e02 First step in resolving #338 --- .../main/java/com/squareup/moshi/Types.java | 16 +++- .../internal/RecursiveTypesResolveTest.java | 79 +++++++++++++++++++ 2 files changed, 93 insertions(+), 2 deletions(-) create mode 100644 moshi/src/test/java/com/squareup/moshi/internal/RecursiveTypesResolveTest.java diff --git a/moshi/src/main/java/com/squareup/moshi/Types.java b/moshi/src/main/java/com/squareup/moshi/Types.java index a9bca93aa..40829e37a 100644 --- a/moshi/src/main/java/com/squareup/moshi/Types.java +++ b/moshi/src/main/java/com/squareup/moshi/Types.java @@ -139,7 +139,13 @@ public static GenericArrayType arrayOf(Type componentType) { * ? extends Object}. */ public static WildcardType subtypeOf(Type bound) { - return new WildcardTypeImpl(new Type[] { bound }, EMPTY_TYPE_ARRAY); + Type[] upperBounds; + if (bound instanceof WildcardType) { + upperBounds = ((WildcardType) bound).getUpperBounds(); + } else { + upperBounds = new Type[] { bound }; + } + return new WildcardTypeImpl(upperBounds, EMPTY_TYPE_ARRAY); } /** @@ -147,7 +153,13 @@ public static WildcardType subtypeOf(Type bound) { * bound} is {@code String.class}, this returns {@code ? super String}. */ public static WildcardType supertypeOf(Type bound) { - return new WildcardTypeImpl(new Type[] { Object.class }, new Type[] { bound }); + Type[] lowerBounds; + if (bound instanceof WildcardType) { + lowerBounds = ((WildcardType) bound).getLowerBounds(); + } else { + lowerBounds = new Type[] { bound }; + } + return new WildcardTypeImpl(new Type[] { Object.class }, lowerBounds); } public static Class getRawType(Type type) { diff --git a/moshi/src/test/java/com/squareup/moshi/internal/RecursiveTypesResolveTest.java b/moshi/src/test/java/com/squareup/moshi/internal/RecursiveTypesResolveTest.java new file mode 100644 index 000000000..a8add0a9b --- /dev/null +++ b/moshi/src/test/java/com/squareup/moshi/internal/RecursiveTypesResolveTest.java @@ -0,0 +1,79 @@ +/* + * Copyright (C) 2017 Gson Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.squareup.moshi.internal; + +import com.squareup.moshi.JsonAdapter; +import com.squareup.moshi.Moshi; +import com.squareup.moshi.Types; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; + +/** + * Test fixes for infinite recursion on {@link Util#resolve(java.lang.reflect.Type, Class, + * java.lang.reflect.Type)}, described at Issue #440 + * and similar issues. + *

+ * These tests originally caused {@link StackOverflowError} because of infinite recursion on attempts to + * resolve generics on types, with an intermediate types like 'Foo2<? extends ? super ? extends ... ? extends A>' + *

+ * Adapted from https://github.com/google/gson/commit/a300148003e3a067875b1444e8268b6e0f0e0e02 in + * service of https://github.com/square/moshi/issues/338. + */ +public final class RecursiveTypesResolveTest { + + private static class Foo1 { + public Foo2 foo2; + } + + private static class Foo2 { + public Foo1 foo1; + } + + /** + * Test simplest case of recursion. + */ + @Test public void testRecursiveResolveSimple() { + JsonAdapter adapter = new Moshi.Builder().build().adapter(Foo1.class); + assertNotNull(adapter); + } + + // + // Tests belows check the behaviour of the methods changed for the fix + // + + @Test public void testDoubleSupertype() { + assertEquals(Types.supertypeOf(Number.class), + Types.supertypeOf(Types.supertypeOf(Number.class))); + } + + @Test public void testDoubleSubtype() { + assertEquals(Types.subtypeOf(Number.class), + Types.subtypeOf(Types.subtypeOf(Number.class))); + } + + @Test public void testSuperSubtype() { + assertEquals(Types.subtypeOf(Object.class), + Types.supertypeOf(Types.subtypeOf(Number.class))); + } + + @Test public void testSubSupertype() { + assertEquals(Types.subtypeOf(Object.class), + Types.subtypeOf(Types.supertypeOf(Number.class))); + } +} From 62e896b0610c771db4d88e1d2de11adf48e625da Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sun, 13 Oct 2019 16:21:35 -0400 Subject: [PATCH 2/6] Port Gson Type resolve() recursion fix for type variables From https://github.com/google/gson/pull/1128 Resolves #338 --- .../com/squareup/moshi/internal/Util.java | 30 +++++++++++++---- .../java/com/squareup/moshi/TypesTest.java | 33 +++++++++++++++++++ 2 files changed, 57 insertions(+), 6 deletions(-) diff --git a/moshi/src/main/java/com/squareup/moshi/internal/Util.java b/moshi/src/main/java/com/squareup/moshi/internal/Util.java index 1c4986976..2480744f9 100644 --- a/moshi/src/main/java/com/squareup/moshi/internal/Util.java +++ b/moshi/src/main/java/com/squareup/moshi/internal/Util.java @@ -33,7 +33,9 @@ import java.lang.reflect.TypeVariable; import java.lang.reflect.WildcardType; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.NoSuchElementException; import java.util.Set; @@ -169,17 +171,29 @@ public static Type removeSubtypeWildcard(Type type) { } public static Type resolve(Type context, Class contextRawType, Type toResolve) { + return resolve(context, contextRawType, toResolve, new HashSet()); + } + + private static Type resolve(Type context, Class contextRawType, Type toResolve, + Collection visitedTypeVariables) { // This implementation is made a little more complicated in an attempt to avoid object-creation. while (true) { if (toResolve instanceof TypeVariable) { TypeVariable typeVariable = (TypeVariable) toResolve; + if (visitedTypeVariables.contains(typeVariable)) { + // cannot reduce due to infinite recursion + return toResolve; + } else { + visitedTypeVariables.add(typeVariable); + } toResolve = resolveTypeVariable(context, contextRawType, typeVariable); if (toResolve == typeVariable) return toResolve; } else if (toResolve instanceof Class && ((Class) toResolve).isArray()) { Class original = (Class) toResolve; Type componentType = original.getComponentType(); - Type newComponentType = resolve(context, contextRawType, componentType); + Type newComponentType = resolve(context, contextRawType, componentType, + visitedTypeVariables); return componentType == newComponentType ? original : arrayOf(newComponentType); @@ -187,7 +201,8 @@ public static Type resolve(Type context, Class contextRawType, Type toResolve } else if (toResolve instanceof GenericArrayType) { GenericArrayType original = (GenericArrayType) toResolve; Type componentType = original.getGenericComponentType(); - Type newComponentType = resolve(context, contextRawType, componentType); + Type newComponentType = resolve(context, contextRawType, componentType, + visitedTypeVariables); return componentType == newComponentType ? original : arrayOf(newComponentType); @@ -195,12 +210,13 @@ public static Type resolve(Type context, Class contextRawType, Type toResolve } else if (toResolve instanceof ParameterizedType) { ParameterizedType original = (ParameterizedType) toResolve; Type ownerType = original.getOwnerType(); - Type newOwnerType = resolve(context, contextRawType, ownerType); + Type newOwnerType = resolve(context, contextRawType, ownerType, visitedTypeVariables); boolean changed = newOwnerType != ownerType; Type[] args = original.getActualTypeArguments(); for (int t = 0, length = args.length; t < length; t++) { - Type resolvedTypeArgument = resolve(context, contextRawType, args[t]); + Type resolvedTypeArgument = resolve(context, contextRawType, args[t], + visitedTypeVariables); if (resolvedTypeArgument != args[t]) { if (!changed) { args = args.clone(); @@ -220,12 +236,14 @@ public static Type resolve(Type context, Class contextRawType, Type toResolve Type[] originalUpperBound = original.getUpperBounds(); if (originalLowerBound.length == 1) { - Type lowerBound = resolve(context, contextRawType, originalLowerBound[0]); + Type lowerBound = resolve(context, contextRawType, originalLowerBound[0], + visitedTypeVariables); if (lowerBound != originalLowerBound[0]) { return supertypeOf(lowerBound); } } else if (originalUpperBound.length == 1) { - Type upperBound = resolve(context, contextRawType, originalUpperBound[0]); + Type upperBound = resolve(context, contextRawType, originalUpperBound[0], + visitedTypeVariables); if (upperBound != originalUpperBound[0]) { return subtypeOf(upperBound); } diff --git a/moshi/src/test/java/com/squareup/moshi/TypesTest.java b/moshi/src/test/java/com/squareup/moshi/TypesTest.java index b2d492005..ad687535c 100644 --- a/moshi/src/test/java/com/squareup/moshi/TypesTest.java +++ b/moshi/src/test/java/com/squareup/moshi/TypesTest.java @@ -321,6 +321,39 @@ interface StringIntegerMap extends Map { } } + // + // Regression tests for https://github.com/square/moshi/issues/338 + // + // Adapted from https://github.com/google/gson/pull/1128 + // + + private static final class RecursiveTypeVars { + RecursiveTypeVars superType; + } + + @Test public void recursiveTypeVariablesResolve() { + new Moshi.Builder().build().adapter(Types + .newParameterizedTypeWithOwner(TypesTest.class, RecursiveTypeVars.class, String.class)); + } + + @Test public void testRecursiveTypeVariablesResolve1() { + JsonAdapter adapter = new Moshi.Builder().build().adapter(TestType.class); + assertThat(adapter).isNotNull(); + } + + @Test public void testRecursiveTypeVariablesResolve12() { + JsonAdapter adapter = new Moshi.Builder().build().adapter(TestType2.class); + assertThat(adapter).isNotNull(); + } + + private static class TestType { + TestType superType; + } + + private static class TestType2 { + TestType2 superReversedType; + } + @JsonClass(generateAdapter = false) static class TestJsonClass { From 86b5eda69630b384354a987dc46f81122598bb5b Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Sun, 13 Oct 2019 16:25:59 -0400 Subject: [PATCH 3/6] Capture adapter to make checkstyle happy --- moshi/src/test/java/com/squareup/moshi/TypesTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/moshi/src/test/java/com/squareup/moshi/TypesTest.java b/moshi/src/test/java/com/squareup/moshi/TypesTest.java index ad687535c..0d3eb3bb0 100644 --- a/moshi/src/test/java/com/squareup/moshi/TypesTest.java +++ b/moshi/src/test/java/com/squareup/moshi/TypesTest.java @@ -332,8 +332,9 @@ private static final class RecursiveTypeVars { } @Test public void recursiveTypeVariablesResolve() { - new Moshi.Builder().build().adapter(Types + JsonAdapter> adapter = new Moshi.Builder().build().adapter(Types .newParameterizedTypeWithOwner(TypesTest.class, RecursiveTypeVars.class, String.class)); + assertThat(adapter).isNotNull(); } @Test public void testRecursiveTypeVariablesResolve1() { From 37aaaae6d31fceef99b78718f36de92a29dc4399 Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Tue, 15 Oct 2019 17:36:33 -0400 Subject: [PATCH 4/6] Move RecursiveTypesResolveTest out of internal package --- .../moshi/{internal => }/RecursiveTypesResolveTest.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) rename moshi/src/test/java/com/squareup/moshi/{internal => }/RecursiveTypesResolveTest.java (94%) diff --git a/moshi/src/test/java/com/squareup/moshi/internal/RecursiveTypesResolveTest.java b/moshi/src/test/java/com/squareup/moshi/RecursiveTypesResolveTest.java similarity index 94% rename from moshi/src/test/java/com/squareup/moshi/internal/RecursiveTypesResolveTest.java rename to moshi/src/test/java/com/squareup/moshi/RecursiveTypesResolveTest.java index a8add0a9b..bf7e1db16 100644 --- a/moshi/src/test/java/com/squareup/moshi/internal/RecursiveTypesResolveTest.java +++ b/moshi/src/test/java/com/squareup/moshi/RecursiveTypesResolveTest.java @@ -14,11 +14,9 @@ * limitations under the License. */ -package com.squareup.moshi.internal; +package com.squareup.moshi; -import com.squareup.moshi.JsonAdapter; -import com.squareup.moshi.Moshi; -import com.squareup.moshi.Types; +import com.squareup.moshi.internal.Util; import org.junit.Test; import static org.junit.Assert.assertEquals; From dafa839ac77af2df1f3dd20f6e2f4ddc490becee Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Tue, 15 Oct 2019 17:42:54 -0400 Subject: [PATCH 5/6] Use moshi convention for tests --- .../com/squareup/moshi/RecursiveTypesResolveTest.java | 10 +++++----- moshi/src/test/java/com/squareup/moshi/TypesTest.java | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/moshi/src/test/java/com/squareup/moshi/RecursiveTypesResolveTest.java b/moshi/src/test/java/com/squareup/moshi/RecursiveTypesResolveTest.java index bf7e1db16..21f7d091b 100644 --- a/moshi/src/test/java/com/squareup/moshi/RecursiveTypesResolveTest.java +++ b/moshi/src/test/java/com/squareup/moshi/RecursiveTypesResolveTest.java @@ -46,7 +46,7 @@ private static class Foo2 { /** * Test simplest case of recursion. */ - @Test public void testRecursiveResolveSimple() { + @Test public void recursiveResolveSimple() { JsonAdapter adapter = new Moshi.Builder().build().adapter(Foo1.class); assertNotNull(adapter); } @@ -55,22 +55,22 @@ private static class Foo2 { // Tests belows check the behaviour of the methods changed for the fix // - @Test public void testDoubleSupertype() { + @Test public void doubleSupertype() { assertEquals(Types.supertypeOf(Number.class), Types.supertypeOf(Types.supertypeOf(Number.class))); } - @Test public void testDoubleSubtype() { + @Test public void doubleSubtype() { assertEquals(Types.subtypeOf(Number.class), Types.subtypeOf(Types.subtypeOf(Number.class))); } - @Test public void testSuperSubtype() { + @Test public void superSubtype() { assertEquals(Types.subtypeOf(Object.class), Types.supertypeOf(Types.subtypeOf(Number.class))); } - @Test public void testSubSupertype() { + @Test public void subSupertype() { assertEquals(Types.subtypeOf(Object.class), Types.subtypeOf(Types.supertypeOf(Number.class))); } diff --git a/moshi/src/test/java/com/squareup/moshi/TypesTest.java b/moshi/src/test/java/com/squareup/moshi/TypesTest.java index 0d3eb3bb0..2c46f5ad6 100644 --- a/moshi/src/test/java/com/squareup/moshi/TypesTest.java +++ b/moshi/src/test/java/com/squareup/moshi/TypesTest.java @@ -337,12 +337,12 @@ private static final class RecursiveTypeVars { assertThat(adapter).isNotNull(); } - @Test public void testRecursiveTypeVariablesResolve1() { + @Test public void recursiveTypeVariablesResolve1() { JsonAdapter adapter = new Moshi.Builder().build().adapter(TestType.class); assertThat(adapter).isNotNull(); } - @Test public void testRecursiveTypeVariablesResolve12() { + @Test public void recursiveTypeVariablesResolve12() { JsonAdapter adapter = new Moshi.Builder().build().adapter(TestType2.class); assertThat(adapter).isNotNull(); } From 5136b6e8c55ca49037885c686526348681d20475 Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Tue, 15 Oct 2019 17:43:08 -0400 Subject: [PATCH 6/6] 2 is not 1 --- moshi/src/test/java/com/squareup/moshi/TypesTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/moshi/src/test/java/com/squareup/moshi/TypesTest.java b/moshi/src/test/java/com/squareup/moshi/TypesTest.java index 2c46f5ad6..e1419ba11 100644 --- a/moshi/src/test/java/com/squareup/moshi/TypesTest.java +++ b/moshi/src/test/java/com/squareup/moshi/TypesTest.java @@ -342,7 +342,7 @@ private static final class RecursiveTypeVars { assertThat(adapter).isNotNull(); } - @Test public void recursiveTypeVariablesResolve12() { + @Test public void recursiveTypeVariablesResolve2() { JsonAdapter adapter = new Moshi.Builder().build().adapter(TestType2.class); assertThat(adapter).isNotNull(); }