Exclude repo from ChangeCacheKey equals/hash code calculation
Without this change ChangeCacheKey.repo field is a part
of the equals/hash code calculation. Other Gerrit libraries can
provide wrappers around repository instance(for example global-refdb
wraps repository with SharedRefDbRepository). This means that we
cannot guarantee that repo object is the same instance for all
ChangeCacheKey entries. This will cause the constant cache entries
reloading.
Bug: Issue 16638
Change-Id: Ie86d249741cb1770b6ca22f2f7ed30655a142ed4
diff --git a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangeCacheKey.java b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangeCacheKey.java
index 9a6b0d6..291ec65 100644
--- a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangeCacheKey.java
+++ b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangeCacheKey.java
@@ -18,6 +18,7 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
+import java.util.concurrent.atomic.AtomicReference;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
@@ -25,7 +26,11 @@
public abstract class ChangeCacheKey {
/* `repo` and `changeId` need to be part of the cache key because the
Loader requires them in order to fetch the relevant changeNote. */
- public abstract Repository repo();
+ private final AtomicReference<Repository> repo = new AtomicReference<>();
+
+ public final Repository repo() {
+ return repo.get();
+ }
public abstract Change.Id changeId();
@@ -39,6 +44,8 @@
Change.Id changeId,
@Nullable ObjectId changeRevision,
Project.NameKey project) {
- return new AutoValue_ChangeCacheKey(repo, changeId, changeRevision, project);
+ ChangeCacheKey cacheKey = new AutoValue_ChangeCacheKey(changeId, changeRevision, project);
+ cacheKey.repo.set(repo);
+ return cacheKey;
}
}
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 1fd85c1..c47ebe7 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
@@ -191,10 +191,24 @@
assertThat(cacheEntry.getKey().project()).isEqualTo(project);
assertThat(cacheEntry.getKey().changeId()).isEqualTo(changeId);
assertThat(cacheEntry.getKey().changeRevision()).isEqualTo(metaRef.getObjectId());
+ assertThat(cacheEntry.getKey().repo()).isNotNull();
assertThat(cacheEntry.getValue()).isFalse();
}
@Test
+ public void testShouldCacheChangeKeyContainRepoAfterDeserializing() throws Exception {
+ Change.Id changeId = Change.id(createChangeAndAbandon());
+ getRefs(cloneProjectChangesRefs(user));
+
+ assertThat(changeOpenCache.asMap().size()).isEqualTo(1);
+
+ Map.Entry<ChangeCacheKey, Boolean> cacheEntry =
+ new ArrayList<>(changeOpenCache.asMap().entrySet()).get(0);
+
+ assertThat(cacheEntry.getKey().repo()).isNotNull();
+ }
+
+ @Test
public void testShouldCacheWhenChangeIsOpen() throws Exception {
createChange();
List<Ref> refs = getRefs(cloneProjectChangesRefs(user));
diff --git a/src/test/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangeCacheKeyTest.java b/src/test/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangeCacheKeyTest.java
new file mode 100644
index 0000000..eae3624
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangeCacheKeyTest.java
@@ -0,0 +1,59 @@
+// 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.modules.gitrefsfilter;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.Project.NameKey;
+import java.io.IOException;
+import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+import org.junit.Test;
+
+public class ChangeCacheKeyTest {
+ private static final String REPO_NAME = "test_repo";
+ private static final Change.Id ID = Change.id(10000);
+ private static final ObjectId CHANGE_REVISION = ObjectId.zeroId();
+ private static final NameKey TEST_REPO = Project.nameKey(REPO_NAME);
+
+ @Test
+ public void shouldExcludeRepoFieldDuringEqualsCalculation() throws IOException {
+ ChangeCacheKey cacheKey1 =
+ ChangeCacheKey.create(newRepository(), ID, CHANGE_REVISION, TEST_REPO);
+ ChangeCacheKey cacheKey2 =
+ ChangeCacheKey.create(newRepository(), ID, CHANGE_REVISION, TEST_REPO);
+ assertThat(cacheKey1).isEqualTo(cacheKey2);
+ }
+
+ @Test
+ public void shouldExcludeRepoFieldDuringHashCodeCalculation() throws IOException {
+ try (Repository repo1 = newRepository();
+ Repository repo2 = newRepository()) {
+ ChangeCacheKey cacheKey1 = ChangeCacheKey.create(repo1, ID, CHANGE_REVISION, TEST_REPO);
+ ChangeCacheKey cacheKey2 = ChangeCacheKey.create(repo2, ID, CHANGE_REVISION, TEST_REPO);
+ assertThat(cacheKey1.hashCode()).isEqualTo(cacheKey2.hashCode());
+ }
+ }
+
+ private Repository newRepository() throws IOException {
+ return new InMemoryRepository.Builder()
+ .setRepositoryDescription(new DfsRepositoryDescription(REPO_NAME))
+ .build();
+ }
+}
