Repository: smithy-lang/smithy
Pull request #2450 (2024-11-12 07:22:13)

Infer defined condition key service
This commit relaxes the pattern on the `@defineConditionKeys` trait's keys to enable inferring the `service` to be the service's `arnNamespace`.


---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.



Patch:
diff --git a/docs/source-2.0/aws/aws-iam.rst b/docs/source-2.0/aws/aws-iam.rst
index ad0c2a8cb..2c24302d3 100644
--- a/docs/source-2.0/aws/aws-iam.rst
+++ b/docs/source-2.0/aws/aws-iam.rst
@@ -402,10 +402,11 @@ Value type
     ``map`` of IAM identifiers to condition key ``structure``
 
 The ``aws.iam#defineConditionKeys`` trait defines additional condition keys
-that appear within a service. Keys in the map must be valid IAM identifiers,
-meaning they must adhere to the following regular expression:
-``"^([A-Za-z0-9][A-Za-z0-9-\\.]{0,62}:[^:]+)$"``.
-Each condition key structure supports the following members:
+that appear within a service. Keys in the map must be valid IAM identifiers
+or names of condition keys, meaning they must adhere to the following regular
+expression: ``"^(([A-Za-z0-9][A-Za-z0-9-\\.]{0,62}:)?[^:\\s]+)$"``. If only a
+condition key name is specified, the service is inferred to be the
+``arnNamespace``. Each condition key structure supports the following members:
 
 .. list-table::
     :header-rows: 1
diff --git a/smithy-aws-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeysIndex.java b/smithy-aws-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeysIndex.java
index 346616d51..be566e909 100644
--- a/smithy-aws-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeysIndex.java
+++ b/smithy-aws-iam-traits/src/main/java/software/amazon/smithy/aws/iam/traits/ConditionKeysIndex.java
@@ -24,6 +24,7 @@ import software.amazon.smithy.aws.traits.ArnReferenceTrait;
 import software.amazon.smithy.aws.traits.ServiceTrait;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.knowledge.KnowledgeIndex;
+import software.amazon.smithy.model.shapes.OperationShape;
 import software.amazon.smithy.model.shapes.ResourceShape;
 import software.amazon.smithy.model.shapes.ServiceShape;
 import software.amazon.smithy.model.shapes.Shape;
