From ca9ee12aeb81edc71d85098df8bf7e21d38ff870 Mon Sep 17 00:00:00 2001 From: "Michael Bradley, Jr" Date: Fri, 8 Jul 2022 13:09:55 -0500 Subject: [PATCH] check column metadata in id/data/timestampCol The goal is to detect mismatches between caller-supplied indexes and original column names, and in that case crash the process by raising Defect. This should help avoid harder to debug errors for such mismatches. These helper procs are now higher-order, which allows column metadata checks to be run only once, i.e. when setting up derivative helpers to be used in an `onData` callback. Use compile-time constants for column names and default indexes. Adjust callback proc annotations to be more precise, and remove unnecessary annotations. --- datastore/sqlite.nim | 12 +- datastore/sqlite_datastore.nim | 223 +++++++++++++--------- tests/datastore/test_sqlite_datastore.nim | 28 ++- 3 files changed, 165 insertions(+), 98 deletions(-) diff --git a/datastore/sqlite.nim b/datastore/sqlite.nim index 6054ab9..8a2ae71 100644 --- a/datastore/sqlite.nim +++ b/datastore/sqlite.nim @@ -3,16 +3,22 @@ import pkg/questionable/results import pkg/sqlite3_abi import pkg/upraises -push: {.upraises: [].} - # Adapted from: # https://github.com/status-im/nwaku/blob/master/waku/v2/node/storage/sqlite.nim +# see https://www.sqlite.org/c3ref/column_database_name.html +# can pass `--forceBuild:on` to the Nim compiler if a SQLite build without +# `-DSQLITE_ENABLE_COLUMN_METADATA` option is stuck in the build cache, +# e.g. `nimble test --forceBuild:on` +{.passC: "-DSQLITE_ENABLE_COLUMN_METADATA".} + +push: {.upraises: [].} + type AutoDisposed*[T: ptr|ref] = object val*: T - DataProc* = proc(s: RawStmtPtr) {.closure.} + DataProc* = proc(s: RawStmtPtr) {.closure, gcsafe.} NoParams* = tuple # empty tuple diff --git a/datastore/sqlite_datastore.nim b/datastore/sqlite_datastore.nim index 86c1775..b256503 100644 --- a/datastore/sqlite_datastore.nim +++ b/datastore/sqlite_datastore.nim @@ -17,6 +17,12 @@ export datastore, sqlite push: {.upraises: [].} type + BoundIdCol = proc (): string {.closure, gcsafe, upraises: [].} + + BoundDataCol = proc (): seq[byte] {.closure, gcsafe, upraises: [].} + + BoundTimestampCol = proc (): int64 {.closure, gcsafe, upraises: [].} + # feels odd to use `void` for prepared statements corresponding to SELECT # queries but it fits with the rest of the SQLite wrapper adapted from # status-im/nwaku, at least in its current form in ./sqlite @@ -33,17 +39,26 @@ type containsStmt: ContainsStmt deleteStmt: DeleteStmt env: SQLite + getDataCol: BoundDataCol getStmt: GetStmt putStmt: PutStmt readOnly: bool const - IdType = "TEXT" - DataType = "BLOB" - TimestampType = "INTEGER" - dbExt* = ".sqlite3" - tableTitle* = "Store" + tableName* = "Store" + + idColName = "id" + dataColName = "data" + timestampColName = "timestamp" + + idColIndex = 0 + dataColIndex = 1 + timestampColIndex = 2 + + idColType = "TEXT" + dataColType = "BLOB" + timestampColType = "INTEGER" # https://stackoverflow.com/a/9756276 # EXISTS returns a boolean value represented by an integer: @@ -51,35 +66,128 @@ const # https://sqlite.org/lang_expr.html#the_exists_operator containsStmtStr = """ SELECT EXISTS( - SELECT 1 FROM """ & tableTitle & """ - WHERE id = ? + SELECT 1 FROM """ & tableName & """ + WHERE """ & idColName & """ = ? ); """ createStmtStr = """ - CREATE TABLE IF NOT EXISTS """ & tableTitle & """ ( - id """ & IdType & """ NOT NULL PRIMARY KEY, - data """ & DataType & """, - timestamp """ & TimestampType & """ NOT NULL + CREATE TABLE IF NOT EXISTS """ & tableName & """ ( + """ & idColName & """ """ & idColType & """ NOT NULL PRIMARY KEY, + """ & dataColName & """ """ & dataColType & """, + """ & timestampColName & """ """ & timestampColType & """ NOT NULL ) WITHOUT ROWID; """ deleteStmtStr = """ - DELETE FROM """ & tableTitle & """ - WHERE id = ?; + DELETE FROM """ & tableName & """ + WHERE """ & idColName & """ = ?; """ getStmtStr = """ - SELECT data FROM """ & tableTitle & """ - WHERE id = ?; + SELECT """ & dataColName & """ FROM """ & tableName & """ + WHERE """ & idColName & """ = ?; """ putStmtStr = """ - REPLACE INTO """ & tableTitle & """ ( - id, data, timestamp + REPLACE INTO """ & tableName & """ ( + """ & idColName & """, + """ & dataColName & """, + """ & timestampColName & """ ) VALUES (?, ?, ?); """ +proc idCol*( + s: RawStmtPtr, + index = idColIndex): BoundIdCol = + + let + i = index.cint + colName = sqlite3_column_origin_name(s, i) + + if colName.isNil or $colName != idColName: + raise (ref Defect)(msg: "original column name for index " & $index & + " was not \"" & idColName & "\"") + + return proc (): string = + let + text = sqlite3_column_text(s, i) + + # detect out-of-memory error + # see the conversion table and final paragraph of: + # https://www.sqlite.org/c3ref/column_blob.html + + # the "id" column is NOT NULL PRIMARY KEY so an out-of-memory error can be + # inferred from a null pointer result + if text.isNil: + let + code = sqlite3_errcode(sqlite3_db_handle(s)) + + raise (ref Defect)(msg: $sqlite3_errstr(code)) + + $text.cstring + +proc dataCol*( + s: RawStmtPtr, + index = dataColIndex): BoundDataCol = + + let + i = index.cint + colName = sqlite3_column_origin_name(s, i) + + if colName.isNil or $colName != dataColName: + raise (ref Defect)(msg: "original column name for index " & $index & + " was not \"" & dataColName & "\"") + + return proc (): seq[byte] = + let + blob = sqlite3_column_blob(s, i) + + # detect out-of-memory error + # see the conversion table and final paragraph of: + # https://www.sqlite.org/c3ref/column_blob.html + # see also https://www.sqlite.org/rescode.html + + # the "data" column can be NULL so in order to detect an out-of-memory error + # it is necessary to check that the result is a null pointer and that the + # result code is an error code + if blob.isNil: + let + code = sqlite3_errcode(sqlite3_db_handle(s)) + + if not (code in [SQLITE_OK, SQLITE_ROW, SQLITE_DONE]): + raise (ref Defect)(msg: $sqlite3_errstr(code)) + + let + dataLen = sqlite3_column_bytes(s, i) + + # an out-of-memory error can be inferred from a null pointer result + if (unsafeAddr dataLen).isNil: + let + code = sqlite3_errcode(sqlite3_db_handle(s)) + + raise (ref Defect)(msg: $sqlite3_errstr(code)) + + let + dataBytes = cast[ptr UncheckedArray[byte]](blob) + + @(toOpenArray(dataBytes, 0, dataLen - 1)) + +proc timestampCol*( + s: RawStmtPtr, + index = timestampColIndex): BoundTimestampCol = + + let + i = index.cint + colName = sqlite3_column_origin_name(s, i) + + if colName.isNil or $colName != timestampColName: + raise (ref Defect)(msg: "original column name for index " & $index & + " was not \"" & timestampColName & "\"") + + return proc (): int64 = + sqlite3_column_int64(s, i) + proc new*( T: type SQLiteDatastore, basePath = "data", @@ -157,9 +265,12 @@ proc new*( # `pepare()` will fail and `new` will return an error with message # "SQL logic error" + let + getDataCol = dataCol(RawStmtPtr(getStmt), 0) + success T(dbPath: dbPath, containsStmt: containsStmt, deleteStmt: deleteStmt, - env: env.release, getStmt: getStmt, putStmt: putStmt, - readOnly: readOnly) + env: env.release, getStmt: getStmt, getDataCol: getDataCol, + putStmt: putStmt, readOnly: readOnly) proc dbPath*(self: SQLiteDatastore): string = self.dbPath @@ -181,71 +292,6 @@ proc close*(self: SQLiteDatastore) = proc timestamp*(t = epochTime()): int64 = (t * 1_000_000).int64 -proc idCol*( - s: RawStmtPtr, - index = 0): string = - - let - text = sqlite3_column_text(s, index.cint).cstring - - # detect out-of-memory error - # see the conversion table and final paragraph of: - # https://www.sqlite.org/c3ref/column_blob.html - - # the "id" column is NOT NULL PRIMARY KEY so an out-of-memory error can be - # inferred from a null pointer result - if text.isNil: - let - code = sqlite3_errcode(sqlite3_db_handle(s)) - - raise (ref Defect)(msg: $sqlite3_errstr(code)) - - $text - -proc dataCol*( - s: RawStmtPtr, - index = 1): seq[byte] = - - let - i = index.cint - blob = sqlite3_column_blob(s, i) - - # detect out-of-memory error - # see the conversion table and final paragraph of: - # https://www.sqlite.org/c3ref/column_blob.html - # see also https://www.sqlite.org/rescode.html - - # the "data" column can be NULL so in order to detect an out-of-memory error - # it is necessary to check that the result is a null pointer and that the - # result code is an error code - if blob.isNil: - let - code = sqlite3_errcode(sqlite3_db_handle(s)) - - if not (code in [SQLITE_OK, SQLITE_ROW, SQLITE_DONE]): - raise (ref Defect)(msg: $sqlite3_errstr(code)) - - let - dataLen = sqlite3_column_bytes(s, i) - - # an out-of-memory error can be inferred from a null pointer result - if (unsafeAddr dataLen).isNil: - let - code = sqlite3_errcode(sqlite3_db_handle(s)) - - raise (ref Defect)(msg: $sqlite3_errstr(code)) - - let - dataBytes = cast[ptr UncheckedArray[byte]](blob) - - @(toOpenArray(dataBytes, 0, dataLen - 1)) - -proc timestampCol*( - s: RawStmtPtr, - index = 2): int64 = - - sqlite3_column_int64(s, index.cint) - method contains*( self: SQLiteDatastore, key: Key): Future[?!bool] {.async, locks: "unknown".} = @@ -253,7 +299,7 @@ method contains*( var exists = false - proc onData(s: RawStmtPtr) {.closure.} = + proc onData(s: RawStmtPtr) = exists = sqlite3_column_int64(s, 0).bool let @@ -283,8 +329,11 @@ method get*( var bytes: ?seq[byte] - proc onData(s: RawStmtPtr) {.closure.} = - bytes = dataCol(s, 0).some + let + dataCol = self.getDataCol + + proc onData(s: RawStmtPtr) = + bytes = dataCol().some let queryRes = self.getStmt.query((key.id), onData) diff --git a/tests/datastore/test_sqlite_datastore.nim b/tests/datastore/test_sqlite_datastore.nim index ce45fb7..be7b750 100644 --- a/tests/datastore/test_sqlite_datastore.nim +++ b/tests/datastore/test_sqlite_datastore.nim @@ -136,7 +136,17 @@ suite "SQLiteDatastore": check: putRes.isOk let - query = "SELECT * FROM " & tableTitle & ";" + prequeryRes = NoParamsStmt.prepare( + ds.env, "SELECT timestamp as foo, id as baz, data as bar FROM " & + tableName & ";") + + assert prequeryRes.isOk + + let + prequery = prequeryRes.get + idCol = idCol(RawStmtPtr(prequery), 1) + dataCol = dataCol(RawStmtPtr(prequery), 2) + timestampCol = timestampCol(RawStmtPtr(prequery), 0) var qId: string @@ -145,13 +155,13 @@ suite "SQLiteDatastore": rowCount = 0 proc onData(s: RawStmtPtr) {.closure.} = - qId = idCol(s) - qData = dataCol(s) - qTimestamp = timestampCol(s) + qId = idCol() + qData = dataCol() + qTimestamp = timestampCol() inc rowCount var - qRes = ds.env.query(query, onData) + qRes = prequery.query((), onData) assert qRes.isOk @@ -169,7 +179,7 @@ suite "SQLiteDatastore": check: putRes.isOk rowCount = 0 - qRes = ds.env.query(query, onData) + qRes = prequery.query((), onData) assert qRes.isOk check: @@ -186,7 +196,7 @@ suite "SQLiteDatastore": check: putRes.isOk rowCount = 0 - qRes = ds.env.query(query, onData) + qRes = prequery.query((), onData) assert qRes.isOk check: @@ -196,6 +206,8 @@ suite "SQLiteDatastore": qTimestamp == timestamp rowCount == 1 + prequery.dispose + asyncTest "delete": let bytes = @[1.byte, 2.byte, 3.byte] @@ -225,7 +237,7 @@ suite "SQLiteDatastore": assert putRes.isOk let - query = "SELECT * FROM " & tableTitle & ";" + query = "SELECT * FROM " & tableName & ";" var rowCount = 0