Retry moving mountpoint sane amount of times

[VicDeo]

Description

• Catch and handle integrity constraint violations and return false as expected
• Make 10 attempts to move the mountpoint

Related Issue

• https://github.com/owncloud/enterprise/issues/4094

Motivation and Context

a newly generated mounpoint path may exist but hasn't been mount yet. An attempt to move to this path will cause breaking constraint on index sh_external_mp for oc_share_external table

How Has This Been Tested?

no exact reproduction steps provided 🙅

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.

[codecov]

Codecov Report

Merging #37956 into master will decrease coverage by 0.50%.
The diff coverage is 58.82%.

@@             Coverage Diff              @@
##             master   #37956      +/-   ##
============================================
- Coverage     65.22%   64.71%   -0.51%     
- Complexity    19445    19448       +3     
============================================
  Files          1286     1286              
  Lines         76031    76043      +12     
  Branches       1336     1336              
============================================
- Hits          49590    49215     -375     
+ Misses        26441    26434       -7     
- Partials          0      394     +394     

Flag	Coverage Δ	Complexity Δ
javascript	54.06% <ø> (-5.23%)	0.00 <ø> (ø)
phpunit	65.89% <58.82%> (+0.01%)	19448.00 <0.00> (+3.00)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
apps/files_sharing/lib/External/Manager.php	89.01% <0.00%> (-1.05%)	34.00 <0.00> (+1.00)	⬇️
apps/files_sharing/lib/External/Mount.php	25.00% <0.00%> (-2.28%)	5.00 <0.00> (+1.00)	⬇️
...b/private/Files/Config/MountProviderCollection.php	91.22% <83.33%> (-1.37%)	19.00 <0.00> (+1.00)	⬇️
core/js/oc-backbone.js	50.00% <0.00%> (-50.00%)	0.00% <0.00%> (ø%)
apps/comments/js/app.js	66.66% <0.00%> (-33.34%)	0.00% <0.00%> (ø%)
core/js/oc-requesttoken.js	75.00% <0.00%> (-25.00%)	0.00% <0.00%> (ø%)
core/js/systemtags/systemtagsmappingcollection.js	47.36% <0.00%> (-21.06%)	0.00% <0.00%> (ø%)
core/js/systemtags/systemtagmodel.js	72.72% <0.00%> (-18.19%)	0.00% <0.00%> (ø%)
core/js/sharescollection.js	42.85% <0.00%> (-14.29%)	0.00% <0.00%> (ø%)
apps/comments/js/commentcollection.js	72.72% <0.00%> (-13.64%)	0.00% <0.00%> (ø%)
... and 56 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 379847c...69b9b85. Read the comment docs.

[jvillafanez]

Is it possible to get the list of the mount points from the DB somehow? Otherwise, I agree we should make the amount of times configurable.

[VicDeo]

@jvillafanez we know nothing about DB in the context of lib/private/Files/Config/MountProviderCollection.php
what we have in MountProviderCollection::getMountsForUser is an array of providers.

[phil-davis]

https://drone.owncloud.com/owncloud/core/27320/139/15

--- Failed scenarios:

    /drone/src/tests/acceptance/features/webUISharingInternalGroups1/shareWithGroups.feature:133
    /drone/src/tests/acceptance/features/webUISharingInternalGroups1/shareWithGroups.feature:150
    /drone/src/tests/acceptance/features/webUISharingInternalGroups1/shareWithGroups.feature:170
    /drone/src/tests/acceptance/features/webUISharingInternalGroups1/shareWithGroups.feature:183

32 scenarios (28 passed, 4 failed)
471 steps (458 passed, 4 failed, 9 skipped)

This sort of problem was reported:

    Then a notification should be displayed on the webUI with the text "Email notification was sent!"              # WebUIGeneralContext::aNotificationShouldBeDisplayedOnTheWebUIWithTheText()
      WebUIGeneralContext::aNotificationShouldBeDisplayedOnTheWebUIWithTheText A notification was expected to be displayed on the webUI with the text 'Email notification was sent!', but got '' instead
      Failed asserting that two strings are equal.
      --- Expected
      +++ Actual
      @@ @@
      -'Email notification was sent!'
      +''

I don't see how the code in this PR could break that. I will restart drone...

[phil-davis]

And this time it passed.

[micbar]

@VicDeo Can we make the amount of retries configurable?

[mmattel]

This is docs relevant (config-to-docs run) because of a change in config/config.sample.php.
I propose to give the description in config/config.sample.php a bit more text for better understanding what it is about.

[VicDeo]

@mmattel frankly speaking I just made it configurable as requested. The only explanation I can add: "if you don't know what is it about then you most likely have no need to adjust it"

[mmattel]

if you don't know what is it about then you most likely have no need to adjust it

I completely agree. In case it gets merged, 3 months later, nobody will remember what it was about. Means, you do not only add a improved description for endusers, but also for ownCloud itself to prevent that.

[VicDeo]

@mmattel I don't know how to explain it properly

0 There could be a collision of the l mount point name for local and remote shares. E.g. when both are named file

1. In this case we move the mount point using the following rule - file (1), file (2), etc. The name is generated by checking whether file (n) exists on the file system.
2. if the storage goes offline for the remote share it could happen that file (n) does NOT exist on the file system but it's still not possible to move the mount point as we already have file (n) in the database but it is not mounted to filesystem . So the move operation returns false
3. When we have false after the move operation we consider file (n) name is unavailable regardless the fact it does NOT exist on the file system and retry the move operation with file (n+1).
4. 3 is repeated exactly filesystem.max_mountpoint_move_attempts times to avoid an infinite loop.

[mmattel]

What about:
This config value avoids infinite loops for seldom cases where a file renaming conflict could occur.
The value defines how many unsuccessful rename attempts are allowed, if the mountpoint to a file is taken but the initiated rename failed. Change this value only under supervision of ownCloud support.

Pls check the text if valid and consistent and adopt if needed.

[VicDeo]

@mmattel what about

/**
 * This config value avoids infinite loops for seldom cases where a file renaming
 * conflict between different share backends could occur.
 * The value defines how many unsuccessful mountpoint rename attempts are allowed.
 * E.g. target mountpoint name could be claimed as unused by the filesystem but
 * renaming to this target name will fail due to some other reasons like database 
 * constraints.
 * Change this value only under supervision of ownCloud support.
 */

Is it clear enough?

[mmattel]

This is great 👍
One thing I would do: even when starting at the beginning of the sentence, I would write E.g.-->e.g.
Else it looks at least to my eyes awful 😄

[phil-davis]

I don't see a changelog entry yet?

And needs a rebase due to the composer 2.0 changes 2 days ago.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
No Duplication information

[phil-davis]

Looks OK to me now.
@jvillafanez review?