Lower resource identifier collisions to warning

This commit updates the validation for resource identifier collisions
to warn instead of error. When an explicit and implicit binding for
the same resource identifier are found, the explicit binding is
preferred.

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 @@ public final class IdentifierBindingIndex implements KnowledgeIndex {
             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 @@ package software.amazon.smithy.model.validation.validators;
 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 final class ResourceIdentifierBindingValidator extends AbstractValidator
     // 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 @@ public final class ResourceIdentifierBindingValidator extends AbstractValidator
         return containers;
     }
 
-    private Map<String, Set<String>> computePotentialStructureBindings(Model model, ShapeId container) {
-        return model.getShape(container).map(struct -> {
+    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 : struct.members()) {
+        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;
-        }).orElse(Collections.emptyMap());
+    }
+
+    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());
+            }
+        }
+
+        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.shapes.StructureShape;
 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 class IdentifierBindingIndexTest {
     }
 
     @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