SONAR-9283 Settings, Configuration and Props trim values at insert

6 files changed, 211 insertions, 17 deletions

diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinitions.java b/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinitions.java
index b0e5275674f..4ba15a7f239 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinitions.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinitions.java
@@ -35,6 +35,8 @@ import org.sonar.api.ce.ComputeEngineSide;
 import org.sonar.api.server.ServerSide;
 import org.sonar.api.utils.AnnotationUtils;
 
+import static java.util.Objects.requireNonNull;
+
 /**
  * Metadata of all the properties declared by plugins
  *
@@ -127,7 +129,9 @@ public final class PropertyDefinitions {
   }
 
   public String validKey(String key) {
-    return StringUtils.defaultString(deprecatedKeys.get(key), key);
+    requireNonNull(key, "key can't be null");
+    String trimmedKey = key.trim();
+    return StringUtils.defaultString(deprecatedKeys.get(trimmedKey), trimmedKey);
   }
 
   /**
diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/config/Settings.java b/sonar-plugin-api/src/main/java/org/sonar/api/config/Settings.java
index 20fb15bff45..05cbf19a726 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/config/Settings.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/config/Settings.java
@@ -60,6 +60,11 @@ public abstract class Settings {
 
   protected abstract Optional<String> get(String key);
 
+  /**
+   * Add the settings with the specified key and value, both are trimmed and neither can be null.
+   *
+   * @throws NullPointerException if {@code key} and/or {@code value} is {@code null}.
+   */
   protected abstract void set(String key, String value);
 
   protected abstract void remove(String key);
