[JENKINS-26535] added support for parameterized parameter types #52

daspilker · 2019-08-19T19:04:23Z

Follow up of https://groups.google.com/forum/#!msg/jenkinsci-dev/-XVFCCiE4Bs

This fixed the issue mentioned on the mailing list, but does probably not work for all cases and might not fix JENKINS-26535 completely.

[JENKINS-26535]

jglick · 2019-08-19T19:25:38Z

plugin/src/main/java/org/jenkinsci/plugins/structs/describable/ParameterType.java

+                return new ArrayType(type, of(Types.getTypeArgument(Types.getBaseClass(type,Collection.class), 0, Object.class)));
+            }
+            Class<?> c = Types.erasure(type);
+            // Assume it is a nested object of some sort.


Most of this is actually reindentation, making the diff look a lot bigger than it actually is. Had to apply the WS ignore setting in GitHub to see it.

jglick · 2019-08-19T19:26:39Z

plugin/src/main/java/org/jenkinsci/plugins/structs/describable/ParameterType.java

-            if (Types.isSubClassOf(type, Collection.class)) {
-                return new ArrayType(type, of(Types.getTypeArgument(Types.getBaseClass(type,Collection.class), 0, Object.class)));
-            }
-            throw new UnsupportedOperationException("do not know how to categorize attributes of type " + type);


This seems to have been removed and I am not sure why. Where is the fallback?

I guess it is just not necessary any more and whatever sanity-checking was needed is already done in DescribableModel?

The fallback became unreachable, so I removed it. I was surprised that no unit tests failed. There is not much sanity-checking in DescribableModel or DescribableParameter.

I thought about limiting support for Homo/HeterogeneousObjectType to Describable types and keeping the fallback, but that triggered a test failure in DescribableModelTest#recursion since Recursion is not a Describable. I'm not sure if that is on purpose.

daspilker · 2019-08-20T15:20:41Z

I pushed some tests for the problems that have been mentioned in the description of JENKINS-26535. So I'm pretty confident that this fixes JENKINS-26535.

I also ran the Job DSL tests successfully with a snapshot build.

baymac · 2019-09-06T16:55:27Z

@daspilker @jglick Is it ready for release?

plugin/src/test/java/org/jenkinsci/plugins/structs/describable/DescribableModelTest.java

…/DescribableModelTest.java Co-Authored-By: Joseph Petersen <josephp90@gmail.com>

jetersen · 2019-10-17T17:16:11Z

@abayer @dwnusbaum @jglick 👋 we have a test case that confirms that JENKINS-26535 is resolved 👍

Would be great with a release!

jetersen · 2019-11-13T13:16:56Z

@car-roll this is still a blocker, can this be added to fix this very old bug

jetersen · 2019-11-26T15:40:55Z

ping @car-roll @jglick 😓

jglick · 2019-11-26T16:01:17Z

I do not maintain this plugin, do not ask me.

jetersen · 2019-11-26T16:07:16Z

You reviewed the code so maybe the maintainer is waiting for your approval? 😅

car-roll

Code-wise everything looks fine to me, but I'm not familiar enough with this part of the code to give an approval. I will defer to @dwnusbaum or @abayer

Also, apologies for the late reply 😢 I had meant to comment on this earlier.

dwnusbaum

The code seems ok to me. I don't feel like I really understand the implications, and I'm not aware of the original intent behind this code to know if there is any reason to not do this (for example, I think that previously, parameters of type Map were not supported in some cases based on some testing I did with ParallelStep, but I retested with this PR, and now they appear to be supported. Is that a problem? 🤷‍♂).

I also ran workflow-cps's tests against this change and didn't see any regressions. Are there any other plugins whose tests should be run against the change?

jglick · 2019-12-05T20:00:57Z

parameters of type Map were not supported in some cases

Because Map is not a legal databinding type. The only compound types supported are arrays or Lists whose element type is assignable to Describable.

jglick · 2019-12-05T20:04:22Z

The basic description is in

structs-plugin/plugin/src/main/java/org/jenkinsci/plugins/structs/describable/DescribableModel.java

Lines 278 to 281 in 696fe7b

    
                * The arguments may be primitives (as wrappers) or {@link String}s if that is their declared type. 
        
                * {@link Character}s, {@link Enum}s, and {@link URL}s may be represented by {@link String}s. 
        
                * Other object types may be passed in “raw” as well, but JSON-like structures are encouraged instead. 
        
                * Specifically a {@link List} may be used to represent any list- or array-valued argument.

though this likely needs to be expanded and clarified some.

dwnusbaum · 2019-12-05T20:06:36Z

Because Map is not a legal databinding type. The only compound types supported are arrays or Lists whose element type is assignable to Describable.

@jglick Ok, well with this PR in its current state, they are supported (at least as constructor parameters). I guess this is related to the old fallback that throws an UnsupportedOperationException no longer being reachable as you noted in #52 (comment).

jetersen · 2019-12-15T17:23:17Z

👋 This is still blocking would be great to have a release 🙏

oleg-nenashev

