Avoid unnecessary loading of members and subgroups (part 2)

The previous change I3630cb4c785 only avoided loading members or
subgroups twice when it was easy to do so. There are some more
situations which hadn't been covered so far as they required further
refactorings.

As there aren't any usages of GroupDescriptions#forAccountGroup anymore,
we can now get rid of that method and implementation of
GroupDescription.Internal. This allows us to add the methods
getMembers() and getSubgroups(), which we can use to directly access
the members and subgroups instead of loading them again.

Change-Id: I9cc818896321cf797786cc64fdbf1d3799675059

gerrit-common/src/main/java/com/google/gerrit/common/data/GroupDescription.java

@@ -15,8 +15,10 @@
 package com.google.gerrit.common.data;
 
 import com.google.gerrit.common.Nullable;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import java.sql.Timestamp;
+import java.util.Set;
 
 /** Group methods exposed by the GroupBackend. */
 public class GroupDescription {
@@ -56,6 +58,10 @@ public class GroupDescription {
     boolean isVisibleToAll();
 
     Timestamp getCreatedOn();
+
+    Set<Account.Id> getMembers();
+
+    Set<AccountGroup.UUID> getSubgroups();
   }
 
   private GroupDescription() {}

gerrit-common/src/main/java/com/google/gerrit/common/data/GroupDescriptions.java

@@ -1,77 +0,0 @@
-// Copyright (C) 2012 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.common.Nullable;
-import com.google.gerrit.common.PageLinks;
-import com.google.gerrit.reviewdb.client.AccountGroup;
-import java.sql.Timestamp;
-
-/** Utility class for building GroupDescription objects. */
-public class GroupDescriptions {
-
-  public static GroupDescription.Internal forAccountGroup(AccountGroup group) {
-    return new GroupDescription.Internal() {
-      @Override
-      public AccountGroup.UUID getGroupUUID() {
-        return group.getGroupUUID();
-      }
-
-      @Override
-      public String getName() {
-        return group.getName();
-      }
-
-      @Override
-      @Nullable
-      public String getEmailAddress() {
-        return null;
-      }
-
-      @Override
-      public String getUrl() {
-        return "#" + PageLinks.toGroup(getGroupUUID());
-      }
-
-      @Override
-      public AccountGroup.Id getId() {
-        return group.getId();
-      }
-
-      @Override
-      @Nullable
-      public String getDescription() {
-        return group.getDescription();
-      }
-
-      @Override
-      public AccountGroup.UUID getOwnerGroupUUID() {
-        return group.getOwnerGroupUUID();
-      }
-
-      @Override
-      public boolean isVisibleToAll() {
-        return group.isVisibleToAll();
-      }
-
-      @Override
-      public Timestamp getCreatedOn() {
-        return group.getCreatedOn();
-      }
-    };
-  }
-
-  private GroupDescriptions() {}
-}

gerrit-server/src/main/java/com/google/gerrit/server/group/InternalGroupDescription.java

@@ -16,9 +16,11 @@ package com.google.gerrit.server.group;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.PageLinks;
 import com.google.gerrit.common.data.GroupDescription;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import java.sql.Timestamp;
 
@@ -77,4 +79,14 @@ public class InternalGroupDescription implements GroupDescription.Internal {
   public Timestamp getCreatedOn() {
     return internalGroup.getCreatedOn();
   }
+
+  @Override
+  public ImmutableSet<Account.Id> getMembers() {
+    return internalGroup.getMembers();
+  }
+
+  @Override
+  public ImmutableSet<AccountGroup.UUID> getSubgroups() {
+    return internalGroup.getSubgroups();
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/group/ListSubgroups.java

@@ -23,12 +23,10 @@ import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.account.GroupControl;
-import com.google.gerrit.server.account.GroupIncludeCache;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
@@ -39,14 +37,11 @@ public class ListSubgroups implements RestReadView<GroupResource> {
   private static final Logger log = org.slf4j.LoggerFactory.getLogger(ListSubgroups.class);
 
   private final GroupControl.Factory controlFactory;
-  private final GroupIncludeCache groupIncludeCache;
   private final GroupJson json;
 
   @Inject
-  ListSubgroups(
+  ListSubgroups(GroupControl.Factory controlFactory, GroupJson json) {
-      GroupControl.Factory controlFactory, GroupIncludeCache groupIncludeCache, GroupJson json) {
     this.controlFactory = controlFactory;
-    this.groupIncludeCache = groupIncludeCache;
     this.json = json;
   }
 
@@ -57,9 +52,7 @@ public class ListSubgroups implements RestReadView<GroupResource> {
 
     boolean ownerOfParent = rsrc.getControl().isOwner();
     List<GroupInfo> included = new ArrayList<>();
-    Collection<AccountGroup.UUID> subgroupUuids =
+    for (AccountGroup.UUID subgroupUuid : group.getSubgroups()) {
-        groupIncludeCache.subgroupsOf(group.getGroupUUID());
-    for (AccountGroup.UUID subgroupUuid : subgroupUuids) {
       try {
         GroupControl i = controlFactory.controlFor(subgroupUuid);
         if (ownerOfParent || i.isVisible()) {

gerrit-server/src/main/java/com/google/gerrit/server/group/MembersCollection.java

@@ -15,7 +15,6 @@
 package com.google.gerrit.server.group;
 
 import com.google.gerrit.common.data.GroupDescription;
-import com.google.gerrit.common.errors.NoSuchGroupException;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.AcceptsCreate;
 import com.google.gerrit.extensions.restapi.AuthException;
@@ -25,8 +24,6 @@ import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestView;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
-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.AccountsCollection;
 import com.google.gerrit.server.group.AddMembers.PutMember;
@@ -43,8 +40,6 @@ public class MembersCollection
   private final DynamicMap<RestView<MemberResource>> views;
   private final Provider<ListMembers> list;
   private final AccountsCollection accounts;
-  private final Groups groups;
-  private final Provider<ReviewDb> db;
   private final AddMembers put;
 
   @Inject
@@ -52,14 +47,10 @@ public class MembersCollection
       DynamicMap<RestView<MemberResource>> views,
       Provider<ListMembers> list,
       AccountsCollection accounts,
-      Groups groups,
-      Provider<ReviewDb> db,
       AddMembers put) {
     this.views = views;
     this.list = list;
     this.accounts = accounts;
-    this.groups = groups;
-    this.db = db;
     this.put = put;
   }
 
@@ -82,14 +73,8 @@ public class MembersCollection
     throw new ResourceNotFoundException(id);
   }
 
-  private boolean isMember(GroupDescription.Internal group, IdentifiedUser user)
+  private static boolean isMember(GroupDescription.Internal group, IdentifiedUser user) {
-      throws OrmException, ResourceNotFoundException {
+    return group.getMembers().contains(user.getAccountId());
-    AccountGroup.UUID groupUuid = group.getGroupUUID();
-    try {
-      return groups.isMember(db.get(), groupUuid, user.getAccountId());
-    } catch (NoSuchGroupException e) {
-      throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
-    }
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/group/SubgroupsCollection.java

@@ -15,7 +15,6 @@
 package com.google.gerrit.server.group;
 
 import com.google.gerrit.common.data.GroupDescription;
-import com.google.gerrit.common.errors.NoSuchGroupException;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.AcceptsCreate;
 import com.google.gerrit.extensions.restapi.AuthException;
@@ -25,11 +24,8 @@ import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestView;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
-import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.group.AddSubgroups.PutSubgroup;
-import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
-import com.google.inject.Provider;
 import com.google.inject.Singleton;
 
 @Singleton
@@ -38,8 +34,6 @@ public class SubgroupsCollection
   private final DynamicMap<RestView<SubgroupResource>> views;
   private final ListSubgroups list;
   private final GroupsCollection groupsCollection;
-  private final Provider<ReviewDb> dbProvider;
-  private final Groups groups;
   private final AddSubgroups addSubgroups;
 
   @Inject
@@ -47,14 +41,10 @@ public class SubgroupsCollection
       DynamicMap<RestView<SubgroupResource>> views,
       ListSubgroups list,
       GroupsCollection groupsCollection,
-      Provider<ReviewDb> dbProvider,
-      Groups groups,
       AddSubgroups addSubgroups) {
     this.views = views;
     this.list = list;
     this.groupsCollection = groupsCollection;
-    this.dbProvider = dbProvider;
-    this.groups = groups;
     this.addSubgroups = addSubgroups;
   }
 
@@ -65,7 +55,7 @@ public class SubgroupsCollection
 
   @Override
   public SubgroupResource parse(GroupResource resource, IdString id)
-      throws MethodNotAllowedException, AuthException, ResourceNotFoundException, OrmException {
+      throws MethodNotAllowedException, AuthException, ResourceNotFoundException {
     GroupDescription.Internal parent =
         resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
 
@@ -77,14 +67,9 @@ public class SubgroupsCollection
     throw new ResourceNotFoundException(id);
   }
 
-  private boolean isSubgroup(GroupDescription.Internal parent, GroupDescription.Basic member)
+  private static boolean isSubgroup(
-      throws OrmException, ResourceNotFoundException {
+      GroupDescription.Internal parent, GroupDescription.Basic member) {
-    try {
+    return parent.getSubgroups().contains(member.getGroupUUID());
-      return groups.isSubgroup(dbProvider.get(), parent.getGroupUUID(), member.getGroupUUID());
-    } catch (NoSuchGroupException e) {
-      throw new ResourceNotFoundException(
-          String.format("Group %s not found", parent.getGroupUUID()));
-    }
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/mail/send/EmailArguments.java

@@ -24,13 +24,11 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.IdentifiedUser.GenericFactory;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.GroupBackend;
-import com.google.gerrit.server.account.GroupIncludeCache;
 import com.google.gerrit.server.config.AllProjectsName;
 import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.config.CanonicalWebUrl;
 import com.google.gerrit.server.config.SitePaths;
 import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.group.Groups;
 import com.google.gerrit.server.mail.EmailSettings;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.patch.PatchListCache;
@@ -53,8 +51,6 @@ public class EmailArguments {
   final ProjectCache projectCache;
   final PermissionBackend permissionBackend;
   final GroupBackend groupBackend;
-  final GroupIncludeCache groupIncludes;
-  final Groups groups;
   final AccountCache accountCache;
   final PatchListCache patchListCache;
   final ApprovalsUtil approvalsUtil;
@@ -86,7 +82,6 @@ public class EmailArguments {
       ProjectCache projectCache,
       PermissionBackend permissionBackend,
       GroupBackend groupBackend,
-      GroupIncludeCache groupIncludes,
       AccountCache accountCache,
       PatchListCache patchListCache,
       ApprovalsUtil approvalsUtil,
@@ -98,7 +93,6 @@ public class EmailArguments {
       AnonymousUser anonymousUser,
       @AnonymousCowardName String anonymousCowardName,
       GerritPersonIdentProvider gerritPersonIdentProvider,
-      Groups groups,
       @CanonicalWebUrl @Nullable Provider<String> urlProvider,
       AllProjectsName allProjectsName,
       ChangeQueryBuilder queryBuilder,
@@ -115,7 +109,6 @@ public class EmailArguments {
     this.projectCache = projectCache;
     this.permissionBackend = permissionBackend;
     this.groupBackend = groupBackend;
-    this.groupIncludes = groupIncludes;
     this.accountCache = accountCache;
     this.patchListCache = patchListCache;
     this.approvalsUtil = approvalsUtil;
@@ -127,7 +120,6 @@ public class EmailArguments {
     this.anonymousUser = anonymousUser;
     this.anonymousCowardName = anonymousCowardName;
     this.gerritPersonIdent = gerritPersonIdentProvider.get();
-    this.groups = groups;
     this.urlProvider = urlProvider;
     this.allProjectsName = allProjectsName;
     this.queryBuilder = queryBuilder;

gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java

@@ -17,13 +17,11 @@ package com.google.gerrit.server.mail.send;
 import com.google.common.base.Strings;
 import com.google.gerrit.common.data.GroupDescription;
 import com.google.gerrit.common.data.GroupReference;
-import com.google.gerrit.common.errors.NoSuchGroupException;
 import com.google.gerrit.index.query.Predicate;
 import com.google.gerrit.index.query.QueryParseException;
 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.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountState;
@@ -166,9 +164,7 @@ public class ProjectWatch {
     }
   }
 
-  private void deliverToMembers(Watchers.List matching, AccountGroup.UUID startUUID)
+  private void deliverToMembers(Watchers.List matching, AccountGroup.UUID startUUID) {
-      throws OrmException {
-    ReviewDb db = args.db.get();
     Set<AccountGroup.UUID> seen = new HashSet<>();
     List<AccountGroup.UUID> q = new ArrayList<>();
 
@@ -193,12 +189,8 @@ public class ProjectWatch {
       }
 
       GroupDescription.Internal ig = (GroupDescription.Internal) group;
-      try {
+      matching.accounts.addAll(ig.getMembers());
-        args.groups.getMembers(db, ig.getGroupUUID()).forEach(matching.accounts::add);
+      for (AccountGroup.UUID m : ig.getSubgroups()) {
-      } catch (NoSuchGroupException e) {
-        continue;
-      }
-      for (AccountGroup.UUID m : args.groupIncludes.subgroupsOf(uuid)) {
         if (seen.add(m)) {
           q.add(m);
         }