From 2fd6310f96b8222abefa49159a44fa88c9e844f6 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Tue, 10 Mar 2020 10:46:14 -0700 Subject: [PATCH 1/3] fix: verify correctness of map -> list equality --- .../main/java/com/google/cloud/Policy.java | 16 +++++++-- .../java/com/google/cloud/PolicyTest.java | 34 +++++++++++++++++++ 2 files changed, 47 insertions(+), 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 index a6ecbae11e..1c5b78c9ea 100644 --- 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 membersBuilder = ImmutableList.builder(); + ImmutableSet.Builder 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 membersBuilder = ImmutableList.builder(); + ImmutableSet.Builder 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 index eb03e33726..4b186a1378 100644 --- 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,40 @@ 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) + .addIdentity(role1, identity1) + .build(); + assertEquals(policy1, policy2); + } + @Test public void testIllegalPolicies() { try { From 694ea76807539f90a819a96bbd17a9dd6acc21d4 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Tue, 10 Mar 2020 11:22:16 -0700 Subject: [PATCH 2/3] address comments --- google-cloud-core/src/test/java/com/google/cloud/PolicyTest.java | 1 - 1 file changed, 1 deletion(-) 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 index 4b186a1378..e0d49820d8 100644 --- a/google-cloud-core/src/test/java/com/google/cloud/PolicyTest.java +++ b/google-cloud-core/src/test/java/com/google/cloud/PolicyTest.java @@ -133,7 +133,6 @@ public void testPolicyMultipleAddIdentitiesShouldNotMatter() { Policy.newBuilder() .addIdentity(role2, identity2) .addIdentity(role1, identity1) - .addIdentity(role1, identity1) .build(); assertEquals(policy1, policy2); } From f6ed440e91393c10e108bffa4ad2d29700cfb04e Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Tue, 10 Mar 2020 11:27:01 -0700 Subject: [PATCH 3/3] format --- .../src/test/java/com/google/cloud/PolicyTest.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) 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 index e0d49820d8..98695c2a4c 100644 --- a/google-cloud-core/src/test/java/com/google/cloud/PolicyTest.java +++ b/google-cloud-core/src/test/java/com/google/cloud/PolicyTest.java @@ -130,10 +130,7 @@ public void testPolicyMultipleAddIdentitiesShouldNotMatter() { .addIdentity(role2, identity2) .build(); Policy policy2 = - Policy.newBuilder() - .addIdentity(role2, identity2) - .addIdentity(role1, identity1) - .build(); + Policy.newBuilder().addIdentity(role2, identity2).addIdentity(role1, identity1).build(); assertEquals(policy1, policy2); }