From fc5ee937d666426cb1a0106d43c9c1f8c162d4bb Mon Sep 17 00:00:00 2001 From: Tim Jacomb Date: Tue, 10 Sep 2019 13:04:11 +0100 Subject: [PATCH] =?UTF-8?q?Allow=20`@CheckForNull`=20annotations=20to=20ov?= =?UTF-8?q?erride=20`@ParametersAreNo=E2=80=A6=20(#1053)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously if someone had put ParametersAreNonnullByDefault on a class / method / package-info there was no way to override and say this field should be nullable. --- .../io/jenkins/plugins/casc/SonarQubeTest.yml | 1 + .../configurators/DataBoundConfigurator.java | 7 ++--- .../DataBoundConfiguratorTest.java | 30 +++++++++++++++---- .../PackageParametersNonNullCheckForNull.java | 22 ++++++++++++++ 4 files changed, 51 insertions(+), 9 deletions(-) create mode 100644 plugin/src/test/java/io/jenkins/plugins/casc/impl/configurators/nonnull/nonnullparampackage/PackageParametersNonNullCheckForNull.java diff --git a/integrations/src/test/resources/io/jenkins/plugins/casc/SonarQubeTest.yml b/integrations/src/test/resources/io/jenkins/plugins/casc/SonarQubeTest.yml index 6f8ff54c02..0bd5ab1c7b 100644 --- a/integrations/src/test/resources/io/jenkins/plugins/casc/SonarQubeTest.yml +++ b/integrations/src/test/resources/io/jenkins/plugins/casc/SonarQubeTest.yml @@ -6,6 +6,7 @@ unclassified: serverUrl: "http://url:9000" serverAuthenticationToken: "token" mojoVersion: "mojoVersion" + additionalProperties: "blah=blah" additionalAnalysisProperties: "additionalAnalysisProperties" triggers: skipScmCause: true diff --git a/plugin/src/main/java/io/jenkins/plugins/casc/impl/configurators/DataBoundConfigurator.java b/plugin/src/main/java/io/jenkins/plugins/casc/impl/configurators/DataBoundConfigurator.java index cfe2b78e5d..72baea392b 100644 --- a/plugin/src/main/java/io/jenkins/plugins/casc/impl/configurators/DataBoundConfigurator.java +++ b/plugin/src/main/java/io/jenkins/plugins/casc/impl/configurators/DataBoundConfigurator.java @@ -1,6 +1,5 @@ package io.jenkins.plugins.casc.impl.configurators; -import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; import hudson.model.Descriptor; import hudson.util.Secret; @@ -26,6 +25,7 @@ import java.util.Set; import java.util.logging.Level; import java.util.logging.Logger; +import javax.annotation.CheckForNull; import javax.annotation.Nonnull; import javax.annotation.ParametersAreNonnullByDefault; import javax.annotation.PostConstruct; @@ -118,7 +118,8 @@ private T tryConstructor(Constructor constructor, Mapping config, Configurati if (value == null && (parameters[i].isAnnotationPresent(Nonnull.class) || constructor.isAnnotationPresent(ParametersAreNonnullByDefault.class) || clazz.isAnnotationPresent(ParametersAreNonnullByDefault.class) || - clazz.getPackage().isAnnotationPresent(ParametersAreNonnullByDefault.class))) { + clazz.getPackage().isAnnotationPresent(ParametersAreNonnullByDefault.class) && + !parameters[i].isAnnotationPresent(CheckForNull.class))) { if (Set.class.isAssignableFrom(t)) { LOGGER.log(Level.FINER, "The parameter to be set is @Nonnull but is not present; " + @@ -128,8 +129,6 @@ private T tryConstructor(Constructor constructor, Mapping config, Configurati LOGGER.log(Level.FINER, "The parameter to be set is @Nonnull but is not present; " + "setting equal to empty list."); args[i] = Collections.emptyList(); - } else if (String.class.isAssignableFrom(t)) { - args[i] = ""; } else { throw new ConfiguratorException(names[i] + " is required to configure " + target); } diff --git a/plugin/src/test/java/io/jenkins/plugins/casc/impl/configurators/DataBoundConfiguratorTest.java b/plugin/src/test/java/io/jenkins/plugins/casc/impl/configurators/DataBoundConfiguratorTest.java index b87454b3cd..63c7179456 100644 --- a/plugin/src/test/java/io/jenkins/plugins/casc/impl/configurators/DataBoundConfiguratorTest.java +++ b/plugin/src/test/java/io/jenkins/plugins/casc/impl/configurators/DataBoundConfiguratorTest.java @@ -9,6 +9,7 @@ import io.jenkins.plugins.casc.impl.configurators.nonnull.ClassParametersAreNonnullByDefault; import io.jenkins.plugins.casc.impl.configurators.nonnull.NonnullParameterConstructor; import io.jenkins.plugins.casc.impl.configurators.nonnull.nonnullparampackage.PackageParametersAreNonnullByDefault; +import io.jenkins.plugins.casc.impl.configurators.nonnull.nonnullparampackage.PackageParametersNonNullCheckForNull; import io.jenkins.plugins.casc.misc.Util; import io.jenkins.plugins.casc.model.CNode; import io.jenkins.plugins.casc.model.Mapping; @@ -23,6 +24,7 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.LoggerRule; @@ -36,6 +38,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -51,6 +54,9 @@ public class DataBoundConfiguratorTest { @Rule public LoggerRule logging = new LoggerRule(); + @Rule + public ExpectedException exceptionRule = ExpectedException.none(); + @Before public void tearUp() { logging.record(Logger.getLogger(DataBoundConfigurator.class.getName()), Level.FINEST).capture(2048); @@ -138,17 +144,31 @@ public void classParametersAreNonnullByDefault() throws Exception { final ClassParametersAreNonnullByDefault configured = (ClassParametersAreNonnullByDefault) registry .lookupOrFail(ClassParametersAreNonnullByDefault.class) .configure(config, new ConfigurationContext(registry)); - assertEquals(0, configured.getStrings().size()); + assertTrue(configured.getStrings().isEmpty()); } @Test public void packageParametersAreNonnullByDefault() throws Exception { Mapping config = new Mapping(); ConfiguratorRegistry registry = ConfiguratorRegistry.get(); - final PackageParametersAreNonnullByDefault configured = (PackageParametersAreNonnullByDefault) registry - .lookupOrFail(PackageParametersAreNonnullByDefault.class) - .configure(config, new ConfigurationContext(registry)); - assertTrue(configured.getString().isEmpty()); + + exceptionRule.expect(ConfiguratorException.class); + exceptionRule.expectMessage("string is required to configure class io.jenkins.plugins.casc.impl.configurators.nonnull.nonnullparampackage.PackageParametersAreNonnullByDefault"); + + registry + .lookupOrFail(PackageParametersAreNonnullByDefault.class) + .configure(config, new ConfigurationContext(registry)); + } + + @Test + @Issue("#1025") + public void packageParametersAreNonnullByDefaultButCanBeNullable() throws Exception { + Mapping config = new Mapping(); + ConfiguratorRegistry registry = ConfiguratorRegistry.get(); + final PackageParametersNonNullCheckForNull configured = (PackageParametersNonNullCheckForNull) registry + .lookupOrFail(PackageParametersNonNullCheckForNull.class) + .configure(config, new ConfigurationContext(registry)); + assertNull(configured.getSecret()); } @Test diff --git a/plugin/src/test/java/io/jenkins/plugins/casc/impl/configurators/nonnull/nonnullparampackage/PackageParametersNonNullCheckForNull.java b/plugin/src/test/java/io/jenkins/plugins/casc/impl/configurators/nonnull/nonnullparampackage/PackageParametersNonNullCheckForNull.java new file mode 100644 index 0000000000..660028db76 --- /dev/null +++ b/plugin/src/test/java/io/jenkins/plugins/casc/impl/configurators/nonnull/nonnullparampackage/PackageParametersNonNullCheckForNull.java @@ -0,0 +1,22 @@ +package io.jenkins.plugins.casc.impl.configurators.nonnull.nonnullparampackage; + +import hudson.util.Secret; +import javax.annotation.CheckForNull; +import org.kohsuke.stapler.DataBoundConstructor; + +/** + * Checks the behaviour of {@link io.jenkins.plugins.casc.impl.configurators.DataBoundConfigurator} with non null + * {@link Secret} when using package-level {@link javax.annotation.ParametersAreNonnullByDefault} annotations. + */ +public class PackageParametersNonNullCheckForNull { + private Secret secret; + + @DataBoundConstructor + public PackageParametersNonNullCheckForNull(@CheckForNull Secret secret) { + this.secret = secret; + } + + public Secret getSecret() { + return secret; + } +}