Skip to content

Commit 960f3b4

Browse files
AntreesyAntreesy
committed
test: add regression coverage for S3 folder mtime handling
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
1 parent d317f45 commit 960f3b4

2 files changed

Lines changed: 130 additions & 2 deletions

File tree

‎apps/files_external/tests/Storage/Amazons3Test.php‎

Lines changed: 80 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
*/
99
namespace OCA\Files_External\Tests\Storage;
1010

11+
use OC\Files\Cache\Scanner;
1112
use OCA\Files_External\Lib\Storage\AmazonS3;
1213

1314
/**
@@ -38,7 +39,84 @@ protected function tearDown(): void {
3839
parent::tearDown();
3940
}
4041

41-
public function testStat(): void {
42-
$this->markTestSkipped('S3 doesn\'t update the parents folder mtime');
42+
/**
43+
* Regression test for the '.' vs '' root path mismatch in getDirectoryMetaData.
44+
*
45+
* normalizePath('') returns '.' for S3 object keys, but the filecache stores the
46+
* storage root under the key ''. Before the fix, getCache()->get('.') returned false,
47+
* causing getDirectoryMetaData to return a fabricated time() on every call, which
48+
* made getCacheEntry always see a changed storage_mtime and fire propagateChange.
49+
*/
50+
public function testStatRootPreservesStorageMtimeFromCache(): void {
51+
$this->instance->getScanner()->scan('', Scanner::SCAN_SHALLOW);
52+
53+
$cachedRoot = $this->instance->getCache()->get('');
54+
$this->assertNotFalse($cachedRoot, 'Root entry must exist in cache after scan');
55+
56+
$cachedStorageMtime = $cachedRoot['storage_mtime'];
57+
58+
$stat = $this->instance->stat('');
59+
$this->assertNotFalse($stat, 'stat(\'\') must return data');
60+
$this->assertEquals(
61+
$cachedStorageMtime,
62+
$stat['storage_mtime'],
63+
'stat(\'\') must return storage_mtime from the cache entry, not a fabricated time()'
64+
);
65+
}
66+
67+
/**
68+
* Regression test for uncached S3 folders with a real directory marker object.
69+
*
70+
* mkdir() creates a `<path>/` object in S3. Before the fix, getDirectoryMetaData()
71+
* ignored that marker on a cache miss and returned time(), so simply reading the
72+
* folder could stamp it as "few seconds ago". stat() must use the marker metadata
73+
* instead, which stays stable across repeated reads.
74+
*/
75+
public function testStatDirectoryMarkerPreservesStorageMtimeWithoutCache(): void {
76+
$this->instance->mkdir('markerdir');
77+
78+
$firstStat = $this->instance->stat('markerdir');
79+
$this->assertNotFalse($firstStat);
80+
81+
sleep(2);
82+
83+
$secondStat = $this->instance->stat('markerdir');
84+
$this->assertNotFalse($secondStat);
85+
$this->assertEquals(
86+
$firstStat['storage_mtime'],
87+
$secondStat['storage_mtime'],
88+
'stat() for an uncached S3 directory marker must not synthesize a fresh timestamp on every read'
89+
);
90+
}
91+
92+
/**
93+
* Regression test for Common::getMetaData discarding storage_mtime for S3 directories.
94+
*
95+
* Common::getMetaData sets storage_mtime = mtime unconditionally. For S3 virtual
96+
* directories, mtime can be updated by mtime propagation while storage_mtime reflects
97+
* the actual last storage change. Without the AmazonS3::getMetaData override the
98+
* scanner would see a spurious storage_mtime change on every read, triggering
99+
* propagateChange and stamping every ancestor folder with the current timestamp.
100+
*/
101+
public function testGetMetaDataDirectoryPreservesStorageMtimeSeparateFromMtime(): void {
102+
$this->instance->mkdir('testmtimedir');
103+
$this->instance->getScanner()->scan('testmtimedir', Scanner::SCAN_SHALLOW);
104+
105+
$cachedEntry = $this->instance->getCache()->get('testmtimedir');
106+
$this->assertNotFalse($cachedEntry);
107+
$originalStorageMtime = $cachedEntry['storage_mtime'];
108+
109+
// Simulate what mtime propagation does: bump mtime without touching storage_mtime.
110+
// This mirrors the state after a child file is written and propagateChange fires.
111+
$bumpedMtime = $originalStorageMtime + 100;
112+
$this->instance->getCache()->put('testmtimedir', ['mtime' => $bumpedMtime]);
113+
114+
$data = $this->instance->getMetaData('testmtimedir');
115+
$this->assertNotNull($data);
116+
$this->assertEquals(
117+
$originalStorageMtime,
118+
$data['storage_mtime'],
119+
'getMetaData for an S3 directory must return the cached storage_mtime, not the (possibly propagation-updated) mtime'
120+
);
43121
}
44122
}

