Finalize SQLite statements at query scope exit

.github/workflows/test.yml

@@ -30,39 +30,36 @@ jobs:
     - name: Checkout source code
       uses: actions/checkout@v4
 
-    - name: Cache choosenim
-      id: cache-choosenim
-      uses: actions/cache@v4
-      with:
-        path: ~/.choosenim
-        key: ${{ runner.os }}-choosenim-stable
-
-    - name: Cache nimble
-      id: cache-nimble
-      uses: actions/cache@v4
-      with:
-        path: ~/.nimble
-        key: ${{ runner.os }}-nimble-stable
-
     - name: Setup nim
       uses: jiro4989/setup-nim-action@v2
       with:
         nim-version: ${{ matrix.nim }}
 
-    - name: Install dependencies
+    - name: Install Atlas
+      shell: bash
+      env:
+        ATLAS_INSTALL_DIR: ${{ runner.temp }}/atlas/bin
       run: |
-        nimble --useSystemNim install -d
-        nimble --useSystemNim install karax -y
-        nimble --useSystemNim install jester -y
-        nimble --useSystemNim install websocket -y
+        curl -fsSL --retry 3 \
+          https://raw.githubusercontent.com/nim-lang/atlas/HEAD/install.sh | bash
+        echo "$ATLAS_INSTALL_DIR" >> "$GITHUB_PATH"
+
+    - name: Cache dependencies
+      uses: actions/cache@v4
+      with:
+        path: deps/
+        key: atlas-${{ runner.os }}-${{ hashFiles('ormin.nimble') }}
+
+    - name: Install dependencies
+      run: atlas install -uk --feature:examples
 
     - name: Run test sqlite
-      run: nimble --useSystemNim test
+      run: nim test
 
     - name: Run test postgres
-      run: nimble --useSystemNim test_postgres
+      run: nim test_postgres
       env:
         PGPASSWORD: test
 
     - name: Build examples
-      run: nimble buildexamples
+      run: nim buildexamples

config.nims

@@ -14,14 +14,12 @@ task test, "Run all test suite":
   buildimporterTask()
   cleanTask()
 
-  exec "nim c -f -r tests/tfeature"
-  exec "nim c -f -r tests/tcommon"
-  exec "nim c -f -r -d:release tests/tquery_types"
-  exec "nim c -f -r tests/tsqlite"
-  exec "nim c -f -r tests/tdb_utils"
-  exec "nim c -f -r tests/timportstatic"
-  exec "nim c -f -r tests/tpostgres_schema_import"
-  exec "nim c -f -r tests/tqualified_schema_queries"
+  # These suites share test.db and must execute serially.
+  exec("atlas-run tests --jobs:1 tests/tfeature.nim tests/tcommon.nim " &
+    "tests/tsqlite.nim tests/tdb_utils.nim tests/timportstatic.nim " &
+    "tests/ttransactions.nim tests/tpostgres_schema_import.nim " &
+    "tests/tqualified_schema_queries.nim")
+  exec "atlas-run tests --jobs:2 tests/tquery_types.nim -- -d:release"
 
 task setup_postgres, "Ensure local Postgres has test DB/user":
   # Use a simple script to avoid Nim/psql quoting pitfalls
@@ -35,10 +33,11 @@ task test_postgres, "Run PostgreSQL test suite":
   exec "./tools/ormin_importer tests/forum_model_postgres.sql"
   exec "./tools/ormin_importer tests/model_postgre.sql"
 
-  exec "nim c -f -d:nimDebugDlOpen -r -d:postgre tests/tfeature"
-  exec "nim c -f -d:nimDebugDlOpen -r -d:postgre tests/tcommon"
-  exec "nim c -f -d:nimDebugDlOpen -r -d:release -d:postgre tests/tquery_types"
-  exec "nim c -f -d:nimDebugDlOpen -r -d:postgre tests/tpostgre"
+  # These suites share one PostgreSQL database and must execute serially.
+  exec("atlas-run tests --jobs:1 tests/tfeature.nim tests/tcommon.nim " &
+    "tests/tpostgre.nim -- -d:nimDebugDlOpen -d:postgre")
+  exec("atlas-run tests --jobs:2 tests/tquery_types.nim -- " &
+    "-d:nimDebugDlOpen -d:release -d:postgre")
 
 task buildexamples, "Build examples: chat and forum":
   buildimporterTask()

ormin.nimble

@@ -15,7 +15,7 @@ requires "db_connector >= 0.1.0"
 feature "examples":
   requires "websocket >= 0.2.2"
   requires "karax"
-  requires "jester"
+  requires "jester >= 0.6.0"
 
 import std/os
 when fileExists("config.nims"):

ormin/ormin_sqlite.nim

@@ -34,6 +34,11 @@ proc prepareStmt*(db: DbConn; q: string): PStmt =
   if prepare_v2(db, q, q.len.cint, result, nil) != SQLITE_OK:
     dbError(db)
 
