diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java index f341970dad..0c3d89c66d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java @@ -20,25 +20,133 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.joining; import com.google.auto.value.AutoValue; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Splitter; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.HashMultimap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.ListMultimap; +import com.google.common.collect.Multimap; import com.google.common.collect.SetMultimap; +import com.google.common.collect.Sets; import com.google.gerrit.common.Nullable; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.index.IndexConfig; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import java.io.IOException; import java.util.List; +import java.util.Optional; +import java.util.Set; import java.util.regex.Pattern; import java.util.stream.StreamSupport; +@Singleton public class StalenessChecker { - public static SetMultimap parseStates( + private static final Logger log = + LoggerFactory.getLogger(StalenessChecker.class); + + private final ImmutableSet FIELDS = ImmutableSet.of( + ChangeField.CHANGE.getName(), + ChangeField.REF_STATE.getName(), + ChangeField.REF_STATE_PATTERN.getName()); + + private final ChangeIndexCollection indexes; + private final GitRepositoryManager repoManager; + private final IndexConfig indexConfig; + private final Provider db; + + @Inject + StalenessChecker( + ChangeIndexCollection indexes, + GitRepositoryManager repoManager, + IndexConfig indexConfig, + Provider db) { + this.indexes = indexes; + this.repoManager = repoManager; + this.indexConfig = indexConfig; + this.db = db; + } + + boolean isStale(Change.Id id) throws IOException, OrmException { + ChangeIndex i = indexes.getSearchIndex(); + if (i == null) { + return false; // No index; caller couldn't do anything if it is stale. + } + if (!i.getSchema().hasField(ChangeField.REF_STATE) + || !i.getSchema().hasField(ChangeField.REF_STATE_PATTERN)) { + return false; // Index version not new enough for this check. + } + + Optional result = i.get( + id, IndexedChangeQuery.createOptions(indexConfig, 0, 1, FIELDS)); + if (!result.isPresent()) { + return true; // Not in index, but caller wants it to be. + } + ChangeData cd = result.get(); + if (reviewDbChangeIsStale( + cd.change(), + ChangeNotes.readOneReviewDbChange(db.get(), cd.getId()))) { + return true; + } + + return isStale(repoManager, id, parseStates(cd), parsePatterns(cd)); + } + + @VisibleForTesting + static boolean isStale(GitRepositoryManager repoManager, + Change.Id id, + SetMultimap states, + Multimap patterns) { + Set projects = + Sets.union(states.keySet(), patterns.keySet()); + + for (Project.NameKey p : projects) { + if (isStale(repoManager, id, p, states, patterns)) { + return true; + } + } + + return false; + } + + @VisibleForTesting + static boolean reviewDbChangeIsStale( + Change indexChange, @Nullable Change reviewDbChange) { + if (reviewDbChange == null) { + return false; // Nothing the caller can do. + } + checkArgument(indexChange.getId().equals(reviewDbChange.getId()), + "mismatched change ID: %s != %s", + indexChange.getId(), reviewDbChange.getId()); + if (PrimaryStorage.of(reviewDbChange) != PrimaryStorage.REVIEW_DB) { + return false; // Not a ReviewDb change, don't check rowVersion. + } + return reviewDbChange.getRowVersion() != indexChange.getRowVersion(); + } + + private SetMultimap parseStates(ChangeData cd) { + return parseStates(cd.getRefStates()); + } + + @VisibleForTesting + static SetMultimap parseStates( Iterable states) { RefState.check(states != null, null); SetMultimap result = HashMultimap.create(); @@ -58,6 +166,11 @@ public class StalenessChecker { return result; } + private Multimap parsePatterns( + ChangeData cd) { + return parsePatterns(cd.getRefStatePatterns()); + } + public static ListMultimap parsePatterns( Iterable patterns) { RefStatePattern.check(patterns != null, null); @@ -75,6 +188,31 @@ public class StalenessChecker { return result; } + private static boolean isStale(GitRepositoryManager repoManager, + Change.Id id, Project.NameKey project, + SetMultimap allStates, + Multimap allPatterns) { + try (Repository repo = repoManager.openRepository(project)) { + Set states = allStates.get(project); + for (RefState state : states) { + if (!state.match(repo)) { + return true; + } + } + for (RefStatePattern pattern : allPatterns.get(project)) { + if (!pattern.match(repo, states)) { + return true; + } + } + return false; + } catch (IOException e) { + log.warn( + String.format("error checking staleness of %s in %s", id, project), + e); + return true; + } + } + @AutoValue public abstract static class RefState { static RefState create(String ref, String sha) { @@ -106,6 +244,12 @@ public class StalenessChecker { abstract String ref(); abstract ObjectId id(); + + private boolean match(Repository repo) throws IOException { + Ref ref = repo.exactRef(ref()); + ObjectId expected = ref != null ? ref.getObjectId() : ObjectId.zeroId(); + return id().equals(expected); + } } /** @@ -147,5 +291,18 @@ public class StalenessChecker { boolean match(String refName) { return regex().matcher(refName).find(); } + + private boolean match(Repository repo, Set expected) + throws IOException { + for (Ref r : repo.getRefDatabase().getRefs(prefix()).values()) { + if (!match(r.getName())) { + continue; + } + if (!expected.contains(RefState.of(r))) { + return false; + } + } + return true; + } } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java index 4c86fa1a96..cdd1e07891 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java @@ -16,17 +16,30 @@ package com.google.gerrit.server.index.change; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assert_; +import static com.google.gerrit.server.index.change.StalenessChecker.isStale; +import static com.google.gerrit.testutil.TestChanges.newChange; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.toList; +import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.ListMultimap; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.index.change.StalenessChecker.RefState; import com.google.gerrit.server.index.change.StalenessChecker.RefStatePattern; +import com.google.gerrit.server.notedb.NoteDbChangeState; import com.google.gerrit.testutil.GerritBaseTests; +import com.google.gerrit.testutil.InMemoryRepositoryManager; +import com.google.gwtorm.protobuf.CodecFactory; +import com.google.gwtorm.protobuf.ProtobufCodec; +import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; +import org.junit.Before; import org.junit.Test; import java.util.stream.Stream; @@ -38,6 +51,26 @@ public class StalenessCheckerTest extends GerritBaseTests { private static final Project.NameKey P1 = new Project.NameKey("project1"); private static final Project.NameKey P2 = new Project.NameKey("project2"); + private static final Change.Id C = new Change.Id(1234); + + private static final ProtobufCodec CHANGE_CODEC = + CodecFactory.encoder(Change.class); + + private GitRepositoryManager repoManager; + private Repository r1; + private Repository r2; + private TestRepository tr1; + private TestRepository tr2; + + @Before + public void setUp() throws Exception { + repoManager = new InMemoryRepositoryManager(); + r1 = repoManager.createRepository(P1); + tr1 = new TestRepository<>(r1); + r2 = repoManager.createRepository(P2); + tr2 = new TestRepository<>(r2); + } + @Test public void parseStates() { assertInvalidState(null); @@ -147,8 +180,180 @@ public class StalenessCheckerTest extends GerritBaseTests { } } + @Test + public void isStaleRefStatesOnly() throws Exception { + String ref1 = "refs/heads/foo"; + ObjectId id1 = tr1.update(ref1, tr1.commit().message("commit 1")); + String ref2 = "refs/heads/bar"; + ObjectId id2 = tr2.update(ref2, tr2.commit().message("commit 2")); + + // Not stale. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P2, RefState.create(ref2, id2.name())), + ImmutableMultimap.of())) + .isFalse(); + + // Wrong ref value. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, SHA1), + P2, RefState.create(ref2, id2.name())), + ImmutableMultimap.of())) + .isTrue(); + + // Swapped repos. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id2.name()), + P2, RefState.create(ref2, id1.name())), + ImmutableMultimap.of())) + .isTrue(); + + // Two refs in same repo, not stale. + String ref3 = "refs/heads/baz"; + ObjectId id3 = tr1.update(ref3, tr1.commit().message("commit 3")); + tr1.update(ref3, id3); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P1, RefState.create(ref3, id3.name())), + ImmutableMultimap.of())) + .isFalse(); + + // Ignore ref not mentioned. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of())) + .isFalse(); + + // One ref wrong. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P1, RefState.create(ref3, SHA1)), + ImmutableMultimap.of())) + .isTrue(); + } + + @Test + public void isStaleWithRefStatePatterns() throws Exception { + String ref1 = "refs/heads/foo"; + ObjectId id1 = tr1.update(ref1, tr1.commit().message("commit 1")); + + // ref1 is only ref matching pattern. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/heads/*")))) + .isFalse(); + + // Now ref2 matches pattern, so stale unless ref2 is present in state map. + String ref2 = "refs/heads/bar"; + ObjectId id2 = tr1.update(ref2, tr1.commit().message("commit 2")); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/heads/*")))) + .isTrue(); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P1, RefState.create(ref2, id2.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/heads/*")))) + .isFalse(); + } + + @Test + public void isStaleWithNonPrefixPattern() throws Exception { + String ref1 = "refs/heads/foo"; + ObjectId id1 = tr1.update(ref1, tr1.commit().message("commit 1")); + tr1.update("refs/heads/bar", tr1.commit().message("commit 2")); + + // ref1 is only ref matching pattern. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/*/foo")))) + .isFalse(); + + // Now ref2 matches pattern, so stale unless ref2 is present in state map. + String ref3 = "refs/other/foo"; + ObjectId id3 = tr1.update(ref3, tr1.commit().message("commit 3")); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/*/foo")))) + .isTrue(); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P1, RefState.create(ref3, id3.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/*/foo")))) + .isFalse(); + } + + @Test + public void reviewDbChangeIsStale() throws Exception { + Change indexChange = newChange(P1, new Account.Id(1)); + indexChange.setNoteDbState(SHA1); + + assertThat(StalenessChecker.reviewDbChangeIsStale(indexChange, null)) + .isFalse(); + + Change noteDbPrimary = clone(indexChange); + noteDbPrimary.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE); + assertThat( + StalenessChecker.reviewDbChangeIsStale(indexChange, noteDbPrimary)) + .isFalse(); + + assertThat( + StalenessChecker.reviewDbChangeIsStale( + indexChange, clone(indexChange))) + .isFalse(); + + // Can't easily change row version to check true case. + } + private static Iterable byteArrays(String... strs) { return Stream.of(strs).map(s -> s != null ? s.getBytes(UTF_8) : null) .collect(toList()); } + + private static Change clone(Change change) { + return CHANGE_CODEC.decode(CHANGE_CODEC.encodeToByteArray(change)); + } + }