SONAR-4069 Minor improvements and fixes for tasks

 * Try to break package cycle
 * Add validation on TaskDefinition

7 files changed, 168 insertions, 13 deletions

diff --git a/sonar-batch/src/main/java/org/sonar/batch/tasks/InspectionModule.java b/sonar-batch/src/main/java/org/sonar/batch/bootstrap/InspectionModule.java
index 116db16216d..c22a9132eff 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/tasks/InspectionModule.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/bootstrap/InspectionModule.java
@@ -17,7 +17,7 @@
  * License along with Sonar; if not, write to the Free Software
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02
  */
-package org.sonar.batch.tasks;
+package org.sonar.batch.bootstrap;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -36,10 +36,6 @@ import org.sonar.batch.ProfileProvider;
 import org.sonar.batch.ProjectTree;
 import org.sonar.batch.ResourceFilters;
 import org.sonar.batch.ViolationFilters;
-import org.sonar.batch.bootstrap.ExtensionInstaller;
-import org.sonar.batch.bootstrap.Module;
-import org.sonar.batch.bootstrap.ProjectSettings;
-import org.sonar.batch.bootstrap.UnsupportedProperties;
 import org.sonar.batch.components.TimeMachineConfiguration;
 import org.sonar.batch.events.EventBus;
 import org.sonar.batch.index.DefaultIndex;
diff --git a/sonar-batch/src/main/java/org/sonar/batch/bootstrapper/Batch.java b/sonar-batch/src/main/java/org/sonar/batch/bootstrapper/Batch.java
index b890f440824..1918abaf85e 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/bootstrapper/Batch.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/bootstrapper/Batch.java
@@ -55,7 +55,7 @@ public final class Batch {
     this.taskCommand = builder.taskCommand;
     projectReactor = builder.projectReactor;
     if (builder.isEnableLoggingConfiguration()) {
-      logging = LoggingConfiguration.create().setProperties(Maps.fromProperties(projectReactor.getRoot().getProperties()));
+      logging = LoggingConfiguration.create().setProperties(globalProperties);
     }
   }
 
diff --git a/sonar-batch/src/main/java/org/sonar/batch/tasks/InspectionTask.java b/sonar-batch/src/main/java/org/sonar/batch/tasks/InspectionTask.java
index bc08ec3f9b4..05ead3d88c8 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/tasks/InspectionTask.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/tasks/InspectionTask.java
@@ -19,6 +19,8 @@
  */
 package org.sonar.batch.tasks;
 
+import org.sonar.batch.bootstrap.InspectionModule;
+
 import org.sonar.api.platform.ComponentContainer;
 import org.sonar.api.resources.Project;
 import org.sonar.api.task.RequiresProject;
diff --git a/sonar-batch/src/main/java/org/sonar/batch/tasks/Tasks.java b/sonar-batch/src/main/java/org/sonar/batch/tasks/Tasks.java
index 9da1ad65271..8e5b8698eb4 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/tasks/Tasks.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/tasks/Tasks.java
@@ -20,16 +20,30 @@
 package org.sonar.batch.tasks;
 
 import org.apache.commons.lang.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.sonar.api.CoreProperties;
 import org.sonar.api.config.Settings;
+import org.sonar.api.task.Task;
+import org.sonar.api.task.TaskComponent;
 import org.sonar.api.task.TaskDefinition;
 import org.sonar.api.utils.SonarException;
 
-public class Tasks {
+import java.util.HashMap;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+public class Tasks implements TaskComponent {
+
+  private static final Logger LOG = LoggerFactory.getLogger(Tasks.class);
+  private static final String COMMAND_PATTERN = "[a-zA-Z0-9\\-\\_]+";
 
   private final TaskDefinition[] taskDefinitions;
   private final Settings settings;
 
+  private final Map<String, TaskDefinition> taskDefByCommand = new HashMap<String, TaskDefinition>();
+  private final Map<Class<? extends Task>, TaskDefinition> taskDefByTask = new HashMap<Class<? extends Task>, TaskDefinition>();
+
   public Tasks(Settings settings, TaskDefinition[] taskDefinitions) {
     this.settings = settings;
     this.taskDefinitions = taskDefinitions;
@@ -43,10 +57,8 @@ public class Tasks {
     }
     // Default to inspection task
     finalCommand = StringUtils.isNotBlank(finalCommand) ? finalCommand : InspectionTask.COMMAND;
-    for (TaskDefinition taskDef : taskDefinitions) {
-      if (finalCommand.equals(taskDef.getCommand())) {
-        return taskDef;
-      }
+    if (taskDefByCommand.containsKey(finalCommand)) {
+      return taskDefByCommand.get(finalCommand);
     }
     throw new SonarException("No task found for command: " + finalCommand);
   }
@@ -55,4 +67,56 @@ public class Tasks {
     return taskDefinitions;
   }
 
+  /**
+   * Perform validation of tasks definitions
+   */
+  public void start() {
+    for (TaskDefinition def : taskDefinitions) {
+      validateTask(def);
+      validateName(def);
+      validateCommand(def);
+      validateDescription(def);
+    }
+  }
+
+  private void validateName(TaskDefinition def) {
+    if (StringUtils.isBlank(def.getName())) {
+      throw new SonarException("Task definition for task '" + def.getTask().getName() + "' doesn't define task name");
+    }
+
+  }
+
+  private void validateCommand(TaskDefinition def) {
+    String command = def.getCommand();
+    if (StringUtils.isBlank(command)) {
+      throw new SonarException("Task definition '" + def.getName() + "' doesn't define task command");
+    }
+    if (!Pattern.matches(COMMAND_PATTERN, command)) {
+      throw new SonarException("Command '" + command + "' for task definition '" + def.getName() + "' is not valid and should match " + COMMAND_PATTERN);
+    }
+    if (taskDefByCommand.containsKey(command)) {
+      throw new SonarException("Task '" + def.getName() + "' uses the same command than task '" + taskDefByCommand.get(command).getName() + "'");
+    }
+    taskDefByCommand.put(command, def);
+  }
+
+  private void validateDescription(TaskDefinition def) {
+    if (StringUtils.isBlank(def.getDescription())) {
+      LOG.warn("Task definition {} doesn't define a description. Using name as description.", def.getName());
+      def.setDescription(def.getName());
+    }
+  }
+
+  private void validateTask(TaskDefinition def) {
+    Class<? extends Task> taskClass = def.getTask();
+    if (taskClass == null) {
+      throw new SonarException("Task definition '" + def.getName() + "' doesn't define the associated task class");
+    }
+    if (taskDefByTask.containsKey(taskClass)) {
+      throw new SonarException("Task '" + def.getTask().getName() + "' is defined twice: first by '" + taskDefByTask.get(taskClass).getName() + "' and then by '" + def.getName()
+        + "'");
+    }
+    taskDefByTask.put(taskClass, def);
+  }
+
 }
diff --git a/sonar-batch/src/test/java/org/sonar/batch/bootstrap/InspectionModuleTest.java b/sonar-batch/src/test/java/org/sonar/batch/bootstrap/InspectionModuleTest.java
index 1bc61c5f990..6b8d3481580 100644
--- a/sonar-batch/src/test/java/org/sonar/batch/bootstrap/InspectionModuleTest.java
+++ b/sonar-batch/src/test/java/org/sonar/batch/bootstrap/InspectionModuleTest.java
@@ -29,7 +29,6 @@ import org.sonar.api.resources.Project;
 import org.sonar.api.resources.Resource;
 import org.sonar.batch.ProjectTree;
 import org.sonar.batch.index.ResourcePersister;
-import org.sonar.batch.tasks.InspectionModule;
 
 import static org.fest.assertions.Assertions.assertThat;
 import static org.mockito.Matchers.any;
diff --git a/sonar-batch/src/test/java/org/sonar/batch/bootstrap/TaskBootstrapModuleTest.java b/sonar-batch/src/test/java/org/sonar/batch/bootstrap/TaskBootstrapModuleTest.java
index 568f9ef9b3d..c47f0774a6a 100644
--- a/sonar-batch/src/test/java/org/sonar/batch/bootstrap/TaskBootstrapModuleTest.java
+++ b/sonar-batch/src/test/java/org/sonar/batch/bootstrap/TaskBootstrapModuleTest.java
@@ -50,7 +50,7 @@ public class TaskBootstrapModuleTest {
     thrown.expect(SonarException.class);
     thrown.expectMessage("Task Sonar project inspection requires to be run on a project");
 
-    module.doStart();
+    module.start();
   }
 
 }
diff --git a/sonar-batch/src/test/java/org/sonar/batch/tasks/TasksTest.java b/sonar-batch/src/test/java/org/sonar/batch/tasks/TasksTest.java
index f95fa5c31c3..6c57398ae67 100644
--- a/sonar-batch/src/test/java/org/sonar/batch/tasks/TasksTest.java
+++ b/sonar-batch/src/test/java/org/sonar/batch/tasks/TasksTest.java
@@ -25,6 +25,7 @@ import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.sonar.api.CoreProperties;
 import org.sonar.api.config.Settings;
+import org.sonar.api.task.Task;
 import org.sonar.api.task.TaskDefinition;
 import org.sonar.api.utils.SonarException;
 
@@ -51,18 +52,21 @@ public class TasksTest {
   @Test
   public void shouldReturnInspectionTask() {
     Tasks tasks = new Tasks(settings, new TaskDefinition[] {InspectionTask.DEFINITION, ListTasksTask.DEFINITION});
+    tasks.start();
     assertThat(tasks.getTaskDefinition(InspectionTask.COMMAND)).isEqualTo(InspectionTask.DEFINITION);
   }
 
   @Test
   public void shouldReturnInspectionTaskByDefault() {
     Tasks tasks = new Tasks(settings, new TaskDefinition[] {InspectionTask.DEFINITION, ListTasksTask.DEFINITION});
+    tasks.start();
     assertThat(tasks.getTaskDefinition(null)).isEqualTo(InspectionTask.DEFINITION);
   }
 
   @Test
   public void shouldReturnUsePropertyWhenNoCommand() {
     Tasks tasks = new Tasks(settings, new TaskDefinition[] {InspectionTask.DEFINITION, ListTasksTask.DEFINITION});
+    tasks.start();
     assertThat(tasks.getTaskDefinition(ListTasksTask.COMMAND)).isEqualTo(ListTasksTask.DEFINITION);
     assertThat(tasks.getTaskDefinition(null)).isEqualTo(InspectionTask.DEFINITION);
 
@@ -80,4 +84,94 @@ public class TasksTest {
 
     tasks.getTaskDefinition("not-exists");
   }
+
+  @Test
+  public void shouldThrowWhenCommandMissing() {
+    Tasks tasks = new Tasks(settings, new TaskDefinition[] {TaskDefinition.create().setName("foo").setTask(FakeTask1.class)});
+
+    thrown.expect(SonarException.class);
+    thrown.expectMessage("Task definition 'foo' doesn't define task command");
+
+    tasks.start();
+  }
+
+  @Test
+  public void shouldThrowWhenCommandInvalid() {
+    Tasks tasks = new Tasks(settings, new TaskDefinition[] {TaskDefinition.create().setName("foo").setTask(FakeTask1.class).setCommand("Azc-12_bbC")});
+    tasks.start();
+
+    tasks = new Tasks(settings, new TaskDefinition[] {TaskDefinition.create().setName("foo").setTask(FakeTask1.class).setCommand("with space")});
+
+    thrown.expect(SonarException.class);
+    thrown.expectMessage("Command 'with space' for task definition 'foo' is not valid and should match [a-zA-Z0-9\\-\\_]+");
+
+    tasks.start();
+  }
+
+  @Test
+  public void shouldThrowWhenDuplicateCommand() {
+    Tasks tasks = new Tasks(settings, new TaskDefinition[] {
+      TaskDefinition.create().setName("foo1").setTask(FakeTask1.class).setCommand("cmd"),
+      TaskDefinition.create().setName("foo2").setTask(FakeTask2.class).setCommand("cmd")});
+
+    thrown.expect(SonarException.class);
+    thrown.expectMessage("Task 'foo2' uses the same command than task 'foo1'");
+
+    tasks.start();
+  }
+
+  @Test
+  public void shouldThrowWhenNameMissing() {
+    Tasks tasks = new Tasks(settings, new TaskDefinition[] {TaskDefinition.create().setCommand("foo").setTask(FakeTask1.class)});
+
+    thrown.expect(SonarException.class);
+    thrown.expectMessage("Task definition for task 'org.sonar.batch.tasks.TasksTest$FakeTask1' doesn't define task name");
+
+    tasks.start();
+  }
+
+  @Test
+  public void shouldThrowWhenTaskMissing() {
+    Tasks tasks = new Tasks(settings, new TaskDefinition[] {TaskDefinition.create().setCommand("foo").setName("bar")});
+
+    thrown.expect(SonarException.class);
+    thrown.expectMessage("Task definition 'bar' doesn't define the associated task class");
+
+    tasks.start();
+  }
+
+  @Test
+  public void shouldThrowWhenDuplicateTask() {
+    Tasks tasks = new Tasks(settings, new TaskDefinition[] {
+      TaskDefinition.create().setName("foo1").setTask(FakeTask1.class).setCommand("cmd1"),
+      TaskDefinition.create().setName("foo2").setTask(FakeTask1.class).setCommand("cmd2")});
+
+    thrown.expect(SonarException.class);
+    thrown.expectMessage("Task 'org.sonar.batch.tasks.TasksTest$FakeTask1' is defined twice: first by 'foo1' and then by 'foo2'");
+
+    tasks.start();
+  }
+
+  @Test
+  public void shouldUseNameWhenDescriptionIsMissing() {
+    Tasks tasks = new Tasks(settings, new TaskDefinition[] {TaskDefinition.create().setName("foo").setCommand("cmd").setTask(FakeTask1.class)});
+    tasks.start();
+
+    assertThat(tasks.getTaskDefinition("cmd").getDescription()).isEqualTo("foo");
+  }
+
+  private static class FakeTask1 implements Task {
+
+    public void execute() {
+    }
+
+  }
+
+  private static class FakeTask2 implements Task {
+
+    public void execute() {
+    }
+
+  }
+
 }