From 3e6c672b99e7f1c706b006d8f59cc149680a2602 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Fri, 25 Aug 2017 17:42:23 +0200 Subject: [PATCH] Cache instances of type InternalGroup instead of AccountGroup by UUID Change-Id: Id4725960eb155945f0344a56e7f36d5f4293750d --- .../gerrit/acceptance/AbstractDaemonTest.java | 8 +- .../rest/change/SuggestReviewersIT.java | 23 +++--- .../elasticsearch/ElasticGroupIndex.java | 2 +- .../gerrit/lucene/LuceneGroupIndex.java | 2 +- .../gerrit/server/account/GroupCache.java | 13 +--- .../gerrit/server/account/GroupCacheImpl.java | 78 ++++++++----------- .../server/account/GroupComparator.java | 26 ------- .../gerrit/server/account/GroupMembers.java | 16 ++-- .../account/IncludingGroupMembership.java | 6 +- .../server/account/InternalGroupBackend.java | 8 +- .../gerrit/server/group/GetAuditLog.java | 8 +- .../gerrit/server/group/GroupsUpdate.java | 4 +- .../com/google/gerrit/server/group/Index.java | 7 +- .../gerrit/server/group/ListGroups.java | 45 +++++++---- .../gerrit/server/group/ListMembers.java | 7 +- .../server/index/group/AllGroupsIndexer.java | 9 ++- .../server/index/group/GroupIndexerImpl.java | 2 +- .../server/query/group/GroupQueryBuilder.java | 7 +- .../sshd/commands/ListGroupsCommand.java | 10 ++- .../sshd/commands/SetMembersCommand.java | 8 +- 20 files changed, 135 insertions(+), 154 deletions(-) delete mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/account/GroupComparator.java 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 0db732b97f..ede83f31b1 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 @@ -92,6 +92,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.InternalGroup; import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.index.change.ChangeIndex; import com.google.gerrit.server.index.change.ChangeIndexCollection; @@ -354,7 +355,7 @@ public abstract class AbstractDaemonTest { // removes all indexed data. // As a workaround, we simply reindex all available groups here. for (AccountGroup group : groupCache.all()) { - groupCache.evict(group); + groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey()); } admin = accountCreator.admin(); @@ -1187,8 +1188,9 @@ public abstract class AbstractDaemonTest { String g = createGroup("cla-test-group"); GroupApi groupApi = gApi.groups().id(g); groupApi.description("CLA test group"); - AccountGroup caGroup = groupCache.get(new AccountGroup.UUID(groupApi.detail().id)); - GroupReference groupRef = GroupReference.forGroup(caGroup); + InternalGroup caGroup = + groupCache.get(new AccountGroup.UUID(groupApi.detail().id)).orElse(null); + GroupReference groupRef = new GroupReference(caGroup.getGroupUUID(), caGroup.getName()); PermissionRule rule = new PermissionRule(groupRef); rule.setAction(PermissionRule.Action.ALLOW); ca = new ContributorAgreement("cla-test"); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java index e5816dd2b5..cb0d768ef2 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java @@ -34,6 +34,7 @@ import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.group.CreateGroup; +import com.google.gerrit.server.group.InternalGroup; import com.google.inject.Inject; import java.util.Arrays; import java.util.List; @@ -44,9 +45,9 @@ import org.junit.Test; public class SuggestReviewersIT extends AbstractDaemonTest { @Inject private CreateGroup.Factory createGroupFactory; - private AccountGroup group1; - private AccountGroup group2; - private AccountGroup group3; + private InternalGroup group1; + private InternalGroup group2; + private InternalGroup group3; private TestAccount user1; private TestAccount user2; @@ -234,8 +235,8 @@ public class SuggestReviewersIT extends AbstractDaemonTest { @GerritConfig(name = "addreviewer.maxAllowed", value = "2") @GerritConfig(name = "addreviewer.maxWithoutConfirmation", value = "1") public void suggestReviewersGroupSizeConsiderations() throws Exception { - AccountGroup largeGroup = group("large"); - AccountGroup mediumGroup = group("medium"); + InternalGroup largeGroup = group("large"); + InternalGroup mediumGroup = group("medium"); // Both groups have Administrator as a member. Add two users to large // group to push it past maxAllowed, and one to medium group to push it @@ -424,19 +425,19 @@ public class SuggestReviewersIT extends AbstractDaemonTest { return gApi.changes().id(changeId).suggestReviewers(query).withLimit(n).get(); } - private AccountGroup group(String name) throws Exception { + private InternalGroup group(String name) throws Exception { GroupInfo group = createGroupFactory.create(name(name)).apply(TopLevelResource.INSTANCE, null); - return groupCache.get(new AccountGroup.UUID(group.id)); + return groupCache.get(new AccountGroup.UUID(group.id)).orElse(null); } - private TestAccount user(String name, String fullName, String emailName, AccountGroup... groups) + private TestAccount user(String name, String fullName, String emailName, InternalGroup... groups) throws Exception { - String[] groupNames = Arrays.stream(groups).map(AccountGroup::getName).toArray(String[]::new); + String[] groupNames = Arrays.stream(groups).map(InternalGroup::getName).toArray(String[]::new); return accountCreator.create( name(name), name(emailName) + "@example.com", fullName, groupNames); } - private TestAccount user(String name, String fullName, AccountGroup... groups) throws Exception { + private TestAccount user(String name, String fullName, InternalGroup... groups) throws Exception { return user(name, fullName, name, groups); } @@ -461,7 +462,7 @@ public class SuggestReviewersIT extends AbstractDaemonTest { private void assertReviewers( List actual, List expectedUsers, - List expectedGroups) { + List expectedGroups) { List actualAccountIds = actual .stream() diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticGroupIndex.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticGroupIndex.java index cb8e6a4cb6..6ca4ad570c 100644 --- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticGroupIndex.java +++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticGroupIndex.java @@ -213,7 +213,7 @@ public class ElasticGroupIndex extends AbstractElasticIndex getInternalGroup(AccountGroup.UUID groupUuid); + Optional get(AccountGroup.UUID groupUuid); /** @return sorted list of groups. */ ImmutableList all(); @@ -49,7 +41,8 @@ public interface GroupCache { /** Notify the cache that a new group was constructed. */ void onCreateGroup(AccountGroup.NameKey newGroupName) throws IOException; - void evict(AccountGroup group) throws IOException; + void evict(AccountGroup.UUID groupUuid, AccountGroup.Id groupId, AccountGroup.NameKey groupName) + throws IOException; void evictAfterRename(AccountGroup.NameKey oldName, AccountGroup.NameKey newName) 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 27a6b772ed..b5674094e9 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 @@ -22,7 +22,6 @@ import com.google.common.cache.LoadingCache; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.TimeUtil; -import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.server.ReviewDb; @@ -63,7 +62,7 @@ public class GroupCacheImpl implements GroupCache { cache(BYNAME_NAME, String.class, new TypeLiteral>() {}) .loader(ByNameLoader.class); - cache(BYUUID_NAME, String.class, new TypeLiteral>() {}) + cache(BYUUID_NAME, String.class, new TypeLiteral>() {}) .loader(ByUUIDLoader.class); bind(GroupCacheImpl.class); @@ -74,7 +73,7 @@ public class GroupCacheImpl implements GroupCache { private final LoadingCache> byId; private final LoadingCache> byName; - private final LoadingCache> byUUID; + private final LoadingCache> byUUID; private final SchemaFactory schema; private final Provider indexer; private final Groups groups; @@ -83,7 +82,7 @@ public class GroupCacheImpl implements GroupCache { GroupCacheImpl( @Named(BYID_NAME) LoadingCache> byId, @Named(BYNAME_NAME) LoadingCache> byName, - @Named(BYUUID_NAME) LoadingCache> byUUID, + @Named(BYUUID_NAME) LoadingCache> byUUID, SchemaFactory schema, Provider indexer, Groups groups) { @@ -107,17 +106,19 @@ public class GroupCacheImpl implements GroupCache { } @Override - public void evict(AccountGroup group) throws IOException { - if (group.getId() != null) { - byId.invalidate(group.getId()); + public void evict( + AccountGroup.UUID groupUuid, AccountGroup.Id groupId, AccountGroup.NameKey groupName) + throws IOException { + if (groupId != null) { + byId.invalidate(groupId); } - if (group.getNameKey() != null) { - byName.invalidate(group.getNameKey().get()); + if (groupName != null) { + byName.invalidate(groupName.get()); } - if (group.getGroupUUID() != null) { - byUUID.invalidate(group.getGroupUUID().get()); + if (groupUuid != null) { + byUUID.invalidate(groupUuid.get()); } - indexer.get().index(group.getGroupUUID()); + indexer.get().index(groupUuid); } @Override @@ -140,51 +141,23 @@ public class GroupCacheImpl implements GroupCache { try { return byName.get(name.get()).orElse(null); } catch (ExecutionException e) { - log.warn(String.format("Cannot lookup group %s by name", name.get()), e); + log.warn(String.format("Cannot look up group %s by name", name.get()), e); return null; } } @Override - public AccountGroup get(AccountGroup.UUID uuid) { - if (uuid == null) { - return null; - } - try { - return byUUID.get(uuid.get()).orElse(null); - } catch (ExecutionException e) { - log.warn(String.format("Cannot lookup group %s by uuid", uuid.get()), e); - return null; - } - } - - @Override - public Optional getInternalGroup(AccountGroup.UUID groupUuid) { + public Optional get(AccountGroup.UUID groupUuid) { if (groupUuid == null) { return Optional.empty(); } - Optional accountGroup = Optional.empty(); try { - accountGroup = byUUID.get(groupUuid.get()); + return byUUID.get(groupUuid.get()); } catch (ExecutionException e) { - log.warn(String.format("Cannot lookup group %s by uuid", groupUuid.get()), e); - } - - if (!accountGroup.isPresent()) { + log.warn(String.format("Cannot look up group %s by uuid", groupUuid.get()), e); return Optional.empty(); } - - try (ReviewDb db = schema.open()) { - ImmutableSet members = groups.getMembers(db, groupUuid).collect(toImmutableSet()); - ImmutableSet includes = - groups.getIncludes(db, groupUuid).collect(toImmutableSet()); - return accountGroup.map(group -> InternalGroup.create(group, members, includes)); - } catch (OrmException | NoSuchGroupException e) { - log.warn( - String.format("Cannot lookup members or sub-groups of group %s", groupUuid.get()), e); - } - return Optional.empty(); } @Override @@ -244,7 +217,7 @@ public class GroupCacheImpl implements GroupCache { } } - static class ByUUIDLoader extends CacheLoader> { + static class ByUUIDLoader extends CacheLoader> { private final SchemaFactory schema; private final Groups groups; @@ -255,9 +228,20 @@ public class GroupCacheImpl implements GroupCache { } @Override - public Optional load(String uuid) throws Exception { + public Optional load(String uuid) throws Exception { try (ReviewDb db = schema.open()) { - return groups.getGroup(db, new AccountGroup.UUID(uuid)); + AccountGroup.UUID groupUuid = new AccountGroup.UUID(uuid); + Optional accountGroup = groups.getGroup(db, groupUuid); + + if (!accountGroup.isPresent()) { + return Optional.empty(); + } + + ImmutableSet members = + groups.getMembers(db, groupUuid).collect(toImmutableSet()); + ImmutableSet includes = + groups.getIncludes(db, groupUuid).collect(toImmutableSet()); + return accountGroup.map(group -> InternalGroup.create(group, members, includes)); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupComparator.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupComparator.java deleted file mode 100644 index 6ba2e5e365..0000000000 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupComparator.java +++ /dev/null @@ -1,26 +0,0 @@ -// Copyright (C) 2011 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.gerrit.server.account; - -import com.google.gerrit.reviewdb.client.AccountGroup; -import java.util.Comparator; - -public class GroupComparator implements Comparator { - - @Override - public int compare(AccountGroup group1, AccountGroup group2) { - return group1.getName().compareTo(group2.getName()); - } -} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupMembers.java index bf8ec33ea3..272514ce10 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupMembers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupMembers.java @@ -20,6 +20,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.ProjectControl; @@ -29,6 +30,7 @@ import com.google.inject.assistedinject.Assisted; import java.io.IOException; import java.util.Collections; import java.util.HashSet; +import java.util.Optional; import java.util.Set; public class GroupMembers { @@ -69,9 +71,9 @@ public class GroupMembers { if (SystemGroupBackend.PROJECT_OWNERS.equals(groupUUID)) { return getProjectOwners(project, seen); } - AccountGroup group = groupCache.get(groupUUID); - if (group != null) { - return getGroupMembers(group, project, seen); + Optional group = groupCache.get(groupUUID); + if (group.isPresent()) { + return getGroupMembers(group.get(), project, seen); } return Collections.emptySet(); } @@ -96,7 +98,7 @@ public class GroupMembers { } private Set getGroupMembers( - final AccountGroup group, Project.NameKey project, Set seen) + InternalGroup group, Project.NameKey project, Set seen) throws NoSuchGroupException, OrmException, NoSuchProjectException, IOException { seen.add(group.getGroupUUID()); final GroupDetail groupDetail = groupDetailFactory.create(group.getGroupUUID()).call(); @@ -107,9 +109,9 @@ public class GroupMembers { } for (AccountGroup.UUID groupIncludeUuid : groupDetail.getIncludes()) { - AccountGroup includedGroup = groupCache.get(groupIncludeUuid); - if (includedGroup != null && !seen.contains(includedGroup.getGroupUUID())) { - members.addAll(listAccounts(includedGroup.getGroupUUID(), project, seen)); + Optional includedGroup = groupCache.get(groupIncludeUuid); + if (includedGroup.isPresent() && !seen.contains(includedGroup.get().getGroupUUID())) { + members.addAll(listAccounts(includedGroup.get().getGroupUUID(), project, seen)); } } return members; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java index 165cbb6c72..d6b3edf967 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java @@ -19,11 +19,13 @@ import com.google.common.collect.Lists; import com.google.common.collect.Sets; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.group.InternalGroup; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -91,8 +93,8 @@ public class IncludingGroupMembership implements GroupMembership { } memberOf.put(id, false); - AccountGroup group = groupCache.get(id); - if (group == null) { + Optional group = groupCache.get(id); + if (!group.isPresent()) { continue; } if (search(includeCache.subgroupsOf(id))) { 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 0721212841..e471d57cce 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 @@ -17,10 +17,10 @@ package com.google.gerrit.server.account; import static java.util.stream.Collectors.toList; import com.google.gerrit.common.data.GroupDescription; -import com.google.gerrit.common.data.GroupDescriptions; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.group.InternalGroupDescription; import com.google.gerrit.server.project.ProjectState; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -56,11 +56,7 @@ public class InternalGroupBackend implements GroupBackend { return null; } - AccountGroup g = groupCache.get(uuid); - if (g == null) { - return null; - } - return GroupDescriptions.forAccountGroup(g); + return groupCache.get(uuid).map(InternalGroupDescription::new).orElse(null); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetAuditLog.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetAuditLog.java index 4ea7041da7..8c94f65f12 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GetAuditLog.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GetAuditLog.java @@ -15,7 +15,6 @@ package com.google.gerrit.server.group; import com.google.gerrit.common.data.GroupDescription; -import com.google.gerrit.common.data.GroupDescriptions; import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.common.GroupAuditEventInfo; import com.google.gerrit.extensions.common.GroupInfo; @@ -38,6 +37,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; import java.util.List; +import java.util.Optional; @Singleton public class GetAuditLog implements RestReadView { @@ -94,10 +94,10 @@ public class GetAuditLog implements RestReadView { for (AccountGroupByIdAud auditEvent : db.get().accountGroupByIdAud().byGroup(group.getId()).toList()) { AccountGroup.UUID includedGroupUUID = auditEvent.getKey().getIncludeUUID(); - AccountGroup includedGroup = groupCache.get(includedGroupUUID); + Optional includedGroup = groupCache.get(includedGroupUUID); GroupInfo member; - if (includedGroup != null) { - member = groupJson.format(GroupDescriptions.forAccountGroup(includedGroup)); + if (includedGroup.isPresent()) { + member = groupJson.format(new InternalGroupDescription(includedGroup.get())); } else { GroupDescription.Basic groupDescription = groupBackend.get(includedGroupUUID); member = new GroupInfo(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java index 339a2761e9..f6ca34115f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java @@ -170,7 +170,7 @@ public class GroupsUpdate { ReviewDb db, AccountGroup.UUID groupUuid, Consumer groupConsumer) throws OrmException, IOException, NoSuchGroupException { AccountGroup updatedGroup = updateGroupInDb(db, groupUuid, groupConsumer); - groupCache.evict(updatedGroup); + groupCache.evict(updatedGroup.getGroupUUID(), updatedGroup.getId(), updatedGroup.getNameKey()); } @VisibleForTesting @@ -221,7 +221,7 @@ public class GroupsUpdate { db.accountGroupNames().deleteKeys(ImmutableList.of(oldName)); - groupCache.evict(group); + groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey()); groupCache.evictAfterRename(oldName, newName); @SuppressWarnings("unused") diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/Index.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/Index.java index 5d076d9a40..b61f954960 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/Index.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/Index.java @@ -24,6 +24,7 @@ import com.google.gerrit.server.group.Index.Input; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; +import java.util.Optional; @Singleton public class Index implements RestModifyView { @@ -49,9 +50,11 @@ public class Index implements RestModifyView { String.format("External Group Not Allowed: %s", groupUuid.get())); } - AccountGroup accountGroup = groupCache.get(groupUuid); + Optional group = groupCache.get(groupUuid); // evicting the group from the cache, reindexes the group - groupCache.evict(accountGroup); + if (group.isPresent()) { + groupCache.evict(group.get().getGroupUUID(), group.get().getId(), group.get().getNameKey()); + } return Response.none(); } } 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 23c5fee37b..ff8396be41 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 @@ -14,8 +14,11 @@ package com.google.gerrit.server.group; +import static com.google.common.collect.ImmutableList.toImmutableList; + import com.google.common.base.MoreObjects; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.gerrit.common.data.GroupDescription; @@ -35,7 +38,6 @@ import com.google.gerrit.server.account.AccountResource; import com.google.gerrit.server.account.GetGroups; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.GroupCache; -import com.google.gerrit.server.account.GroupComparator; import com.google.gerrit.server.account.GroupControl; import com.google.gerrit.server.project.ProjectControl; import com.google.gwtorm.server.OrmException; @@ -43,13 +45,14 @@ import com.google.inject.Inject; import com.google.inject.Provider; import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; +import java.util.Comparator; import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.SortedMap; import java.util.TreeMap; @@ -58,6 +61,8 @@ import org.kohsuke.args4j.Option; /** List groups visible to the calling user. */ public class ListGroups implements RestReadView { + private static final Comparator GROUP_COMPARATOR = + Comparator.comparing(GroupDescription.Basic::getName); protected final GroupCache groupCache; @@ -266,33 +271,32 @@ public class ListGroups implements RestReadView { private List getAllGroups() throws OrmException { List groupInfos; - List groupList; + List groupList; if (!projects.isEmpty()) { - Map groups = new HashMap<>(); + Map groups = new HashMap<>(); for (ProjectControl projectControl : projects) { final Set groupsRefs = projectControl.getAllGroups(); for (GroupReference groupRef : groupsRefs) { - final AccountGroup group = groupCache.get(groupRef.getUUID()); - if (group != null) { - groups.put(group.getGroupUUID(), group); - } + Optional internalGroup = groupCache.get(groupRef.getUUID()); + internalGroup.ifPresent( + group -> groups.put(group.getGroupUUID(), new InternalGroupDescription(group))); } } groupList = filterGroups(groups.values()); } else { - groupList = filterGroups(groupCache.all()); + groupList = filterGroups(getAllExistingInternalGroups()); } groupInfos = Lists.newArrayListWithCapacity(groupList.size()); int found = 0; int foundIndex = 0; - for (AccountGroup group : groupList) { + for (GroupDescription.Internal group : groupList) { if (foundIndex++ < start) { continue; } if (limit > 0 && ++found > limit) { break; } - groupInfos.add(json.addOptions(options).format(GroupDescriptions.forAccountGroup(group))); + groupInfos.add(json.addOptions(options).format(group)); } return groupInfos; } @@ -355,7 +359,7 @@ public class ListGroups implements RestReadView { List groups = new ArrayList<>(); int found = 0; int foundIndex = 0; - for (AccountGroup g : filterGroups(groupCache.all())) { + for (GroupDescription.Internal g : filterGroups(getAllExistingInternalGroups())) { GroupControl ctl = groupControlFactory.controlFor(g); try { if (genericGroupControlFactory.controlFor(user, g.getGroupUUID()).isOwner()) { @@ -374,10 +378,19 @@ public class ListGroups implements RestReadView { return groups; } - private List filterGroups(Collection groups) { - List filteredGroups = new ArrayList<>(groups.size()); + private ImmutableList getAllExistingInternalGroups() { + return groupCache + .all() + .stream() + .map(GroupDescriptions::forAccountGroup) + .collect(toImmutableList()); + } + + private List filterGroups( + Collection groups) { + List filteredGroups = new ArrayList<>(groups.size()); Pattern pattern = Strings.isNullOrEmpty(matchRegex) ? null : Pattern.compile(matchRegex); - for (AccountGroup group : groups) { + for (GroupDescription.Internal group : groups) { if (!Strings.isNullOrEmpty(matchSubstring)) { if (!group .getName() @@ -402,7 +415,7 @@ public class ListGroups implements RestReadView { filteredGroups.add(group); } } - Collections.sort(filteredGroups, new GroupComparator()); + filteredGroups.sort(GROUP_COMPARATOR); return filteredGroups; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListMembers.java index 0f8aa407c9..353d1930cc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListMembers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListMembers.java @@ -34,6 +34,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Optional; import org.kohsuke.args4j.Option; public class ListMembers implements RestReadView { @@ -85,15 +86,15 @@ public class ListMembers implements RestReadView { seenGroups.add(groupUUID); final Map members = new HashMap<>(); - final AccountGroup group = groupCache.get(groupUUID); - if (group == null) { + Optional group = groupCache.get(groupUUID); + if (!group.isPresent()) { // the included group is an external group and can't be resolved return Collections.emptyMap(); } final GroupDetail groupDetail; try { - groupDetail = groupDetailFactory.create(group.getGroupUUID()).call(); + groupDetail = groupDetailFactory.create(group.get().getGroupUUID()).call(); } catch (NoSuchGroupException e) { // the included group is not visible return Collections.emptyMap(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java index 3c72ee2460..2b59675a7f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/group/AllGroupsIndexer.java @@ -94,11 +94,12 @@ public class AllGroupsIndexer extends SiteIndexer { try { - AccountGroup oldGroup = groupCache.get(uuid); - if (oldGroup != null) { - groupCache.evict(oldGroup); + Optional oldGroup = groupCache.get(uuid); + if (oldGroup.isPresent()) { + InternalGroup group = oldGroup.get(); + groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey()); } - Optional internalGroup = groupCache.getInternalGroup(uuid); + Optional internalGroup = groupCache.get(uuid); if (internalGroup.isPresent()) { index.replace(internalGroup.get()); } else { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java index c93a451a92..69b29bcf29 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java @@ -66,7 +66,7 @@ public class GroupIndexerImpl implements GroupIndexer { @Override public void index(AccountGroup.UUID uuid) throws IOException { for (Index i : getWriteIndexes()) { - Optional internalGroup = groupCache.getInternalGroup(uuid); + Optional internalGroup = groupCache.get(uuid); if (internalGroup.isPresent()) { i.replace(internalGroup.get()); } else { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java index 86f40d44ea..2388bb7cf8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java @@ -29,6 +29,7 @@ import com.google.gerrit.server.account.GroupCache; import com.google.gerrit.server.group.InternalGroup; import com.google.inject.Inject; import java.util.List; +import java.util.Optional; /** Parses a query string meant to be applied to group objects. */ public class GroupQueryBuilder extends QueryBuilder { @@ -90,9 +91,9 @@ public class GroupQueryBuilder extends QueryBuilder { @Operator public Predicate owner(String owner) throws QueryParseException { - AccountGroup group = args.groupCache.get(new AccountGroup.UUID(owner)); - if (group != null) { - return GroupPredicates.owner(group.getGroupUUID()); + Optional group = args.groupCache.get(new AccountGroup.UUID(owner)); + if (group.isPresent()) { + return GroupPredicates.owner(group.get().getGroupUUID()); } GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend, owner); if (g == null) { diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListGroupsCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListGroupsCommand.java index 2d42f0121d..ac3784a058 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListGroupsCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListGroupsCommand.java @@ -22,12 +22,14 @@ import com.google.gerrit.extensions.common.GroupInfo; import com.google.gerrit.extensions.restapi.Url; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.server.account.GroupCache; +import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.ListGroups; import com.google.gerrit.server.ioutil.ColumnFormatter; import com.google.gerrit.sshd.CommandMetaData; import com.google.gerrit.sshd.SshCommand; import com.google.gerrit.util.cli.Options; import com.google.inject.Inject; +import java.util.Optional; import org.kohsuke.args4j.Option; @CommandMetaData( @@ -60,15 +62,15 @@ public class ListGroupsCommand extends SshCommand { for (GroupInfo info : listGroups.get()) { formatter.addColumn(MoreObjects.firstNonNull(info.name, "n/a")); if (verboseOutput) { - AccountGroup o = + Optional group = info.ownerId != null ? groupCache.get(new AccountGroup.UUID(Url.decode(info.ownerId))) - : null; + : Optional.empty(); formatter.addColumn(Url.decode(info.id)); formatter.addColumn(Strings.nullToEmpty(info.description)); - formatter.addColumn(o != null ? o.getName() : "n/a"); - formatter.addColumn(o != null ? o.getGroupUUID().get() : ""); + formatter.addColumn(group.map(InternalGroup::getName).orElse("n/a")); + formatter.addColumn(group.map(g -> g.getGroupUUID().get()).orElse("")); formatter.addColumn( Boolean.toString(MoreObjects.firstNonNull(info.options.visibleToAll, Boolean.FALSE))); } diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetMembersCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetMembersCommand.java index ae47175ecb..a5ebaebd6c 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetMembersCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetMembersCommand.java @@ -18,6 +18,7 @@ import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toList; import com.google.common.base.MoreObjects; +import com.google.common.collect.Streams; import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.TopLevelResource; @@ -31,6 +32,7 @@ import com.google.gerrit.server.group.DeleteIncludedGroups; import com.google.gerrit.server.group.DeleteMembers; import com.google.gerrit.server.group.GroupResource; import com.google.gerrit.server.group.GroupsCollection; +import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.sshd.CommandMetaData; import com.google.gerrit.sshd.SshCommand; import com.google.inject.Inject; @@ -149,7 +151,11 @@ public class SetMembersCommand extends SshCommand { String action, GroupResource group, List groupUuidList) throws UnsupportedEncodingException, IOException { String names = - groupUuidList.stream().map(uuid -> groupCache.get(uuid).getName()).collect(joining(", ")); + groupUuidList + .stream() + .map(uuid -> groupCache.get(uuid).map(InternalGroup::getName)) + .flatMap(Streams::stream) + .collect(joining(", ")); out.write( String.format("Groups %s group %s: %s\n", action, group.getName(), names).getBytes(ENC)); }