resourceloader: Support 'versionCallback' for computed package files
authorTimo Tijhof <krinklemail@gmail.com>
Wed, 12 Jun 2019 00:57:17 +0000 (01:57 +0100)
committerAaron Schulz <aschulz@wikimedia.org>
Fri, 14 Jun 2019 17:05:43 +0000 (17:05 +0000)
The use cases we've seen for using computed (or virtual) package files,
involve expensive computations to expand and transform data for the client
that we don't want to evaluate in full just to compute the module's version
hash (e.g. in the StartupModule, where we need to do this for 1000s of
modules).

For such cases, the module can specify a 'versionCallback' of which the
return value will be used to seed the module's version hash. The default
remains the same as before, which is to use the full content to seed the
version hash (via getDefinitionSummary).

Bug: T223260
Change-Id: I76f573239e6bd429287e7adb33a92ffd5e260c20

includes/resourceloader/ResourceLoaderFileModule.php
tests/phpunit/includes/resourceloader/ResourceLoaderFileModuleTest.php

index 015c828..87a4515 100644 (file)
@@ -619,9 +619,24 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
                        $options[$member] = $this->{$member};
                }
 
+               $packageFiles = $this->expandPackageFiles( $context );
+               if ( $packageFiles ) {
+                       // Extract the minimum needed:
+                       // - The 'main' pointer (included as-is).
+                       // - The 'files' array, simplied to only which files exist (the keys of
+                       //   this array), and something that represents their non-file content.
+                       //   For packaged files that reflect files directly from disk, the
+                       //   'getFileHashes' method tracks this already.
+                       //   It is important that the keys of the 'files' array are preserved,
+                       //   as they affect the module output.
+                       $packageFiles['files'] = array_map( function ( $fileInfo ) {
+                               return $fileInfo['definitionSummary'] ?? ( $fileInfo['content'] ?? null );
+                       }, $packageFiles['files'] );
+               }
+
                $summary[] = [
                        'options' => $options,
-                       'packageFiles' => $this->expandPackageFiles( $context ),
+                       'packageFiles' => $packageFiles,
                        'fileHashes' => $this->getFileHashes( $context ),
                        'messageBlob' => $this->getMessageBlob( $context ),
                ];
@@ -1068,16 +1083,22 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
        }
 
        /**
-        * Expand the packageFiles definition into something that's (almost) the right format for
-        * getPackageFiles() to return. This expands shorthands, resolves config vars and callbacks,
-        * but does not expand file paths or read the actual contents of files. Those things are done
-        * by getPackageFiles().
+        * Internal helper for use by getPackageFiles(), getFileHashes() and getDefinitionSummary().
+        *
+        * This expands the 'packageFiles' definition into something that's (almost) the right format
+        * for getPackageFiles() to return. It expands shorthands, resolves config vars, and handles
+        * summarising any non-file data for getVersionHash(). For file-based data, getFileHashes()
+        * handles it instead, which also ends up in getDefinitionSummary().
         *
-        * This is split up in this way so that getFileHashes() can get a list of file names, and
-        * getDefinitionSummary() can get config vars and callback results in their expanded form.
+        * What it does not do is reading the actual contents of any specified files, nor invoking
+        * the computation callbacks. Those things are done by getPackageFiles() instead to improve
+        * backend performance by only doing this work when the module response is needed, and not
+        * when merely computing the version hash for StartupModule, or when checking
+        * If-None-Match headers for a HTTP 304 response.
         *
         * @param ResourceLoaderContext $context
         * @return array|null
+        * @throws MWException If the 'packageFiles' definition is invalid.
         */
        private function expandPackageFiles( ResourceLoaderContext $context ) {
                $hash = $context->getHash();
@@ -1113,19 +1134,32 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
                                }
                        }
 
