* Added some wfProfileIn() calls to file backend code.

* Made FileBackend::getFileProps() final.
* Added exception needed in SwiftFileBackend::getConnection().
* Various FileBackendTests fixes and additions. Optimized it a bit by keeping the backend instance in memory.

diff --git a/includes/filerepo/backend/FileBackend.php b/includes/filerepo/backend/FileBackend.php
index 0e07923..ee0d312 100644 (file)
--- a/includes/filerepo/backend/FileBackend.php
+++ b/includes/filerepo/backend/FileBackend.php
@@ -629,12 +629,14 @@ abstract class FileBackend extends FileBackendBase {
         * @return Status
         */
        final public function createInternal( array $params ) {
+               wfProfileIn( __METHOD__ );
                if ( strlen( $params['content'] ) > $this->maxFileSizeInternal() ) {
                        $status = Status::newFatal( 'backend-fail-create', $params['dst'] );
                } else {
                        $status = $this->doCreateInternal( $params );
                        $this->clearCache( array( $params['dst'] ) );
                }
+               wfProfileOut( __METHOD__ );
                return $status;
        }
 
@@ -656,12 +658,14 @@ abstract class FileBackend extends FileBackendBase {
         * @return Status
         */
        final public function storeInternal( array $params ) {
+               wfProfileIn( __METHOD__ );
                if ( filesize( $params['src'] ) > $this->maxFileSizeInternal() ) {
                        $status = Status::newFatal( 'backend-fail-store', $params['dst'] );
                } else {
                        $status = $this->doStoreInternal( $params );
                        $this->clearCache( array( $params['dst'] ) );
                }
+               wfProfileOut( __METHOD__ );
                return $status;
        }
 
@@ -683,8 +687,10 @@ abstract class FileBackend extends FileBackendBase {
         * @return Status
         */
        final public function copyInternal( array $params ) {
+               wfProfileIn( __METHOD__ );
                $status = $this->doCopyInternal( $params );
                $this->clearCache( array( $params['dst'] ) );
+               wfProfileOut( __METHOD__ );
                return $status;
        }
 
@@ -705,8 +711,10 @@ abstract class FileBackend extends FileBackendBase {
         * @return Status
         */
        final public function deleteInternal( array $params ) {
+               wfProfileIn( __METHOD__ );
                $status = $this->doDeleteInternal( $params );
                $this->clearCache( array( $params['src'] ) );
+               wfProfileOut( __METHOD__ );
                return $status;
        }
 
@@ -728,8 +736,10 @@ abstract class FileBackend extends FileBackendBase {
         * @return Status
         */
        final public function moveInternal( array $params ) {
+               wfProfileIn( __METHOD__ );
                $status = $this->doMoveInternal( $params );
                $this->clearCache( array( $params['src'], $params['dst'] ) );
+               wfProfileOut( __METHOD__ );
                return $status;
        }
 
@@ -739,12 +749,11 @@ abstract class FileBackend extends FileBackendBase {
        protected function doMoveInternal( array $params ) {
                // Copy source to dest
                $status = $this->copyInternal( $params );
-               if ( !$status->isOK() ) {
-                       return $status;
+               if ( $status->isOK() ) {
+                       // Delete source (only fails due to races or medium going down)
+                       $status->merge( $this->deleteInternal( array( 'src' => $params['src'] ) ) );
+                       $status->setResult( true, $status->value ); // ignore delete() errors
                }
-               // Delete source (only fails due to races or medium going down)
-               $status->merge( $this->deleteInternal( array( 'src' => $params['src'] ) ) );
-               $status->setResult( true, $status->value ); // ignore delete() errors
                return $status;
        }
 
@@ -752,17 +761,17 @@ abstract class FileBackend extends FileBackendBase {
         * @see FileBackendBase::concatenate()
         */
        final public function concatenate( array $params ) {
+               wfProfileIn( __METHOD__ );
                $status = Status::newGood();
 
                // Try to lock the source files for the scope of this function
                $scopeLockS = $this->getScopedFileLocks( $params['srcs'], LockManager::LOCK_UW, $status );
-               if ( !$status->isOK() ) {
-                       return $status; // abort
+               if ( $status->isOK() ) {
+                       // Actually do the concatenation
+                       $status->merge( $this->doConcatenate( $params ) );
                }
 
-               // Actually do the concatenation
-               $status->merge( $this->doConcatenate( $params ) );
-
+               wfProfileOut( __METHOD__ );
                return $status;
        }
 
@@ -825,12 +834,16 @@ abstract class FileBackend extends FileBackendBase {
         * @see FileBackendBase::doPrepare()
         */
        final protected function doPrepare( array $params ) {
+               wfProfileIn( __METHOD__ );
+
                $status = Status::newGood();
                list( $fullCont, $dir, $shard ) = $this->resolveStoragePath( $params['dir'] );
                if ( $dir === null ) {
                        $status->fatal( 'backend-fail-invalidpath', $params['dir'] );
+                       wfProfileOut( __METHOD__ );
                        return $status; // invalid storage path
                }
+
                if ( $shard !== null ) { // confined to a single container/shard
                        $status->merge( $this->doPrepareInternal( $fullCont, $dir, $params ) );
                } else { // directory is on several shards
@@ -840,6 +853,8 @@ abstract class FileBackend extends FileBackendBase {
                                $status->merge( $this->doPrepareInternal( "{$fullCont}{$suffix}", $dir, $params ) );
                        }
                }
+
+               wfProfileOut( __METHOD__ );
                return $status;
        }
 
@@ -854,12 +869,16 @@ abstract class FileBackend extends FileBackendBase {
         * @see FileBackendBase::doSecure()
         */
        final protected function doSecure( array $params ) {
+               wfProfileIn( __METHOD__ );
                $status = Status::newGood();
+
                list( $fullCont, $dir, $shard ) = $this->resolveStoragePath( $params['dir'] );
                if ( $dir === null ) {
                        $status->fatal( 'backend-fail-invalidpath', $params['dir'] );
+                       wfProfileOut( __METHOD__ );
                        return $status; // invalid storage path
                }
+
                if ( $shard !== null ) { // confined to a single container/shard
                        $status->merge( $this->doSecureInternal( $fullCont, $dir, $params ) );
                } else { // directory is on several shards
@@ -869,6 +888,8 @@ abstract class FileBackend extends FileBackendBase {
                                $status->merge( $this->doSecureInternal( "{$fullCont}{$suffix}", $dir, $params ) );
                        }
                }
+
+               wfProfileOut( __METHOD__ );
                return $status;
        }
 
@@ -883,18 +904,24 @@ abstract class FileBackend extends FileBackendBase {
         * @see FileBackendBase::doClean()
         */
        final protected function doClean( array $params ) {
+               wfProfileIn( __METHOD__ );
                $status = Status::newGood();
+
                list( $fullCont, $dir, $shard ) = $this->resolveStoragePath( $params['dir'] );
                if ( $dir === null ) {
                        $status->fatal( 'backend-fail-invalidpath', $params['dir'] );
+                       wfProfileOut( __METHOD__ );
                        return $status; // invalid storage path
                }
+
                // Attempt to lock this directory...
                $filesLockEx = array( $params['dir'] );
                $scopedLockE = $this->getScopedFileLocks( $filesLockEx, LockManager::LOCK_EX, $status );
                if ( !$status->isOK() ) {
+                       wfProfileOut( __METHOD__ );
                        return $status; // abort
                }
+
                if ( $shard !== null ) { // confined to a single container/shard
                        $status->merge( $this->doCleanInternal( $fullCont, $dir, $params ) );
                } else { // directory is on several shards
@@ -904,6 +931,8 @@ abstract class FileBackend extends FileBackendBase {
                                $status->merge( $this->doCleanInternal( "{$fullCont}{$suffix}", $dir, $params ) );
                        }
                }
+
+               wfProfileOut( __METHOD__ );
                return $status;
        }
 
@@ -918,47 +947,44 @@ abstract class FileBackend extends FileBackendBase {
         * @see FileBackendBase::fileExists()
         */
        final public function fileExists( array $params ) {
+               wfProfileIn( __METHOD__ );
                $stat = $this->getFileStat( $params );
-               if ( $stat === null ) {
-                       return null; // failure
-               }
-               return (bool)$stat;
+               wfProfileOut( __METHOD__ );
+               return ( $stat === null ) ? null : (bool)$stat; // null => failure
        }
 
        /**
         * @see FileBackendBase::getFileTimestamp()
         */
        final public function getFileTimestamp( array $params ) {
+               wfProfileIn( __METHOD__ );
                $stat = $this->getFileStat( $params );
-               if ( $stat ) {
-                       return $stat['mtime'];
-               } else {
-                       return false;
-               }
+               wfProfileOut( __METHOD__ );
+               return $stat ? $stat['mtime'] : false;
        }
 
        /**
         * @see FileBackendBase::getFileSize()
         */
        final public function getFileSize( array $params ) {
+               wfProfileIn( __METHOD__ );
                $stat = $this->getFileStat( $params );
-               if ( $stat ) {
-                       return $stat['size'];
-               } else {
-                       return false;
-               }
+               wfProfileOut( __METHOD__ );
+               return $stat ? $stat['size'] : false;
        }
 
        /**
         * @see FileBackendBase::getFileStat()
         */
        final public function getFileStat( array $params ) {
+               wfProfileIn( __METHOD__ );
                $path = $params['src'];
                $latest = !empty( $params['latest'] );
                if ( isset( $this->cache[$path]['stat'] ) ) {
                        // If we want the latest data, check that this cached
                        // value was in fact fetched with the latest available data.
                        if ( !$latest || $this->cache[$path]['stat']['latest'] ) {
+                               wfProfileOut( __METHOD__ );
                                return $this->cache[$path]['stat'];
                        }
                }
@@ -968,6 +994,7 @@ abstract class FileBackend extends FileBackendBase {
                        $this->cache[$path]['stat'] = $stat;
                        $this->cache[$path]['stat']['latest'] = $latest;
                }
+               wfProfileOut( __METHOD__ );
                return $stat;
        }
 
@@ -980,13 +1007,16 @@ abstract class FileBackend extends FileBackendBase {
         * @see FileBackendBase::getFileContents()
         */
        public function getFileContents( array $params ) {
+               wfProfileIn( __METHOD__ );
                $tmpFile = $this->getLocalReference( $params );
                if ( !$tmpFile ) {
+                       wfProfileOut( __METHOD__ );
                        return false;
                }
                wfSuppressWarnings();
                $data = file_get_contents( $tmpFile->getPath() );
                wfRestoreWarnings();
+               wfProfileOut( __METHOD__ );
                return $data;
        }
 
@@ -994,8 +1024,10 @@ abstract class FileBackend extends FileBackendBase {
         * @see FileBackendBase::getFileSha1Base36()
         */
        final public function getFileSha1Base36( array $params ) {
+               wfProfileIn( __METHOD__ );
                $path = $params['src'];
                if ( isset( $this->cache[$path]['sha1'] ) ) {
+                       wfProfileOut( __METHOD__ );
                        return $this->cache[$path]['sha1'];
                }
                $hash = $this->doGetFileSha1Base36( $params );
@@ -1003,6 +1035,7 @@ abstract class FileBackend extends FileBackendBase {
                        $this->trimCache(); // limit memory
                        $this->cache[$path]['sha1'] = $hash;
                }
+               wfProfileOut( __METHOD__ );
                return $hash;
        }
 
@@ -1021,21 +1054,22 @@ abstract class FileBackend extends FileBackendBase {
        /**
         * @see FileBackendBase::getFileProps()
         */
-       public function getFileProps( array $params ) {
+       final public function getFileProps( array $params ) {
+               wfProfileIn( __METHOD__ );
                $fsFile = $this->getLocalReference( $params );
-               if ( !$fsFile ) {
-                       return FSFile::placeholderProps();
-               } else {
-                       return $fsFile->getProps();
-               }
+               $props = $fsFile ? $fsFile->getProps() : FSFile::placeholderProps();
+               wfProfileOut( __METHOD__ );
+               return $props;
        }
 
        /**
         * @see FileBackendBase::getLocalReference()
         */
        public function getLocalReference( array $params ) {
+               wfProfileIn( __METHOD__ );
                $path = $params['src'];
                if ( isset( $this->cache[$path]['localRef'] ) ) {
+                       wfProfileOut( __METHOD__ );
                        return $this->cache[$path]['localRef'];
                }
                $tmpFile = $this->getLocalCopy( $params );
@@ -1043,6 +1077,7 @@ abstract class FileBackend extends FileBackendBase {
                        $this->trimCache(); // limit memory
                        $this->cache[$path]['localRef'] = $tmpFile;
                }
+               wfProfileOut( __METHOD__ );
                return $tmpFile;
        }
 
@@ -1050,6 +1085,7 @@ abstract class FileBackend extends FileBackendBase {
         * @see FileBackendBase::streamFile()
         */
        final public function streamFile( array $params ) {
+               wfProfileIn( __METHOD__ );
                $status = Status::newGood();
 
                $info = $this->getFileStat( $params );
@@ -1068,6 +1104,7 @@ abstract class FileBackend extends FileBackendBase {
                        $status->fatal( 'backend-fail-stream', $params['src'] );
                }
 
+               wfProfileOut( __METHOD__ );
                return $status;
        }
 
@@ -1169,6 +1206,7 @@ abstract class FileBackend extends FileBackendBase {
         * @see FileBackendBase::doOperationsInternal()
         */
        protected function doOperationsInternal( array $ops, array $opts ) {
+               wfProfileIn( __METHOD__ );
                $status = Status::newGood();
 
                // Build up a list of FileOps...
@@ -1190,6 +1228,7 @@ abstract class FileBackend extends FileBackendBase {
                        $scopeLockS = $this->getScopedFileLocks( $filesLockSh, LockManager::LOCK_UW, $status );
                        $scopeLockE = $this->getScopedFileLocks( $filesLockEx, LockManager::LOCK_EX, $status );
                        if ( !$status->isOK() ) {
+                               wfProfileOut( __METHOD__ );
                                return $status; // abort
                        }
                }
@@ -1204,6 +1243,7 @@ abstract class FileBackend extends FileBackendBase {
                $status->merge( $subStatus );
                $status->success = $subStatus->success; // not done in merge()
 
+               wfProfileOut( __METHOD__ );
                return $status;
        }
 

diff --git a/includes/filerepo/backend/SwiftFileBackend.php b/includes/filerepo/backend/SwiftFileBackend.php
index a9427f0..a6ddea9 100644 (file)
--- a/includes/filerepo/backend/SwiftFileBackend.php
+++ b/includes/filerepo/backend/SwiftFileBackend.php
@@ -655,7 +655,7 @@ class SwiftFileBackend extends FileBackend {
         */
        protected function getConnection() {
                if ( $this->conn === false ) {
-                       return false; // failed last attempt
+                       throw new InvalidResponseException; // failed last attempt
                }
                // Session keys expire after a while, so we renew them periodically
                if ( $this->conn && ( time() - $this->connStarted ) > $this->authTTL ) {

diff --git a/tests/phpunit/includes/filerepo/FileBackendTest.php b/tests/phpunit/includes/filerepo/FileBackendTest.php
index f491d15..546cd3c 100644 (file)
--- a/tests/phpunit/includes/filerepo/FileBackendTest.php
+++ b/tests/phpunit/includes/filerepo/FileBackendTest.php
@@ -6,21 +6,27 @@
 class FileBackendTest extends MediaWikiTestCase {
        private $backend, $multiBackend;
        private $filesToPrune;
+       private static $backendToUse;
 
        function setUp() {
                global $wgFileBackends;
                parent::setUp();
                $tmpDir = wfTempDir() . '/file-backend-test-' . time() . '-' . mt_rand();
                if ( $this->getCliArg( 'use-filebackend=' ) ) {
-                       $name = $this->getCliArg( 'use-filebackend=' );
-                       $useConfig = array();
-                       foreach ( $wgFileBackends as $conf ) {
-                               if ( $conf['name'] == $name ) {
-                                       $useConfig = $conf;
+                       if ( self::$backendToUse ) {
+                               $this->singleBackend = self::$backendToUse;
+                       } else {
+                               $name = $this->getCliArg( 'use-filebackend=' );
+                               $useConfig = array();
+                               foreach ( $wgFileBackends as $conf ) {
+                                       if ( $conf['name'] == $name ) {
+                                               $useConfig = $conf;
+                                       }
                                }
+                               $useConfig['name'] = 'localtesting'; // swap name
+                               self::$backendToUse = new $conf['class']( $useConfig );
+                               $this->singleBackend = self::$backendToUse;
                        }
-                       $useConfig['name'] = 'localtesting'; // swap name
-                       $this->singleBackend = new $conf['class']( $useConfig );
                } else {
                        $this->singleBackend = new FSFileBackend( array(
                                'name'        => 'localtesting',
@@ -88,11 +94,16 @@ class FileBackendTest extends MediaWikiTestCase {
                $this->backend->prepare( array( 'dir' => dirname( $dest ) ) );
 
                file_put_contents( $source, "Unit test file" );
+
+               if ( isset( $op['overwrite'] ) || isset( $op['overwriteSame'] ) ) {
+                       $this->backend->store( $op );
+               }
+
                $status = $this->backend->doOperation( $op );
 
                $this->assertEquals( array(), $status->errors,
                        "Store from $source to $dest succeeded without warnings ($backendName)." );
-               $this->assertEquals( true, $status->isOK(),
+               $this->assertEquals( array(), $status->errors,
                        "Store from $source to $dest succeeded ($backendName)." );
                $this->assertEquals( array( 0 => true ), $status->success,
                        "Store from $source to $dest has proper 'success' field in Status ($backendName)." );
@@ -123,14 +134,21 @@ class FileBackendTest extends MediaWikiTestCase {
                        $toPath, // dest
                );
 
-               $op['overwrite'] = true;
+               $op2 = $op;
+               $op2['overwrite'] = true;
                $cases[] = array(
-                       $op, // operation
+                       $op2, // operation
                        $tmpName, // source
                        $toPath, // dest
                );
 
-               // @TODO: test overwriteSame
+               $op2 = $op;
+               $op2['overwriteSame'] = true;
+               $cases[] = array(
+                       $op2, // operation
+                       $tmpName, // source
+                       $toPath, // dest
+               );
 
                return $cases;
        }
@@ -158,10 +176,15 @@ class FileBackendTest extends MediaWikiTestCase {
 
                $status = $this->backend->doOperation(
                        array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) );
-               $this->assertEquals( true, $status->isOK(),
+               $this->assertEquals( array(), $status->errors,
                        "Creation of file at $source succeeded ($backendName)." );
 
+               if ( isset( $op['overwrite'] ) || isset( $op['overwriteSame'] ) ) {
+                       $this->backend->copy( $op );
+               }
+
                $status = $this->backend->doOperation( $op );
+
                $this->assertEquals( array(), $status->errors,
                        "Copy from $source to $dest succeeded without warnings ($backendName)." );
                $this->assertEquals( true, $status->isOK(),
@@ -197,9 +220,18 @@ class FileBackendTest extends MediaWikiTestCase {
                        $dest, // dest
                );
 
-               $op['overwrite'] = true;
+               $op2 = $op;
+               $op2['overwrite'] = true;
                $cases[] = array(
-                       $op, // operation
+                       $op2, // operation
+                       $source, // source
+                       $dest, // dest
+               );
+
+               $op2 = $op;
+               $op2['overwriteSame'] = true;
+               $cases[] = array(
+                       $op2, // operation
                        $source, // source
                        $dest, // dest
                );
@@ -230,9 +262,13 @@ class FileBackendTest extends MediaWikiTestCase {
 
                $status = $this->backend->doOperation(
                        array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) );
-               $this->assertEquals( true, $status->isOK(),
+               $this->assertEquals( array(), $status->errors,
                        "Creation of file at $source succeeded ($backendName)." );
 
+               if ( isset( $op['overwrite'] ) || isset( $op['overwriteSame'] ) ) {
+                       $this->backend->copy( $op );
+               }
+
                $status = $this->backend->doOperation( $op );
                $this->assertEquals( array(), $status->errors,
                        "Move from $source to $dest succeeded without warnings ($backendName)." );
@@ -271,9 +307,18 @@ class FileBackendTest extends MediaWikiTestCase {
                        $dest, // dest
                );
 
-               $op['overwrite'] = true;
+               $op2 = $op;
+               $op2['overwrite'] = true;
                $cases[] = array(
-                       $op, // operation
+                       $op2, // operation
+                       $source, // source
+                       $dest, // dest
+               );
+
+               $op2 = $op;
+               $op2['overwriteSame'] = true;
+               $cases[] = array(
+                       $op2, // operation
                        $source, // source
                        $dest, // dest
                );
@@ -304,7 +349,7 @@ class FileBackendTest extends MediaWikiTestCase {
                if ( $withSource ) {
                        $status = $this->backend->doOperation(
                                array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) );
-                       $this->assertEquals( true, $status->isOK(),
+                       $this->assertEquals( array(), $status->errors,
                                "Creation of file at $source succeeded ($backendName)." );
                }
 
@@ -388,7 +433,7 @@ class FileBackendTest extends MediaWikiTestCase {
                if ( $alreadyExists ) {
                        $status = $this->backend->doOperation(
                                array( 'op' => 'create', 'content' => $oldText, 'dst' => $dest ) );
-                       $this->assertEquals( true, $status->isOK(),
+                       $this->assertEquals( array(), $status->errors,
                                "Creation of file at $dest succeeded ($backendName)." );
                }
 
@@ -452,15 +497,26 @@ class FileBackendTest extends MediaWikiTestCase {
                        strlen( $dummyText )
                );
 
-               $op['overwrite'] = true;
+               $op2 = $op;
+               $op2['overwrite'] = true;
                $cases[] = array(
-                       $op, // operation
+                       $op2, // operation
                        $source, // source
                        true, // dest already exists
                        true, // succeeds
                        strlen( $dummyText )
                );
 
+               $op2 = $op;
+               $op2['overwriteSame'] = true;
+               $cases[] = array(
+                       $op2, // operation
+                       $source, // source
+                       true, // dest already exists
+                       false, // succeeds
+                       strlen( $dummyText )
+               );
+
                return $cases;
        }
 
@@ -498,7 +554,7 @@ class FileBackendTest extends MediaWikiTestCase {
                }
                $status = $this->backend->doOperations( $ops );
 
-               $this->assertEquals( true, $status->isOK(),
+               $this->assertEquals( array(), $status->errors,
                        "Creation of source files succeeded ($backendName)." );
 
                $dest = $params['dst'];
@@ -664,7 +720,7 @@ class FileBackendTest extends MediaWikiTestCase {
 
                $status = $this->backend->doOperation(
                        array( 'op' => 'create', 'content' => $content, 'dst' => $source ) );
-               $this->assertEquals( true, $status->isOK(),
+               $this->assertEquals( array(), $status->errors,
                        "Creation of file at $source succeeded ($backendName)." );
 
                $tmpFile = $this->backend->getLocalCopy( array( 'src' => $source ) );
@@ -707,7 +763,7 @@ class FileBackendTest extends MediaWikiTestCase {
 
                $status = $this->backend->doOperation(
                        array( 'op' => 'create', 'content' => $content, 'dst' => $source ) );
-               $this->assertEquals( true, $status->isOK(),
+               $this->assertEquals( array(), $status->errors,
                        "Creation of file at $source succeeded ($backendName)." );
 
                $tmpFile = $this->backend->getLocalReference( array( 'src' => $source ) );
@@ -996,7 +1052,7 @@ class FileBackendTest extends MediaWikiTestCase {
                $iter = $backend->getFileList( array( 'dir' => "$base/$container" ) );
                if ( $iter ) {
                        foreach ( $iter as $file ) {
-                               $backend->doOperation( array( 'op' => 'delete', 'src' => "$base/$container/$file" ) );
+                               $backend->delete( array( 'src' => "$base/$container/$file", 'ignoreMissingSource' => 1 ) );
                                $tmp = $file;
                                while ( $tmp = FileBackend::parentStoragePath( $tmp ) ) {
                                        $backend->clean( array( 'dir' => $tmp ) );