Perf/noid/bulk load share types - #1967
Open
karlitschek wants to merge 3 commits into
Open
Conversation
The suite so far has been integration-only: everything under tests/api/ boots a real Nextcloud, so there was no way to exercise the app's pure logic without a server, and no test at all covered the functions that turn user input into file names. composer.json already declared a `test:unit` script pointing at tests/unit/phpunit.xml, but neither the config nor PHPUnit itself was present. This makes that script real: * tests/unit/phpunit.xml + bootstrap.php — no server, no database, no web server. `composer test:unit` works on a bare checkout. The bootstrap declares OC\Hooks\Emitter, which OCP\Files\IRootFolder extends but nextcloud/ocp does not ship, the same way tests/stubs/ocp.php already fills gaps for Psalm. * phpunit/phpunit and doctrine/dbal as dev dependencies. DBAL is needed because mocking OCP\IDBConnection reflects over IQueryBuilder, whose signatures reference Doctrine's types; the server provides it at runtime. * OCA\Notes\ is mapped in autoload-dev — the app relies on Nextcloud's own app autoloader, which is absent outside a server. * A separate phpunit-unit.yml workflow so these run on every pull request in seconds, independently of the server-backed test.yml. 120 tests covering NoteUtil (category-path normalisation including traversal attempts, title derivation, collision-safe file names, markdown stripping), NotesService (which files count as notes, the folder walk, titles from content), Note (title, category, excerpt, BOM and object-storage content handling), Util::retryIfLocked and ChunkCursor. Three tests are marked in their docblocks as characterization tests: they pin current behaviour that looks wrong so that a fix is a visible change rather than a silent one. No production code is touched by this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The characterization test claimed that every nested subcategory is dropped. That is only true while the parent's accumulated list is at least as long as the recursion's: the "+" union discards an entry whose index is already occupied, so a folder with more children than the parent has collected keeps the later ones. A folder 'Work' containing A, B and C therefore yields ['Work', 'Work/B', 'Work/C'] — 'Work/A' collides with 'Work' at index 0 and is lost, its siblings are not. Added as a second test case so the fix is verified against the real shape of the bug and not against a simpler mental model of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Note::getData() asks NoteUtil::getShareTypes() for the share types of every note it serialises, and that ran one IManager::getSharesBy() query per share type — eight per note. The web index endpoint loads the whole collection at once (chunkSize is 0 there), so rendering the note list issued eight queries times the number of notes: about 4000 for a user with 500 notes, purely to decide whether to draw the "shared" indicator dot. IManager::getSharesInFolder() answers for every file in a folder in one go, so the cost becomes one call per folder instead of eight per note. NoteUtil::loadShareTypes() preloads a whole tree that way and getShareTypes() reads from that cache, falling back to the old per-file lookup for the single-note endpoints where preloading a tree would cost more than it saves. This mirrors TagService::loadTags(), which already solves the same problem for favorites and is called from the same place. getSharesInFolder() only reports on a folder's direct children — passing $shallow = false is rejected by the server — so gatherNoteFiles() now also returns every folder it walked, and loadShareTypes() queries each one. The payload is deliberately unchanged. Shares are filtered against the same eight types the old code asked about and emitted in the same order, so `shareTypes` and `isShared` are identical to before; types the previous code never requested (TYPE_USERGROUP, the per-user half of a group share) stay unreported. A folder whose owner cannot be resolved disables the preload rather than caching an empty result, so a missing owner can never turn a shared note into an unshared-looking one. Also drops the FIXME next to the hardcoded 15 and uses IShare::TYPE_SCIENCEMESH: the constant has existed since Nextcloud 26 and the app now requires 33. Covered by tests/unit/Service/NoteUtilShareTypesTest.php, which asserts that the query count follows the folder count and not the note count, that a preloaded result equals what the per-file path returns, and that the fallbacks still work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
karlitschek
requested review from
enjeck and
juliusknorr
and
a lite review from Copilot
and removed request for
enjeck and
silverkszlo
August 6, 2026 13:14
There was a problem hiding this comment.
Pull request overview
This PR introduces a bulk (per-folder) share-type preload path to avoid N×share-type queries when listing many notes, and adds a new fast unit-test suite (plus CI wiring) to validate key service logic without a running Nextcloud instance.
Changes:
- Add
NoteUtil::loadShareTypes()and a cache-backedgetShareTypes()to replace per-note/per-type share lookups with per-folder bulk fetches. - Extend
NotesService::gatherNoteFiles()to also return the walked folder list and callloadShareTypes()duringgetAll(). - Add a PHPUnit unit-test harness (tests, bootstrap, workflow), plus Makefile/Composer updates to run it.
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
lib/Service/NoteUtil.php |
Introduces share-type constants, caching, and bulk preload logic via getSharesInFolder(). |
lib/Service/NotesService.php |
Collects walked folders and preloads share types during getAll() for list rendering performance. |
tests/unit/Service/NoteUtilShareTypesTest.php |
Adds tests validating query-count reduction and value equivalence for preloaded share types. |
tests/unit/Service/NoteUtilTest.php |
Adds unit tests for path/title normalization and filename collision logic. |
tests/unit/Service/UtilTest.php |
Adds tests pinning retryIfLocked() behavior and edge cases. |
tests/unit/Service/NoteTest.php |
Adds tests for deriving note title/category/content/excerpt/read-only from files. |
tests/unit/Service/NotesServiceTest.php |
Adds tests for tree-walk behavior, note detection, categories, and folder collection for share preload. |
tests/unit/Controller/ChunkCursorTest.php |
Adds tests for cursor round-tripping and malformed cursor rejection. |
tests/unit/bootstrap.php |
Provides minimal OC namespace stubs needed for mocking OCP interfaces. |
tests/unit/phpunit.xml |
Adds dedicated PHPUnit config for server-free unit tests. |
.github/workflows/phpunit-unit.yml |
Adds CI workflow to run the new unit suite across supported PHP versions. |
composer.json |
Adds dev dependencies/autoload needed for unit tests (incl. PHPUnit) and exposes test:unit script. |
composer.lock |
Locks updated dev dependency set introduced for the unit suite. |
Makefile |
Adds test-unit target and includes it in make test. |
.gitignore |
Ignores PHPUnit cache artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| $this->shareManager = $this->createMock(IManager::class); | ||
| $this->shareManager->method('getSharesInFolder') | ||
| ->willReturnCallback(function (string $userId, Folder $folder): array { |
| * {@see testAnEmptyCustomExtensionMatchesEveryExtensionlessFile}. | ||
| * | ||
| * @param array<int|string, string|array<int|string, mixed>> $spec | ||
| * @return array{files: array<int, File>, categories: list<string>} |
| return $this->sharesInFolder[$folder->getPath()] ?? []; | ||
| }); | ||
| $this->shareManager->method('getSharesBy') | ||
| ->willReturnCallback(function (string $userId, int $shareType, ?\OCP\Files\Node $node): array { |
| */ | ||
| private function gather(array $spec, string $customExtension = 'md'): array { | ||
| $method = new \ReflectionMethod(NotesService::class, 'gatherNoteFiles'); | ||
| /** @var array{files: array<int, File>, categories: list<string>} $result */ |
| } | ||
|
|
||
| /** | ||
| * @return list<array{0: string}> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI (if applicable)