From 24a6e8eab64839ba5b5a6954ee35b7f2b04f9f82 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Mon, 29 Oct 2012 12:57:04 -0700 Subject: [PATCH] [FileBackend] Support "ignoreMissingSource" for copy and move operations. * This lets callers use "copy if exist" semantics more easily and avoids extra stat queries to the backend (since the cache is cleared before doOperations()). * Tweaked FileOp::fileSha1() to reduce backend stat requests as 404s are not cached. Change-Id: Icb5ca14b3316f273d53593f48979d14e113990e1 --- includes/filebackend/FSFileBackend.php | 14 ++++ includes/filebackend/FileBackend.php | 4 ++ includes/filebackend/FileBackendStore.php | 26 +++---- includes/filebackend/FileOp.php | 70 +++++++++++++------ includes/filebackend/SwiftFileBackend.php | 8 ++- .../includes/filerepo/FileBackendTest.php | 42 +++++++++++ 6 files changed, 129 insertions(+), 35 deletions(-) diff --git a/includes/filebackend/FSFileBackend.php b/includes/filebackend/FSFileBackend.php index 97abc2720c..04cf29edaf 100644 --- a/includes/filebackend/FSFileBackend.php +++ b/includes/filebackend/FSFileBackend.php @@ -250,6 +250,13 @@ class FSFileBackend extends FileBackendStore { return $status; } + if ( !is_file( $source ) ) { + if ( empty( $params['ignoreMissingSource'] ) ) { + $status->fatal( 'backend-fail-copy', $params['src'] ); + } + return $status; // do nothing; either OK or bad status + } + if ( file_exists( $dest ) ) { $ok = unlink( $dest ); if ( !$ok ) { @@ -310,6 +317,13 @@ class FSFileBackend extends FileBackendStore { return $status; } + if ( !is_file( $source ) ) { + if ( empty( $params['ignoreMissingSource'] ) ) { + $status->fatal( 'backend-fail-move', $params['src'] ); + } + return $status; // do nothing; either OK or bad status + } + if ( file_exists( $dest ) ) { // Windows does not support moving over existing files if ( wfIsWindows() ) { diff --git a/includes/filebackend/FileBackend.php b/includes/filebackend/FileBackend.php index 0ef4cd0131..e01bfc2e5b 100644 --- a/includes/filebackend/FileBackend.php +++ b/includes/filebackend/FileBackend.php @@ -206,6 +206,7 @@ abstract class FileBackend { * 'dst' => , * 'overwrite' => , * 'overwriteSame' => , + * 'ignoreMissingSource' => , # since 1.21 * 'disposition' => * ) * @endcode @@ -218,6 +219,7 @@ abstract class FileBackend { * 'dst' => , * 'overwrite' => , * 'overwriteSame' => , + * 'ignoreMissingSource' => , # since 1.21 * 'disposition' => * ) * @endcode @@ -427,6 +429,7 @@ abstract class FileBackend { * 'op' => 'copy', * 'src' => , * 'dst' => , + * 'ignoreMissingSource' => , # since 1.21 * 'disposition' => * ) * @endcode @@ -436,6 +439,7 @@ abstract class FileBackend { * 'op' => 'move', * 'src' => , * 'dst' => , + * 'ignoreMissingSource' => , # since 1.21 * 'disposition' => * ) * @endcode diff --git a/includes/filebackend/FileBackendStore.php b/includes/filebackend/FileBackendStore.php index dd0bec950b..cb8be0b2cc 100644 --- a/includes/filebackend/FileBackendStore.php +++ b/includes/filebackend/FileBackendStore.php @@ -161,12 +161,13 @@ abstract class FileBackendStore extends FileBackend { * Do not call this function from places outside FileBackend and FileOp. * * $params include: - * - src : source storage path - * - dst : destination storage path - * - disposition : Content-Disposition header value for the destination - * - async : Status will be returned immediately if supported. - * If the status is OK, then its value field will be - * set to a FileBackendStoreOpHandle object. + * - src : source storage path + * - dst : destination storage path + * - ignoreMissingSource : do nothing if the source file does not exist + * - disposition : Content-Disposition header value for the destination + * - async : Status will be returned immediately if supported. + * If the status is OK, then its value field will be + * set to a FileBackendStoreOpHandle object. * * @param $params Array * @return Status @@ -223,12 +224,13 @@ abstract class FileBackendStore extends FileBackend { * Do not call this function from places outside FileBackend and FileOp. * * $params include: - * - src : source storage path - * - dst : destination storage path - * - disposition : Content-Disposition header value for the destination - * - async : Status will be returned immediately if supported. - * If the status is OK, then its value field will be - * set to a FileBackendStoreOpHandle object. + * - src : source storage path + * - dst : destination storage path + * - ignoreMissingSource : do nothing if the source file does not exist + * - disposition : Content-Disposition header value for the destination + * - async : Status will be returned immediately if supported. + * If the status is OK, then its value field will be + * set to a FileBackendStoreOpHandle object. * * @param $params Array * @return Status diff --git a/includes/filebackend/FileOp.php b/includes/filebackend/FileOp.php index 3ac90f2b73..20dfda2a35 100644 --- a/includes/filebackend/FileOp.php +++ b/includes/filebackend/FileOp.php @@ -45,6 +45,7 @@ abstract class FileOp { protected $useLatest = true; // boolean protected $batchId; // string + protected $doOperation = true; // boolean; operation is not a no-op protected $sourceSha1; // string protected $destSameAsSource; // boolean @@ -209,6 +210,9 @@ abstract class FileOp { * @return Array */ final public function getJournalEntries( array $oPredicates, array $nPredicates ) { + if ( !$this->doOperation ) { + return array(); // this is a no-op + } $nullEntries = array(); $updateEntries = array(); $deleteEntries = array(); @@ -239,7 +243,9 @@ abstract class FileOp { } /** - * Check preconditions of the operation without writing anything + * Check preconditions of the operation without writing anything. + * This must update $predicates for each path that the op can change + * except when a failing status object is returned. * * @param $predicates Array * @return Status @@ -275,10 +281,14 @@ abstract class FileOp { return Status::newFatal( 'fileop-fail-attempt-precheck' ); } $this->state = self::STATE_ATTEMPTED; - $status = $this->doAttempt(); - if ( !$status->isOK() ) { - $this->failed = true; - $this->logFailure( 'attempt' ); + if ( $this->doOperation ) { + $status = $this->doAttempt(); + if ( !$status->isOK() ) { + $this->failed = true; + $this->logFailure( 'attempt' ); + } + } else { // no-op + $status = Status::newGood(); } return $status; } @@ -414,6 +424,8 @@ abstract class FileOp { final protected function fileSha1( $source, array $predicates ) { if ( isset( $predicates['sha1'][$source] ) ) { return $predicates['sha1'][$source]; // previous op assures this + } elseif ( isset( $predicates['exists'][$source] ) && !$predicates['exists'][$source] ) { + return false; // previous op assures this } else { $params = array( 'src' => $source, 'latest' => $this->useLatest ); return $this->backend->getFileSha1Base36( $params ); @@ -591,7 +603,7 @@ class CopyFileOp extends FileOp { */ protected function allowedParams() { return array( array( 'src', 'dst' ), - array( 'overwrite', 'overwriteSame', 'disposition' ) ); + array( 'overwrite', 'overwriteSame', 'ignoreMissingSource', 'disposition' ) ); } /** @@ -602,8 +614,16 @@ class CopyFileOp extends FileOp { $status = Status::newGood(); // Check if the source file exists if ( !$this->fileExists( $this->params['src'], $predicates ) ) { - $status->fatal( 'backend-fail-notexists', $this->params['src'] ); - return $status; + if ( $this->getParam( 'ignoreMissingSource' ) ) { + $this->doOperation = false; // no-op + // Update file existence predicates (cache 404s) + $predicates['exists'][$this->params['src']] = false; + $predicates['sha1'][$this->params['src']] = false; + return $status; // nothing to do + } else { + $status->fatal( 'backend-fail-notexists', $this->params['src'] ); + return $status; + } // Check if a file can be placed at the destination } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) { $status->fatal( 'backend-fail-usable', $this->params['dst'] ); @@ -659,7 +679,7 @@ class MoveFileOp extends FileOp { */ protected function allowedParams() { return array( array( 'src', 'dst' ), - array( 'overwrite', 'overwriteSame', 'disposition' ) ); + array( 'overwrite', 'overwriteSame', 'ignoreMissingSource', 'disposition' ) ); } /** @@ -670,8 +690,16 @@ class MoveFileOp extends FileOp { $status = Status::newGood(); // Check if the source file exists if ( !$this->fileExists( $this->params['src'], $predicates ) ) { - $status->fatal( 'backend-fail-notexists', $this->params['src'] ); - return $status; + if ( $this->getParam( 'ignoreMissingSource' ) ) { + $this->doOperation = false; // no-op + // Update file existence predicates (cache 404s) + $predicates['exists'][$this->params['src']] = false; + $predicates['sha1'][$this->params['src']] = false; + return $status; // nothing to do + } else { + $status->fatal( 'backend-fail-notexists', $this->params['src'] ); + return $status; + } // Check if a file can be placed at the destination } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) { $status->fatal( 'backend-fail-usable', $this->params['dst'] ); @@ -735,21 +763,24 @@ class DeleteFileOp extends FileOp { return array( array( 'src' ), array( 'ignoreMissingSource' ) ); } - protected $needsDelete = true; - /** - * @param array $predicates + * @param $predicates array * @return Status */ protected function doPrecheck( array &$predicates ) { $status = Status::newGood(); // Check if the source file exists if ( !$this->fileExists( $this->params['src'], $predicates ) ) { - if ( !$this->getParam( 'ignoreMissingSource' ) ) { + if ( $this->getParam( 'ignoreMissingSource' ) ) { + $this->doOperation = false; // no-op + // Update file existence predicates (cache 404s) + $predicates['exists'][$this->params['src']] = false; + $predicates['sha1'][$this->params['src']] = false; + return $status; // nothing to do + } else { $status->fatal( 'backend-fail-notexists', $this->params['src'] ); return $status; } - $this->needsDelete = false; } // Update file existence predicates $predicates['exists'][$this->params['src']] = false; @@ -761,11 +792,8 @@ class DeleteFileOp extends FileOp { * @return Status */ protected function doAttempt() { - if ( $this->needsDelete ) { - // Delete the source file - return $this->backend->deleteInternal( $this->setFlags( $this->params ) ); - } - return Status::newGood(); + // Delete the source file + return $this->backend->deleteInternal( $this->setFlags( $this->params ) ); } /** diff --git a/includes/filebackend/SwiftFileBackend.php b/includes/filebackend/SwiftFileBackend.php index 5dfad9ead5..8441f8fc8e 100644 --- a/includes/filebackend/SwiftFileBackend.php +++ b/includes/filebackend/SwiftFileBackend.php @@ -400,7 +400,9 @@ class SwiftFileBackend extends FileBackendStore { } catch ( CDNNotEnabledException $e ) { // CDN not enabled; nothing to see here } catch ( NoSuchObjectException $e ) { // source object does not exist - $status->fatal( 'backend-fail-copy', $params['src'], $params['dst'] ); + if ( empty( $params['ignoreMissingSource'] ) ) { + $status->fatal( 'backend-fail-copy', $params['src'], $params['dst'] ); + } } catch ( CloudFilesException $e ) { // some other exception? $this->handleException( $e, $status, __METHOD__, $params ); } @@ -471,7 +473,9 @@ class SwiftFileBackend extends FileBackendStore { } catch ( CDNNotEnabledException $e ) { // CDN not enabled; nothing to see here } catch ( NoSuchObjectException $e ) { // source object does not exist - $status->fatal( 'backend-fail-move', $params['src'], $params['dst'] ); + if ( empty( $params['ignoreMissingSource'] ) ) { + $status->fatal( 'backend-fail-move', $params['src'], $params['dst'] ); + } } catch ( CloudFilesException $e ) { // some other exception? $this->handleException( $e, $status, __METHOD__, $params ); } diff --git a/tests/phpunit/includes/filerepo/FileBackendTest.php b/tests/phpunit/includes/filerepo/FileBackendTest.php index 7201eec3f1..da36e90060 100644 --- a/tests/phpunit/includes/filerepo/FileBackendTest.php +++ b/tests/phpunit/includes/filerepo/FileBackendTest.php @@ -302,6 +302,19 @@ class FileBackendTest extends MediaWikiTestCase { $this->prepare( array( 'dir' => dirname( $source ) ) ); $this->prepare( array( 'dir' => dirname( $dest ) ) ); + if ( isset( $op['ignoreMissingSource'] ) ) { + $status = $this->backend->doOperation( $op ); + $this->assertGoodStatus( $status, + "Move from $source to $dest succeeded without warnings ($backendName)." ); + $this->assertEquals( array( 0 => true ), $status->success, + "Move from $source to $dest has proper 'success' field in Status ($backendName)." ); + $this->assertEquals( false, $this->backend->fileExists( array( 'src' => $source ) ), + "Source file $source does not exist ($backendName)." ); + $this->assertEquals( false, $this->backend->fileExists( array( 'src' => $dest ) ), + "Destination file $dest does not exist ($backendName)." ); + return; // done + } + $status = $this->backend->doOperation( array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) ); $this->assertGoodStatus( $status, @@ -366,6 +379,14 @@ class FileBackendTest extends MediaWikiTestCase { $dest, // dest ); + $op2 = $op; + $op2['ignoreMissingSource'] = true; + $cases[] = array( + $op2, // operation + $source, // source + $dest, // dest + ); + return $cases; } @@ -392,6 +413,19 @@ class FileBackendTest extends MediaWikiTestCase { $this->prepare( array( 'dir' => dirname( $source ) ) ); $this->prepare( array( 'dir' => dirname( $dest ) ) ); + if ( isset( $op['ignoreMissingSource'] ) ) { + $status = $this->backend->doOperation( $op ); + $this->assertGoodStatus( $status, + "Move from $source to $dest succeeded without warnings ($backendName)." ); + $this->assertEquals( array( 0 => true ), $status->success, + "Move from $source to $dest has proper 'success' field in Status ($backendName)." ); + $this->assertEquals( false, $this->backend->fileExists( array( 'src' => $source ) ), + "Source file $source does not exist ($backendName)." ); + $this->assertEquals( false, $this->backend->fileExists( array( 'src' => $dest ) ), + "Destination file $dest does not exist ($backendName)." ); + return; // done + } + $status = $this->backend->doOperation( array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) ); $this->assertGoodStatus( $status, @@ -457,6 +491,14 @@ class FileBackendTest extends MediaWikiTestCase { $dest, // dest ); + $op2 = $op; + $op2['ignoreMissingSource'] = true; + $cases[] = array( + $op2, // operation + $source, // source + $dest, // dest + ); + return $cases; } -- 2.20.1