Process events before scheduling

It's confusing for admins to see tasks scheduled for event types that
listeners haven't configured (or that haven't been globally allowed).
There's very little additional work done in the processing step and the
work saved by not scheduling unwanted events probably more than makes up
for it.

Release-Notes: Skip scheduling tasks for events that should not be processed
Change-Id: I8471a29662d1b88a15e29c5cb6e7e40e25b242f3

src/main/java/com/googlesource/gerrit/plugins/webhooks/EventHandler.java

diff --git a/src/main/java/com/googlesource/gerrit/plugins/webhooks/EventHandler.java b/src/main/java/com/googlesource/gerrit/plugins/webhooks/EventHandler.java
index 3cf1f9c..489bec8 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/EventHandler.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/EventHandler.java
@@ -25,6 +25,7 @@
 import com.google.gerrit.server.events.ProjectEvent;
 import com.google.gerrit.server.project.NoSuchProjectException;
 import com.google.inject.Inject;
+import java.util.Optional;
 import org.eclipse.jgit.lib.Config;
 
 class EventHandler implements EventListener {
@@ -35,6 +36,7 @@
   private final String pluginName;
   private final RemoteConfig.Factory remoteFactory;
   private final PostTask.Factory taskFactory;
+  private final EventProcessor processor;
 
   @Inject
   EventHandler(
@@ -42,12 +44,14 @@
       PluginConfigFactory configFactory,
       @PluginName String pluginName,
       RemoteConfig.Factory remoteFactory,
-      PostTask.Factory taskFactory) {
+      PostTask.Factory taskFactory,
+      EventProcessor processor) {
     this.global = global;
     this.configFactory = configFactory;
     this.pluginName = pluginName;
     this.remoteFactory = remoteFactory;
     this.taskFactory = taskFactory;
+    this.processor = processor;
   }
 
   @Override
@@ -80,7 +84,13 @@
             "remote.%s.url does not match any allowed URL patterns, skipping this remote", name);
         continue;
       }
-      taskFactory.create(projectEvent, remote).schedule();
+      Optional<EventProcessor.Request> content = processor.process(projectEvent, remote);
+      if (content.isEmpty()) {
+        log.atFine().log(
+            "No content (rejected by processing). Webhook [%s] skipped.", remote.getUrl());
+        continue;
+      }
+      taskFactory.create(projectEvent, remote, content.get()).schedule();
     }
   }
 

src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java

diff --git a/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java b/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java
index 844f87a..4eea44d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java
@@ -22,7 +22,6 @@
 import com.google.inject.assistedinject.AssistedInject;
 import com.googlesource.gerrit.plugins.webhooks.HttpResponseHandler.HttpResult;
 import java.io.IOException;
-import java.util.Optional;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
@@ -32,30 +31,30 @@
   private static final FluentLogger log = FluentLogger.forEnclosingClass();
 
   interface Factory {
-    PostTask create(ProjectEvent event, RemoteConfig remote);
+    PostTask create(ProjectEvent event, RemoteConfig remote, EventProcessor.Request content);
   }
 
   private final ScheduledExecutorService executor;
   private final Supplier<HttpSession> session;
   private final RemoteConfig remote;
   private final ProjectEvent event;
