Skip to content

Commit

Permalink
Allow pagination tokens to be maps
Browse files Browse the repository at this point in the history
This relaxes the constraints of pagination tokens to allow maps. This
is intended to enable existing services in the wild and is not
recommended for future services, so a DANGER validation event is
emitted.
  • Loading branch information
JordonPhillips committed Nov 11, 2020
1 parent 83cceca commit a7808dd
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 14 deletions.
7 changes: 4 additions & 3 deletions docs/source/1.0/spec/core/behavior-traits.rst
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,8 @@ The ``paginated`` trait is a structure that contains the following members:
- The name of the operation input member that contains a continuation
token. When this value is provided as input, the service returns
results from where the previous response left off. This input member
MUST NOT be marked as ``required`` and MUST target a string shape.
MUST NOT be marked as ``required`` and SHOULD target a string shape.
It can, but SHOULD NOT target a map shape.

When contained within a service, a paginated operation MUST either
configure ``inputToken`` on the operation itself or inherit it from
Expand All @@ -206,8 +207,8 @@ The ``paginated`` trait is a structure that contains the following members:
it indicates that there are more results to retrieve. To get the next
page of results, the client passes the received output continuation
token to the input continuation token of the next request. This
output member MUST NOT be marked as ``required`` and MUST target a
string shape.
output member MUST NOT be marked as ``required`` and SHOULD target a
string shape. It can, but SHOULD NOT target a map shape.

