From 94c303910627b784958818322dcf70fc926dcc5b Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sun, 8 Jan 2012 09:25:15 +0000 Subject: [PATCH] * Follow-up r107170: Moved FileBackend::concatenate() outside of doOperations() as it's own separate operation. It does not mutate storage files like the others and having it in doOperations() broke FileBackendMultiWrite. This change also makes overriding doOperationsInternal() easier (suching as using a custom batch operation storage API). * Added sanity check to FileBackendMultiWrite constructor. * Moved FileBackend::create() function up a bit. --- includes/filerepo/FileRepo.php | 8 +- includes/filerepo/backend/FileBackend.php | 80 +++++++++---------- .../backend/FileBackendMultiWrite.php | 15 ++++ includes/filerepo/backend/FileOp.php | 55 ------------- .../includes/filerepo/FileBackendTest.php | 21 +++-- 5 files changed, 64 insertions(+), 115 deletions(-) diff --git a/includes/filerepo/FileRepo.php b/includes/filerepo/FileRepo.php index 4761509388..cd9b2bba4c 100644 --- a/includes/filerepo/FileRepo.php +++ b/includes/filerepo/FileRepo.php @@ -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; } diff --git a/includes/filerepo/backend/FileBackend.php b/includes/filerepo/backend/FileBackend.php index 19a6eddeeb..613447dcae 100644 --- a/includes/filerepo/backend/FileBackend.php +++ b/includes/filerepo/backend/FileBackend.php @@ -132,13 +132,7 @@ abstract class FileBackendBase { * 'src' => , * 'ignoreMissingSource' => * ) - * f) Concatenate a list of files within storage into a single temp file - * array( - * 'op' => 'concatenate', - * 'srcs' => , - * 'dst' => - * ) - * 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' ); diff --git a/includes/filerepo/backend/FileBackendMultiWrite.php b/includes/filerepo/backend/FileBackendMultiWrite.php index b1bfd81c7e..10ebd7e456 100644 --- a/includes/filerepo/backend/FileBackendMultiWrite.php +++ b/includes/filerepo/backend/FileBackendMultiWrite.php @@ -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() */ diff --git a/includes/filerepo/backend/FileOp.php b/includes/filerepo/backend/FileOp.php index a43989079c..4f6e226fd8 100644 --- a/includes/filerepo/backend/FileOp.php +++ b/includes/filerepo/backend/FileOp.php @@ -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: diff --git a/tests/phpunit/includes/filerepo/FileBackendTest.php b/tests/phpunit/includes/filerepo/FileBackendTest.php index 3f32891f5c..aef77291c1 100644 --- a/tests/phpunit/includes/filerepo/FileBackendTest.php +++ b/tests/phpunit/includes/filerepo/FileBackendTest.php @@ -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 -- 2.20.1