Add staleness checker for groups in NoteDb

This commit adds a new field to the group index to store the SHA1 of the
groups ref that the index document belongs to. With that, staleness can
be detected by comparing the value of the field to the current HEAD SHA1
on the groups ref. This mechanism is similar to what was used for
changes and accounts.

This commit also adds a staleness checker for groups as well as a test.

Change-Id: I113546ecd27079c03bbd1db728a22483db0a7ee2

java/com/google/gerrit/server/group/InternalGroup.java

@@ -21,6 +21,7 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import java.io.Serializable;
 import java.sql.Timestamp;
+import org.eclipse.jgit.lib.ObjectId;
 
 @AutoValue
 public abstract class InternalGroup implements Serializable {
@@ -30,6 +31,14 @@ public abstract class InternalGroup implements Serializable {
       AccountGroup accountGroup,
       ImmutableSet<Account.Id> members,
       ImmutableSet<AccountGroup.UUID> subgroups) {
+    return create(accountGroup, members, subgroups, null);
+  }
+
+  public static InternalGroup create(
+      AccountGroup accountGroup,
+      ImmutableSet<Account.Id> members,
+      ImmutableSet<AccountGroup.UUID> subgroups,
+      ObjectId refState) {
     return builder()
         .setId(accountGroup.getId())
         .setNameKey(accountGroup.getNameKey())
@@ -40,6 +49,7 @@ public abstract class InternalGroup implements Serializable {
         .setCreatedOn(accountGroup.getCreatedOn())
         .setMembers(members)
         .setSubgroups(subgroups)
+        .setRefState(refState)
         .build();
   }
 
@@ -66,6 +76,11 @@ public abstract class InternalGroup implements Serializable {
 
   public abstract ImmutableSet<AccountGroup.UUID> getSubgroups();
 
+  @Nullable
+  public abstract ObjectId getRefState();
+
+  public abstract Builder toBuilder();
+
   public static Builder builder() {
     return new AutoValue_InternalGroup.Builder();
   }
@@ -90,6 +105,8 @@ public abstract class InternalGroup implements Serializable {
 
     public abstract Builder setSubgroups(ImmutableSet<AccountGroup.UUID> subgroups);
 
+    public abstract Builder setRefState(ObjectId refState);
+
     public abstract InternalGroup build();
   }
 }

java/com/google/gerrit/server/group/db/GroupConfig.java

@@ -25,6 +25,7 @@ import com.google.common.collect.Streams;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.git.VersionedMetaData;
 import com.google.gerrit.server.group.InternalGroup;
 import com.google.gwtorm.server.OrmDuplicateKeyException;
@@ -40,6 +41,7 @@ import java.util.stream.Stream;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -123,7 +125,9 @@ public class GroupConfig extends VersionedMetaData {
       Config config = readConfig(GROUP_CONFIG_FILE);
       ImmutableSet<Account.Id> members = readMembers();
       ImmutableSet<AccountGroup.UUID> subgroups = readSubgroups();
-      loadedGroup = Optional.of(createFrom(groupUuid, config, members, subgroups, createdOn));
+      loadedGroup =
+          Optional.of(
+              createFrom(groupUuid, config, members, subgroups, createdOn, revision.toObjectId()));
     }
 
     isLoaded = true;
@@ -134,7 +138,8 @@ public class GroupConfig extends VersionedMetaData {
       Config config,
       ImmutableSet<Account.Id> members,
       ImmutableSet<AccountGroup.UUID> subgroups,
-      Timestamp createdOn) {
+      Timestamp createdOn,
+      ObjectId refState) {
     InternalGroup.Builder group = InternalGroup.builder();
     group.setGroupUUID(groupUuid);
     Arrays.stream(GroupConfigEntry.values())
@@ -142,9 +147,17 @@ public class GroupConfig extends VersionedMetaData {
     group.setMembers(members);
     group.setSubgroups(subgroups);
     group.setCreatedOn(createdOn);
+    group.setRefState(refState);
     return group.build();
   }
 
+  @Override
+  public RevCommit commit(MetaDataUpdate update) throws IOException {
+    RevCommit c = super.commit(update);
+    loadedGroup = Optional.of(loadedGroup.get().toBuilder().setRefState(c.toObjectId()).build());
+    return c;
+  }
+
   @Override
   protected boolean onSave(CommitBuilder commit) throws IOException, ConfigInvalidException {
     checkLoaded();
@@ -184,7 +197,8 @@ public class GroupConfig extends VersionedMetaData {
                 config,
                 updatedMembers.orElse(originalMembers),
                 updatedSubgroups.orElse(originalSubgroups),
-                createdOn));
+                createdOn,
+                null));
     groupCreation = Optional.empty();
 
     return true;

java/com/google/gerrit/server/group/db/GroupsUpdate.java

@@ -48,6 +48,7 @@ import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.git.RenameGroupOp;
 import com.google.gerrit.server.group.InternalGroup;
+import com.google.gerrit.server.group.InternalGroup.Builder;
 import com.google.gerrit.server.update.RefUpdateUtil;
 import com.google.gwtorm.server.OrmDuplicateKeyException;
 import com.google.gwtorm.server.OrmException;
@@ -62,6 +63,7 @@ import java.util.concurrent.TimeUnit;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -264,7 +266,10 @@ public class GroupsUpdate {
     AccountGroup group = createAccountGroup(groupCreation);
     UpdateResult updateResult = updateGroupInReviewDb(db, group, groupUpdate);
     return InternalGroup.create(
-        group, updateResult.getModifiedMembers(), updateResult.getModifiedSubgroups());
+        group,
+        updateResult.getModifiedMembers(),
+        updateResult.getModifiedSubgroups(),
+        updateResult.getRefState());
   }
 
   public static AccountGroup createAccountGroup(
@@ -504,7 +509,8 @@ public class GroupsUpdate {
             .setGroupId(updatedGroup.getId())
             .setGroupName(updatedGroup.getNameKey())
             .setModifiedMembers(modifiedMembers)
-            .setModifiedSubgroups(modifiedSubgroups);
+            .setModifiedSubgroups(modifiedSubgroups)
+            .setRefState(updatedGroup.getRefState());
     if (!Objects.equals(originalGroup.getNameKey(), updatedGroup.getNameKey())) {
       resultBuilder.setPreviousGroupName(originalGroup.getNameKey());
     }
@@ -637,6 +643,9 @@ public class GroupsUpdate {
 
     abstract ImmutableSet<AccountGroup.UUID> getModifiedSubgroups();
 
+    @Nullable
+    public abstract ObjectId getRefState();
+
     static Builder builder() {
       return new AutoValue_GroupsUpdate_UpdateResult.Builder();
     }
@@ -655,6 +664,8 @@ public class GroupsUpdate {
 
       abstract Builder setModifiedSubgroups(Set<AccountGroup.UUID> modifiedSubgroups);
 
+      public abstract Builder setRefState(ObjectId refState);
+
       abstract UpdateResult build();
     }
   }

java/com/google/gerrit/server/index/IndexUtils.java

@@ -18,6 +18,7 @@ import static com.google.gerrit.server.index.change.ChangeField.CHANGE;
 import static com.google.gerrit.server.index.change.ChangeField.LEGACY_ID;
 import static com.google.gerrit.server.index.change.ChangeField.PROJECT;
 
+import com.google.common.base.Function;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
@@ -30,12 +31,27 @@ import com.google.gerrit.server.index.project.ProjectField;
 import com.google.gerrit.server.query.change.SingleGroupUser;
 import java.io.IOException;
 import java.util.Set;
+import java.util.concurrent.ExecutionException;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 
 public final class IndexUtils {
   public static final ImmutableMap<String, String> CUSTOM_CHAR_MAPPING =
       ImmutableMap.of("_", " ", ".", " ");
 
+  public static final Function<Exception, IOException> MAPPER =
+      new Function<Exception, IOException>() {
+        @Override
+        public IOException apply(Exception in) {
+          if (in instanceof IOException) {
+            return (IOException) in;
+          } else if (in instanceof ExecutionException && in.getCause() instanceof IOException) {
+            return (IOException) in.getCause();
+          } else {
+            return new IOException(in);
+          }
+        }
+      };
+
   public static void setReady(SitePaths sitePaths, String name, int version, boolean ready)
       throws IOException {
     try {

java/com/google/gerrit/server/index/change/ChangeIndexer.java

@@ -17,7 +17,6 @@ package com.google.gerrit.server.index.change;
 import static com.google.gerrit.server.extensions.events.EventUtil.logEventListenerError;
 import static com.google.gerrit.server.git.QueueProvider.QueueType.BATCH;
 
-import com.google.common.base.Function;
 import com.google.common.util.concurrent.Atomics;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
@@ -31,6 +30,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.index.IndexExecutor;
+import com.google.gerrit.server.index.IndexUtils;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -50,7 +50,6 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 import java.util.concurrent.atomic.AtomicReference;
 import org.eclipse.jgit.lib.Config;
@@ -79,23 +78,9 @@ public class ChangeIndexer {
     // ExecutionException, so we can reuse the same mapper as for a single
     // future. Assume the actual contents of the exception are not useful to
     // callers. All exceptions are already logged by IndexTask.
-    return Futures.makeChecked(Futures.allAsList(futures), MAPPER);
+    return Futures.makeChecked(Futures.allAsList(futures), IndexUtils.MAPPER);
   }
 
-  private static final Function<Exception, IOException> MAPPER =
-      new Function<Exception, IOException>() {
-        @Override
-        public IOException apply(Exception in) {
-          if (in instanceof IOException) {
-            return (IOException) in;
-          } else if (in instanceof ExecutionException && in.getCause() instanceof IOException) {
-            return (IOException) in.getCause();
-          } else {
-            return new IOException(in);
-          }
-        }
-      };
-
   private final ChangeIndexCollection indexes;
   private final ChangeIndex index;
   private final SchemaFactory<ReviewDb> schemaFactory;
@@ -335,7 +320,8 @@ public class ChangeIndexer {
   @SuppressWarnings("deprecation")
   private static <T> com.google.common.util.concurrent.CheckedFuture<T, IOException> submit(
       Callable<T> task, ListeningExecutorService executor) {
-    return Futures.makeChecked(Futures.nonCancellationPropagating(executor.submit(task)), MAPPER);
+    return Futures.makeChecked(
+        Futures.nonCancellationPropagating(executor.submit(task)), IndexUtils.MAPPER);
   }
 
   private abstract class AbstractIndexTask<T> implements Callable<T> {

java/com/google/gerrit/server/index/group/GroupField.java

@@ -19,14 +19,18 @@ import static com.google.gerrit.index.FieldDef.exact;
 import static com.google.gerrit.index.FieldDef.fullText;
 import static com.google.gerrit.index.FieldDef.integer;
 import static com.google.gerrit.index.FieldDef.prefix;
+import static com.google.gerrit.index.FieldDef.storedOnly;
 import static com.google.gerrit.index.FieldDef.timestamp;
 
+import com.google.common.base.MoreObjects;
 import com.google.gerrit.index.FieldDef;
 import com.google.gerrit.index.SchemaUtil;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.group.InternalGroup;
 import java.sql.Timestamp;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectId;
 
 /** Secondary index schemas for groups. */
 public class GroupField {
@@ -72,4 +76,14 @@ public class GroupField {
           .buildRepeatable(
               g ->
                   g.getSubgroups().stream().map(AccountGroup.UUID::get).collect(toImmutableList()));
+
+  /** ObjectId of HEAD:refs/groups/<UUID>. */
+  public static final FieldDef<InternalGroup, byte[]> REF_STATE =
+      storedOnly("ref_state")
+          .build(
+              g -> {
+                byte[] a = new byte[Constants.OBJECT_ID_STRING_LENGTH];
+                MoreObjects.firstNonNull(g.getRefState(), ObjectId.zeroId()).copyTo(a, 0);
+                return a;
+              });
 }

java/com/google/gerrit/server/index/group/GroupIndexerImpl.java

@@ -14,20 +14,30 @@
 
 package com.google.gerrit.server.index.group;
 
+import static com.google.gerrit.server.git.QueueProvider.QueueType.BATCH;
+
 import com.google.common.collect.ImmutableSet;
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.events.GroupIndexedListener;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.index.Index;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.account.GroupCache;
+import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.group.InternalGroup;
+import com.google.gerrit.server.index.IndexExecutor;
+import com.google.gerrit.server.index.IndexUtils;
 import com.google.inject.assistedinject.Assisted;
 import com.google.inject.assistedinject.AssistedInject;
 import java.io.IOException;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Optional;
+import java.util.concurrent.Callable;
+import java.util.concurrent.Future;
+import org.eclipse.jgit.lib.Config;
 
 public class GroupIndexerImpl implements GroupIndexer {
   public interface Factory {
@@ -39,16 +49,25 @@ public class GroupIndexerImpl implements GroupIndexer {
   private final GroupCache groupCache;
   private final DynamicSet<GroupIndexedListener> indexedListener;
   private final GroupIndexCollection indexes;
+  private final StalenessChecker stalenessChecker;
+  private final boolean autoReindexIfStale;
   private final GroupIndex index;
+  private final ListeningExecutorService batchExecutor;
 
   @AssistedInject
   GroupIndexerImpl(
       GroupCache groupCache,
       DynamicSet<GroupIndexedListener> indexedListener,
+      StalenessChecker stalenessChecker,
+      @GerritServerConfig Config config,
+      @IndexExecutor(BATCH) ListeningExecutorService batchExecutor,
       @Assisted GroupIndexCollection indexes) {
     this.groupCache = groupCache;
     this.indexedListener = indexedListener;
     this.indexes = indexes;
+    this.stalenessChecker = stalenessChecker;
+    this.autoReindexIfStale = autoReindexIfStale(config);
+    this.batchExecutor = batchExecutor;
     this.index = null;
   }
 
@@ -56,10 +75,16 @@ public class GroupIndexerImpl implements GroupIndexer {
   GroupIndexerImpl(
       GroupCache groupCache,
       DynamicSet<GroupIndexedListener> indexedListener,
+      StalenessChecker stalenessChecker,
+      @GerritServerConfig Config config,
+      @IndexExecutor(BATCH) ListeningExecutorService batchExecutor,
       @Assisted GroupIndex index) {
     this.groupCache = groupCache;
     this.indexedListener = indexedListener;
     this.indexes = null;
+    this.stalenessChecker = stalenessChecker;
+    this.autoReindexIfStale = autoReindexIfStale(config);
+    this.batchExecutor = batchExecutor;
     this.index = index;
   }
 
@@ -74,6 +99,44 @@ public class GroupIndexerImpl implements GroupIndexer {
       }
     }
     fireGroupIndexedEvent(uuid.get());
+    autoReindexIfStale(uuid);
+  }
+
+  private static boolean autoReindexIfStale(Config cfg) {
+    return cfg.getBoolean("index", null, "autoReindexIfStale", true);
+  }
+
+  private void autoReindexIfStale(AccountGroup.UUID uuid) {
+    if (autoReindexIfStale) {
+      // Don't retry indefinitely; if this fails the change will be stale.
+      @SuppressWarnings("unused")
+      Future<?> possiblyIgnoredError = reindexIfStale(uuid);
+    }
+  }
+
+  /**
+   * Asynchronously check if a group is stale, and reindex if it is.
+   *
+   * <p>Always run on the batch executor, even if this indexer instance is configured to use a
+   * different executor.
+   *
+   * @param uuid the unique identified of the group.
+   * @return future for reindexing the group; returns true if the change was stale.
+   */
+  @SuppressWarnings("deprecation")
+  public com.google.common.util.concurrent.CheckedFuture<Boolean, IOException> reindexIfStale(
+      AccountGroup.UUID uuid) {
+    Callable<Boolean> task =
+        () -> {
+          if (stalenessChecker.isStale(uuid)) {
+            index(uuid);
+            return true;
+          }
+          return false;
+        };
+
+    return Futures.makeChecked(
+        Futures.nonCancellationPropagating(batchExecutor.submit(task)), IndexUtils.MAPPER);
   }
 
   private void fireGroupIndexedEvent(String uuid) {

java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java

@@ -34,8 +34,11 @@ public class GroupSchemaDefinitions extends SchemaDefinitions<InternalGroup> {
 
   @Deprecated static final Schema<InternalGroup> V3 = schema(V2, GroupField.CREATED_ON);
 
+  @Deprecated
   static final Schema<InternalGroup> V4 = schema(V3, GroupField.MEMBER, GroupField.SUBGROUP);
 
+  static final Schema<InternalGroup> V5 = schema(V4, GroupField.REF_STATE);
+
   public static final GroupSchemaDefinitions INSTANCE = new GroupSchemaDefinitions();
 
   private GroupSchemaDefinitions() {

java/com/google/gerrit/server/index/group/IndexedGroupQuery.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.index.group;
 
 import com.google.gerrit.index.Index;
+import com.google.gerrit.index.IndexConfig;
 import com.google.gerrit.index.IndexedQuery;
 import com.google.gerrit.index.QueryOptions;
 import com.google.gerrit.index.query.DataSource;
@@ -22,10 +23,22 @@ import com.google.gerrit.index.query.Predicate;
 import com.google.gerrit.index.query.QueryParseException;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.group.InternalGroup;
+import java.util.HashSet;
+import java.util.Set;
 
 public class IndexedGroupQuery extends IndexedQuery<AccountGroup.UUID, InternalGroup>
     implements DataSource<InternalGroup> {
 
+  public static QueryOptions createOptions(
+      IndexConfig config, int start, int limit, Set<String> fields) {
+    // Always include GroupField.UUID since it is needed to load the group from NoteDb.
+    if (!fields.contains(GroupField.UUID.getName())) {
+      fields = new HashSet<>(fields);
+      fields.add(GroupField.UUID.getName());
+    }
+    return QueryOptions.create(config, start, limit, fields);
+  }
+
   public IndexedGroupQuery(
       Index<AccountGroup.UUID, InternalGroup> index,
       Predicate<InternalGroup> pred,

java/com/google/gerrit/server/index/group/StalenessChecker.java

@@ -0,0 +1,94 @@
+// Copyright (C) 2017 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.index.group;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.index.IndexConfig;
+import com.google.gerrit.index.query.FieldBundle;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.util.Optional;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+
+/**
+ * Checks if documents in the group index are stale.
+ *
+ * <p>An index document is considered stale if the stored SHA1 differs from the HEAD SHA1 of the
+ * groups branch.
+ *
+ * <p>Note: This only applies to NoteDb.
+ */
+@Singleton
+public class StalenessChecker {
+  public static final ImmutableSet<String> FIELDS =
+      ImmutableSet.of(GroupField.UUID.getName(), GroupField.REF_STATE.getName());
+
+  private final GroupIndexCollection indexes;
+  private final GitRepositoryManager repoManager;
+  private final IndexConfig indexConfig;
+  private final AllUsersName allUsers;
+  private final Config config;
+
+  @Inject
+  StalenessChecker(
+      GroupIndexCollection indexes,
+      GitRepositoryManager repoManager,
+      IndexConfig indexConfig,
+      AllUsersName allUsers,
+      @GerritServerConfig Config config) {
+    this.indexes = indexes;
+    this.repoManager = repoManager;
+    this.indexConfig = indexConfig;
+    this.allUsers = allUsers;
+    this.config = config;
+  }
+
+  public boolean isStale(AccountGroup.UUID id) throws IOException, OrmException {
+    if (!config.getBoolean("user", "readGroupsFromNoteDb", false)) {
+      return false; // This class only treats staleness for groups in NoteDb.
+    }
+
+    GroupIndex i = indexes.getSearchIndex();
+    if (i == null) {
+      return false; // No index; caller couldn't do anything if it is stale.
+    }
+    if (!i.getSchema().hasField(GroupField.REF_STATE)) {
+      return false; // Index version not new enough for this check.
+    }
+
+    Optional<FieldBundle> result =
+        i.getRaw(id, IndexedGroupQuery.createOptions(indexConfig, 0, 1, FIELDS));
+    if (!result.isPresent()) {
+      // No document in the index
+      return true;
+    }
+
+    try (Repository repo = repoManager.openRepository(allUsers)) {
+      Ref ref = repo.exactRef(RefNames.refsGroups(id));
+      ObjectId head = ref == null ? ObjectId.zeroId() : ref.getObjectId();
+      return !head.equals(ObjectId.fromString(result.get().getValue(GroupField.REF_STATE), 0));
+    }
+  }
+}

java/com/google/gerrit/server/query/group/GroupPredicates.java

@@ -46,6 +46,7 @@ public class GroupPredicates {
     return new GroupPredicate(GroupField.NAME, GroupQueryBuilder.FIELD_NAME, name);
   }
 
+  @SuppressWarnings("deprecation")
   public static Predicate<InternalGroup> owner(AccountGroup.UUID ownerUuid) {
     return new GroupPredicate(
         GroupField.OWNER_UUID, GroupQueryBuilder.FIELD_OWNER, ownerUuid.get());

java/com/google/gerrit/server/query/group/GroupQueryBuilder.java

@@ -49,7 +49,7 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
   public static final String FIELD_DESCRIPTION = "description";
   public static final String FIELD_INNAME = "inname";
   public static final String FIELD_NAME = "name";
-  public static final String FIELD_OWNER = "owner";
+  @Deprecated public static final String FIELD_OWNER = "owner";
   public static final String FIELD_LIMIT = "limit";
 
   private static final QueryBuilder.Definition<InternalGroup, GroupQueryBuilder> mydef =
@@ -109,12 +109,6 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
     return GroupPredicates.name(name);
   }
 
-  @Operator
-  public Predicate<InternalGroup> owner(String owner) throws QueryParseException {
-    AccountGroup.UUID groupUuid = parseGroup(owner);
-    return GroupPredicates.owner(groupUuid);
-  }
-
   @Operator
   public Predicate<InternalGroup> is(String value) throws QueryParseException {
     if ("visibletoall".equalsIgnoreCase(value)) {
@@ -133,11 +127,6 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
     if (!Strings.isNullOrEmpty(query)) {
       preds.add(description(query));
     }
-    try {
-      preds.add(owner(query));
-    } catch (QueryParseException e) {
-      // Skip.
-    }
     return Predicate.or(preds);
   }
 

javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java

@@ -90,7 +90,7 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
     InternalGroup reviewDbGroup = groups.getGroup(db, new AccountGroup.UUID(createdGroup.id)).get();
     deleteGroupRefs(reviewDbGroup);
 
-    assertThat(rebuild(reviewDbGroup)).isEqualTo(roundToSecond(reviewDbGroup));
+    assertThat(removeRefState(rebuild(reviewDbGroup))).isEqualTo(roundToSecond(reviewDbGroup));
   }
 
   @Test
@@ -109,7 +109,7 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
     deleteGroupRefs(reviewDbGroup);
 
     InternalGroup noteDbGroup = rebuild(reviewDbGroup);
-    assertThat(noteDbGroup).isEqualTo(roundToSecond(reviewDbGroup));
+    assertThat(removeRefState(noteDbGroup)).isEqualTo(roundToSecond(reviewDbGroup));
 
     ImmutableList<CommitInfo> log = log(group1);
     assertThat(log).hasSize(4);
@@ -147,6 +147,10 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
     assertThat(log.get(3)).committer().hasSameDateAs(log.get(3).author);
   }
 
+  private static InternalGroup removeRefState(InternalGroup group) throws Exception {
+    return group.toBuilder().setRefState(null).build();
+  }
+
   private void deleteGroupRefs(InternalGroup group) throws Exception {
     try (Repository repo = repoManager.openRepository(allUsers)) {
       String refName = RefNames.refsGroups(group.getGroupUUID());

javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java

@@ -24,6 +24,7 @@ import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
 import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
 import static java.util.stream.Collectors.toList;
 
+import com.google.common.cache.LoadingCache;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -66,9 +67,11 @@ 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.group.db.Groups;
+import com.google.gerrit.server.index.group.StalenessChecker;
 import com.google.gerrit.server.util.MagicBranch;
 import com.google.gerrit.testing.ConfigSuite;
 import com.google.inject.Inject;
+import com.google.inject.name.Named;
 import java.io.IOException;
 import java.sql.Timestamp;
 import java.time.Instant;
@@ -79,6 +82,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.CommitBuilder;
@@ -89,6 +93,7 @@ import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.PushResult;
 import org.eclipse.jgit.transport.RemoteRefUpdate;
@@ -107,6 +112,11 @@ public class GroupsIT extends AbstractDaemonTest {
   @Inject private Groups groups;
   @Inject private GroupIncludeCache groupIncludeCache;
   @Inject private GroupBackend groupBackend;
+  @Inject private StalenessChecker stalenessChecker;
+
+  @Inject
+  @Named("groups_byuuid")
+  LoadingCache<String, Optional<InternalGroup>> groupsByUUIDCache;
 
   @Test
   public void systemGroupCanBeRetrievedFromIndex() throws Exception {
@@ -974,6 +984,47 @@ public class GroupsIT extends AbstractDaemonTest {
     assertThat(rule.getMax()).isEqualTo(0);
   }
 
+  @Test
+  public void stalenessChecker() throws Exception {
+    assume().that(groupsInNoteDb()).isTrue();
+
+    // Newly created group is not stale
+    GroupInfo groupInfo = gApi.groups().create(name("foo")).get();
+    AccountGroup.UUID groupUuid = new AccountGroup.UUID(groupInfo.id);
+    assertThat(stalenessChecker.isStale(groupUuid)).isFalse();
+
+    // Manual update makes index document stale
+    String groupRef = RefNames.refsGroups(groupUuid);
+    try (Repository repo = repoManager.openRepository(allUsers);
+        ObjectInserter oi = repo.newObjectInserter();
+        RevWalk rw = new RevWalk(repo)) {
+      RevCommit commit = rw.parseCommit(repo.exactRef(groupRef).getObjectId());
+
+      PersonIdent ident = new PersonIdent(serverIdent.get(), TimeUtil.nowTs());
+      CommitBuilder cb = new CommitBuilder();
+      cb.setTreeId(commit.getTree());
+      cb.setCommitter(ident);
+      cb.setAuthor(ident);
+      cb.setMessage(commit.getFullMessage());
+      ObjectId emptyCommit = oi.insert(cb);
+      oi.flush();
+
+      RefUpdate updateRef = repo.updateRef(groupRef);
+      updateRef.setExpectedOldObjectId(commit.toObjectId());
+      updateRef.setNewObjectId(emptyCommit);
+      assertThat(updateRef.forceUpdate()).isEqualTo(RefUpdate.Result.FORCED);
+    }
+    // Evict group from cache to be sure that we use the index state for staleness checks. This has
+    // to happen directly on the groupsByUUID cache because GroupsCacheImpl triggers a reindex for
+    // the group.
+    groupsByUUIDCache.invalidate(groupUuid);
+    assertThat(stalenessChecker.isStale(groupUuid)).isTrue();
+
+    // Reindex fixes staleness
+    gApi.groups().id(groupInfo.id).index();
+    assertThat(stalenessChecker.isStale(groupUuid)).isFalse();
+  }
+
   private void pushToGroupBranchForReviewAndSubmit(Project.NameKey project, String expectedError)
       throws Exception {
     grantLabel(

javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java

@@ -381,7 +381,7 @@ public class GroupRebuilderTest extends GerritBaseTests {
   }
 
   private InternalGroup reload(AccountGroup g) throws Exception {
-    return GroupConfig.loadForGroup(repo, g.getGroupUUID()).getLoadedGroup().get();
+    return removeRefState(GroupConfig.loadForGroup(repo, g.getGroupUUID()).getLoadedGroup().get());
   }
 
   private AccountGroup newGroup(String name) {
@@ -473,4 +473,8 @@ public class GroupRebuilderTest extends GerritBaseTests {
     assertThat(commitInfo).committer().date().isEqualTo(commitInfo.author.date);
     assertThat(commitInfo).committer().tz().isEqualTo(commitInfo.author.tz);
   }
+
+  private static InternalGroup removeRefState(InternalGroup group) throws Exception {
+    return group.toBuilder().setRefState(null).build();
+  }
 }

javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java

@@ -245,19 +245,6 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
     assertQuery("description:\"\"");
   }
 
-  @Test
-  public void byOwner() throws Exception {
-    GroupInfo ownerGroup = createGroup(name("owner-group"));
-    GroupInfo group = createGroupWithOwner(name("group"), ownerGroup);
-    createGroup(name("group2"));
-
-    assertQuery("owner:" + group.id);
-
-    // ownerGroup owns itself
-    assertQuery("owner:" + ownerGroup.id, group, ownerGroup);
-    assertQuery("owner:" + ownerGroup.name, group, ownerGroup);
-  }
-
   @Test
   public void byIsVisibleToAll() throws Exception {
     assertQuery("is:visibletoall");