-  private final Supplier<Optional<EventProcessor.Request>> processor;
+  private final EventProcessor.Request content;
   private int execCnt;
 
   @AssistedInject
   public PostTask(
       @WebHooksExecutor ScheduledExecutorService executor,
       HttpSession.Factory session,
-      EventProcessor processor,
       @Assisted ProjectEvent event,
-      @Assisted RemoteConfig remote) {
+      @Assisted RemoteConfig remote,
+      @Assisted EventProcessor.Request content) {
     this.executor = executor;
     this.event = event;
     this.remote = remote;
+    this.content = content;
     // postpone creation of HttpSession so that it is obtained only when processor
     // returns non-empty content
     this.session = Suppliers.memoize(() -> session.create(remote));
-    this.processor = Suppliers.memoize(() -> processor.process(event, remote));
   }
 
   void schedule() {
@@ -71,14 +70,8 @@
   @Override
   public void run() {
     try {
-      Optional<EventProcessor.Request> content = processor.get();
-      if (!content.isPresent()) {
-        log.atFine().log("No content. Webhook [%s] skipped.", remote.getUrl());
-        return;
-      }
-
       execCnt++;
-      HttpResult result = session.get().post(remote, content.get());
+      HttpResult result = session.get().post(remote, content);
       if (!result.successful) {
         if (execCnt < remote.getMaxTries()) {
           logRetry(result.message);

src/test/java/com/googlesource/gerrit/plugins/webhooks/EventHandlerTest.java

diff --git a/src/test/java/com/googlesource/gerrit/plugins/webhooks/EventHandlerTest.java b/src/test/java/com/googlesource/gerrit/plugins/webhooks/EventHandlerTest.java
index 2aae9af..5d4eaf4 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/webhooks/EventHandlerTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/webhooks/EventHandlerTest.java
@@ -27,6 +27,7 @@
 import com.google.gerrit.server.events.Event;
 import com.google.gerrit.server.events.ProjectCreatedEvent;
 import com.google.gerrit.server.project.NoSuchProjectException;
+import java.util.Optional;
 import java.util.regex.Pattern;
 import org.eclipse.jgit.lib.Config;
 import org.junit.Before;
@@ -59,6 +60,10 @@
 
   @Mock private Config config;
 
+  @Mock private EventProcessor processor;
+
+  @Mock private EventProcessor.Request content;
+
   private EventHandler eventHandler;
 
   @Before
@@ -67,8 +72,10 @@
     when(configFactory.getProjectPluginConfigWithInheritance(PROJECT_NAME, PLUGIN))
         .thenReturn(config);
     when(remoteFactory.create(eq(config), eq(FOO))).thenReturn(remote);
-    when(taskFactory.create(eq(projectCreated), eq(remote))).thenReturn(postTask);
-    eventHandler = new EventHandler(global, configFactory, PLUGIN, remoteFactory, taskFactory);
+    when(processor.process(eq(projectCreated), eq(remote))).thenReturn(Optional.of(content));
+    when(taskFactory.create(eq(projectCreated), eq(remote), eq(content))).thenReturn(postTask);
+    eventHandler =
+        new EventHandler(global, configFactory, PLUGIN, remoteFactory, taskFactory, processor);
   }
 
   @Test
@@ -85,7 +92,7 @@
     when(remote.getUrl()).thenReturn(FOO_URL);
 
     eventHandler.onEvent(projectCreated);
-    verify(taskFactory, times(1)).create(eq(projectCreated), eq(remote));
+    verify(taskFactory, times(1)).create(eq(projectCreated), eq(remote), eq(content));
     verify(postTask, times(1)).schedule();
   }
 
@@ -105,7 +112,7 @@
     when(remote.getUrl()).thenReturn(FOO_URL);
 
     eventHandler.onEvent(projectCreated);
-    verify(taskFactory, times(1)).create(eq(projectCreated), eq(remote));
+    verify(taskFactory, times(1)).create(eq(projectCreated), eq(remote), eq(content));
     verify(postTask, times(1)).schedule();
   }
 
@@ -120,4 +127,15 @@
     verifyNoInteractions(taskFactory);
     verifyNoInteractions(postTask);
   }
+
+  @Test
+  public void noScheduleOnEmptyBody() throws Exception {
+    when(config.getSubsections(eq(REMOTE))).thenReturn(ImmutableSet.of(FOO));
+    when(remote.getUrl()).thenReturn(FOO_URL);
+    when(processor.process(eq(projectCreated), eq(remote))).thenReturn(Optional.empty());
+
+    eventHandler.onEvent(projectCreated);
+    verifyNoInteractions(taskFactory);
+    verifyNoInteractions(postTask);
+  }
 }

src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java

diff --git a/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java b/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java
index 54aac2f..572ebe8 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java
@@ -24,7 +24,6 @@
 import com.google.gerrit.server.events.ProjectCreatedEvent;
 import com.googlesource.gerrit.plugins.webhooks.HttpResponseHandler.HttpResult;
 import java.io.IOException;
-import java.util.Optional;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ScheduledThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
@@ -54,8 +53,6 @@
 
   @Mock private ScheduledThreadPoolExecutor executor;
 
-  @Mock private EventProcessor processor;
-
   @Mock private EventProcessor.Request content;
 
   private PostTask task;
@@ -65,18 +62,9 @@
     when(remote.getRetryInterval()).thenReturn(RETRY_INTERVAL);
     when(remote.getMaxTries()).thenReturn(MAX_TRIES);
     when(remote.getUrl()).thenReturn(WEBHOOK_URL);
-    when(processor.process(eq(projectCreated), eq(remote))).thenReturn(Optional.of(content));
     when(sessionFactory.create(eq(remote))).thenReturn(session);
     when(projectCreated.getProjectNameKey()).thenReturn(Project.nameKey("test"));
-    task = new PostTask(executor, sessionFactory, processor, projectCreated, remote);
-  }
-
-  @Test
-  public void noScheduleOnEmptyBody() throws Exception {
-    when(processor.process(eq(projectCreated), eq(remote))).thenReturn(Optional.empty());
-    task.run();
-    verifyNoInteractions(session);
-    verifyNoInteractions(executor);
+    task = new PostTask(executor, sessionFactory, projectCreated, remote, content);
   }
 
   @Test