Skip to content

Commit 54e4ce3

Browse files
committed
wip: add test coverage
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
1 parent 6a35652 commit 54e4ce3

2 files changed

Lines changed: 105 additions & 2 deletions

File tree

apps/files_external/tests/Storage/Amazons3Test.php

Lines changed: 55 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,59 @@ 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 Common::getMetaData discarding storage_mtime for S3 directories.
69+
*
70+
* Common::getMetaData sets storage_mtime = mtime unconditionally. For S3 virtual
71+
* directories, mtime can be updated by mtime propagation while storage_mtime reflects
72+
* the actual last storage change. Without the AmazonS3::getMetaData override the
73+
* scanner would see a spurious storage_mtime change on every read, triggering
74+
* propagateChange and stamping every ancestor folder with the current timestamp.
75+
*/
76+
public function testGetMetaDataDirectoryPreservesStorageMtimeSeparateFromMtime(): void {
77+
$this->instance->mkdir('testmtimedir');
78+
$this->instance->getScanner()->scan('testmtimedir', Scanner::SCAN_SHALLOW);
79+
80+
$cachedEntry = $this->instance->getCache()->get('testmtimedir');
81+
$this->assertNotFalse($cachedEntry);
82+
$originalStorageMtime = $cachedEntry['storage_mtime'];
83+
84+
// Simulate what mtime propagation does: bump mtime without touching storage_mtime.
85+
// This mirrors the state after a child file is written and propagateChange fires.
86+
$bumpedMtime = $originalStorageMtime + 100;
87+
$this->instance->getCache()->put('testmtimedir', ['mtime' => $bumpedMtime]);
88+
89+
$data = $this->instance->getMetaData('testmtimedir');
90+
$this->assertNotNull($data);
91+
$this->assertEquals(
92+
$originalStorageMtime,
93+
$data['storage_mtime'],
94+
'getMetaData for an S3 directory must return the cached storage_mtime, not the (possibly propagation-updated) mtime'
95+
);
4396
}
4497
}

tests/lib/Files/ViewTest.php

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,16 @@ public function instanceOfStorage(string $class): bool {
7676
}
7777
}
7878

79+
/**
80+
* Storage that always reports hasUpdated() = true for any path, simulating the
81+
* behavior of Amazon S3 external storage (which has no real directory objects).
82+
*/
83+
class TemporaryAlwaysUpdated extends Temporary {
84+
public function hasUpdated(string $path, int $time): bool {
85+
return true;
86+
}
87+
}
88+
7989
class TestEventHandler {
8090
public function umount() {
8191
}
@@ -448,6 +458,46 @@ public function testWatcher(): void {
448458
$this->assertEquals(3, $cachedData['size']);
449459
}
450460

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

0 commit comments

Comments
 (0)