From 15b0e653277c98e32004ed86d8b634d3a9d74ed3 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Wed, 28 Feb 2018 20:19:28 -0800 Subject: [PATCH] resourceloader: Fix mw.loader to compute version for current request only Previously, the 'version' query parameter was computed before request-splitting which meant that all requests within the same 'source/group'-batch carried the same 'version' parameter. This was then consistently rejected on the server due to it not batching the combined hash of modules for any given request. In practice this happened very rarely (if at all) in production, because urls don't usually hit anywhere near 2000 in common use. Bug: T188076 Change-Id: I211523d4781623873887a05d048f56cccd28432c --- resources/src/mediawiki/mediawiki.js | 50 +++++++++++-------- tests/qunit/data/load.mock.php | 31 +++++++++++- .../mediawiki/mediawiki.loader.test.js | 33 ++++++++++++ 3 files changed, 91 insertions(+), 23 deletions(-) diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js index aa93ca2b75..450d4ff648 100644 --- a/resources/src/mediawiki/mediawiki.js +++ b/resources/src/mediawiki/mediawiki.js @@ -1541,13 +1541,15 @@ * * @private * @param {string} moduleStr Module list for load.php `module` query parameter - * @param {Object} currReqBase Object with other parameters (other than 'modules') to use in the request + * @param {Array} modules List of module names (not packed) + * @param {Object} currReqBase Object with base parameters for this request * @param {string} sourceLoadScript URL of load.php */ - function doRequest( moduleStr, currReqBase, sourceLoadScript ) { + function doRequest( moduleStr, modules, currReqBase, sourceLoadScript ) { // Optimisation: Inherit (Object.create), not copy ($.extend) var query = Object.create( currReqBase ); query.modules = moduleStr; + query.version = getCombinedVersion( modules ); query = sortQuery( query ); addScript( sourceLoadScript + '?' + $.param( query ) ); } @@ -1592,7 +1594,7 @@ function batchRequest( batch ) { var reqBase, splits, maxQueryLength, b, bSource, bGroup, bSourceGroup, source, group, i, modules, sourceLoadScript, - currReqBase, currReqBaseLength, moduleMap, l, + currReqBase, currReqBaseLength, moduleMap, currReqModules, l, lastDotIndex, prefix, suffix, bytesAdded; if ( !batch.length ) { @@ -1603,7 +1605,7 @@ // misses for otherwise identical content. batch.sort(); - // Build a list of query parameters common to all requests + // Query parameters common to all requests reqBase = { skin: mw.config.get( 'skin' ), lang: mw.config.get( 'wgUserLanguage' ), @@ -1636,51 +1638,57 @@ // modules for this group from this source. modules = splits[ source ][ group ]; + // Query parameters common to requests for this module group // Optimisation: Inherit (Object.create), not copy ($.extend) currReqBase = Object.create( reqBase ); - currReqBase.version = getCombinedVersion( modules ); - - // For user modules append a user name to the query string. + // User modules require a user name in the query string. if ( group === 'user' && mw.config.get( 'wgUserName' ) !== null ) { currReqBase.user = mw.config.get( 'wgUserName' ); } - currReqBaseLength = $.param( currReqBase ).length; + + // In addition to currReqBase, doRequest() will also add 'modules' and 'version'. + // > '&modules='.length === 9 + // > '&version=1234567'.length === 16 + // > 9 + 16 = 25 + currReqBaseLength = $.param( currReqBase ).length + 25; + // We may need to split up the request to honor the query string length limit, // so build it piece by piece. - l = currReqBaseLength + 9; // '&modules='.length == 9 - + l = currReqBaseLength; moduleMap = {}; // { prefix: [ suffixes ] } + currReqModules = []; for ( i = 0; i < modules.length; i++ ) { // Determine how many bytes this module would add to the query string lastDotIndex = modules[ i ].lastIndexOf( '.' ); - // If lastDotIndex is -1, substr() returns an empty string prefix = modules[ i ].substr( 0, lastDotIndex ); suffix = modules[ i ].slice( lastDotIndex + 1 ); - bytesAdded = hasOwn.call( moduleMap, prefix ) ? suffix.length + 3 : // '%2C'.length == 3 modules[ i ].length + 3; // '%7C'.length == 3 - // If the url would become too long, create a new one, - // but don't create empty requests - if ( maxQueryLength > 0 && !$.isEmptyObject( moduleMap ) && l + bytesAdded > maxQueryLength ) { - // This url would become too long, create a new one, and start the old one - doRequest( buildModulesString( moduleMap ), currReqBase, sourceLoadScript ); + // If the url would become too long, create a new one, but don't create empty requests + if ( maxQueryLength > 0 && currReqModules.length && l + bytesAdded > maxQueryLength ) { + // Dispatch what we've got... + doRequest( buildModulesString( moduleMap ), currReqModules, currReqBase, sourceLoadScript ); + // .. and start again. + l = currReqBaseLength; moduleMap = {}; - l = currReqBaseLength + 9; + currReqModules = []; + mw.track( 'resourceloader.splitRequest', { maxQueryLength: maxQueryLength } ); } if ( !hasOwn.call( moduleMap, prefix ) ) { moduleMap[ prefix ] = []; } - moduleMap[ prefix ].push( suffix ); l += bytesAdded; + moduleMap[ prefix ].push( suffix ); + currReqModules.push( modules[ i ] ); } // If there's anything left in moduleMap, request that too - if ( !$.isEmptyObject( moduleMap ) ) { - doRequest( buildModulesString( moduleMap ), currReqBase, sourceLoadScript ); + if ( currReqModules.length ) { + doRequest( buildModulesString( moduleMap ), currReqModules, currReqBase, sourceLoadScript ); } } } diff --git a/tests/qunit/data/load.mock.php b/tests/qunit/data/load.mock.php index fed5746c02..43ee0c7d07 100644 --- a/tests/qunit/data/load.mock.php +++ b/tests/qunit/data/load.mock.php @@ -49,18 +49,45 @@ mw.loader.implement( 'testNotSkipped', function () {}, {}, {}); 'testUsesSkippable' => " mw.loader.implement( 'testUsesSkippable', function () {}, {}, {}); +", + + 'testUrlInc' => " +mw.loader.implement( 'testUrlInc', function () {} ); +", + 'testUrlInc.a' => " +mw.loader.implement( 'testUrlInc.a', function () {} ); +", + 'testUrlInc.b' => " +mw.loader.implement( 'testUrlInc.b', function () {} ); ", ]; $response = ''; -// Only support for non-encoded module names, full module names expected +// Does not support the full behaviour of ResourceLoaderContext::expandModuleNames(), +// Only supports dotless module names joined by comma, +// with the exception of the hardcoded cases for testUrl*. if ( isset( $_GET['modules'] ) ) { - $modules = explode( ',', $_GET['modules'] ); + if ( $_GET['modules'] === 'testUrlInc,testUrlIncDump|testUrlInc.a,b' ) { + $modules = [ 'testUrlInc', 'testUrlIncDump', 'testUrlInc.a', 'testUrlInc.b' ]; + } else { + $modules = explode( ',', $_GET['modules'] ); + } foreach ( $modules as $module ) { if ( isset( $moduleImplementations[$module] ) ) { $response .= $moduleImplementations[$module]; + } elseif ( preg_match( '/^test.*Dump$/', $module ) === 1 ) { + $queryModules = $_GET['modules']; + $queryVersion = isset( $_GET['version'] ) ? strval( $_GET['version'] ) : null; + $response .= 'mw.loader.implement( ' . json_encode( $module ) + . ', function ( $, jQuery, require, module ) {' + . 'module.exports.query = { ' + . 'modules: ' . json_encode( $queryModules ) . ',' + . 'version: ' . json_encode( $queryVersion ) + . ' };' + . '} );'; } else { + // Default $response .= 'mw.loader.state(' . json_encode( $module ) . ', "missing" );' . "\n"; } } diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js index e774112188..6c988ef9f4 100644 --- a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js +++ b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js @@ -550,6 +550,39 @@ assert.strictEqual( mw.loader.getState( 'test.empty' ), 'ready' ); } ); + // @covers mw.loader#batchRequest + // This is a regression test because in the past we called getCombinedVersion() + // for all requested modules, before url splitting took place. + // Discovered as part of T188076, but not directly related. + QUnit.test( 'Url composition (modules considered for version)', function ( assert ) { + mw.loader.register( [ + // [module, version, dependencies, group, source] + [ 'testUrlInc', 'url', [], null, 'testloader' ], + [ 'testUrlIncDump', 'dump', [], null, 'testloader' ] + ] ); + + mw.config.set( 'wgResourceLoaderMaxQueryLength', 10 ); + + return mw.loader.using( [ 'testUrlIncDump', 'testUrlInc' ] ).then( function ( require ) { + assert.propEqual( + require( 'testUrlIncDump' ).query, + { + modules: 'testUrlIncDump', + // Expected: Wrapped hash just for this one module + // $hash = hash( 'fnv132', 'dump'); + // base_convert( $hash, 16, 36 ); // "13e9zzn" + // Previously: Wrapped hash for both modules, despite being in separate requests + // $hash = hash( 'fnv132', 'urldump' ); + // base_convert( $hash, 16, 36 ); // "18kz9ca" + version: '13e9zzn' + }, + 'Query parameters' + ); + + assert.strictEqual( mw.loader.getState( 'testUrlInc' ), 'ready', 'testUrlInc also loaded' ); + } ); + } ); + QUnit.test( 'Broken indirect dependency', function ( assert ) { // don't emit an error event this.sandbox.stub( mw, 'track' ); -- 2.20.1