Forbid setting both onQualifiers and onlyOnQualifiers

2 files changed, 70 insertions, 35 deletions

diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinition.java b/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinition.java
index 0c55c02fb3b..c9d6834344c 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinition.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/config/PropertyDefinition.java
@@ -80,7 +80,8 @@ public final class PropertyDefinition implements BatchExtension, ServerExtension
     this.propertySetKey = builder.propertySetKey;
     this.fields = builder.fields;
     this.deprecatedKey = builder.deprecatedKey;
-    this.qualifiers = builder.qualifiers;
+    this.qualifiers = builder.onQualifiers;
+    this.qualifiers.addAll(builder.onlyOnQualifiers);
     this.index = builder.index;
   }
 
@@ -288,7 +289,8 @@ public final class PropertyDefinition implements BatchExtension, ServerExtension
     private String defaultValue;
     private String category;
     private String subcategory;
-    private List<String> qualifiers;
+    private List<String> onQualifiers;
+    private List<String> onlyOnQualifiers;
     private boolean global;
     private PropertyType type;
     private List<String> options;
@@ -310,7 +312,8 @@ public final class PropertyDefinition implements BatchExtension, ServerExtension
       this.deprecatedKey = "";
       this.global = true;
       this.type = PropertyType.STRING;
-      this.qualifiers = newArrayList();
+      this.onQualifiers = newArrayList();
+      this.onlyOnQualifiers = newArrayList();
       this.options = newArrayList();
       this.fields = newArrayList();
       this.hidden = false;
@@ -343,25 +346,25 @@ public final class PropertyDefinition implements BatchExtension, ServerExtension
     }
 
     public Builder onQualifiers(String first, String... rest) {
-      this.qualifiers = Lists.asList(first, rest);
+      this.onQualifiers.addAll(Lists.asList(first, rest));
       this.global = true;
       return this;
     }
 
     public Builder onQualifiers(List<String> qualifiers) {
-      this.qualifiers = ImmutableList.copyOf(qualifiers);
+      this.onQualifiers.addAll(ImmutableList.copyOf(qualifiers));
       this.global = true;
       return this;
     }
 
     public Builder onlyOnQualifiers(String first, String... rest) {
-      this.qualifiers = Lists.asList(first, rest);
+      this.onlyOnQualifiers.addAll(Lists.asList(first, rest));
       this.global = false;
       return this;
     }
 
     public Builder onlyOnQualifiers(List<String> qualifiers) {
-      this.qualifiers = ImmutableList.copyOf(qualifiers);
+      this.onlyOnQualifiers.addAll(ImmutableList.copyOf(qualifiers));
       this.global = false;
       return this;
     }
