From bef6c3de3e38722a5493928af17c25ca3020168f Mon Sep 17 00:00:00 2001 From: Martin Fick Date: Thu, 11 Dec 2014 17:26:10 -0700 Subject: [PATCH] Split the ChangeHooks interface in 2 The current ChangeHook interface supports methods for event creators and listeners. Since creators and listeners are unlikely to be the same classes, it does not make sense to expose both sets of methods to each type of user. While this change will clean things up API wise, it also paves the way to split out some event firing logic from the ChangeHooks class. By splitting this logic it provides a better path to make it possible for other classes such as a plugins to fire events. Change-Id: I0906c86fa6ea32aa519b452134d2caddf489df09 --- .../rest/change/AbstractSubmit.java | 6 ++--- .../gerrit/common/ChangeHookRunner.java | 4 +++- .../com/google/gerrit/common/ChangeHooks.java | 5 ---- .../gerrit/common/DisabledChangeHooks.java | 2 +- .../com/google/gerrit/common/EventSource.java | 24 +++++++++++++++++++ .../gerrit/sshd/commands/StreamEvents.java | 12 +++++----- 6 files changed, 37 insertions(+), 16 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/common/EventSource.java diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java index 41918e30d0..66911b8710 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java @@ -29,8 +29,8 @@ import com.google.gerrit.acceptance.GitUtil; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.RestResponse; import com.google.gerrit.acceptance.SshSession; -import com.google.gerrit.common.ChangeHooks; import com.google.gerrit.common.EventListener; +import com.google.gerrit.common.EventSource; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.extensions.client.ChangeStatus; @@ -98,13 +98,13 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { private IdentifiedUser.GenericFactory factory; @Inject - ChangeHooks hooks; + EventSource source; @Before public void setUp() throws Exception { mergeResults = Maps.newHashMap(); CurrentUser listenerUser = factory.create(user.id); - hooks.addEventListener(new EventListener() { + source.addEventListener(new EventListener() { @Override public void onEvent(Event event) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java index 90efc48d7d..8c103e2d51 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java +++ b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java @@ -86,7 +86,8 @@ import java.util.concurrent.TimeoutException; /** Spawns local executables when a hook action occurs. */ @Singleton -public class ChangeHookRunner implements ChangeHooks, LifecycleListener { +public class ChangeHookRunner implements ChangeHooks, EventSource, + LifecycleListener { /** A logger for this class. */ private static final Logger log = LoggerFactory.getLogger(ChangeHookRunner.class); @@ -95,6 +96,7 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener { protected void configure() { bind(ChangeHookRunner.class); bind(ChangeHooks.class).to(ChangeHookRunner.class); + bind(EventSource.class).to(ChangeHookRunner.class); listener().to(ChangeHookRunner.class); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHooks.java b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHooks.java index ec8e39d37e..4e8a21e6f1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHooks.java +++ b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHooks.java @@ -22,7 +22,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.events.Event; import com.google.gwtorm.server.OrmException; @@ -34,10 +33,6 @@ import java.util.Set; /** Invokes hooks on server actions. */ public interface ChangeHooks { - public void addEventListener(EventListener listener, CurrentUser user); - - public void removeEventListener(EventListener listener); - /** * Fire the Patchset Created Hook. * diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/DisabledChangeHooks.java b/gerrit-server/src/main/java/com/google/gerrit/common/DisabledChangeHooks.java index bd3ed139cb..8bbb600da5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/common/DisabledChangeHooks.java +++ b/gerrit-server/src/main/java/com/google/gerrit/common/DisabledChangeHooks.java @@ -32,7 +32,7 @@ import java.util.Map; import java.util.Set; /** Does not invoke hooks. */ -public final class DisabledChangeHooks implements ChangeHooks { +public final class DisabledChangeHooks implements ChangeHooks, EventSource { @Override public void addEventListener(EventListener listener, CurrentUser user) { } diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/EventSource.java b/gerrit-server/src/main/java/com/google/gerrit/common/EventSource.java new file mode 100644 index 0000000000..e2c4b34958 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/common/EventSource.java @@ -0,0 +1,24 @@ +// Copyright (C) 2014 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.server.CurrentUser; + +/** Distributes Events to ChangeListeners. Register listeners here. */ +public interface EventSource { + public void addEventListener(EventListener listener, CurrentUser user); + + public void removeEventListener(EventListener listener); +} diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/StreamEvents.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/StreamEvents.java index b02c59195a..889d3fb7fd 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/StreamEvents.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/StreamEvents.java @@ -16,8 +16,8 @@ package com.google.gerrit.sshd.commands; import static com.google.gerrit.sshd.CommandMetaData.Mode.MASTER_OR_SLAVE; -import com.google.gerrit.common.ChangeHooks; import com.google.gerrit.common.EventListener; +import com.google.gerrit.common.EventSource; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.extensions.annotations.RequiresCapability; import com.google.gerrit.server.IdentifiedUser; @@ -52,7 +52,7 @@ final class StreamEvents extends BaseCommand { private IdentifiedUser currentUser; @Inject - private ChangeHooks hooks; + private EventSource source; @Inject @StreamCommandExecutor @@ -132,12 +132,12 @@ final class StreamEvents extends BaseCommand { } stdout = toPrintWriter(out); - hooks.addEventListener(listener, currentUser); + source.addEventListener(listener, currentUser); } @Override protected void onExit(final int rc) { - hooks.removeEventListener(listener); + source.removeEventListener(listener); synchronized (taskLock) { done = true; @@ -148,7 +148,7 @@ final class StreamEvents extends BaseCommand { @Override public void destroy() { - hooks.removeEventListener(listener); + source.removeEventListener(listener); final boolean exit; synchronized (taskLock) { @@ -196,7 +196,7 @@ final class StreamEvents extends BaseCommand { // destroy() above, or it closed the stream and is no longer // accepting output. Either way terminate this instance. // - hooks.removeEventListener(listener); + source.removeEventListener(listener); flush(); onExit(0); return;