[JENKINS-60577] - Prevent the RSS feed in Computer page from returning an error 404 #4411

fcojfernandez · 2020-01-02T13:17:20Z

Accessing the RSS feed for latest builds from the Computer page returns a 404 error. Whilst in User and View class the method doRssLatest exists, in Computer this method is missing.
I've also made a small code refactor to avoid further duplication.

Proposed changelog entries

JENKINS-60577, Prevent the RSS feed in Computer page from returning an error 404

Submitter checklist

JIRA issue is well described
Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
* Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@jenkinsci/core-pr-reviewers

halkeye · 2020-01-02T20:17:52Z

core/src/main/java/hudson/model/Job.java

-            RunList runs) throws IOException, ServletException {
-        RSS.forwardToRss(getDisplayName() + suffix, getUrl(), runs.newBuilds(),
-                Run.FEED_ADAPTER, req, rsp);
+        RSS.rss(req, rsp, getDisplayName() + " failed builds", getUrl(), getBuilds().failureOnly().newBuilds());


I feel like it would be a huge change, but would It make more sense to make RSS an interface, and have a function that returns getBuilds().failureOnly().newBuilds() ?

oleg-nenashev

Thanks for the fix! It is definitely an improvement which worth merging. OTOH it would be great to document new API and limitations of the newly introduced endpoint

oleg-nenashev · 2020-01-03T22:41:45Z

core/src/main/java/hudson/model/RSS.java

@@ -90,4 +91,14 @@ public static void doTrackback( Object it, StaplerRequest req, StaplerResponse r

        req.getView(Jenkins.get(),"/hudson/"+flavor+".jelly").forward(req,rsp);
    }
+
+    public static void rss(StaplerRequest req, StaplerResponse rsp, String title, String url, RunList runList) throws IOException, ServletException {


Nice 2 have. Add Javadoc to newly introduced methods

indeed!. I thought I had added before the commit 🤦‍♂️

oleg-nenashev · 2020-01-03T22:43:33Z

core/src/main/java/hudson/model/Computer.java

+
+    public void doRssLatest( StaplerRequest req, StaplerResponse rsp ) throws IOException, ServletException {
+        final List<Run> lastBuilds = new ArrayList<>();
+        for (AbstractProject<?, ?> p : Jenkins.get().allItems(AbstractProject.class)) {


Should the limitation to AbstractBuild be documented somewhere?

I've just wanted to keep some coherence and I just saw

jenkins/core/src/main/java/hudson/model/User.java

Lines 882 to 896 in 4d05982

public void doRssLatest(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException {

final List<Run> lastBuilds = new ArrayList<>();

for (AbstractProject<?, ?> p : Jenkins.get().allItems(AbstractProject.class)) {

for (AbstractBuild<?, ?> b = p.getLastBuild(); b != null; b = b.getPreviousBuild()) {

if (relatedTo(b)) {

lastBuilds.add(b);

break;

}

}

}

// historically these have been reported sorted by project name, we switched to the lazy iteration

// so we only have to sort the sublist of runs rather than the full list of irrelevant projects

lastBuilds.sort((o1, o2) -> Items.BY_FULL_NAME.compare(o1.getParent(), o2.getParent()));

rss(req, rsp, " latest build", RunList.fromRuns(lastBuilds), Run.FEED_ADAPTER_LATEST);

}

TBH, I don't know if any kind of limitation should apply. I will add a javadoc explaining what is displayed and if you think something else / other documentation is needed just let me know and I'm happy to add it.

I think now I've understood you well ...

Maybe 5a2c62e?

I think Oleg meant that this code only takes into account AbstractProjects, it doesn't take into account, for example, FreeStyleProjects.

Would performance be a concern here? Given we are asking for all the items and looping through them?

TBH, I don't think this feature is widely used since it's been broken for so long, so personally I wound't care (personal opinion).
As I said above, I'm mimicking the current behaviour for the already existing User class. If there is another way to gather that information, I'm unknown. And also I'm open to suggestions if this is not the information that should be retrieved.

Sounds reasonable. A bad performant feature (worst case scenario) is better than a broken one.

I think Oleg meant that this code only takes into account AbstractProjects, it doesn't take into account, for example, FreeStyleProjects.

It does not take Pipeline into account, for example. I know that we are still waiting for proper Node information API to be exposed for Pipeline, but 🤷‍♂

@MRamonLeon and @oleg-nenashev you're right. I should have misread the feed in my tests. It's only displaying the FreeStyleProject and not the pipeline:

<?xml version="1.0" encoding="UTF-8"?> <feed xmlns="http://www.w3.org/2005/Atom"> <title>principal last builds only</title> <link rel="alternate" type="text/html" href="http://localhost:7080/computer/(master)/"/> <updated>2020-01-13T09:52:40Z</updated> <author> <name>Jenkins Server</name> </author> <id>urn:uuid:903deee0-7bfa-11db-9fe1-0800200c9a66</id> <entry> <title>free #3 (estable)</title> <link rel="alternate" type="text/html" href="http://localhost:7080/job/free/3/"/> <id>tag:hudson.dev.java.net,2020:free:3</id> <published>2020-01-13T09:52:40Z</published> <updated>2020-01-13T09:52:40Z</updated> </entry> </feed>

I will change the javadoc!

fcojfernandez · 2020-01-09T11:53:30Z

The failing tests is totally unrelated to this PR. Closing and re-opening so the CI is re-launched.

core/src/main/java/hudson/model/Computer.java

varyvol · 2020-01-09T12:09:03Z

core/src/main/java/hudson/model/Computer.java

+
+    public void doRssLatest( StaplerRequest req, StaplerResponse rsp ) throws IOException, ServletException {
+        final List<Run> lastBuilds = new ArrayList<>();
+        for (AbstractProject<?, ?> p : Jenkins.get().allItems(AbstractProject.class)) {


Would performance be a concern here? Given we are asking for all the items and looping through them?

oleg-nenashev

I believe the doRssLatest() javadoc should explicitly document the limitation that only AbstractProject-based jobs will be supported. Sorry for not explicitly referencing other job types in https://github.com/jenkinsci/jenkins/pull/4411/files#r362985417

core/src/main/java/hudson/model/Computer.java

core/src/main/java/hudson/model/RSS.java

oleg-nenashev · 2020-01-10T09:00:37Z

core/src/main/java/hudson/model/Computer.java

+    /**
+     * Retrieve the RSS feed for the last build for each project executed in this computer
+     */
+    public void doRssLatest( StaplerRequest req, StaplerResponse rsp ) throws IOException, ServletException {


Should it be marked as @Restricted(DoNotlUse.class). AFAIK it is only for Stapler. And yes, I know that the historical methods are not annotated. So it is not blocking

@oleg-nenashev I'm happy to add the annotation. Should I annotate the other methods in this class? and in the other classes? I can do it as part of this PR

I think it is fine to annotate this class in this pull request. If you prefer to be on the safe side, just do it for newly introduced endpoints

Co-Authored-By: Oleg Nenashev <[email protected]>

fcojfernandez · 2020-01-13T11:47:39Z

@oleg-nenashev I've updated the javadoc to document the limitation to AbstractProjects. Let me know if you want me to add the annotation and if it's convenient to add it to the rest of methods

oleg-nenashev

There are minor outstanding comments, but the code looks good to me overall. Thanks a lot @fcojfernandez !

fcojfernandez · 2020-01-13T12:38:33Z

Added the @Restricted annotation. I think this PR is ready for merge, but since I'm the author I will let @oleg-nenashev or any other member of @jenkinsci/core-pr-reviewers add the label.

oleg-nenashev

Thanks for addressing the comments!
I plan to merge it tomorrow if no negative feedback

[JENKINS-60577] Rss Latest Builds for Computer

2021ab5

fcojfernandez requested a review from a team January 2, 2020 13:17

halkeye reviewed Jan 2, 2020

View reviewed changes

halkeye approved these changes Jan 2, 2020

View reviewed changes

MarkEWaite added the bug For changelog: Minor bug. Will be listed after features label Jan 2, 2020

res0nance approved these changes Jan 3, 2020

View reviewed changes

oleg-nenashev self-requested a review January 3, 2020 22:40

oleg-nenashev changed the title ~~[JENKINS-60577] Rss Latest Builds for Computer~~ [JENKINS-60577] - Prevent the RSS feed in Computer page from returning an error 404 Jan 3, 2020

oleg-nenashev requested changes Jan 3, 2020

View reviewed changes

oleg-nenashev added the plugin-api-changes Changes the API of Jenkins available for use in plugins. label Jan 3, 2020

fcojfernandez added 2 commits January 7, 2020 09:32

[JENKINS-60577] Missing javadoc

9b7dd89

[JENKINS-60577] Add builds from the computer

5a2c62e

fcojfernandez requested review from oleg-nenashev and MRamonLeon January 9, 2020 11:51

fcojfernandez closed this Jan 9, 2020

fcojfernandez reopened this Jan 9, 2020

varyvol reviewed Jan 9, 2020

View reviewed changes

Update core/src/main/java/hudson/model/Computer.java

8d40f45

varyvol approved these changes Jan 9, 2020

View reviewed changes

oleg-nenashev requested changes Jan 10, 2020

View reviewed changes

fcojfernandez and others added 2 commits January 13, 2020 12:15

Apply suggestions from code review

e0edecd

Co-Authored-By: Oleg Nenashev <[email protected]>

[JENKINS-60577] Document limitation to AbstractProject

3fdfa0a

fcojfernandez requested a review from oleg-nenashev January 13, 2020 11:47

oleg-nenashev approved these changes Jan 13, 2020

View reviewed changes

[JENKINS-60577] Restrict the RSS methods in Computer

e9d95ac

oleg-nenashev approved these changes Jan 14, 2020

View reviewed changes

oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jan 14, 2020

oleg-nenashev merged commit 3ade716 into jenkinsci:master Jan 15, 2020

fcojfernandez deleted the JENKINS-60577 branch January 15, 2020 06:56

Wadeck mentioned this pull request Jan 25, 2020

[JENKINS-60848] Improve RSS title wording + Add more RSS tests #4442

Merged

9 tasks

daniel-beck mentioned this pull request Mar 16, 2020

Update at-since up to and including 2.226 #4587

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-60577] - Prevent the RSS feed in Computer page from returning an error 404 #4411

[JENKINS-60577] - Prevent the RSS feed in Computer page from returning an error 404 #4411

fcojfernandez commented Jan 2, 2020

halkeye Jan 2, 2020

oleg-nenashev left a comment

oleg-nenashev Jan 3, 2020

fcojfernandez Jan 7, 2020

oleg-nenashev Jan 3, 2020

fcojfernandez Jan 7, 2020

fcojfernandez Jan 7, 2020

fcojfernandez Jan 7, 2020

MRamonLeon Jan 9, 2020

varyvol Jan 9, 2020

fcojfernandez Jan 9, 2020

varyvol Jan 9, 2020

oleg-nenashev Jan 10, 2020

fcojfernandez Jan 13, 2020

fcojfernandez commented Jan 9, 2020

varyvol Jan 9, 2020

oleg-nenashev left a comment

oleg-nenashev Jan 10, 2020

fcojfernandez Jan 13, 2020

oleg-nenashev Jan 13, 2020

fcojfernandez commented Jan 13, 2020

oleg-nenashev left a comment

fcojfernandez commented Jan 13, 2020

oleg-nenashev left a comment

	public void doRssLatest(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException {
	final List<Run> lastBuilds = new ArrayList<>();
	for (AbstractProject<?, ?> p : Jenkins.get().allItems(AbstractProject.class)) {
	for (AbstractBuild<?, ?> b = p.getLastBuild(); b != null; b = b.getPreviousBuild()) {
	if (relatedTo(b)) {
	lastBuilds.add(b);
	break;
	}
	}
	}
	// historically these have been reported sorted by project name, we switched to the lazy iteration
	// so we only have to sort the sublist of runs rather than the full list of irrelevant projects
	lastBuilds.sort((o1, o2) -> Items.BY_FULL_NAME.compare(o1.getParent(), o2.getParent()));
	rss(req, rsp, " latest build", RunList.fromRuns(lastBuilds), Run.FEED_ADAPTER_LATEST);
	}

[JENKINS-60577] - Prevent the RSS feed in Computer page from returning an error 404 #4411

[JENKINS-60577] - Prevent the RSS feed in Computer page from returning an error 404 #4411

Conversation

fcojfernandez commented Jan 2, 2020

Proposed changelog entries

Submitter checklist

Desired reviewers

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcojfernandez commented Jan 9, 2020

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcojfernandez commented Jan 13, 2020

oleg-nenashev left a comment

Choose a reason for hiding this comment

fcojfernandez commented Jan 13, 2020

oleg-nenashev left a comment

Choose a reason for hiding this comment