Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions core/src/main/java/hudson/model/RunMap.java
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
*
* <p>
* This class is multi-thread safe by using copy-on-write technique,
* and it also updates the bi-directional links within {@link Run}
* and it also updates the bidirectional links within {@link Run}
* accordingly.
*
* @author Kohsuke Kawaguchi
Expand Down Expand Up @@ -223,11 +223,30 @@ public R put(R r) {
return super._put(r);
}

/**
* Lookup a build by its numeric ID.
*/
@CheckForNull
@Override public R getById(String id) {
public R getById(int id) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I don't think there is anything wrong with this change I wonder why it's not implemented in the AbstractLazyLazyRunMap or we just point the deprecation notice to #getByNumber?

@jglick Since you mentioned this in #10456 (comment) - any suggestions from you here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dates to #1379.

return getByNumber(id);
}


/**
* Lookup a build by a String ID.
*
* @deprecated Use {@link #getById(int)} instead.
* Jenkins build IDs were sometimes date-time strings.
* This method preserves backward compatibility by attempting to parse
* the ID as an integer and returning null if parsing fails.
*/
@Deprecated
@CheckForNull
@Override
public R getById(String id) {
try {
return getByNumber(Integer.parseInt(id));
} catch (NumberFormatException e) { // see https://issues.jenkins.io/browse/JENKINS-75476
return getById(Integer.parseInt(id));
} catch (NumberFormatException e) {
return null;
}
}
Expand Down Expand Up @@ -306,8 +325,7 @@ protected Class<R> getBuildClass() {
}

/**
* Backward compatibility method that notifies {@link RunMap} of who the owner is.
*
* Backward compatibility method that notifies {@link RunMap} of whom the owner is.
* Traditionally, this method blocked and loaded all the build records on the disk,
* but now all the actual loading happens lazily.
*
Expand Down
33 changes: 27 additions & 6 deletions test/src/test/java/hudson/model/RunMapTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.util.Collections;
import java.util.Comparator;
import java.util.List;
import java.util.Objects;
import java.util.SortedMap;
import java.util.Spliterator;
import java.util.TreeMap;
Expand Down Expand Up @@ -86,7 +87,9 @@ void reloadWhileBuildIsInQueue() throws Exception {
// Note that the bug does not reproduce simply from p.doReload(), since in that case Job identity remains intact:
r.jenkins.reload();
p = r.jenkins.getItemByFullName("p", FreeStyleProject.class);
assert p != null;
FreeStyleBuild b1 = p.getLastBuild();
assert b1 != null;
assertEquals(1, b1.getNumber());
/* Currently fails since Run.project is final. But anyway that is not the problem:
assertEquals(p, b1.getParent());
Expand All @@ -95,17 +98,17 @@ void reloadWhileBuildIsInQueue() throws Exception {
assertEquals(1, items.length);
assertEquals(p, items[0].task); // the real issue: assignBuildNumber was being called on the wrong Job
QueueTaskFuture<Queue.Executable> b2f = items[0].getFuture();
b1.getExecutor().interrupt();
Objects.requireNonNull(b1.getExecutor()).interrupt();
r.assertBuildStatus(Result.ABORTED, r.waitForCompletion(b1));
FreeStyleBuild b2 = (FreeStyleBuild) b2f.waitForStart();
assertEquals(2, b2.getNumber());
assertEquals(p, b2.getParent());
b2.getExecutor().interrupt();
Objects.requireNonNull(b2.getExecutor()).interrupt();
r.assertBuildStatus(Result.ABORTED, r.waitForCompletion(b2));
FreeStyleBuild b3 = p.scheduleBuild2(0).waitForStart();
assertEquals(3, b3.getNumber());
assertEquals(p, b3.getParent());
b3.getExecutor().interrupt();
Objects.requireNonNull(b3.getExecutor()).interrupt();
r.assertBuildStatus(Result.ABORTED, r.waitForCompletion(b3));
}

Expand Down Expand Up @@ -166,7 +169,7 @@ void stream() throws Exception {
}
RunMap<FreeStyleBuild> runMap = p._getRuns();

assertEquals(builds.size(), runMap.entrySet().size());
assertEquals(builds.size(), runMap.size());
assertTrue(
runMap.entrySet().stream().spliterator().hasCharacteristics(Spliterator.DISTINCT));
assertTrue(
Expand Down Expand Up @@ -209,7 +212,7 @@ void runLoadCounterFirst() throws Exception {
}
assertEquals(
10,
RunLoadCounter.assertMaxLoads(p, 2, () -> p.getBuilds().stream().findFirst().orElse(null).number).intValue());
RunLoadCounter.assertMaxLoads(p, 2, () -> Objects.requireNonNull(p.getBuilds().stream().findFirst().orElse(null)).number).intValue());
}

@Test
Expand All @@ -220,6 +223,24 @@ void runLoadCounterLimit() throws Exception {
}
assertEquals(
6,
RunLoadCounter.assertMaxLoads(p, 6, () -> Streams.findLast(p.getBuilds().stream().limit(5)).orElse(null).number).intValue());
RunLoadCounter.assertMaxLoads(p, 6, () -> Objects.requireNonNull(
Streams.findLast(p.getBuilds().stream().limit(5)).orElse(null)).number).intValue());
}

@SuppressWarnings("deprecation")
@Issue("JENKINS-75465")
@Test
void getByIdLegacyAndNumeric() throws Exception {
FreeStyleProject p = r.createFreeStyleProject();
FreeStyleBuild b1 = r.buildAndAssertSuccess(p);
RunMap<FreeStyleBuild> runs = p._getRuns();

assertSame(b1, runs.getById(b1.getNumber()), "getById(int) should find the build");
assertNull(runs.getById(999), "getById(int) should return null for missing ID");
assertSame(b1, runs.getById(String.valueOf(b1.getNumber())), "getById(String) should find the build via parsing");
assertNull(runs.getById("not-a-number"), "getById(String) should return null for non-numeric input");
assertNull(runs.getById(""), "getById(String) should return null for empty string");
assertNull(runs.getById("2007-11-12_12-23-45"), "getById(String) should return null for historical date-time IDs");
assertNull(runs.getById(" " + b1.getNumber()), "getById(String) must strictly follow Integer.parseInt behavior (no trimming)");
}
}