Introduce change-ts cache for showing changes just merged
Many CI/CD systems perform validation also on changes just
merged as post-merge build. Keep the refs of the changes
still visible for a grace time for allowing them to be
fetched by builders.
The grace time is fixed to 24h: the configurability
is addressed on the follow-up change.
Change-Id: I478c6f287a90cad2acc32187316789cd5463513c
diff --git a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangesTsCache.java b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangesTsCache.java
new file mode 100644
index 0000000..cfd7a0f
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangesTsCache.java
@@ -0,0 +1,64 @@
+// Copyright (C) 2022 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.modules.gitrefsfilter;
+
+import com.google.common.cache.CacheLoader;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import com.google.inject.Singleton;
+import com.google.inject.TypeLiteral;
+
+public class ChangesTsCache {
+ public static final String CHANGES_CACHE_TS = "changes_ts";
+
+ public static Module module() {
+ return new CacheModule() {
+ @Override
+ protected void configure() {
+ cache(CHANGES_CACHE_TS, ChangeCacheKey.class, new TypeLiteral<Long>() {})
+ .loader(Loader.class);
+ }
+ };
+ }
+
+ @Singleton
+ static class Loader extends CacheLoader<ChangeCacheKey, Long> {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private final ChangeNotes.Factory changeNotesFactory;
+
+ @Inject
+ Loader(ChangeNotes.Factory changeNotesFactory) {
+ this.changeNotesFactory = changeNotesFactory;
+ }
+
+ @Override
+ public Long load(ChangeCacheKey key) throws Exception {
+ try {
+ return changeNotesFactory
+ .createChecked(key.repo(), key.project(), key.changeId(), key.changeRevision())
+ .getChange()
+ .getLastUpdatedOn()
+ .getTime();
+ } catch (NoSuchChangeException e) {
+ logger.atFine().withCause(e).log(
+ "Change %d does not exist: returning zero epoch", key.changeId());
+ return 0L;
+ }
+ }
+ }
+}
diff --git a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/FilterRefsConfig.java b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/FilterRefsConfig.java
index 2524f5e..6bfafc2 100644
--- a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/FilterRefsConfig.java
+++ b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/FilterRefsConfig.java
@@ -19,6 +19,7 @@
import com.google.inject.Inject;
import java.util.Arrays;
import java.util.List;
+import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Ref;
@@ -27,6 +28,11 @@
public static final String SECTION_GIT_REFS_FILTER = "git-refs-filter";
public static final String KEY_HIDE_REFS = "hideRefs";
+ private static final long CLOSED_CHANGE_GRACE_TIME_SEC_DEFAULT =
+ TimeUnit.SECONDS.convert(24, TimeUnit.HOURS);
+
+ private long closedChangeGraceTimeSec = CLOSED_CHANGE_GRACE_TIME_SEC_DEFAULT;
+
private final List<String> hideRefs;
private final List<String> showRefs;
@@ -68,4 +74,12 @@
return true;
}
+
+ public long getClosedChangeGraceTimeSec() {
+ return closedChangeGraceTimeSec;
+ }
+
+ public void setClosedChangeGraceTimeSec(long graceTimeSec) {
+ this.closedChangeGraceTimeSec = graceTimeSec;
+ }
}
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 75842e3..7e30b5f 100644
--- a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ForProjectWrapper.java
+++ b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ForProjectWrapper.java
@@ -14,6 +14,7 @@
package com.googlesource.gerrit.modules.gitrefsfilter;
+import static com.googlesource.gerrit.modules.gitrefsfilter.ChangesTsCache.CHANGES_CACHE_TS;
import static com.googlesource.gerrit.modules.gitrefsfilter.OpenChangesCache.OPEN_CHANGES_CACHE;
import com.google.common.cache.LoadingCache;
@@ -32,6 +33,9 @@
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.name.Named;
+import java.sql.Timestamp;
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
import java.util.Collection;
import java.util.Map;
import java.util.Set;
@@ -45,6 +49,7 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final LoadingCache<ChangeCacheKey, Boolean> openChangesCache;
+ private final LoadingCache<ChangeCacheKey, Long> changesTsCache;
private final ForProject defaultForProject;
private final Project.NameKey project;
private final FilterRefsConfig config;
@@ -57,9 +62,11 @@
public ForProjectWrapper(
FilterRefsConfig config,
@Named(OPEN_CHANGES_CACHE) LoadingCache<ChangeCacheKey, Boolean> openChangesCache,
+ @Named(CHANGES_CACHE_TS) LoadingCache<ChangeCacheKey, Long> changesTsCache,
@Assisted ForProject defaultForProject,
@Assisted Project.NameKey project) {
this.openChangesCache = openChangesCache;
+ this.changesTsCache = changesTsCache;
this.defaultForProject = defaultForProject;
this.project = project;
this.config = config;
@@ -102,7 +109,8 @@
return (!isChangeRef(refName)
|| (!isChangeMetaRef(refName)
&& changeId != null
- && isOpen(repo, changeId, changeRevisions.get(changeId))));
+ && (isOpen(repo, changeId, changeRevisions.get(changeId))
+ || isRecent(repo, changeId, changeRevisions.get(changeId)))));
})
.collect(Collectors.toList());
}
@@ -130,6 +138,26 @@
}
}
+ private boolean isRecent(Repository repo, Change.Id changeId, @Nullable ObjectId changeRevision) {
+ try {
+ Timestamp cutOffTs =
+ Timestamp.from(
+ Instant.now()
+ .truncatedTo(ChronoUnit.SECONDS)
+ .minusSeconds(config.getClosedChangeGraceTimeSec()));
+ return changesTsCache
+ .get(ChangeCacheKey.create(repo, changeId, changeRevision, project))
+ .longValue()
+ >= cutOffTs.getTime();
+
+ } catch (ExecutionException e) {
+ logger.atWarning().withCause(e).log(
+ "Error getting change '%d' from the cache. Do not hide from the advertised refs",
+ changeId);
+ return true;
+ }
+ }
+
@Override
public BooleanCondition testCond(CoreOrPluginProjectPermission perm) {
return defaultForProject.testCond(perm);
diff --git a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/RefsFilterModule.java b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/RefsFilterModule.java
index 963e529..5d53d22 100644
--- a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/RefsFilterModule.java
+++ b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/RefsFilterModule.java
@@ -25,6 +25,8 @@
@Override
protected void configure() {
+ bind(FilterRefsConfig.class).in(Scopes.SINGLETON);
+
install(
new FactoryModuleBuilder()
.implement(WithUserWrapper.class, WithUserWrapper.class)
@@ -43,5 +45,6 @@
.in(Scopes.SINGLETON);
install(OpenChangesCache.module());
+ install(ChangesTsCache.module());
}
}
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 a8bcf02..44d6cbe 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
@@ -15,6 +15,8 @@
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.googlesource.gerrit.modules.gitrefsfilter.ChangesTsCache.CHANGES_CACHE_TS;
import static com.googlesource.gerrit.modules.gitrefsfilter.OpenChangesCache.OPEN_CHANGES_CACHE;
import com.google.common.cache.LoadingCache;
@@ -31,8 +33,13 @@
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;
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
@@ -53,10 +60,21 @@
@Sandboxed
public class GitRefsFilterTest extends AbstractGitDaemonTest {
@Inject private RequestScopeOperations requestScopeOperations;
+ @Inject private FilterRefsConfig filterConfig;
@Inject
private @Named(OPEN_CHANGES_CACHE) LoadingCache<ChangeCacheKey, Boolean> changeOpenCache;
+ @Inject
+ private @Named(CHANGES_CACHE_TS) LoadingCache<ChangeCacheKey, Long> changesTsCache;
+
+ private static final int CLOSED_CHANGES_GRACE_TIME_SEC = 5;
+
+ private static final Duration TEST_PATIENCE_TIME =
+ Duration.ofSeconds(CLOSED_CHANGES_GRACE_TIME_SEC + 1);
+
+ private volatile Exception getRefsException = null;
+
@Override
public Module createModule() {
return new RefsFilterModule();
@@ -65,13 +83,30 @@
@Before
public void setup() throws Exception {
createFilteredRefsGroup();
+ filterConfig.setClosedChangeGraceTimeSec(CLOSED_CHANGES_GRACE_TIME_SEC);
}
@Test
public void testUserWithFilterOutCapabilityShouldNotSeeAbandonedChangesRefs() throws Exception {
+ Timestamp changeTs = gApi.changes().id(createChangeAndAbandon()).get().updated;
+
+ waitUntil(() -> getRefsUnchecked(user).isEmpty(), TEST_PATIENCE_TIME);
+ checkGetRefsIsSuccessful();
+
+ Timestamp filterCutoffTs =
+ Timestamp.from(
+ Instant.now()
+ .truncatedTo(ChronoUnit.SECONDS)
+ .minusSeconds(CLOSED_CHANGES_GRACE_TIME_SEC));
+
+ assertThat(changeTs.before(filterCutoffTs)).isTrue();
+ }
+
+ @Test
+ public void testUserWithFilterOutCapabilityShouldSeeJustClosedChangesRefs() throws Exception {
createChangeAndAbandon();
- assertThat(getRefs(cloneProjectChangesRefs(user))).isEmpty();
+ assertThat(getRefs(cloneProjectChangesRefs(user))).isNotEmpty();
}
@Test
@@ -95,9 +130,11 @@
}
@Test
- public void testAdminUserShouldSeeAbandonedChangesRefs() throws Exception {
+ public void testAdminUserShouldSeeAbandonedChangesRefsAfterGracePeriod() throws Exception {
createChangeAndAbandon();
+ waitUntil(() -> getRefsUnchecked(user).isEmpty(), TEST_PATIENCE_TIME);
+
assertThat(getRefs(cloneProjectChangesRefs(admin))).isNotEmpty();
}
@@ -132,7 +169,7 @@
Change.Id changeId = Change.id(createChangeAndAbandon());
Ref metaRef = getMetaId(changeId);
- assertThat(getRefs(cloneProjectChangesRefs(user))).isEmpty();
+ getRefs(cloneProjectChangesRefs(user));
assertThat(changeOpenCache.asMap().size()).isEqualTo(1);
@@ -165,6 +202,40 @@
assertThat(cacheEntry.getValue()).isTrue();
}
+ @Test
+ public void testShouldCacheChangeTsWhenAbandoned() throws Exception {
+ Change.Id changeId = Change.id(createChangeAndAbandon());
+ Ref metaRef = getMetaId(changeId);
+
+ getRefs(cloneProjectChangesRefs(user));
+
+ assertThat(changesTsCache.asMap().size()).isEqualTo(1);
+
+ Map.Entry<ChangeCacheKey, Long> cacheEntry =
+ new ArrayList<>(changesTsCache.asMap().entrySet()).get(0);
+
+ assertThat(cacheEntry.getKey().project()).isEqualTo(project);
+ assertThat(cacheEntry.getKey().changeId()).isEqualTo(changeId);
+ assertThat(cacheEntry.getKey().changeRevision()).isEqualTo(metaRef.getObjectId());
+ assertThat(cacheEntry.getValue())
+ .isEqualTo(gApi.changes().id(changeId.get()).get().updated.getTime());
+ }
+
+ private List<Ref> getRefsUnchecked(TestAccount user) {
+ try {
+ return super.getRefs(cloneProjectChangesRefs(user));
+ } catch (Exception e) {
+ getRefsException = e;
+ return new ArrayList<>();
+ }
+ }
+
+ private void checkGetRefsIsSuccessful() throws Exception {
+ if (getRefsException != null) {
+ throw getRefsException;
+ }
+ }
+
protected Stream<String> fetchAllRefs(TestAccount testAccount) throws Exception {
DfsRepositoryDescription desc = new DfsRepositoryDescription("clone of " + project.get());
