SONAR-8003 WS settings/set handles multi value settings

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.<br>" +
+        "Either '%s' or '%s' must be provided, not both.<br> " +
         "Either '%s' or '%s' can be provided, not both.<br> " +
         "Requires one of the following permissions: " +
         "<ul>" +
         "<li>'Administer System'</li>" +
         "<li>'Administer' rights on the specified component</li>" +
-        "</ul>", PARAM_COMPONENT_ID, PARAM_COMPONENT_KEY)
+        "</ul>",
+        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<ComponentDto> component) {
     checkRequest(component.isPresent() || definition.global(), "Setting '%s' cannot be global", request.getKey());
+  }
+
+  private void checkComponentQualifier(SetRequest request, PropertyDefinition definition, Optional<ComponentDto> 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<String> values = valuesFromRequest(request);
+    Optional<PropertyDefinition.Result> 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<String> 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<ComponentDto> 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<ComponentDto> 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());

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<String> 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<String> 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);
     }

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<String, String> multiParams = ArrayListMultimap.create();
   private final Map<String, String> params = new HashMap<>();
   private final Map<String, Part> parts = Maps.newHashMap();
   private String method = "GET";
@@ -48,8 +50,7 @@ public class TestRequest extends ValidatingRequest {
 
   @Override
   protected List<String> 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<String> values) {
+    requireNonNull(key);
+    requireNonNull(values);
+
+    multiParams.putAll(key, values);
+
+    return this;
+  }
+
   public TestResponse execute() {
     try {
       DumbResponse response = new DumbResponse();

sonar-plugin-api/src/main/java/org/sonar/api/server/ws/Request.java

@@ -111,9 +111,7 @@ public abstract class Request {
 
   public List<String> mandatoryMultiParam(String key) {
     List<String> 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;
   }

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<String> 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<String> 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<String> 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<String> 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);
     }
   }

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

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();
   }
-
 }