Skip to content

Commit 33050df

Browse files
committed
Throw syntax exceptions on duplicate bindings
This commit updates the Smithy IDL parsing to throw when duplicate bindings or members are found in the following declarations: service, resource, and operation shapes, applied structure traits, and applied traits with members that are structures. This behavior is unsafe for model writers and shouldn't be valid.
1 parent 8f548cb commit 33050df

13 files changed

+62
-7
lines changed

smithy-model/src/main/java/software/amazon/smithy/model/loader/IdlModelParser.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -542,7 +542,7 @@ private void parseStructuredShape(ShapeId id, SourceLocation location, AbstractS
542542
private void parseOperationStatement(ShapeId id, SourceLocation location) {
543543
ws();
544544
OperationShape.Builder builder = OperationShape.builder().id(id).source(location);
545-
ObjectNode node = IdlNodeParser.parseObjectNode(this);
545+
ObjectNode node = IdlNodeParser.parseObjectNode(this, id.toString());
546546
LoaderUtils.checkForAdditionalProperties(node, id, OPERATION_PROPERTY_NAMES, modelFile.events());
547547
modelFile.onShape(builder);
548548
optionalId(node, "input", builder::input);
@@ -553,7 +553,7 @@ private void parseOperationStatement(ShapeId id, SourceLocation location) {
553553
private void parseServiceStatement(ShapeId id, SourceLocation location) {
554554
ws();
555555
ServiceShape.Builder builder = new ServiceShape.Builder().id(id).source(location);
556-
ObjectNode shapeNode = IdlNodeParser.parseObjectNode(this);
556+
ObjectNode shapeNode = IdlNodeParser.parseObjectNode(this, id.toString());
557557
LoaderUtils.checkForAdditionalProperties(shapeNode, id, SERVICE_PROPERTY_NAMES, modelFile.events());
558558
builder.version(shapeNode.expectStringMember(VERSION_KEY).getValue());
559559
modelFile.onShape(builder);
@@ -580,7 +580,7 @@ private void parseResourceStatement(ShapeId id, SourceLocation location) {
580580
ws();
581581
ResourceShape.Builder builder = ResourceShape.builder().id(id).source(location);
582582
modelFile.onShape(builder);
583-
ObjectNode shapeNode = IdlNodeParser.parseObjectNode(this);
583+
ObjectNode shapeNode = IdlNodeParser.parseObjectNode(this, id.toString());
584584

585585
LoaderUtils.checkForAdditionalProperties(shapeNode, id, RESOURCE_PROPERTY_NAMES, modelFile.events());
586586
optionalId(shapeNode, PUT_KEY, builder::put);

smithy-model/src/main/java/software/amazon/smithy/model/loader/IdlNodeParser.java

+7-3
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ static Node parseNode(IdlModelParser parser) {
4444
char c = parser.peek();
4545
switch (c) {
4646
case '{':
47-
return parseObjectNode(parser);
47+
return parseObjectNode(parser, "object node");
4848
case '[':
4949
return parseArrayNode(parser);
5050
case '"': {
@@ -119,7 +119,7 @@ static Node parseTextBlock(IdlModelParser parser) {
119119
return new StringNode(IdlTextParser.parseQuotedTextAndTextBlock(parser, true), location);
120120
}
121121

122-
static ObjectNode parseObjectNode(IdlModelParser parser) {
122+
static ObjectNode parseObjectNode(IdlModelParser parser, String parent) {
123123
parser.increaseNestingLevel();
124124
SourceLocation location = parser.currentLocation();
125125
Map<StringNode, Node> entries = new LinkedHashMap<>();
@@ -137,7 +137,11 @@ static ObjectNode parseObjectNode(IdlModelParser parser) {
137137
parser.expect(':');
138138
parser.ws();
139139
Node value = parseNode(parser);
140-
entries.put(new StringNode(key, keyLocation), value);
140+
StringNode keyNode = new StringNode(key, keyLocation);
141+
Node previous = entries.put(keyNode, value);
142+
if (previous != null) {
143+
throw parser.syntax("Duplicate member of " + parent + ": '" + keyNode.getValue() + '\'');
144+
}
141145
parser.ws();
142146
if (parser.peek() == ',') {
143147
parser.skip();

smithy-model/src/main/java/software/amazon/smithy/model/loader/IdlTraitParser.java

+6-1
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,8 @@ private static Node parseTraitValueBodyIdentifierOrQuotedString(
121121
private static ObjectNode parseStructuredTrait(IdlModelParser parser, StringNode startingKey) {
122122
Map<StringNode, Node> entries = new LinkedHashMap<>();
123123
Node firstValue = IdlNodeParser.parseNode(parser);
124+
// This put call can be done safely without checking for duplicates,
125+
// as it's always the first member of the trait.
124126
entries.put(startingKey, firstValue);
125127
parser.ws();
126128

@@ -150,6 +152,9 @@ private static void parseTraitStructureKvp(IdlModelParser parser, Map<StringNode
150152
parser.ws();
151153
Node nextValue = IdlNodeParser.parseNode(parser);
152154
parser.ws();
153-
entries.put(nextKey, nextValue);
155+
Node previous = entries.put(nextKey, nextValue);
156+
if (previous != null) {
157+
throw parser.syntax("Duplicate member of trait: '" + nextKey.getValue() + '\'');
158+
}
154159
}
155160
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
[ERROR] -: Parse error at line 5, column 23 near `, | Model
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
namespace com.foo
2+
3+
operation GetFoo {
4+
input: GetFooInput,
5+
input: GetFooInput,
6+
}
7+
8+
structure GetFooInput {}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
[ERROR] -: Parse error at line 8, column 22 near `, | Model
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
namespace com.foo
2+
3+
resource Foo {
4+
identifiers: {
5+
id: String,
6+
},
7+
create: createFoo,
8+
create: createFoo,
9+
}
10+
11+
operation createFoo {}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
[ERROR] -: Parse error at line 5, column 26 near `, | Model
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
namespace com.foo
2+
3+
service Foo {
4+
version: "2020-07-02",
5+
version: "2020-07-02",
6+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
[ERROR] -: Parse error at line 5, column 19 near `, | Model
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
namespace com.foo
2+
3+
@deprecated(
4+
message: "Foo",
5+
message: "Foo",
6+
)
7+
structure Foo {}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
[ERROR] -: Parse error at line 6, column 21 near `, | Model
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
namespace com.foo
2+
3+
@enum([
4+
{
5+
value: "foo",
6+
value: "foo",
7+
},
8+
])
9+
string Foo

0 commit comments

Comments
 (0)