[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

diff --git a/includes/filebackend/FSFileBackend.php b/includes/filebackend/FSFileBackend.php
index c976998..92fee83 100644 (file)
--- 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 638e7cd..2d6d2d7 100644 (file)
--- 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 1a9ac3b..be0d502 100644 (file)
--- 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 e019324..62e9fba 100644 (file)
--- 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 cacaaa3..4eda827 100644 (file)
--- 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." );
                }
        }