-
Notifications
You must be signed in to change notification settings - Fork 78
Fixes to date synchronization. #387
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
Conversation
| } | ||
| } | ||
| if didSet || isRowValueModified { | ||
| if didSet || (syncEngineHasPendingChanges && isRowValueModified) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a somewhat unrelated fixed, but it was exacerbated by the date problem. We should only skip updating a local field of a record if there are pending iCloud changes for that record. That's because in that situation we interpret that to mean the user made local changes that will eventually be sent out to iCloud.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, how is it possible for a row to have a modification without having pending changes enqueued in the sync engine? Aren’t row changes automatically picked up by a trigger and enqueued?
Or is this a guardrail against false positives from the equality check? If so, would it really work, as there might be unrelated changes to other fields of the same record?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukaskubanek It should not be possible, but 1) the user of the library could do something bad like make changes to the database with a different SQLite connection than what the SyncEngine has, and 2) some types may not roundtrip cleanly to and from SQLite (such as dates). Whenever either of those happen we don't want that field to be forever stuck and never update. Adding this condition allows us to kick things in the butt if we detect a change to a field that has no corresponding pending change.
If so, would it really work, as there might be unrelated changes to other fields of the same record?
Yeah, I believe this is not 100% foolproof, but it works one particular use case now, and we have good test coverage on it, so in the future we could explore more robust solutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @lukaskubanek just wanted to let you know that we ended up backing out of these syncEngineHasPendingChanges changes. We added it as a way to protect against data that does not round trip properly to SQLite and back, but it's an imperfect technique and really it's best for us to just require that data always roundtrip.
| extension Date { | ||
| @usableFromInline | ||
| var iso8601String: String { | ||
| let nextUpDate = Date(timeIntervalSinceReferenceDate: timeIntervalSinceReferenceDate.nextUp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the actual fix for the date issue.
|
@mbrandonw Just saw this PR in my inbox and it reminded me of #275 I reported a while back. Does it mean that with this change upserts will no longer yield false positives when checking for modifications on |
Ah I forgot about that issue, but yes this should fix it. The problem is a combination of dates being represented by doubles in Swift and ISO8601 formatted strings in SQLite. For example, using the date I used in the description of this PR, we can turn it into an ISO8601 string and back into a date a few times to see it changes multiple times: Notice that We have fixed this by |
|
@mbrandonw Thanks for keeping me in the loop on this one!
Makes sense to me, especially since it was addressing rather special cases outside the main flow while still not fully covering all edge cases within that flow.
I also believe this PR fixes the issue for dates. Earlier, I had considered an alternative approach where the library would persist the exact raw representation coming from SQLite into CloudKit. For dates, this could mean storing the value as a It’s likely too late to consider this direction for the library, but I’d be curious to hear your thoughts on this approach. |
|
Currently our stance is that we should let each of SQLite and iCloud deal with the types that makes the most sense for them. So that means ISO8601 strings for dates in SQLite, and |
|
That’s a strong argument, thanks for sharing! |
Right now it's possible for dates to change by round tripping them to the database and back into Swift. This is due to the fact that
Dateis the de facto standard for dates in Swift, andDaterepresents time with a double.This is a test from the PR that failed prior to the changes to show the problem:
To fix we need to
nextUpthe date before turning it into an ISO8601 formatted string.