Lower resource identifier collisions to warning

docs/source-1.0/spec/core/model.rst

@@ -1950,6 +1950,10 @@ on ``GetHistoricalForecastInput$customHistoricalIdName`` maps that member
 to the "historicalId" identifier.
 
 
+If an operation input supplies both an explicit and an implicit identifier
+binding, the explicit identifier binding is utilized.
+
+
 .. _lifecycle-operations:
 
 Resource lifecycle operations

docs/source-2.0/spec/service-types.rst

@@ -678,6 +678,10 @@ maps it to the "forecastId" identifier is provided by the
 on ``GetHistoricalForecastInput$customHistoricalIdName`` maps that member
 to the "historicalId" identifier.
 
+If an operation input supplies both an explicit and an implicit identifier
+binding, the explicit identifier binding is utilized.
+
+
 .. _resource-properties:
 
 Resource Properties

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

@@ -169,10 +169,12 @@ private Map<String, String> computeBindings(ResourceShape resource, StructureSha
             if (member.hasTrait(ResourceIdentifierTrait.class)) {
                 // Mark as a binding if the member has an explicit @resourceIdentifier trait.
                 String bindingName = member.expectTrait(ResourceIdentifierTrait.class).getValue();
+                // Override any implicit bindings with explicit trait bindings.
                 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());
+                // Only utilize implicit bindings when an explicit binding wasn't found.
+                bindings.putIfAbsent(member.getMemberName(), member.getMemberName());
             }
         }
         return bindings;

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

@@ -18,7 +18,6 @@
 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;
@@ -63,14 +62,25 @@ public List<ValidationEvent> validate(Model model) {
     // 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);
+            if (!model.getShape(container).isPresent()) {
+                continue;
+            }
+
+            Shape structure = model.expectShape(container);
+            Map<String, Set<String>> bindings = computePotentialStructureBindings(structure);
             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()))));
+                // Only emit this event if the potential bindings contains
+                // more than the implicit binding.
+                if (entry.getValue().size() > 1 && entry.getValue().contains(entry.getKey())) {
+                    Set<String> explicitBindings = entry.getValue();
+                    explicitBindings.remove(entry.getKey());
+                    events.add(warning(structure, String.format(
+                            "Implicit resource identifier for '%s' is overridden by `resourceIdentifier` trait on "
+                            + "members: '%s'", entry.getKey(), String.join("', '", explicitBindings))));
                 }
             }
+
+            validateResourceIdentifierTraitConflicts(structure, events);
         }
     }
 
@@ -82,18 +92,36 @@ private Set<ShapeId> findStructuresWithResourceIdentifierTraits(Model model) {
         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());
+    private Map<String, Set<String>> computePotentialStructureBindings(Shape structure) {
+        Map<String, Set<String>> bindings = new HashMap<>();
+        // Ensure no two members are bound to the same identifier.
+        for (MemberShape member : structure.members()) {
+            String bindingName = member.getTrait(ResourceIdentifierTrait.class)
+                    .map(ResourceIdentifierTrait::getValue)
+                    .orElseGet(member::getMemberName);
+            bindings.computeIfAbsent(bindingName, k -> new HashSet<>()).add(member.getMemberName());
+        }
+        return bindings;
+    }
+
+    private void validateResourceIdentifierTraitConflicts(Shape structure, List<ValidationEvent> events) {
+        Map<String, Set<String>> explicitBindings = new HashMap<>();
+        // Ensure no two members use a resourceIdentifier trait to bind to
+        // the same identifier.
+        for (MemberShape member : structure.members()) {
+            if (member.hasTrait(ResourceIdentifierTrait.class)) {
+                explicitBindings.computeIfAbsent(member.expectTrait(ResourceIdentifierTrait.class).getValue(),
+                        k -> new HashSet<>()).add(member.getMemberName());
             }
-            return bindings;
-        }).orElse(Collections.emptyMap());
+        }
+
+        for (Map.Entry<String, Set<String>> entry : explicitBindings.entrySet()) {
+            if (entry.getValue().size() > 1) {
+                events.add(error(structure, String.format(
+                        "Conflicting resource identifier member bindings found for identifier '%s' between "
+                        + "members: '%s'", entry.getKey(), String.join("', '", entry.getValue()))));
+            }
+        }
     }
 
     private void validateOperationBindings(Model model, List<ValidationEvent> events) {

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

@@ -33,6 +33,7 @@
 import software.amazon.smithy.model.traits.ReadonlyTrait;
 import software.amazon.smithy.model.traits.RequiredTrait;
 import software.amazon.smithy.model.traits.ResourceIdentifierTrait;
+import software.amazon.smithy.utils.MapUtils;
 
 public class IdentifierBindingIndexTest {
 
@@ -144,9 +145,19 @@ public void findsExplicitBindings() {
     }
 
     @Test
-    public void doesNotFailWhenLoadingModelWithCollidingMemberBindings() {
+    public void explicitBindingsPreferred() {
         // 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();
+        Model model = Model.assembler()
+                .addImport(getClass().getResource("colliding-resource-identifiers.smithy"))
+                .assemble()
+                .unwrap();
+        IdentifierBindingIndex index = IdentifierBindingIndex.of(model);
+
+        // Ensure that the explicit trait bindings are preferred over implicit bindings.
+        assertThat(index.getOperationInputBindings(
+                        ShapeId.from("smithy.example#Foo"),
+                        ShapeId.from("smithy.example#GetFoo")),
+                equalTo(MapUtils.of("bar", "bam")));
     }
 }

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

@@ -1,2 +1,4 @@
-[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
+[WARNING] smithy.example#GetFooInput: Implicit resource identifier for 'bar' is overridden by `resourceIdentifier` trait on members: 'bam' | ResourceIdentifierBinding
+[ERROR] smithy.example#PutFooInput: Conflicting resource identifier member bindings found for identifier 'bar' between members: 'a', 'b' | ResourceIdentifierBinding
+[ERROR] smithy.example#DeleteFooInput: Conflicting resource identifier member bindings found for identifier 'bar' between members: 'a', 'b' | ResourceIdentifierBinding
+[WARNING] smithy.example#DeleteFooInput: Implicit resource identifier for 'bar' is overridden by `resourceIdentifier` trait on members: 'a', 'b' | ResourceIdentifierBinding

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

@@ -8,6 +8,7 @@ resource Foo {
     }
     read: GetFoo
     put: PutFoo
+    delete: DeleteFoo
 }
 
 @readonly
@@ -25,7 +26,22 @@ operation GetFoo {
 @idempotent
 operation PutFoo {
     input:= {
-    @required
+        @resourceIdentifier("bar")
+        @required
+        a: String
+
+        @resourceIdentifier("bar")
+        @required
+        b: String
+    }
+}
+
+@idempotent
+operation DeleteFoo {
+    input:= {
+        @required
+        bar: String
+
         @resourceIdentifier("bar")
         @required
         a: String