-
Notifications
You must be signed in to change notification settings - Fork 14
feat(cozy-pouch-link): Handle shared drive recipients' files #1632
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
base: master
Are you sure you want to change the base?
Conversation
* @param {CozyClient} params.client - CozyClient instance for fetching remote documents | ||
* @returns {Promise<Array>} A promise that resolves to an array of all replicated documents | ||
* @throws {Error} Throws an error if driveId is not provided | ||
*/ |
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.
Why is this necessary to make this function? When we discussed about it, I understood that we can call the pouch/couch replication for the shared drive, is it not the case @nono ?
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.
No, we realized that we don't have the revs_diff endpoint (and another endpoint that I don't remember). And we can't implement them in the stack in an efficient way. So, it would be better to implement a "manual" replication client-side.
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.
Ah, that's a pity. Out of curiosity, why is it not possible to make it efficient? Because of the remote access?
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.
Because we don't the paths of the files inside CouchDB, so we can't just make a request to CouchDB to get the relevant data, we need to iterate on each file, get its parent, compute its path, see if it's the shared drive, etc.
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.
But isn't it what we already do, to get the change feed for each shared drive?
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.
No, to get a shared drive's changes feed we filter the main changes feed but don't have to iterate over every file.
eebbbb3
to
6985569
Compare
a495b7d
to
d365e69
Compare
- Added new "virtual" doctype for shared drive files as "io.cozy.shareddrives-<driveId>" - Shared drive doctypes support replication with _changes route for each one - No _all_docs route available, had to fetch all changes for the initial replication
- Implemented addDoctype and removeDoctype methods in both PouchLink and PouchManager classes. - The methods to handle addition and removal of shared drives in real time
- Removed unnecessary fetching of the last remote sequence during initial replication. - Save the last sequence on replication completion
d365e69
to
0c8dd45
Compare
const isSharedDrive = Boolean(replicationOptions.driveId) | ||
if (seq && !isSharedDrive) { | ||
// For shared drives, we always need to keep the last sequence since replication is handled manually, not by PouchDB. | ||
// For standard PouchDB replications, the last sequence is only needed for the initial sync; subsequent runs use PouchDB's internal checkpointing. |
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.
But, for "standard replication", we now save the lastSeq
at the end of each completed replication process. So there is probably no point to destroy it here?
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.
But I did not change that. the lastSeq
was already saved on each completed replication.
It looks like the previous comment was wrong and the PouchDb replication already used the lastSeq
saved in localStorage for every replication (the first one with alldocs expected).
Yes, it means that there is no point at destroying it here but what I don't know is if there is a need to avoid to use the lastSeq in localStorage.
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.
the lastSeq was already saved on each completed replication.
Well, no, that's you who added the save on each completed replication 😄 ac5fb47#diff-6f9271f35894bccd13c05f69ba8e460d9cae55ed63666aa0b2038a5b4bc5a328
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.
Yes sorry. At this point, I think I just should revert ac5fb47
(#1632) since it has no point anymore
} | ||
|
||
startDocId = newLastSeq | ||
await helpers.insertBulkDocs(pouch, filteredDocs) |
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.
There is something I don't get.
First, we first do a manual removal of the deleted docs.
Then, we make a bulk insert of all the docs. Shouldn't we exclude those deleted docs? Also, I don't understand why deletions should be manually tackled, and not updates?
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.
I'm a bit worried about this part, it seems error prone, with no test :/
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.
I can add some unit tests mocking PouchDb and stack, but I doubt of their utility without any "browser" PouchDb since I am trying to bypass an "interface bug" between stack and PouchDb. But here are tests with full mock
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.
I still have issue with my first comment https://github.com/cozy/cozy-client/pull/1632/files#r2407273703 😉
Moreover, if we have any error in the process of getting/deleting pouch docs (e.g. a doc fails to be retrieved) , the whole process will fail, and we might be stucked forever with no possibility to replicate anymore, so maybe we should do specific error handling to continue the process even if it happens
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.
Regarding the deleted doc still sent to bulkDocs, I did it because these docs where ignored by PouchDb anyway. But let's make it cleaner. Done. I also added error handling which I found missing while reviewing this part.
I also added more comments on the why of this workaround and optimized the code a little bit
d1cf7ac
to
f3c7984
Compare
f3c7984
to
763f95c
Compare
c45013b
to
6b121f1
Compare
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.
Just first pass, I didn't understand the all logic yet
return `${authenticatedURL}/data/${doctype}` | ||
if (replicationOptions?.driveId) { | ||
return `${authenticatedURL}/sharings/drives/${replicationOptions?.driveId}` | ||
} else return `${authenticatedURL}/data/${doctype}` |
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.
no need for this else. you can simply return
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.
I found the version with else clearer
*/ | ||
addDoctype(doctype, options) { | ||
this.doctypes.push(doctype) | ||
this.options.doctypesReplicationOptions[doctype] = options |
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.
are we sure that this.options.doctypesReplicationOptions
is always defined? from this line it seems not
cozy-client/packages/cozy-pouch-link/src/PouchManager.js
Lines 94 to 95 in 2d81ae7
this.doctypesReplicationOptions = | |
this.options.doctypesReplicationOptions || {} |
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.
No, not always, now I take that into account
pouchOptions.view_update_changes_batch_size = DEFAULT_VIEW_UPDATE_BATCH | ||
} | ||
|
||
const dbName = getDatabaseName(this.options.prefix, doctype) |
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.
@paultranvan will it works with #1633 ?
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.
No, good catch.
@doubleface I think we could do 2 things here:
- Pass the shared drive as a regular doctype (so inside the
doctypes
array) , I don't think there is a reason to add them manually - Update the persisted db names when we add / remove a doctype: https://github.com/cozy/cozy-client/pull/1633/files#diff-d9481d2a79cecb973aa3435a8718efbc923302bc660ee24ac29026ab3d157a62R88
- Refactored the addDoctype method in PouchManager to streamline the addition of doctypes. - Updated PouchLink to correctly push doctypes to the pouches before starting replication. - Adjusted type definitions to reflect changes in the addDoctype method signature.
When an error occured for example when a db is missing, the replication was blocked because the error was not intercepted and the replication promise was never resolved or rejected Later `startReplication({waitForReplication: true})` was blocked forever because of this
Also only fetch existing documents on initial replication for better performance.
- better comments - less loops to find sort documents - added logs when trying to delete non existing document - intercept errors when deleting documents
- Replaced calls to `lodash/get` with direct property access - Simplified loops in `startReplication` for better performance and readability. - jsdoc for `getReplicationURL` function
3c66207
to
8c76e4f
Compare
No need for cloning the document to add the driveId.
} | ||
this.doctypesReplicationOptions[doctype] = replicationOptions | ||
this.pouches.doctypes.push(doctype) | ||
this.pouches.addDoctype(doctype, replicationOptions) |
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.
addDoctype should do:
this.pouches.doctypes.push(doctype)
This will allow using PouchLink to get the list of shared drives doctypes without knowing shared drives doctypes prefix.
fa2f47e
to
3a11e12
Compare
Now shared drives recipients files have their own "virtual" doctype like
"io.cozy.shareddrives-<driveId>"
which is first replicated using the_changes
route for each shared drive (no_all_docs
available)To handle realtime addition/removal of shared drives, "addDoctype" and "removeDoctype" method have been added to the PouchLink class