Objectstore transfer

[jvillafanez]

Description

Fix files:transfer-ownership command for multibucket objectstores (checked with files_primary_s3 app against a scality server, with multibucket configuration)

NOTE Only files are moved between buckets if needed. Thumbnails and versions remain in the old bucket. The owncloud's filecache also keeps the thumbnails and versions in the same storages (to be investigated further)

Related Issue

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

Motivation and Context

transfer-ownership was causing problems with the shares because the fileid of the transferred files was changing. This PR aims to prevent the fileid change so the shares are still bound to valid ids (the files are transferred anyway)

How Has This Been Tested?

1. With user1, create "file1.txt", "f1/file2.txt" and "f1/f2/file3.txt" (with the required folders)
2. user1 shares "f1/f2" and "f1/file2.txt" with the admin user
3. run occ files:transfer-ownership user1 user2 (user2 must exists and be valid)

The transfer happens. The admin can still access to the previously shared files. Shares are visible for user2 (user2 owns the shares)

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

[jvillafanez]

To be double-checked:

• Versions are lost. The copy between buckets only copies the file (object), but it doesn't copy the versions along with the file. It's unclear if the old object will be completely removed in the old bucket, or if it will remain accessible if it has versions.
• Thumbnails remain in the old bucket and in the filecache. This will use space
• Trashbin hasn't been tested yet. The behaviour is unknown at the moment

[jvillafanez]

We'll have to decide what to do with the versions. They'll likely need special handling because they aren't moved or copied by default, and you'll need to copy the versions one by one (and delete them also one by one if you want to move them).
I haven't seen anything in the s3 api to perform this operations in the server side, so this will kill the performance because we'll need to connect several times to the s3 server per file.

[codecov]

Codecov Report

Merging #36326 into master will decrease coverage by 0.01%.
The diff coverage is 40.47%.

@@             Coverage Diff              @@
##             master   #36326      +/-   ##
============================================
- Coverage     64.86%   64.84%   -0.02%     
- Complexity    19775    19785      +10     
============================================
  Files          1271     1271              
  Lines         74707    74738      +31     
  Branches       1309     1309              
============================================
+ Hits          48455    48462       +7     
- Misses        25866    25890      +24     
  Partials        386      386

Flag	Coverage Δ	Complexity Δ
#javascript	54% <ø> (ø)	0 <ø> (ø)	⬇️
#phpunit	66.03% <40.47%> (-0.03%)	19785 <2> (+10)

Impacted Files	Coverage Δ	Complexity Δ
...b/private/Files/ObjectStore/ObjectStoreStorage.php	77.24% <40.47%> (-6.55%)	105 <2> (+10)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 295d62a...0054ed1. Read the comment docs.

[jvillafanez]

I'm not sure if we can add unittest here. There are a couple of problems:

• the constructor calls some methods of the object.
  • We could try to mock the object without calling the constructor, but there is a risk that any function we test relies on those methods to have been called.
  • Calling the constructor implies calling those methods, but they could fail if the dependencies aren't fully mocked. This means we can't mock correctly the class.
• The moveFromStorage relies on the parent's copyFromStorage method (at least for the scenario we want to test). That method has calls to fclose that can't be mocked.

We might need to pull off the new unittests.

[jvillafanez]

Removed the unittests from the list of changes due to problems with the code. We'll rely on the integration tests to verify this works properly using multibucket.
In addition, added an additional test case to check with folder shares (whose fileid must also remain unchanged)

[jvillafanez]

I don't think I can raise the code coverage. Unittests problems are described in #36326 (comment) and the acceptance tests already tests what is expected.

[phil-davis]

CI passes in files_primary_s3 when run against core objectstore_transfer branch. Great stuff.
So this is "a good thing" from a QA/CI point of view. It should fix issue #36266

@micbar can someone review this from a developer point-of-view? And then if the codecov issue is "OK" then force merge.

[micbar]

We'll have to decide what to do with the versions. They'll likely need special handling because they aren't moved or copied by default, and you'll need to copy the versions one by one (and delete them also one by one if you want to move them).
I haven't seen anything in the s3 api to perform this operations in the server side, so this will kill the performance because we'll need to connect several times to the s3 server per file.

@pmaier @cdamken We need to skip versions here. ok for you?

[phil-davis]

Feedback/review/approval needed here. Getting this sorted will unblock #36266 and let backlogged files_primary_s3 PRs be rebased/merged.
@cdamken @pmaier1 - ^^

[pmaier1]

@pmaier @cdamken We need to skip versions here. ok for you?

As the docs say (https://doc.owncloud.com/server/admin_manual/configuration/server/occ_command.html#the-filestransfer-ownership-command), the command never transfers file versions. So this should be fine for objectstore as well.

[cdamken]

If it fixed the problem from https://github.com/owncloud/enterprise/issues/3520 I'm ok with it ;)

[phil-davis]

Sadly a different test fails in the multi-bucket environment: #36266 (comment)