Skip to content

Commit

Permalink
Make shape ID serialization consistent
Browse files Browse the repository at this point in the history 
Shape ID serialization was inconsistent in that members would use
relative IDs when possible, while direct targets from things like input,
output, and operations would always use absolute IDs. This commit
updates model serialization to use relative shape IDs when possible.
  • Loading branch information
@mtdowling
mtdowling committed Oct 30, 2019
1 parent 309ef22 commit 7001892
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 22 deletions.
56 changes: 34 additions & 22 deletions smithy-model/src/main/java/software/amazon/smithy/model/shapes/ModelSerializer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,50 +207,58 @@ public Node mapShape(MapShape shape) {

@Override
public Node operationShape(OperationShape shape) {
ShapeId id = shape.getId();
return withTraits(shape, createTypedNode(shape)
.withOptionalMember("input", shape.getInput().map(id -> Node.from(id.toString())))
.withOptionalMember("output", shape.getOutput().map(id -> Node.from(id.toString())))
.withOptionalMember("errors", createOptionalIdList(shape.getErrors())));
.withOptionalMember("input", shape.getInput().map(target -> serializeShapeId(id, target)))
.withOptionalMember("output", shape.getOutput().map(target -> serializeShapeId(id, target)))
.withOptionalMember("errors", createOptionalIdList(id, shape.getErrors())));
}

@Override
public Node resourceShape(ResourceShape shape) {
ShapeId id = shape.getId();

Optional<Node> identifiers = Optional.empty();
if (shape.hasIdentifiers()) {
Stream<Map.Entry<String, ShapeId>> ids = shape.getIdentifiers().entrySet().stream();
identifiers = Optional.of(ids.collect(ObjectNode.collectStringKeys(
Map.Entry::getKey,
entry -> Node.from(entry.getValue().toString()))));
entry -> serializeShapeId(id, entry.getValue()))));
}

return withTraits(shape, createTypedNode(shape)
.withOptionalMember("identifiers", identifiers)
.withOptionalMember("put", shape.getPut().map(ShapeId::toString).map(Node::from))
.withOptionalMember("create", shape.getCreate().map(ShapeId::toString).map(Node::from))
.withOptionalMember("read", shape.getRead().map(ShapeId::toString).map(Node::from))
.withOptionalMember("update", shape.getUpdate().map(ShapeId::toString).map(Node::from))
.withOptionalMember("delete", shape.getDelete().map(ShapeId::toString).map(Node::from))
.withOptionalMember("list", shape.getList().map(ShapeId::toString).map(Node::from))
.withOptionalMember("operations", createOptionalIdList(shape.getOperations()))
.withOptionalMember("collectionOperations", createOptionalIdList(shape.getCollectionOperations()))
.withOptionalMember("resources", createOptionalIdList(shape.getResources())));
.withOptionalMember("put", shape.getPut().map(target -> serializeShapeId(id, target)))
.withOptionalMember("create", shape.getCreate().map(target -> serializeShapeId(id, target)))
.withOptionalMember("read", shape.getRead().map(target -> serializeShapeId(id, target)))
.withOptionalMember("update", shape.getUpdate().map(target -> serializeShapeId(id, target)))
.withOptionalMember("delete", shape.getDelete().map(target -> serializeShapeId(id, target)))
.withOptionalMember("list", shape.getList().map(target -> serializeShapeId(id, target)))
.withOptionalMember("operations", createOptionalIdList(id, shape.getOperations()))
.withOptionalMember("collectionOperations",
createOptionalIdList(id, shape.getCollectionOperations()))
.withOptionalMember("resources", createOptionalIdList(id, shape.getResources())));
}

@Override
public Node serviceShape(ServiceShape shape) {
ShapeId id = shape.getId();
return withTraits(shape, createTypedNode(shape)
.withMember("version", Node.from(shape.getVersion()))
.withOptionalMember("operations", createOptionalIdList(shape.getOperations()))
.withOptionalMember("resources", createOptionalIdList(shape.getResources())));
.withOptionalMember("operations", createOptionalIdList(id, shape.getOperations()))
.withOptionalMember("resources", createOptionalIdList(id, shape.getResources())));
}

private Optional<Node> createOptionalIdList(Collection<ShapeId> list) {
private Optional<Node> createOptionalIdList(ShapeId currentNamespace, Collection<ShapeId> list) {
if (list.isEmpty()) {
return Optional.empty();
}

return Optional.of(
list.stream().map(ShapeId::toString).sorted().map(Node::from).collect(ArrayNode.collect()));
Node result = list.stream()
.map(id -> serializeShapeId(currentNamespace, id))
.sorted(Comparator.comparing(StringNode::getValue))
.collect(ArrayNode.collect());
return Optional.of(result);
}

@Override
Expand All @@ -275,11 +283,15 @@ private ObjectNode createStructureAndUnion(Shape shape, Map<String, MemberShape>

@Override
public Node memberShape(MemberShape shape) {
String target = shape.getContainer().getNamespace().equals(shape.getTarget().getNamespace())
? shape.getTarget().asRelativeReference()
: shape.getTarget().toString();
Node target = serializeShapeId(shape.getContainer(), shape.getTarget());
return withTraits(shape, Node.objectNode().withMember("target", target));
}

return withTraits(shape, Node.objectNode().withMember("target", Node.from(target)));
private StringNode serializeShapeId(ShapeId currentNamespace, ShapeId id) {
String target = currentNamespace.getNamespace().equals(id.getNamespace())
? id.asRelativeReference()
: id.toString();
return Node.from(target);
}
}
}
48 changes: 48 additions & 0 deletions smithy-model/src/test/java/software/amazon/smithy/model/shapes/ModelSerializerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,4 +149,52 @@ public void doesNotSerializePreludeTraitsOrShapes() {

assertFalse(serialized.getMember("smithy.api").isPresent());
}

