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());