diff --git a/docs/source/spec/core.rst b/docs/source/spec/core.rst index 0841451423a..14a0374df2b 100644 --- a/docs/source/spec/core.rst +++ b/docs/source/spec/core.rst @@ -1025,10 +1025,29 @@ Service A :dfn:`service` is the entry point of an API that aggregates resources and operations together. The :ref:`resources ` and :ref:`operations ` of an API are bound within the closure of a -service. +service. A service is defined using a :token:`service_statement`. -A service shape is defined using a :token:`service_statement` and supports -the following properties: +.. tabs:: + + .. code-tab:: smithy + + service MyService { + version: "2017-02-11" + } + + .. code-tab:: json + + { + "smithy": "0.5.0", + "shapes": { + "smithy.example#MyService": { + "type": "service", + "version": "2017-02-11" + } + } + } + +The service shape supports the following members: .. list-table:: :header-rows: 1 @@ -1098,13 +1117,6 @@ that do not fit within a resource hierarchy. } } -**Validation** - -1. An operation MUST NOT be bound to multiple shapes within the closure of a - service. -2. Every operation shape contained within the entire closure of a service MUST - have a case-insensitively unique shape name, regardless of their namespaces. - .. _service-resources: @@ -1145,13 +1157,40 @@ shape ID of a resource to the ``resources`` property of a service. } } -**Validation** -1. A resource MUST NOT be bound to multiple shapes within the closure of a - service. -2. Every resource shape contained within the entire closure of a service MUST - have a case-insensitively unique shape name, regardless of their - namespaces. +.. _service-closure: + +Service closure +``````````````` + +The *closure* of a service is the set of shapes connected to a service +through resources, operations, and members. + +.. important:: + + With some exceptions, the shapes that are referenced in the *closure* + of a service MUST have case-insensitively unique names regardless of + their namespace. + +By requiring unique names within a service, each service forms a +`ubiquitous language`_, making it easier for developers to understand the +model and artifacts generated from the model, like code. For example, when +using Java code generated from a Smithy model, a developer should not need +to discern between ``BadRequestException`` classes across multiple packages +that can be thrown by an operation. Uniqueness is required +case-insensitively because many model transformations change the casing +and inflection of shape names to make artifacts more idiomatic. + +:ref:`Simple types ` and :ref:`lists ` or +:ref:`sets ` of compatible simple types are allowed to conflict because +a conflict for these type would rarely have an impact on generated artifacts. +These kinds of conflicts are only allowed if both conflicting shapes are the +same type and have the exact same traits. + +An operation or resource MUST NOT be bound to multiple shapes within the +closure of a service. This constraint allows services to discern between +operations and resources using only their shape name rather than a +fully-qualified path from the service to the shape. .. _operation: @@ -6190,3 +6229,4 @@ model: .. _ECMA 262 regular expression dialect: https://www.ecma-international.org/ecma-262/8.0/index.html#sec-patterns .. _RFC 3986 Host: https://tools.ietf.org/html/rfc3986#section-3.2.2 .. _CommonMark: https://spec.commonmark.org/ +.. _ubiquitous language: https://martinfowler.com/bliki/UbiquitousLanguage.html diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationUtils.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationUtils.java index 9f1aace3af9..64769562ee9 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationUtils.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationUtils.java @@ -98,6 +98,8 @@ public static String tickedList(Stream values) { public static Map> findDuplicateShapeNames(Collection shapes) { return shapes.stream() .map(ToShapeId::toShapeId) + // Exclude IDs with members since these need to be validated separately. + .filter(id -> !id.getMember().isPresent()) // Group by the lowercase name of each shape, and collect the shape IDs as strings. .collect(groupingBy(id -> id.getName().toLowerCase(Locale.US))) .entrySet().stream() diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ServiceValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ServiceValidator.java index bf54bd8af74..0f5cb1fd18e 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ServiceValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ServiceValidator.java @@ -1,5 +1,5 @@ /* - * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * Copyright 2020 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. @@ -16,55 +16,210 @@ package software.amazon.smithy.model.validation.validators; import java.util.ArrayList; +import java.util.Collections; +import java.util.EnumMap; +import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.stream.Collectors; import software.amazon.smithy.model.Model; -import software.amazon.smithy.model.knowledge.TopDownIndex; +import software.amazon.smithy.model.knowledge.NeighborProviderIndex; +import software.amazon.smithy.model.neighbor.Walker; +import software.amazon.smithy.model.shapes.CollectionShape; +import software.amazon.smithy.model.shapes.MemberShape; import software.amazon.smithy.model.shapes.ServiceShape; +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.SimpleShape; import software.amazon.smithy.model.validation.AbstractValidator; +import software.amazon.smithy.model.validation.Severity; import software.amazon.smithy.model.validation.ValidationEvent; import software.amazon.smithy.model.validation.ValidationUtils; +import software.amazon.smithy.utils.Pair; /** - * Validates that service shapes do not contain duplicate resource or - * operation shape names. + * Validates that service closures do not contain duplicate case-insensitive + * shape names. + * + *

This validator allows some kinds of conflicts when they are likely + * inconsequential. Some classes of conflicts are permitted, and in those + * cases a WARNING or NOTE is emitted. A conflict is permitted if the shapes + * are the same type; the two shapes are either a simple shape, list, or set; + * both shapes have the same exact traits; and both shapes have equivalent + * members (that is, the members follow these same rules). Permitted conflicts + * detected between simple shapes are emitted as a NOTE, permitted conflicts + * detected on other shapes are emitted as a WARNING, forbidden conflicts + * detected for an operation or resource are emitted as an ERROR, and all + * other forbidden conflicts are emitted as DANGER. */ public class ServiceValidator extends AbstractValidator { @Override public List validate(Model model) { - TopDownIndex topDownIndex = model.getKnowledge(TopDownIndex.class); return model.shapes(ServiceShape.class) - .flatMap(shape -> validateService(topDownIndex, shape).stream()) + .flatMap(shape -> validateService(model, shape).stream()) .collect(Collectors.toList()); } - private List validateService(TopDownIndex topDownIndex, ServiceShape shape) { - List events = new ArrayList<>(); + private List validateService(Model model, ServiceShape service) { + // Ensure that shapes bound to the service have unique shape names. + Walker walker = new Walker(model.getKnowledge(NeighborProviderIndex.class).getProvider()); + Set serviceClosure = walker.walkShapes(service); + Map> conflicts = ValidationUtils.findDuplicateShapeNames(serviceClosure); - // Ensure that resources bound to the service have unique shape names. - Map> duplicateResourceNames = ValidationUtils.findDuplicateShapeNames( - topDownIndex.getContainedResources(shape.getId())); - if (!duplicateResourceNames.isEmpty()) { - events.add(conflictingNames(shape, "resources", duplicateResourceNames)); + if (conflicts.isEmpty()) { + return Collections.emptyList(); } - // Ensure that operations bound to the service have unique shape names. - Map> duplicateOperationNames = ValidationUtils.findDuplicateShapeNames( - topDownIndex.getContainedOperations(shape)); - if (!duplicateOperationNames.isEmpty()) { - events.add(conflictingNames(shape, "operations", duplicateOperationNames)); + // Determine the severity of each conflict. + ConflictDetector detector = new ConflictDetector(model); + List events = new ArrayList<>(); + + // Figure out if each conflict can be ignored, and then emit events for + // both sides of the conflict using the appropriate severity. + for (Map.Entry> entry : conflicts.entrySet()) { + List ids = entry.getValue(); + for (int i = 0; i < ids.size(); i++) { + Shape subject = model.expectShape(ids.get(i)); + for (int j = 0; j < ids.size(); j++) { + if (i != j) { + Shape other = model.expectShape(ids.get(j)); + Severity severity = detector.detect(subject, other); + if (severity != null) { + events.add(conflictingNames(severity, service, subject, other)); + events.add(conflictingNames(severity, service, other, subject)); + } + } + } + } } return events; } - private ValidationEvent conflictingNames(ServiceShape shape, String descriptor, Map> dupes) { - return error(shape, String.format( - "All %s contained within a service hierarchy must have case-insensitively unique names regardless of " - + "their namespaces. The following %s were found in this service to have conflicting names: %s", - descriptor, descriptor, dupes)); + private ValidationEvent conflictingNames(Severity severity, ServiceShape shape, Shape subject, Shape other) { + // Whether it's a should or a must based on the severity. + String declaration = severity == Severity.DANGER || severity == Severity.ERROR ? "must" : "should"; + + return ValidationEvent.builder() + .eventId(getName()) + .severity(severity) + .shape(subject) + .message(String.format( + "Shape name `%s` conflicts with `%s` in the `%s` service closure. These shapes in the closure " + + "of a service %s have case-insensitively unique names regardless of their namespaces.", + subject.getId().getName(), + other.getId(), + shape.getId(), + declaration)) + .build(); + } + + private static final class ConflictDetector { + + private static final EnumMap FORBIDDEN = new EnumMap<>(ShapeType.class); + + static { + // Service types are never allowed to conflict. + FORBIDDEN.put(ShapeType.RESOURCE, Severity.ERROR); + FORBIDDEN.put(ShapeType.OPERATION, Severity.ERROR); + FORBIDDEN.put(ShapeType.SERVICE, Severity.ERROR); + // These aggregate types are never allowed to conflict either, but + // we will present the ability to suppress the violation if needed. + FORBIDDEN.put(ShapeType.MAP, Severity.DANGER); + FORBIDDEN.put(ShapeType.STRUCTURE, Severity.DANGER); + FORBIDDEN.put(ShapeType.UNION, Severity.DANGER); + } + + private final Model model; + private final Map, Severity> cache = new HashMap<>(); + + ConflictDetector(Model model) { + this.model = model; + } + + Severity detect(Shape a, Shape b) { + // Treat null values as allowed so that this validator just + // ignores cases where a member target is broken. + if (a == null || b == null) { + return null; + } + + // Create a normalized cache key since the comparison of a to b + // and b to a is the same result. + Pair cacheKey = a.getId().compareTo(b.getId()) < 0 + ? Pair.of(a.getId(), b.getId()) + : Pair.of(b.getId(), a.getId()); + + // Don't use computeIfAbsent here since we don't want to lock the HashMap. + // Computing if there is a conflict for aggregate shapes requires that + // both the aggregate and its members are checked recursively. + if (cache.containsKey(cacheKey)) { + return cache.get(cacheKey); + } + + Severity result = detectConflicts(a, b); + cache.put(cacheKey, result); + return result; + } + + private Severity detectConflicts(Shape a, Shape b) { + // Some types can never have conflicts since they're almost + // universally code generated as named types. + if (FORBIDDEN.containsKey(a.getType())) { + return FORBIDDEN.get(a.getType()); + } else if (FORBIDDEN.containsKey(b.getType())) { + return FORBIDDEN.get(b.getType()); + } + + // Conflicting shapes must have the same types. + if (a.getType() != b.getType()) { + return Severity.DANGER; + } + + // When shapes conflict, they must have the same traits. + if (!a.getAllTraits().equals(b.getAllTraits())) { + return Severity.DANGER; + } + + // Detect type-specific member conflicts. Return early if the + // severity is a greater violation than the remaining checks. + Severity memberConflict = detectMemberConflicts(a, b); + if (memberConflict == Severity.WARNING + || memberConflict == Severity.DANGER + || memberConflict == Severity.ERROR) { + return memberConflict; + } + + // Simple shape conflicts are almost always benign and can be + // ignored, so issue a NOTE instead of a WARNING. + if (a instanceof SimpleShape) { + return Severity.NOTE; + } + + return Severity.WARNING; + } + + private Severity detectMemberConflicts(Shape a, Shape b) { + if (a instanceof MemberShape) { + // Member shapes must have the same traits and they must + // target the same kind of shape. The target can be different + // as long as the targets are effectively the same. + MemberShape aMember = (MemberShape) a; + MemberShape bMember = (MemberShape) b; + Shape aTarget = model.getShape(aMember.getTarget()).orElse(null); + Shape bTarget = model.getShape(bMember.getTarget()).orElse(null); + return detect(aTarget, bTarget); + } else if (a instanceof CollectionShape) { + // Collections/map shapes can conflict if they have the same traits and members. + CollectionShape aCollection = (CollectionShape) a; + CollectionShape bCollection = (CollectionShape) b; + return detect(aCollection.getMember(), bCollection.getMember()); + } else { + return null; + } + } } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/service-conflict-validator.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/service-conflict-validator.errors new file mode 100644 index 00000000000..9a656535c66 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/service-conflict-validator.errors @@ -0,0 +1,23 @@ +[ERROR] ns.foo#OperationA: Shape name `OperationA` conflicts with `another.ns#OperationA` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[ERROR] another.ns#OperationA: Shape name `OperationA` conflicts with `ns.foo#OperationA` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[ERROR] ns.foo#ResourceA: Shape name `ResourceA` conflicts with `another.ns#ResourceA` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[ERROR] another.ns#ResourceA: Shape name `ResourceA` conflicts with `ns.foo#ResourceA` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[WARNING] ns.foo#ListA: Shape name `ListA` conflicts with `another.ns#ListA` in the `ns.foo#MyService` service closure. These shapes in the closure of a service should have case-insensitively unique names regardless of their namespaces. | Service +[WARNING] ns.foo#ListA: Shape name `ListA` conflicts with `even.more.ns#ListA` in the `ns.foo#MyService` service closure. These shapes in the closure of a service should have case-insensitively unique names regardless of their namespaces. | Service +[WARNING] another.ns#ListA: Shape name `ListA` conflicts with `even.more.ns#ListA` in the `ns.foo#MyService` service closure. These shapes in the closure of a service should have case-insensitively unique names regardless of their namespaces. | Service +[WARNING] another.ns#ListA: Shape name `ListA` conflicts with `ns.foo#ListA` in the `ns.foo#MyService` service closure. These shapes in the closure of a service should have case-insensitively unique names regardless of their namespaces. | Service +[WARNING] even.more.ns#ListA: Shape name `ListA` conflicts with `another.ns#ListA` in the `ns.foo#MyService` service closure. These shapes in the closure of a service should have case-insensitively unique names regardless of their namespaces. | Service +[WARNING] even.more.ns#ListA: Shape name `ListA` conflicts with `ns.foo#ListA` in the `ns.foo#MyService` service closure. These shapes in the closure of a service should have case-insensitively unique names regardless of their namespaces. | Service +[DANGER] ns.foo#ListB: Shape name `ListB` conflicts with `another.ns#ListB` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[DANGER] another.ns#ListB: Shape name `ListB` conflicts with `ns.foo#ListB` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[DANGER] ns.foo#ListC: Shape name `ListC` conflicts with `another.ns#ListC` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[DANGER] another.ns#ListC: Shape name `ListC` conflicts with `ns.foo#ListC` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[NOTE] ns.foo#String: Shape name `String` conflicts with `smithy.api#String` in the `ns.foo#MyService` service closure. These shapes in the closure of a service should have case-insensitively unique names regardless of their namespaces. | Service +[DANGER] ns.foo#Foo: Shape name `Foo` conflicts with `another.ns#Foo` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[DANGER] another.ns#Foo: Shape name `Foo` conflicts with `ns.foo#Foo` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[DANGER] ns.foo#Baz: Shape name `Baz` conflicts with `another.ns#Baz` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[DANGER] another.ns#Baz: Shape name `Baz` conflicts with `ns.foo#Baz` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[DANGER] ns.foo#MapA: Shape name `MapA` conflicts with `another.ns#MapA` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[DANGER] another.ns#MapA: Shape name `MapA` conflicts with `ns.foo#MapA` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[DANGER] ns.foo#Qux: Shape name `Qux` conflicts with `another.ns#Qux` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service +[DANGER] another.ns#Qux: Shape name `Qux` conflicts with `ns.foo#Qux` in the `ns.foo#MyService` service closure. These shapes in the closure of a service must have case-insensitively unique names regardless of their namespaces. | Service diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/service-conflict-validator.json b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/service-conflict-validator.json new file mode 100644 index 00000000000..b9c6c6c2393 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/service-conflict-validator.json @@ -0,0 +1,158 @@ +{ + "smithy": "0.5.0", + "shapes": { + "ns.foo#MyService": { + "type": "service", + "version": "2017-01-17", + "operations": [ + { + "target": "ns.foo#OperationA" + }, + { + "target": "another.ns#OperationA" + }, + { + "target": "ns.foo#OperationB" + } + ], + "resources": [ + { + "target": "ns.foo#ResourceA" + }, + { + "target": "another.ns#ResourceA" + }, + { + "target": "ns.foo#ResourceB" + } + ] + }, + "ns.foo#OperationA": { + "type": "operation" + }, + "another.ns#OperationA": { + "type": "operation" + }, + "ns.foo#OperationB": { + "type": "operation", + "input": { + "target": "ns.foo#OperationBInput" + } + }, + "ns.foo#ResourceA": { + "type": "resource" + }, + "another.ns#ResourceA": { + "type": "resource" + }, + "ns.foo#ResourceB": { + "type": "resource" + }, + "ns.foo#OperationBInput": { + "type": "structure", + "members": { + "allowedListConflict1": {"target": "ns.foo#ListA"}, + "allowedListConflict2": {"target": "another.ns#ListA"}, + "allowedListConflict3": {"target": "even.more.ns#ListA"}, + "conflictingListTargets1": {"target": "ns.foo#ListB"}, + "conflictingListTargets2": {"target": "another.ns#ListB"}, + "conflictingMemberTraits1": {"target": "ns.foo#ListC"}, + "conflictingMemberTraits2": {"target": "another.ns#ListC"}, + "conflictingTypes1": {"target": "ns.foo#Foo"}, + "conflictingTypes2": {"target": "another.ns#Foo"}, + "mapConflict1": {"target": "ns.foo#MapA"}, + "mapConflict2": {"target": "another.ns#MapA"}, + "structureConflict1": {"target": "ns.foo#Baz"}, + "structureConflict2": {"target": "another.ns#Baz"}, + "unionConflict1": {"target": "ns.foo#Qux"}, + "unionConflict2": {"target": "another.ns#Qux"} + } + }, + "ns.foo#ListA": { + "type": "list", + "member": { + "target": "ns.foo#String" + } + }, + "another.ns#ListA": { + "type": "list", + "member": { + "target": "ns.foo#String" + } + }, + "even.more.ns#ListA": { + "type": "list", + "member": { + "target": "smithy.api#String" + } + }, + "ns.foo#ListB": { + "type": "list", + "member": { + "target": "ns.foo#String" + } + }, + "another.ns#ListB": { + "type": "list", + "member": { + "target": "smithy.api#Integer" + } + }, + "ns.foo#ListC": { + "type": "list", + "member": { + "target": "ns.foo#String" + } + }, + "another.ns#ListC": { + "type": "list", + "member": { + "target": "ns.foo#String", + "traits": { + "smithy.api#deprecated": true + } + } + }, + "ns.foo#String": { + "type": "string" + }, + "ns.foo#Foo": { + "type": "structure" + }, + "another.ns#Foo": { + "type": "boolean" + }, + "ns.foo#Baz": { + "type": "structure" + }, + "another.ns#Baz": { + "type": "structure" + }, + "ns.foo#MapA": { + "type": "map", + "key": {"target": "smithy.api#String"}, + "value": {"target": "smithy.api#String"} + }, + "another.ns#MapA": { + "type": "map", + "key": {"target": "smithy.api#String"}, + "value": {"target": "smithy.api#String"} + }, + "ns.foo#Qux": { + "type": "union", + "members": { + "a": { + "target": "smithy.api#String" + } + } + }, + "another.ns#Qux": { + "type": "union", + "members": { + "a": { + "target": "smithy.api#String" + } + } + } + } +} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/target-validator.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/target-validator.errors index 801a679c06f..98402235a9c 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/target-validator.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/target-validator.errors @@ -22,3 +22,5 @@ [ERROR] ns.foo#InvalidResourceLifecycle: Resource update lifecycle operation must target an operation, but found (integer: `ns.foo#Integer`) | Target [ERROR] ns.foo#InvalidResourceLifecycle: resource shape `operation` relationships must target a operation shape, but found (integer: `ns.foo#Integer`) | Target [ERROR] ns.foo#InvalidTraitReference$member: Found a MEMBER_TARGET reference to trait definition `ns.foo#fooTrait`. Trait definitions cannot be targeted by members or referenced by shapes in any other context other than applying them as traits. | Target +[NOTE] ns.foo#String: Shape name `String` conflicts with `another.ns#String` in the `ns.foo#MyService` service closure. These shapes in the closure of a service should have case-insensitively unique names regardless of their namespaces. | Service +[NOTE] another.ns#String: Shape name `String` conflicts with `ns.foo#String` in the `ns.foo#MyService` service closure. These shapes in the closure of a service should have case-insensitively unique names regardless of their namespaces. | Service