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] make structs plugin aware of symbols #7

Merged
merged 38 commits into from Jul 28, 2016

Conversation

Projects
None yet
4 participants
@kohsuke
Copy link
Member

kohsuke commented Jun 29, 2016

Introduced a new UninstantiatedDescribable type that represents a de-composed instance with possible symbol & class annotation.

@kohsuke kohsuke force-pushed the JENKINS-29922 branch from 52450fe to 797d893 Jun 29, 2016

*/
public class UninstantiatedDescribable {
private String symbol;
private String $class;

This comment has been minimized.

Copy link
@jglick

jglick Jun 29, 2016

Member

Can we rename this to klazz or similar?

This comment has been minimized.

Copy link
@jglick

jglick Jun 29, 2016

Member

or clazz like in Descriptor

}

/**
* Computes arguments suitable to pass to {@link #instantiate} to reconstruct this object.

This comment has been minimized.

Copy link
@jglick

jglick Jun 29, 2016

Member

Linking to the wrong version of the method.

*
* @author Kohsuke Kawaguchi
*/
public class UninstantiatedDescribable {

This comment has been minimized.

Copy link
@jglick

jglick Jun 29, 2016

Member

Add a toString override please.

@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jun 29, 2016

@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jun 29, 2016

But let's hold the merge until all the changes for JENKINS-29922 is ready

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Jun 29, 2016

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.

}
nested.setSymbol(symbolOf(o));

This comment has been minimized.

Copy link
@jglick

jglick Jun 30, 2016

Member

So this is something I forgot to mention yesterday. Do we now require the symbol even on elements of homomorphic collections? So

checkout(git(userRemoteConfigs: [
  userRemoteConfig(url: '', credentialsId: '')]))

By contrast, $class can be omitted in such cases; the fact that the list element is a Map suffices to infer its type as UserRemoteConfig:

checkout([$class: 'GitSCM', userRemoteConfigs: [
  [url: '', credentialsId: '']]])

If we wanted to carry over that simplification, we could either continue to access Maps in mixed usage:

checkout(git(userRemoteConfigs: [
  [url: '', credentialsId: '']]))

or introduce a dummy function which just creates an UninstantiatedDescribable without a symbol set:

checkout(git(userRemoteConfigs: [
  _(url: '', credentialsId: '')]))

This comment has been minimized.

Copy link
@jglick

jglick Jun 30, 2016

Member

@kohsuke feels it is clearer to require the symbol on each element. When picking that symbol, we may prefer to use a concise name, since it is not used to disambiguate, and the full meaning is clear from the parameter name; for example:

checkout(git(userRemoteConfigs: [
  config(url: '', credentialsId: '')]))

This comment has been minimized.

Copy link
@kohsuke

kohsuke Jun 30, 2016

Author Member

I think we should require it even for the homomorphic collection for clarity. It'll also be less work and tolerates the schema evolution better.

* Can be a short name if it's contextually unambiguous, or a FQCN.
*
* <p>
* Either this or {@link #getSymbol()} has to return a non-null value.

This comment has been minimized.

Copy link
@jglick

jglick Jun 30, 2016

Member

Then getSymbol should have matching Javadoc, to justify its being @Nullable rather than @CheckForNull.

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

(resolved)

this.arguments = arguments;
}

public UninstantiatedDescribable(Map<String, Object> arguments) {

This comment has been minimized.

Copy link
@jglick

jglick Jun 30, 2016

Member

This seems really unsafe. IMO there should be one constructor taking a @Nonnull String symbol, another taking a @Nonnull String klass, and there should no setters for either.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jun 30, 2016

Provisional 👍 until the downstream PR(s) are complete.

kohsuke added some commits Jun 30, 2016

[JENKINS-29922] allow the single argument to be given with the magic …
…argument name

... as a short hand for Describable with a single argument.
[JENKINS-29922] allow UninstantiatedDescribable to refer to a model.
This allows clients to special case single arg case that ANONYMOUS_KEY is trying to handle
@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jun 30, 2016

Note to myself: Jesse's comments so far are all acommodated

kohsuke added some commits Jun 30, 2016

Can't serialize this field
This is bit unkosher, and makes me wonder perhaps I shouldn't have kept this reference, but this reference is convenient when the caller is inspecting a tree of UDs, and for the intended use cases not having this field persisted is OK, so oh well.

 If this needs to be revisited, we can make DescribableModel serializable by making it only persist its type.
Descriptor d = ((Describable) o).getDescriptor();
return symbolOf(d.getClass());
}
// TODO JENKINS-26093 hack, pending core change

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

This would work for uninstantiation. But how could it work for instantiation, at runtime? resolveClass would need the corresponding patch I think. Is this tested?

This comment has been minimized.

Copy link
@kohsuke

kohsuke Jul 12, 2016

Author Member

The resolveClass method already does this via its sub-routine findSubtypes

}

/**
* {@code $class} is an alternative means to specify the class in case there's no symbol.

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

Use {@link DescribableModel#CLAZZ}.

private String symbol;
private String klass;
private final Map<String,?> arguments;
private transient DescribableModel model;

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

You do not need to have anything restoring this in readResolve, so it is not really safely Serializable if anyone calls methods like instantiate later. Did you actually test serializing it?

def x = testAttachments('**/*.png')
semaphore 'restart-here'
junit testDataPublishers: [x]

