Ignore conflicted records during resolve

[AlexeyBarabash]

Fixes #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 #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.

[darkdh]

why not extend #313 to handle DELETE cases like this

1. [[CREATE, object], [UPDATE, object]] => [[CREATE, object], [UPDATE, object]]
2. [[CREATE, null], [UPDATE, null]] => [[CREATE(merged), null]]
3. [[CREATE/UPDATE, null]..., [DELETE, null]] => ignore
4. [[CREATE/UPDATE, object]..., [DELETE, object]] => [[DELETE, object]]

[AlexeyBarabash]

why not extend #313 to handle DELETE cases like this

this sounds good, but probably contains another surprises

[darkdh]

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

1. device A and device B are synced
2. Add bookmark title "BM" on device A and wait for it to show up on device B
3. edit the bookmark on device A to be "BM1" and don't click SAVE yet
4. edit the bookmark on device B to be "BM2" and don't click SAVE yet
5. click SAVE on device A and then click SAVE on device B as fast as you can
6. We will have BM2 on device A and BM1 on device B (the expected result would be BM2 on all devices if we consider timestamp before merging)

[AlexeyBarabash]

@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 #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.

[SergeyZhukovsky]

++

[bridiver]

This may be some other issue, but it's not the issue that is described in #261

[AlexeyBarabash]

Closing this PR as for #261 (comment)