From: Teryk Bellahsene Date: Fri, 26 Aug 2016 11:43:08 +0000 (+0200) Subject: SONAR-8003 WS settings/set handles multi value settings X-Git-Tag: 6.1-RC1~283 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=ac946d0b0e7633eb9ac9c98cf6b1adcb932abf35;p=sonarqube.git SONAR-8003 WS settings/set handles multi value settings --- diff --git a/server/sonar-server/src/main/java/org/sonar/server/settings/ws/SetAction.java b/server/sonar-server/src/main/java/org/sonar/server/settings/ws/SetAction.java index c7fb7ac44e7..25136c0a62b 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/settings/ws/SetAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/settings/ws/SetAction.java @@ -20,8 +20,12 @@ package org.sonar.server.settings.ws; +import com.google.common.base.Joiner; +import java.util.Collections; +import java.util.List; import java.util.Locale; import java.util.Optional; +import java.util.stream.Collectors; import org.sonar.api.config.PropertyDefinition; import org.sonar.api.config.PropertyDefinitions; import org.sonar.api.i18n.I18n; @@ -40,15 +44,17 @@ 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; import static org.sonarqube.ws.client.setting.SettingsWsParameters.PARAM_COMPONENT_KEY; import static org.sonarqube.ws.client.setting.SettingsWsParameters.PARAM_KEY; import static org.sonarqube.ws.client.setting.SettingsWsParameters.PARAM_VALUE; - -import static org.sonar.server.ws.WsUtils.checkRequest; +import static org.sonarqube.ws.client.setting.SettingsWsParameters.PARAM_VALUES; public class SetAction implements SettingsWsAction { + private static final Joiner COMMA_JOINER = Joiner.on(","); private final PropertyDefinitions propertyDefinitions; private final I18n i18n; private final DbClient dbClient; @@ -67,12 +73,15 @@ public class SetAction implements SettingsWsAction { public void define(WebService.NewController context) { WebService.NewAction action = context.createAction(ACTION_SET) .setDescription("Update a setting value.
" + + "Either '%s' or '%s' must be provided, not both.
" + "Either '%s' or '%s' can be provided, not both.
" + "Requires one of the following permissions: " + "", PARAM_COMPONENT_ID, PARAM_COMPONENT_KEY) + "", + PARAM_VALUE, PARAM_VALUES, + PARAM_COMPONENT_ID, PARAM_COMPONENT_KEY) .setSince("6.1") .setPost(true) .setHandler(this); @@ -84,8 +93,11 @@ public class SetAction implements SettingsWsAction { action.createParam(PARAM_VALUE) .setDescription("Setting value. To reset a value, please use the reset web service.") - .setExampleValue("git@github.com:SonarSource/sonarqube.git") - .setRequired(true); + .setExampleValue("git@github.com:SonarSource/sonarqube.git"); + + action.createParam(PARAM_VALUES) + .setDescription("Setting multi value. To set several values, the parameter must be called once for each value.") + .setExampleValue("values=firstValue&values=secondValue&values=thirdValue"); action.createParam(PARAM_COMPONENT_ID) .setDescription("Component id") @@ -120,19 +132,55 @@ public class SetAction implements SettingsWsAction { return; } - PropertyDefinition.Result result = definition.validate(request.getValue()); + checkType(request, definition); + checkSingleOrMultiValue(request, definition); + checkGlobalOrProject(request, definition, component); + checkComponentQualifier(request, definition, component); + } - checkRequest(result.isValid(), - i18n.message(Locale.ENGLISH, "property.error." + result.getErrorKey(), "Error when validating setting with key '%s' and value '%s'"), - request.getKey(), request.getValue()); + private static void checkSingleOrMultiValue(SetRequest request, PropertyDefinition definition) { + checkRequest(definition.multiValues() ^ request.getValue() != null, + "Parameter '%s' must be used for single value setting. Parameter '%s' must be used for multi value setting.", PARAM_VALUE, PARAM_VALUES); + } + private static void checkGlobalOrProject(SetRequest request, PropertyDefinition definition, Optional component) { checkRequest(component.isPresent() || definition.global(), "Setting '%s' cannot be global", request.getKey()); + } + + private void checkComponentQualifier(SetRequest request, PropertyDefinition definition, Optional component) { String qualifier = component.isPresent() ? component.get().qualifier() : ""; checkRequest(!component.isPresent() - || definition.qualifiers().contains(component.get().qualifier()), + || definition.qualifiers().contains(component.get().qualifier()), "Setting '%s' cannot be set on a %s", request.getKey(), i18n.message(Locale.ENGLISH, "qualifier." + qualifier, null)); } + private void checkType(SetRequest request, PropertyDefinition definition) { + List values = valuesFromRequest(request); + Optional failingResult = values.stream() + .map(definition::validate) + .filter(result -> !result.isValid()) + .findAny(); + String errorKey = failingResult.isPresent() ? failingResult.get().getErrorKey() : null; + checkRequest(errorKey == null, + i18n.message(Locale.ENGLISH, "property.error." + errorKey, "Error when validating setting with key '%s' and value '%s'"), + request.getKey(), request.getValue()); + } + + private static void checkValueIsSet(SetRequest request) { + checkRequest(isNullOrEmpty(request.getValue()) ^ request.getValues().isEmpty(), + "Either '%s' or '%s' must be provided, not both", PARAM_VALUE, PARAM_VALUES); + } + + private static List valuesFromRequest(SetRequest request) { + return request.getValue() == null ? request.getValues() : Collections.singletonList(request.getValue()); + } + + private static String persistedValue(SetRequest request) { + return request.getValue() == null + ? COMMA_JOINER.join(request.getValues().stream().map(value -> value.replace(",", "%2C")).collect(Collectors.toList())) + : request.getValue(); + } + private void checkPermissions(Optional component) { if (component.isPresent()) { userSession.checkComponentUuidPermission(UserRole.ADMIN, component.get().uuid()); @@ -142,12 +190,17 @@ public class SetAction implements SettingsWsAction { } private static SetRequest toWsRequest(Request request) { - return SetRequest.builder() + SetRequest setRequest = SetRequest.builder() .setKey(request.mandatoryParam(PARAM_KEY)) - .setValue(request.mandatoryParam(PARAM_VALUE)) + .setValue(request.param(PARAM_VALUE)) + .setValues(request.multiParam(PARAM_VALUES)) .setComponentId(request.param(PARAM_COMPONENT_ID)) .setComponentKey(request.param(PARAM_COMPONENT_KEY)) .build(); + + checkValueIsSet(setRequest); + + return setRequest; } private Optional searchComponent(DbSession dbSession, SetRequest request) { @@ -164,9 +217,11 @@ public class SetAction implements SettingsWsAction { PropertyDefinition definition = propertyDefinitions.get(request.getKey()); // handles deprecated key but persist the new key String key = definition == null ? request.getKey() : definition.key(); + String value = persistedValue(request); + PropertyDto property = new PropertyDto() .setKey(key) - .setValue(request.getValue()); + .setValue(value); if (component.isPresent()) { property.setResourceId(component.get().getId()); diff --git a/server/sonar-server/src/test/java/org/sonar/server/settings/ws/SetActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/settings/ws/SetActionTest.java index 6c1e01e85cd..afb06287fc5 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/settings/ws/SetActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/settings/ws/SetActionTest.java @@ -52,6 +52,7 @@ import org.sonar.server.ws.TestRequest; import org.sonar.server.ws.TestResponse; import org.sonar.server.ws.WsActionTester; +import static com.google.common.collect.Lists.newArrayList; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.groups.Tuple.tuple; import static org.sonar.db.component.ComponentTesting.newView; @@ -93,9 +94,9 @@ public class SetActionTest { @Test public void persist_new_global_property() { - callForGlobalProperty("my.key", "my value"); + callForGlobalProperty("my.key", "my,value"); - assertGlobalProperty("my.key", "my value"); + assertGlobalProperty("my.key", "my,value"); } @Test @@ -141,6 +142,20 @@ public class SetActionTest { assertProjectProperty("my.key", "my new project value", project.getId()); } + @Test + public void persist_several_multi_value_property() { + callForMultiValueGlobalProperty("my.key", newArrayList("first,Value", "second,Value", "third,Value")); + + assertGlobalProperty("my.key", "first%2CValue,second%2CValue,third%2CValue"); + } + + @Test + public void persist_one_multi_value_property() { + callForMultiValueGlobalProperty("my.key", newArrayList("first,Value")); + + assertGlobalProperty("my.key", "first%2CValue"); + } + @Test public void user_property_is_not_updated() { propertyDb.insertProperty(newGlobalPropertyDto("my.key", "my user value").setUserId(42L)); @@ -180,22 +195,23 @@ public class SetActionTest { @Test public void fail_when_empty_key_value() { expectedException.expect(IllegalArgumentException.class); - expectedException.expectMessage("Setting key is mandatory and must not be empty."); + expectedException.expectMessage("Setting key is mandatory and must not be empty"); callForGlobalProperty(" ", "my value"); } @Test public void fail_when_no_value() { - expectedException.expect(IllegalArgumentException.class); + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Either 'value' or 'values' must be provided, not both"); callForGlobalProperty("my.key", null); } @Test public void fail_when_empty_value() { - expectedException.expect(IllegalArgumentException.class); - expectedException.expectMessage("Setting value is mandatory and must not be empty."); + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Either 'value' or 'values' must be provided, not both"); callForGlobalProperty("my.key", ""); } @@ -281,6 +297,48 @@ public class SetActionTest { callForProjectPropertyByUuid("my.key", "My Value", view.uuid()); } + @Test + public void fail_when_single_and_multi_value_provided() { + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Either 'value' or 'values' must be provided, not both"); + + call("my.key", "My Value", newArrayList("Another Value"), null, null); + } + + @Test + public void fail_when_multi_definition_and_single_value_provided() { + propertyDefinitions.addComponent(PropertyDefinition + .builder("my.key") + .name("foo") + .description("desc") + .category("cat") + .type(PropertyType.STRING) + .multiValues(true) + .build()); + + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Parameter 'value' must be used for single value setting. Parameter 'values' must be used for multi value setting."); + + callForGlobalProperty("my.key", "My Value"); + } + + @Test + public void fail_when_single_definition_and_multi_value_provided() { + propertyDefinitions.addComponent(PropertyDefinition + .builder("my.key") + .name("foo") + .description("desc") + .category("cat") + .type(PropertyType.STRING) + .multiValues(false) + .build()); + + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Parameter 'value' must be used for single value setting. Parameter 'values' must be used for multi value setting."); + + callForMultiValueGlobalProperty("my.key", newArrayList("My Value")); + } + @Test public void definition() { WebService.Action definition = ws.getDef(); @@ -289,7 +347,7 @@ public class SetActionTest { assertThat(definition.isPost()).isTrue(); assertThat(definition.since()).isEqualTo("6.1"); assertThat(definition.params()).extracting(Param::key) - .containsOnlyOnce("key", "value"); + .containsOnly("key", "value", "values", "componentId", "componentKey"); } private void assertGlobalProperty(String key, String value) { @@ -317,18 +375,22 @@ public class SetActionTest { } private void callForGlobalProperty(@Nullable String key, @Nullable String value) { - call(key, value, null, null); + call(key, value, null, null, null); + } + + private void callForMultiValueGlobalProperty(@Nullable String key, @Nullable List values) { + call(key, null, values, null, null); } private void callForProjectPropertyByUuid(@Nullable String key, @Nullable String value, @Nullable String componentUuid) { - call(key, value, componentUuid, null); + call(key, value, null, componentUuid, null); } private void callForProjectPropertyByKey(@Nullable String key, @Nullable String value, @Nullable String componentKey) { - call(key, value, null, componentKey); + call(key, value, null, null, componentKey); } - private void call(@Nullable String key, @Nullable String value, @Nullable String componentUuid, @Nullable String componentKey) { + private void call(@Nullable String key, @Nullable String value, @Nullable List values, @Nullable String componentUuid, @Nullable String componentKey) { TestRequest request = ws.newRequest(); if (key != null) { @@ -339,6 +401,10 @@ public class SetActionTest { request.setParam("value", value); } + if (values != null) { + request.setMultiParam("values", values); + } + if (componentUuid != null) { request.setParam("componentId", componentUuid); } diff --git a/server/sonar-server/src/test/java/org/sonar/server/ws/TestRequest.java b/server/sonar-server/src/test/java/org/sonar/server/ws/TestRequest.java index 69a49043eb0..8d4ef2fac3f 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/ws/TestRequest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/ws/TestRequest.java @@ -20,6 +20,8 @@ package org.sonar.server.ws; import com.google.common.base.Throwables; +import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.ListMultimap; import com.google.common.collect.Maps; import java.io.InputStream; import java.util.HashMap; @@ -30,11 +32,11 @@ import org.sonar.api.server.ws.internal.PartImpl; import org.sonar.api.server.ws.internal.ValidatingRequest; import static com.google.common.base.Preconditions.checkNotNull; -import static java.util.Collections.emptyList; -import static java.util.Collections.singletonList; +import static java.util.Objects.requireNonNull; public class TestRequest extends ValidatingRequest { + private final ListMultimap multiParams = ArrayListMultimap.create(); private final Map params = new HashMap<>(); private final Map parts = Maps.newHashMap(); private String method = "GET"; @@ -48,8 +50,7 @@ public class TestRequest extends ValidatingRequest { @Override protected List readMultiParam(String key) { - String value = params.get(key); - return value == null ? emptyList() : singletonList(value); + return multiParams.get(key); } @Override @@ -115,6 +116,15 @@ public class TestRequest extends ValidatingRequest { return this; } + public TestRequest setMultiParam(String key, List values) { + requireNonNull(key); + requireNonNull(values); + + multiParams.putAll(key, values); + + return this; + } + public TestResponse execute() { try { DumbResponse response = new DumbResponse(); diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/server/ws/Request.java b/sonar-plugin-api/src/main/java/org/sonar/api/server/ws/Request.java index bf32ff65ad8..c9d23cb6386 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/server/ws/Request.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/server/ws/Request.java @@ -111,9 +111,7 @@ public abstract class Request { public List mandatoryMultiParam(String key) { List values = multiParam(key); - if (values.isEmpty()) { - throw new IllegalArgumentException(String.format("The '%s' parameter is missing", key)); - } + checkArgument(!values.isEmpty(), "The '%s' parameter is missing", key); return values; } diff --git a/sonar-ws/src/main/java/org/sonarqube/ws/client/setting/SetRequest.java b/sonar-ws/src/main/java/org/sonarqube/ws/client/setting/SetRequest.java index ee19ef6fc1d..ab5c1bd340e 100644 --- a/sonar-ws/src/main/java/org/sonarqube/ws/client/setting/SetRequest.java +++ b/sonar-ws/src/main/java/org/sonarqube/ws/client/setting/SetRequest.java @@ -20,20 +20,24 @@ package org.sonarqube.ws.client.setting; +import java.util.List; import javax.annotation.CheckForNull; import javax.annotation.Nullable; import static com.google.common.base.Preconditions.checkArgument; +import static java.util.Collections.emptyList; public class SetRequest { private final String key; private final String value; + private final List values; private final String componentId; private final String componentKey; public SetRequest(Builder builder) { this.key = builder.key; this.value = builder.value; + this.values = builder.values; this.componentId = builder.componentId; this.componentKey = builder.componentKey; } @@ -42,10 +46,15 @@ public class SetRequest { return key; } + @CheckForNull public String getValue() { return value; } + public List getValues() { + return values; + } + @CheckForNull public String getComponentId() { return componentId; @@ -63,6 +72,7 @@ public class SetRequest { public static class Builder { private String key; private String value; + private List values = emptyList(); private String componentId; private String componentKey; @@ -75,11 +85,16 @@ public class SetRequest { return this; } - public Builder setValue(String value) { + public Builder setValue(@Nullable String value) { this.value = value; return this; } + public Builder setValues(List values) { + this.values = values; + return this; + } + public Builder setComponentId(@Nullable String componentId) { this.componentId = componentId; return this; @@ -91,8 +106,8 @@ public class SetRequest { } public SetRequest build() { - checkArgument(key != null && !key.isEmpty(), "Setting key is mandatory and must not be empty."); - checkArgument(value != null && !value.isEmpty(), "Setting value is mandatory and must not be empty."); + checkArgument(key != null && !key.isEmpty(), "Setting key is mandatory and must not be empty"); + checkArgument(values != null, "Setting values must not be null"); return new SetRequest(this); } } diff --git a/sonar-ws/src/main/java/org/sonarqube/ws/client/setting/SettingsWsParameters.java b/sonar-ws/src/main/java/org/sonarqube/ws/client/setting/SettingsWsParameters.java index 74daf45d569..5c6e35d243d 100644 --- a/sonar-ws/src/main/java/org/sonarqube/ws/client/setting/SettingsWsParameters.java +++ b/sonar-ws/src/main/java/org/sonarqube/ws/client/setting/SettingsWsParameters.java @@ -33,6 +33,7 @@ public class SettingsWsParameters { public static final String PARAM_KEYS = "keys"; public static final String PARAM_KEY = "key"; public static final String PARAM_VALUE = "value"; + public static final String PARAM_VALUES = "values"; private SettingsWsParameters() { // Only static stuff diff --git a/sonar-ws/src/test/java/org/sonarqube/ws/client/setting/SetRequestTest.java b/sonar-ws/src/test/java/org/sonarqube/ws/client/setting/SetRequestTest.java index b1e48020a30..461daa1d29f 100644 --- a/sonar-ws/src/test/java/org/sonarqube/ws/client/setting/SetRequestTest.java +++ b/sonar-ws/src/test/java/org/sonarqube/ws/client/setting/SetRequestTest.java @@ -39,6 +39,7 @@ public class SetRequestTest { assertThat(result.getKey()).isEqualTo("my.key"); assertThat(result.getValue()).isEqualTo("my value"); + assertThat(result.getValues()).isNotNull().isEmpty(); assertThat(result.getComponentKey()).isNull(); assertThat(result.getComponentId()).isNull(); } @@ -66,6 +67,7 @@ public class SetRequestTest { @Test public void fail_when_empty_key() { expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Setting key is mandatory and must not be empty"); underTest .setKey("") @@ -74,13 +76,10 @@ public class SetRequestTest { } @Test - public void fail_when_empty_value() { + public void fail_when_values_is_null() { expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Setting values must not be null"); - underTest - .setKey("key") - .setValue(null) - .build(); + underTest.setKey("my.key").setValues(null).build(); } - }