From 7bbe67fd3651f9294b6c3bcb3265a3872d2f1383 Mon Sep 17 00:00:00 2001 From: Ajay Kannan Date: Mon, 21 Mar 2016 16:29:10 -0700 Subject: [PATCH] Take care of minor changes --- .../java/com/google/gcloud/IamPolicy.java | 44 +++++++++++-------- .../java/com/google/gcloud/IamPolicyTest.java | 29 +++++++++--- 2 files changed, 49 insertions(+), 24 deletions(-) diff --git a/gcloud-java-core/src/main/java/com/google/gcloud/IamPolicy.java b/gcloud-java-core/src/main/java/com/google/gcloud/IamPolicy.java index 3d7c1422e6fa..9cce4b23c864 100644 --- a/gcloud-java-core/src/main/java/com/google/gcloud/IamPolicy.java +++ b/gcloud-java-core/src/main/java/com/google/gcloud/IamPolicy.java @@ -17,17 +17,16 @@ package com.google.gcloud; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import java.io.Serializable; import java.util.Arrays; -import java.util.Collection; import java.util.HashMap; import java.util.HashSet; -import java.util.LinkedList; -import java.util.List; +import java.util.LinkedHashSet; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -69,12 +68,16 @@ protected Builder() {} /** * Replaces the builder's map of bindings with the given map of bindings. * - * @throws IllegalArgumentException if the provided map is null or contain any null values + * @throws NullPointerException if the given map is null or contains any null keys or values + * @throws IllegalArgumentException if any identities in the given map are null */ public final B bindings(Map> bindings) { - checkArgument(bindings != null, "The provided map of bindings cannot be null."); + checkNotNull(bindings, "The provided map of bindings cannot be null."); for (Map.Entry> binding : bindings.entrySet()) { - verifyBinding(binding.getKey(), binding.getValue()); + checkNotNull(binding.getKey(), "The role cannot be null."); + Set identities = binding.getValue(); + checkNotNull(identities, "A role cannot be assigned to a null set of identities."); + checkArgument(!identities.contains(null), "Null identities are not permitted."); } this.bindings.clear(); for (Map.Entry> binding : bindings.entrySet()) { @@ -83,30 +86,30 @@ public final B bindings(Map> bindings) { return self(); } - private void verifyBinding(R role, Collection identities) { - checkArgument(role != null, "The role cannot be null."); - checkArgument(identities != null, "A role cannot be assigned to a null set of identities."); - checkArgument(!identities.contains(null), "Null identities are not permitted."); - } - /** - * Removes the binding associated with the specified role. + * Removes the role (and all identities associated with that role) from the policy. */ - public final B removeBinding(R role) { + public final B removeRole(R role) { bindings.remove(role); return self(); } /** - * Adds one or more identities to the policy under the role specified. Creates a new role - * binding if the binding corresponding to the given role did not previously exist. + * Adds one or more identities to the policy under the role specified. + * + * @throws NullPointerException if the role or any of the identities is null. */ public final B addIdentity(R role, Identity first, Identity... others) { - List toAdd = new LinkedList<>(); + String nullIdentityMessage = "Null identities are not permitted."; + checkNotNull(first, nullIdentityMessage); + checkNotNull(others, nullIdentityMessage); + for (Identity identity : others) { + checkNotNull(identity, nullIdentityMessage); + } + Set toAdd = new LinkedHashSet<>(); toAdd.add(first); toAdd.addAll(Arrays.asList(others)); - verifyBinding(role, toAdd); - Set identities = bindings.get(role); + Set identities = bindings.get(checkNotNull(role, "The role cannot be null.")); if (identities == null) { identities = new HashSet(); bindings.put(role, identities); @@ -125,6 +128,9 @@ public final B removeIdentity(R role, Identity first, Identity... others) { identities.remove(first); identities.removeAll(Arrays.asList(others)); } + if (identities != null && identities.isEmpty()) { + bindings.remove(role); + } return self(); } diff --git a/gcloud-java-core/src/test/java/com/google/gcloud/IamPolicyTest.java b/gcloud-java-core/src/test/java/com/google/gcloud/IamPolicyTest.java index 2f40c3b76001..235c2c2b1c85 100644 --- a/gcloud-java-core/src/test/java/com/google/gcloud/IamPolicyTest.java +++ b/gcloud-java-core/src/test/java/com/google/gcloud/IamPolicyTest.java @@ -29,6 +29,7 @@ import org.junit.Test; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -94,7 +95,7 @@ public void testBuilder() { assertEquals(editorBinding, policy.bindings()); assertEquals("etag", policy.etag()); assertEquals(1, policy.version().intValue()); - policy = SIMPLE_POLICY.toBuilder().removeBinding("editor").build(); + policy = SIMPLE_POLICY.toBuilder().removeRole("editor").build(); assertEquals(ImmutableMap.of("viewer", BINDINGS.get("viewer")), policy.bindings()); assertNull(policy.etag()); assertNull(policy.version()); @@ -109,6 +110,8 @@ public void testBuilder() { policy = PolicyImpl.builder() .removeIdentity("viewer", USER) .addIdentity("owner", USER, SERVICE_ACCOUNT) + .addIdentity("editor", GROUP) + .removeIdentity("editor", GROUP) .build(); assertEquals( ImmutableMap.of("owner", ImmutableSet.of(USER, SERVICE_ACCOUNT)), policy.bindings()); @@ -121,19 +124,25 @@ public void testIllegalPolicies() { try { PolicyImpl.builder().addIdentity(null, USER); fail("Null role should cause exception."); - } catch (IllegalArgumentException ex) { + } catch (NullPointerException ex) { assertEquals("The role cannot be null.", ex.getMessage()); } try { PolicyImpl.builder().addIdentity("viewer", null, USER); fail("Null identity should cause exception."); - } catch (IllegalArgumentException ex) { + } catch (NullPointerException ex) { + assertEquals("Null identities are not permitted.", ex.getMessage()); + } + try { + PolicyImpl.builder().addIdentity("viewer", USER, (Identity[]) null); + fail("Null identity should cause exception."); + } catch (NullPointerException ex) { assertEquals("Null identities are not permitted.", ex.getMessage()); } try { PolicyImpl.builder().bindings(null); fail("Null bindings map should cause exception."); - } catch (IllegalArgumentException ex) { + } catch (NullPointerException ex) { assertEquals("The provided map of bindings cannot be null.", ex.getMessage()); } try { @@ -141,9 +150,19 @@ public void testIllegalPolicies() { bindings.put("viewer", null); PolicyImpl.builder().bindings(bindings); fail("Null set of identities should cause exception."); - } catch (IllegalArgumentException ex) { + } catch (NullPointerException ex) { assertEquals("A role cannot be assigned to a null set of identities.", ex.getMessage()); } + try { + Map> bindings = new HashMap<>(); + Set identities = new HashSet<>(); + identities.add(null); + bindings.put("viewer", identities); + PolicyImpl.builder().bindings(bindings); + fail("Null identity should cause exception."); + } catch (IllegalArgumentException ex) { + assertEquals("Null identities are not permitted.", ex.getMessage()); + } } @Test