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

[FIXED JENKINS-22828] Expect an AbstractProject for input validation. #36

Merged
merged 1 commit into from May 3, 2014

Conversation

Projects
None yet
5 participants
@daniel-beck
Copy link
Member

commented Apr 30, 2014

Input validation leads to wrong validation in case of a Cloudbees Templates plugin/Cloudbees Folders plugin combination, because the getParent() of the @AncestorInPath AbstractItem doesn't have to be the current AbstractProject's getParent().

Steps to reproduce

  1. Create a folder /F
  2. Create a freestyle job /F/fs
  3. Create a job template /templ with an Heterogeneous components from descriptor attribute, use hudson.tasks.Builder as class. Use /F/fs as base for the Groovy transformation, but don't add any code (unnecessary).
  4. Create a job based on /templ named /F/templ-inst.
  5. Add a Copy Artifact build step. Enter fs as (relative) source project path.

Expected results

Valid input.

Actual results

No such project 'fs'. Did you mean 'fs'?

Notes

Using AbstractProject since that's used a few lines down for finding the recommendation.

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Apr 30, 2014

plugins » copyartifact-plugin #67 SUCCESS
This pull request looks good

@daniel-beck daniel-beck changed the title [FIXED JENKINS-22828] Expect an AbstractProject for autocompletion. [FIXED JENKINS-22828] Expect an AbstractProject for input validation. Apr 30, 2014

@daniel-beck

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2014

s/autocompletion/input validation/g
@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Apr 30, 2014

plugins » copyartifact-plugin #68 SUCCESS
This pull request looks good

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented May 1, 2014

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@ikedam

This comment has been minimized.

Copy link
Member

commented May 1, 2014

I cannot get how the problem happens (I don't use Cloudbees plugins).
What does @AncestorInPath AbstractItem get in this case?
Please let me know an example URL when configuring templated projects.

@daniel-beck

This comment has been minimized.

Copy link
Member Author

commented May 1, 2014

It returns the job template /templ (hence the different getParent(). The Templates plugin uses JobProperty.getJobOverrides() to create a custom job config page with only what you allow changed (see figure 10.8 here). You don't even see anything in the URL, it's still /job/F/job/templ-inst/configure. (If you ever wondered why only Job is StaplerOverridable, I'm pretty sure this use case is why!)

These are the StaplerRequest.getAncestors() as seen from doCheckProjectName:

[0] = {org.kohsuke.stapler.AncestorImpl@8048}"hudson.model.Hudson@5bb4a316"
[1] = {org.kohsuke.stapler.AncestorImpl@8049}"com.cloudbees.hudson.plugins.folder.Folder@7f2267eb[F]"
[2] = {org.kohsuke.stapler.AncestorImpl@8050}"hudson.model.FreeStyleProject@6e8c5ba[F/templ-inst]"
[3] = {org.kohsuke.stapler.AncestorImpl@8051}"com.cloudbees.hudson.plugins.modeling.impl.entity.Configurator@57292bf1"
[4] = {org.kohsuke.stapler.AncestorImpl@8052}"com.cloudbees.hudson.plugins.modeling.impl.jobTemplate.JobTemplate@3628e57f[templ]"
[5] = {org.kohsuke.stapler.AncestorImpl@8053}"com.cloudbees.hudson.plugins.modeling.Attribute@448b9a6e"
[6] = {org.kohsuke.stapler.AncestorImpl@8054}"com.cloudbees.hudson.plugins.modeling.controls.HeteroDescribableListControl@2947ceea"
[7] = {org.kohsuke.stapler.AncestorImpl@8055}"hudson.plugins.copyartifact.CopyArtifact$DescriptorImpl@65e42a6c"
@ikedam

This comment has been minimized.

Copy link
Member

commented May 1, 2014

Thanks, I got it.
I'll review and merge.

@ikedam

This comment has been minimized.

Copy link
Member

commented May 2, 2014

What I will do:

  • Test what happens when no AbstractProject in an anscestor of the path.
    • To verify what happens in regression.
    • Expected: null is passed or doCheckProjectName is not called.
    • In former case, we'd better add null check.
    • I don't think any regressions happen with this change. I'll do that just to be safe.
  • List up classes derived from AbstractItem, but not from AbstractProject.
    • Especially, are there classes derived directly from Job?
    • It's impossible to check all plugins and I'll have a look on Jenkins core and some major plugins (like maven-plugin).
    • For example, ViewJob is a subclass of Job. But I don't think copyartifact-plugin is used with ViewJob.

This problem happens also in other plugins. (e.g. https://github.com/jenkinsci/parameterized-trigger-plugin/blob/master/src/main/java/hudson/plugins/parameterizedtrigger/BuildTriggerConfig.java#L533 ).
I'll also check them.

@daniel-beck

This comment has been minimized.

Copy link
Member Author

commented May 2, 2014

Test what happens when no AbstractProject in an ancestor of the path.

AbstractProject is the common superclass of AbstractMavenProject and Project/FreestyleProject, so this should cover the major ones. Is this even a realistic use case? Why would something that isn't an AbstractProject have builders? That class is what introduces workspaces to the hierarchy!

Still, if this is only about the null-safety for the freak cases, I'll add that.

List up classes derived from AbstractItem, but not from AbstractProject.

Off the top of my head:

@ikedam

This comment has been minimized.

Copy link
Member

commented May 3, 2014

Thanks for listing up!

That class is what introduces workspaces to the hierarchy!

This makes sence.
This means copyartifact can work only with AbstractProject.

And BuildStepCompatibilityLayer.perform is called with AbstractBuild, whose parent is always AbstractProject.

I have not to worry about a regression.

ikedam added a commit that referenced this pull request May 3, 2014

Merge pull request #36 from daniel-beck/JENKINS-22828
[FIXED JENKINS-22828] Expect an AbstractProject for input validation.

@ikedam ikedam merged commit 59d7982 into jenkinsci:master May 3, 2014

@jglick

This comment has been minimized.

Copy link
Member

commented May 17, 2014

I think the fix is correct but the explanation is not. getJobOverrides only inserts the Configurator, which is not an Item. The JobTemplate, along with the Attribute and Control, are inserted using f:withCustomDescriptorByName (from the Configurator’s configuration screen). While the page URL remains that of the templatized job, if you look at the “AJAX” URL being passed to the doCheck* method (or whatever), you will see something longer that matches the ancestor list mentioned earlier.

@jglick

This comment has been minimized.

Copy link
Member

commented May 17, 2014

Unfortunately (@kohsuke) it seems that no OSS plugins currently use f:withCustomDescriptorByName or getJobOverrides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.