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-29922] mark meta step #6

Merged
merged 20 commits into from Jul 28, 2016
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
Expand Up @@ -80,7 +80,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>structs</artifactId>
<version>1.1</version>
<version>1.3-SNAPSHOT</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
Expand Down
Expand Up @@ -24,15 +24,26 @@

package org.jenkinsci.plugins.workflow.steps;

import com.google.common.base.Predicate;
import com.google.common.collect.Iterables;
import hudson.ExtensionList;
import hudson.Util;
import hudson.model.Describable;
import hudson.model.Descriptor;

import java.io.IOException;
import java.util.Collections;
import java.util.Map;
import java.util.Set;
import java.util.logging.Level;
import java.util.logging.Logger;

import org.jenkinsci.plugins.structs.describable.DescribableModel;
import org.jenkinsci.plugins.structs.describable.DescribableParameter;
import org.jenkinsci.plugins.structs.describable.UninstantiatedDescribable;
import org.kohsuke.stapler.DataBoundConstructor;

import javax.annotation.Nullable;

/**
* @author Kohsuke Kawaguchi
Expand Down Expand Up @@ -92,6 +103,79 @@ public boolean isAdvanced() {
return false;
}

/**
* Some steps, such as {@code CoreStep} or {@code GenericSCMStep} can take
Copy link
Member

Choose a reason for hiding this comment

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

But you have no PR modifying GenericSCMStep, do you?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we do that in a follow up change? There's some additional complication around how to migrate svn and git steps, too.

Copy link
Member

Choose a reason for hiding this comment

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

Well, but other SCM implementations should work right away.

* arbitrary {@link Describable}s of a certain type and execute it as a step.
* Such a step should return true from this method so that {@link Describable}s that
* it supports can be directly written as a step as a short-hand.
*
* <p>
* Meta-step works as an invisible adapter that creates an illusion that {@link Describable}s are
* steps.
*
* <p>
* For example, in Jenkins Pipeline, if there is a meta step that can handle a {@link Describable},
* and it has a symbol, it allows the following short-hand:
*
* <pre>
* public class Xyz extends Foo {
* &#64;DataBoundConstructor
Copy link
Member

Choose a reason for hiding this comment

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

IIRC if you just wrap this all in {@code …} you do not need to bother with such escapes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just tried but it doesn't handle multiple lines

Copy link
Member

Choose a reason for hiding this comment

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

Yes it does!

Copy link
Member

Choose a reason for hiding this comment

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

Huh, I swore it did, but it seems it does not.

Copy link
Member

@johnou johnou Jul 26, 2016

Choose a reason for hiding this comment

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

@jglick @kohsuke it does, however if you use curly braces "{}" in your example code the first closing brace will terminate the @code block :(

Copy link
Member

Choose a reason for hiding this comment

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

Right, I found a lengthy list of answers on StackOverflow about it.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like we read the same page.

Copy link
Member

Choose a reason for hiding this comment

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

* public Xyz(String value) { ... }
*
* &#64;Extension &#64;Symbol("xyz")
* public static class DescriptorImpl extends FooDescriptor { ... }
* }
*
* public class MetaStepForFoo extends AbstractStepImpl {
* &#64;DataBoundConstructor
* public MetaStepForFoo(Foo delegate) {
* ...
* }
*
* ...
* &#64;Extension
* public static class DescriptorImpl extends AbstractStepDescriptorImpl {
* &#64;Override
* public String getFunctionName() {
* return "metaStepForFoo";
* }
* }
Copy link
Member

Choose a reason for hiding this comment

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

This should also show you overriding isMetaStep!

* }
*
* // this is the short-hand that users will use
* xyz('hello')
* // but this is how it actually gets executed
* metaStepForFoo(xyz('hello'))
* </pre>
*
* <p>
* Meta-step must have a {@link DataBoundConstructor} whose first argument represents a
* {@link Describable} that it handles.
*/
public boolean isMetaStep() {
return false;
}

/**
* For a {@linkplain #isMetaStep() meta step}, return the type that this meta step handles.
* Otherwise null.
*/
// not @CheckForNull because often the caller is using allMeta()
Copy link
Member

Choose a reason for hiding this comment

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

Then use @Nullable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think adding @Nullable to return value to anything as far as tools are concerned.

Copy link
Member

Choose a reason for hiding this comment

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

It is better for documentation purposes.

