Fetch change version from refdb on Git protocol v2
The Git protocol v2 has a reduced refs advertisement
phase which is limited to the sole refs requested by
the client. The implications are that the change /meta
revision cannot be extracted anymore from the list of refs
passed to the filter.
Use a secondary lookup table based on the refdb, in order
to avoid caching a status or timestamp using only
the change-id.
Before this change, the Git protocol v1 was working as
expected, however, requests made using protocol v2 and
the only ref pointing to the patch-set were resulting
in serving stale data from the cache.
The cache key for the change open status and its
timestamp has the following three components:
- project
- change id
- change /meta revision
When the change /meta revision was not found in the
incoming list of refs to filter, the cache lookup
was made by simply project and change id.
The consequence were disastrous because an open change
was cached as being open and never invalidated, even
if later on it was abandoned or merged.
The fallback to a direct lookup into the refdb
would solve the problem, even though it may add an extra
lookup with a small performance penalty.
Add IT test with Git protocol v2 for verifing that the fix works
when the client would request a single patch-set ref to fetch.
Bug: Issue 299602346
Change-Id: I9018dee8de0570857d214935f8f4a32b47960ae0
diff --git a/BUILD b/BUILD
index b20f6fb..52ef2e5 100644
--- a/BUILD
+++ b/BUILD
@@ -15,7 +15,10 @@
junit_tests(
name = "git_refs_filter_tests",
srcs = glob(
- ["src/test/java/**/*Test.java"],
+ [
+ "src/test/java/**/*Test.java",
+ "src/test/java/**/*IT.java",
+ ],
exclude = ["src/test/java/**/Abstract*.java"],
),
visibility = ["//visibility:public"],
diff --git a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ForProjectWrapper.java b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ForProjectWrapper.java
index 5347f2d..3b5725b 100644
--- a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ForProjectWrapper.java
+++ b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ForProjectWrapper.java
@@ -19,8 +19,8 @@
import com.google.common.cache.LoadingCache;
import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Change.Id;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.access.CoreOrPluginProjectPermission;
@@ -34,16 +34,19 @@
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.name.Named;
+import java.io.IOException;
import java.sql.Timestamp;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.Collection;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.stream.Collectors;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.RefDatabase;
import org.eclipse.jgit.lib.Repository;
public class ForProjectWrapper extends ForProject {
@@ -96,10 +99,11 @@
@Override
public Collection<Ref> filter(Collection<Ref> refs, Repository repo, RefFilterOptions opts)
throws PermissionBackendException {
- Map<Change.Id, ObjectId> changeRevisions =
+ Map<Optional<Id>, ObjectId> changeRevisions =
refs.stream()
.filter(ref -> ref.getName().endsWith("/meta"))
.collect(Collectors.toMap(ForProjectWrapper::changeIdFromRef, Ref::getObjectId));
+ RefDatabase refDb = repo.getRefDatabase();
return defaultForProject
.filter(refs, repo, opts)
.parallelStream()
@@ -108,30 +112,42 @@
.filter(config::isRefToShow)
.filter(
(ref) -> {
- Change.Id changeId = changeIdFromRef(ref);
+ Optional<Id> changeId = changeIdFromRef(ref);
+ Optional<ObjectId> changeRevision =
+ changeId.flatMap(cid -> Optional.ofNullable(changeRevisions.get(changeId)));
+ if (!changeRevision.isPresent()) {
+ changeRevision = changeRevisionFromRefDb(refDb, changeId);
+ }
String refName = ref.getName();
- return (!isChangeRef(refName)
- || (!isChangeMetaRef(refName)
- && changeId != null
- && (isOpen(repo, changeId, changeRevisions.get(changeId))
- || isRecent(repo, changeId, changeRevisions.get(changeId)))));
+ return (!changeId.isPresent()
+ || !changeRevision.isPresent()
+ || (!RefNames.isNoteDbMetaRef(refName)
+ && (isOpen(repo, changeId.get(), changeRevision.get())
+ || isRecent(repo, changeId.get(), changeRevision.get()))));
})
.collect(Collectors.toList());
}
- private static Change.Id changeIdFromRef(Ref ref) {
- return Change.Id.fromRef(ref.getName());
+ private static Optional<ObjectId> changeRevisionFromRefDb(
+ RefDatabase refDb, Optional<Change.Id> changeId) {
+ return changeId.flatMap(cid -> exactRefUnchecked(refDb, cid)).map(Ref::getObjectId);
}
- private boolean isChangeRef(String changeKey) {
- return changeKey.startsWith("refs/changes");
+ private static Optional<Ref> exactRefUnchecked(RefDatabase refDb, Change.Id changeId) {
+ try {
+ return Optional.ofNullable(refDb.exactRef(RefNames.changeMetaRef(changeId)));
+ } catch (IOException e) {
+ logger.atWarning().withCause(e).log(
+ "Error looking up change '%d' meta-ref from refs db.", changeId);
+ return Optional.empty();
+ }
}
- private boolean isChangeMetaRef(String changeKey) {
- return isChangeRef(changeKey) && changeKey.endsWith("/meta");
+ private static Optional<Change.Id> changeIdFromRef(Ref ref) {
+ return Optional.ofNullable(Change.Id.fromRef(ref.getName()));
}
- private boolean isOpen(Repository repo, Change.Id changeId, @Nullable ObjectId changeRevision) {
+ private boolean isOpen(Repository repo, Change.Id changeId, ObjectId changeRevision) {
try {
return openChangesCache.get(ChangeCacheKey.create(repo, changeId, changeRevision, project));
} catch (ExecutionException e) {
@@ -142,7 +158,7 @@
}
}
- private boolean isRecent(Repository repo, Change.Id changeId, @Nullable ObjectId changeRevision) {
+ private boolean isRecent(Repository repo, Change.Id changeId, ObjectId changeRevision) {
try {
Timestamp cutOffTs =
Timestamp.from(
diff --git a/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/AbstractGitDaemonTest.java b/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/AbstractGitDaemonTest.java
index c58f0b8..243baa9 100644
--- a/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/AbstractGitDaemonTest.java
+++ b/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/AbstractGitDaemonTest.java
@@ -22,13 +22,22 @@
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.groups.GroupApi;
import com.google.gerrit.extensions.api.groups.GroupInput;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.git.meta.MetaDataUpdate;
+import com.google.gerrit.server.project.ProjectConfig;
import com.google.inject.Inject;
+import com.google.inject.Module;
import com.googlesource.gerrit.modules.gitrefsfilter.FilterRefsCapability;
+import com.googlesource.gerrit.modules.gitrefsfilter.FilterRefsConfig;
+import com.googlesource.gerrit.modules.gitrefsfilter.RefsFilterModule;
import java.io.IOException;
+import java.time.Duration;
import java.util.List;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
@@ -43,6 +52,11 @@
@Inject private RequestScopeOperations requestScopeOperations;
@Inject private ProjectOperations projectOperations;
+ @Override
+ public Module createModule() {
+ return new RefsFilterModule();
+ }
+
protected int createChangeAndAbandon() throws Exception, RestApiException {
requestScopeOperations.setApiUser(admin.id());
createChange();
@@ -63,6 +77,10 @@
groupApi.removeMembers(admin.username());
String groupId = groupApi.detail().id;
+ setHideClosedChangesRefs(groupId);
+ }
+
+ protected void setHideClosedChangesRefs(String groupId) {
projectOperations
.allProjectsForUpdate()
.add(
@@ -120,4 +138,20 @@
*/
return Integer.parseInt(ref.getName().split("/")[4]);
}
+
+ protected void setProjectClosedChangesGraceTime(Project.NameKey project, Duration graceTime)
+ throws IOException, ConfigInvalidException, RepositoryNotFoundException {
+ try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) {
+ ProjectConfig projectConfig = projectConfigFactory.create(project);
+ projectConfig.load(md);
+ projectConfig.updatePluginConfig(
+ "gerrit",
+ cfg ->
+ cfg.setLong(
+ FilterRefsConfig.PROJECT_CONFIG_CLOSED_CHANGES_GRACE_TIME_SEC,
+ graceTime.toSeconds()));
+ projectConfig.commit(md);
+ projectCache.evict(project);
+ }
+ }
}
diff --git a/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/GitRefsFilterProtocolV2IT.java b/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/GitRefsFilterProtocolV2IT.java
new file mode 100644
index 0000000..0d56ec8
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/GitRefsFilterProtocolV2IT.java
@@ -0,0 +1,186 @@
+// Copyright (C) 2023 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.googlesource.gerrit.libmodule.plugins.test;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.WaitUtil.waitUntil;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.io.ByteStreams;
+import com.google.gerrit.acceptance.GitClientVersion;
+import com.google.gerrit.acceptance.UseLocalDisk;
+import com.google.gerrit.acceptance.UseSsh;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.Permission;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.ChangeInput;
+import com.google.gerrit.httpd.CanonicalWebUrl;
+import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.group.SystemGroupBackend;
+import com.google.inject.Inject;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InterruptedIOException;
+import java.nio.file.Path;
+import java.time.Duration;
+import org.junit.Test;
+
+@UseSsh
+@UseLocalDisk
+public class GitRefsFilterProtocolV2IT extends AbstractGitDaemonTest {
+ private final String[] GIT_FETCH = new String[] {"git", "-c", "protocol.version=2", "fetch"};
+ private final String[] GIT_INIT = new String[] {"git", "init"};
+
+ private static final Duration TEST_PATIENCE_TIME = Duration.ofSeconds(1);
+
+ @Inject private ProjectOperations projectOperations;
+ @Inject private SitePaths sitePaths;
+ @Inject private CanonicalWebUrl url;
+
+ public static void assertGitClientVersion() throws Exception {
+ // Minimum required git-core version that supports wire protocol v2 is 2.18.0
+ GitClientVersion requiredGitVersion = new GitClientVersion(2, 18, 0);
+ GitClientVersion actualGitVersion =
+ new GitClientVersion(execute(ImmutableList.of("git", "version"), new File("/")));
+ // If git client version cannot be updated, consider to skip this tests. Due to
+ // an existing issue in bazel, JUnit assumption violation feature cannot be used.
+ assertThat(actualGitVersion).isAtLeast(requiredGitVersion);
+ }
+
+ @Test
+ public void testGitWireProtocolV2HidesAbandonedChange() throws Exception {
+ assertGitClientVersion();
+
+ Project.NameKey allRefsVisibleProject = Project.nameKey("all-refs-visible");
+ gApi.projects().create(allRefsVisibleProject.get());
+
+ setProjectPermissionReadAllRefs(allRefsVisibleProject);
+ setHideClosedChangesRefs(SystemGroupBackend.ANONYMOUS_USERS.get());
+ setProjectClosedChangesGraceTime(allRefsVisibleProject, Duration.ofSeconds(0));
+
+ // Create new change and retrieve refs for the created patch set
+ ChangeInput visibleChangeIn =
+ new ChangeInput(allRefsVisibleProject.get(), "master", "Test public change");
+ visibleChangeIn.newBranch = true;
+ ChangeInfo changeInfo = gApi.changes().create(visibleChangeIn).info();
+ int visibleChangeNumber = changeInfo._number;
+ Change.Id changeId = Change.id(visibleChangeNumber);
+ String visibleChangeNumberRef = RefNames.patchSetRef(PatchSet.id(changeId, 1));
+ String patchSetSha1 = gApi.changes().id(visibleChangeNumber).get().currentRevision;
+ String visibleChangeNumberWithSha1 = patchSetSha1 + " " + visibleChangeNumberRef;
+
+ execute(ImmutableList.<String>builder().add(GIT_INIT).build(), ImmutableMap.of());
+ String gitProtocolOutActiveChange = execFetch(allRefsVisibleProject, visibleChangeNumberRef);
+ assertThat(gitProtocolOutActiveChange).contains(visibleChangeNumberWithSha1);
+
+ gApi.changes().id(changeId.get()).abandon();
+
+ waitUntil(
+ () -> {
+ try {
+ return !execFetch(allRefsVisibleProject, visibleChangeNumberRef)
+ .contains(visibleChangeNumberWithSha1);
+ } catch (Exception e) {
+ throw new IllegalStateException(e);
+ }
+ },
+ TEST_PATIENCE_TIME);
+ }
+
+ private String execFetch(Project.NameKey project, String refs) throws Exception {
+ String outAnonymousLsRemote =
+ execute(
+ ImmutableList.<String>builder()
+ .add(GIT_FETCH)
+ .add(url.get(null) + "/" + project.get())
+ .add(refs)
+ .build(),
+ ImmutableMap.of("GIT_TRACE_PACKET", "1"));
+ assertGitProtocolV2(outAnonymousLsRemote);
+ return outAnonymousLsRemote;
+ }
+
+ private void assertGitProtocolV2(String outAnonymousLsRemote) {
+ assertThat(outAnonymousLsRemote).contains("git< version 2");
+ }
+
+ private void setProjectPermissionReadAllRefs(Project.NameKey project) {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.READ).ref("refs/*").group(SystemGroupBackend.ANONYMOUS_USERS))
+ .update();
+ }
+
+ private String execute(ImmutableList<String> cmd, ImmutableMap<String, String> env)
+ throws Exception {
+ return execute(cmd, sitePaths.data_dir.toFile(), env);
+ }
+
+ private static String execute(ImmutableList<String> cmd, File dir) throws Exception {
+ return execute(cmd, dir, ImmutableMap.of());
+ }
+
+ protected static String execute(
+ ImmutableList<String> cmd, File dir, ImmutableMap<String, String> env) throws IOException {
+ return execute(cmd, dir, env, null);
+ }
+
+ protected static String execute(
+ ImmutableList<String> cmd,
+ File dir,
+ @Nullable ImmutableMap<String, String> env,
+ @Nullable Path outputPath)
+ throws IOException {
+ ProcessBuilder pb = new ProcessBuilder(cmd);
+ pb.directory(dir);
+ if (outputPath != null) {
+ pb.redirectOutput(outputPath.toFile());
+ } else {
+ pb.redirectErrorStream(true);
+ }
+ pb.environment().putAll(env);
+ Process p = pb.start();
+ byte[] out;
+ try (InputStream in = p.getInputStream()) {
+ out = ByteStreams.toByteArray(in);
+ } finally {
+ p.getOutputStream().close();
+ }
+
+ try {
+ p.waitFor();
+ } catch (InterruptedException e) {
+ InterruptedIOException iioe =
+ new InterruptedIOException(
+ "interrupted waiting for: " + Joiner.on(' ').join(pb.command()));
+ iioe.initCause(e);
+ throw iioe;
+ }
+
+ String result = new String(out, UTF_8);
+ return result.trim();
+ }
+}
diff --git a/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/GitRefsFilterTest.java b/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/GitRefsFilterTest.java
index c47ebe7..7663d76 100644
--- a/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/GitRefsFilterTest.java
+++ b/src/test/java/com/googlesource/gerrit/libmodule/plugins/test/GitRefsFilterTest.java
@@ -29,14 +29,9 @@
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.RefNames;
-import com.google.gerrit.server.git.meta.MetaDataUpdate;
-import com.google.gerrit.server.project.ProjectConfig;
import com.google.inject.Inject;
-import com.google.inject.Module;
import com.google.inject.name.Named;
import com.googlesource.gerrit.modules.gitrefsfilter.ChangeCacheKey;
-import com.googlesource.gerrit.modules.gitrefsfilter.FilterRefsConfig;
-import com.googlesource.gerrit.modules.gitrefsfilter.RefsFilterModule;
import java.io.IOException;
import java.sql.Timestamp;
import java.time.Duration;
@@ -76,26 +71,10 @@
private volatile Exception getRefsException = null;
- @Override
- public Module createModule() {
- return new RefsFilterModule();
- }
-
@Before
public void setup() throws Exception {
createFilteredRefsGroup();
- try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) {
- ProjectConfig projectConfig = projectConfigFactory.create(project);
- projectConfig.load(md);
- projectConfig.updatePluginConfig(
- "gerrit",
- cfg ->
- cfg.setLong(
- FilterRefsConfig.PROJECT_CONFIG_CLOSED_CHANGES_GRACE_TIME_SEC,
- CLOSED_CHANGES_GRACE_TIME_SEC));
- projectConfig.commit(md);
- projectCache.evict(project);
- }
+ setProjectClosedChangesGraceTime(project, Duration.ofSeconds(CLOSED_CHANGES_GRACE_TIME_SEC));
}
@Test
