GroupCache: Remove all() method
The results of all() are not cached, so it should not be a member of
the GroupCache class.
Remove it, and replace usage of it with Groups.getAll().
Change-Id: I52f8c66ec890a19c8bc8390054be8c883dad3f96
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 1af8f7d..d5e2d9e 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -93,6 +93,7 @@
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.group.Groups;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.index.change.ChangeIndex;
@@ -254,6 +255,7 @@
@Inject private InProcessProtocol inProcessProtocol;
@Inject private Provider<AnonymousUser> anonymousUser;
@Inject private SchemaFactory<ReviewDb> reviewDbProvider;
+ @Inject private Groups groups;
private List<Repository> toClose;
@@ -344,6 +346,8 @@
Transport.register(inProcessProtocol);
toClose = Collections.synchronizedList(new ArrayList<Repository>());
+ db = reviewDbProvider.open();
+
// All groups which were added during the server start (e.g. in SchemaCreator) aren't contained
// in the instance of the group index which is available here and in tests. There are two
// reasons:
@@ -353,7 +357,7 @@
// later on. As test indexes are non-permanent, closing an instance and opening another one
// removes all indexed data.
// As a workaround, we simply reindex all available groups here.
- for (AccountGroup group : groupCache.all()) {
+ for (AccountGroup group : groups.getAll(db).collect(toList())) {
groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey());
}
@@ -367,8 +371,6 @@
adminRestSession = new RestSession(server, admin);
userRestSession = new RestSession(server, user);
- db = reviewDbProvider.open();
-
testRequiresSsh = classDesc.useSshAnnotation() || methodDesc.useSshAnnotation();
if (testRequiresSsh
&& SshMode.useSsh()
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
index eb4df15..305a2b0 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -30,7 +30,7 @@
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.extensions.api.groups.GroupApi;
import com.google.gerrit.extensions.api.groups.GroupInput;
-import com.google.gerrit.extensions.api.groups.Groups;
+import com.google.gerrit.extensions.api.groups.Groups.ListRequest;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.GroupAuditEventInfo;
import com.google.gerrit.extensions.common.GroupAuditEventInfo.GroupMemberAuditEventInfo;
@@ -46,6 +46,7 @@
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.server.group.Groups;
import com.google.gerrit.server.group.GroupsUpdate;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.ServerInitiated;
@@ -63,6 +64,7 @@
@NoHttpd
public class GroupsIT extends AbstractDaemonTest {
@Inject @ServerInitiated private Provider<GroupsUpdate> groupsUpdateProvider;
+ @Inject private Groups groups;
@Test
public void systemGroupCanBeRetrievedFromIndex() throws Exception {
@@ -481,7 +483,7 @@
@Test
public void listAllGroups() throws Exception {
List<String> expectedGroups =
- groupCache.all().stream().map(a -> a.getName()).sorted().collect(toList());
+ groups.getAll(db).map(a -> a.getName()).sorted().collect(toList());
assertThat(expectedGroups.size()).isAtLeast(2);
assertThat(gApi.groups().list().getAsMap().keySet())
.containsExactlyElementsIn(expectedGroups)
@@ -692,7 +694,7 @@
groupsUpdateProvider.get().updateGroup(db, groupUuid, group -> group.setCreatedOn(null));
}
- private void assertBadRequest(Groups.ListRequest req) throws Exception {
+ private void assertBadRequest(ListRequest req) throws Exception {
try {
req.get();
fail("Expected BadRequestException");
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java
index 82bcce3..d985426 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java
@@ -14,7 +14,6 @@
package com.google.gerrit.server.account;
-import com.google.common.collect.ImmutableList;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.group.InternalGroup;
import java.io.IOException;
@@ -49,9 +48,6 @@
*/
Optional<InternalGroup> get(AccountGroup.UUID groupUuid);
- /** @return sorted list of groups. */
- ImmutableList<AccountGroup> all();
-
/** Notify the cache that a new group was constructed. */
void onCreateGroup(AccountGroup group) throws IOException;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java
index edbc2d8..81996ab 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java
@@ -14,11 +14,8 @@
package com.google.gerrit.server.account;
-import static com.google.common.collect.ImmutableList.toImmutableList;
-
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
-import com.google.common.collect.ImmutableList;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.cache.CacheModule;
@@ -26,7 +23,6 @@
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.index.group.GroupIndexer;
import com.google.gerrit.server.query.group.InternalGroupQuery;
-import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
import com.google.inject.Module;
@@ -71,24 +67,18 @@
private final LoadingCache<AccountGroup.Id, Optional<InternalGroup>> byId;
private final LoadingCache<String, Optional<InternalGroup>> byName;
private final LoadingCache<String, Optional<InternalGroup>> byUUID;
- private final SchemaFactory<ReviewDb> schema;
private final Provider<GroupIndexer> indexer;
- private final Groups groups;
@Inject
GroupCacheImpl(
@Named(BYID_NAME) LoadingCache<AccountGroup.Id, Optional<InternalGroup>> byId,
@Named(BYNAME_NAME) LoadingCache<String, Optional<InternalGroup>> byName,
@Named(BYUUID_NAME) LoadingCache<String, Optional<InternalGroup>> byUUID,
- SchemaFactory<ReviewDb> schema,
- Provider<GroupIndexer> indexer,
- Groups groups) {
+ Provider<GroupIndexer> indexer) {
this.byId = byId;
this.byName = byName;
this.byUUID = byUUID;
- this.schema = schema;
this.indexer = indexer;
- this.groups = groups;
}
@Override
@@ -152,16 +142,6 @@
}
@Override
- public ImmutableList<AccountGroup> all() {
- try (ReviewDb db = schema.open()) {
- return groups.getAll(db).collect(toImmutableList());
- } catch (OrmException e) {
- log.warn("Cannot list internal groups", e);
- return ImmutableList.of();
- }
- }
-
- @Override
public void onCreateGroup(AccountGroup group) throws IOException {
indexer.get().index(group.getGroupUUID());
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java
index e471d57c..60e6297 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java
@@ -19,12 +19,17 @@
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.group.Groups;
import com.google.gerrit.server.group.InternalGroupDescription;
import com.google.gerrit.server.project.ProjectState;
+import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.util.Collection;
+import java.util.Collections;
import org.eclipse.jgit.lib.ObjectId;
/** Implementation of GroupBackend for the internal group system. */
@@ -32,15 +37,21 @@
public class InternalGroupBackend implements GroupBackend {
private final GroupControl.Factory groupControlFactory;
private final GroupCache groupCache;
+ private final Groups groups;
+ private final Provider<ReviewDb> db;
private final IncludingGroupMembership.Factory groupMembershipFactory;
@Inject
InternalGroupBackend(
GroupControl.Factory groupControlFactory,
GroupCache groupCache,
+ Groups groups,
+ Provider<ReviewDb> db,
IncludingGroupMembership.Factory groupMembershipFactory) {
this.groupControlFactory = groupControlFactory;
this.groupCache = groupCache;
+ this.groups = groups;
+ this.db = db;
this.groupMembershipFactory = groupMembershipFactory;
}
@@ -61,16 +72,19 @@
@Override
public Collection<GroupReference> suggest(String name, ProjectState project) {
- return groupCache
- .all()
- .stream()
- .filter(
- group ->
- // startsWithIgnoreCase && isVisible
- group.getName().regionMatches(true, 0, name, 0, name.length())
- && groupControlFactory.controlFor(group).isVisible())
- .map(GroupReference::forGroup)
- .collect(toList());
+ try {
+ return groups
+ .getAll(db.get())
+ .filter(
+ group ->
+ // startsWithIgnoreCase && isVisible
+ group.getName().regionMatches(true, 0, name, 0, name.length())
+ && groupControlFactory.controlFor(group).isVisible())
+ .map(GroupReference::forGroup)
+ .collect(toList());
+ } catch (OrmException e) {
+ return Collections.emptyList();
+ }
}
@Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java
index 970269b..0f9cc89 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java
@@ -33,6 +33,7 @@
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountResource;
import com.google.gerrit.server.account.GetGroups;
@@ -75,6 +76,8 @@
private final GetGroups accountGetGroups;
private final GroupJson json;
private final GroupBackend groupBackend;
+ private final Groups groups;
+ private final Provider<ReviewDb> db;
private EnumSet<ListGroupsOption> options = EnumSet.noneOf(ListGroupsOption.class);
private boolean visibleToAll;
@@ -215,7 +218,9 @@
final IdentifiedUser.GenericFactory userFactory,
final GetGroups accountGetGroups,
GroupJson json,
- GroupBackend groupBackend) {
+ GroupBackend groupBackend,
+ Groups groups,
+ Provider<ReviewDb> db) {
this.groupCache = groupCache;
this.groupControlFactory = groupControlFactory;
this.genericGroupControlFactory = genericGroupControlFactory;
@@ -224,6 +229,8 @@
this.accountGetGroups = accountGetGroups;
this.json = json;
this.groupBackend = groupBackend;
+ this.groups = groups;
+ this.db = db;
}
public void setOptions(EnumSet<ListGroupsOption> options) {
@@ -379,11 +386,14 @@
}
private ImmutableList<GroupDescription.Internal> getAllExistingInternalGroups() {
- return groupCache
- .all()
- .stream()
- .map(GroupDescriptions::forAccountGroup)
- .collect(toImmutableList());
+ try {
+ return groups
+ .getAll(db.get())
+ .map(GroupDescriptions::forAccountGroup)
+ .collect(toImmutableList());
+ } catch (OrmException e) {
+ return ImmutableList.of();
+ }
}
private List<GroupDescription.Internal> filterGroups(
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/SystemGroupBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/SystemGroupBackend.java
index f0421a5..151b43d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/SystemGroupBackend.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/SystemGroupBackend.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.group;
import static com.google.common.base.Preconditions.checkNotNull;
+import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toSet;
import com.google.common.annotations.VisibleForTesting;
@@ -24,16 +25,18 @@
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.StartupCheck;
import com.google.gerrit.server.StartupException;
import com.google.gerrit.server.account.AbstractGroupBackend;
-import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.account.ListGroupMembership;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.project.ProjectState;
+import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.util.ArrayList;
import java.util.Collection;
@@ -184,12 +187,14 @@
public static class NameCheck implements StartupCheck {
private final Config cfg;
- private final GroupCache groupCache;
+ private final Groups groups;
+ private final Provider<ReviewDb> db;
@Inject
- NameCheck(@GerritServerConfig Config cfg, GroupCache groupCache) {
+ NameCheck(@GerritServerConfig Config cfg, Groups groups, Provider<ReviewDb> db) {
this.cfg = cfg;
- this.groupCache = groupCache;
+ this.groups = groups;
+ this.db = db;
}
@Override
@@ -206,7 +211,13 @@
if (configuredNames.isEmpty()) {
return;
}
- for (AccountGroup g : groupCache.all()) {
+ List<AccountGroup> allGroups;
+ try {
+ allGroups = groups.getAll(db.get()).collect(toList());
+ } catch (OrmException e) {
+ return;
+ }
+ for (AccountGroup g : allGroups) {
String name = g.getName().toLowerCase(Locale.US);
if (byLowerCaseConfiguredName.keySet().contains(name)) {
AccountGroup.UUID uuidSystemGroup = byLowerCaseConfiguredName.get(name);