@@ -47,34 +48,38 @@ public final class ConditionKeysIndex implements KnowledgeIndex {
     private final Map<ShapeId, Map<ShapeId, Set<String>>> resourceConditionKeys = new HashMap<>();
 
     public ConditionKeysIndex(Model model) {
-        model.shapes(ServiceShape.class).forEach(service -> {
-            service.getTrait(ServiceTrait.class).ifPresent(trait -> {
-                // Copy over the explicitly defined condition keys into the service map.
-                // This will be mutated when adding inferred resource condition keys.
-                serviceConditionKeys.put(service.getId(), new HashMap<>(
-                        service.getTrait(DefineConditionKeysTrait.class)
-                                .map(DefineConditionKeysTrait::getConditionKeys)
-                                .orElse(MapUtils.of())));
-                resourceConditionKeys.put(service.getId(), new HashMap<>());
+        for (ServiceShape service : model.getServiceShapesWithTrait(ServiceTrait.class)) {
+            // Defines the scoping of any derived condition keys.
+            String arnNamespace = service.expectTrait(ServiceTrait.class).getArnNamespace();
 
-                // Defines the scoping of any derived condition keys.
-                String arnRoot = trait.getArnNamespace();
+            // Copy over the explicitly defined condition keys into the service map.
+            // This will be mutated when adding inferred resource condition keys.
+            Map<String, ConditionKeyDefinition> serviceKeys = new HashMap<>();
+            if (service.hasTrait(DefineConditionKeysTrait.ID)) {
+                DefineConditionKeysTrait trait = service.expectTrait(DefineConditionKeysTrait.class);
+                for (Map.Entry<String, ConditionKeyDefinition> entry : trait.getConditionKeys().entrySet()) {
+                    // If no colon is present, we infer that this condition key is for the
+                    // current service and apply its ARN namespace.
+                    String key = entry.getKey();
+                    if (!key.contains(":")) {
+                        key = arnNamespace + ":" + key;
+                    }
+                    serviceKeys.put(key, entry.getValue());
+                }
+            }
+            serviceConditionKeys.put(service.getId(), serviceKeys);
+            resourceConditionKeys.put(service.getId(), new HashMap<>());
 
-                // Compute the keys of child resources.
-                service.getResources().stream()
-                        .flatMap(id -> OptionalUtils.stream(model.getShape(id)))
-                        .forEach(resource -> {
-                            compute(model, service, arnRoot, resource, null);
-                        });
+            // Compute the keys of child resources.
+            for (ShapeId resourceId : service.getResources()) {
+                compute(model, service, arnNamespace, model.expectShape(resourceId, ResourceShape.class), null);
+            }
 
-                // Compute the keys of operations of the service.
-                service.getOperations().stream()
-                        .flatMap(id -> OptionalUtils.stream(model.getShape(id)))
-                        .forEach(operation -> {
-                            compute(model, service, arnRoot, operation, null);
-                        });
-            });
-        });
+            // Compute the keys of operations of the service.
+            for (ShapeId operationId : service.getOperations()) {
+                compute(model, service, arnNamespace, model.expectShape(operationId, OperationShape.class), null);
+            }
+        }
     }
 
     public static ConditionKeysIndex of(Model model) {
diff --git a/smithy-aws-iam-traits/src/main/resources/META-INF/smithy/aws.iam.smithy b/smithy-aws-iam-traits/src/main/resources/META-INF/smithy/aws.iam.smithy
index 144ad78e4..d0c9530c0 100644
--- a/smithy-aws-iam-traits/src/main/resources/META-INF/smithy/aws.iam.smithy
+++ b/smithy-aws-iam-traits/src/main/resources/META-INF/smithy/aws.iam.smithy
@@ -28,7 +28,7 @@ string conditionKeyValue
 /// inferred and global condition keys.
 @trait(selector: "service")
 map defineConditionKeys {
-    key: IamIdentifier
+    key: ConditionKeyName
     value: ConditionKeyDefinition
 }
 
@@ -156,6 +156,10 @@ list ResourceNameList {
     member: ResourceName
 }
 
+@private
+@pattern("^(([A-Za-z0-9][A-Za-z0-9-\\.]{0,62}:)?[^:\\s]+)$")
+string ConditionKeyName
+
 /// The IAM policy type of the value that will supplied for this context key
 @private
 enum ConditionKeyType {
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/transform/CreateDedicatedInputAndOutput.java b/smithy-model/src/main/java/software/amazon/smithy/model/transform/CreateDedicatedInputAndOutput.java
index 879b83602..862929c87 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/transform/CreateDedicatedInputAndOutput.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/transform/CreateDedicatedInputAndOutput.java
@@ -147,7 +147,7 @@ final class CreateDedicatedInputAndOutput {
             return struct;
         }
 
-        LOGGER.info(() -> "Renaming " + struct.getId() + " to " + expectedName);
+        LOGGER.fine(() -> "Renaming " + struct.getId() + " to " + expectedName);
         ShapeId newId = createSyntheticShapeId(model, operation, suffix);
 
         return struct.toBuilder()
@@ -229,7 +229,7 @@ final class CreateDedicatedInputAndOutput {
     private static ShapeId resolveConflict(ShapeId id, String suffix) {
         // Say GetFooRequest exists. This then returns GetFooOperationRequest.
         String updatedName = id.getName().replace(suffix, "Operation" + suffix);
-        LOGGER.info(() -> "Deconflicting synthetic ID from " + id + " to use name " + updatedName);
+        LOGGER.fine(() -> "Deconflicting synthetic ID from " + id + " to use name " + updatedName);
         return ShapeId.fromParts(id.getNamespace(), updatedName);
     }
 }
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/XmlFlattenedTraitValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/XmlFlattenedTraitValidator.java
new file mode 100644
index 000000000..6a5dc14e8
--- /dev/null
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/XmlFlattenedTraitValidator.java
@@ -0,0 +1,54 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package software.amazon.smithy.model.validation.validators;
+
+import java.util.ArrayList;
+import java.util.List;
+import software.amazon.smithy.model.Model;
+import software.amazon.smithy.model.shapes.ListShape;
+import software.amazon.smithy.model.shapes.MemberShape;
+import software.amazon.smithy.model.shapes.Shape;
+import software.amazon.smithy.model.traits.XmlFlattenedTrait;
+import software.amazon.smithy.model.traits.XmlNameTrait;
+import software.amazon.smithy.model.validation.AbstractValidator;
+import software.amazon.smithy.model.validation.ValidationEvent;
+
+/**
+ * Validates that xmlFlattened members aren't unintentionally ignoring the
+ * xmlName of their targets.
+ */
+public final class XmlFlattenedTraitValidator extends AbstractValidator {
+    @Override
+    public List<ValidationEvent> validate(Model model) {
+        List<ValidationEvent> events = new ArrayList<>();
+        for (MemberShape member : model.getMemberShapesWithTrait(XmlFlattenedTrait.class)) {
+            // Don't emit the event if they're being explicit about the xmlName on this member
+            if (member.hasTrait(XmlNameTrait.class)) {
+                continue;
+            }
+
+            Shape target = model.expectShape(member.getTarget());
+            if (target instanceof ListShape) {
+                ListShape targetList = (ListShape) target;
+                validateMemberTargetingList(member, targetList, events);
+            }
+        }
+        return events;
+    }
+
+    private void validateMemberTargetingList(MemberShape member, ListShape targetList, List<ValidationEvent> events) {
+        if (targetList.getMember().hasTrait(XmlNameTrait.class)) {
+            XmlNameTrait xmlName = targetList.getMember().expectTrait(XmlNameTrait.class);
+            if (!member.getMemberName().equals(xmlName.getValue())) {
+                events.add(warning(member, String.format(
+                        "Member is `@xmlFlattened`, so `@xmlName` of target's member (`%s`) has no effect."
+                        + " The flattened list elements will have the name of this member - `%s`. If this"
+                        + " is unintended, you can add `@xmlName(\"%s\")` to this member.",
+                        targetList.getMember().getId(), member.getMemberName(), xmlName.getValue())));
+            }
+        }
+    }
+}
diff --git a/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator b/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator
index a43408752..4b76f0194 100644
--- a/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator
+++ b/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator
@@ -59,3 +59,4 @@ software.amazon.smithy.model.validation.validators.UnitTypeValidator
 software.amazon.smithy.model.validation.validators.UnstableTraitValidator
 software.amazon.smithy.model.validation.validators.XmlNamespaceTraitValidator
 software.amazon.smithy.model.validation.validators.TraitValidatorsValidator
+software.amazon.smithy.model.validation.validators.XmlFlattenedTraitValidator


Test patch:
diff --git a/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/ConditionKeysIndexTest.java b/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/ConditionKeysIndexTest.java
index d5e7f9c8b..743e6449a 100644
--- a/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/ConditionKeysIndexTest.java
+++ b/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/ConditionKeysIndexTest.java
@@ -40,9 +40,9 @@ public class ConditionKeysIndexTest {
 
         ConditionKeysIndex index = ConditionKeysIndex.of(model);
         assertThat(index.getConditionKeyNames(service), containsInAnyOrder(
-                "aws:accountId", "foo:baz", "myservice:Resource1Id1", "myservice:ResourceTwoId2"));
+                "aws:accountId", "foo:baz", "myservice:Resource1Id1", "myservice:ResourceTwoId2", "myservice:bar"));
         assertThat(index.getConditionKeyNames(service, ShapeId.from("smithy.example#Operation1")),
-                containsInAnyOrder("aws:accountId", "foo:baz"));
+                containsInAnyOrder("aws:accountId", "myservice:bar"));
         assertThat(index.getConditionKeyNames(service, ShapeId.from("smithy.example#Resource1")),
                 containsInAnyOrder("aws:accountId", "foo:baz", "myservice:Resource1Id1"));
         // Note that ID1 is not duplicated but rather reused on the child operation.
diff --git a/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/DefineConditionKeysTraitTest.java b/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/DefineConditionKeysTraitTest.java
index 5ab1dab9c..146f64920 100644
--- a/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/DefineConditionKeysTraitTest.java
+++ b/smithy-aws-iam-traits/src/test/java/software/amazon/smithy/aws/iam/traits/DefineConditionKeysTraitTest.java
@@ -32,7 +32,7 @@ public class DefineConditionKeysTraitTest {
         DefineConditionKeysTrait trait = shape.expectTrait(DefineConditionKeysTrait.class);
         assertEquals(3,trait.getConditionKeys().size());
         assertFalse(trait.getConditionKey("myservice:Bar").get().isRequired());
-        assertFalse(trait.getConditionKey("myservice:Foo").get().isRequired());
+        assertFalse(trait.getConditionKey("Foo").get().isRequired());
         assertTrue(trait.getConditionKey("myservice:Baz").get().isRequired());
     }
 }
