From 0ae06654d764c82a54c85d3bd7a080b9f2c57ecc Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Tue, 4 Jun 2019 15:52:19 +1000 Subject: [PATCH] Expand ResponseFactory MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit * Factor out json_encode() call into ResponseFactory::encodeJson(). * Add createJson() and standardize on JSON for 4xx and 5xx responses * Add methods for redirect generation, providing an HTML link in the body as recommended by RFC 7231 Most of the code was written by Gergő Tisza. The differences compared to I747e34faecbcd are: * Remove JsonResponse. * Swap parameter order of createJson() reflecting the fact that the value is now usually provided. * Remove unnecessary ResponseFactory::setStatus() * Don't do ['code' => 'http500'] by default, use httpCode and httpReason to provide that information * In createFromReturnValue(), don't wrap numerically-indexed arrays. * Added tests. Bug: T223240 Change-Id: Ie185b2bd43690633f1ccbe6328a0518e43a9f2f9 --- includes/Rest/ResponseFactory.php | 222 ++++++++++++++++-- includes/Rest/Router.php | 6 +- .../includes/Rest/ResponseFactoryTest.php | 139 +++++++++++ 3 files changed, 338 insertions(+), 29 deletions(-) create mode 100644 tests/phpunit/includes/Rest/ResponseFactoryTest.php diff --git a/includes/Rest/ResponseFactory.php b/includes/Rest/ResponseFactory.php index 21307bcc44..7ccb612748 100644 --- a/includes/Rest/ResponseFactory.php +++ b/includes/Rest/ResponseFactory.php @@ -2,51 +2,221 @@ namespace MediaWiki\Rest; +use Exception; +use HttpStatus; +use InvalidArgumentException; +use MWExceptionHandler; +use stdClass; +use Throwable; + /** - * MOCK UP ONLY - * @unstable + * 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'; - public function create404() { - $response = new Response( 'Path not found' ); - $response->setStatus( 404 ); - $response->setHeader( 'Content-Type', self::CT_PLAIN ); + /** + * Encode a stdClass object or array to a JSON string + * + * @param array|stdClass $value + * @return string + * @throws JsonEncodingException + */ + public function encodeJson( $value ) { + $json = json_encode( $value, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE ); + if ( $json === false ) { + throw new JsonEncodingException( json_last_error_msg(), json_last_error() ); + } + return $json; + } + + /** + * Create an unspecified response. It is the caller's responsibility to set specifics + * like response code, content type etc. + * @return Response + */ + public function create() { + return new Response(); + } + + /** + * Create a successful JSON response. + * @param array|stdClass $value JSON value + * @param string|null $contentType HTTP content type (should be 'application/json+...') + * or null for plain 'application/json' + * @return Response + */ + public function createJson( $value, $contentType = null ) { + $contentType = $contentType ?? self::CT_JSON; + $response = new Response( $this->encodeJson( $value ) ); + $response->setHeader( 'Content-Type', $contentType ); + return $response; + } + + /** + * Create a 204 (No Content) response, used to indicate that an operation which does + * not return anything (e.g. a PUT request) was successful. + * + * Headers are generally interpreted to refer to the target of the operation. E.g. if + * this was a PUT request, the caller of this method might want to add an ETag header + * describing the created resource. + * + * @return Response + */ + public function createNoContent() { + $response = new Response(); + $response->setStatus( 204 ); + return $response; + } + + /** + * Creates a permanent (301) redirect. + * This indicates that the caller of the API should update their indexes and call + * the new URL in the future. 301 redirects tend to get cached and are hard to undo. + * Client behavior for methods other than GET/HEAD is not well-defined and this type + * of response should be avoided in such cases. + * @param string $target Redirect URL (can be relative) + * @return Response + */ + public function createPermanentRedirect( $target ) { + $response = $this->createRedirectBase( $target ); + $response->setStatus( 301 ); + return $response; + } + + /** + * Creates a temporary (307) redirect. + * This indicates that the operation the client was trying to perform can temporarily + * be achieved by using a different URL. Clients will preserve the request method when + * retrying the request with the new URL. + * @param string $target Redirect URL (can be relative) + * @return Response + */ + public function createTemporaryRedirect( $target ) { + $response = $this->createRedirectBase( $target ); + $response->setStatus( 307 ); return $response; } - public function create500( $message ) { - $response = new Response( $message ); - $response->setStatus( 500 ); - $response->setHeader( 'Content-Type', self::CT_PLAIN ); + /** + * Creates a See Other (303) redirect. + * This indicates that the target resource might be of interest to the client, without + * necessarily implying that it is the same resource. The client will always use GET + * (or HEAD) when following the redirection. Useful for GET-after-POST. + * @param string $target Redirect URL (can be relative) + * @return Response + */ + public function createSeeOther( $target ) { + $response = $this->createRedirectBase( $target ); + $response->setStatus( 303 ); return $response; } - public function createFromException( \Exception $exception ) { + /** + * Create a 304 (Not Modified) response, used when the client has an up-to-date cached response. + * + * Per RFC 7232 the response should contain all Cache-Control, Content-Location, Date, + * ETag, Expires, and Vary headers that would have been sent with the 200 OK answer + * if the requesting client did not have a valid cached response. This is the responsibility + * of the caller of this method. + * + * @return Response + */ + public function createNotModified() { + $response = new Response(); + $response->setStatus( 304 ); + return $response; + } + + /** + * Create a HTTP 4xx or 5xx response. + * @param int $errorCode HTTP error code + * @param array $bodyData An array of data to be included in the JSON response + * @return Response + * @throws InvalidArgumentException + */ + public function createHttpError( $errorCode, array $bodyData = [] ) { + if ( $errorCode < 400 || $errorCode >= 600 ) { + throw new InvalidArgumentException( 'error code must be 4xx or 5xx' ); + } + $response = $this->createJson( $bodyData + [ + 'httpCode' => $errorCode, + 'httpReason' => HttpStatus::getMessage( $errorCode ) + ] ); + // TODO add link to error code documentation + $response->setStatus( $errorCode ); + return $response; + } + + /** + * Turn an exception into a JSON error response. + * @param Exception|Throwable $exception + * @return Response + */ + public function createFromException( $exception ) { if ( $exception instanceof HttpException ) { - $response = new Response( $exception->getMessage() ); - $response->setStatus( $exception->getCode() ); - $response->setHeader( 'Content-Type', self::CT_PLAIN ); - return $response; + // FIXME can HttpException represent 2xx or 3xx responses? + $response = $this->createHttpError( $exception->getCode(), + [ 'message' => $exception->getMessage() ] ); } else { - return $this->create500( "Error: exception of type " . gettype( $exception ) ); + $response = $this->createHttpError( 500, [ + 'message' => 'Error: exception of type ' . get_class( $exception ), + '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? } + return $response; } + /** + * Create a JSON response from an arbitrary value. + * This is a fallback; it's preferable to use createJson() instead. + * @param mixed $value A structure containing only scalars, arrays and stdClass objects + * @return Response + * @throws InvalidArgumentException When $value cannot be reasonably represented as JSON + */ public function createFromReturnValue( $value ) { - if ( is_scalar( $value ) - || ( is_object( $value ) && method_exists( $value, '__toString' ) ) - ) { - $value = [ 'value' => (string)$value ]; - } - $json = json_encode( $value, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE ); - if ( $json === false ) { - throw new JsonEncodingException( json_last_error_msg(), json_last_error() ); + $originalValue = $value; + if ( is_scalar( $value ) ) { + $data = [ 'value' => $value ]; + } elseif ( is_array( $value ) || $value instanceof stdClass ) { + $data = $value; + } else { + $type = gettype( $originalValue ); + if ( $type === 'object' ) { + $type = get_class( $originalValue ); + } + throw new InvalidArgumentException( __METHOD__ . ": Invalid return value type $type" ); } - $response = new Response( $json ); - $response->setHeader( 'Content-Type', self::CT_JSON ); + $response = $this->createJson( $data ); return $response; } + + /** + * Create a redirect response with type / response code unspecified. + * @param string $target Redirect target (an absolute URL) + * @return Response + */ + protected function createRedirectBase( $target ) { + $response = new Response( $this->getHyperLink( $target ) ); + $response->setHeader( 'Content-Type', self::CT_HTML ); + $response->setHeader( 'Location', $target ); + return $response; + } + + /** + * Returns a minimal HTML document that links to the given URL, as suggested by + * RFC 7231 for 3xx responses. + * @param string $url An absolute URL + * @return string + */ + protected function getHyperLink( $url ) { + $url = htmlspecialchars( $url ); + return "Redirect$url"; + } + } diff --git a/includes/Rest/Router.php b/includes/Rest/Router.php index 0c458391ae..ab544392f9 100644 --- a/includes/Rest/Router.php +++ b/includes/Rest/Router.php @@ -191,16 +191,16 @@ class Router { $matchers = $this->getMatchers(); $matcher = $matchers[$request->getMethod()] ?? null; if ( $matcher === null ) { - return $this->responseFactory->create404(); + return $this->responseFactory->createHttpError( 404 ); } $path = $request->getUri()->getPath(); if ( substr_compare( $path, $this->rootPath, 0, strlen( $this->rootPath ) ) !== 0 ) { - return $this->responseFactory->create404(); + return $this->responseFactory->createHttpError( 404 ); } $relPath = substr( $path, strlen( $this->rootPath ) ); $match = $matcher->match( $relPath ); if ( !$match ) { - return $this->responseFactory->create404(); + return $this->responseFactory->createHttpError( 404 ); } $request->setAttributes( $match['params'] ); $spec = $match['userData']; diff --git a/tests/phpunit/includes/Rest/ResponseFactoryTest.php b/tests/phpunit/includes/Rest/ResponseFactoryTest.php new file mode 100644 index 0000000000..6ccacda717 --- /dev/null +++ b/tests/phpunit/includes/Rest/ResponseFactoryTest.php @@ -0,0 +1,139 @@ +assertSame( $expected, $rf->encodeJson( $input ) ); + } + + public function testCreateJson() { + $rf = new ResponseFactory; + $response = $rf->createJson( [] ); + $response->getBody()->rewind(); + $this->assertSame( 'application/json', $response->getHeaderLine( 'Content-Type' ) ); + $this->assertSame( '[]', $response->getBody()->getContents() ); + // Make sure getSize() is functional, since testCreateNoContent() depends on it + $this->assertSame( 2, $response->getBody()->getSize() ); + } + + public function testCreateNoContent() { + $rf = new ResponseFactory; + $response = $rf->createNoContent(); + $this->assertSame( [], $response->getHeader( 'Content-Type' ) ); + $this->assertSame( 0, $response->getBody()->getSize() ); + $this->assertSame( 204, $response->getStatusCode() ); + } + + public function testCreatePermanentRedirect() { + $rf = new ResponseFactory; + $response = $rf->createPermanentRedirect( 'http://www.example.com/' ); + $this->assertSame( [ 'http://www.example.com/' ], $response->getHeader( 'Location' ) ); + $this->assertSame( 301, $response->getStatusCode() ); + } + + public function testCreateTemporaryRedirect() { + $rf = new ResponseFactory; + $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; + $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; + $response = $rf->createNotModified(); + $this->assertSame( 0, $response->getBody()->getSize() ); + $this->assertSame( 304, $response->getStatusCode() ); + } + + /** @expectedException \InvalidArgumentException */ + public function testCreateHttpErrorInvalid() { + $rf = new ResponseFactory; + $rf->createHttpError( 200 ); + } + + public function testCreateHttpError() { + $rf = new ResponseFactory; + $response = $rf->createHttpError( 415, [ 'message' => '...' ] ); + $this->assertSame( 415, $response->getStatusCode() ); + $body = $response->getBody(); + $body->rewind(); + $data = json_decode( $body->getContents(), true ); + $this->assertSame( 415, $data['httpCode'] ); + $this->assertSame( '...', $data['message'] ); + } + + public function testCreateFromExceptionUnlogged() { + $rf = new ResponseFactory; + $response = $rf->createFromException( new HttpException( 'hello', 415 ) ); + $this->assertSame( 415, $response->getStatusCode() ); + $body = $response->getBody(); + $body->rewind(); + $data = json_decode( $body->getContents(), true ); + $this->assertSame( 415, $data['httpCode'] ); + $this->assertSame( 'hello', $data['message'] ); + } + + public function testCreateFromExceptionLogged() { + $rf = new ResponseFactory; + $response = $rf->createFromException( new \Exception( "hello", 415 ) ); + $this->assertSame( 500, $response->getStatusCode() ); + $body = $response->getBody(); + $body->rewind(); + $data = json_decode( $body->getContents(), true ); + $this->assertSame( 500, $data['httpCode'] ); + $this->assertSame( 'Error: exception of type Exception', $data['message'] ); + } + + public static function provideCreateFromReturnValue() { + return [ + [ 'hello', '{"value":"hello"}' ], + [ true, '{"value":true}' ], + [ [ 'x' => 'y' ], '{"x":"y"}' ], + [ [ 'x', 'y' ], '["x","y"]' ], + [ [ 'a', 'x' => 'y' ], '{"0":"a","x":"y"}' ], + [ (object)[ 'a', 'x' => 'y' ], '{"0":"a","x":"y"}' ], + [ [], '[]' ], + [ (object)[], '{}' ], + ]; + } + + /** @dataProvider provideCreateFromReturnValue */ + public function testCreateFromReturnValue( $input, $expected ) { + $rf = new ResponseFactory; + $response = $rf->createFromReturnValue( $input ); + $body = $response->getBody(); + $body->rewind(); + $this->assertSame( $expected, $body->getContents() ); + } + + /** @expectedException \InvalidArgumentException */ + public function testCreateFromReturnValueInvalid() { + $rf = new ResponseFactory; + $rf->createFromReturnValue( new ArrayIterator ); + } +} -- 2.20.1