Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-74795] Job created via REST API attaches to default view #9947

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

basil
Copy link
Member

@basil basil commented Nov 5, 2024

Problem

See JENKINS-74795. Creating jobs via the createItem REST API erroneously attaches those items to the default view after #9672.

Evaluation

In the working scenario prior to #9672, the Jenkins#doCreateItem method is invoked by Stapler via its routable interface method ModifiableItemGroup#doCreateItem. In the failing scenario after #9672, the (incorrect) ListView#doCreateItem method is invoked by Stapler via its routable superclass method View#doCreateItem. This (mostly) works by accident, since View#doCreateItem happens to delegate to Jenkins#doCreateItem, but the incorrect behavior is still apparent when the item is incorrectly added to the default view. View#doCreateItem should never be invoked in this scenario, and the only reason it is was a regression in routing in #9672.

Solution

Make the Jenkins#doCreateItem(StaplerRequest2, StaplerResponse2) default method routable, as the equivalent interface method was prior to #9672. For consistency with View#doCreateItem(StaplerRequest2, StaplerResponse2), add a @RequirePOST annotation as well. The deprecated compatibility methods remain non-routable as before.

Testing done

Proposed changelog entries

  • Do not add jobs created via the REST API to the default view.

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Preview Give feedback

Closes #9933

@basil basil added the regression-fix Pull request that fixes a regression in one of the previous Jenkins releases label Nov 5, 2024
Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>
basil added a commit to basil/acceptance-test-harness that referenced this pull request Nov 5, 2024
basil added a commit to basil/bom that referenced this pull request Nov 5, 2024
@basil basil added the needs-security-review Awaiting review by a security team member label Nov 5, 2024
@basil basil requested a review from a team November 5, 2024 22:11
@basil basil marked this pull request as ready for review November 5, 2024 22:11
basil added a commit to basil/bom that referenced this pull request Nov 6, 2024
@daniel-beck daniel-beck added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Nov 7, 2024
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

Compared Stapler traces of 2.474, 2.484, and PR build, for both Jenkins and Folder (legacy types without 2), and XML POST and copy mode, and this essentially restores the previous behavior. GET gets rejected as expected.

(Note to myself: RFE idea: Show parameter types in Stapler traces for web methods.)

@basil basil requested a review from a team November 7, 2024 16:08
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

I compared 2.474, 2.475, and the pre-release build from this pull request. The results from this build match the results from 2.474. I checked:

  • Job created with createItem at root no longer assigns the job to the primary view
  • Job created with createItem inside a view assigns the job to the view

Those behaviors are consistent between 2.474 and the build of this pull request. Thanks very much!

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.

/label ready-for-merge

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Nov 7, 2024
Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>
@basil basil merged commit 8298257 into jenkinsci:master Nov 8, 2024
16 checks passed
krisstern pushed a commit to krisstern/jenkins that referenced this pull request Nov 9, 2024
…nkinsci#9947)

Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>
(cherry picked from commit 8298257)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback regression-fix Pull request that fixes a regression in one of the previous Jenkins releases security-approved @jenkinsci/core-security-review reviewed this PR for security issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants