Skip to content

Commit 7bbe67f

Browse files
author
Ajay Kannan
committed
Take care of minor changes
1 parent c05e738 commit 7bbe67f

2 files changed

Lines changed: 49 additions & 24 deletions

File tree

‎gcloud-java-core/src/main/java/com/google/gcloud/IamPolicy.java‎

Lines changed: 25 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,16 @@
1717
package com.google.gcloud;
1818

1919
import static com.google.common.base.Preconditions.checkArgument;
20+
import static com.google.common.base.Preconditions.checkNotNull;
2021

2122
import com.google.common.collect.ImmutableMap;
2223
import com.google.common.collect.ImmutableSet;
2324

2425
import java.io.Serializable;
2526
import java.util.Arrays;
26-
import java.util.Collection;
2727
import java.util.HashMap;
2828
import java.util.HashSet;
29-
import java.util.LinkedList;
30-
import java.util.List;
29+
import java.util.LinkedHashSet;
3130
import java.util.Map;
3231
import java.util.Objects;
3332
import java.util.Set;
@@ -69,12 +68,16 @@ protected Builder() {}
6968
/**
7069
* Replaces the builder's map of bindings with the given map of bindings.
7170
*
72-
* @throws IllegalArgumentException if the provided map is null or contain any null values
71+
* @throws NullPointerException if the given map is null or contains any null keys or values
72+
* @throws IllegalArgumentException if any identities in the given map are null
7373
*/
7474
public final B bindings(Map<R, Set<Identity>> bindings) {
75-
checkArgument(bindings != null, "The provided map of bindings cannot be null.");
75+
checkNotNull(bindings, "The provided map of bindings cannot be null.");
7676
for (Map.Entry<R, Set<Identity>> binding : bindings.entrySet()) {
77-
verifyBinding(binding.getKey(), binding.getValue());
77+
checkNotNull(binding.getKey(), "The role cannot be null.");
78+
Set<Identity> identities = binding.getValue();
79+
checkNotNull(identities, "A role cannot be assigned to a null set of identities.");
80+
checkArgument(!identities.contains(null), "Null identities are not permitted.");
7881
}
7982
this.bindings.clear();
8083
for (Map.Entry<R, Set<Identity>> binding : bindings.entrySet()) {
@@ -83,30 +86,30 @@ public final B bindings(Map<R, Set<Identity>> bindings) {
8386
return self();
8487
}
8588

86-
private void verifyBinding(R role, Collection<Identity> identities) {
87-
checkArgument(role != null, "The role cannot be null.");
88-
checkArgument(identities != null, "A role cannot be assigned to a null set of identities.");
89-
checkArgument(!identities.contains(null), "Null identities are not permitted.");
90-
}
91-
9289
/**
93-
* Removes the binding associated with the specified role.
90+
* Removes the role (and all identities associated with that role) from the policy.
9491
*/
95-
public final B removeBinding(R role) {
92+
public final B removeRole(R role) {
9693
bindings.remove(role);
9794
return self();
9895
}
9996

10097
/**
101-
* Adds one or more identities to the policy under the role specified. Creates a new role
102-
* binding if the binding corresponding to the given role did not previously exist.
98+
* Adds one or more identities to the policy under the role specified.
99+
*
100+
* @throws NullPointerException if the role or any of the identities is null.
103101
*/
104102
public final B addIdentity(R role, Identity first, Identity... others) {
105-
List<Identity> toAdd = new LinkedList<>();
103+
String nullIdentityMessage = "Null identities are not permitted.";
104+
checkNotNull(first, nullIdentityMessage);
105+
checkNotNull(others, nullIdentityMessage);
106+
for (Identity identity : others) {
107+
checkNotNull(identity, nullIdentityMessage);
108+
}
109+
Set<Identity> toAdd = new LinkedHashSet<>();
106110
toAdd.add(first);
107111
toAdd.addAll(Arrays.asList(others));
108-
verifyBinding(role, toAdd);
109-
Set<Identity> identities = bindings.get(role);
112+
Set<Identity> identities = bindings.get(checkNotNull(role, "The role cannot be null."));
110113
if (identities == null) {
111114
identities = new HashSet<Identity>();
112115
bindings.put(role, identities);
@@ -125,6 +128,9 @@ public final B removeIdentity(R role, Identity first, Identity... others) {
125128
identities.remove(first);
126129
identities.removeAll(Arrays.asList(others));
127130
}
131+
if (identities != null && identities.isEmpty()) {
132+
bindings.remove(role);
133+
}
128134
return self();
129135
}
130136

‎gcloud-java-core/src/test/java/com/google/gcloud/IamPolicyTest.java‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import org.junit.Test;
3030

3131
import java.util.HashMap;
32+
import java.util.HashSet;
3233
import java.util.Map;
3334
import java.util.Set;
3435

@@ -94,7 +95,7 @@ public void testBuilder() {
9495
assertEquals(editorBinding, policy.bindings());
9596
assertEquals("etag", policy.etag());
9697
assertEquals(1, policy.version().intValue());
97-
policy = SIMPLE_POLICY.toBuilder().removeBinding("editor").build();
98+
policy = SIMPLE_POLICY.toBuilder().removeRole("editor").build();
9899
assertEquals(ImmutableMap.of("viewer", BINDINGS.get("viewer")), policy.bindings());
99100
assertNull(policy.etag());
100101
assertNull(policy.version());
@@ -109,6 +110,8 @@ public void testBuilder() {
109110
policy = PolicyImpl.builder()
110111
.removeIdentity("viewer", USER)
111112
.addIdentity("owner", USER, SERVICE_ACCOUNT)
113+
.addIdentity("editor", GROUP)
114+
.removeIdentity("editor", GROUP)
112115
.build();
113116
assertEquals(
114117
ImmutableMap.of("owner", ImmutableSet.of(USER, SERVICE_ACCOUNT)), policy.bindings());
@@ -121,29 +124,45 @@ public void testIllegalPolicies() {
121124
try {
122125
PolicyImpl.builder().addIdentity(null, USER);
123126
fail("Null role should cause exception.");
124-
} catch (IllegalArgumentException ex) {
127+
} catch (NullPointerException ex) {
125128
assertEquals("The role cannot be null.", ex.getMessage());
126129
}
127130
try {
128131
PolicyImpl.builder().addIdentity("viewer", null, USER);
129132
fail("Null identity should cause exception.");
130-
} catch (IllegalArgumentException ex) {
133+
} catch (NullPointerException ex) {
134+
assertEquals("Null identities are not permitted.", ex.getMessage());
135+
}
136+
try {
137+
PolicyImpl.builder().addIdentity("viewer", USER, (Identity[]) null);
138+
fail("Null identity should cause exception.");
139+
} catch (NullPointerException ex) {
131140
assertEquals("Null identities are not permitted.", ex.getMessage());
132141
}
133142
try {
134143
PolicyImpl.builder().bindings(null);
135144
fail("Null bindings map should cause exception.");
136-
} catch (IllegalArgumentException ex) {
145+
} catch (NullPointerException ex) {
137146
assertEquals("The provided map of bindings cannot be null.", ex.getMessage());
138147
}
139148
try {
140149
Map<String, Set<Identity>> bindings = new HashMap<>();
141150
bindings.put("viewer", null);
142151
PolicyImpl.builder().bindings(bindings);
143152
fail("Null set of identities should cause exception.");
144-
} catch (IllegalArgumentException ex) {
153+
} catch (NullPointerException ex) {
145154
assertEquals("A role cannot be assigned to a null set of identities.", ex.getMessage());
146155
}
156+
try {
157+
Map<String, Set<Identity>> bindings = new HashMap<>();
158+
Set<Identity> identities = new HashSet<>();
159+
identities.add(null);
160+
bindings.put("viewer", identities);
161+
PolicyImpl.builder().bindings(bindings);
162+
fail("Null identity should cause exception.");
163+
} catch (IllegalArgumentException ex) {
164+
assertEquals("Null identities are not permitted.", ex.getMessage());
165+
}
147166
}
148167

149168
@Test

0 commit comments

Comments
 (0)