Offering default methods on ParameterizedJob #2864

Merged
merged 7 commits into from May 13, 2017

Conversation

5 participants
@jglick
Member

jglick commented May 1, 2017

Description

Allows subtypes to have less boilerplate. Mentioned as a design pattern in #2730.

Changelog entries

None required.

Desired reviewers

@reviewbybees

@jglick jglick requested a review from abayer May 1, 2017

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees May 1, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

+ * Cancels a scheduled build.
+ * @see ParameterizedJobMixIn#doCancelQueue
+ */
+ @RequirePOST

This comment has been minimized.

@jglick jglick referenced this pull request in jenkinsci/workflow-job-plugin May 1, 2017

Merged

Simplifying code using default methods on ParameterizedJob #45

@abayer

abayer approved these changes May 1, 2017

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick May 1, 2017

Member
[ERROR] …/core/src/main/java/jenkins/model/ParameterizedJobMixIn.java:165: error: reference not found
[ERROR] * Standard implementations of {@link ParameterizedJob#isParametrized}.
[ERROR] ^
[ERROR] 
Member

jglick commented May 1, 2017

[ERROR] …/core/src/main/java/jenkins/model/ParameterizedJobMixIn.java:165: error: reference not found
[ERROR] * Standard implementations of {@link ParameterizedJob#isParametrized}.
[ERROR] ^
[ERROR] 
@oleg-nenashev

Kinda 🐛 since it advertises non-Restricted & non-RequirePOST method in all ParameterizedJob implementations. Even though it is a historic API in AbstractProject, I am not sure we want to offer it in other job types

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

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 2, 2017

Member

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

@oleg-nenashev

oleg-nenashev May 2, 2017

Member

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

This comment has been minimized.

@jglick

jglick May 2, 2017

Member

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

@jglick

jglick May 2, 2017

Member

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

+ * Currently only String parameters are supported.
+ * @see ParameterizedJobMixIn#doBuildWithParameters
+ */
+ default void doBuildWithParameters(StaplerRequest req, StaplerResponse rsp, @QueryParameter TimeDuration delay) throws IOException, ServletException {

This comment has been minimized.

@stephenc

Looks good to me.

The public doBuild method that is being moved does not currently have any annotations, so while I think it should have a @RequirePOST annotation, this refactoring is not making the code worse than it already is and therefore the lack of an annotation is not blocking this PR IMHO

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 2, 2017

Member

@stephenc

... this refactoring is not making the code worse than it already is and therefore the lack of an annotation is not blocking this PR IMHO

From what I see it adds new API to the ParameterizedJob interface (new default method doBuilds(), which was not defined in the interface before), so IMHO it makes the code worse.

Member

oleg-nenashev commented May 2, 2017

@stephenc

... this refactoring is not making the code worse than it already is and therefore the lack of an annotation is not blocking this PR IMHO

From what I see it adds new API to the ParameterizedJob interface (new default method doBuilds(), which was not defined in the interface before), so IMHO it makes the code worse.

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc May 2, 2017

Member

From what I see it adds new API to the ParameterizedJob interface (new default method doBuilds(), which was not defined in the interface before), so IMHO it makes the code worse.

Well ParameterizedJob is a marker interface and this is just a refactoring of methods in classes implementing that marker interface, so I see it as non-blocking for this PR but the lack of annotation should be fixed (just in a different PR with a separate JIRA to track that change as I think the annotation change could affect users that may have been accidentally using the "feature" so to co-mingle with this PR would hide the change)

Member

stephenc commented May 2, 2017

From what I see it adds new API to the ParameterizedJob interface (new default method doBuilds(), which was not defined in the interface before), so IMHO it makes the code worse.

Well ParameterizedJob is a marker interface and this is just a refactoring of methods in classes implementing that marker interface, so I see it as non-blocking for this PR but the lack of annotation should be fixed (just in a different PR with a separate JIRA to track that change as I think the annotation change could affect users that may have been accidentally using the "feature" so to co-mingle with this PR would hide the change)

+ return getParameterizedJobMixIn().scheduleBuild(quietPeriod, c);
+ }
+
+ // omitting scheduleBuild2(int, Action...) since it is defined in SCMTriggerItem (could include less commonly used overloads if desired)

This comment has been minimized.

@jglick

jglick May 2, 2017

Member

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.

@jglick

jglick May 2, 2017

Member

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.

+
+ // omitting scheduleBuild2(int, Action...) since it is defined in SCMTriggerItem (could include less commonly used overloads if desired)
+
+ // cannot offer makeSearchIndex() since it is defined in Job

This comment has been minimized.

@jglick

jglick May 2, 2017

Member

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.

@jglick

jglick May 2, 2017

Member

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 {

This comment has been minimized.

@jglick

jglick May 2, 2017

Member

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

@jglick

jglick May 2, 2017

Member

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

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick May 2, 2017

Member

Test failures are bogus.

Withdrawing for the moment since I want to check if I can offer default versions of even (public) methods defined in supertypes.

Member

jglick commented May 2, 2017

Test failures are bogus.

Withdrawing for the moment since I want to check if I can offer default versions of even (public) methods defined in supertypes.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick May 2, 2017

Member

Hmm,

Failed to execute goal com.infradna.tool:bridge-method-injector:1.15:process (default) on project jenkins-core: Failed to process @WithBridgeMethods: Failed to process …/core/target/classes/hudson/model/AbstractProject.class: INVOKESPECIAL/STATIC on interfaces require ASM 5

seems like exactly what infradna/bridge-method-injector#14 should have been solving. Investigating.

Member

jglick commented May 2, 2017

Hmm,

Failed to execute goal com.infradna.tool:bridge-method-injector:1.15:process (default) on project jenkins-core: Failed to process @WithBridgeMethods: Failed to process …/core/target/classes/hudson/model/AbstractProject.class: INVOKESPECIAL/STATIC on interfaces require ASM 5

seems like exactly what infradna/bridge-method-injector#14 should have been solving. Investigating.

Explained a week ago why the design is intentional; no response since.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick May 9, 2017

Member

BTW test failures in this & related PRs are due to INFRA-1176; will make sure tests pass before merging.

Member

jglick commented May 9, 2017

BTW test failures in this & related PRs are due to INFRA-1176; will make sure tests pass before merging.

@abayer

abayer approved these changes May 9, 2017

@oleg-nenashev

I have checked all usages of "instanceof BuildableItem". Actually there are extra implementations in tests, but I do not see anything risky within jenkinsci org and several other repos I usually check. And yes, the ParameterizedJobMixIn seems to be always buildable according to Javadoc

Here is a list of ones which attracted my attention:

*/
- 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 {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 10, 2017

Member

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.

@oleg-nenashev

oleg-nenashev May 10, 2017

Member

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.

This comment has been minimized.

@jglick

jglick May 10, 2017

Member

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.

@jglick

jglick May 10, 2017

Member

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.

@oleg-nenashev

🐝 after the review of possible regressions due to making all ParameterizedJobs BuildableItems.

@reviewbybees done, passing to @jenkinsci/code-reviewers

@@ -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> {

This comment has been minimized.

@jglick

jglick May 11, 2017

Member

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.

@jglick

jglick May 11, 2017

Member

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.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 13, 2017

Member

According to the discussion at the governance meeting on May 10, merging it towards 2.61

Member

oleg-nenashev commented May 13, 2017

According to the discussion at the governance meeting on May 10, merging it towards 2.61

@oleg-nenashev oleg-nenashev merged commit 37dfa99 into jenkinsci:master May 13, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details

Ykus added a commit to Ykus/jenkins that referenced this pull request May 13, 2017

Offering default methods on ParameterizedJob (#2864)
* Offering default methods on ParameterizedJob.

* Javadoc typo.

* Cleaner use of default methods in ParameterizedJob.

* Need to pick up infradna/bridge-method-injector#15 to be able to build.

* Using new type bounds.

* bridge-method-injector 1.17

@jglick jglick deleted the jglick:ParameterizedJobMixIn-defaults branch May 15, 2017

jglick added a commit to jglick/maven-plugin that referenced this pull request Jan 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment