opendev/gerrit
Code Issues Proposed changes

Avoid unnecessary loading of members and subgroups

Browse Source 
Change-Id: I3630cb4c7859c5bd63d059654abb7ed8ad2f49ee
This commit is contained in:
Alice Kober-Sotzek 2017-08-25 17:51:34 +02:00
parent 3e6c672b99
commit b93e9b1ad9
11 changed files with 83 additions and 192 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

37
gerrit-common/src/main/java/com/google/gerrit/common/data/GroupDetail.java
View File

@ -1,37 +0,0 @@
// Copyright (C) 2008 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.common.data;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import java.util.Set;
public class GroupDetail {
private Set<Account.Id> members;
private Set<AccountGroup.UUID> includes;
public GroupDetail(Set<Account.Id> members, Set<AccountGroup.UUID> includes) {
this.members = members;
this.includes = includes;
}
public Set<Account.Id> getMembers() {
return members;
}
public Set<AccountGroup.UUID> getIncludes() {
return includes;
}
}

78
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupDetailFactory.java
View File

@ -1,78 +0,0 @@
// Copyright (C) 2009 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 static com.google.common.collect.ImmutableSet.toImmutableSet;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.common.data.GroupDetail;
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;
import com.google.gerrit.server.group.Groups;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.util.concurrent.Callable;
public class GroupDetailFactory implements Callable<GroupDetail> {
public interface Factory {
GroupDetailFactory create(AccountGroup.UUID groupUuid);
}
private final ReviewDb db;
private final GroupControl.Factory groupControl;
private final Groups groups;
private final GroupIncludeCache groupIncludeCache;
private final AccountGroup.UUID groupUuid;
private GroupControl control;
@Inject
GroupDetailFactory(
ReviewDb db,
GroupControl.Factory groupControl,
Groups groups,
GroupIncludeCache groupIncludeCache,
@Assisted AccountGroup.UUID groupUuid) {
this.db = db;
this.groupControl = groupControl;
this.groups = groups;
this.groupIncludeCache = groupIncludeCache;
this.groupUuid = groupUuid;
}
@Override
public GroupDetail call() throws OrmException, NoSuchGroupException {
control = groupControl.validateFor(groupUuid);
ImmutableSet<Account.Id> members = loadMembers();
ImmutableSet<AccountGroup.UUID> includes = loadIncludes();
return new GroupDetail(members, includes);
}
private ImmutableSet<Account.Id> loadMembers() throws OrmException, NoSuchGroupException {
return groups.getMembers(db, groupUuid).filter(control::canSeeMember).collect(toImmutableSet());
}
private ImmutableSet<AccountGroup.UUID> loadIncludes() {
if (!control.canSeeGroup()) {
return ImmutableSet.of();
}
return ImmutableSet.copyOf(groupIncludeCache.subgroupsOf(groupUuid));
}
}

46
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupMembers.java
View File

