verify order in search_path, add tests

modules/cli/shared/src/test/scala/dumbo/cli/CliSpec.scala

@@ -53,7 +53,7 @@ class CliSpec extends FunSuite {
 
     assert(res.isRight)
     val (dmb, connection) = res.toOption.get
-    assertEquals(dmb.allSchemas, Set("public"))
+    assertEquals(dmb.allSchemas, List("public"))
     assertEquals(dmb.historyTable, "public.flyway_schema_history")
     assertEquals(dmb.validateOnMigrate, true)
     assertEquals(dmb.resReader.location, Some("/abd/efg"))
@@ -81,7 +81,7 @@ class CliSpec extends FunSuite {
 
     assert(res.isRight)
     val (dmb, connection) = res.toOption.get
-    assertEquals(dmb.allSchemas, Set("schema1", "schema2"))
+    assertEquals(dmb.allSchemas, List("schema1", "schema2"))
     assertEquals(dmb.historyTable, "schema1.some_table")
     assertEquals(dmb.validateOnMigrate, false)
     assertEquals(dmb.resReader.location, Some("/abd/efg"))

modules/core/shared/src/main/scala/dumbo/Dumbo.scala

@@ -173,7 +173,7 @@ class Dumbo[F[_]: Sync: Console](
 ) {
   import Dumbo.*
 
-  private[dumbo] val allSchemas   = defaultSchema :: schemas.toList
+  private[dumbo] val allSchemas   = combineSchemas(defaultSchema, schemas)
   private[dumbo] val historyTable = s"${defaultSchema}.${schemaHistoryTable}"
   private val dumboHistory        = History(historyTable)
 
@@ -346,18 +346,42 @@ class Dumbo[F[_]: Sync: Console](
 
   def runMigration: F[MigrationResult] = sessionResource.use(migrateBySession)
 
+  // search_path needs to include default schema before other schemas
+  private def verifySearchPath(sp: String): F[Option[String]] = {
+    val spSchemas          = sp.split(",").map(_.trim).toVector
+    val sps                = spSchemas.mkString(", ")
+    val expectedSearchPath = toSearchPath(defaultSchema, schemas)
+
+    allSchemas.diff(spSchemas) match {
+      case Nil =>
+        // validate the order
+        val defaultIdx = spSchemas.indexOf(defaultSchema)
+        if (schemas.forall(s => spSchemas.indexOf(s) > defaultIdx)) {
+          none[String].pure[F]
+        } else {
+          Console[F]
+            .println(
+              s"""|WARNING: Default schema '$defaultSchema' is not in the right position of the search path '$sps'.
+                  |The search_path will be set to '${expectedSearchPath}'. Consider adding it to session parameters instead.""".stripMargin
+            )
+            .as(Some(expectedSearchPath))
+        }
+      case missing =>
+        Console[F]
+          .println(
+            s"""|WARNING: Following schemas are not included in the search path '$sps': ${missing.mkString(", ")}.
+                |The search_path will be set to '$expectedSearchPath'. Consider adding it to session parameters instead.""".stripMargin
+          )
+          .as(Some(expectedSearchPath))
+    }
+  }
+
   private def migrateBySession(session: Session[F]): F[Dumbo.MigrationResult] = for {
-    // verify search_path
     _ <- session.unique(sql"SHOW search_path".query(text)).flatMap { sp =>
-           val spSchemas = sp.split(",").map(_.trim).toSet
-           allSchemas.diff(spSchemas.toList) match {
-             case Nil => ().pure[F]
-             case missing =>
-               val newSearchPath = toSearchPath(defaultSchema, schemas)
-               Console[F].println(
-                 s"""|WARNING: Following schemas are not included in the search path '$sp': ${missing.mkString(", ")}.
-                     |The search_path will be set to '${newSearchPath}'. Consider adding it to session parameters instead.""".stripMargin
-               ) >> session.execute(sql"SET search_path TO #${newSearchPath}".command).void
+           verifySearchPath(sp).flatMap {
+             case Some(searchPathUpdate) =>
+               session.execute(sql"SET search_path TO #${searchPathUpdate}".command).void
+             case _ => ().pure[F]
            }
          }
     dbVersion <- session.unique(sql"SELECT version()".query(text))
@@ -569,7 +593,10 @@ object Dumbo extends internal.DumboPlatform {
              .drain
     } yield crc32.getValue().toInt
 
-  // need to ensure that default schema appears first in the search_path
+  // need to ensure that default schema appears first in the list
+  private[dumbo] def combineSchemas(defaultSchema: String, schemas: Set[String]) =
+    (defaultSchema :: schemas.toList).distinct
+
   private[dumbo] def toSearchPath(defaultSchema: String, schemas: Set[String]) =
-    (defaultSchema :: schemas.toList).mkString(",")
+    combineSchemas(defaultSchema, schemas).mkString(", ")
 }

modules/tests/shared/src/main/scala/ffstest/FFramework.scala

@@ -27,6 +27,7 @@ trait FTest extends CatsEffectSuite with FTestPlatform {
 
   def dbTest(name: String)(f: => IO[Unit]): Unit = test(name)(dropSchemas >> f)
 
+  // note: schema name should not start with "pg_" or "crdb_" to avoid conflicts with reserved ones
   def someSchemaName: String = {
     val chars = "abcdefghijklmnopqrstuvwxyz"
     LazyList.continually(chars.charAt(Random.nextInt(chars.length))).take(15).mkString
@@ -112,10 +113,9 @@ trait FTest extends CatsEffectSuite with FTestPlatform {
     for {
       customSchemas <-
         s.execute(
-          sql"""
-        SELECT schema_name::text
-        FROM information_schema.schemata 
-        WHERE schema_name NOT LIKE 'pg_%' AND schema_name NOT LIKE 'crdb_%' AND schema_name NOT IN ('information_schema', 'public')"""
+          sql"""|SELECT schema_name::text
+                |FROM information_schema.schemata 
+                |WHERE schema_name NOT LIKE 'pg\_%' AND schema_name NOT LIKE 'crdb\_%' AND schema_name NOT IN ('information_schema', 'public')""".stripMargin
             .query(skunk.codec.text.text)
         )
       _ <- IO.println(s"Dropping schemas ${customSchemas.mkString(", ")}")
@@ -132,7 +132,9 @@ class TestConsole extends Console[IO] {
 
   override def readLineWithCharset(charset: Charset): IO[String] = ???
   override def print[A](a: A)(implicit S: Show[A]): IO[Unit]     = ???
-  override def println[A](a: A)(implicit S: Show[A]): IO[Unit]   = IO(logs.getAndUpdate(_ :+ S.show(a))).void
-  override def error[A](a: A)(implicit S: Show[A]): IO[Unit]     = IO.println(S.show(a))
-  override def errorln[A](a: A)(implicit S: Show[A]): IO[Unit]   = IO.println(S.show(a))
+  override def println[A](a: A)(implicit S: Show[A]): IO[Unit] = IO {
+    println(S.show(a)); logs.getAndUpdate(_ :+ S.show(a))
+  }.void
+  override def error[A](a: A)(implicit S: Show[A]): IO[Unit]   = IO.println(S.show(a))
+  override def errorln[A](a: A)(implicit S: Show[A]): IO[Unit] = IO.println(S.show(a))
 }

modules/tests/shared/src/test/scala/DumboMigrationSpec.scala

@@ -174,25 +174,34 @@ trait DumboMigrationSpec extends ffstest.FTest {
     val withResources = dumboWithResources("db/test_search_path")
     val schemas       = List("schema_1", "schema_2")
     val testConsole   = new TestConsole
-    val hasWarning = (m: String) =>
+    val hasWarning    = (m: String) => m.contains("""The search_path will be set to 'schema_1, schema_2'""")
+    val hasMissingSchemaWarning = (m: String) =>
       m.contains(
-        """WARNING: Following schemas are not included in the search path '"$user", public': schema_1, schema_2"""
-      ) &&
-      m.contains("""The search_path will be set to 'schema_1,schema_2'""")
+        """Following schemas are not included in the search path '"$user", public': schema_1, schema_2"""
+      ) && hasWarning(m)
+
+    val hasWrongOrderWarning = (m: String) =>
+      m.contains(
+        """Default schema 'schema_1' is not in the right position of the search path 'schema_2, schema_1'"""
+      ) && hasWarning(m)
 
     def migrateBySession(params: Map[String, String] = Map.empty) =
       dumboMigrateWithSession(schemas.head, withResources, session(params), schemas.tail)(testConsole).attempt
 
     for {
+      // warn about missing schemas in the search_path
       dumboRes <- migrateBySession()
       _         = assert(dumboRes.isRight)
-      _         = assert(testConsole.logs.get().exists(hasWarning))
-      // reset
-      _ <- dropSchemas
-      _  = testConsole.flush()
-      // succeed without warning if search_path is set
-      dumboResB <- migrateBySession(Map("search_path" -> "schema_1,schema_2"))
+      _         = assert(testConsole.logs.get().exists(hasMissingSchemaWarning))
+      // warn about wrong order in the search_path
+      _         <- { testConsole.flush(); dropSchemas }
+      dumboResB <- migrateBySession(Map("search_path" -> "schema_2,schema_1"))
       _          = assert(dumboResB.isRight)
+      _          = assert(testConsole.logs.get().exists(hasWrongOrderWarning))
+      // succeed without warning if search_path is set as expected
+      _         <- { testConsole.flush(); dropSchemas }
+      dumboResC <- migrateBySession(Map("search_path" -> "schema_1,schema_2"))
+      _          = assert(dumboResC.isRight)
       _          = assert(!testConsole.logs.get().exists(hasWarning))
     } yield ()
   }
@@ -237,6 +246,15 @@ trait DumboMigrationSpec extends ffstest.FTest {
         // migrate by custom session
         _ <- dumboMigrateWithSession(schemaDefault, withResources, session(), schemas)
         _ <- assertDefaultSchemaHasTable
+        // migrate by custom session with random search_path order
+        _ <- dropSchemas
+        _ <- dumboMigrateWithSession(
+               schemaDefault,
+               withResources,
+               session(Map("search_path" -> scala.util.Random.shuffle(schemaDefault :: schemas).mkString(","))),
+               schemas,
+             )
+        _ <- assertDefaultSchemaHasTable
       } yield ()
     }
   }