When contained within a service, a paginated operation MUST either
configure ``outputToken`` on the operation itself or inherit it from
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.TreeSet;
import java.util.regex.Pattern;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.knowledge.OperationIndex;
Expand Down Expand Up @@ -57,7 +58,8 @@
public final class PaginatedTraitValidator extends AbstractValidator {
private static final Set<ShapeType> ITEM_SHAPES = SetUtils.of(ShapeType.LIST, ShapeType.MAP);
private static final Set<ShapeType> PAGE_SHAPES = SetUtils.of(ShapeType.INTEGER);
private static final Set<ShapeType> STRING_SET = SetUtils.of(ShapeType.STRING);
private static final Set<ShapeType> TOKEN_SHAPES = SetUtils.of(ShapeType.STRING, ShapeType.MAP);
private static final Set<ShapeType> DANGER_TOKEN_SHAPES = SetUtils.of(ShapeType.MAP);
private static final Pattern PATH_PATTERN = Pattern.compile("\\.");

@Override
Expand Down Expand Up @@ -160,12 +162,23 @@ private List<ValidationEvent> validateMember(
}

Shape target = model.getShape(member.getTarget()).orElse(null);
if (target != null && !validator.validTargets().contains(target.getType())) {
events.add(error(operation, trait, String.format(
"%spaginated trait `%s` member `%s` targets a %s shape, but must target one of "
+ "the following: [%s]",
prefix, validator.propertyName(), member.getId().getName(), target.getType(),
ValidationUtils.tickedList(validator.validTargets()))));
if (target != null) {
if (!validator.validTargets().contains(target.getType())) {
events.add(error(operation, trait, String.format(
"%spaginated trait `%s` member `%s` targets a %s shape, but must target one of "
+ "the following: [%s]",
prefix, validator.propertyName(), member.getId().getName(), target.getType(),
ValidationUtils.tickedList(validator.validTargets()))));
}
if (validator.dangerTargets().contains(target.getType())) {
Set<ShapeType> preferredTargets = new TreeSet<>(validator.validTargets());
preferredTargets.removeAll(validator.dangerTargets());
events.add(danger(operation, trait, String.format(
"%spaginated trait `%s` member `%s` targets a %s shape, but this is not recommended. "
+ "One of [%s] SHOULD be targeted.",
prefix, validator.propertyName(), member.getId().getName(), target.getType(),
ValidationUtils.tickedList(preferredTargets))));
}
}

if (validator.pathsAllowed() && PATH_PATTERN.split(memberPath).length > 2) {
Expand All @@ -188,6 +201,10 @@ private abstract static class PropertyValidator {

abstract Set<ShapeType> validTargets();

Set<ShapeType> dangerTargets() {
return Collections.emptySet();
}

abstract Optional<String> getMemberPath(OperationIndex opIndex, OperationShape operation, PaginatedTrait trait);

abstract Optional<MemberShape> getMember(
Expand Down Expand Up @@ -229,7 +246,11 @@ String propertyName() {
}

Set<ShapeType> validTargets() {
return STRING_SET;
return TOKEN_SHAPES;
}

Set<ShapeType> dangerTargets() {
return DANGER_TOKEN_SHAPES;
}

Optional<String> getMemberPath(OperationIndex opIndex, OperationShape operation, PaginatedTrait trait) {
Expand Down Expand Up @@ -258,7 +279,11 @@ String propertyName() {
}

Set<ShapeType> validTargets() {
return STRING_SET;
return TOKEN_SHAPES;
}

Set<ShapeType> dangerTargets() {
return DANGER_TOKEN_SHAPES;
}

Optional<String> getMemberPath(OperationIndex opIndex, OperationShape operation, PaginatedTrait trait) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
[ERROR] ns.foo#Invalid1: paginated operations require an input | PaginatedTrait
[ERROR] ns.foo#Invalid2: paginated trait `inputToken` targets a member `nextToken` that does not exist | PaginatedTrait
[ERROR] ns.foo#Invalid2: paginated trait `outputToken` targets a member `nextToken` that does not exist | PaginatedTrait
[ERROR] ns.foo#Invalid3: paginated trait `inputToken` member `InputNotString` targets a integer shape, but must target one of the following: [`string`] | PaginatedTrait
[ERROR] ns.foo#Invalid3: paginated trait `inputToken` member `InputNotString` targets a integer shape, but must target one of the following: [`map`, `string`] | PaginatedTrait
[ERROR] ns.foo#Invalid3: paginated trait `items` targets a member `items` that does not exist | PaginatedTrait
[ERROR] ns.foo#Invalid3: paginated trait `outputToken` member `OutputNotString` targets a integer shape, but must target one of the following: [`string`] | PaginatedTrait
[ERROR] ns.foo#Invalid3: paginated trait `outputToken` member `OutputNotString` targets a integer shape, but must target one of the following: [`map`, `string`] | PaginatedTrait
[ERROR] ns.foo#Invalid4: paginated trait `inputToken` member `nextToken` must not be required | PaginatedTrait
[ERROR] ns.foo#Invalid4: paginated trait `outputToken` member `nextToken` must not be required | PaginatedTrait
[WARNING] ns.foo#Invalid4: paginated trait `pageSize` member `pageSize` should not be required | PaginatedTrait
Expand All @@ -22,3 +22,5 @@
[WARNING] ns.foo#DeeplyNestedOutputOperation: paginated trait `outputToken` contains a path with more than two parts, which can make your API cumbersome to use | PaginatedTrait
[ERROR] ns.foo#InvalidNestedInput: paginated trait `inputToken` does not allow path values | PaginatedTrait
[ERROR] ns.foo#InvalidNestedInput: paginated trait `pageSize` does not allow path values | PaginatedTrait
[DANGER] ns.foo#MapTokens: paginated trait `inputToken` member `MapTokenInputOutput` targets a map shape, but this is not recommended. One of [`string`] SHOULD be targeted. | PaginatedTrait
[DANGER] ns.foo#MapTokens: paginated trait `outputToken` member `MapTokenInputOutput` targets a map shape, but this is not recommended. One of [`string`] SHOULD be targeted. | PaginatedTrait
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@
},
{
"target": "ns.foo#InvalidNestedInput"
},
{
"target": "ns.foo#MapTokens"
}
]
},
Expand Down Expand Up @@ -589,6 +592,39 @@
"target": "ns.foo#StringList"
}
}
},
"ns.foo#MapTokens": {
"type": "operation",
"input": {
"target": "ns.foo#MapTokenInputOutput"
},
"output": {
"target": "ns.foo#MapTokenInputOutput"
},
"traits": {
"smithy.api#readonly": {},
"smithy.api#paginated": {
"inputToken": "nextToken",
"outputToken": "nextToken"
}
}
},
"ns.foo#MapTokenInputOutput": {
"type": "structure",
"members": {
"nextToken": {
"target": "ns.foo#StringMap"
}
}
},
"ns.foo#StringMap": {
"type": "map",
"key": {
"target": "smithy.api#String"
},
"value": {
"target": "smithy.api#String"
}
}
}
}

0 comments on commit a7808dd

Please sign in to comment.