fix(triggers): prevent double-triggering for pipelines with >1 trigge…

…rs (#529)

* fix(triggers): prevent double-triggering for pipelines with >1 triggers

A pipeline with 2 triggers and an event matching one of them could
actually be triggered twice. The problem is that withMatchingTrigger
would consider the event and all triggers of the pipeline, and it would
be called once for each trigger.

The key fix is to matchTriggerFor(event) in getMatchingPipelines().
As a result, we can change the inputs of getMatchingPipelines to an event
and a matching trigger instead of an event and a pipeline (with all its
triggers).

echo-pipelinetriggers/src/main/java/com/netflix/spinnaker/echo/pipelinetriggers/eventhandlers/BaseTriggerEventHandler.java

@@ -22,16 +22,18 @@
 import com.netflix.spinnaker.echo.model.Pipeline;
 import com.netflix.spinnaker.echo.model.Trigger;
 import com.netflix.spinnaker.echo.model.trigger.TriggerEvent;
+import com.netflix.spinnaker.echo.pipelinetriggers.PipelineCache;
 import com.netflix.spinnaker.echo.pipelinetriggers.artifacts.ArtifactMatcher;
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
 import lombok.Value;
 import lombok.extern.slf4j.Slf4j;
 
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Optional;
+import java.util.*;
+import java.util.concurrent.TimeoutException;
 import java.util.function.Function;
 import java.util.function.Predicate;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 /**
  * Base implementation of {@link TriggerEventHandler} for events that require looking for matching
@@ -48,24 +50,34 @@ public abstract class BaseTriggerEventHandler<T extends TriggerEvent> implements
     this.objectMapper = objectMapper;
   }
 
-  public Optional<Pipeline> withMatchingTrigger(T event, Pipeline pipeline) {
-    if (pipeline.getTriggers() == null || pipeline.isDisabled()) {
+  @Override
+  public List<Pipeline> getMatchingPipelines(T event, PipelineCache pipelineCache) throws TimeoutException {
+    if (!isSuccessfulTriggerEvent(event)) {
+      return Collections.emptyList();
+    }
+
+    Map<String, List<Trigger>> triggers = pipelineCache.getEnabledTriggersSync();
+    return supportedTriggerTypes().stream()
+      .flatMap(triggerType -> Optional.ofNullable(triggers.get(triggerType)).orElse(Collections.emptyList()).stream())
+      .filter(this::isValidTrigger)
+      .filter(matchTriggerFor(event))
+      .map(trigger -> withMatchingTrigger(event, trigger))
+      .filter(Optional::isPresent)
+      .map(Optional::get)
+      .collect(Collectors.toList());
+  }
+
+  private Optional<Pipeline> withMatchingTrigger(T event, Trigger trigger) {
+    try {
+      return Stream.of(trigger)
+        .map(buildTrigger(event))
+        .map(t -> new TriggerWithArtifacts(t, getArtifacts(event, t)))
+        .filter(ta -> ArtifactMatcher.anyArtifactsMatchExpected(ta.artifacts, ta.trigger, ta.trigger.getParent().getExpectedArtifacts()))
+        .findFirst()
+        .map(ta -> ta.trigger.getParent().withTrigger(ta.trigger).withReceivedArtifacts(ta.artifacts));
+    } catch (Exception e) {
+      onSubscriberError(e);
       return Optional.empty();
-    } else {
-      try {
-        return pipeline.getTriggers()
-          .stream()
-          .filter(this::isValidTrigger)
-          .filter(matchTriggerFor(event))
-          .map(buildTrigger(event))
-          .map(t -> new TriggerWithArtifacts(t, getArtifacts(event, t)))
-          .filter(ta -> ArtifactMatcher.anyArtifactsMatchExpected(ta.artifacts, ta.trigger, pipeline.getExpectedArtifacts()))
-          .findFirst()
-          .map(ta -> pipeline.withTrigger(ta.trigger).withReceivedArtifacts(ta.artifacts));
-      } catch (Exception e) {
-        onSubscriberError(e);
-        return Optional.empty();
-      }
     }
   }
 

echo-pipelinetriggers/src/main/java/com/netflix/spinnaker/echo/pipelinetriggers/eventhandlers/ManualEventHandler.java

@@ -89,8 +89,7 @@ public ManualEvent convertEvent(Event event) {
     return objectMapper.convertValue(event, ManualEvent.class);
   }
 
-  @Override
-  public Optional<Pipeline> withMatchingTrigger(ManualEvent manualEvent, Pipeline pipeline) {
+  private Optional<Pipeline> withMatchingTrigger(ManualEvent manualEvent, Pipeline pipeline) {
     Content content = manualEvent.getContent();
     String application = content.getApplication();
     String pipelineNameOrId = content.getPipelineNameOrId();

echo-pipelinetriggers/src/main/java/com/netflix/spinnaker/echo/pipelinetriggers/eventhandlers/TriggerEventHandler.java

@@ -53,36 +53,14 @@ public interface TriggerEventHandler<T extends TriggerEvent> {
    */
   T convertEvent(Event event);
 
-  /**
-   * Determines whether the given pipeline should be triggered by the given event, and if so returns
-   * the fully-formed pipeline (with the trigger set) that should be executed.
-   * @param event The triggering event
-   * @param pipeline The pipeline to potentially trigger
-   * @return An Optional containing the pipeline to be triggered, or empty if it should not be triggered
-   */
-  Optional<Pipeline> withMatchingTrigger(T event, Pipeline pipeline);
-
   /**
    * Given a list of pipelines and an event, returns the pipelines that should be triggered
    * by the event
    * @param event The triggering event
    * @param pipelineCache a source for pipelines and triggers to consider
    * @return The pipelines that should be triggered
    */
-  default List<Pipeline> getMatchingPipelines(T event, PipelineCache pipelineCache) throws TimeoutException {
-    if (!isSuccessfulTriggerEvent(event)) {
-      return Collections.emptyList();
-    }
-
-    Map<String, List<Trigger>> triggers = pipelineCache.getEnabledTriggersSync();
-    return supportedTriggerTypes().stream()
-      .flatMap(triggerType -> Optional.ofNullable(triggers.get(triggerType)).orElse(Collections.emptyList()).stream())
-      .map(Trigger::getParent)
-      .map(p -> withMatchingTrigger(event, p))
-      .filter(Optional::isPresent)
-      .map(Optional::get)
-      .collect(Collectors.toList());
-  }
+  List<Pipeline> getMatchingPipelines(T event, PipelineCache pipelineCache) throws TimeoutException;
 
   /**
    * Given a pipeline, gets any additional tags that should be associated with metrics recorded

echo-pipelinetriggers/src/test/groovy/com/netflix/spinnaker/echo/pipelinetriggers/eventhandlers/BuildEventHandlerSpec.groovy

@@ -101,6 +101,27 @@ class BuildEventHandlerSpec extends Specification implements RetrofitStubs {
     }
   }
 
+  def "an event triggers a pipeline with 2 triggers only once"() {
+    given:
+    def pipeline = Pipeline.builder()
+      .application("application")
+      .name("pipeline")
+      .id("id")
+      .triggers([
+        enabledJenkinsTrigger,
+        enabledJenkinsTrigger.withJob("someOtherJob")])
+      .build()
+    def cache = handlerSupport.pipelineCache(pipeline)
+    def event = createBuildEventWith(SUCCESS)
+
+    when:
+    def matchingPipelines = eventHandler.getMatchingPipelines(event, cache)
+
+    then:
+    matchingPipelines.size() == 1
+    matchingPipelines.get(0).trigger.job == "job"
+  }
+
   @Unroll
   def "does not trigger pipelines for #description builds"() {
     when: