Merge "REST: gracefully handle all exceptions."

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"