Use SortedMap in GradleResolver, fix property tests (#377)

Co-authored-by: Oscar Boykin <oboykin@netflix.com>

Author: johnynek

src/scala/com/github/johnynek/bazel_deps/GradleLockFile.scala

@@ -3,6 +3,7 @@ package com.github.johnynek.bazel_deps
 import io.circe.Decoder.Result
 import io.circe.{Decoder, Error, HCursor, Json, KeyDecoder, Parser}
 import io.circe.generic.auto
+import scala.collection.immutable.SortedMap
 
 import scala.util.{Failure, Success, Try}
 
@@ -69,10 +70,10 @@ object GradleLockFile {
 }
 
 case class GradleLockFile(
-    annotationProcessor: Option[Map[String, GradleLockDependency]],
-    compileClasspath: Option[Map[String, GradleLockDependency]],
-    resolutionRules: Option[Map[String, GradleLockDependency]],
-    runtimeClasspath: Option[Map[String, GradleLockDependency]],
-    testCompileClasspath: Option[Map[String, GradleLockDependency]],
-    testRuntimeClasspath: Option[Map[String, GradleLockDependency]]
+    annotationProcessor: Option[SortedMap[String, GradleLockDependency]],
+    compileClasspath: Option[SortedMap[String, GradleLockDependency]],
+    resolutionRules: Option[SortedMap[String, GradleLockDependency]],
+    runtimeClasspath: Option[SortedMap[String, GradleLockDependency]],
+    testCompileClasspath: Option[SortedMap[String, GradleLockDependency]],
+    testRuntimeClasspath: Option[SortedMap[String, GradleLockDependency]]
 )

src/scala/com/github/johnynek/bazel_deps/GradleResolver.scala

@@ -42,7 +42,7 @@ class GradleResolver(
   // we just want one, so we merge them all
   private def collapseDepTypes(
       lockFile: GradleLockFile
-  ): Try[Map[String, GradleLockDependency]] =
+  ): Try[SortedMap[String, GradleLockDependency]] =
     TryMerge.tryMergeAll(
       None,
       NonEmptyList.of(
@@ -54,7 +54,7 @@ class GradleResolver(
         lockFile.testRuntimeClasspath
       )
     )
-    .map(_.getOrElse(Map.empty))
+    .map(_.getOrElse(SortedMap.empty[String, GradleLockDependency]))
 
   private val unit = Validated.valid(())
 
@@ -238,7 +238,7 @@ private def cleanUpMap(
     }
 
   def buildGraphFromDepMap(
-    depMap: Map[String, GradleLockDependency]
+    depMap: SortedMap[String, GradleLockDependency]
   ): Try[Graph[MavenCoordinate, Unit]] =
     assertConnectedDependencyMap(depMap)
       .map(_ => cleanUpMap(depMap))
@@ -274,7 +274,6 @@ private def cleanUpMap(
 
         depMap
           .toList
-          .sortBy(_._1)
           .traverse[V, (MavenCoordinate, List[MavenCoordinate])] { case (n, deps) =>
             toCoord(n)
               .product(

src/scala/com/github/johnynek/bazel_deps/TryMerge.scala

@@ -3,6 +3,7 @@ package com.github.johnynek.bazel_deps
 import cats.{Foldable, Reducible}
 import cats.data.NonEmptyList
 import scala.util.{Failure, Success, Try}
+import scala.collection.immutable.SortedMap
 
 import cats.syntax.all._
 
@@ -40,33 +41,29 @@ object TryMerge {
       }
     }
 
-  implicit def tryStringMapMerge[T: TryMerge]: TryMerge[Map[String, T]] =
-    new TryMerge[Map[String, T]] {
-      private val rev = reverse
+  implicit def tryStringMapMerge[T: TryMerge]: TryMerge[SortedMap[String, T]] =
+    new TryMerge[SortedMap[String, T]] {
       def tryMerge(
         debugName: Option[String],
-          left: Map[String, T],
-          right: Map[String, T]
-      ): Try[Map[String, T]] = {
-        if (right.size > left.size) rev.tryMerge(debugName, right, left)
-        else {
-          // right <= left in size
-          val overlaps = right.exists { case (k, _) => left.contains(k) }
-          if (!overlaps) Success(left ++ right)
-          else {
-            right.toList.sortBy(_._1)
-              .foldM(left) { case (acc, (k, rv)) =>
-                acc.get(k) match {
-                  case Some(lv) =>
-                    TryMerge.tryMerge(Some(debugName.fold(k) {p => s"$p:$k"}), lv, rv)
-                      .map(acc.updated(k, _))
-                  case None =>
-                    Success(acc.updated(k, rv))
-                }  
-              }
+          left: SortedMap[String, T],
+          right: SortedMap[String, T]
+      ): Try[SortedMap[String, T]] = {
+        val tmt = implicitly[TryMerge[T]]
+        val (big, small, tm) =
+          if (right.size > left.size) (right, left, tmt.reverse)
+          else (left, right, tmt)
+
+        small.toStream
+          .foldM(big) { case (acc, (k, rv)) =>
+            acc.get(k) match {
+              case Some(lv) =>
+                val nextDebug = Some(debugName.fold(k) {p => s"$p:$k"})
+                tm.tryMerge(nextDebug, lv, rv).map(acc.updated(k, _))
+              case None =>
+                Success(acc.updated(k, rv))
+            }  
           }
       }
     }
-  }
 }
 

test/scala/com/github/johnynek/bazel_deps/BUILD.bazel

@@ -203,4 +203,16 @@ scala_test(
   ]
 )
 
+scala_test(
+  name = "trymergetest",
+  srcs = ["TryMergeTest.scala"],
+  deps = [
+    "//src/scala/com/github/johnynek/bazel_deps:trymerge",
+    "//3rdparty/jvm/org/scalacheck",
+    "//3rdparty/jvm/org/scalatest:scalatest_propspec",
+    "//3rdparty/jvm/org/scalatestplus:scalacheck_1_17",
+    "//3rdparty/jvm/org/typelevel:cats_core",
+  ]
+)
+
 test_suite(name = "all_tests")

test/scala/com/github/johnynek/bazel_deps/GradleResolverTest.scala

@@ -4,6 +4,7 @@ import io.circe.jawn.JawnParser
 import java.nio.file.Paths
 import org.scalatest.funsuite.AnyFunSuite
 import scala.util.{Success, Failure}
+import scala.collection.immutable.SortedMap
 
 import Decoders._
 
@@ -18,8 +19,8 @@ class GradleResolverTest extends AnyFunSuite {
       case Left(error) => sys.error(s"Failed to decode $str: ${error}")
     }
 
-  def deps(str: String): Map[String, GradleLockDependency] =
-    (new JawnParser).decode[Map[String, GradleLockDependency]](str) match {
+  def deps(str: String): SortedMap[String, GradleLockDependency] =
+    (new JawnParser).decode[SortedMap[String, GradleLockDependency]](str) match {
       case Right(t) => t
       case Left(error) => sys.error(s"Failed to decode $str: ${error}")
     }

test/scala/com/github/johnynek/bazel_deps/GraphPropTest.scala

@@ -1,10 +1,10 @@
 package com.github.johnynek.bazel_deps
 
 import org.scalatest.propspec.AnyPropSpec
-import org.scalacheck.Prop.forAll
+import org.scalatestplus.scalacheck.ScalaCheckPropertyChecks
 import org.scalacheck.{Gen, Arbitrary}
 
-class GraphPropTest extends AnyPropSpec {
+class GraphPropTest extends AnyPropSpec with ScalaCheckPropertyChecks {
 
   def graphGen[N, E](g: Gen[(N, Option[(N, E)])]): Gen[Graph[N, E]] =
     Gen.sized { s =>
@@ -89,21 +89,22 @@ class GraphPropTest extends AnyPropSpec {
   property("Sanity checks on generated graphs (non-dag) 4") {
     forAll(graphGen(genIntNode), nodeGen) { (g, n) =>
       val newG = g.removeNode(n)
-      (!newG.nodes(n)) && (!newG.edgeIterator.exists { case Edge(s, d, _) =>
+      assert((!newG.nodes(n)) && (!newG.edgeIterator.exists { case Edge(s, d, _) =>
         (s == n) || (d == n)
-      })
+      }))
     }
   }
 
   property("Sanity checks on generated graphs (non-dag) 5") {
     forAll(graphGen(genIntNode), nodeGen, nodeGen) { (g, s, d) =>
       val newEdge = Edge(s, d, ())
       val newG = g.addEdge(newEdge)
-      newG.nodes(s) &&
-      newG.nodes(d) &&
-      newG.hasSource(s)(newEdge) &&
-      newG.hasDestination(d)(newEdge) &&
-      newG.edgeIterator.toSet(newEdge)
+
+      assert(newG.nodes(s) &&
+        newG.nodes(d) &&
+        newG.hasSource(s)(newEdge) &&
+        newG.hasDestination(d)(newEdge) &&
+        newG.edgeIterator.toSet(newEdge))
     }
   }
 
@@ -114,8 +115,8 @@ class GraphPropTest extends AnyPropSpec {
     }) {
       case (g, Some(e)) =>
         val newG = g.removeEdge(e)
-        !(newG.edgeIterator.exists(_ == e))
-      case (g, None) => true
+        assert(!(newG.edgeIterator.exists(_ == e)))
+      case (g, None) => ()
     }
   }
 
@@ -126,9 +127,12 @@ class GraphPropTest extends AnyPropSpec {
     }) {
       case (g, Some(n)) =>
         val newG = g.removeNode(n)
-        newG.hasDestination(n).isEmpty &&
-        newG.hasSource(n).isEmpty &&
-        (!newG.nodes(n))
+
+        assert(
+          newG.hasDestination(n).isEmpty &&
+          newG.hasSource(n).isEmpty &&
+          (!newG.nodes(n))
+        )
       case (g, None) => true
     }
   }
@@ -142,7 +146,7 @@ class GraphPropTest extends AnyPropSpec {
     } yield (g, optPair)
 
     forAll(genEnds) {
-      case (g, Some((s, e))) => g.reflexiveTransitiveClosure(List(s))(e)
+      case (g, Some((s, e))) => assert(g.reflexiveTransitiveClosure(List(s))(e))
       case (g, None)         => true
     }
   }
@@ -152,6 +156,7 @@ class GraphPropTest extends AnyPropSpec {
       g <- graphGen(genIntNode)
       e <- edgeFrom(g)
     } yield (g, e)
+
     forAll(gen, nodeGen, nodeGen) { case ((g, e), n1, n2) =>
       val maybePresent = Edge(n1, n2, ())
       val maybeCheck =
@@ -164,9 +169,9 @@ class GraphPropTest extends AnyPropSpec {
             g.hasDestination(n2).contains(maybePresent))
         }
 
-      g.edgeIterator.forall(g.hasEdge) &&   
+      assert(g.edgeIterator.forall(g.hasEdge) &&   
         e.forall(g.hasEdge(_)) &&
-        maybeCheck
+        maybeCheck)
     }
   }
-}
+}

