SONAR-7970 WS settings/set forbid empty values with a clean message

diff --git a/server/sonar-server/src/main/java/org/sonar/server/setting/ws/SetAction.java b/server/sonar-server/src/main/java/org/sonar/server/setting/ws/SetAction.java
index 957180725316bd43746002b32fde54424e645aca..38fbb074e17628859f18419292dfedb075d54b82 100644 (file)
--- a/server/sonar-server/src/main/java/org/sonar/server/setting/ws/SetAction.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/setting/ws/SetAction.java
@@ -36,6 +36,7 @@ import java.util.stream.Collector;
 import java.util.stream.Collectors;
 import java.util.stream.IntStream;
 import javax.annotation.Nullable;
+import org.apache.commons.lang.StringUtils;
 import org.sonar.api.PropertyType;
 import org.sonar.api.config.PropertyDefinition;
 import org.sonar.api.config.PropertyDefinitions;
@@ -59,7 +60,6 @@ import org.sonar.server.user.UserSession;
 import org.sonar.server.ws.KeyExamples;
 import org.sonarqube.ws.client.setting.SetRequest;
 
-import static com.google.common.base.Strings.isNullOrEmpty;
 import static org.sonar.server.ws.WsUtils.checkRequest;
 import static org.sonarqube.ws.client.setting.SettingsWsParameters.ACTION_SET;
 import static org.sonarqube.ws.client.setting.SettingsWsParameters.PARAM_COMPONENT_ID;
