nim-lang / db_connector

Unified db connector in Nim
MIT License
20 stars 6 forks source link

db_sqlite.tryInsertID does raise exceptions in 1.6.0 #3

Open gradha opened 2 years ago

gradha commented 2 years ago

The following code compiles with nim 1.4.8:

import db_sqlite

proc mark_seen_url*(url: string) {.raises: [].} =
  var conn: DB_conn
  let result = conn.try_insert_id(sql"evil insert", url)

This code stopped compiling in 1.6.0 and later because it raises now DbError:

$ choosenim stable
   Switched to Nim 1.6.4
$ nim c dbtest.nim 
Hint: used config file '/Users/gradha/.choosenim/toolchains/nim-1.6.4/config/nim.cfg' [Conf]
Hint: used config file '/Users/gradha/.choosenim/toolchains/nim-1.6.4/config/config.nims' [Conf]
..........................................................
/Users/gradha/.choosenim/toolchains/nim-1.6.4/lib/impure/db_sqlite.nim(184, 1) Warning: Circular dependency detected. `codeReordering` pragma may not be able to reorder some nodes properly [User]
.....
/private/tmp/t/dbtest.nim(5, 7) Hint: 'result' is declared but not used [XDeclaredButNotUsed]
/private/tmp/t/dbtest.nim(3, 44) template/generic instantiation from here
/private/tmp/t/dbtest.nim(5, 20) Error: tryInsertID(conn,
  SqlQuery(r"evil insert"), [url]) can raise an unlisted exception: DbError

The odbc tryInsertId and mysql tryInsertId versions still maintain their well ment raises: [] pragma. The postgres and sqlite seem to have grown unexpected exceptions.

The sqlite module gained the API breaking change in this commit while work was being done to fix nim-lang/Nim#18669, which wasn't related to the tryInsertID proc. I haven't found any discussions in the forums or github issues about this tryInsertID API breakage. Given that other tryInsertID procs still raise no exceptions in their definitions, and a try* method ideally should keep its meaning and raise no unsuspected exceptions, this feels like a bug in both db_sqlite and db_postgres.

ringabout commented 2 years ago

The regression of the dbsqlite is already fixed. But db* modules is highly likely to move out of the stds. Fixing the bug of db_postgres is not high priority.

ringabout commented 2 years ago

see https://github.com/nim-lang/Nim/pull/19745#issuecomment-1108105729