Batching files for sending to TsLint assuming 4096 char max command l…

…ine argument length

pom.xml

@@ -5,7 +5,7 @@
   <groupId>com.pablissimo.sonar</groupId>
   <artifactId>sonar-typescript-plugin</artifactId>
   <packaging>sonar-plugin</packaging>
-  <version>0.4-SNAPSHOT</version>
+  <version>0.5-SNAPSHOT</version>
 
   <name>TypeScript</name>
   <description>Analyse TypeScript projects</description>
@@ -14,7 +14,7 @@
     <connection>scm:git:git@github.com:Pablissimo/SonarTsPlugin.git</connection>
     <developerConnection>scm:git:git@github.com:Pablissimo/SonarTsPlugin.git</developerConnection>
     <url>https://github.com/Pablissimo/SonarTsPlugin</url>
-    <tag>0.4</tag>
+    <tag>0.5</tag>
   </scm>
 
   <properties>

src/main/java/com/pablissimo/sonar/TsLintExecutor.java

@@ -1,5 +1,7 @@
 package com.pablissimo.sonar;
 
+import java.util.List;
+
 public interface TsLintExecutor {
-    String execute(String pathToTsLint, String configFile, String rulesDir, String file, Integer timeoutMs);
+    String execute(String pathToTsLint, String configFile, String rulesDir, List<String> files, Integer timeoutMs);
 }

src/main/java/com/pablissimo/sonar/TsLintExecutorImpl.java

@@ -1,34 +1,71 @@
 package com.pablissimo.sonar;
 
