diff --git a/modules/cli/shared/src/test/scala/dumbo/cli/CliSpec.scala b/modules/cli/shared/src/test/scala/dumbo/cli/CliSpec.scala index d8d2405..b2f8fdc 100644 --- a/modules/cli/shared/src/test/scala/dumbo/cli/CliSpec.scala +++ b/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")) diff --git a/modules/core/shared/src/main/scala/dumbo/Dumbo.scala b/modules/core/shared/src/main/scala/dumbo/Dumbo.scala index 408758d..c60dd75 100644 --- a/modules/core/shared/src/main/scala/dumbo/Dumbo.scala +++ b/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(", ") } diff --git a/modules/tests/shared/src/main/scala/ffstest/FFramework.scala b/modules/tests/shared/src/main/scala/ffstest/FFramework.scala index b8685bb..0be604c 100644 --- a/modules/tests/shared/src/main/scala/ffstest/FFramework.scala +++ b/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)) } diff --git a/modules/tests/shared/src/test/scala/DumboMigrationSpec.scala b/modules/tests/shared/src/test/scala/DumboMigrationSpec.scala index 050779d..abf5a48 100644 --- a/modules/tests/shared/src/test/scala/DumboMigrationSpec.scala +++ b/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 () } }