Defending against duplicate plugin entries

[jvanzyl]

If you have something like the following where you accidentally list a plugin twice:

  dependencies:
    - "mvn://ca.vanzyl.concord.plugins:concord-k8s-plugin:0.0.1-SNAPSHOT"
    - "mvn://com.walmartlabs.concord.plugins:terraform-task:1.16.0"
    - "mvn://com.walmartlabs.concord.plugins:git:1.16.0"
    - "mvn://com.walmartlabs.concord.plugins:terraform-task:1.19.1"

We might want to warn or probably just fail fast and now allow it. Not sure how the plugin retriever or plugin classloader works but if can allow the random ordering of the plugins and how they are loaded it might lead to unexpected results.

[mosabua]

Might want to figure out how this works for import and other composed configs as well and ensure some sort of sanity .. maybe warning and latest version wins? Or fail fast to be on the safe side..

[ibodrov]

All mvn dependencies are resolved in a single org.eclipse.aether.collection.CollectRequest. The resulting list is sorted to ensure consistent loading order. E.g.

configuration:
  debug: true
  dependencies:
    - "mvn://com.walmartlabs.concord.plugins:git:1.19.0"
    - "mvn://com.walmartlabs.concord.plugins:git:1.19.1"

flows:
  default:
    - log: "Hello!"

when running locally produces

Effective dependencies:
	file:/home/ibodrov/.m2/repository/com/fasterxml/jackson/core/jackson-annotations/2.9.9/jackson-annotations-2.9.9.jar
	file:/home/ibodrov/.m2/repository/com/fasterxml/jackson/core/jackson-core/2.9.9/jackson-core-2.9.9.jar
	file:/home/ibodrov/.m2/repository/com/fasterxml/jackson/core/jackson-databind/2.9.9/jackson-databind-2.9.9.jar
	file:/home/ibodrov/.m2/repository/com/google/code/gson/gson/2.8.0/gson-2.8.0.jar
	file:/home/ibodrov/.m2/repository/com/googlecode/javaewah/JavaEWAH/1.1.6/JavaEWAH-1.1.6.jar
	file:/home/ibodrov/.m2/repository/com/jcraft/jsch/0.1.55/jsch-0.1.55.jar
	file:/home/ibodrov/.m2/repository/com/jcraft/jzlib/1.1.1/jzlib-1.1.1.jar
	file:/home/ibodrov/.m2/repository/com/squareup/okhttp/okhttp/2.7.5/okhttp-2.7.5.jar
	file:/home/ibodrov/.m2/repository/com/squareup/okio/okio/1.15.0/okio-1.15.0.jar
	file:/home/ibodrov/.m2/repository/com/walmartlabs/concord/plugins/basic/concord-tasks/1.35.2-SNAPSHOT/concord-tasks-1.35.2-SNAPSHOT.jar
	file:/home/ibodrov/.m2/repository/com/walmartlabs/concord/plugins/basic/http-tasks/1.35.2-SNAPSHOT/http-tasks-1.35.2-SNAPSHOT.jar
	file:/home/ibodrov/.m2/repository/com/walmartlabs/concord/plugins/basic/slack-tasks/1.35.2-SNAPSHOT/slack-tasks-1.35.2-SNAPSHOT.jar
	file:/home/ibodrov/.m2/repository/com/walmartlabs/concord/plugins/git/1.19.1/git-1.19.1.jar
	file:/home/ibodrov/.m2/repository/commons-codec/commons-codec/1.11/commons-codec-1.11.jar
	file:/home/ibodrov/.m2/repository/commons-logging/commons-logging/1.2/commons-logging-1.2.jar
	file:/home/ibodrov/.m2/repository/org/apache/commons/commons-compress/1.19/commons-compress-1.19.jar
	file:/home/ibodrov/.m2/repository/org/apache/httpcomponents/httpclient/4.5.9/httpclient-4.5.9.jar
	file:/home/ibodrov/.m2/repository/org/apache/httpcomponents/httpcore/4.4.10/httpcore-4.4.10.jar
	file:/home/ibodrov/.m2/repository/org/apache/httpcomponents/httpmime/4.5.10/httpmime-4.5.10.jar
	file:/home/ibodrov/.m2/repository/org/eclipse/jgit/org.eclipse.jgit/5.2.0.201812061821-r/org.eclipse.jgit-5.2.0.201812061821-r.jar
	file:/home/ibodrov/.m2/repository/org/eclipse/mylyn/github/org.eclipse.egit.github.core/5.3.0.201903130848-r/org.eclipse.egit.github.core-5.3.0.201903130848-r.jar
	file:/home/ibodrov/.m2/repository/org/jboss/spec/javax/ws/rs/jboss-jaxrs-api_2.0_spec/1.0.1.Final/jboss-jaxrs-api_2.0_spec-1.0.1.Final.jar
	file:/home/ibodrov/.m2/repository/org/slf4j/slf4j-api/1.7.2/slf4j-api-1.7.2.jar

[mosabua]

that would mean that provided semver is used that newer plugin versions automatically override older ones. At least for one digit semver... e.g.

1.2.3 before 1.2.2 before 1.1.2

Might not work for e.g. is 1.1.1.jar before 1.1.10.jar although I think it should ( symbol before digit?)

[swayamraina]

I guess latest version libs override the older version (not sure of does will this work with 1.1.1 and 1.1.10 as pointed out by @mosabua ) Have we concluded that do we need to go ahead with the same approach and fix the (1.1.1 and 1.1.10) issue or fail fast and report duplicates?

[ibodrov]

I'm failing to see where the issue is.

At the moment the list of mvn:// dependencies is handed over to Aether - including all "duplicates". And Aether produces a list of resolved dependencies excluding "duplicates" according to its internal logic.

E.g.

configuration:
  debug: true
  dependencies:
    - "mvn://com.walmartlabs.concord.plugins.basic:http-tasks:1.1.0"
    - "mvn://com.walmartlabs.concord.plugins.basic:http-tasks:1.53.1"

flows:
  default:
    - log: "Hello!"

Aether picks only the artifacts of the 1.53.1 version, which is IMO the expected behaviour. In addition to "duplicates" directly declared in dependencies you might have transitive dependencies pointing to different versions of the same artifact. Those situations are also handled by Aether.

We might want to warn users if they have duplicates within a dependencies block, but additional dependencies blocks can also come though the mechanism of imports. So, realistically, we can only warn about duplicates in a single block of dependencies and not across all loaded blocks - as it'd be fairly useless/annoying.