@ -14,13 +14,16 @@
package com.google.gerrit.server.account;
import com.google.gerrit.common.data.GroupDetail;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import com.google.common.collect.Sets;
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.client.Project;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.InternalGroupDescription;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectControl;
@ -39,20 +42,20 @@ public class GroupMembers {
}
private final GroupCache groupCache;
private final GroupDetailFactory.Factory groupDetailFactory;
private final GroupControl.Factory groupControlFactory;
private final AccountCache accountCache;
private final ProjectControl.GenericFactory projectControl;
private final CurrentUser currentUser;
@Inject
GroupMembers(
final GroupCache groupCache,
final GroupDetailFactory.Factory groupDetailFactory,
final AccountCache accountCache,
final ProjectControl.GenericFactory projectControl,
@Assisted final CurrentUser currentUser) {
GroupCache groupCache,
GroupControl.Factory groupControlFactory,
AccountCache accountCache,
ProjectControl.GenericFactory projectControl,
@Assisted CurrentUser currentUser) {
this.groupCache = groupCache;
this.groupDetailFactory = groupDetailFactory;
this.groupControlFactory = groupControlFactory;
this.accountCache = accountCache;
this.projectControl = projectControl;
this.currentUser = currentUser;
@ -101,19 +104,26 @@ public class GroupMembers {
InternalGroup group, Project.NameKey project, Set<AccountGroup.UUID> seen)
throws NoSuchGroupException, OrmException, NoSuchProjectException, IOException {
seen.add(group.getGroupUUID());
final GroupDetail groupDetail = groupDetailFactory.create(group.getGroupUUID()).call();
GroupControl groupControl = groupControlFactory.controlFor(new InternalGroupDescription(group));
final Set<Account> members = new HashSet<>();
for (Account.Id memberId : groupDetail.getMembers()) {
members.add(accountCache.get(memberId).getAccount());
}
Set<Account> directMembers =
group
.getMembers()
.stream()
.filter(groupControl::canSeeMember)
.map(accountCache::get)
.map(AccountState::getAccount)
.collect(toImmutableSet());
for (AccountGroup.UUID groupIncludeUuid : groupDetail.getIncludes()) {
Optional<InternalGroup> includedGroup = groupCache.get(groupIncludeUuid);
if (includedGroup.isPresent() && !seen.contains(includedGroup.get().getGroupUUID())) {
members.addAll(listAccounts(includedGroup.get().getGroupUUID(), project, seen));
Set<Account> indirectMembers = new HashSet<>();
if (groupControl.canSeeGroup()) {
for (AccountGroup.UUID subgroupUuid : group.getIncludes()) {
if (!seen.contains(subgroupUuid)) {
indirectMembers.addAll(listAccounts(subgroupUuid, project, seen));
}
}
}
return members;
return Sets.union(directMembers, indirectMembers);
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java
View File

@ -97,7 +97,7 @@ public class IncludingGroupMembership implements GroupMembership {
if (!group.isPresent()) {
continue;
}
if (search(includeCache.subgroupsOf(id))) {
if (search(group.get().getIncludes())) {
memberOf.put(id, true);
return true;
}

2
gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
View File

@ -90,7 +90,6 @@ import com.google.gerrit.server.account.ChangeUserName;
import com.google.gerrit.server.account.EmailExpander;
import com.google.gerrit.server.account.GroupCacheImpl;
import com.google.gerrit.server.account.GroupControl;
import com.google.gerrit.server.account.GroupDetailFactory;
import com.google.gerrit.server.account.GroupIncludeCacheImpl;
import com.google.gerrit.server.account.GroupMembers;
import com.google.gerrit.server.account.VersionedAuthorizedKeys;
@ -250,7 +249,6 @@ public class GerritGlobalModule extends FactoryModule {
factory(ChangeData.AssistedFactory.class);
factory(ChangeJson.AssistedFactory.class);
factory(CreateChangeSender.Factory.class);
factory(GroupDetailFactory.Factory.class);
factory(GroupMembers.Factory.class);
factory(EmailMerge.Factory.class);
factory(MergedSender.Factory.class);

6
gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java
View File

@ -34,6 +34,7 @@ import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
@ -86,7 +87,7 @@ public class AddIncludedGroups implements RestModifyView<GroupResource, Input> {
@Override
public List<GroupInfo> apply(GroupResource resource, Input input)
throws MethodNotAllowedException, AuthException, UnprocessableEntityException, OrmException,
ResourceNotFoundException {
ResourceNotFoundException, IOException {
GroupDescription.Internal group =
resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
input = Input.init(input);
@ -126,7 +127,8 @@ public class AddIncludedGroups implements RestModifyView<GroupResource, Input> {
@Override
public GroupInfo apply(GroupResource resource, Input input)
throws AuthException, MethodNotAllowedException, ResourceNotFoundException, OrmException {
throws AuthException, MethodNotAllowedException, ResourceNotFoundException, OrmException,
IOException {
AddIncludedGroups.Input in = new AddIncludedGroups.Input();
in.groups = ImmutableList.of(id);
try {

5
gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteIncludedGroups.java
View File

@ -31,6 +31,7 @@ import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.HashSet;
import java.util.Set;
@ -53,7 +54,7 @@ public class DeleteIncludedGroups implements RestModifyView<GroupResource, Input
@Override
public Response<?> apply(GroupResource resource, Input input)
throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException,
ResourceNotFoundException {
ResourceNotFoundException, IOException {
GroupDescription.Internal internalGroup =
resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
input = Input.init(input);
@ -95,7 +96,7 @@ public class DeleteIncludedGroups implements RestModifyView<GroupResource, Input
@Override
public Response<?> apply(IncludedGroupResource resource, Input input)
throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException,
ResourceNotFoundException {
ResourceNotFoundException, IOException {
AddIncludedGroups.Input in = new AddIncludedGroups.Input();
in.groups = ImmutableList.of(resource.getMember().get());
return delete.get().apply(resource, in);

14
gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsUpdate.java
View File

@ -259,7 +259,7 @@ public class GroupsUpdate {
* @param groupUuid the UUID of the group
* @param accountIds a set of IDs of accounts to add
* @throws OrmException if an error occurs while reading/writing from/to ReviewDb
* @throws IOException if the cache entry of one of the new members couldn't be invalidated
* @throws IOException if the group or one of the new members couldn't be indexed
* @throws NoSuchGroupException if the specified group doesn't exist
*/
public void addGroupMembers(ReviewDb db, AccountGroup.UUID groupUuid, Set<Account.Id> accountIds)
@ -293,6 +293,7 @@ public class GroupsUpdate {
auditService.dispatchAddAccountsToGroup(currentUser.getAccountId(), newMembers);
}
db.accountGroupMembers().insert(newMembers);
groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey());
for (AccountGroupMember newMember : newMembers) {
accountCache.evict(newMember.getAccountId());
}
@ -306,7 +307,7 @@ public class GroupsUpdate {
* @param groupUuid the UUID of the group
* @param accountIds a set of IDs of accounts to remove
* @throws OrmException if an error occurs while reading/writing from/to ReviewDb
* @throws IOException if the cache entry of one of the removed members couldn't be invalidated
* @throws IOException if the group or one of the removed members couldn't be indexed
* @throws NoSuchGroupException if the specified group doesn't exist
*/
public void removeGroupMembers(
@ -331,6 +332,7 @@ public class GroupsUpdate {
auditService.dispatchDeleteAccountsFromGroup(currentUser.getAccountId(), membersToRemove);
}
db.accountGroupMembers().delete(membersToRemove);
groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey());
for (AccountGroupMember member : membersToRemove) {
accountCache.evict(member.getAccountId());
}
@ -349,11 +351,12 @@ public class GroupsUpdate {
* @param parentGroupUuid the UUID of the parent group
* @param includedGroupUuids a set of IDs of the groups to add as subgroups
* @throws OrmException if an error occurs while reading/writing from/to ReviewDb
* @throws IOException if the parent group couldn't be indexed
* @throws NoSuchGroupException if the specified parent group doesn't exist
*/
public void addIncludedGroups(
ReviewDb db, AccountGroup.UUID parentGroupUuid, Set<AccountGroup.UUID> includedGroupUuids)
throws OrmException, NoSuchGroupException {
throws OrmException, NoSuchGroupException, IOException {
AccountGroup parentGroup = groups.getExistingGroup(db, parentGroupUuid);
AccountGroup.Id parentGroupId = parentGroup.getId();
Set<AccountGroupById> newIncludedGroups = new HashSet<>();
@ -373,6 +376,7 @@ public class GroupsUpdate {
auditService.dispatchAddGroupsToGroup(currentUser.getAccountId(), newIncludedGroups);
}
db.accountGroupById().insert(newIncludedGroups);
groupCache.evict(parentGroup.getGroupUUID(), parentGroup.getId(), parentGroup.getNameKey());
for (AccountGroupById newIncludedGroup : newIncludedGroups) {
groupIncludeCache.evictParentGroupsOf(newIncludedGroup.getIncludeUUID());
}
@ -390,11 +394,12 @@ public class GroupsUpdate {
* @param parentGroupUuid the UUID of the parent group
* @param includedGroupUuids a set of IDs of the subgroups to remove from the parent group
* @throws OrmException if an error occurs while reading/writing from/to ReviewDb
* @throws IOException if the parent group couldn't be indexed
* @throws NoSuchGroupException if the specified parent group doesn't exist
*/
public void deleteIncludedGroups(
ReviewDb db, AccountGroup.UUID parentGroupUuid, Set<AccountGroup.UUID> includedGroupUuids)
throws OrmException, NoSuchGroupException {
throws OrmException, NoSuchGroupException, IOException {
AccountGroup parentGroup = groups.getExistingGroup(db, parentGroupUuid);
AccountGroup.Id parentGroupId = parentGroup.getId();
Set<AccountGroupById> includedGroupsToRemove = new HashSet<>();
@ -415,6 +420,7 @@ public class GroupsUpdate {
currentUser.getAccountId(), includedGroupsToRemove);
}
db.accountGroupById().delete(includedGroupsToRemove);
groupCache.evict(parentGroup.getGroupUUID(), parentGroup.getId(), parentGroup.getNameKey());
for (AccountGroupById groupToRemove : includedGroupsToRemove) {
groupIncludeCache.evictParentGroupsOf(groupToRemove.getIncludeUUID());
}

76
gerrit-server/src/main/java/com/google/gerrit/server/group/ListMembers.java
View File

@ -14,10 +14,11 @@
package com.google.gerrit.server.group;
import com.google.common.collect.Lists;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupDetail;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.RestReadView;
@ -25,21 +26,20 @@ import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.account.GroupDetailFactory;
import com.google.gerrit.server.account.GroupControl;
import com.google.gerrit.server.api.accounts.AccountInfoComparator;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import java.util.Collections;
import java.util.HashMap;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import org.kohsuke.args4j.Option;
public class ListMembers implements RestReadView<GroupResource> {
private final GroupCache groupCache;
private final GroupDetailFactory.Factory groupDetailFactory;
private final GroupControl.Factory groupControlFactory;
private final AccountLoader accountLoader;
@Option(name = "--recursive", usage = "to resolve included groups recursively")
@ -48,10 +48,10 @@ public class ListMembers implements RestReadView<GroupResource> {
@Inject
protected ListMembers(
GroupCache groupCache,
GroupDetailFactory.Factory groupDetailFactory,
GroupControl.Factory groupControlFactory,
AccountLoader.Factory accountLoaderFactory) {
this.groupCache = groupCache;
this.groupDetailFactory = groupDetailFactory;
this.groupControlFactory = groupControlFactory;
this.accountLoader = accountLoaderFactory.create(true);
}
@ -68,52 +68,40 @@ public class ListMembers implements RestReadView<GroupResource> {
return apply(group.getGroupUUID());
}
public List<AccountInfo> apply(AccountGroup group) throws OrmException {
return apply(group.getGroupUUID());
}
public List<AccountInfo> apply(AccountGroup.UUID groupId) throws OrmException {
final Map<Account.Id, AccountInfo> members =
getMembers(groupId, new HashSet<AccountGroup.UUID>());
final List<AccountInfo> memberInfos = Lists.newArrayList(members.values());
Collections.sort(memberInfos, AccountInfoComparator.ORDER_NULLS_FIRST);
Set<Account.Id> members = getMembers(groupId, new HashSet<>());
List<AccountInfo> memberInfos = new ArrayList<>(members.size());
for (Account.Id member : members) {
memberInfos.add(accountLoader.get(member));
}
accountLoader.fill();
memberInfos.sort(AccountInfoComparator.ORDER_NULLS_FIRST);
return memberInfos;
}
private Map<Account.Id, AccountInfo> getMembers(
final AccountGroup.UUID groupUUID, HashSet<AccountGroup.UUID> seenGroups)
throws OrmException {
private Set<Account.Id> getMembers(
AccountGroup.UUID groupUUID, HashSet<AccountGroup.UUID> seenGroups) {
seenGroups.add(groupUUID);
final Map<Account.Id, AccountInfo> members = new HashMap<>();
Optional<InternalGroup> group = groupCache.get(groupUUID);
if (!group.isPresent()) {
// the included group is an external group and can't be resolved
return Collections.emptyMap();
Optional<InternalGroup> internalGroup = groupCache.get(groupUUID);
if (!internalGroup.isPresent()) {
return ImmutableSet.of();
}
InternalGroup group = internalGroup.get();
final GroupDetail groupDetail;
try {
groupDetail = groupDetailFactory.create(group.get().getGroupUUID()).call();
} catch (NoSuchGroupException e) {
// the included group is not visible
return Collections.emptyMap();
}
GroupControl groupControl = groupControlFactory.controlFor(new InternalGroupDescription(group));
for (Account.Id member : groupDetail.getMembers()) {
if (!members.containsKey(member)) {
members.put(member, accountLoader.get(member));
}
}
Set<Account.Id> directMembers =
group.getMembers().stream().filter(groupControl::canSeeMember).collect(toImmutableSet());
if (recursive) {
for (AccountGroup.UUID includedGroupUuid : groupDetail.getIncludes()) {
if (!seenGroups.contains(includedGroupUuid)) {
members.putAll(getMembers(includedGroupUuid, seenGroups));
Set<Account.Id> indirectMembers = new HashSet<>();
if (recursive && groupControl.canSeeGroup()) {
for (AccountGroup.UUID subgroupUuid : group.getIncludes()) {
if (!seenGroups.contains(subgroupUuid)) {
indirectMembers.addAll(getMembers(subgroupUuid, seenGroups));
}
}
}
accountLoader.fill();
return members;
return Sets.union(directMembers, indirectMembers);
}
}

3
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/CreateGroupCommand.java
View File

@ -142,7 +142,8 @@ final class CreateGroupCommand extends SshCommand {
addMembers.apply(rsrc, input);
}
private void addIncludedGroups(GroupResource rsrc) throws RestApiException, OrmException {
private void addIncludedGroups(GroupResource rsrc)
throws RestApiException, OrmException, IOException {
AddIncludedGroups.Input input =
AddIncludedGroups.Input.fromGroups(
initialGroups.stream().map(AccountGroup.UUID::get).collect(toList()));

6
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListMembersCommand.java
View File

@ -22,7 +22,7 @@ import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.account.GroupDetailFactory.Factory;
import com.google.gerrit.server.account.GroupControl;
import com.google.gerrit.server.group.ListMembers;
import com.google.gerrit.server.ioutil.ColumnFormatter;
import com.google.gerrit.sshd.CommandMetaData;
@ -61,9 +61,9 @@ public class ListMembersCommand extends SshCommand {
@Inject
protected ListMembersCommandImpl(
GroupCache groupCache,
Factory groupDetailFactory,
GroupControl.Factory groupControlFactory,
AccountLoader.Factory accountLoaderFactory) {
super(groupCache, groupDetailFactory, accountLoaderFactory);
super(groupCache, groupControlFactory, accountLoaderFactory);
this.groupCache = groupCache;
}