Use sets for OpenAPI security settings

smithy-openapi/src/main/java/software/amazon/smithy/openapi/model/OpenApi.java

@@ -18,9 +18,11 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Comparator;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.TreeMap;
 import software.amazon.smithy.model.node.ArrayNode;
 import software.amazon.smithy.model.node.Node;
@@ -143,7 +145,9 @@ public static final class Builder extends Component.Builder<Builder, OpenApi> {
         private final List<ServerObject> servers = new ArrayList<>();
         private Map<String, PathItem> paths = new TreeMap<>();
         private ComponentsObject components;
-        private final List<Map<String, List<String>>> security = new ArrayList<>();
+        // Use a set for security as duplicate entries are unnecessary (effectively
+        // represent an "A or A" security posture) and can cause downstream issues.
+        private final Set<Map<String, List<String>>> security = new LinkedHashSet<>();
         private final List<TagObject> tags = new ArrayList<>();
         private ExternalDocumentation externalDocs;
 

smithy-openapi/src/main/java/software/amazon/smithy/openapi/model/OperationObject.java

@@ -19,9 +19,11 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.TreeMap;
 import software.amazon.smithy.model.node.ArrayNode;
 import software.amazon.smithy.model.node.Node;
@@ -189,7 +191,9 @@ public static final class Builder extends Component.Builder<Builder, OperationOb
         private final List<ParameterObject> parameters = new ArrayList<>();
         private final Map<String, ResponseObject> responses = new TreeMap<>();
         private final Map<String, CallbackObject> callbacks = new TreeMap<>();
-        private List<Map<String, List<String>>> security;
+        // Use a set for security as duplicate entries are unnecessary (effectively
+        // represent an "A or A" security posture) and can cause downstream issues.
+        private Set<Map<String, List<String>>> security;
         private final List<ServerObject> servers = new ArrayList<>();
         private String summary;
         private String description;
@@ -280,14 +284,14 @@ public Builder deprecated(boolean deprecated) {
         }
 
         public Builder security(Collection<Map<String, List<String>>> security) {
-            this.security = new ArrayList<>();
+            this.security = new LinkedHashSet<>();
             this.security.addAll(security);
             return this;
         }
 
         public Builder addSecurity(Map<String, List<String>> security) {
             if (this.security == null) {
-                this.security = new ArrayList<>();
+                this.security = new LinkedHashSet<>();
             }
             this.security.add(security);
             return this;

smithy-openapi/src/test/java/software/amazon/smithy/openapi/fromsmithy/OpenApiConverterTest.java

@@ -21,6 +21,7 @@
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.not;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 
 import java.util.Collections;
 import java.util.List;
@@ -254,14 +255,14 @@ public void protocolsCanOmitOperations() {
                 .convert(model);
 
         for (PathItem pathItem : result.getPaths().values()) {
-            Assertions.assertFalse(pathItem.getGet().isPresent());
-            Assertions.assertFalse(pathItem.getHead().isPresent());
-            Assertions.assertFalse(pathItem.getDelete().isPresent());
-            Assertions.assertFalse(pathItem.getPatch().isPresent());
-            Assertions.assertFalse(pathItem.getPost().isPresent());
-            Assertions.assertFalse(pathItem.getPut().isPresent());
-            Assertions.assertFalse(pathItem.getTrace().isPresent());
-            Assertions.assertFalse(pathItem.getOptions().isPresent());
+            assertFalse(pathItem.getGet().isPresent());
+            assertFalse(pathItem.getHead().isPresent());
+            assertFalse(pathItem.getDelete().isPresent());
+            assertFalse(pathItem.getPatch().isPresent());
+            assertFalse(pathItem.getPost().isPresent());
+            assertFalse(pathItem.getPut().isPresent());
+            assertFalse(pathItem.getTrace().isPresent());
+            assertFalse(pathItem.getOptions().isPresent());
         }
     }
 
@@ -353,6 +354,34 @@ public void canChangeSecurityRequirementName() {
         assertThat(result.getPaths().get("/2").getGet().get().getSecurity().get().get(0).keySet(), contains("foo_baz"));
     }
 
+    @Test
+    public void consolidatesSameSecurityRequirements() {
+        // This service model has multiple auth types throughout it for both
+        // the top level and operation level security setting. Validate that,
+        // after they're set to use the same name, they're consolidated for
+        // being the same.
+        Model model = Model.assembler()
+                .addImport(getClass().getResource("consolidates-security-service.json"))
+                .discoverModels()
+                .assemble()
+                .unwrap();
+        OpenApiConfig config = new OpenApiConfig();
+        config.setService(ShapeId.from("smithy.example#Service"));
+        OpenApi result = OpenApiConverter.create()
+                .addOpenApiMapper(new ConstantSecurity())
+                .config(config)
+                .convert(model);
+
+        assertThat(result.getSecurity().size(), equalTo(1));
+        assertThat(result.getSecurity().get(0).keySet(), contains("foo_baz"));
+        // This security matches the service, so isn't applied.
+        assertFalse(result.getPaths().get("/1").getGet().get().getSecurity().isPresent());
+        assertThat(result.getPaths().get("/2").getGet().get().getSecurity().get().size(), equalTo(1));
+        assertThat(result.getPaths().get("/2").getGet().get().getSecurity().get().get(0).keySet(), contains("foo_baz"));
+        assertThat(result.getPaths().get("/3").getGet().get().getSecurity().get().size(), equalTo(1));
+        assertThat(result.getPaths().get("/3").getGet().get().getSecurity().get().get(0).keySet(), contains("foo_baz"));
+    }
+
     @Test
     public void mergesInSchemaDocumentExtensions() {
         OpenApiConfig config = new OpenApiConfig();

smithy-openapi/src/test/resources/software/amazon/smithy/openapi/fromsmithy/consolidates-security-service.json

@@ -0,0 +1,79 @@
+{
+    "smithy": "1.0",
+    "shapes": {
+        "smithy.example#Service": {
+            "type": "service",
+            "version": "2006-03-01",
+            "operations": [
+                {
+                    "target": "smithy.example#Operation1"
+                },
+                {
+                    "target": "smithy.example#Operation2"
+                },
+                {
+                    "target": "smithy.example#Operation3"
+                },
+                {
+                    "target": "smithy.example#UnauthenticatedOperation"
+                }
+            ],
+            "traits": {
+                "aws.protocols#restJson1": {},
+                "aws.auth#sigv4": {
+                    "name": "example"
+                },
+                "smithy.api#httpBasicAuth": {},
+                "smithy.api#httpDigestAuth": {},
+                "smithy.api#auth": [
+                    "aws.auth#sigv4",
+                    "smithy.api#httpDigestAuth"
+                ]
+            }
+        },
+        "smithy.example#Operation1": {
+            "type": "operation",
+            "traits": {
+                "smithy.api#http": {
+                    "uri": "/1",
+                    "method": "GET"
+                }
+            }
+        },
+        "smithy.example#Operation2": {
+            "type": "operation",
+            "traits": {
+                "smithy.api#http": {
+                    "uri": "/2",
+                    "method": "GET"
+                },
+                "smithy.api#auth": [
+                    "smithy.api#httpBasicAuth"
+                ]
+            }
+        },
+        "smithy.example#Operation3": {
+            "type": "operation",
+            "traits": {
+                "smithy.api#http": {
+                    "uri": "/3",
+                    "method": "GET"
+                },
+                "smithy.api#auth": [
+                    "smithy.api#httpBasicAuth",
+                    "aws.auth#sigv4"
+                ]
+            }
+        },
+        "smithy.example#UnauthenticatedOperation": {
+            "type": "operation",
+            "traits": {
+                "smithy.api#http": {
+                    "uri": "/4",
+                    "method": "GET"
+                },
+                "smithy.api#auth": []
+            }
+        }
+    }
+}