Closed AlexeyBarabash closed 4 years ago
why not extend #313 to handle DELETE cases like this
this sounds good, but probably contains another surprises
this sounds good, but probably contains another surprises
it's ok, just a thought.
We really need to consider timestamp when doing UPDATE
merge, consider this scenario
@anthony your STR from above BM1, BM2 are interesting.
First I tried it in my local branch, where I work on current PR - and could not reproduce it with several attemps.
Then I tried on Nightly, which is roughly saying the current brave-core master
- and I was able easily reproduce it from the first try.
The only difference was at https://github.com/brave/brave-core/blob/master/components/brave_sync/brave_profile_sync_service_impl.cc#L793, I disable such ignore to see https://github.com/brave/sync/issues/261 STR works.
Rebuild of my branch without mentioned workaround have made STR BM1, BM2
show the issue.
So workaround of ingnore records from this device
makes the harm, meaning it allows to apply the changes to the object from the record if record_time < object_modified_time.
Device should receive and and process the record from itself because such record can "absorb" earlier record from other device and prevent unwanted change.
Workaround should be reverted on Desktop, Android ~and iOS~.
I think, except the current PR, no other actions required, because the browser gets get-existing-objects
sorted by timestampt:
response from SQS https://github.com/brave/sync/blob/staging/lib/s3Helper.js#L197
response from S3 is not sorted by us, it goes already sorted.
On fetch record operation, the time is get from S3 key: https://github.com/brave/sync/blob/staging/lib/s3Helper.js#L77 and overwrites the time if it was specified in the record.
The time of record is ignored when we send it to AWS https://github.com/brave/sync/blob/staging/lib/s3Helper.js#L51
So I think, there are no any specific processing of records time during resolve required.
This may be some other issue, but it's not the issue that is described in #261
Closing this PR as for https://github.com/brave/sync/issues/261#issuecomment-545640724
Fixes https://github.com/brave/sync/issues/261
When sync lib gets unexpected data, during the merge it can lead to wrong resolve result calculated changes.
These are in https://github.com/brave/sync/blob/staging/client/recordUtil.js#L212 when
nullIgnore()
is returned: 1) CREATE when existing object is not null; 2) DELETE when existing object is already null;If this records are ignored by themselves , it doesn't cause a trouble. But before to be ignored, such records absorb the records of the same
objectId
and then the required changes become wrongly ignored. The illustration is https://github.com/brave/sync/issues/261 .Obviously, if we would not do resolve and apply each record to the model, this would give the result.
So to avoid the mess on resolve, such pairs which would be resolved to null, but can make other important changes to be wrongly ignored - such pairs are removed from the merge on resolve operation.
Will mark as not draft when add the test cases.