Fix resource identifier collision and validation

smithy-model/src/main/java/software/amazon/smithy/model/knowledge/IdentifierBindingIndex.java

@@ -21,8 +21,6 @@
 import java.util.Optional;
 import java.util.SortedSet;
 import java.util.TreeSet;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.shapes.MemberShape;
 import software.amazon.smithy.model.shapes.ResourceShape;
@@ -31,7 +29,6 @@
 import software.amazon.smithy.model.shapes.ToShapeId;
 import software.amazon.smithy.model.traits.RequiredTrait;
 import software.amazon.smithy.model.traits.ResourceIdentifierTrait;
-import software.amazon.smithy.utils.Pair;
 
 /**
  * Index of operation shapes to the identifiers bound to the operation.
@@ -155,25 +152,29 @@ private void processResource(ResourceShape resource, OperationIndex operationInd
     }
 
     private boolean isCollection(ResourceShape resource, ToShapeId operationId) {
-        return resource.getCollectionOperations().contains(operationId)
+        return resource.getCollectionOperations().contains(operationId.toShapeId())
                 || (resource.getCreate().isPresent() && resource.getCreate().get().toShapeId().equals(operationId))
                 || (resource.getList().isPresent() && resource.getList().get().toShapeId().equals(operationId));
     }
 
-    private boolean isImplicitIdentifierBinding(Map.Entry<String, MemberShape> memberEntry, ResourceShape resource) {
-        return resource.getIdentifiers().containsKey(memberEntry.getKey())
-                && memberEntry.getValue().getTrait(RequiredTrait.class).isPresent()
-                && memberEntry.getValue().getTarget().equals(resource.getIdentifiers().get(memberEntry.getKey()));
+    private boolean isImplicitIdentifierBinding(MemberShape member, ResourceShape resource) {
+        return resource.getIdentifiers().containsKey(member.getMemberName())
+                && member.getTrait(RequiredTrait.class).isPresent()
+                && member.getTarget().equals(resource.getIdentifiers().get(member.getMemberName()));
     }
 
     private Map<String, String> computeBindings(ResourceShape resource, StructureShape shape) {
-        return shape.getAllMembers().entrySet().stream()
-                .flatMap(entry -> entry.getValue().getTrait(ResourceIdentifierTrait.class)
-                        .map(trait -> Stream.of(Pair.of(trait.getValue(), entry.getKey())))
-                        .orElseGet(() -> isImplicitIdentifierBinding(entry, resource)
-                                ? Stream.of(Pair.of(entry.getKey(), entry.getKey()))
-                                : Stream.empty()))
-                .collect(Collectors.toMap(Pair::getLeft, Pair::getRight));
-
+        Map<String, String> bindings = new HashMap<>();
+        for (MemberShape member : shape.getAllMembers().values()) {
+            if (member.hasTrait(ResourceIdentifierTrait.class)) {
+                // Mark as a binding if the member has an explicit @resourceIdentifier trait.
+                String bindingName = member.expectTrait(ResourceIdentifierTrait.class).getValue();
+                bindings.put(bindingName, member.getMemberName());
+            } else if (isImplicitIdentifierBinding(member, resource)) {
+                // Mark as a binding if the member is an implicit identifier binding.
+                bindings.put(member.getMemberName(), member.getMemberName());
+            }
+        }
+        return bindings;
     }
 }

smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ResourceIdentifierBindingValidator.java

@@ -17,6 +17,10 @@
 
 import static java.lang.String.format;
 
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -26,9 +30,12 @@
 import java.util.stream.Stream;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.knowledge.IdentifierBindingIndex;
+import software.amazon.smithy.model.shapes.MemberShape;
 import software.amazon.smithy.model.shapes.OperationShape;
 import software.amazon.smithy.model.shapes.ResourceShape;
 import software.amazon.smithy.model.shapes.Shape;
+import software.amazon.smithy.model.shapes.ShapeId;
+import software.amazon.smithy.model.traits.ResourceIdentifierTrait;
 import software.amazon.smithy.model.validation.AbstractValidator;
 import software.amazon.smithy.model.validation.ValidationEvent;
 import software.amazon.smithy.model.validation.ValidationUtils;
@@ -38,7 +45,7 @@
 
 /**
  * Validates that operations bound to resource shapes have identifier
- * bindings for all of the identifiers of the parent of the binding resource,
+ * bindings for all the identifiers of the parent of the binding resource,
  * that operations bound to a resource with the {@code collection}
  * trait are bound using a collection binding, and operations bound with
  * no {@code collection} trait are bound using an instance binding.
@@ -47,16 +54,58 @@ public final class ResourceIdentifierBindingValidator extends AbstractValidator
 
     @Override
     public List<ValidationEvent> validate(Model model) {
-        IdentifierBindingIndex bindingIndex = IdentifierBindingIndex.of(model);
+        List<ValidationEvent> events = new ArrayList<>();
+        validateResourceIdentifierTraits(model, events);
+        validateOperationBindings(model, events);
+        return events;
+    }
 
-        return Stream.of(
+    // Check if this shape has conflicting resource identifier bindings due to trait bindings.
+    private void validateResourceIdentifierTraits(Model model, List<ValidationEvent> events) {
+        for (ShapeId container : findStructuresWithResourceIdentifierTraits(model)) {
+            Map<String, Set<String>> bindings = computePotentialStructureBindings(model, container);
+            for (Map.Entry<String, Set<String>> entry : bindings.entrySet()) {
+                if (entry.getValue().size() > 1) {
+                    events.add(error(model.expectShape(container), String.format(
+                            "Conflicting resource identifier member bindings found for identifier '%s' between "
+                            + "members %s", entry.getKey(), String.join(", ", entry.getValue()))));
+                }
+            }
+        }
+    }
+
+    private Set<ShapeId> findStructuresWithResourceIdentifierTraits(Model model) {
+        Set<ShapeId> containers = new HashSet<>();
+        for (MemberShape member : model.getMemberShapesWithTrait(ResourceIdentifierTrait.class)) {
+            containers.add(member.getContainer());
+        }
+        return containers;
+    }
+
+    private Map<String, Set<String>> computePotentialStructureBindings(Model model, ShapeId container) {
+        return model.getShape(container).map(struct -> {
+            Map<String, Set<String>> bindings = new HashMap<>();
+            // Ensure no two members are bound to the same identifier.
+            for (MemberShape member : struct.members()) {
+                String bindingName = member.getTrait(ResourceIdentifierTrait.class)
+                        .map(ResourceIdentifierTrait::getValue)
+                        .orElseGet(member::getMemberName);
+                bindings.computeIfAbsent(bindingName, k -> new HashSet<>()).add(member.getMemberName());
+            }
+            return bindings;
+        }).orElse(Collections.emptyMap());
+    }
+
+    private void validateOperationBindings(Model model, List<ValidationEvent> events) {
+        IdentifierBindingIndex bindingIndex = IdentifierBindingIndex.of(model);
+        Stream.of(
                 model.shapes(ResourceShape.class)
                         .flatMap(resource -> validateResource(model, resource, bindingIndex)),
                 model.shapes(ResourceShape.class)
                         .flatMap(resource -> validateCollectionBindings(model, resource, bindingIndex)),
                 model.shapes(ResourceShape.class)
                         .flatMap(resource -> validateInstanceBindings(model, resource, bindingIndex))
-        ).flatMap(Function.identity()).collect(Collectors.toList());
+        ).flatMap(Function.identity()).forEach(events::add);
     }
 
     private Stream<ValidationEvent> validateResource(

smithy-model/src/test/java/software/amazon/smithy/model/knowledge/IdentifierBindingIndexTest.java

@@ -142,4 +142,11 @@ public void findsExplicitBindings() {
                    equalTo(IdentifierBindingIndex.BindingType.INSTANCE));
         assertThat(index.getOperationInputBindings(resource.getId(), operation.getId()), equalTo(expectedBindings));
     }
+
+    @Test
+    public void doesNotFailWhenLoadingModelWithCollidingMemberBindings() {
+        // Ensure that this does not fail to load. This previously failed when using Collectors.toMap due to
+        // a collision in the keys used to map an identifier to multiple members.
+        Model.assembler().addImport(getClass().getResource("colliding-resource-identifiers.smithy")).assemble();
+    }
 }

smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-identifiers/detects-conflicting-bindings.errors

@@ -0,0 +1,2 @@
+[ERROR] smithy.example#GetFooInput: Conflicting resource identifier member bindings found for identifier 'bar' between members bar, bam | ResourceIdentifierBinding
+[ERROR] smithy.example#PutFooInput: Conflicting resource identifier member bindings found for identifier 'bar' between members a, b | ResourceIdentifierBinding

smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-identifiers/detects-conflicting-bindings.smithy

@@ -0,0 +1,37 @@
+$version: "2.0"
+
+namespace smithy.example
+
+resource Foo {
+    identifiers: {
+        bar: String
+    }
+    read: GetFoo
+    put: PutFoo
+}
+
+@readonly
+operation GetFoo {
+    input:= {
+        @required
+        bar: String
+
+        @resourceIdentifier("bar")
+        @required
+        bam: String
+    }
+}
+
+@idempotent
+operation PutFoo {
+    input:= {
+    @required
+        @resourceIdentifier("bar")
+        @required
+        a: String
+
+        @resourceIdentifier("bar")
+        @required
+        b: String
+    }
+}

smithy-model/src/test/resources/software/amazon/smithy/model/knowledge/colliding-resource-identifiers.smithy

@@ -0,0 +1,22 @@
+$version: "2.0"
+
+namespace smithy.example
+
+resource Foo {
+    identifiers: {
+        bar: String
+    }
+    read: GetFoo
+}
+
+@readonly
+operation GetFoo {
+    input:= {
+        @required
+        bar: String
+
+        @resourceIdentifier("bar")
+        @required
+        bam: String
+    }
+}