ZK compareAndPut should alway ignore the ignored refs
Ingored Refs should never be persisted in the shared ref-db
even when newRef is a NullRef.
Change-Id: I2e993a9b424f84978db084024850eee90e70bf43
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/SharedRefDatabase.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/SharedRefDatabase.java
index b475a72..87ebc96 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/SharedRefDatabase.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/SharedRefDatabase.java
@@ -120,11 +120,10 @@
/**
* Some references should not be stored in the SharedRefDatabase.
*
- * @param ref
+ * @param refName
* @return true if it's to be ignore; false otherwise
*/
- default boolean ignoreRefInSharedDb(Ref ref) {
- String refName = ref.getName();
+ default boolean ignoreRefInSharedDb(String refName) {
return refName == null
|| refName.startsWith("refs/draft-comments")
|| (refName.startsWith("refs/changes") && !refName.endsWith("/meta"));
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/zookeeper/ZkSharedRefDatabase.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/zookeeper/ZkSharedRefDatabase.java
index 97f6be5..503d93f 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/zookeeper/ZkSharedRefDatabase.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/zookeeper/ZkSharedRefDatabase.java
@@ -43,12 +43,12 @@
@Override
public boolean compareAndRemove(String project, Ref oldRef) throws IOException {
- return ignoreRefInSharedDb(oldRef) || compareAndPut(project, oldRef, NULL_REF);
+ return compareAndPut(project, oldRef, NULL_REF);
}
@Override
public boolean compareAndPut(String projectName, Ref oldRef, Ref newRef) throws IOException {
- if (newRef != NULL_REF && ignoreRefInSharedDb(newRef)) {
+ if (ignoreRefInSharedDb(MoreObjects.firstNonNull(oldRef.getName(), newRef.getName()))) {
return true;
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/zookeeper/ZkSharedRefDatabaseTest.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/zookeeper/ZkSharedRefDatabaseTest.java
index bcdf475..c3f3ace 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/zookeeper/ZkSharedRefDatabaseTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/zookeeper/ZkSharedRefDatabaseTest.java
@@ -161,32 +161,51 @@
@Test
public void anImmutableChangeShouldBeIgnored() {
Ref immutableChangeRef = zkSharedRefDatabase.newRef(A_REF_NAME_OF_A_PATCHSET, AN_OBJECT_ID_1);
- assertThat(zkSharedRefDatabase.ignoreRefInSharedDb(immutableChangeRef)).isTrue();
+ assertThat(zkSharedRefDatabase.ignoreRefInSharedDb(immutableChangeRef.getName())).isTrue();
}
@Test
public void aChangeMetaShouldNotBeIgnored() {
Ref immutableChangeRef = zkSharedRefDatabase.newRef("refs/changes/01/1/meta", AN_OBJECT_ID_1);
- assertThat(zkSharedRefDatabase.ignoreRefInSharedDb(immutableChangeRef)).isFalse();
+ assertThat(zkSharedRefDatabase.ignoreRefInSharedDb(immutableChangeRef.getName())).isFalse();
}
@Test
public void aDraftCommentsShouldBeIgnored() {
Ref immutableChangeRef =
zkSharedRefDatabase.newRef("refs/draft-comments/01/1/1000000", AN_OBJECT_ID_1);
- assertThat(zkSharedRefDatabase.ignoreRefInSharedDb(immutableChangeRef)).isTrue();
+ assertThat(zkSharedRefDatabase.ignoreRefInSharedDb(immutableChangeRef.getName())).isTrue();
}
@Test
public void regularRefHeadsMasterShouldNotBeIgnored() {
Ref immutableChangeRef = zkSharedRefDatabase.newRef("refs/heads/master", AN_OBJECT_ID_1);
- assertThat(zkSharedRefDatabase.ignoreRefInSharedDb(immutableChangeRef)).isFalse();
+ assertThat(zkSharedRefDatabase.ignoreRefInSharedDb(immutableChangeRef.getName())).isFalse();
}
@Test
public void regularCommitShouldNotBeIgnored() {
Ref immutableChangeRef = zkSharedRefDatabase.newRef("refs/heads/stable-2.16", AN_OBJECT_ID_1);
- assertThat(zkSharedRefDatabase.ignoreRefInSharedDb(immutableChangeRef)).isFalse();
+ assertThat(zkSharedRefDatabase.ignoreRefInSharedDb(immutableChangeRef.getName())).isFalse();
+ }
+
+ @Test
+ public void compareAndPutShouldAlwaysIngoreAlwaysDraftCommentsEvenOutOfOrder() throws Exception {
+ // Test to reproduce a production bug where ignored refs were persisted in ZK because
+ // newRef == NULL
+ Ref existingRef =
+ zkSharedRefDatabase.newRef("refs/draft-comments/56/450756/1013728", AN_OBJECT_ID_1);
+ Ref oldRefToIgnore =
+ zkSharedRefDatabase.newRef("refs/draft-comments/56/450756/1013728", AN_OBJECT_ID_2);
+ Ref nullRef = SharedRefDatabase.NULL_REF;
+ String projectName = A_TEST_PROJECT_NAME;
+
+ // This ref should be ignored even if newRef is null
+ assertThat(zkSharedRefDatabase.compareAndPut(A_TEST_PROJECT_NAME, existingRef, nullRef))
+ .isTrue();
+
+ // This ignored ref should also be ignored
+ assertThat(zkSharedRefDatabase.compareAndPut(projectName, oldRefToIgnore, nullRef)).isTrue();
}
@Override
