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

Do not remove a folder that has files that were not uploaded yet during propagation #4268

Merged
merged 4 commits into from
Feb 25, 2022
Merged

Do not remove a folder that has files that were not uploaded yet during propagation #4268

merged 4 commits into from
Feb 25, 2022

Conversation

allexzander
Copy link
Contributor

@allexzander allexzander commented Feb 11, 2022 •
edited
Loading

Signed-off-by: alex-z blackslayer4@gmail.com

This will fix #4265

@allexzander allexzander requested a review from mgallien February 11, 2022 17:23
@allexzander allexzander force-pushed the bugfix/new-files-removed-with-parent-dir-removal branch from 324c61f to 2bdcada Compare February 11, 2022 17:24
@allexzander allexzander changed the title Do not remove a folder that has files that were not uploaded yet duri… Do not remove a folder that has files that were not uploaded yet during propagation Feb 11, 2022
@allexzander
Copy link
Contributor Author

ProcessDirectoryJob for the topmost (root folder that was deleted, e.g. "test-folder/sub-folder/sub-folder-with-new-files", the top folder would be "test-folder") is processed and deleted, before child jobs begin (e.g. for the "sub-folder-with-new-files" that we wish to keep). Hence, we can't simply propagate _childModified = true; to every parent folder's ProcessDirectoryJob.

The only way I was able to come up with, this is to fix the vector of SyncFileItems in the void OwncloudPropagator::start(SyncFileItemVector &&items).

What I am doing here is going through every item in the vector, checking if it is a new folder that we wish to re-upload (the folder that has new files will always be marked with reupload, because its ProcessDirectoryJob's _childModified is set to true). If we have such a folder, then I am iterating back to its root folder ("test-folder/). Then, if the root folder is set to be removed, I know it is the scenario for the issue to happen. Now, I am going through every subfolder (including root), and setting them to re-upload.

Files and folders that are within the tree and are not new files, will get removed as planned. So, those are not affected.

@allexzander allexzander requested a review from mgallien February 14, 2022 08:24
@allexzander allexzander force-pushed the bugfix/new-files-removed-with-parent-dir-removal branch from e60006f to 630ae83 Compare February 14, 2022 09:00
@allexzander allexzander marked this pull request as ready for review February 14, 2022 09:01
@allexzander allexzander force-pushed the bugfix/new-files-removed-with-parent-dir-removal branch from dde0865 to 4e10653 Compare February 14, 2022 09:37
@allexzander allexzander force-pushed the bugfix/new-files-removed-with-parent-dir-removal branch 2 times, most recently from c801cf8 to 2d68954 Compare February 17, 2022 16:25
mgallien
mgallien reviewed Feb 23, 2022
View reviewed changes
Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

small comments or questions

src/libsync/owncloudpropagator.cpp
if (itemRootFolderName.isEmpty()) {
continue;
}
// #2 iterate backwords (for optimization) and find the root folder by name
Copy link
Collaborator

Choose a reason for hiding this comment

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

are you sure ?
I would have expected the opposite
that in the items container a would be before a/b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mgallien You are right, the folder a will go before folder a/b because the container is sorted. That's why, to find the root of folder a/b/, I must iterate backward to find the a.

image

Copy link
Collaborator

@mgallien mgallien Feb 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

OK but that is not necessarily what you achieve here
you are always using std::rbegin(items) as starting point for the iteration so you will most probably iterate as many items as if you start from std::begin(items)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mgallien True! And, good catch. I will need to fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matthieugallien Fixed

test/testsyncengine.cpp Outdated Show resolved Hide resolved
@allexzander allexzander requested a review from mgallien February 24, 2022 19:19
@mgallien
Copy link
Collaborator

/rebase

@allexzander @mgallien
Do not remove a folder that has files that were not uploaded yet duri…
6c690a1 
…ng propagation.

Signed-off-by: alex-z <blackslayer4@gmail.com>
@allexzander @mgallien
Unit tests for new files restore logic when the parent folder is remo…
1a300d0 
…ved on the server.

Signed-off-by: alex-z <blackslayer4@gmail.com>
@allexzander @mgallien
Fix review comments.
37168fc 
Signed-off-by: alex-z <blackslayer4@gmail.com>
@allexzander @mgallien
Fix review comments from Matthieu. Using curent reverse iterator when…
bf80efc 
… searching for parent.

Signed-off-by: alex-z <blackslayer4@gmail.com>
@nextcloud-command nextcloud-command force-pushed the bugfix/new-files-removed-with-parent-dir-removal branch from 537368f to bf80efc Compare February 25, 2022 15:31
@mgallien mgallien merged commit 108ff4e into master Feb 25, 2022
@mgallien mgallien deleted the bugfix/new-files-removed-with-parent-dir-removal branch February 25, 2022 15:51
@sonarcloud
Copy link

sonarcloud bot commented Feb 25, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

87.9% 87.9% Coverage
0.0% 0.0% Duplication

@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-4268-bf80efc7ab826470a8dc156460a4c001ef7e8408-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@mgallien
Copy link
Collaborator

/backport to stable-3.4

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Feb 25, 2022
Merged
@ghost ghost mentioned this pull request Feb 25, 2022
Closed
@mgallien mgallien added this to the 3.5.0 milestone Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claucambra claucambra claucambra approved these changes

@mgallien mgallien mgallien approved these changes

@camilasan camilasan Awaiting requested review from camilasan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.5.0
Development

Successfully merging this pull request may close these issues.

Do not remove a folder that has files that were not uploaded yet during propagation
4 participants
@allexzander @mgallien @nextcloud-desktop-bot @claucambra