From 723d36372bc7f7fa2f60cc38dfabcab8bb59e4fe Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Fri, 21 Aug 2026 07:02:59 +0200 Subject: [PATCH 1/2] perf(appstore): index the whole catalogue on fetch instead of discarding it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The App Store's `apps.json` IGNORES its `filter` parameter. Measured 2026-08-21 against the endpoint the code actually calls (garm3.nextcloud.com/api/v1): GET /apps.json?filter=notes -> 200, 755 entries, 31,701,904 bytes So a lookup for ONE app already downloads every app. The response was then searched for the requested id and the other 754 entries thrown away — and because `listAdvisories()` and `listVersions()` each resolve a payload, an advisory sweep over 88 enabled apps re-downloaded that catalogue per app. That is the real cost behind #160: not 176 calls, but 176 calls each pulling a ~31.7 MB body. This writes every entry in a downloaded catalogue through the SAME per-app cache, with the same TTL and the same shape. The first lookup in a sweep pays for the download; every later app is a cache hit. A caller asking for a single app in isolation behaves exactly as before. The test asserts on HTTP CALL COUNT, not elapsed time: three apps, one GET. It also asserts each app gets ITS OWN payload, because a cache that returned the first app's data for every id would satisfy the call-count assertion too. Control: with the two `cacheCatalogueEntries()` call sites removed, the test fails with `Failed asserting that null is identical to '5.4.0'` — the second app cannot be served at all. Restored, it passes. 506 unit tests, 1024 assertions, no failure outside tests/unit/Command (19 errors there are a missing symfony/console in the local vendor copy). psalm clean on the changed file; gate-16 spec-coverage count=0. --- lib/Service/Source/AppStoreSource.php | 61 ++++++++++++++++++ .../Service/Source/AppStoreSourceTest.php | 62 +++++++++++++++++++ 2 files changed, 123 insertions(+) diff --git a/lib/Service/Source/AppStoreSource.php b/lib/Service/Source/AppStoreSource.php index 02828d97..88431766 100644 --- a/lib/Service/Source/AppStoreSource.php +++ b/lib/Service/Source/AppStoreSource.php @@ -326,6 +326,8 @@ private function fetchAppPayloadUncached(string $appId): ?array { if (!is_array($decoded)) { return null; } + // The whole catalogue arrived regardless of the filter; keep it. + $this->cacheCatalogueEntries($decoded); $appPayload = $this->extractAppPayload($decoded, $appId); if (is_array($appPayload)) { return $appPayload; @@ -357,6 +359,9 @@ private function fetchAppPayloadUncached(string $appId): ?array { if (!is_array($decoded)) { continue; } + // Same reasoning as the filtered endpoint above: this response + // is the whole platform catalogue, so index all of it. + $this->cacheCatalogueEntries($decoded); $appPayload = $this->extractAppPayload($decoded, $appId); if (is_array($appPayload)) { return $appPayload; @@ -416,6 +421,62 @@ private function arrayField(array $payload, string $key): ?array { * @param array $entries * @return array|null */ + /** + * Caches EVERY app in a freshly-downloaded catalogue, not just the one that + * was asked for. + * + * The App Store's `apps.json` IGNORES its `filter` parameter — measured + * 2026-08-21, `?filter=notes` returned all 755 entries and 31.7 MB — so a + * lookup for one app already pays for the whole catalogue. Keeping one + * entry and discarding 754 meant a full advisory sweep over 88 enabled + * apps downloaded ~31.7 MB per app, and did it twice per app because + * `listAdvisories()` and `listVersions()` each resolve a payload. + * + * Indexing the whole response makes the FIRST lookup pay for the download + * and every subsequent app in the same sweep a cache hit. Nothing else + * changes: entries are written through the same per-app cache with the same + * TTL, so a caller asking for one app in isolation behaves exactly as before. + * + * @spec openspec/specs/security-advisory-correlation/spec.md + * @param array $decoded A decoded catalogue response. + * @return int Number of entries cached (0 when the shape is unrecognised). + */ + private function cacheCatalogueEntries(array $decoded): int { + $entries = null; + $data = $this->arrayField($decoded, 'data'); + if ($data !== null && array_is_list($data)) { + $entries = $data; + } elseif (array_is_list($decoded)) { + $entries = $decoded; + } else { + $apps = $this->arrayField($decoded, 'apps'); + if ($apps !== null && array_is_list($apps)) { + $entries = $apps; + } + } + + if ($entries === null) { + return 0; + } + + $cached = 0; + /** @var mixed $entry */ + foreach ($entries as $entry) { + if (!is_array($entry)) { + continue; + } + /** @var mixed $id */ + $id = $entry['id'] ?? null; + if (!is_string($id) || $id === '') { + continue; + } + $this->writeCachedPayload($id, $entry); + $cached++; + } + + return $cached; + } + private function findById(array $entries, string $appId): ?array { /** @var mixed $entry */ foreach ($entries as $entry) { diff --git a/tests/unit/Service/Source/AppStoreSourceTest.php b/tests/unit/Service/Source/AppStoreSourceTest.php index bc395342..a194a809 100644 --- a/tests/unit/Service/Source/AppStoreSourceTest.php +++ b/tests/unit/Service/Source/AppStoreSourceTest.php @@ -277,4 +277,66 @@ function (string $app, string $key, string $default = '') use ($body): string { $this->assertSame('2.3.0', $result['versions'][0]['version'], 'stale cache must serve during an upstream outage'); } + + /** + * ONE DOWNLOAD SERVES THE WHOLE SWEEP. + * + * The App Store ignores `?filter=`, so a lookup for one app already + * downloads every app (measured: 755 entries, 31.7 MB). Before this, the + * other 754 were discarded, so an advisory sweep over 88 enabled apps + * re-downloaded the catalogue per app. + * + * The assertion is on HTTP calls, not on elapsed time: the second app must + * be answered without touching the network at all. + */ + public function testCachesEveryAppInTheCatalogueSoASweepDownloadsItOnce(): void { + $catalogue = ['data' => [ + ['id' => 'notes', 'releases' => [['version' => '4.13.0']]], + ['id' => 'calendar', 'releases' => [['version' => '5.4.0']]], + ['id' => 'deck', 'releases' => [['version' => '1.14.0']]], + ]]; + + $response = $this->createMock(IResponse::class); + $response->method('getStatusCode')->willReturn(200); + $response->method('getBody')->willReturn(json_encode($catalogue, JSON_THROW_ON_ERROR)); + + $client = $this->createMock(IClient::class); + // THE ASSERTION: exactly one GET for three apps. + $client->expects($this->once())->method('get')->willReturn($response); + + $clientService = $this->createMock(IClientService::class); + $clientService->method('newClient')->willReturn($client); + + // An in-memory app config, so a cache write by one lookup is visible to + // the next — which is the whole mechanism under test. + $store = []; + $config = $this->createMock(IConfig::class); + $config->method('getSystemValueString')->willReturn('28.0.0'); + $config->method('setAppValue')->willReturnCallback( + static function (string $app, string $key, string $value) use (&$store): void { + $store[$key] = $value; + }, + ); + $config->method('getAppValue')->willReturnCallback( + static function (string $app, string $key, string $default = '') use (&$store): string { + return $store[$key] ?? $default; + }, + ); + + $l10nFactory = $this->createMock(IFactory::class); + $l10nFactory->method('findLanguage')->willReturn('en'); + + $source = new AppStoreSource($clientService, $config, $l10nFactory); + + $first = $source->listVersions('notes', $this->binding()); + $second = $source->listVersions('calendar', $this->binding()); + $third = $source->listVersions('deck', $this->binding()); + + // Each app must still get ITS OWN payload — a shared cache that returned + // the first app's data for every lookup would also satisfy the call + // count above, so assert the versions differ. + $this->assertSame('4.13.0', $first['versions'][0]['version']); + $this->assertSame('5.4.0', $second['versions'][0]['version'], 'the second app must be served from cache, with its own payload'); + $this->assertSame('1.14.0', $third['versions'][0]['version'], 'the third app must be served from cache, with its own payload'); + } } From aa250da0bbb813b4f48b2fc691f102d064998bf3 Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Fri, 21 Aug 2026 09:35:52 +0200 Subject: [PATCH 2/2] fix(appstore): cacheCatalogueEntries returns void psalm UnusedReturnValue: the count was documented and returned but no caller used it, and AppStoreSource has no logger to report it through. Returning void says what the method actually does rather than leaving a value that exists only to be discarded. --- lib/Service/Source/AppStoreSource.php | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/lib/Service/Source/AppStoreSource.php b/lib/Service/Source/AppStoreSource.php index 88431766..5a46ca88 100644 --- a/lib/Service/Source/AppStoreSource.php +++ b/lib/Service/Source/AppStoreSource.php @@ -439,9 +439,8 @@ private function arrayField(array $payload, string $key): ?array { * * @spec openspec/specs/security-advisory-correlation/spec.md * @param array $decoded A decoded catalogue response. - * @return int Number of entries cached (0 when the shape is unrecognised). */ - private function cacheCatalogueEntries(array $decoded): int { + private function cacheCatalogueEntries(array $decoded): void { $entries = null; $data = $this->arrayField($decoded, 'data'); if ($data !== null && array_is_list($data)) { @@ -456,10 +455,9 @@ private function cacheCatalogueEntries(array $decoded): int { } if ($entries === null) { - return 0; + return; } - $cached = 0; /** @var mixed $entry */ foreach ($entries as $entry) { if (!is_array($entry)) { @@ -471,10 +469,7 @@ private function cacheCatalogueEntries(array $decoded): int { continue; } $this->writeCachedPayload($id, $entry); - $cached++; } - - return $cached; } private function findById(array $entries, string $appId): ?array {