AccessSection: Consistently disallow passing null into the methods
It is inconsistent that
AccessSection.remove(null)
is a no-op while
AccessSection.removePermission(null)
fails with an exception.
Make the behavior of all methods in AccessSection consistent when null
is passed in by always failing with a NullPointerException in this case.
This also fixes a weird behavior when calling getPermission(null,
true). This call created a Permission with name null and added it to
the permission list. Now it fails with a NullPointerException.
We did consider making the behavior of the AccessSection methods
consistent by always allowing/ignoring null values (see change
I1aa786c8f4), however this might have obfuscated some cases where null
was accidentally passed in and we would have liked to get to know about
it.
Change-Id: I2b95651b92a96d4cb78228b7648e24218ad947e0
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/common/data/AccessSection.java b/java/com/google/gerrit/common/data/AccessSection.java
index f658066..0530cc0 100644
--- a/java/com/google/gerrit/common/data/AccessSection.java
+++ b/java/com/google/gerrit/common/data/AccessSection.java
@@ -43,6 +43,8 @@
}
public void setPermissions(List<Permission> list) {
+ checkNotNull(list);
+
Set<String> names = new HashSet<>();
for (Permission p : list) {
if (!names.add(p.getName().toLowerCase())) {
@@ -60,6 +62,8 @@
@Nullable
public Permission getPermission(String name, boolean create) {
+ checkNotNull(name);
+
if (permissions != null) {
for (Permission p : permissions) {
if (p.getName().equalsIgnoreCase(name)) {
@@ -82,6 +86,8 @@
}
public void addPermission(Permission permission) {
+ checkNotNull(permission);
+
if (permissions == null) {
permissions = new ArrayList<>();
}
@@ -96,18 +102,21 @@
}
public void remove(Permission permission) {
- if (permission != null) {
- removePermission(permission.getName());
- }
+ checkNotNull(permission);
+ removePermission(permission.getName());
}
public void removePermission(String name) {
+ checkNotNull(name);
+
if (permissions != null) {
permissions.removeIf(permission -> name.equalsIgnoreCase(permission.getName()));
}
}
public void mergeFrom(AccessSection section) {
+ checkNotNull(section);
+
for (Permission src : section.getPermissions()) {
Permission dst = getPermission(src.getName());
if (dst != null) {
@@ -118,6 +127,13 @@
}
}
+ // TODO(ekempin): Once the GWT UI is gone use com.google.common.base.Preconditions.checkNotNull
+ private static void checkNotNull(Object reference) {
+ if (reference == null) {
+ throw new NullPointerException();
+ }
+ }
+
@Override
public int compareTo(AccessSection o) {
return comparePattern().compareTo(o.comparePattern());
diff --git a/javatests/com/google/gerrit/common/data/AccessSectionTest.java b/javatests/com/google/gerrit/common/data/AccessSectionTest.java
index ecdb3c8..a59c015 100644
--- a/javatests/com/google/gerrit/common/data/AccessSectionTest.java
+++ b/javatests/com/google/gerrit/common/data/AccessSectionTest.java
@@ -60,6 +60,9 @@
Permission submitPermission = new Permission(Permission.SUBMIT);
accessSection.setPermissions(ImmutableList.of(submitPermission));
assertThat(accessSection.getPermissions()).containsExactly(submitPermission);
+
+ exception.expect(NullPointerException.class);
+ accessSection.setPermissions(null);
}
@Test
@@ -92,6 +95,9 @@
Permission submitPermission = new Permission(Permission.SUBMIT);
accessSection.setPermissions(ImmutableList.of(submitPermission));
assertThat(accessSection.getPermission(Permission.SUBMIT)).isEqualTo(submitPermission);
+
+ exception.expect(NullPointerException.class);
+ accessSection.getPermission(null);
}
@Test
@@ -108,6 +114,9 @@
assertThat(accessSection.getPermission(Permission.SUBMIT, true))
.isEqualTo(new Permission(Permission.SUBMIT));
+
+ exception.expect(NullPointerException.class);
+ accessSection.getPermission(null, true);
}
@Test
@@ -124,6 +133,9 @@
assertThat(accessSection.getPermissions())
.containsExactly(abandonPermission, rebasePermission, submitPermission)
.inOrder();
+
+ exception.expect(NullPointerException.class);
+ accessSection.addPermission(null);
}
@Test
@@ -172,6 +184,9 @@
assertThat(accessSection.getPermissions())
.containsExactly(abandonPermission, rebasePermission)
.inOrder();
+
+ exception.expect(NullPointerException.class);
+ accessSection.remove(null);
}
@Test
@@ -189,6 +204,9 @@
assertThat(accessSection.getPermissions())
.containsExactly(abandonPermission, rebasePermission)
.inOrder();
+
+ exception.expect(NullPointerException.class);
+ accessSection.removePermission(null);
}
@Test
@@ -229,6 +247,9 @@
assertThat(accessSection1.getPermissions())
.containsExactly(abandonPermission, rebasePermission, submitPermission)
.inOrder();
+
+ exception.expect(NullPointerException.class);
+ accessSection.mergeFrom(null);
}
@Test
