From 823a4c25d4adc99e655c7ae91d54bf824b633762 Mon Sep 17 00:00:00 2001 From: Marco Miller Date: Wed, 6 Apr 2016 17:42:29 -0400 Subject: [PATCH] Refactor array usage to list for RepositoryConfig and GroupSetProvider Have getOwnerGroups return a list of strings rather than an array in RepositoryConfig. Replace string array usage with a list for the GroupSetProvider constructor, to simplify the life of its caller(s) now using the aforementioned list-returning getOwnerGroups. Update the impacted RepositoryConfigTest tests accordingly. That includes a minor refactoring in testAllBasePath, for consistency purposes. Change-Id: I540dae480a37c3bbb144fef48a95ba4ff29a019f --- .../config/GitReceivePackGroupsProvider.java | 5 +- .../config/GitUploadPackGroupsProvider.java | 5 +- .../server/config/GroupSetProvider.java | 3 +- .../server/config/RepositoryConfig.java | 7 +-- .../server/config/RepositoryConfigTest.java | 51 +++++++++---------- 5 files changed, 37 insertions(+), 34 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GitReceivePackGroupsProvider.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GitReceivePackGroupsProvider.java index 5dd2784dbb..49b34672d8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GitReceivePackGroupsProvider.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GitReceivePackGroupsProvider.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.config; +import com.google.common.collect.ImmutableList; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.util.ServerRequestContext; @@ -30,8 +31,8 @@ public class GitReceivePackGroupsProvider extends GroupSetProvider { @GerritServerConfig Config config, ThreadLocalRequestContext threadContext, ServerRequestContext serverCtx) { - super(gb, threadContext, serverCtx, config.getStringList("receive", null, - "allowGroup")); + super(gb, threadContext, serverCtx, ImmutableList.copyOf( + config.getStringList("receive", null, "allowGroup"))); // If no group was set, default to "registered users" // diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GitUploadPackGroupsProvider.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GitUploadPackGroupsProvider.java index 545f48b854..b7720891e3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GitUploadPackGroupsProvider.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GitUploadPackGroupsProvider.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.config; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.group.SystemGroupBackend; @@ -29,8 +30,8 @@ public class GitUploadPackGroupsProvider extends GroupSetProvider { @GerritServerConfig Config config, ThreadLocalRequestContext threadContext, ServerRequestContext serverCtx) { - super(gb, threadContext, serverCtx, config.getStringList("upload", null, - "allowGroup")); + super(gb, threadContext, serverCtx, ImmutableList.copyOf( + config.getStringList("upload", null, "allowGroup"))); // If no group was set, default to "registered users" and "anonymous" // diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GroupSetProvider.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GroupSetProvider.java index 9e55a7f3f6..0307b7c609 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GroupSetProvider.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GroupSetProvider.java @@ -28,6 +28,7 @@ import com.google.inject.Provider; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.util.List; import java.util.Set; public abstract class GroupSetProvider implements @@ -40,7 +41,7 @@ public abstract class GroupSetProvider implements @Inject protected GroupSetProvider(GroupBackend groupBackend, ThreadLocalRequestContext threadContext, - ServerRequestContext serverCtx, String[] groupNames) { + ServerRequestContext serverCtx, List groupNames) { RequestContext ctx = threadContext.setContext(serverCtx); try { ImmutableSet.Builder builder = ImmutableSet.builder(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/RepositoryConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/RepositoryConfig.java index 13188dca3b..e25039528e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/RepositoryConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/RepositoryConfig.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.config; +import com.google.common.collect.ImmutableList; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.reviewdb.client.Project; import com.google.inject.Inject; @@ -46,9 +47,9 @@ public class RepositoryConfig { DEFAULT_SUBMIT_TYPE_NAME, SubmitType.MERGE_IF_NECESSARY); } - public String[] getOwnerGroups(Project.NameKey project) { - return cfg.getStringList(SECTION_NAME, findSubSection(project.get()), - OWNER_GROUP_NAME); + public List getOwnerGroups(Project.NameKey project) { + return ImmutableList.copyOf(cfg.getStringList(SECTION_NAME, + findSubSection(project.get()), OWNER_GROUP_NAME)); } public Path getBasePath(Project.NameKey project) { diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/config/RepositoryConfigTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/config/RepositoryConfigTest.java index b64bd1ab4d..bf36738f15 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/config/RepositoryConfigTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/config/RepositoryConfigTest.java @@ -16,7 +16,7 @@ package com.google.gerrit.server.config; import static com.google.common.truth.Truth.assertThat; -import com.google.common.collect.Lists; +import com.google.common.collect.ImmutableList; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.reviewdb.client.Project.NameKey; @@ -26,7 +26,6 @@ import org.junit.Test; import java.nio.file.Path; import java.nio.file.Paths; -import java.util.Arrays; import java.util.List; public class RepositoryConfigTest { @@ -98,48 +97,46 @@ public class RepositoryConfigTest { @Test public void testOwnerGroupsWhenNotConfigured() { - assertThat(repoCfg.getOwnerGroups(new NameKey("someProject"))).isEqualTo( - new String[] {}); + assertThat(repoCfg.getOwnerGroups(new NameKey("someProject"))).isEmpty(); } @Test public void testOwnerGroupsForStarFilter() { - String[] ownerGroups = new String[] {"group1", "group2"}; - configureOwnerGroups("*", Lists.newArrayList(ownerGroups)); - assertThat(repoCfg.getOwnerGroups(new NameKey("someProject"))).isEqualTo( - ownerGroups); + ImmutableList ownerGroups = ImmutableList.of("group1", "group2"); + configureOwnerGroups("*", ownerGroups); + assertThat(repoCfg.getOwnerGroups(new NameKey("someProject"))) + .containsExactlyElementsIn(ownerGroups); } @Test public void testOwnerGroupsForSpecificFilter() { - String[] ownerGroups = new String[] {"group1", "group2"}; - configureOwnerGroups("someProject", Lists.newArrayList(ownerGroups)); + ImmutableList ownerGroups = ImmutableList.of("group1", "group2"); + configureOwnerGroups("someProject", ownerGroups); assertThat(repoCfg.getOwnerGroups(new NameKey("someOtherProject"))) - .isEqualTo(new String[] {}); - assertThat(repoCfg.getOwnerGroups(new NameKey("someProject"))).isEqualTo( - ownerGroups); + .isEmpty(); + assertThat(repoCfg.getOwnerGroups(new NameKey("someProject"))) + .containsExactlyElementsIn(ownerGroups); } @Test public void testOwnerGroupsForStartWithFilter() { - String[] ownerGroups1 = new String[] {"group1"}; - String[] ownerGroups2 = new String[] {"group2"}; - String[] ownerGroups3 = new String[] {"group3"}; + ImmutableList ownerGroups1 = ImmutableList.of("group1"); + ImmutableList ownerGroups2 = ImmutableList.of("group2"); + ImmutableList ownerGroups3 = ImmutableList.of("group3"); - configureOwnerGroups("*", Lists.newArrayList(ownerGroups1)); - configureOwnerGroups("somePath/*", Lists.newArrayList(ownerGroups2)); - configureOwnerGroups("somePath/somePath/*", - Lists.newArrayList(ownerGroups3)); + configureOwnerGroups("*", ownerGroups1); + configureOwnerGroups("somePath/*", ownerGroups2); + configureOwnerGroups("somePath/somePath/*", ownerGroups3); - assertThat(repoCfg.getOwnerGroups(new NameKey("someProject"))).isEqualTo( - ownerGroups1); + assertThat(repoCfg.getOwnerGroups(new NameKey("someProject"))) + .containsExactlyElementsIn(ownerGroups1); assertThat(repoCfg.getOwnerGroups(new NameKey("somePath/someProject"))) - .isEqualTo(ownerGroups2); + .containsExactlyElementsIn(ownerGroups2); assertThat( repoCfg.getOwnerGroups(new NameKey("somePath/somePath/someProject"))) - .isEqualTo(ownerGroups3); + .containsExactlyElementsIn(ownerGroups3); } private void configureOwnerGroups(String projectFilter, @@ -196,8 +193,10 @@ public class RepositoryConfigTest { @Test public void testAllBasePath() { - List allBasePaths = Arrays.asList(Paths.get("/someBasePath1"), - Paths.get("/someBasePath2"), Paths.get("/someBasePath2")); + ImmutableList allBasePaths = ImmutableList.of( + Paths.get("/someBasePath1"), + Paths.get("/someBasePath2"), + Paths.get("/someBasePath2")); configureBasePath("*", allBasePaths.get(0).toString()); configureBasePath("project/*", allBasePaths.get(1).toString());