+                       // Perform expansions (except 'file' and 'callback'), creating one of these keys:
+                       // - 'content': literal value.
+                       // - 'filePath': content to be read from a file.
+                       // - 'callback': content computed by a callable.
                        if ( isset( $fileInfo['content'] ) ) {
                                $expanded['content'] = $fileInfo['content'];
                        } elseif ( isset( $fileInfo['file'] ) ) {
                                $expanded['filePath'] = $fileInfo['file'];
                        } elseif ( isset( $fileInfo['callback'] ) ) {
-                               if ( is_callable( $fileInfo['callback'] ) ) {
-                                       $expanded['content'] = $fileInfo['callback']( $context );
-                               } else {
+                               if ( !is_callable( $fileInfo['callback'] ) ) {
                                        $msg = __METHOD__ . ": invalid callback for package file \"{$fileInfo['name']}\"" .
                                                " in module \"{$this->getName()}\"";
                                        wfDebugLog( 'resourceloader', $msg );
                                        throw new MWException( $msg );
                                }
+                               if ( isset( $fileInfo['versionCallback'] ) ) {
+                                       if ( !is_callable( $fileInfo['versionCallback'] ) ) {
+                                               throw new MWException( __METHOD__ . ": invalid versionCallback for file" .
+                                                       " \"{$fileInfo['name']}\" in module \"{$this->getName()}\"" );
+                                       }
+                                       $expanded['definitionSummary'] = ( $fileInfo['versionCallback'] )( $context );
+                                       // Don't invoke 'callback' here as it may be expensive (T223260).
+                                       $expanded['callback'] = $fileInfo['callback'];
+                               } else {
+                                       $expanded['content'] = ( $fileInfo['callback'] )( $context );
+                               }
                        } elseif ( isset( $fileInfo['config'] ) ) {
                                if ( $type !== 'data' ) {
                                        $msg = __METHOD__ . ": invalid use of \"config\" for package file \"{$fileInfo['name']}\" " .
@@ -1184,6 +1218,8 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
 
                // Expand file contents
                foreach ( $expandedPackageFiles['files'] as &$fileInfo ) {
+                       // Turn any 'filePath' or 'callback' key into actual 'content',
+                       // and remove the key after that.
                        if ( isset( $fileInfo['filePath'] ) ) {
                                $localPath = $this->getLocalPath( $fileInfo['filePath'] );
                                if ( !file_exists( $localPath ) ) {
@@ -1198,7 +1234,13 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
                                }
                                $fileInfo['content'] = $content;
                                unset( $fileInfo['filePath'] );
+                       } elseif ( isset( $fileInfo['callback'] ) ) {
+                               $fileInfo['content'] = ( $fileInfo['callback'] )( $context );
+                               unset( $fileInfo['callback'] );
                        }
+
+                       // Not needed for client response, exists for getDefinitionSummary().
+                       unset( $fileInfo['definitionSummary'] );
                }
 
                return $expandedPackageFiles;
index 2aa0d27..1585cbc 100644 (file)
@@ -373,6 +373,68 @@ class ResourceLoaderFileModuleTest extends ResourceLoaderTestCase {
                        'lessVars' => [ 'key' => 'value' ],
                ];
                yield 'identical Less variables' => [ $x, $x, true ];
+
+               $a = [
+                       'packageFiles' => [ [ 'name' => 'data.json', 'callback' => function () {
+                               return [ 'aaa' ];
+                       } ] ]
+               ];
+               $b = [
+                       'packageFiles' => [ [ 'name' => 'data.json', 'callback' => function () {
+                               return [ 'bbb' ];
+                       } ] ]
+               ];
+               yield 'packageFiles with different callback' => [ $a, $b, false ];
+
+               $a = [
+                       'packageFiles' => [ [ 'name' => 'aaa.json', 'callback' => function () {
+                               return [ 'x' ];
+                       } ] ]
+               ];
+               $b = [
+                       'packageFiles' => [ [ 'name' => 'bbb.json', 'callback' => function () {
+                               return [ 'x' ];
+                       } ] ]
+               ];
+               yield 'packageFiles with different file name and a callback' => [ $a, $b, false ];
+
+               $a = [
+                       'packageFiles' => [ [ 'name' => 'data.json', 'versionCallback' => function () {
+                               return [ 'A-version' ];
+                       }, 'callback' => function () {
+                               throw new Exception( 'Unexpected computation' );
+                       } ] ]
+               ];
+               $b = [
+                       'packageFiles' => [ [ 'name' => 'data.json', 'versionCallback' => function () {
+                               return [ 'B-version' ];
+                       }, 'callback' => function () {
+                               throw new Exception( 'Unexpected computation' );
+                       } ] ]
+               ];
+               yield 'packageFiles with different versionCallback' => [ $a, $b, false ];
+
+               $a = [
+                       'packageFiles' => [ [ 'name' => 'aaa.json',
+                               'versionCallback' => function () {
+                                       return [ 'X-version' ];
+                               },
+                               'callback' => function () {
+                                       throw new Exception( 'Unexpected computation' );
+                               }
+                       ] ]
+               ];
+               $b = [
+                       'packageFiles' => [ [ 'name' => 'bbb.json',
+                               'versionCallback' => function () {
+                                       return [ 'X-version' ];
+                               },
+                               'callback' => function () {
+                                       throw new Exception( 'Unexpected computation' );
+                               }
+                       ] ]
+               ];
+               yield 'packageFiles with different file name and a versionCallback' => [ $a, $b, false ];
        }
 
        /**
@@ -471,7 +533,7 @@ class ResourceLoaderFileModuleTest extends ResourceLoaderTestCase {
                                        'main' => 'init.js'
                                ]
                        ],
-                       [
+                       'package file with callback' => [
                                $base + [
                                        'packageFiles' => [
                                                [ 'name' => 'foo.json', 'content' => [ 'Hello' => 'world' ] ],
@@ -518,6 +580,34 @@ class ResourceLoaderFileModuleTest extends ResourceLoaderTestCase {
                                        'lang' => 'fy'
                                ]
                        ],
+                       'package file with callback and versionCallback' => [
+                               $base + [
+                                       'packageFiles' => [
+                                               [ 'name' => 'bar.js', 'content' => "console.log('Hello');" ],
+                                               [ 'name' => 'data.json', 'versionCallback' => function ( $context ) {
+                                                       return $context->getLanguage();
+                                               }, 'callback' => function ( $context ) {
+                                                       return [ 'langCode' => $context->getLanguage() ];
+                                               } ],
+                                       ]
+                               ],
+                               [
+                                       'files' => [
+                                               'bar.js' => [
+                                                       'type' => 'script',
+                                                       'content' => "console.log('Hello');",
+                                               ],
+                                               'data.json' => [
+                                                       'type' => 'data',
+                                                       'content' => [ 'langCode' => 'fy' ]
+                                               ],
+                                       ],
+                                       'main' => 'bar.js'
+                               ],
+                               [
+                                       'lang' => 'fy'
+                               ]
+                       ],
                        [
                                $base + [
                                        'packageFiles' => [
@@ -526,7 +616,7 @@ class ResourceLoaderFileModuleTest extends ResourceLoaderTestCase {
                                ],
                                false
                        ],
-                       [
+                       'package file with invalid callback' => [
                                $base + [
                                        'packageFiles' => [
                                                [ 'name' => 'foo.json', 'callback' => 'functionThatDoesNotExist142857' ]