Add additional configuation parameters to RESTBagOStuff

Class RESTBagOStuff is needed for the Kask session storage
service. Add additional configuration parameters so that
they will work together.

Bug: T215533
Change-Id: Ifbb9f8847bd10d65cc5e24f4aa6cb20cb71de3ca

diff --git a/includes/libs/objectcache/RESTBagOStuff.php b/includes/libs/objectcache/RESTBagOStuff.php
index 4d8ae59..a10d1a4 100644 (file)
--- a/includes/libs/objectcache/RESTBagOStuff.php
+++ b/includes/libs/objectcache/RESTBagOStuff.php
@@ -7,26 +7,42 @@ use Psr\Log\LoggerInterface;
  *
  * Uses URL of the form "baseURL/{KEY}" to store, fetch, and delete values.
  *
- * E.g., when base URL is `/v1/sessions/`, then the store would do:
+ * E.g., when base URL is `/sessions/v1`, then the store would do:
  *
- * `PUT /v1/sessions/12345758`
+ * `PUT /sessions/v1/12345758`
  *
  * and fetch would do:
  *
- * `GET /v1/sessions/12345758`
+ * `GET /sessions/v1/12345758`
  *
  * delete would do:
  *
- * `DELETE /v1/sessions/12345758`
+ * `DELETE /sessions/v1/12345758`
  *
- * Configure with:
+ * Minimal generic configuration:
  *
  * @code
  * $wgObjectCaches['sessions'] = array(
  *     'class' => 'RESTBagOStuff',
- *     'url' => 'http://localhost:7231/wikimedia.org/v1/sessions/'
+ *     'url' => 'http://localhost:7231/wikimedia.org/somepath/'
  * );
  * @endcode
