From 903f1810c8d1fafe27c10636b4c2fc982b89c48e Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 30 Aug 2019 00:01:29 -0700 Subject: [PATCH] filebackend: improve internal use of FileBackend constants Add more result constants and split up FileBackend::UNKNOWN for clarity. This follows up 5719815f3b, which added that constant. Make internal FileBackendStore::doGet* methods distinguish I/O errors from missing files; the return types of public FileBackend methods are unchanged. Avoid process caching any mtime/size/sha1 values in the case of I/O errors. Use error constants consistently for stat methods when given invalid paths. Also: * Factor out FileBackendStore::processCacheAndPersistStatEntries() method to reduce significant code duplication. * Consolidate duplicated isPathUsable() checks in FileOp subclasses to FileOp::precheck(). * Remove null process cache value check from FileBackend::getFileStat() as null values are never stored in the process cache to begin with. * Reformat some oddly wrapped lines to look cleaner. Change-Id: Id0e4b0da0bb2ed3184847b35142d587c7f3d953d --- includes/libs/filebackend/FSFileBackend.php | 124 +++-- includes/libs/filebackend/FileBackend.php | 153 ++++-- .../filebackend/FileBackendMultiWrite.php | 33 +- .../libs/filebackend/FileBackendStore.php | 395 +++++++++------ .../libs/filebackend/MemoryFileBackend.php | 14 +- .../libs/filebackend/SwiftFileBackend.php | 174 +++---- .../libs/filebackend/fileop/CopyFileOp.php | 12 +- .../libs/filebackend/fileop/CreateFileOp.php | 24 +- .../libs/filebackend/fileop/DeleteFileOp.php | 12 +- .../filebackend/fileop/DescribeFileOp.php | 15 +- includes/libs/filebackend/fileop/FileOp.php | 26 +- .../libs/filebackend/fileop/MoveFileOp.php | 14 +- .../libs/filebackend/fileop/StoreFileOp.php | 29 +- .../includes/filebackend/FileBackendTest.php | 458 ++++++++++-------- 14 files changed, 885 insertions(+), 598 deletions(-) diff --git a/includes/libs/filebackend/FSFileBackend.php b/includes/libs/filebackend/FSFileBackend.php index c1a796f75a..c333a5e221 100644 --- a/includes/libs/filebackend/FSFileBackend.php +++ b/includes/libs/filebackend/FSFileBackend.php @@ -137,7 +137,7 @@ class FSFileBackend extends FileBackendStore { } } - return null; + return null; // invalid } /** @@ -576,25 +576,23 @@ class FSFileBackend extends FileBackendStore { protected function doGetFileStat( array $params ) { $source = $this->resolveToFSPath( $params['src'] ); if ( $source === null ) { - return false; // invalid storage path + return self::$RES_ERROR; // invalid storage path } $this->trapWarnings(); // don't trust 'false' if there were errors $stat = is_file( $source ) ? stat( $source ) : false; // regular files only $hadError = $this->untrapWarnings(); - if ( $stat ) { + if ( is_array( $stat ) ) { $ct = new ConvertibleTimestamp( $stat['mtime'] ); return [ 'mtime' => $ct->getTimestamp( TS_MW ), 'size' => $stat['size'] ]; - } elseif ( !$hadError ) { - return false; // file does not exist - } else { - return self::UNKNOWN; // failure } + + return $hadError ? self::$RES_ERROR : self::$RES_ABSENT; } protected function doClearCache( array $paths = null ) { @@ -610,7 +608,7 @@ class FSFileBackend extends FileBackendStore { $exists = is_dir( $dir ); $hadError = $this->untrapWarnings(); - return $hadError ? self::UNKNOWN : $exists; + return $hadError ? self::$RES_ERROR : $exists; } /** @@ -624,18 +622,27 @@ class FSFileBackend extends FileBackendStore { list( , $shortCont, ) = FileBackend::splitStoragePath( $params['dir'] ); $contRoot = $this->containerFSRoot( $shortCont, $fullCont ); // must be valid $dir = ( $dirRel != '' ) ? "{$contRoot}/{$dirRel}" : $contRoot; + + $this->trapWarnings(); // don't trust 'false' if there were errors $exists = is_dir( $dir ); - if ( !$exists ) { - $this->logger->warning( __METHOD__ . "() given directory does not exist: '$dir'\n" ); + $isReadable = $exists ? is_readable( $dir ) : false; + $hadError = $this->untrapWarnings(); - return []; // nothing under this dir - } elseif ( !is_readable( $dir ) ) { - $this->logger->warning( __METHOD__ . "() given directory is unreadable: '$dir'\n" ); + if ( $isReadable ) { + return new FSFileBackendDirList( $dir, $params ); + } elseif ( $exists ) { + $this->logger->warning( __METHOD__ . ": given directory is unreadable: '$dir'" ); - return self::UNKNOWN; // bad permissions? - } + return self::$RES_ERROR; // bad permissions? + } elseif ( $hadError ) { + $this->logger->warning( __METHOD__ . ": given directory was unreachable: '$dir'" ); + + return self::$RES_ERROR; + } else { + $this->logger->info( __METHOD__ . ": given directory does not exist: '$dir'" ); - return new FSFileBackendDirList( $dir, $params ); + return []; // nothing under this dir + } } /** @@ -649,18 +656,27 @@ class FSFileBackend extends FileBackendStore { list( , $shortCont, ) = FileBackend::splitStoragePath( $params['dir'] ); $contRoot = $this->containerFSRoot( $shortCont, $fullCont ); // must be valid $dir = ( $dirRel != '' ) ? "{$contRoot}/{$dirRel}" : $contRoot; + + $this->trapWarnings(); // don't trust 'false' if there were errors $exists = is_dir( $dir ); - if ( !$exists ) { - $this->logger->warning( __METHOD__ . "() given directory does not exist: '$dir'\n" ); + $isReadable = $exists ? is_readable( $dir ) : false; + $hadError = $this->untrapWarnings(); - return []; // nothing under this dir - } elseif ( !is_readable( $dir ) ) { - $this->logger->warning( __METHOD__ . "() given directory is unreadable: '$dir'\n" ); + if ( $exists && $isReadable ) { + return new FSFileBackendFileList( $dir, $params ); + } elseif ( $exists ) { + $this->logger->warning( __METHOD__ . ": given directory is unreadable: '$dir'\n" ); - return self::UNKNOWN; // bad permissions? - } + return self::$RES_ERROR; // bad permissions? + } elseif ( $hadError ) { + $this->logger->warning( __METHOD__ . ": given directory was unreachable: '$dir'\n" ); - return new FSFileBackendFileList( $dir, $params ); + return self::$RES_ERROR; + } else { + $this->logger->info( __METHOD__ . ": given directory does not exist: '$dir'\n" ); + + return []; // nothing under this dir + } } protected function doGetLocalReferenceMulti( array $params ) { @@ -668,10 +684,21 @@ class FSFileBackend extends FileBackendStore { foreach ( $params['srcs'] as $src ) { $source = $this->resolveToFSPath( $src ); - if ( $source === null || !is_file( $source ) ) { - $fsFiles[$src] = null; // invalid path or file does not exist - } else { + if ( $source === null ) { + $fsFiles[$src] = self::$RES_ERROR; // invalid path + continue; + } + + $this->trapWarnings(); // don't trust 'false' if there were errors + $isFile = is_file( $source ); // regular files only + $hadError = $this->untrapWarnings(); + + if ( $isFile ) { $fsFiles[$src] = new FSFile( $source ); + } elseif ( $hadError ) { + $fsFiles[$src] = self::$RES_ERROR; + } else { + $fsFiles[$src] = self::$RES_ABSENT; } } @@ -684,26 +711,31 @@ class FSFileBackend extends FileBackendStore { foreach ( $params['srcs'] as $src ) { $source = $this->resolveToFSPath( $src ); if ( $source === null ) { - $tmpFiles[$src] = null; // invalid path + $tmpFiles[$src] = self::$RES_ERROR; // invalid path + continue; + } + // Create a new temporary file with the same extension... + $ext = FileBackend::extensionFromPath( $src ); + $tmpFile = $this->tmpFileFactory->newTempFSFile( 'localcopy_', $ext ); + if ( !$tmpFile ) { + $tmpFiles[$src] = self::$RES_ERROR; + continue; + } + + $tmpPath = $tmpFile->getPath(); + // Copy the source file over the temp file + $this->trapWarnings(); + $isFile = is_file( $source ); // regular files only + $copySuccess = $isFile ? copy( $source, $tmpPath ) : false; + $hadError = $this->untrapWarnings(); + + if ( $copySuccess ) { + $this->chmod( $tmpPath ); + $tmpFiles[$src] = $tmpFile; + } elseif ( $hadError ) { + $tmpFiles[$src] = self::$RES_ERROR; // copy failed } else { - // Create a new temporary file with the same extension... - $ext = FileBackend::extensionFromPath( $src ); - $tmpFile = $this->tmpFileFactory->newTempFSFile( 'localcopy_', $ext ); - if ( !$tmpFile ) { - $tmpFiles[$src] = null; - } else { - $tmpPath = $tmpFile->getPath(); - // Copy the source file over the temp file - $this->trapWarnings(); - $ok = copy( $source, $tmpPath ); - $this->untrapWarnings(); - if ( !$ok ) { - $tmpFiles[$src] = null; - } else { - $this->chmod( $tmpPath ); - $tmpFiles[$src] = $tmpFile; - } - } + $tmpFiles[$src] = self::$RES_ABSENT; } } diff --git a/includes/libs/filebackend/FileBackend.php b/includes/libs/filebackend/FileBackend.php index 428fec6062..6ab1707be6 100644 --- a/includes/libs/filebackend/FileBackend.php +++ b/includes/libs/filebackend/FileBackend.php @@ -131,8 +131,28 @@ abstract class FileBackend implements LoggerAwareInterface { const ATTR_METADATA = 2; // files can be stored with metadata key/values const ATTR_UNICODE_PATHS = 4; // files can have Unicode paths (not just ASCII) - /** @var null Idiom for "could not determine due to I/O errors" */ - const UNKNOWN = null; + /** @var false Idiom for "no info; non-existant file" (since 1.34) */ + const STAT_ABSENT = false; + + /** @var null Idiom for "no info; I/O errors" (since 1.34) */ + const STAT_ERROR = null; + /** @var null Idiom for "no file/directory list; I/O errors" (since 1.34) */ + const LIST_ERROR = null; + /** @var null Idiom for "no temp URL; not supported or I/O errors" (since 1.34) */ + const TEMPURL_ERROR = null; + /** @var null Idiom for "existence unknown; I/O errors" (since 1.34) */ + const EXISTENCE_ERROR = null; + + /** @var false Idiom for "no timestamp; missing file or I/O errors" (since 1.34) */ + const TIMESTAMP_FAIL = false; + /** @var false Idiom for "no content; missing file or I/O errors" (since 1.34) */ + const CONTENT_FAIL = false; + /** @var false Idiom for "no metadata; missing file or I/O errors" (since 1.34) */ + const XATTRS_FAIL = false; + /** @var false Idiom for "no size; missing file or I/O errors" (since 1.34) */ + const SIZE_FAIL = false; + /** @var false Idiom for "no SHA1 hash; missing file or I/O errors" (since 1.34) */ + const SHA1_FAIL = false; /** * Create a new backend instance from configuration. @@ -157,9 +177,9 @@ abstract class FileBackend implements LoggerAwareInterface { * Allowed values are "implicit", "explicit" and "off". * - concurrency : How many file operations can be done in parallel. * - tmpDirectory : Directory to use for temporary files. - * - tmpFileFactory : Optional TempFSFileFactory object. Only has an effect if tmpDirectory is - * not set. If both are unset or null, then the backend will try to discover a usable - * temporary directory. + * - tmpFileFactory : Optional TempFSFileFactory object. Only has an effect if + * tmpDirectory is not set. If both are unset or null, then the backend will + * try to discover a usable temporary directory. * - obResetFunc : alternative callback to clear the output buffer * - streamMimeFunc : alternative method to determine the content type from the path * - logger : Optional PSR logger object. @@ -946,20 +966,29 @@ abstract class FileBackend implements LoggerAwareInterface { * Check if a file exists at a storage path in the backend. * This returns false if only a directory exists at the path. * + * Callers that only care if a file is readily accessible can use non-strict + * comparisons on the result. If "does not exist" and "existence is unknown" + * must be distinguished, then strict comparisons to true/null should be used. + * + * @see FileBackend::EXISTENCE_ERROR + * @see FileBackend::directoryExists() + * * @param array $params Parameters include: * - src : source storage path * - latest : use the latest available data - * @return bool|null Returns null on failure + * @return bool|null Whether the file exists or null (I/O error) */ abstract public function fileExists( array $params ); /** * Get the last-modified timestamp of the file at a storage path. * + * @see FileBackend::TIMESTAMP_FAIL + * * @param array $params Parameters include: * - src : source storage path * - latest : use the latest available data - * @return string|bool TS_MW timestamp or false on failure + * @return string|false TS_MW timestamp or false (missing file or I/O error) */ abstract public function getFileTimestamp( array $params ); @@ -967,22 +996,22 @@ abstract class FileBackend implements LoggerAwareInterface { * Get the contents of a file at a storage path in the backend. * This should be avoided for potentially large files. * + * @see FileBackend::CONTENT_FAIL + * * @param array $params Parameters include: * - src : source storage path * - latest : use the latest available data - * @return string|bool Returns false on failure + * @return string|false Content string or false (missing file or I/O error) */ final public function getFileContents( array $params ) { - $contents = $this->getFileContentsMulti( - [ 'srcs' => [ $params['src'] ] ] + $params ); + $contents = $this->getFileContentsMulti( [ 'srcs' => [ $params['src'] ] ] + $params ); return $contents[$params['src']]; } /** * Like getFileContents() except it takes an array of storage paths - * and returns a map of storage paths to strings (or null on failure). - * The map keys (paths) are in the same order as the provided list of paths. + * and returns an order preserved map of storage paths to their content. * * @see FileBackend::getFileContents() * @@ -990,7 +1019,7 @@ abstract class FileBackend implements LoggerAwareInterface { * - srcs : list of source storage paths * - latest : use the latest available data * - parallelize : try to do operations in parallel when possible - * @return array Map of (path name => string or false on failure) + * @return string[]|false[] Map of (path name => file content or false on failure) * @since 1.20 */ abstract public function getFileContentsMulti( array $params ); @@ -1006,11 +1035,13 @@ abstract class FileBackend implements LoggerAwareInterface { * * Use FileBackend::hasFeatures() to check how well this is supported. * + * @see FileBackend::XATTRS_FAIL + * * @param array $params * $params include: * - src : source storage path * - latest : use the latest available data - * @return array|bool Returns false on failure + * @return array|false File metadata array or false (missing file or I/O error) * @since 1.23 */ abstract public function getFileXAttributes( array $params ); @@ -1018,10 +1049,12 @@ abstract class FileBackend implements LoggerAwareInterface { /** * Get the size (bytes) of a file at a storage path in the backend. * + * @see FileBackend::SIZE_FAIL + * * @param array $params Parameters include: * - src : source storage path * - latest : use the latest available data - * @return int|bool Returns false on failure + * @return int|false File size in bytes or false (missing file or I/O error) */ abstract public function getFileSize( array $params ); @@ -1033,36 +1066,41 @@ abstract class FileBackend implements LoggerAwareInterface { * - size : the file size (bytes) * Additional values may be included for internal use only. * + * @see FileBackend::STAT_ABSENT + * @see FileBackend::STAT_ERROR + * * @param array $params Parameters include: * - src : source storage path * - latest : use the latest available data - * @return array|bool|null Returns null on failure + * @return array|false|null Attribute map, false (missing file), or null (I/O error) */ abstract public function getFileStat( array $params ); /** - * Get a SHA-1 hash of the file at a storage path in the backend. + * Get a SHA-1 hash of the content of the file at a storage path in the backend. + * + * @see FileBackend::SHA1_FAIL * * @param array $params Parameters include: * - src : source storage path * - latest : use the latest available data - * @return string|bool Hash string or false on failure + * @return string|false Hash string or false (missing file or I/O error) */ abstract public function getFileSha1Base36( array $params ); /** - * Get the properties of the file at a storage path in the backend. + * Get the properties of the content of the file at a storage path in the backend. * This gives the result of FSFile::getProps() on a local copy of the file. * * @param array $params Parameters include: * - src : source storage path * - latest : use the latest available data - * @return array Returns FSFile::placeholderProps() on failure + * @return array Properties map; FSFile::placeholderProps() if file missing or on I/O error */ abstract public function getFileProps( array $params ); /** - * Stream the file at a storage path in the backend. + * Stream the content of the file at a storage path in the backend. * * If the file does not exists, an HTTP 404 error will be given. * Appropriate HTTP headers (Status, Content-Type, Content-Length) @@ -1083,34 +1121,36 @@ abstract class FileBackend implements LoggerAwareInterface { abstract public function streamFile( array $params ); /** - * Returns a file system file, identical to the file at a storage path. + * Returns a file system file, identical in content to the file at a storage path. * The file returned is either: - * - a) A local copy of the file at a storage path in the backend. + * - a) A TempFSFile local copy of the file at a storage path in the backend. * The temporary copy will have the same extension as the source. - * - b) An original of the file at a storage path in the backend. - * Temporary files may be purged when the file object falls out of scope. + * Temporary files may be purged when the file object falls out of scope. + * - b) An FSFile pointing to the original file at a storage path in the backend. + * This is applicable for backends layered directly on top of file systems. * - * Write operations should *never* be done on this file as some backends - * may do internal tracking or may be instances of FileBackendMultiWrite. - * In that latter case, there are copies of the file that must stay in sync. - * Additionally, further calls to this function may return the same file. + * Never modify the returned file since it might be the original, it might be shared + * among multiple callers of this method, or the backend might internally keep FSFile + * references for deferred operations. * * @param array $params Parameters include: * - src : source storage path * - latest : use the latest available data - * @return FSFile|null Returns null on failure + * @return FSFile|null Local file copy or null (missing file or I/O error) */ final public function getLocalReference( array $params ) { - $fsFiles = $this->getLocalReferenceMulti( - [ 'srcs' => [ $params['src'] ] ] + $params ); + $fsFiles = $this->getLocalReferenceMulti( [ 'srcs' => [ $params['src'] ] ] + $params ); return $fsFiles[$params['src']]; } /** - * Like getLocalReference() except it takes an array of storage paths - * and returns a map of storage paths to FSFile objects (or null on failure). - * The map keys (paths) are in the same order as the provided list of paths. + * Like getLocalReference() except it takes an array of storage paths and + * yields an order-preserved map of storage paths to temporary local file copies. + * + * Never modify the returned files since they might be originals, they might be shared + * among multiple callers of this method, or the backend might internally keep FSFile + * references for deferred operations. * * @see FileBackend::getLocalReference() * @@ -1128,22 +1168,24 @@ abstract class FileBackend implements LoggerAwareInterface { * The temporary copy will have the same file extension as the source. * Temporary files may be purged when the file object falls out of scope. * + * Multiple calls to this method for the same path will create new copies. + * * @param array $params Parameters include: * - src : source storage path * - latest : use the latest available data - * @return TempFSFile|null Returns null on failure + * @return TempFSFile|null Temporary local file copy or null (missing file or I/O error) */ final public function getLocalCopy( array $params ) { - $tmpFiles = $this->getLocalCopyMulti( - [ 'srcs' => [ $params['src'] ] ] + $params ); + $tmpFiles = $this->getLocalCopyMulti( [ 'srcs' => [ $params['src'] ] ] + $params ); return $tmpFiles[$params['src']]; } /** - * Like getLocalCopy() except it takes an array of storage paths and - * returns a map of storage paths to TempFSFile objects (or null on failure). - * The map keys (paths) are in the same order as the provided list of paths. + * Like getLocalCopy() except it takes an array of storage paths and yields + * an order preserved-map of storage paths to temporary local file copies. + * + * Multiple calls to this method for the same path will create new copies. * * @see FileBackend::getLocalCopy() * @@ -1166,10 +1208,12 @@ abstract class FileBackend implements LoggerAwareInterface { * Otherwise, one would need to use getLocalReference(), which involves loading * the entire file on to local disk. * + * @see FileBackend::TEMPURL_ERROR + * * @param array $params Parameters include: * - src : source storage path * - ttl : lifetime (seconds) if pre-authenticated; default is 1 day - * @return string|null + * @return string|null URL or null (not supported or I/O error) * @since 1.21 */ abstract public function getFileHttpUrl( array $params ); @@ -1191,11 +1235,12 @@ abstract class FileBackend implements LoggerAwareInterface { * * Storage backends with eventual consistency might return stale data. * + * @see FileBackend::EXISTENCE_ERROR * @see FileBackend::clean() * * @param array $params Parameters include: * - dir : storage directory - * @return bool|null Whether a directory exists or null on failure + * @return bool|null Whether a directory exists or null (I/O error) * @since 1.20 */ abstract public function directoryExists( array $params ); @@ -1213,12 +1258,13 @@ abstract class FileBackend implements LoggerAwareInterface { * * Failures during iteration can result in FileBackendError exceptions (since 1.22). * + * @see FileBackend::LIST_ERROR * @see FileBackend::directoryExists() * * @param array $params Parameters include: * - dir : storage directory * - topOnly : only return direct child dirs of the directory - * @return Traversable|array|null Directory list enumerator null on failure + * @return Traversable|array|null Directory list enumerator or null (initial I/O error) * @since 1.20 */ abstract public function getDirectoryList( array $params ); @@ -1231,11 +1277,12 @@ abstract class FileBackend implements LoggerAwareInterface { * * Failures during iteration can result in FileBackendError exceptions (since 1.22). * + * @see FileBackend::LIST_ERROR * @see FileBackend::directoryExists() * * @param array $params Parameters include: * - dir : storage directory - * @return Traversable|array|null Directory list enumerator or null on failure + * @return Traversable|array|null Directory list enumerator or null (initial I/O error) * @since 1.20 */ final public function getTopDirectoryList( array $params ) { @@ -1254,11 +1301,13 @@ abstract class FileBackend implements LoggerAwareInterface { * * Failures during iteration can result in FileBackendError exceptions (since 1.22). * + * @see FileBackend::LIST_ERROR + * * @param array $params Parameters include: * - dir : storage directory * - topOnly : only return direct child files of the directory (since 1.20) * - adviseStat : set to true if stat requests will be made on the files (since 1.22) - * @return Traversable|array|null File list enumerator or null on failure + * @return Traversable|array|null File list enumerator or null (initial I/O error) */ abstract public function getFileList( array $params ); @@ -1270,6 +1319,8 @@ abstract class FileBackend implements LoggerAwareInterface { * * Failures during iteration can result in FileBackendError exceptions (since 1.22). * + * @see FileBackend::LIST_ERROR + * * @param array $params Parameters include: * - dir : storage directory * - adviseStat : set to true if stat requests will be made on the files (since 1.22) @@ -1360,7 +1411,7 @@ abstract class FileBackend implements LoggerAwareInterface { * @param int|string $type LockManager::LOCK_* constant or "mixed" * @param StatusValue $status StatusValue to update on lock/unlock * @param int $timeout Timeout in seconds (0 means non-blocking) (since 1.24) - * @return ScopedLock|null Returns null on failure + * @return ScopedLock|null RAII-style self-unlocking lock or null on failure */ final public function getScopedFileLocks( array $paths, $type, StatusValue $status, $timeout = 0 @@ -1389,7 +1440,7 @@ abstract class FileBackend implements LoggerAwareInterface { * * @param array $ops List of file operations to FileBackend::doOperations() * @param StatusValue $status StatusValue to update on lock/unlock - * @return ScopedLock|null + * @return ScopedLock|null RAII-style self-unlocking lock or null on failure * @since 1.20 */ abstract public function getScopedLocksForOps( array $ops, StatusValue $status ); @@ -1487,7 +1538,7 @@ abstract class FileBackend implements LoggerAwareInterface { * Returns null if the path is not of the format of a valid storage path. * * @param string $storagePath - * @return string|null + * @return string|null Normalized storage path or null on failure */ final public static function normalizeStoragePath( $storagePath ) { list( $backend, $container, $relPath ) = self::splitStoragePath( $storagePath ); @@ -1509,7 +1560,7 @@ abstract class FileBackend implements LoggerAwareInterface { * "mwstore://backend/container/...", or null if there is no parent. * * @param string $storagePath - * @return string|null + * @return string|null Parent storage path or null on failure */ final public static function parentStoragePath( $storagePath ) { $storagePath = dirname( $storagePath ); @@ -1582,7 +1633,7 @@ abstract class FileBackend implements LoggerAwareInterface { * This uses the same traversal protection as Title::secureAndSplit(). * * @param string $path Storage path relative to a container - * @return string|null + * @return string|null Normalized container path or null on failure */ final protected static function normalizeContainerPath( $path ) { // Normalize directory separators diff --git a/includes/libs/filebackend/FileBackendMultiWrite.php b/includes/libs/filebackend/FileBackendMultiWrite.php index 9bfb5c5f35..cbfd76e407 100644 --- a/includes/libs/filebackend/FileBackendMultiWrite.php +++ b/includes/libs/filebackend/FileBackendMultiWrite.php @@ -248,7 +248,7 @@ class FileBackendMultiWrite extends FileBackend { $masterBackend = $this->backends[$this->masterIndex]; $masterParams = $this->substOpPaths( $params, $masterBackend ); $masterStat = $masterBackend->getFileStat( $masterParams ); - if ( $masterStat === self::UNKNOWN ) { + if ( $masterStat === self::STAT_ERROR ) { $status->fatal( 'backend-fail-stat', $path ); continue; } @@ -357,7 +357,7 @@ class FileBackendMultiWrite extends FileBackend { $masterParams = $this->substOpPaths( $params, $masterBackend ); $masterPath = $masterParams['src']; $masterStat = $masterBackend->getFileStat( $masterParams ); - if ( $masterStat === self::UNKNOWN ) { + if ( $masterStat === self::STAT_ERROR ) { $status->fatal( 'backend-fail-stat', $path ); $this->logger->error( "$fname: file '$masterPath' is not available" ); continue; @@ -379,7 +379,7 @@ class FileBackendMultiWrite extends FileBackend { $cloneParams = $this->substOpPaths( $params, $cloneBackend ); $clonePath = $cloneParams['src']; $cloneStat = $cloneBackend->getFileStat( $cloneParams ); - if ( $cloneStat === self::UNKNOWN ) { + if ( $cloneStat === self::STAT_ERROR ) { $status->fatal( 'backend-fail-stat', $path ); $this->logger->error( "$fname: file '$clonePath' is not available" ); continue; @@ -501,7 +501,7 @@ class FileBackendMultiWrite extends FileBackend { * * @param array|string $paths List of paths or single string path * @param FileBackendStore $backend - * @return array|string + * @return string[]|string */ protected function substPaths( $paths, FileBackendStore $backend ) { return preg_replace( @@ -515,12 +515,13 @@ class FileBackendMultiWrite extends FileBackend { * Substitute the backend of internal storage paths with the proxy backend's name * * @param array|string $paths List of paths or single string path - * @return array|string + * @param FileBackendStore $backend internal storage backend + * @return string[]|string */ - protected function unsubstPaths( $paths ) { + protected function unsubstPaths( $paths, FileBackendStore $backend ) { return preg_replace( - '!^mwstore://([^/]+)!', - StringUtils::escapeRegexReplacement( "mwstore://{$this->name}" ), + '!^mwstore://' . preg_quote( $backend->getName(), '!' ) . '/!', + StringUtils::escapeRegexReplacement( "mwstore://{$this->name}/" ), $paths // string or array ); } @@ -674,7 +675,7 @@ class FileBackendMultiWrite extends FileBackend { $contents = []; // (path => FSFile) mapping using the proxy backend's name foreach ( $contentsM as $path => $data ) { - $contents[$this->unsubstPaths( $path )] = $data; + $contents[$this->unsubstPaths( $path, $this->backends[$index] )] = $data; } return $contents; @@ -709,7 +710,7 @@ class FileBackendMultiWrite extends FileBackend { $fsFiles = []; // (path => FSFile) mapping using the proxy backend's name foreach ( $fsFilesM as $path => $fsFile ) { - $fsFiles[$this->unsubstPaths( $path )] = $fsFile; + $fsFiles[$this->unsubstPaths( $path, $this->backends[$index] )] = $fsFile; } return $fsFiles; @@ -723,7 +724,7 @@ class FileBackendMultiWrite extends FileBackend { $tempFiles = []; // (path => TempFSFile) mapping using the proxy backend's name foreach ( $tempFilesM as $path => $tempFile ) { - $tempFiles[$this->unsubstPaths( $path )] = $tempFile; + $tempFiles[$this->unsubstPaths( $path, $this->backends[$index] )] = $tempFile; } return $tempFiles; @@ -784,8 +785,14 @@ class FileBackendMultiWrite extends FileBackend { $paths = $this->backends[$this->masterIndex]->getPathsToLockForOpsInternal( $fileOps ); // Get the paths under the proxy backend's name $pbPaths = [ - LockManager::LOCK_UW => $this->unsubstPaths( $paths[LockManager::LOCK_UW] ), - LockManager::LOCK_EX => $this->unsubstPaths( $paths[LockManager::LOCK_EX] ) + LockManager::LOCK_UW => $this->unsubstPaths( + $paths[LockManager::LOCK_UW], + $this->backends[$this->masterIndex] + ), + LockManager::LOCK_EX => $this->unsubstPaths( + $paths[LockManager::LOCK_EX], + $this->backends[$this->masterIndex] + ) ]; // Actually acquire the locks diff --git a/includes/libs/filebackend/FileBackendStore.php b/includes/libs/filebackend/FileBackendStore.php index e6375654f4..f2c07e8253 100644 --- a/includes/libs/filebackend/FileBackendStore.php +++ b/includes/libs/filebackend/FileBackendStore.php @@ -59,6 +59,16 @@ abstract class FileBackendStore extends FileBackend { const CACHE_CHEAP_SIZE = 500; // integer; max entries in "cheap cache" const CACHE_EXPENSIVE_SIZE = 5; // integer; max entries in "expensive cache" + /** @var false Idiom for "no result due to missing file" (since 1.34) */ + protected static $RES_ABSENT = false; + /** @var null Idiom for "no result due to I/O errors" (since 1.34) */ + protected static $RES_ERROR = null; + + /** @var string File does not exist according to a normal stat query */ + protected static $ABSENT_NORMAL = 'FNE-N'; + /** @var string File does not exist according to a "latest"-mode stat query */ + protected static $ABSENT_LATEST = 'FNE-L'; + /** * @see FileBackend::__construct() * Additional $config params include: @@ -91,9 +101,10 @@ abstract class FileBackendStore extends FileBackend { } /** - * Check if a file can be created or changed at a given storage path. - * FS backends should check if the parent directory exists, files can be - * written under it, and that any file already there is writable. + * Check if a file can be created or changed at a given storage path in the backend + * + * FS backends should check that the parent directory exists, files can be written + * under it, and that any file already there is both readable and writable. * Backends using key/value stores should check if the container exists. * * @param string $storagePath @@ -122,6 +133,7 @@ abstract class FileBackendStore extends FileBackend { final public function createInternal( array $params ) { /** @noinspection PhpUnusedLocalVariableInspection */ $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); + if ( strlen( $params['content'] ) > $this->maxFileSizeInternal() ) { $status = $this->newStatus( 'backend-fail-maxsize', $params['dst'], $this->maxFileSizeInternal() ); @@ -164,6 +176,7 @@ abstract class FileBackendStore extends FileBackend { final public function storeInternal( array $params ) { /** @noinspection PhpUnusedLocalVariableInspection */ $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); + if ( filesize( $params['src'] ) > $this->maxFileSizeInternal() ) { $status = $this->newStatus( 'backend-fail-maxsize', $params['dst'], $this->maxFileSizeInternal() ); @@ -207,6 +220,7 @@ abstract class FileBackendStore extends FileBackend { final public function copyInternal( array $params ) { /** @noinspection PhpUnusedLocalVariableInspection */ $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); + $status = $this->doCopyInternal( $params ); $this->clearCache( [ $params['dst'] ] ); if ( !isset( $params['dstExists'] ) || $params['dstExists'] ) { @@ -240,6 +254,7 @@ abstract class FileBackendStore extends FileBackend { final public function deleteInternal( array $params ) { /** @noinspection PhpUnusedLocalVariableInspection */ $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); + $status = $this->doDeleteInternal( $params ); $this->clearCache( [ $params['src'] ] ); $this->deleteFileCache( $params['src'] ); // persistent cache @@ -275,6 +290,7 @@ abstract class FileBackendStore extends FileBackend { final public function moveInternal( array $params ) { /** @noinspection PhpUnusedLocalVariableInspection */ $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); + $status = $this->doMoveInternal( $params ); $this->clearCache( [ $params['src'], $params['dst'] ] ); $this->deleteFileCache( $params['src'] ); // persistent cache @@ -322,6 +338,7 @@ abstract class FileBackendStore extends FileBackend { final public function describeInternal( array $params ) { /** @noinspection PhpUnusedLocalVariableInspection */ $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); + if ( count( $params['headers'] ) ) { $status = $this->doDescribeInternal( $params ); $this->clearCache( [ $params['src'] ] ); @@ -617,54 +634,71 @@ abstract class FileBackendStore extends FileBackend { final public function fileExists( array $params ) { /** @noinspection PhpUnusedLocalVariableInspection */ $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); + $stat = $this->getFileStat( $params ); + if ( is_array( $stat ) ) { + return true; + } - return ( $stat === self::UNKNOWN ) ? self::UNKNOWN : (bool)$stat; + return ( $stat === self::$RES_ABSENT ) ? false : self::EXISTENCE_ERROR; } final public function getFileTimestamp( array $params ) { /** @noinspection PhpUnusedLocalVariableInspection */ $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); + $stat = $this->getFileStat( $params ); + if ( is_array( $stat ) ) { + return $stat['mtime']; + } - return $stat ? $stat['mtime'] : false; + return self::TIMESTAMP_FAIL; // all failure cases } final public function getFileSize( array $params ) { /** @noinspection PhpUnusedLocalVariableInspection */ $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); + $stat = $this->getFileStat( $params ); + if ( is_array( $stat ) ) { + return $stat['size']; + } - return $stat ? $stat['size'] : false; + return self::SIZE_FAIL; // all failure cases } final public function getFileStat( array $params ) { + /** @noinspection PhpUnusedLocalVariableInspection */ + $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); + $path = self::normalizeStoragePath( $params['src'] ); if ( $path === null ) { - return false; // invalid storage path + return self::STAT_ERROR; // invalid storage path } - /** @noinspection PhpUnusedLocalVariableInspection */ - $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); - $latest = !empty( $params['latest'] ); // use latest data? - $requireSHA1 = !empty( $params['requireSHA1'] ); // require SHA-1 if file exists? + // Whether to bypass cache except for process cache entries loaded directly from + // high consistency backend queries (caller handles any cache flushing and locking) + $latest = !empty( $params['latest'] ); + // Whether to ignore cache entries missing the SHA-1 field for existing files + $requireSHA1 = !empty( $params['requireSHA1'] ); + $stat = $this->cheapCache->getField( $path, 'stat', self::CACHE_TTL ); + // Load the persistent stat cache into process cache if needed if ( !$latest ) { - $stat = $this->cheapCache->getField( $path, 'stat', self::CACHE_TTL ); - // Note that some backends, like SwiftFileBackend, sometimes set file stat process - // cache entries from mass object listings that do not include the SHA-1. In that - // case, loading the persistent stat cache will likely yield the SHA-1. if ( - $stat === self::UNKNOWN || + // File stat is not in process cache + $stat === null || + // Key/value store backends might opportunistically set file stat process + // cache entries from object listings that do not include the SHA-1. In that + // case, loading the persistent stat cache will likely yield the SHA-1. ( $requireSHA1 && is_array( $stat ) && !isset( $stat['sha1'] ) ) ) { - $this->primeFileCache( [ $path ] ); // check persistent cache + $this->primeFileCache( [ $path ] ); + // Get any newly process-cached entry + $stat = $this->cheapCache->getField( $path, 'stat', self::CACHE_TTL ); } } - $stat = $this->cheapCache->getField( $path, 'stat', self::CACHE_TTL ); - // If we want the latest data, check that this cached - // value was in fact fetched with the latest available data. if ( is_array( $stat ) ) { if ( ( !$latest || $stat['latest'] ) && @@ -672,42 +706,90 @@ abstract class FileBackendStore extends FileBackend { ) { return $stat; } - } elseif ( in_array( $stat, [ 'NOT_EXIST', 'NOT_EXIST_LATEST' ], true ) ) { - if ( !$latest || $stat === 'NOT_EXIST_LATEST' ) { - return false; + } elseif ( $stat === self::$ABSENT_LATEST ) { + return self::STAT_ABSENT; + } elseif ( $stat === self::$ABSENT_NORMAL ) { + if ( !$latest ) { + return self::STAT_ABSENT; } } + // Load the file stat from the backend and update caches $stat = $this->doGetFileStat( $params ); + $this->ingestFreshFileStats( [ $path => $stat ], $latest ); - if ( is_array( $stat ) ) { // file exists - // Strongly consistent backends can automatically set "latest" - $stat['latest'] = $stat['latest'] ?? $latest; - $this->cheapCache->setField( $path, 'stat', $stat ); - $this->setFileCache( $path, $stat ); // update persistent cache - if ( isset( $stat['sha1'] ) ) { // some backends store SHA-1 as metadata - $this->cheapCache->setField( $path, 'sha1', - [ 'hash' => $stat['sha1'], 'latest' => $latest ] ); - } - if ( isset( $stat['xattr'] ) ) { // some backends store headers/metadata - $stat['xattr'] = self::normalizeXAttributes( $stat['xattr'] ); - $this->cheapCache->setField( $path, 'xattr', - [ 'map' => $stat['xattr'], 'latest' => $latest ] ); + if ( is_array( $stat ) ) { + return $stat; + } + + return ( $stat === self::$RES_ERROR ) ? self::STAT_ERROR : self::STAT_ABSENT; + } + + /** + * Ingest file stat entries that just came from querying the backend (not cache) + * + * @param array[]|bool[]|null[] $stats Map of (path => doGetFileStat() stype result) + * @param bool $latest Whether doGetFileStat()/doGetFileStatMulti() had the 'latest' flag + * @return bool Whether all files have non-error stat replies + */ + final protected function ingestFreshFileStats( array $stats, $latest ) { + $success = true; + + foreach ( $stats as $path => $stat ) { + if ( is_array( $stat ) ) { + // Strongly consistent backends might automatically set this flag + $stat['latest'] = $stat['latest'] ?? $latest; + + $this->cheapCache->setField( $path, 'stat', $stat ); + if ( isset( $stat['sha1'] ) ) { + // Some backends store the SHA-1 hash as metadata + $this->cheapCache->setField( + $path, + 'sha1', + [ 'hash' => $stat['sha1'], 'latest' => $latest ] + ); + } + if ( isset( $stat['xattr'] ) ) { + // Some backends store custom headers/metadata + $stat['xattr'] = self::normalizeXAttributes( $stat['xattr'] ); + $this->cheapCache->setField( + $path, + 'xattr', + [ 'map' => $stat['xattr'], 'latest' => $latest ] + ); + } + // Update persistent cache (@TODO: set all entries in one batch) + $this->setFileCache( $path, $stat ); + } elseif ( $stat === self::$RES_ABSENT ) { + $this->cheapCache->setField( + $path, + 'stat', + $latest ? self::$ABSENT_LATEST : self::$ABSENT_NORMAL + ); + $this->cheapCache->setField( + $path, + 'xattr', + [ 'map' => self::XATTRS_FAIL, 'latest' => $latest ] + ); + $this->cheapCache->setField( + $path, + 'sha1', + [ 'hash' => self::SHA1_FAIL, 'latest' => $latest ] + ); + $this->logger->debug( + __METHOD__ . ': File {path} does not exist', + [ 'path' => $path ] + ); + } else { + $success = false; + $this->logger->error( + __METHOD__ . ': Could not stat file {path}', + [ 'path' => $path ] + ); } - } elseif ( $stat === false ) { // file does not exist - $this->cheapCache->setField( $path, 'stat', $latest ? 'NOT_EXIST_LATEST' : 'NOT_EXIST' ); - $this->cheapCache->setField( $path, 'xattr', [ 'map' => false, 'latest' => $latest ] ); - $this->cheapCache->setField( $path, 'sha1', [ 'hash' => false, 'latest' => $latest ] ); - $this->logger->debug( __METHOD__ . ': File {path} does not exist', [ - 'path' => $path, - ] ); - } else { // an error occurred - $this->logger->warning( __METHOD__ . ': Could not stat file {path}', [ - 'path' => $path, - ] ); } - return $stat; + return $success; } /** @@ -722,6 +804,11 @@ abstract class FileBackendStore extends FileBackend { $params = $this->setConcurrencyFlags( $params ); $contents = $this->doGetFileContentsMulti( $params ); + foreach ( $contents as $path => $content ) { + if ( !is_string( $content ) ) { + $contents[$path] = self::CONTENT_FAIL; // used for all failure cases + } + } return $contents; } @@ -729,26 +816,34 @@ abstract class FileBackendStore extends FileBackend { /** * @see FileBackendStore::getFileContentsMulti() * @param array $params - * @return array + * @return string[]|bool[]|null[] Map of (path => string, false (missing), or null (error)) */ protected function doGetFileContentsMulti( array $params ) { $contents = []; foreach ( $this->doGetLocalReferenceMulti( $params ) as $path => $fsFile ) { - AtEase::suppressWarnings(); - $contents[$path] = $fsFile ? file_get_contents( $fsFile->getPath() ) : false; - AtEase::restoreWarnings(); + if ( $fsFile instanceof FSFile ) { + AtEase::suppressWarnings(); + $content = file_get_contents( $fsFile->getPath() ); + AtEase::restoreWarnings(); + $contents[$path] = is_string( $content ) ? $content : self::$RES_ERROR; + } elseif ( $fsFile === self::$RES_ABSENT ) { + $contents[$path] = self::$RES_ABSENT; + } else { + $contents[$path] = self::$RES_ERROR; + } } return $contents; } final public function getFileXAttributes( array $params ) { + /** @noinspection PhpUnusedLocalVariableInspection */ + $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); + $path = self::normalizeStoragePath( $params['src'] ); if ( $path === null ) { - return false; // invalid storage path + return self::XATTRS_FAIL; // invalid storage path } - /** @noinspection PhpUnusedLocalVariableInspection */ - $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); $latest = !empty( $params['latest'] ); // use latest data? if ( $this->cheapCache->hasField( $path, 'xattr', self::CACHE_TTL ) ) { $stat = $this->cheapCache->getField( $path, 'xattr' ); @@ -759,8 +854,22 @@ abstract class FileBackendStore extends FileBackend { } } $fields = $this->doGetFileXAttributes( $params ); - $fields = is_array( $fields ) ? self::normalizeXAttributes( $fields ) : false; - $this->cheapCache->setField( $path, 'xattr', [ 'map' => $fields, 'latest' => $latest ] ); + if ( is_array( $fields ) ) { + $fields = self::normalizeXAttributes( $fields ); + $this->cheapCache->setField( + $path, + 'xattr', + [ 'map' => $fields, 'latest' => $latest ] + ); + } elseif ( $fields === self::$RES_ABSENT ) { + $this->cheapCache->setField( + $path, + 'xattr', + [ 'map' => self::XATTRS_FAIL, 'latest' => $latest ] + ); + } else { + $fields = self::XATTRS_FAIL; // used for all failure cases + } return $fields; } @@ -768,19 +877,20 @@ abstract class FileBackendStore extends FileBackend { /** * @see FileBackendStore::getFileXAttributes() * @param array $params - * @return array[][]|false + * @return array[][]|false|null Attributes, false (missing file), or null (error) */ protected function doGetFileXAttributes( array $params ) { return [ 'headers' => [], 'metadata' => [] ]; // not supported } final public function getFileSha1Base36( array $params ) { + /** @noinspection PhpUnusedLocalVariableInspection */ + $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); + $path = self::normalizeStoragePath( $params['src'] ); if ( $path === null ) { - return false; // invalid storage path + return self::SHA1_FAIL; // invalid storage path } - /** @noinspection PhpUnusedLocalVariableInspection */ - $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); $latest = !empty( $params['latest'] ); // use latest data? if ( $this->cheapCache->hasField( $path, 'sha1', self::CACHE_TTL ) ) { $stat = $this->cheapCache->getField( $path, 'sha1' ); @@ -790,33 +900,49 @@ abstract class FileBackendStore extends FileBackend { return $stat['hash']; } } - $hash = $this->doGetFileSha1Base36( $params ); - $this->cheapCache->setField( $path, 'sha1', [ 'hash' => $hash, 'latest' => $latest ] ); + $sha1 = $this->doGetFileSha1Base36( $params ); + if ( is_string( $sha1 ) ) { + $this->cheapCache->setField( + $path, + 'sha1', + [ 'hash' => $sha1, 'latest' => $latest ] + ); + } elseif ( $sha1 === self::$RES_ABSENT ) { + $this->cheapCache->setField( + $path, + 'sha1', + [ 'hash' => self::SHA1_FAIL, 'latest' => $latest ] + ); + } else { + $sha1 = self::SHA1_FAIL; // used for all failure cases + } - return $hash; + return $sha1; } /** * @see FileBackendStore::getFileSha1Base36() * @param array $params - * @return bool|string + * @return bool|string|null SHA1, false (missing file), or null (error) */ protected function doGetFileSha1Base36( array $params ) { $fsFile = $this->getLocalReference( $params ); - if ( !$fsFile ) { - return false; - } else { - return $fsFile->getSha1Base36(); + if ( $fsFile instanceof FSFile ) { + $sha1 = $fsFile->getSha1Base36(); + + return is_string( $sha1 ) ? $sha1 : self::$RES_ERROR; } + + return ( $fsFile === self::$RES_ERROR ) ? self::$RES_ERROR : self::$RES_ABSENT; } final public function getFileProps( array $params ) { /** @noinspection PhpUnusedLocalVariableInspection */ $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); + $fsFile = $this->getLocalReference( $params ); - $props = $fsFile ? $fsFile->getProps() : FSFile::placeholderProps(); - return $props; + return $fsFile ? $fsFile->getProps() : FSFile::placeholderProps(); } final public function getLocalReferenceMulti( array $params ) { @@ -844,10 +970,15 @@ abstract class FileBackendStore extends FileBackend { // Fetch local references of any remaning files... $params['srcs'] = array_diff( $params['srcs'], array_keys( $fsFiles ) ); foreach ( $this->doGetLocalReferenceMulti( $params ) as $path => $fsFile ) { - $fsFiles[$path] = $fsFile; - if ( $fsFile ) { // update the process cache... - $this->expensiveCache->setField( $path, 'localRef', - [ 'object' => $fsFile, 'latest' => $latest ] ); + if ( $fsFile instanceof FSFile ) { + $fsFiles[$path] = $fsFile; + $this->expensiveCache->setField( + $path, + 'localRef', + [ 'object' => $fsFile, 'latest' => $latest ] + ); + } else { + $fsFiles[$path] = null; // used for all failure cases } } @@ -857,7 +988,7 @@ abstract class FileBackendStore extends FileBackend { /** * @see FileBackendStore::getLocalReferenceMulti() * @param array $params - * @return array + * @return string[]|bool[]|null[] Map of (path => FSFile, false (missing), or null (error)) */ protected function doGetLocalReferenceMulti( array $params ) { return $this->doGetLocalCopyMulti( $params ); @@ -869,6 +1000,11 @@ abstract class FileBackendStore extends FileBackend { $params = $this->setConcurrencyFlags( $params ); $tmpFiles = $this->doGetLocalCopyMulti( $params ); + foreach ( $tmpFiles as $path => $tmpFile ) { + if ( !$tmpFile ) { + $tmpFiles[$path] = null; // used for all failure cases + } + } return $tmpFiles; } @@ -876,7 +1012,7 @@ abstract class FileBackendStore extends FileBackend { /** * @see FileBackendStore::getLocalCopyMulti() * @param array $params - * @return array + * @return string[]|bool[]|null[] Map of (path => TempFSFile, false (missing), or null (error)) */ abstract protected function doGetLocalCopyMulti( array $params ); @@ -886,7 +1022,7 @@ abstract class FileBackendStore extends FileBackend { * @return string|null */ public function getFileHttpUrl( array $params ) { - return null; // not supported + return self::TEMPURL_ERROR; // not supported } final public function streamFile( array $params ) { @@ -947,7 +1083,7 @@ abstract class FileBackendStore extends FileBackend { final public function directoryExists( array $params ) { list( $fullCont, $dir, $shard ) = $this->resolveStoragePath( $params['dir'] ); if ( $dir === null ) { - return false; // invalid storage path + return self::EXISTENCE_ERROR; // invalid storage path } if ( $shard !== null ) { // confined to a single container/shard return $this->doDirectoryExists( $fullCont, $dir, $params ); @@ -957,11 +1093,11 @@ abstract class FileBackendStore extends FileBackend { $res = false; // response foreach ( $this->getContainerSuffixes( $shortCont ) as $suffix ) { $exists = $this->doDirectoryExists( "{$fullCont}{$suffix}", $dir, $params ); - if ( $exists ) { + if ( $exists === true ) { $res = true; break; // found one! - } elseif ( $exists === null ) { // error? - $res = self::UNKNOWN; // if we don't find anything, it is indeterminate + } elseif ( $exists === self::$RES_ERROR ) { + $res = self::EXISTENCE_ERROR; } } @@ -981,8 +1117,8 @@ abstract class FileBackendStore extends FileBackend { final public function getDirectoryList( array $params ) { list( $fullCont, $dir, $shard ) = $this->resolveStoragePath( $params['dir'] ); - if ( $dir === null ) { // invalid storage path - return self::UNKNOWN; + if ( $dir === null ) { + return self::EXISTENCE_ERROR; // invalid storage path } if ( $shard !== null ) { // File listing is confined to a single container/shard @@ -1005,14 +1141,14 @@ abstract class FileBackendStore extends FileBackend { * @param string $container Resolved container name * @param string $dir Resolved path relative to container * @param array $params - * @return Traversable|array|null Returns null on failure + * @return Traversable|array|null Iterable list or null (error) */ abstract public function getDirectoryListInternal( $container, $dir, array $params ); final public function getFileList( array $params ) { list( $fullCont, $dir, $shard ) = $this->resolveStoragePath( $params['dir'] ); - if ( $dir === null ) { // invalid storage path - return self::UNKNOWN; + if ( $dir === null ) { + return self::LIST_ERROR; // invalid storage path } if ( $shard !== null ) { // File listing is confined to a single container/shard @@ -1035,7 +1171,7 @@ abstract class FileBackendStore extends FileBackend { * @param string $container Resolved container name * @param string $dir Resolved path relative to container * @param array $params - * @return Traversable|string[]|null Returns null on failure + * @return Traversable|string[]|null Iterable list or null (error) */ abstract public function getFileListInternal( $container, $dir, array $params ); @@ -1356,7 +1492,6 @@ abstract class FileBackendStore extends FileBackend { final public function preloadFileStat( array $params ) { /** @noinspection PhpUnusedLocalVariableInspection */ $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); - $success = true; // no network errors $params['concurrency'] = ( $this->parallelize !== 'off' ) ? $this->concurrency : 1; $stats = $this->doGetFileStatMulti( $params ); @@ -1364,45 +1499,10 @@ abstract class FileBackendStore extends FileBackend { return true; // not supported } - $latest = !empty( $params['latest'] ); // use latest data? - foreach ( $stats as $path => $stat ) { - $path = FileBackend::normalizeStoragePath( $path ); - if ( $path === null ) { - continue; // this shouldn't happen - } - if ( is_array( $stat ) ) { // file exists - // Strongly consistent backends can automatically set "latest" - $stat['latest'] = $stat['latest'] ?? $latest; - $this->cheapCache->setField( $path, 'stat', $stat ); - $this->setFileCache( $path, $stat ); // update persistent cache - if ( isset( $stat['sha1'] ) ) { // some backends store SHA-1 as metadata - $this->cheapCache->setField( $path, 'sha1', - [ 'hash' => $stat['sha1'], 'latest' => $latest ] ); - } - if ( isset( $stat['xattr'] ) ) { // some backends store headers/metadata - $stat['xattr'] = self::normalizeXAttributes( $stat['xattr'] ); - $this->cheapCache->setField( $path, 'xattr', - [ 'map' => $stat['xattr'], 'latest' => $latest ] ); - } - } elseif ( $stat === false ) { // file does not exist - $this->cheapCache->setField( $path, 'stat', - $latest ? 'NOT_EXIST_LATEST' : 'NOT_EXIST' ); - $this->cheapCache->setField( $path, 'xattr', - [ 'map' => false, 'latest' => $latest ] ); - $this->cheapCache->setField( $path, 'sha1', - [ 'hash' => false, 'latest' => $latest ] ); - $this->logger->debug( __METHOD__ . ': File {path} does not exist', [ - 'path' => $path, - ] ); - } else { // an error occurred - $success = false; - $this->logger->warning( __METHOD__ . ': Could not stat file {path}', [ - 'path' => $path, - ] ); - } - } + // Whether this queried the backend in high consistency mode + $latest = !empty( $params['latest'] ); - return $success; + return $this->ingestFreshFileStats( $stats, $latest ); } /** @@ -1810,7 +1910,7 @@ abstract class FileBackendStore extends FileBackend { $paths[] = FileBackend::normalizeStoragePath( $item ); } } - // Get rid of any paths that failed normalization... + // Get rid of any paths that failed normalization $paths = array_filter( $paths, 'strlen' ); // remove nulls // Get all the corresponding cache keys for paths... foreach ( $paths as $path ) { @@ -1819,22 +1919,33 @@ abstract class FileBackendStore extends FileBackend { $pathNames[$this->fileCacheKey( $path )] = $path; } } - // Get all cache entries for these file cache keys... + // Get all cache entries for these file cache keys. + // Note that negatives are not cached by getFileStat()/preloadFileStat(). $values = $this->memCache->getMulti( array_keys( $pathNames ) ); - foreach ( $values as $cacheKey => $val ) { + // Load all of the results into process cache... + foreach ( array_filter( $values, 'is_array' ) as $cacheKey => $stat ) { $path = $pathNames[$cacheKey]; - if ( is_array( $val ) ) { - $val['latest'] = false; // never completely trust cache - $this->cheapCache->setField( $path, 'stat', $val ); - if ( isset( $val['sha1'] ) ) { // some backends store SHA-1 as metadata - $this->cheapCache->setField( $path, 'sha1', - [ 'hash' => $val['sha1'], 'latest' => false ] ); - } - if ( isset( $val['xattr'] ) ) { // some backends store headers/metadata - $val['xattr'] = self::normalizeXAttributes( $val['xattr'] ); - $this->cheapCache->setField( $path, 'xattr', - [ 'map' => $val['xattr'], 'latest' => false ] ); - } + // Sanity; this flag only applies to stat info loaded directly + // from a high consistency backend query to the process cache + unset( $stat['latest'] ); + + $this->cheapCache->setField( $path, 'stat', $stat ); + if ( isset( $stat['sha1'] ) && strlen( $stat['sha1'] ) == 31 ) { + // Some backends store SHA-1 as metadata + $this->cheapCache->setField( + $path, + 'sha1', + [ 'hash' => $stat['sha1'], 'latest' => false ] + ); + } + if ( isset( $stat['xattr'] ) && is_array( $stat['xattr'] ) ) { + // Some backends store custom headers/metadata + $stat['xattr'] = self::normalizeXAttributes( $stat['xattr'] ); + $this->cheapCache->setField( + $path, + 'xattr', + [ 'map' => $stat['xattr'], 'latest' => false ] + ); } } } diff --git a/includes/libs/filebackend/MemoryFileBackend.php b/includes/libs/filebackend/MemoryFileBackend.php index 88b281e380..f3bbecb814 100644 --- a/includes/libs/filebackend/MemoryFileBackend.php +++ b/includes/libs/filebackend/MemoryFileBackend.php @@ -41,7 +41,7 @@ class MemoryFileBackend extends FileBackendStore { } public function isPathUsableInternal( $storagePath ) { - return true; + return ( $this->resolveHashKey( $storagePath ) !== null ); } protected function doCreateInternal( array $params ) { @@ -148,7 +148,7 @@ class MemoryFileBackend extends FileBackendStore { protected function doGetFileStat( array $params ) { $src = $this->resolveHashKey( $params['src'] ); if ( $src === null ) { - return false; // invalid path + return self::$RES_ERROR; // invalid path } if ( isset( $this->files[$src] ) ) { @@ -158,15 +158,17 @@ class MemoryFileBackend extends FileBackendStore { ]; } - return false; + return self::$RES_ABSENT; } protected function doGetLocalCopyMulti( array $params ) { $tmpFiles = []; // (path => TempFSFile) foreach ( $params['srcs'] as $srcPath ) { $src = $this->resolveHashKey( $srcPath ); - if ( $src === null || !isset( $this->files[$src] ) ) { - $fsFile = null; + if ( $src === null ) { + $fsFile = self::$RES_ERROR; + } elseif ( !isset( $this->files[$src] ) ) { + $fsFile = self::$RES_ABSENT; } else { // Create a new temporary file with the same extension... $ext = FileBackend::extensionFromPath( $src ); @@ -174,7 +176,7 @@ class MemoryFileBackend extends FileBackendStore { if ( $fsFile ) { $bytes = file_put_contents( $fsFile->getPath(), $this->files[$src]['data'] ); if ( $bytes !== strlen( $this->files[$src]['data'] ) ) { - $fsFile = null; + $fsFile = self::$RES_ERROR; } } } diff --git a/includes/libs/filebackend/SwiftFileBackend.php b/includes/libs/filebackend/SwiftFileBackend.php index 6d6451e32b..56a2177d64 100644 --- a/includes/libs/filebackend/SwiftFileBackend.php +++ b/includes/libs/filebackend/SwiftFileBackend.php @@ -145,8 +145,11 @@ class SwiftFileBackend extends FileBackendStore { } public function getFeatures() { - return ( FileBackend::ATTR_UNICODE_PATHS | - FileBackend::ATTR_HEADERS | FileBackend::ATTR_METADATA ); + return ( + FileBackend::ATTR_UNICODE_PATHS | + FileBackend::ATTR_HEADERS | + FileBackend::ATTR_METADATA + ); } protected function resolveContainerPath( $container, $relStoragePath ) { @@ -593,7 +596,7 @@ class SwiftFileBackend extends FileBackendStore { $stat = $this->getContainerStat( $fullCont ); if ( is_array( $stat ) ) { return $status; // already there - } elseif ( $stat === self::UNKNOWN ) { + } elseif ( $stat === self::$RES_ERROR ) { $status->fatal( 'backend-fail-internal', $this->name ); $this->logger->error( __METHOD__ . ': cannot get container stat' ); @@ -777,8 +780,6 @@ class SwiftFileBackend extends FileBackendStore { } protected function doGetFileContentsMulti( array $params ) { - $contents = []; - $auth = $this->getAuthentication(); $ep = array_diff_key( $params, [ 'srcs' => 1 ] ); // for error logging @@ -786,11 +787,12 @@ class SwiftFileBackend extends FileBackendStore { // if the file does not exist. Do not waste time doing file stats here. $reqs = []; // (path => op) + // Initial dummy values to preserve path order + $contents = array_fill_keys( $params['srcs'], self::$RES_ERROR ); foreach ( $params['srcs'] as $path ) { // each path in this concurrent batch list( $srcCont, $srcRel ) = $this->resolveStoragePathReal( $path ); if ( $srcRel === null || !$auth ) { - $contents[$path] = false; - continue; + continue; // invalid storage path or auth error } // Create a new temporary memory file... $handle = fopen( 'php://temp', 'wb' ); @@ -803,7 +805,6 @@ class SwiftFileBackend extends FileBackendStore { 'stream' => $handle, ]; } - $contents[$path] = false; } $opts = [ 'maxConnsPerHost' => $params['concurrency'] ]; @@ -812,10 +813,21 @@ class SwiftFileBackend extends FileBackendStore { list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $op['response']; if ( $rcode >= 200 && $rcode <= 299 ) { rewind( $op['stream'] ); // start from the beginning - $contents[$path] = stream_get_contents( $op['stream'] ); + $content = (string)stream_get_contents( $op['stream'] ); + $size = strlen( $content ); + // Make sure that stream finished + if ( $size === (int)$rhdrs['content-length'] ) { + $contents[$path] = $content; + } else { + $contents[$path] = self::$RES_ERROR; + $rerr = "Got {$size}/{$rhdrs['content-length']} bytes"; + $this->onError( null, __METHOD__, + [ 'src' => $path ] + $ep, $rerr, $rcode, $rdesc ); + } } elseif ( $rcode === 404 ) { - $contents[$path] = false; + $contents[$path] = self::$RES_ABSENT; } else { + $contents[$path] = self::$RES_ERROR; $this->onError( null, __METHOD__, [ 'src' => $path ] + $ep, $rerr, $rcode, $rdesc ); } @@ -832,7 +844,7 @@ class SwiftFileBackend extends FileBackendStore { return ( count( $status->value ) ) > 0; } - return self::UNKNOWN; // error + return self::$RES_ERROR; } /** @@ -874,6 +886,7 @@ class SwiftFileBackend extends FileBackendStore { return $dirs; // nothing more } + /** @noinspection PhpUnusedLocalVariableInspection */ $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); $prefix = ( $dir == '' ) ? null : "{$dir}/"; @@ -956,10 +969,11 @@ class SwiftFileBackend extends FileBackendStore { return $files; // nothing more } + /** @noinspection PhpUnusedLocalVariableInspection */ $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); $prefix = ( $dir == '' ) ? null : "{$dir}/"; - // $objects will contain a list of unfiltered names or CF_Object items + // $objects will contain a list of unfiltered names or stdClass items // Non-recursive: only list files right under $dir if ( !empty( $params['topOnly'] ) ) { if ( !empty( $params['adviseStat'] ) ) { @@ -982,7 +996,7 @@ class SwiftFileBackend extends FileBackendStore { } $objects = $status->value; - $files = $this->buildFileObjectListing( $params, $dir, $objects ); + $files = $this->buildFileObjectListing( $objects ); // Page on the unfiltered object listing (what is returned may be filtered) if ( count( $objects ) < $limit ) { @@ -997,14 +1011,12 @@ class SwiftFileBackend extends FileBackendStore { /** * Build a list of file objects, filtering out any directories - * and extracting any stat info if provided in $objects (for CF_Objects) + * and extracting any stat info if provided in $objects * - * @param array $params Parameters for getDirectoryList() - * @param string $dir Resolved container directory path - * @param array $objects List of CF_Object items or object names + * @param stdClass[]|string[] $objects List of stdClass items or object names * @return array List of (names,stat array or null) entries */ - private function buildFileObjectListing( array $params, $dir, array $objects ) { + private function buildFileObjectListing( array $objects ) { $names = []; foreach ( $objects as $object ) { if ( is_object( $object ) ) { @@ -1042,18 +1054,17 @@ class SwiftFileBackend extends FileBackendStore { protected function doGetFileXAttributes( array $params ) { $stat = $this->getFileStat( $params ); - if ( $stat ) { - if ( !isset( $stat['xattr'] ) ) { - // Stat entries filled by file listings don't include metadata/headers - $this->clearCache( [ $params['src'] ] ); - $stat = $this->getFileStat( $params ); - } + // Stat entries filled by file listings don't include metadata/headers + if ( is_array( $stat ) && !isset( $stat['xattr'] ) ) { + $this->clearCache( [ $params['src'] ] ); + $stat = $this->getFileStat( $params ); + } - // @phan-suppress-next-line PhanTypeArraySuspiciousNullable + if ( is_array( $stat ) ) { return $stat['xattr']; - } else { - return false; } + + return ( $stat === self::$RES_ERROR ) ? self::$RES_ERROR : self::$RES_ABSENT; } protected function doGetFileSha1base36( array $params ) { @@ -1062,11 +1073,11 @@ class SwiftFileBackend extends FileBackendStore { $params['requireSHA1'] = true; $stat = $this->getFileStat( $params ); - if ( $stat ) { + if ( is_array( $stat ) ) { return $stat['sha1']; - } else { - return false; } + + return ( $stat === self::$RES_ERROR ) ? self::$RES_ERROR : self::$RES_ABSENT; } protected function doStreamFile( array $params ) { @@ -1136,9 +1147,6 @@ class SwiftFileBackend extends FileBackendStore { } protected function doGetLocalCopyMulti( array $params ) { - /** @var TempFSFile[] $tmpFiles */ - $tmpFiles = []; - $auth = $this->getAuthentication(); $ep = array_diff_key( $params, [ 'srcs' => 1 ] ); // for error logging @@ -1146,56 +1154,62 @@ class SwiftFileBackend extends FileBackendStore { // if the file does not exist. Do not waste time doing file stats here. $reqs = []; // (path => op) + // Initial dummy values to preserve path order + $tmpFiles = array_fill_keys( $params['srcs'], self::$RES_ERROR ); foreach ( $params['srcs'] as $path ) { // each path in this concurrent batch list( $srcCont, $srcRel ) = $this->resolveStoragePathReal( $path ); if ( $srcRel === null || !$auth ) { - $tmpFiles[$path] = null; - continue; + continue; // invalid storage path or auth error } // Get source file extension $ext = FileBackend::extensionFromPath( $path ); // Create a new temporary file... $tmpFile = $this->tmpFileFactory->newTempFSFile( 'localcopy_', $ext ); - if ( $tmpFile ) { - $handle = fopen( $tmpFile->getPath(), 'wb' ); - if ( $handle ) { - $reqs[$path] = [ - 'method' => 'GET', - 'url' => $this->storageUrl( $auth, $srcCont, $srcRel ), - 'headers' => $this->authTokenHeaders( $auth ) - + $this->headersFromParams( $params ), - 'stream' => $handle, - ]; - } else { - $tmpFile = null; - } + $handle = $tmpFile ? fopen( $tmpFile->getPath(), 'wb' ) : false; + if ( $handle ) { + $reqs[$path] = [ + 'method' => 'GET', + 'url' => $this->storageUrl( $auth, $srcCont, $srcRel ), + 'headers' => $this->authTokenHeaders( $auth ) + + $this->headersFromParams( $params ), + 'stream' => $handle, + ]; + $tmpFiles[$path] = $tmpFile; } - $tmpFiles[$path] = $tmpFile; } - $isLatest = ( $this->isRGW || !empty( $params['latest'] ) ); + // Ceph RADOS Gateway is in use (strong consistency) or X-Newest will be used + $latest = ( $this->isRGW || !empty( $params['latest'] ) ); + $opts = [ 'maxConnsPerHost' => $params['concurrency'] ]; $reqs = $this->http->runMulti( $reqs, $opts ); foreach ( $reqs as $path => $op ) { list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $op['response']; fclose( $op['stream'] ); // close open handle if ( $rcode >= 200 && $rcode <= 299 ) { - $size = $tmpFiles[$path] ? $tmpFiles[$path]->getSize() : 0; - // Double check that the disk is not full/broken - if ( $size != $rhdrs['content-length'] ) { - $tmpFiles[$path] = null; + /** @var TempFSFile $tmpFile */ + $tmpFile = $tmpFiles[$path]; + // Make sure that the stream finished and fully wrote to disk + $size = $tmpFile->getSize(); + if ( $size !== (int)$rhdrs['content-length'] ) { + $tmpFiles[$path] = self::$RES_ERROR; $rerr = "Got {$size}/{$rhdrs['content-length']} bytes"; $this->onError( null, __METHOD__, [ 'src' => $path ] + $ep, $rerr, $rcode, $rdesc ); } // Set the file stat process cache in passing $stat = $this->getStatFromHeaders( $rhdrs ); - $stat['latest'] = $isLatest; + $stat['latest'] = $latest; $this->cheapCache->setField( $path, 'stat', $stat ); } elseif ( $rcode === 404 ) { - $tmpFiles[$path] = false; + $tmpFiles[$path] = self::$RES_ABSENT; + $this->cheapCache->setField( + $path, + 'stat', + $latest ? self::$ABSENT_LATEST : self::$ABSENT_NORMAL + ); } else { - $tmpFiles[$path] = null; + $tmpFiles[$path] = self::$RES_ERROR; $this->onError( null, __METHOD__, [ 'src' => $path ] + $ep, $rerr, $rcode, $rdesc ); } @@ -1210,12 +1224,12 @@ class SwiftFileBackend extends FileBackendStore { ) { list( $srcCont, $srcRel ) = $this->resolveStoragePathReal( $params['src'] ); if ( $srcRel === null ) { - return null; // invalid path + return self::TEMPURL_ERROR; // invalid path } $auth = $this->getAuthentication(); if ( !$auth ) { - return null; + return self::TEMPURL_ERROR; } $ttl = $params['ttl'] ?? 86400; @@ -1255,7 +1269,7 @@ class SwiftFileBackend extends FileBackendStore { } } - return null; + return self::TEMPURL_ERROR; } protected function directoriesAreVirtual() { @@ -1394,6 +1408,7 @@ class SwiftFileBackend extends FileBackendStore { * @return array|bool|null False on 404, null on failure */ protected function getContainerStat( $container, $bypassCache = false ) { + /** @noinspection PhpUnusedLocalVariableInspection */ $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); if ( $bypassCache ) { // purge cache @@ -1404,7 +1419,7 @@ class SwiftFileBackend extends FileBackendStore { if ( !$this->containerStatCache->hasField( $container, 'stat' ) ) { $auth = $this->getAuthentication(); if ( !$auth ) { - return self::UNKNOWN; + return self::$RES_ERROR; } list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $this->http->run( [ @@ -1425,12 +1440,12 @@ class SwiftFileBackend extends FileBackendStore { $this->setContainerCache( $container, $stat ); // update persistent cache } } elseif ( $rcode === 404 ) { - return false; + return self::$RES_ABSENT; } else { $this->onError( null, __METHOD__, [ 'cont' => $container ], $rerr, $rcode, $rdesc ); - return self::UNKNOWN; + return self::$RES_ERROR; } } @@ -1595,24 +1610,21 @@ class SwiftFileBackend extends FileBackendStore { $auth = $this->getAuthentication(); - $reqs = []; + $reqs = []; // (path => op) + // (a) Check the containers of the paths... foreach ( $params['srcs'] as $path ) { list( $srcCont, $srcRel ) = $this->resolveStoragePathReal( $path ); - if ( $srcRel === null ) { - $stats[$path] = false; - continue; // invalid storage path - } elseif ( !$auth ) { - $stats[$path] = self::UNKNOWN; - continue; + if ( $srcRel === null || !$auth ) { + $stats[$path] = self::$RES_ERROR; + continue; // invalid storage path or auth error } - // (a) Check the container $cstat = $this->getContainerStat( $srcCont ); - if ( $cstat === false ) { - $stats[$path] = false; + if ( $cstat === self::$RES_ABSENT ) { + $stats[$path] = self::$RES_ABSENT; continue; // ok, nothing to do } elseif ( !is_array( $cstat ) ) { - $stats[$path] = self::UNKNOWN; + $stats[$path] = self::$RES_ERROR; continue; } @@ -1623,15 +1635,11 @@ class SwiftFileBackend extends FileBackendStore { ]; } + // (b) Check the files themselves... $opts = [ 'maxConnsPerHost' => $params['concurrency'] ]; $reqs = $this->http->runMulti( $reqs, $opts ); - - foreach ( $params['srcs'] as $path ) { - if ( array_key_exists( $path, $stats ) ) { - continue; // some sort of failure above - } - // (b) Check the file - list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $reqs[$path]['response']; + foreach ( $reqs as $path => $op ) { + list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $op['response']; if ( $rcode === 200 || $rcode === 204 ) { // Update the object if it is missing some headers if ( !empty( $params['requireSHA1'] ) ) { @@ -1643,9 +1651,9 @@ class SwiftFileBackend extends FileBackendStore { $stat['latest'] = true; // strong consistency } } elseif ( $rcode === 404 ) { - $stat = false; + $stat = self::$RES_ABSENT; } else { - $stat = self::UNKNOWN; + $stat = self::$RES_ERROR; $this->onError( null, __METHOD__, $params, $rerr, $rcode, $rdesc ); } $stats[$path] = $stat; diff --git a/includes/libs/filebackend/fileop/CopyFileOp.php b/includes/libs/filebackend/fileop/CopyFileOp.php index 527de6a5e4..59af94473b 100644 --- a/includes/libs/filebackend/fileop/CopyFileOp.php +++ b/includes/libs/filebackend/fileop/CopyFileOp.php @@ -36,8 +36,10 @@ class CopyFileOp extends FileOp { protected function doPrecheck( array &$predicates ) { $status = StatusValue::newGood(); - // Check if the source file exists - if ( !$this->fileExists( $this->params['src'], $predicates ) ) { + + // Check source file existence + $srcExists = $this->fileExists( $this->params['src'], $predicates ); + if ( $srcExists === false ) { if ( $this->getParam( 'ignoreMissingSource' ) ) { $this->doOperation = false; // no-op // Update file existence predicates (cache 404s) @@ -50,10 +52,8 @@ class CopyFileOp extends FileOp { return $status; } - // Check if a file can be placed/changed at the destination - } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) { - $status->fatal( 'backend-fail-usable', $this->params['dst'] ); - $status->fatal( 'backend-fail-copy', $this->params['src'], $this->params['dst'] ); + } elseif ( $srcExists === FileBackend::EXISTENCE_ERROR ) { + $status->fatal( 'backend-fail-stat', $this->params['src'] ); return $status; } diff --git a/includes/libs/filebackend/fileop/CreateFileOp.php b/includes/libs/filebackend/fileop/CreateFileOp.php index f45b055cae..b68b98f669 100644 --- a/includes/libs/filebackend/fileop/CreateFileOp.php +++ b/includes/libs/filebackend/fileop/CreateFileOp.php @@ -34,21 +34,15 @@ class CreateFileOp extends FileOp { protected function doPrecheck( array &$predicates ) { $status = StatusValue::newGood(); - // Check if the source data is too big - if ( strlen( $this->getParam( 'content' ) ) > $this->backend->maxFileSizeInternal() ) { - $status->fatal( 'backend-fail-maxsize', - $this->params['dst'], $this->backend->maxFileSizeInternal() ); - $status->fatal( 'backend-fail-create', $this->params['dst'] ); - return $status; - // Check if a file can be placed/changed at the destination - } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) { - $status->fatal( 'backend-fail-usable', $this->params['dst'] ); - $status->fatal( 'backend-fail-create', $this->params['dst'] ); + // Check if the source data is too big + $maxBytes = $this->backend->maxFileSizeInternal(); + if ( strlen( $this->getParam( 'content' ) ) > $maxBytes ) { + $status->fatal( 'backend-fail-maxsize', $this->params['dst'], $maxBytes ); return $status; } - // Check if destination file exists + // Check if an incompatible destination file exists $status->merge( $this->precheckDestExistence( $predicates ) ); $this->params['dstExists'] = $this->destExists; // see FileBackendStore::setFileCache() if ( $status->isOK() ) { @@ -61,12 +55,14 @@ class CreateFileOp extends FileOp { } protected function doAttempt() { - if ( !$this->overwriteSameCase ) { + if ( $this->overwriteSameCase ) { + $status = StatusValue::newGood(); // nothing to do + } else { // Create the file at the destination - return $this->backend->createInternal( $this->setFlags( $this->params ) ); + $status = $this->backend->createInternal( $this->setFlags( $this->params ) ); } - return StatusValue::newGood(); + return $status; } protected function getSourceSha1Base36() { diff --git a/includes/libs/filebackend/fileop/DeleteFileOp.php b/includes/libs/filebackend/fileop/DeleteFileOp.php index 1047a985ec..3b48881e72 100644 --- a/includes/libs/filebackend/fileop/DeleteFileOp.php +++ b/includes/libs/filebackend/fileop/DeleteFileOp.php @@ -32,8 +32,10 @@ class DeleteFileOp extends FileOp { protected function doPrecheck( array &$predicates ) { $status = StatusValue::newGood(); - // Check if the source file exists - if ( !$this->fileExists( $this->params['src'], $predicates ) ) { + + // Check source file existence + $srcExists = $this->fileExists( $this->params['src'], $predicates ); + if ( $srcExists === false ) { if ( $this->getParam( 'ignoreMissingSource' ) ) { $this->doOperation = false; // no-op // Update file existence predicates (cache 404s) @@ -46,10 +48,8 @@ class DeleteFileOp extends FileOp { 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-delete', $this->params['src'] ); + } elseif ( $srcExists === FileBackend::EXISTENCE_ERROR ) { + $status->fatal( 'backend-fail-stat', $this->params['src'] ); return $status; } diff --git a/includes/libs/filebackend/fileop/DescribeFileOp.php b/includes/libs/filebackend/fileop/DescribeFileOp.php index 0d1e553265..3604b26ae5 100644 --- a/includes/libs/filebackend/fileop/DescribeFileOp.php +++ b/includes/libs/filebackend/fileop/DescribeFileOp.php @@ -32,21 +32,20 @@ class DescribeFileOp extends FileOp { protected function doPrecheck( array &$predicates ) { $status = StatusValue::newGood(); - // Check if the source file exists - if ( !$this->fileExists( $this->params['src'], $predicates ) ) { + + // Check source file existence + $srcExists = $this->fileExists( $this->params['src'], $predicates ); + if ( $srcExists === false ) { $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'] ); + } elseif ( $srcExists === FileBackend::EXISTENCE_ERROR ) { + $status->fatal( 'backend-fail-stat', $this->params['src'] ); return $status; } // Update file existence predicates - $predicates['exists'][$this->params['src']] = - $this->fileExists( $this->params['src'], $predicates ); + $predicates['exists'][$this->params['src']] = $srcExists; $predicates['sha1'][$this->params['src']] = $this->fileSha1( $this->params['src'], $predicates ); diff --git a/includes/libs/filebackend/fileop/FileOp.php b/includes/libs/filebackend/fileop/FileOp.php index 961fdb93b4..a0465880e8 100644 --- a/includes/libs/filebackend/fileop/FileOp.php +++ b/includes/libs/filebackend/fileop/FileOp.php @@ -255,6 +255,18 @@ abstract class FileOp { return StatusValue::newFatal( 'fileop-fail-state', self::STATE_NEW, $this->state ); } $this->state = self::STATE_CHECKED; + + $status = StatusValue::newGood(); + $storagePaths = array_merge( $this->storagePathsRead(), $this->storagePathsChanged() ); + foreach ( array_unique( $storagePaths ) as $storagePath ) { + if ( !$this->backend->isPathUsableInternal( $storagePath ) ) { + $status->fatal( 'backend-fail-usable', $storagePath ); + } + } + if ( !$status->isOK() ) { + return $status; + } + $status = $this->doPrecheck( $predicates ); if ( !$status->isOK() ) { $this->failed = true; @@ -391,6 +403,8 @@ abstract class FileOp { return $status; } + } elseif ( $this->destExists === FileBackend::EXISTENCE_ERROR ) { + $status->fatal( 'backend-fail-stat', $this->params['dst'] ); } return $status; @@ -409,9 +423,12 @@ abstract class FileOp { /** * Check if a file will exist in storage when this operation is attempted * + * Ideally, the file stat entry should already be preloaded via preloadFileStat(). + * Otherwise, this will query the backend. + * * @param string $source Storage path * @param array $predicates - * @return bool + * @return bool|null Whether the file will exist or null on error */ final protected function fileExists( $source, array $predicates ) { if ( isset( $predicates['exists'][$source] ) ) { @@ -424,11 +441,14 @@ abstract class FileOp { } /** - * Get the SHA-1 of a file in storage when this operation is attempted + * Get the SHA-1 hash a file in storage will have when this operation is attempted + * + * Ideally, file the stat entry should already be preloaded via preloadFileStat() and + * the backend tracks hashes as extended attributes. Otherwise, this will query the backend. * * @param string $source Storage path * @param array $predicates - * @return string|bool False on failure + * @return string|bool The SHA-1 hash the file will have or false if non-existent or on error */ final protected function fileSha1( $source, array $predicates ) { if ( isset( $predicates['sha1'][$source] ) ) { diff --git a/includes/libs/filebackend/fileop/MoveFileOp.php b/includes/libs/filebackend/fileop/MoveFileOp.php index 55dca516cd..0a83370db9 100644 --- a/includes/libs/filebackend/fileop/MoveFileOp.php +++ b/includes/libs/filebackend/fileop/MoveFileOp.php @@ -36,8 +36,10 @@ class MoveFileOp extends FileOp { protected function doPrecheck( array &$predicates ) { $status = StatusValue::newGood(); - // Check if the source file exists - if ( !$this->fileExists( $this->params['src'], $predicates ) ) { + + // Check source file existence + $srcExists = $this->fileExists( $this->params['src'], $predicates ); + if ( $srcExists === false ) { if ( $this->getParam( 'ignoreMissingSource' ) ) { $this->doOperation = false; // no-op // Update file existence predicates (cache 404s) @@ -50,14 +52,12 @@ class MoveFileOp extends FileOp { return $status; } - // Check if a file can be placed/changed at the destination - } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) { - $status->fatal( 'backend-fail-usable', $this->params['dst'] ); - $status->fatal( 'backend-fail-move', $this->params['src'], $this->params['dst'] ); + } elseif ( $srcExists === FileBackend::EXISTENCE_ERROR ) { + $status->fatal( 'backend-fail-stat', $this->params['src'] ); return $status; } - // Check if destination file exists + // Check if an incompatible destination file exists $status->merge( $this->precheckDestExistence( $predicates ) ); $this->params['dstExists'] = $this->destExists; // see FileBackendStore::setFileCache() if ( $status->isOK() ) { diff --git a/includes/libs/filebackend/fileop/StoreFileOp.php b/includes/libs/filebackend/fileop/StoreFileOp.php index 5783a822af..b8cfbf6bdf 100644 --- a/includes/libs/filebackend/fileop/StoreFileOp.php +++ b/includes/libs/filebackend/fileop/StoreFileOp.php @@ -38,26 +38,21 @@ class StoreFileOp extends FileOp { protected function doPrecheck( array &$predicates ) { $status = StatusValue::newGood(); - // Check if the source file exists on the file system + + // Check if the source file exists in the file system and is not too big if ( !is_file( $this->params['src'] ) ) { $status->fatal( 'backend-fail-notexists', $this->params['src'] ); return $status; - // Check if the source file is too big - } elseif ( filesize( $this->params['src'] ) > $this->backend->maxFileSizeInternal() ) { - $status->fatal( 'backend-fail-maxsize', - $this->params['dst'], $this->backend->maxFileSizeInternal() ); - $status->fatal( 'backend-fail-store', $this->params['src'], $this->params['dst'] ); - - return $status; - // Check if a file can be placed/changed at the destination - } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) { - $status->fatal( 'backend-fail-usable', $this->params['dst'] ); - $status->fatal( 'backend-fail-store', $this->params['src'], $this->params['dst'] ); + } + // Check if the source file is too big + $maxBytes = $this->backend->maxFileSizeInternal(); + if ( filesize( $this->params['src'] ) > $maxBytes ) { + $status->fatal( 'backend-fail-maxsize', $this->params['dst'], $maxBytes ); return $status; } - // Check if destination file exists + // Check if an incompatible destination file exists $status->merge( $this->precheckDestExistence( $predicates ) ); $this->params['dstExists'] = $this->destExists; // see FileBackendStore::setFileCache() if ( $status->isOK() ) { @@ -70,12 +65,14 @@ class StoreFileOp extends FileOp { } protected function doAttempt() { - if ( !$this->overwriteSameCase ) { + if ( $this->overwriteSameCase ) { + $status = StatusValue::newGood(); // nothing to do + } else { // Store the file at the destination - return $this->backend->storeInternal( $this->setFlags( $this->params ) ); + $status = $this->backend->storeInternal( $this->setFlags( $this->params ) ); } - return StatusValue::newGood(); + return $status; } protected function getSourceSha1Base36() { diff --git a/tests/phpunit/includes/filebackend/FileBackendTest.php b/tests/phpunit/includes/filebackend/FileBackendTest.php index bc3696dc0b..062087da05 100644 --- a/tests/phpunit/includes/filebackend/FileBackendTest.php +++ b/tests/phpunit/includes/filebackend/FileBackendTest.php @@ -293,7 +293,7 @@ class FileBackendTest extends MediaWikiTestCase { $this->assertEquals( $props1, $props2, "Source and destination have the same props ($backendName)." ); - $this->assertBackendPathsConsistent( [ $dest ] ); + $this->assertBackendPathsConsistent( [ $dest ], true ); } public static function provider_testStore() { @@ -318,19 +318,19 @@ class FileBackendTest extends MediaWikiTestCase { /** * @dataProvider provider_testCopy */ - public function testCopy( $op ) { + public function testCopy( $op, $srcContent, $dstContent, $okStatus, $okSyncStatus ) { $this->backend = $this->singleBackend; $this->tearDownFiles(); - $this->doTestCopy( $op ); + $this->doTestCopy( $op, $srcContent, $dstContent, $okStatus, $okSyncStatus ); $this->tearDownFiles(); $this->backend = $this->multiBackend; $this->tearDownFiles(); - $this->doTestCopy( $op ); + $this->doTestCopy( $op, $srcContent, $dstContent, $okStatus, $okSyncStatus ); $this->tearDownFiles(); } - private function doTestCopy( $op ) { + private function doTestCopy( $op, $srcContent, $dstContent, $okStatus, $okSyncStatus ) { $backendName = $this->backendClass(); $source = $op['src']; @@ -338,99 +338,128 @@ class FileBackendTest extends MediaWikiTestCase { $this->prepare( [ 'dir' => dirname( $source ) ] ); $this->prepare( [ '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( [ 0 => true ], $status->success, - "Move from $source to $dest has proper 'success' field in Status ($backendName)." ); - $this->assertEquals( false, $this->backend->fileExists( [ 'src' => $source ] ), - "Source file $source does not exist ($backendName)." ); - $this->assertEquals( false, $this->backend->fileExists( [ 'src' => $dest ] ), - "Destination file $dest does not exist ($backendName)." ); - - return; + if ( is_string( $srcContent ) ) { + $status = $this->backend->create( [ 'content' => $srcContent, 'dst' => $source ] ); + $this->assertGoodStatus( $status, "Creation of $source succeeded ($backendName)." ); } - - $status = $this->backend->doOperation( - [ 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ] ); - $this->assertGoodStatus( $status, - "Creation of file at $source succeeded ($backendName)." ); - - if ( isset( $op['overwrite'] ) || isset( $op['overwriteSame'] ) ) { - $this->backend->copy( $op ); + if ( is_string( $dstContent ) ) { + $status = $this->backend->create( [ 'content' => $dstContent, 'dst' => $dest ] ); + $this->assertGoodStatus( $status, "Creation of $dest succeeded ($backendName)." ); } $status = $this->backend->doOperation( $op ); - $this->assertGoodStatus( $status, - "Copy from $source to $dest succeeded without warnings ($backendName)." ); - $this->assertEquals( true, $status->isOK(), - "Copy from $source to $dest succeeded ($backendName)." ); - $this->assertEquals( [ 0 => true ], $status->success, - "Copy from $source to $dest has proper 'success' field in Status ($backendName)." ); - $this->assertEquals( true, $this->backend->fileExists( [ 'src' => $source ] ), - "Source file $source still exists ($backendName)." ); - $this->assertEquals( true, $this->backend->fileExists( [ 'src' => $dest ] ), - "Destination file $dest exists after copy ($backendName)." ); - - $this->assertEquals( - $this->backend->getFileSize( [ 'src' => $source ] ), - $this->backend->getFileSize( [ 'src' => $dest ] ), - "Destination file $dest has correct size ($backendName)." ); + if ( $okStatus ) { + $this->assertGoodStatus( + $status, + "Copy from $source to $dest succeeded without warnings ($backendName)." ); + $this->assertEquals( true, $status->isOK(), + "Copy from $source to $dest succeeded ($backendName)." ); + $this->assertEquals( [ 0 => true ], $status->success, + "Copy from $source to $dest has proper 'success' field in Status ($backendName)." ); + if ( !is_string( $srcContent ) ) { + $this->assertSame( + is_string( $dstContent ), + $this->backend->fileExists( [ 'src' => $dest ] ), + "Destination file $dest unchanged after no-op copy ($backendName)." ); + $this->assertSame( + $dstContent, + $this->backend->getFileContents( [ 'src' => $dest ] ), + "Destination file $dest unchanged after no-op copy ($backendName)." ); + } else { + $this->assertEquals( + $this->backend->getFileSize( [ 'src' => $source ] ), + $this->backend->getFileSize( [ 'src' => $dest ] ), + "Destination file $dest has correct size ($backendName)." ); + $props1 = $this->backend->getFileProps( [ 'src' => $source ] ); + $props2 = $this->backend->getFileProps( [ 'src' => $dest ] ); + $this->assertEquals( + $props1, + $props2, + "Source and destination have the same props ($backendName)." ); + } + } else { + $this->assertBadStatus( + $status, + "Copy from $source to $dest fails ($backendName)." ); + $this->assertSame( + is_string( $dstContent ), + (bool)$this->backend->fileExists( [ 'src' => $dest ] ), + "Destination file $dest unchanged after failed copy ($backendName)." ); + $this->assertSame( + $dstContent, + $this->backend->getFileContents( [ 'src' => $dest ] ), + "Destination file $dest unchanged after failed copy ($backendName)." ); + } - $props1 = $this->backend->getFileProps( [ 'src' => $source ] ); - $props2 = $this->backend->getFileProps( [ 'src' => $dest ] ); - $this->assertEquals( $props1, $props2, - "Source and destination have the same props ($backendName)." ); + $this->assertSame( + is_string( $srcContent ), + (bool)$this->backend->fileExists( [ 'src' => $source ] ), + "Source file $source unchanged after copy ($backendName)." + ); + $this->assertSame( + $srcContent, + $this->backend->getFileContents( [ 'src' => $source ] ), + "Source file $source unchanged after copy ($backendName)." + ); + if ( is_string( $dstContent ) ) { + $this->assertTrue( + (bool)$this->backend->fileExists( [ 'src' => $dest ] ), + "Destination file $dest exists after copy ($backendName)." ); + } - $this->assertBackendPathsConsistent( [ $source, $dest ] ); + $this->assertBackendPathsConsistent( [ $source, $dest ], $okSyncStatus ); } + /** + * @return array (op, source exists, dest exists, op succeeds, sync check succeeds) + */ public static function provider_testCopy() { $cases = []; $source = self::baseStorePath() . '/unittest-cont1/e/file.txt'; - $dest = self::baseStorePath() . '/unittest-cont2/a/fileMoved.txt'; + $dest = self::baseStorePath() . '/unittest-cont2/a/fileCopied.txt'; + $opBase = [ 'op' => 'copy', 'src' => $source, 'dst' => $dest ]; - $op = [ 'op' => 'copy', 'src' => $source, 'dst' => $dest ]; - $cases[] = [ - $op, // operation - $source, // source - $dest, // dest - ]; + $op = $opBase; + $cases[] = [ $op, 'yyy', false, true, true ]; - $op2 = $op; - $op2['overwrite'] = true; - $cases[] = [ - $op2, // operation - $source, // source - $dest, // dest - ]; + $op = $opBase; + $op['overwrite'] = true; + $cases[] = [ $op, 'yyy', false, true, true ]; - $op2 = $op; - $op2['overwriteSame'] = true; - $cases[] = [ - $op2, // operation - $source, // source - $dest, // dest - ]; + $op = $opBase; + $op['overwrite'] = true; + $cases[] = [ $op, 'yyy', 'xxx', true, true ]; - $op2 = $op; - $op2['ignoreMissingSource'] = true; - $cases[] = [ - $op2, // operation - $source, // source - $dest, // dest - ]; + $op = $opBase; + $op['overwriteSame'] = true; + $cases[] = [ $op, 'yyy', false, true, true ]; - $op2 = $op; - $op2['ignoreMissingSource'] = true; - $cases[] = [ - $op2, // operation - self::baseStorePath() . '/unittest-cont-bad/e/file.txt', // source - $dest, // dest - ]; + $op = $opBase; + $op['overwriteSame'] = true; + $cases[] = [ $op, 'yyy', 'yyy', true, true ]; + + $op = $opBase; + $op['overwriteSame'] = true; + $cases[] = [ $op, 'yyy', 'zzz', false, true ]; + + $op = $opBase; + $op['ignoreMissingSource'] = true; + $cases[] = [ $op, 'xxx', false, true, true ]; + + $op = $opBase; + $op['ignoreMissingSource'] = true; + $cases[] = [ $op, false, false, true, true ]; + + $op = $opBase; + $op['ignoreMissingSource'] = true; + $cases[] = [ $op, false, 'xxx', true, true ]; + + $op = $opBase; + $op['src'] = 'mwstore://wrongbackend/unittest-cont1/e/file.txt'; + $op['ignoreMissingSource'] = true; + $cases[] = [ $op, false, false, false, false ]; return $cases; } @@ -438,19 +467,19 @@ class FileBackendTest extends MediaWikiTestCase { /** * @dataProvider provider_testMove */ - public function testMove( $op ) { + public function testMove( $op, $srcContent, $dstContent, $okStatus, $okSyncStatus ) { $this->backend = $this->singleBackend; $this->tearDownFiles(); - $this->doTestMove( $op ); + $this->doTestMove( $op, $srcContent, $dstContent, $okStatus, $okSyncStatus ); $this->tearDownFiles(); $this->backend = $this->multiBackend; $this->tearDownFiles(); - $this->doTestMove( $op ); + $this->doTestMove( $op, $srcContent, $dstContent, $okStatus, $okSyncStatus ); $this->tearDownFiles(); } - private function doTestMove( $op ) { + private function doTestMove( $op, $srcContent, $dstContent, $okStatus, $okSyncStatus ) { $backendName = $this->backendClass(); $source = $op['src']; @@ -458,100 +487,128 @@ class FileBackendTest extends MediaWikiTestCase { $this->prepare( [ 'dir' => dirname( $source ) ] ); $this->prepare( [ '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( [ 0 => true ], $status->success, - "Move from $source to $dest has proper 'success' field in Status ($backendName)." ); - $this->assertEquals( false, $this->backend->fileExists( [ 'src' => $source ] ), - "Source file $source does not exist ($backendName)." ); - $this->assertEquals( false, $this->backend->fileExists( [ 'src' => $dest ] ), - "Destination file $dest does not exist ($backendName)." ); - - return; + if ( is_string( $srcContent ) ) { + $status = $this->backend->create( [ 'content' => $srcContent, 'dst' => $source ] ); + $this->assertGoodStatus( $status, "Creation of $source succeeded ($backendName)." ); } - - $status = $this->backend->doOperation( - [ 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ] ); - $this->assertGoodStatus( $status, - "Creation of file at $source succeeded ($backendName)." ); - - if ( isset( $op['overwrite'] ) || isset( $op['overwriteSame'] ) ) { - $this->backend->copy( $op ); + if ( is_string( $dstContent ) ) { + $status = $this->backend->create( [ 'content' => $dstContent, 'dst' => $dest ] ); + $this->assertGoodStatus( $status, "Creation of $dest succeeded ($backendName)." ); } + $oldSrcProps = $this->backend->getFileProps( [ 'src' => $source ] ); + $status = $this->backend->doOperation( $op ); - $this->assertGoodStatus( $status, - "Move from $source to $dest succeeded without warnings ($backendName)." ); - $this->assertEquals( true, $status->isOK(), - "Move from $source to $dest succeeded ($backendName)." ); - $this->assertEquals( [ 0 => true ], $status->success, - "Move from $source to $dest has proper 'success' field in Status ($backendName)." ); - $this->assertEquals( false, $this->backend->fileExists( [ 'src' => $source ] ), - "Source file $source does not still exists ($backendName)." ); - $this->assertEquals( true, $this->backend->fileExists( [ 'src' => $dest ] ), - "Destination file $dest exists after move ($backendName)." ); - $this->assertNotEquals( - $this->backend->getFileSize( [ 'src' => $source ] ), - $this->backend->getFileSize( [ 'src' => $dest ] ), - "Destination file $dest has correct size ($backendName)." ); + if ( $okStatus ) { + $this->assertGoodStatus( + $status, + "Move from $source to $dest succeeded without warnings ($backendName)." ); + $this->assertEquals( true, $status->isOK(), + "Move from $source to $dest succeeded ($backendName)." ); + $this->assertEquals( [ 0 => true ], $status->success, + "Move from $source to $dest has proper 'success' field in Status ($backendName)." ); + if ( !is_string( $srcContent ) ) { + $this->assertSame( + is_string( $dstContent ), + $this->backend->fileExists( [ 'src' => $dest ] ), + "Destination file $dest unchanged after no-op move ($backendName)." ); + $this->assertSame( + $dstContent, + $this->backend->getFileContents( [ 'src' => $dest ] ), + "Destination file $dest unchanged after no-op move ($backendName)." ); + } else { + $this->assertEquals( + $this->backend->getFileSize( [ 'src' => $dest ] ), + strlen( $srcContent ), + "Destination file $dest has correct size ($backendName)." ); + $this->assertEquals( + $oldSrcProps, + $this->backend->getFileProps( [ 'src' => $dest ] ), + "Source and destination have the same props ($backendName)." ); + } + } else { + $this->assertBadStatus( + $status, + "Move from $source to $dest fails ($backendName)." ); + $this->assertSame( + is_string( $dstContent ), + (bool)$this->backend->fileExists( [ 'src' => $dest ] ), + "Destination file $dest unchanged after failed move ($backendName)." ); + $this->assertSame( + $dstContent, + $this->backend->getFileContents( [ 'src' => $dest ] ), + "Destination file $dest unchanged after failed move ($backendName)." ); + $this->assertSame( + is_string( $srcContent ), + (bool)$this->backend->fileExists( [ 'src' => $source ] ), + "Source file $source unchanged after failed move ($backendName)." + ); + $this->assertSame( + $srcContent, + $this->backend->getFileContents( [ 'src' => $source ] ), + "Source file $source unchanged after failed move ($backendName)." + ); + } - $props1 = $this->backend->getFileProps( [ 'src' => $source ] ); - $props2 = $this->backend->getFileProps( [ 'src' => $dest ] ); - $this->assertEquals( false, $props1['fileExists'], - "Source file does not exist accourding to props ($backendName)." ); - $this->assertEquals( true, $props2['fileExists'], - "Destination file exists accourding to props ($backendName)." ); + if ( is_string( $dstContent ) ) { + $this->assertTrue( + (bool)$this->backend->fileExists( [ 'src' => $dest ] ), + "Destination file $dest exists after move ($backendName)." ); + } - $this->assertBackendPathsConsistent( [ $source, $dest ] ); + $this->assertBackendPathsConsistent( [ $source, $dest ], $okSyncStatus ); } + /** + * @return array (op, source exists, dest exists, op succeeds, sync check succeeds) + */ public static function provider_testMove() { $cases = []; $source = self::baseStorePath() . '/unittest-cont1/e/file.txt'; $dest = self::baseStorePath() . '/unittest-cont2/a/fileMoved.txt'; + $opBase = [ 'op' => 'move', 'src' => $source, 'dst' => $dest ]; - $op = [ 'op' => 'move', 'src' => $source, 'dst' => $dest ]; - $cases[] = [ - $op, // operation - $source, // source - $dest, // dest - ]; + $op = $opBase; + $cases[] = [ $op, 'yyy', false, true, true ]; - $op2 = $op; - $op2['overwrite'] = true; - $cases[] = [ - $op2, // operation - $source, // source - $dest, // dest - ]; + $op = $opBase; + $op['overwrite'] = true; + $cases[] = [ $op, 'yyy', false, true, true ]; - $op2 = $op; - $op2['overwriteSame'] = true; - $cases[] = [ - $op2, // operation - $source, // source - $dest, // dest - ]; + $op = $opBase; + $op['overwrite'] = true; + $cases[] = [ $op, 'yyy', 'xxx', true, true ]; - $op2 = $op; - $op2['ignoreMissingSource'] = true; - $cases[] = [ - $op2, // operation - $source, // source - $dest, // dest - ]; + $op = $opBase; + $op['overwriteSame'] = true; + $cases[] = [ $op, 'yyy', false, true, true ]; - $op2 = $op; - $op2['ignoreMissingSource'] = true; - $cases[] = [ - $op2, // operation - self::baseStorePath() . '/unittest-cont-bad/e/file.txt', // source - $dest, // dest - ]; + $op = $opBase; + $op['overwriteSame'] = true; + $cases[] = [ $op, 'yyy', 'yyy', true, true ]; + + $op = $opBase; + $op['overwriteSame'] = true; + $cases[] = [ $op, 'yyy', 'zzz', false, true ]; + + $op = $opBase; + $op['ignoreMissingSource'] = true; + $cases[] = [ $op, 'xxx', false, true, true ]; + + $op = $opBase; + $op['ignoreMissingSource'] = true; + $cases[] = [ $op, false, false, true, true ]; + + $op = $opBase; + $op['ignoreMissingSource'] = true; + $cases[] = [ $op, false, 'xxx', true, true ]; + + $op = $opBase; + $op['src'] = 'mwstore://wrongbackend/unittest-cont1/e/file.txt'; + $op['ignoreMissingSource'] = true; + $cases[] = [ $op, false, false, false, false ]; return $cases; } @@ -559,27 +616,27 @@ class FileBackendTest extends MediaWikiTestCase { /** * @dataProvider provider_testDelete */ - public function testDelete( $op, $withSource, $okStatus ) { + public function testDelete( $op, $srcContent, $okStatus, $okSyncStatus ) { $this->backend = $this->singleBackend; $this->tearDownFiles(); - $this->doTestDelete( $op, $withSource, $okStatus ); + $this->doTestDelete( $op, $srcContent, $okStatus, $okSyncStatus ); $this->tearDownFiles(); $this->backend = $this->multiBackend; $this->tearDownFiles(); - $this->doTestDelete( $op, $withSource, $okStatus ); + $this->doTestDelete( $op, $srcContent, $okStatus, $okSyncStatus ); $this->tearDownFiles(); } - private function doTestDelete( $op, $withSource, $okStatus ) { + private function doTestDelete( $op, $srcContent, $okStatus, $okSyncStatus ) { $backendName = $this->backendClass(); $source = $op['src']; $this->prepare( [ 'dir' => dirname( $source ) ] ); - if ( $withSource ) { + if ( is_string( $srcContent ) ) { $status = $this->backend->doOperation( - [ 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ] ); + [ 'op' => 'create', 'content' => $srcContent, 'dst' => $source ] ); $this->assertGoodStatus( $status, "Creation of file at $source succeeded ($backendName)." ); } @@ -597,7 +654,8 @@ class FileBackendTest extends MediaWikiTestCase { "Deletion of file at $source failed ($backendName)." ); } - $this->assertEquals( false, $this->backend->fileExists( [ 'src' => $source ] ), + $this->assertFalse( + (bool)$this->backend->fileExists( [ 'src' => $source ] ), "Source file $source does not exist after move ($backendName)." ); $this->assertFalse( @@ -605,44 +663,40 @@ class FileBackendTest extends MediaWikiTestCase { "Source file $source has correct size (false) ($backendName)." ); $props1 = $this->backend->getFileProps( [ 'src' => $source ] ); - $this->assertFalse( $props1['fileExists'], + $this->assertFalse( + $props1['fileExists'], "Source file $source does not exist according to props ($backendName)." ); - $this->assertBackendPathsConsistent( [ $source ] ); + $this->assertBackendPathsConsistent( [ $source ], $okSyncStatus ); } + /** + * @return array (op, source content, op succeeds, sync check succeeds) + */ public static function provider_testDelete() { $cases = []; $source = self::baseStorePath() . '/unittest-cont1/e/myfacefile.txt'; + $baseOp = [ 'op' => 'delete', 'src' => $source ]; - $op = [ 'op' => 'delete', 'src' => $source ]; - $cases[] = [ - $op, // operation - true, // with source - true // succeeds - ]; + $op = $baseOp; + $cases[] = [ $op, 'xxx', true, true ]; - $cases[] = [ - $op, // operation - false, // without source - false // fails - ]; + $op = $baseOp; + $op['ignoreMissingSource'] = true; + $cases[] = [ $op, 'xxx', true, true ]; + + $op = $baseOp; + $cases[] = [ $op, false, false, true ]; + $op = $baseOp; $op['ignoreMissingSource'] = true; - $cases[] = [ - $op, // operation - false, // without source - true // succeeds - ]; + $cases[] = [ $op, false, true, true ]; + $op = $baseOp; $op['ignoreMissingSource'] = true; - $op['src'] = self::baseStorePath() . '/unittest-cont-bad/e/file.txt'; - $cases[] = [ - $op, // operation - false, // without source - true // succeeds - ]; + $op['src'] = 'mwstore://wrongbackend/unittest-cont1/e/file.txt'; + $cases[] = [ $op, false, false, false ]; return $cases; } @@ -708,7 +762,7 @@ class FileBackendTest extends MediaWikiTestCase { "Describe of file at $source failed ($backendName)." ); } - $this->assertBackendPathsConsistent( [ $source ] ); + $this->assertBackendPathsConsistent( [ $source ], true ); } private function assertHasHeaders( array $headers, array $attr ) { @@ -809,7 +863,7 @@ class FileBackendTest extends MediaWikiTestCase { "Destination file $dest has original size according to props ($backendName)." ); } - $this->assertBackendPathsConsistent( [ $dest ] ); + $this->assertBackendPathsConsistent( [ $dest ], true ); } /** @@ -2608,7 +2662,7 @@ class FileBackendTest extends MediaWikiTestCase { } function tearDownFiles() { - $containers = [ 'unittest-cont1', 'unittest-cont2', 'unittest-cont-bad' ]; + $containers = [ 'unittest-cont1', 'unittest-cont2' ]; foreach ( $containers as $container ) { $this->deleteFiles( $container ); } @@ -2627,14 +2681,24 @@ class FileBackendTest extends MediaWikiTestCase { $this->backend->clean( [ 'dir' => "$base/$container", 'recursive' => 1 ] ); } - function assertBackendPathsConsistent( array $paths ) { - if ( $this->backend instanceof FileBackendMultiWrite ) { - $status = $this->backend->consistencyCheck( $paths ); + private function assertBackendPathsConsistent( array $paths, $okSyncStatus ) { + if ( !$this->backend instanceof FileBackendMultiWrite ) { + return; + } + + $status = $this->backend->consistencyCheck( $paths ); + if ( $okSyncStatus ) { $this->assertGoodStatus( $status, "Files synced: " . implode( ',', $paths ) ); + } else { + $this->assertBadStatus( $status, "Files not synced: " . implode( ',', $paths ) ); } } - function assertGoodStatus( StatusValue $status, $msg ) { + private function assertGoodStatus( StatusValue $status, $msg ) { $this->assertEquals( print_r( [], 1 ), print_r( $status->getErrors(), 1 ), $msg ); } + + private function assertBadStatus( StatusValue $status, $msg ) { + $this->assertNotEquals( print_r( [], 1 ), print_r( $status->getErrors(), 1 ), $msg ); + } } -- 2.20.1