From 35cfea2d2ccb461b71471463daac951ebbf984d2 Mon Sep 17 00:00:00 2001 From: Erwan Guyader Date: Tue, 4 Jan 2022 18:59:58 +0100 Subject: [PATCH] core/merge: Handle more remote deletion situations Handling remote deletions requires different actions to be taken and thus different metadata to be merged in PouchDB depending on the situation before the deletion (e.g. the document was moved or its new content was added on the local filesystem in case of a folder). We aim at addressing most of these in this commit. This is particularly necessary since folders excluded via the partial synchronization feature are seen as deleted folders from the Desktop's point of view. --- core/merge.js | 145 ++++++++++----- test/unit/merge.js | 437 ++++++++++++++++++++++++++++++++------------- 2 files changed, 412 insertions(+), 170 deletions(-) diff --git a/core/merge.js b/core/merge.js index 69344cd16..5dfd1d9f4 100644 --- a/core/merge.js +++ b/core/merge.js @@ -895,76 +895,125 @@ class Merge { // Remove a folder // + // This method is only called for complete remote deletions (i.e. the document + // was trashed and then completely removed from the Trash). This means we + // don't have to keep remote metadata in the PouchDB record as the remote + // document is unrecoverable. + // It also means we can fetch the document from PouchDB via its _id. + // // When a folder is removed in PouchDB, we also remove the files and folders - // inside it to ensure consistency. The watchers often detects the deletion - // of a nested folder after the deletion of its parent. In this case, the - // call to deleteFolder for the child is considered as successful, even if - // the folder is missing in pouchdb (error 404). + // inside it to ensure consistency. The remote watcher will sort the deletion + // of a folder before the deletion of its content. async deleteFolderAsync(side /*: SideName */, doc /*: SavedMetadata */) { log.debug({ path: doc.path }, 'deleteFolderAsync') - const folder /*: ?SavedMetadata */ = await this.pouch.bySyncedPath(doc.path) + const folder /*: ?SavedMetadata */ = await this.pouch.byIdMaybe(doc._id) if (!folder) { - log.debug({ path }, 'Nothing to delete') + log.debug({ path: doc.path }, 'Nothing to delete') return } - if (folder.moveFrom) { - // We don't want Sync to pick up this move hint and try to synchronize a - // move so we delete it. - delete folder.moveFrom - } - - if (folder.sides && folder.sides[side]) { - return this.deleteFolderRecursivelyAsync(side, folder) - } else { - // It can happen after a conflict - return - } - } - - // Remove a folder and every thing inside it - async deleteFolderRecursivelyAsync( - side /*: SideName */, - folder /*: SavedMetadata */ - ) { // In the changes feed, nested subfolder must be deleted // before their parents, hence the reverse order. - const docs = await this.pouch.byRecursivePath(folder.path, { + const children = await this.pouch.byRecursivePath(folder.path, { descending: true }) - const toPreserve = new Set() - for (let doc of docs) { - if (doc.trashed) continue - + for (let child of children) { if ( - toPreserve.has(doc.path) || - (doc.sides && !metadata.isUpToDate(side, doc)) + child.trashed && + !metadata.isAtLeastUpToDate(otherSide(side), child) ) { - log.warn( - { - path: doc.path, - ancestorPath: folder.path, - otherSide: otherSide(side) - }, - 'Cannot be trashed with ancestor: document was modified on the other side.' - ) - log.info({ path: doc.path }, 'Dissociating from remote...') - metadata.dissociateRemote(doc) - toPreserve.add(path.dirname(doc.path)) + // No need to generate extra changes if the document was already marked + // for deletion on the remote side. + continue + } + + const { moveFrom } = child + + if (moveFrom && child.local) { + if (child.path.normalize() === child.local.path.normalize()) { + if ( + folder.moveFrom && + child.moveFrom.path.startsWith(folder.moveFrom.path + path.sep) + ) { + // The child was moved on the local side with the deleted folder thus + // we need to trash it on the local filesystem. + metadata.markSide(side, child, child) + } else { + // The child was moved on the local side into the deleted folder thus + // we need to trash it on the Cozy. + metadata.markSide(otherSide(side), child, child) + // XXX: the path displayed in logs will be the one after the child + // was moved (and became a child of folder) which can be a bit + // confusing but since we use the remote _id to apply actions on the + // remote Cozy we don't really care. + } + } else { + // The child was moved on the remote side into the deleted folder thus + // we need to trash its former location on the local filesystem. + // XXX: Since the Local module uses the main path instead of the local + // path, we have to "revert" the remote move by changing the main path + // back into its local value. + // TODO: use the local path in the Local module so we don't have to deal + // with such issues. + child.path = child.local.path + + metadata.markSide(side, child, child) + } + } else if (child.sides && !child.sides[side]) { + metadata.markSide(otherSide(side), child, child) + } else { + metadata.markSide(side, child, child) + } + + child.trashed = true + + // We don't want Sync to pick up other hints than the deletion and + // try to synchronize them. + delete child.moveFrom + delete child.overwrite + delete child.errors + } + + const { moveFrom, overwrite } = folder + + if (moveFrom && folder.local) { + if (folder.path.normalize() === folder.local.path.normalize()) { + if (overwrite) { + // TODO: Should we call deleteFolderAsync if overwrite is a folder to + // be sure we delete all its content as well? + overwrite.trashed = true + metadata.markSide(otherSide(side), overwrite, overwrite) + + delete overwrite.moveFrom + delete overwrite.overwrite + delete overwrite.errors + delete overwrite._id + delete overwrite._rev + + children.push(overwrite) + } } else { - metadata.markSide(side, doc, doc) - doc.trashed = true - delete doc.errors + // The folder was moved on the remote side before being deleted so we + // need to trash its former location on the local filesystem. + // XXX: Since the Local module uses the main path instead of the local + // path, we have to "revert" the remote move by changing the main path + // back into its local value. + // TODO: use the local path in the Local module so we don't have to deal + // with such issues. + folder.path = folder.local.path } } metadata.markSide(side, folder, folder) folder.trashed = true + // We don't want Sync to pick up other hints than the deletion and try to + // synchronize them. + delete folder.moveFrom + delete folder.overwrite delete folder.errors - docs.push(folder) - return this.pouch.bulkDocs(docs) + return this.pouch.bulkDocs(children.concat(folder)) } } diff --git a/test/unit/merge.js b/test/unit/merge.js index ddf72caf2..7715bb4bb 100644 --- a/test/unit/merge.js +++ b/test/unit/merge.js @@ -4033,8 +4033,17 @@ describe('Merge', function() { }) describe('deleteFolderAsync', function() { + before(function() { + // XXX: deleteFolderAsync is only used for remote deletions + this.side = 'remote' + }) + after(function() { + // XXX: 'local' is the current side value but it could change + this.side = 'local' + }) + context('when a record is found in Pouch', () => { - it('deletes a folder', async function() { + it('marks the folder for deletion on the local filesystem', async function() { const doc = await builders .metadir() .path('FOLDER') @@ -4059,135 +4068,323 @@ describe('Merge', function() { }) }) - it('removes nested content', async function() { - const doc = await builders - .metadir() - .path('FOLDER') - .sides({ [this.side]: 1 }) - .create() - const subdir = await builders - .metafile() - .path('FOLDER/DIR') - .sides({ [this.side]: 1 }) - .create() - const subsubdir = await builders - .metafile() - .path('FOLDER/DIR/DIR') - .sides({ [this.side]: 1 }) - .create() - const subsubsubfile = await builders - .metafile() - .path('FOLDER/DIR/DIR/FILE') - .sides({ [this.side]: 1 }) - .data('content') - .create() + context('and it has children', () => { + it('marks children for deletion on the local filesystem', async function() { + const doc = await builders + .metadir() + .path('FOLDER') + .upToDate() + .create() + const subdir = await builders + .metadir() + .path('FOLDER/DIR') + .upToDate() + .create() + const subsubdir = await builders + .metadir() + .path('FOLDER/DIR/DIR') + .upToDate() + .create() + const subsubsubfile = await builders + .metafile() + .path('FOLDER/DIR/DIR/FILE') + .data('content') + .upToDate() + .create() - const sideEffects = await mergeSideEffects(this, () => - this.merge.deleteFolderAsync(this.side, _.cloneDeep(doc)) - ) + const sideEffects = await mergeSideEffects(this, () => + this.merge.deleteFolderAsync(this.side, _.cloneDeep(doc)) + ) - should(sideEffects).deepEqual({ - savedDocs: [ - _.defaults( - { - sides: increasedSides(subsubsubfile.sides, this.side, 1), - trashed: true - }, - _.omit(subsubsubfile, ['_rev']) - ), - _.defaults( - { - sides: increasedSides(subsubdir.sides, this.side, 1), - trashed: true - }, - _.omit(subsubdir, ['_rev']) - ), - _.defaults( - { - sides: increasedSides(subdir.sides, this.side, 1), - trashed: true - }, - _.omit(subdir, ['_rev']) - ), - _.defaults( - { - sides: increasedSides(doc.sides, this.side, 1), - trashed: true - }, - _.omit(doc, ['_rev']) - ) - ], - resolvedConflicts: [] + should(sideEffects).deepEqual({ + savedDocs: [ + _.defaults( + { + sides: increasedSides(subsubsubfile.sides, this.side, 1), + trashed: true + }, + _.omit(subsubsubfile, ['_rev']) + ), + _.defaults( + { + sides: increasedSides(subsubdir.sides, this.side, 1), + trashed: true + }, + _.omit(subsubdir, ['_rev']) + ), + _.defaults( + { + sides: increasedSides(subdir.sides, this.side, 1), + trashed: true + }, + _.omit(subdir, ['_rev']) + ), + _.defaults( + { + sides: increasedSides(doc.sides, this.side, 1), + trashed: true + }, + _.omit(doc, ['_rev']) + ) + ], + resolvedConflicts: [] + }) }) - }) - it('removes move hints', async function() { - // TODO: make sure we remove the move hints on children. This will be part - // of a larger refactoring to make sure folders are trashed with their - // hierarchy. - const old = await builders - .metadir() - .path('FOLDER') - .sides({ [this.side]: 1 }) - .create() - const doc = await builders - .metadir() - .moveFrom(old) - .path('MOVED') - .sides({ [this.side]: 1 }) - .create() + context( + 'and child was moved into the folder on the local filesystem', + () => { + it('marks it for deletion on the remote Cozy', async function() { + const doc = await builders + .metadir() + .path('folder') + .upToDate() + .create() - const sideEffects = await mergeSideEffects(this, () => - this.merge.deleteFolderAsync(this.side, _.cloneDeep(doc)) + const dir = await builders + .metadir() + .path('dir') + .upToDate() + .create() + const movedDir = await builders + .metadir() + .moveFrom(dir) + .path('folder/dir') + .changedSide(otherSide(this.side)) + .create() + + const sideEffects = await mergeSideEffects(this, () => + this.merge.deleteFolderAsync(this.side, _.cloneDeep(doc)) + ) + + should(sideEffects).deepEqual({ + savedDocs: [ + _.defaults( + { + [this.side]: dir[this.side], + sides: increasedSides( + movedDir.sides, + otherSide(this.side), + 1 + ), + trashed: true + }, + _.omit(movedDir, ['_rev', 'moveFrom']) + ), + _.defaults( + { + sides: increasedSides(doc.sides, this.side, 1), + trashed: true + }, + _.omit(doc, ['_rev']) + ) + ], + resolvedConflicts: [] + }) + }) + } ) + }) - should(sideEffects).deepEqual({ - savedDocs: [ - _.defaults( - { - sides: increasedSides(doc.sides, this.side, 1), - trashed: true - }, - _.omit(doc, ['_rev', 'moveFrom']) + context('and it was moved on the local filesystem', () => { + it('removes move hints', async function() { + const old = await builders + .metadir() + .path('FOLDER') + .upToDate() + .create() + const doc = await builders + .metadir() + .moveFrom(old) + .path('MOVED') + .changedSide(otherSide(this.side)) + .create() + + const sideEffects = await mergeSideEffects(this, () => + this.merge.deleteFolderAsync(this.side, _.cloneDeep(doc)) + ) + + should(sideEffects).deepEqual({ + savedDocs: [ + _.defaults( + { + // XXX: folder was moved on local side so the remote side is + // increased by 2 to compensate. + sides: increasedSides(doc.sides, this.side, 2), + trashed: true + }, + _.omit(doc, ['_rev', 'moveFrom']) + ) + ], + resolvedConflicts: [] + }) + }) + + context('with children', () => { + it('marks the children for deletion on the local filesystem', async function() { + const dir = await builders + .metadir() + .path('folder') + .upToDate() + .create() + const subdir = await builders + .metadir() + .path('folder/subdir') + .upToDate() + .create() + const file = await builders + .metafile() + .path('folder/subdir/file') + .data('content') + .upToDate() + .create() + + const movedDir = await builders + .metadir() + .moveFrom(dir) + .path('dir') + .changedSide(otherSide(this.side)) + .create() + const movedSubdir = await builders + .metadir() + .moveFrom(subdir, { childMove: true }) + .path('dir/subdir') + .changedSide(otherSide(this.side)) + .create() + const movedFile = await builders + .metadir() + .moveFrom(file, { childMove: true }) + .path('dir/subdir/file') + .changedSide(otherSide(this.side)) + .create() + + const sideEffects = await mergeSideEffects(this, () => + this.merge.deleteFolderAsync(this.side, _.cloneDeep(movedDir)) ) - ], - resolvedConflicts: [] + + should(sideEffects).deepEqual({ + savedDocs: [ + _.defaultsDeep( + { + [this.side]: movedFile[this.side], + // XXX: child was moved on local side so the remote side is + // increased by 2 to compensate. + sides: increasedSides(movedFile.sides, this.side, 2), + trashed: true + }, + _.omit(movedFile, ['_rev', 'moveFrom', 'childMove']) + ), + _.defaults( + { + [this.side]: movedSubdir[this.side], + // XXX: child was moved on local side so the remote side is + // increased by 2 to compensate. + sides: increasedSides(movedSubdir.sides, this.side, 2), + trashed: true + }, + _.omit(movedSubdir, ['_rev', 'moveFrom', 'childMove']) + ), + _.defaults( + { + [this.side]: movedDir[this.side], + sides: increasedSides(movedDir.sides, this.side, 2), + trashed: true + }, + _.omit(movedDir, ['_rev', 'moveFrom']) + ) + ], + resolvedConflicts: [] + }) + }) + }) + + context('overwriting another document', () => { + it('marks the overwritten document for deletion on the remote Cozy', async function() { + const overwritten = await builders + .metafile() + .path('MOVED') + .upToDate() + .create() + const old = await builders + .metadir() + .path('FOLDER') + .upToDate() + .create() + const doc = await builders + .metadir() + .moveFrom(old) + .overwrite(overwritten) + .path('MOVED') + .changedSide(otherSide(this.side)) + .create() + + const sideEffects = await mergeSideEffects(this, () => + this.merge.deleteFolderAsync(this.side, _.cloneDeep(doc)) + ) + + should(sideEffects).deepEqual({ + savedDocs: [ + _.defaults( + { + sides: increasedSides( + overwritten.sides, + otherSide(this.side), + 1 + ), + trashed: true + }, + // XXX: overwritten is recreated so we don't keep its _id + _.omit(overwritten, ['_id', '_rev']) + ), + _.defaults( + { + // XXX: folder was moved on local side so the remote side is + // increased by 2 to compensate. + sides: increasedSides(doc.sides, this.side, 2), + trashed: true + }, + _.omit(doc, ['_rev', 'moveFrom', 'overwrite']) + ) + ], + resolvedConflicts: [] + }) + }) }) }) - }) - context('when a trashed record is found in Pouch', () => { - it('marks it for deletion and updates sides info', async function() { - const was = await builders - .metadir() - .path('FOLDER') - .trashed() - .changedSide(otherSide(this.side)) - .create() - const doc = builders - .metadir(was) - .trashed() - .unmerged(this.side) - .build() + context('and it was moved on the remote Cozy', () => { + it('marks the previous location for deletion on the local filesystem', async function() { + const old = await builders + .metadir() + .path('FOLDER') + .upToDate() + .create() + const doc = await builders + .metadir() + .moveFrom(old) + .path('MOVED') + .changedSide(this.side) + .create() - const sideEffects = await mergeSideEffects(this, () => - this.merge.deleteFolderAsync(this.side, _.cloneDeep(doc)) - ) + const sideEffects = await mergeSideEffects(this, () => + this.merge.deleteFolderAsync(this.side, _.cloneDeep(doc)) + ) - should(sideEffects).deepEqual({ - savedDocs: [ - _.defaults( - { - // We increase the side by 2 since the other side was increased - // when `was` was trashed - sides: increasedSides(was.sides, this.side, 2), - [this.side]: doc[this.side], - trashed: true - }, - _.omit(was, ['_rev']) - ) - ], - resolvedConflicts: [] + should(sideEffects).deepEqual({ + savedDocs: [ + _.defaults( + { + // XXX: folder was moved on local side so the remote side is + // increased by 2 to compensate. + sides: increasedSides(doc.sides, this.side, 1), + path: doc.local.path, + trashed: true + }, + _.omit(doc, ['_rev', 'moveFrom']) + ) + ], + resolvedConflicts: [] + }) }) }) }) @@ -4200,11 +4397,7 @@ describe('Merge', function() { .trashed() .changedSide(otherSide(this.side)) .create() - const doc = builders - .metadir(was) - .trashed() - .unmerged(this.side) - .build() + const doc = builders.metadir(was).build() const sideEffects = await mergeSideEffects(this, () => this.merge.deleteFolderAsync(this.side, _.cloneDeep(doc))