fix: verify correctness of map -> list equality (#174)

* fix: verify correctness of map -> list equality * address comments * format
googleapis · Mar 10, 2020 · f45d913 · f45d913
1 parent d28f885
commit f45d913
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 3 deletions.
diff --git a/google-cloud-core/src/main/java/com/google/cloud/Policy.java b/google-cloud-core/src/main/java/com/google/cloud/Policy.java
@@ -260,7 +260,7 @@ public final Builder addIdentity(Role role, Identity first, Identity... others)
         Binding binding = bindingsList.get(i);
         if (binding.getRole().equals(role.getValue())) {
           Binding.Builder bindingBuilder = binding.toBuilder();
-          ImmutableList.Builder<String> membersBuilder = ImmutableList.builder();
+          ImmutableSet.Builder<String> membersBuilder = ImmutableSet.builder();
           membersBuilder.addAll(binding.getMembers());
           membersBuilder.add(first.strValue());
           for (Identity identity : others) {
@@ -273,7 +273,7 @@ public final Builder addIdentity(Role role, Identity first, Identity... others)
       }
       // Binding does not yet exist.
       Binding.Builder bindingBuilder = Binding.newBuilder().setRole(role.getValue());
-      ImmutableList.Builder<String> membersBuilder = ImmutableList.builder();
+      ImmutableSet.Builder<String> membersBuilder = ImmutableSet.builder();
       membersBuilder.add(first.strValue());
       for (Identity identity : others) {
         membersBuilder.add(identity.strValue());
@@ -432,9 +432,19 @@ public boolean equals(Object obj) {
       return false;
     }
     Policy other = (Policy) obj;
-    if (!bindingsList.equals(other.getBindingsList())) {
+    if (bindingsList == null && other.getBindingsList() == null) {
+      return true;
+    }
+    if ((bindingsList == null && other.getBindingsList() != null)
+        || bindingsList != null && other.getBindingsList() == null
+        || bindingsList.size() != other.getBindingsList().size()) {
       return false;
     }
+    for (Binding binding : bindingsList) {
+      if (!other.getBindingsList().contains(binding)) {
+        return false;
+      }
+    }
     return Objects.equals(etag, other.getEtag()) && version == other.getVersion();
   }
 

diff --git a/google-cloud-core/src/test/java/com/google/cloud/PolicyTest.java b/google-cloud-core/src/test/java/com/google/cloud/PolicyTest.java
@@ -104,6 +104,36 @@ public void testBuilder() {
     assertEquals(0, policy.getVersion());
   }
 
+  @Test
+  public void testPolicyOrderShouldNotMatter() {
+    Role role1 = Role.of("role1");
+    Identity identity1 = Identity.user("user1@example.com");
+    Role role2 = Role.of("role2");
+    Identity identity2 = Identity.user("user2@example.com");
+    Policy policy1 =
+        Policy.newBuilder().addIdentity(role1, identity1).addIdentity(role2, identity2).build();
+    Policy policy2 =
+        Policy.newBuilder().addIdentity(role2, identity2).addIdentity(role1, identity1).build();
+    assertEquals(policy1, policy2);
+  }
+
+  @Test
+  public void testPolicyMultipleAddIdentitiesShouldNotMatter() {
+    Role role1 = Role.of("role1");
+    Identity identity1 = Identity.user("user1@example.com");
+    Role role2 = Role.of("role2");
+    Identity identity2 = Identity.user("user2@example.com");
+    Policy policy1 =
+        Policy.newBuilder()
+            .addIdentity(role1, identity1)
+            .addIdentity(role2, identity2)
+            .addIdentity(role2, identity2)
+            .build();
+    Policy policy2 =
+        Policy.newBuilder().addIdentity(role2, identity2).addIdentity(role1, identity1).build();
+    assertEquals(policy1, policy2);
+  }
+
   @Test
   public void testIllegalPolicies() {
     try {