+ *
+ * Configuration for Kask (session storage):
+ * @code
+ * $wgObjectCaches['sessions'] = array(
+ *     'class' => 'RESTBagOStuff',
+ *     'url' => 'https://kaskhost:1234/sessions/v1/',
+ *     'httpParams' => [
+ *             'readHeaders' => [],
+ *             'writeHeaders' => [ 'content-type' => 'application/octet-stream' ],
+ *             'deleteHeaders' => [],
+ *             'writeMethod' => 'POST',
+ *     ],
+ *     'extendedErrorBodyFields' => [ 'type', 'title', 'detail', 'instance' ]
+ * );
+ * $wgSessionCacheType = 'sessions';
+ * @endcode
  */
 class RESTBagOStuff extends BagOStuff {
        /**
@@ -52,10 +68,21 @@ class RESTBagOStuff extends BagOStuff {
         */
        private $url;
 
+       /**
+        * @var array http parameters: readHeaders, writeHeaders, deleteHeaders, writeMethod
+        */
+       private $httpParams;
+
+       /**
+        * @var array additional body fields to log on error, if possible
+        */
+       private $extendedErrorBodyFields;
+
        public function __construct( $params ) {
                if ( empty( $params['url'] ) ) {
                        throw new InvalidArgumentException( 'URL parameter is required' );
                }
+
                if ( empty( $params['client'] ) ) {
                        // Pass through some params to the HTTP client.
                        $clientParams = [
@@ -71,10 +98,19 @@ class RESTBagOStuff extends BagOStuff {
                } else {
                        $this->client = $params['client'];
                }
+
+               $this->httpParams['writeMethod'] = $params['httpParams']['writeMethod'] ?? 'PUT';
+               $this->httpParams['readHeaders'] = $params['httpParams']['readHeaders'] ?? [];
+               $this->httpParams['writeHeaders'] = $params['httpParams']['writeHeaders'] ?? [];
+               $this->httpParams['deleteHeaders'] = $params['httpParams']['deleteHeaders'] ?? [];
+               $this->extendedErrorBodyFields = $params['extendedErrorBodyFields'] ?? [];
+
                // The parent constructor calls setLogger() which sets the logger in $this->client
                parent::__construct( $params );
+
                // Make sure URL ends with /
                $this->url = rtrim( $params['url'], '/' ) . '/';
+
                // Default config, R+W > N; no locks on reads though; writes go straight to state-machine
                $this->attrMap[self::ATTR_SYNCWRITES] = self::QOS_SYNCWRITES_QC;
        }
@@ -90,12 +126,13 @@ class RESTBagOStuff extends BagOStuff {
                $req = [
                        'method' => 'GET',
                        'url' => $this->url . rawurlencode( $key ),
+                       'headers' => $this->httpParams['readHeaders'],
                ];
 
                list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $this->client->run( $req );
                if ( $rcode === 200 ) {
                        if ( is_string( $rbody ) ) {
-                               $value = unserialize( $rbody );
+                               $value = $this->decodeBody( $rbody );
                                /// @FIXME: use some kind of hash or UUID header as CAS token
                                $casToken = ( $value !== false ) ? $rbody : null;
 
@@ -104,7 +141,7 @@ class RESTBagOStuff extends BagOStuff {
                        return false;
                }
                if ( $rcode === 0 || ( $rcode >= 400 && $rcode != 404 ) ) {
-                       return $this->handleError( "Failed to fetch $key", $rcode, $rerr );
+                       return $this->handleError( "Failed to fetch $key", $rcode, $rerr, $rhdrs, $rbody );
                }
                return false;
        }
@@ -113,15 +150,17 @@ class RESTBagOStuff extends BagOStuff {
                // @TODO: respect WRITE_SYNC (e.g. EACH_QUORUM)
                // @TODO: respect $exptime
                $req = [
-                       'method' => 'PUT',
+                       'method' => $this->httpParams['writeMethod'],
                        'url' => $this->url . rawurlencode( $key ),
-                       'body' => serialize( $value )
+                       'body' => $this->encodeBody( $value ),
+                       'headers' => $this->httpParams['writeHeaders'],
                ];
+
                list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $this->client->run( $req );
                if ( $rcode === 200 || $rcode === 201 || $rcode === 204 ) {
                        return true;
                }
-               return $this->handleError( "Failed to store $key", $rcode, $rerr );
+               return $this->handleError( "Failed to store $key", $rcode, $rerr, $rhdrs, $rbody );
        }
 
        public function add( $key, $value, $exptime = 0, $flags = 0 ) {
@@ -138,12 +177,14 @@ class RESTBagOStuff extends BagOStuff {
                $req = [
                        'method' => 'DELETE',
                        'url' => $this->url . rawurlencode( $key ),
+                       'headers' => $this->httpParams['deleteHeaders'],
                ];
+
                list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $this->client->run( $req );
                if ( in_array( $rcode, [ 200, 204, 205, 404, 410 ] ) ) {
                        return true;
                }
-               return $this->handleError( "Failed to delete $key", $rcode, $rerr );
+               return $this->handleError( "Failed to delete $key", $rcode, $rerr, $rhdrs, $rbody );
        }
 
        public function incr( $key, $value = 1 ) {
@@ -158,18 +199,65 @@ class RESTBagOStuff extends BagOStuff {
                return false;
        }
 
+       /**
+        * Processes the response body.
+        *
+        * @param string $body request body to process
+        * @return mixed|bool the processed body, or false on error
+        */
+       private function decodeBody( $body ) {
+               $value = json_decode( $body, true );
+               return ( json_last_error() === JSON_ERROR_NONE ) ? $value : false;
+       }
+
+       /**
+        * Prepares the request body (the "value" portion of our key/value store) for transmission.
+        *
+        * @param string $body request body to prepare
+        * @return string the prepared body, or an empty string on error
+        * @throws LogicException
+        */
+       private function encodeBody( $body ) {
+               $value = json_encode( $body );
+               if ( $value === false ) {
+                       throw new InvalidArgumentException( __METHOD__ . ": body could not be encoded." );
+               }
+               return $value;
+       }
+
        /**
         * Handle storage error
         * @param string $msg Error message
         * @param int $rcode Error code from client
         * @param string $rerr Error message from client
+        * @param array $rhdrs Response headers
+        * @param string $rbody Error body from client (if any)
         * @return false
         */
-       protected function handleError( $msg, $rcode, $rerr ) {
-               $this->logger->error( "$msg : ({code}) {error}", [
+       protected function handleError( $msg, $rcode, $rerr, $rhdrs, $rbody ) {
+               $message = "$msg : ({code}) {error}";
+               $context = [
                        'code' => $rcode,
                        'error' => $rerr
-               ] );
+               ];
+
+               if ( $this->extendedErrorBodyFields !== [] ) {
+                       $body = $this->decodeBody( $rbody );
+                       if ( $body ) {
+                               $extraFields = '';
+                               foreach ( $this->extendedErrorBodyFields as $field ) {
+                                       if ( isset( $body[$field] ) ) {
+                                               $extraFields .= " : ({$field}) {$body[$field]}";
+                                       }
+                               }
+                               if ( $extraFields !== '' ) {
+                                       $message .= " {extra_fields}";
+                                       $context['extra_fields'] = $extraFields;
+                               }
+                       }
+               }
+
+               $this->logger->error( $message, $context );
                $this->setLastError( $rcode === 0 ? self::ERR_UNREACHABLE : self::ERR_UNEXPECTED );
                return false;
        }

diff --git a/tests/phpunit/includes/objectcache/RESTBagOStuffTest.php b/tests/phpunit/includes/objectcache/RESTBagOStuffTest.php
index 66754fc..dfbca70 100644 (file)
--- a/tests/phpunit/includes/objectcache/RESTBagOStuffTest.php
+++ b/tests/phpunit/includes/objectcache/RESTBagOStuffTest.php
@@ -28,9 +28,10 @@ class RESTBagOStuffTest extends MediaWikiTestCase {
        public function testGet() {
                $this->client->expects( $this->once() )->method( 'run' )->with( [
                        'method' => 'GET',
-                       'url' => 'http://test/rest/42xyz42'
+                       'url' => 'http://test/rest/42xyz42',
+                       'headers' => []
                        // list( $rcode, $rdesc, $rhdrs, $rbody, $rerr )
-               ] )->willReturn( [ 200, 'OK', [], 's:8:"somedata";', 0 ] );
+               ] )->willReturn( [ 200, 'OK', [], '"somedata"', 0 ] );
                $result = $this->bag->get( '42xyz42' );
                $this->assertEquals( 'somedata', $result );
        }
@@ -38,7 +39,8 @@ class RESTBagOStuffTest extends MediaWikiTestCase {
        public function testGetNotExist() {
                $this->client->expects( $this->once() )->method( 'run' )->with( [
                        'method' => 'GET',
-                       'url' => 'http://test/rest/42xyz42'
+                       'url' => 'http://test/rest/42xyz42',
+                       'headers' => []
                        // list( $rcode, $rdesc, $rhdrs, $rbody, $rerr )
                ] )->willReturn( [ 404, 'Not found', [], 'Nothing to see here', 0 ] );
                $result = $this->bag->get( '42xyz42' );
@@ -48,7 +50,8 @@ class RESTBagOStuffTest extends MediaWikiTestCase {
        public function testGetBadClient() {
                $this->client->expects( $this->once() )->method( 'run' )->with( [
                        'method' => 'GET',
-                       'url' => 'http://test/rest/42xyz42'
+                       'url' => 'http://test/rest/42xyz42',
+                       'headers' => []
                        // list( $rcode, $rdesc, $rhdrs, $rbody, $rerr )
                ] )->willReturn( [ 0, '', [], '', 'cURL has failed you today' ] );
                $result = $this->bag->get( '42xyz42' );
@@ -59,7 +62,8 @@ class RESTBagOStuffTest extends MediaWikiTestCase {
        public function testGetBadServer() {
                $this->client->expects( $this->once() )->method( 'run' )->with( [
                        'method' => 'GET',
-                       'url' => 'http://test/rest/42xyz42'
+                       'url' => 'http://test/rest/42xyz42',
+                       'headers' => []
                        // list( $rcode, $rdesc, $rhdrs, $rbody, $rerr )
                ] )->willReturn( [ 500, 'Too busy', [], 'Server is too busy', '' ] );
                $result = $this->bag->get( '42xyz42' );
@@ -71,7 +75,8 @@ class RESTBagOStuffTest extends MediaWikiTestCase {
                $this->client->expects( $this->once() )->method( 'run' )->with( [
                        'method' => 'PUT',
                        'url' => 'http://test/rest/42xyz42',
-                       'body' => 's:8:"postdata";'
+                       'body' => '"postdata"',
+                       'headers' => []
                        // list( $rcode, $rdesc, $rhdrs, $rbody, $rerr )
                ] )->willReturn( [ 200, 'OK', [], 'Done', 0 ] );
                $result = $this->bag->set( '42xyz42', 'postdata' );
@@ -82,6 +87,7 @@ class RESTBagOStuffTest extends MediaWikiTestCase {
                $this->client->expects( $this->once() )->method( 'run' )->with( [
                        'method' => 'DELETE',
                        'url' => 'http://test/rest/42xyz42',
+                       'headers' => []
                        // list( $rcode, $rdesc, $rhdrs, $rbody, $rerr )
                ] )->willReturn( [ 200, 'OK', [], 'Done', 0 ] );
                $result = $this->bag->delete( '42xyz42' );