From 9a7dcc5b3ce82c75de74ea3fc20e8d69f8e43a24 Mon Sep 17 00:00:00 2001 From: Julien HENRY Date: Fri, 30 Jun 2017 14:51:35 +0200 Subject: [PATCH] SONAR-9198 Support multi-valued properties containing comma --- pom.xml | 4 +- sonar-scanner-engine/pom.xml | 4 + .../org/sonar/scanner/config/CsvParser.java | 105 ----------------- .../scanner/config/DefaultConfiguration.java | 38 ++++-- .../settings/DefaultSettingsLoader.java | 7 +- .../ModuleFileSystemInitializer.java | 24 ++-- .../config/DefaultConfigurationTest.java | 109 ++++++++++++++++++ .../settings/DefaultSettingsLoaderTest.java | 9 ++ .../ModuleFileSystemInitializerTest.java | 22 ++++ 9 files changed, 197 insertions(+), 125 deletions(-) delete mode 100644 sonar-scanner-engine/src/main/java/org/sonar/scanner/config/CsvParser.java create mode 100644 sonar-scanner-engine/src/test/java/org/sonar/scanner/config/DefaultConfigurationTest.java diff --git a/pom.xml b/pom.xml index 14228506e9c..36be56adfa3 100644 --- a/pom.xml +++ b/pom.xml @@ -715,7 +715,7 @@ com.google.code.findbugs jsr305 - 3.0.0 + 3.0.2 commons-dbutils @@ -730,7 +730,7 @@ org.apache.commons commons-csv - 1.0 + 1.4 commons-codec diff --git a/sonar-scanner-engine/pom.xml b/sonar-scanner-engine/pom.xml index e3b4186507e..5b4192b82f3 100644 --- a/sonar-scanner-engine/pom.xml +++ b/sonar-scanner-engine/pom.xml @@ -80,6 +80,10 @@ com.google.code.gson gson + + org.apache.commons + commons-csv + org.freemarker diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/config/CsvParser.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/config/CsvParser.java deleted file mode 100644 index efdfc5bccdb..00000000000 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/config/CsvParser.java +++ /dev/null @@ -1,105 +0,0 @@ -/* - * SonarQube - * Copyright (C) 2009-2017 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 java.util.ArrayList; -import java.util.List; -import java.util.concurrent.atomic.AtomicInteger; - -public class CsvParser { - - public static String[] parseLine(String line) { - List result = new ArrayList<>(); - - AtomicInteger i = new AtomicInteger(0); - while (true) { - String cell = parseNextCell(line, i); - if (cell == null) - break; - result.add(cell); - } - - return result.toArray(new String[0]); - } - - // returns iterator after delimiter or after end of string - private static String parseNextCell(String line, AtomicInteger i) { - if (i.get() >= line.length()) - return null; - - if (line.charAt(i.get()) != '"') - return parseNotEscapedCell(line, i); - else - return parseEscapedCell(line, i); - } - - // returns iterator after delimiter or after end of string - private static String parseNotEscapedCell(String line, AtomicInteger i) { - StringBuilder sb = new StringBuilder(); - while (true) { - if (i.get() >= line.length()) { - // return iterator after end of string - break; - } - if (line.charAt(i.get()) == ',') { - // return iterator after delimiter - i.incrementAndGet(); - break; - } - sb.append(line.charAt(i.get())); - i.incrementAndGet(); - } - return sb.toString(); - } - - // returns iterator after delimiter or after end of string - private static String parseEscapedCell(String line, AtomicInteger i) { - i.incrementAndGet(); // omit first character (quotation mark) - StringBuilder sb = new StringBuilder(); - while (true) { - if (i.get() >= line.length()) { - break; - } - if (line.charAt(i.get()) == '"') { - i.incrementAndGet(); // we're more interested in the next character - if (i.get() >= line.length()) { - // quotation mark was closing cell - // return iterator after end of string - break; - } - if (line.charAt(i.get()) == ',') { - // quotation mark was closing cell - // return iterator after delimiter - i.incrementAndGet(); - break; - } - if (line.charAt(i.get()) == '"') { - // it was doubled (escaped) quotation mark - // do nothing -- we've already skipped first quotation mark - } - - } - sb.append(line.charAt(i.get())); - i.incrementAndGet(); - } - - return sb.toString(); - } -} 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 35d9ac9b683..a58caf232de 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 @@ -19,20 +19,27 @@ */ package org.sonar.scanner.config; +import java.io.IOException; +import java.io.StringReader; +import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Optional; import javax.annotation.concurrent.Immutable; +import org.apache.commons.csv.CSVFormat; +import org.apache.commons.csv.CSVParser; +import org.apache.commons.csv.CSVRecord; import org.apache.commons.lang.ArrayUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import org.sonar.api.batch.AnalysisMode; import org.sonar.api.config.Configuration; import org.sonar.api.config.Encryption; import org.sonar.api.config.PropertyDefinition; import org.sonar.api.config.PropertyDefinitions; import org.sonar.api.utils.MessageException; +import org.sonar.api.utils.log.Logger; +import org.sonar.api.utils.log.Loggers; import static java.util.Objects.requireNonNull; import static org.apache.commons.lang.StringUtils.trim; @@ -40,7 +47,7 @@ import static org.apache.commons.lang.StringUtils.trim; @Immutable public abstract class DefaultConfiguration implements Configuration { - private static final Logger LOG = LoggerFactory.getLogger(DefaultConfiguration.class); + private static final Logger LOG = Loggers.get(DefaultConfiguration.class); private final PropertyDefinitions definitions; private final Encryption encryption; @@ -83,7 +90,7 @@ public abstract class DefaultConfiguration implements Configuration { String effectiveKey = definitions.validKey(key); PropertyDefinition def = definitions.get(effectiveKey); if (def != null && def.multiValues()) { - LOG.warn("Access to the multi-valued property '{}' should be made using 'getStringArray' method. The SonarQube plugin using this property should be updated."); + LOG.warn("Access to the multi-valued property '{}' should be made using 'getStringArray' method. The SonarQube plugin using this property should be updated.", key); } return getInternal(effectiveKey); } @@ -93,15 +100,32 @@ public abstract class DefaultConfiguration implements Configuration { String effectiveKey = definitions.validKey(key); PropertyDefinition def = definitions.get(effectiveKey); if (def != null && !def.multiValues()) { - LOG.warn("Property '{}' is not declared as multi-valued but was read using 'getStringArray' method. The SonarQube plugin declaring this property should be updated."); + LOG.warn("Property '{}' is not declared as multi-valued but was read using 'getStringArray' method. The SonarQube plugin declaring this property should be updated.", key); } - Optional value = getInternal(key); + Optional value = getInternal(effectiveKey); if (value.isPresent()) { - return CsvParser.parseLine(value.get()); + return parseAsCsv(effectiveKey, value.get()); } return ArrayUtils.EMPTY_STRING_ARRAY; } + public static String[] parseAsCsv(String key, String value) { + List result = new ArrayList<>(); + try (CSVParser csvParser = CSVFormat.RFC4180 + .withHeader((String) null) + .withIgnoreSurroundingSpaces(true) + .parse(new StringReader(value))) { + List records = csvParser.getRecords(); + if (records.isEmpty()) { + return ArrayUtils.EMPTY_STRING_ARRAY; + } + records.get(0).iterator().forEachRemaining(result::add); + return result.toArray(new String[result.size()]); + } catch (IOException e) { + throw new IllegalStateException("Property: '" + key + "' doesn't contain a valid CSV value: '" + value + "'", e); + } + } + private Optional getInternal(String key) { if (mode.isIssues() && key.endsWith(".secured") && !key.contains(".license")) { throw MessageException.of("Access to the secured property '" + key diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/settings/DefaultSettingsLoader.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/settings/DefaultSettingsLoader.java index 00f0f852490..928d9c8ba1f 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/settings/DefaultSettingsLoader.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/settings/DefaultSettingsLoader.java @@ -20,14 +20,15 @@ package org.sonar.scanner.repository.settings; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Joiner; import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import javax.annotation.Nullable; +import org.apache.commons.lang.StringEscapeUtils; import org.sonar.api.utils.log.Logger; import org.sonar.api.utils.log.Loggers; import org.sonar.api.utils.log.Profiler; @@ -76,7 +77,7 @@ public class DefaultSettingsLoader implements SettingsLoader { result.put(s.getKey(), s.getValue()); break; case VALUES: - result.put(s.getKey(), Joiner.on(',').join(s.getValues().getValuesList())); + result.put(s.getKey(), s.getValues().getValuesList().stream().map(StringEscapeUtils::escapeCsv).collect(Collectors.joining(","))); break; case FIELDVALUES: convertPropertySetToProps(result, s); @@ -99,6 +100,6 @@ public class DefaultSettingsLoader implements SettingsLoader { ids.add(String.valueOf(id)); id++; } - result.put(s.getKey(), Joiner.on(',').join(ids)); + result.put(s.getKey(), ids.stream().collect(Collectors.joining(","))); } } diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/ModuleFileSystemInitializer.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/ModuleFileSystemInitializer.java index 6ead3ed7d39..2c5c9e54e8d 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/ModuleFileSystemInitializer.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/ModuleFileSystemInitializer.java @@ -28,6 +28,8 @@ import org.sonar.api.batch.bootstrap.ProjectDefinition; import org.sonar.api.scan.filesystem.PathResolver; import org.sonar.api.utils.TempFolder; +import static org.sonar.scanner.config.DefaultConfiguration.parseAsCsv; + /** * @since 3.5 */ @@ -60,19 +62,25 @@ public class ModuleFileSystemInitializer { } private void initSources(ProjectDefinition module, PathResolver pathResolver) { - for (String sourcePath : module.sources()) { - File dirOrFile = pathResolver.relativeFile(module.getBaseDir(), sourcePath); - if (dirOrFile.exists()) { - sourceDirsOrFiles.add(dirOrFile); + String srcPropValue = module.properties().get(ProjectDefinition.SOURCES_PROPERTY); + if (srcPropValue != null) { + for (String sourcePath : parseAsCsv(ProjectDefinition.SOURCES_PROPERTY, srcPropValue)) { + File dirOrFile = pathResolver.relativeFile(module.getBaseDir(), sourcePath); + if (dirOrFile.exists()) { + sourceDirsOrFiles.add(dirOrFile); + } } } } private void initTests(ProjectDefinition module, PathResolver pathResolver) { - for (String testPath : module.tests()) { - File dirOrFile = pathResolver.relativeFile(module.getBaseDir(), testPath); - if (dirOrFile.exists()) { - testDirsOrFiles.add(dirOrFile); + String testPropValue = module.properties().get(ProjectDefinition.TESTS_PROPERTY); + if (testPropValue != null) { + for (String testPath : parseAsCsv(ProjectDefinition.TESTS_PROPERTY, testPropValue)) { + File dirOrFile = pathResolver.relativeFile(module.getBaseDir(), testPath); + if (dirOrFile.exists()) { + testDirsOrFiles.add(dirOrFile); + } } } } 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 new file mode 100644 index 00000000000..2f65ebceaa0 --- /dev/null +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/config/DefaultConfigurationTest.java @@ -0,0 +1,109 @@ +/* + * SonarQube + * Copyright (C) 2009-2017 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.google.common.collect.ImmutableMap; +import java.util.Arrays; +import org.junit.Rule; +import org.junit.Test; +import org.sonar.api.batch.AnalysisMode; +import org.sonar.api.config.Configuration; +import org.sonar.api.config.Encryption; +import org.sonar.api.config.PropertyDefinition; +import org.sonar.api.config.PropertyDefinitions; +import org.sonar.api.utils.log.LogTester; +import org.sonar.api.utils.log.LoggerLevel; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.fail; +import static org.mockito.Mockito.mock; + +public class DefaultConfigurationTest { + + @Rule + public LogTester logTester = new LogTester(); + + @Test + public void accessingMultiValuedPropertiesShouldBeConsistentWithDeclaration() { + Configuration config = new DefaultConfiguration(new PropertyDefinitions(Arrays.asList( + PropertyDefinition.builder("single").multiValues(false).build(), + PropertyDefinition.builder("multiA").multiValues(true).build())), new Encryption(null), + mock(AnalysisMode.class), + ImmutableMap.of("single", "foo", "multiA", "a,b", "notDeclared", "c,d")) { + }; + + assertThat(config.get("multiA")).hasValue("a,b"); + assertThat(logTester.logs(LoggerLevel.WARN)) + .contains("Access to the multi-valued property 'multiA' should be made using 'getStringArray' method. The SonarQube plugin using this property should be updated."); + + logTester.clear(); + + assertThat(config.getStringArray("single")).containsExactly("foo"); + assertThat(logTester.logs(LoggerLevel.WARN)) + .contains("Property 'single' is not declared as multi-valued but was read using 'getStringArray' method. The SonarQube plugin declaring this property should be updated."); + + logTester.clear(); + + assertThat(config.get("notDeclared")).hasValue("c,d"); + assertThat(config.getStringArray("notDeclared")).containsExactly("c", "d"); + assertThat(logTester.logs(LoggerLevel.WARN)).isEmpty(); + } + + @Test + public void getDefaultValues() { + Configuration config = new DefaultConfiguration(new PropertyDefinitions(Arrays.asList( + PropertyDefinition.builder("single").multiValues(false).defaultValue("default").build(), + PropertyDefinition.builder("multiA").multiValues(true).defaultValue("foo,bar").build())), new Encryption(null), + mock(AnalysisMode.class), + ImmutableMap.of()) { + }; + + assertThat(config.get("multiA")).hasValue("foo,bar"); + assertThat(config.getStringArray("multiA")).containsExactly("foo", "bar"); + assertThat(config.get("single")).hasValue("default"); + assertThat(config.getStringArray("single")).containsExactly("default"); + } + + @Test + public void testParsingMultiValues() { + assertThat(getStringArray("")).isEmpty(); + assertThat(getStringArray(",")).containsExactly("", ""); + assertThat(getStringArray(",,")).containsExactly("", "", ""); + assertThat(getStringArray("a")).containsExactly("a"); + assertThat(getStringArray("a b")).containsExactly("a b"); + assertThat(getStringArray("a , b")).containsExactly("a", "b"); + assertThat(getStringArray("\"a \",\" b\"")).containsExactly("a ", " b"); + assertThat(getStringArray("\"a,b\",c")).containsExactly("a,b", "c"); + try { + getStringArray("\"a ,b"); + fail("Expected exception"); + } catch (Exception e) { + assertThat(e).hasMessage("Property: 'multi' doesn't contain a valid CSV value: '\"a ,b'"); + } + } + + private String[] getStringArray(String value) { + return new DefaultConfiguration(new PropertyDefinitions(Arrays.asList( + PropertyDefinition.builder("multi").multiValues(true).build())), new Encryption(null), + mock(AnalysisMode.class), + ImmutableMap.of("multi", value)) { + }.getStringArray("multi"); + } +} diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/repository/settings/DefaultSettingsLoaderTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/repository/settings/DefaultSettingsLoaderTest.java index 49886eaca04..f2ecbe4d5fb 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/repository/settings/DefaultSettingsLoaderTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/repository/settings/DefaultSettingsLoaderTest.java @@ -40,6 +40,14 @@ public class DefaultSettingsLoaderTest { .containsExactly(entry("sonar.preview.supportedPlugins", "java,php")); } + @Test + public void should_escape_global_multivalue_settings() { + assertThat(DefaultSettingsLoader.toMap(asList(Setting.newBuilder() + .setKey("sonar.preview.supportedPlugins") + .setValues(Values.newBuilder().addValues("ja,va").addValues("p\"hp")).build()))) + .containsExactly(entry("sonar.preview.supportedPlugins", "\"ja,va\",\"p\"\"hp\"")); + } + @Test public void should_load_global_propertyset_settings() { Builder valuesBuilder = Value.newBuilder(); @@ -60,4 +68,5 @@ public class DefaultSettingsLoaderTest { entry("sonar.issue.exclusions.multicriteria.2.filepattern", "**/*.java"), entry("sonar.issue.exclusions.multicriteria.2.rulepattern", "*:S456")); } + } diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/filesystem/ModuleFileSystemInitializerTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/filesystem/ModuleFileSystemInitializerTest.java index 1c6db535f84..4790f48ae6e 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/filesystem/ModuleFileSystemInitializerTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/filesystem/ModuleFileSystemInitializerTest.java @@ -78,6 +78,28 @@ public class ModuleFileSystemInitializerTest { assertThat(path(initializer.tests().get(0))).endsWith("src/test/java"); } + @Test + public void supportFilenamesWithComma() throws IOException { + File baseDir = temp.newFolder("base"); + File sourceFile = new File(baseDir, "my,File.cs"); + sourceFile.createNewFile(); + File testFile = new File(baseDir, "my,TestFile.cs"); + testFile.createNewFile(); + + ProjectDefinition project = ProjectDefinition.create() + .setBaseDir(baseDir) + .addSources("\"my,File.cs\"") + .addTests("\"my,TestFile.cs\""); + + ModuleFileSystemInitializer initializer = new ModuleFileSystemInitializer(project, mock(TempFolder.class), pathResolver); + + assertThat(initializer.baseDir().getCanonicalPath()).isEqualTo(baseDir.getCanonicalPath()); + assertThat(initializer.sources()).hasSize(1); + assertThat(initializer.sources().get(0)).isEqualTo(sourceFile); + assertThat(initializer.tests()).hasSize(1); + assertThat(initializer.tests().get(0)).isEqualTo(testFile); + } + private String path(File f) throws IOException { return FilenameUtils.separatorsToUnix(f.getCanonicalPath()); } -- 2.39.5