Merge pull request #41 from ckipp01/purl

fix: ensure invalid PURLs are not included

Author: ckipp01

build.sc

@@ -62,9 +62,13 @@ object plugin extends Common with BuildInfo {
 
   override def moduleDeps = Seq(domain)
   override def compileIvyDeps = super.compileIvyDeps() ++ Agg(
-    ivy"com.lihaoyi::mill-scalalib:$millVersion",
+    ivy"com.lihaoyi::mill-scalalib:$millVersion"
+  )
+
+  override def ivyDeps = super.ivyDeps() ++ Agg(
     ivy"com.lihaoyi::upickle:2.0.0",
-    ivy"com.lihaoyi::requests:0.7.1"
+    ivy"com.lihaoyi::requests:0.7.1",
+    ivy"com.github.package-url:packageurl-java:1.4.1"
   )
 
   override def buildInfoMembers = Map(
@@ -100,6 +104,9 @@ object itest extends MillIntegrationTestModule {
         ),
         PathRef(testBase / "range") -> Seq(
           TestInvocation.Targets(Seq("verify"), noServer = true)
+        ),
+        PathRef(testBase / "reconciledRange") -> Seq(
+          TestInvocation.Targets(Seq("verify"), noServer = true)
         )
       )
     }

itest/src/reconciledRange/build.sc

@@ -0,0 +1,67 @@
+import mill._, scalalib._
+import $exec.plugins
+import io.kipp.mill.github.dependency.graph.Graph
+import mill.eval.Evaluator
+import $ivy.`org.scalameta::munit:0.7.29`
+import munit.Assertions._
+
+object minimalDep extends ScalaModule {
+  def scalaVersion = "2.13.8"
+
+  def ivyDeps = Agg(
+    ivy"com.fasterxml.jackson.core:jackson-core:2.13.3"
+  )
+}
+
+object minimal extends ScalaModule {
+  def moduleDeps = Seq(`minimalDep`)
+
+  def scalaVersion = "2.13.8"
+
+  def ivyDeps = Agg(
+    ivy"org.jongo:jongo:1.5.0"
+  )
+}
+
+def verify(ev: Evaluator) = T.command {
+  val manifestMapping = Graph.generate(ev)()
+  assert(manifestMapping.size == 2)
+
+  // OK, this is a super weird one. Jongo here has a range dep which is fine, and when
+  // it's resolved by itself, it correctly gets 2.12.3 like you can see below:
+  //
+  // ❯ cs resolve org.jongo:jongo:1.5.0
+  // com.fasterxml.jackson.core:jackson-annotations:2.12.3:default
+  // com.fasterxml.jackson.core:jackson-core:2.12.3:default
+  // com.fasterxml.jackson.core:jackson-databind:2.12.3:default
+  // de.undercouch:bson4jackson:2.12.0:default
+  // org.jongo:jongo:1.5.0:default
+  //
+  // The issue enteres with a setup like the above. For some reason coursier
+  // will actually retain the range as the reconciledVersion in the
+  // DependencyTree when it should be the actual reconciled versions.
+  //
+  // dep version: [2.7.0,2.12.3]
+  // retained version: [2.7.0,2.12.3]
+  // reconciled version: [2.7.0,2.12.3]
+  //
+  // Since I believe this to be a bug in coursier for now we'll just throw them
+  // out to ensure we're not creating invalid PURLs.
+  val expected = Set(
+    "org.scala-lang:scala-library:2.13.8",
+    "com.fasterxml.jackson.core:jackson-core:2.13.3",
+    // NOTICE that com.fasterxml.jackson.core:jackson-core:[2.7.0,2.12.3] is not here
+    "com.fasterxml.jackson.core:jackson-core:2.12.3",
+    "com.fasterxml.jackson.core:jackson-databind:2.12.3",
+    "com.fasterxml.jackson.core:jackson-annotations:2.12.3",
+    "org.jongo:jongo:1.5.0",
+    "de.undercouch:bson4jackson:2.12.0"
+  )
+
+  val results = manifestMapping.foldLeft[Set[String]](Set.empty) {
+    case (set, mapping) =>
+      set ++ mapping._2.resolved.keySet
+  }
+
+  assertEquals(results, expected)
+}

