diff --git a/appinfo/routes.php b/appinfo/routes.php index e2e20d98..21eb6272 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -273,6 +273,13 @@ ['name' => 'portfolioReport#index', 'url' => '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/api/portfolio-report', 'verb' => 'GET'], // SPA catch-all — serves the Vue app for any frontend route (history mode routing) - ['name' => 'dashboard#page', 'url' => '/{path}', 'verb' => 'GET', 'requirements' => ['path' => '.+'], 'defaults' => ['path' => '']], + // `postfix` keeps this SPA catch-all from colliding with the bare-root + // `dashboard#page` route above: both entries target the same + // controller#method, so without a postfix they generate the same + // internal route name and the later one silently displaces the first — + // which 404'd the app's own entry point (`/apps/softwarecatalog/`) for + // every user, because this route's `path` requirement ('.+') can never + // match an empty path. + ['name' => 'dashboard#page', 'url' => '/{path}', 'verb' => 'GET', 'requirements' => ['path' => '.+'], 'defaults' => ['path' => ''], 'postfix' => 'spa'], ], ]; diff --git a/lib/Controller/SbomController.php b/lib/Controller/SbomController.php index 83686f33..e97a5f6e 100644 --- a/lib/Controller/SbomController.php +++ b/lib/Controller/SbomController.php @@ -41,6 +41,7 @@ use OCA\SoftwareCatalog\Exception\UnsupportedSbomFormatException; use OCA\SoftwareCatalog\Service\SbomImportService; use OCP\AppFramework\Controller; +use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; @@ -94,6 +95,21 @@ public function __construct( /** * Import an SBOM for a `moduleVersie`. Multipart upload only. * + * `DoesNotExistException` is caught here (not just the explicit + * `RuntimeException` "moduleVersie not found" guard already inside + * `SbomImportService::importForModuleVersie()`): OpenRegister's real + * `ObjectService::find()` does not reliably return `null` for a missing + * object the way the unit-test stub does — for a well-formed but + * non-existent uuid its cross-table fallback lookup re-throws + * `OCP\AppFramework\Db\DoesNotExistException` instead. That exception can + * originate from `authorizeManage()` (via + * `SbomImportService::resolveParentModuleUuid()`, for a non-admin/ + * manage-group caller) or from `importForModuleVersie()` itself, so the + * whole method body is covered by one outer try/catch rather than + * threading a guard through each call site individually. Uncaught, this + * escaped as a 500 (confirmed live for `GET .../sbom` on + * `getSbomImportStatus()` — the same defect class applies here). + * * @param string $moduleVersieUuid The target moduleVersie's uuid. * * @return JSONResponse The import result summary, or a 400/401/403/404/422/500. @@ -106,23 +122,28 @@ public function __construct( #[NoCSRFRequired] public function importSbom(string $moduleVersieUuid): JSONResponse { - $guard = $this->authorizeManage(moduleVersieUuid: $moduleVersieUuid); - if ($guard instanceof JSONResponse) { - return $guard; - } + try { + $guard = $this->authorizeManage(moduleVersieUuid: $moduleVersieUuid); + if ($guard instanceof JSONResponse) { + return $guard; + } - $validated = $this->validateUpload(moduleVersieUuid: $moduleVersieUuid); - if ($validated instanceof JSONResponse) { - return $validated; - } + $validated = $this->validateUpload(moduleVersieUuid: $moduleVersieUuid); + if ($validated instanceof JSONResponse) { + return $validated; + } - try { $result = $this->importService->importForModuleVersie( moduleVersieUuid: $moduleVersieUuid, rawJson: $validated['contents'], format: $validated['format'], fileName: $validated['fileName'] ); + } catch (DoesNotExistException $e) { + return new JSONResponse( + data: ['message' => 'moduleVersie not found: '.$moduleVersieUuid, 'error' => 'MODULE_VERSION_NOT_FOUND'], + statusCode: Http::STATUS_NOT_FOUND + ); } catch (UnsupportedSbomFormatException $e) { return new JSONResponse( data: ['message' => $e->getMessage(), 'error' => 'UNSUPPORTED_SBOM_FORMAT'], @@ -228,12 +249,26 @@ private function validateUpload(string $moduleVersieUuid): array|JSONResponse * Read SBOM import status/provenance for a `moduleVersie`, optionally * including a `progress-tracking` snapshot when `operationId` is given. * + * Confirmed live (500 on a non-existent `moduleVersieUuid`): + * `SbomImportService::getStatus()` calls OpenRegister's real + * `ObjectService::find()`, which — despite its `?ObjectEntity` return + * type suggesting `null` on a miss (and despite the local + * `if ($moduleVersie !== null)` guard already inside `getStatus()`) — + * can re-throw `OCP\AppFramework\Db\DoesNotExistException` from its + * cross-table fallback lookup for a well-formed but unresolvable uuid, + * rather than returning `null`. Uncaught, that propagated straight + * through this controller method as a 500. Caught here and translated + * to a proper 404 — the endpoint is a plain "read status for this uuid" + * lookup, so a missing `moduleVersie` is an ordinary not-found, not a + * server error. + * * @param string $moduleVersieUuid The target moduleVersie's uuid. * - * @return JSONResponse `{sbomLastImportedAt, sbomFormat, sbomFileName, progress}`. + * @return JSONResponse `{sbomLastImportedAt, sbomFormat, sbomFileName, progress}`, or a 404. * * @NoAdminRequired * @spec openspec/specs/sbom-import/spec.md#requirement-large-imports-run-in-bounded-batches-with-progress-reporting + * @spec openspec/specs/sbom-import/spec.md#requirement-moduleversie-records-sbom-import-provenance */ #[NoAdminRequired] public function getSbomImportStatus(string $moduleVersieUuid): JSONResponse @@ -247,12 +282,19 @@ public function getSbomImportStatus(string $moduleVersieUuid): JSONResponse $operationId = null; } - return new JSONResponse( - data: $this->importService->getStatus( - moduleVersieUuid: $moduleVersieUuid, - operationId: $operationId - ) - ); + try { + return new JSONResponse( + data: $this->importService->getStatus( + moduleVersieUuid: $moduleVersieUuid, + operationId: $operationId + ) + ); + } catch (DoesNotExistException $e) { + return new JSONResponse( + data: ['message' => 'moduleVersie not found: '.$moduleVersieUuid, 'error' => 'MODULE_VERSION_NOT_FOUND'], + statusCode: Http::STATUS_NOT_FOUND + ); + } }//end getSbomImportStatus() /** diff --git a/lib/Service/FacetService.php b/lib/Service/FacetService.php index bd9a2326..7c6a8e25 100644 --- a/lib/Service/FacetService.php +++ b/lib/Service/FacetService.php @@ -437,7 +437,7 @@ private function fetchBaseObjects(ObjectService $objectService, string $schema, $pagedQuery['_page'] = $page; $paginated = $objectService->searchObjectsPaginated($pagedQuery); - $results = $paginated['results'] ?? []; + $results = array_map([$this, 'normalizeObject'], $paginated['results'] ?? []); $allObjects = array_merge($allObjects, $results); $totalPages = (int) ($paginated['pages'] ?? 1); @@ -464,6 +464,52 @@ private function fetchBaseObjects(ObjectService $objectService, string $schema, }//end fetchBaseObjects() + /** + * Normalize a single OpenRegister search result into a plain data-bag + * array, tolerant of the several shapes OpenRegister can hand back. + * + * `ObjectService::searchObjectsPaginated()`/`searchObjects()` return + * `OCA\OpenRegister\Db\ObjectEntity` instances in production (confirmed + * live: `ObjectEntity` given where an `array` was assumed), NOT plain + * arrays — only the unit-test stub returned arrays, which is how this + * shipped green and dead. This is the single boundary every OR search + * result MUST pass through before any other FacetService method (all of + * which are `array`-typed) touches it. + * + * Preference order: already an array → returned as-is. An `ObjectEntity` + * (or any object exposing `jsonSerialize()`) → `jsonSerialize()`, which + * merges the payload properties with `@self` metadata AND mirrors the id + * at the top level (see `ObjectEntity::jsonSerialize()`), so both the + * payload fields `extractRelatedIdentifiers()`/`extractRelatedNames()` + * read AND the `id`/`@self.id` shapes `objectIdentifier()` reads survive. + * An object exposing only `getObject()` → that (payload only, `id` + * still present at top level per `ObjectEntity::getObject()`). Anything + * else → cast to array as a last-resort fallback. + * + * @param mixed $object A single OpenRegister search result entry. + * + * @return array The normalized data-bag array. + * + * @spec openspec/specs/gemma-faceted-search/spec.md#requirement-facet-aggregation-endpoint-returns-gemma-dimension-counts + */ + private function normalizeObject(mixed $object): array + { + if (is_array($object) === true) { + return $object; + } + + if (is_object($object) === true && method_exists($object, 'jsonSerialize') === true) { + return (array) $object->jsonSerialize(); + } + + if (is_object($object) === true && method_exists($object, 'getObject') === true) { + return (array) $object->getObject(); + } + + return (array) $object; + + }//end normalizeObject() + /** * Resolve the module object(s) backing each base object's GEMMA dimensions. * @@ -567,8 +613,16 @@ private function fetchModulesByIdentifiers(ObjectService $objectService, array $ return []; } + if (is_array($results) === false) { + // Defensive: `searchObjects()`'s declared return type is + // `array|int` (a `count`-mode caller could pass a shape that + // resolves to an int); this call site is never in count mode, + // but stay defensive rather than fatal on an unexpected shape. + return []; + } + $byId = []; - foreach ($results as $module) { + foreach (array_map([$this, 'normalizeObject'], $results) as $module) { $key = $this->objectIdentifier(object: $module); $byId[$key] = $module; } diff --git a/src/views/organisaties/PortfolioReport.vue b/src/views/organisaties/PortfolioReport.vue index 9b1428dd..b904b27b 100644 --- a/src/views/organisaties/PortfolioReport.vue +++ b/src/views/organisaties/PortfolioReport.vue @@ -352,6 +352,33 @@ export default { /** * Load the organisation collection the picker depends on. + * + * `getSchemaConfig('organisatie')` cannot be used here: it only + * resolves a type from the `voorzieningenConfig` blob when that + * blob's `_schema` key holds a NUMERIC schema id, and + * `organisatie_schema` is unset on instances where the voorzieningen + * auto-configure flow never wrote it (it only ever populates + * `module`/`compliancy`/`moduleVersie`/`sbomComponent` — see + * `SettingsService::normalizeVoorzieningenConfig()`). That is exactly + * why the picker rendered "No results" despite the register holding + * real `Gemeente`/`Samenwerking`/supplier organisations: the schema + * never got registered, so `fetchCollection()` threw + * "Object type ... is not registered" before any request was even + * sent. + * + * The manifest-driven `Organisaties` index page (`src/manifest.json` + * `pages[].config` for route `/organisaties`) never hits this gap: the + * shared library's self-fetch path + * (`node_modules/@conduction/nextcloud-vue/src/components/CnIndexPage/useSelfFetchList.js`) + * calls `registerObjectType(type, props.schema, props.register, ...)` + * using the SCHEMA SLUG itself (`'organisatie'`) as the id — which + * OpenRegister's `/api/objects/{register}/{schemaSlugOrId}` accepts + * interchangeably with a numeric id — rather than resolving a numeric + * schema id from a config blob first. This mirrors that proven path: + * register the slug directly against the voorzieningen register id + * (which IS reliably populated — `voorzieningenConfig.register`), + * with no dependency on `organisatie_schema` ever being set. + * * @return {Promise} */ async loadOrganisations() { @@ -359,17 +386,17 @@ export default { if (!objectStore.settings && typeof objectStore.fetchSettings === 'function') { await objectStore.fetchSettings() } + const voorzieningenConfig = objectStore.settings?.voorzieningen + || objectStore.settings?.voorzieningenConfig + || {} + const registerId = voorzieningenConfig.register if (typeof objectStore.registerObjectType === 'function' + && registerId && !objectStore.objectTypeRegistry?.organisatie) { - let cfg = null - try { - cfg = objectStore.getSchemaConfig?.('organisatie') - } catch (cfgError) { - // getSchemaConfig throws when no schema/register resolves; fall through. - } - if (cfg?.register && cfg?.schema) { - objectStore.registerObjectType('organisatie', cfg.schema, cfg.register) - } + objectStore.registerObjectType('organisatie', 'organisatie', registerId, { + registerSlug: 'voorzieningen', + schemaSlug: 'organisatie', + }) } if (typeof objectStore.fetchCollection === 'function') { await objectStore.fetchCollection('organisatie', { _limit: 1000 }) diff --git a/tests/Unit/Controller/SbomControllerTest.php b/tests/Unit/Controller/SbomControllerTest.php index f53281fb..b2284af5 100644 --- a/tests/Unit/Controller/SbomControllerTest.php +++ b/tests/Unit/Controller/SbomControllerTest.php @@ -27,6 +27,7 @@ use OCA\SoftwareCatalog\Controller\SbomController; use OCA\SoftwareCatalog\Service\SbomImportService; +use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Http; use OCP\IGroupManager; use OCP\IRequest; @@ -306,4 +307,68 @@ public function testStatusRefusesUnauthenticated(): void $this->assertSame(Http::STATUS_UNAUTHORIZED, $response->getStatus()); }//end testStatusRefusesUnauthenticated() + + /** + * REGRESSION: a non-existent `moduleVersieUuid` returns 404, not 500. + * + * Confirmed live: `GET /apps/softwarecatalog/api/moduleversies/{uuid}/sbom` + * for a well-formed but non-existent uuid 500'd with an uncaught + * `OCP\AppFramework\Db\DoesNotExistException` ("Object with identifier + * '...' not found in any magic table") — OpenRegister's real + * `ObjectService::find()` re-throws instead of returning `null` for this + * shape, unlike the assumption `SbomImportService::getStatus()`'s own + * `if ($moduleVersie !== null)` guard was written under. + * + * @return void + */ + public function testStatusReturns404ForNonExistentModuleVersie(): void + { + $request = $this->createMock(IRequest::class); + $request->method('getParam')->willReturn(null); + $this->userSession = $this->createMock(IUserSession::class); + $this->groupManager = $this->createMock(IGroupManager::class); + $this->importService = $this->createMock(SbomImportService::class); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('caller-uid'); + $this->userSession->method('getUser')->willReturn($user); + + $this->importService->method('getStatus') + ->willThrowException(new DoesNotExistException("Object with identifier '00000000-0000-0000-0000-000000000000' not found in any magic table")); + + $controller = new SbomController( + $request, + $this->userSession, + $this->groupManager, + $this->importService, + $this->createMock(LoggerInterface::class) + ); + + $response = $controller->getSbomImportStatus('00000000-0000-0000-0000-000000000000'); + + $this->assertSame(Http::STATUS_NOT_FOUND, $response->getStatus()); + $this->assertSame('MODULE_VERSION_NOT_FOUND', $response->getData()['error'] ?? null); + }//end testStatusReturns404ForNonExistentModuleVersie() + + /** + * REGRESSION: `importSbom()` also translates a `DoesNotExistException` + * escaping `SbomImportService::importForModuleVersie()` (same underlying + * OpenRegister `find()` behaviour as `testStatusReturns404ForNonExistentModuleVersie()`) + * to 404, not 500. + * + * @return void + */ + public function testImportReturns404ForNonExistentModuleVersie(): void + { + $upload = $this->uploadedFile('{"bomFormat":"CycloneDX","specVersion":"1.6","components":[]}'); + + $controller = $this->makeController(isAdmin: true, memberGroups: [], uploadedFile: $upload); + $this->importService->method('importForModuleVersie') + ->willThrowException(new DoesNotExistException("Object with identifier '00000000-0000-0000-0000-000000000000' not found in any magic table")); + + $response = $controller->importSbom('00000000-0000-0000-0000-000000000000'); + + $this->assertSame(Http::STATUS_NOT_FOUND, $response->getStatus()); + $this->assertSame('MODULE_VERSION_NOT_FOUND', $response->getData()['error'] ?? null); + }//end testImportReturns404ForNonExistentModuleVersie() }//end class diff --git a/tests/Unit/Service/FacetServiceTest.php b/tests/Unit/Service/FacetServiceTest.php index 3f4ad88b..32597eb2 100644 --- a/tests/Unit/Service/FacetServiceTest.php +++ b/tests/Unit/Service/FacetServiceTest.php @@ -38,6 +38,50 @@ use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; +/** + * Minimal fake mirroring the runtime contract of the REAL + * `OCA\OpenRegister\Db\ObjectEntity` (not the test-only abstract stub in + * `tests/Stubs/Db/ObjectEntity.php`, which cannot be instantiated directly): + * `jsonSerialize()` merges the payload properties with `@self` metadata and + * mirrors `id` at the top level, exactly as + * `OCA\OpenRegister\Db\ObjectEntity::jsonSerialize()` does in production. + * + * `searchObjectsPaginated()`/`searchObjects()` return real `ObjectEntity` + * instances in production, never plain arrays — the OLD test double fed + * `FacetService` plain arrays only, which is exactly how the production 500 + * (`FacetService::objectIdentifier(): Argument #1 ($object) must be of type + * array, OCA\OpenRegister\Db\ObjectEntity given`) shipped green. + * + * @category Test + * @package OCA\SoftwareCatalog\Tests\Unit\Service + */ +final class FakeObjectEntity implements \JsonSerializable +{ + + /** + * @param string $id The object's identifier. + * @param array $payload The object's payload properties (no `id`/`@self`). + */ + public function __construct(private readonly string $id, private readonly array $payload) + { + } + + /** + * Mirrors `ObjectEntity::jsonSerialize()`: payload + `@self.id` + top-level `id`. + * + * @return array + */ + public function jsonSerialize(): array + { + $data = $this->payload; + $data['@self'] = ['id' => $this->id]; + $data['id'] = $this->id; + + return $data; + + }//end jsonSerialize() +}//end class + /** * Unit tests for FacetService. * @@ -275,6 +319,78 @@ public function testGetFacetsAggregatesDirectModuleFields(): void }//end testGetFacetsAggregatesDirectModuleFields() + /** + * REGRESSION: the SAME assertions as + * `testGetFacetsAggregatesDirectModuleFields()`, but `searchObjectsPaginated()` + * returns `FakeObjectEntity` instances (mirroring the real OpenRegister + * `ObjectEntity` contract) instead of plain arrays — this is the shape + * production actually hands back. Proves `FacetService` normalizes at + * the `fetchBaseObjects()` boundary rather than assuming `array` + * (the exact defect that produced the live 500: + * `objectIdentifier(): Argument #1 ($object) must be of type array, + * OCA\OpenRegister\Db\ObjectEntity given`). + * + * @spec openspec/specs/gemma-faceted-search/spec.md#requirement-facet-aggregation-endpoint-returns-gemma-dimension-counts + * + * @return void + */ + public function testGetFacetsAggregatesDirectModuleFieldsWhenResultsAreObjectEntityInstances(): void + { + $modules = [ + new FakeObjectEntity( + id: 'm1', + payload: [ + 'referentieComponenten' => [['identifier' => 'rc-1']], + 'standaardVersies' => [['name' => 'StUF-ZKN']], + ] + ), + new FakeObjectEntity( + id: 'm2', + payload: [ + 'referentieComponenten' => [['identifier' => 'rc-1']], + 'standaardVersies' => [['name' => 'StUF-ZKN']], + ] + ), + new FakeObjectEntity( + id: 'm3', + payload: [ + 'referentieComponenten' => ['rc-2'], + 'standaardVersies' => [], + ] + ), + ]; + + $captured = []; + $objectService = $this->makePaginatedObjectService(results: $modules, capturedRef: $captured); + + $archiMateService = $this->createMock(ArchiMateService::class); + $archiMateService->method('getElementObjects')->willReturn( + [ + ['identifier' => 'rc-1', 'name' => 'Zaakregistratiecomponent', 'domein' => 'Bedrijfsvoering'], + ['identifier' => 'rc-2', 'name' => 'Klantcontactcomponent', 'domein' => 'Dienstverlening'], + ] + ); + $archiMateService->method('getRelationshipObjects')->willReturn([]); + + $service = $this->makeService(objectService: $objectService, archiMateService: $archiMateService); + + $result = $service->getFacets(schema: 'module'); + + $refCompByValue = array_column($result['referentiecomponent'], 'count', 'value'); + $this->assertSame(2, $refCompByValue['Zaakregistratiecomponent']); + $this->assertSame(1, $refCompByValue['Klantcontactcomponent']); + + $standaardByValue = array_column($result['standaard'], 'count', 'value'); + $this->assertSame(2, $standaardByValue['StUF-ZKN']); + + $domeinByValue = array_column($result['domein'], 'count', 'value'); + $this->assertSame(2, $domeinByValue['Bedrijfsvoering']); + $this->assertSame(1, $domeinByValue['Dienstverlening']); + + $this->assertSame(3, $result['_meta']['totalMatched']); + + }//end testGetFacetsAggregatesDirectModuleFieldsWhenResultsAreObjectEntityInstances() + /** * `applicatieservice` is resolved via a `relation` connecting a referentiecomponent * element to an element with gemmaType === 'Applicatieservice'. @@ -607,4 +723,61 @@ function (array $query) use (&$capturedPaginated): array { $this->assertSame(1, $refCompByValue['Zaakregistratiecomponent']); }//end testGetFacetsResolvesDienstFacetsTransitivelyViaModules() + + /** + * REGRESSION: the SAME scenario as + * `testGetFacetsResolvesDienstFacetsTransitivelyViaModules()`, but BOTH + * OpenRegister boundaries return `FakeObjectEntity` instances instead of + * plain arrays — the bounded base-object page (`searchObjectsPaginated()`, + * the `dienst` results) AND the batch module lookup + * (`searchObjects()`, resolved inside `fetchModulesByIdentifiers()`). + * Proves normalization happens at BOTH boundaries `FacetService` + * consumes OpenRegister search results from, not just the first one. + * + * @spec openspec/specs/gemma-faceted-search/spec.md#requirement-facet-aggregation-endpoint-returns-gemma-dimension-counts + * + * @return void + */ + public function testGetFacetsResolvesDienstFacetsWhenBothLookupsReturnObjectEntityInstances(): void + { + $objectService = $this->createMock(ObjectService::class); + + $capturedPaginated = []; + $objectService->method('searchObjectsPaginated')->willReturnCallback( + function (array $query) use (&$capturedPaginated): array { + $capturedPaginated[] = $query; + return [ + 'results' => [ + new FakeObjectEntity(id: 'd1', payload: ['modules' => [['id' => 'm1']]]), + ], + 'total' => 1, + 'page' => 1, + 'pages' => 1, + ]; + } + ); + $objectService->method('searchObjects')->willReturn( + [ + new FakeObjectEntity( + id: 'm1', + payload: ['referentieComponenten' => [['identifier' => 'rc-1']], 'standaardVersies' => []] + ), + ] + ); + + $archiMateService = $this->createMock(ArchiMateService::class); + $archiMateService->method('getElementObjects')->willReturn( + [['identifier' => 'rc-1', 'name' => 'Zaakregistratiecomponent']] + ); + $archiMateService->method('getRelationshipObjects')->willReturn([]); + + $service = $this->makeService(objectService: $objectService, archiMateService: $archiMateService); + + $result = $service->getFacets(schema: 'dienst'); + + $refCompByValue = array_column($result['referentiecomponent'], 'count', 'value'); + $this->assertSame(1, $refCompByValue['Zaakregistratiecomponent']); + $this->assertSame(1, $result['_meta']['totalMatched']); + + }//end testGetFacetsResolvesDienstFacetsWhenBothLookupsReturnObjectEntityInstances() }//end class