mediawiki.api: Prevent misusing #saveOptions

Task T214963 is about how we misused #saveOptions in VisualEditor and
made MediaWiki sad. I'm not sure whether we should fix the issues
there or here, but it seems like the mistakes would be easy to make in
other software, so let's try here first and see what folks think about
it.

* Do not send action=options API requests for IP users
* Wait for the previous request to finish before sending another

Bug: T214963
Change-Id: I85cfc6b5829bcd96e6245431cd979c24630a8fd8

diff --git a/resources/src/mediawiki.api/options.js b/resources/src/mediawiki.api/options.js
index f0ca272..2f69a7a 100644 (file)
--- a/resources/src/mediawiki.api/options.js
+++ b/resources/src/mediawiki.api/options.js
@@ -3,6 +3,8 @@
  */
 ( function () {
 
+       var saveOptionsRequests = {};
+
        $.extend( mw.Api.prototype, {
 
                /**
@@ -29,13 +31,38 @@
                 * If necessary, the options will be saved using several sequential API requests. Only one promise
                 * is always returned that will be resolved when all requests complete.
                 *
+                * If a request from a previous #saveOptions call is still pending, this will wait for it to be
+                * completed, otherwise MediaWiki gets sad. No requests are sent for anonymous users, as they
+                * would fail anyway. See T214963.
+                *
                 * @param {Object} options Options as a `{ name: value, … }` object
                 * @return {jQuery.Promise}
                 */
                saveOptions: function ( options ) {
                        var name, value, bundleable,
                                grouped = [],
+                               promise;
+
+                       // Logged-out users can't have user options; we can't depend on mw.user, that'd be circular
+                       if ( mw.config.get( 'wgUserName' ) === null ) {
+                               return $.Deferred().reject( 'notloggedin' ).promise();
+                       }
+
+                       // If another options request to this API is pending, wait for it first
+                       if (
+                               saveOptionsRequests[ this.defaults.ajax.url ] &&
+                               // Avoid long chains of promises, they may cause memory leaks
+                               saveOptionsRequests[ this.defaults.ajax.url ].state() === 'pending'
+                       ) {
+                               promise = saveOptionsRequests[ this.defaults.ajax.url ].then( function () {
+                                       // Don't expose the old promise's result, it would be confusing
+                                       return $.Deferred().resolve();
+                               }, function () {
+                                       return $.Deferred().resolve();
+                               } );
+                       } else {
                                promise = $.Deferred().resolve();
+                       }
 
                        for ( name in options ) {
                                value = options[ name ] === null ? null : String( options[ name ] );
@@ -89,6 +116,8 @@
                                }.bind( this ) );
                        }
 
+                       saveOptionsRequests[ this.defaults.ajax.url ] = promise;
+
                        return promise;
                }
 

diff --git a/tests/qunit/suites/resources/mediawiki.api/mediawiki.api.options.test.js b/tests/qunit/suites/resources/mediawiki.api/mediawiki.api.options.test.js
index 549deb0..5691a1b 100644 (file)
--- a/tests/qunit/suites/resources/mediawiki.api/mediawiki.api.options.test.js
+++ b/tests/qunit/suites/resources/mediawiki.api/mediawiki.api.options.test.js
@@ -1,5 +1,8 @@
 ( function () {
        QUnit.module( 'mediawiki.api.options', QUnit.newMwEnvironment( {
+               config: {
+                       wgUserName: 'Foo'
+               },
                setup: function () {
                        this.server = this.sandbox.useFakeServer();
                        this.server.respondImmediately = true;
@@ -138,4 +141,21 @@
                        } )
                );
        } );
+
+       QUnit.test( 'saveOptions (anonymous)', function ( assert ) {
+               var promise, test = this;
+
+               mw.config.set( 'wgUserName', null );
+               promise = new mw.Api().saveOptions( { foo: 'bar' } );
+
+               assert.rejects( promise, /notloggedin/, 'Can not save options while not logged in' );
+
+               return promise
+                       .catch( function () {
+                               return $.Deferred().resolve();
+                       } )
+                       .then( function () {
+                               assert.strictEqual( test.server.requests.length, 0, 'No requests made' );
+                       } );
+       } );
 }() );