* Follow-up r107170: Moved FileBackend::concatenate() outside of doOperations() as...
authorAaron Schulz <aaron@users.mediawiki.org>
Sun, 8 Jan 2012 09:25:15 +0000 (09:25 +0000)
committerAaron Schulz <aaron@users.mediawiki.org>
Sun, 8 Jan 2012 09:25:15 +0000 (09:25 +0000)
* Added sanity check to FileBackendMultiWrite constructor.
* Moved FileBackend::create() function up a bit.

includes/filerepo/FileRepo.php
includes/filerepo/backend/FileBackend.php
includes/filerepo/backend/FileBackendMultiWrite.php
includes/filerepo/backend/FileOp.php
tests/phpunit/includes/filerepo/FileBackendTest.php

index 4761509..cd9b2bb 100644 (file)
@@ -791,8 +791,6 @@ class FileRepo {
         */
        function concatenate( $srcPaths, $dstPath, $flags = 0 ) {
                $status = $this->newGood();
-               // Resolve target to a storage path if virtual
-               $dest = $this->resolveToStoragePath( $dstPath );
 
                $sources = array();
                $deleteOperations = array(); // post-concatenate ops
@@ -805,9 +803,9 @@ class FileRepo {
                        }
                }
 
-               // Concatenate the chunks into one file
-               $op = array( 'op' => 'concatenate', 'srcs' => $sources, 'dst' => $dest );
-               $status->merge( $this->backend->doOperation( $op ) );
+               // Concatenate the chunks into one FS file
+               $params = array( 'srcs' => $sources, 'dst' => $dstPath );
+               $status->merge( $this->backend->concatenate( $params ) );
                if ( !$status->isOK() ) {
                        return $status;
                }
index 19a6edd..613447d 100644 (file)
@@ -132,13 +132,7 @@ abstract class FileBackendBase {
         *         'src'                 => <storage path>,
         *         'ignoreMissingSource' => <boolean>
         *     )
-        * f) Concatenate a list of files within storage into a single temp file
-        *     array(
-        *         'op'                  => 'concatenate',
-        *         'srcs'                => <ordered array of storage paths>,
-        *         'dst'                 => <file system path to 0-byte temp file>
-        *     )
-        * g) Do nothing (no-op)
+        * f) Do nothing (no-op)
         *     array(
         *         'op'                  => 'null',
         *     )
@@ -202,6 +196,21 @@ abstract class FileBackendBase {
                return $this->doOperations( array( $op ), $opts );
        }
 
+       /**
+        * Performs a single create operation.
+        * This sets $params['op'] to 'create' and passes it to doOperation().
+        *
+        * @see FileBackendBase::doOperation()
+        *
+        * @param $params Array Operation parameters
+        * @param $opts Array Operation options
+        * @return Status
+        */
+       final public function create( array $params, array $opts = array() ) {
+               $params['op'] = 'create';
+               return $this->doOperation( $params, $opts );
+       }
+
        /**
         * Performs a single store operation.
         * This sets $params['op'] to 'store' and passes it to doOperation().
@@ -263,34 +272,15 @@ abstract class FileBackendBase {
        }
 
        /**
-        * Performs a single create operation.
-        * This sets $params['op'] to 'create' and passes it to doOperation().
-        *
-        * @see FileBackendBase::doOperation()
-        *
-        * @param $params Array Operation parameters
-        * @param $opts Array Operation options
-        * @return Status
-        */
-       final public function create( array $params, array $opts = array() ) {
-               $params['op'] = 'create';
-               return $this->doOperation( $params, $opts );
-       }
-
-       /**
-        * Performs a single concatenate operation.
-        * This sets $params['op'] to 'concatenate' and passes it to doOperation().
-        *
-        * @see FileBackendBase::doOperation()
+        * Concatenate a list of storage files into a single file on the file system
+        * $params include:
+        *     srcs          : ordered source storage paths (e.g. chunk1, chunk2, ...)
+        *     dst           : file system path to 0-byte temp file
         *
         * @param $params Array Operation parameters
-        * @param $opts Array Operation options
         * @return Status
         */
