Add shellcheck support (#190)

* Add basic shellcheck support

* Add shellcheck exclude rules

* Update junit annotations

* Shellcheck review

src/main/java/pl/touk/sputnik/configuration/GeneralOption.java

@@ -89,7 +89,10 @@ public enum GeneralOption implements ConfigurationOption {
     JAVA_TEST_DIR("java.test.dir", "Java root test directory", Paths.SRC_TEST),
 
     DETEKT_ENABLED("detekt.enabled", "Detekt enabled", "false"),
-    DETEKT_CONFIG_FILE("detekt.config.file", "Detekt configuration file location", null);
+    DETEKT_CONFIG_FILE("detekt.config.file", "Detekt configuration file location", null),
+
+    SHELLCHECK_ENABLED("shellcheck.enabled", "Shellcheck enabled", "false"),
+    SHELLCHECK_EXCLUDE("shellcheck.exclude", "Shellcheck exclude rules", null);
 
     private final String key;
     private final String description;

src/main/java/pl/touk/sputnik/exec/ExternalProcess.java

@@ -20,7 +20,8 @@ public String executeCommand(String... args) {
             return executor().command(args)
                     .timeout(60, TimeUnit.SECONDS)
                     .redirectError(Slf4jStream.of(getClass()).asInfo())
-                    .readOutput(true).execute()
+                    .readOutput(true)
+                    .execute()
                     .outputUTF8();
         } catch (Exception e) {
             log.warn("Exception while calling command " + Arrays.asList(args) + ": " + e);

src/main/java/pl/touk/sputnik/processor/shellcheck/ShellcheckException.java

@@ -0,0 +1,11 @@
+package pl.touk.sputnik.processor.shellcheck;
+
+class ShellcheckException extends RuntimeException {
+    ShellcheckException(String message) {
+        super(message);
+    }
+
+    ShellcheckException(String message, Throwable t) {
+        super(message, t);
+    }
+}

src/main/java/pl/touk/sputnik/processor/shellcheck/ShellcheckExecutor.java

@@ -0,0 +1,47 @@
+package pl.touk.sputnik.processor.shellcheck;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.lang3.StringUtils;
+import org.jetbrains.annotations.NotNull;
+import pl.touk.sputnik.exec.ExternalProcess;
+
+import java.util.List;
+
+@Slf4j
+class ShellcheckExecutor {
+    private static final String SHELLCHECK_EXECUTABLE = "shellcheck";
+    private static final String SHELLCHECK_OUTPUT_FORMAT = "-fjson";
+    private static final String SHELLCHECK_EXCLUDE_RULES = "-e";
+
+    private final List<String> excludeRules;
+
+    ShellcheckExecutor(List<String> excludeRules) {
+        this.excludeRules = excludeRules;
+    }
+
+    String runOnFile(String filePath) {
+        log.info("Review on file: {}", filePath);
+        return new ExternalProcess().executeCommand(buildParams(filePath));
+    }
+
+    @NotNull
+    private String[] buildParams(String filePath) {
+        List<String> shellcheckArgs = ImmutableList.of(
+                SHELLCHECK_EXECUTABLE,
+                SHELLCHECK_OUTPUT_FORMAT);
+        List<String> filePathArg = ImmutableList.of(filePath);
+        List<String> excludeRulesArg = getExcludeRulesArgs();
+        List<String> allArgs = Lists.newArrayList(Iterables.concat(shellcheckArgs, filePathArg, excludeRulesArg));
+        return allArgs.toArray(new String[allArgs.size()]);
+    }
+
+    private List<String> getExcludeRulesArgs() {
+        if (excludeRules.isEmpty()) {
+            return ImmutableList.of();
+        }
+        return ImmutableList.of(SHELLCHECK_EXCLUDE_RULES + StringUtils.join(excludeRules, ","));
+    }
+}

src/main/java/pl/touk/sputnik/processor/shellcheck/ShellcheckProcessor.java

@@ -0,0 +1,57 @@
+package pl.touk.sputnik.processor.shellcheck;
+
+import lombok.extern.slf4j.Slf4j;
+import org.jetbrains.annotations.NotNull;
+import pl.touk.sputnik.configuration.Configuration;
+import pl.touk.sputnik.configuration.GeneralOption;
+import pl.touk.sputnik.processor.tools.externalprocess.ExternalProcessResultParser;
+import pl.touk.sputnik.processor.tools.externalprocess.ProcessorRunningExternalProcess;
+import pl.touk.sputnik.review.filter.FileFilter;
+import pl.touk.sputnik.review.filter.ShellFilter;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+@Slf4j
+class ShellcheckProcessor extends ProcessorRunningExternalProcess {
+
+    private ShellcheckExecutor shellcheckExecutor;
+    private ShellcheckResultParser shellcheckResultParser;
+
+    ShellcheckProcessor(Configuration configuration) {
+        shellcheckExecutor = new ShellcheckExecutor(getExcludeRules(configuration));
+        shellcheckResultParser = new ShellcheckResultParser();
+    }
+
+    @NotNull
+    @Override
+    public String getName() {
+        return "Shellcheck";
+    }
+
+    @Override
+    public FileFilter getReviewFileFilter() {
+        return new ShellFilter();
+    }
+
+    @Override
+    public ExternalProcessResultParser getParser() {
+        return shellcheckResultParser;
+    }
+
+    @Override
+    public String processFileAndDumpOutput(File fileToReview) {
+        return shellcheckExecutor.runOnFile(fileToReview.getAbsolutePath());
+    }
+
+    @NotNull
+    private List<String> getExcludeRules(Configuration configuration) {
+        String excludeRules = configuration.getProperty(GeneralOption.SHELLCHECK_EXCLUDE);
+        if (excludeRules == null) {
+            return new ArrayList<>();
+        }
+        return Arrays.asList(excludeRules.split(","));
+    }
+}

src/main/java/pl/touk/sputnik/processor/shellcheck/ShellcheckProcessorFactory.java

@@ -0,0 +1,17 @@
+package pl.touk.sputnik.processor.shellcheck;
+
+import pl.touk.sputnik.configuration.Configuration;
+import pl.touk.sputnik.configuration.GeneralOption;
+import pl.touk.sputnik.processor.ReviewProcessorFactory;
+
+public class ShellcheckProcessorFactory implements ReviewProcessorFactory<ShellcheckProcessor> {
+    @Override
+    public boolean isEnabled(Configuration configuration) {
+        return Boolean.valueOf(configuration.getProperty(GeneralOption.SHELLCHECK_ENABLED));
+    }
+
+    @Override
+    public ShellcheckProcessor create(Configuration configuration) {
+        return new ShellcheckProcessor(configuration);
+    }
+}

src/main/java/pl/touk/sputnik/processor/shellcheck/ShellcheckResultParser.java

@@ -0,0 +1,66 @@
+package pl.touk.sputnik.processor.shellcheck;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.commons.lang3.StringUtils;
+import pl.touk.sputnik.processor.shellcheck.json.ShellcheckMessage;
+import pl.touk.sputnik.processor.tools.externalprocess.ExternalProcessResultParser;
+import pl.touk.sputnik.review.Severity;
+import pl.touk.sputnik.review.Violation;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.List;
+
+import static java.util.stream.Collectors.toList;
+
+class ShellcheckResultParser implements ExternalProcessResultParser {
+
+    private final ObjectMapper objectMapper = new ObjectMapper();
+
+    private static final String SHELLCHECK_ERROR = "error";
+    private static final String SHELLCHECK_WARNING = "warning";
+    private static final String SHELLCHECK_INFO = "info";
+
+    @Override
+    public List<Violation> parse(String shellcheckOutput) {
+        if (StringUtils.isEmpty(shellcheckOutput)) {
+            return Collections.emptyList();
+        }
+        try {
+            List<ShellcheckMessage> violationMessages = objectMapper.readValue(shellcheckOutput,
+                    new TypeReference<List<ShellcheckMessage>>() {
+                    });
+
+            return violationMessages
+                    .stream()
+                    .map(violationMessage -> mapToViolation(violationMessage))
+                    .collect(toList());
+        } catch (IOException e) {
+            throw new ShellcheckException("Error when converting from json format", e);
+        }
+    }
+
+    private Violation mapToViolation(ShellcheckMessage violationMessage) {
+        return new Violation(violationMessage.getFile(),
+                violationMessage.getLine(),
+                formatViolationMessage(violationMessage),
+                shellcheckMessageTypeToSeverity(violationMessage.getLevel()));
+    }
+
+    private String formatViolationMessage(ShellcheckMessage violationMessage) {
+        return "[Code:" + violationMessage.getCode() + "] Message: " + violationMessage.getMessage();
+    }
+
+    private Severity shellcheckMessageTypeToSeverity(String messageType) {
+        if (SHELLCHECK_ERROR.equals(messageType)) {
+            return Severity.ERROR;
+        } else if (SHELLCHECK_WARNING.equals(messageType)) {
+            return Severity.WARNING;
+        } else if (SHELLCHECK_INFO.equals(messageType)) {
+            return Severity.INFO;
+        } else {
+            throw new ShellcheckException("Unknown message type returned by shellcheck (type = " + messageType + ")");
+        }
+    }
+}

src/main/java/pl/touk/sputnik/processor/shellcheck/json/ShellcheckMessage.java

@@ -0,0 +1,18 @@
+package pl.touk.sputnik.processor.shellcheck.json;
+
+import lombok.Data;
+import lombok.NoArgsConstructor;
+
+@Data
+@NoArgsConstructor
+public class ShellcheckMessage {
+    private String file;
+    private int line;
+    private Integer endLine;
+    private int column;
+    private Integer endColumn;
+    private String level;
+    private String code;
+    private String message;
+    private String fix;
+}

src/main/java/pl/touk/sputnik/review/filter/ShellFilter.java

@@ -0,0 +1,10 @@
+package pl.touk.sputnik.review.filter;
+
+import java.util.Collections;
+
+public class ShellFilter extends FileExtensionFilter {
+
+    public ShellFilter() {
+        super(Collections.singletonList("sh"));
+    }
+}

src/test/java/pl/touk/sputnik/processor/shellcheck/ShellcheckProcessorTest.java

@@ -0,0 +1,117 @@
+package pl.touk.sputnik.processor.shellcheck;
+
+import org.junit.jupiter.api.Assumptions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.platform.commons.util.StringUtils;
+import pl.touk.sputnik.TestEnvironment;
+import pl.touk.sputnik.configuration.ConfigurationBuilder;
+import pl.touk.sputnik.exec.ExternalProcess;
+import pl.touk.sputnik.review.Review;
+import pl.touk.sputnik.review.ReviewFile;
+import pl.touk.sputnik.review.ReviewFormatterFactory;
+import pl.touk.sputnik.review.ReviewResult;
+import pl.touk.sputnik.review.Severity;
+import pl.touk.sputnik.review.Violation;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class ShellcheckProcessorTest extends TestEnvironment {
+
+    private static final String CONFIGURATION_WITH_SHELLCHECK_ENABLED = "shellcheck/sputnik/noConfigurationFile.properties";
+    private static final String REVIEW_FILE_WITH_ONE_VIOLATION = "src/test/resources/shellcheck/testFiles/oneViolation.sh";
+    private static final String REVIEW_FILE_WITH_EXCLUDED_VIOLATION = "src/test/resources/shellcheck/testFiles/excludedViolation.sh";
+    private static final String REVIEW_FILE_WITH_MULTIPLE_VIOLATIONS = "src/test/resources/shellcheck/testFiles/multipleViolations.sh";
+    private ShellcheckProcessor sut;
+
+    @BeforeEach
+    public void setUp() {
+        config = ConfigurationBuilder.initFromResource(CONFIGURATION_WITH_SHELLCHECK_ENABLED);
+        sut = new ShellcheckProcessor(config);
+    }
+
+    @Test
+    public void shouldReturnNoViolationsWhenThereIsNoFileToReview() {
+        ReviewResult reviewResult = sut.process(nonExistentReview());
+
+        assertThat(reviewResult).isNotNull();
+        assertThat(reviewResult.getViolations()).isEmpty();
+    }
+
+    @Test
+    public void shouldReturnOneViolationsForFile() {
+        Assumptions.assumeTrue(isShellcheckInstalled());
+        Review review = getReview(REVIEW_FILE_WITH_ONE_VIOLATION);
+
+        ReviewResult result = sut.process(review);
+
+        assertThat(result).isNotNull();
+        assertThat(result.getViolations())
+                .hasSize(1);
+        Violation violation = result.getViolations().get(0);
+        assertThat(violation.getFilenameOrJavaClassName()).contains(REVIEW_FILE_WITH_ONE_VIOLATION);
+        assertThat(violation.getLine()).isEqualTo(3);
+        assertThat(violation.getMessage()).isEqualTo("[Code:2154] Message: variable is referenced but not assigned.");
+        assertThat(violation.getSeverity()).isEqualTo(Severity.WARNING);
+    }
+
+    @Test
+    public void shouldReturnMultipleViolationsForFile() {
+        Assumptions.assumeTrue(isShellcheckInstalled());
+        Review review = getReview(REVIEW_FILE_WITH_MULTIPLE_VIOLATIONS);
+
+        ReviewResult result = sut.process(review);
+
+        assertThat(result).isNotNull();
+        assertThat(result.getViolations())
+                .hasSize(3);
+        Violation violation1 = result.getViolations().get(0);
+        assertThat(violation1.getFilenameOrJavaClassName()).contains(REVIEW_FILE_WITH_MULTIPLE_VIOLATIONS);
+        assertThat(violation1.getLine()).isEqualTo(3);
+        assertThat(violation1.getMessage()).isEqualTo("[Code:2154] Message: variable is referenced but not assigned.");
+        assertThat(violation1.getSeverity()).isEqualTo(Severity.WARNING);
+
+        Violation violation2 = result.getViolations().get(1);
+        assertThat(violation2.getFilenameOrJavaClassName()).contains(REVIEW_FILE_WITH_MULTIPLE_VIOLATIONS);
+        assertThat(violation2.getLine()).isEqualTo(5);
+        assertThat(violation2.getMessage()).isEqualTo("[Code:1037] Message: Braces are required for positionals over 9, e.g. ${10}.");
+        assertThat(violation2.getSeverity()).isEqualTo(Severity.ERROR);
+
+        Violation violation3 = result.getViolations().get(2);
+        assertThat(violation3.getFilenameOrJavaClassName()).contains(REVIEW_FILE_WITH_MULTIPLE_VIOLATIONS);
+        assertThat(violation3.getLine()).isEqualTo(7);
+        assertThat(violation3.getMessage()).isEqualTo("[Code:2078] Message: This expression is constant. Did you forget a $ somewhere?");
+        assertThat(violation3.getSeverity()).isEqualTo(Severity.ERROR);
+    }
+
+    @Test
+    public void shouldReturnNoViolationWhenRuleExcludedInConfig() {
+        Assumptions.assumeTrue(isShellcheckInstalled());
+        Review review = getReview(REVIEW_FILE_WITH_EXCLUDED_VIOLATION);
+
+        ReviewResult result = sut.process(review);
+
+        assertThat(result).isNotNull();
+        assertThat(result.getViolations()).isEmpty();
+    }
+
+    private boolean isShellcheckInstalled() {
+        try {
+            String result = new ExternalProcess().executeCommand("shellcheck");
+            return StringUtils.isBlank(result);
+        } catch (Exception e) {
+            return false;
+        }
+    }
+
+    private Review getReview(String... filePaths) {
+        List<ReviewFile> files = new ArrayList<>();
+        for (String filePath : filePaths) {
+            files.add(new ReviewFile(filePath));
+        }
+        return new Review(files, ReviewFormatterFactory.get(config));
+    }
+}