From 8c8c4e506e1e6ffbc95711a0b43b3510728e4fa5 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Thu, 27 Jul 2017 11:42:09 +0200 Subject: [PATCH] Remove access of groups related db tables from GroupIncludeCacheImpl Change-Id: I5672d0a82e38e89acd950bf46ab006c5dfb42cbf --- .../server/account/GroupIncludeCacheImpl.java | 57 ++++++++----------- .../google/gerrit/server/group/Groups.java | 27 ++++++++- 2 files changed, 49 insertions(+), 35 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java index e1fc1011d6..a70abbf501 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java @@ -14,13 +14,15 @@ 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.client.AccountGroupById; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.cache.CacheModule; +import com.google.gerrit.server.group.Groups; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; @@ -30,9 +32,7 @@ import com.google.inject.TypeLiteral; import com.google.inject.name.Named; import java.util.Collection; import java.util.Collections; -import java.util.HashSet; -import java.util.List; -import java.util.Set; +import java.util.Objects; import java.util.concurrent.ExecutionException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -137,25 +137,18 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache { static class SubgroupsLoader extends CacheLoader> { private final SchemaFactory schema; + private final Groups groups; @Inject - SubgroupsLoader(SchemaFactory sf) { + SubgroupsLoader(SchemaFactory sf, Groups groups) { schema = sf; + this.groups = groups; } @Override public ImmutableList load(AccountGroup.UUID key) throws OrmException { try (ReviewDb db = schema.open()) { - List group = db.accountGroups().byUUID(key).toList(); - if (group.size() != 1) { - return ImmutableList.of(); - } - - Set ids = new HashSet<>(); - for (AccountGroupById agi : db.accountGroupById().byGroup(group.get(0).getId())) { - ids.add(agi.getIncludeUUID()); - } - return ImmutableList.copyOf(ids); + return groups.getIncludes(db, key).collect(toImmutableList()); } } } @@ -163,47 +156,43 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache { static class ParentGroupsLoader extends CacheLoader> { private final SchemaFactory schema; + private final GroupCache groupCache; + private final Groups groups; @Inject - ParentGroupsLoader(SchemaFactory sf) { + ParentGroupsLoader(SchemaFactory sf, GroupCache groupCache, Groups groups) { schema = sf; + this.groupCache = groupCache; + this.groups = groups; } @Override public ImmutableList load(AccountGroup.UUID key) throws OrmException { try (ReviewDb db = schema.open()) { - Set ids = new HashSet<>(); - for (AccountGroupById agi : db.accountGroupById().byIncludeUUID(key)) { - ids.add(agi.getGroupId()); - } - - Set groupArray = new HashSet<>(); - for (AccountGroup g : db.accountGroups().get(ids)) { - groupArray.add(g.getGroupUUID()); - } - return ImmutableList.copyOf(groupArray); + return groups + .getParentGroups(db, key) + .map(groupCache::get) + .map(AccountGroup::getGroupUUID) + .filter(Objects::nonNull) + .collect(toImmutableList()); } } } static class AllExternalLoader extends CacheLoader> { private final SchemaFactory schema; + private final Groups groups; @Inject - AllExternalLoader(SchemaFactory sf) { + AllExternalLoader(SchemaFactory sf, Groups groups) { schema = sf; + this.groups = groups; } @Override public ImmutableList load(String key) throws Exception { try (ReviewDb db = schema.open()) { - Set ids = new HashSet<>(); - for (AccountGroupById agi : db.accountGroupById().all()) { - if (!AccountGroup.isInternalGroup(agi.getIncludeUUID())) { - ids.add(agi.getIncludeUUID()); - } - } - return ImmutableList.copyOf(ids); + return groups.getExternalGroups(db).collect(toImmutableList()); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java index a28915320f..4ad95b2337 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java @@ -80,10 +80,21 @@ public class Groups { return Streams.stream(accountGroupMembers).map(AccountGroupMember::getAccountId); } + public Stream getIncludes(ReviewDb db, AccountGroup.UUID groupUuid) + throws OrmException { + Optional foundGroup = get(db, groupUuid); + if (!foundGroup.isPresent()) { + return Stream.empty(); + } + + AccountGroup group = foundGroup.get(); + return getIncludes(db, group.getId()); + } + public Stream getIncludes(ReviewDb db, AccountGroup.Id groupId) throws OrmException { ResultSet accountGroupByIds = db.accountGroupById().byGroup(groupId); - return Streams.stream(accountGroupByIds).map(AccountGroupById::getIncludeUUID); + return Streams.stream(accountGroupByIds).map(AccountGroupById::getIncludeUUID).distinct(); } public Stream getGroupsWithMember(ReviewDb db, Account.Id accountId) @@ -92,4 +103,18 @@ public class Groups { db.accountGroupMembers().byAccount(accountId); return Streams.stream(accountGroupMembers).map(AccountGroupMember::getAccountGroupId); } + + public Stream getParentGroups(ReviewDb db, AccountGroup.UUID includedGroupUuid) + throws OrmException { + ResultSet accountGroupByIds = + db.accountGroupById().byIncludeUUID(includedGroupUuid); + return Streams.stream(accountGroupByIds).map(AccountGroupById::getGroupId); + } + + public Stream getExternalGroups(ReviewDb db) throws OrmException { + return Streams.stream(db.accountGroupById().all()) + .map(AccountGroupById::getIncludeUUID) + .distinct() + .filter(groupUuid -> !AccountGroup.isInternalGroup(groupUuid)); + } }