+import java.util.ArrayList;
+import java.util.List;
+
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.sonar.api.utils.command.Command;
 import org.sonar.api.utils.command.CommandExecutor;
 import org.sonar.api.utils.command.StreamConsumer;
 
 public class TsLintExecutorImpl implements TsLintExecutor {
+    public static final int MAX_COMMAND_LENGTH = 4096;
     private static final Logger LOG = LoggerFactory.getLogger(TsLintExecutorImpl.class);
-
+    
     private StringBuilder stdOut;
     private StringBuilder stdErr;
 
-    public String execute(String pathToTsLint, String configFile, String rulesDir, String file, Integer timeoutMs) {
-        LOG.info("TsLint executing for " + file);
-        Command command = Command.create("node");
-        command.addArgument(pathToTsLint);
-        command.addArgument("--format");
-        command.addArgument("json");
+    private static Command getBaseCommand(String pathToTsLint, String configFile, String rulesDir) {
+        Command command = 
+                Command
+                .create("node")
+                .addArgument(pathToTsLint)
+                .addArgument("--format")
+                .addArgument("json");
 
         if (rulesDir != null && rulesDir.length() > 0) {
-            command.addArgument("--rules-dir");
-            command.addArgument(rulesDir);
+            command
+                .addArgument("--rules-dir")
+                .addArgument(rulesDir);
         }
 
-        command.addArgument("--config");
-        command.addArgument(configFile);
-        command.addArgument(file.trim());
-        command.setNewShell(false);
-
+        command
+            .addArgument("--config")
+            .addArgument(configFile)
+            .setNewShell(false);
+
+        return command;
+    }
+
+    public String execute(String pathToTsLint, String configFile, String rulesDir, List<String> files, Integer timeoutMs) {
+        // New up a command that's everything we need except the files to process
+        // We'll use this as our reference for chunking up files
+        int baseCommandLength = getBaseCommand(pathToTsLint, configFile, rulesDir).toCommandLine().length();
+        int availableForBatching = MAX_COMMAND_LENGTH - baseCommandLength;
+
+        List<List<String>> batches = new ArrayList<List<String>>();
+        List<String> currentBatch = new ArrayList<String>();
+        batches.add(currentBatch);
+
+        int currentBatchLength = 0;
+        for (int i = 0; i < files.size(); i++) {
+            String nextPath = files.get(i).trim();
+
+            // +1 for the space we'll be adding between filenames
+            if (currentBatchLength + nextPath.length() + 1 > availableForBatching) { 
+                // Too long to add to this batch, create new
+                currentBatch = new ArrayList<String>();
+                currentBatchLength = 0;
+                batches.add(currentBatch);
+            }
+
+            currentBatch.add(nextPath);
+            currentBatchLength += nextPath.length() + 1;
+        }
+
+        LOG.debug("Split " + files.size() + " files into " + batches.size() + " batches for processing");
+
         this.stdOut = new StringBuilder();
         this.stdErr = new StringBuilder();
 
@@ -46,9 +83,26 @@ public void consumeLine(String line) {
             }
         };
 
-        this.createExecutor().execute(command, stdOutConsumer, stdErrConsumer, timeoutMs);
-
-        return stdOut.toString();
+        for (int i = 0; i < batches.size(); i++) {
+            List<String> thisBatch = batches.get(i);
+            Command thisCommand = getBaseCommand(pathToTsLint, configFile, rulesDir);
+
+            for (int fileIndex = 0; fileIndex < thisBatch.size(); fileIndex++) {
+                thisCommand.addArgument(thisBatch.get(fileIndex));
+            }
+
+            LOG.debug("Executing TsLint with command: " + thisCommand.toCommandLine());
+
+            // Timeout is specified per file, not per batch (which can vary a lot)
+            // so multiply it up
+            this.createExecutor().execute(thisCommand, stdOutConsumer, stdErrConsumer, timeoutMs * thisBatch.size());
+        }
+
+        String rawOutput = stdOut.toString();
+
+        // TsLint returns nonsense for its JSON output when faced with multiple files
+        // so we need to fix it up before we do anything else
+        return "[" + rawOutput.replaceAll("\\]\\[", "],[") + "]";
     }
 
     protected CommandExecutor createExecutor() {

src/main/java/com/pablissimo/sonar/TsLintParser.java

@@ -3,5 +3,5 @@
 import com.pablissimo.sonar.model.TsLintIssue;
 
 public interface TsLintParser {
-    TsLintIssue[] parse(String toParse);
+    TsLintIssue[][] parse(String toParse);
 }

src/main/java/com/pablissimo/sonar/TsLintParserImpl.java

@@ -5,10 +5,10 @@
 import com.pablissimo.sonar.model.TsLintIssue;
 
 public class TsLintParserImpl implements TsLintParser {
-    public TsLintIssue[] parse(String toParse) {
+    public TsLintIssue[][] parse(String toParse) {
         GsonBuilder builder = new GsonBuilder();
         Gson gson = builder.create();
 
-        return gson.fromJson(toParse, TsLintIssue[].class);
+        return gson.fromJson(toParse, TsLintIssue[][].class);
     }
 }

src/main/java/com/pablissimo/sonar/TsLintSensor.java

@@ -5,6 +5,7 @@
 import java.nio.charset.Charset;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 
@@ -90,37 +91,62 @@ else if (pathToTsLintConfig == null) {
             ruleNames.add(rule.getKey());
         }
 
+        List<String> paths = new ArrayList<String>();
+        HashMap<String, File> fileMap = new HashMap<String, File>();
+
         for (File file : fileSystem.files(this.filePredicates.hasLanguage(TypeScriptLanguage.LANGUAGE_KEY))) {
             if (skipTypeDefFiles && file.getName().toLowerCase().endsWith("." + TypeScriptLanguage.LANGUAGE_DEFINITION_EXTENSION)) {
                 continue;
             }
-
+
+            String pathAdjusted = file.getAbsolutePath().replace('\\', '/');
+            paths.add(pathAdjusted);
+            fileMap.put(pathAdjusted, file);
+        }
+
+        String jsonResult = executor.execute(pathToTsLint, pathToTsLintConfig, rulesDir, paths, tsLintTimeoutMs);
+
+        TsLintIssue[][] issues = parser.parse(jsonResult);
+
+        if (issues == null) {
+            LOG.warn("TsLint returned no result at all");
+            return;
+        }
+
+        // Each issue bucket will contain info about a single file
+        for (TsLintIssue[] batchIssues : issues) {
+            if (batchIssues == null || batchIssues.length == 0) {
+                continue;
+            }
+
+            String filePath = batchIssues[0].getName();
+
+            if (!fileMap.containsKey(filePath)) {
+                LOG.warn("TsLint reported issues against a file that wasn't sent to it - will be ignored: " + filePath);
+                continue;
+            }
+
+            File file = fileMap.get(filePath);
             Resource resource = this.getFileFromIOFile(file, project);
             Issuable issuable = perspectives.as(Issuable.class, resource);
-
-            String jsonResult = executor.execute(pathToTsLint, pathToTsLintConfig, rulesDir, file.getAbsolutePath(), tsLintTimeoutMs);
-
-            TsLintIssue[] issues = parser.parse(jsonResult);
-
-            if (issues != null) {
-                for (TsLintIssue issue : issues) {
-                    // Make sure the rule we're violating is one we recognise - if not, we'll
-                    // fall back to the generic 'tslint-issue' rule
-                    String ruleName = issue.getRuleName();
-                    if (!ruleNames.contains(ruleName)) {
-                        ruleName = TsRulesDefinition.RULE_TSLINT_ISSUE;
-                    }
-
-                    issuable.addIssue
-                    (
-                            issuable
-                            .newIssueBuilder()
-                            .line(issue.getStartPosition().getLine() + 1)
-                            .message(issue.getFailure())
-                            .ruleKey(RuleKey.of(TsRulesDefinition.REPOSITORY_NAME, ruleName))
-                            .build()
-                            );
+
+            for (TsLintIssue issue : batchIssues) {
+                // Make sure the rule we're violating is one we recognise - if not, we'll
+                // fall back to the generic 'tslint-issue' rule
+                String ruleName = issue.getRuleName();
+                if (!ruleNames.contains(ruleName)) {
+                    ruleName = TsRulesDefinition.RULE_TSLINT_ISSUE;
                 }
+
+                issuable.addIssue
+                (
+                        issuable
+                        .newIssueBuilder()
+                        .line(issue.getStartPosition().getLine() + 1)
+                        .message(issue.getFailure())
+                        .ruleKey(RuleKey.of(TsRulesDefinition.REPOSITORY_NAME, ruleName))
+                        .build()
+                        );
             }
         }
     }

src/test/java/com/pablissimo/sonar/TsLintExecutorImplTest.java

@@ -4,6 +4,8 @@
 import static org.mockito.Mockito.*;
 
 import java.util.ArrayList;
+import java.util.List;
+
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.invocation.InvocationOnMock;
@@ -12,6 +14,8 @@
 import org.sonar.api.utils.command.CommandExecutor;
 import org.sonar.api.utils.command.StreamConsumer;
 
+import edu.emory.mathcs.backport.java.util.Arrays;
+
 public class TsLintExecutorImplTest {
     TsLintExecutorImpl executorImpl;
     CommandExecutor commandExecutor;
@@ -24,7 +28,7 @@ public void setUp() throws Exception {
     }
 
     @Test
-    public void executesCommandWithCorrectArguments() {
+    public void executesCommandWithCorrectArgumentsAndTimeouts() {
         final ArrayList<Command> capturedCommands = new ArrayList<Command>();
         final ArrayList<Long> capturedTimeouts = new ArrayList<Long>();
 
@@ -38,14 +42,93 @@ public Integer answer(InvocationOnMock invocation) throws Throwable {
         };
 
         when(this.commandExecutor.execute(any(Command.class), any(StreamConsumer.class), any(StreamConsumer.class), any(long.class))).then(captureCommand);
-        this.executorImpl.execute("path/to/tslint", "path/to/config", "path/to/rules", "path/to/file", 40000);
+        this.executorImpl.execute("path/to/tslint", "path/to/config", "path/to/rules", Arrays.asList(new String[] { "path/to/file", "path/to/another" }), 40000);
 
         assertEquals(1, capturedCommands.size());
 
         Command theCommand = capturedCommands.get(0);
         long theTimeout = capturedTimeouts.get(0);
 
-        assertEquals("node path/to/tslint --format json --rules-dir path/to/rules --config path/to/config path/to/file", theCommand.toCommandLine());
-        assertEquals(40000, theTimeout);        
+        assertEquals("node path/to/tslint --format json --rules-dir path/to/rules --config path/to/config path/to/file path/to/another", theCommand.toCommandLine());
+        // Expect one timeout period per file processed
+        assertEquals(2 * 40000, theTimeout);        
+    }
+
+    @Test
+    public void DoesNotAddRulesDirParameter_IfNull() {
+        final ArrayList<Command> capturedCommands = new ArrayList<Command>();
+        final ArrayList<Long> capturedTimeouts = new ArrayList<Long>();
+
+        Answer<Integer> captureCommand = new Answer<Integer>() {
+            @Override
+            public Integer answer(InvocationOnMock invocation) throws Throwable {
+                capturedCommands.add((Command) invocation.getArguments()[0]);
+                capturedTimeouts.add((long) invocation.getArguments()[3]);
+                return 0;
+            }
+        };
+
+        when(this.commandExecutor.execute(any(Command.class), any(StreamConsumer.class), any(StreamConsumer.class), any(long.class))).then(captureCommand);
+        this.executorImpl.execute("path/to/tslint", "path/to/config", null, Arrays.asList(new String[] { "path/to/file" }), 40000);
+
+        Command theCommand = capturedCommands.get(0);
+        assertFalse(theCommand.toCommandLine().contains("--rules-dir"));
+    }
+
+    @Test
+    public void DoesNotAddRulesDirParameter_IfEmptyString() {
+        final ArrayList<Command> capturedCommands = new ArrayList<Command>();
+        final ArrayList<Long> capturedTimeouts = new ArrayList<Long>();
+
+        Answer<Integer> captureCommand = new Answer<Integer>() {
+            @Override
+            public Integer answer(InvocationOnMock invocation) throws Throwable {
+                capturedCommands.add((Command) invocation.getArguments()[0]);
+                capturedTimeouts.add((long) invocation.getArguments()[3]);
+                return 0;
+            }
+        };
+
+        when(this.commandExecutor.execute(any(Command.class), any(StreamConsumer.class), any(StreamConsumer.class), any(long.class))).then(captureCommand);
+        this.executorImpl.execute("path/to/tslint", "path/to/config", "", Arrays.asList(new String[] { "path/to/file" }), 40000);
+
+        Command theCommand = capturedCommands.get(0);
+        assertFalse(theCommand.toCommandLine().contains("--rules-dir"));
+    }
+
+    @Test
+    public void BatchesExecutions_IfTooManyFilesForCommandLine() {
+        List<String> filenames = new ArrayList<String>();
+        int currentLength = 0;
+        int standardCmdLength = "node path/to/tslint --format json --rules-dir path/to/rules --config path/to/config".length();
+
+        String firstBatch = "first batch";
+        while (currentLength + 12 < TsLintExecutorImpl.MAX_COMMAND_LENGTH - standardCmdLength) {
+            filenames.add(firstBatch);
+            currentLength += firstBatch.length() + 1;
+        }
+        filenames.add("second batch");
+
+        final ArrayList<Command> capturedCommands = new ArrayList<Command>();
+        final ArrayList<Long> capturedTimeouts = new ArrayList<Long>();
+
+        Answer<Integer> captureCommand = new Answer<Integer>() {
+            @Override
+            public Integer answer(InvocationOnMock invocation) throws Throwable {
+                capturedCommands.add((Command) invocation.getArguments()[0]);
+                capturedTimeouts.add((long) invocation.getArguments()[3]);
+                return 0;
+            }
+        };
+
+        when(this.commandExecutor.execute(any(Command.class), any(StreamConsumer.class), any(StreamConsumer.class), any(long.class))).then(captureCommand);
+        this.executorImpl.execute("path/to/tslint", "path/to/config", "path/to/rules", filenames, 40000);
+
+        assertEquals(2, capturedCommands.size());
+
+        Command theSecondCommand = capturedCommands.get(1);
+
+        assertFalse(theSecondCommand.toCommandLine().contains("first batch"));
+        assertTrue(theSecondCommand.toCommandLine().contains("second batch"));
     }
 }