From 815e2e5b2d3daf3ae452f0e742552e356c23911c Mon Sep 17 00:00:00 2001
From: Michael Dowling <mtdowling@gmail.com>
Date: Fri, 24 Feb 2023 13:28:43 -0800
Subject: [PATCH] Fix NodeMapper handling of lists with generic types

We previously deserialized lists with generic types incorrectly,
causing the list to be created, but storing invalid types in the list.
This caused the use of the list at runtime to fail due to a
ClassCastException.
---
 .../model/node/DefaultNodeDeserializers.java  |  7 +++
 .../smithy/model/node/NodeMapperTest.java     | 55 +++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeDeserializers.java b/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeDeserializers.java
index 8e6261894b6..553213f9515 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeDeserializers.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeDeserializers.java
@@ -186,6 +186,13 @@ public NodeMapper.ObjectCreator getCreator(NodeType nodeType, Type target, NodeM
 
                 // Extract out the expected generic type of the collection.
                 Type memberType = Object.class;
+
+                // If given a Class, then attempt to find the generic superclass (e.g., extending ArrayList with a
+                // concrete generic type).
+                if (targetType instanceof Class) {
+                    targetType = ((Class<?>) target).getGenericSuperclass();
+                }
+
                 if (targetType instanceof ParameterizedType) {
                     Type[] genericTypes = ((ParameterizedType) targetType).getActualTypeArguments();
                     if (genericTypes.length > 0) {
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java
index 76eebb21ce7..d1d15729b10 100644
--- a/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java
+++ b/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java
@@ -5,8 +5,10 @@
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasKey;
+import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.hasValue;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.Matchers.startsWith;
@@ -940,6 +942,59 @@ public static final class NonZeroArgConstructorCollection extends ArrayList<Stri
         public NonZeroArgConstructorCollection(String notGonnaWork) {}
     }
 
+    // We previously reported this kind of deserialization as working, but it actually was storing objects of the
+    // wrong type in collections, causing ClassCastExceptions at runtime.
+    @Test
+    public void deserializesIntoTypedList() {
+        Node value = Node.parse("[{\"a\": true}, {\"a\": false}]");
+        NodeMapper mapper = new NodeMapper();
+
+        FooList result = mapper.deserialize(value, FooList.class);
+
+        assertThat(result, hasSize(2));
+        assertThat(result.get(0), instanceOf(FooList.Foo.class));
+        assertThat(result.get(0).a(), is(true));
+        assertThat(result.get(1), instanceOf(FooList.Foo.class));
+        assertThat(result.get(1).a(), is(false));
+    }
+
+    public static final class FooList extends ArrayList<FooList.Foo> {
+        public static final class Foo {
+            boolean a;
+
+            public boolean a() {
+                return a;
+            }
+
+            public void a(boolean a) {
+                this.a = a;
+            }
+        }
+    }
+
+    // Throw some nested generics at the NodeMapper to ensure it works with them.
+    @Test
+    public void deserializesIntoTypedListWithNestedGenerics() {
+        Node value = Node.parse("[ [[{\"a\": true}]], [[{\"a\": false}]] ]");
+        NodeMapper mapper = new NodeMapper();
+        NestedFooList result = mapper.deserialize(value, NestedFooList.class);
+
+        assertThat(result, hasSize(2));
+        assertThat(result.get(0), instanceOf(ArrayList.class));
+        assertThat(result.get(0), hasSize(1));
+        assertThat(result.get(0).get(0), hasSize(1));
+        assertThat(result.get(0).get(0), instanceOf(FooList.class));
+        assertThat(result.get(0).get(0).get(0), instanceOf(FooList.Foo.class));
+
+        assertThat(result.get(1), instanceOf(ArrayList.class));
+        assertThat(result.get(1), hasSize(1));
+        assertThat(result.get(1).get(0), hasSize(1));
+        assertThat(result.get(1).get(0), instanceOf(FooList.class));
+        assertThat(result.get(1).get(0).get(0), instanceOf(FooList.Foo.class));
+    }
+
+    public static final class NestedFooList extends ArrayList<ArrayList<FooList>> {}
+
     @Test
     public void deserializedIntoMap() {
         Node heterogeneous = Node.parse("{\"foo\":\"foo\",\"baz\":10}");