SONAR-2706 Verify "context conditions" only once

9 files changed, 262 insertions, 28 deletions

diff --git a/sonar-core/src/main/java/org/sonar/core/review/workflow/Workflow.java b/sonar-core/src/main/java/org/sonar/core/review/workflow/Workflow.java
index 6293a95106a..3a4004e5b40 100644
--- a/sonar-core/src/main/java/org/sonar/core/review/workflow/Workflow.java
+++ b/sonar-core/src/main/java/org/sonar/core/review/workflow/Workflow.java
@@ -44,6 +44,17 @@ public class Workflow implements ServerComponent {
    */
   private List<String> projectPropertyKeys = Lists.newArrayList();
 
+  /**
+   * Optimization: fast way to get all context conditions
+   */
+  private ListMultimap<String, Condition> contextConditionsByCommand = ArrayListMultimap.create();
+
+  /**
+   * Optimization: fast way to get all review conditions
+   */
+  private ListMultimap<String, Condition> reviewConditionsByCommand = ArrayListMultimap.create();
+
+
   public Workflow addCommand(String key) {
     Preconditions.checkArgument(!Strings.isNullOrEmpty(key), "Empty command key");
     commands.add(key);
@@ -62,10 +73,21 @@ public class Workflow implements ServerComponent {
     return projectPropertyKeys;
   }
 
+  /**
+   * Shortcut for: getReviewConditions(commandKey) + getContextConditions(commandKey)
+   */
   public List<Condition> getConditions(String commandKey) {
     return conditionsByCommand.get(commandKey);
   }
 
+  public List<Condition> getReviewConditions(String commandKey) {
+    return reviewConditionsByCommand.get(commandKey);
+  }
+
+  public List<Condition> getContextConditions(String commandKey) {
+    return contextConditionsByCommand.get(commandKey);
+  }
+
   public Workflow addCondition(String commandKey, Condition condition) {
     Preconditions.checkArgument(hasCommand(commandKey), "Unknown command: " + commandKey);
     Preconditions.checkNotNull(condition);
@@ -73,6 +95,11 @@ public class Workflow implements ServerComponent {
     if (condition instanceof ProjectPropertyCondition) {
       projectPropertyKeys.add(((ProjectPropertyCondition) condition).getPropertyKey());
     }
+    if (condition.isOnContext()) {
+      contextConditionsByCommand.put(commandKey, condition);
+    } else {
+      reviewConditionsByCommand.put(commandKey, condition);
+    }
     return this;
   }
 
diff --git a/sonar-core/src/main/java/org/sonar/core/review/workflow/WorkflowEngine.java b/sonar-core/src/main/java/org/sonar/core/review/workflow/WorkflowEngine.java
index b5b10b2489e..903df42e1ba 100644
--- a/sonar-core/src/main/java/org/sonar/core/review/workflow/WorkflowEngine.java
+++ b/sonar-core/src/main/java/org/sonar/core/review/workflow/WorkflowEngine.java
@@ -55,10 +55,13 @@ public class WorkflowEngine implements ServerComponent {
 
     completeProjectSettings(context);
 
-    for (Review review : reviews) {
-      for (Map.Entry<String, Screen> entry : workflow.getScreensByCommand().entrySet()) {
-        if (!verifyConditions || verifyConditions(review, context, entry.getKey())) {
-          result.put(review.getViolationId(), entry.getValue());
+    for (Map.Entry<String, Screen> entry : workflow.getScreensByCommand().entrySet()) {
+      String commandKey = entry.getKey();
+      if (!verifyConditions || verifyConditions(null, context, workflow.getContextConditions(commandKey))) {
+        for (Review review : reviews) {
+          if (!verifyConditions || verifyConditions(review, context, workflow.getReviewConditions(commandKey))) {
+            result.put(review.getViolationId(), entry.getValue());
+          }
         }
       }
     }
@@ -69,7 +72,8 @@ public class WorkflowEngine implements ServerComponent {
     List<Screen> result = Lists.newArrayList();
     completeProjectSettings(context);
     for (Map.Entry<String, Screen> entry : workflow.getScreensByCommand().entrySet()) {
-      if (!verifyConditions || verifyConditions(review, context, entry.getKey())) {
+      String commandKey = entry.getKey();
+      if (!verifyConditions || verifyConditions(review, context, workflow.getConditions(commandKey))) {
         result.add(entry.getValue());
 
       }
@@ -87,13 +91,15 @@ public class WorkflowEngine implements ServerComponent {
   public void execute(String commandKey, MutableReview review, DefaultWorkflowContext context, Map<String, String> parameters) {
     Preconditions.checkArgument(!Strings.isNullOrEmpty(commandKey), "Missing command");
     Preconditions.checkArgument(workflow.hasCommand(commandKey), "Unknown command: " + commandKey);
-    Preconditions.checkState(verifyConditions(review, context, commandKey), "Conditions are not respected");
 
     completeProjectSettings(context);
+
+    Preconditions.checkState(verifyConditions(review, context, workflow.getConditions(commandKey)), "Conditions are not respected");
+
     ImmutableMap<String, String> immutableParameters = ImmutableMap.copyOf(parameters);
 
     // TODO execute functions are change state before functions that consume state (like "create-jira-issue")
-    Review initialReview = ((DefaultReview)review).cloneImmutable();
+    Review initialReview = ((DefaultReview) review).cloneImmutable();
     for (Function function : workflow.getFunctions(commandKey)) {
       function.doExecute(review, initialReview, context, immutableParameters);
     }
@@ -104,10 +110,6 @@ public class WorkflowEngine implements ServerComponent {
     // TODO notify listeners
   }
 
-  private boolean verifyConditions(Review review, WorkflowContext context, String command) {
-    return verifyConditions(review, context, workflow.getConditions(command));
-  }
-
   private boolean verifyConditions(Review review, WorkflowContext context, List<Condition> conditions) {
     for (Condition condition : conditions) {
       if (!condition.doVerify(review, context)) {
diff --git a/sonar-core/src/main/java/org/sonar/core/review/workflow/condition/Condition.java b/sonar-core/src/main/java/org/sonar/core/review/workflow/condition/Condition.java
index dac2503aafc..e44e4c88d3e 100644
--- a/sonar-core/src/main/java/org/sonar/core/review/workflow/condition/Condition.java
+++ b/sonar-core/src/main/java/org/sonar/core/review/workflow/condition/Condition.java
@@ -22,18 +22,25 @@ package org.sonar.core.review.workflow.condition;
 import org.sonar.core.review.workflow.review.Review;
 import org.sonar.core.review.workflow.review.WorkflowContext;
 
+import javax.annotation.Nullable;
+
 public abstract class Condition {
 
-  private final boolean oncePerGroup;
+  private final boolean onContext;
 
-  protected Condition(boolean oncePerGroup) {
-    this.oncePerGroup = oncePerGroup;
+  protected Condition(boolean onContext) {
+    this.onContext = onContext;
   }
 
-  public final boolean isOncePerGroup() {
-    return oncePerGroup;
+  public final boolean isOnContext() {
+    return onContext;
   }
 
-  public abstract boolean doVerify(Review review, WorkflowContext context);
+  /**
+   * @param review the review on "review conditions" like StatusCondition, null on "context conditions" like AdminRoleCondition or ProjectPropertyCondition
+   * @param context
+   * @return is the condition verified ?
+   */
+  public abstract boolean doVerify(@Nullable Review review, WorkflowContext context);
 
 }
diff --git a/sonar-core/src/main/java/org/sonar/core/review/workflow/condition/NotCondition.java b/sonar-core/src/main/java/org/sonar/core/review/workflow/condition/NotCondition.java
index 614155b78d5..60cd6761189 100644
--- a/sonar-core/src/main/java/org/sonar/core/review/workflow/condition/NotCondition.java
+++ b/sonar-core/src/main/java/org/sonar/core/review/workflow/condition/NotCondition.java
@@ -27,7 +27,7 @@ public final class NotCondition extends Condition {
   private Condition condition;
 
   public NotCondition(Condition c) {
-    super(c.isOncePerGroup());
+    super(c.isOnContext());
     this.condition = c;
   }
 
diff --git a/sonar-core/src/main/java/org/sonar/core/review/workflow/review/DefaultReview.java b/sonar-core/src/main/java/org/sonar/core/review/workflow/review/DefaultReview.java
index 60bce16a177..55adaa16a14 100644
--- a/sonar-core/src/main/java/org/sonar/core/review/workflow/review/DefaultReview.java
+++ b/sonar-core/src/main/java/org/sonar/core/review/workflow/review/DefaultReview.java
@@ -200,8 +200,8 @@ public final class DefaultReview implements MutableReview {
   /**
    * Note : implementation is still mutable.
    */
-  public Review cloneImmutable() {
-    DefaultReview clone = new DefaultReview();
+  public ImmutableReview cloneImmutable() {
+    ImmutableReview clone = new ImmutableReview();
     clone.setAssigneeId(assigneeId);
     clone.setLine(line);
     clone.setManual(manual);
@@ -214,6 +214,7 @@ public final class DefaultReview implements MutableReview {
     clone.setStatus(status);
     clone.setSwitchedOff(switchedOff);
     clone.setViolationId(violationId);
+    clone.setProperties(ImmutableMap.copyOf(getProperties()));
     return clone;
   }
 }
diff --git a/sonar-core/src/main/java/org/sonar/core/review/workflow/review/ImmutableReview.java b/sonar-core/src/main/java/org/sonar/core/review/workflow/review/ImmutableReview.java
new file mode 100644
index 00000000000..94a6941e87a
--- /dev/null
+++ b/sonar-core/src/main/java/org/sonar/core/review/workflow/review/ImmutableReview.java
@@ -0,0 +1,133 @@
+/*
+ * Sonar, open source software quality management tool.
+ * Copyright (C) 2008-2012 SonarSource
+ * mailto:contact AT sonarsource DOT com
+ *
+ * Sonar is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * Sonar is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with Sonar; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02
+ */
+package org.sonar.core.review.workflow.review;
+
+import java.util.Map;
+
+public class ImmutableReview implements Review {
+  private Long violationId;
+  private Long reviewId;
+  private Long ruleId;
+  private Long assigneeId;
+  private Long line;
+  private boolean switchedOff = false;
+  private boolean manual = false;
+  private String message;
+  private String status;
+  private String resolution;
+  private String severity;
+  private Map<String, String> properties;
+
+  public Long getViolationId() {
+    return violationId;
+  }
+
+  void setViolationId(Long violationId) {
+    this.violationId = violationId;
+  }
+
+  public Long getReviewId() {
+    return reviewId;
+  }
+
+  void setReviewId(Long reviewId) {
+    this.reviewId = reviewId;
+  }
+
+  public Long getRuleId() {
+    return ruleId;
+  }
+
+  void setRuleId(Long ruleId) {
+    this.ruleId = ruleId;
+  }
+
+  public Long getAssigneeId() {
+    return assigneeId;
+  }
+
+  void setAssigneeId(Long assigneeId) {
+    this.assigneeId = assigneeId;
+  }
+
+  public Long getLine() {
+    return line;
+  }
+
+  void setLine(Long line) {
+    this.line = line;
+  }
+
+  public boolean isSwitchedOff() {
+    return switchedOff;
+  }
+
+  void setSwitchedOff(boolean switchedOff) {
+    this.switchedOff = switchedOff;
+  }
+
+  public boolean isManual() {
+    return manual;
+  }
+
+  void setManual(boolean manual) {
+    this.manual = manual;
+  }
+
+  public String getMessage() {
+    return message;
+  }
+
+  void setMessage(String message) {
+    this.message = message;
+  }
+
+  public String getStatus() {
+    return status;
+  }
+
+  void setStatus(String status) {
+    this.status = status;
+  }
+
+  public String getResolution() {
+    return resolution;
+  }
+
+  void setResolution(String resolution) {
+    this.resolution = resolution;
+  }
+
+  public String getSeverity() {
+    return severity;
+  }
+
+  void setSeverity(String severity) {
+    this.severity = severity;
+  }
+
+  public Map<String, String> getProperties() {
+    return properties;
+  }
+
+  void setProperties(Map<String, String> properties) {
+    this.properties = properties;
+  }
+}
diff --git a/sonar-core/src/test/java/org/sonar/core/review/workflow/WorkflowEngineTest.java b/sonar-core/src/test/java/org/sonar/core/review/workflow/WorkflowEngineTest.java
index 4e4cc610536..e290e8bd6aa 100644
--- a/sonar-core/src/test/java/org/sonar/core/review/workflow/WorkflowEngineTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/review/workflow/WorkflowEngineTest.java
@@ -20,18 +20,20 @@
 package org.sonar.core.review.workflow;
 
 import com.google.common.collect.ListMultimap;
+import com.google.common.collect.Maps;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.sonar.api.config.Settings;
 import org.sonar.core.review.workflow.condition.Condition;
 import org.sonar.core.review.workflow.condition.HasProjectPropertyCondition;
-import org.sonar.core.review.workflow.review.DefaultReview;
-import org.sonar.core.review.workflow.review.DefaultWorkflowContext;
-import org.sonar.core.review.workflow.review.Review;
-import org.sonar.core.review.workflow.review.WorkflowContext;
+import org.sonar.core.review.workflow.function.Function;
+import org.sonar.core.review.workflow.review.*;
 import org.sonar.core.review.workflow.screen.CommentScreen;
 import org.sonar.core.review.workflow.screen.Screen;
 
 import java.util.List;
+import java.util.Map;
 
 import static org.fest.assertions.Assertions.assertThat;
 import static org.junit.matchers.JUnitMatchers.hasItem;
@@ -39,6 +41,10 @@ import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.*;
 
 public class WorkflowEngineTest {
+
+  @Rule
+  public ExpectedException thrown = ExpectedException.none();
+
   @Test
   public void listAvailableScreensForReview_empty() {
     WorkflowEngine engine = new WorkflowEngine(new Workflow(), mock(ReviewStore.class), new Settings());
@@ -117,4 +123,49 @@ public class WorkflowEngineTest {
 
     verify(store).completeProjectSettings(eq(300L), any(Settings.class), (List<String>) argThat(hasItem("foo")));
   }
+
+  @Test
+  public void execute_conditions_pass() {
+    Workflow workflow = new Workflow();
+    workflow.addCommand("resolve");
+    workflow.addCondition("resolve", new HasProjectPropertyCondition("foo"));
+    Function function = mock(Function.class);
+    workflow.addFunction("resolve", function);
+
+    ReviewStore store = mock(ReviewStore.class);
+    Settings settings = new Settings();
+    settings.setProperty("foo", "bar");
+    WorkflowEngine engine = new WorkflowEngine(workflow, store, settings);
+
+    MutableReview review = new DefaultReview().setViolationId(1000L);
+    Map<String, String> parameters = Maps.newHashMap();
+    DefaultWorkflowContext context = new DefaultWorkflowContext().setProjectId(300L);
+
+    engine.execute("resolve", review, context, parameters);
+
+    verify(store).completeProjectSettings(eq(300L), any(Settings.class), (List<String>) argThat(hasItem("foo")));
+    verify(function).doExecute(eq(review), any(ImmutableReview.class), eq(context), eq(parameters));
+  }
+
+  @Test
+  public void execute_fail_if_conditions_dont_pass() {
+    thrown.expect(IllegalStateException.class);
+    thrown.expectMessage("Conditions are not respected");
+
+    Workflow workflow = new Workflow();
+    workflow.addCommand("resolve");
+    workflow.addCondition("resolve", new HasProjectPropertyCondition("foo"));
+    Function function = mock(Function.class);
+    workflow.addFunction("resolve", function);
+
+    ReviewStore store = mock(ReviewStore.class);
+    Settings settings = new Settings();// missing property 'foo'
+    WorkflowEngine engine = new WorkflowEngine(workflow, store, settings);
+
+    MutableReview review = new DefaultReview().setViolationId(1000L);
+    Map<String, String> parameters = Maps.newHashMap();
+    DefaultWorkflowContext context = new DefaultWorkflowContext().setProjectId(300L);
+
+    engine.execute("resolve", review, context, parameters);
+  }
 }
diff --git a/sonar-core/src/test/java/org/sonar/core/review/workflow/WorkflowTest.java b/sonar-core/src/test/java/org/sonar/core/review/workflow/WorkflowTest.java
index 0d5446cf6f0..e4e7dc40205 100644
--- a/sonar-core/src/test/java/org/sonar/core/review/workflow/WorkflowTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/review/workflow/WorkflowTest.java
@@ -74,7 +74,7 @@ public class WorkflowTest {
   }
 
   @Test
-  public void addAndGetCondition() {
+  public void addCondition() {
     Workflow workflow = new Workflow();
     Condition condition = new StatusCondition("OPEN");
     workflow.addCommand("resolve");
@@ -110,7 +110,20 @@ public class WorkflowTest {
   }
 
   @Test
-  public void addAndGetFunctions() {
+  public void keepFastLinksToReviewAndContextConditions() {
+    Workflow workflow = new Workflow();
+    workflow.addCommand("create-jira-issue");
+    Condition contextCondition = new HasProjectPropertyCondition("jira.url");
+    workflow.addCondition("create-jira-issue", contextCondition);
+    Condition reviewCondition = new StatusCondition("OPEN");
+    workflow.addCondition("create-jira-issue", reviewCondition);
+
+    assertThat(workflow.getContextConditions("create-jira-issue")).containsExactly(contextCondition);
+    assertThat(workflow.getReviewConditions("create-jira-issue")).containsExactly(reviewCondition);
+  }
+
+  @Test
+  public void addFunction() {
     Workflow workflow = new Workflow();
     workflow.addCommand("resolve");
 
diff --git a/sonar-core/src/test/java/org/sonar/core/review/workflow/condition/ConditionTest.java b/sonar-core/src/test/java/org/sonar/core/review/workflow/condition/ConditionTest.java
index 91822ec9fb9..9a3e74a4557 100644
--- a/sonar-core/src/test/java/org/sonar/core/review/workflow/condition/ConditionTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/review/workflow/condition/ConditionTest.java
@@ -34,7 +34,7 @@ public class ConditionTest {
         return false;
       }
     };
-    assertThat(condition.isOncePerGroup()).isTrue();
+    assertThat(condition.isOnContext()).isTrue();
   }
 
   @Test
@@ -45,7 +45,7 @@ public class ConditionTest {
         return false;
       }
     };
-    assertThat(condition.isOncePerGroup()).isFalse();
+    assertThat(condition.isOnContext()).isFalse();
   }
 
 }