considering timestamp when merge records

[darkdh]

Currently sync library prefer local record over remote record when merging "resolve-sync-records"
That cause changes overridden by old record.

STR:

1. Establish sync chain between device a and b
2. create "Welcome" bookmark on device a
3. Wait for it to appear on device b
4. Rename the bookmark to "WelcomeHOME" on device b
5. The bookmark is still "Welcome" on device a

because on device a
""resolve-sync-records" category_name="BOOKMARKS" recordsAndExistingObjects=[[{"action":0,"bookmark":{"hideInToolbar":false,"isFolder":false,"order":"1.1.0.1","site":{"creationTime":0,"customTitle":"Welcome","favicon":"","lastAccessedTime":0,"location":"chrome://welcome/","title":"Welcome"},"parentFolderObjectId":{}},"deviceId":{"0":1},"objectData":"bookmark","objectId":{"0":95,"1":163,"2":30,"3":184,"4":95,"5":136,"6":222,"7":204,"8":194,"9":47,"10":96,"11":15,"12":11,"13":18,"14":38,"15":71},"syncTimestamp":1540869759729},{"action":1,"bookmark":{"hideInToolbar":false,"isFolder":false,"order":"1.1.0.1","site":{"creationTime":0,"customTitle":"Welcome","favicon":"","lastAccessedTime":0,"location":"chrome://welcome/","title":"Welcome"},"parentFolderObjectId":{}},"deviceId":{"0":0},"objectData":"bookmark","objectId":{"0":95,"1":163,"2":30,"3":184,"4":95,"5":136,"6":222,"7":204,"8":194,"9":47,"10":96,"11":15,"12":11,"13":18,"14":38,"15":71},"syncTimestamp":1540869759729}],[{"action":1,"bookmark":{"hideInToolbar":false,"isFolder":false,"order":"1.1.0.1","site":{"creationTime":0,"customTitle":"WelcomeHOME","favicon":"","lastAccessedTime":0,"location":"chrome://welcome/","title":"WelcomeHOME"},"parentFolderObjectId":{}},"deviceId":{"0":1},"objectData":"bookmark","objectId":{"0":95,"1":163,"2":30,"3":184,"4":95,"5":136,"6":222,"7":204,"8":194,"9":47,"10":96,"11":15,"12":11,"13":18,"14":38,"15":71},"syncTimestamp":1540869775901},{"action":1,"bookmark":{"hideInToolbar":false,"isFolder":false,"order":"1.1.0.1","site":{"creationTime":0,"customTitle":"Welcome","favicon":"","lastAccessedTime":0,"location":"chrome://welcome/","title":"Welcome"},"parentFolderObjectId":{}},"deviceId":{"0":0},"objectData":"bookmark","objectId":{"0":95,"1":163,"2":30,"3":184,"4":95,"5":136,"6":222,"7":204,"8":194,"9":47,"10":96,"11":15,"12":11,"13":18,"14":38,"15":71},"syncTimestamp":1540869759729}]]"

and it got resolved-sync-records" categoryName="BOOKMARKS" records=[]"

It will be be changed once you add a new bookmark on device b and wait for it to propagate

[AlexeyBarabash]

This issue is not reproduced in current brave-core. There are 3 worksrounds which prevent this:

1. js sync lib - we don't send to client sync records from SQS, if they were sent from S3,
   https://github.com/brave/sync/blob/staging/lib/s3Helper.js#L178
2. brave-core and android code - on resolve operation we are ignoring the records with device_id==current_device_id,
   https://github.com/brave/brave-core/blob/master/components/brave_sync/brave_profile_sync_service_impl.cc#L794
   https://github.com/brave/browser-android-tabs/blob/base-77.0.3865.116/chrome/android/java/src/org/chromium/chrome/browser/BraveSyncWorker.java#L1510
3. brave-core code - we don't send bookmarks until we have 2 or more devices in the chain
   https://github.com/brave/brave-core/blob/master/components/brave_sync/brave_profile_sync_service_impl.cc#L219

With disabled pt1 and pt2 I could reproduce the issue.

Here how the data looks:
At https://github.com/brave/sync/blob/staging/client/recordUtil.js#L287

"recordUtil.resolveRecords 001 NORMALIZED=[
[{
"action":0,"bookmark":{"site":{"customTitle":"Welcome","location":"http://welcome.com/",
"title":"Welcome"},},
"deviceId":{"0":0},"objectData":"bookmark",
"syncTimestamp":1571 136 707 693},
{
"action":1,"bookmark":{"site":{"creationTime":0,"customTitle":"Welcome","location":"http://welcome.com/",
"title":"Welcome"}},
"deviceId":{"0":0},"objectData":"bookmark",
"syncTimestamp":1571 136 704 142.282
}],
[{
"action":1,"bookmark":{"site":{"customTitle":"WelcomeHOME","location":"http://welcome.com/",
"title":"WelcomeHOME"}},
"deviceId":{"0":1},"objectData":"bookmark",
"syncTimestamp":1571 136 762 021},
{
"action":1,"bookmark":{"site":{"customTitle":"Welcome","location":"http://welcome.com/",
"title":"Welcome"}},
"deviceId":{"0":0},"objectData":"bookmark",
"syncTimestamp":1571 136 704 142.282
}]
]"

At https://github.com/brave/sync/blob/staging/client/recordUtil.js#L288

"recordUtil.resolveRecords 002 MERGED=[
[{
"action":0,"bookmark":{"site":{"customTitle":"WelcomeHOME","location":"http://welcome.com/",
"title":"WelcomeHOME"}},"deviceId":[1],"objectData":"bookmark",
"syncTimestamp":1571 136 762 021},
{
"action":1,"bookmark":{"isFolder":false,"site":{"customTitle":"Welcome","location":"http://welcome.com/",
"title":"Welcome"}},"deviceId":{"0":0},"objectData":"bookmark",
"syncTimestamp":1571 136 704 142.282
}]
]"

And finally it resolves to [] at https://github.com/brave/sync/blob/staging/client/recordUtil.js#L215 because operation is CREATE and the existing object is not NULL.

The root of the problem is the first CREATE which is the record from our device (workaround pt2) , so it absorbs then next UPDATE.

Also I already made attempt to modify lib to keep alone operation and object id pairs
#313 , but we had to revert it due to #317 .

Options I see:
a. Workaround pt2 can be moved into sync js lib and work through the field deviceId of serverRecord and syncObject. So it will be available for all platforms and will work in the same way.

b. Simplify all by removing resolve operation at all.
pros - client will be apply all the changes from the sync one-by-one; simplifying migration to native lib;
cons - will take more execution time in the model; not clear how it work with chromium-based syncer at brave-core

c. improve #313 to allow merge CREATE+DELETE or UPDATE+DELETE, but forbid merge of CREATE+UPDATE

d. make merge don't ignore CREATE with existingObject!=NULL at https://github.com/brave/sync/blob/staging/client/recordUtil.js#L215 , but this way had some downside I cannot recall now.

[AlexeyBarabash]

As per Slack discussion with @bridiver and @darkdh :
#349 does not solves the root of the issue.

let’s take it to a two steps fixes
1. getting rid of merge, resolving and purely comparing timestamps among remote records
2. fixing how local records set timestamp

pt1 is a change in sync lib, meaning resolve-sync-records will get the latest record per each objectId and give them as resolved-sync-records . We can do this because all clients send full records to the sync and not only the fields had been changed.

pt2 means Android and iOS for resolve-sync-records should provide a timestamp field actual to the time of change object (bookmark). Desktop already does.

[AlexeyBarabash]

Closing as moving to sync v2