From 1ed12ebda4ac6f87f0192c596959375b2b8a7df0 Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Thu, 25 May 2023 09:58:31 -0400 Subject: [PATCH] Merge aggregate, structure shape grammar/parsing This commit combines aggregate shapes (list, map, union) and structure shapes in the grammar and parser. Previously, the way these shapes were defined only differed by aggregate shapes not allowing `for ` syntax. With this change, aggregate shapes can now use `for ` syntax and elide resource identifiers/properties. The parser was also updated so parsing inline structures uses the same method of parsing aggregate shapes. Tests making sure `for ` couldn't be used with non structure shapes were removed, and tests were added for verifying `for ` behavior with lists, maps, and unions. The tests added for list/map/union elided members in elided-members.smithy mirror the tests for structure shapes in the same file. This also updates the naming of shape productions in the grammar. --- docs/source-2.0/spec/idl.rst | 52 ++--- .../smithy/model/loader/IdlModelLoader.java | 35 +--- .../invalid-list-and-map-elision.errors | 10 + .../invalid-list-and-map-elision.smithy | 55 +++++ .../elided-union-member-from-resource.smithy | 14 -- .../model/loader/valid/elided-members.json | 196 ++++++++++++++++++ .../model/loader/valid/elided-members.smithy | 78 +++++++ 7 files changed, 365 insertions(+), 75 deletions(-) create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/invalid-list-and-map-elision.errors create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/invalid-list-and-map-elision.smithy delete mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/loader/invalid/elision/elided-union-member-from-resource.smithy diff --git a/docs/source-2.0/spec/idl.rst b/docs/source-2.0/spec/idl.rst index 4fa37771064..2e62567ddad 100644 --- a/docs/source-2.0/spec/idl.rst +++ b/docs/source-2.0/spec/idl.rst @@ -174,44 +174,36 @@ string support defined in :rfc:`7405`. UseStatement :%s"use" `SP` `AbsoluteRootShapeId` `BR` ShapeStatements :`ShapeOrApplyStatement` *(`BR` `ShapeOrApplyStatement`) ShapeOrApplyStatement :`ShapeStatement` / `ApplyStatement` - ShapeStatement :`TraitStatements` `ShapeBody` - ShapeBody :`SimpleShapeStatement` - :/ `EnumStatement` - :/ `AggregateShapeStatement` - :/ `StructureStatement` - :/ `EntityStatement` - :/ `OperationStatement` - SimpleShapeStatement :`SimpleTypeName` `SP` `Identifier` [`Mixins`] + ShapeStatement :`TraitStatements` `Shape` + Shape :`SimpleShape` / `EnumShape` / `AggregateShape` / `EntityShape` / `OperationShape` + SimpleShape :`SimpleTypeName` `SP` `Identifier` [`Mixins`] SimpleTypeName :%s"blob" / %s"boolean" / %s"document" / %s"string" :/ %s"byte" / %s"short" / %s"integer" / %s"long" :/ %s"float" / %s"double" / %s"bigInteger" :/ %s"bigDecimal" / %s"timestamp" Mixins :[`SP`] %s"with" [`WS`] "[" [`WS`] 1*(`ShapeId` [`WS`]) "]" - EnumStatement :`EnumTypeName` `SP` `Identifier` [`Mixins`] [`WS`] `EnumShapeMembers` + EnumShape :`EnumTypeName` `SP` `Identifier` [`Mixins`] [`WS`] `EnumShapeMembers` EnumTypeName :%s"enum" / %s"intEnum" EnumShapeMembers :"{" [`WS`] 1*(`TraitStatements` `Identifier` [`ValueAssignment`] [`WS`]) "}" ValueAssignment :[`SP`] "=" [`SP`] `NodeValue` [`SP`] [`Comma`] `BR` - AggregateShapeStatement :`AggregateTypeName` `SP` `Identifier` [`Mixins`] `ShapeMembers` - AggregateTypeName :%s"list" / %s"map" / %s"union" - StructureStatement :%s"structure" `SP` `Identifier` [`StructureResource`] - : [`Mixins`] [`WS`] `ShapeMembers` - StructureResource :`SP` %s"for" `SP` `ShapeId` + AggregateShape :`AggregateTypeName` `SP` `Identifier` [`ForResource`] [`Mixins`] [`WS`] `ShapeMembers` + AggregateTypeName :%s"list" / %s"map" / %s"union" / %s"structure" + ForResource :`SP` %s"for" `SP` `ShapeId` ShapeMembers :"{" [`WS`] *(`TraitStatements` `ShapeMember` [`WS`]) "}" ShapeMember :(`ExplicitShapeMember` / `ElidedShapeMember`) [`ValueAssignment`] ExplicitShapeMember :`Identifier` [`SP`] ":" [`SP`] `ShapeId` ElidedShapeMember :"$" `Identifier` - EntityStatement :`EntityTypeName` `SP` `Identifier` [`Mixins`] [`WS`] `NodeObject` + EntityShape :`EntityTypeName` `SP` `Identifier` [`Mixins`] [`WS`] `NodeObject` EntityTypeName :%s"service" / %s"resource" - OperationStatement :%s"operation" `SP` `Identifier` [`Mixins`] [`WS`] `OperationBody` + OperationShape :%s"operation" `SP` `Identifier` [`Mixins`] [`WS`] `OperationBody` OperationBody :"{" [`WS`] : *(`OperationInput` / `OperationOutput` / `OperationErrors`) : [`WS`] "}" : ; only one of each property can be specified. - OperationInput :%s"input" [`WS`] (`InlineStructure` / (":" [`WS`] `ShapeId`)) - OperationOutput :%s"output" [`WS`] (`InlineStructure` / (":" [`WS`] `ShapeId`)) + OperationInput :%s"input" [`WS`] (`InlineAggregateShape` / (":" [`WS`] `ShapeId`)) + OperationOutput :%s"output" [`WS`] (`InlineAggregateShape` / (":" [`WS`] `ShapeId`)) OperationErrors :%s"errors" [`WS`] ":" [`WS`] "[" [`WS`] *(`ShapeId` [`WS`]) "]" - InlineStructure :":=" [`WS`] `TraitStatements` [`StructureResource`] - : [`Mixins`] [`WS`] `ShapeMembers` + InlineAggregateShape :":=" [`WS`] `TraitStatements` [`ForResource`] [`Mixins`] [`WS`] `ShapeMembers` .. rubric:: Traits @@ -705,7 +697,7 @@ Simple shapes ------------- :ref:`Simple shapes ` are defined using a -:token:`smithy:SimpleShapeStatement`. +:token:`smithy:SimpleShape`. The following example defines a ``string`` shape: @@ -768,7 +760,7 @@ The following example defines an ``integer`` shape with a :ref:`range-trait`: Enum shapes ----------- -The :ref:`enum` shape is defined using an :token:`smithy:EnumStatement`. +The :ref:`enum` shape is defined using an :token:`smithy:EnumShape`. The following example defines an :ref:`enum` shape: @@ -831,7 +823,7 @@ IntEnum shapes -------------- The :ref:`intEnum` shape is defined using an -:token:`smithy:EnumStatement`. +:token:`smithy:EnumShape`. .. note:: The :ref:`enumValue trait ` is required on all @@ -879,7 +871,7 @@ The above intEnum is exactly equivalent to the following intEnum: List shapes ----------- -A :ref:`list ` shape is defined using a :token:`smithy:AggregateShapeStatement`. +A :ref:`list ` shape is defined using a :token:`smithy:AggregateShape`. The following example defines a list with a string member from the :ref:`prelude `: @@ -960,7 +952,7 @@ Traits can be applied to the list shape and its member: Map shapes ---------- -A :ref:`map ` shape is defined using a :token:`smithy:AggregateShapeStatement`. +A :ref:`map ` shape is defined using a :token:`smithy:AggregateShape`. The following example defines a map of strings to integers: @@ -1057,7 +1049,7 @@ Structure shapes ---------------- A :ref:`structure ` shape is defined using a -:token:`smithy:StructureStatement`. +:token:`smithy:AggregateShape`. The following example defines a structure with two members: @@ -1170,7 +1162,7 @@ Is exactly equivalent to: Union shapes ------------ -A :ref:`union ` shape is defined using a :token:`smithy:AggregateShapeStatement`. +A :ref:`union ` shape is defined using a :token:`smithy:AggregateShape`. The following example defines a union shape with several members: @@ -1224,7 +1216,7 @@ The following example defines a union shape with several members: Service shape ------------- -A service shape is defined using a :token:`smithy:EntityStatement` and the provided +A service shape is defined using a :token:`smithy:EntityShape` and the provided :token:`smithy:NodeObject` supports the same properties defined in the :ref:`service specification `. @@ -1273,7 +1265,7 @@ a resource named ``Model`` and an operation named ``PingService``: Operation shape --------------- -An operation shape is defined using an :token:`smithy:OperationStatement` and +An operation shape is defined using an :token:`smithy:OperationShape` and the same properties defined in the :ref:`operation specification `. The following example defines an operation shape that accepts an input @@ -1430,7 +1422,7 @@ The suffixes for the generated names can be customized using the Resource shape -------------- -A resource shape is defined using a :token:`smithy:EntityStatement` and the +A resource shape is defined using a :token:`smithy:EntityShape` and the provided :token:`smithy:NodeObject` supports the same properties defined in the :ref:`resource specification `. diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/IdlModelLoader.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/IdlModelLoader.java index 5e194b21b16..c8390a5dd46 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/IdlModelLoader.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/IdlModelLoader.java @@ -42,7 +42,6 @@ import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.shapes.ShapeType; import software.amazon.smithy.model.shapes.StructureShape; -import software.amazon.smithy.model.shapes.UnionShape; import software.amazon.smithy.model.traits.DefaultTrait; import software.amazon.smithy.model.traits.DocumentationTrait; import software.amazon.smithy.model.traits.EnumValueTrait; @@ -558,6 +557,8 @@ private void parseShapeOrApply(List traits) { case LIST: case SET: case MAP: + case UNION: + case STRUCTURE: parseAggregateShape(id, location, type.createBuilderForType()); break; case ENUM: @@ -566,12 +567,6 @@ private void parseShapeOrApply(List traits) { case INT_ENUM: parseEnumShape(id, location, IntEnumShape.builder()); break; - case STRUCTURE: - parseStructuredShape(id, location, StructureShape.builder()); - break; - case UNION: - parseStructuredShape(id, location, UnionShape.builder()); - break; case SERVICE: parseServiceStatement(id, location); break; @@ -700,25 +695,7 @@ private void parseEnumShape(ShapeId id, SourceLocation location, AbstractShapeBu private void parseAggregateShape(ShapeId id, SourceLocation location, AbstractShapeBuilder builder) { LoadOperation.DefineShape operation = createShape(builder.id(id).source(location)); - parseMixins(operation); - parseMembers(operation); - operations.accept(operation); - } - - private void parseStructuredShape( - ShapeId id, - SourceLocation location, - AbstractShapeBuilder builder - ) { - LoadOperation.DefineShape operation = createShape(builder.id(id).source(location)); - - // If it's a structure, parse the optional "from" statement to enable - // eliding member targets for resource identifiers. - if (builder.getShapeType() == ShapeType.STRUCTURE) { - parseForResource(operation); - } - - // Parse optional "with" statements to add mixins, but only if it's supported by the version. + parseForResource(operation); parseMixins(operation); parseMembers(operation); operations.accept(operation); @@ -999,12 +976,8 @@ private ShapeId parseInlineStructure(String name, IdlTraitParser.Result defaultT ShapeId id = ShapeId.fromRelative(expectNamespace(), name); SourceLocation location = tokenizer.getCurrentTokenLocation(); StructureShape.Builder builder = StructureShape.builder().id(id).source(location); - LoadOperation.DefineShape operation = createShape(builder); - parseMixins(operation); - parseForResource(operation); - parseMembers(operation); + parseAggregateShape(id, location, builder); addTraits(id, traits); - operations.accept(operation); return id; } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/invalid-list-and-map-elision.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/invalid-list-and-map-elision.errors new file mode 100644 index 00000000000..761ab650883 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/invalid-list-and-map-elision.errors @@ -0,0 +1,10 @@ +[ERROR] com.test#WrongMemberList: Missing required member of shape `com.test#WrongMemberList`: member | Model +[ERROR] com.test#WrongMemberList: List shapes may only have a `member` member, but found `id` | Model +[ERROR] com.test#ExtraElidedMemberList: List shapes may only have a `member` member, but found `other` | Model +[ERROR] com.test#ConflictingTypesList$member: Member conflicts with an inherited mixin member: `com.test#MixinList$member` | Model +[ERROR] com.test#WrongMembersMap: Missing required members of shape `com.test#WrongMembersMap`: key, value | Model +[ERROR] com.test#WrongMembersMap: Map shapes may only have `key` and `value` members, but found `id` | Model +[ERROR] com.test#WrongMembersMap: Map shapes may only have `key` and `value` members, but found `other` | Model +[ERROR] com.test#ExtraMemberMap: Map shapes may only have `key` and `value` members, but found `other` | Model +[ERROR] com.test#ConflictingTypesMap$key: Member conflicts with an inherited mixin member: `com.test#MixinMap$key` | Model +[ERROR] com.test#ConflictingTypesMap$value: Member conflicts with an inherited mixin member: `com.test#MixinMap$value` | Model diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/invalid-list-and-map-elision.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/invalid-list-and-map-elision.smithy new file mode 100644 index 00000000000..e06d6fab4d7 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/invalid-list-and-map-elision.smithy @@ -0,0 +1,55 @@ +$version: "2.0" + +namespace com.test + +resource MyResource { + identifiers: { + id: String + member: String + key: String + } + properties: { + value: String + other: String + } +} + +list WrongMemberList for MyResource { + $id +} + +list ExtraElidedMemberList for MyResource { + $member + $other +} + +@mixin +list MixinList { + member: Integer +} + +list ConflictingTypesList for MyResource with [MixinList] { + $member +} + +map WrongMembersMap for MyResource { + $id + $other +} + +map ExtraMemberMap for MyResource { + $key + $value + $other +} + +@mixin +map MixinMap { + key: Integer + value: Integer +} + +map ConflictingTypesMap for MyResource with [MixinMap] { + $key + $value +} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/invalid/elision/elided-union-member-from-resource.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/invalid/elision/elided-union-member-from-resource.smithy deleted file mode 100644 index 37e0200522b..00000000000 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/invalid/elision/elided-union-member-from-resource.smithy +++ /dev/null @@ -1,14 +0,0 @@ -// Syntax error at line 12, column 15: Expected LBRACE('{') but found IDENTIFIER('for') | Model -$version: "2.0" - -namespace smithy.example - -resource MyResource { - identifiers: { - id: String - } -} - -union MyUnion for MyResource { - $id -} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/elided-members.json b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/elided-members.json index da367c9cd60..ae647d72376 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/elided-members.json +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/elided-members.json @@ -9,6 +9,48 @@ } ] }, + "smithy.example#MixedListWithTraits": { + "type": "list", + "mixins": [ + { + "target": "smithy.example#MixinList" + } + ] + }, + "smithy.example#MixedListWithTraits$member": { + "type": "apply", + "traits": { + "smithy.api#pattern": ".*" + } + }, + "smithy.example#ResourceList": { + "type": "list", + "member": { + "target": "smithy.api#String" + } + }, + "smithy.example#MixedResourceList": { + "type": "list", + "mixins": [ + { + "target": "smithy.example#MixinList" + } + ] + }, + "smithy.example#MixedResourceListWithTrait": { + "type": "list", + "mixins": [ + { + "target": "smithy.example#MixinList" + } + ] + }, + "smithy.example#MixedResourceListWithTrait$member": { + "type": "apply", + "traits": { + "smithy.api#pattern": ".*" + } + }, "smithy.example#MixedMap": { "type": "map", "mixins": [ @@ -17,6 +59,63 @@ } ] }, + "smithy.example#MixedMapWithTraits": { + "type": "map", + "mixins": [ + { + "target": "smithy.example#MixinMap" + } + ] + }, + "smithy.example#MixedMapWithTraits$key": { + "type": "apply", + "traits": { + "smithy.api#pattern": ".*" + } + }, + "smithy.example#MixedMapWithTraits$value": { + "type": "apply", + "traits": { + "smithy.api#pattern": ".*" + } + }, + "smithy.example#ResourceMap": { + "type": "map", + "key": { + "target": "smithy.api#String" + }, + "value": { + "target": "smithy.api#String" + } + }, + "smithy.example#MixedResourceMap": { + "type": "map", + "mixins": [ + { + "target": "smithy.example#MixinMap" + } + ] + }, + "smithy.example#MixedResourceMapWithTraits": { + "type": "map", + "mixins": [ + { + "target": "smithy.example#MixinMap" + } + ] + }, + "smithy.example#MixedResourceMapWithTraits$key": { + "type": "apply", + "traits": { + "smithy.api#pattern": ".*" + } + }, + "smithy.example#MixedResourceMapWithTraits$value": { + "type": "apply", + "traits": { + "smithy.api#pattern": ".*" + } + }, "smithy.example#MixedResourceStructure": { "type": "structure", "mixins": [ @@ -116,6 +215,82 @@ ], "members": {} }, + "smithy.example#MixedUnionWithTraits": { + "type": "union", + "mixins": [ + { + "target": "smithy.example#MixinUnion" + } + ], + "members": {} + }, + "smithy.example#MixedUnionWithTraits$singleton": { + "type": "apply", + "traits": { + "smithy.api#pattern": ".*" + } + }, + "smithy.example#ResourceUnion": { + "type": "union", + "members": { + "id": { + "target": "smithy.api#String" + }, + "property": { + "target": "smithy.api#String" + } + } + }, + "smithy.example#MixedResourceUnion": { + "type": "union", + "mixins": [ + { + "target": "smithy.example#MixinUnion" + } + ], + "members": { + "id": { + "target": "smithy.api#String" + }, + "property": { + "target": "smithy.api#String" + } + } + }, + "smithy.example#MixedResourceUnionWithTraits": { + "type": "union", + "mixins": [ + { + "target": "smithy.example#MixinUnion" + } + ], + "members": { + "id": { + "target": "smithy.api#String" + }, + "property": { + "target": "smithy.api#String" + } + } + }, + "smithy.example#MixedResourceUnionWithTraits$id": { + "type": "apply", + "traits": { + "smithy.api#pattern": ".*" + } + }, + "smithy.example#MixedResourceUnionWithTraits$property": { + "type": "apply", + "traits": { + "smithy.api#pattern": ".*" + } + }, + "smithy.example#MixedResourceUnionWithTraits$singleton": { + "type": "apply", + "traits": { + "smithy.api#pattern": ".*" + } + }, "smithy.example#MixinList": { "type": "list", "member": { @@ -178,11 +353,20 @@ "identifiers": { "id": { "target": "smithy.api#String" + }, + "key": { + "target": "smithy.api#String" } }, "properties": { "property": { "target": "smithy.api#String" + }, + "member": { + "target": "smithy.api#String" + }, + "value": { + "target": "smithy.api#String" } }, "operations": [ @@ -231,8 +415,20 @@ "smithy.api#required": {} } }, + "key": { + "target": "smithy.api#String", + "traits": { + "smithy.api#required": {} + } + }, "property": { "target": "smithy.api#String" + }, + "member": { + "target": "smithy.api#String" + }, + "value": { + "target": "smithy.api#String" } }, "traits": { diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/elided-members.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/elided-members.smithy index 33b5ec2b58e..160785e9d4d 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/elided-members.smithy +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/elided-members.smithy @@ -5,9 +5,12 @@ namespace smithy.example resource MyResource { identifiers: { id: String + key: String } properties: { property: String + member: String + value: String } operations: [ThrowAway] } @@ -74,6 +77,33 @@ union MixedUnion with [MixinUnion] { $singleton } +union MixedUnionWithTraits with [MixinUnion] { + @pattern(".*") + $singleton +} + +union ResourceUnion for MyResource { + $id + $property +} + +union MixedResourceUnion for MyResource with [MixinUnion] { + $id + $singleton + $property +} + +union MixedResourceUnionWithTraits for MyResource with [MixinUnion] { + @pattern(".*") + $id + + @pattern(".*") + $property + + @pattern(".*") + $singleton +} + @mixin list MixinList { member: String @@ -83,6 +113,24 @@ list MixedList with [MixinList] { $member } +list MixedListWithTraits with [MixinList] { + @pattern(".*") + $member +} + +list ResourceList for MyResource { + $member +} + +list MixedResourceList for MyResource with [MixinList] { + $member +} + +list MixedResourceListWithTrait for MyResource with [MixinList] { + @pattern(".*") + $member +} + @mixin map MixinMap { key: String @@ -94,12 +142,42 @@ map MixedMap with [MixinMap] { $value } +map MixedMapWithTraits with [MixinMap] { + @pattern(".*") + $key + + @pattern(".*") + $value +} + +map ResourceMap for MyResource { + $key + $value +} + +map MixedResourceMap for MyResource with [MixinMap] { + $key + $value +} + +map MixedResourceMapWithTraits for MyResource with [MixinMap] { + @pattern(".*") + $key + + @pattern(".*") + $value +} + /// Operation needed to utilize property for validity operation ThrowAway { input := { @required id: String + @required + key: String property: String + member: String + value: String } output := {} }