diff --git a/.github/workflows/phpunit-unit.yml b/.github/workflows/phpunit-unit.yml index f5d7a8c8f..deaf82c8a 100644 --- a/.github/workflows/phpunit-unit.yml +++ b/.github/workflows/phpunit-unit.yml @@ -25,7 +25,7 @@ jobs: - name: Get version matrix id: versions - uses: icewind1991/nextcloud-version-matrix@8a7bac6300b2f0f3100088b297995a229558ddba # v1.0.0 + uses: icewind1991/nextcloud-version-matrix@cd0211ffcef1065e2020cd579e4843b8746e7a58 # v1.0.0 unit-tests: runs-on: ubuntu-latest diff --git a/lib/Controller/Helper.php b/lib/Controller/Helper.php index c876368a8..4ee546b2e 100644 --- a/lib/Controller/Helper.php +++ b/lib/Controller/Helper.php @@ -103,6 +103,12 @@ public function getNotesAndCategories( // if the chunk does not contain all remaining notes, then generate new chunk cursor $newChunkCursor = $numPendingNotes ? ChunkCursor::fromNote($lastUpdate, end($chunkedNotes)) : null; + $notesById = []; + foreach ($chunkedNotes as $chunked) { + $notesById[$chunked->note->getId()] = $chunked->note->getFile(); + } + $this->notesService->preloadShareTypes($data['folders'], $notesById); + // load data for the current chunk $notesData = array_map(function (MetaNote $m) use ($exclude) { return $this->getNoteData($m->note, $exclude, $m->meta); diff --git a/lib/Service/NoteUtil.php b/lib/Service/NoteUtil.php index 5b4faf8ea..e60c1706e 100644 --- a/lib/Service/NoteUtil.php +++ b/lib/Service/NoteUtil.php @@ -324,22 +324,137 @@ public function ensureNoteIsWritable(Node $node) : void { } } + /** + * Share types a note is reported as shared through, in this order. + * + * TYPE_USERGROUP is left out: it is the per-user half of a group share and + * would double-report one share. + * + * @var list + * @psalm-suppress DeprecatedConstant TYPE_SCIENCEMESH is reported for shares + * that already exist, so it cannot be dropped from the list + */ + private const SHARE_TYPES = [ + IShare::TYPE_USER, + IShare::TYPE_GROUP, + IShare::TYPE_LINK, + IShare::TYPE_REMOTE, + IShare::TYPE_EMAIL, + IShare::TYPE_ROOM, + IShare::TYPE_DECK, + IShare::TYPE_SCIENCEMESH, + ]; + + /** + * Share types per file id, or null when nothing has been preloaded. + * + * A file id present with an empty list means "looked up, not shared". + * + * @var array>|null + */ + private ?array $cachedShareTypes = null; + + /** + * Preload the share types for a whole notes tree. + * + * Every folder has to be passed in: getSharesInFolder() only looks at a + * folder's direct children and the server rejects $shallow = false. It + * reports what one user has shared, so a lookup answers for a note only + * when that note is a direct child of the folder and carries the same + * owner the lookup ran as. A note the walk found elsewhere — one shared + * into the tree on its own, beside a folder from the same owner — is left + * out of the cache and falls back to a per-note lookup. + * + * @param list $folders every folder of the notes tree, the notes folder included + * @param array $files the notes the caller is going to ask about, by id + */ + public function loadShareTypes(array $folders, array $files): void { + $this->cachedShareTypes = null; + + // one getSharesInFolder() call costs about what one note costs through + // the eight getSharesBy() calls it replaces + if (count($files) < count($folders)) { + return; + } + + $byOwnerAndParent = self::indexByParent($files); + $collected = []; + + foreach ($folders as $folder) { + $owner = $folder->getOwner(); + if ($owner === null) { + return; + } + + $uid = $owner->getUID(); + $covered = $byOwnerAndParent[$uid][rtrim($folder->getPath(), '/')] ?? []; + if ($covered === []) { + continue; + } + + $collected += array_fill_keys(array_keys($covered), []); + $sharesByFileId = $this->shareManager->getSharesInFolder($uid, $folder, false); + foreach ($sharesByFileId as $fileId => $shares) { + if (!isset($covered[$fileId])) { + continue; + } + foreach ($shares as $share) { + $collected[$fileId][$share->getShareType()] = true; + } + } + } + + $this->cachedShareTypes = array_map( + static fn (array $present): array + => array_values(array_intersect(self::SHARE_TYPES, array_keys($present))), + $collected, + ); + } + + /** + * The notes grouped by the lookup that can answer for them: their owner, + * then the folder they sit directly in. + * + * @param array $files + * @return array>> + */ + private static function indexByParent(array $files): array { + $byOwnerAndParent = []; + foreach ($files as $fileId => $file) { + $owner = $file->getOwner()?->getUID(); + if ($owner !== null) { + $byOwnerAndParent[$owner][dirname($file->getPath())][$fileId] = true; + } + } + return $byOwnerAndParent; + } + + /** + * @return list share types of $file, in SHARE_TYPES order + */ public function getShareTypes(File $file): array { - $userId = $file->getOwner()->getUID(); - $requestedShareTypes = [ - IShare::TYPE_USER, - IShare::TYPE_GROUP, - IShare::TYPE_LINK, - IShare::TYPE_REMOTE, - IShare::TYPE_EMAIL, - IShare::TYPE_ROOM, - IShare::TYPE_DECK, - // FIXME: Move to constant once Nextcloud 26 is the minimum supported version - 15, // IShare::TYPE_SCIENCEMESH, - ]; + $fileId = $file->getId(); + if ($this->cachedShareTypes !== null && array_key_exists($fileId, $this->cachedShareTypes)) { + return $this->cachedShareTypes[$fileId]; + } + return $this->fetchShareTypes($file); + } + + /** + * Per-file fallback for callers that have not preloaded a tree. + * + * @return list + */ + private function fetchShareTypes(File $file): array { + $owner = $file->getOwner(); + if ($owner === null) { + return []; + } + + $userId = $owner->getUID(); $shareTypes = []; - foreach ($requestedShareTypes as $shareType) { + foreach (self::SHARE_TYPES as $shareType) { $shares = $this->shareManager->getSharesBy($userId, $shareType, $file, false, 1, 0); if (count($shares)) { diff --git a/lib/Service/NotesService.php b/lib/Service/NotesService.php index b172ff796..87a7d1095 100644 --- a/lib/Service/NotesService.php +++ b/lib/Service/NotesService.php @@ -41,9 +41,17 @@ public function getAll(string $userId, bool $autoCreateNotesFolder = false) : ar }, $data['files']); } catch (NotesFolderException $e) { $notes = []; - $data = [ 'categories' => [] ]; + $data = [ 'categories' => [], 'folders' => [] ]; } - return [ 'notes' => $notes, 'categories' => $data['categories'] ]; + return [ 'notes' => $notes, 'categories' => $data['categories'], 'folders' => $data['folders'] ]; + } + + /** + * @param list $folders the walked folders, from getAll() + * @param array $notes the notes that will be serialised, by id + */ + public function preloadShareTypes(array $folders, array $notes) : void { + $this->noteUtil->loadShareTypes($folders, $notes); } public function getTopNotes(string $userId) : array { @@ -244,6 +252,8 @@ private function getNotesFolder(string $userId, bool $create = true) : Folder { /** * gather note files in given directory and all subdirectories + * + * @return array{files: array, categories: array, folders: list} */ private static function gatherNoteFiles( string $customExtension, @@ -254,6 +264,7 @@ private static function gatherNoteFiles( $data = [ 'files' => [], 'categories' => [], + 'folders' => [$folder], ]; $nodes = $folder->getDirectoryListing(); foreach ($nodes as $node) { @@ -271,7 +282,8 @@ private static function gatherNoteFiles( $data_sub = self::gatherNoteFiles($customExtension, $node, $showHidden, $subCategory . '/'); $data['files'] = $data['files'] + $data_sub['files']; $data['categories'] = array_merge($data['categories'], $data_sub['categories']); - } elseif (self::isNote($node, $customExtension)) { + $data['folders'] = array_merge($data['folders'], $data_sub['folders']); + } elseif ($node instanceof File && self::isNote($node, $customExtension)) { $data['files'][$node->getId()] = $node; } } diff --git a/tests/unit/NotesTestCase.php b/tests/unit/NotesTestCase.php index 275b1805f..5d949e28a 100644 --- a/tests/unit/NotesTestCase.php +++ b/tests/unit/NotesTestCase.php @@ -22,7 +22,7 @@ use Psr\Log\LoggerInterface; abstract class NotesTestCase extends TestCase { - protected function createNoteUtil(): NoteUtil { + protected function createNoteUtil(?IManager $shareManager = null): NoteUtil { $l10n = $this->createMock(IL10N::class); $l10n->method('t')->willReturnArgument(0); @@ -34,7 +34,7 @@ protected function createNoteUtil(): NoteUtil { $this->createMock(IRootFolder::class), $db, $this->createMock(TagService::class), - $this->createMock(IManager::class), + $shareManager ?? $this->createMock(IManager::class), $this->createMock(IUserSession::class), $this->createMock(SettingsService::class), ); diff --git a/tests/unit/Service/NoteUtilShareTypesTest.php b/tests/unit/Service/NoteUtilShareTypesTest.php new file mode 100644 index 000000000..0b6e42bea --- /dev/null +++ b/tests/unit/Service/NoteUtilShareTypesTest.php @@ -0,0 +1,322 @@ +>> keyed by folder path */ + private array $sharesInFolder = []; + /** @var array> keyed by "fileId:shareType" */ + private array $sharesByFile = []; + + private int $getSharesInFolderCalls = 0; + private int $getSharesByCalls = 0; + + private NoteUtil $noteUtil; + + protected function setUp(): void { + parent::setUp(); + + $shareManager = $this->createMock(IManager::class); + $shareManager->method('getSharesInFolder') + ->willReturnCallback(function (string $userId, Folder $folder, bool $reshares = false, bool $shallow = true): array { + $this->getSharesInFolderCalls++; + self::assertSame($folder->getOwner()?->getUID(), $userId); + self::assertFalse($reshares); + self::assertTrue($shallow); + return $this->sharesInFolder[$folder->getPath()] ?? []; + }); + $shareManager->method('getSharesBy') + ->willReturnCallback(function ( + string $userId, + int $shareType, + ?Node $node = null, + bool $reshares = false, + int $limit = 50, + int $offset = 0, + ): array { + $this->getSharesByCalls++; + self::assertFalse($reshares); + return $this->sharesByFile[($node?->getId() ?? 0) . ':' . $shareType] ?? []; + }); + + $this->noteUtil = $this->createNoteUtil($shareManager); + } + + /** @return Folder&MockObject */ + private function folder(string $path, bool $withOwner = true, string $owner = self::OWNER): Folder { + $folder = $this->createMock(Folder::class); + $folder->method('getPath')->willReturn($path); + $folder->method('getOwner')->willReturn($withOwner ? $this->user($owner) : null); + return $folder; + } + + /** @return File&MockObject */ + private function file(int $id, string $path = '/n/note.md', ?string $owner = self::OWNER): File { + $file = $this->createMock(File::class); + $file->method('getId')->willReturn($id); + $file->method('getPath')->willReturn($path); + $file->method('getOwner')->willReturn($owner === null ? null : $this->user($owner)); + return $file; + } + + /** + * @param list $ids + * @return array + */ + private function notesIn(Folder $folder, array $ids, ?string $owner = self::OWNER): array { + $files = []; + foreach ($ids as $id) { + $files[$id] = $this->file($id, $folder->getPath() . '/note' . $id . '.md', $owner); + } + return $files; + } + + /** @return IUser&MockObject */ + private function user(string $uid = self::OWNER): IUser { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($uid); + return $user; + } + + /** + * @param list $shareTypes + * @return list + */ + private function shares(array $shareTypes): array { + return array_map(function (int $type): IShare { + $share = $this->createMock(IShare::class); + $share->method('getShareType')->willReturn($type); + return $share; + }, $shareTypes); + } + + public function testPreloadCostsOneCallPerFolderRegardlessOfNoteCount(): void { + $root = $this->folder('/alice/files/Notes'); + $work = $this->folder('/alice/files/Notes/Work'); + + $this->noteUtil->loadShareTypes([$root, $work], $this->notesIn($root, range(1, 25)) + + $this->notesIn($work, range(26, 50))); + + self::assertSame(2, $this->getSharesInFolderCalls); + self::assertSame(0, $this->getSharesByCalls); + } + + public function testReadingPreloadedNotesIssuesNoFurtherQueries(): void { + $root = $this->folder('/alice/files/Notes'); + $this->sharesInFolder['/alice/files/Notes'] = [ + 7 => $this->shares([IShare::TYPE_LINK]), + ]; + + $this->noteUtil->loadShareTypes([$root], $this->notesIn($root, range(1, 50))); + $this->getSharesInFolderCalls = 0; + + for ($id = 1; $id <= 50; $id++) { + $this->noteUtil->getShareTypes($this->file($id)); + } + + self::assertSame(0, $this->getSharesByCalls); + self::assertSame(0, $this->getSharesInFolderCalls); + } + + public function testPreloadedShareTypesMatchThePerFileLookup(): void { + $expected = [IShare::TYPE_USER, IShare::TYPE_LINK, IShare::TYPE_DECK]; + + foreach ($expected as $type) { + $this->sharesByFile['1:' . $type] = $this->shares([$type]); + } + $perFile = $this->noteUtil->getShareTypes($this->file(1)); + + $root = $this->folder('/alice/files/Notes'); + $this->sharesInFolder['/alice/files/Notes'] = [1 => $this->shares($expected)]; + $this->noteUtil->loadShareTypes([$root], $this->notesIn($root, [1])); + + self::assertSame($expected, $perFile); + self::assertSame($perFile, $this->noteUtil->getShareTypes($this->file(1))); + } + + public function testTypesAreReportedInTheDeclaredOrderAndOnlyOnce(): void { + $root = $this->folder('/n'); + $this->sharesInFolder['/n'] = [ + 1 => $this->shares([IShare::TYPE_DECK, IShare::TYPE_LINK, IShare::TYPE_USER, IShare::TYPE_USER]), + ]; + + $this->noteUtil->loadShareTypes([$root], $this->notesIn($root, [1])); + + self::assertSame( + [IShare::TYPE_USER, IShare::TYPE_LINK, IShare::TYPE_DECK], + $this->noteUtil->getShareTypes($this->file(1)), + ); + } + + public function testShareTypesOutsideTheReportedSetAreIgnored(): void { + $root = $this->folder('/n'); + $this->sharesInFolder['/n'] = [ + 1 => $this->shares([IShare::TYPE_USERGROUP, IShare::TYPE_CIRCLE, IShare::TYPE_GUEST]), + 2 => $this->shares([IShare::TYPE_USERGROUP, IShare::TYPE_GROUP]), + ]; + + $this->noteUtil->loadShareTypes([$root], $this->notesIn($root, [1, 2])); + + self::assertSame([], $this->noteUtil->getShareTypes($this->file(1))); + self::assertSame([IShare::TYPE_GROUP], $this->noteUtil->getShareTypes($this->file(2))); + } + + public function testAnUnsharedNoteIsAnsweredFromTheCache(): void { + $root = $this->folder('/n'); + $this->sharesInFolder['/n'] = [1 => $this->shares([IShare::TYPE_LINK])]; + + $this->noteUtil->loadShareTypes([$root], $this->notesIn($root, [1, 2])); + + self::assertSame([], $this->noteUtil->getShareTypes($this->file(2))); + self::assertSame(0, $this->getSharesByCalls); + } + + public function testSharesOnNonNotesInTheSameFolderAreIgnored(): void { + $root = $this->folder('/n'); + $this->sharesInFolder['/n'] = [ + 1 => $this->shares([IShare::TYPE_LINK]), + 99 => $this->shares([IShare::TYPE_USER]), + 98 => $this->shares([IShare::TYPE_GROUP]), + ]; + + $this->noteUtil->loadShareTypes([$root], $this->notesIn($root, [1])); + + self::assertSame([IShare::TYPE_LINK], $this->noteUtil->getShareTypes($this->file(1))); + } + + public function testAFileOutsideThePreloadFallsBackToAPerFileLookup(): void { + $root = $this->folder('/n'); + $this->noteUtil->loadShareTypes([$root], $this->notesIn($root, [1])); + $this->sharesByFile['42:' . IShare::TYPE_LINK] = $this->shares([IShare::TYPE_LINK]); + + self::assertSame([IShare::TYPE_LINK], $this->noteUtil->getShareTypes($this->file(42))); + self::assertGreaterThan(0, $this->getSharesByCalls); + } + + public function testWithoutAnyPreloadEveryLookupIsPerFile(): void { + $this->sharesByFile['1:' . IShare::TYPE_EMAIL] = $this->shares([IShare::TYPE_EMAIL]); + + self::assertSame([IShare::TYPE_EMAIL], $this->noteUtil->getShareTypes($this->file(1))); + self::assertSame(0, $this->getSharesInFolderCalls); + } + + public function testAFolderWithoutAnOwnerDisablesThePreload(): void { + $ownerless = $this->folder('/n', withOwner: false); + $this->sharesByFile['1:' . IShare::TYPE_LINK] = $this->shares([IShare::TYPE_LINK]); + + $this->noteUtil->loadShareTypes([$ownerless], $this->notesIn($ownerless, [1])); + + self::assertSame([IShare::TYPE_LINK], $this->noteUtil->getShareTypes($this->file(1))); + } + + public function testAFileWithoutAnOwnerReportsNoShares(): void { + $ownerless = $this->folder('/n', withOwner: false); + $this->noteUtil->loadShareTypes([$ownerless], $this->notesIn($ownerless, [1])); + + self::assertSame([], $this->noteUtil->getShareTypes($this->file(1, owner: null))); + self::assertSame(0, $this->getSharesByCalls); + } + + public function testFewerNotesThanFoldersIsNotWorthAPreload(): void { + $folders = [$this->folder('/n'), $this->folder('/n/a'), $this->folder('/n/b')]; + $this->sharesByFile['1:' . IShare::TYPE_LINK] = $this->shares([IShare::TYPE_LINK]); + + $this->noteUtil->loadShareTypes($folders, $this->notesIn($folders[0], [1, 2])); + + self::assertSame(0, $this->getSharesInFolderCalls); + self::assertSame([IShare::TYPE_LINK], $this->noteUtil->getShareTypes($this->file(1))); + } + + public function testMoreNotesThanFoldersIsWorthAPreload(): void { + $folders = [$this->folder('/n'), $this->folder('/n/a'), $this->folder('/n/b')]; + $this->sharesInFolder['/n'] = [1 => $this->shares([IShare::TYPE_LINK])]; + + $this->noteUtil->loadShareTypes($folders, $this->notesIn($folders[0], range(1, 48)) + + $this->notesIn($folders[1], [49]) + + $this->notesIn($folders[2], [50])); + + self::assertSame(3, $this->getSharesInFolderCalls); + self::assertSame([IShare::TYPE_LINK], $this->noteUtil->getShareTypes($this->file(1))); + self::assertSame(0, $this->getSharesByCalls); + } + + public function testAPreloadThatIsNotWorthMakingClearsAnEarlierOne(): void { + $root = $this->folder('/n'); + $this->sharesInFolder['/n'] = [1 => $this->shares([IShare::TYPE_LINK])]; + $this->noteUtil->loadShareTypes([$root], $this->notesIn($root, [1])); + + $this->noteUtil->loadShareTypes([$root, $this->folder('/n/a')], $this->notesIn($root, [1])); + + self::assertSame([], $this->noteUtil->getShareTypes($this->file(1))); + self::assertSame(8, $this->getSharesByCalls); + } + + public function testANoteOwnedBySomeoneElseIsLookedUpPerFile(): void { + $root = $this->folder('/n'); + $this->sharesInFolder['/n'] = []; + $this->sharesByFile['1:' . IShare::TYPE_USER] = $this->shares([IShare::TYPE_USER]); + + $this->noteUtil->loadShareTypes([$root], $this->notesIn($root, [1], owner: 'someone-else')); + + self::assertSame( + [IShare::TYPE_USER], + $this->noteUtil->getShareTypes($this->file(1, owner: 'someone-else')), + ); + } + + /** + * A folder shared in by another user answers only for the notes inside it, + * so a note of the same owner sitting beside it still needs a per-file + * lookup rather than being reported as unshared. + */ + public function testANoteSharedInBesideAFolderFromTheSameOwnerIsNotReportedUnshared(): void { + $root = $this->folder('/n'); + $bob = $this->folder('/n/bob', owner: 'bob'); + $this->sharesInFolder['/n'] = []; + $this->sharesInFolder['/n/bob'] = [2 => $this->shares([IShare::TYPE_USER])]; + $this->sharesByFile['1:' . IShare::TYPE_USER] = $this->shares([IShare::TYPE_USER]); + + $this->noteUtil->loadShareTypes( + [$root, $bob], + $this->notesIn($root, [1], owner: 'bob') + $this->notesIn($bob, [2], owner: 'bob'), + ); + + self::assertSame([IShare::TYPE_USER], $this->noteUtil->getShareTypes($this->file(1, owner: 'bob'))); + self::assertGreaterThan(0, $this->getSharesByCalls); + + $this->getSharesByCalls = 0; + self::assertSame([IShare::TYPE_USER], $this->noteUtil->getShareTypes($this->file(2, owner: 'bob'))); + self::assertSame(0, $this->getSharesByCalls); + } + + public function testASecondPreloadReplacesTheFirst(): void { + $root = $this->folder('/n'); + $this->sharesInFolder['/n'] = [1 => $this->shares([IShare::TYPE_LINK])]; + $this->noteUtil->loadShareTypes([$root], $this->notesIn($root, [1])); + self::assertSame([IShare::TYPE_LINK], $this->noteUtil->getShareTypes($this->file(1))); + + $this->sharesInFolder['/n'] = []; + $this->noteUtil->loadShareTypes([$root], $this->notesIn($root, [1])); + + self::assertSame([], $this->noteUtil->getShareTypes($this->file(1))); + } +} diff --git a/tests/unit/Service/NotesServiceTest.php b/tests/unit/Service/NotesServiceTest.php index 0ca9f8311..d39bb792d 100644 --- a/tests/unit/Service/NotesServiceTest.php +++ b/tests/unit/Service/NotesServiceTest.php @@ -71,11 +71,11 @@ private function file(string $name): File { /** * @param array> $spec - * @return array{files: array, categories: list} + * @return array{files: array, categories: array, folders: list} */ private function gather(array $spec, string $customExtension = 'md', bool $showHidden = false): array { $method = new \ReflectionMethod(NotesService::class, 'gatherNoteFiles'); - /** @var array{files: array, categories: list} $result */ + /** @var array{files: array, categories: array, folders: list} $result */ $result = $method->invoke(null, $customExtension, $this->folder($spec), $showHidden); return $result; } @@ -142,6 +142,32 @@ public function testAttachmentFoldersAreNotCategories(): void { self::assertSame(['Work'], array_values($categories)); } + public function testTheWalkReportsEveryFolderIncludingTheNotesFolderItself(): void { + $result = $this->gather([ + 'top.txt', + 'Work' => [ + 'a.txt', + 'Projects' => [ + '2026' => ['deep.md'], + ], + ], + 'Personal' => [], + ]); + + self::assertCount(5, $result['folders']); + self::assertContainsOnlyInstancesOf(Folder::class, $result['folders']); + } + + public function testWalkedFoldersAreAListWithNoGaps(): void { + $result = $this->gather([ + 'Work' => ['Projects' => []], + 'Personal' => ['Recipes' => []], + ]); + + self::assertSame(range(0, count($result['folders']) - 1), array_keys($result['folders'])); + self::assertCount(5, $result['folders']); + } + public function testCollectsCategoriesIncludingFoldersWithoutNotes(): void { $categories = $this->gather([ 'loose.txt',