From: Tim Starling Date: Tue, 16 Jul 2019 22:43:43 +0000 (+1000) Subject: Use TextFormatter in the REST API X-Git-Tag: 1.34.0-rc.0~180^2 X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=8b1a6cc58af722aff053c8b30948ae751e6d7f29 Use TextFormatter in the REST API * Add ResponseFactory::createLocalizedHttpError(), which generates a JSON response body from a MessageValue * ResponseFactory::__construct() accepts an array of TextFormatter objects. For ease of testing, the array may be empty. The integrated ResponseFactory has a TextFormatter for English, and one for $wgContLang if that is different. * Use createLocalizedHttpError() to show helpful error messages for errors generated by Router. Change-Id: I897a0aee42227916c568333ab384966f1b87f599 --- diff --git a/includes/Rest/EntryPoint.php b/includes/Rest/EntryPoint.php index ee3441e595..4fdd1f8c5a 100644 --- a/includes/Rest/EntryPoint.php +++ b/includes/Rest/EntryPoint.php @@ -10,6 +10,7 @@ use MediaWiki\Rest\Validator\Validator; use RequestContext; use Title; use WebResponse; +use Wikimedia\Message\ITextFormatter; class EntryPoint { /** @var RequestInterface */ @@ -49,6 +50,8 @@ class EntryPoint { 'cookiePrefix' => $conf->get( 'CookiePrefix' ) ] ); + $responseFactory = new ResponseFactory( self::getTextFormatters( $services ) ); + // @phan-suppress-next-line PhanAccessMethodInternal $authorizer = new MWBasicAuthorizer( $context->getUser(), $services->getPermissionManager() ); @@ -62,7 +65,7 @@ class EntryPoint { ExtensionRegistry::getInstance()->getAttribute( 'RestRoutes' ), $conf->get( 'RestPath' ), $services->getLocalServerObjectCache(), - new ResponseFactory, + $responseFactory, $authorizer, $objectFactory, $restValidator @@ -76,6 +79,25 @@ class EntryPoint { $entryPoint->execute(); } + /** + * Get a TextFormatter array from MediaWikiServices + * + * @param MediaWikiServices $services + * @return ITextFormatter[] + */ + public static function getTextFormatters( MediaWikiServices $services ) { + $langs = array_unique( [ + $services->getMainConfig()->get( 'ContLang' )->getCode(), + 'en' + ] ); + $textFormatters = []; + $factory = $services->getMessageFormatterFactory(); + foreach ( $langs as $lang ) { + $textFormatters[] = $factory->getTextFormatter( $lang ); + } + return $textFormatters; + } + public function __construct( RequestContext $context, RequestInterface $request, WebResponse $webResponse, Router $router ) { diff --git a/includes/Rest/LocalizedHttpException.php b/includes/Rest/LocalizedHttpException.php index 10d3a4034a..184fe164b7 100644 --- a/includes/Rest/LocalizedHttpException.php +++ b/includes/Rest/LocalizedHttpException.php @@ -5,7 +5,14 @@ namespace MediaWiki\Rest; use Wikimedia\Message\MessageValue; class LocalizedHttpException extends HttpException { - public function __construct( MessageValue $message, $code = 500 ) { - parent::__construct( 'Localized exception with key ' . $message->getKey(), $code ); + private $messageValue; + + public function __construct( MessageValue $messageValue, $code = 500 ) { + parent::__construct( 'Localized exception with key ' . $messageValue->getKey(), $code ); + $this->messageValue = $messageValue; + } + + public function getMessageValue() { + return $this->messageValue; } } diff --git a/includes/Rest/ResponseFactory.php b/includes/Rest/ResponseFactory.php index 5e5a19852d..fd0f3c7975 100644 --- a/includes/Rest/ResponseFactory.php +++ b/includes/Rest/ResponseFactory.php @@ -5,19 +5,31 @@ namespace MediaWiki\Rest; use Exception; use HttpStatus; use InvalidArgumentException; +use LanguageCode; use MWExceptionHandler; use stdClass; use Throwable; +use Wikimedia\Message\ITextFormatter; +use Wikimedia\Message\MessageValue; /** * Generates standardized response objects. */ class ResponseFactory { - const CT_PLAIN = 'text/plain; charset=utf-8'; const CT_HTML = 'text/html; charset=utf-8'; const CT_JSON = 'application/json'; + /** @var ITextFormatter[] */ + private $textFormatters; + + /** + * @param ITextFormatter[] $textFormatters + */ + public function __construct( $textFormatters ) { + $this->textFormatters = $textFormatters; + } + /** * Encode a stdClass object or array to a JSON string * @@ -167,13 +179,23 @@ class ResponseFactory { return $response; } + /** + * Create an HTTP 4xx or 5xx response with error message localisation + */ + public function createLocalizedHttpError( $errorCode, MessageValue $messageValue ) { + return $this->createHttpError( $errorCode, $this->formatMessage( $messageValue ) ); + } + /** * Turn an exception into a JSON error response. * @param Exception|Throwable $exception * @return Response */ public function createFromException( $exception ) { - if ( $exception instanceof HttpException ) { + if ( $exception instanceof LocalizedHttpException ) { + $response = $this->createLocalizedHttpError( $exception->getCode(), + $exception->getMessageValue() ); + } elseif ( $exception instanceof HttpException ) { // FIXME can HttpException represent 2xx or 3xx responses? $response = $this->createHttpError( $exception->getCode(), @@ -240,4 +262,18 @@ class ResponseFactory { return "Redirect$url"; } + public function formatMessage( MessageValue $messageValue ) { + if ( !$this->textFormatters ) { + // For unit tests + return []; + } + $translations = []; + foreach ( $this->textFormatters as $formatter ) { + $lang = LanguageCode::bcp47( $formatter->getLangCode() ); + $messageText = $formatter->format( $messageValue ); + $translations[$lang] = $messageText; + } + return [ 'messageTranslations' => $translations ]; + } + } diff --git a/includes/Rest/Router.php b/includes/Rest/Router.php index a520130d06..1c5f66b2be 100644 --- a/includes/Rest/Router.php +++ b/includes/Rest/Router.php @@ -4,6 +4,7 @@ namespace MediaWiki\Rest; use AppendIterator; use BagOStuff; +use Wikimedia\Message\MessageValue; use MediaWiki\Rest\BasicAccess\BasicAuthorizerInterface; use MediaWiki\Rest\PathTemplateMatcher\PathMatcher; use MediaWiki\Rest\Validator\Validator; @@ -226,7 +227,10 @@ class Router { $path = $request->getUri()->getPath(); $relPath = $this->getRelativePath( $path ); if ( $relPath === false ) { - return $this->responseFactory->createHttpError( 404 ); + return $this->responseFactory->createLocalizedHttpError( 404, + ( new MessageValue( 'rest-prefix-mismatch' ) ) + ->plaintextParams( $path, $this->rootPath ) + ); } $matchers = $this->getMatchers(); @@ -245,12 +249,20 @@ class Router { } } if ( $allowed ) { - $response = $this->responseFactory->createHttpError( 405 ); + $response = $this->responseFactory->createLocalizedHttpError( 405, + ( new MessageValue( 'rest-wrong-method' ) ) + ->textParams( $request->getMethod() ) + ->commaListParams( $allowed ) + ->numParams( count( $allowed ) ) + ); $response->setHeader( 'Allow', $allowed ); return $response; } else { // Did not match with any other method, must be 404 - return $this->responseFactory->createHttpError( 404 ); + return $this->responseFactory->createLocalizedHttpError( 404, + ( new MessageValue( 'rest-no-match' ) ) + ->plaintextParams( $relPath ) + ); } } @@ -272,6 +284,7 @@ class Router { /** * Execute a fully-constructed handler + * * @param Handler $handler * @return ResponseInterface */ diff --git a/languages/i18n/en.json b/languages/i18n/en.json index f5af2c546c..69d4fe2a00 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -4262,5 +4262,8 @@ "mycustomjsredirectprotected": "You do not have permission to edit this JavaScript page because it is a redirect and it does not point inside your userspace.", "easydeflate-invaliddeflate": "Content provided is not properly deflated", "unprotected-js": "For security reasons JavaScript cannot be loaded from unprotected pages. Please only create javascript in the MediaWiki: namespace or as a User subpage", - "userlogout-continue": "Do you want to log out?" + "userlogout-continue": "Do you want to log out?", + "rest-prefix-mismatch": "The requested path ($1) was not inside the REST API root path ($2)", + "rest-wrong-method": "The request method ($1) was not {{PLURAL:$3|the allowed method for this path|one of the allowed methods for this path}} ($2)", + "rest-no-match": "The requested relative path ($1) did not match any known handler" } diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index a33608f5fb..436c11ba9c 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -4472,5 +4472,8 @@ "mycustomjsredirectprotected": "Error message shown when user tries to edit their own JS page that is a foreign redirect without the 'mycustomjsredirectprotected' right. See also {{msg-mw|mycustomjsprotected}}.", "easydeflate-invaliddeflate": "Error message if the content passed to easydeflate was not deflated (compressed) properly", "unprotected-js": "Error message shown when trying to load javascript via action=raw that is not protected", - "userlogout-continue": "Shown if user attempted to log out without a token specified. Probably the user clicked on an old link that hasn't been updated to use the new system. $1 - url that user should click on in order to log out." + "userlogout-continue": "Shown if user attempted to log out without a token specified. Probably the user clicked on an old link that hasn't been updated to use the new system. $1 - url that user should click on in order to log out.", + "rest-prefix-mismatch": "Error message for REST API debugging, shown if $wgRestPath is incorrect or otherwise not matched. Parameters:\n* $1: The requested path.\n* $2: The configured root path ($wgRestPath).", + "rest-wrong-method": "Error message for REST API debugging, shown if the HTTP method is incorrect. Parameters:\n* $1: The received request method.\n* $2: A comma-separated list of allowed methods for this path.\n* $3: The number of items in the list $2", + "rest-no-match": "Error message for REST API debugging, shown if the path has the correct prefix but did not match any registered handler. Parameters:\n* $1: The received request path, relative to $wgRestPath." } diff --git a/tests/phpunit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php b/tests/phpunit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php index 2d1fd986cc..a3102420c8 100644 --- a/tests/phpunit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php +++ b/tests/phpunit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php @@ -46,7 +46,7 @@ class MWBasicRequestAuthorizerTest extends MediaWikiTestCase { [], '/rest', new \EmptyBagOStuff(), - new ResponseFactory(), + new ResponseFactory( [] ), new MWBasicAuthorizer( $user, MediaWikiServices::getInstance()->getPermissionManager() ), $objectFactory, new Validator( $objectFactory, $request, $user ) diff --git a/tests/phpunit/includes/Rest/EntryPointTest.php b/tests/phpunit/includes/Rest/EntryPointTest.php index b984895281..1c9bc41608 100644 --- a/tests/phpunit/includes/Rest/EntryPointTest.php +++ b/tests/phpunit/includes/Rest/EntryPointTest.php @@ -38,7 +38,7 @@ class EntryPointTest extends \MediaWikiTestCase { [], '/rest', new EmptyBagOStuff(), - new ResponseFactory(), + new ResponseFactory( [] ), new StaticBasicAuthorizer(), $objectFactory, new Validator( $objectFactory, $request, new User ) diff --git a/tests/phpunit/unit/includes/Rest/Handler/HelloHandlerTest.php b/tests/phpunit/unit/includes/Rest/Handler/HelloHandlerTest.php index 91652a253a..7d682fda62 100644 --- a/tests/phpunit/unit/includes/Rest/Handler/HelloHandlerTest.php +++ b/tests/phpunit/unit/includes/Rest/Handler/HelloHandlerTest.php @@ -62,7 +62,7 @@ class HelloHandlerTest extends \MediaWikiUnitTestCase { [], '/rest', new EmptyBagOStuff(), - new ResponseFactory(), + new ResponseFactory( [] ), new StaticBasicAuthorizer(), $objectFactory, new Validator( $objectFactory, $request, new User ) diff --git a/tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php b/tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php index 04d54de899..0a98686f0a 100644 --- a/tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php +++ b/tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php @@ -6,6 +6,8 @@ use ArrayIterator; use MediaWiki\Rest\HttpException; use MediaWiki\Rest\ResponseFactory; use MediaWikiUnitTestCase; +use Wikimedia\Message\ITextFormatter; +use Wikimedia\Message\MessageValue; /** @covers \MediaWiki\Rest\ResponseFactory */ class ResponseFactoryTest extends MediaWikiUnitTestCase { @@ -18,14 +20,27 @@ class ResponseFactoryTest extends MediaWikiUnitTestCase { ]; } + private function createResponseFactory() { + $fakeTextFormatter = new class implements ITextFormatter { + function getLangCode() { + return 'qqx'; + } + + function format( MessageValue $message ) { + return $message->getKey(); + } + }; + return new ResponseFactory( [ $fakeTextFormatter ] ); + } + /** @dataProvider provideEncodeJson */ public function testEncodeJson( $input, $expected ) { - $rf = new ResponseFactory; + $rf = $this->createResponseFactory(); $this->assertSame( $expected, $rf->encodeJson( $input ) ); } public function testCreateJson() { - $rf = new ResponseFactory; + $rf = $this->createResponseFactory(); $response = $rf->createJson( [] ); $response->getBody()->rewind(); $this->assertSame( 'application/json', $response->getHeaderLine( 'Content-Type' ) ); @@ -35,7 +50,7 @@ class ResponseFactoryTest extends MediaWikiUnitTestCase { } public function testCreateNoContent() { - $rf = new ResponseFactory; + $rf = $this->createResponseFactory(); $response = $rf->createNoContent(); $this->assertSame( [], $response->getHeader( 'Content-Type' ) ); $this->assertSame( 0, $response->getBody()->getSize() ); @@ -43,35 +58,35 @@ class ResponseFactoryTest extends MediaWikiUnitTestCase { } public function testCreatePermanentRedirect() { - $rf = new ResponseFactory; + $rf = $this->createResponseFactory(); $response = $rf->createPermanentRedirect( 'http://www.example.com/' ); $this->assertSame( [ 'http://www.example.com/' ], $response->getHeader( 'Location' ) ); $this->assertSame( 301, $response->getStatusCode() ); } public function testCreateLegacyTemporaryRedirect() { - $rf = new ResponseFactory; + $rf = $this->createResponseFactory(); $response = $rf->createLegacyTemporaryRedirect( 'http://www.example.com/' ); $this->assertSame( [ 'http://www.example.com/' ], $response->getHeader( 'Location' ) ); $this->assertSame( 302, $response->getStatusCode() ); } public function testCreateTemporaryRedirect() { - $rf = new ResponseFactory; + $rf = $this->createResponseFactory(); $response = $rf->createTemporaryRedirect( 'http://www.example.com/' ); $this->assertSame( [ 'http://www.example.com/' ], $response->getHeader( 'Location' ) ); $this->assertSame( 307, $response->getStatusCode() ); } public function testCreateSeeOther() { - $rf = new ResponseFactory; + $rf = $this->createResponseFactory(); $response = $rf->createSeeOther( 'http://www.example.com/' ); $this->assertSame( [ 'http://www.example.com/' ], $response->getHeader( 'Location' ) ); $this->assertSame( 303, $response->getStatusCode() ); } public function testCreateNotModified() { - $rf = new ResponseFactory; + $rf = $this->createResponseFactory(); $response = $rf->createNotModified(); $this->assertSame( 0, $response->getBody()->getSize() ); $this->assertSame( 304, $response->getStatusCode() ); @@ -79,12 +94,12 @@ class ResponseFactoryTest extends MediaWikiUnitTestCase { /** @expectedException \InvalidArgumentException */ public function testCreateHttpErrorInvalid() { - $rf = new ResponseFactory; + $rf = $this->createResponseFactory(); $rf->createHttpError( 200 ); } public function testCreateHttpError() { - $rf = new ResponseFactory; + $rf = $this->createResponseFactory(); $response = $rf->createHttpError( 415, [ 'message' => '...' ] ); $this->assertSame( 415, $response->getStatusCode() ); $body = $response->getBody(); @@ -95,7 +110,7 @@ class ResponseFactoryTest extends MediaWikiUnitTestCase { } public function testCreateFromExceptionUnlogged() { - $rf = new ResponseFactory; + $rf = $this->createResponseFactory(); $response = $rf->createFromException( new HttpException( 'hello', 415 ) ); $this->assertSame( 415, $response->getStatusCode() ); $body = $response->getBody(); @@ -106,7 +121,7 @@ class ResponseFactoryTest extends MediaWikiUnitTestCase { } public function testCreateFromExceptionLogged() { - $rf = new ResponseFactory; + $rf = $this->createResponseFactory(); $response = $rf->createFromException( new \Exception( "hello", 415 ) ); $this->assertSame( 500, $response->getStatusCode() ); $body = $response->getBody(); @@ -131,7 +146,7 @@ class ResponseFactoryTest extends MediaWikiUnitTestCase { /** @dataProvider provideCreateFromReturnValue */ public function testCreateFromReturnValue( $input, $expected ) { - $rf = new ResponseFactory; + $rf = $this->createResponseFactory(); $response = $rf->createFromReturnValue( $input ); $body = $response->getBody(); $body->rewind(); @@ -140,7 +155,17 @@ class ResponseFactoryTest extends MediaWikiUnitTestCase { /** @expectedException \InvalidArgumentException */ public function testCreateFromReturnValueInvalid() { - $rf = new ResponseFactory; + $rf = $this->createResponseFactory(); $rf->createFromReturnValue( new ArrayIterator ); } + + public function testCreateLocalizedHttpError() { + $rf = $this->createResponseFactory(); + $response = $rf->createLocalizedHttpError( 404, new MessageValue( 'rftest' ) ); + $body = $response->getBody(); + $body->rewind(); + $this->assertSame( + '{"messageTranslations":{"qqx":"rftest"},"httpCode":404,"httpReason":"Not Found"}', + $body->getContents() ); + } } diff --git a/tests/phpunit/unit/includes/Rest/RouterTest.php b/tests/phpunit/unit/includes/Rest/RouterTest.php index e16ea25c9a..9f51371a55 100644 --- a/tests/phpunit/unit/includes/Rest/RouterTest.php +++ b/tests/phpunit/unit/includes/Rest/RouterTest.php @@ -29,7 +29,7 @@ class RouterTest extends \MediaWikiUnitTestCase { [], '/rest', new \EmptyBagOStuff(), - new ResponseFactory(), + new ResponseFactory( [] ), new StaticBasicAuthorizer( $authError ), $objectFactory, new Validator( $objectFactory, $request, new User )