REST: gracefully handle all exceptions.

ResponseFactory::createFromException already had support for arbitrary
exceptions, but Router was so far only using it for HttpExceptions,
leaving other kinds of exceptions uncaught.

In addition to catching all exceptions and generating an appropriate
JSON response for them, this patch introduces the ErrorReporter
interface, with an MWErrorReporter implementation which calls
MWExceptionHandler::rollbackMasterChangesAndLog(). This is how uncaught
errors are handled for requests coming in via api.php, so it seems
appropriate to use the same approach for requests coming in via
rest.php.

Bug: T285984
Change-Id: I0605a7693821ef58fac80ab67f51a742556a37fd

includes/Rest/EntryPoint.php

@@ -9,6 +9,7 @@ use MediaWiki\Config\ServiceOptions;
 use MediaWiki\MediaWikiServices;
 use MediaWiki\Rest\BasicAccess\CompoundAuthorizer;
 use MediaWiki\Rest\BasicAccess\MWBasicAuthorizer;
+use MediaWiki\Rest\Reporter\MWErrorReporter;
 use MediaWiki\Rest\Validator\Validator;
 use RequestContext;
 use Title;
@@ -75,6 +76,7 @@ class EntryPoint {
 			$authority,
 			$objectFactory,
 			$restValidator,
+			new MWErrorReporter(),
 			$services->getHookContainer()
 		) )->setCors( $cors );
 	}

includes/Rest/Reporter/ErrorReporter.php

@@ -0,0 +1,24 @@
+<?php
+
+namespace MediaWiki\Rest\Reporter;
+
+use MediaWiki\Rest\Handler;
+use MediaWiki\Rest\RequestInterface;
+use Throwable;
+
+/**
+ * An ErrorReporter internally reports an error that happened during the handling of a request.
+ * It must have no effect on the response sent to the client.
+ *
+ * @since 1.38
+ */
+interface ErrorReporter {
+
+	/**
+	 * @param Throwable $error
+	 * @param Handler $handler
+	 * @param RequestInterface $request
+	 */
+	public function reportError( Throwable $error, Handler $handler, RequestInterface $request );
+
+}

includes/Rest/Reporter/MWErrorReporter.php

@@ -0,0 +1,29 @@
+<?php
+
+namespace MediaWiki\Rest\Reporter;
+
+use MediaWiki\Rest\Handler;
+use MediaWiki\Rest\RequestInterface;
+use MWExceptionHandler;
+use Throwable;
+
+/**
+ * Error reporter based on MWExceptionHandler.
+ * @see MWExceptionHandler
+ * @since 1.38
+ */
+class MWErrorReporter implements ErrorReporter {
+
+	/**
+	 * @param Throwable $error
+	 * @param Handler $handler
+	 * @param RequestInterface $request
+	 */
+	public function reportError( Throwable $error, Handler $handler, RequestInterface $request ) {
+		MWExceptionHandler::rollbackPrimaryChangesAndLog(
+			$error,
+			MWExceptionHandler::CAUGHT_BY_ENTRYPOINT
+		);
+	}
+
+}

includes/Rest/Reporter/PHPErrorReporter.php

@@ -0,0 +1,35 @@
+<?php
+
+namespace MediaWiki\Rest\Reporter;
+
+use MediaWiki\Rest\Handler;
+use MediaWiki\Rest\RequestInterface;
+use Throwable;
+
+/**
+ * Error reporter based on php's native trigger_error() method.
+ * @since 1.38
+ */
+class PHPErrorReporter implements ErrorReporter {
+
+	/** @var int */
+	private $level;
+
+	/**
+	 * @param int $level The error level to pass to trigger_error
+	 */
+	public function __construct( $level = E_USER_WARNING ) {
+		$this->level = $level;
+	}
+
+	/**
+	 * @param Throwable $error
+	 * @param Handler $handler
+	 * @param RequestInterface $request
+	 */
+	public function reportError( Throwable $error, Handler $handler, RequestInterface $request ) {
+		$firstLine = preg_split( '#$#m', (string)$error, 0 )[0];
+		trigger_error( $firstLine, $this->level );
+	}
+
+}

includes/Rest/ResponseFactory.php

@@ -231,11 +231,12 @@ class ResponseFactory {
 			}
 		} else {
 			$response = $this->createHttpError( 500, [
-				'message' => 'Error: exception of type ' . get_class( $exception ),
+				'message' => 'Error: exception of type ' . get_class( $exception ) . ': '
+					. $exception->getMessage(),
 				'exception' => MWExceptionHandler::getStructuredExceptionData( $exception )
 			] );
-			// FIXME should we try to do something useful with ILocalizedException?
-			// FIXME should we try to do something useful with common MediaWiki errors like ReadOnlyError?
+			// XXX: should we try to do something useful with ILocalizedException?
+			// XXX: should we try to do something useful with common MediaWiki errors like ReadOnlyError?
 		}
 		return $response;
 	}

