Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a failable iterator when selecting records #1917

Merged
merged 2 commits into from
Aug 26, 2021
Merged

Use a failable iterator when selecting records #1917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9a2b25
Use a failable iterator when selecting records
calvincestari Aug 23, 2021
ed2eef3
Add test to verify failable iterator behaviour
calvincestari Aug 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Apollo.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@
DED46042261CEA8A0086EF63 /* TestServerURLs.swift in Sources */ = {isa = PBXBuildFile; fileRef = DED45C172615308E0086EF63 /* TestServerURLs.swift */; };
DED46051261CEAD20086EF63 /* StarWarsAPI.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 9FCE2CFA1E6C213D00E34457 /* StarWarsAPI.framework */; };
E616B6D126C3335600DB049E /* ExecutionTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E616B6D026C3335600DB049E /* ExecutionTests.swift */; };
E61DD76526D60C1800C41614 /* SQLiteDotSwiftDatabaseBehaviorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E61DD76426D60C1800C41614 /* SQLiteDotSwiftDatabaseBehaviorTests.swift */; };
E86D8E05214B32FD0028EFE1 /* JSONTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E86D8E03214B32DA0028EFE1 /* JSONTests.swift */; };
F16D083C21EF6F7300C458B8 /* QueryFromJSONBuildingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F16D083B21EF6F7300C458B8 /* QueryFromJSONBuildingTests.swift */; };
F82E62E122BCD223000C311B /* AutomaticPersistedQueriesTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F82E62E022BCD223000C311B /* AutomaticPersistedQueriesTests.swift */; };
Expand Down Expand Up @@ -804,6 +805,7 @@
DED45FB2261CDE980086EF63 /* Apollo-CITestPlan.xctestplan */ = {isa = PBXFileReference; lastKnownFileType = text; path = "Apollo-CITestPlan.xctestplan"; sourceTree = "<group>"; };
DED45FB3261CDEC60086EF63 /* Apollo-CodegenTestPlan.xctestplan */ = {isa = PBXFileReference; lastKnownFileType = text; path = "Apollo-CodegenTestPlan.xctestplan"; sourceTree = "<group>"; };
E616B6D026C3335600DB049E /* ExecutionTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ExecutionTests.swift; sourceTree = "<group>"; };
E61DD76426D60C1800C41614 /* SQLiteDotSwiftDatabaseBehaviorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SQLiteDotSwiftDatabaseBehaviorTests.swift; sourceTree = "<group>"; };
E86D8E03214B32DA0028EFE1 /* JSONTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = JSONTests.swift; sourceTree = "<group>"; };
F16D083B21EF6F7300C458B8 /* QueryFromJSONBuildingTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = QueryFromJSONBuildingTests.swift; sourceTree = "<group>"; };
F82E62E022BCD223000C311B /* AutomaticPersistedQueriesTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = AutomaticPersistedQueriesTests.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1489,6 +1491,7 @@
9FF90A6B1DDDEB420034C3B6 /* ReadFieldValueTests.swift */,
C338DF1622DD9DE9006AF33E /* RequestBodyCreatorTests.swift */,
9B96500824BE6201003C29C0 /* RequestChainTests.swift */,
E61DD76426D60C1800C41614 /* SQLiteDotSwiftDatabaseBehaviorTests.swift */,
9B9BBB1A24DB75E60021C30F /* UploadRequestTests.swift */,
5BB2C0222380836100774170 /* VersionNumberTests.swift */,
);
Expand Down Expand Up @@ -2618,6 +2621,7 @@
9F91CF8F1F6C0DB2008DD0BE /* MutatingResultsTests.swift in Sources */,
E616B6D126C3335600DB049E /* ExecutionTests.swift in Sources */,
9B9BBB1C24DB760B0021C30F /* UploadRequestTests.swift in Sources */,
E61DD76526D60C1800C41614 /* SQLiteDotSwiftDatabaseBehaviorTests.swift in Sources */,
9BC139A424EDCA6C00876D29 /* InterceptorTests.swift in Sources */,
F82E62E122BCD223000C311B /* AutomaticPersistedQueriesTests.swift in Sources */,
9BC139A824EDCE4F00876D29 /* RetryToCountThenSucceedInterceptor.swift in Sources */,
Expand Down
2 changes: 1 addition & 1 deletion Sources/ApolloSQLite/SQLiteDotSwiftDatabase.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public final class SQLiteDotSwiftDatabase: SQLiteDatabase {

public func selectRawRows(forKeys keys: Set<CacheKey>) throws -> [DatabaseRow] {
let query = self.records.filter(keys.contains(keyColumn))
return try self.db.prepare(query).map { row in
return try self.db.prepareRowIterator(query).map { row in
let record = row[self.recordColumn]
let key = row[self.keyColumn]

Expand Down
26 changes: 26 additions & 0 deletions Tests/ApolloTests/SQLiteDotSwiftDatabaseBehaviorTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import XCTest
@testable import ApolloSQLite
import ApolloTestSupport
import SQLite

class SQLiteDotSwiftDatabaseBehaviorTests: XCTestCase {

func testSelection_withForcedError_shouldThrow() throws {
Copy link
Member Author

@calvincestari calvincestari Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnthonyMDev is this what you had in mind for a test? Directly calling selectRawRows(forKeys:) is as close to the change as we can get. The table we use is extremely basic so I don't think it's possible to force an error through data without doing something more drastic. So I chose to drop the table instead of the nuclear option like deleting the database.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so! Let's just verify by making sure this test fails once we update to SQLite 13.0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd have to cherry-pick the test into a branch that doesn't contain the change to use prepareRowIterator - but yes, good validation once we move.

Copy link
Member Author

@calvincestari calvincestari Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prove that this change works I created a branch that:

  1. Bumped the SQLite.swift dependency version to 0.13.0 which contains the iterator behaviour change (from this)
  2. cherry-picked the test from this PR
  3. Pushed to GitHub to trigger a CI build

The CI build failed as expected which proves the hypothesis that if this change were to go out in a 0.12.3 release then developers using apollo-ios would get silently changed behaviour.

I'm not sure how long CI builds are kept for so for prosperity the screenshot below is from the failed CI build
Screen Shot 2021-08-26 at 1 50 11 PM

And all four of those failed steps are failing with the same error
Screen Shot 2021-08-26 at 1 52 24 PM

let sqliteFileURL = SQLiteTestCacheProvider.temporarySQLiteFileURL()
let db = try! SQLiteDotSwiftDatabase(fileURL: sqliteFileURL)

try! db.createRecordsTableIfNeeded()
try! db.addOrUpdateRecordString("record", for: "key")

var rows = [DatabaseRow]()
XCTAssertNoThrow(rows = try db.selectRawRows(forKeys: ["key"]))
XCTAssertEqual(rows.count, 1)

// Use SQLite directly to manipulate the database (cannot be done with SQLiteDotSwiftDatabase)
let connection = try Connection(.uri(sqliteFileURL.absoluteString), readonly: false)
let table = Table(SQLiteDotSwiftDatabase.tableName)
try! connection.run(table.drop(ifExists: false))

XCTAssertThrowsError(try db.selectRawRows(forKeys: ["key"]))
}
}