Update some code to respect some java best practices
This commit updates some code introduced by I977a77286f
to improve its readability and also honor some java best
practices, e.g.:
* explicitly prohibit inheritance if a class isn't
intended for that (Effective Java Item 19's advice:
design and document for inheritance or else prohibit it).
* Write unit tests in three sections, aka "arrange",
"act", "assert".
Change-Id: I8939370f16c2465c7ba102afd3877a31babd0c89
diff --git a/java/com/google/gerrit/server/permissions/PluginPermissionsUtil.java b/java/com/google/gerrit/server/permissions/PluginPermissionsUtil.java
index bb6153c..a837ed7 100644
--- a/java/com/google/gerrit/server/permissions/PluginPermissionsUtil.java
+++ b/java/com/google/gerrit/server/permissions/PluginPermissionsUtil.java
@@ -27,7 +27,7 @@
/** Utilities for plugin permissions. */
@Singleton
-public class PluginPermissionsUtil {
+public final class PluginPermissionsUtil {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private static final String PLUGIN_NAME_PATTERN_STRING = "[a-zA-Z0-9-]+";
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/PluginAccessIT.java b/javatests/com/google/gerrit/acceptance/rest/project/PluginAccessIT.java
index 617883f..e7663f7 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/PluginAccessIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/PluginAccessIT.java
@@ -32,9 +32,10 @@
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
import com.google.inject.Module;
+import java.util.Set;
import org.junit.Test;
-public class PluginAccessIT extends AbstractDaemonTest {
+public final class PluginAccessIT extends AbstractDaemonTest {
private static final String TEST_PLUGIN_NAME = "gerrit";
private static final String TEST_PLUGIN_CAPABILITY = "aPluginCapability";
private static final String TEST_PLUGIN_PROJECT_PERMISSION = "aPluginProjectPermission";
@@ -69,40 +70,47 @@
}
@Test
- public void addPluginCapability() throws Exception {
- addPluginPermission(AccessSection.GLOBAL_CAPABILITIES, TEST_PLUGIN_CAPABILITY);
+ public void setAccessAddPluginCapabilitySucceed() throws Exception {
+ String pluginCapability = TEST_PLUGIN_NAME + "-" + TEST_PLUGIN_CAPABILITY;
+ ProjectAccessInput accessInput =
+ createAccessInput(AccessSection.GLOBAL_CAPABILITIES, pluginCapability);
+ ProjectAccessInfo projectAccessInfo =
+ gApi.projects().name(allProjects.get()).access(accessInput);
+
+ Set<String> capabilities =
+ projectAccessInfo.local.get(AccessSection.GLOBAL_CAPABILITIES).permissions.keySet();
+ assertThat(capabilities).contains(pluginCapability);
// Verifies the plugin defined capability could be listed.
- assertThat(pluginPermissionsUtil.collectPluginCapabilities())
- .containsKey(TEST_PLUGIN_NAME + "-" + TEST_PLUGIN_CAPABILITY);
+ assertThat(pluginPermissionsUtil.collectPluginCapabilities()).containsKey(pluginCapability);
}
@Test
- public void addPluginProjectPermission() throws Exception {
- addPluginPermission("refs/heads/plugin-permission", TEST_PLUGIN_PROJECT_PERMISSION);
+ public void setAccessAddPluginProjectPermissionSucceed() throws Exception {
+ String pluginProjectPermission =
+ "plugin-" + TEST_PLUGIN_NAME + "-" + TEST_PLUGIN_PROJECT_PERMISSION;
+ String accessSection = "refs/heads/plugin-permission";
+ ProjectAccessInput accessInput = createAccessInput(accessSection, pluginProjectPermission);
+ ProjectAccessInfo projectAccessInfo =
+ gApi.projects().name(allProjects.get()).access(accessInput);
+
+ Set<String> permissions = projectAccessInfo.local.get(accessSection).permissions.keySet();
+ assertThat(permissions).contains(pluginProjectPermission);
// Verifies the plugin defined capability could be listed.
assertThat(pluginPermissionsUtil.collectPluginProjectPermissions())
- .containsKey("plugin-" + TEST_PLUGIN_NAME + "-" + TEST_PLUGIN_PROJECT_PERMISSION);
+ .containsKey(pluginProjectPermission);
}
- private void addPluginPermission(String accessSection, String permission) throws Exception {
+ private static ProjectAccessInput createAccessInput(String accessSection, String permissionName) {
ProjectAccessInput accessInput = new ProjectAccessInput();
PermissionRuleInfo ruleInfo = new PermissionRuleInfo(PermissionRuleInfo.Action.ALLOW, false);
PermissionInfo email = new PermissionInfo(null, null);
email.rules = ImmutableMap.of(SystemGroupBackend.REGISTERED_USERS.get(), ruleInfo);
- String permissionConfigName = TEST_PLUGIN_NAME + "-" + permission;
- if (!accessSection.equals(AccessSection.GLOBAL_CAPABILITIES)) {
- permissionConfigName = "plugin-" + permissionConfigName;
- }
AccessSectionInfo accessSectionInfo = new AccessSectionInfo();
- accessSectionInfo.permissions = ImmutableMap.of(permissionConfigName, email);
+ accessSectionInfo.permissions = ImmutableMap.of(permissionName, email);
accessInput.add = ImmutableMap.of(accessSection, accessSectionInfo);
- ProjectAccessInfo updatedAccessSectionInfo =
- gApi.projects().name(allProjects.get()).access(accessInput);
-
- assertThat(updatedAccessSectionInfo.local.get(accessSection).permissions.keySet())
- .containsAllIn(accessSectionInfo.permissions.keySet());
+ return accessInput;
}
}
diff --git a/javatests/com/google/gerrit/server/permissions/PluginPermissionsUtilTest.java b/javatests/com/google/gerrit/server/permissions/PluginPermissionsUtilTest.java
index fd68da3..85d85fb 100644
--- a/javatests/com/google/gerrit/server/permissions/PluginPermissionsUtilTest.java
+++ b/javatests/com/google/gerrit/server/permissions/PluginPermissionsUtilTest.java
@@ -17,23 +17,40 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.server.permissions.PluginPermissionsUtil.isPluginPermission;
+import com.google.common.collect.ImmutableList;
import org.junit.Test;
/** Small tests for {@link PluginPermissionsUtil}. */
-public class PluginPermissionsUtilTest {
+public final class PluginPermissionsUtilTest {
@Test
- public void pluginPermissionNameInConfigPattern() {
- assertThat(isPluginPermission("create")).isFalse();
- assertThat(isPluginPermission("label-Code-Review")).isFalse();
- assertThat(isPluginPermission("plugin-foo")).isFalse();
- assertThat(isPluginPermission("plugin-foo")).isFalse();
-
- assertThat(isPluginPermission("plugin-foo-a")).isTrue();
+ public void isPluginPermissionReturnsTrueForValidName() {
// "-" is allowed for a plugin name. Here "foo-a" should be the name of the plugin.
- assertThat(isPluginPermission("plugin-foo-a-b")).isTrue();
+ ImmutableList<String> validPluginPermissions =
+ ImmutableList.of("plugin-foo-a", "plugin-foo-a-b");
- assertThat(isPluginPermission("plugin-foo-a-")).isFalse();
- assertThat(isPluginPermission("plugin-foo-a1")).isFalse();
+ for (String permission : validPluginPermissions) {
+ assertThat(isPluginPermission(permission))
+ .named("valid plugin permission: %s", permission)
+ .isTrue();
+ }
+ }
+
+ @Test
+ public void isPluginPermissionReturnsFalseForInvalidName() {
+ ImmutableList<String> inValidPluginPermissions =
+ ImmutableList.of(
+ "create",
+ "label-Code-Review",
+ "plugin-foo",
+ "plugin-foo",
+ "plugin-foo-a-",
+ "plugin-foo-a1");
+
+ for (String permission : inValidPluginPermissions) {
+ assertThat(isPluginPermission(permission))
+ .named("invalid plugin permission: %s", permission)
+ .isFalse();
+ }
}
}
