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.

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

Conversation

calvincestari
Copy link
Member

@calvincestari calvincestari commented Aug 23, 2021

The prepare function being used in selectRawRows(forKeys:) is changing behaviour such that errors during iteration would be hidden. We can keep the expected error-on-iteration behaviour by switching to a row iterator that supports failure. This PR changes the internals of selectRawRows(forKeys:) to use prepareRowIterator, returning a RowIterator which conforms to FailableIterator therefore any error thrown would still be propagated out to the caller.

This PR does not move the dependency version of SQLite.swift to 0.13.0

  • It requires apollo-ios to move to a minimum deployment target of 10.15 for macOS
  • There are multiple deprecations in the change to target 10.15 that should be addressed separately
  • Once the deprecations are handled moving to SQLite.swift 0.13.0 will be seamless

@calvincestari
Copy link
Member Author

calvincestari commented Aug 23, 2021

I don't think there is any test required for this change. I initially wanted to ensure that RowIterator.map uses failableNext internally which throws errors but that's both the wrong thing to test and this is the wrong place for that test.

I added testSelection_withForcedError_shouldThrow.

@calvincestari calvincestari marked this pull request as draft August 26, 2021 15:20

class SQLiteDotSwiftDatabaseBehaviorTests: XCTestCase {

func testSelection_withForcedError_shouldThrow() throws {
Copy link
Member Author

@calvincestari calvincestari Aug 26, 2021

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

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

@calvincestari calvincestari marked this pull request as ready for review August 26, 2021 19:00
Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

Thanks for this Calvin!

@calvincestari calvincestari merged commit 45d382d into main Aug 26, 2021
@calvincestari calvincestari deleted the sqlite-failable-iterator branch August 26, 2021 20:59
@calvincestari calvincestari mentioned this pull request Sep 2, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants