allow storages to explicitly opt-out of uploading .part files

[curiousercreative]

Description

Allow for storage classes to decide whether to use .part files. Default to using part files to preserve previous behavior.

Related Issue

• Fixes Make it possible for storage implementations to disable part files #25826

Motivation and Context

Google Drive external storage (and others?) should not create a duplicate file with the same name every time a file is changed.

How Has This Been Tested?

• no serious testing, just UAT that ownCloud is no longer generating duplicate Google Drive files

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

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[DeepDiver1975]

Generally speaking this looks good - THX
I left a few minor comments

[codecov]

Codecov Report

Merging #37062 into master will decrease coverage by 0.00%.
The diff coverage is 53.84%.

@@             Coverage Diff              @@
##             master   #37062      +/-   ##
============================================
- Coverage     64.68%   64.67%   -0.01%     
- Complexity    19331    19340       +9     
============================================
  Files          1277     1279       +2     
  Lines         75506    75578      +72     
  Branches       1331     1331              
============================================
+ Hits          48838    48879      +41     
- Misses        26276    26307      +31     
  Partials        392      392              

Flag	Coverage Δ	Complexity Δ
#javascript	54.14% <ø> (ø)	0.00 <ø> (ø)
#phpunit	65.83% <53.84%> (-0.01%)	19340.00 <4.00> (+9.00)	⬇️

Impacted Files	Coverage Δ	Complexity Δ
apps/files_external/lib/Lib/Storage/Google.php	0.00% <0.00%> (ø)	0.00 <0.00> (ø)
apps/files_external/lib/Lib/Storage/OwnCloud.php	85.18% <0.00%> (-6.82%)	0.00 <0.00> (ø)
apps/files_sharing/lib/External/Storage.php	28.14% <0.00%> (-0.43%)	50.00 <1.00> (+1.00)	⬇️
apps/dav/lib/Connector/Sabre/File.php	83.81% <100.00%> (-0.16%)	114.00 <0.00> (-3.00)
...b/private/Files/ObjectStore/ObjectStoreStorage.php	76.10% <100.00%> (+0.16%)	106.00 <1.00> (+1.00)
lib/private/Files/Storage/Common.php	84.80% <100.00%> (+0.09%)	139.00 <1.00> (+1.00)
lib/private/Files/Storage/Wrapper/Wrapper.php	94.55% <100.00%> (+0.07%)	73.00 <1.00> (+1.00)
lib/private/Session/CryptoWrapper.php	33.33% <0.00%> (-13.73%)	7.00% <0.00%> (+1.00%)	⬇️
apps/dav/lib/Files/RootCollection.php	9.52% <0.00%> (-4.77%)	7.00% <0.00%> (+2.00%)	⬇️
lib/private/legacy/util.php	74.53% <0.00%> (ø)	219.00% <0.00%> (ø%)
... and 11 more

---

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 702d4cb...121aa97. Read the comment docs.

[curiousercreative]

@DeepDiver1975 prior revision requests should be resolved and this is ready for another review

[curiousercreative]

@DeepDiver1975 @jvillafanez bump for this PR. Would be great to either:

1. get this merged before more conflicting/redundant work is done
2. discuss a more sustainable solution and assign the work

[jvillafanez]

Changes look good to me. Maybe @micbar can move this forward.

[mmattel]

Adding @micbar and @phil-davis to reviewers.
Would solve a long lasting problem for gdrive.
Would be great to get into 10.5

[phil-davis]

@micbar I am not really qualified to approve this. The code looks reasonable to me. Can you decide what happens next?

[jvillafanez]

Technically the problem is solved with b14e8e4 , but this is a better option and we should use something like this.

[mmattel]

What about reverting b14e8e4 and use this PR instead?
As you stated, it is a better solution...

[jvillafanez]

Merging this PR will overwrite that piece, so there is no need to revert anything

IMO review comments have been addressed

[karakayasemi]

The PR can also help to resolve #35660 . Disabling part file usage in webdav external storage should be enough with this pr. We can open a new pr for it after merging this one.

[micbar]

@curiousercreative This looks good to me.

I would like to have more technical info in the changelog item, as this not only affects gdrive but external storages in general

[micbar]

Is it possible to cover that by a unit test?

[curiousercreative]

I've not written any unit tests for PHP code let alone whatever test runner and/or framework ownCloud is using (PHP Unit?). I did some digging and see two potential directions for adding the test:

1. apps/dav/tests/unit/Connector/Sabre/FileTest.php
2. or a new test that, similar to this extends apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php

If adding to the first file, I was thinking to add a test function named testNeedsPartFileMatchesStorage. Are we wanting to test anything other than our connector relaying the underlying storages preference for needsPartFile? More ambitious would be something closer to a functional test, likely the second idea above and try to do the following (but I'm not familiar enough with this codebase, PHP, etc):

1. create an arbitrary Storage backend (akin to the Google Drive external Storage)
2. override needsPartFile method and have that return true
3. start a file upload
4. assert that file exists during upload with a part file name
5. duplicate test for needsPartFile returning false and assert that file exists during upload with part file name

For further discussion, what is the history of this system setting? Is it in any way affected by this pull request (obsoleted perhaps)?

[curiousercreative]

@micbar updated changelog. LMK if you're still missing any particular detail. See above comment re: unit tests.

[mmattel]

@phil-davis can you restart drone, stuck on connecting to github...

[mmattel]

Any update?
Squashing commits would restart the checking process, then we see also the result of codecov/patch.

[mmattel]

@karakayasemi could you support for tests?

[karakayasemi]

@curiousercreative Thank you for contribution! I cherry-picked your commit and completed rest of the things in #37755. Now, it is merged. Closing this one.