@Test
public void serializesAbsoluteShapeIdsOnlyWhenNeeded() {
Model model = Model.assembler()
.addImport(getClass().getResource("test-model.json"))
.assemble()
.unwrap();
ModelSerializer serializer = ModelSerializer.builder().build();
ObjectNode serialized = serializer.serialize(model);

ObjectNode resourceWithRelativeIds = serialized.expectObjectMember("ns.foo")
.expectObjectMember("shapes")
.expectObjectMember("MyResource");
ObjectNode resourceWithAbsoluteIds = serialized.expectObjectMember("ns.foo")
.expectObjectMember("shapes")
.expectObjectMember("ResourceNeedingAbsoluteShapeIds");
ObjectNode structureWithAbsoluteIds = serialized.expectObjectMember("ns.resource.needing.ids")
.expectObjectMember("shapes")
.expectObjectMember("GetResourceNeedingAbsoluteShapeIdsInput");
ObjectNode structureWithMixedIds = serialized.expectObjectMember("ns.foo")
.expectObjectMember("shapes")
.expectObjectMember("Structure");

assertThat(resourceWithAbsoluteIds.expectObjectMember("identifiers").expectStringMember("id").getValue(),
equalTo("ns.baz#String"));
assertThat(resourceWithAbsoluteIds.expectStringMember("read").getValue(),
equalTo("ns.resource.needing.ids#GetResourceNeedingAbsoluteShapeIds"));

assertThat(resourceWithRelativeIds.expectObjectMember("identifiers").expectStringMember("id").getValue(),
equalTo("MyResourceId"));
assertThat(resourceWithRelativeIds.expectStringMember("put").getValue(), equalTo("PutMyResource"));
assertThat(resourceWithRelativeIds.expectStringMember("read").getValue(), equalTo("GetMyResource"));
assertThat(resourceWithRelativeIds.expectStringMember("delete").getValue(), equalTo("DeleteMyResource"));
assertThat(resourceWithRelativeIds.expectArrayMember("collectionOperations")
.get(0).get().expectStringNode().getValue(),
equalTo("BatchDeleteMyResource"));

assertThat(structureWithAbsoluteIds.expectObjectMember("members").expectObjectMember("id")
.expectStringMember("target").getValue(),
equalTo("ns.baz#String"));

assertThat(structureWithMixedIds.expectObjectMember("members").expectObjectMember("a")
.expectStringMember("target").getValue(),
equalTo("MyString"));
assertThat(structureWithMixedIds.expectObjectMember("members").expectObjectMember("c")
.expectStringMember("target").getValue(),
equalTo("ns.shapes#String"));
}
}
42 changes: 42 additions & 0 deletions smithy-model/src/test/resources/software/amazon/smithy/model/shapes/test-model.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@
"a": {
"target": "MyString",
"required": true
},
"c": {
"target": "ns.shapes#String"
}
},
"documentation": "abc"
Expand All @@ -173,6 +176,41 @@
"a": {
"type": "structure",
"trait": true
},
"OperationNeedingAbsoluteShapeIds": {
"type": "operation",
"idempotent": true,
"input": "ns.shapes#Structure",
"output": "ns.shapes#Structure",
"errors": ["ns.shapes#ErrorStructure"]
},
"ResourceNeedingAbsoluteShapeIds": {
"type": "resource",
"identifiers": {"id": "ns.baz#String"},
"read": "ns.resource.needing.ids#GetResourceNeedingAbsoluteShapeIds"
}
}
},
"ns.resource.needing.ids": {
"shapes": {
"GetResourceNeedingAbsoluteShapeIds": {
"type": "operation",
"readonly": true,
"input": "GetResourceNeedingAbsoluteShapeIdsInput",
"output": "GetResourceNeedingAbsoluteShapeIdsOutput"
},
"GetResourceNeedingAbsoluteShapeIdsInput": {
"type": "structure",
"members": {
"id": {
"target": "ns.baz#String",
"required": true
}
}
},
"GetResourceNeedingAbsoluteShapeIdsOutput": {
"type": "structure",
"members": {}
}
}
},
Expand Down Expand Up @@ -251,6 +289,10 @@
}
}
},
"ErrorStructure": {
"type": "structure",
"error": "client"
},
"TaggedUnion": {
"type": "union",
"members": {
Expand Down

0 comments on commit 7001892

Please sign in to comment.