opendev/gerrit
Code Issues Proposed changes

GroupCache: Remove all() method

Browse Source 
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
This commit is contained in:
David Pursehouse
2017-09-22 21:43:09 +09:00
parent c956c41543
commit 5d1becedef
7 changed files with 67 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@@ -93,6 +93,7 @@ import com.google.gerrit.server.config.PluginConfigFactory;
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 @@ public abstract class AbstractDaemonTest {
@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 @@ public abstract class AbstractDaemonTest {
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 @@ public abstract class AbstractDaemonTest {
// 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 @@ public abstract class AbstractDaemonTest {
adminRestSession = new RestSession(server, admin);
userRestSession = new RestSession(server, user);
db = reviewDbProvider.open();
testRequiresSsh = classDesc.useSshAnnotation() || methodDesc.useSshAnnotation();
if (testRequiresSsh
&& SshMode.useSsh()

8
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
View File

@@ -30,7 +30,7 @@ import com.google.gerrit.common.TimeUtil;
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.UnprocessableEntityException;
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 @@ import org.junit.Test;
@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 @@ public class GroupsIT extends AbstractDaemonTest {
@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 @@ public class GroupsIT extends AbstractDaemonTest {
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");

4
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java
View File

@@ -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 @@ public interface GroupCache {
*/
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;

22
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java
View File

@@ -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.Groups;
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 @@ public class GroupCacheImpl implements GroupCache {
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
@@ -151,16 +141,6 @@ public class GroupCacheImpl implements GroupCache {
}
}
@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());

34
gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java
View File

@@ -19,12 +19,17 @@ import static java.util.stream.Collectors.toList;
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 @@ import org.eclipse.jgit.lib.ObjectId;
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 @@ public class InternalGroupBackend implements GroupBackend {
@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

22
gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java
View File

@@ -33,6 +33,7 @@ import com.google.gerrit.extensions.restapi.TopLevelResource;
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 @@ public class ListGroups implements RestReadView<TopLevelResource> {
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 @@ public class ListGroups implements RestReadView<TopLevelResource> {
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 @@ public class ListGroups implements RestReadView<TopLevelResource> {
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 @@ public class ListGroups implements RestReadView<TopLevelResource> {
}
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(

21
gerrit-server/src/main/java/com/google/gerrit/server/group/SystemGroupBackend.java
View File

@@ -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.common.collect.ImmutableSet;
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 class SystemGroupBackend extends AbstractGroupBackend {
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 @@ public class SystemGroupBackend extends AbstractGroupBackend {
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);