filebackend: add idiom constant to FileBackend for null results
authorAaron Schulz <aschulz@wikimedia.org>
Thu, 29 Aug 2019 08:14:24 +0000 (01:14 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Thu, 29 Aug 2019 08:19:33 +0000 (01:19 -0700)
Change-Id: I65f043e87d82192c13d3627fb45ef222f0290bf8

includes/libs/filebackend/FSFileBackend.php
includes/libs/filebackend/FileBackend.php
includes/libs/filebackend/FileBackendStore.php
includes/libs/filebackend/MemoryFileBackend.php
includes/libs/filebackend/SwiftFileBackend.php

index c05dc28..c1a796f 100644 (file)
@@ -593,7 +593,7 @@ class FSFileBackend extends FileBackendStore {
                } elseif ( !$hadError ) {
                        return false; // file does not exist
                } else {
-                       return null; // failure
+                       return self::UNKNOWN; // failure
                }
        }
 
@@ -610,7 +610,7 @@ class FSFileBackend extends FileBackendStore {
                $exists = is_dir( $dir );
                $hadError = $this->untrapWarnings();
 
-               return $hadError ? null : $exists;
+               return $hadError ? self::UNKNOWN : $exists;
        }
 
        /**
@@ -632,7 +632,7 @@ class FSFileBackend extends FileBackendStore {
                } elseif ( !is_readable( $dir ) ) {
                        $this->logger->warning( __METHOD__ . "() given directory is unreadable: '$dir'\n" );
 
-                       return null; // bad permissions?
+                       return self::UNKNOWN; // bad permissions?
                }
 
                return new FSFileBackendDirList( $dir, $params );
@@ -657,7 +657,7 @@ class FSFileBackend extends FileBackendStore {
                } elseif ( !is_readable( $dir ) ) {
                        $this->logger->warning( __METHOD__ . "() given directory is unreadable: '$dir'\n" );
 
-                       return null; // bad permissions?
+                       return self::UNKNOWN; // bad permissions?
                }
 
                return new FSFileBackendFileList( $dir, $params );
index 4cacb7a..72200a8 100644 (file)
@@ -131,6 +131,9 @@ 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;
+
        /**
         * Create a new backend instance from configuration.
         * This should only be called from within FileBackendGroup.
@@ -209,7 +212,8 @@ abstract class FileBackend implements LoggerAwareInterface {
        }
 
        /**
-        * Get the unique backend name.
+        * Get the unique backend name
+        *
         * We may have multiple different backends of the same type.
         * For example, we can have two Swift backends using different proxies.
         *
@@ -231,6 +235,7 @@ abstract class FileBackend implements LoggerAwareInterface {
 
        /**
         * Alias to getDomainId()
+        *
         * @return string
         * @since 1.20
         * @deprecated Since 1.34 Use getDomainId()
@@ -1164,21 +1169,34 @@ abstract class FileBackend implements LoggerAwareInterface {
        abstract public function getFileHttpUrl( array $params );
 
        /**
-        * Check if a directory exists at a given storage path.
-        * Backends using key/value stores will check if the path is a
-        * virtual directory, meaning there are files under the given directory.
+        * Check if a directory exists at a given storage path
+        *
+        * For backends using key/value stores, a directory is said to exist whenever
+        * there exist any files with paths using the given directory path as a prefix
+        * followed by a forward slash. For example, if there is a file called
+        * "mwstore://backend/container/dir/path.svg" then directories are said to exist
+        * at "mwstore://backend/container" and "mwstore://backend/container/dir". These
+        * can be thought of as "virtual" directories.
+        *
+        * Backends that directly use a filesystem layer might enumerate empty directories.
+        * The clean() method should always be used when files are deleted or moved if this
+        * is a concern. This is a trade-off to avoid write amplication/contention on file
+        * changes or read amplification when calling this method.
         *
         * Storage backends with eventual consistency might return stale data.
         *
+        * @see FileBackend::clean()
+        *
         * @param array $params Parameters include:
         *   - dir : storage directory
-        * @return bool|null Returns null on failure
+        * @return bool|null Whether a directory exists or null on failure
         * @since 1.20
         */
        abstract public function directoryExists( array $params );
 
        /**
-        * Get an iterator to list *all* directories under a storage directory.
+        * Get an iterator to list *all* directories under a storage directory
+        *
         * If the directory is of the form "mwstore://backend/container",
         * then all directories in the container will be listed.
         * If the directory is of form "mwstore://backend/container/dir",
@@ -1189,10 +1207,12 @@ abstract class FileBackend implements LoggerAwareInterface {
         *
         * Failures during iteration can result in FileBackendError exceptions (since 1.22).
         *
+        * @see FileBackend::directoryExists()
+        *
         * @param array $params Parameters include:
         *   - dir     : storage directory
         *   - topOnly : only return direct child dirs of the directory
-        * @return Traversable|array|null Returns null on failure
+        * @return Traversable|array|null Directory list enumerator null on failure
         * @since 1.20
         */
        abstract public function getDirectoryList( array $params );