includes/Rest/Router.php

@@ -8,7 +8,9 @@ use MediaWiki\HookContainer\HookContainer;
 use MediaWiki\Permissions\Authority;
 use MediaWiki\Rest\BasicAccess\BasicAuthorizerInterface;
 use MediaWiki\Rest\PathTemplateMatcher\PathMatcher;
+use MediaWiki\Rest\Reporter\ErrorReporter;
 use MediaWiki\Rest\Validator\Validator;
+use Throwable;
 use Wikimedia\Message\MessageValue;
 use Wikimedia\ObjectFactory;
 
@@ -63,6 +65,9 @@ class Router {
 	/** @var CorsUtils|null */
 	private $cors;
 
+	/** @var ErrorReporter */
+	private $errorReporter;
+
 	/** @var HookContainer */
 	private $hookContainer;
 
@@ -77,6 +82,7 @@ class Router {
 	 * @param Authority $authority
 	 * @param ObjectFactory $objectFactory
 	 * @param Validator $restValidator
+	 * @param ErrorReporter $errorReporter
 	 * @param HookContainer $hookContainer
 	 * @internal
 	 */
@@ -91,6 +97,7 @@ class Router {
 		Authority $authority,
 		ObjectFactory $objectFactory,
 		Validator $restValidator,
+		ErrorReporter $errorReporter,
 		HookContainer $hookContainer
 	) {
 		$this->routeFiles = $routeFiles;
@@ -103,6 +110,7 @@ class Router {
 		$this->authority = $authority;
 		$this->objectFactory = $objectFactory;
 		$this->restValidator = $restValidator;
+		$this->errorReporter = $errorReporter;
 		$this->hookContainer = $hookContainer;
 	}
 
@@ -330,6 +338,9 @@ class Router {
 			return $this->executeHandler( $handler );
 		} catch ( HttpException $e ) {
 			return $this->responseFactory->createFromException( $e );
+		} catch ( Throwable $e ) {
+			$this->errorReporter->reportError( $e, $handler, $request );
+			return $this->responseFactory->createFromException( $e );
 		}
 	}
 

tests/phpunit/includes/Rest/EntryPointTest.php

@@ -10,6 +10,7 @@ use MediaWiki\Rest\BasicAccess\StaticBasicAuthorizer;
 use MediaWiki\Rest\CorsUtils;
 use MediaWiki\Rest\EntryPoint;
 use MediaWiki\Rest\Handler;
+use MediaWiki\Rest\Reporter\PHPErrorReporter;
 use MediaWiki\Rest\RequestData;
 use MediaWiki\Rest\RequestInterface;
 use MediaWiki\Rest\ResponseFactory;
@@ -47,6 +48,7 @@ class EntryPointTest extends \MediaWikiIntegrationTestCase {
 			$authority,
 			$objectFactory,
 			new Validator( $objectFactory, $request, $authority ),
+			new PHPErrorReporter(),
 			$this->createHookContainer()
 		);
 	}

tests/phpunit/unit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php

@@ -6,6 +6,7 @@ use GuzzleHttp\Psr7\Uri;
 use MediaWiki\Permissions\SimpleAuthority;
 use MediaWiki\Rest\BasicAccess\MWBasicAuthorizer;
 use MediaWiki\Rest\Handler;
+use MediaWiki\Rest\Reporter\PHPErrorReporter;
 use MediaWiki\Rest\RequestData;
 use MediaWiki\Rest\ResponseFactory;
 use MediaWiki\Rest\Router;
@@ -41,6 +42,7 @@ class MWBasicRequestAuthorizerTest extends MediaWikiUnitTestCase {
 			$authority,
 			$objectFactory,
 			new Validator( $objectFactory, $request, $authority ),
+			new PHPErrorReporter(),
 			$this->createHookContainer()
 		);
 	}

tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php

