Limit job RSS changelog feed size by default - #27088
Conversation
|
Yay, your first pull request towards Jenkins core was created successfully! Thank you so much! |
There was a problem hiding this comment.
Pull request overview
This PR improves the scalability of Job.doRssChangelog by limiting the amount of changelog history included in generated RSS/Atom feeds by default, reducing unnecessary build record loading and feed payload size.
Changes:
- Add a default cap of 20 changelog entries to
Job.doRssChangelog, with overrides via?max=and a system property. - Stop iterating older builds once the requested number of changelog entries has been collected.
- Add functional test coverage validating the new default and override behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/src/main/java/hudson/model/Job.java | Introduces a default entry limit and early-exit logic when generating the RSS/Atom changelog feed. |
| test/src/test/java/hudson/model/RSSTest.java | Adds tests and helpers to validate default limiting and override mechanisms for rssChangelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Default 20 entries (JENKINS-18992). Overridable via system property or ?max= | ||
| int maxEntries = SystemProperties.getInteger(Job.class.getName() + ".rssChangelogMaxEntries", 20); | ||
| String maxParam = req.getParameter("max"); | ||
| if (maxParam != null) { | ||
| try { | ||
| maxEntries = Math.max(1, Integer.parseInt(maxParam)); |
bacea92 to
7f839b7
Compare
| } | ||
|
|
||
| // Default 20 entries (JENKINS-18992). Overridable via system property or ?max= | ||
| int maxEntries = SystemProperties.getInteger(Job.class.getName() + ".rssChangelogMaxEntries", 20); |
| private JenkinsRule j; | ||
|
|
||
| private static final String RSS_CHANGELOG_MAX_ENTRIES = Job.class.getName() + ".rssChangelogMaxEntries"; | ||
|
|
||
| @BeforeEach | ||
| void setUp(JenkinsRule rule) { | ||
| j = rule; | ||
| } | ||
|
|
||
| @AfterEach | ||
| void clearRssChangelogMaxEntries() { | ||
| System.clearProperty(RSS_CHANGELOG_MAX_ENTRIES); | ||
| } |
| // Default 20 entries (JENKINS-18992). Overridable via system property or ?max= | ||
| int maxEntries = Math.max(1, SystemProperties.getInteger(Job.class.getName() + ".rssChangelogMaxEntries", 20)); | ||
| String maxParam = req.getParameter("max"); | ||
| if (maxParam != null) { | ||
| try { | ||
| maxEntries = Math.max(1, Integer.parseInt(maxParam)); | ||
| } catch (NumberFormatException e) { | ||
| // keep default | ||
| } | ||
| } |
| void rssChangelogMaxParameter() throws Exception { | ||
| createJobWithChangelogEntries("rssChangelogMax", 15); | ||
| JenkinsRule.WebClient wc = j.createWebClient(); | ||
| assertEquals(5, countChangelogEntries(getRssChangelogPage(wc, "rssChangelogMax", "max=5", true), true)); | ||
| assertEquals(15, countChangelogEntries(getRssChangelogPage(wc, "rssChangelogMax", "max=50", true), true)); | ||
| } |
| String maxParam = req.getParameter("max"); | ||
| if (maxParam != null) { | ||
| try { | ||
| maxEntries = Math.min(hardLimit, Math.max(1, Integer.parseInt(maxParam))); | ||
| } catch (NumberFormatException e) { | ||
| // keep default | ||
| } | ||
| } |
| // Default 20 entries (JENKINS-18992). Overridable via system property or ?max=, | ||
| // but always hard-capped so callers cannot reintroduce unbounded feeds. |
850d4e0 to
44f8d13
Compare
|
Hi maintainers, This PR has been open for a bit with CI green and the Copilot feedback addressed (default limit, hard cap, property restore in tests, long parsing for oversized Would you be able to take a look when you have a moment? cc @jenkinsci/core-pr-reviewers Thanks! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
core/src/main/java/hudson/model/Job.java:1163
- The PR description documents only hudson.model.Job.rssChangelogMaxEntries and ?max=N, but this change also introduces a hard cap via hudson.model.Job.rssChangelogHardLimit (and enforces it for ?max=). If this is intentional, it should be called out in the PR description / release notes so administrators understand why large values for ?max= are being clamped.
// Default 20 entries (JENKINS-18992).
// hudson.model.Job.rssChangelogMaxEntries — default feed size (min 1)
// hudson.model.Job.rssChangelogHardLimit — absolute ceiling (default 1000)
// ?max=N overrides the default size but cannot exceed the hard limit.
int hardLimit =
| private void createJobWithChangelogEntries(String name, int n) throws Exception { | ||
| FreeStyleProject p = j.createFreeStyleProject(name); | ||
| FakeChangeLogSCM scm = new FakeChangeLogSCM(); | ||
| p.setScm(scm); | ||
| for (int i = 0; i < n; i++) { | ||
| scm.addChange().withAuthor("u" + i); | ||
| j.buildAndAssertSuccess(p); | ||
| } | ||
| } |
| void rssChangelogMaxParameter() throws Exception { | ||
| // More than the default (20) so we prove ?max= can raise the limit, not only lower it. | ||
| createJobWithChangelogEntries("rssChangelogMax", 25); | ||
| JenkinsRule.WebClient wc = j.createWebClient(); | ||
| assertEquals(5, countChangelogEntries(getRssChangelogPage(wc, "rssChangelogMax", "max=5", true), true)); | ||
| assertEquals(25, countChangelogEntries(getRssChangelogPage(wc, "rssChangelogMax", "max=50", true), true)); | ||
| } |
rssChangelog previously walked every build and could produce very large feeds. Cap at 20 entries by default, with overrides via system property or the max query parameter. Fixes jenkinsci#14784
Ensure the system property cannot produce an empty feed when set to 0 or a negative value. Restore the original property value after each RSSTest method instead of always clearing it.
Cap ?max= and system-property overrides with a hard limit so feeds cannot become unbounded. Strengthen tests for raising the limit above the default and for the hard cap / minimum clamps.
Name the system properties in the limit comment for operators, and parse ?max= as a long so oversized numeric values clamp to the hard limit instead of falling back to the default.
Batch FakeChangeLogSCM entries into a single build so the suite does not pay for one build per entry. Also assert Atom and RSS 2.0 for ?max=.
cdf0a33 to
30315ee
Compare
Generated with core pull request: * jenkinsci/jenkins#27206 Includes pull requests: * jenkinsci/jenkins#27205 * jenkinsci/jenkins#27201 * jenkinsci/jenkins#27193 * jenkinsci/jenkins#27163 * jenkinsci/jenkins#27088 * jenkinsci/jenkins#27083 * jenkinsci/jenkins#27067 * jenkinsci/jenkins#27065 * jenkinsci/jenkins#27063 * jenkinsci/jenkins#27032 * jenkinsci/jenkins#26923 * jenkinsci/jenkins#26922 * jenkinsci/jenkins#26913 * jenkinsci/jenkins#26880 * jenkinsci/jenkins#26690 * jenkinsci/jenkins#26668 * jenkinsci/jenkins#26600 * jenkinsci/jenkins#26587 * jenkinsci/jenkins#11216 * jenkinsci/jenkins#10432 * jenkinsci/jenkins#8559
Fixes #14784
Job.doRssChangelogpreviously walked every build for a job and could produce very large feeds (hundreds of entries / hundreds of KB). That loads unnecessary build records, hurts performance, and undermines lazy loading.This change caps the feed at 20 entries by default (as suggested in the issue). The limit can be changed with:
hudson.model.Job.rssChangelogMaxEntries?max=NOnce the limit is reached, older builds are not loaded.
Testing done
Ran the updated functional tests locally:
Result:
Tests run: 26, Failures: 0, Errors: 0, Skipped: 0New coverage in
RSSTest:?max=overrides the defaultAlso compiled core/test modules successfully with:
Screenshots (UI changes only)
N/A (no UI change; RSS/Atom feed response size only)
Before
After
Proposed changelog entries
Proposed changelog category
/label rfe
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@jenkinsci/core-pr-reviewers
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.