HeteroDescribableConfigurator fails on duplicate keys.

[dgarzon]

Please make sure to provide following information in your issue description

• Jenkins version (v. 2.138.3)
• Plugin version (v. 1.3)
• OS (macOS Mojave)

Currently, HeteroDescribableConfigurator is prone to throwing IllegalStateException and ConfiguratorException when 2 or more Descriptor have the same name and are not annotated with the @Symbol. This can be seen when generating the Schema and when Configuring from a YAML.

For example:

Given the following two implementations for BranchDiscoveryTrait that lack the @Symbol annotation:

1. jenkins.plugins.git.traits.BranchDiscoveryTrait
2. org.jenkinsci.plugins.github_branch_source.BranchDiscoveryTrait

Then, when generating the Schema we get an IllegalStateException due to a duplicate key in the getImplementors() logic. This is because DescribableAttribute.getSymbols(descriptor, api, target), returns branchDiscoveryTrait for both implementations.

On the other hand, when we configure based on the provided YAML configuration, we get a ConfiguratorException because we are being too optimistic in the sense that we assume that a symbol can only be mapped to one Descriptor implementation. Instead, we should not make such assumption and do a brute-force implementation that will try to instantiate the configured object for N number of Descriptor.

[dgarzon]

A solution for the schema generation could be in HeteroDescribableConfigurator:

    public Map<String, Set<Class>> getImplementors() {
        final Class<?> api = getImplementedAPI();
        final List<Descriptor> descriptors = Jenkins.getInstance().getDescriptorList(target);
        return descriptors.stream()
                .collect(Collectors.groupingBy(
                        d -> DescribableAttribute.getSymbols(d, api, target).get(0),
                        Collectors.mapping(d -> d.getKlass().toJavaClass(), Collectors.toSet())));
    }

And then the schema.jelly:

<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler">
  "${it.target.name}": {
    "type": "object",
    "oneOf" : [
  <j:forEach items="${it.implementors.entrySet()}" var="impl" varStatus="st">
    <j:forEach items="${impl.value}" var="nested" varStatus="nt">
        {
          "properties" : {
            "${impl.key}" : { "type" : "#/definitions/${nested}" }
          }
        }
        <j:if test="${!nt.last}">,</j:if>
    </j:forEach>
    <j:if test="${!st.last}">,</j:if>
  </j:forEach>
    ]
  }
</j:jelly>

Which in turn results in a schema:

{
    properties: {
        branchDiscoveryTrait: {
            type: "#/definitions/class jenkins.plugins.git.traits.BranchDiscoveryTrait"
        }
    }
},
{
    properties: {
        branchDiscoveryTrait: {
            type: "#/definitions/class org.jenkinsci.plugins.github_branch_source.BranchDiscoveryTrait"
        }
    }
},

[dgarzon]

As for how to avoid the ConfiguratorException I am not familiar enough with the configure code to make any proposals. Does any of you have any idea what needs to change in order to fix this bug? @ndeloof @ewelinawilkosz

[dgarzon]

Although this code is very ugly, and I still see errors in the UI, it got my CasC working and no exceptions logged into the console:

        return Either.sequenceRight(Stream.ofAll(candidates)
                .filter(d -> DescribableAttribute.getSymbols(d, api, target).get(0).equalsIgnoreCase(shortname))
                .map(d -> d.getKlass().toJavaClass())
                .map(context::lookup)
                .filter(Objects::nonNull)
                .map(c -> {
                    try {
                        return Either.<T, ConfiguratorException>left((T) c.configure(finalSubconfig, context));
                    } catch (ConfiguratorException e) {
                        return Either.<T, ConfiguratorException>right(e);
                    }
                })
                .toList()).swap().getOrElseThrow(e -> e.find(Objects::nonNull).getOrElseThrow(() -> new IllegalStateException("No configurator implementation to manage " + shortname)));

[ndeloof]

Not sure I get your point, git implementation has symbol:

https://github.com/jenkinsci/git-plugin/blob/master/src/main/java/jenkins/plugins/git/traits/BranchDiscoveryTrait.java#L83

[ndeloof]

Generally speaking, imho Symbol is the solution to be adopted to such a conflict, and a minimal change to plugins so we can expect them to be adopted by maintainer without long debates in a pull-request.

Using a "try then invoke other" logic can have terrible side effects if wrong candidate get partially configured by some common attributes before configuration fails.

[dgarzon]

@ndeloof I agree with your statement about possibility of things going wrong, but given these are descriptors that implement an interface, something like this may be needed. Developers may choose to name their classes the same as other implementations and even use the same symbol. An example is: https://github.com/jenkinsci/github-branch-source-plugin/blob/master/src/main/java/org/jenkinsci/plugins/github_branch_source/BranchDiscoveryTrait.java. Here we see that one implementation is annotated and the other is not, and our logic to figure out the key will yield the same key for both.

[ndeloof]

by definition of Symbol plugin maintainer has to carefully select a name that won't collide with another plugin. Yes, there's a risk this happens.
A proper fix is to be proactive in plugin maintenance and get adequate symbols used in various places. "coordination".

From CasC point of view we should detect such collision and log a warning ("please report to plugin maintainer") and then only support FQCN as (not such a) symbol.

Your proposed approach assume we can safely "try to configure" and assume this had no actual impact, which is wrong in many places, and is way too risky.

[dgarzon]

@ndeloof I get your point now, thank you for the explanation! For the plugin in question (github-branch-source) I have made a PR to add @Symbol annotation with unique identifiers so that we don't get collisions in CasC, here: jenkinsci/github-branch-source-plugin#193

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[jetersen]

This was fixed in #674