-       final public function concatenate( array $params, array $opts = array() ) {
-               $params['op'] = 'concatenate';
-               return $this->doOperation( $params, $opts );
-       }
+       abstract public function concatenate( array $params );
 
        /**
         * Prepare a storage path for usage. This will create containers
@@ -677,24 +667,27 @@ abstract class FileBackend extends FileBackendBase {
        }
 
        /**
-        * Combines files from several storage paths into a new file in the backend.
-        * Do not call this function from places outside FileBackend and FileOp.
-        * $params include:
-        *     srcs          : ordered source storage paths (e.g. chunk1, chunk2, ...)
-        *     dst           : file system path to 0-byte temp file
-        * 
-        * @param $params Array
-        * @return Status
+        * @see FileBackendBase::concatenate()
         */
-       final public function concatenateInternal( array $params ) {
-               $status = $this->doConcatenateInternal( $params );
+       final public function concatenate( array $params ) {
+               $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
+               }
+
+               // Actually do the concatenation
+               $status->merge( $this->doConcatenate( $params ) );
+
                return $status;
        }
 
        /**
-        * @see FileBackend::concatenateInternal()
+        * @see FileBackend::concatenate()
         */
-       protected function doConcatenateInternal( array $params ) {
+       protected function doConcatenate( array $params ) {
                $status = Status::newGood();
                $tmpPath = $params['dst']; // convenience
 
@@ -1035,7 +1028,6 @@ abstract class FileBackend extends FileBackendBase {
                        'copy'        => 'CopyFileOp',
                        'move'        => 'MoveFileOp',
                        'delete'      => 'DeleteFileOp',
-                       'concatenate' => 'ConcatenateFileOp',
                        'create'      => 'CreateFileOp',
                        'null'        => 'NullFileOp'
                );
index b1bfd81..10ebd7e 100644 (file)
@@ -36,9 +36,15 @@ class FileBackendMultiWrite extends FileBackendBase {
         */
        public function __construct( array $config ) {
                parent::__construct( $config );
+               $namesUsed = array();
                // Construct backends here rather than via registration
                // to keep these backends hidden from outside the proxy.
                foreach ( $config['backends'] as $index => $config ) {
+                       $name = $config['name'];
+                       if ( isset( $namesUsed[$name] ) ) { // don't break FileOp predicates
+                               throw new MWException( "Two or more backends defined with the name $name." );
+                       }
+                       $namesUsed[$name] = 1;
                        if ( !isset( $config['class'] ) ) {
                                throw new MWException( 'No class given for a backend config.' );
                        }
@@ -244,6 +250,15 @@ class FileBackendMultiWrite extends FileBackendBase {
                return $status;
        }
 
+       /**
+        * @see FileBackendBase::getFileList()
+        */
+       public function concatenate( array $params ) {
+               // We are writing to an FS file, so we don't need to do this per-backend
+               $realParams = $this->substOpPaths( $params, $this->backends[$this->masterIndex] );
+               return $this->backends[$this->masterIndex]->concatenate( $realParams );
+       }
+
        /**
         * @see FileBackendBase::fileExists()
         */
index a439890..4f6e226 100644 (file)
@@ -807,61 +807,6 @@ class MoveFileOp extends FileOp {
        }
 }
 
-/**
- * Combines files from severals storage paths into a new file in the backend.
- * Parameters similar to FileBackend::concatenate(), which include:
- *     srcs          : ordered source storage paths (e.g. chunk1, chunk2, ...)
- *     dst           : destination file system path to 0-byte temp file
- */
-class ConcatenateFileOp extends FileOp {
-       protected function allowedParams() {
-               return array( 'srcs', 'dst' );
-       }
-
-       protected function doPrecheck( array &$predicates ) {
-               $status = Status::newGood();
-               // Check destination temp file
-               wfSuppressWarnings();
-               $ok = ( is_file( $this->params['dst'] ) && !filesize( $this->params['dst'] ) );
-               wfRestoreWarnings();
-               if ( !$ok ) { // not present or not empty
-                       $status->fatal( 'backend-fail-opentemp', $this->params['dst'] );
-                       return $status;
-               }
-               // Check that source files exists
-               foreach ( $this->params['srcs'] as $source ) {
-                       if ( !$this->fileExists( $source, $predicates ) ) {
-                               $status->fatal( 'backend-fail-notexists', $source );
-                               return $status;
-                       }
-               }
-               return $status;
-       }
-
-       protected function doAttempt() {
-               $status = Status::newGood();
-               // Concatenate the file at the destination
-               $status->merge( $this->backend->concatenateInternal( $this->params ) );
-               return $status;
-       }
-
-       protected function doRevert() {
-               $status = Status::newGood();
-               // Clear out the temp file back to 0-bytes
-               wfSuppressWarnings();
-               $ok = file_put_contents( $this->params['dst'], '' );
-               wfRestoreWarnings();
-               if ( !$ok ) {
-                       $status->fatal( 'backend-fail-writetemp', $this->params['dst'] );
-               }
-               return $status;
-       }
-
-       public function storagePathsRead() {
-               return $this->params['srcs'];
-       }
-}
-
 /**
  * Delete a file at the storage path.
  * Parameters similar to FileBackend::delete(), which include:
index 3f32891..aef7729 100644 (file)
@@ -437,13 +437,12 @@ class FileBackendTest extends MediaWikiTestCase {
                $this->doTestConcatenate( $op, $srcs, $srcsContent, $alreadyExists, $okStatus );
                $this->tearDownFiles();
 
-               # FIXME
-               #$this->backend = $this->multiBackend;
-               #$this->doTestConcatenate( $op, $srcs, $srcsContent, $alreadyExists, $okStatus );
-               #$this->tearDownFiles();
+               $this->backend = $this->multiBackend;
+               $this->doTestConcatenate( $op, $srcs, $srcsContent, $alreadyExists, $okStatus );
+               $this->tearDownFiles();
        }
 
-       public function doTestConcatenate( $op, $srcs, $srcsContent, $alreadyExists, $okStatus ) {
+       public function doTestConcatenate( $params, $srcs, $srcsContent, $alreadyExists, $okStatus ) {
                $backendName = $this->backendClass();
 
                $expContent = '';
@@ -462,7 +461,7 @@ class FileBackendTest extends MediaWikiTestCase {
                $this->assertEquals( true, $status->isOK(),
                        "Creation of source files succeeded ($backendName)." );
 
-               $dest = $op['dst'];
+               $dest = $params['dst'];
                if ( $alreadyExists ) {
                        $ok = file_put_contents( $dest, 'blah...blah...waahwaah' ) !== false;
                        $this->assertEquals( true, $ok,
@@ -473,8 +472,8 @@ class FileBackendTest extends MediaWikiTestCase {
                                "Creation of 0-byte file at $dest succeeded ($backendName)." );
                }
 
-               // Combine them
-               $status = $this->backend->doOperation( $op );
+               // Combine the files into one
+               $status = $this->backend->concatenate( $params );
                if ( $okStatus ) {
                        $this->assertEquals( array(), $status->errors,
                                "Creation of concat file at $dest succeeded without warnings ($backendName)." );
@@ -534,10 +533,10 @@ class FileBackendTest extends MediaWikiTestCase {
                        'lkaem;a',
                        'legma'
                );
-               $op = array( 'op' => 'concatenate', 'srcs' => $srcs, 'dst' => $dest );
+               $params = array( 'srcs' => $srcs, 'dst' => $dest );
 
                $cases[] = array(
-                       $op, // operation
+                       $params, // operation
                        $srcs, // sources
                        $content, // content for each source
                        false, // no dest already exists
@@ -545,7 +544,7 @@ class FileBackendTest extends MediaWikiTestCase {
                );
 
                $cases[] = array(
-                       $op, // operation
+                       $params, // operation
                        $srcs, // sources
                        $content, // content for each source
                        true, // dest already exists