diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index 4da56c9527d9b..d2cdf048640bd 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -135,54 +135,71 @@ private function headObject(string $key): array|false { /** * Return true if directory exists * - * There are no folders in s3. A folder like structure could be archived - * by prefixing files with the folder name. + * There are no folders in S3. A folder-like structure is represented either + * by object keys sharing a prefix, or by an explicit directory marker object. * - * Implementation from flysystem-aws-s3-v3: - * https://github.com/thephpleague/flysystem-aws-s3-v3/blob/8241e9cc5b28f981e0d24cdaf9867f14c7498ae4/src/AwsS3Adapter.php#L670-L694 - * - * @throws \Exception + * Implementation inspired by https://github.com/thephpleague/flysystem-aws-s3-v3/ + * + * @throws S3Exception */ private function doesDirectoryExist(string $path): bool { - if ($path === '.' || $path === '') { + if ($path === '.' || $path === '') { // THE root path always exists return true; } - $path = rtrim($path, '/') . '/'; - if (isset($this->directoryCache[$path])) { + $path = rtrim($path, '/') . '/'; // normalize to S3 directory prefix representation + + if (isset($this->directoryCache[$path])) { // cache hit return $this->directoryCache[$path]; } + try { - // Maybe this isn't an actual key, but a prefix. - // Do a prefix listing of objects to determine. + // Check for any objects with the prefix ("$path/*"). + // Returns both "non-empty" directories and "empty" directory markers in most cases. $result = $this->getConnection()->listObjectsV2([ 'Bucket' => $this->bucket, 'Prefix' => $path, 'MaxKeys' => 1, ]); - if (isset($result['Contents'])) { - $this->directoryCache[$path] = true; - return true; + $hasPrefixedObjects = !empty($result['Contents']); + if ($hasPrefixedObjects) { + return $this->cacheDirectoryExists($path, true); } - // empty directories have their own object - $object = $this->headObject($path); - - if ($object) { - $this->directoryCache[$path] = true; - return true; + // Check for an object with the exact key ("$path/"). + // Fallback for edge cases where the "empty" directory marker wasn't returned above for some reason. + // In practice this fallback (probably) rarely needed, but we call it anyway (impacting performance). + // @todo: determine what scenarios this fallback is really needed and consider removing or at least + // turning off by default (gating it). + $directoryMarker = $this->headObject($path); + if ($directoryMarker) { + return $this->cacheDirectoryExists($path, true); } + + // Not found + return $this->cacheDirectoryExists($path, false); } catch (S3Exception $e) { if ($e->getStatusCode() >= 400 && $e->getStatusCode() < 500) { + // Treat client-side 4xx as non-existing for cache purposes. + // @todo: this may be incorrect; still rethrows 4xx after caching false, but doesn't return false. $this->directoryCache[$path] = false; } throw $e; } + // CONCERNS/Checklist: + // - handling of different error scenarios from listObjectsV2() + // - handling of non-404s from headObject() + // - necessity / default use of headObject() fallback (performance hit; unnecessary with AWS and probably most others...) + // - caching interaction (short-lived -- user facing tasks) + // - caching interaction (long-lived -- e.g. scans/non-user tasks) + // - common root checks and path & prefix normalization (throughout class) + } - $this->directoryCache[$path] = false; - return false; + private function cacheDirectoryExists(string $path, bool $exists): bool { + $this->directoryCache[$path] = $exists; + return $exists; } protected function remove(string $path): bool {