Merge changes I859f7999,I611ab4b0,Ia24f9671

* changes:
  Move DeleteChangeOp.unwrap to BatchUpdateReviewDb
  Move LimitedByteArrayOutputStream into server/ioutil
  Enumerate group members with GroupMembers in ChangeQueryBuilder

java/com/google/gerrit/server/account/GroupMembers.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.account;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 
 import com.google.common.collect.Sets;
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.Project;
@@ -52,6 +53,25 @@ public class GroupMembers {
     this.projectCache = projectCache;
   }
 
+  /**
+   * Recursively enumerate the members of the given group. Should not be used with the
+   * PROJECT_OWNERS magical group.
+   */
+  public Set<Account> listAccounts(AccountGroup.UUID groupUUID) throws IOException {
+    if (SystemGroupBackend.PROJECT_OWNERS.equals(groupUUID)) {
+      throw new IllegalStateException("listAccounts called with PROJECT_OWNERS argument");
+    }
+    try {
+      return listAccounts(groupUUID, null, new HashSet<AccountGroup.UUID>());
+    } catch (NoSuchProjectException e) {
+      throw new IllegalStateException(e);
+    }
+  }
+
+  /**
+   * Recursively enumerate the members of the given group. The project should be specified so the
+   * PROJECT_OWNERS magical group can be expanded.
+   */
   public Set<Account> listAccounts(AccountGroup.UUID groupUUID, Project.NameKey project)
       throws NoSuchProjectException, IOException {
     return listAccounts(groupUUID, project, new HashSet<AccountGroup.UUID>());
@@ -59,7 +79,7 @@ public class GroupMembers {
 
   private Set<Account> listAccounts(
       final AccountGroup.UUID groupUUID,
-      final Project.NameKey project,
+      @Nullable final Project.NameKey project,
       final Set<AccountGroup.UUID> seen)
       throws NoSuchProjectException, IOException {
     if (SystemGroupBackend.PROJECT_OWNERS.equals(groupUUID)) {
@@ -94,7 +114,7 @@ public class GroupMembers {
   }
 
   private Set<Account> getGroupMembers(
-      InternalGroup group, Project.NameKey project, Set<AccountGroup.UUID> seen)
+      InternalGroup group, @Nullable Project.NameKey project, Set<AccountGroup.UUID> seen)
       throws NoSuchProjectException, IOException {
     seen.add(group.getGroupUUID());
     GroupControl groupControl = groupControlFactory.controlFor(new InternalGroupDescription(group));

java/com/google/gerrit/server/change/ConsistencyChecker.java

@@ -52,6 +52,7 @@ import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.BatchUpdateOp;
+import com.google.gerrit.server.update.BatchUpdateReviewDb;
 import com.google.gerrit.server.update.ChangeContext;
 import com.google.gerrit.server.update.RepoContext;
 import com.google.gerrit.server.update.RetryHelper;
@@ -667,7 +668,7 @@ public class ConsistencyChecker {
     public boolean updateChange(ChangeContext ctx)
         throws OrmException, PatchSetInfoNotAvailableException {
       // Delete dangling key references.
-      ReviewDb db = DeleteChangeOp.unwrap(ctx.getDb());
+      ReviewDb db = BatchUpdateReviewDb.unwrap(ctx.getDb());
       accountPatchReviewStore.get().clearReviewed(psId);
       db.changeMessages().delete(db.changeMessages().byChange(psId.getParentKey()));
       db.patchSetApprovals().delete(db.patchSetApprovals().byPatchSet(psId));

java/com/google/gerrit/server/change/DeleteChangeOp.java

@@ -23,7 +23,6 @@ import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.StarredChangesUtil;
 import com.google.gerrit.server.project.NoSuchChangeException;
@@ -47,16 +46,6 @@ class DeleteChangeOp implements BatchUpdateOp {
     return cfg.getBoolean("change", "allowDrafts", true);
   }
 
-  static ReviewDb unwrap(ReviewDb db) {
-    // This is special. We want to delete exactly the rows that are present in
-    // the database, even when reading everything else from NoteDb, so we need
-    // to bypass the write-only wrapper.
-    if (db instanceof BatchUpdateReviewDb) {
-      db = ((BatchUpdateReviewDb) db).unsafeGetDelegate();
-    }
-    return ReviewDbUtil.unwrapDb(db);
-  }
-
   private final PatchSetUtil psUtil;
   private final StarredChangesUtil starredChangesUtil;
   private final DynamicItem<AccountPatchReviewStore> accountPatchReviewStore;
@@ -123,7 +112,11 @@ class DeleteChangeOp implements BatchUpdateOp {
   private void deleteChangeElementsFromDb(ChangeContext ctx, Change.Id id) throws OrmException {
     // Only delete from ReviewDb here; deletion from NoteDb is handled in
     // BatchUpdate.
-    ReviewDb db = unwrap(ctx.getDb());
+    //
+    // This is special. We want to delete exactly the rows that are present in
+    // the database, even when reading everything else from NoteDb, so we need
+    // to bypass the write-only wrapper.
+    ReviewDb db = BatchUpdateReviewDb.unwrap(ctx.getDb());
     db.patchComments().delete(db.patchComments().byChange(id));
     db.patchSetApprovals().delete(db.patchSetApprovals().byChange(id));
     db.patchSets().delete(db.patchSets().byChange(id));

java/com/google/gerrit/server/change/PreviewSubmit.java

@@ -29,11 +29,12 @@ import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.change.LimitedByteArrayOutputStream.LimitExceededException;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.MergeOp;
 import com.google.gerrit.server.git.MergeOpRepoManager;
 import com.google.gerrit.server.git.MergeOpRepoManager.OpenRepo;
+import com.google.gerrit.server.ioutil.LimitedByteArrayOutputStream;
+import com.google.gerrit.server.ioutil.LimitedByteArrayOutputStream.LimitExceededException;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.project.NoSuchProjectException;
 import com.google.gerrit.server.update.UpdateException;

java/com/google/gerrit/server/ioutil/BUILD

@@ -4,6 +4,7 @@ java_library(
     visibility = ["//visibility:public"],
     deps = [
         "//java/com/google/gerrit/reviewdb:client",
+        "//lib:guava",
         "//lib/jgit/org.eclipse.jgit.archive:jgit-archive",
         "//lib/jgit/org.eclipse.jgit:jgit",
     ],

java/com/google/gerrit/server/ioutil/LimitedByteArrayOutputStream.java

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package com.google.gerrit.server.change;
+package com.google.gerrit.server.ioutil;
 
 import static com.google.common.base.Preconditions.checkArgument;
 
@@ -20,7 +20,8 @@ import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
 
-class LimitedByteArrayOutputStream extends OutputStream {
+/** A stream that throws an exception if it consumes data beyond a configured byte count. */
+public class LimitedByteArrayOutputStream extends OutputStream {
 
   private final int maxSize;
   private final ByteArrayOutputStream buffer;
@@ -32,7 +33,7 @@ class LimitedByteArrayOutputStream extends OutputStream {
    * @param max the maximum size in bytes which may be stored.
    * @param initial the initial size. It must be smaller than the max size.
    */
-  LimitedByteArrayOutputStream(int max, int initial) {
+  public LimitedByteArrayOutputStream(int max, int initial) {
     checkArgument(initial <= max);
     maxSize = max;
     buffer = new ByteArrayOutputStream(initial);
@@ -61,7 +62,7 @@ class LimitedByteArrayOutputStream extends OutputStream {
     return buffer.toByteArray();
   }
 
-  static class LimitExceededException extends IOException {
+  public static class LimitExceededException extends IOException {
     private static final long serialVersionUID = 1L;
   }
 }

java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java

@@ -51,6 +51,7 @@ import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.account.GroupBackend;
 import com.google.gerrit.server.account.GroupBackends;
+import com.google.gerrit.server.account.GroupMembers;
 import com.google.gerrit.server.account.VersionedAccountDestinations;
 import com.google.gerrit.server.account.VersionedAccountQueries;
 import com.google.gerrit.server.change.ChangeTriplet;
@@ -59,7 +60,6 @@ import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.strategy.SubmitDryRun;
-import com.google.gerrit.server.group.ListMembers;
 import com.google.gerrit.server.index.change.ChangeField;
 import com.google.gerrit.server.index.change.ChangeIndex;
 import com.google.gerrit.server.index.change.ChangeIndexCollection;
@@ -211,11 +211,11 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
     final ProjectCache projectCache;
     final Provider<InternalChangeQuery> queryProvider;
     final ChildProjects childProjects;
-    final Provider<ListMembers> listMembers;
     final Provider<ReviewDb> db;
     final StarredChangesUtil starredChangesUtil;
     final SubmitDryRun submitDryRun;
     final boolean allowsDrafts;
+    final GroupMembers groupMembers;
 
     private final Provider<CurrentUser> self;
 
@@ -245,11 +245,11 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
         SubmitDryRun submitDryRun,
         ConflictsCache conflictsCache,
         IndexConfig indexConfig,
-        Provider<ListMembers> listMembers,
         StarredChangesUtil starredChangesUtil,
         AccountCache accountCache,
         @GerritServerConfig Config cfg,
-        NotesMigration notesMigration) {
+        NotesMigration notesMigration,
+        GroupMembers groupMembers) {
       this(
           db,
           queryProvider,
@@ -274,11 +274,11 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
           conflictsCache,
           indexes != null ? indexes.getSearchIndex() : null,
           indexConfig,
-          listMembers,
           starredChangesUtil,
           accountCache,
           cfg == null ? true : cfg.getBoolean("change", "allowDrafts", true),
-          notesMigration);
+          notesMigration,
+          groupMembers);
     }
 
     private Arguments(
@@ -305,11 +305,11 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
         ConflictsCache conflictsCache,
         ChangeIndex index,
         IndexConfig indexConfig,
-        Provider<ListMembers> listMembers,
         StarredChangesUtil starredChangesUtil,
         AccountCache accountCache,
         boolean allowsDrafts,
-        NotesMigration notesMigration) {
+        NotesMigration notesMigration,
+        GroupMembers groupMembers) {
       this.db = db;
       this.queryProvider = queryProvider;
       this.rewriter = rewriter;
@@ -332,12 +332,12 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
       this.conflictsCache = conflictsCache;
       this.index = index;
       this.indexConfig = indexConfig;
-      this.listMembers = listMembers;
       this.starredChangesUtil = starredChangesUtil;
       this.accountCache = accountCache;
       this.allowsDrafts = allowsDrafts;
       this.hasOperands = hasOperands;
       this.notesMigration = notesMigration;
+      this.groupMembers = groupMembers;
     }
 
     Arguments asUser(CurrentUser otherUser) {
@@ -365,11 +365,11 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
           conflictsCache,
           index,
           indexConfig,
-          listMembers,
           starredChangesUtil,
           accountCache,
           allowsDrafts,
-          notesMigration);
+          notesMigration,
+          groupMembers);
     }
 
     Arguments asUser(Account.Id otherId) {
@@ -775,12 +775,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
     // expand a group predicate into multiple user predicates
     if (group != null) {
       Set<Account.Id> allMembers =
-          args.listMembers
-              .get()
-              .getTransitiveMembers(group)
-              .stream()
-              .map(a -> new Account.Id(a._accountId))
-              .collect(toSet());
+          args.groupMembers.listAccounts(group).stream().map(a -> a.getId()).collect(toSet());
+
       int maxTerms = args.indexConfig.maxTerms();
       if (allMembers.size() > maxTerms) {
         // limit the number of query terms otherwise Gerrit will barf
@@ -906,7 +902,6 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
     }
 
     // If its not an account, maybe its a group?
-    //
     Collection<GroupReference> suggestions = args.groupBackend.suggest(who, null);
     if (!suggestions.isEmpty()) {
       HashSet<AccountGroup.UUID> ids = new HashSet<>();

java/com/google/gerrit/server/update/BatchUpdateReviewDb.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.update;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.server.ChangeAccess;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.reviewdb.server.ReviewDbWrapper;
 import com.google.gwtorm.server.AtomicUpdate;
 
@@ -28,6 +29,14 @@ public class BatchUpdateReviewDb extends ReviewDbWrapper {
     changesWrapper = new BatchUpdateChanges(delegate.changes());
   }
 
+  /** @return the underlying delegate. Supports BatchUpdateReviewDb too. */
+  public static ReviewDb unwrap(ReviewDb db) {
+    if (db instanceof BatchUpdateReviewDb) {
+      db = ((BatchUpdateReviewDb) db).unsafeGetDelegate();
+    }
+    return ReviewDbUtil.unwrapDb(db);
+  }
+
   public ReviewDb unsafeGetDelegate() {
     return delegate;
   }