Bookmarks - Local Mutations DB

[mohannad-hassan]

Introduces the repository for the local mutations database of bookmarks. This stores full records, but encapsulates the operation as well.

The backend's logic for bookmarks doesn't allow editing; only creation and removal.

This PR is pending on #683 and will be rebased on master after the other PR is merged.

All changes for this PR are in the Data/SyncedPageBookmarkPersistence package.

[codecov]

Codecov Report

Attention: Patch coverage is 96.41434% with 9 lines in your changes missing coverage. Please review.

Project coverage is 41.73%. Comparing base (d9fc366) to head (d2cbbc6).
Report is 43 commits behind head on main.

Files with missing lines	Patch %	Lines
...e/Sources/GRDBMutatedPageBookmarkPersistence.swift	92.92%	7 Missing ⚠️
...ests/GRDBMutatedPageBookmarkPersistenceTests.swift	98.68%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #685      +/-   ##
==========================================
+ Coverage   40.92%   41.73%   +0.80%     
==========================================
  Files         525      544      +19     
  Lines       20880    22100    +1220     
==========================================
+ Hits         8546     9224     +678     
- Misses      12334    12876     +542     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mohamede1945]

A couple of comments here:

1. I think we need to represent the mutation itself here (i.e. create, update, delete)
2. I don't think we need deleted this should be known from Reading & Listening #1
3. I would add remoteID as an associated value to the mutation enum's update & delete cases.
4. I think we need to rename this to be PageBookmarkMutation since the main purpose of this is to hold the mutation.

[mohannad-hassan]

@mohamede1945 I don't quite understand the second point.

[mohannad-hassan]

Regarding the other points:

I like defining a property that declares the mutation that happened to the object.

However, I'd say that it's better to keep the object as similar to how the app's bookmark model would look like. As I mentioned before, I prefer this repository to return records in their latest state, locally. In doing so, it'll aggregate the mutations.

Here's how the merging should look like in the (upper) repo:

let syncedObjects = // retrieved from synced persistence 
let mutatedObjects = // retrieved from local mutations persistence 

let uneditedSynced = syncedObjects.filter{ !mutatedObjects.has($0.paage) }
// For bookmarks we only have creation and deletion mutations.
let newBookmarks = mutatedObjects.filter{ ! $0.deleted } // Or $0.mutation == .create

return uneditedSynced + newBookmarks // After converting to the app's bookmark model

[mohamede1945]

Mutations should be a mutations (i.e. changes that happened), so it needs to keep track of the Create, Update, Delete operations separately. The payload of the mutation can be whatever you think make sense whether the diff or the full object, I think that's fine either way. But the main point here is that, we need to keep track of the changes of the object. For the bookmarks the data would look like (there's no updates to bookmarks):

• CREATE, page: 1, time: 12:00
• Delete, page: 1, time: 12:10
• Delete, page: 2, time: 12:20
• Create, page: 3, time: 12:30

Then when a down sync happens we get a similarly shaped data from the the backend, we merge them together sorted by time and then do an up sync. So we need to keep track of the full change log.

Let me know if this is not very clear

[mohannad-hassan]

The diff at the top of this thread is outdated. Check out the updated model.

I've added a mutation field that clearly defines this. It's more important as there's a use case where a synced bookmark (with a non-nil remote ID) gets deleted, and then a new bookmark is created for the same page.

But I still kept the name as a mutated bookmark. I feel that if I clearly defined as mutation event, it'd signal to the client that it needs to be aggregated.

[mohamede1945]

But the data should not be aggregated. This table should store raw mutations that can be replayed at any time. This ensures that when an opportunity for an up-sync arises, all mutations can be properly replayed.

It serves as a log for user actions such as create, update, and delete. These actions should remain unaggregated because they are local to each device. A user may have multiple devices, each maintaining its own list of actions. If we aggregate actions per device, we lose the ability to achieve a global aggregation during synchronization.

For example:

Device A records the following actions:

• Time 1: Create, page=1
• Time 5: Remove, page=1

Device B records the following action:

• Time 3: Create, page=1

If we aggregate the data, the final state may incorrectly show a bookmark on page 1 because Device A would have an empty list. However, by persisting each event individually, we maintain a consistent history across devices (all devices can come up with the below list):

• Device A, Time 1: Create, page=1
• Device B, Time 3: Create, page=1
• Device A, Time 5: Remove, page=1

This way the app can determine that the user does not need a bookmark on page 1.

[mohannad-hassan]

In that case specifically (for bookmarks), I'd ignore the events on device A; that's a base for my idea for the design. The change in the server (say, assuming device B was quicker to sync) is upheld.

I'd create two records if a synced bookmark is deleted, and then a new bookmark is created for the same page.

A clearer scenario for aggregating events (when applicable):

• Create note N (downstream)
• Edit N's color
• Edit N's text
• Append new verses to N

I'd coalesce all these into a single create event. If N was a synced record, I'd coalesce all later events in a single edit event.

Let me clarify the directions of the design I'm thinking of:

• A conservative approach for conflict resolution: that's how I treated the case mentioned above. As another example, if a note's text was edited in one branch, and deleted in the other; I think it's better to keep the note. That's why I thought that in some cases the user should be prompted to manually resolve some conflicts.
• The latest state of the resources first, modification date second. Attempt to reconcile the conflicting two states in their latest form, and use the modification date as a factor.

[mohamede1945]

Is there a use case for the clear operation?

[mohannad-hassan]

Clearing all local mutations after upstream synchronization is done.

My working idea is to only update the synced and local mutations tables after the whole synchronization operation is done, along with any needed misc data (lastSynedAt value).

So given all goes correctly we'll have bunch of updates to be created in the synced database and the local mutations will be cleared; all that will be in one go.

[mohamede1945]

I would use bookmark mutation in the name since these represent mutations rather than the bookmarks themselves. This allows multiple mutations on the same bookmark page to be stored here.

Additionally, the operations should be CRUD actions on the mutation, not the bookmark itself. For example:

• insertBookmarkMutation: The mutation could be create, update, or delete.
• removeBookmarkMutation: The mutation could be create, update, or delete.
• And so on.

[mohannad-hassan]

I'd rather keep the decision of how to define and declare mutation events and how to resolve them with earlier mutations to the local mutations repository.

If the repo (the upper one) creates a bookmark, it'll directly forward the call to the mutations DB. The publisher would update its output; its responsibility then would be to merge that with the synced data.
The merging logic in that case would work as simply as replacing a synced object with its mutated counterpart, or delete if it was deleted locally, appending these with the rest of both the unedited synced objects and unsynced locally created ones.