From 6dc02229b446aa24b0f3a222cdd9a939eb2d8292 Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 3 Mar 2026 10:11:46 -0500 Subject: [PATCH 1/5] refactor(files_external/s3): move existence check to top in touch - Earlier return and clearer intent. - $this->file_exists() never throws S3Exception so was pointless. - Also added comment about what's really going on since returning false seemed wrong here at first Signed-off-by: Josh --- apps/files_external/lib/Lib/Storage/AmazonS3.php | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index fdcbf33627e17..932b8a9bbc822 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -450,6 +450,17 @@ public function fopen(string $path, string $mode) { } public function touch(string $path, ?int $mtime = null): bool { + if ($this->file_exists($path)) { + // If the object already exists, return false so the higher filesystem layer + // (View::touch()) can emulate touch by updating the cached mtime. + // This avoids an extra remote request against S3 and improves performance. + // + // Note: this does not change the object's native LastModified timestamp in S3. + // External consumers that only observe S3 metadata (not Nextcloud's cache) will + // not see the updated mtime. + return false; + } + if (is_null($mtime)) { $mtime = time(); } @@ -458,10 +469,6 @@ public function touch(string $path, ?int $mtime = null): bool { ]; try { - if ($this->file_exists($path)) { - return false; - } - $mimeType = $this->mimeDetector->detectPath($path); $this->getConnection()->putObject([ 'Bucket' => $this->bucket, From 4c365081b17cbabc7ef4966cab9883f0be8097e8 Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 3 Mar 2026 10:53:40 -0500 Subject: [PATCH 2/5] refactor(files_external/s3): tidy up mtime code (clarity change only) Signed-off-by: Josh --- .../files_external/lib/Lib/Storage/AmazonS3.php | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index 932b8a9bbc822..c70b3be962383 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -461,21 +461,22 @@ public function touch(string $path, ?int $mtime = null): bool { return false; } - if (is_null($mtime)) { + if ($mtime === null) { $mtime = time(); } - $metadata = [ - 'lastmodified' => gmdate(\DateTime::RFC1123, $mtime) - ]; try { $mimeType = $this->mimeDetector->detectPath($path); + $lastModified = gmdate(\DateTime::RFC1123, $mtime); + $this->getConnection()->putObject([ 'Bucket' => $this->bucket, 'Key' => $this->cleanKey($path), - 'Metadata' => $metadata, 'Body' => '', 'ContentType' => $mimeType, + 'Metadata' => [ + 'lastmodified' => $lastModified, + ], 'MetadataDirective' => 'REPLACE', ] + $this->getSSECParameters()); $this->testTimeout(); @@ -489,6 +490,12 @@ public function touch(string $path, ?int $mtime = null): bool { $this->invalidateCache($path); return true; + // NOTES/WIP: + // - Not sure we actually refer to the Metadata field's `lastmodified` value ever (we seem to use the S3 LastModified) + // - When $mtime = null, perhaps just rely on S3's automatic LastModified field? + // - mtime handling possibly inconsistent with other remote storages (i.e. SMB) - TBD + // - why isn't $this->normalizePath utilized? also cleanKey usage + // - Is the MetadataDirective value correct? } public function copy(string $source, string $target, ?bool $isFile = null): bool { From 5f6d3f206e699b669a150ca82d975050efe02e20 Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 3 Mar 2026 10:59:56 -0500 Subject: [PATCH 3/5] refactor(files_external/s3): drop unused MetadataDirective in touch This parameter is only used for copyObject not putObject. Signed-off-by: Josh --- apps/files_external/lib/Lib/Storage/AmazonS3.php | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index c70b3be962383..9cfb3e80bd2f7 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -477,7 +477,6 @@ public function touch(string $path, ?int $mtime = null): bool { 'Metadata' => [ 'lastmodified' => $lastModified, ], - 'MetadataDirective' => 'REPLACE', ] + $this->getSSECParameters()); $this->testTimeout(); } catch (S3Exception $e) { From 813ba1b912c9b0e59a1f98031c613c2ef8581051 Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 3 Mar 2026 12:08:59 -0500 Subject: [PATCH 4/5] fix(files_external/s3): normalize touch path for consistent key handling Signed-off-by: Josh --- apps/files_external/lib/Lib/Storage/AmazonS3.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index 9cfb3e80bd2f7..6786f22624374 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -450,6 +450,8 @@ public function fopen(string $path, string $mode) { } public function touch(string $path, ?int $mtime = null): bool { + $path = $this->normalizePath($path); + if ($this->file_exists($path)) { // If the object already exists, return false so the higher filesystem layer // (View::touch()) can emulate touch by updating the cached mtime. @@ -468,10 +470,11 @@ public function touch(string $path, ?int $mtime = null): bool { try { $mimeType = $this->mimeDetector->detectPath($path); $lastModified = gmdate(\DateTime::RFC1123, $mtime); + $key = $this->cleanKey($path); $this->getConnection()->putObject([ 'Bucket' => $this->bucket, - 'Key' => $this->cleanKey($path), + 'Key' => $key, 'Body' => '', 'ContentType' => $mimeType, 'Metadata' => [ @@ -494,7 +497,7 @@ public function touch(string $path, ?int $mtime = null): bool { // - When $mtime = null, perhaps just rely on S3's automatic LastModified field? // - mtime handling possibly inconsistent with other remote storages (i.e. SMB) - TBD // - why isn't $this->normalizePath utilized? also cleanKey usage - // - Is the MetadataDirective value correct? + // - mtime range validation guard? } public function copy(string $source, string $target, ?bool $isFile = null): bool { From aea1e47d0c50eda5722a9d712e3a151c4ae8e9e0 Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 3 Mar 2026 12:42:29 -0500 Subject: [PATCH 5/5] docs(files_external/s3): clarify non-null mtime handling for new objects Signed-off-by: Josh --- apps/files_external/lib/Lib/Storage/AmazonS3.php | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index 6786f22624374..5e9e50ad70123 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -468,10 +468,15 @@ public function touch(string $path, ?int $mtime = null): bool { } try { + $key = $this->cleanKey($path); $mimeType = $this->mimeDetector->detectPath($path); $lastModified = gmdate(\DateTime::RFC1123, $mtime); - $key = $this->cleanKey($path); + // When creating a missing object via touch() and the caller provided a non-null $mtime, + // upper layers (View::touch()) will not emulate mtime (because we return true here). + // As a result, the cached mtime will reflect S3's LastModified (write time), not the + // requested $mtime. The custom 'lastmodified' metadata is currently not read by this + // storage when building stat()/filecache entries. $this->getConnection()->putObject([ 'Bucket' => $this->bucket, 'Key' => $key, @@ -492,12 +497,6 @@ public function touch(string $path, ?int $mtime = null): bool { $this->invalidateCache($path); return true; - // NOTES/WIP: - // - Not sure we actually refer to the Metadata field's `lastmodified` value ever (we seem to use the S3 LastModified) - // - When $mtime = null, perhaps just rely on S3's automatic LastModified field? - // - mtime handling possibly inconsistent with other remote storages (i.e. SMB) - TBD - // - why isn't $this->normalizePath utilized? also cleanKey usage - // - mtime range validation guard? } public function copy(string $source, string $target, ?bool $isFile = null): bool {