plugin/src/io/kipp/mill/github/dependency/graph/ModuleTrees.scala

@@ -1,10 +1,12 @@
 package io.kipp.mill.github.dependency.graph
 
+import com.github.packageurl.PackageURLBuilder
 import coursier.graph.DependencyTree
 import io.kipp.github.dependency.graph.domain._
 import mill.scalalib.JavaModule
 
 import scala.collection.mutable
+import scala.util.Try
 
 /** Represents a project modules an the dependency trees that belong to it.
   *
@@ -25,7 +27,9 @@ final case class ModuleTrees(
     * @return Mapping of the name of the dependency and the DependencyNode that
     * corresponds to it. The format of the name is org:module:version.
     */
-  def toFlattenedNodes(): Map[String, DependencyNode] = {
+  def toFlattenedNodes()(implicit
+      ctx: mill.api.Ctx
+  ): Map[String, DependencyNode] = {
 
     val allDependencies = mutable.Map[String, DependencyNode]()
 
@@ -37,16 +41,34 @@ final case class ModuleTrees(
 
       def putTogether: DependencyNode = {
         // TODO consider classifiers
-        val packageUrl =
-          s"pkg:maven/${dep.module.organization.value}/${dep.module.name.value}@${reconciledVersion}"
+
+        val purl = Try(
+          PackageURLBuilder
+            .aPackageURL()
+            .withType("maven")
+            .withNamespace(dep.module.organization.value)
+            .withName(dep.module.name.value)
+            .withVersion(reconciledVersion)
+            .build()
+        ).fold(
+          e => {
+            ctx.log.error(
+              s"PURL can't be created from: ${dep.module.orgName}:${reconciledVersion}"
+            )
+            ctx.log.error(e.getMessage())
+            None
+          },
+          validPurl => Some(validPurl.toString())
+        )
+
         val relationShip: DependencyRelationship =
           if (root) DependencyRelationship.direct
           else DependencyRelationship.indirect
         val dependencies = tree.children.map { child =>
           s"${child.dependency.module.orgName}:${child.reconciledVersion}"
         }
         DependencyNode(
-          Some(packageUrl),
+          purl,
           // TODO we can check if original == reconciled here and add metadata that it is a reconciled version
           Map.empty,
           Some(relationShip),
@@ -60,15 +82,36 @@ final case class ModuleTrees(
 
       allDependencies.get(name) match {
         // If the node is found and the relationship is correct just do nothing
-        case Some(node) if verifyRelationship(node) => ()
+        case Some(node) if verifyRelationship(node) =>
+          ctx.log.debug(
+            s"Already seen ${name} with this relationship in this manifest, so skipping..."
+          )
         // If the node is found and the relationship is incorrect, but it's a
         // root node, then make sure to mark it as direct
         case Some(node) if root =>
+          ctx.log.debug(
+            s"Already seen ${name} but we're at the root level so marking as direct..."
+          )
           val updated =
             node.copy(relationship = Some(DependencyRelationship.direct))
           allDependencies += ((name, updated))
-        // Should never really happen, but it it does do nothing
-        case Some(_) => ()
+        case Some(_) =>
+          ctx.log.debug(
+            s"Found ${name}, but it's already marked as direct so skipping..."
+          )
+        // Not a very elegant check, but we don't want to include a range in
+        // here. These shouldn't still be a range at this point, but it is for
+        // whatever reason. For now ignore it. This should be incredibly rare
+        // and I believe a bug in coursier.
+        case None if reconciledVersion.contains(",") =>
+          ctx.log.error(
+            s"""Found what I think is a range version that shouldn't be here...
+                |
+                |${dep.module.organization.value}:${dep.module.name.value}:${reconciledVersion}
+                |
+                |If you see this, report it. Skipping...
+                |""".stripMargin
+          )
         // Unseen dependency, create a node for it
         case None =>
           val node = putTogether
@@ -82,7 +125,7 @@ final case class ModuleTrees(
     allDependencies.toMap
   }
 
-  def toManifest() = {
+  def toManifest()(implicit ctx: mill.api.Ctx) = {
     // NOTE: That this may seem odd when reading the spec that we have a
     // manifest per module basically, but we did check with the GitHub team and
     // they verified the manifests that we showed them.