From 424eb42f37dfac68960bd3114ecbc04d737ba647 Mon Sep 17 00:00:00 2001 From: Dan Date: Thu, 12 Oct 2023 10:23:31 -0600 Subject: [PATCH 1/2] Add tests which show the failing behavior The SQLiteStorageEngineTest succeeds, because its implementation of `readDataAndKeys` does not rely on the fault logic inherited from the default implementation on `StorageEngine` The DiskStorageEngineTest fails. Only five keys exist in the database (`0`, `1`, `2`, `3`, and `4`). However, 10 get requested in reverse order. The faulty logic means that key `9` gets the data associated with key `4`, key `8` with key `3`, and so on down to key `5` getting the data meant for key `0`. This means that comparing the returned _keys_ fails, but the comparing the returned _data_ succeeds. The ObjectStorageTest fails, for similar reasons. It is also using `zip()` to construct the returned array, blindly assuming that the object retrieved for each key exists. --- Tests/BodegaTests/DiskStorageEngineTests.swift | 13 +++++++++++++ Tests/BodegaTests/ObjectStorageTests.swift | 13 +++++++++++++ Tests/BodegaTests/SQLiteStorageEngineTests.swift | 13 +++++++++++++ 3 files changed, 39 insertions(+) diff --git a/Tests/BodegaTests/DiskStorageEngineTests.swift b/Tests/BodegaTests/DiskStorageEngineTests.swift index 31941e3..b52d288 100644 --- a/Tests/BodegaTests/DiskStorageEngineTests.swift +++ b/Tests/BodegaTests/DiskStorageEngineTests.swift @@ -84,6 +84,19 @@ final class DiskStorageEngineTests: XCTestCase { "Value 9" ]) } + + func testReadingDataAndNonExistentKeys() async throws { + try await self.writeItemsToDisk(count: 5) + let allDataAndKeys = await storage.readAllDataAndKeys() + .sorted(by: { $0.key.value > $1.key.value }) + + let goodAndBadKeys = (0 ..< 10).reversed().map({ CacheKey(verbatim: "\($0)") }) + let validDataAndKeys = await storage.readDataAndKeys(keys: goodAndBadKeys) + .sorted(by: { $0.key.value > $1.key.value }) + + XCTAssertEqual(allDataAndKeys.map(\.key), validDataAndKeys.map(\.key)) + XCTAssertEqual(allDataAndKeys.map(\.data), validDataAndKeys.map(\.data)) + } func testReadingAllDataSucceeds() async throws { try await self.writeItemsToDisk(count: 10) diff --git a/Tests/BodegaTests/ObjectStorageTests.swift b/Tests/BodegaTests/ObjectStorageTests.swift index 8079571..47dc5e9 100644 --- a/Tests/BodegaTests/ObjectStorageTests.swift +++ b/Tests/BodegaTests/ObjectStorageTests.swift @@ -153,6 +153,19 @@ class ObjectStorageTests: XCTestCase { ]) } + func testReadingObjectsAndNonExistentKeys() async throws { + try await self.writeObjectsToDisk(count: 5) + let allDataAndKeys = await storage.allObjectsAndKeys() + .sorted(by: { $0.key.value > $1.key.value }) + + let goodAndBadKeys = (0 ..< 10).reversed().map({ CacheKey(verbatim: "\($0)") }) + let validObjectsAndKeys = await storage.objectsAndKeys(keys: goodAndBadKeys) + .sorted(by: { $0.key.value > $1.key.value }) + + XCTAssertEqual(allDataAndKeys.map(\.key), validObjectsAndKeys.map(\.key)) + XCTAssertEqual(allDataAndKeys.map(\.object), validObjectsAndKeys.map(\.object)) + } + func testReadingAllObjectsSucceeds() async throws { try await self.writeObjectsToDisk(count: 10) diff --git a/Tests/BodegaTests/SQLiteStorageEngineTests.swift b/Tests/BodegaTests/SQLiteStorageEngineTests.swift index 431ee31..6538aa8 100644 --- a/Tests/BodegaTests/SQLiteStorageEngineTests.swift +++ b/Tests/BodegaTests/SQLiteStorageEngineTests.swift @@ -84,6 +84,19 @@ final class SQLiteStorageEngineTests: XCTestCase { "Value 9" ]) } + + func testReadingDataAndNonExistentKeys() async throws { + try await self.writeItemsToDatabase(count: 5) + let allDataAndKeys = await storage.readAllDataAndKeys() + .sorted(by: { $0.key.value > $1.key.value }) + + let goodAndBadKeys = (0 ..< 10).reversed().map({ CacheKey(verbatim: "\($0)") }) + let validDataAndKeys = await storage.readDataAndKeys(keys: goodAndBadKeys) + .sorted(by: { $0.key.value > $1.key.value }) + + XCTAssertEqual(allDataAndKeys.map(\.key), validDataAndKeys.map(\.key)) + XCTAssertEqual(allDataAndKeys.map(\.data), validDataAndKeys.map(\.data)) + } func testReadingAllDataSucceeds() async throws { try await self.writeItemsToDatabase(count: 10) From 2c18369a2999a16d0f12c41e50d62da996542ad5 Mon Sep 17 00:00:00 2001 From: Dan Date: Thu, 12 Oct 2023 10:41:43 -0600 Subject: [PATCH 2/2] Don't return values for non-existent keys Fixes #24 Using `zip()` loses the association between keys and values, because the array returned from `read(keys:)` does not provide information about _skipped_ keys (See #26). --- Sources/Bodega/DiskStorageEngine.swift | 13 +++++++++---- Sources/Bodega/ObjectStorage.swift | 13 +++++++++---- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/Sources/Bodega/DiskStorageEngine.swift b/Sources/Bodega/DiskStorageEngine.swift index 5e73fd5..52c244b 100644 --- a/Sources/Bodega/DiskStorageEngine.swift +++ b/Sources/Bodega/DiskStorageEngine.swift @@ -72,10 +72,15 @@ public actor DiskStorageEngine: StorageEngine { /// - Returns: An array of `[(CacheKey, Data)]` read from disk if the ``CacheKey``s exist, /// and an empty array if there are no `Data` items matching the `keys` passed in. public func readDataAndKeys(keys: [CacheKey]) async -> [(key: CacheKey, data: Data)] { - return zip( - keys, - await self.read(keys: keys) - ).map { ($0, $1) } + var dataAndKeys: [(key: CacheKey, data: Data)] = [] + + for key in keys { + if let data = self.read(key: key) { + dataAndKeys.append((key, data)) + } + } + + return dataAndKeys } /// Reads all the `[Data]` located in the `directory`. diff --git a/Sources/Bodega/ObjectStorage.swift b/Sources/Bodega/ObjectStorage.swift index 0fbc362..3b2b572 100644 --- a/Sources/Bodega/ObjectStorage.swift +++ b/Sources/Bodega/ObjectStorage.swift @@ -84,10 +84,15 @@ public actor ObjectStorage { /// - Returns: An array of `[(CacheKey, Object)]` read if it exists, /// and an empty array if there are no `Objects`s matching the `keys` passed in. public func objectsAndKeys(keys: [CacheKey]) async -> [(key: CacheKey, object: Object)] { - return zip( - keys, - await self.objects(forKeys: keys) - ).map { ($0, $1) } + var objectsAndKeys: [(key: CacheKey, object: Object)] = [] + + for key in keys { + if let object = await self.object(forKey: key) { + objectsAndKeys.append((key, object)) + } + } + + return objectsAndKeys } /// Reads all `[Object]` objects.