diff --git a/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/define-condition-keys.smithy b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/define-condition-keys.smithy
index e26626550..23a604cc2 100644
--- a/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/define-condition-keys.smithy
+++ b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/define-condition-keys.smithy
@@ -19,7 +19,7 @@ use aws.iam#serviceResolvedConditionKeys
         externalDocumentation: "http://baz.com"
         required: true
     }
-    "myservice:Foo": {
+    "Foo": {
         type: "String"
         documentation: "The Foo string"
         externalDocumentation: "http://foo.com"
diff --git a/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/successful-condition-keys.smithy b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/successful-condition-keys.smithy
index f6a209a05..27a71a68e 100644
--- a/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/successful-condition-keys.smithy
+++ b/smithy-aws-iam-traits/src/test/resources/software/amazon/smithy/aws/iam/traits/successful-condition-keys.smithy
@@ -1,4 +1,4 @@
-$version: "1.0"
+$version: "2.0"
 namespace smithy.example
 
 use aws.api#arnReference
@@ -8,39 +8,44 @@ use aws.iam#defineConditionKeys
 use aws.iam#disableConditionKeyInference
 use aws.iam#iamResource
 
-@service(sdkId: "My")
+@service(sdkId: "My", arnNamespace: "myservice")
 @defineConditionKeys(
   "foo:baz": {
-    type: "String",
-    documentation: "Foo baz",
+    type: "String"
+    documentation: "Foo baz"
+    relativeDocumentation: "condition-keys.html"
+  }
+  "bar": {
+    type: "String"
+    documentation: "Foo bar"
     relativeDocumentation: "condition-keys.html"
   }
 )
 service MyService {
-  version: "2019-02-20",
-  operations: [Operation1],
+  version: "2019-02-20"
+  operations: [Operation1]
   resources: [Resource1]
 }
 
-@conditionKeys(["aws:accountId", "foo:baz"])
+@conditionKeys(["aws:accountId", "myservice:bar"])
 operation Operation1 {}
 
 @conditionKeys(["aws:accountId", "foo:baz"])
 resource Resource1 {
   identifiers: {
-    id1: ArnString,
-  },
+    id1: ArnString
+  }
   resources: [Resource2, Resource3, Resource4]
 }
 
 @iamResource(name: "ResourceTwo")
 resource Resource2 {
   identifiers: {
-    id1: ArnString,
-    id2: FooString,
-  },
-  read: GetResource2,
-  list: ListResource2,
+    id1: ArnString
+    id2: FooString
+  }
+  read: GetResource2
+  list: ListResource2
 }
 
 @disableConditionKeyInference