@@ -419,7 +422,8 @@ public final class PropertyDefinition implements BatchExtension, ServerExtension
     public PropertyDefinition build() {
       Preconditions.checkArgument(!Strings.isNullOrEmpty(key), "Key must be set");
       fixType(key, type);
-      Preconditions.checkArgument(!hidden || qualifiers.isEmpty(), "Cannot be hidden and defining qualifiers on which to display");
+      Preconditions.checkArgument(onQualifiers.isEmpty() || onlyOnQualifiers.isEmpty(), "Cannot define both onQualifiers and onlyOnQualifiers");
+      Preconditions.checkArgument((!hidden || (onQualifiers.isEmpty()) && (!hidden || (onlyOnQualifiers.isEmpty()))), "Cannot be hidden and defining qualifiers on which to display");
       if (hidden) {
         global = false;
       }
diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/config/PropertyDefinitionTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/config/PropertyDefinitionTest.java
index 35ee091cdaa..84978ff1795 100644
--- a/sonar-plugin-api/src/test/java/org/sonar/api/config/PropertyDefinitionTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/config/PropertyDefinitionTest.java
@@ -28,22 +28,23 @@ import org.sonar.api.resources.Qualifiers;
 import org.sonar.api.utils.AnnotationUtils;
 
 import static org.fest.assertions.Assertions.assertThat;
+import static org.fest.assertions.Fail.fail;
 
 public class PropertyDefinitionTest {
 
   @Test
   public void should_create_property() {
     PropertyDefinition def = PropertyDefinition.builder("hello")
-        .name("Hello")
-        .defaultValue("world")
-        .category("categ")
-        .options("de", "en")
-        .description("desc")
-        .type(PropertyType.FLOAT)
-        .onlyOnQualifiers(Qualifiers.FILE, Qualifiers.CLASS)
-        .multiValues(true)
-        .propertySetKey("set")
-        .build();
+      .name("Hello")
+      .defaultValue("world")
+      .category("categ")
+      .options("de", "en")
+      .description("desc")
+      .type(PropertyType.FLOAT)
+      .onlyOnQualifiers(Qualifiers.FILE, Qualifiers.CLASS)
+      .multiValues(true)
+      .propertySetKey("set")
+      .build();
 
     assertThat(def.key()).isEqualTo("hello");
     assertThat(def.name()).isEqualTo("Hello");
@@ -95,8 +96,8 @@ public class PropertyDefinitionTest {
   @Test
   public void should_create_property_with_default_values() {
     PropertyDefinition def = PropertyDefinition.builder("hello")
-        .name("Hello")
-        .build();
+      .name("Hello")
+      .build();
 
     assertThat(def.key()).isEqualTo("hello");
     assertThat(def.name()).isEqualTo("Hello");
@@ -136,12 +137,12 @@ public class PropertyDefinitionTest {
   @Test
   public void should_support_property_sets() {
     PropertyDefinition def = PropertyDefinition.builder("hello")
-        .name("Hello")
-        .fields(
-            PropertyFieldDefinition.build("first").name("First").description("Description").options("A", "B").build(),
-            PropertyFieldDefinition.build("second").name("Second").type(PropertyType.INTEGER).indicativeSize(5).build()
-        )
-        .build();
+      .name("Hello")
+      .fields(
+        PropertyFieldDefinition.build("first").name("First").description("Description").options("A", "B").build(),
+        PropertyFieldDefinition.build("second").name("Second").type(PropertyType.INTEGER).indicativeSize(5).build()
+      )
+      .build();
 
     assertThat(def.fields()).hasSize(2);
     assertThat(def.fields().get(0).key()).isEqualTo("first");
@@ -259,24 +260,54 @@ public class PropertyDefinitionTest {
     assertThat(def.type()).isEqualTo(PropertyType.LICENSE);
   }
 
-  @Test(expected = IllegalArgumentException.class)
-  public void should_not_define_qualifier_and_hidden() {
-    PropertyDefinition.builder("foo").name("foo").onQualifiers(Qualifiers.FILE).hidden().build();
+  @Test
+  public void should_not_authorise_empty_key() {
+    try {
+      PropertyDefinition.builder(null).build();
+      fail();
+    } catch (Exception e) {
+      assertThat(e).hasMessage("Key must be set").isInstanceOf(IllegalArgumentException.class);
+    }
+  }
+
+  @Test
+  public void should_not_authorize_defining_on_qualifiers_and_hidden() {
+    try {
+      PropertyDefinition.builder("foo").name("foo").onQualifiers(Qualifiers.FILE).hidden().build();
+      fail();
+    } catch (Exception e) {
+      assertThat(e).hasMessage("Cannot be hidden and defining qualifiers on which to display").isInstanceOf(IllegalArgumentException.class);
+    }
   }
 
-  @Test(expected = IllegalArgumentException.class)
-  public void should_not_authorise_empty_key() {
-    PropertyDefinition.builder(null).build();
+  @Test
+  public void should_not_authorize_defining_ony_on_qualifiers_and_hidden() {
+    try {
+      PropertyDefinition.builder("foo").name("foo").onlyOnQualifiers(Qualifiers.FILE).hidden().build();
+      fail();
+    } catch (Exception e) {
+      assertThat(e).hasMessage("Cannot be hidden and defining qualifiers on which to display").isInstanceOf(IllegalArgumentException.class);
+    }
+  }
+
+  @Test
+  public void should_not_authorize_defining_on_qualifiers_and_only_on_qualifiers() {
+    try {
+      PropertyDefinition.builder("foo").name("foo").onQualifiers(Qualifiers.FILE).onlyOnQualifiers(Qualifiers.PROJECT).build();
+      fail();
+    } catch (Exception e) {
+      assertThat(e).hasMessage("Cannot define both onQualifiers and onlyOnQualifiers").isInstanceOf(IllegalArgumentException.class);
+    }
   }
 
   @Properties(@Property(key = "hello", name = "Hello", defaultValue = "world", description = "desc",
-      options = {"de", "en"}, category = "categ", type = PropertyType.FLOAT, global = false, project = true, module = true, multiValues = true, propertySetKey = "set"))
+    options = {"de", "en"}, category = "categ", type = PropertyType.FLOAT, global = false, project = true, module = true, multiValues = true, propertySetKey = "set"))
   static class Init {
   }
 
   @Properties(@Property(key = "hello", name = "Hello", fields = {
-      @PropertyField(key = "first", name = "First", description = "Description", options = {"A", "B"}),
-      @PropertyField(key = "second", name = "Second", type = PropertyType.INTEGER, indicativeSize = 5)}))
+    @PropertyField(key = "first", name = "First", description = "Description", options = {"A", "B"}),
+    @PropertyField(key = "second", name = "Second", type = PropertyType.INTEGER, indicativeSize = 5)}))
   static class WithPropertySet {
   }