Skip to content

Do not log WebDAV maintenance mode exception #6132

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.

Sign up for GitHub

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

Jump to bottom
Merged
MorrisJobke merged 1 commit into from
Aug 15, 2017
Merged

Do not log WebDAV maintenance mode exception #6132

MorrisJobke merged 1 commit into from
Aug 15, 2017

Conversation

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Aug 15, 2017

Log the maintenance mode exception only in debug level. Fixes #6124

How to test:

  • enable maintenance mode
  • enable log level 0 (debug)
  • access webdav (PROPFIND against root for example)
  • before: maintenance mode is logged in error log level (4)
  • after: maintenance mode is logging in debug level (0)
  • additional: update needed should still be logged in error level

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews papercut Annoying recurring UX issue with possibly simple fix. labels Aug 15, 2017
@MorrisJobke MorrisJobke added this to the Nextcloud 13 milestone Aug 15, 2017
@MorrisJobke MorrisJobke mentioned this pull request Aug 15, 2017
Closed
@MorrisJobke
Copy link
Member Author

MorrisJobke commented Aug 15, 2017

@mwries please test this as well 😉

LukasReschke
LukasReschke requested changes Aug 15, 2017
View reviewed changes
apps/dav/lib/Connector/Sabre/ExceptionLoggerPlugin.php Outdated
$level = \OCP\Util::FATAL;
if (isset($this->nonFatalExceptions[$exceptionClass])) {
if (isset($this->nonFatalExceptions[$exceptionClass]) ||
($exceptionClass === 'Sabre\DAV\Exception\ServiceUnavailable' && $ex->getMessage() === 'System in maintenance mode.')) {
Copy link
Member

@LukasReschke LukasReschke Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer a custom exception in \OCA\DAV\Connector\Sabre\MaintenancePlugin::checkMaintenanceMode and then match also on ::class.

Right now this, for example, doesn't catch the Upgrade needed. A custom exception would help here.

Copy link
Member Author

@MorrisJobke MorrisJobke Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is, that then the webdav response would change

Copy link
Member Author

@MorrisJobke MorrisJobke Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the "upgrade needed" should also not be catched, because this is more fishy I think and worth to log.

Copy link
Member Author

@MorrisJobke MorrisJobke Aug 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would change then the response too:

<?xml version="1.0" encoding="utf-8"?>
<d:error
	xmlns:d="DAV:"
	xmlns:s="http://sabredav.org/ns">
	<s:exception>Sabre\DAV\Exception\ServiceUnavailable</s:exception>
	<s:message>System in maintenance mode.</s:message>
</d:error>

apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php Outdated
return [
[0, '', new NotFound()],
[0, 'System in maintenance mode.', new ServiceUnavailable('System in maintenance mode.')],
[4, 'Upgrade needed', new InvalidPath('Upgrade needed')],
Copy link
Member

@LukasReschke LukasReschke Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InvalidPath?

Copy link
Member Author

@MorrisJobke MorrisJobke Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ummm sorry -.-

@MorrisJobke MorrisJobke force-pushed the do-not-log-maintenance-mode branch 2 times, most recently from f61805f to dbb247a Compare August 15, 2017 13:02
@MorrisJobke
Do not log WebDAV maintenance mode exception
fc12bd0 
Log the maintenance mode exception only in debug level. Fixes #6124

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke force-pushed the do-not-log-maintenance-mode branch from dbb247a to fc12bd0 Compare August 15, 2017 13:03
@LukasReschke LukasReschke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 15, 2017
@MorrisJobke MorrisJobke merged commit 60a485b into master Aug 15, 2017
@MorrisJobke MorrisJobke deleted the do-not-log-maintenance-mode branch August 15, 2017 19:26
@nickvergessen nickvergessen mentioned this pull request Oct 13, 2017
Closed
@nickvergessen
Copy link
Member

nickvergessen commented Oct 13, 2017

Reported again, time to backport?
#6824

@MorrisJobke
Copy link
Member Author

MorrisJobke commented Oct 23, 2017

Reported again, time to backport?

I'm open for this. Stable12 should be okay.

@MorrisJobke MorrisJobke self-assigned this Oct 23, 2017
@MorrisJobke
Copy link
Member Author

MorrisJobke commented Oct 23, 2017

Let me do it

@MorrisJobke MorrisJobke mentioned this pull request Oct 23, 2017
Merged
@MorrisJobke
Copy link
Member Author

MorrisJobke commented Oct 23, 2017

Backport to stable12 in #6908

@MorrisJobke MorrisJobke removed their assignment Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@LukasReschke LukasReschke LukasReschke approved these changes

@rullzer rullzer Awaiting requested review from rullzer

@georgehrke georgehrke Awaiting requested review from georgehrke

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish papercut Annoying recurring UX issue with possibly simple fix.

Projects

None yet

Milestone

Nextcloud 13

Development

Successfully merging this pull request may close these issues.

4 participants

@MorrisJobke @nickvergessen @LukasReschke