Remove empty directories from the files_versions

[jvillafanez]

Description

Empty directories were left inside the files_versions directory. For large installations this is a waste of space because there could be a lot of empty directories that aren't useful in any way.

Related Issue

https://github.com/owncloud/enterprise/issues/5471

Motivation and Context

How Has This Been Tested?

Mostly tested by deleting files from the web UI

1. Create "/f1/f2/f3" folder and upload a file with some versions
2. Check that the there are versions in <userdir>/files_versions/f1/f2/f3/
3. Remove the uploaded file

There should be only the <userdir>/files_versions folder without any directory in it. This is because the "f1/f2/f3" folders were empty.

Additional checks done:

• With multiple files, removing only one will keep the folder in the files_versions because there are more versions inside
• Removing the directory containing the files (removing "/f1/f2/f3" folder so "/f1/f2" is empty). Only <userdir>/files_versions exists because the rest of the folders were empty.
• Moving the files / directory around. As long as the source parent folder doesn't have versions, the folders will also be removed.
• occ versions:cleanup and occ versions:expire works with the same conditions.

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

Note:
Existing empty version folder won't be removed unless a version ends up inside any of them. In this case, there won't be any difference and the folder will be removed once the last version is removed from the folder.

[pako81]

Should we move this forward?

[jvillafanez]

As far as I know, this just needs review and merge. The failing test is due to the missing windows server.

[DeepDiver1975]

Any chance to add some tests to ensure that:

• empty folders are deleted
• non-empty folders are not deleted

THX

[phil-davis]

Any chance to add some tests

Not sure if you mean unit tests or acceptance tests. The behavior to test is in the back-end implementation, so not directly visible to any API call. But we do have the testing app, and that has https://github.com/owncloud/testing/blob/master/lib/ServerFiles.php which allows access to "see" what files and directories exist in the back-end data storage.

So, I can add some acceptance tests that check the back-end storage behavior of files_versions.

[mrow4a]

Any chance to add some tests

Not sure if you mean unit tests or acceptance tests. The behavior to test is in the back-end implementation, so not directly visible to any API call. But we do have the testing app, and that has https://github.com/owncloud/testing/blob/master/lib/ServerFiles.php which allows access to "see" what files and directories exist in the back-end data storage.

So, I can add some acceptance tests that check the back-end storage behavior of files_versions.

I think what is meant here are unit tests in https://github.com/owncloud/core/blob/master/apps/files_versions/tests/VersioningTest.php

[DeepDiver1975]

Unit tests it would be

[DeepDiver1975]

let's first merge #40531 to limit the rebase effort. THX

[pako81]

#40531 has been merged. Rebase this?

[phil-davis]

LGTM. @DeepDiver1975 please review again.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

63.3% Coverage
0.0% Duplication

[DeepDiver1975]

old topic - self::assertCount() but this is all over our code base ....