From f5eaf6470bfb32f9632bee9d0ac09b57b9fc9af0 Mon Sep 17 00:00:00 2001 From: Daimona Eaytoy Date: Mon, 7 Jan 2019 11:18:39 +0100 Subject: [PATCH] externalstore: Fix insert*() return docs and remove redundant checks ExternalStorage::insertWithFallback is reported to return false on failure, but it doesn't. It has a single exit point, and return value is checked with strlen(), so actually it can only return the URL or throw. Thus, update any related doc and remove a redundant check from code calling insertToDefault. Change-Id: Ic95c3aed19118b987aef105f8077d55558f39127 --- includes/Storage/SqlBlobStore.php | 3 -- includes/externalstore/ExternalStore.php | 6 +-- includes/externalstore/ExternalStoreDB.php | 6 +++ .../externalstore/ExternalStoreMwstore.php | 39 ++++++++++--------- maintenance/migrateArchiveText.php | 3 -- 5 files changed, 30 insertions(+), 27 deletions(-) diff --git a/includes/Storage/SqlBlobStore.php b/includes/Storage/SqlBlobStore.php index 82410cc5b9..467a8ac968 100644 --- a/includes/Storage/SqlBlobStore.php +++ b/includes/Storage/SqlBlobStore.php @@ -220,9 +220,6 @@ class SqlBlobStore implements IDBAccessObject, BlobStore { if ( $this->useExternalStore ) { // Store and get the URL $data = ExternalStore::insertToDefault( $data ); - if ( !$data ) { - throw new BlobAccessException( "Failed to store text to external storage" ); - } if ( $flags ) { $flags .= ','; } diff --git a/includes/externalstore/ExternalStore.php b/includes/externalstore/ExternalStore.php index 9cf8e15d83..76f20f0ed3 100644 --- a/includes/externalstore/ExternalStore.php +++ b/includes/externalstore/ExternalStore.php @@ -159,7 +159,7 @@ class ExternalStore { * * @param string $data * @param array $params Map of ExternalStoreMedium::__construct context parameters - * @return string|bool The URL of the stored data item, or false on error + * @return string The URL of the stored data item * @throws MWException */ public static function insertToDefault( $data, array $params = [] ) { @@ -177,7 +177,7 @@ class ExternalStore { * @param array $tryStores Refer to $wgDefaultExternalStore * @param string $data * @param array $params Map of ExternalStoreMedium::__construct context parameters - * @return string|bool The URL of the stored data item, or false on error + * @return string The URL of the stored data item * @throws MWException */ public static function insertWithFallback( array $tryStores, $data, array $params = [] ) { @@ -245,7 +245,7 @@ class ExternalStore { /** * @param string $data * @param string $wiki - * @return string|bool The URL of the stored data item, or false on error + * @return string The URL of the stored data item * @throws MWException */ public static function insertToForeignDefault( $data, $wiki ) { diff --git a/includes/externalstore/ExternalStoreDB.php b/includes/externalstore/ExternalStoreDB.php index cac16b6a90..15bc3e0eb7 100644 --- a/includes/externalstore/ExternalStoreDB.php +++ b/includes/externalstore/ExternalStoreDB.php @@ -92,6 +92,9 @@ class ExternalStoreDB extends ExternalStoreMedium { return $ret; } + /** + * @inheritDoc + */ public function store( $location, $data ) { $dbw = $this->getMaster( $location ); $dbw->insert( $this->getTable( $dbw ), @@ -105,6 +108,9 @@ class ExternalStoreDB extends ExternalStoreMedium { return "DB://$location/$id"; } + /** + * @inheritDoc + */ public function isReadOnly( $location ) { $lb = $this->getLoadBalancer( $location ); $domainId = $this->getDomainId( $lb->getServerInfo( $lb->getWriterIndex() ) ); diff --git a/includes/externalstore/ExternalStoreMwstore.php b/includes/externalstore/ExternalStoreMwstore.php index 30c742d10a..7414f23e9e 100644 --- a/includes/externalstore/ExternalStoreMwstore.php +++ b/includes/externalstore/ExternalStoreMwstore.php @@ -73,29 +73,32 @@ class ExternalStoreMwstore extends ExternalStoreMedium { return $blobs; } + /** + * @inheritDoc + */ public function store( $backend, $data ) { $be = FileBackendGroup::singleton()->get( $backend ); - if ( $be instanceof FileBackend ) { - // Get three random base 36 characters to act as shard directories - $rand = Wikimedia\base_convert( mt_rand( 0, 46655 ), 10, 36, 3 ); - // Make sure ID is roughly lexicographically increasing for performance - $id = str_pad( UIDGenerator::newTimestampedUID128( 32 ), 26, '0', STR_PAD_LEFT ); - // Segregate items by wiki ID for the sake of bookkeeping - // @FIXME: this does not include the domain for b/c but it ideally should - $wiki = $this->params['wiki'] ?? wfWikiID(); + // Get three random base 36 characters to act as shard directories + $rand = Wikimedia\base_convert( mt_rand( 0, 46655 ), 10, 36, 3 ); + // Make sure ID is roughly lexicographically increasing for performance + $id = str_pad( UIDGenerator::newTimestampedUID128( 32 ), 26, '0', STR_PAD_LEFT ); + // Segregate items by wiki ID for the sake of bookkeeping + // @FIXME: this does not include the domain for b/c but it ideally should + $wiki = $this->params['wiki'] ?? wfWikiID(); - $url = $be->getContainerStoragePath( 'data' ) . '/' . rawurlencode( $wiki ); - $url .= ( $be instanceof FSFileBackend ) - ? "/{$rand[0]}/{$rand[1]}/{$rand[2]}/{$id}" // keep directories small - : "/{$rand[0]}/{$rand[1]}/{$id}"; // container sharding is only 2-levels + $url = $be->getContainerStoragePath( 'data' ) . '/' . rawurlencode( $wiki ); + $url .= ( $be instanceof FSFileBackend ) + ? "/{$rand[0]}/{$rand[1]}/{$rand[2]}/{$id}" // keep directories small + : "/{$rand[0]}/{$rand[1]}/{$id}"; // container sharding is only 2-levels - $be->prepare( [ 'dir' => dirname( $url ), 'noAccess' => 1, 'noListing' => 1 ] ); - if ( $be->create( [ 'dst' => $url, 'content' => $data ] )->isOK() ) { - return $url; - } - } + $be->prepare( [ 'dir' => dirname( $url ), 'noAccess' => 1, 'noListing' => 1 ] ); + $status = $be->create( [ 'dst' => $url, 'content' => $data ] ); - return false; + if ( $status->isOK() ) { + return $url; + } else { + throw new MWException( __METHOD__ . ": operation failed: $status" ); + } } public function isReadOnly( $backend ) { diff --git a/maintenance/migrateArchiveText.php b/maintenance/migrateArchiveText.php index b2b14cbdc4..71fff56f75 100644 --- a/maintenance/migrateArchiveText.php +++ b/maintenance/migrateArchiveText.php @@ -98,9 +98,6 @@ class MigrateArchiveText extends LoggedUpdateMaintenance { if ( $wgDefaultExternalStore ) { $data = ExternalStore::insertToDefault( $data ); - if ( !$data ) { - throw new MWException( "Unable to store text to external storage" ); - } if ( $flags ) { $flags .= ','; } -- 2.20.1