‎tests/lib/Files/ViewTest.php‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,16 @@ public function instanceOfStorage(string $class): bool {
8080
}
8181
}
8282

83+
/**
84+
* Storage that always reports hasUpdated() = true for any path, simulating the
85+
* behavior of Amazon S3 external storage (which has no real directory objects).
86+
*/
87+
class TemporaryAlwaysUpdated extends Temporary {
88+
public function hasUpdated(string $path, int $time): bool {
89+
return true;
90+
}
91+
}
92+
8393
class TestEventHandler {
8494
public function umount() {
8595
}
@@ -456,6 +466,46 @@ public function testWatcher(): void {
456466
$this->assertEquals(3, $cachedData['size']);
457467
}
458468

469+
/**
470+
* Regression test for View::getCacheEntry unconditionally calling propagateChange
471+
* when the watcher reports needsUpdate = true, regardless of whether the scanner
472+
* found any actual storage change.
473+
*
474+
* Backends like Amazon S3 always return hasUpdated() = true for directory paths
475+
* because S3 has no real directory objects. Before the fix, every getFileInfo()
476+
* call on a folder fired propagateChange(path, time()), stamping all ancestor
477+
* folders in the filecache with the current request timestamp.
478+
*
479+
* After the fix, propagateChange is only called when watcher->update() actually
480+
* changes the cached metadata for the path.
481+
*/
482+
public function testWatcherDoesNotPropagateWhenStorageMtimeUnchanged(): void {
483+
$storage = $this->getTestStorage(true, TemporaryAlwaysUpdated::class);
484+
Filesystem::mount($storage, [], '/');
485+
$storage->getWatcher()->setPolicy(Watcher::CHECK_ALWAYS);
486+
487+
$rootView = new View('');
488+
489+
// Note the root mtime right after the initial scan.
490+
$rootMtimeBefore = $storage->getCache()->get('')['mtime'];
491+
492+
// Access a subfolder. The watcher will fire (hasUpdated always returns true),
493+
// but the scanner leaves the cached metadata for 'folder' unchanged.
494+
// getCacheEntry must therefore NOT call propagateChange('folder', time()),
495+
// which would update the root entry's mtime to the current timestamp.
496+
$rootView->getFileInfo('folder');
497+
498+
// Read the root mtime directly from the cache to avoid triggering another watcher cycle.
499+
$rootMtimeAfter = $storage->getCache()->get('')['mtime'];
500+
501+
$this->assertEquals(
502+
$rootMtimeBefore,
503+
$rootMtimeAfter,
504+
'Root folder mtime must not be updated when the watcher fires but cached metadata has not changed'
505+
);
506+
}
507+
508+
459509
public function testCopyBetweenStorageNoCross(): void {
460510
$storage1 = $this->getTestStorage(true, TemporaryNoCross::class);
461511
$storage2 = $this->getTestStorage(true, TemporaryNoCross::class);

0 commit comments

Comments
 (0)