I think it is good to go. Thanks a lot @daspilker !

oleg-nenashev · 2020-01-04T22:34:11Z

Apparently I am listed as a plugin maintainer. I would be interested to cut the release if @jglick and @dwnusbaum are satisfied by the current state

jglick · 2020-01-04T22:40:49Z

I would leave it to @dwnusbaum.

dwnusbaum

@oleg-nenashev My understanding of this PR is that in addition to fixing the things described in JENKINS-26535 that we want to fix, it allows some other things that we do not want to allow. For example, in master, DescribableModel does not support this class:

public class MapParameter {
    @DataBoundConstructor
    public MapParameter(Map<String, String> data) { }
}

This test (copy it into DescribableModelTest if you want to try it yourself (make sure that MapParameter is marked as static if it is an inner class)), which passes against master, shows what happens if you try to use DescribableModel with MapParameter today:

@Test
public void mapParameter() throws Exception {
    schema(MapParameter.class, "MapParameter(data: java.lang.UnsupportedOperationException: do not know how to categorize attributes of type java.util.Map<java.lang.String, java.lang.String>)");
}

That test no longer passes when run against this PR, because now, DescribableModel supports Map parameters. Here is the passing version of the test when using this PR:

@Test
public void mapParameter() throws Exception {
    schema(MapParameter.class, "MapParameter(data: Map{})");
}

I think this change in behavior would be the same for any class with type parameters, not just Map.

@jglick's comment here leads me to believe that this is a bad thing, but I do not totally understand the implications. I don't think it would cause any regressions, but I'm not really sure how it would affect existing use cases of DescribableModel.

As @daspilker noted here, one seemingly reasonable way to disallow these types breaks an existing test (DescribableModelTest#recursion), so it is unclear what should be done.

To recap, I think we need answers to these questions before we can merge this PR:

Does the fact that the MapParameter class is now supported by DescribableModel represent a problem with this PR that needs to be fixed before it is merged?
Should DescribableModelTest#recursion pass as-is, or does master have a bug and Recursion should need to implement Describable for the test to pass?
- If the latter, what are the implications of fixing the code so that that test fails? Does any existing code rely on that behavior?

darxriggs · 2020-01-27T22:21:32Z

It looks like this pull request is stuck. I therefore like to ask who could follow up on it?

Now all traits consistently have a @symbol annotation. This improves configuration with JobDSL. Otherwise there is a clash e.g. with the github-branch-source-plugin.

timja · 2020-04-25T07:25:32Z

Hi @dwnusbaum, just taking a bit of a look at this,

The test case you suggested doesn't appear to be passing anymore on master,

java.lang.ArrayIndexOutOfBoundsException: 1
	at org.jenkinsci.plugins.structs.describable.DescribableModel.<init>(DescribableModel.java:160)
	at org.jenkinsci.plugins.structs.describable.DescribableModelTest.schema(DescribableModelTest.java:925)
	at org.jenkinsci.plugins.structs.describable.DescribableModelTest.mapParameter(DescribableModelTest.java:81)

Is there any chance you could update it, I would be interested in taking a look here, but not familiar with the code at all,

thanks!

dwnusbaum · 2020-04-27T14:30:49Z

@timja, sorry, the problem is just that public class MapParameter should be public static class MapParameter when you copy it into DescribableModelTest. I will update my earlier comment. FWIW, for the questions I asked in #52 (review), my answers would be:

Yes, supporting types such as MapParameter is a problem. I think the fix is to do as @daspilker describes here.
DescribableModelTest#recursion should not pass as-is in master. Recursion should need to implement Describable for the test to pass.
- The implications of making DescribableModel more restrictive for HeterogeneousObjectType and HomogenousObjectType are unclear. I would run the PCT against as many plugins as possible with the proposed change to see if any existing types are affected so they can be fixed in advance. (EDIT: I randomly noticed RateLimitBranchProperty$Throttle, which looks like it would be affected by implementing a restriction like that and would need to be modified)

jglick · 2020-04-27T17:32:53Z

I cannot recall now all the reasons why I permitted non-Describable types with @DataBoundConstructor. I do not think Recursion specifically was intended to be testing that loophole, though. The one corner case I can think of is ParameterValue, which has non-Describable subtypes with @DataBoundConstructor on them, critical for example for the Pipeline build step. See hacks like parameterValueClass. Unless and until the core patch suggested in JENKINS-26093 is in the baseline, you need to retain this special case.

zbynek · 2021-03-04T22:06:51Z

@jglick

Because Map is not a legal databinding type.

Is there a reason why List<String> is legal and Map<String, String> is not? What would it take to make Map legal as well?

As you can see in the step docs there are multiple steps that take Map as an argument. I was hoping the description of Map and List could be similar in the docs, but the way structs describes Lists and Maps is different (ArrrayType vs ErrorType).

jglick · 2021-03-04T22:13:12Z

Is there a reason why List<String> is legal and Map<String, String> is not?

Neither are legal. Besides atomic-ish types, only List<SomeKindOfDescribable> is legal.

jglick · 2021-03-04T22:14:26Z

there are multiple steps that take Map as an argument

And these plugins are implemented incorrectly.

added support for parameterized parameter types

68d5fb2

[JENKINS-26535]

jglick requested review from jglick, abayer and dwnusbaum August 19, 2019 19:22

jglick reviewed Aug 19, 2019

View reviewed changes

added more test cases for JENKINS-26535

4c5d515

jetersen previously approved these changes Aug 29, 2019

View reviewed changes

jetersen reviewed Sep 6, 2019

View reviewed changes

plugin/src/test/java/org/jenkinsci/plugins/structs/describable/DescribableModelTest.java Show resolved Hide resolved

Update plugin/src/test/java/org/jenkinsci/plugins/structs/describable…

7a7a5b6

…/DescribableModelTest.java Co-Authored-By: Joseph Petersen <josephp90@gmail.com>

jetersen mentioned this pull request Nov 13, 2019

traits stopped working in 2.6.0 for job-dsl jenkinsci/bitbucket-branch-source-plugin#258

Closed

car-roll reviewed Nov 27, 2019

View reviewed changes

dwnusbaum reviewed Dec 2, 2019

View reviewed changes

jetersen mentioned this pull request Dec 15, 2019

[JENKINS-45504] - Add @Symbol annotations to traits jenkinsci/bitbucket-branch-source-plugin#257

Merged

oleg-nenashev approved these changes Jan 4, 2020

View reviewed changes

oleg-nenashev requested review from car-roll, dwnusbaum and jglick January 4, 2020 22:32

dwnusbaum requested changes Jan 6, 2020

View reviewed changes

car-roll mentioned this pull request Mar 4, 2021

Add support for map and parametric types #80

Closed

batmat dismissed jetersen’s stale review via 7a7a5b6 January 27, 2022 21:13

dwnusbaum mentioned this pull request Sep 26, 2022

Add groovy SimpleTemplateEngine utility step jenkinsci/pipeline-utility-steps-plugin#162

Closed

4 tasks

daspilker closed this by deleting the head repository Nov 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-26535] added support for parameterized parameter types #52

