resourceloader: Enforce strict string keys in mw.Map

* No longer perform any key-to-string casting in get(), set(),
  or exists().
  - Setting a non-string key is ignored.
  - Looking for a non-string key that looks like a string key
    that exists will not find it.

Clean up:

* Remove needless `selection=slice.call(selection)` in mw.Map#get
  Follows-up 77d71f743 (r75486). Not sure why this was here.

* Inline logic for the multi-key retrieval instead of recursing.
  This also removes undocumented behaviour where a nested array
  could be passed to mw.config.get(), e.g. mw.config.get([['debug']]),
  which would produces a nested object.

Change-Id: I84506930919e59ce16d96d6d623db5aae5f273e1

diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js
index 00b275f..33f146b 100644 (file)
--- a/resources/src/mediawiki/mediawiki.js
+++ b/resources/src/mediawiki/mediawiki.js
@@ -52,6 +52,7 @@
                /* eslint-enable no-bitwise */
        }
 
+       // <https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Set>
        StringSet = window.Set || ( function () {
                /**
                 * @private
@@ -64,7 +65,7 @@
                        this.set[ value ] = true;
                };
                StringSet.prototype.has = function ( value ) {
-                       return this.set.hasOwnProperty( value );
+                       return hasOwn.call( this.set, value );
                };
                return StringSet;
        }() );
@@ -133,22 +134,22 @@
                 *
                 * @param {string|Array} [selection] Key or array of keys to retrieve values for.
                 * @param {Mixed} [fallback=null] Value for keys that don't exist.
-                * @return {Mixed|Object| null} If selection was a string, returns the value,
+                * @return {Mixed|Object|null} If selection was a string, returns the value,
                 *  If selection was an array, returns an object of key/values.
-                *  If no selection is passed, the internal container is returned. (Beware that,
-                *  as is the default in JavaScript, the object is returned by reference.)
+                *  If no selection is passed, a new object with all key/values is returned.
                 */
                get: function ( selection, fallback ) {
                        var results, i;
-                       // If we only do this in the `return` block, it'll fail for the
-                       // call to get() from the mutli-selection block.
                        fallback = arguments.length > 1 ? fallback : null;
 
                        if ( $.isArray( selection ) ) {
-                               selection = slice.call( selection );
                                results = {};
                                for ( i = 0; i < selection.length; i++ ) {
-                                       results[ selection[ i ] ] = this.get( selection[ i ], fallback );
+                                       if ( typeof selection[ i ] === 'string' ) {
+                                               results[ selection[ i ] ] = hasOwn.call( this.values, selection[ i ] ) ?
+                                                       this.values[ selection[ i ] ] :
+                                                       fallback;
+                                       }
                                }
                                return results;
                        }
@@ -160,11 +161,15 @@
                        }
 
                        if ( selection === undefined ) {
-                               return this.values;
+                               results = {};
+                               for ( i in this.values ) {
+                                       results[ i ] = this.values[ i ];
+                               }
+                               return results;
                        }
 
                        // Invalid selection key
-                       return null;
+                       return fallback;
                },
 
                /**

diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
index 697f32d..119222a 100644 (file)
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
@@ -114,16 +114,23 @@
                assert.strictEqual( conf.set( arry, 'Arry' ), false, 'Map.set returns boolean false if key was invalid (Array)' );
                assert.strictEqual( conf.set( nummy, 'Nummy' ), false, 'Map.set returns boolean false if key was invalid (Number)' );
 
-               assert.strictEqual( conf.get( funky ), null, 'Map.get ruturns null if selection was invalid (Function)' );
-               assert.strictEqual( conf.get( nummy ), null, 'Map.get ruturns null if selection was invalid (Number)' );
-
                conf.set( String( nummy ), 'I used to be a number' );
 
+               assert.strictEqual( conf.get( funky ), null, 'Map.get returns null if selection was invalid (Function)' );
+               assert.strictEqual( conf.get( nummy ), null, 'Map.get returns null if selection was invalid (Number)' );
+               assert.propEqual( conf.get( [ nummy ] ), {}, 'Map.get returns null if selection was invalid (multiple)' );
+               assert.strictEqual( conf.get( nummy, false ), false, 'Map.get returns custom fallback for invalid selection' );
+
                assert.strictEqual( conf.exists( 'doesNotExist' ), false, 'Map.exists where property does not exist' );
                assert.strictEqual( conf.exists( 'undef' ), true, 'Map.exists where value is `undefined`' );
-               assert.strictEqual( conf.exists( nummy ), false, 'Map.exists where key is invalid but looks like an existing key' );
+               assert.strictEqual( conf.exists( [ 'undef', 'example' ] ), true, 'Map.exists with multiple keys (all existing)' );
+               assert.strictEqual( conf.exists( [ 'example', 'doesNotExist' ] ), false, 'Map.exists with multiple keys (some non-existing)' );
+               assert.strictEqual( conf.exists( [] ), true, 'Map.exists with no keys' );
+               assert.strictEqual( conf.exists( nummy ), false, 'Map.exists with invalid key that looks like an existing key' );
+               assert.strictEqual( conf.exists( [ nummy ] ), false, 'Map.exists with invalid key that looks like an existing key' );
 
                // Multiple values at once
+               conf = new mw.Map();
                someValues = {
                        foo: 'bar',
                        lorem: 'ipsum',
@@ -140,9 +147,10 @@
                        notExist: null
                }, 'Map.get return includes keys that were not found as null values' );
 
-               // Interacting with globals and accessing the values object
-               assert.strictEqual( conf.get(), conf.values, 'Map.get returns the entire values object by reference (if called without arguments)' );
+               assert.propEqual( conf.values, someValues, 'Map.values is an internal object with all values (exposed for convenience)' );
+               assert.propEqual( conf.get(), someValues, 'Map.get() returns an object with all values' );
 
+               // Interacting with globals
                conf.set( 'globalMapChecker', 'Hi' );
 
                assert.ok( ( 'globalMapChecker' in window ) === false, 'Map does not its store values in the window object by default' );