From cb613f06b55951b30cb327c2124d8ec65a8dbe45 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Mon, 21 May 2012 17:06:04 +0200 Subject: [PATCH] SONAR-2706 Verify "context conditions" only once --- .../sonar/core/review/workflow/Workflow.java | 27 ++++ .../core/review/workflow/WorkflowEngine.java | 24 ++-- .../review/workflow/condition/Condition.java | 19 ++- .../workflow/condition/NotCondition.java | 2 +- .../review/workflow/review/DefaultReview.java | 5 +- .../workflow/review/ImmutableReview.java | 133 ++++++++++++++++++ .../review/workflow/WorkflowEngineTest.java | 59 +++++++- .../core/review/workflow/WorkflowTest.java | 17 ++- .../workflow/condition/ConditionTest.java | 4 +- 9 files changed, 262 insertions(+), 28 deletions(-) create mode 100644 sonar-core/src/main/java/org/sonar/core/review/workflow/review/ImmutableReview.java 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 projectPropertyKeys = Lists.newArrayList(); + /** + * Optimization: fast way to get all context conditions + */ + private ListMultimap contextConditionsByCommand = ArrayListMultimap.create(); + + /** + * Optimization: fast way to get all review conditions + */ + private ListMultimap 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 getConditions(String commandKey) { return conditionsByCommand.get(commandKey); } + public List getReviewConditions(String commandKey) { + return reviewConditionsByCommand.get(commandKey); + } + + public List 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 entry : workflow.getScreensByCommand().entrySet()) { - if (!verifyConditions || verifyConditions(review, context, entry.getKey())) { - result.put(review.getViolationId(), entry.getValue()); + for (Map.Entry 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 result = Lists.newArrayList(); completeProjectSettings(context); for (Map.Entry 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 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 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 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 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 getProperties() { + return properties; + } + + void setProperties(Map 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) 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 parameters = Maps.newHashMap(); + DefaultWorkflowContext context = new DefaultWorkflowContext().setProjectId(300L); + + engine.execute("resolve", review, context, parameters); + + verify(store).completeProjectSettings(eq(300L), any(Settings.class), (List) 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 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(); } } -- 2.39.5