@@ -162,7 +162,7 @@ class ResponseFactoryTest extends MediaWikiUnitTestCase {
 		$body->rewind();
 		$data = json_decode( $body->getContents(), true );
 		$this->assertSame( 500, $data['httpCode'] );
-		$this->assertSame( 'Error: exception of type Exception', $data['message'] );
+		$this->assertSame( 'Error: exception of type Exception: hello', $data['message'] );
 	}
 
 	public function testCreateFromExceptionWrapped() {

tests/phpunit/unit/includes/Rest/RouterTest.php

@@ -7,6 +7,7 @@ use MediaWiki\Rest\BasicAccess\StaticBasicAuthorizer;
 use MediaWiki\Rest\Handler;
 use MediaWiki\Rest\HttpException;
 use MediaWiki\Rest\RedirectException;
+use MediaWiki\Rest\Reporter\ErrorReporter;
 use MediaWiki\Rest\RequestData;
 use MediaWiki\Rest\RequestInterface;
 use MediaWiki\Rest\ResponseException;
@@ -14,7 +15,10 @@ use MediaWiki\Rest\ResponseFactory;
 use MediaWiki\Rest\Router;
 use MediaWiki\Rest\Validator\Validator;
 use MediaWiki\Tests\Unit\Permissions\MockAuthorityTrait;
+use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Container\ContainerInterface;
+use RuntimeException;
+use Throwable;
 use Wikimedia\ObjectFactory;
 
 /**
@@ -23,6 +27,9 @@ use Wikimedia\ObjectFactory;
 class RouterTest extends \MediaWikiUnitTestCase {
 	use MockAuthorityTrait;
 
+	/** @var Throwable[] */
+	private $reportedErrors = [];
+
 	/**
 	 * @param RequestInterface $request
 	 * @param string|null $authError
@@ -39,6 +46,14 @@ class RouterTest extends \MediaWikiUnitTestCase {
 		);
 		$routeFiles = array_merge( [ __DIR__ . '/testRoutes.json' ], $additionalRouteFiles );
 		$authority = $this->mockAnonUltimateAuthority();
+
+		/** @var MockObject|ErrorReporter $mockErrorReporter */
+		$mockErrorReporter = $this->createNoOpMock( ErrorReporter::class, [ 'reportError' ] );
+		$mockErrorReporter->method( 'reportError' )
+			->willReturnCallback( function ( $e ) {
+				$this->reportedErrors[] = $e;
+			} );
+
 		return new Router(
 			$routeFiles,
 			[],
@@ -50,6 +65,7 @@ class RouterTest extends \MediaWikiUnitTestCase {
 			$authority,
 			$objectFactory,
 			new Validator( $objectFactory, $request, $authority ),
+			$mockErrorReporter,
 			$this->createHookContainer()
 		);
 	}
@@ -99,6 +115,14 @@ class RouterTest extends \MediaWikiUnitTestCase {
 		};
 	}
 
+	public static function fatalHandlerFactory() {
+		return new class extends Handler {
+			public function execute() {
+				throw new RuntimeException( 'Fatal mock error', 12345 );
+			}
+		};
+	}
+
 	public static function throwRedirectHandlerFactory() {
 		return new class extends Handler {
 			public function execute() {
@@ -117,7 +141,7 @@ class RouterTest extends \MediaWikiUnitTestCase {
 		};
 	}
 
-	public function testException() {
+	public function testHttpException() {
 		$request = new RequestData( [ 'uri' => new Uri( '/rest/mock/RouterTest/throw' ) ] );
 		$router = $this->createRouter( $request );
 		$response = $router->execute( $request );
@@ -128,6 +152,23 @@ class RouterTest extends \MediaWikiUnitTestCase {
 		$this->assertSame( 'Mock error', $data['message'] );
 	}
 
+	public function testFatalException() {
+		$request = new RequestData( [ 'uri' => new Uri( '/rest/mock/RouterTest/fatal' ) ] );
+		$router = $this->createRouter( $request );
+		$response = $router->execute( $request );
+		$this->assertSame( 500, $response->getStatusCode() );
+		$body = $response->getBody();
+		$body->rewind();
+		$data = json_decode( $body->getContents(), true );
+		$this->assertStringContainsString( 'RuntimeException', $data['message'] );
+		$this->assertStringContainsString( 'Fatal mock error', $data['message'] );
+		$this->assertSame( RuntimeException::class, $data['exception']['type'] );
+		$this->assertSame( 12345, $data['exception']['code'] );
+		$this->assertSame( 'Fatal mock error', $data['exception']['message'] );
+		$this->assertNotEmpty( $this->reportedErrors );
+		$this->assertInstanceOf( RuntimeException::class, $this->reportedErrors[0] );
+	}
+
 	public function testRedirectException() {
 		$request = new RequestData( [ 'uri' => new Uri( '/rest/mock/RouterTest/throwRedirect' ) ] );
 		$router = $this->createRouter( $request );

tests/phpunit/unit/includes/Rest/testRoutes.json

@@ -15,6 +15,10 @@
 		"path": "/mock/RouterTest/throw",
 		"factory": "MediaWiki\\Tests\\Rest\\RouterTest::throwHandlerFactory"
 	},
+	{
+		"path": "/mock/RouterTest/fatal",
+		"factory": "MediaWiki\\Tests\\Rest\\RouterTest::fatalHandlerFactory"
+	},
 	{
 		"path": "/mock/RouterTest/throwRedirect",
 		"factory": "MediaWiki\\Tests\\Rest\\RouterTest::throwRedirectHandlerFactory"