[JENKINS-26535] added support for parameterized parameter types #52

daspilker commented Aug 19, 2019

jglick Aug 19, 2019

jglick Aug 19, 2019

jglick Aug 19, 2019

daspilker Aug 19, 2019

daspilker commented Aug 20, 2019

baymac commented Sep 6, 2019

jetersen commented Oct 17, 2019 •

edited

jetersen commented Nov 13, 2019

jetersen commented Nov 26, 2019

jglick commented Nov 26, 2019

jetersen commented Nov 26, 2019

car-roll left a comment •

edited

dwnusbaum left a comment

jglick commented Dec 5, 2019

jglick commented Dec 5, 2019

dwnusbaum commented Dec 5, 2019 •

edited by jglick

jetersen commented Dec 15, 2019

oleg-nenashev left a comment

oleg-nenashev commented Jan 4, 2020

jglick commented Jan 4, 2020

dwnusbaum left a comment •

edited

darxriggs commented Jan 27, 2020

timja commented Apr 25, 2020

dwnusbaum commented Apr 27, 2020 •

edited

jglick commented Apr 27, 2020

zbynek commented Mar 4, 2021

jglick commented Mar 4, 2021

jglick commented Mar 4, 2021

[JENKINS-26535] added support for parameterized parameter types #52

[JENKINS-26535] added support for parameterized parameter types #52

Conversation

daspilker commented Aug 19, 2019

jglick Aug 19, 2019

Choose a reason for hiding this comment

jglick Aug 19, 2019

Choose a reason for hiding this comment

jglick Aug 19, 2019

Choose a reason for hiding this comment

daspilker Aug 19, 2019

Choose a reason for hiding this comment

daspilker commented Aug 20, 2019

baymac commented Sep 6, 2019

jetersen commented Oct 17, 2019 • edited

jetersen commented Nov 13, 2019

jetersen commented Nov 26, 2019

jglick commented Nov 26, 2019

jetersen commented Nov 26, 2019

car-roll left a comment • edited

Choose a reason for hiding this comment

dwnusbaum left a comment

Choose a reason for hiding this comment

jglick commented Dec 5, 2019

jglick commented Dec 5, 2019

dwnusbaum commented Dec 5, 2019 • edited by jglick

jetersen commented Dec 15, 2019

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Jan 4, 2020

jglick commented Jan 4, 2020

dwnusbaum left a comment • edited

Choose a reason for hiding this comment

darxriggs commented Jan 27, 2020

timja commented Apr 25, 2020

dwnusbaum commented Apr 27, 2020 • edited

jglick commented Apr 27, 2020

zbynek commented Mar 4, 2021

jglick commented Mar 4, 2021

jglick commented Mar 4, 2021

jetersen commented Oct 17, 2019 •

edited

car-roll left a comment •

edited

dwnusbaum commented Dec 5, 2019 •

edited by jglick

dwnusbaum left a comment •

edited

dwnusbaum commented Apr 27, 2020 •

edited