Preserve instanceId on event when already set

Plugins can inject external events into Gerrit
also coming from other remote instances. Preserve
the instanceId contained in the forwarded event
if set and do not overwrite with the local Gerrit one.

Only events that are locally generated in Gerrit
can set the intanceId but not the forwarded ones.

Failing to preserve the original instanceId of the
forwarded events may cause events duplication, like
the use-case of the high-availability plugin not
recognising a remote event because it is re-stamped
with the local instanceId.

Bug: Issue 13307
Change-Id: I324a48a6f2ffca59bad059e18550a8dc44224f34

java/com/google/gerrit/server/events/EventBroker.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.events;
 
+import com.google.common.base.Strings;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.entities.BranchNameKey;
@@ -111,7 +112,7 @@ public class EventBroker implements EventDispatcher {
   }
 
   protected void fireEvent(Change change, ChangeEvent event) throws PermissionBackendException {
-    setInstanceId(event);
+    setInstanceIdWhenEmpty(event);
     for (PluginSetEntryContext<UserScopedEventListener> c : listeners) {
       CurrentUser user = c.call(UserScopedEventListener::getUser);
       if (isVisibleTo(change, user)) {
@@ -122,7 +123,7 @@ public class EventBroker implements EventDispatcher {
   }
 
   protected void fireEvent(Project.NameKey project, ProjectEvent event) {
-    setInstanceId(event);
+    setInstanceIdWhenEmpty(event);
     for (PluginSetEntryContext<UserScopedEventListener> c : listeners) {
 
       CurrentUser user = c.call(UserScopedEventListener::getUser);
@@ -135,7 +136,7 @@ public class EventBroker implements EventDispatcher {
 
   protected void fireEvent(BranchNameKey branchName, RefEvent event)
       throws PermissionBackendException {
-    setInstanceId(event);
+    setInstanceIdWhenEmpty(event);
     for (PluginSetEntryContext<UserScopedEventListener> c : listeners) {
       CurrentUser user = c.call(UserScopedEventListener::getUser);
       if (isVisibleTo(branchName, user)) {
@@ -146,7 +147,7 @@ public class EventBroker implements EventDispatcher {
   }
 
   protected void fireEvent(Event event) throws PermissionBackendException {
-    setInstanceId(event);
+    setInstanceIdWhenEmpty(event);
     for (PluginSetEntryContext<UserScopedEventListener> c : listeners) {
       CurrentUser user = c.call(UserScopedEventListener::getUser);
       if (isVisibleTo(event, user)) {
@@ -156,8 +157,10 @@ public class EventBroker implements EventDispatcher {
     fireEventForUnrestrictedListeners(event);
   }
 
-  protected void setInstanceId(Event event) {
+  protected void setInstanceIdWhenEmpty(Event event) {
-    event.instanceId = gerritInstanceId;
+    if (Strings.isNullOrEmpty(event.instanceId)) {
+      event.instanceId = gerritInstanceId;
+    }
   }
 
   protected boolean isVisibleTo(Project.NameKey project, CurrentUser user) {

javatests/com/google/gerrit/acceptance/config/InstanceIdFromPluginIT.java

@@ -19,10 +19,19 @@ import static com.google.common.truth.Truth.assertThat;
 import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
 import com.google.gerrit.acceptance.TestPlugin;
 import com.google.gerrit.common.Nullable;
+import com.google.gerrit.extensions.registration.DynamicItem;
+import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.server.config.GerritInstanceId;
+import com.google.gerrit.server.events.Event;
+import com.google.gerrit.server.events.EventDispatcher;
+import com.google.gerrit.server.events.EventListener;
+import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.inject.AbstractModule;
 import com.google.inject.Inject;
+import com.google.inject.Key;
 import com.google.inject.Scopes;
+import java.util.ArrayList;
+import java.util.List;
 import org.junit.Test;
 
 @TestPlugin(
@@ -35,6 +44,8 @@ public class InstanceIdFromPluginIT extends LightweightPluginDaemonTest {
     @Override
     protected void configure() {
       bind(InstanceIdLoader.class).in(Scopes.SINGLETON);
+      bind(TestEventListener.class).in(Scopes.SINGLETON);
+      DynamicSet.bind(binder(), EventListener.class).to(TestEventListener.class);
     }
   }
 
@@ -47,6 +58,27 @@ public class InstanceIdFromPluginIT extends LightweightPluginDaemonTest {
     }
   }
 
+  public static class TestEventListener implements EventListener {
+    private final List<Event> events = new ArrayList<>();
+
+    @Override
+    public void onEvent(Event event) {
+      events.add(event);
+    }
+
+    public List<Event> getEvents() {
+      return events;
+    }
+  }
+
+  public static class TestEvent extends Event {
+
+    protected TestEvent(String instanceId) {
+      super("test");
+      this.instanceId = instanceId;
+    }
+  }
+
   @Test
   @GerritConfig(name = "gerrit.instanceId", value = "testInstanceId")
   public void shouldReturnInstanceIdWhenDefined() {
@@ -58,6 +90,21 @@ public class InstanceIdFromPluginIT extends LightweightPluginDaemonTest {
     assertThat(getInstanceIdLoader().gerritInstanceId).isNull();
   }
 
+  @Test
+  @GerritConfig(name = "gerrit.instanceId", value = "testInstanceId")
+  public void shouldPreserveEventInstanceIdWhenDefined() throws PermissionBackendException {
+    EventDispatcher dispatcher =
+        plugin.getSysInjector().getInstance(new Key<DynamicItem<EventDispatcher>>() {}).get();
+    String eventInstanceId = "eventInstanceId";
+    TestEventListener eventListener = plugin.getSysInjector().getInstance(TestEventListener.class);
+    TestEvent testEvent = new TestEvent(eventInstanceId);
+
+    dispatcher.postEvent(testEvent);
+    List<Event> receivedEvents = eventListener.getEvents();
+    assertThat(receivedEvents).hasSize(1);
+    assertThat(receivedEvents.get(0).instanceId).isEqualTo(eventInstanceId);
+  }
+
   private InstanceIdLoader getInstanceIdLoader() {
     return plugin.getSysInjector().getInstance(InstanceIdLoader.class);
   }