+proc finalizeStmt*(s: PStmt) {.raises: [].} =
+  ## Release a query's statement, including when bindings or result conversion raise.
+  ## stopQuery reports execution errors before this nonraising scope cleanup runs.
+  discard sqlite3.finalize(s)
+
 template startBindings*(s: PStmt; n: int) =
   if clear_bindings(s) != SQLITE_OK: dbError(db)
 

ormin/queries.nim

@@ -165,24 +165,30 @@ type
 
 # Execute a non-row SQL statement strictly (errors on failure)
 template execNoRowsStrict*(sqlStmt: string) =
-  when defined(debugOrminTrace):
-    echo "[[Ormin Executing]]: ", q
-  let s {.gensym.} = prepareStmt(db, sqlStmt)
-  startQuery(db, s)
-  if stepQuery(db, s, false):
-    stopQuery(db, s)
-  else:
-    stopQuery(db, s)
-    dbError(db)
+  block:
+    when defined(debugOrminTrace):
+      echo "[[Ormin Executing]]: ", sqlStmt
+    let s {.gensym.} = prepareStmt(db, sqlStmt)
+    when dbBackend == DbBackend.sqlite:
+      defer: finalizeStmt(s)
+    startQuery(db, s)
+    if stepQuery(db, s, false):
+      stopQuery(db, s)
+    else:
+      stopQuery(db, s)
+      dbError(db)
 
 # Execute a non-row SQL statement, relying on startQuery to raise on failure
 template execNoRowsLoose(sqlStmt: string) =