public final Class<?> getMetaStepArgumentType() {
if (!isMetaStep()) return null;

DescribableModel<?> m = new DescribableModel(clazz);
DescribableParameter p = m.getFirstRequiredParameter();
if (p==null) {
LOGGER.log(Level.WARNING, getClass()+" claims to be a meta-step but it has no parameter in @DataBoundConstructor");
// don't punish users for a mistake by a plugin developer. return null instead of throwing an error.
// returning a type that doesn't match anything normally prevents this broken StepDescriptor from getting used.
return Void.class;
Copy link
Member

Choose a reason for hiding this comment

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

Would not null be more appropriate?

Copy link
Member

Choose a reason for hiding this comment

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

(the line above claims you are returning null, but then you do not)

Copy link
Member

Choose a reason for hiding this comment

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

And do you really mean Void.class, which does match a null reference, rather than Void.TYPE ~ void.class, which is the bottom type?

}

return p.getErasedType();
}

/**
* Used when a {@link Step} is instantiated programmatically.
* The default implementation just uses {@link DescribableModel#instantiate}.
Expand All @@ -109,9 +193,33 @@ public Step newInstance(Map<String,Object> arguments) throws Exception {
* @param step a fully-configured step (assignable to {@link #clazz})
* @return arguments that could be passed to {@link #newInstance} to create a similar step instance
* @throws UnsupportedOperationException if this descriptor lacks the ability to do such a calculation
* @deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Also add @Deprecated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

* Use {@link #uninstantiate(Step)}
*/
public Map<String,Object> defineArguments(Step step) throws UnsupportedOperationException {
return DescribableModel.uninstantiate_(step);
if (Util.isOverridden(StepDescriptor.class, getClass(), "uninstantiate", Step.class)) {
// if the subtype has defined the uninstantiate() method, delegate to that
return uninstantiate(step).toMap();
Copy link
Member

Choose a reason for hiding this comment

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

This is not really going to work, is it? Because it will be returning a map with entries containing UninstantiatedDescribable values; whereas callers of this method will be expecting entries containing Map values.

Copy link
Member Author

Choose a reason for hiding this comment

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

UninstantiatedDescribable.toMap() recursively replaces UninstantiatedDescribable to a map thereby correctly implementing the expected semantics.

} else {
// otherwise assume legacy usage
return DescribableModel.uninstantiate_(step);
}
}

/**
* Determine which arguments went into the configuration of a step configured through a form submission.
* @param step a fully-configured step (assignable to {@link #clazz})
* @return arguments that could be passed to {@link #newInstance} to create a similar step instance
* @throws UnsupportedOperationException if this descriptor lacks the ability to do such a calculation
*/
public UninstantiatedDescribable uninstantiate(Step step) throws UnsupportedOperationException {
if (Util.isOverridden(StepDescriptor.class, getClass(), "uninstantiate", Step.class)) {
// newer clients are called older implementations
return new UninstantiatedDescribable(defineArguments(step));
Copy link
Member

Choose a reason for hiding this comment

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

Again this does not look right. You would need to translate Map values to UninstantiatedDescribable values, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can, but doing so doesn't buy us much, so we don't need to.

a literal map is a valid part of the object graph in UninstantiatedDescribable, and we'll just end up producing that with a map entry named "$class". We cannot reliably look at such a map and recreate a UninstantiatedDescribable without knowing the expected type.

We talked about this earlier and we said it's OK for legacy step implementations to behave poorly wrt snippetizer, the primary client of this functionality.

Copy link
Member

Choose a reason for hiding this comment

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

OK but best to put this explanation in comments so the tradeoff is clear.

} else {
// the default behaviour in the absence of any overrides
return DescribableModel.uninstantiate2_(step);
}
}


Expand All @@ -131,4 +239,18 @@ public final void checkContextAvailability(StepContext c) throws MissingContextV
public static ExtensionList<StepDescriptor> all() {
return ExtensionList.lookup(StepDescriptor.class);
}

/**
* Convenience method to iterate all meta step descriptors.
*/
public static Iterable<StepDescriptor> allMeta() {
return Iterables.filter(all(), new Predicate<StepDescriptor>() {
@Override
public boolean apply(@Nullable StepDescriptor i) {
return i!=null && i.isMetaStep();
Copy link
Member

Choose a reason for hiding this comment

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

Huh? How could ExtensionList.lookup be returning null elements?

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 is another "let's keep findbugs happy exercise"

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried a few variants just now like (1) not having annotation, (2) switch it to @Nonnull etc but either my IDE is unhappy or findbugs is unhappy.

Copy link
Member

Choose a reason for hiding this comment

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

@SuppressFBWarning if it is giving a false positive.

Copy link
Member

@jglick jglick Jul 12, 2016

Choose a reason for hiding this comment

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

Pending -source 8 we cannot annotate type parameters on the return value of lookup.

}
});
}

private static final Logger LOGGER = Logger.getLogger(StepDescriptor.class.getName());
}