Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix edge case file upload bugs #2527

Closed
wants to merge 2 commits into from
Closed

Fix edge case file upload bugs #2527

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9426134
Fix ignored file upload
JC-comp Oct 23, 2023
38bc4a3
Fix moving file 409 conflict
JC-comp Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 41 additions & 19 deletions src/sync.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4591,10 +4591,17 @@ class SyncEngine {

// No 404 or otherwise was triggered, meaning that the file already exists online and passes the POSIX test ...
log.vdebug("fileDetailsFromOneDrive after exist online check: ", fileDetailsFromOneDrive);

// Save item to the database
saveItem(fileDetailsFromOneDrive);

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think the updated logic here is correct.

If the response from fileDetailsFromOneDrive = checkFileOneDriveApiInstance.getPathDetailsByDriveId(parentItem.driveId, fileToUpload) generates a 200 we drop to the POSIX test.

In your code, you are then saving this data blindly to the DB, without validating that the details from online match what was intended to upload - so that DB save should not be happening until the validation of the data is done

Please can you review your thinking here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea was that if we maintain the latest online version in the local database, it can help force triggering a reupload
later during the database inconsistency check in case the corresponding upload process fail. Otherwise, there might be a risk of losing this inconsistency.

// Does the data from online match our local file?
if (performUploadIntegrityValidationChecks(fileDetailsFromOneDrive, fileToUpload, thisFileSize)) {
// Save item to the database
saveItem(fileDetailsFromOneDrive);
if (disableUploadValidation || !performUploadIntegrityValidationChecks(fileDetailsFromOneDrive, fileToUpload, thisFileSize)) {
// The item already exists online, replace it instead
string changedItemParentId = fileDetailsFromOneDrive["parentReference"]["driveId"].str;
string changedItemId = fileDetailsFromOneDrive["id"].str;
databaseItemsWhereContentHasChanged ~= [changedItemParentId, changedItemId, fileToUpload];
processChangedLocalItemsToUpload();
}
} catch (OneDriveException exception) {
// If we get a 404 .. the file is not online .. this is what we want .. file does not exist online
Expand Down Expand Up @@ -6427,34 +6434,49 @@ class SyncEngine {
];

// Perform the move operation on OneDrive
bool isMoveSuccess = false;
JSONValue response;

// Create a new API Instance for this thread and initialise it
OneDriveApi movePathOnlineApiInstance;
movePathOnlineApiInstance = new OneDriveApi(appConfig);
movePathOnlineApiInstance.initialise();

try {
response = movePathOnlineApiInstance.updateById(oldItem.driveId, oldItem.id, data, oldItem.eTag);
} catch (OneDriveException e) {
if (e.httpStatusCode == 412) {
// OneDrive threw a 412 error, most likely: ETag does not match current item's value
// Retry without eTag
log.vdebug("File Move Failed - OneDrive eTag / cTag match issue");
log.vlog("OneDrive returned a 'HTTP 412 - Precondition Failed' when attempting to move the file - gracefully handling error");
string nullTag = null;
// move the file but without the eTag
response = movePathOnlineApiInstance.updateById(oldItem.driveId, oldItem.id, data, nullTag);
}
}
const(char)[] eTag = oldItem.eTag;
for (int i = 0; i < 3; i++) {
try {
response = movePathOnlineApiInstance.updateById(oldItem.driveId, oldItem.id, data, eTag);
isMoveSuccess = true;
break;
} catch (OneDriveException e) {
if (e.httpStatusCode == 412) {
// OneDrive threw a 412 error, most likely: ETag does not match current item's value
// Retry without eTag
log.vdebug("File Move Failed - OneDrive eTag / cTag match issue");
log.vlog("OneDrive returned a 'HTTP 412 - Precondition Failed' when attempting to move the file - gracefully handling error");
// move the file but without the eTag
eTag = null;
} else if (e.httpStatusCode == 409) {
// Destination item already exists, delete it first
log.log("Moved local item overwrote an existing item - deleting old online item");
uploadDeletedItem(newItem, newPath);
} else
break;
}
}
// Shutdown API instance
movePathOnlineApiInstance.shutdown();
// Free object and memory
object.destroy(movePathOnlineApiInstance);

// save the move response from OneDrive in the database
// Is the response a valid JSON object - validation checking done in saveItem
saveItem(response);
if (isMoveSuccess) {
// save the move response from OneDrive in the database
// Is the response a valid JSON object - validation checking done in saveItem
saveItem(response);
log.log("Moving ", oldPath, " to ", newPath, " ... done.");
} else {
log.error("ERROR: Unable to move from ", oldPath, " to ", newPath);
}
}
} else {
// Moved item is unwanted
Expand Down