core Restore and delete operations in trashbin will affect versions

Description

Versions of files were left in the trashbin even though the associated file was either restored or deleted. This causes a performance loss for large scenarios because we have to deal with too many versions while restoring or deleting files from the trashbin.

Related Issue

https://github.com/owncloud/core/issues/40286

Motivation and Context

Versions keep piling up without any chance to clean them up.

How Has This Been Tested?

test environment:
test case 1:
test case 2:
...

Screenshots (if appropriate):

Types of changes

[x] 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:

[x] Code changes
[ ] Unit tests added
[ ] Acceptance tests added
[ ] Documentation ticket raised:
[ ] Changelog item, see TEMPLATE

Aug 16 '22 12:08 jvillafanez

:boom: Acceptance tests pipeline webUIUpload-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/36601/159

Aug 16 '22 12:08 ownclouders

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

75.3% Coverage
0.0% Duplication

Aug 17 '22 15:08 sonarqubecloud[bot]

I don't think I can add unit tests to cover these cases. The files_trashbin app is handling the restoration and deletion of the versions (which is wrong, at least the way it's being done), and core doesn't seem to provide a way to access the versions because they seem to be completely handled by the files_versions app. If I want to restore a file with multiple versions, I have to know where those versions will end up. If the file is deleted along with the versions, I'd need a way to verify that those versions aren't present anywhere.

@phil-davis is it possible to include some acceptance tests to cover these scenarios? Maybe we have better luck using API calls.

Aug 17 '22 16:08 jvillafanez

is it possible to include some acceptance tests to cover these scenarios?

We can try - but I suspect that after restoring or deleting a file from the trashbin, there will be no way to externally access the versions that might be left behind in the trashbin. First we need to try on a current core system to access such "lost" trashed versions with the API. If we can do that, then we can make a test scenario that accesses those, then reverse the checks of those steps (so that they expect the access attempts to fail), then confirm that the test fails on current master, and passes with the code in this PR.

I put issue #40286 in out QA automation sprint to get someone to try.

Aug 23 '22 11:08 phil-davis

A test was added in PR https://github.com/owncloud/core/pull/40322 that confirms that versions are restored after a file is deleted and then restored from the trashbin. That is "a good thing" and already works without this PR.

We did not find a way to have an end-to-end automated test that verifies that, when a file is deleted from the trashbin, the versions are also deleted from storage and from the database. The existence of versions of a trashbin file seems to be hidden from access via the external API - a user cannot see the versions that might be saved in the trashbin along with a file in the trashbin. So end-to-end tests cannot verify that versions exist in the trashbin, and cannot then verify that the versions are "really" gone when the file is deleted from the trashbin.

This is a common challege for any end-to-end test that just accesses an API. On any server that stores persistent data, the data can be verified to exist by accessing it from the API, and checking that the response has the expected data. Then use the API to delete the data, and verify that a GET via the API now returns a "not found". But the server could have only removed the data reference from whatever "database" it uses to know what data items exist. The data might still exist and be taking up space in the persistent storage - there is no way for any "ordinary" end-to-test to know that.

Sep 05 '22 07:09 phil-davis

IMO this code is good-to-go. It will need manual verification during the release testing.

Needs a developer review.

When do we want this released? @pmaier1 @jnweiger

Sep 05 '22 07:09 phil-davis

@jvillafanez how do existing "stranded" version files get cleaned up from the trashbin after someone upgrades to this code?

Sep 05 '22 07:09 phil-davis

@jvillafanez how to existing "stranded" version files get cleaned up from the trashbin after someone upgrades to this code?

I think that those stranded versions come from files within trashed folders. If I remember correctly, deleting or restoring that folder (the one that shows in the root of the trashbin) should fix the issue.

Sep 19 '22 07:09 jvillafanez

@jvillafanez lots of code has been merged-back to master with the release of 10.11.0 yesterday. Please rebase so that we get up-to-date CI. And request people to review. IMO this can move forward now.

Sep 21 '22 05:09 phil-davis

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
88 Code Smells