diff --git a/java/com/google/gerrit/server/group/InternalGroup.java b/java/com/google/gerrit/server/group/InternalGroup.java index ad6fa5c4c8..7828586271 100644 --- a/java/com/google/gerrit/server/group/InternalGroup.java +++ b/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 members, ImmutableSet subgroups) { + return create(accountGroup, members, subgroups, null); + } + + public static InternalGroup create( + AccountGroup accountGroup, + ImmutableSet members, + ImmutableSet 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 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 subgroups); + public abstract Builder setRefState(ObjectId refState); + public abstract InternalGroup build(); } } diff --git a/java/com/google/gerrit/server/group/db/GroupConfig.java b/java/com/google/gerrit/server/group/db/GroupConfig.java index 8ac533c51c..0cdcdecb76 100644 --- a/java/com/google/gerrit/server/group/db/GroupConfig.java +++ b/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 members = readMembers(); ImmutableSet 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 members, ImmutableSet 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; diff --git a/java/com/google/gerrit/server/group/db/GroupsUpdate.java b/java/com/google/gerrit/server/group/db/GroupsUpdate.java index 2e67ec48ee..f6f1242ae4 100644 --- a/java/com/google/gerrit/server/group/db/GroupsUpdate.java +++ b/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 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 modifiedSubgroups); + public abstract Builder setRefState(ObjectId refState); + abstract UpdateResult build(); } } diff --git a/java/com/google/gerrit/server/index/IndexUtils.java b/java/com/google/gerrit/server/index/IndexUtils.java index b37ed61c54..9730e3f07c 100644 --- a/java/com/google/gerrit/server/index/IndexUtils.java +++ b/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 CUSTOM_CHAR_MAPPING = ImmutableMap.of("_", " ", ".", " "); + public static final Function MAPPER = + new Function() { + @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 { diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/java/com/google/gerrit/server/index/change/ChangeIndexer.java index f62b6623e0..d205e9197a 100644 --- a/java/com/google/gerrit/server/index/change/ChangeIndexer.java +++ b/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 MAPPER = - new Function() { - @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 schemaFactory; @@ -335,7 +320,8 @@ public class ChangeIndexer { @SuppressWarnings("deprecation") private static com.google.common.util.concurrent.CheckedFuture submit( Callable 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 implements Callable { diff --git a/java/com/google/gerrit/server/index/group/GroupField.java b/java/com/google/gerrit/server/index/group/GroupField.java index 078433aaf4..29e386731e 100644 --- a/java/com/google/gerrit/server/index/group/GroupField.java +++ b/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/. */ + public static final FieldDef 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; + }); } diff --git a/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java b/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java index 69b29bcf29..cb69ea8265 100644 --- a/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java +++ b/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 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 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 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. + * + *

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 reindexIfStale( + AccountGroup.UUID uuid) { + Callable 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) { diff --git a/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java b/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java index b280b253e0..912524fe3c 100644 --- a/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java +++ b/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java @@ -34,8 +34,11 @@ public class GroupSchemaDefinitions extends SchemaDefinitions { @Deprecated static final Schema V3 = schema(V2, GroupField.CREATED_ON); + @Deprecated static final Schema V4 = schema(V3, GroupField.MEMBER, GroupField.SUBGROUP); + static final Schema V5 = schema(V4, GroupField.REF_STATE); + public static final GroupSchemaDefinitions INSTANCE = new GroupSchemaDefinitions(); private GroupSchemaDefinitions() { diff --git a/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java b/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java index 255df329c6..79f25c0861 100644 --- a/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java +++ b/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 implements DataSource { + public static QueryOptions createOptions( + IndexConfig config, int start, int limit, Set 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 index, Predicate pred, diff --git a/java/com/google/gerrit/server/index/group/StalenessChecker.java b/java/com/google/gerrit/server/index/group/StalenessChecker.java new file mode 100644 index 0000000000..dba89ad6da --- /dev/null +++ b/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. + * + *

An index document is considered stale if the stored SHA1 differs from the HEAD SHA1 of the + * groups branch. + * + *

Note: This only applies to NoteDb. + */ +@Singleton +public class StalenessChecker { + public static final ImmutableSet 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 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)); + } + } +} diff --git a/java/com/google/gerrit/server/query/group/GroupPredicates.java b/java/com/google/gerrit/server/query/group/GroupPredicates.java index d02f6a46fa..210651e9c8 100644 --- a/java/com/google/gerrit/server/query/group/GroupPredicates.java +++ b/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 owner(AccountGroup.UUID ownerUuid) { return new GroupPredicate( GroupField.OWNER_UUID, GroupQueryBuilder.FIELD_OWNER, ownerUuid.get()); diff --git a/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java b/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java index 057cc440ea..c7fdfba76c 100644 --- a/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java +++ b/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java @@ -49,7 +49,7 @@ public class GroupQueryBuilder extends QueryBuilder { 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 mydef = @@ -109,12 +109,6 @@ public class GroupQueryBuilder extends QueryBuilder { return GroupPredicates.name(name); } - @Operator - public Predicate owner(String owner) throws QueryParseException { - AccountGroup.UUID groupUuid = parseGroup(owner); - return GroupPredicates.owner(groupUuid); - } - @Operator public Predicate is(String value) throws QueryParseException { if ("visibletoall".equalsIgnoreCase(value)) { @@ -133,11 +127,6 @@ public class GroupQueryBuilder extends QueryBuilder { if (!Strings.isNullOrEmpty(query)) { preds.add(description(query)); } - try { - preds.add(owner(query)); - } catch (QueryParseException e) { - // Skip. - } return Predicate.or(preds); } diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java index 3437cf4eb4..dbbd07ea81 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java +++ b/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 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()); diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java index 631f96ef2a..a9b4e880b3 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java +++ b/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> 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( diff --git a/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java b/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java index 1d26b2fd70..863d195278 100644 --- a/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java +++ b/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(); + } } diff --git a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java index 54c22f932c..68cdaae388 100644 --- a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java +++ b/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");