@@ -265,7 +270,8 @@ public abstract class Settings {
    * </ul>
    */
   public String[] getStringArray(String key) {
-    Optional<PropertyDefinition> def = getDefinition(key);
+    String effectiveKey = definitions.validKey(key);
+    Optional<PropertyDefinition> def = getDefinition(effectiveKey);
     if ((def.isPresent()) && (def.get().multiValues())) {
       String value = getString(key);
       if (value == null) {
@@ -324,7 +330,9 @@ public abstract class Settings {
   }
 
   public Settings setProperty(String key, @Nullable String[] values) {
-    Optional<PropertyDefinition> def = getDefinition(key);
+    requireNonNull(key, "key can't be null");
+    String effectiveKey = key.trim();
+    Optional<PropertyDefinition> def = getDefinition(effectiveKey);
     if (!def.isPresent() || (!def.get().multiValues())) {
       throw new IllegalStateException("Fail to set multiple values on a single value property " + key);
     }
@@ -361,7 +369,6 @@ public abstract class Settings {
     String validKey = definitions.validKey(key);
     if (value == null) {
       removeProperty(validKey);
-
     } else {
       set(validKey, trim(value));
     }
diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/config/internal/MapSettings.java b/sonar-plugin-api/src/main/java/org/sonar/api/config/internal/MapSettings.java
index 9a58e9eb6a0..6990165fc3c 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/config/internal/MapSettings.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/config/internal/MapSettings.java
@@ -28,6 +28,7 @@ import org.sonar.api.config.PropertyDefinitions;
 import org.sonar.api.config.Settings;
 
 import static java.util.Collections.unmodifiableMap;
+import static java.util.Objects.requireNonNull;
 
 /**
  * In-memory map-based implementation of {@link Settings}. It must be used
@@ -58,7 +59,9 @@ public class MapSettings extends Settings {
 
   @Override
   protected void set(String key, String value) {
-    props.put(key, value);
+    props.put(
+      requireNonNull(key, "key can't be null"),
+      requireNonNull(value, "value can't be null").trim());
   }
 
   @Override
diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/config/ConfigurationTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/config/ConfigurationTest.java
index 1f712844412..6950f8dd5fc 100644
--- a/sonar-plugin-api/src/test/java/org/sonar/api/config/ConfigurationTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/config/ConfigurationTest.java
@@ -82,7 +82,7 @@ public class ConfigurationTest {
     private final Map<String, String> keyValues = new HashMap<>();
 
     public Configuration put(String key, String value) {
-      keyValues.put(key, value);
+      keyValues.put(key, value.trim());
       return this;
     }
 
diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/config/PropertyDefinitionsTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/config/PropertyDefinitionsTest.java
index 649ace7b722..b0d01e89132 100644
--- a/sonar-plugin-api/src/test/java/org/sonar/api/config/PropertyDefinitionsTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/config/PropertyDefinitionsTest.java
@@ -21,22 +21,29 @@ package org.sonar.api.config;
 
 import java.util.Arrays;
 import java.util.List;
+import java.util.Random;
+import java.util.stream.IntStream;
+import org.apache.commons.lang.RandomStringUtils;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.sonar.api.Properties;
 import org.sonar.api.Property;
 import org.sonar.api.resources.Qualifiers;
 
+import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
 
 public class PropertyDefinitionsTest {
+  @Rule
+  public ExpectedException expectedException = ExpectedException.none();
 
   @Test
   public void should_build_with_predefined_list_of_definitions() {
     List<PropertyDefinition> list = Arrays.asList(
       PropertyDefinition.builder("foo").name("Foo").build(),
       PropertyDefinition.builder("one").name("One").build(),
-      PropertyDefinition.builder("two").name("Two").defaultValue("2").build()
-    );
+      PropertyDefinition.builder("two").name("Two").defaultValue("2").build());
     PropertyDefinitions def = new PropertyDefinitions(list);
 
     assertProperties(def);
@@ -47,8 +54,7 @@ public class PropertyDefinitionsTest {
     PropertyDefinitions def = new PropertyDefinitions(
       PropertyDefinition.builder("foo").name("Foo").build(),
       PropertyDefinition.builder("one").name("One").build(),
-      PropertyDefinition.builder("two").name("Two").defaultValue("2").build()
-    );
+      PropertyDefinition.builder("two").name("Two").defaultValue("2").build());
 
     assertProperties(def);
   }
@@ -71,8 +77,7 @@ public class PropertyDefinitionsTest {
   public void test_categories() {
     PropertyDefinitions def = new PropertyDefinitions(
       PropertyDefinition.builder("inCateg").name("In Categ").category("categ").build(),
-      PropertyDefinition.builder("noCateg").name("No categ").build()
-    );
+      PropertyDefinition.builder("noCateg").name("No categ").build());
 
     assertThat(def.getCategory("inCateg")).isEqualTo("categ");
     assertThat(def.getCategory("noCateg")).isEmpty();
@@ -125,8 +130,7 @@ public class PropertyDefinitionsTest {
       PropertyDefinition.builder("project").name("Project").category("catProject").onlyOnQualifiers(Qualifiers.PROJECT).build(),
       PropertyDefinition.builder("module").name("Module").category("catModule").onlyOnQualifiers(Qualifiers.MODULE).build(),
       PropertyDefinition.builder("view").name("View").category("catView").onlyOnQualifiers(Qualifiers.VIEW).build(),
-      PropertyDefinition.builder("app").name("Application").category("catApp").onlyOnQualifiers(Qualifiers.APP).build()
-    );
+      PropertyDefinition.builder("app").name("Application").category("catApp").onlyOnQualifiers(Qualifiers.APP).build());
 
     assertThat(def.propertiesByCategory(null).keySet()).contains(new Category("catGlobal1"), new Category("catGlobal2"));
     assertThat(def.propertiesByCategory(Qualifiers.PROJECT).keySet()).containsOnly(new Category("catProject"));
@@ -142,8 +146,7 @@ public class PropertyDefinitionsTest {
       PropertyDefinition.builder("global1").name("Global1").category("catGlobal1").subCategory("sub1").build(),
       PropertyDefinition.builder("global2").name("Global2").category("catGlobal1").subCategory("sub2").build(),
       PropertyDefinition.builder("global3").name("Global3").category("catGlobal1").build(),
-      PropertyDefinition.builder("global4").name("Global4").category("catGlobal2").build()
-    );
+      PropertyDefinition.builder("global4").name("Global4").category("catGlobal2").build());
 
     assertThat(def.propertiesByCategory(null).get(new Category("catGlobal1")).keySet()).containsOnly(new SubCategory("catGlobal1"), new SubCategory("sub1"),
       new SubCategory("sub2"));
@@ -171,6 +174,48 @@ public class PropertyDefinitionsTest {
     assertThat(definitions.getAll().size()).isEqualTo(3);
   }
 
+  @Test
+  public void validKey_throws_NPE_if_key_is_null() {
+    PropertyDefinitions underTest = new PropertyDefinitions();
+
+    expectedException.expect(NullPointerException.class);
+    expectedException.expectMessage("key can't be null");
+
+    underTest.validKey(null);
+  }
+
+  @Test
+  public void get_throws_NPE_if_key_is_null() {
+    PropertyDefinitions underTest = new PropertyDefinitions();
+
+    expectedException.expect(NullPointerException.class);
+    expectedException.expectMessage("key can't be null");
+
+    underTest.get(null);
+  }
+
+  @Test
+  public void get_trims_key_before_looking_for_replacement() {
+    Random random = new Random();
+    String key = RandomStringUtils.randomAlphanumeric(4);
+    String deprecatedKey = RandomStringUtils.randomAlphanumeric(4);
+    PropertyDefinitions underTest = new PropertyDefinitions(singletonList(
+      PropertyDefinition.builder(key)
+        .deprecatedKey(deprecatedKey)
+        .build()));
+
+    String untrimmedKey = blank(random) + deprecatedKey + blank(random);
+    assertThat(underTest.get(untrimmedKey).key())
+        .describedAs("expecting key %s being returned for get(%s)", key, untrimmedKey)
+        .isEqualTo(key);
+  }
+
+  private static String blank(Random random) {
+    StringBuilder b = new StringBuilder();
+    IntStream.range(0, random.nextInt(3)).mapToObj(s -> " ").forEach(b::append);
+    return b.toString();
+  }
+
   @Property(key = "foo", name = "Foo")
   static final class PluginWithProperty {
   }
diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/config/internal/MapSettingsTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/config/internal/MapSettingsTest.java
index e28a73cdb9c..48e6a67aece 100644
--- a/sonar-plugin-api/src/test/java/org/sonar/api/config/internal/MapSettingsTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/config/internal/MapSettingsTest.java
@@ -19,23 +19,37 @@
  */
 package org.sonar.api.config.internal;
 
+import com.tngtech.java.junit.dataprovider.DataProvider;
+import com.tngtech.java.junit.dataprovider.DataProviderRunner;
+import com.tngtech.java.junit.dataprovider.UseDataProvider;
+import java.util.Arrays;
 import java.util.Date;
+import java.util.List;
+import java.util.Random;
+import java.util.function.BiConsumer;
+import java.util.stream.IntStream;
 import org.assertj.core.data.Offset;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
 import org.sonar.api.Properties;
 import org.sonar.api.Property;
 import org.sonar.api.PropertyType;
+import org.sonar.api.config.PropertyDefinition;
 import org.sonar.api.config.PropertyDefinitions;
 import org.sonar.api.config.Settings;
-import org.sonar.api.config.internal.MapSettings;
 import org.sonar.api.utils.DateUtils;
 
+import static java.util.Collections.singletonList;
+import static org.apache.commons.lang.RandomStringUtils.randomAlphanumeric;
 import static org.assertj.core.api.Assertions.assertThat;
 
+@RunWith(DataProviderRunner.class)
 public class MapSettingsTest {
+  @Rule
+  public ExpectedException expectedException = ExpectedException.none();
 
   private PropertyDefinitions definitions;
 
@@ -66,12 +80,133 @@ public class MapSettingsTest {
   }
 
   @Test
+  public void set_throws_NPE_if_key_is_null() {
+    MapSettings underTest = new MapSettings();
+
+    expectKeyNullNPE();
+
+    underTest.set(null, randomAlphanumeric(3));
+  }
+
+  @Test
+  public void set_throws_NPE_if_value_is_null() {
+    MapSettings underTest = new MapSettings();
+
+    expectedException.expect(NullPointerException.class);
+    expectedException.expectMessage("value can't be null");
+
+    underTest.set(randomAlphanumeric(3), null);
+  }
+
+  @Test
+  public void set_accepts_empty_value_and_trims_it() {
+    MapSettings underTest = new MapSettings();
+    Random random = new Random();
+    String key = randomAlphanumeric(3);
+
+    underTest.set(key, blank(random));
+
+    assertThat(underTest.getString(key)).isEmpty();
+  }
+
+  @Test
   public void default_values_should_be_loaded_from_definitions() {
     Settings settings = new MapSettings(definitions);
     assertThat(settings.getDefaultValue("hello")).isEqualTo("world");
   }
 
   @Test
+  @UseDataProvider("setPropertyCalls")
+  public void all_setProperty_methods_throws_NPE_if_key_is_null(BiConsumer<Settings, String> setPropertyCaller) {
+    Settings settings = new MapSettings();
+
+    expectKeyNullNPE();
+
+    setPropertyCaller.accept(settings, null);
+  }
+
+  @Test
+  public void set_property_string_throws_NPE_if_key_is_null() {
+    String key = randomAlphanumeric(3);
+
+    Settings underTest = new MapSettings(new PropertyDefinitions(singletonList(PropertyDefinition.builder(key).multiValues(true).build())));
+
+    expectKeyNullNPE();
+
+    underTest.setProperty(null, new String[] {"1", "2"});
+  }
+
+  private void expectKeyNullNPE() {
+    expectedException.expect(NullPointerException.class);
+    expectedException.expectMessage("key can't be null");
+  }
+
+  @Test
+  @UseDataProvider("setPropertyCalls")
+  public void all_set_property_methods_trims_key(BiConsumer<Settings, String> setPropertyCaller) {
+    Settings underTest = new MapSettings();
+
+    Random random = new Random();
+    String blankBefore = blank(random);
+    String blankAfter = blank(random);
+    String key = randomAlphanumeric(3);
+
+    setPropertyCaller.accept(underTest, blankBefore + key + blankAfter);
+
+    assertThat(underTest.hasKey(key)).isTrue();
+  }
+
+  @Test
+  public void set_property_string_array_trims_key() {
+    String key = randomAlphanumeric(3);
+
+    Settings underTest = new MapSettings(new PropertyDefinitions(singletonList(PropertyDefinition.builder(key).multiValues(true).build())));
+
+    Random random = new Random();
+    String blankBefore = blank(random);
+    String blankAfter = blank(random);
+
+    underTest.setProperty(blankBefore + key + blankAfter, new String[] {"1", "2"});
+
+    assertThat(underTest.hasKey(key)).isTrue();
+  }
+
+  private static String blank(Random random) {
+    StringBuilder b = new StringBuilder();
+    IntStream.range(0, random.nextInt(3)).mapToObj(s -> " ").forEach(b::append);
+    return b.toString();
+  }
+
+  @DataProvider
+  public static Object[][] setPropertyCalls() {
+    List<BiConsumer<Settings, String>> callers = Arrays.asList(
+      (settings, key) -> settings.setProperty(key, 123),
+      (settings, key) -> settings.setProperty(key, 123L),
+      (settings, key) -> settings.setProperty(key, 123.2F),
+      (settings, key) -> settings.setProperty(key, 123.2D),
+      (settings, key) -> settings.setProperty(key, false),
+      (settings, key) -> settings.setProperty(key, new Date()),
+      (settings, key) -> settings.setProperty(key, new Date(), true));
+
+    return callers.stream().map(t -> new Object[] {t}).toArray(Object[][]::new);
+  }
+
+  @Test
+  public void setProperty_methods_trims_value() {
+    Settings underTest = new MapSettings();
+
+    Random random = new Random();
+    String blankBefore = blank(random);
+    String blankAfter = blank(random);
+    String key = randomAlphanumeric(3);
+    String value = randomAlphanumeric(3);
+
+    underTest.setProperty(key, blankBefore + value + blankAfter);
+
+    assertThat(underTest.getString(key)).isEqualTo(value);
+  }
+
+  @Test
   public void set_property_int() {
     Settings settings = new MapSettings();
     settings.setProperty("foo", 123);