From 59b2db1523833d3668e3793099429f41d3194d8a Mon Sep 17 00:00:00 2001 From: Erwan Guyader Date: Mon, 13 Jan 2025 23:38:57 +0100 Subject: [PATCH] fix: Do not mark undefined remote as trashed (#2372) When merging the trashing of a document, we add a `trashed` marker in its metadata for the side on which it was trashed (e.g. in the `remote` attribute if the document was trashed on the Cozy) as well as its global metadata. However, when a directory is trashed, we also mark all its children and these might not have been synchronized yet and the trashed side metadata can be missing. e.g. given we have the synchronized directory `dir/` and a local only file `dir/file`, when merging the trashing of `dir/` on the Cozy we: 1. mark `dir/` as trashed and its `remote` attribute 2. try to do the same for `dir/file` but it has no `remote` attribute, only a `local` one We were not checking for the presence of the `remote` attribute and in this case we would end up with an error while trying to mark `dir/file` as we'd try to add a `trashed` attribute on `undefined` (i.e. the missing `remote` attribute). By checking for the presence of the `remote` attribute we make sure we don't get this error and don't block the synchronization of the parent directory trashing. The child will still be removed with its parent. --- core/metadata.js | 16 ++++++---- test/integration/trash.js | 30 ++++++++++++++++++ test/unit/metadata.js | 67 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 105 insertions(+), 8 deletions(-) diff --git a/core/metadata.js b/core/metadata.js index f52b4b035..97e85cdf7 100644 --- a/core/metadata.js +++ b/core/metadata.js @@ -517,13 +517,15 @@ function dissociateLocal(doc /*: Metadata */) { function markAsTrashed(doc /*: Metadata */, sideName /*: SideName */) { if (sideName === 'remote') { - if (doc.remote && doc.remote.type === REMOTE_DIR_TYPE) { - // FIXME: Remote directories have no `trashed` attribute so we know - // they're trashed when their path is within the remote trashbin. We - // should find a way to reconstruct that path or stop relying on this - // function altogether. - } else { - doc.remote.trashed = true + if (doc.remote) { + if (doc.remote.type === REMOTE_DIR_TYPE) { + // FIXME: Remote directories have no `trashed` attribute so we know + // they're trashed when their path is within the remote trashbin. We + // should find a way to reconstruct that path or stop relying on this + // function altogether. + } else { + doc.remote.trashed = true + } } } else if (doc.local) { doc.local.trashed = true diff --git a/test/integration/trash.js b/test/integration/trash.js index 246e52681..f0aba01e1 100644 --- a/test/integration/trash.js +++ b/test/integration/trash.js @@ -5,6 +5,7 @@ const path = require('path') const should = require('should') +const pathUtils = require('../../core/utils/path') const TestHelpers = require('../support/helpers') const configHelpers = require('../support/helpers/config') const cozyHelpers = require('../support/helpers/cozy') @@ -381,6 +382,35 @@ describe('Trash', () => { 'parent/' ]) }) + + context('with unsynced local-only content', () => { + beforeEach(async () => { + await helpers.local.syncDir.outputFile( + `${pathUtils.remoteToLocal(dir.attributes.path)}/local-child-file`, + 'content' + ) + await helpers.local.scan() + + helpers.resetPouchSpy() + }) + + it('trashes the directory and its content on the local filesystem', async () => { + await cozy.files.trashById(dir._id) + + await helpers.remote.pullChanges() + should(helpers.putDocs('path', 'trashed')).deepEqual([ + // Recursively trash parent/dir; children are trashed first + { path: path.normalize('parent/dir/subdir/file'), trashed: true }, + { path: path.normalize('parent/dir/subdir'), trashed: true }, + { + path: path.normalize('parent/dir/local-child-file'), + trashed: true + }, + { path: path.normalize('parent/dir/empty-subdir'), trashed: true }, + { path: path.normalize('parent/dir'), trashed: true } + ]) + }) + }) }) }) }) diff --git a/test/unit/metadata.js b/test/unit/metadata.js index be87cb4e4..ec44c254b 100644 --- a/test/unit/metadata.js +++ b/test/unit/metadata.js @@ -17,6 +17,7 @@ const { invalidChecksum, invalidPath, markSide, + markAsTrashed, markAsUpToDate, assignPlatformIncompatibilities, detectIncompatibilities, @@ -31,6 +32,7 @@ const { createConflictingDoc } = metadata const { DIR_TYPE, NOTE_MIME_TYPE } = require('../../core/remote/constants') +const { otherSide } = require('../../core/side') const pathUtils = require('../../core/utils/path') const timestamp = require('../../core/utils/timestamp') const Builders = require('../support/builders') @@ -39,7 +41,7 @@ const { onPlatform, onPlatforms } = require('../support/helpers/platform') const pouchHelpers = require('../support/helpers/pouch') /*:: -import type { Metadata, MetadataRemoteFile, MetadataRemoteDir, MetadataLocalInfo } from '../../core/metadata' +import type { Metadata, MetadataRemoteFile, MetadataRemoteDir, MetadataLocalInfo, MetadataRemoteInfo } from '../../core/metadata' import type { RemoteBase } from '../../core/remote/document' */ @@ -1239,6 +1241,69 @@ describe('metadata', function() { }) }) + describe('markAsTrashed', () => { + let doc + + for (const sideName of ['local', 'remote']) { + context(`on the ${sideName} side`, () => { + context('with a synchronized file', () => { + beforeEach(() => { + doc = builders + .metafile() + .upToDate() + .build() + + markAsTrashed(doc, sideName) + }) + + it('marks the document as trashed', () => { + should(doc.trashed).be.true() + }) + + it(`marks the ${sideName} side as trashed`, () => { + // $FlowFixMe we know both sides are present + should(doc[sideName].trashed).be.true() + }) + }) + + context(`with a ${sideName}-only file`, () => { + beforeEach(() => { + doc = builders + .metafile() + .unmerged(sideName) // XXX: we use unmerged to have only one side but markAsTrashed will usually be called on merged documents + .build() + + markAsTrashed(doc, sideName) + }) + + it('marks the document as trashed', () => { + should(doc.trashed).be.true() + }) + + it(`marks the ${sideName} side as trashed`, () => { + // $FlowFixMe we know both sides are present + should(doc[sideName].trashed).be.true() + }) + }) + + context(`with a ${otherSide(sideName)}-only file`, () => { + beforeEach(() => { + doc = builders + .metafile() + .unmerged(otherSide(sideName)) // XXX: we use unmerged to have only one side but markAsTrashed will usually be called on merged documents + .build() + + markAsTrashed(doc, sideName) + }) + + it('marks the document as trashed', () => { + should(doc.trashed).be.true() + }) + }) + }) + } + }) + describe('outOfDateSide', () => { it('returns nothing if sides are not set', () => { const doc1 = builders