[JENKINS-42717] - Prevent NPE when a non-existent Default View is spe…

…cified in the global config (#2815) * [JENKINS-42717] - Document view management methods in Jenkins and ViewGroupMixIn * [KENKINS-42717] - GlobalDefauldViewConfiguration should not fail with NPE when the view is missing * [JENKINS-42717] - Draft the direct unit test * [JENKINS-42717] - Fix the tes implementation * [JENKINS-42717] - Make FormException localizable * [JENKINS-42717] - Fix te build glitch (cherry picked from commit 4074818)
jenkinsci · May 2, 2017 · 0e2cadf · 0e2cadf
1 parent 161095e
commit 0e2cadf
Show file tree

Hide file tree

Showing 6 changed files with 180 additions and 4 deletions.
diff --git a/core/src/main/java/hudson/model/ViewGroupMixIn.java b/core/src/main/java/hudson/model/ViewGroupMixIn.java
@@ -35,6 +35,7 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import javax.annotation.CheckForNull;
 
 /**
  * Implements {@link ViewGroup} to be used as a "mix-in".
@@ -91,7 +92,14 @@ public synchronized void deleteView(View view) throws IOException {
         owner.save();
     }
 
-    public View getView(String name) {
+    /**
+     * Gets a view by the specified name.
+     * The method iterates through {@link ViewGroup}s if required.
+     * @param name Name of the view
+     * @return View instance or {@code null} if it is missing
+     */
+    @CheckForNull
+    public View getView(@CheckForNull String name) {
         for (View v : views()) {
             if(v.getViewName().equals(name))
                 return v;

diff --git a/core/src/main/java/hudson/views/GlobalDefaultViewConfiguration.java b/core/src/main/java/hudson/views/GlobalDefaultViewConfiguration.java
@@ -24,6 +24,7 @@
 package hudson.views;
 
 import hudson.Extension;
+import hudson.model.View;
 import jenkins.model.GlobalConfiguration;
 import jenkins.model.Jenkins;
 import net.sf.json.JSONObject;
@@ -41,7 +42,18 @@ public class GlobalDefaultViewConfiguration extends GlobalConfiguration {
     public boolean configure(StaplerRequest req, JSONObject json) throws FormException {
         // for compatibility reasons, the actual value is stored in Jenkins
         Jenkins j = Jenkins.getInstance();
-        j.setPrimaryView(json.has("primaryView") ? j.getView(json.getString("primaryView")) : j.getViews().iterator().next());
+        if (json.has("primaryView")) {
+            final String viewName = json.getString("primaryView");
+            final View newPrimaryView = j.getView(viewName);
+            if (newPrimaryView == null) {
+                throw new FormException(Messages.GlobalDefaultViewConfiguration_ViewDoesNotExist(viewName), "primaryView");
+            }
+            j.setPrimaryView(newPrimaryView);
+        } else {
+            // Fallback if the view is not specified
+            j.setPrimaryView(j.getViews().iterator().next());
+        }
+
         return true;
     }
 }

diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
@@ -1825,7 +1825,14 @@ public Collection<String> getTopLevelItemNames() {
         return names;
     }
 
-    public View getView(String name) {
+    /**
+     * Gets a view by the specified name.
+     * The method iterates through {@link ViewGroup}s if required.
+     * @param name Name of the view
+     * @return View instance or {@code null} if it is missing
+     */
+    @CheckForNull
+    public View getView(@CheckForNull String name) {
         return viewGroupMixIn.getView(name);
     }
 
@@ -1884,7 +1891,7 @@ public View getPrimaryView() {
         return viewGroupMixIn.getPrimaryView();
      }
 
-    public void setPrimaryView(View v) {
+    public void setPrimaryView(@Nonnull View v) {
         this.primaryView = v.getViewName();
     }
 

diff --git a/core/src/main/resources/hudson/views/Messages.properties b/core/src/main/resources/hudson/views/Messages.properties
@@ -30,3 +30,5 @@ StatusColumn.DisplayName=Status
 WeatherColumn.DisplayName=Weather
 DefaultViewsTabsBar.DisplayName=Default Views TabBar
 DefaultMyViewsTabsBar.DisplayName=Default My Views TabBar
+
+GlobalDefaultViewConfiguration.ViewDoesNotExist=The specified view does not exist: {0}
diff --git a/test/src/test/java/hudson/views/GlobalDefaultViewConfigurationTest.java b/test/src/test/java/hudson/views/GlobalDefaultViewConfigurationTest.java
@@ -0,0 +1,66 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2017 Oleg Nenashev.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+package hudson.views;
+
+import hudson.model.Descriptor;
+import net.sf.json.JSONObject;
+import static org.hamcrest.Matchers.*;
+import org.junit.Assert;
+import static org.junit.Assert.assertThat;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.kohsuke.stapler.MockStaplerRequestBuilder;
+import org.kohsuke.stapler.StaplerRequest;
+
+/**
+ * Tests of {@link GlobalDefaultViewConfiguration}.
+ * @author Oleg Nenashev
+ */
+public class GlobalDefaultViewConfigurationTest {
+
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+
+    @Test
+    @Issue("JENKINS-42717")
+    public void shouldNotFailIfTheDefaultViewIsMissing() {
+        String viewName = "NonExistentView";
+        GlobalDefaultViewConfiguration c = new GlobalDefaultViewConfiguration();
+
+        StaplerRequest create = new MockStaplerRequestBuilder(j, "/configure").build();
+        JSONObject params = new JSONObject();
+        params.accumulate("primaryView", viewName);
+        try {
+            c.configure(create, params);
+        } catch(Descriptor.FormException ex) {
+            assertThat("Wrong exception message for the form failure", 
+                    ex.getMessage(), containsString(Messages.GlobalDefaultViewConfiguration_ViewDoesNotExist(viewName)));
+            return;
+        }
+        Assert.fail("Expected FormException");
+    }
+
+}
diff --git a/test/src/test/java/org/kohsuke/stapler/MockStaplerRequestBuilder.java b/test/src/test/java/org/kohsuke/stapler/MockStaplerRequestBuilder.java
@@ -0,0 +1,81 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2017 Oleg Nenashev.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+package org.kohsuke.stapler;
+
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import javax.annotation.Nonnull;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import jenkins.model.Jenkins;
+import org.eclipse.jetty.server.Request;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.mockito.Mockito;
+import org.springframework.web.multipart.support.DefaultMultipartHttpServletRequest;
+
+/**
+ * Mocked version of {@link StaplerRequest}.
+ * @author Oleg Nenashev
+ */
+public class MockStaplerRequestBuilder{
+
+    private final JenkinsRule r;
+
+    private final List<AncestorImpl> ancestors = new ArrayList<>();
+    private final TokenList tokens;
+    final Map<String,Object> getters = new HashMap<>();
+    private Stapler stapler;
+
+    public MockStaplerRequestBuilder(@Nonnull JenkinsRule r, String url) {
+        this.r = r;
+        this.tokens = new TokenList(url);
+    }
+
+    public MockStaplerRequestBuilder withStapler(Stapler stapler) {
+        this.stapler = stapler;
+        return this;
+    }
+
+    public MockStaplerRequestBuilder withGetter(String objectName, Object object) {
+        this.getters.put(objectName, object);
+        return this;
+    }
+
+    public MockStaplerRequestBuilder withAncestor(AncestorImpl ancestor) {
+        this.ancestors.add(ancestor);
+        return this;
+    }
+
+    public StaplerRequest build() throws AssertionError {        
+        HttpServletRequest rawRequest = Mockito.mock(HttpServletRequest.class);
+        return new RequestImpl(stapler != null ? stapler : new Stapler(), rawRequest, ancestors, tokens);
+    }
+
+}