Merge "Split ChangeHookRunner along new interface lines"

gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java

@@ -20,6 +20,7 @@ import static java.nio.charset.StandardCharsets.UTF_8;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
 import com.google.gerrit.common.ChangeHookRunner;
+import com.google.gerrit.common.EventBroker;
 import com.google.gerrit.gpg.GpgModule;
 import com.google.gerrit.httpd.AllRequestFilter;
 import com.google.gerrit.httpd.GerritOptions;
@@ -340,6 +341,7 @@ public class Daemon extends SiteProgram {
 
     modules.add(new WorkQueue.Module());
     modules.add(new ChangeHookRunner.Module());
+    modules.add(new EventBroker.Module());
     modules.add(new ReceiveCommitsExecutorModule());
     modules.add(new DiffExecutorModule());
     modules.add(new MimeUtil2Module());

gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java

@@ -32,7 +32,6 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.AccountState;
 import com.google.gerrit.server.config.AnonymousCowardName;
@@ -44,7 +43,6 @@ import com.google.gerrit.server.data.ChangeAttribute;
 import com.google.gerrit.server.data.PatchSetAttribute;
 import com.google.gerrit.server.data.RefUpdateAttribute;
 import com.google.gerrit.server.events.ChangeAbandonedEvent;
-import com.google.gerrit.server.events.ChangeEvent;
 import com.google.gerrit.server.events.ChangeMergedEvent;
 import com.google.gerrit.server.events.ChangeRestoredEvent;
 import com.google.gerrit.server.events.CommentAddedEvent;
@@ -54,17 +52,12 @@ import com.google.gerrit.server.events.HashtagsChangedEvent;
 import com.google.gerrit.server.events.MergeFailedEvent;
 import com.google.gerrit.server.events.PatchSetCreatedEvent;
 import com.google.gerrit.server.events.ProjectCreatedEvent;
-import com.google.gerrit.server.events.ProjectEvent;
-import com.google.gerrit.server.events.RefEvent;
 import com.google.gerrit.server.events.RefUpdatedEvent;
 import com.google.gerrit.server.events.ReviewerAddedEvent;
 import com.google.gerrit.server.events.TopicChangedEvent;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.WorkQueue;
-import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.project.ProjectControl;
-import com.google.gerrit.server.project.ProjectState;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
@@ -100,8 +93,8 @@ import java.util.concurrent.TimeoutException;
 
 /** Spawns local executables when a hook action occurs. */
 @Singleton
-public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
+public class ChangeHookRunner implements ChangeHooks, LifecycleListener,
-    LifecycleListener, NewProjectCreatedListener {
+    NewProjectCreatedListener {
     /** A logger for this class. */
     private static final Logger log = LoggerFactory.getLogger(ChangeHookRunner.class);
 
@@ -110,7 +103,6 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       protected void configure() {
         bind(ChangeHookRunner.class);
         bind(ChangeHooks.class).to(ChangeHookRunner.class);
-        bind(EventDispatcher.class).to(ChangeHookRunner.class);
         DynamicSet.bind(binder(), NewProjectCreatedListener.class).to(ChangeHookRunner.class);
         listener().to(ChangeHookRunner.class);
       }
@@ -164,13 +156,6 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       }
     }
 
-    /** Listeners to receive changes as they happen (limited by visibility
-     *  of user). */
-    private final DynamicSet<UserScopedEventListener> listeners;
-
-    /** Listeners to receive all changes as they happen. */
-    private final DynamicSet<EventListener> unrestrictedListeners;
-
     /** Path of the new patchset hook. */
     private final Optional<Path> patchsetCreatedHook;
 
@@ -235,7 +220,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
     /** Timeout value for synchronous hooks */
     private final int syncHookTimeout;
 
-    private final ChangeNotes.Factory notesFactory;
+    private final EventDispatcher dispatcher;
 
     /**
      * Create a new ChangeHookRunner.
@@ -255,9 +240,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       ProjectCache projectCache,
       AccountCache accountCache,
       EventFactory eventFactory,
-      DynamicSet<UserScopedEventListener> listeners,
+      EventDispatcher dispatcher) {
-      DynamicSet<EventListener> unrestrictedListeners,
-      ChangeNotes.Factory notesFactory) {
         this.anonymousCowardName = anonymousCowardName;
         this.repoManager = repoManager;
         this.hookQueue = queue.createQueue(1, "hook");
@@ -265,9 +248,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
         this.accountCache = accountCache;
         this.eventFactory = eventFactory;
         this.sitePaths = sitePath;
-        this.listeners = listeners;
+        this.dispatcher = dispatcher;
-        this.unrestrictedListeners = unrestrictedListeners;
-        this.notesFactory = notesFactory;
 
         Path hooksPath;
         String hooksPathConfig = config.getString("hooks", null, "path");
@@ -353,7 +334,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       event.projectName = project.get();
       event.headName = headName;
 
-      fireEvent(project, event);
+      dispatcher.postEvent(project, event);
 
       if (!projectCreatedHook.isPresent()) {
         return;
@@ -378,7 +359,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.uploader = accountAttributeSupplier(uploader);
 
-      fireEvent(change, event, db);
+      dispatcher.postEvent(change, event, db);
 
       if (!patchsetCreatedHook.isPresent()) {
         return;
@@ -415,7 +396,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.uploader = accountAttributeSupplier(uploader);
 
-      fireEvent(change, event, db);
+      dispatcher.postEvent(change, event, db);
 
       if (!draftPublishedHook.isPresent()) {
         return;
@@ -467,7 +448,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
             }
           });
 
-      fireEvent(change, event, db);
+      dispatcher.postEvent(change, event, db);
 
       if (!commentAddedHook.isPresent()) {
         return;
@@ -511,7 +492,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.newRev = mergeResultRev;
 
-      fireEvent(change, event, db);
+      dispatcher.postEvent(change, event, db);
 
       if (!changeMergedHook.isPresent()) {
         return;
@@ -546,7 +527,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.reason = reason;
 
-      fireEvent(change, event, db);
+      dispatcher.postEvent(change, event, db);
 
       if (!mergeFailedHook.isPresent()) {
         return;
@@ -581,7 +562,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.reason = reason;
 
-      fireEvent(change, event, db);
+      dispatcher.postEvent(change, event, db);
 
       if (!changeAbandonedHook.isPresent()) {
         return;
@@ -616,7 +597,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.reason = reason;
 
-      fireEvent(change, event, db);
+      dispatcher.postEvent(change, event, db);
 
       if (!changeRestoredHook.isPresent()) {
         return;
@@ -662,7 +643,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
             }
           });
 
-      fireEvent(refName, event);
+      dispatcher.postEvent(refName, event);
 
       if (!refUpdatedHook.isPresent()) {
         return;
@@ -691,7 +672,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.reviewer = accountAttributeSupplier(account);
 
-      fireEvent(change, event, db);
+      dispatcher.postEvent(change, event, db);
 
       if (!reviewerAddedHook.isPresent()) {
         return;
@@ -721,7 +702,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       event.changer = accountAttributeSupplier(account);
       event.oldTopic = oldTopic;
 
-      fireEvent(change, event, db);
+      dispatcher.postEvent(change, event, db);
 
       if (!topicChangedHook.isPresent()) {
         return;
@@ -762,7 +743,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       event.added = hashtagArray(added);
       event.removed = hashtagArray(removed);
 
-      fireEvent(change, event, db);
+      dispatcher.postEvent(change, event, db);
 
       if (!hashtagsChangedHook.isPresent()) {
         return;
@@ -810,28 +791,6 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       }
     }
 
-    @Override
-    public void postEvent(Change change, ChangeEvent event, ReviewDb db)
-        throws OrmException {
-      fireEvent(change, event, db);
-    }
-
-    @Override
-    public void postEvent(Branch.NameKey branchName, RefEvent event) {
-      fireEvent(branchName, event);
-    }
-
-    @Override
-    public void postEvent(Project.NameKey projectName, ProjectEvent event) {
-      fireEvent(projectName, event);
-    }
-
-    @Override
-    public void postEvent(com.google.gerrit.server.events.Event event,
-        ReviewDb db) throws OrmException {
-      fireEvent(event, db);
-    }
-
     private Supplier<AccountState> getAccountSupplier(
         final Account.Id account) {
       return Suppliers.memoize(
@@ -894,100 +853,6 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
           });
     }
 
-    private void fireEventForUnrestrictedListeners(com.google.gerrit.server.events.Event event) {
-      for (EventListener listener : unrestrictedListeners) {
-        listener.onEvent(event);
-      }
-    }
-
-    private void fireEvent(Change change, ChangeEvent event, ReviewDb db)
-        throws OrmException {
-      for (UserScopedEventListener listener : listeners) {
-        if (isVisibleTo(change, listener.getUser(), db)) {
-          listener.onEvent(event);
-        }
-      }
-
-      fireEventForUnrestrictedListeners( event );
-    }
-
-    private void fireEvent(Project.NameKey project, ProjectEvent event) {
-      for (UserScopedEventListener listener : listeners) {
-        if (isVisibleTo(project, listener.getUser())) {
-          listener.onEvent(event);
-        }
-      }
-
-      fireEventForUnrestrictedListeners(event);
-    }
-
-    private void fireEvent(Branch.NameKey branchName, RefEvent event) {
-      for (UserScopedEventListener listener : listeners) {
-        if (isVisibleTo(branchName, listener.getUser())) {
-          listener.onEvent(event);
-        }
-      }
-
-      fireEventForUnrestrictedListeners(event);
-    }
-
-    private void fireEvent(com.google.gerrit.server.events.Event event,
-        ReviewDb db) throws OrmException {
-      for (UserScopedEventListener listener : listeners) {
-        if (isVisibleTo(event, listener.getUser(), db)) {
-          listener.onEvent(event);
-        }
-      }
-
-      fireEventForUnrestrictedListeners(event);
-    }
-
-    private boolean isVisibleTo(Project.NameKey project, CurrentUser user) {
-      ProjectState pe = projectCache.get(project);
-      if (pe == null) {
-        return false;
-      }
-      return pe.controlFor(user).isVisible();
-    }
-
-    private boolean isVisibleTo(Change change, CurrentUser user, ReviewDb db)
-        throws OrmException {
-      if (change == null) {
-        return false;
-      }
-      ProjectState pe = projectCache.get(change.getProject());
-      if (pe == null) {
-        return false;
-      }
-      ProjectControl pc = pe.controlFor(user);
-      return pc.controlFor(db, change).isVisible(db);
-    }
-
-    private boolean isVisibleTo(Branch.NameKey branchName, CurrentUser user) {
-      ProjectState pe = projectCache.get(branchName.getParentKey());
-      if (pe == null) {
-        return false;
-      }
-      ProjectControl pc = pe.controlFor(user);
-      return pc.controlForRef(branchName).isVisible();
-    }
-
-    private boolean isVisibleTo(com.google.gerrit.server.events.Event event,
-        CurrentUser user, ReviewDb db) throws OrmException {
-      if (event instanceof RefEvent) {
-        RefEvent refEvent = (RefEvent) event;
-        String ref = refEvent.getRefName();
-        if (PatchSet.isChangeRef(ref)) {
-          Change.Id cid = PatchSet.Id.fromRef(ref).getParentKey();
-          Change change = notesFactory
-              .create(db, refEvent.getProjectNameKey(), cid).getChange();
-          return isVisibleTo(change, user, db);
-        }
-        return isVisibleTo(refEvent.getBranchNameKey(), user);
-      }
-      return true;
-    }
-
     /**
      * Create an ApprovalAttribute for the given approval suitable for serialization to JSON.
      * @param approval

gerrit-server/src/main/java/com/google/gerrit/common/EventBroker.java

@@ -0,0 +1,181 @@
+// Copyright (C) 2016 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.google.gerrit.common;
+
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.lifecycle.LifecycleModule;
+import com.google.gerrit.reviewdb.client.Branch;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.events.ChangeEvent;
+import com.google.gerrit.server.events.Event;
+import com.google.gerrit.server.events.ProjectEvent;
+import com.google.gerrit.server.events.RefEvent;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectControl;
+import com.google.gerrit.server.project.ProjectState;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+
+/** Distributes Events to listeners if they are allowed to see them */
+@Singleton
+public class EventBroker implements EventDispatcher {
+
+  public static class Module extends LifecycleModule {
+    @Override
+    protected void configure() {
+      bind(EventDispatcher.class).to(EventBroker.class);
+    }
+  }
+
+  /**
+   * Listeners to receive changes as they happen (limited by visibility of
+   * user).
+   */
+  private final DynamicSet<UserScopedEventListener> listeners;
+
+  /** Listeners to receive all changes as they happen. */
+  private final DynamicSet<EventListener> unrestrictedListeners;
+
+  private final ProjectCache projectCache;
+
+  private final ChangeNotes.Factory notesFactory;
+
+  @Inject
+  public EventBroker(DynamicSet<UserScopedEventListener> listeners,
+      DynamicSet<EventListener> unrestrictedListeners,
+      ProjectCache projectCache,
+      ChangeNotes.Factory notesFactory) {
+    this.listeners = listeners;
+    this.unrestrictedListeners = unrestrictedListeners;
+    this.projectCache = projectCache;
+    this.notesFactory = notesFactory;
+  }
+
+  @Override
+  public void postEvent(Change change, ChangeEvent event, ReviewDb db)
+      throws OrmException {
+    fireEvent(change, event, db);
+  }
+
+  @Override
+  public void postEvent(Branch.NameKey branchName, RefEvent event) {
+    fireEvent(branchName, event);
+  }
+
+  @Override
+  public void postEvent(Project.NameKey projectName, ProjectEvent event) {
+    fireEvent(projectName, event);
+  }
+
+  @Override
+  public void postEvent(Event event, ReviewDb db) throws OrmException {
+    fireEvent(event, db);
+  }
+
+  private void fireEventForUnrestrictedListeners(Event event) {
+    for (EventListener listener : unrestrictedListeners) {
+      listener.onEvent(event);
+    }
+  }
+
+  protected void fireEvent(Change change, ChangeEvent event, ReviewDb db)
+      throws OrmException {
+    for (UserScopedEventListener listener : listeners) {
+      if (isVisibleTo(change, listener.getUser(), db)) {
+        listener.onEvent(event);
+      }
+    }
+    fireEventForUnrestrictedListeners(event);
+  }
+
+  protected void fireEvent(Project.NameKey project, ProjectEvent event) {
+    for (UserScopedEventListener listener : listeners) {
+      if (isVisibleTo(project, listener.getUser())) {
+        listener.onEvent(event);
+      }
+    }
+    fireEventForUnrestrictedListeners(event);
+  }
+
+  protected void fireEvent(Branch.NameKey branchName, RefEvent event) {
+    for (UserScopedEventListener listener : listeners) {
+      if (isVisibleTo(branchName, listener.getUser())) {
+        listener.onEvent(event);
+      }
+    }
+    fireEventForUnrestrictedListeners(event);
+  }
+
+  protected void fireEvent(Event event, ReviewDb db) throws OrmException {
+    for (UserScopedEventListener listener : listeners) {
+      if (isVisibleTo(event, listener.getUser(), db)) {
+        listener.onEvent(event);
+      }
+    }
+    fireEventForUnrestrictedListeners(event);
+  }
+
+  protected boolean isVisibleTo(Project.NameKey project, CurrentUser user) {
+    ProjectState pe = projectCache.get(project);
+    if (pe == null) {
+      return false;
+    }
+    return pe.controlFor(user).isVisible();
+  }
+
+  protected boolean isVisibleTo(Change change, CurrentUser user, ReviewDb db)
+      throws OrmException {
+    if (change == null) {
+      return false;
+    }
+    ProjectState pe = projectCache.get(change.getProject());
+    if (pe == null) {
+      return false;
+    }
+    ProjectControl pc = pe.controlFor(user);
+    return pc.controlFor(db, change).isVisible(db);
+  }
+
+  protected boolean isVisibleTo(Branch.NameKey branchName, CurrentUser user) {
+    ProjectState pe = projectCache.get(branchName.getParentKey());
+    if (pe == null) {
+      return false;
+    }
+    ProjectControl pc = pe.controlFor(user);
+    return pc.controlForRef(branchName).isVisible();
+  }
+
+  protected boolean isVisibleTo(Event event, CurrentUser user, ReviewDb db)
+      throws OrmException {
+    if (event instanceof RefEvent) {
+      RefEvent refEvent = (RefEvent) event;
+      String ref = refEvent.getRefName();
+      if (PatchSet.isChangeRef(ref)) {
+        Change.Id cid = PatchSet.Id.fromRef(ref).getParentKey();
+        Change change = notesFactory
+            .create(db, refEvent.getProjectNameKey(), cid).getChange();
+        return isVisibleTo(change, user, db);
+      }
+      return isVisibleTo(refEvent.getBranchNameKey(), user);
+    }
+    return true;
+  }
+}

gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java

@@ -19,6 +19,7 @@ import static com.google.inject.Stage.PRODUCTION;
 
 import com.google.common.base.Splitter;
 import com.google.gerrit.common.ChangeHookRunner;
+import com.google.gerrit.common.EventBroker;
 import com.google.gerrit.gpg.GpgModule;
 import com.google.gerrit.httpd.auth.oauth.OAuthModule;
 import com.google.gerrit.httpd.auth.openid.OpenIdModule;
@@ -294,6 +295,7 @@ public class WebAppInitializer extends GuiceServletContextListener
   private Injector createSysInjector() {
     final List<Module> modules = new ArrayList<>();
     modules.add(new DropWizardMetricMaker.RestModule());
+    modules.add(new EventBroker.Module());
     modules.add(new ChangeHookRunner.Module());
     modules.add(new ReceiveCommitsExecutorModule());
     modules.add(new DiffExecutorModule());