Return false if the rename fails except for connect exceptions#35598
Return false if the rename fails except for connect exceptions#35598DeepDiver1975 merged 5 commits intomasterfrom
Conversation
| $this->swallow(__FUNCTION__, $e); | ||
| $result = false; | ||
| } else { | ||
| $ex = new StorageNotAvailableException($e->getMessage(), $e->getCode(), $e); |
There was a problem hiding this comment.
This code logic is there in other operations as well. Please double check those locations as well. THX
Codecov Report
@@ Coverage Diff @@
## master #35598 +/- ##
============================================
+ Coverage 65.67% 65.67% +<.01%
Complexity 18763 18763
============================================
Files 1222 1222
Lines 70887 70886 -1
Branches 1289 1289
============================================
Hits 46555 46555
+ Misses 23954 23953 -1
Partials 378 378
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #35598 +/- ##
============================================
+ Coverage 65.67% 65.73% +0.06%
- Complexity 18763 18779 +16
============================================
Files 1222 1222
Lines 70887 70903 +16
Branches 1289 1289
============================================
+ Hits 46555 46609 +54
+ Misses 23954 23916 -38
Partials 378 378
Continue to review full report at Codecov.
|
|
@jvillafanez question: |
|
It's very likely. In any case, the error isn't caused by the storage not being available but due to invalid chars in the file name. |
|
Rename UI problem should be fixed with 1799af8 (checked in the "old" UI, not in phoenix). Throwing a Forbidden exception might not be nice, but I haven't seen a better alternative. I've also notice that we've been writing information in the cache even though the operation failed. Maybe that has been the cause of ghost entries. They should be fixed with 5780701 (pending to be verified, but changes make sense to me) |
Maybe http 422 (Unprocessable Entity) ? |
|
I think we should throw any of the exceptions in https://github.com/sabre-io/dav/tree/master/lib/DAV/Exception which should be the ones that are expected to be handled by the sabre library (which will be the one handling the request anyway). It's unlikely that any other exception outside of that list will be properly handled. |
|
ok, got the client to run on ubuntu 18.04. did a test with the client with a local file nice 👍 I just read that 422 Unprocessable Entity is currently not present, but that maybe a good chance to implement it as that would be much clearer... @DeepDiver1975 didn´t you do some coding in sabre dav 😄 |
|
I don't think I can improve coverage here. Most of the changes related to the storage can't be unittested, and testing the changes in the view, if possible, is too complex. |
no - see https://tools.ietf.org/html/rfc4918#section-11.2 422 has a clear definition which does not match to this case. 403 looks reasonable and is consistent with the way we use this exception in other places |
|
Backport in #35683 |
|
@jvillafanez I am doing manual testing of this pr. How can I achieve step 1 When a parent folder is shared with read-only permission the receiver cannot edit the child files. |
6 participants
Description
A failure during the rename operation due to insufficient permissions was causing the storage to be unavailable. In this case, the operation should return false instead of throwing a StorageNotAvailableException
Related Issue
https://github.com/owncloud/phoenix/issues/1326
Motivation and Context
It should fail normally instead of blocking the storage
How Has This Been Tested?
This has been checked with the "old" UI, not with phoenix, although it's expected a similar behaviour.
Note: it seems the UI behaves badly under these circumstances. There is an error shown, but the "renamed" file disappears. The storage is available, and you can go outside and then inside the folder to make the file reappear again. The "move" http request ends up with a 404 error code.
Screenshots (if appropriate):
Types of changes
Checklist:
Open tasks: