From b80bd6159bdf946c1d4fa1e143fea221937d848a Mon Sep 17 00:00:00 2001 From: Jan Gerber Date: Wed, 24 Oct 2012 08:18:29 +0000 Subject: [PATCH] [FileBackend] Added support for changing headers on existing objects. * Added a 'describe' file operation type to doOperations()/doQuickOperations(). This can be used by scripts to fill in headers like X-Content-Duration for files that already exists. * Removed wrong comments about removing headers (they don't get removed with null). * Added some quick unit tests. Change-Id: I43c5907b59421beaa9487eefac0cdbf8bc6c6d85 --- includes/AutoLoader.php | 1 + includes/filebackend/FileBackend.php | 31 ++++++- includes/filebackend/FileBackendStore.php | 82 ++++++++++++++----- includes/filebackend/FileOp.php | 52 ++++++++++++ includes/filebackend/SwiftFileBackend.php | 39 +++++++++ languages/messages/MessagesEn.php | 1 + languages/messages/MessagesQqq.php | 4 +- maintenance/language/messages.inc | 1 + .../includes/filebackend/FileBackendTest.php | 71 +++++++++++++++- 9 files changed, 258 insertions(+), 24 deletions(-) diff --git a/includes/AutoLoader.php b/includes/AutoLoader.php index a49d901adc..8e2447d719 100644 --- a/includes/AutoLoader.php +++ b/includes/AutoLoader.php @@ -580,6 +580,7 @@ $wgAutoloadLocalClasses = array( 'MoveFileOp' => 'includes/filebackend/FileOp.php', 'DeleteFileOp' => 'includes/filebackend/FileOp.php', 'CreateFileOp' => 'includes/filebackend/FileOp.php', + 'DescribeFileOp' => 'includes/filebackend/FileOp.php', 'NullFileOp' => 'includes/filebackend/FileOp.php', # includes/filerepo diff --git a/includes/filebackend/FileBackend.php b/includes/filebackend/FileBackend.php index 25126a3cdc..ef49fe5442 100644 --- a/includes/filebackend/FileBackend.php +++ b/includes/filebackend/FileBackend.php @@ -172,6 +172,7 @@ abstract class FileBackend { * - copy * - move * - delete + * - describe (since 1.21) * - null * * a) Create a new file in storage with the contents of a string @@ -235,7 +236,17 @@ abstract class FileBackend { * ) * @endcode * - * f) Do nothing (no-op) + * f) Update metadata for a file within storage + * @code + * array( + * 'op' => 'describe', + * 'src' => , + * 'disposition' => , + * 'headers' => + * ) + * @endcode + * + * g) Do nothing (no-op) * @code * array( * 'op' => 'null', @@ -409,6 +420,7 @@ abstract class FileBackend { * - copy * - move * - delete + * - describe (since 1.21) * - null * * a) Create a new file in storage with the contents of a string @@ -421,6 +433,7 @@ abstract class FileBackend { * 'headers' => # since 1.21 * ) * @endcode + * * b) Copy a file system file into storage * @code * array( @@ -431,6 +444,7 @@ abstract class FileBackend { * 'headers' => # since 1.21 * ) * @endcode + * * c) Copy a file within storage * @code * array( @@ -441,6 +455,7 @@ abstract class FileBackend { * 'disposition' => * ) * @endcode + * * d) Move a file within storage * @code * array( @@ -451,6 +466,7 @@ abstract class FileBackend { * 'disposition' => * ) * @endcode + * * e) Delete a file within storage * @code * array( @@ -459,7 +475,18 @@ abstract class FileBackend { * 'ignoreMissingSource' => * ) * @endcode - * f) Do nothing (no-op) + * + * f) Update metadata for a file within storage + * @code + * array( + * 'op' => 'describe', + * 'src' => , + * 'disposition' => , + * 'headers' => + * ) + * @endcode + * + * g) Do nothing (no-op) * @code * array( * 'op' => 'null', diff --git a/includes/filebackend/FileBackendStore.php b/includes/filebackend/FileBackendStore.php index 2d7fa072fe..9bbcc0a375 100644 --- a/includes/filebackend/FileBackendStore.php +++ b/includes/filebackend/FileBackendStore.php @@ -90,13 +90,13 @@ abstract class FileBackendStore extends FileBackend { * Do not call this function from places outside FileBackend and FileOp. * * $params include: - * - content : the raw file contents - * - dst : destination storage path - * - disposition : Content-Disposition header value for the destination - * - headers : HTTP header name/value map - * - async : Status will be returned immediately if supported. - * If the status is OK, then its value field will be - * set to a FileBackendStoreOpHandle object. + * - content : the raw file contents + * - dst : destination storage path + * - disposition : Content-Disposition header value for the destination + * - headers : HTTP header name/value map + * - 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 @@ -119,6 +119,7 @@ abstract class FileBackendStore extends FileBackend { /** * @see FileBackendStore::createInternal() + * @return Status */ abstract protected function doCreateInternal( array $params ); @@ -128,13 +129,13 @@ abstract class FileBackendStore extends FileBackend { * Do not call this function from places outside FileBackend and FileOp. * * $params include: - * - src : source path on disk - * - dst : destination storage path - * - disposition : Content-Disposition header value for the destination - * - headers : HTTP header name/value map - * - 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 path on disk + * - dst : destination storage path + * - disposition : Content-Disposition header value for the destination + * - headers : HTTP header name/value map + * - 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 @@ -157,6 +158,7 @@ abstract class FileBackendStore extends FileBackend { /** * @see FileBackendStore::storeInternal() + * @return Status */ abstract protected function doStoreInternal( array $params ); @@ -190,6 +192,7 @@ abstract class FileBackendStore extends FileBackend { /** * @see FileBackendStore::copyInternal() + * @return Status */ abstract protected function doCopyInternal( array $params ); @@ -220,6 +223,7 @@ abstract class FileBackendStore extends FileBackend { /** * @see FileBackendStore::deleteInternal() + * @return Status */ abstract protected function doDeleteInternal( array $params ); @@ -268,6 +272,40 @@ abstract class FileBackendStore extends FileBackend { return $status; } + /** + * Alter metadata for a file at the storage path. + * Do not call this function from places outside FileBackend and FileOp. + * + * $params include: + * - src : source storage path + * - disposition : Content-Disposition header value for the destination + * - headers : HTTP header name/value map + * - 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 + */ + 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 + wfProfileOut( __METHOD__ . '-' . $this->name ); + wfProfileOut( __METHOD__ ); + return $status; + } + + /** + * @see FileBackendStore::describeInternal() + * @return Status + */ + protected function doDescribeInternal( array $params ) { + return Status::newGood(); + } + /** * No-op file operation that does nothing. * Do not call this function from places outside FileBackend and FileOp. @@ -997,12 +1035,13 @@ abstract class FileBackendStore extends FileBackend { */ final public function getOperationsInternal( array $ops ) { $supportedOps = array( - 'store' => 'StoreFileOp', - 'copy' => 'CopyFileOp', - 'move' => 'MoveFileOp', - 'delete' => 'DeleteFileOp', - 'create' => 'CreateFileOp', - 'null' => 'NullFileOp' + 'store' => 'StoreFileOp', + 'copy' => 'CopyFileOp', + 'move' => 'MoveFileOp', + 'delete' => 'DeleteFileOp', + 'create' => 'CreateFileOp', + 'describe' => 'DescribeFileOp', + 'null' => 'NullFileOp' ); $performOps = array(); // array of FileOp objects @@ -1229,6 +1268,9 @@ abstract class FileBackendStore extends FileBackend { if ( strlen( $name ) > 255 || strlen( $value ) > 255 ) { trigger_error( "Header '$name: $value' is too long." ); unset( $op['headers'][$name] ); + } elseif ( !strlen( $value ) ) { + trigger_error( "Header value for '$name' is empty." ); + unset( $op['headers'][$name] ); // ignore } } } diff --git a/includes/filebackend/FileOp.php b/includes/filebackend/FileOp.php index 60be5eea20..bfa2757f0c 100644 --- a/includes/filebackend/FileOp.php +++ b/includes/filebackend/FileOp.php @@ -809,6 +809,58 @@ class DeleteFileOp extends FileOp { } } +/** + * Change metadata for a file at the given storage path in the backend. + * Parameters for this operation are outlined in FileBackend::doOperations(). + */ +class DescribeFileOp extends FileOp { + /** + * @return array + */ + protected function allowedParams() { + return array( array( 'src' ), array( 'disposition', 'headers' ) ); + } + + /** + * @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 ) ) { + $status->fatal( 'backend-fail-notexists', $this->params['src'] ); + return $status; + // Check if a file can be placed/changed at the source + } elseif ( !$this->backend->isPathUsableInternal( $this->params['src'] ) ) { + $status->fatal( 'backend-fail-usable', $this->params['src'] ); + $status->fatal( 'backend-fail-describe', $this->params['src'] ); + return $status; + } + // Update file existence predicates + $predicates['exists'][$this->params['src']] = + $this->fileExists( $this->params['src'], $predicates ); + $predicates['sha1'][$this->params['src']] = + $this->fileSha1( $this->params['src'], $predicates ); + return $status; // safe to call attempt() + } + + /** + * @return Status + */ + protected function doAttempt() { + // Update the source file's metadata + return $this->backend->describeInternal( $this->setFlags( $this->params ) ); + } + + /** + * @return array + */ + public function storagePathsChanged() { + return array( $this->params['src'] ); + } +} + /** * Placeholder operation that has no params and does nothing */ diff --git a/includes/filebackend/SwiftFileBackend.php b/includes/filebackend/SwiftFileBackend.php index 624374b3d8..6a27e9bff4 100644 --- a/includes/filebackend/SwiftFileBackend.php +++ b/includes/filebackend/SwiftFileBackend.php @@ -562,6 +562,45 @@ class SwiftFileBackend extends FileBackendStore { } } + /** + * @see FileBackendStore::doDescribeInternal() + * @return Status + */ + protected function doDescribeInternal( array $params ) { + $status = Status::newGood(); + + list( $srcCont, $srcRel ) = $this->resolveStoragePathReal( $params['src'] ); + if ( $srcRel === null ) { + $status->fatal( 'backend-fail-invalidpath', $params['src'] ); + return $status; + } + + $hdrs = isset( $params['headers'] ) ? $params['headers'] : array(); + // Set the Content-Disposition header if requested + if ( isset( $params['disposition'] ) ) { + $hdrs['Content-Disposition'] = $this->truncDisp( $params['disposition'] ); + } + + try { + $sContObj = $this->getContainer( $srcCont ); + $srcObj = new CF_Object( $sContObj, $srcRel, false, false ); // skip HEAD + // Merge in the new metadata and header values... + $srcObj->headers = $hdrs; + $srcObj->sync_metadata(); // save to Swift + $this->purgeCDNCache( array( $srcObj ) ); + } catch ( CDNNotEnabledException $e ) { + // CDN not enabled; nothing to see here + } catch ( NoSuchContainerException $e ) { + $status->fatal( 'backend-fail-describe', $params['src'] ); + } catch ( NoSuchObjectException $e ) { + $status->fatal( 'backend-fail-describe', $params['src'] ); + } catch ( CloudFilesException $e ) { // some other exception? + $this->handleException( $e, $status, __METHOD__, $params ); + } + + return $status; + } + /** * @see FileBackendStore::doPrepareInternal() * @return Status diff --git a/languages/messages/MessagesEn.php b/languages/messages/MessagesEn.php index 5e33d62425..861b145469 100644 --- a/languages/messages/MessagesEn.php +++ b/languages/messages/MessagesEn.php @@ -2315,6 +2315,7 @@ If the problem persists, contact an [[Special:ListUsers/sysop|administrator]].', 'backend-fail-notsame' => 'A non-identical file already exists at "$1".', 'backend-fail-invalidpath' => '"$1" is not a valid storage path.', 'backend-fail-delete' => 'Could not delete file "$1".', +'backend-fail-describe' => 'Could not change metadata for file "$1".', 'backend-fail-alreadyexists' => 'The file "$1" already exists.', 'backend-fail-store' => 'Could not store file "$1" at "$2".', 'backend-fail-copy' => 'Could not copy file "$1" to "$2".', diff --git a/languages/messages/MessagesQqq.php b/languages/messages/MessagesQqq.php index 84291718d3..b6b3ecb459 100644 --- a/languages/messages/MessagesQqq.php +++ b/languages/messages/MessagesQqq.php @@ -2106,6 +2106,8 @@ Extensions making use of it: * $1 is a storage path.', 'backend-fail-delete' => 'Parameters: * $1 is a file path.', +'backend-fail-describe' => 'Parameters: +* $1 is a file path.', 'backend-fail-alreadyexists' => 'Parameters: * $1 is a filename.', 'backend-fail-store' => 'Parameters: @@ -3694,7 +3696,7 @@ Parameters: Start with a lowercase letter, unless the first word is “SVG”.', 'svg-long-desc-animated' => 'Displayed under an SVG image at the image description page if the image is animated. Non-animated images use {{msg-mw|svg-long-desc}}. * $1 is the width in pixels -* $2 is the height in pixels, and +* $2 is the height in pixels, and * $3 is the file size including a unit (for example "10 KB"). Start with a lowercase letter, unless the first word is “SVG”.', diff --git a/maintenance/language/messages.inc b/maintenance/language/messages.inc index d7a396f6d3..ddf2369024 100644 --- a/maintenance/language/messages.inc +++ b/maintenance/language/messages.inc @@ -1399,6 +1399,7 @@ $wgMessageStructure = array( 'backend-fail-notsame', 'backend-fail-invalidpath', 'backend-fail-delete', + 'backend-fail-describe', 'backend-fail-alreadyexists', 'backend-fail-store', 'backend-fail-copy', diff --git a/tests/phpunit/includes/filebackend/FileBackendTest.php b/tests/phpunit/includes/filebackend/FileBackendTest.php index add7066679..5369112964 100644 --- a/tests/phpunit/includes/filebackend/FileBackendTest.php +++ b/tests/phpunit/includes/filebackend/FileBackendTest.php @@ -585,6 +585,73 @@ class FileBackendTest extends MediaWikiTestCase { return $cases; } + /** + * @dataProvider provider_testDescribe + */ + public function testDescribe( $op, $withSource, $okStatus ) { + $this->backend = $this->singleBackend; + $this->tearDownFiles(); + $this->doTestDescribe( $op, $withSource, $okStatus ); + $this->tearDownFiles(); + + $this->backend = $this->multiBackend; + $this->tearDownFiles(); + $this->doTestDescribe( $op, $withSource, $okStatus ); + $this->tearDownFiles(); + } + + private function doTestDescribe( $op, $withSource, $okStatus ) { + $backendName = $this->backendClass(); + + $source = $op['src']; + $this->prepare( array( 'dir' => dirname( $source ) ) ); + + if ( $withSource ) { + $status = $this->backend->doOperation( + array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) ); + $this->assertGoodStatus( $status, + "Creation of file at $source succeeded ($backendName)." ); + } + + $status = $this->backend->doOperation( $op ); + if ( $okStatus ) { + $this->assertGoodStatus( $status, + "Describe of file at $source succeeded without warnings ($backendName)." ); + $this->assertEquals( true, $status->isOK(), + "Describe of file at $source succeeded ($backendName)." ); + $this->assertEquals( array( 0 => true ), $status->success, + "Describe of file at $source has proper 'success' field in Status ($backendName)." ); + } else { + $this->assertEquals( false, $status->isOK(), + "Describe of file at $source failed ($backendName)." ); + } + + $this->assertBackendPathsConsistent( array( $source ) ); + } + + public static function provider_testDescribe() { + $cases = array(); + + $source = self::baseStorePath() . '/unittest-cont1/e/myfacefile.txt'; + + $op = array( 'op' => 'describe', 'src' => $source, + 'headers' => array( 'X-Content-Length' => '91.3' ), + 'disposition' => 'inline' ); + $cases[] = array( + $op, // operation + true, // with source + true // succeeds + ); + + $cases[] = array( + $op, // operation + false, // without source + false // fails + ); + + return $cases; + } + /** * @dataProvider provider_testCreate */ @@ -1362,6 +1429,8 @@ class FileBackendTest extends MediaWikiTestCase { $this->prepare( array( 'dir' => dirname( $fileD ) ) ); $status = $this->backend->doOperations( array( + array( 'op' => 'describe', 'src' => $fileA, + 'headers' => array( 'X-Content-Length' => '91.3' ), 'disposition' => 'inline' ), array( 'op' => 'copy', 'src' => $fileA, 'dst' => $fileC, 'overwrite' => 1 ), // Now: A:, B:, C:, D: (file:) array( 'op' => 'copy', 'src' => $fileC, 'dst' => $fileA, 'overwriteSame' => 1 ), @@ -1392,7 +1461,7 @@ class FileBackendTest extends MediaWikiTestCase { $this->assertGoodStatus( $status, "Operation batch succeeded" ); $this->assertEquals( true, $status->isOK(), "Operation batch succeeded" ); - $this->assertEquals( 13, count( $status->success ), + $this->assertEquals( 14, count( $status->success ), "Operation batch has correct success array" ); $this->assertEquals( false, $this->backend->fileExists( array( 'src' => $fileA ) ), -- 2.20.1