From 72b32952d70ddaad96b709fd5ab7bac35a0f7769 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Fri, 22 Jul 2022 12:25:25 -0700 Subject: [PATCH 1/4] Fix some deprecations in 2.0 --- .../aws/iam/traits/ConditionKeysIndexTest.java | 2 +- .../aws/traits/HttpChecksumTraitValidator.java | 1 + .../cli/commands/Upgrade1to2Command.java | 1 + .../jsonschema/JsonSchemaConverterTest.java | 2 ++ .../smithy/model/knowledge/NullableIndex.java | 7 +++++++ .../smithy/model/loader/ModelUpgrader.java | 7 ++++--- .../traits/HttpMessageTestCase.java | 2 +- ...ameterizedHttpMalformedRequestTestCase.java | 18 +++++++++++------- 8 files changed, 28 insertions(+), 12 deletions(-) diff --git a/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/ConditionKeysIndexTest.java b/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/ConditionKeysIndexTest.java index 7f7070cbe61..d649fe1baf2 100644 --- a/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/ConditionKeysIndexTest.java +++ b/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/ConditionKeysIndexTest.java @@ -75,7 +75,7 @@ public void detectsUnknownConditionKeys() { assertTrue(result.isBroken()); assertThat(result.getValidationEvents(Severity.ERROR).stream() - .map(ValidationEvent::getEventId) + .map(ValidationEvent::getId) .collect(Collectors.toSet()), contains("ConditionKeys")); } diff --git a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java index bac173704bc..f8a87f64c55 100644 --- a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java +++ b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java @@ -130,6 +130,7 @@ private Optional validateAlgorithmMember( return Optional.empty(); } + @SuppressWarnings("deprecation") private Optional validateEnumMember( Model model, HttpChecksumTrait trait, diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Upgrade1to2Command.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Upgrade1to2Command.java index 558cf5bfb99..d8a5a2d436e 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Upgrade1to2Command.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Upgrade1to2Command.java @@ -66,6 +66,7 @@ import software.amazon.smithy.utils.StringUtils; @SmithyInternalApi +@SuppressWarnings("deprecation") public final class Upgrade1to2Command extends SimpleCommand { private static final Logger LOGGER = Logger.getLogger(Upgrade1to2Command.class.getName()); private static final Pattern VERSION_1 = Pattern.compile("(?m)^\\s*\\$\\s*version:\\s*\"1\\.0\"\\s*$"); diff --git a/smithy-jsonschema/src/test/java/software/amazon/smithy/jsonschema/JsonSchemaConverterTest.java b/smithy-jsonschema/src/test/java/software/amazon/smithy/jsonschema/JsonSchemaConverterTest.java index 0e512c98c2c..c24d0942140 100644 --- a/smithy-jsonschema/src/test/java/software/amazon/smithy/jsonschema/JsonSchemaConverterTest.java +++ b/smithy-jsonschema/src/test/java/software/amazon/smithy/jsonschema/JsonSchemaConverterTest.java @@ -308,6 +308,7 @@ public void supportsStringLengthTrait() { } @Test + @SuppressWarnings("deprecation") public void supportsListAndSetLengthTrait() { StringShape string = StringShape.builder().id("smithy.api#String").build(); MemberShape member = MemberShape.builder() @@ -428,6 +429,7 @@ public void supportsDocumentation() { } @Test + @SuppressWarnings("deprecation") public void supportsEnum() { StringShape string = StringShape.builder() .id("smithy.api#String") diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java b/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java index 534e86a2283..f546251f21f 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java @@ -21,6 +21,7 @@ import software.amazon.smithy.model.shapes.MemberShape; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ToShapeId; +import software.amazon.smithy.model.traits.BoxTrait; import software.amazon.smithy.model.traits.ClientOptionalTrait; import software.amazon.smithy.model.traits.DefaultTrait; import software.amazon.smithy.model.traits.InputTrait; @@ -118,10 +119,16 @@ public boolean isMemberNullable(MemberShape member) { * @param checkMode The mode used when checking if the member is considered nullable. * @return Returns true if the member is optional. */ + @SuppressWarnings("deprecation") public boolean isMemberNullable(MemberShape member, CheckMode checkMode) { Model m = Objects.requireNonNull(model.get()); Shape container = m.expectShape(member.getContainer()); + // Support box trait for 1.0 loaded models that weren't converted to 2.0. + if (member.hasTrait(BoxTrait.class)) { + return true; + } + // Client mode honors the nullable and input trait. if (checkMode == CheckMode.CLIENT && (member.hasTrait(ClientOptionalTrait.class) || container.hasTrait(InputTrait.class))) { diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelUpgrader.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelUpgrader.java index a2bff3cf756..adbf7e02d98 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelUpgrader.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelUpgrader.java @@ -98,9 +98,8 @@ ValidatedResult transform() { // Also attempt to upgrade unknown versions since they could be 1.0 and // trying to upgrade 2.0 shapes has no effect. // For v1 shape checks, we need to know the containing shape type to apply the appropriate transform. - model.getShape(member.getContainer()).ifPresent(container -> { - upgradeV1Member(container.getType(), member); - }); + model.getShape(member.getContainer()) + .ifPresent(container -> upgradeV1Member(container.getType(), member)); } } @@ -182,6 +181,7 @@ private boolean isZeroValidDefault(MemberShape member) { return true; } + @SuppressWarnings("deprecation") private boolean shouldV1MemberHaveDefaultTrait(ShapeType containerType, MemberShape member, Shape target) { return containerType == ShapeType.STRUCTURE // Only when the targeted shape had a default value by default in v1 or if @@ -210,6 +210,7 @@ private MemberShape.Builder createOrReuseBuilder(MemberShape member, MemberShape return builder == null ? member.toBuilder() : builder; } + @SuppressWarnings("deprecation") private void validateV2Member(MemberShape member) { if (REMOVED_PRIMITIVE_SHAPES.containsKey(member.getTarget())) { emitWhenTargetingRemovedPreludeShape(Severity.ERROR, member); diff --git a/smithy-protocol-test-traits/src/main/java/software/amazon/smithy/protocoltests/traits/HttpMessageTestCase.java b/smithy-protocol-test-traits/src/main/java/software/amazon/smithy/protocoltests/traits/HttpMessageTestCase.java index fb1013302b4..5dd5264e9c9 100644 --- a/smithy-protocol-test-traits/src/main/java/software/amazon/smithy/protocoltests/traits/HttpMessageTestCase.java +++ b/smithy-protocol-test-traits/src/main/java/software/amazon/smithy/protocoltests/traits/HttpMessageTestCase.java @@ -242,7 +242,7 @@ void updateBuilder(Builder builder) { .appliesTo(appliesTo); } - abstract static class Builder implements SmithyBuilder { + abstract static class Builder, T extends HttpMessageTestCase> implements SmithyBuilder { private String id; private String documentation; diff --git a/smithy-protocol-test-traits/src/main/java/software/amazon/smithy/protocoltests/traits/ParameterizedHttpMalformedRequestTestCase.java b/smithy-protocol-test-traits/src/main/java/software/amazon/smithy/protocoltests/traits/ParameterizedHttpMalformedRequestTestCase.java index d6407ec08cd..0ed2e0d0f09 100644 --- a/smithy-protocol-test-traits/src/main/java/software/amazon/smithy/protocoltests/traits/ParameterizedHttpMalformedRequestTestCase.java +++ b/smithy-protocol-test-traits/src/main/java/software/amazon/smithy/protocoltests/traits/ParameterizedHttpMalformedRequestTestCase.java @@ -28,9 +28,9 @@ import software.amazon.smithy.model.node.ToNode; import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.shapes.ToShapeId; -import software.amazon.smithy.utils.CodeWriter; import software.amazon.smithy.utils.ListUtils; import software.amazon.smithy.utils.MapUtils; +import software.amazon.smithy.utils.SimpleCodeWriter; import software.amazon.smithy.utils.SmithyBuilder; import software.amazon.smithy.utils.SmithyUnstableApi; import software.amazon.smithy.utils.Tagged; @@ -117,7 +117,7 @@ public List generateTestCasesFromParameters() { .orElseThrow(IllegalStateException::new); final List testCases = new ArrayList<>(paramLength); for (int i = 0; i < paramLength; i++) { - final CodeWriter writer = new CodeWriter(); + final SimpleCodeWriter writer = new SimpleCodeWriter(); for (Map.Entry> e : testParameters.entrySet()) { writer.putContext(e.getKey(), e.getValue().get(i)); } @@ -135,8 +135,10 @@ public List generateTestCasesFromParameters() { return testCases; } - private static HttpMalformedResponseDefinition interpolateResponse(HttpMalformedResponseDefinition response, - CodeWriter writer) { + private static HttpMalformedResponseDefinition interpolateResponse( + HttpMalformedResponseDefinition response, + SimpleCodeWriter writer + ) { HttpMalformedResponseDefinition.Builder responseBuilder = response.toBuilder().headers(formatHeaders(writer, response.getHeaders())); response.getBody() @@ -151,8 +153,10 @@ private static HttpMalformedResponseDefinition interpolateResponse(HttpMalformed return responseBuilder.build(); } - private static HttpMalformedRequestDefinition interpolateRequest(HttpMalformedRequestDefinition request, - CodeWriter writer) { + private static HttpMalformedRequestDefinition interpolateRequest( + HttpMalformedRequestDefinition request, + SimpleCodeWriter writer + ) { HttpMalformedRequestDefinition.Builder requestBuilder = request.toBuilder() .headers(formatHeaders(writer, request.getHeaders())) .queryParams(request.getQueryParams().stream().map(writer::format).collect(Collectors.toList())); @@ -161,7 +165,7 @@ private static HttpMalformedRequestDefinition interpolateRequest(HttpMalformedRe return requestBuilder.build(); } - private static Map formatHeaders(CodeWriter writer, Map headers) { + private static Map formatHeaders(SimpleCodeWriter writer, Map headers) { Map newHeaders = new HashMap<>(); for (Map.Entry headerEntry : headers.entrySet()) { newHeaders.put(writer.format(headerEntry.getKey()), writer.format(headerEntry.getValue())); From 97843638d74e5239eeb0615361f5f4f732fbe513 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Fri, 22 Jul 2022 12:55:44 -0700 Subject: [PATCH 2/4] Account for removed prelude shapes in NullableIndex This commit adds a bit more 1.0 support to NullableIndex to account for shapes that weren't converted to Smithy 2.0 but still target a removed primitive prelude shape that implies a member is non-nullable by default. --- .../smithy/model/knowledge/NullableIndex.java | 49 +++++++++++++------ .../model/knowledge/NullableIndexTest.java | 17 +++++++ 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java b/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java index f546251f21f..2a4fc2af12c 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java @@ -17,22 +17,40 @@ import java.lang.ref.WeakReference; import java.util.Objects; +import java.util.Set; import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.loader.ModelAssembler; import software.amazon.smithy.model.shapes.MemberShape; import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.shapes.ToShapeId; -import software.amazon.smithy.model.traits.BoxTrait; import software.amazon.smithy.model.traits.ClientOptionalTrait; import software.amazon.smithy.model.traits.DefaultTrait; import software.amazon.smithy.model.traits.InputTrait; import software.amazon.smithy.model.traits.RequiredTrait; import software.amazon.smithy.model.traits.SparseTrait; +import software.amazon.smithy.utils.SetUtils; /** * An index that checks if a member is nullable. + * + *

Note: this index assumes Smithy 2.0 nullability semantics. + * There is basic support for detecting 1.0 models by detecting + * when a removed primitive prelude shape is targeted by a member. + * Beyond that, 1.0 models SHOULD be loaded through a {@link ModelAssembler} + * to upgrade them to IDL 2.0. */ public class NullableIndex implements KnowledgeIndex { + private static final Set REMOVED_PRIMITIVE_SHAPES = SetUtils.of( + ShapeId.from("smithy.api#PrimitiveBoolean"), + ShapeId.from("smithy.api#PrimitiveByte"), + ShapeId.from("smithy.api#PrimitiveShort"), + ShapeId.from("smithy.api#PrimitiveInteger"), + ShapeId.from("smithy.api#PrimitiveLong"), + ShapeId.from("smithy.api#PrimitiveFloat"), + ShapeId.from("smithy.api#PrimitiveDouble")); + private final WeakReference model; public NullableIndex(Model model) { @@ -119,26 +137,27 @@ public boolean isMemberNullable(MemberShape member) { * @param checkMode The mode used when checking if the member is considered nullable. * @return Returns true if the member is optional. */ - @SuppressWarnings("deprecation") public boolean isMemberNullable(MemberShape member, CheckMode checkMode) { Model m = Objects.requireNonNull(model.get()); Shape container = m.expectShape(member.getContainer()); - // Support box trait for 1.0 loaded models that weren't converted to 2.0. - if (member.hasTrait(BoxTrait.class)) { - return true; - } - - // Client mode honors the nullable and input trait. - if (checkMode == CheckMode.CLIENT - && (member.hasTrait(ClientOptionalTrait.class) || container.hasTrait(InputTrait.class))) { - return true; - } - switch (container.getType()) { case STRUCTURE: - // Structure members are nullable by default; non-null when marked as @default / @required. - return !member.hasTrait(DefaultTrait.class) && !member.hasTrait(RequiredTrait.class); + // Client mode honors the nullable and input trait. + if (checkMode == CheckMode.CLIENT + && (member.hasTrait(ClientOptionalTrait.class) || container.hasTrait(InputTrait.class))) { + return true; + } + + // Structure members that are @required or @default are not nullable. + if (member.hasTrait(DefaultTrait.class) || member.hasTrait(RequiredTrait.class)) { + return false; + } + + // Detect if the member targets a 1.0 primitive prelude shape and the shape wasn't upgraded. + // These removed prelude shapes are impossible to appear in a 2.0 model, so it's safe to + // detect them and honor 1.0 semantics here. + return !REMOVED_PRIMITIVE_SHAPES.contains(member.getTarget()); case UNION: case SET: // Union and set members are never null. diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/NullableIndexTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/NullableIndexTest.java index ce3dea604dd..401690eb034 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/NullableIndexTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/NullableIndexTest.java @@ -22,11 +22,13 @@ import java.util.Collection; import java.util.stream.Stream; import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.node.Node; +import software.amazon.smithy.model.shapes.IntegerShape; import software.amazon.smithy.model.shapes.ListShape; import software.amazon.smithy.model.shapes.MapShape; import software.amazon.smithy.model.shapes.SetShape; @@ -228,4 +230,19 @@ public static Stream inputTraitTests() { Arguments.of(NullableIndex.CheckMode.SERVER, false, false, true) ); } + + @Test + public void detectsPreviousPrimitivePreludeShapes() { + IntegerShape integer = IntegerShape.builder() + .id("smithy.api#PrimitiveInteger") + .build(); + StructureShape struct = StructureShape.builder() + .id("smithy.example#Struct") + .addMember("foo", integer.getId()) + .build(); + Model model = Model.builder().addShapes(integer, struct).build(); + NullableIndex index = NullableIndex.of(model); + + assertThat(index.isMemberNullable(struct.getMember("foo").get()), is(false)); + } } From b278e18c0f23c190ec47c58c4a37f679c24169f1 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Fri, 22 Jul 2022 14:00:22 -0700 Subject: [PATCH 3/4] Add v1 nullability resolver Add a nullability resolver that uses only v1 model semantics to account for manually created models that didn't pass through the upgrade functionality of a ModelAssembler. Needing to use this method should be very rare, but adding it in case as a kind of utility method. --- .../smithy/model/knowledge/NullableIndex.java | 68 ++++++++++++++- .../model/knowledge/NullableIndexTest.java | 82 +++++++++++++++++++ 2 files changed, 146 insertions(+), 4 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java b/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java index 2a4fc2af12c..fbf67d888c8 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java @@ -23,7 +23,9 @@ import software.amazon.smithy.model.shapes.MemberShape; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.model.shapes.ShapeType; import software.amazon.smithy.model.shapes.ToShapeId; +import software.amazon.smithy.model.traits.BoxTrait; import software.amazon.smithy.model.traits.ClientOptionalTrait; import software.amazon.smithy.model.traits.DefaultTrait; import software.amazon.smithy.model.traits.InputTrait; @@ -42,7 +44,7 @@ */ public class NullableIndex implements KnowledgeIndex { - private static final Set REMOVED_PRIMITIVE_SHAPES = SetUtils.of( + private static final Set V1_REMOVED_PRIMITIVE_SHAPES = SetUtils.of( ShapeId.from("smithy.api#PrimitiveBoolean"), ShapeId.from("smithy.api#PrimitiveByte"), ShapeId.from("smithy.api#PrimitiveShort"), @@ -51,6 +53,19 @@ public class NullableIndex implements KnowledgeIndex { ShapeId.from("smithy.api#PrimitiveFloat"), ShapeId.from("smithy.api#PrimitiveDouble")); + private static final Set V1_INHERENTLY_BOXED = SetUtils.of( + ShapeType.STRING, + ShapeType.BLOB, + ShapeType.TIMESTAMP, + ShapeType.BIG_DECIMAL, + ShapeType.BIG_INTEGER, + ShapeType.LIST, + ShapeType.SET, + ShapeType.MAP, + ShapeType.STRUCTURE, + ShapeType.UNION, + ShapeType.DOCUMENT); + private final WeakReference model; public NullableIndex(Model model) { @@ -125,7 +140,7 @@ public boolean isMemberNullable(MemberShape member) { } /** - * Checks if a member is nullable. + * Checks if a member is nullable using v2 nullability rules. * *

A {@code checkMode} parameter is required to declare what kind of * model consumer is checking if the member is optional. The authoritative @@ -133,6 +148,12 @@ public boolean isMemberNullable(MemberShape member) { * {@link ClientOptionalTrait}, while non-authoritative consumers like clients * must honor these traits. * + *

This method will also attempt to detect when a member targets a + * primitive prelude shape that was removed in Smithy IDL 2.0 to account + * for models that were created manually without passing through a + * ModelAssembler. If a member targets a removed primitive prelude shape, + * the member is considered non-null. + * * @param member Member to check. * @param checkMode The mode used when checking if the member is considered nullable. * @return Returns true if the member is optional. @@ -145,7 +166,7 @@ public boolean isMemberNullable(MemberShape member, CheckMode checkMode) { case STRUCTURE: // Client mode honors the nullable and input trait. if (checkMode == CheckMode.CLIENT - && (member.hasTrait(ClientOptionalTrait.class) || container.hasTrait(InputTrait.class))) { + && (member.hasTrait(ClientOptionalTrait.class) || container.hasTrait(InputTrait.class))) { return true; } @@ -157,7 +178,7 @@ public boolean isMemberNullable(MemberShape member, CheckMode checkMode) { // Detect if the member targets a 1.0 primitive prelude shape and the shape wasn't upgraded. // These removed prelude shapes are impossible to appear in a 2.0 model, so it's safe to // detect them and honor 1.0 semantics here. - return !REMOVED_PRIMITIVE_SHAPES.contains(member.getTarget()); + return !V1_REMOVED_PRIMITIVE_SHAPES.contains(member.getTarget()); case UNION: case SET: // Union and set members are never null. @@ -175,4 +196,43 @@ public boolean isMemberNullable(MemberShape member, CheckMode checkMode) { return false; } } + + /** + * Checks if a member is nullable using v1 nullability rules. + * + *

This method matches the previous behavior seen in NullableIndex prior + * to Smithy 1.0. Most models are sent through a ModelAssembler which makes + * using the normal {@link #isMemberNullable(MemberShape)} the best choice. + * However, in some cases, a model might get created directly in code + * using Smithy 1.0 semantics. In those cases, this method can be used to + * detect if the member is nullable or not. + * + *

This method ignores the default trait, clientOptional trait, + * input trait, and required trait. + * + * @param member Member to check. + * @return Returns true if the member is nullable using 1.0 resolution rules. + */ + public boolean isMemberNullableInV1(MemberShape member) { + Model m = Objects.requireNonNull(model.get()); + Shape container = m.getShape(member.getContainer()).orElse(null); + Shape target = m.getShape(member.getTarget()).orElse(null); + + // Ignore broken models in this index. Other validators handle these checks. + if (container == null || target == null) { + return false; + } + + // Defer to 2.0 checks for shapes that aren't structures, since the logic is the same. + if (container.getType() != ShapeType.STRUCTURE) { + return isMemberNullable(member); + } + + // Check if the member or the target has the box trait. + if (member.getMemberTrait(m, BoxTrait.class).isPresent()) { + return true; + } + + return V1_INHERENTLY_BOXED.contains(target.getType()); + } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/NullableIndexTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/NullableIndexTest.java index 401690eb034..dbc6c8a8bf1 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/NullableIndexTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/NullableIndexTest.java @@ -245,4 +245,86 @@ public void detectsPreviousPrimitivePreludeShapes() { assertThat(index.isMemberNullable(struct.getMember("foo").get()), is(false)); } + + @Test + public void worksWithV1NullabilityRulesForInteger() { + // In Smithy v1, integer was non-nullable by default. + IntegerShape integer = IntegerShape.builder() + .id("smithy.example#Integer") + .build(); + StructureShape struct = StructureShape.builder() + .id("smithy.example#Struct") + .addMember("foo", integer.getId()) + .build(); + Model model = Model.builder().addShapes(integer, struct).build(); + NullableIndex index = NullableIndex.of(model); + + assertThat(index.isMemberNullableInV1(struct.getMember("foo").get()), is(false)); + } + + @Test + public void worksWithV1NullabilityRulesForString() { + StringShape string = StringShape.builder() + .id("smithy.example#String") + .build(); + StructureShape struct = StructureShape.builder() + .id("smithy.example#Struct") + .addMember("foo", string.getId()) + .build(); + Model model = Model.builder().addShapes(string, struct).build(); + NullableIndex index = NullableIndex.of(model); + + assertThat(index.isMemberNullableInV1(struct.getMember("foo").get()), is(true)); + } + + @Test + @SuppressWarnings("deprecation") + public void worksWithV1NullabilityRulesForBoxedMember() { + IntegerShape integer = IntegerShape.builder() + .id("smithy.example#Integer") + .build(); + StructureShape struct = StructureShape.builder() + .id("smithy.example#Struct") + .addMember("foo", integer.getId(), b -> b.addTrait(new BoxTrait())) + .build(); + Model model = Model.builder().addShapes(integer, struct).build(); + NullableIndex index = NullableIndex.of(model); + + assertThat(index.isMemberNullableInV1(struct.getMember("foo").get()), is(true)); + } + + @Test + @SuppressWarnings("deprecation") + public void worksWithV1NullabilityRulesForBoxedTarget() { + IntegerShape integer = IntegerShape.builder() + .id("smithy.example#Integer") + .addTrait(new BoxTrait()) + .build(); + StructureShape struct = StructureShape.builder() + .id("smithy.example#Struct") + .addMember("foo", integer.getId()) + .build(); + Model model = Model.builder().addShapes(integer, struct).build(); + NullableIndex index = NullableIndex.of(model); + + assertThat(index.isMemberNullableInV1(struct.getMember("foo").get()), is(true)); + } + + @Test + @SuppressWarnings("deprecation") + public void worksWithV1NullabilityRulesIgnoringRequired() { + IntegerShape integer = IntegerShape.builder() + .id("smithy.example#Integer") + .addTrait(new BoxTrait()) + .build(); + StructureShape struct = StructureShape.builder() + .id("smithy.example#Struct") + // The required trait isn't used in v1 to determine nullability. + .addMember("foo", integer.getId(), b -> b.addTrait(new RequiredTrait())) + .build(); + Model model = Model.builder().addShapes(integer, struct).build(); + NullableIndex index = NullableIndex.of(model); + + assertThat(index.isMemberNullableInV1(struct.getMember("foo").get()), is(true)); + } } From 45c42cf843d07a5cc08bb98b9a3c06ed91297aeb Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Fri, 22 Jul 2022 16:47:00 -0700 Subject: [PATCH 4/4] Refactor hierarchy of shapes in IDL 2 Rather than expose a NamedMembers interface, I think it makes more sense to just move all member accessors to Shape. This gets rid of a lot of cruft when trying to deal with different kinds of shapes (for example, if you want to write something that works with both structures and unions). Furthermore, structurallyExclusive only impacts structure shapes. This fixes an issue where it was implemented to also check enums, intEnums, and unions. --- .../smithy/model/FromSourceLocation.java | 2 +- .../smithy/model/shapes/CollectionShape.java | 31 +-- .../amazon/smithy/model/shapes/EnumShape.java | 50 +--- .../smithy/model/shapes/IntEnumShape.java | 49 +--- .../amazon/smithy/model/shapes/MapShape.java | 81 ++++-- .../smithy/model/shapes/NamedMembers.java | 46 ---- .../model/shapes/NamedMembersShape.java | 230 ------------------ .../shapes/NamedMembersShapeBuilder.java | 154 ++++++++++++ .../amazon/smithy/model/shapes/Shape.java | 45 +++- .../shapes/SmithyIdlModelSerializer.java | 2 +- .../smithy/model/shapes/StructureShape.java | 19 +- .../smithy/model/shapes/UnionShape.java | 19 +- ...xclusiveStructureMemberTraitValidator.java | 9 +- 13 files changed, 306 insertions(+), 431 deletions(-) delete mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMembers.java delete mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMembersShape.java create mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMembersShapeBuilder.java diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/FromSourceLocation.java b/smithy-model/src/main/java/software/amazon/smithy/model/FromSourceLocation.java index 9842662c074..2813f9a5f04 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/FromSourceLocation.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/FromSourceLocation.java @@ -34,7 +34,7 @@ default SourceLocation getSourceLocation() { * * @param s1 the first FromSourceLocation to compare. * @param s2 the second FromSourceLocation to compare. - * @return the value 0 if s1 == s2; a value less than 0 if s1 < s2; and a value greater than 0 if s1 > s2. + * @return the value 0 if s1 == s2; a value less than 0 if s1 < s2; and a value greater than 0 if s1 > s2. */ static int compare(FromSourceLocation s1, FromSourceLocation s2) { return s1.getSourceLocation().compareTo(s2.getSourceLocation()); 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 index 622dd2b09f4..c8d37c25acb 100644 --- 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 @@ -17,8 +17,8 @@ import java.util.Collection; import java.util.Collections; +import java.util.Map; import java.util.Objects; -import java.util.Optional; import java.util.function.Consumer; import software.amazon.smithy.model.SourceLocation; import software.amazon.smithy.model.traits.Trait; @@ -29,10 +29,18 @@ public abstract class CollectionShape extends Shape { private final MemberShape member; + private transient Map memberMap; CollectionShape(Builder builder) { super(builder, false); - member = builder.member != null ? builder.member : getRequiredMixinMember(builder, "member"); + + // Get the member from the builder or from any mixins. + if (builder.member != null) { + member = builder.member; + } else { + member = getRequiredMixinMember(builder, "member"); + } + ShapeId expected = getId().withMember("member"); if (!member.getId().equals(expected)) { throw new IllegalArgumentException(String.format( @@ -51,18 +59,13 @@ public final MemberShape getMember() { } @Override - public final Optional getMember(String name) { - return name.equals("member") ? Optional.of(member) : Optional.empty(); - } - - @Override - public Collection members() { - return Collections.singletonList(member); - } - - @Override - public boolean equals(Object other) { - return super.equals(other) && getMember().equals(((CollectionShape) other).getMember()); + public final Map getAllMembers() { + Map members = memberMap; + if (members == null) { + members = Collections.singletonMap("member", member); + memberMap = members; + } + return members; } /** diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/EnumShape.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/EnumShape.java index 03a4726b50f..ceebb4c2548 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/EnumShape.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/EnumShape.java @@ -18,7 +18,6 @@ import java.util.Collection; import java.util.Collections; import java.util.LinkedHashMap; -import java.util.List; import java.util.Map; import java.util.Optional; import java.util.function.Consumer; @@ -35,15 +34,13 @@ import software.amazon.smithy.model.traits.UnitTypeTrait; import software.amazon.smithy.model.traits.synthetic.SyntheticEnumTrait; import software.amazon.smithy.utils.BuilderRef; -import software.amazon.smithy.utils.ListUtils; -public final class EnumShape extends StringShape implements NamedMembers { +public final class EnumShape extends StringShape { private static final Pattern CONVERTABLE_VALUE = Pattern.compile("^[a-zA-Z-_.:/\\s]+[a-zA-Z_0-9-.:/\\s]*$"); private static final Logger LOGGER = Logger.getLogger(EnumShape.class.getName()); private final Map members; - private volatile List memberNames; private volatile Map enumValues; private EnumShape(Builder builder) { @@ -82,35 +79,8 @@ public Map getAllMembers() { return members; } - /** - * Returns an ordered list of member names based on the order they are - * defined in the model, including mixin members. - * - * @return Returns an immutable list of member names. - */ - @Override - public List getMemberNames() { - List names = memberNames; - if (names == null) { - names = ListUtils.copyOf(members.keySet()); - memberNames = names; - } - - return names; - } - - @Override - public boolean equals(Object other) { - if (!super.equals(other)) { - return false; - } - - // Members are ordered, so do a test on the ordering and their values. - EnumShape b = (EnumShape) other; - return getMemberNames().equals(b.getMemberNames()) && members.equals(b.members); - } - @Override + @SuppressWarnings("deprecation") public Optional findTrait(ShapeId id) { if (id.equals(EnumTrait.ID)) { return super.findTrait(SyntheticEnumTrait.ID); @@ -118,22 +88,6 @@ public Optional findTrait(ShapeId id) { return super.findTrait(id); } - /** - * Get a specific member by name. - * - * @param name Name of the member to retrieve. - * @return Returns the optional member. - */ - @Override - public Optional getMember(String name) { - return Optional.ofNullable(members.get(name)); - } - - @Override - public Collection members() { - return members.values(); - } - public static Builder builder() { return new Builder(); } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/IntEnumShape.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/IntEnumShape.java index 23987168c8b..add8d6d3520 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/IntEnumShape.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/IntEnumShape.java @@ -18,7 +18,6 @@ import java.util.Collection; import java.util.Collections; import java.util.LinkedHashMap; -import java.util.List; import java.util.Map; import java.util.Optional; import java.util.function.Consumer; @@ -26,12 +25,10 @@ import software.amazon.smithy.model.traits.EnumValueTrait; import software.amazon.smithy.model.traits.UnitTypeTrait; import software.amazon.smithy.utils.BuilderRef; -import software.amazon.smithy.utils.ListUtils; -public final class IntEnumShape extends IntegerShape implements NamedMembers { +public final class IntEnumShape extends IntegerShape { private final Map members; - private volatile List memberNames; private volatile Map enumValues; private IntEnumShape(Builder builder) { @@ -70,50 +67,6 @@ public Map getAllMembers() { return members; } - /** - * Returns an ordered list of member names based on the order they are - * defined in the model, including mixin members. - * - * @return Returns an immutable list of member names. - */ - @Override - public List getMemberNames() { - List names = memberNames; - if (names == null) { - names = ListUtils.copyOf(members.keySet()); - memberNames = names; - } - - return names; - } - - @Override - public boolean equals(Object other) { - if (!super.equals(other)) { - return false; - } - - // Members are ordered, so do a test on the ordering and their values. - IntEnumShape b = (IntEnumShape) other; - return getMemberNames().equals(b.getMemberNames()) && members.equals(b.members); - } - - /** - * Get a specific member by name. - * - * @param name Name of the member to retrieve. - * @return Returns the optional member. - */ - @Override - public Optional getMember(String name) { - return Optional.ofNullable(members.get(name)); - } - - @Override - public Collection members() { - return members.values(); - } - public static Builder builder() { return new Builder(); } 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 index 47cc859addc..ee38f7acf18 100644 --- 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 @@ -15,14 +15,20 @@ package software.amazon.smithy.model.shapes; +import java.util.AbstractMap; import java.util.Collection; import java.util.Collections; +import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.function.Consumer; import software.amazon.smithy.model.SourceLocation; import software.amazon.smithy.model.traits.Trait; import software.amazon.smithy.utils.ListUtils; +import software.amazon.smithy.utils.Pair; +import software.amazon.smithy.utils.SetUtils; import software.amazon.smithy.utils.ToSmithyBuilder; /** @@ -32,6 +38,7 @@ public final class MapShape extends Shape implements ToSmithyBuilder { private final MemberShape key; private final MemberShape value; + private transient Map memberMap; private MapShape(Builder builder) { super(builder, false); @@ -83,32 +90,60 @@ public MemberShape getKey() { } @Override - public Collection members() { - return ListUtils.of(key, value); - } + public Map getAllMembers() { + Map result = memberMap; + + // Create a two-entry map that only knows about "key" and "value". + if (result == null) { + result = new AbstractMap() { + private transient Set> entries; + private transient List values; + + @Override + public MemberShape get(Object keyName) { + if ("key".equals(keyName)) { + return key; + } else if ("value".equals(keyName)) { + return value; + } else { + return null; + } + } - @Override - public Optional getMember(String name) { - switch (name) { - case "key": - return Optional.of(key); - case "value": - return Optional.of(value); - default: - return Optional.empty(); - } - } + @Override + public boolean containsKey(Object keyName) { + return "key".equals(keyName) || "value".equals(keyName); + } - @Override - public boolean equals(Object other) { - if (!super.equals(other)) { - return false; - } else { - MapShape otherShape = (MapShape) other; - return super.equals(otherShape) - && getKey().equals(otherShape.getKey()) - && getValue().equals(((MapShape) other).getValue()); + @Override + public Set keySet() { + return SetUtils.of("key", "value"); + } + + @Override + public Collection values() { + List result = values; + if (result == null) { + result = ListUtils.of(key, value); + values = result; + } + return result; + } + + @Override + public Set> entrySet() { + Set> result = entries; + if (result == null) { + result = SetUtils.of(Pair.of("key", key), Pair.of("value", value)); + entries = result; + } + return result; + } + }; + memberMap = result; } + + return result; } /** diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMembers.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMembers.java deleted file mode 100644 index f5ca4b954c3..00000000000 --- a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMembers.java +++ /dev/null @@ -1,46 +0,0 @@ -/* - * Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"). - * You may not use this file except in compliance with the License. - * A copy of the License is located at - * - * http://aws.amazon.com/apache2.0 - * - * or in the "license" file accompanying this file. This file 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 software.amazon.smithy.model.shapes; - -import java.util.List; -import java.util.Map; -import java.util.Optional; - -public interface NamedMembers { - - /** - * Gets the members of the shape, including mixin members. - * - * @return Returns the immutable member map. - */ - Map getAllMembers(); - - /** - * Returns an ordered list of member names based on the order they are - * defined in the model, including mixin members. - * - * @return Returns an immutable list of member names. - */ - List getMemberNames(); - - /** - * Get a specific member by name. - * - * @param name Name of the member to retrieve. - * @return Returns the optional member. - */ - Optional getMember(String name); -} 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 deleted file mode 100644 index 20d2240fbb9..00000000000 --- a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMembersShape.java +++ /dev/null @@ -1,230 +0,0 @@ -/* - * Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"). - * You may not use this file except in compliance with the License. - * A copy of the License is located at - * - * http://aws.amazon.com/apache2.0 - * - * or in the "license" file accompanying this file. This file 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 software.amazon.smithy.model.shapes; - -import java.util.Collection; -import java.util.List; -import java.util.Map; -import java.util.Optional; -import java.util.function.Consumer; -import software.amazon.smithy.utils.BuilderRef; -import software.amazon.smithy.utils.ListUtils; - -/** - * Abstract classes shared by structure and union shapes. - * - *

The order of members in structures and unions are the same as the - * order that they are defined in the model. - */ -public abstract class NamedMembersShape extends Shape implements NamedMembers { - - private final Map members; - private volatile List memberNames; - - NamedMembersShape(NamedMembersShape.Builder builder) { - super(builder, false); - members = NamedMemberUtils.computeMixinMembers( - builder.getMixins(), builder.members, getId(), getSourceLocation()); - validateMemberShapeIds(); - } - - /** - * Gets the members of the shape, including mixin members. - * - * @return Returns the immutable member map. - */ - @Override - public Map getAllMembers() { - return members; - } - - /** - * Returns an ordered list of member names based on the order they are - * defined in the model, including mixin members. - * - * @return Returns an immutable list of member names. - */ - @Override - public List getMemberNames() { - List names = memberNames; - if (names == null) { - names = ListUtils.copyOf(members.keySet()); - memberNames = names; - } - - return names; - } - - /** - * Get a specific member by name. - * - * @param name Name of the member to retrieve. - * @return Returns the optional member. - */ - @Override - public Optional getMember(String name) { - return Optional.ofNullable(members.get(name)); - } - - @Override - public Collection members() { - return members.values(); - } - - @Override - public boolean equals(Object other) { - if (!super.equals(other)) { - return false; - } - - // Members are ordered, so do a test on the ordering and their values. - NamedMembersShape b = (NamedMembersShape) other; - return getMemberNames().equals(b.getMemberNames()) && members.equals(b.members); - } - - /** - * Builder used to create a List or Set shape. - * @param Concrete builder type. - * @param Shape type being created. - */ - public abstract static class Builder, S extends NamedMembersShape> - extends AbstractShapeBuilder { - - private final BuilderRef> members = BuilderRef.forOrderedMap(); - - @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.peek().values()) { - addMember(member.toBuilder().id(shapeId.withMember(member.getMemberName())).build()); - } - return super.id(shapeId); - } - - /** - * Replaces the members of the builder. - * - * @param members Members to add to the builder. - * @return Returns the builder. - */ - @SuppressWarnings("unchecked") - public B members(Collection members) { - clearMembers(); - for (MemberShape member : members) { - addMember(member); - } - return (B) this; - } - - /** - * Removes all members from the shape. - * - * @return Returns the builder. - */ - @SuppressWarnings("unchecked") - public B clearMembers() { - members.clear(); - return (B) this; - } - - @Override - @SuppressWarnings("unchecked") - public B addMember(MemberShape member) { - members.get().put(member.getMemberName(), member); - return (B) this; - } - - /** - * Adds a member to the builder. - * - * @param memberName Member name to add. - * @param target Target of the member. - * @return Returns the builder. - */ - public B addMember(String memberName, ShapeId target) { - return addMember(memberName, target, null); - } - - /** - * Adds a member to the builder. - * - * @param memberName Member name to add. - * @param target Target of the member. - * @param memberUpdater Consumer that can update the created member shape. - * @return Returns the builder. - */ - public B addMember(String memberName, ShapeId target, Consumer memberUpdater) { - if (getId() == null) { - throw new IllegalStateException("An id must be set before setting a member with a target"); - } - - MemberShape.Builder builder = MemberShape.builder().target(target).id(getId().withMember(memberName)); - - if (memberUpdater != null) { - memberUpdater.accept(builder); - } - - return addMember(builder.build()); - } - - /** - * Removes a member by name. - * - *

Note that removing a member that was added by a mixin results in - * an inconsistent model. It's best to use ModelTransform to ensure - * that the model remains consistent when removing members. - * - * @param member Member name to remove. - * @return Returns the builder. - */ - @SuppressWarnings("unchecked") - public B removeMember(String member) { - if (members.hasValue()) { - members.get().remove(member); - } - return (B) this; - } - - @Override - @SuppressWarnings("unchecked") - public B addMixin(Shape shape) { - if (getId() == null) { - throw new IllegalStateException("An id must be set before adding a mixin"); - } - super.addMixin(shape); - NamedMemberUtils.cleanMixins(shape, members.get()); - return (B) this; - } - - @Override - @SuppressWarnings("unchecked") - public B removeMixin(ToShapeId shape) { - super.removeMixin(shape); - NamedMemberUtils.removeMixin(shape, members.get()); - return (B) this; - } - - @Override - @SuppressWarnings("unchecked") - public B flattenMixins() { - if (getMixins().isEmpty()) { - return (B) this; - } - members(NamedMemberUtils.flattenMixins(members.get(), getMixins(), getId(), getSourceLocation())); - return super.flattenMixins(); - } - } -} diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMembersShapeBuilder.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMembersShapeBuilder.java new file mode 100644 index 00000000000..eae1d88951d --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMembersShapeBuilder.java @@ -0,0 +1,154 @@ +/* + * Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file 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 software.amazon.smithy.model.shapes; + +import java.util.Collection; +import java.util.Map; +import java.util.function.Consumer; +import software.amazon.smithy.utils.BuilderRef; + +/** + * Builder used to create a List or Set shape. + * @param Concrete builder type. + * @param Shape type being created. + */ +abstract class NamedMembersShapeBuilder, S extends Shape> + extends AbstractShapeBuilder { + + protected final BuilderRef> members = BuilderRef.forOrderedMap(); + + @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.peek().values()) { + addMember(member.toBuilder().id(shapeId.withMember(member.getMemberName())).build()); + } + return super.id(shapeId); + } + + /** + * Replaces the members of the builder. + * + * @param members Members to add to the builder. + * @return Returns the builder. + */ + @SuppressWarnings("unchecked") + public B members(Collection members) { + clearMembers(); + for (MemberShape member : members) { + addMember(member); + } + return (B) this; + } + + /** + * Removes all members from the shape. + * + * @return Returns the builder. + */ + @SuppressWarnings("unchecked") + public B clearMembers() { + members.clear(); + return (B) this; + } + + @Override + @SuppressWarnings("unchecked") + public B addMember(MemberShape member) { + members.get().put(member.getMemberName(), member); + return (B) this; + } + + /** + * Adds a member to the builder. + * + * @param memberName Member name to add. + * @param target Target of the member. + * @return Returns the builder. + */ + public B addMember(String memberName, ShapeId target) { + return addMember(memberName, target, null); + } + + /** + * Adds a member to the builder. + * + * @param memberName Member name to add. + * @param target Target of the member. + * @param memberUpdater Consumer that can update the created member shape. + * @return Returns the builder. + */ + public B addMember(String memberName, ShapeId target, Consumer memberUpdater) { + if (getId() == null) { + throw new IllegalStateException("An id must be set before setting a member with a target"); + } + + MemberShape.Builder builder = MemberShape.builder().target(target).id(getId().withMember(memberName)); + + if (memberUpdater != null) { + memberUpdater.accept(builder); + } + + return addMember(builder.build()); + } + + /** + * Removes a member by name. + * + *

Note that removing a member that was added by a mixin results in + * an inconsistent model. It's best to use ModelTransform to ensure + * that the model remains consistent when removing members. + * + * @param member Member name to remove. + * @return Returns the builder. + */ + @SuppressWarnings("unchecked") + public B removeMember(String member) { + if (members.hasValue()) { + members.get().remove(member); + } + return (B) this; + } + + @Override + @SuppressWarnings("unchecked") + public B addMixin(Shape shape) { + if (getId() == null) { + throw new IllegalStateException("An id must be set before adding a mixin"); + } + super.addMixin(shape); + NamedMemberUtils.cleanMixins(shape, members.get()); + return (B) this; + } + + @Override + @SuppressWarnings("unchecked") + public B removeMixin(ToShapeId shape) { + super.removeMixin(shape); + NamedMemberUtils.removeMixin(shape, members.get()); + return (B) this; + } + + @Override + @SuppressWarnings("unchecked") + public B flattenMixins() { + if (getMixins().isEmpty()) { + return (B) this; + } + members(NamedMemberUtils.flattenMixins(members.get(), getMixins(), getId(), getSourceLocation())); + return super.flattenMixins(); + } +} diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/Shape.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/Shape.java index 5027e354798..b7ee68f2512 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/Shape.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/Shape.java @@ -31,6 +31,7 @@ import software.amazon.smithy.model.traits.MixinTrait; import software.amazon.smithy.model.traits.TagsTrait; import software.amazon.smithy.model.traits.Trait; +import software.amazon.smithy.utils.ListUtils; import software.amazon.smithy.utils.MapUtils; import software.amazon.smithy.utils.SmithyBuilder; import software.amazon.smithy.utils.Tagged; @@ -53,6 +54,7 @@ public abstract class Shape implements FromSourceLocation, Tagged, ToShapeId, Co private final Map introducedTraits; private final Map mixins; private final transient SourceLocation source; + private transient List memberNames; /** * This class is package-private, which means that all subclasses of this @@ -671,12 +673,12 @@ public final boolean isTimestampShape() { } /** - * Gets all of the members contained in the shape. + * Gets all the members contained in the shape. * * @return Returns the members contained in the shape (if any). */ public Collection members() { - return Collections.emptyList(); + return getAllMembers().values(); } /** @@ -688,7 +690,35 @@ public Collection members() { * @return Returns the optional member. */ public Optional getMember(String name) { - return Optional.empty(); + return Optional.ofNullable(getAllMembers().get(name)); + } + + /** + * Gets the members of the shape, including mixin members. + * + * @return Returns the immutable member map. + */ + public Map getAllMembers() { + return Collections.emptyMap(); + } + + /** + * Returns an ordered list of member names based on the order they are + * defined in the model, including mixin members. + * + *

The order in which map key and value members are returned might + * not match the order in which they were defined in the model because + * their ordering is insignificant. + * + * @return Returns an immutable list of member names. + */ + public List getMemberNames() { + List result = memberNames; + if (result == null) { + result = ListUtils.copyOf(getAllMembers().keySet()); + memberNames = result; + } + return result; } /** @@ -749,10 +779,13 @@ public boolean equals(Object o) { } Shape other = (Shape) o; - return getId().equals(other.getId()) - && getType() == other.getType() + return getType() == other.getType() + && getId().equals(other.getId()) && traits.equals(other.traits) - && mixins.equals(other.mixins); + && mixins.equals(other.mixins) + // Ensure members are equal and defined in the same order. + && getAllMembers().equals(other.getAllMembers()) + && getMemberNames().equals(other.getMemberNames()); } /** diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/SmithyIdlModelSerializer.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/SmithyIdlModelSerializer.java index 3a9c817d256..2020b64a5e3 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/SmithyIdlModelSerializer.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/SmithyIdlModelSerializer.java @@ -875,7 +875,7 @@ private void serializeKeyValuePairs(ObjectNode node, Shape shape) { if (shape != null && shape.isMapShape()) { // For maps the value member will always be the same. member = shape.asMapShape().get().getValue(); - } else if (shape instanceof NamedMembersShape) { + } else if (shape instanceof StructureShape || shape instanceof UnionShape) { member = members.get(name.getValue()); } else { // At this point the shape is either null or a document shape. diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/StructureShape.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/StructureShape.java index c20d6763bc3..87cd9a23247 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/StructureShape.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/StructureShape.java @@ -1,5 +1,5 @@ /* - * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"). * You may not use this file except in compliance with the License. @@ -15,16 +15,22 @@ package software.amazon.smithy.model.shapes; +import java.util.Map; import java.util.Optional; import software.amazon.smithy.utils.ToSmithyBuilder; /** * Structure shape that maps shape names to members. */ -public final class StructureShape extends NamedMembersShape implements ToSmithyBuilder { +public final class StructureShape extends Shape implements ToSmithyBuilder { + + private final Map members; private StructureShape(Builder builder) { - super(builder); + super(builder, false); + members = NamedMemberUtils.computeMixinMembers( + builder.getMixins(), builder.members, getId(), getSourceLocation()); + validateMemberShapeIds(); } /** @@ -54,10 +60,15 @@ public ShapeType getType() { return ShapeType.STRUCTURE; } + @Override + public Map getAllMembers() { + return members; + } + /** * Builder used to create a {@link StructureShape}. */ - public static final class Builder extends NamedMembersShape.Builder { + public static final class Builder extends NamedMembersShapeBuilder { @Override public StructureShape build() { return new StructureShape(this); diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/UnionShape.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/UnionShape.java index 0116b5fb483..77bb11e4ca0 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/UnionShape.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/UnionShape.java @@ -1,5 +1,5 @@ /* - * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"). * You may not use this file except in compliance with the License. @@ -15,16 +15,22 @@ package software.amazon.smithy.model.shapes; +import java.util.Map; import java.util.Optional; import software.amazon.smithy.utils.ToSmithyBuilder; /** * Tagged union shape that maps member names to member definitions. */ -public final class UnionShape extends NamedMembersShape implements ToSmithyBuilder { +public final class UnionShape extends Shape implements ToSmithyBuilder { + + private final Map members; private UnionShape(Builder builder) { - super(builder); + super(builder, false); + members = NamedMemberUtils.computeMixinMembers( + builder.getMixins(), builder.members, getId(), getSourceLocation()); + validateMemberShapeIds(); } public static Builder builder() { @@ -51,10 +57,15 @@ public ShapeType getType() { return ShapeType.UNION; } + @Override + public Map getAllMembers() { + return members; + } + /** * Builder used to create a {@link UnionShape}. */ - public static final class Builder extends NamedMembersShape.Builder { + public static final class Builder extends NamedMembersShapeBuilder { @Override public UnionShape build() { return new UnionShape(this); diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ExclusiveStructureMemberTraitValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ExclusiveStructureMemberTraitValidator.java index 47152b1a651..135292bb1c9 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ExclusiveStructureMemberTraitValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ExclusiveStructureMemberTraitValidator.java @@ -21,7 +21,6 @@ import java.util.Set; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.shapes.MemberShape; -import software.amazon.smithy.model.shapes.NamedMembers; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.traits.Trait; @@ -51,11 +50,9 @@ public List validate(Model model) { } List events = new ArrayList<>(); - for (Shape shape : model.toSet()) { - if (shape instanceof NamedMembers) { - validateExclusiveMembers(shape, exclusiveMemberTraits, events); - validateExclusiveTargets(model, shape, exclusiveTargetTraits, events); - } + for (Shape shape : model.getStructureShapes()) { + validateExclusiveMembers(shape, exclusiveMemberTraits, events); + validateExclusiveTargets(model, shape, exclusiveTargetTraits, events); } return events;