Disable CAS check when saving TestUser data.

During testing, we are not worried about data loss, so we can safely
bypass the CAS check when setting up a test fixture.

This change was added to address sporadic test failures like the following:

18:03:38 1) ApiEchoMarkReadTest::testMarkReadWithList
18:03:38 MWException: CAS update failed on user_touched for user ID '2' (read from slave); the version of the user to be saved is older than the current version.
18:03:38
18:03:38 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/includes/user/User.php:3931
18:03:38 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/includes/TestUser.php:83
18:03:38 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/includes/api/ApiTestCase.php:30
18:03:38 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/Echo/tests/phpunit/api/ApiEchoMarkReadTest.php:11
18:03:38 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:370

Bug: T131178
Change-Id: I99b43e0db85bc2c1cd335c82971df4e95520d34b

diff --git a/includes/user/User.php b/includes/user/User.php
index ce2ac83..eb5fdaf 100644 (file)
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -1467,6 +1467,24 @@ class User implements IDBAccessObject {
                return $toPromote;
        }
 
+       /**
+        * Builds update conditions. Additional conditions may be added to $conditions to
+        * protected against race conditions using a compare-and-set (CAS) mechanism
+        * based on comparing $this->mTouched with the user_touched field.
+        *
+        * @param DatabaseBase $db
+        * @param array $conditions WHERE conditions for use with DatabaseBase::update
+        * @return array WHERE conditions for use with DatabaseBase::update
+        */
+       protected function makeUpdateConditions( DatabaseBase $db, array $conditions ) {
+               if ( $this->mTouched ) {
+                       // CAS check: only update if the row wasn't changed sicne it was loaded.
+                       $conditions['user_touched'] = $db->timestamp( $this->mTouched );
+               }
+
+               return $conditions;
+       }
+
        /**
         * Bump user_touched if it didn't change since this object was loaded
         *
@@ -1484,16 +1502,14 @@ class User implements IDBAccessObject {
                }
 
                // Get a new user_touched that is higher than the old one
-               $oldTouched = $this->mTouched;
                $newTouched = $this->newTouchedTimestamp();
 
                $dbw = wfGetDB( DB_MASTER );
                $dbw->update( 'user',
                        [ 'user_touched' => $dbw->timestamp( $newTouched ) ],
-                       [
+                       $this->makeUpdateConditions( $dbw, [
                                'user_id' => $this->mId,
-                               'user_touched' => $dbw->timestamp( $oldTouched ) // CAS check
-                       ],
+                       ] ),
                        __METHOD__
                );
                $success = ( $dbw->affectedRows() > 0 );
@@ -3908,7 +3924,6 @@ class User implements IDBAccessObject {
                // Get a new user_touched that is higher than the old one.
                // This will be used for a CAS check as a last-resort safety
                // check against race conditions and slave lag.
-               $oldTouched = $this->mTouched;
                $newTouched = $this->newTouchedTimestamp();
 
                $dbw = wfGetDB( DB_MASTER );
@@ -3922,10 +3937,9 @@ class User implements IDBAccessObject {
                                'user_token' => strval( $this->mToken ),
                                'user_email_token' => $this->mEmailToken,
                                'user_email_token_expires' => $dbw->timestampOrNull( $this->mEmailTokenExpires ),
-                       ], [ /* WHERE */
+                       ], $this->makeUpdateConditions( $dbw, [ /* WHERE */
                                'user_id' => $this->mId,
-                               'user_touched' => $dbw->timestamp( $oldTouched ) // CAS check
-                       ], __METHOD__
+                       ] ), __METHOD__
                );
 
                if ( !$dbw->affectedRows() ) {

diff --git a/tests/phpunit/includes/TestUser.php b/tests/phpunit/includes/TestUser.php
index 247b6e4..8b8cbcd 100644 (file)
--- a/tests/phpunit/includes/TestUser.php
+++ b/tests/phpunit/includes/TestUser.php
@@ -78,6 +78,12 @@ class TestUser {
                        $this->user->removeGroup( $group );
                }
                if ( $change ) {
+                       // Disable CAS check before saving. The User object may have been initialized from cached
+                       // information that may be out of whack with the database during testing. If tests were
+                       // perfectly isolated, this would not happen. But if it does happen, let's just ignore the
+                       // inconsistency, and just write the data we want - during testing, we are not worried
+                       // about data loss.
+                       $this->user->mTouched = '';
                        $this->user->saveSettings();
                }
        }