Simplify handling of a null StaplerRequest

jenkinsci · Dec 4, 2013 · 9c2e68c · 9c2e68c
1 parent eb8f2c1
commit 9c2e68c
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 52 deletions.
diff --git a/src/main/java/hudson/plugins/timestamper/TimestampNote.java b/src/main/java/hudson/plugins/timestamper/TimestampNote.java
@@ -30,6 +30,9 @@
 import hudson.plugins.timestamper.action.TimestampsAction;
 import hudson.plugins.timestamper.format.TimestampFormatter;
 
+import org.kohsuke.stapler.Stapler;
+import org.kohsuke.stapler.StaplerRequest;
+
 /**
  * Time-stamp note that was inserted into the console note by the Timestamper
  * plugin prior to version 1.4.
@@ -96,9 +99,11 @@ public Timestamp getTimestamp(Run<?, ?> build) {
   @Override
   public ConsoleAnnotator<Object> annotate(Object context, MarkupText text,
       int charPos) {
-    if (context instanceof Run<?, ?>) {
+    StaplerRequest request = Stapler.getCurrentRequest();
+    // JENKINS-16778: The request can be null when the slave goes off-line.
+    if (context instanceof Run<?, ?> && request != null) {
       Run<?, ?> build = (Run<?, ?>) context;
-      TimestampFormatter formatter = TimestamperConfig.formatter();
+      TimestampFormatter formatter = TimestamperConfig.formatter(request);
       Timestamp timestamp = getTimestamp(build);
       formatter.markup(text, timestamp);
     }

diff --git a/src/main/java/hudson/plugins/timestamper/TimestamperConfig.java b/src/main/java/hudson/plugins/timestamper/TimestamperConfig.java
@@ -32,17 +32,16 @@
 import java.text.SimpleDateFormat;
 
 import javax.annotation.CheckForNull;
-import javax.servlet.http.HttpServletRequest;
+import javax.annotation.Nonnull;
 
 import jenkins.model.GlobalConfiguration;
 import net.sf.json.JSONObject;
 
 import org.apache.commons.lang.time.DurationFormatUtils;
-import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.StaplerRequest;
 
+import com.google.common.base.Function;
 import com.google.common.base.Objects;
-import com.google.common.base.Supplier;
 
 /**
  * Global configuration for the Timestamper plug-in, as shown on the Jenkins
@@ -53,12 +52,12 @@
 @Extension
 public final class TimestamperConfig extends GlobalConfiguration {
 
-  private static Supplier<TimestampFormatter> formatterSupplier = new Supplier<TimestampFormatter>() {
+  private static Function<StaplerRequest, TimestampFormatter> timestampFormatterProvider = new Function<StaplerRequest, TimestampFormatter>() {
+
     @Override
-    public TimestampFormatter get() {
+    public TimestampFormatter apply(@Nonnull StaplerRequest request) {
       TimestamperConfig config = GlobalConfiguration.all().get(
           TimestamperConfig.class);
-      HttpServletRequest request = Stapler.getCurrentRequest();
       return new TimestampFormatterImpl(config.getSystemTimeFormat(),
           config.getElapsedTimeFormat(), request);
     }
@@ -147,9 +146,11 @@ public boolean configure(StaplerRequest req, JSONObject json)
   /**
    * Get a time-stamp formatter based on the current settings.
    * 
+   * @param request
+   *          the current stapler request
    * @return a time-stamp formatter
    */
-  public static TimestampFormatter formatter() {
-    return formatterSupplier.get();
+  public static TimestampFormatter formatter(StaplerRequest request) {
+    return timestampFormatterProvider.apply(request);
   }
 }
diff --git a/src/main/java/hudson/plugins/timestamper/annotator/TimestampAnnotatorFactory.java b/src/main/java/hudson/plugins/timestamper/annotator/TimestampAnnotatorFactory.java
@@ -29,8 +29,6 @@
 import hudson.plugins.timestamper.TimestamperConfig;
 import hudson.plugins.timestamper.format.TimestampFormatter;
 
-import javax.annotation.CheckForNull;
-
 import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.StaplerRequest;
 
@@ -49,8 +47,13 @@ public final class TimestampAnnotatorFactory extends
    */
   @Override
   public ConsoleAnnotator<Object> newInstance(Object context) {
-    TimestampFormatter formatter = TimestamperConfig.formatter();
-    long offset = getOffset(Stapler.getCurrentRequest());
+    StaplerRequest request = Stapler.getCurrentRequest();
+    // JENKINS-16778: The request can be null when the slave goes off-line.
+    if (request == null) {
+      return null; // do not annotate
+    }
+    TimestampFormatter formatter = TimestamperConfig.formatter(request);
+    long offset = getOffset(request);
     ConsoleLogParser logParser = new ConsoleLogParserImpl(offset);
     return new TimestampAnnotator(formatter, logParser);
   }