This comment has been minimized.

Copy link
@kohsuke

kohsuke Jul 12, 2016

Author Member

For instantiation this field is not used, so your code fragment above will work as expected. See comment in 8b12c66. I'll leave this as a comment in code.

This comment has been minimized.

Copy link
@jglick

jglick Jul 13, 2016

Member

Not sure I follow. There are several methods that behave differently if model is unset. How can you be sure that they will never be called on a deserialized instance?

And was there actually a test somewhere that demonstrated instantiation after restart? If so, I must have missed it.

if (v instanceof List) {
ListIterator litr = ((List) v).listIterator();
while (litr.hasNext()) {
litr.set(toMap(litr.next()));

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

Just create a fresh List, do not mutate the original.

if (klass !=null)
r.put(DescribableModel.CLAZZ, klass);
if (symbol!=null)
r.put(DescribableModel.SYMBOL,symbol);

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

Is SYMBOL retrieved anywhere? This seems to be the only usage unless I missed something.

/**
* @author Kohsuke Kawaguchi
*/
public interface Fishing {

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

An ExtensionPoint?

/**
* @author Kohsuke Kawaguchi
*/
public interface Tech {

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

ditto

@@ -53,12 +60,16 @@
import java.util.logging.Level;
import java.util.logging.Logger;

import static org.bouncycastle.asn1.x500.style.RFC4519Style.dc;

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

🐛 !!!

import static org.junit.Assert.*;
import org.junit.Ignore;

@SuppressWarnings("unchecked") // generic array construction
public class DescribableModelTest {
@ClassRule
public static JenkinsRule rule = new JenkinsRule();

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

Would not be necessary if you fixed SymbolLookup to fall back to Thread.contextClassLoader as DescribableModel already did.

This comment has been minimized.

Copy link
@kohsuke

kohsuke Jul 12, 2016

Author Member

I don't see how. I need to find an instance via Guice here

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

I see the problem here—Describable.getDescriptor needs Jenkins loaded, since we are not finding the symbol on the Describable class as you might expect, but rather its associated extension.

Sigh. So then isUnitTest is obsolete. Will fix that.

if (o instanceof ParameterValue) {
try {
Class def = o.getClass().getClassLoader().loadClass(o.getClass().getName().replaceFirst("Value$", "Definition"));
Descriptor d = assertNotNull(Jenkins.getInstance()).getDescriptor(def);

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

🐜 just use Jenkins.getActiveInstance() if you can. If not, inline and throw IllegalStateException (or just return null).

kohsuke and others added some commits Jul 12, 2016

I should be earning like 10 brownie points from Jesse today
Can't wait to see what I get as a prize.
SnippetizerTest.buildTriggerStep was failing in -Djenkins.version=2.7…
….1 due to lack of a complete JENKINS-26093 workaround.

java.lang.AssertionError: java.lang.UnsupportedOperationException: Undefined symbol ‘string’
	at org.jenkinsci.plugins.structs.describable.DescribableModel.resolveClass(DescribableModel.java:425)
	at org.jenkinsci.plugins.structs.describable.UninstantiatedDescribable.instantiate(UninstantiatedDescribable.java:174)
	at org.jenkinsci.plugins.structs.describable.DescribableModel.coerce(DescribableModel.java:363)
	at org.jenkinsci.plugins.structs.describable.DescribableModel.coerceList(DescribableModel.java:442)
	at org.jenkinsci.plugins.structs.describable.DescribableModel.coerce(DescribableModel.java:356)
	at org.jenkinsci.plugins.structs.describable.DescribableModel.injectSetters(DescribableModel.java:332)
	at org.jenkinsci.plugins.structs.describable.DescribableModel.instantiate(DescribableModel.java:258)
	at org.jenkinsci.plugins.workflow.steps.StepDescriptor.newInstance(StepDescriptor.java:195)
	at org.jenkinsci.plugins.workflow.cps.SnippetizerTestRule$1.invokeStep(SnippetizerTestRule.java:99)
	at org.jenkinsci.plugins.workflow.cps.DSL.invokeMethod(DSL.java:117)
	at org.jenkinsci.plugins.workflow.cps.DelegatingScript.invokeMethod(DelegatingScript.java:25)
	at org.codehaus.groovy.runtime.callsite.PogoMetaClassSite.callCurrent(PogoMetaClassSite.java:75)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallCurrent(CallSiteArray.java:52)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:154)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:166)
	at Script1.run(Script1.groovy:1)
	at org.jenkinsci.plugins.workflow.cps.SnippetizerTestRule.assertRoundTrip(SnippetizerTestRule.java:106)
	at org.jenkinsci.plugins.workflow.cps.SnippetizerTest.buildTriggerStep(SnippetizerTest.java:159)
@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jul 27, 2016

🐝

1 similar comment
@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jul 27, 2016

🐝

@jglick jglick merged commit ca1ac40 into master Jul 28, 2016

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the JENKINS-29922 branch Jul 28, 2016

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.