@@ -71,6 +71,7 @@ import static org.sonarqube.ws.client.setting.SettingsWsParameters.PARAM_VALUES;
 
 public class SetAction implements SettingsWsAction {
   private static final Collector<CharSequence, ?, String> COMMA_JOINER = Collectors.joining(",");
+  private static final String MSG_NO_EMPTY_VALUE = "A non empty value must be provided";
 
   private final PropertyDefinitions propertyDefinitions;
   private final DbClient dbClient;
@@ -217,12 +218,10 @@ public class SetAction implements SettingsWsAction {
 
     request.getFieldValues().stream()
       .map(oneFieldValues -> readOneFieldValues(oneFieldValues, request.getKey()))
+      .peek(map -> checkRequest(map.values().stream().anyMatch(StringUtils::isNotBlank), MSG_NO_EMPTY_VALUE))
       .flatMap(map -> map.entrySet().stream())
       .peek(entry -> valuesByFieldKeys.put(entry.getKey(), entry.getValue()))
-      .forEach(entry -> {
-        checkRequest(fieldKeys.contains(entry.getKey()), "Unknown field key '%s' for setting '%s'", entry.getKey(), request.getKey());
-        checkRequest(entry.getValue() != null, "Parameter field '%s' must not be null", entry.getKey());
-      });
+      .forEach(entry -> checkRequest(fieldKeys.contains(entry.getKey()), "Unknown field key '%s' for setting '%s'", entry.getKey(), request.getKey()));
 
     checkFieldType(request, definition, valuesByFieldKeys);
   }
@@ -253,10 +252,13 @@ public class SetAction implements SettingsWsAction {
   }
 
   private static void checkValueIsSet(SetRequest request) {
-    checkRequest(!isNullOrEmpty(request.getValue())
-      ^ !request.getValues().isEmpty()
-      ^ !request.getFieldValues().isEmpty(),
+    checkRequest(
+      request.getValue() != null
+        ^ !request.getValues().isEmpty()
+        ^ !request.getFieldValues().isEmpty(),
       "One and only one of '%s', '%s', '%s' must be provided", PARAM_VALUE, PARAM_VALUES, PARAM_FIELD_VALUES);
+    checkRequest(request.getValues().stream().allMatch(StringUtils::isNotBlank), MSG_NO_EMPTY_VALUE);
+    checkRequest(request.getValue() == null || StringUtils.isNotBlank(request.getValue()), MSG_NO_EMPTY_VALUE);
   }
 
   private static List<String> valuesFromRequest(SetRequest request) {

diff --git a/server/sonar-server/src/test/java/org/sonar/server/setting/ws/SetActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/setting/ws/SetActionTest.java
index ce7f1f2d649f0dc379689365b39d87d497070fe8..5d23b359fd0369fd64f2d19961d098168ae1c6fb 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/setting/ws/SetActionTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/setting/ws/SetActionTest.java
@@ -405,11 +405,20 @@ public class SetActionTest {
   @Test
   public void fail_when_empty_value() {
     expectedException.expect(BadRequestException.class);
-    expectedException.expectMessage("One and only one of 'value', 'values', 'fieldValues' must be provided");
+    expectedException.expectMessage("A non empty value must be provided");
 
     callForGlobalSetting("my.key", "");
   }
 
+  @Test
+  public void fail_when_one_empty_value_on_multi_value() {
+    expectedException.expect(BadRequestException.class);
+    expectedException.expectMessage("A non empty value must be provided");
+
+    callForMultiValueGlobalSetting("my.key", newArrayList("oneValue", "  ", "anotherValue"));
+
+  }
+
   @Test
   public void fail_when_insufficient_privileges() {
     userSession.anonymous().setGlobalPermissions(GlobalPermissions.QUALITY_PROFILE_ADMIN);
@@ -576,6 +585,65 @@ public class SetActionTest {
     callForMultiValueGlobalSetting("my.key", newArrayList("My Value"));
   }
 
+  @Test
+  public void fail_when_empty_values_on_one_property_set() {
+    definitions.addComponent(PropertyDefinition
+      .builder("my.key")
+      .name("foo")
+      .description("desc")
+      .category("cat")
+      .subCategory("subCat")
+      .type(PropertyType.PROPERTY_SET)
+      .defaultValue("default")
+      .fields(newArrayList(
+        PropertyFieldDefinition.build("firstField")
+          .name("First Field")
+          .type(PropertyType.STRING)
+          .build(),
+        PropertyFieldDefinition.build("secondField")
+          .name("Second Field")
+          .type(PropertyType.STRING)
+          .build()))
+      .build());
+
+    expectedException.expect(BadRequestException.class);
+    expectedException.expectMessage("A non empty value must be provided");
+
+    callForGlobalPropertySet("my.key", newArrayList(
+      GSON.toJson(ImmutableMap.of("firstField", "firstValue", "secondField", "secondValue")),
+      GSON.toJson(ImmutableMap.of("firstField", "", "secondField", "")),
+      GSON.toJson(ImmutableMap.of("firstField", "yetFirstValue", "secondField", "yetSecondValue"))));
+  }
+
+  @Test
+  public void do_not_fail_when_only_one_empty_value_on_one_property_set() {
+    definitions.addComponent(PropertyDefinition
+      .builder("my.key")
+      .name("foo")
+      .description("desc")
+      .category("cat")
+      .subCategory("subCat")
+      .type(PropertyType.PROPERTY_SET)
+      .defaultValue("default")
+      .fields(newArrayList(
+        PropertyFieldDefinition.build("firstField")
+          .name("First Field")
+          .type(PropertyType.STRING)
+          .build(),
+        PropertyFieldDefinition.build("secondField")
+          .name("Second Field")
+          .type(PropertyType.STRING)
+          .build()))
+      .build());
+
+    callForGlobalPropertySet("my.key", newArrayList(
+      GSON.toJson(ImmutableMap.of("firstField", "firstValue", "secondField", "secondValue")),
+      GSON.toJson(ImmutableMap.of("firstField", "anotherFirstValue", "secondField", "")),
+      GSON.toJson(ImmutableMap.of("firstField", "yetFirstValue", "secondField", "yetSecondValue"))));
+
+    assertGlobalSetting("my.key", "1,2,3");
+  }
+
   @Test
   public void fail_when_property_set_setting_is_not_defined() {
     expectedException.expect(BadRequestException.class);
@@ -648,7 +716,7 @@ public class SetActionTest {
       .build());
 
     expectedException.expect(BadRequestException.class);
-    expectedException.expectMessage("Parameter field 'field' must not be null");
+    expectedException.expectMessage("A non empty value must be provided");
 
     callForGlobalPropertySet("my.key", newArrayList("{\"field\": null}"));
   }

diff --git a/sonar-core/src/main/resources/org/sonar/l10n/core.properties b/sonar-core/src/main/resources/org/sonar/l10n/core.properties
index 8c0c489c251b6a3e6c75cd751c15dec9bad2eef7..2f86ef35f56a13e4e17dcd75a956d35449b22dd0 100644 (file)
--- a/sonar-core/src/main/resources/org/sonar/l10n/core.properties
+++ b/sonar-core/src/main/resources/org/sonar/l10n/core.properties
@@ -1928,7 +1928,7 @@ settings.not_set=(not set)
 settings.state.saving=Saving...
 settings.state.saved=Saved!
 settings.state.validation_failed=Validation failed. {0}
-settings.state.value_cant_be_empty=Value can't be empty. Use "Reset" to set value to the default one.
+settings.state.value_cant_be_empty=Provide a value or use "Reset" to set the value to the default one.
 settings._default=(default)
 settings.boolean.true=True
 settings.boolean.false=False