-  when defined(debugOrminTrace):
-    echo "[[Ormin Executing]]: ", sqlStmt
-  let s {.gensym.} = prepareStmt(db, sqlStmt)
-  startQuery(db, s)
-  discard stepQuery(db, s, false)
-  stopQuery(db, s)
+  block:
+    when defined(debugOrminTrace):
+      echo "[[Ormin Executing]]: ", sqlStmt
+    let s {.gensym.} = prepareStmt(db, sqlStmt)
+    when dbBackend == DbBackend.sqlite:
+      defer: finalizeStmt(s)
+    startQuery(db, s)
+    discard stepQuery(db, s, false)
+    stopQuery(db, s)
 
 proc newQueryBuilder(): QueryBuilder {.compileTime.} =
   QueryBuilder(head: "", fromm: "", join: "", values: "", where: "",
@@ -810,6 +816,11 @@ proc cond(n: NimNode; q: var string; params: var Params;
   else:
     macros.error "construct not supported in condition: " & treeRepr n, n
 
+proc addStatementCleanup(body, statement: NimNode) =
+  when dbBackend == DbBackend.sqlite:
+    body.add quote do:
+      defer: finalizeStmt(`statement`)
+
 proc generateRoutine(name: NimNode, q: QueryBuilder;
                      sql: string; k: NimNodeKind): NimNode =
   let prepStmt = ident($name & "PrepStmt")
@@ -818,6 +829,7 @@ proc generateRoutine(name: NimNode, q: QueryBuilder;
   let body = newStmtList()
   # Ensure the prepared statement is created before binding/starting the query
   body.add prepare
+  addStatementCleanup(body, prepStmt)
 
   var finalParams = newNimNode(nnkFormalParams)
   if q.retTypeIsJson:
@@ -887,7 +899,7 @@ proc generateRoutine(name: NimNode, q: QueryBuilder;
     newEmptyNode(), # pragmas
     newEmptyNode(),
     body)
-  result = newStmtList(prepare, iter)
+  result = iter
 
 proc getColumnName(n: NimNode): string =
   case n.kind
@@ -1607,9 +1619,10 @@ proc queryImpl(q: QueryBuilder; body: NimNode; attempt, produceJson: bool): NimN
   let prepStmt = genSym(nskLet)
   let res = genSym(nskVar)
   result = newTree(
-    if q.retType.len > 0: nnkStmtListExpr else: nnkStmtList,
+    nnkStmtList,
     newLetStmt(prepStmt, newCall(bindSym"prepareStmt", ident"db", newLit sql))
   )
+  addStatementCleanup(result, prepStmt)
   let rtyp = if q.retType.len > 1 or q.retType.len == 0:
     q.retType
   else:
@@ -1724,6 +1737,7 @@ proc queryImpl(q: QueryBuilder; body: NimNode; attempt, produceJson: bool): NimN
   result.add newTree(nnkBlockStmt, newEmptyNode(), blk)
   if q.retType.len > 0:
     result.add res
+  result = newTree(nnkBlockStmt, newEmptyNode(), result)
 
 proc queryHookImpl(q: QueryBuilder; body: NimNode; attempt: bool; retType: NimNode): NimNode =
   expectKind body, nnkStmtList
@@ -1742,9 +1756,10 @@ proc queryHookImpl(q: QueryBuilder; body: NimNode; attempt: bool; retType: NimNo
   let prepStmt = genSym(nskLet)
   let res = genSym(nskVar)
   result = newTree(
-    nnkStmtListExpr,
+    nnkStmtList,
     newLetStmt(prepStmt, newCall(bindSym"prepareStmt", ident"db", newLit sql))
   )
+  addStatementCleanup(result, prepStmt)
   if q.singleRow:
     result.add newTree(nnkVarSection, newIdentDefs(res, copyNimTree(retType), newEmptyNode()))
   else:
@@ -1794,6 +1809,7 @@ proc queryHookImpl(q: QueryBuilder; body: NimNode; attempt: bool; retType: NimNo
 
   result.add newTree(nnkBlockStmt, newEmptyNode(), blk)
   result.add res
+  result = newTree(nnkBlockStmt, newEmptyNode(), result)
 
 macro query*(args: varargs[untyped]): untyped =
   if args.len == 1:

tests/tsqlite.nim

@@ -2,6 +2,7 @@ import unittest, db_connector/sqlite3, json, times, sequtils
 from db_connector/db_sqlite import exec, getValue
 import ormin
 import ormin/db_utils
+import ./tstatement_lifetime
 
 importModel(DbBackend.sqlite, "model_sqlite")
 

tests/tstatement_lifetime.nim

@@ -0,0 +1,111 @@
+import std/assertions
+
+import db_connector/db_sqlite as sqliteDb except DbConn, open, close, dbError
+import ormin
+importModel(DbBackend.sqlite, "model_sqlite", includeStatic = true)
+
+proc newDatabase(): DbConn =
+  result = open(":memory:", "", "", "")
+  sqliteDb.exec(result, sql"CREATE TABLE tb_string(typstring TEXT NOT NULL UNIQUE)")
+  sqliteDb.exec(result, sql"INSERT INTO tb_string VALUES ('first'), ('second')")
+
+block repeated_queries_release_statements_before_connection_close:
+  let db = newDatabase()
+  for index in 0 ..< 100:
+    let rows = query:
+      select tb_string(typstring)
+    doAssert rows.len == 2
+    let typed = query(string):
+      select tb_string(typstring)
+    doAssert typed.len == 2
+    let missing = "missing"
+    let absent = tryQuery:
+      select tb_string(typstring)
+      where typstring == ?missing
+      limit 1
+    discard absent
+  close(db)
+
+block unused_routine_declarations_do_not_prepare_statements:
+  let db = newDatabase()
+  createProc unusedProc:
+    select tb_string(typstring)
+  createIter unusedIterator:
+    select tb_string(typstring)
+  close(db)
+
+createIter strings:
+  select tb_string(typstring)
+
+createProc allStrings:
+  select tb_string(typstring)
+
+block generated_proc_and_complete_iteration_release_statements:
+  let db = newDatabase()
+  for index in 0 ..< 100:
+    doAssert allStrings(db).len == 2
+    var count = 0
+    for value in strings(db):
+      inc count
+    doAssert count == 2
+  close(db)
+
+block iterator_break_releases_its_statement:
+  let db = newDatabase()
+  for value in strings(db):
+    doAssert value.len > 0
+    break
+  close(db)
+
+proc returnFromIterator(db: DbConn): string =
+  for value in strings(db):
+    return value
+
+block caller_return_releases_iterator_statement:
+  let db = newDatabase()
+  doAssert returnFromIterator(db).len > 0
+  close(db)
+
+block caller_exception_releases_iterator_statement:
+  let db = newDatabase()
+  doAssertRaises ValueError:
+    for value in strings(db):
+      raise newException(ValueError, value)
+  close(db)
+
+type
+  ReadFailure = distinct string
+  BindFailure = distinct string
+
+proc fromQueryHook*(dest: var ReadFailure, value: string) =
+  raise newException(ValueError, "result conversion failed")
+
+proc toQueryHook*(dest: var string, value: BindFailure) =
+  raise newException(ValueError, "parameter conversion failed")
+
+block result_hook_exception_releases_statement:
+  let db = newDatabase()
+  doAssertRaises ValueError:
+    discard query(ReadFailure):
+      select tb_string(typstring)
+  close(db)
+
+block binding_hook_exception_releases_statement:
+  let db = newDatabase()
+  let value = BindFailure("first")
+  doAssertRaises ValueError:
+    discard query:
+      select tb_string(typstring)
+      where typstring == ?value
+  close(db)
+
+block execution_error_preserves_error_and_releases_statement:
+  let db = newDatabase()
+  doAssertRaises DbError:
+    query:
+      insert tb_string(typstring = "first")
+  # The failed query must not poison a subsequent query or connection close.
+  let rows = query:
+    select tb_string(typstring)
+  doAssert rows.len == 2
+  close(db)