From: jenkins-bot Date: Thu, 13 Jun 2019 22:18:28 +0000 (+0000) Subject: Merge "Expand ResponseFactory" X-Git-Tag: 1.34.0-rc.0~1419 X-Git-Url: https://git.cyclocoop.org/%7B%7B%20url_for%28%27admin_user_edit%27%2C%20iduser=user.userid%29%20%7D%7D?a=commitdiff_plain;h=5074ec954b4ce3890a27562163d3a7a7c7bc3495;hp=7c966af21ad97198ed46513588c27a90014fe81f;p=lhc%2Fweb%2Fwiklou.git Merge "Expand ResponseFactory" --- 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 ); + } +}