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.

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

Restore and delete operations in trashbin will affect versions #40291

Merged
merged 8 commits into from
Nov 10, 2022

Conversation

jvillafanez
Copy link
Member

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

#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

  • 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

@ownclouders
Copy link
Contributor

ownclouders commented Aug 16, 2022

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

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

@jvillafanez jvillafanez force-pushed the fix_versions_in_trashbin branch from 442b47a to a1766e3 Compare August 17, 2022 14:40
@jvillafanez jvillafanez marked this pull request as ready for review August 17, 2022 15:43
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

75.3% 75.3% Coverage
0.0% 0.0% Duplication

@jvillafanez
Copy link
Member Author

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.

@phil-davis
Copy link
Contributor

phil-davis commented Aug 23, 2022

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.

@phil-davis
Copy link
Contributor

A test was added in PR #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.

@phil-davis
Copy link
Contributor

phil-davis commented Sep 5, 2022

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

@owncloud owncloud deleted a comment from update-docs bot Sep 5, 2022
@phil-davis
Copy link
Contributor

phil-davis commented Sep 5, 2022

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

@jvillafanez
Copy link
Member Author

@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.

@phil-davis
Copy link
Contributor

@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.

@jvillafanez jvillafanez force-pushed the fix_versions_in_trashbin branch from 77a4f54 to 4032d73 Compare September 21, 2022 09:38
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

75.3% 75.3% Coverage
0.0% 0.0% Duplication

@pako81
Copy link

pako81 commented Oct 10, 2022

Can we get this reviewed and eventually merged, please? Thx.

@pako81
Copy link

pako81 commented Oct 25, 2022

can we move this forward?

Copy link
Contributor

@mrow4a mrow4a left a comment

Choose a reason for hiding this comment

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

I just hope these function were not "forcefully" used in other apps, as with this change they would brake. just make sure to do quick regex. but change itself looks very legit

@jvillafanez jvillafanez merged commit 1dfc04c into master Nov 10, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix_versions_in_trashbin branch November 10, 2022 08:20
@@ -513,14 +513,21 @@ private static function copy(View $view, $source, $target) {
* @throws LockedException
* @throws StorageNotAvailableException
*/
public static function restore($file, $filename, $timestamp, $targetLocation = null) {
public static function restore($filename, $timestamp, $targetLocation = null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this is an interface change. See issue https://github.com/owncloud/ransomware_protection/issues/221 and PR https://github.com/owncloud/ransomware_protection/pull/306

We need to adjust the call that is in the ransomware_protection app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants