[JENKINS-44052] Document & fix intended behaviour (#3043)

* [JENKINS-44052] Document & fix intended behaviour The unit was actually ignored... * Add tests for getTimeInSeconds
jenkinsci · Oct 1, 2017 · ff36adf · ff36adf
1 parent fbaa059
commit ff36adf
Show file tree

Hide file tree

Showing 4 changed files with 69 additions and 8 deletions.
diff --git a/core/src/main/java/hudson/model/ParametersDefinitionProperty.java b/core/src/main/java/hudson/model/ParametersDefinitionProperty.java
@@ -171,7 +171,7 @@ public void _doBuild(StaplerRequest req, StaplerResponse rsp, @QueryParameter Ti
         }
 
     	WaitingItem item = Jenkins.getInstance().getQueue().schedule(
-                getJob(), delay.getTime(), new ParametersAction(values), new CauseAction(new Cause.UserIdCause()));
+                getJob(), delay.getTimeInSeconds(), new ParametersAction(values), new CauseAction(new Cause.UserIdCause()));
         if (item!=null) {
             String url = formData.optString("redirectTo");
             if (url==null || !Util.isSafeToRedirectTo(url))   // avoid open redirect
@@ -199,7 +199,7 @@ public void buildWithParameters(StaplerRequest req, StaplerResponse rsp, @CheckF
         if (delay==null)    delay=new TimeDuration(getJob().getQuietPeriod());
 
         Queue.Item item = Jenkins.getInstance().getQueue().schedule2(
-                getJob(), delay.getTime(), new ParametersAction(values), ParameterizedJobMixIn.getBuildCause(getJob(), req)).getItem();
+                getJob(), delay.getTimeInSeconds(), new ParametersAction(values), ParameterizedJobMixIn.getBuildCause(getJob(), req)).getItem();
 
         if (item != null) {
             rsp.sendRedirect(SC_CREATED, req.getContextPath() + '/' + item.getUrl());

diff --git a/core/src/main/java/jenkins/model/ParameterizedJobMixIn.java b/core/src/main/java/jenkins/model/ParameterizedJobMixIn.java
@@ -213,7 +213,7 @@ public final void doBuild(StaplerRequest req, StaplerResponse rsp, @QueryParamet
         }
 
 
-        Queue.Item item = Jenkins.getInstance().getQueue().schedule2(asJob(), delay.getTime(), getBuildCause(asJob(), req)).getItem();
+        Queue.Item item = Jenkins.getInstance().getQueue().schedule2(asJob(), delay.getTimeInSeconds(), getBuildCause(asJob(), req)).getItem();
         if (item != null) {
             rsp.sendRedirect(SC_CREATED, req.getContextPath() + '/' + item.getUrl());
         } else {

diff --git a/core/src/main/java/jenkins/util/TimeDuration.java b/core/src/main/java/jenkins/util/TimeDuration.java
@@ -21,27 +21,59 @@ public TimeDuration(long millis) {
         this.millis = millis;
     }
 
+    /**
+     * Returns the duration of this instance in <em>milliseconds</em>.
+     * @deprecated use {@link #getTimeInMillis()} instead.
+     *
+     * This method has always returned a time in milliseconds, when various callers incorrectly assumed seconds.
+     * And this spread through the codebase. So this has been deprecated for clarity in favour of more explicitly named
+     * methods.
+     */
+    @Deprecated
     public int getTime() {
         return (int)millis;
     }
 
+    /**
+     * Returns the duration of this instance in <em>milliseconds</em>.
+     */
     public long getTimeInMillis() {
         return millis;
     }
 
+    /**
+     * Returns the duration of this instance in <em>milliseconds</em>.
+     */
+    public int getTimeInSeconds() {
+        return (int) (millis / 1000L);
+    }
+
+
     public long as(TimeUnit t) {
         return t.convert(millis,TimeUnit.MILLISECONDS);
     }
 
-    public static @CheckForNull TimeDuration fromString(@CheckForNull String delay) {
-        if (delay==null)
+    /**
+     * Creates a {@link TimeDuration} from the delay passed in parameter
+     * @param delay the delay either in milliseconds without unit, or in seconds if suffixed by sec or secs.
+     * @return the TimeDuration created from the delay expressed as a String.
+     */
+    @CheckForNull
+    public static TimeDuration fromString(@CheckForNull String delay) {
+        if (delay == null) {
             return null;
+        }
 
+        long unitMultiplier = 1L;
+        delay = delay.trim();
         try {
             // TODO: more unit handling
-            if(delay.endsWith("sec"))   delay=delay.substring(0,delay.length()-3);
-            if(delay.endsWith("secs"))  delay=delay.substring(0,delay.length()-4);
-            return new TimeDuration(Long.parseLong(delay));
+            if (delay.endsWith("sec") || delay.endsWith("secs")) {
+                delay = delay.substring(0, delay.lastIndexOf("sec"));
+                delay = delay.trim();
+                unitMultiplier = 1000L;
+            }
+            return new TimeDuration(Long.parseLong(delay.trim()) * unitMultiplier);
         } catch (NumberFormatException e) {
             throw new IllegalArgumentException("Invalid time duration value: "+delay);
         }

diff --git a/core/src/test/java/jenkins/util/TimeDurationTest.java b/core/src/test/java/jenkins/util/TimeDurationTest.java
@@ -0,0 +1,29 @@
+package jenkins.util;
+
+import org.junit.Test;
+
+import java.util.concurrent.TimeUnit;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.*;
+
+import static jenkins.util.TimeDuration.*;
+
+
+public class TimeDurationTest {
+
+    @Test
+    public void fromString() throws Exception {
+        assertEquals(1, TimeDuration.fromString("1").getTimeInMillis());
+
+        assertEquals(1000, TimeDuration.fromString("1sec").getTimeInMillis());
+        assertEquals(1000, TimeDuration.fromString("1secs").getTimeInMillis());
+        assertEquals(1000, TimeDuration.fromString("1 secs ").getTimeInMillis());
+        assertEquals(1000, TimeDuration.fromString(" 1 secs ").getTimeInMillis());
+        assertEquals(1, TimeDuration.fromString(" 1 secs ").getTimeInSeconds());
+
+        assertEquals(21000, TimeDuration.fromString(" 21  secs ").getTimeInMillis());
+        assertEquals(21, TimeDuration.fromString(" 21  secs ").getTimeInSeconds());
+    }
+
+}