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

[stable7] Convert StorageNotAvailableException to SabreDAV exception #11994

Merged

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Nov 6, 2014

Convert \OCP\Files\StorageNotAvailableException to
\Sabre\DAV\Exception\ServiceUnavailable for every file/directory
operation happening inside of SabreDAV.

This is necessary to avoid having the exception bubble up to remote.php
which would return an exception page instead of an appropriate response.

Backport of #11992 to stable7

@DeepDiver1975 @craigpg @icewind1991

Before we accept this backport we need to test whether that really fixes #11869 (CC @karlitschek)

Convert \OCP\Files\StorageNotAvailableException to
\Sabre\DAV\Exception\ServiceUnavailable for every file/directory
operation happening inside of SabreDAV.

This is necessary to avoid having the exception bubble up to remote.php
which would return an exception page instead of an appropriate response.

Conflicts:
	lib/private/connector/sabre/directory.php
	lib/private/connector/sabre/file.php
@PVince81 PVince81 changed the title Convert StorageNotAvailableException to SabreDAV exception [stable7] Convert StorageNotAvailableException to SabreDAV exception Nov 6, 2014
@PVince81 PVince81 mentioned this pull request Nov 6, 2014
@scrutinizer-notifier
Copy link

The inspection completed: 2 new issues, 4 updated code elements

@ghost
Copy link

ghost commented Nov 6, 2014

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/2230/

Build result: FAILURE

[...truncated 6 lines...] > git rev-parse --verify HEAD # timeout=10No valid HEAD. Skipping the resetting > git clean -fdx # timeout=10Fetching upstream changes from https://github.com/owncloud/core.git > git --version # timeout=10 > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/11994/merge^{commit} # timeout=10Checking out Revision 0d5e8b045e1009947946da7aff96158b8f184a64 (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f 0d5e8b045e1009947946da7aff96158b8f184a64 > git rev-list 836bc95db5ec5a71139dca4cc0399eedbe2926bc # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » vm-slave-02Configuration pull-request-analyser-ng-simple » vm-slave-02 is still in the queue: Waiting for next available executor on vm-slave-02pull-request-analyser-ng-simple » vm-slave-02 completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 3 minutes 56 seconds
💣 Test FAILed. 💣

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2014

@owncloud-bot retest this please

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2014

Setting to gold/showstopper/ship blocker due to #11869

@ghost
Copy link

ghost commented Nov 7, 2014

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/2357/

Build result: FAILURE

[...truncated 11 lines...] > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10Cleaning workspace > git rev-parse --verify HEAD # timeout=10No valid HEAD. Skipping the resetting > git clean -fdx # timeout=10Fetching upstream changes from https://github.com/owncloud/core.git > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/11994/merge^{commit} # timeout=10Checking out Revision 9d375d1b5409f38c44a8df8d23eb88228cd956fc (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f 9d375d1b5409f38c44a8df8d23eb88228cd956fc > git rev-list e5a420af510751bef6dbc0e44666a139a2b3b9f4 # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » vm-slave-02pull-request-analyser-ng-simple » vm-slave-02 completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 3 second
💣 Test FAILed. 💣

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2014

@owncloud-bot retest this please

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2014

I checked the logs and tests pass.
The failing JS unit tests will pass during the next run thanks to this backport: #11931 (comment)

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2014

@icewind1991 @schiesbn @DeepDiver1975 please review

@icewind1991
Copy link
Contributor

Can't this be handled with a sabre plugin which listens to the exception event?
Or does this require the exceptions to be the sabre exceptions in the first place?

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2014

Good question. I tried this, but the problem is that we have no control over the output in that event.
We'd need to duplicate the code that creates the exception response (https://github.com/owncloud/3rdparty/blob/master/sabre/dav/lib/Sabre/DAV/Server.php#L218) and quit brutally with "exit()".

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2014

Another idea would be to have a SabreFileView that wraps View that translates exceptions for every operation, but that would add additional overhead (extra/deeper function calls).

@icewind1991
Copy link
Contributor

Ok, looking at the sabre code it only handles sabre exceptions, this seems like the best approach 👍

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2014

@craigpg has confirmed that the stable7 patch works in #11869, so counting as 👍

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2014

@karlitschek please confirm this backport

(and let's hope Jenkins will pass this time)

@ghost
Copy link

ghost commented Nov 7, 2014

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/2371/

Build result: FAILURE

[...truncated 10 lines...] > git config remote.origin.fetch +refs/heads/:refs/remotes/origin/ # timeout=10 > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10Cleaning workspace > git rev-parse --verify HEAD # timeout=10No valid HEAD. Skipping the resetting > git clean -fdx # timeout=10Fetching upstream changes from https://github.com/owncloud/core.git > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/11994/merge^{commit} # timeout=10Checking out Revision 9d375d1b5409f38c44a8df8d23eb88228cd956fc (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f 9d375d1b5409f38c44a8df8d23eb88228cd956fc > git rev-list ab11ef49090af244b47754cf78d05897389050c1 # timeout=10 > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » vm-slave-02pull-request-analyser-ng-simple » vm-slave-02 completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 2 second
💣 Test FAILed. 💣

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 7, 2014

@owncloud-bot retest this please (that one should pass now)

@ghost
Copy link

ghost commented Nov 7, 2014

🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/2376/
🚀 Test PASSed. 🚀

PVince81 pushed a commit that referenced this pull request Nov 7, 2014
…otavailableexception

[stable7] Convert StorageNotAvailableException to SabreDAV exception
@PVince81 PVince81 merged commit b6c868e into stable7 Nov 7, 2014
@PVince81 PVince81 deleted the stable7-sabre-convertstoragenotavailableexception branch November 7, 2014 15:25
@lock lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants