Added numeric getById to RunMap and deprecate String API#26096
Added numeric getById to RunMap and deprecate String API#26096somiljain2006 wants to merge 17 commits intojenkinsci:masterfrom
Conversation
Co-authored-by: strangelookingnerd <[email protected]>
Co-authored-by: strangelookingnerd <[email protected]>
Co-authored-by: strangelookingnerd <[email protected]>
Co-authored-by: strangelookingnerd <[email protected]>
There was a problem hiding this comment.
Pull request overview
This pull request introduces a canonical numeric getById(int) API for RunMap and deprecates the legacy String-based getById(String) overload. The String-based method is retained for backward compatibility and gracefully handles non-numeric IDs by returning null instead of throwing an exception.
Changes:
- Added new
getById(int)method that delegates togetByNumber(int) - Deprecated existing
getById(String)with enhanced error handling for non-numeric IDs - Added comprehensive test coverage for both legacy and new APIs
- Minor improvements to test robustness and code quality (null safety checks, spelling corrections)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/src/main/java/hudson/model/RunMap.java | Introduces the new numeric getById(int) API and modifies the String-based overload to handle non-numeric IDs gracefully; includes minor documentation improvements (spelling corrections) |
| test/src/test/java/hudson/model/RunMapTest.java | Adds comprehensive test coverage for both legacy String-based and new numeric getById methods; includes incidental null safety improvements in existing tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * | ||
| * @since TODO |
Copilot
AI
Jan 12, 2026
•
There was a problem hiding this comment.
The @since tag is set to "TODO". This should be updated to the actual version number before merging. Please replace "TODO" with the appropriate Jenkins version number for this release.
| * | |
| * @since TODO |
There was a problem hiding this comment.
This comment by GitHub Copilot is incorrect. In the Jenkins project, we use @since TODO as a marker so that the release number can be updated after the first release that includes the new code.
It also would be much better if the markdown generated by GitHub Copilot used @since to show it is code, instead of referring to a GitHub user of that name. I corrected that mistake in the Copilot comment.
Co-authored-by: Copilot <[email protected]>
MarkEWaite
left a comment
There was a problem hiding this comment.
Comment needs to revert back to the original value, at least as far as I can tell. It was correct and GitHub Copilot requested an incorrect change
Co-authored-by: Mark Waite <[email protected]>
MarkEWaite
left a comment
There was a problem hiding this comment.
Please complete the "Testing done" section of the pull request template and describe how you tested this code interactively.
|
@MarkEWaite I’ve updated the Testing section of the PR. |
| */ | ||
| @CheckForNull | ||
| @Override public R getById(String id) { | ||
| public R getById(int id) { |
There was a problem hiding this comment.
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?
jglick
left a comment
There was a problem hiding this comment.
Does not actually seem to do the work of replacing the calls to the now-deprecated overload.
| /** | ||
| * Suitable for {@link Job#getBuild}. | ||
| */ | ||
| public final RunT getBuild(String id) { |
There was a problem hiding this comment.
So why is this still using a String? The point is to get rid of all the gratuitous decimal parsing/formatting in the critical path, deprecating this stuff and leaving it as dead code.
There was a problem hiding this comment.
I agree. I have reverted the changes in LazyBuildMixIn and removed the redundant parsing logic, leaving it as a simple pass-through to the deprecated RunMap#getById(String).
I also searched the codebase for internal calls to getById(String), but I didn't find any other call sites in Core that need to be migrated. If there are specific call sites I missed, please let me know, and I will update them.
Fixes #16693
This change introduces a canonical getById(int) API and deprecates the legacy getById(String) overload. The String-based method is retained for backward compatibility and preserves behavior by returning null for non-numeric IDs instead of throwing an exception.
Testing done
RunMap#getById(int)API andgetById(String)behavior.nullreturn for non-numeric inputsgetById(String)no longer throwsNumberFormatExceptionfor invalid input.RunMapTestcases continue to pass without behavioral changes.A Freestyle job with a completed build was created, and behavior was verified via the Script Console:
getById(int)successfully returns the buildgetById(String)continues to work for numeric IDsnullwithout throwing exceptionsProposed changelog entries
Proposed changelog category
/label developer
Proposed upgrade guidelines
N/A
Desired reviewers
@jenkinsci/core-pr-reviewers, @daniel-beck, @MarkEWaite, @strangelookingnerd
Before the changes are marked as
ready-for-merge:Maintainer checklist
upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidateto be considered.