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-26781] Revise #1443 to support descriptors with ID != clazz.name #1563

Merged
merged 16 commits into from Apr 15, 2015

Conversation

@ndeloof
Copy link
Member

commented Feb 4, 2015

JENKINS-26781

so descriptor can be retrieved by ID, which may not be clazz.name

@reviewbybees

@@ -222,7 +222,7 @@ public Descriptor getItemTypeDescriptorOrDie() {
/**
* Help file redirect, keyed by the field name to the path.
*
* @see #getHelpFile(String)
* @see #getHelpFile(String)

This comment has been minimized.

Copy link
@jglick

jglick Feb 4, 2015

Member

Please do not make insignificant whitespace changes in PRs.

@ndeloof ndeloof force-pushed the JENKINS-26781 branch from 094d1cf to 3edce88 Feb 4, 2015

@@ -918,6 +918,7 @@ private URL getStaticHelpUrl(Klass<?> c, String suffix) {
if (d != null) {
items.add(d.newInstance(req, jo));
}
LOGGER.warning("Received unexpected formData for descriptor " + kind);

This comment has been minimized.

Copy link
@jglick

jglick Feb 4, 2015

Member

Should be in an else-clause.

This comment has been minimized.

Copy link
@ndeloof

ndeloof Feb 4, 2015

Author Member

feeling stupid

@@ -34,7 +34,7 @@ THE SOFTWARE.
structured form submission. Mutually exclusive with clazz.
</st:attribute>
</st:documentation>
<j:set var="clazz" value="${attrs.clazz ?: attrs.descriptor.clazz.name}" />
<j:set var="clazz" value="${attrs.clazz ?: attrs.descriptor.id}" />

This comment has been minimized.

Copy link
@jglick

jglick Feb 4, 2015

Member

Should adjust documentation for clazz to emphasize that it should be Descriptor.id more generally.

@ndeloof ndeloof force-pushed the JENKINS-26781 branch from 3edce88 to 57c9b48 Feb 4, 2015

@ndeloof

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2015

investigating on strange regression found by test harness ...

@ndeloof

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2015

this test case demonstrate some code still relies on stapler-class

@ndeloof

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2015

commented on #1443 (comment)
IHMO there's an implementation issue using stapler "$class" parameter to pass an arbitrary ID

@ndeloof ndeloof force-pushed the JENKINS-26781 branch from 57c9b48 to 8881703 Feb 4, 2015

@ndeloof ndeloof changed the title [JENKINS-26781] set $class in submitted form to use descriptor.id [JENKINS-26781] review PR1443 to support descriptors with ID != clazz.name Feb 4, 2015

Descriptor<T> d = null;
String kind = jo.optString("kind", null);
if (kind != null) {
d = find(descriptors, kind);

This comment has been minimized.

Copy link
@ndeloof

ndeloof Feb 5, 2015

Author Member

I wonder it would be cleaner to introduce new "FindById" and "findByClassName" methods and explicitly invoke one of them. find would then use both and marked as deprecated.

This comment has been minimized.

Copy link
@jglick

jglick Feb 5, 2015

Member

Might made things clearer.


/**
* Finds a descriptor from a collection by its class name.
* @deprecated Since we introduced {@link }Descriptor#getId()}, it is a preferred method of identifying descriptor by a string.

This comment has been minimized.

Copy link
@jglick

jglick Feb 5, 2015

Member

Extraneous }.

return d;
}
return null;
}

/**
* Finds a descriptor from a collection by its class name.

This comment has been minimized.

Copy link
@jglick

jglick Feb 5, 2015

Member

Documentation should clarify: class name or ID.

deprecated find(string) and introduce explicit methods
to lookup descriptor by id or by Describable class name

@ndeloof ndeloof force-pushed the JENKINS-26781 branch from b3268bd to 6a8ce6c Feb 5, 2015

@ndeloof

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2015

@mattmoor being author for the initial PR your review would be appreciated

@mattmoor

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2015

Can you please add a test for this "supported" behavior? It clearly isn't covered today.

IMO the evolution of this should be that "$class" and "kind" are mutually exclusive. Emit "kind" iff it isn't clazz.name, otherwise emit it as "$class".

My $0.02

@jglick

This comment has been minimized.

Copy link
Member

commented Feb 6, 2015

Agreed that test coverage for this needs to be added to core. IIUC @kohsuke introduced an overridable id to make things like cloudbees-template possible, but did not cover this mode in a core test case.

Using $class when d.id == d.clazz.name and kind otherwise makes some sense to me. In the latter case the default implementation of newInstance would not work; the descriptor impl is expected to have custom handling.

@ndeloof

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2015

I honestly don't understand descriptor-based databinding enough so I can propose a unit test to cover this.
to be more precise, I have no idea which API call such a test would need to run. Just ensure findById to retrieve a descriptor by ID ?

@ndeloof

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2015

We can't define $class and kind to be mutually exclusive, as DownloadFromUrlInstaller.DescriptorImpl do override getId, so this would enforce ALL tool installer plugins to override newInstance.

@jglick

This comment has been minimized.

Copy link
Member

commented Feb 6, 2015

Only DownloadFromUrlInstaller subclasses would need to override newInstance in the descriptor, not all ToolInstallers.

Of course in this case I question why the getId override is needed to begin with. It seems gratuitous. Simpler and safer would be to do the character substitution in the call to createDownloadable.

make $class and kind mutually exclusive
Use kind when descriptor.id != descriptor.clazz.name and $clazz otherwise.

@ndeloof ndeloof force-pushed the JENKINS-26781 branch from 08081f6 to 901194a Feb 8, 2015

@stephenc

This comment has been minimized.

Copy link
Member

commented Apr 7, 2015

Where are we on this? it seems to me like this pull request highlights that the choice of $class and the "standardization" around it was incorrect and probably needs to be "reverted" with a different approach taken

@jglick

This comment has been minimized.

Copy link
Member

commented Apr 7, 2015

Where are we on this?

I have not personally looked into it deeply enough to take a stance.

@jglick jglick changed the title [JENKINS-26781] review PR1443 to support descriptors with ID != clazz.name [JENKINS-26781] Revise #1443 to support descriptors with ID != clazz.name Apr 11, 2015

jglick added 2 commits Apr 11, 2015
[JENKINS-26781] Reproduced problem in test.
Besides failing in master, it also fails in 4e0af43 (merge of #1443), but passes in the 1.598-SNAPSHOT parent 239caf8.
@kohsuke

This comment has been minimized.

Copy link
Member

commented Apr 11, 2015

OK. I added doc to capture that.

@kohsuke

This comment has been minimized.

Copy link
Member

commented Apr 11, 2015

👍

@jglick

This comment has been minimized.

Copy link
Member

commented Apr 15, 2015

FTR: the PR builder was down, then the build of 2dcb6d3 timed out, then the build of f4ddac1 got eight failures but all from

java.lang.NoClassDefFoundError: Could not initialize class com.cloudbees.plugins.credentials.SystemCredentialsProvider$ProviderImpl

which is a “well-known” random failure (CC @stephenc). I reran those eight tests locally in this branch and they all pass. So merging.

@jglick jglick merged commit f3070d9 into master Apr 15, 2015

1 check was pending

Jenkins Jenkins is validating pull request ...
Details
jglick added a commit that referenced this pull request Apr 15, 2015

@jglick jglick deleted the JENKINS-26781 branch Apr 15, 2015

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Apr 20, 2015

Appears to have broken Copy Artifact: JENKINS-28011

jglick added a commit to jglick/jenkins that referenced this pull request Apr 20, 2015
@@ -122,15 +122,16 @@ protected DescriptorImpl() {
}

protected Downloadable createDownloadable() {
return new Downloadable(getId());
return new Downloadable(getDownloadableId());

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Apr 25, 2015

Member

This broke the tool installer for NodeJS Plugin: JENKINS-28093

tfennelly added a commit to tfennelly/jenkins that referenced this pull request Apr 26, 2015
Merge branch 'master' into JENKINS-26445-pagination-search
* master: (71 commits)
  Noting JENKINS-28062
  print the computer's name as toString() is probably useless
  Noting jenkinsci#1667
  jesse wants a comment explaining devaition from pattern
  [FIXED JENKINS-28062] A Launcher.afterDisconnect() method that propagates an exception is a sign of a bad Launcher not a problem closing the channel.
  [FIXED JENKINS-28058] Don't send a reference to Computer.class over remoting channels
  Revert "[JENKINS-10629] - Migrate the Tar archives handling code to commons-compress"
  Revert "FIXED JENKINS-10629] - Enable BigNumber mode to support archiving of files with size >8Gb"
  [FIXED JENKINS-27218]
  Change Last Active column to reflect only commit activity.
  Allow the advertised TCP slave agent port number to be modified.
  If CLI main thread fails, kill the JVM no matter what.
  [FIXED JENKINS-28011] Amending jenkinsci#1563, ${descriptor} is not necessarily ${attrs.descriptor}!
  Made minor changes to the contributing section in the README.
  [JENKINS-27995] Added contributing section to the README.
  Noting jenkinsci#1645
  updated changelog for release
  [maven-release-plugin] prepare release jenkins-1.610
  [maven-release-plugin] prepare for next development iteration
  Integrating a new version with better diagnostics
  ...
@daniel-beck

This comment has been minimized.

Copy link
Member

commented Apr 27, 2015

Appears to have broken more than just the tool installer in NodeJS Plugin: JENKINS-28114

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Apr 27, 2015

May have broken Performance Plugin: JENKINS-28110

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Apr 27, 2015

With plugins seemingly breaking left and right I wonder whether this should be rolled back.

@jglick

This comment has been minimized.

Copy link
Member

commented Apr 27, 2015

I plan to take a look at these and see if they are really three separate issues or if there is some duplication.

<j:choose>
<j:when test="${descriptor != null and descriptor.id != clazz}">
<input type="hidden" name="kind" value="${attrs.descriptor.id}" />
</j:when>

This comment has been minimized.

Copy link
@ikedam

ikedam Apr 28, 2015

Member

JENKINS-28114 NodeJsInstaller exception is caused for here.

This comment has been minimized.

Copy link
@ikedam
@jglick

This comment has been minimized.

Copy link
Member

commented Apr 28, 2015

@ikedam thanks for tips, that should help me get started faster.

jglick added a commit to jglick/jenkins that referenced this pull request Apr 28, 2015
jglick added a commit to jglick/jenkins that referenced this pull request Apr 28, 2015
@jglick jglick referenced this pull request Apr 28, 2015
5 of 5 tasks complete
@jglick

This comment has been minimized.

Copy link
Member

commented Apr 28, 2015

Why the performance plugin overrides getId is beyond me. This seems be used only by a getById method, which is unused. jenkinsci/performance-plugin@d93fb0c At any rate, it serves as a demo of a plugin which does something weird and which must be supported.

@kohsuke

This comment has been minimized.

Copy link
Member

commented Apr 29, 2015

Since Jesse made the point that I am responsible for the problematic commit in the performance plugin, I feel the need to defend myself. AFAICT, the performance plugin had getId() method first, then the core added Descriptor.getId() later.

@jglick

This comment has been minimized.

Copy link
Member

commented Apr 29, 2015

I see, did not realize this was a recentish addition.

(A situation where it would be nice to make javac upgrade the warning about missing @Override to an error.)

jglick added a commit to jglick/jenkins that referenced this pull request May 14, 2015
[FIXED JENKINS-28011] Amending jenkinsci#1563, ${descriptor} is not n…
…ecessarily ${attrs.descriptor}!

(cherry picked from commit 343382b)
jglick added a commit to jglick/jenkins that referenced this pull request May 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.