From c328cf6401ce100bb1ceb0c0257cd0334c281961 Mon Sep 17 00:00:00 2001 From: ASchulz Date: Wed, 20 Mar 2013 16:25:05 -0700 Subject: [PATCH] [FileBackend] Cleanup behavior for coping/moving a file over itself. * Previously, for doQuickOperations(), the default move function would just delete the file if it was moved over itself. In fact, the php-cloudfiles bindings have this same bug in its move function. * For both copy and move, when the source and destination are the same path, make sure that the headers get updated as specified. This only applies for the 'overwrite' case (not 'overwriteSame'). * Fixed bad status for doQuickOperations() when copying a file over itself using FSFileBackend. * Clarified the documentation for 'overwriteSame' option. * Renamed destSameAsSource to overwriteSameCase in FileOp for clarity. Change-Id: I3f609d9413795c654de27958b1e807b1fc785f31 --- includes/filebackend/FSFileBackend.php | 4 +- includes/filebackend/FileBackend.php | 7 +- includes/filebackend/FileBackendStore.php | 16 +++-- includes/filebackend/FileOp.php | 65 ++++++++++++------- .../includes/filebackend/FileBackendTest.php | 50 +++++++++++--- 5 files changed, 100 insertions(+), 42 deletions(-) diff --git a/includes/filebackend/FSFileBackend.php b/includes/filebackend/FSFileBackend.php index c976998929..92fee8350b 100644 --- a/includes/filebackend/FSFileBackend.php +++ b/includes/filebackend/FSFileBackend.php @@ -319,7 +319,7 @@ class FSFileBackend extends FileBackendStore { $status->value = new FSFileOpHandle( $this, $params, 'Copy', $cmd, $dest ); } else { // immediate write $this->trapWarnings(); - $ok = copy( $source, $dest ); + $ok = ( $source === $dest ) ? true : copy( $source, $dest ); $this->untrapWarnings(); // In some cases (at least over NFS), copy() returns true when it fails if ( !$ok || ( filesize( $source ) !== filesize( $dest ) ) ) { @@ -383,7 +383,7 @@ class FSFileBackend extends FileBackendStore { $status->value = new FSFileOpHandle( $this, $params, 'Move', $cmd ); } else { // immediate write $this->trapWarnings(); - $ok = rename( $source, $dest ); + $ok = ( $source === $dest ) ? true : rename( $source, $dest ); $this->untrapWarnings(); clearstatcache(); // file no longer at source if ( !$ok ) { diff --git a/includes/filebackend/FileBackend.php b/includes/filebackend/FileBackend.php index 638e7cd18f..2d6d2d77f1 100644 --- a/includes/filebackend/FileBackend.php +++ b/includes/filebackend/FileBackend.php @@ -261,9 +261,10 @@ abstract class FileBackend { * - ignoreMissingSource : The operation will simply succeed and do * nothing if the source file does not exist. * - overwrite : Any destination file will be overwritten. - * - overwriteSame : An error will not be given if a file already - * exists at the destination that has the same - * contents as the new contents to be written there. + * - overwriteSame : If a file already exists at the destination with the + * same contents, then do nothing to the destination file + * instead of giving an error. This does not compare headers. + * This option is ignored if 'overwrite' is already provided. * - headers : If supplied, the backend will return these headers when * GETs/HEADs of the destination file are made. Header values * should be smaller than 256 bytes, often options or numbers. diff --git a/includes/filebackend/FileBackendStore.php b/includes/filebackend/FileBackendStore.php index 1a9ac3b7cd..be0d502550 100644 --- a/includes/filebackend/FileBackendStore.php +++ b/includes/filebackend/FileBackendStore.php @@ -276,10 +276,12 @@ abstract class FileBackendStore extends FileBackend { */ protected function doMoveInternal( array $params ) { unset( $params['async'] ); // two steps, won't work here :) + $nsrc = FileBackend::normalizeStoragePath( $params['src'] ); + $ndst = Filebackend::normalizeStoragePath( $params['dst'] ); // Copy source to dest $status = $this->copyInternal( $params ); - if ( $status->isOK() ) { - // Delete source (only fails due to races or medium going down) + if ( $nsrc !== $ndst && $status->isOK() ) { + // Delete source (only fails due to races or network problems) $status->merge( $this->deleteInternal( array( 'src' => $params['src'] ) ) ); $status->setResult( true, $status->value ); // ignore delete() errors } @@ -303,9 +305,13 @@ abstract class FileBackendStore extends FileBackend { final public function describeInternal( array $params ) { wfProfileIn( __METHOD__ ); wfProfileIn( __METHOD__ . '-' . $this->name ); - $status = $this->doDescribeInternal( $params ); - $this->clearCache( array( $params['src'] ) ); - $this->deleteFileCache( $params['src'] ); // persistent cache + if ( count( $params['headers'] ) ) { + $status = $this->doDescribeInternal( $params ); + $this->clearCache( array( $params['src'] ) ); + $this->deleteFileCache( $params['src'] ); // persistent cache + } else { + $status = Status::newGood(); // nothing to do + } wfProfileOut( __METHOD__ . '-' . $this->name ); wfProfileOut( __METHOD__ ); return $status; diff --git a/includes/filebackend/FileOp.php b/includes/filebackend/FileOp.php index e019324df8..62e9fbada6 100644 --- a/includes/filebackend/FileOp.php +++ b/includes/filebackend/FileOp.php @@ -46,7 +46,7 @@ abstract class FileOp { protected $doOperation = true; // boolean; operation is not a no-op protected $sourceSha1; // string - protected $destSameAsSource; // boolean + protected $overwriteSameCase; // boolean protected $destExists; // boolean /* Object life-cycle */ @@ -55,7 +55,7 @@ abstract class FileOp { const STATE_ATTEMPTED = 3; /** - * Build a new file operation transaction + * Build a new batch file operation transaction * * @param $backend FileBackendStore * @param $params Array @@ -64,8 +64,10 @@ abstract class FileOp { final public function __construct( FileBackendStore $backend, array $params ) { $this->backend = $backend; list( $required, $optional ) = $this->allowedParams(); + // @TODO: normalizeAnyStoragePaths() calls are overzealous, use a parameter list foreach ( $required as $name ) { if ( isset( $params[$name] ) ) { + // Normalize paths so the paths to the same file have the same string $this->params[$name] = self::normalizeAnyStoragePaths( $params[$name] ); } else { throw new MWException( "File operation missing parameter '$name'." ); @@ -73,6 +75,7 @@ abstract class FileOp { } foreach ( $optional as $name ) { if ( isset( $params[$name] ) ) { + // Normalize paths so the paths to the same file have the same string $this->params[$name] = self::normalizeAnyStoragePaths( $params[$name] ); } } @@ -341,7 +344,7 @@ abstract class FileOp { /** * Check for errors with regards to the destination file already existing. - * Also set the destExists, destSameAsSource and sourceSha1 member variables. + * Also set the destExists, overwriteSameCase and sourceSha1 member variables. * A bad status will be returned if there is no chance it can be overwritten. * * @param $predicates Array @@ -354,7 +357,7 @@ abstract class FileOp { if ( $this->sourceSha1 === null ) { // file in storage? $this->sourceSha1 = $this->fileSha1( $this->params['src'], $predicates ); } - $this->destSameAsSource = false; + $this->overwriteSameCase = false; $this->destExists = $this->fileExists( $this->params['dst'], $predicates ); if ( $this->destExists ) { if ( $this->getParam( 'overwrite' ) ) { @@ -368,7 +371,7 @@ abstract class FileOp { // Give an error if the files are not identical $status->fatal( 'backend-fail-notsame', $this->params['dst'] ); } else { - $this->destSameAsSource = true; // OK + $this->overwriteSameCase = true; // OK } return $status; // do nothing; either OK or bad status } else { @@ -489,7 +492,7 @@ class CreateFileOp extends FileOp { * @return Status */ protected function doAttempt() { - if ( !$this->destSameAsSource ) { + if ( !$this->overwriteSameCase ) { // Create the file at the destination return $this->backend->createInternal( $this->setFlags( $this->params ) ); } @@ -561,8 +564,8 @@ class StoreFileOp extends FileOp { * @return Status */ protected function doAttempt() { - // Store the file at the destination - if ( !$this->destSameAsSource ) { + if ( !$this->overwriteSameCase ) { + // Store the file at the destination return $this->backend->storeInternal( $this->setFlags( $this->params ) ); } return Status::newGood(); @@ -638,14 +641,19 @@ class CopyFileOp extends FileOp { * @return Status */ protected function doAttempt() { - // Do nothing if the src/dst paths are the same - if ( $this->params['src'] !== $this->params['dst'] ) { - // Copy the file into the destination - if ( !$this->destSameAsSource ) { - return $this->backend->copyInternal( $this->setFlags( $this->params ) ); - } + if ( $this->overwriteSameCase ) { + $status = Status::newGood(); // nothing to do + } elseif ( $this->params['src'] === $this->params['dst'] ) { + // Just update the destination file headers + $headers = $this->getParam( 'headers' ) ?: array(); + $status = $this->backend->describeInternal( $this->setFlags( array( + 'src' => $this->params['dst'], 'headers' => $headers + ) ) ); + } else { + // Copy the file to the destination + $status = $this->backend->copyInternal( $this->setFlags( $this->params ) ); } - return Status::newGood(); + return $status; } /** @@ -717,18 +725,27 @@ class MoveFileOp extends FileOp { * @return Status */ protected function doAttempt() { - // Do nothing if the src/dst paths are the same - if ( $this->params['src'] !== $this->params['dst'] ) { - if ( !$this->destSameAsSource ) { - // Move the file into the destination - return $this->backend->moveInternal( $this->setFlags( $this->params ) ); + if ( $this->overwriteSameCase ) { + if ( $this->params['src'] === $this->params['dst'] ) { + // Do nothing to the destination (which is also the source) + $status = Status::newGood(); } else { - // Just delete source as the destination needs no changes - $params = array( 'src' => $this->params['src'] ); - return $this->backend->deleteInternal( $this->setFlags( $params ) ); + // Just delete the source as the destination file needs no changes + $status = $this->backend->deleteInternal( $this->setFlags( + array( 'src' => $this->params['src'] ) + ) ); } + } elseif ( $this->params['src'] === $this->params['dst'] ) { + // Just update the destination file headers + $headers = $this->getParam( 'headers' ) ?: array(); + $status = $this->backend->describeInternal( $this->setFlags( + array( 'src' => $this->params['dst'], 'headers' => $headers ) + ) ); + } else { + // Move the file to the destination + $status = $this->backend->moveInternal( $this->setFlags( $this->params ) ); } - return Status::newGood(); + return $status; } /** diff --git a/tests/phpunit/includes/filebackend/FileBackendTest.php b/tests/phpunit/includes/filebackend/FileBackendTest.php index cacaaa3474..4eda827006 100644 --- a/tests/phpunit/includes/filebackend/FileBackendTest.php +++ b/tests/phpunit/includes/filebackend/FileBackendTest.php @@ -817,32 +817,66 @@ class FileBackendTest extends MediaWikiTestCase { "$base/unittest-cont1/e/fileB.a", "$base/unittest-cont1/e/fileC.a" ); - $ops = array(); + $createOps = array(); $purgeOps = array(); foreach ( $files as $path ) { $status = $this->prepare( array( 'dir' => dirname( $path ) ) ); $this->assertGoodStatus( $status, "Preparing $path succeeded without warnings ($backendName)." ); - $ops[] = array( 'op' => 'create', 'dst' => $path, 'content' => mt_rand(0, 50000) ); + $createOps[] = array( 'op' => 'create', 'dst' => $path, 'content' => mt_rand(0, 50000) ); + $copyOps[] = array( 'op' => 'copy', 'src' => $path, 'dst' => "$path-2" ); + $moveOps[] = array( 'op' => 'move', 'src' => "$path-2", 'dst' => "$path-3" ); $purgeOps[] = array( 'op' => 'delete', 'src' => $path ); + $purgeOps[] = array( 'op' => 'delete', 'src' => "$path-3" ); } $purgeOps[] = array( 'op' => 'null' ); - $status = $this->backend->doQuickOperations( $ops ); - $this->assertGoodStatus( $status, - "Creation of source files succeeded ($backendName)." ); + $this->assertGoodStatus( + $this->backend->doQuickOperations( $createOps ), + "Creation of source files succeeded ($backendName)." ); foreach ( $files as $file ) { $this->assertTrue( $this->backend->fileExists( array( 'src' => $file ) ), "File $file exists." ); } - $status = $this->backend->doQuickOperations( $purgeOps ); - $this->assertGoodStatus( $status, - "Quick deletion of source files succeeded ($backendName)." ); + $this->assertGoodStatus( + $this->backend->doQuickOperations( $copyOps ), + "Quick copy of source files succeeded ($backendName)." ); + foreach ( $files as $file ) { + $this->assertTrue( $this->backend->fileExists( array( 'src' => "$file-2" ) ), + "File $file-2 exists." ); + } + + $this->assertGoodStatus( + $this->backend->doQuickOperations( $moveOps ), + "Quick move of source files succeeded ($backendName)." ); + foreach ( $files as $file ) { + $this->assertTrue( $this->backend->fileExists( array( 'src' => "$file-3" ) ), + "File $file-3 move in." ); + $this->assertFalse( $this->backend->fileExists( array( 'src' => "$file-2" ) ), + "File $file-2 moved away." ); + } + $this->assertGoodStatus( + $this->backend->quickCopy( array( 'src' => $files[0], 'dst' => $files[0] ) ), + "Copy of file {$files[0]} over itself succeeded ($backendName)." ); + $this->assertTrue( $this->backend->fileExists( array( 'src' => $files[0] ) ), + "File {$files[0]} still exists." ); + + $this->assertGoodStatus( + $this->backend->quickMove( array( 'src' => $files[0], 'dst' => $files[0] ) ), + "Move of file {$files[0]} over itself succeeded ($backendName)." ); + $this->assertTrue( $this->backend->fileExists( array( 'src' => $files[0] ) ), + "File {$files[0]} still exists." ); + + $this->assertGoodStatus( + $this->backend->doQuickOperations( $purgeOps ), + "Quick deletion of source files succeeded ($backendName)." ); foreach ( $files as $file ) { $this->assertFalse( $this->backend->fileExists( array( 'src' => $file ) ), "File $file purged." ); + $this->assertFalse( $this->backend->fileExists( array( 'src' => "$file-3" ) ), + "File $file-3 purged." ); } } -- 2.20.1