@@ -1205,9 +1225,11 @@ abstract class FileBackend implements LoggerAwareInterface {
         *
         * Failures during iteration can result in FileBackendError exceptions (since 1.22).
         *
+        * @see FileBackend::directoryExists()
+        *
         * @param array $params Parameters include:
         *   - dir : storage directory
-        * @return Traversable|array|null Returns null on failure
+        * @return Traversable|array|null Directory list enumerator or null on failure
         * @since 1.20
         */
        final public function getTopDirectoryList( array $params ) {
@@ -1215,12 +1237,12 @@ abstract class FileBackend implements LoggerAwareInterface {
        }
 
        /**
-        * Get an iterator to list *all* stored files under a storage directory.
-        * If the directory is of the form "mwstore://backend/container",
-        * then all files in the container will be listed.
-        * If the directory is of form "mwstore://backend/container/dir",
-        * then all files under that directory will be listed.
-        * Results will be storage paths relative to the given directory.
+        * Get an iterator to list *all* stored files under a storage directory
+        *
+        * If the directory is of the form "mwstore://backend/container", then all
+        * files in the container will be listed. If the directory is of form
+        * "mwstore://backend/container/dir", then all files under that directory will
+        * be listed. Results will be storage paths relative to the given directory.
         *
         * Storage backends with eventual consistency might return stale data.
         *
@@ -1230,7 +1252,7 @@ abstract class FileBackend implements LoggerAwareInterface {
         *   - 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 Returns null on failure
+        * @return Traversable|array|null File list enumerator or null on failure
         */
        abstract public function getFileList( array $params );
 
@@ -1245,7 +1267,7 @@ abstract class FileBackend implements LoggerAwareInterface {
         * @param array $params Parameters include:
         *   - dir        : storage directory
         *   - adviseStat : set to true if stat requests will be made on the files (since 1.22)
-        * @return Traversable|array|null Returns null on failure
+        * @return Traversable|array|null File list enumerator or null on failure
         * @since 1.20
         */
        final public function getTopFileList( array $params ) {
@@ -1283,7 +1305,7 @@ abstract class FileBackend implements LoggerAwareInterface {
         * @param array $params Parameters include:
         *   - srcs        : list of source storage paths
         *   - latest      : use the latest available data
-        * @return bool All requests proceeded without I/O errors (since 1.24)
+        * @return bool Whether all requests proceeded without I/O errors (since 1.24)
         * @since 1.23
         */
        abstract public function preloadFileStat( array $params );
index aa95ee4..9b901dd 100644 (file)
@@ -604,7 +604,7 @@ abstract class FileBackendStore extends FileBackend {
                $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" );
                $stat = $this->getFileStat( $params );
 
-               return ( $stat === null ) ? null : (bool)$stat; // null => failure
+               return ( $stat === self::UNKNOWN ) ? self::UNKNOWN : (bool)$stat;
        }
 
        final public function getFileTimestamp( array $params ) {
@@ -637,7 +637,7 @@ abstract class FileBackendStore extends FileBackend {
                        // 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 === null ||
+                               $stat === self::UNKNOWN ||
                                ( $requireSHA1 && is_array( $stat ) && !isset( $stat['sha1'] ) )
                        ) {
                                $this->primeFileCache( [ $path ] ); // check persistent cache
@@ -936,7 +936,7 @@ abstract class FileBackendStore extends FileBackend {
                                        $res = true;
                                        break; // found one!
                                } elseif ( $exists === null ) { // error?
-                                       $res = null; // if we don't find anything, it is indeterminate
+                                       $res = self::UNKNOWN; // if we don't find anything, it is indeterminate
                                }
                        }
 
@@ -957,7 +957,7 @@ 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 null;
+                       return self::UNKNOWN;
                }
                if ( $shard !== null ) {
                        // File listing is confined to a single container/shard
@@ -987,7 +987,7 @@ abstract class FileBackendStore extends FileBackend {
        final public function getFileList( array $params ) {
                list( $fullCont, $dir, $shard ) = $this->resolveStoragePath( $params['dir'] );
                if ( $dir === null ) { // invalid storage path
-                       return null;
+                       return self::UNKNOWN;
                }
                if ( $shard !== null ) {
                        // File listing is confined to a single container/shard
index f0cbf3b..88b281e 100644 (file)
@@ -148,7 +148,7 @@ class MemoryFileBackend extends FileBackendStore {
        protected function doGetFileStat( array $params ) {
                $src = $this->resolveHashKey( $params['src'] );
                if ( $src === null ) {
-                       return null;
+                       return false; // invalid path
                }
 
                if ( isset( $this->files[$src] ) ) {
index afd1688..e576c64 100644 (file)
@@ -593,7 +593,7 @@ class SwiftFileBackend extends FileBackendStore {
                $stat = $this->getContainerStat( $fullCont );
                if ( is_array( $stat ) ) {
                        return $status; // already there
-               } elseif ( $stat === null ) {
+               } elseif ( $stat === self::UNKNOWN ) {
                        $status->fatal( 'backend-fail-internal', $this->name );
                        $this->logger->error( __METHOD__ . ': cannot get container stat' );
 
@@ -832,7 +832,7 @@ class SwiftFileBackend extends FileBackendStore {
                        return ( count( $status->value ) ) > 0;
                }
 
-               return null; // error
+               return self::UNKNOWN; // error
        }
 
        /**
@@ -1401,7 +1401,7 @@ class SwiftFileBackend extends FileBackendStore {
                if ( !$this->containerStatCache->hasField( $container, 'stat' ) ) {
                        $auth = $this->getAuthentication();
                        if ( !$auth ) {
-                               return null;
+                               return self::UNKNOWN;
                        }
 
                        list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $this->http->run( [
@@ -1427,7 +1427,7 @@ class SwiftFileBackend extends FileBackendStore {
                                $this->onError( null, __METHOD__,
                                        [ 'cont' => $container ], $rerr, $rcode, $rdesc );
 
-                               return null;
+                               return self::UNKNOWN;
                        }
                }
 
@@ -1599,7 +1599,7 @@ class SwiftFileBackend extends FileBackendStore {
                                $stats[$path] = false;
                                continue; // invalid storage path
                        } elseif ( !$auth ) {
-                               $stats[$path] = null;
+                               $stats[$path] = self::UNKNOWN;
                                continue;
                        }
 
@@ -1609,7 +1609,7 @@ class SwiftFileBackend extends FileBackendStore {
                                $stats[$path] = false;
                                continue; // ok, nothing to do
                        } elseif ( !is_array( $cstat ) ) {
-                               $stats[$path] = null;
+                               $stats[$path] = self::UNKNOWN;
                                continue;
                        }
 
@@ -1642,7 +1642,7 @@ class SwiftFileBackend extends FileBackendStore {
                        } elseif ( $rcode === 404 ) {
                                $stat = false;
                        } else {
-                               $stat = null;
+                               $stat = self::UNKNOWN;
                                $this->onError( null, __METHOD__, $params, $rerr, $rcode, $rdesc );
                        }
                        $stats[$path] = $stat;