Expand ResponseFactory
authorTim Starling <tstarling@wikimedia.org>
Tue, 4 Jun 2019 05:52:19 +0000 (15:52 +1000)
committerTim Starling <tstarling@wikimedia.org>
Wed, 12 Jun 2019 00:22:34 +0000 (10:22 +1000)
* 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
includes/Rest/Router.php
tests/phpunit/includes/Rest/ResponseFactoryTest.php [new file with mode: 0644]

index 21307bc..7ccb612 100644 (file)
 
 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 "<!doctype html><title>Redirect</title><a href=\"$url\">$url</a>";
+       }
+
 }
index 0c45839..ab54439 100644 (file)
@@ -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 (file)
index 0000000..6ccacda
--- /dev/null
@@ -0,0 +1,139 @@
+<?php
+
+namespace MediaWiki\Tests\Rest;
+
+use ArrayIterator;
+use MediaWiki\Rest\HttpException;
+use MediaWiki\Rest\ResponseFactory;
+use MediaWikiTestCase;
+
+/** @covers \MediaWiki\Rest\ResponseFactory */
+class ResponseFactoryTest extends MediaWikiTestCase {
+       public static function provideEncodeJson() {
+               return [
+                       [ (object)[], '{}' ],
+                       [ '/', '"/"' ],
+                       [ '£', '"£"' ],
+                       [ [], '[]' ],
+               ];
+       }
+
+       /** @dataProvider provideEncodeJson */
+       public function testEncodeJson( $input, $expected ) {
+               $rf = new ResponseFactory;
+               $this->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 );
+       }
+}