test/scala/com/github/johnynek/bazel_deps/TryMergeTest.scala

@@ -0,0 +1,87 @@
+package com.github.johnynek.bazel_deps
+
+import org.scalatest.propspec.AnyPropSpec
+import org.scalatestplus.scalacheck.ScalaCheckPropertyChecks
+import org.scalacheck.{Prop, Gen, Arbitrary}
+import cats.{Semigroup, Eq}
+import scala.util.{Success, Failure}
+import scala.collection.immutable.SortedMap
+
+import TryMerge.tryMerge
+import java.util.concurrent.Semaphore
+
+class TryMergeTest extends AnyPropSpec with ScalaCheckPropertyChecks {
+  def associative[A: TryMerge: Eq](a: A, b: A, c: A) = {
+    val right = tryMerge(None, b, c).flatMap(tryMerge(None, a, _))
+    val left = tryMerge(None, a, b).flatMap(tryMerge(None, _, c))
+
+    (left, right) match {
+      case (Success(l), Success(r)) => assert(Eq.eqv(l, r))
+      case (Failure(_), Failure(_)) => ()
+      case mismatch => fail(s"associative mismatch: $a, $b, $c => $mismatch")
+    }
+  }
+
+  implicit val tryMergeI: TryMerge[Int] =
+    new TryMerge[Int] {
+      def tryMerge(debug: Option[String], a: Int, b: Int) =
+        // take the high 16 bits of a and low 16 bits of b
+        // this is associative but not commutative
+        Success((a & 0xFFFF0000) | (b & 0x0000FFFF))
+    }
+
+  def revTry[A: TryMerge]: TryMerge[A] = implicitly[TryMerge[A]].reverse
+  
+  property("try merge is associative Int") {
+    forAll { (a: Int, b: Int, c: Int) =>
+      associative(a, b, c)
+      associative(a, b, c)(revTry, implicitly)
+    }
+  }
+
+
+  property("try merge is associative Option[Int]") {
+    forAll { (a: Option[Int], b: Option[Int], c: Option[Int]) =>
+      associative(a, b, c)
+      associative(a, b, c)(revTry, implicitly)
+    }
+  }
+
+  property("try merge is associative SortedMap[String, Int]") {
+    forAll { (a: SortedMap[String, Int], b: SortedMap[String, Int], c: SortedMap[String, Int]) =>
+      associative(a, b, c)
+      associative(a, b, c)(revTry, implicitly)
+    }
+  }
+
+  property("try merge with taking this rhs is the same as ++ on SortedMap") {
+    forAll { (a: SortedMap[String, Int], b: SortedMap[String, Int]) =>
+      implicit val rhs = new TryMerge[Int] {
+        def tryMerge(d: Option[String], a: Int, b: Int) = Success(b)
+      }
+      assert(tryMerge(None, a, b) == Success(a ++ b))
+    }
+  }
+
+  property("if we always return it's the same as a Semigroup") {
+    forAll { (a: SortedMap[String, Int], b: SortedMap[String, Int]) =>
+      implicit val sgBits = new Semigroup[Int] {
+        def combine(a: Int, b: Int) = (a & 0xFFFF0000) | (b & 0x0000FFFF)
+      }
+      val sm = new Semigroup[SortedMap[String, Int]] {
+        def combine(a: SortedMap[String, Int], b: SortedMap[String, Int]) = {
+          val allKeys = a.keySet | b.keySet
+          SortedMap(allKeys.iterator.map { k =>
+            (a.get(k), b.get(k)) match {
+              case (Some(l), Some(r)) => (k, sgBits.combine(l, r))
+              case (Some(l), None) => (k, l)
+              case (None, Some(r)) => (k, r)
+              case (None, None) => sys.error(s"unreachable: non-sense key: $k")
+            }
+          }.toList: _*)
+        }
+      }
+      assert(tryMerge(None, a, b) == Success(sm.combine(a, b)))
+    }
+  }
+}