[JENKINS-43934] Flatten GString’s found in step arguments, even when present in nested collections #123
Conversation
…present in nested collections.
| if (b.length() > 0) { | ||
| b.append(' '); | ||
| } | ||
| b.append((String) arg); |
There was a problem hiding this comment.
Admittedly artificial, but at any rate this throws a ClassCastException without the fix.
|
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. |
| List<Object> r = new ArrayList<>(); | ||
| for (Object o : ((List<?>) v)) { | ||
| Object o2 = flattenGString(o); | ||
| mutated |= o != o2; |
| r.put(k2, o2); | ||
| } | ||
| return mutated ? r : v; | ||
| } else { |
There was a problem hiding this comment.
@jglick What about Sets, general collections?
Probably omitting those types is a bug, but this is still better than before, so I'm happy seeing it merged that way because it is still a million times better.
There was a problem hiding this comment.
I deliberately considered only Lists and Maps because these are the types that DescribableModel processes and that are generally used in Describable configuration and thus Step syntax. (Actually the interpreted Maps are more specific than that—only String-keyed maps, and only for the purpose of representing nested Describables à la $class—but easiest to just process Maps generically, and should be harmless.)
There was a problem hiding this comment.
That's reasonable enough, thanks for explaining.
Not sure if there's an explicit restriction on Set/Collection types here, but that's good enough for me.
|
Argh, npm strikes again! I need to set aside some time to try to use jenkinsci/plugin-pom#57 … |
|
@reviewbybees done Now trying to rerun tests on my laptop since the npm download is busted, sigh; see INFRA-1139. |
JENKINS-43934
Never really posed a problem before, probably because there is no existing
StepDescriptor.newInstanceoverride which would be expecting evaluatedStrings in a collection type, but the previous behavior was at least potentially wrong…and this is needed for #98 so we might as well do it anyway.DescribableModelalready hasGStringhandling, which really should never have been there, but we cannot remove that compatibly.@reviewbybees