From dc148a71a1c184ccad588b66251980c994879dff Mon Sep 17 00:00:00 2001 From: Teryk Bellahsene Date: Tue, 4 Oct 2016 16:45:59 +0200 Subject: [PATCH] SONAR-7970 WS settings/set forbid empty values with a clean message --- .../sonar/server/setting/ws/SetAction.java | 18 ++--- .../server/setting/ws/SetActionTest.java | 72 ++++++++++++++++++- .../resources/org/sonar/l10n/core.properties | 2 +- 3 files changed, 81 insertions(+), 11 deletions(-) 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 95718072531..38fbb074e17 100644 --- 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 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 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 ce7f1f2d649..5d23b359fd0 100644 --- 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 8c0c489c251..2f86ef35f56 100644 --- 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 -- 2.39.5