@@ -71,7 +76,7 @@ operation GetResource2 {
 
 structure GetResource2Input {
   @required
-  id1: ArnString,
+  id1: ArnString
 
   @required
   id2: FooString
@@ -82,13 +87,13 @@ string FooString
 
 @readonly
 operation ListResource2 {
-    input: ListResource2Input,
+    input: ListResource2Input
     output: ListResource2Output
 }
 
 structure ListResource2Input {
   @required
-  id1: ArnString,
+  id1: ArnString
 }
 
 structure ListResource2Output {}
diff --git a/smithy-aws-protocol-tests/model/awsQuery/xml-lists.smithy b/smithy-aws-protocol-tests/model/awsQuery/xml-lists.smithy
index be3c89818..5d61ded7e 100644
--- a/smithy-aws-protocol-tests/model/awsQuery/xml-lists.smithy
+++ b/smithy-aws-protocol-tests/model/awsQuery/xml-lists.smithy
@@ -185,8 +185,10 @@ structure XmlListsOutput {
     @xmlName("renamed")
     renamedListMembers: RenamedListMembers,
 
+    @suppress(["XmlFlattenedTrait"])
     @xmlFlattened
     // The xmlname on the targeted list is ignored, and the member name is used.
+    // The validation that flags this is suppressed so we can test this behavior.
     flattenedList: RenamedListMembers,
 
     @xmlName("customName")
diff --git a/smithy-aws-protocol-tests/model/ec2Query/xml-lists.smithy b/smithy-aws-protocol-tests/model/ec2Query/xml-lists.smithy
index cd8f59f04..c68123881 100644
--- a/smithy-aws-protocol-tests/model/ec2Query/xml-lists.smithy
+++ b/smithy-aws-protocol-tests/model/ec2Query/xml-lists.smithy
@@ -183,8 +183,10 @@ structure XmlListsOutput {
     @xmlName("renamed")
     renamedListMembers: RenamedListMembers,
 
+    @suppress(["XmlFlattenedTrait"])
     @xmlFlattened
     // The xmlname on the targeted list is ignored, and the member name is used.
+    // The validation that flags this is suppressed so we can test this behavior.
     flattenedList: RenamedListMembers,
 
     @xmlName("customName")
diff --git a/smithy-aws-protocol-tests/model/restXml/document-lists.smithy b/smithy-aws-protocol-tests/model/restXml/document-lists.smithy
index b2dc28b99..c5ed12f0b 100644
--- a/smithy-aws-protocol-tests/model/restXml/document-lists.smithy
+++ b/smithy-aws-protocol-tests/model/restXml/document-lists.smithy
@@ -352,8 +352,10 @@ structure XmlListsInputOutput {
     @xmlName("renamed")
     renamedListMembers: RenamedListMembers,
 
+    @suppress(["XmlFlattenedTrait"])
     @xmlFlattened
     // The xmlname on the targeted list is ignored, and the member name is used.
+    // The validation that flags this is suppressed so we can test this behavior.
     flattenedList: RenamedListMembers,
 
     @xmlName("customName")
@@ -376,7 +378,10 @@ structure XmlListsInputOutput {
     @xmlName("myStructureList")
     structureList: StructureList,
 
+    @suppress(["XmlFlattenedTrait"])
     @xmlFlattened
+    // The xmlname on the targeted list is ignored, and the member name is used.
+    // The validation that flags this is suppressed so we can test this behavior.
     flattenedStructureList: StructureList
 }
 
diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/xml-flattened-and-name.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/xml-flattened-and-name.errors
new file mode 100644
index 000000000..a8b044edf
--- /dev/null
+++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/xml-flattened-and-name.errors
@@ -0,0 +1,1 @@
+[WARNING] smithy.example#Struct$flattenedNoXmlName: Member is `@xmlFlattened`, so `@xmlName` of target's member (`smithy.example#ListWithXmlName$member`) has no effect. The flattened list elements will have the name of this member - `flattenedNoXmlName`. If this is unintended, you can add `@xmlName("customMember")` to this member. | XmlFlattenedTrait
diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/xml-flattened-and-name.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/xml-flattened-and-name.smithy
new file mode 100644
index 000000000..ae2080f56
--- /dev/null
+++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/xml-flattened-and-name.smithy
@@ -0,0 +1,43 @@
+$version: "2"
+
+namespace smithy.example
+
+structure Struct {
+    // No event because not flattened
+    notFlattened: ListWithXmlName
+
+    // Event because flattened and non-matching member name
+    @xmlFlattened
+    flattenedNoXmlName: ListWithXmlName
+
+    // No event because the member name matches the xml name
+    @xmlFlattened
+    customMember: ListWithXmlName
+
+    // No event because you're being explicit about the name to use
+    @xmlFlattened
+    @xmlName("customMember")
+    flattenedMatchingXmlName: ListWithXmlName
+
+    // No event because you're being explicit about the name to use
+    @xmlFlattened
+    @xmlName("Bar")
+    flattenedNonMatchingXmlName: ListWithXmlName
+
+    // Validator doesn't apply to maps
+    @xmlFlattened
+    flattenedMap: MapWithXmlName
+}
+
+list ListWithXmlName {
+    @xmlName("customMember")
+    member: String
+}
+
+map MapWithXmlName {
+    @xmlName("customKey")
+    key: String
+
+    @xmlName("customValue")
+    value: String
+}