diff --git a/core/metadata.js b/core/metadata.js index 74768f3af..5cebccff3 100644 --- a/core/metadata.js +++ b/core/metadata.js @@ -166,7 +166,7 @@ export type Metadata = { trashed?: true, errors?: number, - skipped?: boolean, + skipped?: string, overwrite?: SavedMetadata, childMove?: boolean, incompatibilities?: *, diff --git a/core/migrations/migrations.js b/core/migrations/migrations.js index e0eed0b68..398b52161 100644 --- a/core/migrations/migrations.js +++ b/core/migrations/migrations.js @@ -409,5 +409,24 @@ module.exports = ([ { concurrency: 10 } ) } + }, + { + baseSchemaVersion: 14, + targetSchemaVersion: 15, + description: 'Convert legacy skipped:true boolean to UserSkipped code', + affectedDocs: (docs /*: SavedMetadata[] */) /*: SavedMetadata[] */ => { + return docs.filter(doc => { + // $FlowFixMe legacy `skipped` was a boolean before this migration + return doc.skipped === true + }) + }, + run: (docs /*: SavedMetadata[] */) /*: Promise */ => { + return Promise.resolve( + docs.map(doc => { + doc.skipped = 'UserSkipped' + return doc + }) + ) + } } ] /*: Migration[] */) diff --git a/core/sync/errors.js b/core/sync/errors.js index d3bf4a44c..a84a56ce4 100644 --- a/core/sync/errors.js +++ b/core/sync/errors.js @@ -25,8 +25,10 @@ const EXCLUDED_DIR_CODE = 'ExcludedDir' const INCOMPATIBLE_DOC_CODE = 'IncompatibleDoc' const MISSING_PERMISSIONS_CODE = 'MissingPermissions' const NO_DISK_SPACE_CODE = 'NoDiskSpace' +const SKIPPED_DEPENDENCY_CODE = 'SkippedDependency' const UNSYNCED_PARENT_MOVE_CODE = 'UnsyncedParentMove' const UNKNOWN_SYNC_ERROR_CODE = 'UnknownSyncError' +const USER_SKIPPED_CODE = 'UserSkipped' class UnsyncedParentMoveError extends Error { /*:: @@ -52,7 +54,7 @@ class SyncError extends Error { code: string message: string - sideName: SideName + sideName: ?SideName originalErr: Error doc: SavedMetadata */ @@ -63,7 +65,7 @@ class SyncError extends Error { sideName, err, doc - } /*: { code?: string, sideName: SideName, err: Error, doc: SavedMetadata } */ + } /*: { code?: string, sideName: ?SideName, err: Error, doc: SavedMetadata } */ ) { super(err.message) @@ -157,6 +159,27 @@ const minRetryDelay = ( return Math.min(...causes.map(c => retryDelay(c.err))) } +// Synthetic error emitted when a change is skipped because one of its +// prerequisite changes was skipped. `prereqPath` is carried to the GUI so +// `viewByCode` can build a localized message with the path wrapped in +// backticks, rendered as a clickable chip by `Util.DecorationParser`. +const skippedDependencyErr = ( + change /*: Change */, + prereqPath /*: string */ +) /*: SyncError */ => { + const err = new Error( + `Change skipped: a prerequisite change on ${prereqPath} was skipped` + ) + // $FlowFixMe: Error has no prereqPath by default, we add it on purpose. + err.prereqPath = prereqPath + return new SyncError({ + code: SKIPPED_DEPENDENCY_CODE, + sideName: null, + err, + doc: change.doc + }) +} + const retryAll = async ( causes /*: Array<{| err: RemoteError |} | {| err: SyncError, change: Change |}> */, sync /*: Sync */ @@ -219,7 +242,16 @@ const skip = async ( clearInterval(sync.retryInterval) if (cause.change) { - await sync.skipChange(cause.change, cause.err) + // Wrap the original error so the skip is recorded as user-initiated + // (fatal), regardless of the original error's code (which may be + // non-fatal like MISSING_DOCUMENT). + const wrappedErr = new SyncError({ + code: USER_SKIPPED_CODE, + sideName: cause.err.sideName, + err: cause.err, + doc: cause.change.doc + }) + await sync.skipChange(cause.change, wrappedErr) } // Fire-and-forget: awaiting watcher.start() would deadlock with the @@ -353,12 +385,15 @@ module.exports = { INCOMPATIBLE_DOC_CODE, MISSING_PERMISSIONS_CODE, NO_DISK_SPACE_CODE, + SKIPPED_DEPENDENCY_CODE, UNKNOWN_SYNC_ERROR_CODE, UNSYNCED_PARENT_MOVE_CODE, + USER_SKIPPED_CODE, UnsyncedParentMoveError, SyncError, retryDelay, minRetryDelay, + skippedDependencyErr, retry, retryAll, skip, diff --git a/core/sync/index.js b/core/sync/index.js index 979e79008..21f473fd8 100644 --- a/core/sync/index.js +++ b/core/sync/index.js @@ -75,6 +75,19 @@ const wasSkipped = (change /*: PouchDBFeedData */) => { return change.doc.skipped } +// Skip codes whose dependants should be cascaded (fatal skips). +// Non-fatal skips (e.g. CONFLICTING_NAME, INVALID_METADATA, +// MISSING_DOCUMENT when marked for deletion) are auto-resolved by a +// future merge/re-scan: cascading them would leave dependants stuck. +const FATAL_SKIP_CODES = new Set([ + remoteErrors.MISSING_PARENT_CODE, + syncErrors.SKIPPED_DEPENDENCY_CODE, + syncErrors.USER_SKIPPED_CODE +]) + +const isFatalSkip = (skipped /*: ?string */) => + typeof skipped === 'string' && FATAL_SKIP_CODES.has(skipped) + // Returns the given side metadata of the given PouchDB record. // It is meant to get the outdated side metadata of the record to compare it // against the new metadata and decide which actions to take. @@ -570,12 +583,29 @@ class Sync { if (blockedIds.size === 0) { await this.pouch.setLocalSeq(change.seq) } + blockedIds.add(change.id) this.resolveBlockingCause(change.id) continue } - if (graph.directPrerequisites(change).some(d => blockedIds.has(d.id))) { + const prereqs = graph.directPrerequisites(change) + if (prereqs.some(c => blockedIds.has(c.id))) { blockedIds.add(change.id) + + const skippedPrereq = prereqs.find(c => isFatalSkip(c.doc.skipped)) + if (skippedPrereq) { + const err = syncErrors.skippedDependencyErr( + change, + skippedPrereq.doc.path + ) + await this.skipChange(change, err) + this.events.emit( + 'user-alert', + err, + change.seq, + change.operation.side != null && change.operation.side + ) + } continue } @@ -591,7 +621,8 @@ class Sync { const result = await this.handleSyncError(err, change) if (result === 'fatal') return - if (result === 'blocked') blockedIds.add(change.id) + if (result === 'blocked' || result === 'skipped') + blockedIds.add(change.id) if ( (result === 'skipped' || result === 'recovered') && blockedIds.size === 0 @@ -616,8 +647,8 @@ class Sync { err /*: SyncError */, change /*: Change */ ) /*: Promise<'blocked' | 'skipped' | 'recovered' | 'fatal'> */ { + const sideName = err.sideName || 'local' const { - sideName, doc: { path } } = err @@ -1359,7 +1390,7 @@ class Sync { doc.errors = (doc.errors || 0) + 1 // Make sure isUpToDate(sourceSideName, doc) is still true - const sourceSideName = otherSide(err.sideName) + const sourceSideName = err.sideName ? otherSide(err.sideName) : 'local' metadata.markSide(sourceSideName, doc, doc) change.doc = await this.pouch.put(doc, { checkInvariants: false }) @@ -1399,7 +1430,7 @@ class Sync { this._blockedCauses.delete(change.id) - doc.skipped = true + doc.skipped = err.code await this.pouch.put(doc, { checkInvariants: false }) } diff --git a/core/syncstate.js b/core/syncstate.js index 89b727e51..a1d97189a 100644 --- a/core/syncstate.js +++ b/core/syncstate.js @@ -75,6 +75,7 @@ const makeAlert = ( side: side || null, doc: doc ? { id: doc._id, docType: doc.docType, path: doc.path } : null, links: links || null, + prereqPath: err.prereqPath || null, lastSeenAt: Date.now() } } diff --git a/gui/elm/Data/UserAlert.elm b/gui/elm/Data/UserAlert.elm index 127a4d9cd..da0188e9a 100644 --- a/gui/elm/Data/UserAlert.elm +++ b/gui/elm/Data/UserAlert.elm @@ -20,6 +20,7 @@ import I18n exposing (Helpers) import Icons import Time import Util.Conditional exposing (ShowInWeb, inWeb, onOS) +import Util.DecorationParser exposing (DecorationResult(..), findDecorations) type alias UserAlertCode = @@ -44,7 +45,7 @@ type Side type alias SynchronizationErrorInfo = - { status : UserActionStatus, seq : Int, id : String, docType : String, path : String, side : Maybe Side, lastSeenAt : Time.Posix } + { status : UserActionStatus, seq : Int, id : String, docType : String, path : String, side : Maybe Side, prereqPath : Maybe String, lastSeenAt : Time.Posix } type alias RemoteWarningInfo = @@ -119,6 +120,7 @@ type alias EncodedUserAlert = Maybe { self : String } + , prereqPath : Maybe String , lastSeenAt : Maybe Int } @@ -128,7 +130,7 @@ type alias EncodedCommand = decode : EncodedUserAlert -> Maybe UserAlert -decode { seq, status, code, side, doc, links, lastSeenAt } = +decode { seq, status, code, side, doc, links, prereqPath, lastSeenAt } = let decodedStatus = decodeUserActionStatus status @@ -149,6 +151,7 @@ decode { seq, status, code, side, doc, links, lastSeenAt } = , docType = docType , path = path , side = decodedSide side + , prereqPath = prereqPath , lastSeenAt = decodedLastSeenAt } ) @@ -167,6 +170,7 @@ encode alert = , side = encodedSide a.side , doc = Just { id = a.id, docType = a.docType, path = a.path } , links = Nothing + , prereqPath = a.prereqPath , lastSeenAt = Just (Time.posixToMillis a.lastSeenAt) } @@ -177,6 +181,7 @@ encode alert = , side = Nothing , doc = Nothing , links = Nothing + , prereqPath = Nothing , lastSeenAt = Nothing } @@ -187,6 +192,7 @@ encode alert = , side = Nothing , links = Just { self = a.link } , doc = Nothing + , prereqPath = Nothing , lastSeenAt = Nothing } @@ -350,7 +356,7 @@ viewSyncError helpers platform now alert info = [ text (Path.toString dirPath) ] ] , span [ class "file-line-content u-spacenormal u-errorColorDark u-mt-half" ] - (alertContent helpers content) + (alertContent helpers platform content) , div [ class "u-flex u-mt-half u-pb-1" ] buttons ] @@ -373,7 +379,7 @@ viewRemoteError helpers platform now alert = [ span [ class "file-parent-folder" ] [ text (helpers.t "UserAlert System") ] ] , span [ class "file-line-content u-spacenormal u-errorColorDark u-mt-half" ] - (alertContent helpers content) + (alertContent helpers platform content) , div [ class "u-flex u-mt-half u-pb-1" ] buttons ] @@ -588,6 +594,15 @@ viewByCode helpers alert = [ actionButton helpers (SendCommand Retry alert) "UserAlert Retry" Primary ] } + SynchronizationError "SkippedDependency" { prereqPath } -> + { title = "Error Skipped dependency" + , content = + [ Maybe.withDefault "" prereqPath + |> (\p -> helpers.interpolate [ p ] "Error Change skipped: a prerequisite change on `{0}` was skipped.") + ] + , buttons = [] + } + SynchronizationError "UnknownRemoteError" { docType } -> let localDocType = @@ -617,14 +632,43 @@ localDocTypeLabel docType = "Helpers file" -alertContent : Helpers -> List String -> List (Html Msg) -alertContent helpers details = +alertContent : Helpers -> Platform -> List String -> List (Html Msg) +alertContent helpers platform details = details |> List.map helpers.capitalize - |> List.map text + |> List.map (viewActionContentLine platform) |> List.intersperse (br [] []) +viewActionContentLine : Platform -> String -> Html Msg +viewActionContentLine platform line = + let + toHTML = + \decoration -> + case decoration of + Decorated path -> + Path.fromString platform path + |> decoratedName + + Normal str -> + text str + in + span [] + (findDecorations line + |> List.map toHTML + ) + + +decoratedName : Path -> Html Msg +decoratedName path = + span + [ class "u-bg-frenchPass u-bdrs-4 u-ph-half u-pv-0 u-c-pointer" + , title (Path.toString path) + , onClick (ShowInParent path onOS) + ] + [ text (Path.name path) ] + + classList : List String -> List ( Maybe Bool, String ) -> String classList baseList optionalClasses = let diff --git a/gui/elm/Util/DecorationParser.elm b/gui/elm/Util/DecorationParser.elm new file mode 100644 index 000000000..aa94eb871 --- /dev/null +++ b/gui/elm/Util/DecorationParser.elm @@ -0,0 +1,141 @@ +module Util.DecorationParser exposing (DecorationResult(..), findDecorations) + +import Parser exposing (..) + + +type DecorationResult + = Decorated String + | Normal String + + +decorationChar = + "`" + + +isDecorationStartChar : Char -> Bool +isDecorationStartChar char = + String.fromChar char == decorationChar + + +isNormalChar : Char -> Bool +isNormalChar char = + not (isDecorationStartChar char) + + +trimDecorationChars : String -> () -> String +trimDecorationChars decorated parsed = + decorated |> String.dropLeft 1 |> String.dropRight 1 + + +checkEnding : Bool -> Parser () +checkEnding badEnding = + if badEnding then + problem "normal string should not end with decoration character" + + else + commit () + + +endsWithDecorationChar : Parser Bool +endsWithDecorationChar = + oneOf + [ map (\_ -> True) (symbol decorationChar) + , succeed False + ] + + + +{- A decorated string is a bunch of characters delimited by decoration + characters. Decorated strings are not nestable. +-} + + +decoratedString : Parser DecorationResult +decoratedString = + backtrackable (symbol decorationChar) + |. chompWhile isNormalChar + |. symbol decorationChar + |> mapChompedString trimDecorationChars + |> map Decorated + + + +{- A normal string is a string starting with a bunch of decoration characters (1 + or more) and not ending with another decoration character, or a string not + containing any decoration character. +-} + + +normalString : Parser DecorationResult +normalString = + oneOf + [ backtrackable (symbol decorationChar) + |. chompWhile isDecorationStartChar + |. chompWhile isNormalChar + |. end + , chompWhile isNormalChar + ] + |> getChompedString + |> map Normal + + +decoration : Parser DecorationResult +decoration = + oneOf + [ backtrackable normalString, decoratedString ] + + + +{- The decorations parser returns a list of parts of the parsed string with a + decoration hint (i.e. Decorated or Normal). +-} + + +decorations : Parser (List DecorationResult) +decorations = + loop ( [], 0 ) <| + ifProgress <| + decoration + + +findDecorations : String -> List DecorationResult +findDecorations string = + let + mergeNormalStrings = + \dec prevDecs -> + let + prevDec = + List.head prevDecs + in + case ( dec, prevDec ) of + ( Normal currStr, Just (Normal prevStr) ) -> + Normal (currStr ++ prevStr) :: List.drop 1 prevDecs + + _ -> + dec :: prevDecs + in + case run decorations string of + Result.Ok parsedDecorations -> + List.foldr mergeNormalStrings [] parsedDecorations + + _ -> + [ Normal string ] + + + +-- Loop helper + + +ifProgress : Parser a -> ( List a, Int ) -> Parser (Step ( List a, Int ) (List a)) +ifProgress parser ( prevDecs, offset ) = + succeed (\dec newOffset -> ( dec, newOffset )) + |= parser + |= getOffset + |> map + (\( dec, newOffset ) -> + if offset == newOffset then + Done (List.reverse prevDecs) + + else + Loop ( dec :: prevDecs, newOffset ) + ) diff --git a/gui/locales/en.json b/gui/locales/en.json index 87421dd29..d9264c06b 100644 --- a/gui/locales/en.json +++ b/gui/locales/en.json @@ -123,6 +123,8 @@ "Error Twake Desktop encountered an unexpected error while trying to synchronise the {0}.": "Twake Desktop encountered an unexpected error while trying to synchronise the {0}.", "Error Your hosting provider is working on fixing the issue and the synchronization will automatically be retried periodically.": "Your hosting provider is working on fixing the issue and the synchronization will automatically be retried periodically.", "Error Maintenance in progress": "Maintenance in progress", + "Error Skipped dependency": "Skipped dependency.", + "Error Change skipped: a prerequisite change on `{0}` was skipped.": "Change skipped: a prerequisite change on `{0}` was skipped.", "Error The synchronization of your documents is momentarily paused.": "The synchronization of your documents is momentarily paused.", "Error It will resume once the maintenance is over.": "It will resume once the maintenance is over.", "Error Document path incompatible with current OS": "Document path incompatible with current OS", diff --git a/gui/locales/fr.json b/gui/locales/fr.json index cd0636bca..09219b6da 100644 --- a/gui/locales/fr.json +++ b/gui/locales/fr.json @@ -123,6 +123,8 @@ "Error Twake Desktop encountered an unexpected error while trying to synchronise the {0}.": "Twake Desktop a rencontré une erreur inattendue en essayant de synchroniser le {0}.", "Error Your hosting provider is working on fixing the issue and the synchronization will automatically be retried periodically.": "L'hébergeur de votre Twake Workplace doit être en train de corriger le problème et une nouvelle tentative de synchronisation sera effectuée automatiquement et périodiquement.", "Error Maintenance in progress": "Maintenance en cours", + "Error Skipped dependency": "Dépendance ignorée.", + "Error Change skipped: a prerequisite change on `{0}` was skipped.": "Changement ignoré : un changement préalable sur `{0}` a été ignoré.", "Error The synchronization of your documents is momentarily paused.": "La synchronisation de vos documents est momentanément interrompue.", "Error It will resume once the maintenance is over.": "Elle reprendra automatiquement à la fin de cette maintenance.", "Error Document path incompatible with current OS": "Chemin incompatible avec l'OS courant", diff --git a/test/elm/DecorationParserTest.elm b/test/elm/DecorationParserTest.elm new file mode 100644 index 000000000..dbe9c73a4 --- /dev/null +++ b/test/elm/DecorationParserTest.elm @@ -0,0 +1,37 @@ +module DecorationParserTest exposing (suite) + +import Expect +import Test exposing (..) +import Util.DecorationParser exposing (DecorationResult(..), findDecorations) + + +suite : Test +suite = + describe "Util.DecorationParser" + [ describe "findDecorations" + [ test "parses a decorated segment between backticks" <| + \_ -> + findDecorations "Change skipped: a prerequisite change on `/foo` was skipped" + |> Expect.equal + [ Normal "Change skipped: a prerequisite change on " + , Decorated "/foo" + , Normal " was skipped" + ] + , test "returns a single Normal segment when there are no backticks" <| + \_ -> + findDecorations "No decoration here" + |> Expect.equal [ Normal "No decoration here" ] + , test "parses multiple decorated segments" <| + \_ -> + findDecorations "`a` and `b`" + |> Expect.equal + [ Decorated "a" + , Normal " and " + , Decorated "b" + ] + , test "falls back to a single Normal segment on unbalanced backticks" <| + \_ -> + findDecorations "unbalanced `foo" + |> Expect.equal [ Normal "unbalanced `foo" ] + ] + ] diff --git a/test/integration/add.js b/test/integration/add.js index f69aeeb9d..0d8d7f573 100644 --- a/test/integration/add.js +++ b/test/integration/add.js @@ -8,6 +8,7 @@ const should = require('should') const sinon = require('sinon') const metadata = require('../../core/metadata') +const remoteErrors = require('../../core/remote/errors') const { logger } = require('../../core/utils/logger') const TestHelpers = require('../support/helpers') const configHelpers = require('../support/helpers/config') @@ -415,7 +416,7 @@ describe('Add', () => { path: path.normalize('parent/dir'), local: { path: path.normalize('parent/dir') }, sides: { target: 1, local: 1 }, - skipped: true + skipped: remoteErrors.CONFLICTING_NAME_CODE }, // The conflict is solved when the remote watcher fetches the remote // doc and links it to the local one during Merge. diff --git a/test/integration/multiple_sync_problems.js b/test/integration/multiple_sync_problems.js index 2d76c2e72..47f5e7be5 100644 --- a/test/integration/multiple_sync_problems.js +++ b/test/integration/multiple_sync_problems.js @@ -4,6 +4,7 @@ const should = require('should') const sinon = require('sinon') +const remoteErrors = require('../../core/remote/errors') const syncErrors = require('../../core/sync/errors') const TestHelpers = require('../support/helpers') const configHelpers = require('../support/helpers/config') @@ -183,6 +184,115 @@ describe('Multiple sync problems', () => { }) }) + describe('transitive skip of dependants', () => { + it('does not cascade a non-fatal skip (CONFLICTING_NAME on parent) to its child', async function() { + // Pre-existing remote directory (no pullChanges → no merge → no link). + await helpers.remote.createDirectoryByPath('/non-fatal-parent') + + // Local side: parent dir + child file under it. + await helpers.local.syncDir.ensureDir('non-fatal-parent') + await helpers.local.syncDir.outputFile( + 'non-fatal-parent/child', + 'child content' + ) + await helpers.local.scan() + + const alertPaths = [] + helpers.events.on('user-alert', err => { + if (err.doc) alertPaths.push(err.doc.path) + }) + + // First sync: parent ADD hits CONFLICTING_NAME (dir already on Cozy) + // → non-fatal skip. Child must NOT be alerted SKIPPED_DEPENDENCY. + await helpers.sync() + + const parentDoc = await helpers.pouch.bySyncedPath('non-fatal-parent') + should(parentDoc.skipped).equal(remoteErrors.CONFLICTING_NAME_CODE) + should(alertPaths).not.containEql('non-fatal-parent/child') + + const childDoc = await helpers.pouch.bySyncedPath( + 'non-fatal-parent/child' + ) + should(childDoc.skipped).be.undefined() + + // Recovery: watcher fetches the remote dir, Merge.save clears the + // parent's skipped flag, the child is then synced to Cozy. + await helpers.remote.pullChanges() + await helpers.syncAll() + + const remoteTree = await helpers.remote.treeWithoutTrash() + should(remoteTree).containDeep([ + 'non-fatal-parent/', + 'non-fatal-parent/child' + ]) + }) + + it('cascades a fatal skip (MISSING_PARENT) to its grandchild', async function() { + // Create a parent dir + mid dir + leaf file under it. + await helpers.local.syncDir.ensureDir('fatal-parent') + await helpers.local.syncDir.ensureDir('fatal-parent/mid') + await helpers.local.syncDir.outputFile('fatal-parent/mid/leaf', 'leaf') + await helpers.local.scan() + + // Stub addFolderAsync to throw MISSING_PARENT for the mid dir. + // MISSING_PARENT is fatal: its dependants must be cascaded with + // SKIPPED_DEPENDENCY rather than merely blocked for this batch. + const originalAddDir = helpers.remote.side.addFolderAsync + sinon + .stub(helpers.remote.side, 'addFolderAsync') + .callsFake(async (doc, ...args) => { + if (doc.path && doc.path.includes('fatal-parent/mid')) { + throw new syncErrors.SyncError({ + code: remoteErrors.MISSING_PARENT_CODE, + sideName: 'local', + err: new Error('Parent directory is missing'), + doc + }) + } + return originalAddDir(doc, ...args) + }) + + // Force retry exhaustion so MISSING_PARENT skips instead of blocking. + sinon.stub(helpers._sync, 'scheduleRetry').resolves() + process.env.SYNC_SHOULD_NOT_RETRY = 'true' + + const alertPaths = [] + const alertErrs = [] + helpers.events.on('user-alert', err => { + if (err.doc) { + alertPaths.push(err.doc.path) + alertErrs.push(err) + } + }) + + try { + await helpers.sync() + + // The mid dir was fatally skipped (MISSING_PARENT_CODE). + const midDoc = await helpers.pouch.bySyncedPath('fatal-parent/mid') + should(midDoc.skipped).equal(remoteErrors.MISSING_PARENT_CODE) + + // The leaf was cascaded with SKIPPED_DEPENDENCY (transitive fatal + // cascade), not merely blocked for this batch. + should(alertPaths).containEql('fatal-parent/mid/leaf') + const leafAlert = + alertErrs.find(err => err.doc.path === 'fatal-parent/mid/leaf') || {} + should(leafAlert).not.be.empty() + should(leafAlert.code).equal(syncErrors.SKIPPED_DEPENDENCY_CODE) + should(leafAlert.prereqPath).equal('fatal-parent/mid') + + const leafDoc = await helpers.pouch.bySyncedPath( + 'fatal-parent/mid/leaf' + ) + should(leafDoc.skipped).equal(syncErrors.SKIPPED_DEPENDENCY_CODE) + } finally { + helpers.remote.side.addFolderAsync.restore() + helpers._sync.scheduleRetry.restore() + delete process.env.SYNC_SHOULD_NOT_RETRY + } + }) + }) + describe('cross-side conflicting moves', () => { beforeEach(async () => { await helpers.local.syncDir.ensureDir('dir1') diff --git a/test/support/builders/metadata/base.js b/test/support/builders/metadata/base.js index 9c2c20a81..eaa4832e7 100644 --- a/test/support/builders/metadata/base.js +++ b/test/support/builders/metadata/base.js @@ -276,8 +276,8 @@ module.exports = class BaseMetadataBuilder { return this } - skipped(bool /*: boolean */) /*: this */ { - this.doc.skipped = bool + skipped(code /*: string */) /*: this */ { + this.doc.skipped = code return this } diff --git a/test/unit/migrations/index.js b/test/unit/migrations/index.js index 869f87c7f..f7d9f721b 100644 --- a/test/unit/migrations/index.js +++ b/test/unit/migrations/index.js @@ -619,4 +619,26 @@ describe('core/migrations', function() { }) }) }) + + describe('[migration] Convert legacy skipped:true to UserSkipped', () => { + const migration = migrations.find(m => m.targetSchemaVersion === 15) + if (!migration) throw new Error('migration 15 not found') + + describe('affectedDocs()', () => { + it('returns only docs with skipped === true', () => { + const docs /*: any */ = [{ skipped: true, path: 'a' }, { path: 'b' }] + const affected = migration.affectedDocs(docs) + should(affected).have.length(1) + should(affected[0].path).equal('a') + }) + }) + + describe('run()', () => { + it('sets skipped to UserSkipped', async function() { + const docs /*: any */ = [{ skipped: true, path: 'a' }] + const migrated = await migration.run(docs, this) + should(migrated[0].skipped).equal('UserSkipped') + }) + }) + }) }) diff --git a/test/unit/sync/multiple_errors.js b/test/unit/sync/multiple_errors.js index fd26257fa..efcbdc4f9 100644 --- a/test/unit/sync/multiple_errors.js +++ b/test/unit/sync/multiple_errors.js @@ -414,12 +414,159 @@ describe('Multiple sync errors', function() { }) }) + describe('transitive skip of dependants', () => { + it('skips a dependant when its prerequisite was skipped and emits SKIPPED_DEPENDENCY alert', async function() { + // A (skipped) → B (depends on A, path under A) + const docA = await builders + .metafile() + .path('skiproot') + .sides({ local: 1 }) + .create() + const docB = await builders + .metafile() + .path('skiproot/child') + .sides({ local: 1 }) + .create() + + const changeA = { + changes: [{ rev: docA._rev }], + doc: docA, + id: docA._id, + seq: 40, + operation: { type: 'ADD', side: 'local' } + } + const changeB = { + changes: [{ rev: docB._rev }], + doc: docB, + id: docB._id, + seq: 41, + operation: { type: 'ADD', side: 'local' } + } + + // A is already marked skipped (e.g. user skipped it earlier). + docA.skipped = syncErrors.USER_SKIPPED_CODE + await this.pouch.put(docA) + + sinon + .stub(this.sync, 'getNextChanges') + .onFirstCall() + .resolves([changeA, changeB]) + .onSecondCall() + .resolves([]) + + const applyStub = sinon.stub(this.sync, 'apply') + applyStub.callsFake(async change => { + if (change.id === docB._id) { + throw new Error('B should not be applied, only skipped') + } + }) + + sinon.stub(this.sync, 'scheduleRetry').resolves() + const emitSpy = sinon.spy(this.events, 'emit') + + await this.sync.syncBatch() + + // B was skipped transitively (not applied). + should(applyStub).not.have.been.called() + + // B's doc is now marked skipped. + const skippedB = await this.pouch.bySyncedPath(docB.path) + should(skippedB.skipped).equal(syncErrors.SKIPPED_DEPENDENCY_CODE) + + // A SKIPPED_DEPENDENCY alert was emitted for B with A's path in backticks. + const alertCalls = emitSpy.args.filter(args => args[0] === 'user-alert') + should(alertCalls).have.length(1) + const alertErr = alertCalls[0][1] + should(alertErr.code).equal(syncErrors.SKIPPED_DEPENDENCY_CODE) + should(alertErr.prereqPath).equal('skiproot') + + this.sync.getNextChanges.restore() + this.sync.apply.restore() + this.sync.scheduleRetry.restore() + emitSpy.restore() + }) + + it('skips deep transitive chain [A(skip) → B(dep A) → C(dep B)]', async function() { + const docA = await builders + .metafile() + .path('deep') + .sides({ local: 1 }) + .create() + const docB = await builders + .metafile() + .path('deep/mid') + .sides({ local: 1 }) + .create() + const docC = await builders + .metafile() + .path('deep/mid/leaf') + .sides({ local: 1 }) + .create() + + const changeA = { + changes: [{ rev: docA._rev }], + doc: docA, + id: docA._id, + seq: 50, + operation: { type: 'ADD', side: 'local' } + } + const changeB = { + changes: [{ rev: docB._rev }], + doc: docB, + id: docB._id, + seq: 51, + operation: { type: 'ADD', side: 'local' } + } + const changeC = { + changes: [{ rev: docC._rev }], + doc: docC, + id: docC._id, + seq: 52, + operation: { type: 'ADD', side: 'local' } + } + + docA.skipped = syncErrors.USER_SKIPPED_CODE + await this.pouch.put(docA) + + sinon + .stub(this.sync, 'getNextChanges') + .onFirstCall() + .resolves([changeA, changeB, changeC]) + .onSecondCall() + .resolves([]) + + const applyStub = sinon.stub(this.sync, 'apply').resolves(true) + sinon.stub(this.sync, 'scheduleRetry').resolves() + const emitSpy = sinon.spy(this.events, 'emit') + + await this.sync.syncBatch() + + // B and C were not applied (skipped transitively). + should(applyStub).not.have.been.called() + + // Both B and C are marked skipped. + const skippedB = await this.pouch.bySyncedPath(docB.path) + const skippedC = await this.pouch.bySyncedPath(docC.path) + should(skippedB.skipped).equal(syncErrors.SKIPPED_DEPENDENCY_CODE) + should(skippedC.skipped).equal(syncErrors.SKIPPED_DEPENDENCY_CODE) + + // Two SKIPPED_DEPENDENCY alerts (B and C). + const alertCalls = emitSpy.args.filter(args => args[0] === 'user-alert') + should(alertCalls).have.length(2) + + this.sync.getNextChanges.restore() + this.sync.apply.restore() + this.sync.scheduleRetry.restore() + emitSpy.restore() + }) + }) + describe('retry exhaustion', () => { it('calls setLocalSeq when a change was skipped', async function() { const doc = await builders .metafile() .path('exhausted') - .skipped(true) + .skipped(syncErrors.SKIPPED_DEPENDENCY_CODE) .sides({ local: 1 }) .create()