From ade52744b495a50138d10da69444d1525b8f39ad Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Tue, 30 May 2017 18:06:05 +0200 Subject: [PATCH] SONAR-9351 enforce in API allowed qualifiers on PropertyDefinition --- .../sonar/api/config/PropertyDefinition.java | 39 +++++- .../api/config/PropertyDefinitionTest.java | 118 +++++++++++++++++- 2 files changed, 148 insertions(+), 9 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 193ae59be35..305fb5492c2 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 @@ -22,14 +22,17 @@ package org.sonar.api.config; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; import java.util.ArrayList; import java.util.Arrays; import java.util.EnumMap; import java.util.List; +import java.util.Set; import java.util.function.Function; import java.util.regex.Pattern; import java.util.regex.PatternSyntaxException; +import java.util.stream.Stream; import javax.annotation.Nullable; import org.apache.commons.lang.StringUtils; import org.apache.commons.lang.math.NumberUtils; @@ -43,6 +46,7 @@ import org.sonar.api.server.ServerSide; import org.sonarsource.api.sonarlint.SonarLintSide; import static com.google.common.base.Preconditions.checkArgument; +import static java.util.Objects.requireNonNull; import static org.apache.commons.lang.StringUtils.isBlank; import static org.sonar.api.PropertyType.BOOLEAN; import static org.sonar.api.PropertyType.FLOAT; @@ -89,6 +93,8 @@ import static org.sonar.api.PropertyType.SINGLE_SELECT_LIST; @ExtensionPoint public final class PropertyDefinition { + private static final Set SUPPORTED_QUALIFIERS = ImmutableSet.of(Qualifiers.PROJECT, Qualifiers.VIEW, Qualifiers.MODULE, Qualifiers.SUBVIEW); + private String key; private String defaultValue; private String name; @@ -430,9 +436,12 @@ public final class PropertyDefinition { *
* See supported constant values in {@link Qualifiers}. By default property is available * only in General Settings. + * + * @throws IllegalArgumentException only qualifiers {@link Qualifiers#PROJECT PROJECT}, {@link Qualifiers#MODULE MODULE}, + * {@link Qualifiers#VIEW VIEW} and {@link Qualifiers#SUBVIEW SVW} are allowed. */ public Builder onQualifiers(String first, String... rest) { - this.onQualifiers.addAll(Lists.asList(first, rest)); + addQualifiers(this.onQualifiers, first, rest); this.global = true; return this; } @@ -446,9 +455,12 @@ public final class PropertyDefinition { *
* See supported constant values in {@link Qualifiers}. By default property is available * only in General Settings. + * + * @throws IllegalArgumentException only qualifiers {@link Qualifiers#PROJECT PROJECT}, {@link Qualifiers#MODULE MODULE}, + * {@link Qualifiers#VIEW VIEW} and {@link Qualifiers#SUBVIEW SVW} are allowed. */ public Builder onQualifiers(List qualifiers) { - this.onQualifiers.addAll(ImmutableList.copyOf(qualifiers)); + addQualifiers(this.onQualifiers, qualifiers); this.global = true; return this; } @@ -462,9 +474,12 @@ public final class PropertyDefinition { *
* See supported constant values in {@link Qualifiers}. By default property is available * only in General Settings. + * + * @throws IllegalArgumentException only qualifiers {@link Qualifiers#PROJECT PROJECT}, {@link Qualifiers#MODULE MODULE}, + * {@link Qualifiers#VIEW VIEW} and {@link Qualifiers#SUBVIEW SVW} are allowed. */ public Builder onlyOnQualifiers(String first, String... rest) { - this.onlyOnQualifiers.addAll(Lists.asList(first, rest)); + addQualifiers(this.onlyOnQualifiers, first, rest); this.global = false; return this; } @@ -478,13 +493,29 @@ public final class PropertyDefinition { *
* See supported constant values in {@link Qualifiers}. By default property is available * only in General Settings. + * + * @throws IllegalArgumentException only qualifiers {@link Qualifiers#PROJECT PROJECT}, {@link Qualifiers#MODULE MODULE}, + * {@link Qualifiers#VIEW VIEW} and {@link Qualifiers#SUBVIEW SVW} are allowed. */ public Builder onlyOnQualifiers(List qualifiers) { - this.onlyOnQualifiers.addAll(ImmutableList.copyOf(qualifiers)); + addQualifiers(this.onlyOnQualifiers, qualifiers); this.global = false; return this; } + private static void addQualifiers(List target, String first, String... rest) { + Stream.concat(Stream.of(first), Arrays.stream(rest)).peek(PropertyDefinition.Builder::validateQualifier).forEach(target::add); + } + + private static void addQualifiers(List target, List qualifiers) { + qualifiers.stream().peek(PropertyDefinition.Builder::validateQualifier).forEach(target::add); + } + + private static void validateQualifier(@Nullable String qualifier) { + requireNonNull(qualifier, "Qualifier cannot be null"); + checkArgument(SUPPORTED_QUALIFIERS.contains(qualifier), "Qualifier must be one of %s", SUPPORTED_QUALIFIERS); + } + /** * @see org.sonar.api.config.PropertyDefinition#type() */ 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 6fd4c01b389..2ba172ed75b 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 @@ -19,6 +19,12 @@ */ package org.sonar.api.config; +import com.google.common.collect.ImmutableSet; +import java.util.Arrays; +import java.util.Collections; +import java.util.Set; +import java.util.function.BiConsumer; +import java.util.function.Consumer; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -29,7 +35,9 @@ import org.sonar.api.PropertyType; import org.sonar.api.resources.Qualifiers; import org.sonar.api.utils.AnnotationUtils; +import static org.apache.commons.lang.RandomStringUtils.randomAlphabetic; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.fail; public class PropertyDefinitionTest { @Rule @@ -50,7 +58,7 @@ public class PropertyDefinitionTest { .options("de", "en") .description("desc") .type(PropertyType.FLOAT) - .onlyOnQualifiers(Qualifiers.FILE) + .onlyOnQualifiers(Qualifiers.MODULE) .multiValues(true) .propertySetKey("set") .build(); @@ -63,7 +71,7 @@ public class PropertyDefinitionTest { assertThat(def.description()).isEqualTo("desc"); assertThat(def.type()).isEqualTo(PropertyType.FLOAT); assertThat(def.global()).isFalse(); - assertThat(def.qualifiers()).containsOnly(Qualifiers.FILE); + assertThat(def.qualifiers()).containsOnly(Qualifiers.MODULE); assertThat(def.multiValues()).isTrue(); assertThat(def.propertySetKey()).isEqualTo("set"); assertThat(def.fields()).isEmpty(); @@ -309,7 +317,7 @@ public class PropertyDefinitionTest { thrown.expect(IllegalArgumentException.class); thrown.expectMessage("Cannot be hidden and defining qualifiers on which to display"); - PropertyDefinition.builder("foo").name("foo").onQualifiers(Qualifiers.FILE).hidden().build(); + PropertyDefinition.builder("foo").name("foo").onQualifiers(Qualifiers.VIEW).hidden().build(); } @Test @@ -317,7 +325,7 @@ public class PropertyDefinitionTest { thrown.expect(IllegalArgumentException.class); thrown.expectMessage("Cannot be hidden and defining qualifiers on which to display"); - PropertyDefinition.builder("foo").name("foo").onlyOnQualifiers(Qualifiers.FILE).hidden().build(); + PropertyDefinition.builder("foo").name("foo").onlyOnQualifiers(Qualifiers.PROJECT).hidden().build(); } @Test @@ -325,7 +333,107 @@ public class PropertyDefinitionTest { thrown.expect(IllegalArgumentException.class); thrown.expectMessage("Cannot define both onQualifiers and onlyOnQualifiers"); - PropertyDefinition.builder("foo").name("foo").onQualifiers(Qualifiers.FILE).onlyOnQualifiers(Qualifiers.PROJECT).build(); + PropertyDefinition.builder("foo").name("foo").onQualifiers(Qualifiers.MODULE).onlyOnQualifiers(Qualifiers.PROJECT).build(); + } + + private static final Set ALLOWED_QUALIFIERS = ImmutableSet.of("TRK", "VW", "BRC", "SVW"); + private static final Set NOT_ALLOWED_QUALIFIERS = ImmutableSet.of("FIL", "DIR", "UTS", "", randomAlphabetic(3)); + + @Test + public void onQualifiers_with_varargs_parameter_fails_with_IAE_when_qualifier_is_not_supported() { + failsWithIAEForUnsupportedQualifiers((builder, qualifier) -> builder.onQualifiers(qualifier)); + failsWithIAEForUnsupportedQualifiers((builder, qualifier) -> builder.onQualifiers("TRK", qualifier, "BRC")); + } + + @Test + public void onQualifiers_with_list_parameter_fails_with_IAE_when_qualifier_is_not_supported() { + failsWithIAEForUnsupportedQualifiers((builder, qualifier) -> builder.onQualifiers(Collections.singletonList(qualifier))); + failsWithIAEForUnsupportedQualifiers((builder, qualifier) -> builder.onQualifiers(Arrays.asList("TRK", qualifier, "BRC"))); + } + + @Test + public void onlyOnQualifiers_with_varargs_parameter_fails_with_IAE_when_qualifier_is_not_supported() { + failsWithIAEForUnsupportedQualifiers((builder, qualifier) -> builder.onlyOnQualifiers(qualifier)); + failsWithIAEForUnsupportedQualifiers((builder, qualifier) -> builder.onlyOnQualifiers("TRK", qualifier, "BRC")); + } + + @Test + public void onlyOnQualifiers_with_list_parameter_fails_with_IAE_when_qualifier_is_not_supported() { + failsWithIAEForUnsupportedQualifiers((builder, qualifier) -> builder.onlyOnQualifiers(Collections.singletonList(qualifier))); + failsWithIAEForUnsupportedQualifiers((builder, qualifier) -> builder.onlyOnQualifiers(Arrays.asList("TRK", qualifier, "BRC"))); + } + + @Test + public void onQualifiers_with_varargs_parameter_fails_with_NPE_when_qualifier_is_null() { + failsWithNPEForNullQualifiers(builder -> builder.onQualifiers((String) null)); + failsWithNPEForNullQualifiers(builder -> builder.onQualifiers("TRK", null, "BRC")); + } + + @Test + public void onQualifiers_with_list_parameter_fails_with_NPE_when_qualifier_is_null() { + failsWithNPEForNullQualifiers(builder -> builder.onQualifiers(Collections.singletonList(null))); + failsWithNPEForNullQualifiers(builder -> builder.onlyOnQualifiers("TRK", null, "BRC")); + } + + @Test + public void onlyOnQualifiers_with_varargs_parameter_fails_with_NPE_when_qualifier_is_null() { + failsWithNPEForNullQualifiers(builder -> builder.onlyOnQualifiers((String) null)); + failsWithNPEForNullQualifiers(builder -> builder.onlyOnQualifiers("TRK", null, "BRC")); + } + + @Test + public void onlyOnQualifiers_with_list_parameter_fails_with_NPE_when_qualifier_is_null() { + failsWithNPEForNullQualifiers(builder -> builder.onlyOnQualifiers(Collections.singletonList(null))); + failsWithNPEForNullQualifiers(builder -> builder.onlyOnQualifiers(Arrays.asList("TRK", null, "BRC"))); + } + + @Test + public void onQualifiers_with_varargs_parameter_accepts_supported_qualifiers() { + acceptsSupportedQualifiers((builder, qualifier) -> builder.onQualifiers(qualifier)); + } + + @Test + public void onQualifiers_with_list_parameter_accepts_supported_qualifiers() { + acceptsSupportedQualifiers((builder, qualifier) -> builder.onQualifiers(Collections.singletonList(qualifier))); + } + + @Test + public void onlyOnQualifiers_with_varargs_parameter_accepts_supported_qualifiers() { + acceptsSupportedQualifiers((builder, qualifier) -> builder.onlyOnQualifiers(qualifier)); + } + + @Test + public void onlyOnQualifiers_with_list_parameter_accepts_supported_qualifiers() { + acceptsSupportedQualifiers((builder, qualifier) -> builder.onlyOnQualifiers(Collections.singletonList(qualifier))); + } + + private static void failsWithIAEForUnsupportedQualifiers(BiConsumer biConsumer) { + PropertyDefinition.Builder builder = PropertyDefinition.builder(randomAlphabetic(3)); + NOT_ALLOWED_QUALIFIERS.forEach(qualifier -> { + try { + biConsumer.accept(builder, qualifier); + fail("A IllegalArgumentException should have been thrown for qualifier " + qualifier); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessage("Qualifier must be one of [TRK, VW, BRC, SVW]"); + } + }); + } + + private static void acceptsSupportedQualifiers(BiConsumer biConsumer) { + PropertyDefinition.Builder builder = PropertyDefinition.builder(randomAlphabetic(3)); + ALLOWED_QUALIFIERS.forEach(qualifier -> biConsumer.accept(builder, qualifier)); + } + + private static void failsWithNPEForNullQualifiers(Consumer consumer) { + PropertyDefinition.Builder builder = PropertyDefinition.builder(randomAlphabetic(3)); + NOT_ALLOWED_QUALIFIERS.forEach(qualifier -> { + try { + consumer.accept(builder); + fail("A NullPointerException should have been thrown for null qualifier"); + } catch (NullPointerException e) { + assertThat(e).hasMessage("Qualifier cannot be null"); + } + }); } @Properties(@Property(key = "hello", name = "Hello", defaultValue = "world", description = "desc", -- 2.39.5