From 9d49c8016e6760bbc43418e716fc8e0b73665537 Mon Sep 17 00:00:00 2001 From: Dmitriy Ryajov Date: Tue, 22 Nov 2022 15:23:23 -0600 Subject: [PATCH] Several fixes and missing features (#38) * shorten lines * only return data when `query.value == true` * test `query.value = false` * close mounted ds * allow passing dispose to query iter constructor * fix fs querying * use currentSourcePath * remove `dsobj` extensions from directories * don't return error on missing key delete * return `DatastoreKeyNotFound` on empty `get` * return `DatastoreKeyNotFound` on missing sql key --- datastore/fsds.nim | 48 +++++++++++---- datastore/mountedds.nim | 7 +++ datastore/query.nim | 16 ++--- datastore/sql/sqliteds.nim | 37 +++++++---- datastore/sql/sqlitedsdb.nim | 7 ++- tests/datastore/querycommontests.nim | 85 ++++++++++++++++++-------- tests/datastore/sql/testsqliteds.nim | 2 +- tests/datastore/sql/testsqlitedsdb.nim | 4 +- tests/datastore/testfsds.nim | 6 +- tests/datastore/testkey.nim | 10 ++- tests/datastore/testmountedds.nim | 2 +- tests/datastore/testtieredds.nim | 12 ++-- 12 files changed, 161 insertions(+), 75 deletions(-) diff --git a/datastore/fsds.nim b/datastore/fsds.nim index 849bd35..cf39db4 100644 --- a/datastore/fsds.nim +++ b/datastore/fsds.nim @@ -29,6 +29,7 @@ proc isRootSubdir*(self: FSDatastore, path: string): bool = proc path*(self: FSDatastore, key: Key): ?!string = ## Return filename corresponding to the key ## or failure if the key doesn't correspond to a valid filename + ## if not self.validDepth(key): return failure "Path has invalid depth!" @@ -38,21 +39,25 @@ proc path*(self: FSDatastore, key: Key): ?!string = for ns in key: let basename = ns.value.extractFilename - if basename=="" or not basename.isValidFilename: + if basename == "" or not basename.isValidFilename: return failure "Filename contains invalid chars!" if ns.field == "": segments.add(ns.value) else: let basename = ns.field.extractFilename - if basename=="" or not basename.isValidFilename: + if basename == "" or not basename.isValidFilename: return failure "Filename contains invalid chars!" # `:` are replaced with `/` segments.add(ns.field / ns.value) let - fullname = (self.root / segments.joinPath()).absolutePath().catch().get().addFileExt(FileExt) + fullname = (self.root / segments.joinPath()) + .absolutePath() + .catch() + .get() + .addFileExt(FileExt) if not self.isRootSubdir(fullname): return failure "Path is outside of `root` directory!" @@ -67,14 +72,15 @@ method delete*(self: FSDatastore, key: Key): Future[?!void] {.async.} = return failure error if not path.fileExists(): - return failure newException(DatastoreKeyNotFound, "Key not found!") + return success() try: removeFile(path) - return success() except OSError as e: return failure e + return success() + method delete*(self: FSDatastore, keys: seq[Key]): Future[?!void] {.async.} = for key in keys: if err =? (await self.delete(key)).errorOption: @@ -117,14 +123,15 @@ method get*(self: FSDatastore, key: Key): Future[?!seq[byte]] {.async.} = return failure error if not path.fileExists(): - return failure(newException(DatastoreKeyNotFound, "Key doesn't exist")) + return failure( + newException(DatastoreKeyNotFound, "Key doesn't exist")) return self.readFile(path) method put*( self: FSDatastore, key: Key, - data: seq[byte]): Future[?!void] {.async, locks: "unknown".} = + data: seq[byte]): Future[?!void] {.async.} = without path =? self.path(key), error: return failure error @@ -139,7 +146,7 @@ method put*( method put*( self: FSDatastore, - batch: seq[BatchEntry]): Future[?!void] {.async, locks: "unknown".} = + batch: seq[BatchEntry]): Future[?!void] {.async.} = for entry in batch: if err =? (await self.put(entry.key, entry.data)).errorOption: @@ -155,12 +162,26 @@ proc dirWalker(path: string): iterator: string {.gcsafe.} = except CatchableError as exc: raise newException(Defect, exc.msg) +method close*(self: FSDatastore): Future[?!void] {.async.} = + return success() + method query*( self: FSDatastore, query: Query): Future[?!QueryIter] {.async.} = - without basePath =? self.path(query.key).?parentDir, error: + without path =? self.path(query.key), error: return failure error + + let basePath = + # it there is a file in the directory + # with the same name then list the contents + # of the directory, otherwise recurse + # into subdirectories + if path.fileExists: + path.parentDir + else: + path.changeFileExt("") + let walker = dirWalker(basePath) @@ -175,9 +196,6 @@ method query*( iter.finished = true return success (Key.none, EmptyBytes) - without data =? self.readFile((basePath / path).absolutePath), err: - return failure err - var keyPath = basePath @@ -187,6 +205,12 @@ method query*( let key = Key.init(keyPath).expect("should not fail") + data = + if query.value: + self.readFile((basePath / path).absolutePath) + .expect("Should read file") + else: + @[] return success (key.some, data) diff --git a/datastore/mountedds.nim b/datastore/mountedds.nim index 1a2eb66..db51104 100644 --- a/datastore/mountedds.nim +++ b/datastore/mountedds.nim @@ -119,6 +119,13 @@ method put*( return success() +method close*(self: MountedDatastore): Future[?!void] {.async.} = + for s in self.stores.values: + discard await s.store.close() + + # TODO: how to handle failed close? + return success() + func new*( T: type MountedDatastore, stores: Table[Key, Datastore] = initTable[Key, Datastore]()): ?!T = diff --git a/datastore/query.nim b/datastore/query.nim index cecbdd2..1c51a6e 100644 --- a/datastore/query.nim +++ b/datastore/query.nim @@ -12,11 +12,11 @@ type Descending Query* = object - key*: Key - value*: bool - limit*: int - offset*: int - sort*: SortOrder + key*: Key # Key to be queried + value*: bool # Flag to indicate if data should be returned + limit*: int # Max items to return - not available in all backends + offset*: int # Offset from which to start querying - not available in all backends + sort*: SortOrder # Sort order - not available in all backends QueryResponse* = tuple[key: ?Key, data: seq[byte]] QueryEndedError* = object of DatastoreError @@ -35,13 +35,13 @@ iterator items*(q: QueryIter): Future[?!QueryResponse] = proc defaultDispose(): Future[?!void] {.upraises: [], gcsafe, async.} = return success() -proc new*(T: type QueryIter): T = - QueryIter(dispose: defaultDispose) +proc new*(T: type QueryIter, dispose = defaultDispose): T = + QueryIter(dispose: dispose) proc init*( T: type Query, key: Key, - value = false, + value = true, sort = SortOrder.Assending, offset = 0, limit = -1): T = diff --git a/datastore/sql/sqliteds.nim b/datastore/sql/sqliteds.nim index 00a739b..fdadeb6 100644 --- a/datastore/sql/sqliteds.nim +++ b/datastore/sql/sqliteds.nim @@ -1,4 +1,5 @@ import std/times +import std/options import pkg/chronos import pkg/questionable @@ -47,7 +48,7 @@ method delete*(self: SQLiteDatastore, key: Key): Future[?!void] {.async.} = method delete*(self: SQLiteDatastore, keys: seq[Key]): Future[?!void] {.async.} = if err =? self.db.beginStmt.exec().errorOption: - return failure err.msg + return failure(err) for key in keys: if err =? self.db.deleteStmt.exec((key.id)).errorOption: @@ -72,10 +73,12 @@ method get*(self: SQLiteDatastore, key: Key): Future[?!seq[byte]] {.async.} = proc onData(s: RawStmtPtr) = bytes = self.db.getDataCol() - if ( - let res = self.db.getStmt.query((key.id), onData); - res.isErr): - return failure res.error.msg + if err =? self.db.getStmt.query((key.id), onData).errorOption: + return failure(err) + + if bytes.len <= 0: + return failure( + newException(DatastoreKeyNotFound, "Key doesn't exist")) return success bytes @@ -109,7 +112,10 @@ method query*( var iter = QueryIter() - queryStr = QueryStmtStr + queryStr = if query.value: + QueryStmtDataIdStr + else: + QueryStmtIdStr if query.sort == SortOrder.Descending: queryStr &= QueryStmtOrderDescending @@ -158,10 +164,14 @@ method query*( of SQLITE_ROW: let key = Key.init( - $sqlite3_column_text_not_null(s,QueryStmtIdCol)) + $sqlite3_column_text_not_null(s, QueryStmtIdCol)) .expect("should not fail") - blob = sqlite3_column_blob(s, QueryStmtDataCol) + blob: ?pointer = + if query.value: + sqlite3_column_blob(s, QueryStmtDataCol).some + else: + pointer.none # detect out-of-memory error # see the conversion table and final paragraph of: @@ -171,7 +181,7 @@ method query*( # 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: + if blob.isSome and blob.get().isNil: let v = sqlite3_errcode(sqlite3_db_handle(s)) @@ -181,8 +191,13 @@ method query*( let dataLen = sqlite3_column_bytes(s, QueryStmtDataCol) - dataBytes = cast[ptr UncheckedArray[byte]](blob) - data = @(toOpenArray(dataBytes, 0, dataLen - 1)) + data = if blob.isSome: + @( + toOpenArray(cast[ptr UncheckedArray[byte]](blob.get), + 0, + dataLen - 1)) + else: + @[] return success (key.some, data) of SQLITE_DONE: diff --git a/datastore/sql/sqlitedsdb.nim b/datastore/sql/sqlitedsdb.nim index bc00b2f..503dea4 100644 --- a/datastore/sql/sqlitedsdb.nim +++ b/datastore/sql/sqlitedsdb.nim @@ -93,7 +93,12 @@ const ) VALUES (?, ?, ?) """ - QueryStmtStr* = """ + QueryStmtIdStr* = """ + SELECT """ & IdColName & """ FROM """ & TableName & + """ WHERE """ & IdColName & """ GLOB ? + """ + + QueryStmtDataIdStr* = """ SELECT """ & IdColName & """, """ & DataColName & """ FROM """ & TableName & """ WHERE """ & IdColName & """ GLOB ? """ diff --git a/tests/datastore/querycommontests.nim b/tests/datastore/querycommontests.nim index c4b7caa..4f0a15d 100644 --- a/tests/datastore/querycommontests.nim +++ b/tests/datastore/querycommontests.nim @@ -10,15 +10,24 @@ import pkg/stew/byteutils import pkg/datastore template queryTests*(ds: Datastore, extended = true) {.dirty.} = - test "Key should query all key and all it's children": - let - key1 = Key.init("/a").tryGet - key2 = Key.init("/a/b").tryGet - key3 = Key.init("/a/b/c").tryGet - val1 = "value for 1".toBytes - val2 = "value for 2".toBytes - val3 = "value for 3".toBytes + var + key1: Key + key2: Key + key3: Key + val1: seq[byte] + val2: seq[byte] + val3: seq[byte] + setupAll: + key1 = Key.init("/a").tryGet + key2 = Key.init("/a/b").tryGet + key3 = Key.init("/a/b/c").tryGet + val1 = "value for 1".toBytes + val2 = "value for 2".toBytes + val3 = "value for 3".toBytes + + test "Key should query all keys and all it's children": + let q = Query.init(key1) (await ds.put(key1, val1)).tryGet @@ -44,15 +53,35 @@ template queryTests*(ds: Datastore, extended = true) {.dirty.} = (await iter.dispose()).tryGet + test "Key should query all keys without values": + let + q = Query.init(key1, value = false) + + (await ds.put(key1, val1)).tryGet + (await ds.put(key2, val2)).tryGet + (await ds.put(key3, val3)).tryGet + + let + iter = (await ds.query(q)).tryGet + res = (await allFinished(toSeq(iter))) + .mapIt( it.read.tryGet ) + .filterIt( it.key.isSome ) + + check: + res.len == 3 + res[0].key.get == key1 + res[0].data.len == 0 + + res[1].key.get == key2 + res[1].data.len == 0 + + res[2].key.get == key3 + res[2].data.len == 0 + + (await iter.dispose()).tryGet + test "Key should not query parent": let - key1 = Key.init("/a").tryGet - key2 = Key.init("/a/b").tryGet - key3 = Key.init("/a/b/c").tryGet - val1 = "value for 1".toBytes - val2 = "value for 2".toBytes - val3 = "value for 3".toBytes - q = Query.init(key2) (await ds.put(key1, val1)).tryGet @@ -78,13 +107,6 @@ template queryTests*(ds: Datastore, extended = true) {.dirty.} = test "Key should all list all keys at the same level": let queryKey = Key.init("/a").tryGet - key1 = Key.init("/a/1").tryGet - key2 = Key.init("/a/2").tryGet - key3 = Key.init("/a/3").tryGet - val1 = "value for 1".toBytes - val2 = "value for 2".toBytes - val3 = "value for 3".toBytes - q = Query.init(queryKey) (await ds.put(key1, val1)).tryGet @@ -117,13 +139,16 @@ template queryTests*(ds: Datastore, extended = true) {.dirty.} = if extended: test "Should apply limit": - let key = Key.init("/a").tryGet q = Query.init(key, limit = 10) for i in 0..<100: - (await ds.put(Key.init(key, Key.init("/" & $i).tryGet).tryGet, ("val " & $i).toBytes)).tryGet + let + key = Key.init(key, Key.init("/" & $i).tryGet).tryGet + val = ("val " & $i).toBytes + + (await ds.put(key, val)).tryGet let iter = (await ds.query(q)).tryGet @@ -142,7 +167,11 @@ template queryTests*(ds: Datastore, extended = true) {.dirty.} = q = Query.init(key, offset = 90) for i in 0..<100: - (await ds.put(Key.init(key, Key.init("/" & $i).tryGet).tryGet, ("val " & $i).toBytes)).tryGet + let + key = Key.init(key, Key.init("/" & $i).tryGet).tryGet + val = ("val " & $i).toBytes + + (await ds.put(key, val)).tryGet let iter = (await ds.query(q)).tryGet @@ -161,7 +190,11 @@ template queryTests*(ds: Datastore, extended = true) {.dirty.} = q = Query.init(key, offset = 95, limit = 5) for i in 0..<100: - (await ds.put(Key.init(key, Key.init("/" & $i).tryGet).tryGet, ("val " & $i).toBytes)).tryGet + let + key = Key.init(key, Key.init("/" & $i).tryGet).tryGet + val = ("val " & $i).toBytes + + (await ds.put(key, val)).tryGet let iter = (await ds.query(q)).tryGet diff --git a/tests/datastore/sql/testsqliteds.nim b/tests/datastore/sql/testsqliteds.nim index c13432a..ba7afe9 100644 --- a/tests/datastore/sql/testsqliteds.nim +++ b/tests/datastore/sql/testsqliteds.nim @@ -27,7 +27,7 @@ suite "Test Basic SQLiteDatastore": suite "Test Read Only SQLiteDatastore": let - (path, _, _) = instantiationInfo(-1, fullPaths = true) # get this file's name + path = currentSourcePath() # get this file's name basePath = "tests_data" basePathAbs = path.parentDir / basePath filename = "test_store" & DbExt diff --git a/tests/datastore/sql/testsqlitedsdb.nim b/tests/datastore/sql/testsqlitedsdb.nim index da6e864..d104933 100644 --- a/tests/datastore/sql/testsqlitedsdb.nim +++ b/tests/datastore/sql/testsqlitedsdb.nim @@ -11,7 +11,7 @@ import pkg/datastore/sql/sqliteds suite "Test Open SQLite Datastore DB": let - (path, _, _) = instantiationInfo(-1, fullPaths = true) # get this file's name + path = currentSourcePath() # get this file's name basePath = "tests_data" basePathAbs = path.parentDir / basePath filename = "test_store" & DbExt @@ -70,7 +70,7 @@ suite "Test Open SQLite Datastore DB": suite "Test SQLite Datastore DB operations": let - (path, _, _) = instantiationInfo(-1, fullPaths = true) # get this file's name + path = currentSourcePath() # get this file's name basePath = "tests_data" basePathAbs = path.parentDir / basePath filename = "test_store" & DbExt diff --git a/tests/datastore/testfsds.nim b/tests/datastore/testfsds.nim index cd81291..900bf11 100644 --- a/tests/datastore/testfsds.nim +++ b/tests/datastore/testfsds.nim @@ -15,7 +15,7 @@ import ./querycommontests suite "Test Basic FSDatastore": let - (path, _, _) = instantiationInfo(-1, fullPaths = true) # get this file's name + path = currentSourcePath() # get this file's name basePath = "tests_data" basePathAbs = path.parentDir / basePath key = Key.init("/a/b").tryGet() @@ -40,7 +40,7 @@ suite "Test Basic FSDatastore": suite "Test Misc FSDatastore": let - (path, _, _) = instantiationInfo(-1, fullPaths = true) # get this file's name + path = currentSourcePath() # get this file's name basePath = "tests_data" basePathAbs = path.parentDir / basePath bytes = "some bytes".toBytes @@ -116,7 +116,7 @@ suite "Test Misc FSDatastore": suite "Test Query": let - (path, _, _) = instantiationInfo(-1, fullPaths = true) # get this file's name + path = currentSourcePath() # get this file's name basePath = "tests_data" basePathAbs = path.parentDir / basePath diff --git a/tests/datastore/testkey.nim b/tests/datastore/testkey.nim index d5fd2fd..e974a3a 100644 --- a/tests/datastore/testkey.nim +++ b/tests/datastore/testkey.nim @@ -145,10 +145,16 @@ suite "Key": test "should equal": check: - Key.init(Namespace.init("a:b").tryGet(), Namespace.init("c").tryGet()).tryGet() == Key.init("a:b/c").tryGet() + Key.init( + Namespace.init("a:b").tryGet(), + Namespace.init("c").tryGet()).tryGet() == Key.init("a:b/c").tryGet() + Key.init("a:b", "c").tryGet() == Key.init("a:b/c").tryGet() Key.init("a:b/c").tryGet() == Key.init("a:b/c").tryGet() - Key.init(Namespace.init("a:b").tryGet(), Namespace.init("c").tryGet()).tryGet() != Key.init("c:b/a").tryGet() + Key.init( + Namespace.init("a:b").tryGet(), + Namespace.init("c").tryGet()).tryGet() != Key.init("c:b/a").tryGet() + Key.init("a:b/c").tryGet() == Key.init("/a:b/c/").tryGet() Key.init("a:b/c").tryGet() == Key.init("///a:b///c///").tryGet() Key.init("a:X/b/c").tryGet() == Key.init("a:X/b/c").tryGet() diff --git a/tests/datastore/testmountedds.nim b/tests/datastore/testmountedds.nim index d58b910..fbe0dfd 100644 --- a/tests/datastore/testmountedds.nim +++ b/tests/datastore/testmountedds.nim @@ -16,7 +16,7 @@ import ./dscommontests suite "Test Basic Mounted Datastore": let root = "tests" / "test_data" - (path, _, _) = instantiationInfo(-1, fullPaths = true) # get this file's name + path = currentSourcePath() # get this file's name rootAbs = path.parentDir / root key = Key.init("a:b/c/d:e").get diff --git a/tests/datastore/testtieredds.nim b/tests/datastore/testtieredds.nim index 956fe3a..9b643b3 100644 --- a/tests/datastore/testtieredds.nim +++ b/tests/datastore/testtieredds.nim @@ -18,7 +18,7 @@ suite "Test Basic Tired Datastore": otherBytes = "some other bytes".toBytes key = Key.init("a:b/c/d:e").get root = "tests" / "test_data" - (path, _, _) = instantiationInfo(-1, fullPaths = true) # get this file's name + path = currentSourcePath() # get this file's name rootAbs = path.parentDir / root var @@ -47,7 +47,7 @@ suite "TieredDatastore": bytes = @[1.byte, 2.byte, 3.byte] key = Key.init("a:b/c/d:e").get root = "tests" / "test_data" - (path, _, _) = instantiationInfo(-1, fullPaths = true) # get this file's name + path = currentSourcePath() # get this file's name rootAbs = path.parentDir / root var @@ -105,8 +105,8 @@ suite "TieredDatastore": (await ds.put(key, bytes)).tryGet (await ds.delete(key)).tryGet - check: - (await ds1.get(key)).tryGet.len == 0 + expect DatastoreKeyNotFound: + discard (await ds1.get(key)).tryGet expect DatastoreKeyNotFound: discard (await ds2.get(key)).tryGet @@ -153,7 +153,3 @@ suite "TieredDatastore": (await ds2.get(key)).tryGet == bytes (await ds.get(key)).tryGet == bytes (await ds1.get(key)).tryGet == bytes - - # # test "query": - # # check: - # # true