fix the last inconsistencies in generics

[twogee]

trafficstars

... use collections, not arrays

[jaikiran]

Overall, this looks fine to me. I just need some inputs from the rest of the team on one specific change, for which I have raised a dev list thread https://www.mail-archive.com/[email protected]/msg46002.html

[twogee]

The new method is taken from AbstractOSGiResolver where a corresponding method is made public. All resolvers implement only the new method and the fallback for the deprecated method is moved to AbstractResolver. The new method is crucial for the SearchEngine to work properly; leave it out and you may as well leave out everything.

[twogee]

The methods in AbstractResolver must be defined the other way around: the default implementation of the method with new signature should call the method with the old signature. May I amend the commit?

[twogee]

The same applies to ChainResolver (which should check resolver type as suggested by @jaikiran); actually, all custom resolvers that do not extend AbstractResolver could be wrapped in the chain resolver as a workaround.

[twogee]

Is this sufficient, or should I add an extra check in ChainResolver

            if (resolver instanceof AbstractResolver || hasGenericMethod(resolver)) {
...
            }
...

    private boolean hasGenericMethod(DependencyResolver resolver) {
        try {
            return resolver.getClass().getDeclaredMethod("listTokenValues",
                    Set.class, Map.class).getReturnType().getSimpleName().equals("Set");
        } catch (NoSuchMethodException e) {
            return false;
        }
    }

[jaikiran]

Hi Gintas,

To be clear - what I meant/proposed in the dev list was:

• It's fine to have all the changes related to introducing that new method with Java generics on the implementation classes, including the AbstractResolver. No objections to it.

• However, I think we shouldn't yet introduce it on the DependencyResolver interface in this release.

• Finally, as you note in the latest comment, we would need to do a type check in our (internal) classes where we use the DependencyResolver interface's listTokenValues method to see if it's of type AbstractResolver and if it is, then invoke the new generic method. Else invoke the older method. We don't need to do a reflection check to see whether the resolver has the generic listTokenValues method, since the other existing method should just function as normal. So ultimately something like:

    if (resolver instanceof AbstractResolver) {
       Set<Map<String, String>> tokenVals = resolver.listTokenValues(...) // the new generic method
    } else {
       // call the method that's exposed via the DependencyResolver interface - the one which doesn't use generics
      Map<String, String>[] tokenVals = resolver.listTokenValues(...);
    }

In short, in this release, don't expose this as a new API on the interface but expose it on the AbstractResolver with an internal implementation that will trickle down to all external resolvers that extend the AbstractResolver.

Again, I'm being a bit extra cautious on this specific interface unlike some other interface/class changes that we have been doing since this one acts as the central well known contract to the developers who have extended Ivy. Plus the fact that we do have a way to do all the changes being proposed in this PR without having to force the implementations of the DependencyResolver to implement that new method in this specific release.

[twogee]

Thanks for clarifying.

My point was that the use of the method is limited to two classes: SearchEngine and ChainResolver (because it is essentially a proxy).

A quick search seems to indicate that most (if not all) custom resolver implementations out there extend AbstractResolver, and thus providing a default implementation of the method with the new signature as a wrapper in the abstract class will mitigate the problems due to the lack of it.

Therefore I would like to insist on adding the method to the interface as well as changing the signature in all resolvers that ship with Ivy. Any public method in the abstract class should be declared in the interface, and going halfway does not make much sense.

[hibnico]

@twogee, you have looked into the open sources softwares that use Ivy, but Ivy is under the ASL, not the GPL, it might be used in some closed, commercial products :) So we cannot know for sure that we won't break softwares by changing that part of the API.

The hierarchy of the classes of the DependencyResolver is not of the best design, it would have been great to have more composition than inheritance. But that's we have. So unless we want to break things, rewrite things and make an Ivy3, I think we should stick with it.

To move forward, I suggest that this PR doesn't break the API at all. And if you still think DependencyResolver deserve a probably-safe API break, you're welcomed to discuss it on ant-dev so we can get a consensus.

[twogee]

@nlalevee, we already broke a few things by going Java 7 (and gained native symlinks 😉). For more examples, look at commons-lang3 breaking off commons-text and introducing escapeHtml3 and escapeHtml4 instead of escapeHtml -- all without changing a major version. Pointing to some hypothetical closed code is a poor excuse -- as if commercial vendors (or their customers) have no QA and expect the dependencies to be drop-ins every time. I will present the argument on ant-dev in a more coherent form later.