Allow @CheckForNull annotations to override `@ParametersAreNo… (#1053)

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.
jenkinsci · Sep 10, 2019 · fc5ee93 · fc5ee93
1 parent 2869786
commit fc5ee93
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 9 deletions.
diff --git 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
@@ -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<T> 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<T> 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/...n/src/test/java/io/jenkins/plugins/casc/impl/configurators/DataBoundConfiguratorTest.java b/...n/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/.../impl/configurators/nonnull/nonnullparampackage/PackageParametersNonNullCheckForNull.java b/.../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;
+    }
+}