Fix exception when consuming index events of deleted changes
When producing ChangeIndex events, the producing side does not set the
name of the project in the event[1]. This is due to the `onChangeDeleted`
interface, which can only provide the change Id and not the project name
(see here [2]).
This, in turn, causes the global-refdb library to throw an exception,
when attempting to filter projects that the receiver is interested in
consuming (see here[3]), since it cannot tell which project the event is
associated to.
The `ChangeIndexEvent` cannot be hydrated with the name of the project at
production time, because the change has been deleted already, so
attempts to lookup the project name would fail.
The receiving side, on the other hand, can still find the deleted
change in the index and thus recover the name of the project before
asking the global-refdb to match the project name.
[1] https://gerrit.googlesource.com/plugins/multi-site/+/refs/heads/master/src/main/java/com/googlesource/gerrit/plugins/multisite/index/IndexEventHandler.java#136
[2]https://gerrit.googlesource.com/gerrit/+/refs/heads/master/java/com/google/gerrit/extensions/events/ChangeIndexedListener.java#46
[3]https://review.gerrithub.io/plugins/gitiles/GerritForge/global-refdb/+/refs/heads/master/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/ProjectsFilter.java#126
Bug: Issue 15433
Change-Id: I1914eb5b73e5cca5eedfbb47c433b2fc27583329
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/consumer/IndexEventSubscriber.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/consumer/IndexEventSubscriber.java
index 274c34f..480fc50 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/consumer/IndexEventSubscriber.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/consumer/IndexEventSubscriber.java
@@ -15,7 +15,9 @@
package com.googlesource.gerrit.plugins.multisite.consumer;
import com.gerritforge.gerrit.globalrefdb.validation.ProjectsFilter;
+import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.server.change.ChangeFinder;
import com.google.gerrit.server.config.GerritInstanceId;
import com.google.gerrit.server.events.Event;
import com.google.inject.Inject;
@@ -26,10 +28,12 @@
import com.googlesource.gerrit.plugins.multisite.forwarder.events.EventTopic;
import com.googlesource.gerrit.plugins.multisite.forwarder.events.ProjectIndexEvent;
import com.googlesource.gerrit.plugins.multisite.forwarder.router.IndexEventRouter;
+import java.util.Optional;
@Singleton
public class IndexEventSubscriber extends AbstractSubcriber {
private final ProjectsFilter projectsFilter;
+ private final ChangeFinder changeFinder;
@Inject
public IndexEventSubscriber(
@@ -39,9 +43,11 @@
MessageLogger msgLog,
SubscriberMetrics subscriberMetrics,
Configuration cfg,
- ProjectsFilter projectsFilter) {
+ ProjectsFilter projectsFilter,
+ ChangeFinder changeFinder) {
super(eventRouter, droppedEventListeners, instanceId, msgLog, subscriberMetrics, cfg);
this.projectsFilter = projectsFilter;
+ this.changeFinder = changeFinder;
}
@Override
@@ -52,11 +58,24 @@
@Override
protected Boolean shouldConsumeEvent(Event event) {
if (event instanceof ChangeIndexEvent) {
- return projectsFilter.matches(((ChangeIndexEvent) event).projectName);
+ ChangeIndexEvent changeIndexEvent = (ChangeIndexEvent) event;
+ String projectName = changeIndexEvent.projectName;
+ if (isDeletedChangeWithEmptyProject(changeIndexEvent)) {
+ projectName = findProjectFromChangeId(changeIndexEvent.changeId).orElse(projectName);
+ }
+ return projectsFilter.matches(projectName);
}
if (event instanceof ProjectIndexEvent) {
return projectsFilter.matches(((ProjectIndexEvent) event).projectName);
}
return true;
}
+
+ private boolean isDeletedChangeWithEmptyProject(ChangeIndexEvent changeIndexEvent) {
+ return changeIndexEvent.deleted && changeIndexEvent.projectName.isEmpty();
+ }
+
+ private Optional<String> findProjectFromChangeId(int changeId) {
+ return changeFinder.findOne(Change.id(changeId)).map(c -> c.getChange().getProject().get());
+ }
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/consumer/IndexEventSubscriberTest.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/consumer/IndexEventSubscriberTest.java
index 70f811b..0e63528 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/multisite/consumer/IndexEventSubscriberTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/consumer/IndexEventSubscriberTest.java
@@ -14,14 +14,23 @@
package com.googlesource.gerrit.plugins.multisite.consumer;
+import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
import com.google.common.collect.ImmutableList;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.change.ChangeFinder;
import com.google.gerrit.server.events.Event;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.util.time.TimeUtil;
import com.googlesource.gerrit.plugins.multisite.forwarder.CacheNotFoundException;
import com.googlesource.gerrit.plugins.multisite.forwarder.events.AccountIndexEvent;
import com.googlesource.gerrit.plugins.multisite.forwarder.events.ChangeIndexEvent;
@@ -31,11 +40,16 @@
import com.googlesource.gerrit.plugins.multisite.forwarder.router.IndexEventRouter;
import java.io.IOException;
import java.util.List;
+import java.util.Optional;
import org.junit.Test;
+import org.mockito.Mock;
public class IndexEventSubscriberTest extends AbstractSubscriberTestBase {
private static final boolean DELETED = false;
private static final int CHANGE_ID = 1;
+ private static final String EMPTY_PROJECT_NAME = "";
+
+ @Mock protected ChangeFinder changeFinderMock;
@SuppressWarnings("unchecked")
@Test
@@ -50,6 +64,38 @@
verify(droppedEventListeners, never()).onEventDropped(event);
}
+ @SuppressWarnings("unchecked")
+ @Test
+ public void shouldConsumeDeleteChangeIndexEventWithEmptyProjectNameWhenFound()
+ throws IOException, PermissionBackendException, CacheNotFoundException {
+ ChangeIndexEvent event = new ChangeIndexEvent(EMPTY_PROJECT_NAME, CHANGE_ID, true, INSTANCE_ID);
+
+ ChangeNotes changeNotesMock = mock(ChangeNotes.class);
+ when(changeNotesMock.getChange()).thenReturn(newChange());
+ when(changeFinderMock.findOne(any(Change.Id.class))).thenReturn(Optional.of(changeNotesMock));
+ when(projectsFilter.matches(PROJECT_NAME)).thenReturn(true);
+
+ objectUnderTest.getConsumer().accept(event);
+
+ verify(projectsFilter, times(1)).matches(PROJECT_NAME);
+ verify(eventRouter, times(1)).route(event);
+ }
+
+ @SuppressWarnings("unchecked")
+ @Test
+ public void shouldNOTConsumeDeleteChangeIndexEventWithEmptyProjectNameWhenNotFound()
+ throws IOException, PermissionBackendException, CacheNotFoundException {
+ ChangeIndexEvent event = new ChangeIndexEvent("", CHANGE_ID, true, INSTANCE_ID);
+
+ when(changeFinderMock.findOne(any(Change.Id.class))).thenReturn(Optional.empty());
+ when(projectsFilter.matches(EMPTY_PROJECT_NAME)).thenReturn(false);
+
+ objectUnderTest.getConsumer().accept(event);
+
+ verify(projectsFilter, times(1)).matches(EMPTY_PROJECT_NAME);
+ verify(eventRouter, never()).route(event);
+ }
+
@SuppressWarnings("rawtypes")
@Override
protected ForwardedEventRouter eventRouter() {
@@ -72,6 +118,16 @@
msgLog,
subscriberMetrics,
cfg,
- projectsFilter);
+ projectsFilter,
+ changeFinderMock);
+ }
+
+ private Change newChange() {
+ return new Change(
+ Change.key(Integer.toString(CHANGE_ID)),
+ Change.id(CHANGE_ID),
+ Account.id(9999),
+ BranchNameKey.create(Project.nameKey(PROJECT_NAME), "refs/heads/master"),
+ TimeUtil.nowTs());
}
}
