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

Offering default methods on ParameterizedJob #2864

Merged
merged 7 commits into from
May 13, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 1 addition & 69 deletions core/src/main/java/hudson/model/AbstractProject.java
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@
* @see AbstractBuild
*/
@SuppressWarnings("rawtypes")
public abstract class AbstractProject<P extends AbstractProject<P,R>,R extends AbstractBuild<P,R>> extends Job<P,R> implements BuildableItem, LazyBuildMixIn.LazyLoadingJob<P,R>, ParameterizedJobMixIn.ParameterizedJob {
public abstract class AbstractProject<P extends AbstractProject<P,R>,R extends AbstractBuild<P,R>> extends Job<P,R> implements BuildableItem, LazyBuildMixIn.LazyLoadingJob<P,R>, ParameterizedJobMixIn.ParameterizedJob<P, R> {
Copy link
Member Author

Choose a reason for hiding this comment

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

implements BuildableItem is now redundant, could be removed I guess.

I have checked all usages of "instanceof BuildableItem".

The only implementations of ParameterizedJob were already BuildableItems so retrofitting it to implement that interface should not cause any behavioral change even if someone were doing that instanceof check.


/**
* {@link SCM} associated with the project.
Expand Down Expand Up @@ -292,15 +292,6 @@ private LazyBuildMixIn<P,R> createBuildMixIn() {
return buildMixIn;
}

private ParameterizedJobMixIn<P,R> getParameterizedJobMixIn() {
return new ParameterizedJobMixIn<P,R>() {
@SuppressWarnings("unchecked") // untypable
@Override protected P asJob() {
return (P) AbstractProject.this;
}
};
}

@Override
public synchronized void save() throws IOException {
super.save();
Expand Down Expand Up @@ -805,39 +796,6 @@ public void doConfigSubmit( StaplerRequest req, StaplerResponse rsp ) throws IOE
Jenkins.getInstance().rebuildDependencyGraphAsync();
}

/**
* @deprecated
* Use {@link #scheduleBuild(Cause)}. Since 1.283
*/
@Deprecated
public boolean scheduleBuild() {
return getParameterizedJobMixIn().scheduleBuild();
}

/**
* @deprecated
* Use {@link #scheduleBuild(int, Cause)}. Since 1.283
*/
@Deprecated
public boolean scheduleBuild(int quietPeriod) {
return getParameterizedJobMixIn().scheduleBuild(quietPeriod);
}

/**
* Schedules a build of this project.
*
* @return
* true if the project is added to the queue.
* false if the task was rejected from the queue (such as when the system is being shut down.)
*/
public boolean scheduleBuild(Cause c) {
return getParameterizedJobMixIn().scheduleBuild(c);
}

public boolean scheduleBuild(int quietPeriod, Cause c) {
return getParameterizedJobMixIn().scheduleBuild(quietPeriod, c);
}

/**
* Schedules a build.
*
Expand Down Expand Up @@ -1746,21 +1704,11 @@ protected HistoryWidget createHistoryWidget() {
return buildMixIn.createHistoryWidget();
}

public boolean isParameterized() {
return getParameterizedJobMixIn().isParameterized();
}

//
//
// actions
//
//
/**
* Schedules a new build command.
*/
public void doBuild( StaplerRequest req, StaplerResponse rsp, @QueryParameter TimeDuration delay ) throws IOException, ServletException {
getParameterizedJobMixIn().doBuild(req, rsp, delay);
}

/** @deprecated use {@link #doBuild(StaplerRequest, StaplerResponse, TimeDuration)} */
@Deprecated
Expand Down Expand Up @@ -1789,14 +1737,6 @@ public int getDelay(StaplerRequest req) throws ServletException {
}
}

/**
* Supports build trigger with parameters via an HTTP GET or POST.
* Currently only String parameters are supported.
*/
public void doBuildWithParameters(StaplerRequest req, StaplerResponse rsp, @QueryParameter TimeDuration delay) throws IOException, ServletException {
getParameterizedJobMixIn().doBuildWithParameters(req, rsp, delay);
}

/** @deprecated use {@link #doBuildWithParameters(StaplerRequest, StaplerResponse, TimeDuration)} */
@Deprecated
public void doBuildWithParameters(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException {
Expand All @@ -1812,14 +1752,6 @@ public void doPolling( StaplerRequest req, StaplerResponse rsp ) throws IOExcept
rsp.sendRedirect(".");
}

/**
* Cancels a scheduled build.
*/
@RequirePOST
public void doCancelQueue( StaplerRequest req, StaplerResponse rsp ) throws IOException, ServletException {
getParameterizedJobMixIn().doCancelQueue(req, rsp);
}

@Override
protected void submit(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException, FormException {
super.submit(req,rsp);
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/triggers/Trigger.java
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ public void run(AbstractProject p) {
}

// Process all triggers, except SCMTriggers when synchronousPolling is set
for (ParameterizedJobMixIn.ParameterizedJob p : inst.allItems(ParameterizedJobMixIn.ParameterizedJob.class)) {
for (ParameterizedJobMixIn.ParameterizedJob<?, ?> p : inst.allItems(ParameterizedJobMixIn.ParameterizedJob.class)) {
for (Trigger t : p.getTriggers().values()) {
if (!(t instanceof SCMTrigger && scmd.synchronousPolling)) {
if (t !=null && t.spec != null && t.tabs != null) {
Expand Down
118 changes: 103 additions & 15 deletions core/src/main/java/jenkins/model/ParameterizedJobMixIn.java
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import javax.servlet.ServletException;
import static javax.servlet.http.HttpServletResponse.SC_CREATED;
import static javax.servlet.http.HttpServletResponse.SC_CONFLICT;
import jenkins.triggers.SCMTriggerItem;
import jenkins.util.TimeDuration;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
Expand Down Expand Up @@ -95,7 +96,7 @@ public final boolean scheduleBuild(int quietPeriod, Cause c) {
}

/**
* Provides a standard implementation of an optional method of the same name in a {@link Job} type to schedule a build with the ability to wait for its result.
* Provides a standard implementation of {@link SCMTriggerItem#scheduleBuild2} to schedule a build with the ability to wait for its result.
* That job method is often used during functional tests ({@code JenkinsRule.assertBuildStatusSuccess}).
* @param quietPeriod seconds to wait before starting (normally 0)
* @param actions various actions to associate with the scheduling, such as {@link ParametersAction} or {@link CauseAction}
Expand Down Expand Up @@ -161,15 +162,14 @@ private List<ParameterValue> getDefaultParametersValues() {
}

/**
* A job should define a method of the same signature for use from {@link BuildButtonColumn}.
* Standard implementations of {@link ParameterizedJob#isParametrized}.
*/
public final boolean isParameterized() {
return asJob().getProperty(ParametersDefinitionProperty.class) != null;
}

/**
* Schedules a new build command.
* Create a method on your job with the same signature and delegate to this.
* Standard implementation of {@link ParameterizedJob#doBuild}.
*/
@SuppressWarnings("deprecation")
public final void doBuild(StaplerRequest req, StaplerResponse rsp, @QueryParameter TimeDuration delay) throws IOException, ServletException {
Expand Down Expand Up @@ -206,9 +206,7 @@ public final void doBuild(StaplerRequest req, StaplerResponse rsp, @QueryParamet
}

/**
* Supports build trigger with parameters via an HTTP GET or POST.
* Currently only String parameters are supported.
* Create a method on your job with the same signature and delegate to this.
* Standard implementation of {@link ParameterizedJob#doBuildWithParameters}.
*/
@SuppressWarnings("deprecation")
public final void doBuildWithParameters(StaplerRequest req, StaplerResponse rsp, @QueryParameter TimeDuration delay) throws IOException, ServletException {
Expand All @@ -226,8 +224,7 @@ public final void doBuildWithParameters(StaplerRequest req, StaplerResponse rsp,
}

/**
* Cancels a scheduled build.
* Create a method on your job marked {@link RequirePOST} but with the same signature and delegate to this.
* Standard implementation of {@link ParameterizedJob#doCancelQueue}.
*/
@RequirePOST
public final void doCancelQueue( StaplerRequest req, StaplerResponse rsp ) throws IOException, ServletException {
Expand Down Expand Up @@ -275,7 +272,6 @@ public static final CauseAction getBuildCause(ParameterizedJob job, StaplerReque

/**
* Suggested implementation of {@link ParameterizedJob#getBuildNowText}.
* Uses {@link #BUILD_NOW_TEXT}.
*/
public final String getBuildNowText() {
return isParameterized() ? AlternativeUiTextProvider.get(BUILD_NOW_TEXT, asJob(), Messages.ParameterizedJobMixIn_build_with_parameters())
Expand All @@ -294,7 +290,7 @@ public final String getBuildNowText() {
if (!(job instanceof ParameterizedJob)) {
return null;
}
for (Trigger<?> t : ((ParameterizedJob) job).getTriggers().values()) {
for (Trigger<?> t : ((ParameterizedJob<?, ?>) job).getTriggers().values()) {
if (clazz.isInstance(t)) {
return clazz.cast(t);
}
Expand All @@ -303,16 +299,42 @@ public final String getBuildNowText() {
}

/**
* Marker for job using this mixin.
* Marker for job using this mixin, and default implementations of many methods.
*/
public interface ParameterizedJob extends hudson.model.Queue.Task, hudson.model.Item {
public interface ParameterizedJob<JobT extends Job<JobT, RunT> & ParameterizedJobMixIn.ParameterizedJob<JobT, RunT> & Queue.Task, RunT extends Run<JobT, RunT> & Queue.Executable> extends BuildableItem {
Copy link
Member

Choose a reason for hiding this comment

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

To follow-up on my bug in #2874 (comment) . Effectively it makes any any Parameterized job explicitly buildable. After some consideration, it seems to be a valid change according to the ParameterizedJobMixIn documentation.

🐜 since I expect ParameterizedJob Javadoc to explicitly say it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well the mixin Javadoc does say

be scheduled in various ways

which I guess is enough. Anyway BuildableItem is an interface which is not used all that much and not especially interesting to people grokking this code.


/**
* Creates a helper object.
* (Would have been done entirely as an interface with default methods had this been designed for Java 8.)
*/
default ParameterizedJobMixIn<JobT, RunT> getParameterizedJobMixIn() {
return new ParameterizedJobMixIn<JobT, RunT>() {
@SuppressWarnings("unchecked") // untypable
@Override protected JobT asJob() {
return (JobT) ParameterizedJob.this;
}
};
}

@SuppressWarnings("deprecation")
@CheckForNull hudson.model.BuildAuthorizationToken getAuthToken();

int getQuietPeriod();
/**
* Quiet period for the job.
* @return by default, {@link Jenkins#getQuietPeriod}
*/
default int getQuietPeriod() {
return Jenkins.getInstance().getQuietPeriod();
}

String getBuildNowText();
/**
* Text to display for a build button.
* Uses {@link #BUILD_NOW_TEXT}.
* @see ParameterizedJobMixIn#getBuildNowText
*/
default String getBuildNowText() {
return getParameterizedJobMixIn().getBuildNowText();
}

/**
* Gets currently configured triggers.
Expand All @@ -322,6 +344,72 @@ public interface ParameterizedJob extends hudson.model.Queue.Task, hudson.model.
*/
Map<TriggerDescriptor,Trigger<?>> getTriggers();

/**
* @deprecated use {@link #scheduleBuild(Cause)}
*/
@Deprecated
@Override
default boolean scheduleBuild() {
return getParameterizedJobMixIn().scheduleBuild();
}

@Override
default boolean scheduleBuild(Cause c) {
return getParameterizedJobMixIn().scheduleBuild(c);
}

/**
* @deprecated use {@link #scheduleBuild(int, Cause)}
*/
@Deprecated
@Override
default boolean scheduleBuild(int quietPeriod) {
return getParameterizedJobMixIn().scheduleBuild(quietPeriod);
}

@Override
default boolean scheduleBuild(int quietPeriod, Cause c) {
return getParameterizedJobMixIn().scheduleBuild(quietPeriod, c);
}

// omitting scheduleBuild2(int, Action...) since it is defined in SCMTriggerItem (could include less commonly used overloads if desired)
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually as per this tip it could be defined here. Implementing classes would still need to define the method, but at least they could select a known super implementation, which is probably more discoverable than looking for similar methods on a mixin.


// cannot offer makeSearchIndex() since it is defined in Job
Copy link
Member Author

Choose a reason for hiding this comment

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

This case would still be tricky however, since the defining method is protected and we do not really want to force implementations to override it as public.


/**
* Schedules a new build command.
* @see ParameterizedJobMixIn#doBuild
*/
default void doBuild(StaplerRequest req, StaplerResponse rsp, @QueryParameter TimeDuration delay) throws IOException, ServletException {
Copy link
Member

Choose a reason for hiding this comment

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

So, no Restricted/RequirePOST annotations here? I am not sure it is safe to offer such default impl. 🐛 , but probably I miss something.

Copy link
Member Author

Choose a reason for hiding this comment

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

The original had neither annotation. @Restricted cannot be added compatibly. @RequirePOST is unwanted since the implementation checks the request method explicitly.

getParameterizedJobMixIn().doBuild(req, rsp, delay);
}

/**
* Supports build trigger with parameters via an HTTP GET or POST.
* Currently only String parameters are supported.
* @see ParameterizedJobMixIn#doBuildWithParameters
*/
default void doBuildWithParameters(StaplerRequest req, StaplerResponse rsp, @QueryParameter TimeDuration delay) throws IOException, ServletException {
Copy link
Member

Choose a reason for hiding this comment

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

same

getParameterizedJobMixIn().doBuildWithParameters(req, rsp, delay);
}

/**
* Cancels a scheduled build.
* @see ParameterizedJobMixIn#doCancelQueue
*/
@RequirePOST
Copy link
Member Author

Choose a reason for hiding this comment

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

default void doCancelQueue(StaplerRequest req, StaplerResponse rsp ) throws IOException, ServletException {
getParameterizedJobMixIn().doCancelQueue(req, rsp);
}

/**
* For use from {@link BuildButtonColumn}.
* @see ParameterizedJobMixIn#isParameterized
*/
default boolean isParameterized() {
return getParameterizedJobMixIn().isParameterized();
}

}

}
7 changes: 7 additions & 0 deletions core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java
Original file line number Diff line number Diff line change
Expand Up @@ -271,10 +271,17 @@ public final HistoryWidget createHistoryWidget() {
*/
public interface LazyLoadingJob<JobT extends Job<JobT,RunT> & Queue.Task & LazyBuildMixIn.LazyLoadingJob<JobT,RunT>, RunT extends Run<JobT,RunT> & LazyLoadingRun<JobT,RunT>> {
LazyBuildMixIn<JobT,RunT> getLazyBuildMixIn();
// not offering default implementation for _getRuns(), removeRun(R), getBuild(String), getBuildByNumber(int), getFirstBuild(), getLastBuild(), getNearestBuild(int), getNearestOldBuild(int), or createHistoryWidget() since they are defined in Job
// nor for createExecutable() since that typically calls isDisabled() first
}

/**
* Marker for a {@link Run} which uses this mixin.
*/
public interface LazyLoadingRun<JobT extends Job<JobT,RunT> & Queue.Task & LazyBuildMixIn.LazyLoadingJob<JobT,RunT>, RunT extends Run<JobT,RunT> & LazyLoadingRun<JobT,RunT>> {
RunMixIn<JobT,RunT> getRunMixIn();
// not offering default implementations for createReference() or dropLinks() since they are protected
// nor for getPreviousBuild() or getNextBuild() since they are defined in Run
}

/**
Expand Down