SONAR-10288 scanner multivalue property parsing ignore empty fields

sonar-plugin-api/src/main/java/org/sonar/api/config/Configuration.java

@@ -106,13 +106,15 @@ public interface Configuration {
    * <p>
    * See {@link PropertyDefinition.Builder#multiValues(boolean)}
    * Multi-valued properties coming from scanner are parsed as CSV lines (ie comma separator and optional double quotes to escape values).
-   * Non quoted values are trimmed.
+   * Non quoted values are trimmed and empty fields are ignored.
    * <br>
    * Examples :
    * <ul>
    * <li>"one,two,three " -&gt; ["one", "two", "three"]</li>
    * <li>"  one, two, three " -&gt; ["one", "two", "three"]</li>
-   * <li>"one, , three" -&gt; ["one", "", "three"]</li>
+   * <li>"one, three" -&gt; ["one", "three"]</li>
+   * <li>"one,"", three" -&gt; ["one", "", "three"]</li>
+   * <li>"one,  "  " , three" -&gt; ["one", "  ", "three"]</li>
    * <li>"one,\"two,three\",\" four \"" -&gt; ["one", "two,three", " four "]</li>
    * </ul>
    */

sonar-scanner-engine/pom.xml

@@ -92,6 +92,11 @@
       <artifactId>junit</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>com.tngtech.java</groupId>
+      <artifactId>junit-dataprovider</artifactId>
+      <scope>test</scope>
+    </dependency>
     <dependency>
       <groupId>org.assertj</groupId>
       <artifactId>assertj-core</artifactId>

sonar-scanner-engine/src/main/java/org/sonar/scanner/config/DefaultConfiguration.java

@@ -120,12 +120,13 @@ public abstract class DefaultConfiguration implements Configuration {
   }
 
   public static String[] parseAsCsv(String key, String value) {
+    String cleanValue = MultivaluePropertyCleaner.trimFieldsAndRemoveEmptyFields(value);
     List<String> result = new ArrayList<>();
     try (CSVParser csvParser = CSVFormat.RFC4180
       .withHeader((String) null)
       .withIgnoreEmptyLines()
       .withIgnoreSurroundingSpaces()
-      .parse(new StringReader(value))) {
+      .parse(new StringReader(cleanValue))) {
       List<CSVRecord> records = csvParser.getRecords();
       if (records.isEmpty()) {
         return ArrayUtils.EMPTY_STRING_ARRAY;

sonar-scanner-engine/src/main/java/org/sonar/scanner/config/MultivaluePropertyCleaner.java

@@ -0,0 +1,130 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2018 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program 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.
+ *
+ * This program 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 this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.scanner.config;
+
+class MultivaluePropertyCleaner {
+  private MultivaluePropertyCleaner() {
+    // prevents instantiation
+  }
+
+  /**
+   * Removes the empty fields from the value of a multi-value property from empty fields, including trimming each field.
+   * <p>
+   * Quotes can be used to prevent an empty field to be removed (as it is used to preserve empty spaces).
+   * <ul>
+   *    <li>{@code "" => ""}</li>
+   *    <li>{@code " " => ""}</li>
+   *    <li>{@code "," => ""}</li>
+   *    <li>{@code ",," => ""}</li>
+   *    <li>{@code ",,," => ""}</li>
+   *    <li>{@code ",a" => "a"}</li>
+   *    <li>{@code "a," => "a"}</li>
+   *    <li>{@code ",a," => "a"}</li>
+   *    <li>{@code "a,,b" => "a,b"}</li>
+   *    <li>{@code "a,   ,b" => "a,b"}</li>
+   *    <li>{@code "a,\"\",b" => "a,b"}</li>
+   *    <li>{@code "\"a\",\"b\"" => "\"a\",\"b\""}</li>
+   *    <li>{@code "\"  a  \",\"b \"" => "\"  a  \",\"b \""}</li>
+   *    <li>{@code "\"a\",\"\",\"b\"" => "\"a\",\"\",\"b\""}</li>
+   *    <li>{@code "\"a\",\"  \",\"b\"" => "\"a\",\"  \",\"b\""}</li>
+   *    <li>{@code "\"  a,,b,c  \",\"d \"" => "\"  a,,b,c  \",\"d \""}</li>
+   *    <li>{@code "a,\"  \",b" => "ab"]}</li>
+   * </ul>
+   */
+  public static String trimFieldsAndRemoveEmptyFields(String str) {
+    char[] chars = str.toCharArray();
+    char[] res = new char[chars.length];
+    /*
+     * set when reading the first non trimmable char after a separator char (or the beginning of the string)
+     * unset when reading a separator
+     */
+    boolean inField = false;
+    boolean inQuotes = false;
+    int i = 0;
+    int resI = 0;
+    for (; i < chars.length; i++) {
+      boolean isSeparator = chars[i] == ',';
+      if (!inQuotes && isSeparator) {
+        // exiting field (may already be unset)
+        inField = false;
+        if (resI > 0) {
+          resI = retroTrim(res, resI);
+        }
+      } else {
+        boolean isTrimmed = !inQuotes && istrimmable(chars[i]);
+        if (isTrimmed && !inField) {
+          // we haven't meet any non trimmable char since the last separator yet
+          continue;
+        }
+
+        boolean isEscape = isEscapeChar(chars[i]);
+        if (isEscape) {
+          inQuotes = !inQuotes;
+        }
+
+        // add separator as we already had one field
+        if (!inField && resI > 0) {
+          res[resI] = ',';
+          resI++;
+        }
+
+        // register in field (may already be set)
+        inField = true;
+        // copy current char
+        res[resI] = chars[i];
+        resI++;
+      }
+    }
+    // inQuotes can only be true at this point if quotes are unbalanced
+    if (!inQuotes) {
+      // trim end of str
+      resI = retroTrim(res, resI);
+    }
+    return new String(res, 0, resI);
+  }
+
+  private static boolean isEscapeChar(char aChar) {
+    return aChar == '"';
+  }
+
+  private static boolean istrimmable(char aChar) {
+    return aChar <= ' ';
+  }
+
+  /**
+   * Reads from index {@code resI} to the beginning into {@code res} looking up the location of the trimmable char with
+   * the lowest index before encountering a non-trimmable char.
+   * <p>
+   * This basically trims {@code res} from any trimmable char at its end.
+   *
+   * @return index of next location to put new char in res
+   */
+  private static int retroTrim(char[] res, int resI) {
+    int i = resI;
+    while (i >= 1) {
+      if (!istrimmable(res[i - 1])) {
+        return i;
+      }
+      i--;
+    }
+    return i;
+  }
+
+}

sonar-scanner-engine/src/test/java/org/sonar/scanner/config/DefaultConfigurationTest.java

@@ -107,8 +107,8 @@ public class DefaultConfigurationTest {
   @Test
   public void testParsingMultiValues() {
     assertThat(getStringArray("")).isEmpty();
-    assertThat(getStringArray(",")).containsExactly("", "");
-    assertThat(getStringArray(",,")).containsExactly("", "", "");
+    assertThat(getStringArray(",")).isEmpty();
+    assertThat(getStringArray(",,")).isEmpty();
     assertThat(getStringArray("a")).containsExactly("a");
     assertThat(getStringArray("a b")).containsExactly("a b");
     assertThat(getStringArray("a , b")).containsExactly("a", "b");
@@ -117,7 +117,10 @@ public class DefaultConfigurationTest {
     assertThat(getStringArray("\"a\nb\",c")).containsExactly("a\nb", "c");
     assertThat(getStringArray("\"a\",\n  b\n")).containsExactly("a", "b");
     assertThat(getStringArray("a\n,b\n")).containsExactly("a", "b");
-    assertThat(getStringArray("a\n,,b\n")).containsExactly("a", "", "b");
+    assertThat(getStringArray("a\n,b\n,\"\"")).containsExactly("a", "b", "");
+    assertThat(getStringArray("a\n,  \"  \"  ,b\n")).containsExactly("a", "  ", "b");
+    assertThat(getStringArray("  \" , ,, \", a\n,b\n")).containsExactly(" , ,, ", "a","b");
+    assertThat(getStringArray("a\n,,b\n")).containsExactly("a", "b");
     assertThat(getStringArray("a,\n\nb,c")).containsExactly("a", "b", "c");
     assertThat(getStringArray("a,b\n\nc,d")).containsExactly("a", "b\nc", "d");
     try {

sonar-scanner-engine/src/test/java/org/sonar/scanner/config/MultivaluePropertyCleanerTest.java

@@ -0,0 +1,223 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2018 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program 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.
+ *
+ * This program 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 this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.scanner.config;
+
+import com.tngtech.java.junit.dataprovider.DataProvider;
+import com.tngtech.java.junit.dataprovider.DataProviderRunner;
+import com.tngtech.java.junit.dataprovider.UseDataProvider;
+import java.util.Random;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
+
+import static org.apache.commons.lang.RandomStringUtils.randomAlphanumeric;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.sonar.scanner.config.MultivaluePropertyCleaner.trimFieldsAndRemoveEmptyFields;
+
+@RunWith(DataProviderRunner.class)
+public class MultivaluePropertyCleanerTest {
+  @Rule
+  public ExpectedException expectedException = ExpectedException.none();
+
+  @Test
+  public void clean_throws_NPE_if_arg_is_null() {
+    expectedException.expect(NullPointerException.class);
+
+    trimFieldsAndRemoveEmptyFields(null);
+  }
+
+  @Test
+  @UseDataProvider("plains")
+  public void ignoreEmptyFields(String str) {
+    assertThat(trimFieldsAndRemoveEmptyFields("")).isEqualTo("");
+    assertThat(trimFieldsAndRemoveEmptyFields(str)).isEqualTo(str);
+
+    assertThat(trimFieldsAndRemoveEmptyFields(',' + str)).isEqualTo(str);
+    assertThat(trimFieldsAndRemoveEmptyFields(str + ',')).isEqualTo(str);
+    assertThat(trimFieldsAndRemoveEmptyFields(",,," + str)).isEqualTo(str);
+    assertThat(trimFieldsAndRemoveEmptyFields(str + ",,,")).isEqualTo(str);
+
+    assertThat(trimFieldsAndRemoveEmptyFields(str + ',' + str)).isEqualTo(str + ',' + str);
+    assertThat(trimFieldsAndRemoveEmptyFields(str + ",,," + str)).isEqualTo(str + ',' + str);
+    assertThat(trimFieldsAndRemoveEmptyFields(',' + str + ',' + str)).isEqualTo(str + ',' + str);
+    assertThat(trimFieldsAndRemoveEmptyFields("," + str + ",,," + str)).isEqualTo(str + ',' + str);
+    assertThat(trimFieldsAndRemoveEmptyFields(",,," + str + ",,," + str)).isEqualTo(str + ',' + str);
+
+    assertThat(trimFieldsAndRemoveEmptyFields(str + ',' + str + ',')).isEqualTo(str + ',' + str);
+    assertThat(trimFieldsAndRemoveEmptyFields(str + ",,," + str + ",")).isEqualTo(str + ',' + str);
+    assertThat(trimFieldsAndRemoveEmptyFields(str + ",,," + str + ",,")).isEqualTo(str + ',' + str);
+
+    assertThat(trimFieldsAndRemoveEmptyFields(',' + str + ',' + str + ',')).isEqualTo(str + ',' + str);
+    assertThat(trimFieldsAndRemoveEmptyFields(",," + str + ',' + str + ',')).isEqualTo(str + ',' + str);
+    assertThat(trimFieldsAndRemoveEmptyFields(',' + str + ",," + str + ',')).isEqualTo(str + ',' + str);
+    assertThat(trimFieldsAndRemoveEmptyFields(',' + str + ',' + str + ",,")).isEqualTo(str + ',' + str);
+    assertThat(trimFieldsAndRemoveEmptyFields(",,," + str + ",,," + str + ",,")).isEqualTo(str + ',' + str);
+
+    assertThat(trimFieldsAndRemoveEmptyFields(str + ',' + str + ',' + str)).isEqualTo(str + ',' + str + ',' + str);
+    assertThat(trimFieldsAndRemoveEmptyFields(str + ',' + str + ',' + str)).isEqualTo(str + ',' + str + ',' + str);
+  }
+
+  @DataProvider
+  public static Object[][] plains() {
+    return new Object[][] {
+      {randomAlphanumeric(1)},
+      {randomAlphanumeric(2)},
+      {randomAlphanumeric(3 + new Random().nextInt(5))}
+    };
+  }
+
+  @Test
+  @UseDataProvider("emptyAndtrimmable")
+  public void ignoreEmptyFieldsAndTrimFields(String empty, String trimmable) {
+    String expected = trimmable.trim();
+    assertThat(empty.trim()).isEmpty();
+
+    assertThat(trimFieldsAndRemoveEmptyFields(trimmable)).isEqualTo(expected);
+    assertThat(trimFieldsAndRemoveEmptyFields(trimmable + ',' + empty)).isEqualTo(expected);
+    assertThat(trimFieldsAndRemoveEmptyFields(trimmable + ",," + empty)).isEqualTo(expected);
+    assertThat(trimFieldsAndRemoveEmptyFields(empty + ',' + trimmable)).isEqualTo(expected);
+    assertThat(trimFieldsAndRemoveEmptyFields(empty + ",," + trimmable)).isEqualTo(expected);
+    assertThat(trimFieldsAndRemoveEmptyFields(empty + ',' + trimmable + ',' + empty)).isEqualTo(expected);
+    assertThat(trimFieldsAndRemoveEmptyFields(empty + ",," + trimmable + ",,," + empty)).isEqualTo(expected);
+
+    assertThat(trimFieldsAndRemoveEmptyFields(trimmable + ',' + empty + ',' + empty)).isEqualTo(expected);
+    assertThat(trimFieldsAndRemoveEmptyFields(trimmable + ",," + empty + ",,," + empty)).isEqualTo(expected);
+
+    assertThat(trimFieldsAndRemoveEmptyFields(empty + ',' + empty + ',' + trimmable)).isEqualTo(expected);
+    assertThat(trimFieldsAndRemoveEmptyFields(empty + ",,,," + empty + ",," + trimmable)).isEqualTo(expected);
+
+    assertThat(trimFieldsAndRemoveEmptyFields(trimmable + ',' + trimmable)).isEqualTo(expected + ',' + expected);
+    assertThat(trimFieldsAndRemoveEmptyFields(trimmable + ',' + trimmable + ',' + trimmable)).isEqualTo(expected + ',' + expected + ',' + expected);
+    assertThat(trimFieldsAndRemoveEmptyFields(trimmable + "," + trimmable + ',' + trimmable)).isEqualTo(expected + ',' + expected + ',' + expected);
+  }
+
+  @Test
+  public void trimAccordingToStringTrim() {
+    String str = randomAlphanumeric(4);
+    for (int i = 0; i <= ' '; i++) {
+      String prefixed = (char) i + str;
+      String suffixed = (char) i + str;
+      String both = (char) i + str + (char) i;
+      assertThat(trimFieldsAndRemoveEmptyFields(prefixed)).isEqualTo(prefixed.trim());
+      assertThat(trimFieldsAndRemoveEmptyFields(suffixed)).isEqualTo(suffixed.trim());
+      assertThat(trimFieldsAndRemoveEmptyFields(both)).isEqualTo(both.trim());
+    }
+  }
+
+  @DataProvider
+  public static Object[][] emptyAndtrimmable() {
+    Random random = new Random();
+    String oneEmpty = randomTrimmedChars(1, random);
+    String twoEmpty = randomTrimmedChars(2, random);
+    String threePlusEmpty = randomTrimmedChars(3 + random.nextInt(5), random);
+    String onePlusEmpty = randomTrimmedChars(1 + random.nextInt(5), random);
+
+    String plain = randomAlphanumeric(1);
+    String plainWithtrimmable = randomAlphanumeric(2) + onePlusEmpty + randomAlphanumeric(3);
+    String quotedWithSeparator = '"' + randomAlphanumeric(3) + ',' + randomAlphanumeric(2) + '"';
+    String quotedWithDoubleSeparator = '"' + randomAlphanumeric(3) + ",," + randomAlphanumeric(2) + '"';
+    String quotedWithtrimmable = '"' + randomAlphanumeric(3) + onePlusEmpty + randomAlphanumeric(2) + '"';
+
+    String[] empties = {oneEmpty, twoEmpty, threePlusEmpty};
+    String[] strings = {plain, plainWithtrimmable,
+      onePlusEmpty + plain, plain + onePlusEmpty, onePlusEmpty + plain + onePlusEmpty,
+      onePlusEmpty + plainWithtrimmable, plainWithtrimmable + onePlusEmpty, onePlusEmpty + plainWithtrimmable + onePlusEmpty,
+      onePlusEmpty + quotedWithSeparator, quotedWithSeparator + onePlusEmpty, onePlusEmpty + quotedWithSeparator + onePlusEmpty,
+      onePlusEmpty + quotedWithDoubleSeparator, quotedWithDoubleSeparator + onePlusEmpty, onePlusEmpty + quotedWithDoubleSeparator + onePlusEmpty,
+      onePlusEmpty + quotedWithtrimmable, quotedWithtrimmable + onePlusEmpty, onePlusEmpty + quotedWithtrimmable + onePlusEmpty
+    };
+
+    Object[][] res = new Object[empties.length * strings.length][2];
+    int i = 0;
+    for (String empty : empties) {
+      for (String string : strings) {
+        res[i][0] = empty;
+        res[i][1] = string;
+        i++;
+      }
+    }
+    return res;
+  }
+
+  @Test
+  @UseDataProvider("emptys")
+  public void quotes_allow_to_preserve_fields(String empty) {
+    String quotedEmpty = '"' + empty + '"';
+
+    assertThat(trimFieldsAndRemoveEmptyFields(quotedEmpty)).isEqualTo(quotedEmpty);
+    assertThat(trimFieldsAndRemoveEmptyFields(',' + quotedEmpty)).isEqualTo(quotedEmpty);
+    assertThat(trimFieldsAndRemoveEmptyFields(quotedEmpty + ',')).isEqualTo(quotedEmpty);
+    assertThat(trimFieldsAndRemoveEmptyFields(',' + quotedEmpty + ',')).isEqualTo(quotedEmpty);
+
+    assertThat(trimFieldsAndRemoveEmptyFields(quotedEmpty + ',' + quotedEmpty)).isEqualTo(quotedEmpty + ',' + quotedEmpty);
+    assertThat(trimFieldsAndRemoveEmptyFields(quotedEmpty + ",," + quotedEmpty)).isEqualTo(quotedEmpty + ',' + quotedEmpty);
+
+    assertThat(trimFieldsAndRemoveEmptyFields(quotedEmpty + ',' + quotedEmpty + ',' + quotedEmpty)).isEqualTo(quotedEmpty + ',' + quotedEmpty + ',' + quotedEmpty);
+  }
+
+  @DataProvider
+  public static Object[][] emptys() {
+    Random random = new Random();
+    return new Object[][] {
+      {randomTrimmedChars(1, random)},
+      {randomTrimmedChars(2, random)},
+      {randomTrimmedChars(3 + random.nextInt(5), random)}
+    };
+  }
+
+  @Test
+  public void supports_escaped_quote_in_quotes() {
+    assertThat(trimFieldsAndRemoveEmptyFields("\"f\"\"oo\"")).isEqualTo("\"f\"\"oo\"");
+    assertThat(trimFieldsAndRemoveEmptyFields("\"f\"\"oo\",\"bar\"\"\"")).isEqualTo("\"f\"\"oo\",\"bar\"\"\"");
+  }
+
+  @Test
+  public void does_not_fail_on_unbalanced_quotes() {
+    assertThat(trimFieldsAndRemoveEmptyFields("\"")).isEqualTo("\"");
+    assertThat(trimFieldsAndRemoveEmptyFields("\"foo")).isEqualTo("\"foo");
+    assertThat(trimFieldsAndRemoveEmptyFields("foo\"")).isEqualTo("foo\"");
+
+    assertThat(trimFieldsAndRemoveEmptyFields("\"foo\",\"")).isEqualTo("\"foo\",\"");
+    assertThat(trimFieldsAndRemoveEmptyFields("\",\"foo\"")).isEqualTo("\",\"foo\"");
+
+    assertThat(trimFieldsAndRemoveEmptyFields("\"foo\",\",  ")).isEqualTo("\"foo\",\",  ");
+
+    assertThat(trimFieldsAndRemoveEmptyFields(" a ,,b , c,  \"foo\",\"  ")).isEqualTo("a,b,c,\"foo\",\"  ");
+    assertThat(trimFieldsAndRemoveEmptyFields("\" a ,,b , c,  ")).isEqualTo("\" a ,,b , c,  ");
+  }
+
+  private static final char[] SOME_PRINTABLE_TRIMMABLE_CHARS = {
+    ' ', '\t', '\n', '\r'
+  };
+
+  /**
+   * Result of randomTrimmedChars being used as arguments to JUnit test method through the DataProvider feature, they
+   * are printed to surefire report. Some of those chars breaks the parsing of the surefire report during sonar analysis.
+   * Therefor, we only use a subset of the trimmable chars.
+   */
+  private static String randomTrimmedChars(int length, Random random) {
+    char[] chars = new char[length];
+    for (int i = 0; i < chars.length; i++) {
+      chars[i] = SOME_PRINTABLE_TRIMMABLE_CHARS[random.nextInt(SOME_PRINTABLE_TRIMMABLE_CHARS.length)];
+    }
+    return new String(chars);
+  }
+}

sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/ProjectReactorBuilderTest.java

@@ -536,7 +536,7 @@ public class ProjectReactorBuilderTest {
     Map<String, String> props = new HashMap<>();
 
     props.put("prop", "  foo  ,,  bar  , toto,tutu");
-    assertThat(ProjectReactorBuilder.getListFromProperty(props, "prop")).containsOnly("foo", "", "bar", "toto", "tutu");
+    assertThat(ProjectReactorBuilder.getListFromProperty(props, "prop")).containsOnly("foo", "bar", "toto", "tutu");
   }
 
   @Test
@@ -560,7 +560,7 @@ public class ProjectReactorBuilderTest {
     String filePath = "shouldGetList/foo.properties";
     Map<String, String> props = loadPropsFromFile(filePath);
 
-    assertThat(ProjectReactorBuilder.getListFromProperty(props, "prop")).containsOnly("foo", "bar", "toto", "tutu", "");
+    assertThat(ProjectReactorBuilder.getListFromProperty(props, "prop")).containsOnly("foo", "bar", "toto", "tutu");
   }
 
   @Test