@@ -63,11 +66,7 @@ public ConsoleAnnotator<Object> newInstance(Object context) {
    * @param request
    * @return the offset in bytes
    */
-  private static long getOffset(@CheckForNull StaplerRequest request) {
-    if (request == null) {
-      // JENKINS-16778: The request can be null when the slave goes off-line.
-      return 0;
-    }
+  private static long getOffset(StaplerRequest request) {
     String path = request.getPathInfo();
     if (path == null) {
       // JENKINS-16438

diff --git a/src/main/java/hudson/plugins/timestamper/format/TimestampFormatterImpl.java b/src/main/java/hudson/plugins/timestamper/format/TimestampFormatterImpl.java
@@ -30,7 +30,6 @@
 import java.io.Serializable;
 import java.util.Date;
 
-import javax.annotation.CheckForNull;
 import javax.annotation.concurrent.Immutable;
 import javax.servlet.http.Cookie;
 import javax.servlet.http.HttpServletRequest;
@@ -55,7 +54,6 @@ public final class TimestampFormatterImpl implements TimestampFormatter {
    * Function that converts a time-stamp to a formatted string representation of
    * that time-stamp.
    */
-  @CheckForNull
   private final Function<Timestamp, String> formatTimestamp;
 
   /**
@@ -71,12 +69,6 @@ public final class TimestampFormatterImpl implements TimestampFormatter {
   public TimestampFormatterImpl(String systemTimeFormat,
       String elapsedTimeFormat, HttpServletRequest request) {
 
-    if (request == null) {
-      // JENKINS-16778: The request can be null when the slave goes off-line.
-      formatTimestamp = null;
-      return;
-    }
-
     String cookieValue = null;
     Cookie[] cookies = request.getCookies();
     if (cookies != null) {
@@ -103,9 +95,6 @@ public TimestampFormatterImpl(String systemTimeFormat,
    */
   @Override
   public void markup(MarkupText text, Timestamp timestamp) {
-    if (formatTimestamp == null) {
-      return;
-    }
     String timestampString = formatTimestamp.apply(timestamp);
     // Wrap the time-stamp in a span element, which is used to detect the
     // time-stamp when inspecting the page with Javascript.

diff --git a/src/test/java/hudson/plugins/timestamper/TimestampNoteTest.java b/src/test/java/hudson/plugins/timestamper/TimestampNoteTest.java
@@ -38,12 +38,15 @@
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.kohsuke.stapler.RequestImpl;
+import org.kohsuke.stapler.Stapler;
+import org.kohsuke.stapler.StaplerRequest;
 import org.powermock.api.mockito.PowerMockito;
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
 import org.powermock.reflect.Whitebox;
 
-import com.google.common.base.Supplier;
+import com.google.common.base.Function;
 
 /**
  * Unit test for the {@link TimestampNote} class.
@@ -64,24 +67,30 @@ public class TimestampNoteTest {
   private MarkupText text;
 
   /**
+   * @throws Exception
    */
   @Before
-  public void setUp() {
+  public void setUp() throws Exception {
     build = PowerMockito.mock(Run.class);
     when(build.getTimeInMillis()).thenReturn(1l);
 
     note = new TimestampNote(3);
 
     formatter = mock(TimestampFormatter.class);
-    Whitebox.setInternalState(TimestamperConfig.class, Supplier.class,
-        new Supplier<TimestampFormatter>() {
+    Whitebox.setInternalState(TimestamperConfig.class, Function.class,
+        new Function<StaplerRequest, TimestampFormatter>() {
           @Override
-          public TimestampFormatter get() {
+          public TimestampFormatter apply(StaplerRequest input) {
             return formatter;
           }
         });
 
     text = new MarkupText("");
+
+    @SuppressWarnings("unchecked")
+    ThreadLocal<RequestImpl> currentRequest = (ThreadLocal<RequestImpl>) Whitebox
+        .getField(Stapler.class, "CURRENT_REQUEST").get(null);
+    currentRequest.set(mock(RequestImpl.class));
   }
 
   /**

diff --git a/src/test/java/hudson/plugins/timestamper/format/TimestampFormatterImplTest.java b/src/test/java/hudson/plugins/timestamper/format/TimestampFormatterImplTest.java
@@ -46,6 +46,7 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
 import org.junit.runners.Parameterized.Parameters;
 
 /**
@@ -66,8 +67,7 @@ public static Collection<Object[]> data() {
         { request("elapsed"), span("00.123 "), span("00.123 ") },
         { request("none"), span(""), span("") },
         { request(), span("00:00:42 "), span("08:00:42 ") },
-        { request((String[]) null), span("00:00:42 "), span("08:00:42 ") },
-        { null, "", "" } });
+        { request((String[]) null), span("00:00:42 "), span("08:00:42 ") } });
   }
 
   private static HttpServletRequest request(String... cookieValues) {
@@ -87,28 +87,25 @@ private static String span(String timestampString) {
     return "<span class=\"timestamp\">" + timestampString + "</span>";
   }
 
-  private HttpServletRequest request;
+  /**
+   */
+  @Parameter(0)
+  public HttpServletRequest request;
 
-  private String prefix;
+  /**
+   */
+  @Parameter(1)
+  public String prefix;
 
-  private String prefixInDifferentTimezone;
+  /**
+   */
+  @Parameter(2)
+  public String prefixInDifferentTimezone;
 
   private TimeZone systemDefaultTimeZone;
 
   private boolean serialize;
 
-  /**
-   * @param request
-   * @param prefix
-   * @param prefixInDifferentTimezone
-   */
-  public TimestampFormatterImplTest(HttpServletRequest request, String prefix,
-      String prefixInDifferentTimezone) {
-    this.request = request;
-    this.prefix = prefix;
-    this.prefixInDifferentTimezone = prefixInDifferentTimezone;
-  }
-
   /**
    */
   @Before