From: Sébastien Lesaint Date: Mon, 15 Jan 2018 15:57:48 +0000 (+0100) Subject: SONAR-10288 scanner multivalue property parsing ignore empty fields X-Git-Tag: 7.5~1774 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=7506fd298b7dbf2d8504536d8da20e790752eab4;p=sonarqube.git SONAR-10288 scanner multivalue property parsing ignore empty fields --- diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/config/Configuration.java b/sonar-plugin-api/src/main/java/org/sonar/api/config/Configuration.java index f4e433c8533..3e89c24371e 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/config/Configuration.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/config/Configuration.java @@ -106,13 +106,15 @@ public interface Configuration { *

* 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. *
* Examples : *

*/ diff --git a/sonar-scanner-engine/pom.xml b/sonar-scanner-engine/pom.xml index eb629492360..5b2147d285f 100644 --- a/sonar-scanner-engine/pom.xml +++ b/sonar-scanner-engine/pom.xml @@ -92,6 +92,11 @@ junit test + + com.tngtech.java + junit-dataprovider + test + org.assertj assertj-core diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/config/DefaultConfiguration.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/config/DefaultConfiguration.java index 1a0ac7384cf..0b32c98a1d5 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/config/DefaultConfiguration.java +++ b/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 result = new ArrayList<>(); try (CSVParser csvParser = CSVFormat.RFC4180 .withHeader((String) null) .withIgnoreEmptyLines() .withIgnoreSurroundingSpaces() - .parse(new StringReader(value))) { + .parse(new StringReader(cleanValue))) { List records = csvParser.getRecords(); if (records.isEmpty()) { return ArrayUtils.EMPTY_STRING_ARRAY; diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/config/MultivaluePropertyCleaner.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/config/MultivaluePropertyCleaner.java new file mode 100644 index 00000000000..a5ae8d807e6 --- /dev/null +++ b/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. + *

+ * Quotes can be used to prevent an empty field to be removed (as it is used to preserve empty spaces). + *

    + *
  • {@code "" => ""}
    • + *
  • {@code " " => ""}
    • + *
  • {@code "," => ""}
    • + *
  • {@code ",," => ""}
    • + *
  • {@code ",,," => ""}
    • + *
  • {@code ",a" => "a"}
    • + *
  • {@code "a," => "a"}
    • + *
  • {@code ",a," => "a"}
    • + *
  • {@code "a,,b" => "a,b"}
    • + *
  • {@code "a, ,b" => "a,b"}
    • + *
  • {@code "a,\"\",b" => "a,b"}
    • + *
  • {@code "\"a\",\"b\"" => "\"a\",\"b\""}
    • + *
  • {@code "\" a \",\"b \"" => "\" a \",\"b \""}
    • + *
  • {@code "\"a\",\"\",\"b\"" => "\"a\",\"\",\"b\""}
    • + *
  • {@code "\"a\",\" \",\"b\"" => "\"a\",\" \",\"b\""}
    • + *
  • {@code "\" a,,b,c \",\"d \"" => "\" a,,b,c \",\"d \""}
    • + *
  • {@code "a,\" \",b" => "ab"]}
    • + *
+ */ + 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. + *

+ * 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; + } + +} diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/config/DefaultConfigurationTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/config/DefaultConfigurationTest.java index 1c6473409f1..ba7842a2b2c 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/config/DefaultConfigurationTest.java +++ b/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 { diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/config/MultivaluePropertyCleanerTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/config/MultivaluePropertyCleanerTest.java new file mode 100644 index 00000000000..70be5c59b90 --- /dev/null +++ b/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); + } +} diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/ProjectReactorBuilderTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/ProjectReactorBuilderTest.java index d81967cd66c..afdfd12a7e1 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/ProjectReactorBuilderTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/ProjectReactorBuilderTest.java @@ -536,7 +536,7 @@ public class ProjectReactorBuilderTest { Map 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 props = loadPropsFromFile(filePath); - assertThat(ProjectReactorBuilder.getListFromProperty(props, "prop")).containsOnly("foo", "bar", "toto", "tutu", ""); + assertThat(ProjectReactorBuilder.getListFromProperty(props, "prop")).containsOnly("foo", "bar", "toto", "tutu"); } @Test