Update shape builders to update member ids

This updates the shape builders to update member ids if the shape id changes. This prevents a situation where changing the id would put the shape into an invalid state. It also makes it extremely simple to create a duplicate shape.
smithy-lang · Sep 9, 2020 · 29497c2 · 29497c2
1 parent 5c91efc
commit 29497c2
Showing 9 changed files with 120 additions and 1 deletion.
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/AbstractShapeBuilder.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/AbstractShapeBuilder.java
@@ -68,7 +68,7 @@ public ShapeId getId() {
      * @return Returns the builder.
      */
     @SuppressWarnings("unchecked")
-    public final B id(ShapeId shapeId) {
+    public B id(ShapeId shapeId) {
         id = shapeId;
         return (B) this;
     }

diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/CollectionShape.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/CollectionShape.java
@@ -68,6 +68,15 @@ public abstract static class Builder<B extends Builder, S extends CollectionShap
 
         private MemberShape member;
 
+        @Override
+        public final B id(ShapeId shapeId) {
+            if (member != null) {
+                // Update the member name so it isn't pointing to the old shape id.
+                member(member.toBuilder().id(shapeId.withMember(member.getMemberName())).build());
+            }
+            return super.id(shapeId);
+        }
+
         /**
          * Sets the member of the collection.
          * @param member Member of the collection to set.

diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/MapShape.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/MapShape.java
@@ -123,6 +123,18 @@ public ShapeType getShapeType() {
             return ShapeType.MAP;
         }
 
+        @Override
+        public Builder id(ShapeId shapeId) {
+            // If the shape id has changed then the key and value member ids also need to be updated.
+            if (key != null) {
+                key(key.toBuilder().id(shapeId.withMember(key.getMemberName())).build());
+            }
+            if (value != null) {
+                value(value.toBuilder().id(shapeId.withMember(value.getMemberName())).build());
+            }
+            return super.id(shapeId);
+        }
+
         public Builder key(MemberShape member) {
             this.key = Objects.requireNonNull(member);
             return this;

diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMembersShape.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMembersShape.java
@@ -113,6 +113,15 @@ abstract static class Builder<B extends Builder, S extends NamedMembersShape> ex
 
         Map<String, MemberShape> members = new LinkedHashMap<>();
 
+        @Override
+        public final B id(ShapeId shapeId) {
+            // If there are already any members set, update their ids to point to the new parent id.
+            for (MemberShape member : members.values()) {
+                addMember(member.toBuilder().id(shapeId.withMember(member.getMemberName())).build());
+            }
+            return super.id(shapeId);
+        }
+
         /**
          * Replaces the members of the builder.
          *

diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/shapes/ListShapeTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/shapes/ListShapeTest.java
@@ -16,6 +16,7 @@
 package software.amazon.smithy.model.shapes;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
@@ -143,4 +144,15 @@ public void returnsMembers() {
 
         assertThat(shape.members(), hasSize(1));
     }
+
+    @Test
+    public void builderUpdatesMemberId() {
+        ListShape shape = ListShape.builder()
+                .id("ns.foo#bar")
+                .member(ShapeId.from("ns.foo#bam"))
+                .id("ns.bar#bar")
+                .build();
+        assertThat(shape.getMember().getId(), equalTo(ShapeId.from("ns.bar#bar$member")));
+        assertThat(shape.getMember().getTarget(), equalTo(ShapeId.from("ns.foo#bam")));
+    }
 }
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/shapes/MapShapeTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/shapes/MapShapeTest.java
@@ -16,6 +16,7 @@
 package software.amazon.smithy.model.shapes;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
@@ -93,4 +94,19 @@ public void returnsMembers() {
 
         assertThat(shape.members(), hasSize(2));
     }
+
+    @Test
+    public void builderUpdatesMembers() {
+        MapShape shape = MapShape.builder()
+                .id("ns.foo#bar")
+                .key(ShapeId.from("ns.foo#bam"))
+                .value(ShapeId.from("ns.foo#bam"))
+                .id("ns.bar#bar")
+                .build();
+
+        assertThat(shape.getKey().getId(), equalTo(ShapeId.from("ns.bar#bar$key")));
+        assertThat(shape.getKey().getTarget(), equalTo(ShapeId.from("ns.foo#bam")));
+        assertThat(shape.getValue().getId(), equalTo(ShapeId.from("ns.bar#bar$value")));
+        assertThat(shape.getValue().getTarget(), equalTo(ShapeId.from("ns.foo#bam")));
+    }
 }
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/shapes/SetShapeTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/shapes/SetShapeTest.java
@@ -0,0 +1,21 @@
+package software.amazon.smithy.model.shapes;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import org.junit.jupiter.api.Test;
+
+public class SetShapeTest {
+
+    @Test
+    public void builderUpdatesMemberId() {
+        SetShape shape = SetShape.builder()
+                .id("ns.foo#bar")
+                .member(ShapeId.from("ns.foo#bam"))
+                .id("ns.bar#bar")
+                .build();
+        assertThat(shape.getMember().getId(), equalTo(ShapeId.from("ns.bar#bar$member")));
+        assertThat(shape.getMember().getTarget(), equalTo(ShapeId.from("ns.foo#bam")));
+    }
+
+}
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/shapes/StructureShapeTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/shapes/StructureShapeTest.java
@@ -98,4 +98,23 @@ public void memberOrderMattersForEqualComparison() {
 
         assertThat(a, not(equalTo(b)));
     }
+
+    @Test
+    public void builderUpdatesMemberIds() {
+        StructureShape original = StructureShape.builder()
+                .id("ns.foo#bar")
+                .addMember("foo", ShapeId.from("ns.foo#bam"))
+                .addMember("baz", ShapeId.from("ns.foo#bam"))
+                .build();
+
+        StructureShape actual = original.toBuilder().id(ShapeId.from("ns.bar#bar")).build();
+
+        StructureShape expected = StructureShape.builder()
+                .id("ns.bar#bar")
+                .addMember("foo", ShapeId.from("ns.foo#bam"))
+                .addMember("baz", ShapeId.from("ns.foo#bam"))
+                .build();
+
+        assertThat(actual, equalTo(expected));
+    }
 }
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/shapes/UnionShapeTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/shapes/UnionShapeTest.java
@@ -15,6 +15,8 @@
 
 package software.amazon.smithy.model.shapes;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 import org.junit.jupiter.api.Assertions;
@@ -41,4 +43,23 @@ public void mustNotContainMembersInShapeId() {
             UnionShape.builder().id("ns.foo#bar$baz").build();
         });
     }
+
+    @Test
+    public void builderUpdatesMemberIds() {
+        UnionShape original = UnionShape.builder()
+                .id("ns.foo#bar")
+                .addMember("foo", ShapeId.from("ns.foo#bam"))
+                .addMember("baz", ShapeId.from("ns.foo#bam"))
+                .build();
+
+        UnionShape actual = original.toBuilder().id(ShapeId.from("ns.bar#bar")).build();
+
+        UnionShape expected = UnionShape.builder()
+                .id("ns.bar#bar")
+                .addMember("foo", ShapeId.from("ns.foo#bam"))
+                .addMember("baz", ShapeId.from("ns.foo#bam"))
+                .build();
+
+        assertThat(actual, equalTo(expected));
+    }
 }