WIP: refactor(files_external/s3): improve doesDirectoryExist() code clarity

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 {