Copilot commented on code in PR #6238:
URL: https://github.com/apache/texera/pull/6238#discussion_r3540350876
##########
common/dao/src/test/scala/org/apache/texera/dao/MockTexeraDB.scala:
##########
@@ -22,143 +22,164 @@ package org.apache.texera.dao
import io.zonky.test.db.postgres.embedded.EmbeddedPostgres
import org.jooq.impl.DSL
import org.jooq.{DSLContext, SQLDialect}
+import org.scalatest.Outcome
+import org.scalatest.flatspec.AnyFlatSpecLike
import java.nio.file.Paths
import java.sql.{Connection, DriverManager}
import scala.io.Source
-
-trait MockTexeraDB {
-
- private var dbInstance: Option[EmbeddedPostgres] = None
- private var dslContext: Option[DSLContext] = None
- private val database: String = "texera_db"
+import scala.util.Using
+
+/**
+ * Provides a JVM-singleton EmbeddedPostgres for tests. Multiple specs that
mix
+ * in this trait share one Postgres instance for the lifetime of the JVM,
which
+ * avoids the OverlappingFileLockException that occurs when each spec tries to
+ * extract the embedded Postgres binaries into the same directory in parallel.
+ */
+object MockTexeraDB {
private val username: String = "postgres"
private val password: String = ""
- def executeScriptInJDBC(conn: Connection, script: String): Unit = {
- assert(dbInstance.nonEmpty)
- conn.prepareStatement(script).execute()
- conn.close()
- }
+ @volatile private var dbInstance: Option[EmbeddedPostgres] = None
+ @volatile private var ddlScript: Option[String] = None
- def getDSLContext: DSLContext = {
- dslContext match {
- case Some(value) => value
- case None =>
- throw new RuntimeException(
- "test database is not initialized. Did you call
initializeDBAndReplaceDSLContext()?"
- )
- }
- }
+ def ensureInitialized(): Unit =
+ synchronized {
+ if (dbInstance.isDefined) return
+
+ val driver = new org.postgresql.Driver()
+ DriverManager.registerDriver(driver)
+
+ // Boot the heavy JVM engine exactly once
+ val embedded = EmbeddedPostgres.builder().start()
+ dbInstance = Some(embedded)
Review Comment:
`ensureInitialized()` can leave the singleton in a partially-initialized
state: it sets `dbInstance = Some(embedded)` before reading/processing the DDL.
If DDL loading throws (missing file, IO error, regex failure), subsequent calls
return early on `dbInstance.isDefined` and `ddlScript` stays `None`, causing
later tests to fail with a misleading "DDL not loaded" and preventing recovery.
##########
common/dao/src/test/scala/org/apache/texera/dao/MockTexeraDB.scala:
##########
@@ -22,143 +22,164 @@ package org.apache.texera.dao
import io.zonky.test.db.postgres.embedded.EmbeddedPostgres
import org.jooq.impl.DSL
import org.jooq.{DSLContext, SQLDialect}
+import org.scalatest.Outcome
+import org.scalatest.flatspec.AnyFlatSpecLike
import java.nio.file.Paths
import java.sql.{Connection, DriverManager}
import scala.io.Source
-
-trait MockTexeraDB {
-
- private var dbInstance: Option[EmbeddedPostgres] = None
- private var dslContext: Option[DSLContext] = None
- private val database: String = "texera_db"
+import scala.util.Using
+
+/**
+ * Provides a JVM-singleton EmbeddedPostgres for tests. Multiple specs that
mix
+ * in this trait share one Postgres instance for the lifetime of the JVM,
which
+ * avoids the OverlappingFileLockException that occurs when each spec tries to
+ * extract the embedded Postgres binaries into the same directory in parallel.
+ */
+object MockTexeraDB {
private val username: String = "postgres"
private val password: String = ""
- def executeScriptInJDBC(conn: Connection, script: String): Unit = {
- assert(dbInstance.nonEmpty)
- conn.prepareStatement(script).execute()
- conn.close()
- }
+ @volatile private var dbInstance: Option[EmbeddedPostgres] = None
+ @volatile private var ddlScript: Option[String] = None
- def getDSLContext: DSLContext = {
- dslContext match {
- case Some(value) => value
- case None =>
- throw new RuntimeException(
- "test database is not initialized. Did you call
initializeDBAndReplaceDSLContext()?"
- )
- }
- }
+ def ensureInitialized(): Unit =
+ synchronized {
+ if (dbInstance.isDefined) return
+
+ val driver = new org.postgresql.Driver()
+ DriverManager.registerDriver(driver)
+
+ // Boot the heavy JVM engine exactly once
+ val embedded = EmbeddedPostgres.builder().start()
+ dbInstance = Some(embedded)
+
+ val ddlPath = Paths.get("sql/texera_ddl.sql").toRealPath()
+ val source = Source.fromFile(ddlPath.toString)
+ val content =
+ try source.mkString
+ finally source.close()
- def getDBInstance: EmbeddedPostgres = {
- dbInstance match {
- case Some(value) => value
- case None =>
- throw new RuntimeException(
- "test database is not initialized. Did you call
initializeDBAndReplaceDSLContext()?"
- )
+ val parts: Array[String] = content.split("(?m)^CREATE DATABASE
:\"DB_NAME\";")
+ val sqlBody = if (parts.length > 1) parts(1) else content
+
+ def removeCCommands(sql: String): String =
+ sql.linesIterator.filterNot(_.trim.startsWith("\\c")).mkString("\n")
+
+ var tablesAndIndexCreation = removeCCommands(sqlBody)
+
+ val blockPattern =
+ """(?s)-- START Fulltext search index creation \(DO NOT EDIT THIS
LINE\).*?-- END Fulltext search index creation \(DO NOT EDIT THIS LINE\)\n?""".r
+ val replacementText =
+ """CREATE INDEX idx_workflow_name_description_content ON workflow
USING GIN (to_tsvector('english', COALESCE(name, '') || ' ' ||
COALESCE(description, '') || ' ' || COALESCE(content, '')));
+ |CREATE INDEX idx_user_name ON "user" USING GIN
(to_tsvector('english', COALESCE(name, '')));
+ |CREATE INDEX idx_user_project_name_description ON project USING GIN
(to_tsvector('english', COALESCE(name, '') || ' ' || COALESCE(description,
'')));
+ |CREATE INDEX idx_dataset_name_description ON dataset USING GIN
(to_tsvector('english', COALESCE(name, '') || ' ' || COALESCE(description,
'')));
+ |CREATE INDEX idx_dataset_version_name ON dataset_version USING GIN
(to_tsvector('english', COALESCE(name, '')));""".stripMargin
+
+ // Cache the cleaned script so parallel suites don't have to re-read the
file
+ ddlScript = Some(blockPattern.replaceAllIn(tablesAndIndexCreation,
replacementText).trim)
}
- }
- def shutdownDB(): Unit = {
- dbInstance match {
- case Some(value) =>
- value.close()
- dbInstance = None
- dslContext = None
- case None =>
- // do nothing
+ def getDBInstance: EmbeddedPostgres =
+ dbInstance.getOrElse(throw new RuntimeException("DB not initialized"))
+ def getDDLScript: String = ddlScript.getOrElse(throw new
RuntimeException("DDL not loaded"))
+}
+
+trait MockTexeraDB extends AnyFlatSpecLike {
+ private var testScopedContext: Option[DSLContext] = None
+ protected var connection: Option[Connection] = None
+ protected var uniqueDbName: String = ""
+
+ def initializeDBAndReplaceDSLContext(): Unit =
+ synchronized {
+ if (connection.isEmpty || connection.get.isClosed) {
+ MockTexeraDB.ensureInitialized()
+ val embedded = MockTexeraDB.getDBInstance
+
+ uniqueDbName = "texera_db_" +
java.util.UUID.randomUUID().toString.replace("-", "")
+ Using.resource(embedded.getPostgresDatabase.getConnection) {
defaultConn =>
+ Using.resource(defaultConn.createStatement()) { stmt =>
+ stmt.execute(s"CREATE DATABASE $uniqueDbName")
+ }
+ }
+
+ val conn = embedded.getDatabase("postgres", uniqueDbName).getConnection
+
+ // AutoCommit is TRUE by default, meaning any records inserted in
beforeAll()
+ // will be permanently committed to this suite's specific isolated
database!
+ Using.resource(conn.createStatement()) { stmt =>
+ stmt.execute(MockTexeraDB.getDDLScript)
+ }
+
+ connection = Some(conn)
+ val scopedCtx = DSL.using(conn, SQLDialect.POSTGRES)
+ testScopedContext = Some(scopedCtx)
+
+ // Point the Texera backend exactly to this suite's isolated database
+ SqlServer.initConnection(embedded.getJdbcUrl("postgres",
uniqueDbName), "postgres", "")
+ SqlServer.getInstance().replaceDSLContext(scopedCtx)
+ }
}
- }
- def initializeDBAndReplaceDSLContext(): Unit = {
- assert(dbInstance.isEmpty && dslContext.isEmpty)
+ override def withFixture(test: NoArgTest): Outcome = {
+ initializeDBAndReplaceDSLContext()
- val driver = new org.postgresql.Driver()
- DriverManager.registerDriver(driver)
+ val conn = connection.get
+ val sqlServerInstance = SqlServer.getInstance()
+ val activeContext = testScopedContext.get
- val embedded = EmbeddedPostgres.builder().start()
+ conn.setAutoCommit(true)
- dbInstance = Some(embedded)
+ try {
+ sqlServerInstance.replaceDSLContext(activeContext)
+ super.withFixture(test)
+ } finally {
+ try {
+ if (!conn.isClosed) {
+ scala.util.Using.resource(conn.createStatement()) { stmt =>
+ stmt.execute(
+ """
+ DO $$ DECLARE
+ r RECORD;
+ BEGIN
+ FOR r IN (SELECT tablename FROM pg_tables WHERE schemaname =
'public') LOOP
+ EXECUTE 'TRUNCATE TABLE ' || quote_ident(r.tablename) ||
' CASCADE';
+ END LOOP;
+ END $$;
+ """
+ )
+ }
+ }
+ } catch {
+ case e: Exception => e.printStackTrace()
+ }
+ }
Review Comment:
The per-test cleanup truncates tables using whatever `autoCommit` state the
test left behind. If a test sets `conn.setAutoCommit(false)` (or leaves an open
transaction/lock) and forgets to restore it, the TRUNCATE may run inside an
uncommitted transaction (or block on locks), leaking state and potentially
breaking subsequent tests.
##########
common/dao/src/main/scala/org/apache/texera/dao/SqlServer.scala:
##########
@@ -94,13 +94,37 @@ object SqlServer {
* @return
*/
def withTransaction[T](dsl: DSLContext)(block: DSLContext => T): T = {
- var result: Option[T] = None
+ val provider = dsl.configuration().connectionProvider()
+ val conn = provider.acquire()
+ val originalAutoCommit = conn.getAutoCommit
- dsl.transaction(configuration => {
- val ctx = DSL.using(configuration)
- result = Some(block(ctx))
- })
+ try {
+ if (originalAutoCommit) {
+ conn.setAutoCommit(false)
+ }
- result.getOrElse(throw new RuntimeException("Transaction failed without
result!"))
+ val txCtx = org.jooq.impl.DSL.using(conn, dsl.dialect())
+ val result = block(txCtx)
+
+ if (originalAutoCommit) {
+ conn.commit()
+ }
+
+ result
+ } catch {
+ case e: Throwable =>
+ if (originalAutoCommit) {
+ conn.rollback()
+ }
+ throw e
+ } finally {
+ try {
+ if (originalAutoCommit != conn.getAutoCommit) {
+ conn.setAutoCommit(originalAutoCommit)
+ }
+ } finally {
+ provider.release(conn)
+ }
+ }
Review Comment:
`withTransaction` builds `txCtx` via `DSL.using(conn, dsl.dialect())`, which
drops the original jOOQ configuration (settings, listeners, execute listeners,
etc.). This can subtly change behavior compared to executing via the passed-in
`dsl`.
Also, the `finally` block can mask the original exception if
`conn.getAutoCommit` throws (e.g., connection already closed), because that
exception would escape the finally before `provider.release(conn)` completes
normally.
##########
common/dao/src/test/scala/org/apache/texera/dao/MockTexeraDB.scala:
##########
@@ -22,143 +22,164 @@ package org.apache.texera.dao
import io.zonky.test.db.postgres.embedded.EmbeddedPostgres
import org.jooq.impl.DSL
import org.jooq.{DSLContext, SQLDialect}
+import org.scalatest.Outcome
+import org.scalatest.flatspec.AnyFlatSpecLike
import java.nio.file.Paths
import java.sql.{Connection, DriverManager}
import scala.io.Source
-
-trait MockTexeraDB {
-
- private var dbInstance: Option[EmbeddedPostgres] = None
- private var dslContext: Option[DSLContext] = None
- private val database: String = "texera_db"
+import scala.util.Using
+
+/**
+ * Provides a JVM-singleton EmbeddedPostgres for tests. Multiple specs that
mix
+ * in this trait share one Postgres instance for the lifetime of the JVM,
which
+ * avoids the OverlappingFileLockException that occurs when each spec tries to
+ * extract the embedded Postgres binaries into the same directory in parallel.
+ */
+object MockTexeraDB {
private val username: String = "postgres"
private val password: String = ""
- def executeScriptInJDBC(conn: Connection, script: String): Unit = {
- assert(dbInstance.nonEmpty)
- conn.prepareStatement(script).execute()
- conn.close()
- }
+ @volatile private var dbInstance: Option[EmbeddedPostgres] = None
+ @volatile private var ddlScript: Option[String] = None
- def getDSLContext: DSLContext = {
- dslContext match {
- case Some(value) => value
- case None =>
- throw new RuntimeException(
- "test database is not initialized. Did you call
initializeDBAndReplaceDSLContext()?"
- )
- }
- }
+ def ensureInitialized(): Unit =
+ synchronized {
+ if (dbInstance.isDefined) return
+
+ val driver = new org.postgresql.Driver()
+ DriverManager.registerDriver(driver)
+
+ // Boot the heavy JVM engine exactly once
+ val embedded = EmbeddedPostgres.builder().start()
+ dbInstance = Some(embedded)
+
+ val ddlPath = Paths.get("sql/texera_ddl.sql").toRealPath()
+ val source = Source.fromFile(ddlPath.toString)
+ val content =
+ try source.mkString
+ finally source.close()
- def getDBInstance: EmbeddedPostgres = {
- dbInstance match {
- case Some(value) => value
- case None =>
- throw new RuntimeException(
- "test database is not initialized. Did you call
initializeDBAndReplaceDSLContext()?"
- )
+ val parts: Array[String] = content.split("(?m)^CREATE DATABASE
:\"DB_NAME\";")
+ val sqlBody = if (parts.length > 1) parts(1) else content
+
+ def removeCCommands(sql: String): String =
+ sql.linesIterator.filterNot(_.trim.startsWith("\\c")).mkString("\n")
+
+ var tablesAndIndexCreation = removeCCommands(sqlBody)
+
+ val blockPattern =
+ """(?s)-- START Fulltext search index creation \(DO NOT EDIT THIS
LINE\).*?-- END Fulltext search index creation \(DO NOT EDIT THIS LINE\)\n?""".r
+ val replacementText =
+ """CREATE INDEX idx_workflow_name_description_content ON workflow
USING GIN (to_tsvector('english', COALESCE(name, '') || ' ' ||
COALESCE(description, '') || ' ' || COALESCE(content, '')));
+ |CREATE INDEX idx_user_name ON "user" USING GIN
(to_tsvector('english', COALESCE(name, '')));
+ |CREATE INDEX idx_user_project_name_description ON project USING GIN
(to_tsvector('english', COALESCE(name, '') || ' ' || COALESCE(description,
'')));
+ |CREATE INDEX idx_dataset_name_description ON dataset USING GIN
(to_tsvector('english', COALESCE(name, '') || ' ' || COALESCE(description,
'')));
+ |CREATE INDEX idx_dataset_version_name ON dataset_version USING GIN
(to_tsvector('english', COALESCE(name, '')));""".stripMargin
+
+ // Cache the cleaned script so parallel suites don't have to re-read the
file
+ ddlScript = Some(blockPattern.replaceAllIn(tablesAndIndexCreation,
replacementText).trim)
}
- }
- def shutdownDB(): Unit = {
- dbInstance match {
- case Some(value) =>
- value.close()
- dbInstance = None
- dslContext = None
- case None =>
- // do nothing
+ def getDBInstance: EmbeddedPostgres =
+ dbInstance.getOrElse(throw new RuntimeException("DB not initialized"))
+ def getDDLScript: String = ddlScript.getOrElse(throw new
RuntimeException("DDL not loaded"))
+}
+
+trait MockTexeraDB extends AnyFlatSpecLike {
Review Comment:
The refactor description emphasizes keeping `MockTexeraDB` as a lightweight,
backward-compatible mixin, but the new trait now extends `AnyFlatSpecLike`,
which unnecessarily constrains all consumers to the FlatSpec style. This is a
behavioral/API constraint (and can cause linearization conflicts) if any
existing or future test suite wants to use `MockTexeraDB` from a different
ScalaTest style (e.g., WordSpec/FunSuite).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
