From fb7dcdabb376a3539e5e441a0210809853da1a9c Mon Sep 17 00:00:00 2001 From: abraunegg Date: Thu, 14 Nov 2024 06:45:04 +1100 Subject: [PATCH 1/2] Ensure bypass_data_preservation operates as intended * Ensure that if bypass_data_preservation is set to true, local file backups are not created when replacing local files. --- src/sync.d | 80 +++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 55 insertions(+), 25 deletions(-) diff --git a/src/sync.d b/src/sync.d index 99bb7f260..2d1061b02 100644 --- a/src/sync.d +++ b/src/sync.d @@ -1424,10 +1424,16 @@ class SyncEngine { if (debugLogging) {addLogEntry("Flagging to delete item locally: " ~ to!string(onedriveJSONItem), ["debug"]);} idsToDelete ~= [thisItemDriveId, thisItemId]; } else { - // local data protection is configured, safeBackup the local file, passing in if we are performing a --dry-run or not - // In case the renamed path is needed - string renamedPath; - safeBackup(localPathToDelete, dryRun, renamedPath); + // Has the user configured to IGNORE local data protection rules? + if (bypassDataPreservation) { + // The user has configured to ignore data safety checks and overwrite local data rather than preserve & safeBackup + addLogEntry("WARNING: Local Data Protection has been disabled. You may experience data loss on this file: " ~ localPathToDelete, ["info", "notify"]); + } else { + // local data protection is configured, safeBackup the local file, passing in if we are performing a --dry-run or not + // In case the renamed path is needed + string renamedPath; + safeBackup(localPathToDelete, dryRun, renamedPath); + } } } else { // Flag to ignore @@ -2249,20 +2255,32 @@ class SyncEngine { // The destination item is in-sync if (verboseLogging) {addLogEntry("Destination is in sync and will be overwritten", ["verbose"]);} } else { - // The destination item is different - if (verboseLogging) {addLogEntry("The destination is occupied with a different item, renaming the conflicting file...", ["verbose"]);} + // Has the user configured to IGNORE local data protection rules? + if (bypassDataPreservation) { + // The user has configured to ignore data safety checks and overwrite local data rather than preserve & safeBackup + addLogEntry("WARNING: Local Data Protection has been disabled. You may experience data loss on this file: " ~ changedItemPath, ["info", "notify"]); + } else { + // The destination item is different + if (verboseLogging) {addLogEntry("The destination is occupied with a different item, renaming the conflicting file...", ["verbose"]);} + // Backup this item, passing in if we are performing a --dry-run or not + // In case the renamed path is needed + string renamedPath; + safeBackup(changedItemPath, dryRun, renamedPath); + } + } + } else { + // Has the user configured to IGNORE local data protection rules? + if (bypassDataPreservation) { + // The user has configured to ignore data safety checks and overwrite local data rather than preserve & safeBackup + addLogEntry("WARNING: Local Data Protection has been disabled. You may experience data loss on this file: " ~ changedItemPath, ["info", "notify"]); + } else { + // The to be overwritten item is not already in the itemdb, so it should saved to avoid data loss + if (verboseLogging) {addLogEntry("The destination is occupied by an existing un-synced file, renaming the conflicting file...", ["verbose"]);} // Backup this item, passing in if we are performing a --dry-run or not // In case the renamed path is needed string renamedPath; safeBackup(changedItemPath, dryRun, renamedPath); } - } else { - // The to be overwritten item is not already in the itemdb, so it should saved to avoid data loss - if (verboseLogging) {addLogEntry("The destination is occupied by an existing un-synced file, renaming the conflicting file...", ["verbose"]);} - // Backup this item, passing in if we are performing a --dry-run or not - // In case the renamed path is needed - string renamedPath; - safeBackup(changedItemPath, dryRun, renamedPath); } } @@ -2455,13 +2473,18 @@ class SyncEngine { // Does the DB (what we think is in sync) hash match the existing local file hash? if (!testFileHash(newItemPath, databaseItem)) { - // local file is different to what we know to be true - addLogEntry("The local file to replace (" ~ newItemPath ~ ") has been modified locally since the last download. Renaming it to avoid potential local data loss."); - - // Perform the local safeBackup of the existing local file, passing in if we are performing a --dry-run or not - // In case the renamed path is needed - string renamedPath; - safeBackup(newItemPath, dryRun, renamedPath); + // Has the user configured to IGNORE local data protection rules? + if (bypassDataPreservation) { + // The user has configured to ignore data safety checks and overwrite local data rather than preserve & safeBackup + addLogEntry("WARNING: Local Data Protection has been disabled. You may experience data loss on this file: " ~ newItemPath, ["info", "notify"]); + } else { + // local file is different to what we know to be true + addLogEntry("The local file to replace (" ~ newItemPath ~ ") has been modified locally since the last download. Renaming it to avoid potential local data loss."); + // Perform the local safeBackup of the existing local file, passing in if we are performing a --dry-run or not + // In case the renamed path is needed + string renamedPath; + safeBackup(newItemPath, dryRun, renamedPath); + } } } @@ -9449,11 +9472,18 @@ class SyncEngine { // Attempt to apply this changed item applyPotentiallyChangedItem(existingDatabaseItem, existingItemPath, downloadSharedFileDbItem, newItemPath, fileToDownload); } else { - // File exists locally, it is not in sync, there is no record in the DB of this file - // In case the renamed path is needed - string renamedPath; - // Rename the local file - safeBackup(newItemPath, dryRun, renamedPath); + // Has the user configured to IGNORE local data protection rules? + if (bypassDataPreservation) { + // The user has configured to ignore data safety checks and overwrite local data rather than preserve & safeBackup + addLogEntry("WARNING: Local Data Protection has been disabled. You may experience data loss on this file: " ~ newItemPath, ["info", "notify"]); + } else { + // File exists locally, it is not in sync, there is no record in the DB of this file + // In case the renamed path is needed + string renamedPath; + // Rename the local file + safeBackup(newItemPath, dryRun, renamedPath); + } + // Submit this shared file to be processed further for downloading applyPotentiallyNewLocalItem(downloadSharedFileDbItem, fileToDownload, newItemPath); } From 74a91f3245e48690e54af2e44d1a7f9f26962d5a Mon Sep 17 00:00:00 2001 From: abraunegg Date: Thu, 14 Nov 2024 07:20:50 +1100 Subject: [PATCH 2/2] Update PR * Refactor use of safeBackup passing in bypassDataPreservation to determine if the backup should be taken to ensure consistency in messaging --- src/sync.d | 119 +++++++++++++++++------------------------------------ src/util.d | 98 +++++++++++++++++++++++-------------------- 2 files changed, 91 insertions(+), 126 deletions(-) diff --git a/src/sync.d b/src/sync.d index 2d1061b02..f4975d1e5 100644 --- a/src/sync.d +++ b/src/sync.d @@ -1424,16 +1424,10 @@ class SyncEngine { if (debugLogging) {addLogEntry("Flagging to delete item locally: " ~ to!string(onedriveJSONItem), ["debug"]);} idsToDelete ~= [thisItemDriveId, thisItemId]; } else { - // Has the user configured to IGNORE local data protection rules? - if (bypassDataPreservation) { - // The user has configured to ignore data safety checks and overwrite local data rather than preserve & safeBackup - addLogEntry("WARNING: Local Data Protection has been disabled. You may experience data loss on this file: " ~ localPathToDelete, ["info", "notify"]); - } else { - // local data protection is configured, safeBackup the local file, passing in if we are performing a --dry-run or not - // In case the renamed path is needed - string renamedPath; - safeBackup(localPathToDelete, dryRun, renamedPath); - } + // If local data protection is configured (bypassDataPreservation = false), safeBackup the local file, passing in if we are performing a --dry-run or not + // In case the renamed path is needed + string renamedPath; + safeBackup(localPathToDelete, dryRun, bypassDataPreservation, renamedPath); } } else { // Flag to ignore @@ -2114,16 +2108,10 @@ class SyncEngine { // 3. The local modified time > remote modified time // 4. The id of the item from OneDrive is not in the database - // Has the user configured to IGNORE local data protection rules? - if (bypassDataPreservation) { - // The user has configured to ignore data safety checks and overwrite local data rather than preserve & safeBackup - addLogEntry("WARNING: Local Data Protection has been disabled. You may experience data loss on this file: " ~ newItemPath, ["info", "notify"]); - } else { - // local data protection is configured, safeBackup the local file, passing in if we are performing a --dry-run or not - // In case the renamed path is needed - string renamedPath; - safeBackup(newItemPath, dryRun, renamedPath); - } + // If local data protection is configured (bypassDataPreservation = false), safeBackup the local file, passing in if we are performing a --dry-run or not + // In case the renamed path is needed + string renamedPath; + safeBackup(newItemPath, dryRun, bypassDataPreservation, renamedPath); } } else { // Is the remote newer? @@ -2135,16 +2123,10 @@ class SyncEngine { addLogEntry("itemModifiedTime (OneDrive item): " ~ to!string(itemModifiedTime), ["debug"]); } - // Has the user configured to IGNORE local data protection rules? - if (bypassDataPreservation) { - // The user has configured to ignore data safety checks and overwrite local data rather than preserve & safeBackup - addLogEntry("WARNING: Local Data Protection has been disabled. You may experience data loss on this file: " ~ newItemPath, ["info", "notify"]); - } else { - // local data protection is configured, safeBackup the local file, passing in if we are performing a --dry-run or not - // In case the renamed path is needed - string renamedPath; - safeBackup(newItemPath, dryRun, renamedPath); - } + // If local data protection is configured (bypassDataPreservation = false), safeBackup the local file, passing in if we are performing a --dry-run or not + // In case the renamed path is needed + string renamedPath; + safeBackup(newItemPath, dryRun, bypassDataPreservation, renamedPath); } // Are the timestamps equal? @@ -2255,32 +2237,20 @@ class SyncEngine { // The destination item is in-sync if (verboseLogging) {addLogEntry("Destination is in sync and will be overwritten", ["verbose"]);} } else { - // Has the user configured to IGNORE local data protection rules? - if (bypassDataPreservation) { - // The user has configured to ignore data safety checks and overwrite local data rather than preserve & safeBackup - addLogEntry("WARNING: Local Data Protection has been disabled. You may experience data loss on this file: " ~ changedItemPath, ["info", "notify"]); - } else { - // The destination item is different - if (verboseLogging) {addLogEntry("The destination is occupied with a different item, renaming the conflicting file...", ["verbose"]);} - // Backup this item, passing in if we are performing a --dry-run or not - // In case the renamed path is needed - string renamedPath; - safeBackup(changedItemPath, dryRun, renamedPath); - } - } - } else { - // Has the user configured to IGNORE local data protection rules? - if (bypassDataPreservation) { - // The user has configured to ignore data safety checks and overwrite local data rather than preserve & safeBackup - addLogEntry("WARNING: Local Data Protection has been disabled. You may experience data loss on this file: " ~ changedItemPath, ["info", "notify"]); - } else { - // The to be overwritten item is not already in the itemdb, so it should saved to avoid data loss - if (verboseLogging) {addLogEntry("The destination is occupied by an existing un-synced file, renaming the conflicting file...", ["verbose"]);} - // Backup this item, passing in if we are performing a --dry-run or not + // The destination item is different + if (verboseLogging) {addLogEntry("The destination is occupied with a different item, renaming the conflicting file...", ["verbose"]);} + // If local data protection is configured (bypassDataPreservation = false), safeBackup the local file, passing in if we are performing a --dry-run or not // In case the renamed path is needed string renamedPath; - safeBackup(changedItemPath, dryRun, renamedPath); + safeBackup(changedItemPath, dryRun, bypassDataPreservation, renamedPath); } + } else { + // The to be overwritten item is not already in the itemdb, so it should saved to avoid data loss + if (verboseLogging) {addLogEntry("The destination is occupied by an existing un-synced file, renaming the conflicting file...", ["verbose"]);} + // If local data protection is configured (bypassDataPreservation = false), safeBackup the local file, passing in if we are performing a --dry-run or not + // In case the renamed path is needed + string renamedPath; + safeBackup(changedItemPath, dryRun, bypassDataPreservation, renamedPath); } } @@ -2473,18 +2443,12 @@ class SyncEngine { // Does the DB (what we think is in sync) hash match the existing local file hash? if (!testFileHash(newItemPath, databaseItem)) { - // Has the user configured to IGNORE local data protection rules? - if (bypassDataPreservation) { - // The user has configured to ignore data safety checks and overwrite local data rather than preserve & safeBackup - addLogEntry("WARNING: Local Data Protection has been disabled. You may experience data loss on this file: " ~ newItemPath, ["info", "notify"]); - } else { - // local file is different to what we know to be true - addLogEntry("The local file to replace (" ~ newItemPath ~ ") has been modified locally since the last download. Renaming it to avoid potential local data loss."); - // Perform the local safeBackup of the existing local file, passing in if we are performing a --dry-run or not - // In case the renamed path is needed - string renamedPath; - safeBackup(newItemPath, dryRun, renamedPath); - } + // local file is different to what we know to be true + addLogEntry("The local file to replace (" ~ newItemPath ~ ") has been modified locally since the last download. Renaming it to avoid potential local data loss."); + // If local data protection is configured (bypassDataPreservation = false), safeBackup the local file, passing in if we are performing a --dry-run or not + // In case the renamed path is needed + string renamedPath; + safeBackup(newItemPath, dryRun, bypassDataPreservation, renamedPath); } } @@ -4576,8 +4540,8 @@ class SyncEngine { // Online is newer, rename local, then upload the renamed file // We need to know the renamed path so we can upload it string renamedPath; - // Rename the local path - safeBackup(localFilePath, dryRun, renamedPath); + // Rename the local path - we WANT this to occur regardless of bypassDataPreservation setting + safeBackup(localFilePath, dryRun, false, renamedPath); // Upload renamed local file as a new file uploadNewFile(renamedPath); @@ -5949,8 +5913,8 @@ class SyncEngine { // Online is newer, rename local, then upload the renamed file // We need to know the renamed path so we can upload it string renamedPath; - // Rename the local path - safeBackup(fileToUpload, dryRun, renamedPath); + // Rename the local path - we WANT this to occur regardless of bypassDataPreservation setting + safeBackup(fileToUpload, dryRun, false, renamedPath); // Upload renamed local file as a new file uploadNewFile(renamedPath); // Process the database entry removal for the original file. In a --dry-run scenario, this is being done against a DB copy. @@ -9472,18 +9436,11 @@ class SyncEngine { // Attempt to apply this changed item applyPotentiallyChangedItem(existingDatabaseItem, existingItemPath, downloadSharedFileDbItem, newItemPath, fileToDownload); } else { - // Has the user configured to IGNORE local data protection rules? - if (bypassDataPreservation) { - // The user has configured to ignore data safety checks and overwrite local data rather than preserve & safeBackup - addLogEntry("WARNING: Local Data Protection has been disabled. You may experience data loss on this file: " ~ newItemPath, ["info", "notify"]); - } else { - // File exists locally, it is not in sync, there is no record in the DB of this file - // In case the renamed path is needed - string renamedPath; - // Rename the local file - safeBackup(newItemPath, dryRun, renamedPath); - } - + // File exists locally, it is not in sync, there is no record in the DB of this file + // In case the renamed path is needed + string renamedPath; + // If local data protection is configured (bypassDataPreservation = false), safeBackup the local file, passing in if we are performing a --dry-run or not + safeBackup(newItemPath, dryRun, bypassDataPreservation, renamedPath); // Submit this shared file to be processed further for downloading applyPotentiallyNewLocalItem(downloadSharedFileDbItem, fileToDownload, newItemPath); } diff --git a/src/util.d b/src/util.d index 322e25d79..92c6853b5 100644 --- a/src/util.d +++ b/src/util.d @@ -55,55 +55,63 @@ shared static this() { } // Creates a safe backup of the given item, and only performs the function if not in a --dry-run scenario -void safeBackup(const(char)[] path, bool dryRun, out string renamedPath) { - auto ext = extension(path); - auto newPath = path.chomp(ext) ~ "-" ~ deviceName ~ "-safeBackup-"; - int n = 1; - - // Limit to 1000 iterations .. 1000 file backups - while (exists(newPath ~ format("%04d", n) ~ ext) && n < 1000) { - n++; - } - - // Check if unique file name was found - if (exists(newPath ~ format("%04d", n) ~ ext)) { - // On the 1000th backup of this file, this should be triggered - addLogEntry("Failed to backup " ~ to!string(path) ~ ": Unique file name could not be found after 1000 attempts", ["error"]); - return; // Exit function as a unique file name could not be found - } +void safeBackup(const(char)[] path, bool dryRun, bool bypassDataPreservation, out string renamedPath) { + // Do we actually perform a safe backup? + // Has the user configured to IGNORE local data protection rules? + if (bypassDataPreservation) { + // The user has configured to ignore data safety checks and overwrite local data rather than preserve & safeBackup + addLogEntry("WARNING: Local Data Protection has been disabled - not renaming local file. You may experience data loss on this file: " ~ to!string(path), ["info", "notify"]); + } else { + // configure variables + auto ext = extension(path); + auto newPath = path.chomp(ext) ~ "-" ~ deviceName ~ "-safeBackup-"; + int n = 1; + + // Limit to 1000 iterations .. 1000 file backups + while (exists(newPath ~ format("%04d", n) ~ ext) && n < 1000) { + n++; + } - // Configure the new name with zero-padded counter - newPath ~= format("%04d", n) ~ ext; + // Check if unique file name was found + if (exists(newPath ~ format("%04d", n) ~ ext)) { + // On the 1000th backup of this file, this should be triggered + addLogEntry("Failed to backup " ~ to!string(path) ~ ": Unique file name could not be found after 1000 attempts", ["error"]); + return; // Exit function as a unique file name could not be found + } - // Log that we are performing the backup by renaming the file - if (verboseLogging) { - addLogEntry("The local item is out-of-sync with OneDrive, renaming to preserve existing file and prevent local data loss: " ~ to!string(path) ~ " -> " ~ to!string(newPath), ["verbose"]); - } + // Configure the new name with zero-padded counter + newPath ~= format("%04d", n) ~ ext; - if (!dryRun) { - // Not a --dry-run scenario - do the file rename - // - // There are 2 options to rename a file - // rename() - https://dlang.org/library/std/file/rename.html - // std.file.copy() - https://dlang.org/library/std/file/copy.html - // - // rename: - // It is not possible to rename a file across different mount points or drives. On POSIX, the operation is atomic. That means, if to already exists there will be no time period during the operation where to is missing. - // - // std.file.copy - // Copy file from to file to. File timestamps are preserved. File attributes are preserved, if preserve equals Yes.preserveAttributes - // - // Use rename() as Linux is POSIX compliant, we have an atomic operation where at no point in time the 'to' is missing. - try { - rename(path, newPath); - renamedPath = to!string(newPath); - } catch (Exception e) { - // Handle exceptions, e.g., log error - addLogEntry("Renaming of local file failed for " ~ to!string(path) ~ ": " ~ e.msg, ["error"]); + // Log that we are performing the backup by renaming the file + if (verboseLogging) { + addLogEntry("The local item is out-of-sync with OneDrive, renaming to preserve existing file and prevent local data loss: " ~ to!string(path) ~ " -> " ~ to!string(newPath), ["verbose"]); } - } else { - if (debugLogging) { - addLogEntry("DRY-RUN: Skipping renaming local file to preserve existing file and prevent data loss: " ~ to!string(path) ~ " -> " ~ to!string(newPath), ["debug"]); + + if (!dryRun) { + // Not a --dry-run scenario - do the file rename + // + // There are 2 options to rename a file + // rename() - https://dlang.org/library/std/file/rename.html + // std.file.copy() - https://dlang.org/library/std/file/copy.html + // + // rename: + // It is not possible to rename a file across different mount points or drives. On POSIX, the operation is atomic. That means, if to already exists there will be no time period during the operation where to is missing. + // + // std.file.copy + // Copy file from to file to. File timestamps are preserved. File attributes are preserved, if preserve equals Yes.preserveAttributes + // + // Use rename() as Linux is POSIX compliant, we have an atomic operation where at no point in time the 'to' is missing. + try { + rename(path, newPath); + renamedPath = to!string(newPath); + } catch (Exception e) { + // Handle exceptions, e.g., log error + addLogEntry("Renaming of local file failed for " ~ to!string(path) ~ ": " ~ e.msg, ["error"]); + } + } else { + if (debugLogging) { + addLogEntry("DRY-RUN: Skipping renaming local file to preserve existing file and prevent data loss: " ~ to!string(path) ~ " -> " ~ to!string(newPath), ["debug"]); + } } } }