fix(analytics): iterate sqlite rows with failableNext #3857
Conversation
| if let event = PinpointEvent.convertToEvent(element) { | ||
| result.append(event) | ||
| do { | ||
| while let element = try rows.failableNext() { |
There was a problem hiding this comment.
Is there a scenario where the next row can fail, but subsequent rows can still be successfully retrieved?
If so, would it better (or even possible) to continue iterating through instead of bailing on the first error thrown?
There was a problem hiding this comment.
hmm, it didn’t fail while processing the iterator's current element. It failed when the iterator tried to get the next element, and there should be no more elements after that failure.
There was a problem hiding this comment.
What's funny is that I was gonna suggest the opposite: If we get an error when attempting to retrieve a subsequent row, we should fail the whole operation instead of just returning whatever results we have so far.
The way I see it is that we're explicitly running a SELECT query that we expect to contain the things we've asked, so just giving us back a subset of stuff just because the DB failed at some point sounds incorrect.
There was a problem hiding this comment.
The code here attempts to send as many events as possible. Please correct me if I'm wrong, but even if some events fail to process during the failableNext iteration, they will not be removed from the local database and will still be included in the results of the next flushEvent call.
In the case of issue #3855, the first failableNext call will fail due to data protection policy. I can modify the logic to propagate the error to the caller, rather than returning an array of results processed up to that point.
There was a problem hiding this comment.
Yeah, I think just propagating the error would be enough, since it will be caught and handled by the callers of this method:
There was a problem hiding this comment.
👍 updated the PR to propagte the errors.
5d
force-pushed
the
5d/fix-sqlite-crash
branch
from
a44a666
to
6e3889e
Compare
5d
had a problem deploying
to
IntegrationTest
GitHub Actions
Failure
5d
had a problem deploying
to
IntegrationTest
GitHub Actions
Failure
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3857 +/- ##
==========================================
+ Coverage 68.40% 68.44% +0.04%
==========================================
Files 1082 1082
Lines 37699 37699
==========================================
+ Hits 25787 25804 +17
+ Misses 11912 11895 -17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
5d
had a problem deploying
to
IntegrationTest
GitHub Actions
Failure
Issue #
#3855
Description
The default iteration behavior for SQLite statement results uses
try!, which throws a runtime error that can't be caught by Swift's standarddo..trymechanism. Since the analytics category runs as a background monitoring thread, it shouldn't cause the app to enter an error state. Instead, use the alternative throwable APIfailableNextto avoid runtime errors.General Checklist
Given When Theninline code documentation and are named accordinglytestThing_condition_expectation()By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.