diff --git a/.debt-baseline.json b/.debt-baseline.json index 5175562432..39c206b942 100644 --- a/.debt-baseline.json +++ b/.debt-baseline.json @@ -2,7 +2,7 @@ "eslint.inlineDisables": 14, "eslint.suppressedViolations": 308, "phpcs.inlineIgnores": 3, - "phpmd.suppressWarnings": 182, + "phpmd.suppressWarnings": 180, "phpstan.ignorePatterns": 11, "phpstan.inlineIgnores": 4, "phpunit.skips": 5, diff --git a/lib/AppInfo/Registrar/DerivedStatusListenerRegistrar.php b/lib/AppInfo/Registrar/DerivedStatusListenerRegistrar.php index 5cb9026359..f0207c7625 100644 --- a/lib/AppInfo/Registrar/DerivedStatusListenerRegistrar.php +++ b/lib/AppInfo/Registrar/DerivedStatusListenerRegistrar.php @@ -17,7 +17,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -36,7 +36,7 @@ * * @psalm-suppress UnusedClass * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class DerivedStatusListenerRegistrar { @@ -65,7 +65,7 @@ class DerivedStatusListenerRegistrar { * * @return void * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function register(IRegistrationContext $context): void { // ONE journal for the two listeners. An autowired class is built again diff --git a/lib/Controller/BrcController.php b/lib/Controller/BrcController.php index db1e6f327c..4c1a4769a7 100644 --- a/lib/Controller/BrcController.php +++ b/lib/Controller/BrcController.php @@ -455,13 +455,22 @@ private function destroyDecision(string $uuid): JSONResponse { return $this->zgwService->mappingNotFoundResponse(self::ZGW_API, $resource); } + $object = $resource . ' ' . $uuid; try { - // Validate the besluit exists (will throw if not found). $existingObj = $objectService->find( $uuid, register: $mappingConfig['sourceRegister'], schema: $mappingConfig['sourceSchema'] ); + } catch (\Throwable $e) { + return $this->zgwService->storeFailureResponse(error: $e, operation: 'BRC delete besluit', object: $object); + } + + if ($existingObj === null) { + return $this->zgwService->notFoundResponse(); + } + + try { $existingData = $this->objectToArray(row: $existingObj); // Run destroy business rules. @@ -493,25 +502,12 @@ private function destroyDecision(string $uuid): JSONResponse { // Delete the besluit itself. $objectService->deleteObject(uuid: $uuid); - - $baseUrl = $this->zgwService->buildBaseUrl($this->request, self::ZGW_API, $resource); - $this->zgwService->publishNotification( - self::ZGW_API, - $resource, - $baseUrl . '/' . $uuid, - 'destroy' - ); - - return new JSONResponse(data: [], statusCode: Http::STATUS_NO_CONTENT); } catch (\Throwable $e) { - $this->zgwService->getLogger()->error( - 'BRC delete besluit error: ' . $e->getMessage(), - ['exception' => $e] - ); - return new JSONResponse( - data: ['detail' => 'Not found'], - statusCode: Http::STATUS_NOT_FOUND - ); + return $this->zgwService->storeFailureResponse(error: $e, operation: 'BRC delete besluit', object: $object); }//end try + + $this->zgwService->notifyDeleted(request: $this->request, zgwApi: self::ZGW_API, resource: $resource, uuid: $uuid); + + return new JSONResponse(data: [], statusCode: Http::STATUS_NO_CONTENT); }//end destroyBesluit() }//end class diff --git a/lib/Controller/DrcController.php b/lib/Controller/DrcController.php index c68624e30d..8073a6fd86 100644 --- a/lib/Controller/DrcController.php +++ b/lib/Controller/DrcController.php @@ -30,6 +30,7 @@ namespace OCA\Dossiq\Controller; use OCA\Dossiq\Service\ZgwService; +use OCA\Dossiq\Service\Zgw\ZgwRuleResults; use OCA\Dossiq\Service\Zgw\ZgwSearchScope; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\AnonRateLimit; @@ -649,7 +650,8 @@ private function destroyGebruiksrecht(string $uuid): JSONResponse { /** * Delete an enkelvoudiginformatieobject. * - * Refused while ObjectInformatieObjecten point at it (drc-008a). After a successful delete its + * Refused while ObjectInformatieObjecten point at it (drc-008a), and refused when that could + * not be looked up. After a successful delete its * gebruiksrechten go (drc-008), and its stored files go when the document could be read * before the delete. * @@ -661,9 +663,14 @@ private function destroyEio(string $uuid): JSONResponse { $hasStoredFiles = $this->isReadableEio(uuid: $uuid); // Drc-008a (VNG): Block EIO deletion when OIO relations exist. - if ($this->zgwService->getObjectService() !== null - && empty($this->findOioRelationsForEio(eioUuid: $uuid)) === false - ) { + try { + $relations = $this->findOioRelationsForEio(eioUuid: $uuid); + } catch (\Throwable $e) { + // The search did not run, so nobody knows whether relations point here. Refuse. + return $this->relationCheckNotCompleted(uuid: $uuid, cause: $e->getMessage()); + } + + if (empty($relations) === false) { return new JSONResponse( [ 'detail' => $this->l10n->t('The document cannot be deleted: there are related ObjectInformatieObjecten.'), @@ -701,6 +708,26 @@ private function destroyEio(string $uuid): JSONResponse { return $response; }//end destroyEio() + /** + * The answer when the relation check of a document delete could not run (drc-008a). + * + * The same refusal the rules give, logged with the rule and the document. Nothing is deleted. + * + * @param string $uuid The document uuid. + * @param string $cause Why the check could not run. + * + * @return JSONResponse + */ + private function relationCheckNotCompleted(string $uuid, string $cause): JSONResponse { + $refusal = (new ZgwRuleResults(logger: $this->zgwService->getLogger())) + ->notVerified(rule: 'drc-008a', object: 'document ' . $uuid, cause: $cause); + + return new JSONResponse( + data: $this->zgwService->buildValidationError(ruleResult: $refusal), + statusCode: $refusal['status'] + ); + }//end relationCheckNotCompleted() + /** * Whether the document can be read, which is what decides whether its files are cleaned up. * @@ -1177,6 +1204,8 @@ public function audittrailShow(string $resource, string $uuid, string $auditUuid * @param string $eioUuid The EIO UUID * * @return array List of related object IDs linked to this EIO + * + * @throws \Throwable When a schema cannot be searched. */ private function findOioRelationsForEio(string $eioUuid): array { $objectService = $this->zgwService->getObjectService(); @@ -1237,7 +1266,9 @@ private function findOioRelationsForEio(string $eioUuid): array { * @param string $register The register ID * @param string $schema The schema ID * - * @return array List of related object IDs + * @return array List of related object IDs. Empty means the search ran and found none. + * + * @throws \Throwable When the schema cannot be searched. */ private function searchRelationsInSchema( object $objectService, @@ -1245,36 +1276,30 @@ private function searchRelationsInSchema( string $register, string $schema, ): array { - try { - // Try exact UUID match (OIO may store just the UUID). - $query = $objectService->buildSearchQuery( - requestParams: ['document' => $eioUuid, '_limit' => 1], - register: $register, - schema: $schema - ); - $result = $objectService->searchObjectsPaginated(query: $query); - $ids = $this->extractIdsFromResults(result: $result); - if (empty($ids) === false) { - return $ids; - } - - // Fallback: full-text search by UUID (document field stores - // the full URL, and field-specific LIKE is not supported). - $query = $objectService->buildSearchQuery( - requestParams: ['_search' => $eioUuid, '_limit' => 1], - register: $register, - schema: $schema - ); - $result = $objectService->searchObjectsPaginated(query: $query); - $ids = $this->extractIdsFromResults(result: $result); - if (empty($ids) === false) { - return $ids; - } - } catch (\Throwable $e) { - $this->zgwService->getLogger()->warning( - 'drc-008a: Relation search failed for schema ' . $schema . ': ' . $e->getMessage() - ); - }//end try + // Try exact UUID match (OIO may store just the UUID). + $query = $objectService->buildSearchQuery( + requestParams: ['document' => $eioUuid, '_limit' => 1], + register: $register, + schema: $schema + ); + $result = $objectService->searchObjectsPaginated(query: $query); + $ids = $this->extractIdsFromResults(result: $result); + if (empty($ids) === false) { + return $ids; + } + + // Fallback: full-text search by UUID (document field stores + // the full URL, and field-specific LIKE is not supported). + $query = $objectService->buildSearchQuery( + requestParams: ['_search' => $eioUuid, '_limit' => 1], + register: $register, + schema: $schema + ); + $result = $objectService->searchObjectsPaginated(query: $query); + $ids = $this->extractIdsFromResults(result: $result); + if (empty($ids) === false) { + return $ids; + } return []; }//end searchRelationsInSchema() diff --git a/lib/Listener/DerivedStatusListener.php b/lib/Listener/DerivedStatusListener.php index 3a2a039769..3c39b84eed 100644 --- a/lib/Listener/DerivedStatusListener.php +++ b/lib/Listener/DerivedStatusListener.php @@ -40,7 +40,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -63,7 +63,7 @@ * * @implements IEventListener * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class DerivedStatusListener implements IEventListener { @@ -115,7 +115,7 @@ public function __construct( * * @return void * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function handle(Event $event): void { if ($event instanceof ObjectUpdatingEvent === false) { @@ -171,7 +171,7 @@ public function handle(Event $event): void { * * @return array|null The payload, or null. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function payload(ObjectEntity $entity): ?array { try { @@ -191,7 +191,7 @@ private function payload(ObjectEntity $entity): ?array { * * @return bool True when this is a case. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function isCaseSchema(array $object): bool { $expected = $this->settingsService->getConfigValue('case_schema'); diff --git a/lib/Service/ProcessMining/DwellTimeAnalyzer.php b/lib/Service/ProcessMining/DwellTimeAnalyzer.php index 49fd08adf3..9ed3fe7bae 100644 --- a/lib/Service/ProcessMining/DwellTimeAnalyzer.php +++ b/lib/Service/ProcessMining/DwellTimeAnalyzer.php @@ -112,7 +112,7 @@ public function clock(): string { * * @return array Working days, keyed by statusType id. * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ public function heldTotalsByStatus(array $casesById, ?DateTimeImmutable $now = null): array { if ($this->held === null) { diff --git a/lib/Service/ProcessMiningService.php b/lib/Service/ProcessMiningService.php index 3cda135779..566b123a00 100644 --- a/lib/Service/ProcessMiningService.php +++ b/lib/Service/ProcessMiningService.php @@ -92,7 +92,7 @@ public function __construct( * @return array The structured response body. * * @spec openspec/changes/process-mining-bottlenecks/tasks.md#T01 - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ public function getReport(array $params): array { $to = ($this->dates->tryParse($params['to'] ?? null) ?? $this->dates->today()); diff --git a/lib/Service/Settings/SchemaAnnotationReconciler.php b/lib/Service/Settings/SchemaAnnotationReconciler.php index cf08eace7f..d6cf1059bb 100644 --- a/lib/Service/Settings/SchemaAnnotationReconciler.php +++ b/lib/Service/Settings/SchemaAnnotationReconciler.php @@ -215,7 +215,6 @@ private function reconcileSchemaAnnotationBlocks(object $schemaMapper, int|strin * on a case-type store it never reads. * * @spec openspec/specs/status-transition-engine/spec.md - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md */ private function mergeOntoLiveSchema(object $schemaMapper, string $slug, array $annotations): int { // 🔴 RESOLVE INSIDE OUR OWN REGISTER. An unscoped slug lookup here merged diff --git a/lib/Service/Status/CaseStateFieldRuleProjector.php b/lib/Service/Status/CaseStateFieldRuleProjector.php index e802cd9623..cd63179b23 100644 --- a/lib/Service/Status/CaseStateFieldRuleProjector.php +++ b/lib/Service/Status/CaseStateFieldRuleProjector.php @@ -38,7 +38,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -55,7 +55,7 @@ /** * Projects a case type's per-status field rules onto the live case schema. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class CaseStateFieldRuleProjector { @@ -133,7 +133,7 @@ public function __construct( * * @return array> The states block. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function statesOf(string $caseTypeId): array { $statusTypes = $this->store->rowsOfType( @@ -183,7 +183,7 @@ public function statesOf(string $caseTypeId): array { * * @return bool True when the schema configuration was rewritten. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function publish(string $caseTypeId): bool { if ($caseTypeId === '') { @@ -227,7 +227,7 @@ public function publish(string $caseTypeId): bool { * * @return array The lifecycle block. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function lifecycleOf(array $configuration): array { $lifecycle = ($configuration[self::LIFECYCLE_KEY] ?? []); @@ -245,7 +245,7 @@ private function lifecycleOf(array $configuration): array { * * @return array The states. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function statesIn(array $lifecycle): array { $states = ($lifecycle['states'] ?? []); @@ -268,7 +268,7 @@ private function statesIn(array $lifecycle): array { * * @return array The lifecycle block. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function withStates(array $lifecycle, array $states): array { unset($lifecycle['states']); @@ -289,7 +289,7 @@ private function withStates(array $lifecycle, array $states): array { * * @return object|null The schema entity. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function liveCaseSchema(): ?object { try { @@ -318,7 +318,7 @@ private function liveCaseSchema(): ?object { * * @return array The configuration. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function configurationOf(object $schema): array { $configuration = ($schema->getConfiguration() ?? []); @@ -342,7 +342,7 @@ private function configurationOf(object $schema): array { * * @return bool True when the schema was written. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function write(object $schema, array $configuration, string $caseTypeId): bool { if ($this->schemaMapper === null) { @@ -387,7 +387,7 @@ private function write(object $schema, array $configuration, string $caseTypeId) * * @return mixed The block to write. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public static function carryForwardStates(string $annotationKey, mixed $declared, mixed $live): mixed { if ($annotationKey !== self::LIFECYCLE_KEY || is_array($declared) === false) { @@ -422,7 +422,7 @@ public static function carryForwardStates(string $annotationKey, mixed $declared * * @return array The statusType uuids. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function stateKeysOf(string $caseTypeId): array { $keys = []; @@ -448,7 +448,7 @@ private function stateKeysOf(string $caseTypeId): array { * * @return array> Field names keyed by statusType uuid. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function requiredByStatus(string $caseTypeId): array { $properties = $this->store->rowsOfType( @@ -486,7 +486,7 @@ private function requiredByStatus(string $caseTypeId): array { * * @return array>> The block. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function foldInRequired(array $fields, array $required): array { if ($required === []) { @@ -524,7 +524,7 @@ private function foldInRequired(array $fields, array $required): array { * * @return array The merged states. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function mergeStates(array $live, array $own, array $ownKeys): array { $merged = $live; diff --git a/lib/Service/Status/DerivedStatusEvaluator.php b/lib/Service/Status/DerivedStatusEvaluator.php index 9fe2bf9537..449eb21bab 100644 --- a/lib/Service/Status/DerivedStatusEvaluator.php +++ b/lib/Service/Status/DerivedStatusEvaluator.php @@ -27,7 +27,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -37,7 +37,7 @@ /** * Evaluates a status's declared conditions against a case. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class DerivedStatusEvaluator { @@ -84,7 +84,7 @@ public function __construct( * `satisfied` whether every condition holds, and `unmet` names what * is missing, in declaration order. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function evaluate(array $statusType, array $case): array { $conditions = $this->declaration->derivedWhen(statusType: $statusType); @@ -112,7 +112,7 @@ public function evaluate(array $statusType, array $case): array { * * @return bool * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function holds(array $condition, array $case): bool { $kind = (string)($condition['kind'] ?? ''); @@ -148,7 +148,7 @@ private function holds(array $condition, array $case): bool { * * @return bool * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function hasDocumentOfType(array $case, string $documentType): bool { if ($documentType === '') { @@ -191,7 +191,7 @@ private function hasDocumentOfType(array $case, string $documentType): bool { * * @return mixed The value, or null when the path resolves to nothing. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function read(array $case, string $path): mixed { $cursor = $case; @@ -218,7 +218,7 @@ private function read(array $case, string $path): mixed { * * @return bool * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function isPresent(mixed $value): bool { if ($value === null || $value === false) { @@ -243,7 +243,7 @@ private function isPresent(mixed $value): bool { * * @return string * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function scalar(mixed $value): string { if (is_bool($value) === true) { @@ -273,7 +273,7 @@ private function scalar(mixed $value): string { * * @return string * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function reasonFor(array $condition): string { $label = trim((string)($condition['label'] ?? '')); diff --git a/lib/Service/Status/DerivedStatusService.php b/lib/Service/Status/DerivedStatusService.php index 7283e0b049..caaa19e6c7 100644 --- a/lib/Service/Status/DerivedStatusService.php +++ b/lib/Service/Status/DerivedStatusService.php @@ -29,7 +29,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -42,7 +42,7 @@ /** * Resolves the derived status of a case and the reasons a derivation has not fired. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class DerivedStatusService { @@ -72,7 +72,7 @@ public function __construct( * * @return array}> * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function verdictsFor(array $case): array { $caseTypeId = (string)($case['caseType'] ?? ''); @@ -121,7 +121,7 @@ public function verdictsFor(array $case): array { * * @return bool * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function isDerivedStatus(string $caseTypeId, string $statusTypeId): bool { if ($caseTypeId === '' || $statusTypeId === '') { @@ -161,7 +161,7 @@ public function isDerivedStatus(string $caseTypeId, string $statusTypeId): bool * * @return string|null The statusType id to move to, or null for no move. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function statusFor(array $case): ?string { $currentId = (string)($case['status'] ?? ''); @@ -195,7 +195,7 @@ public function statusFor(array $case): ?string { * * @return array{statusId: string, name: string, unmet: array}|null * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function blockingReasonFor(array $case): ?array { $currentOrder = $this->orderOf(case: $case, statusId: (string)($case['status'] ?? '')); @@ -223,7 +223,7 @@ public function blockingReasonFor(array $case): ?array { * * @return int The order, or 0 when the status does not resolve. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function orderOf(array $case, string $statusId): int { if ($statusId === '') { diff --git a/lib/Service/Status/StatusDeclaration.php b/lib/Service/Status/StatusDeclaration.php index f579351ccb..fe30fa5392 100644 --- a/lib/Service/Status/StatusDeclaration.php +++ b/lib/Service/Status/StatusDeclaration.php @@ -27,7 +27,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -37,7 +37,7 @@ /** * Reads the three declarations a statusType may carry. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class StatusDeclaration { @@ -106,7 +106,7 @@ class StatusDeclaration { * * @return array> The conditions worth evaluating. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function derivedWhen(array $statusType): array { $declared = ($statusType['derivedWhen'] ?? null); @@ -148,7 +148,7 @@ public function derivedWhen(array $statusType): array { * * @return bool True when the status declares at least one condition. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function isDerived(array $statusType): bool { return $this->derivedWhen(statusType: $statusType) !== []; @@ -161,7 +161,7 @@ public function isDerived(array $statusType): bool { * * @return string One of the three values; `us` when the status declares nothing. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function waitingOn(array $statusType): string { $declared = trim((string)($statusType['waitingOn'] ?? '')); @@ -179,7 +179,7 @@ public function waitingOn(array $statusType): string { * * @return int|null The maximum, or null when the status declares none. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function maximumDwell(array $statusType): ?int { $declared = ($statusType['maximumDwell'] ?? null); diff --git a/lib/Service/Status/StatusDeclarations.php b/lib/Service/Status/StatusDeclarations.php index f8edd79a72..1be396e5ff 100644 --- a/lib/Service/Status/StatusDeclarations.php +++ b/lib/Service/Status/StatusDeclarations.php @@ -27,7 +27,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -41,7 +41,7 @@ /** * The declarations of the status a case is in, and the dwell bookkeeping. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class StatusDeclarations { @@ -82,7 +82,7 @@ public function __construct( * derivation: array{statusId: string, name: string, unmet: array}|null * } * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function panelFor(array $case, ?DateTimeImmutable $now = null): array { $statusType = $this->lookup->rowFor(statusTypeId: (string)($case['status'] ?? '')); @@ -106,7 +106,7 @@ public function panelFor(array $case, ?DateTimeImmutable $now = null): array { * * @return bool * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function isDerivedStatus(string $caseTypeId, string $statusTypeId): bool { return $this->derived->isDerivedStatus(caseTypeId: $caseTypeId, statusTypeId: $statusTypeId); @@ -121,7 +121,7 @@ public function isDerivedStatus(string $caseTypeId, string $statusTypeId): bool * * @return array The payload. * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ public function applyStatusChange(array $case, string $toStatus, ?DateTimeImmutable $now = null): array { return $this->dwell->applyStatusChange(case: $case, toStatus: $toStatus, now: $now); @@ -140,7 +140,7 @@ public function applyStatusChange(array $case, string $toStatus, ?DateTimeImmuta * * @return string|null The armed timer uuid, or null when nothing was armed. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function retime(string $caseId, string $toStatus): ?string { $this->timer->cancel(caseId: $caseId, reason: 'the case left the status this clock was about'); diff --git a/lib/Service/Status/StatusDwellService.php b/lib/Service/Status/StatusDwellService.php index 30a40e341d..690393bde9 100644 --- a/lib/Service/Status/StatusDwellService.php +++ b/lib/Service/Status/StatusDwellService.php @@ -40,7 +40,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ declare(strict_types=1); @@ -54,7 +54,7 @@ /** * Writes and reads the dwell numbers the case carries. * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ class StatusDwellService { @@ -83,7 +83,7 @@ public function __construct( * * @return array The payload, with the dwell fields settled. * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ public function applyStatusChange(array $case, string $toStatus, ?DateTimeImmutable $now = null): array { $moment = ($now ?? new DateTimeImmutable()); @@ -118,7 +118,7 @@ public function applyStatusChange(array $case, string $toStatus, ?DateTimeImmuta * * @return int Working days, never negative. * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ public function elapsedWorkingDays(array $case, ?DateTimeImmutable $now = null): int { $entered = $this->enteredAt(case: $case); @@ -146,7 +146,7 @@ public function elapsedWorkingDays(array $case, ?DateTimeImmutable $now = null): * * @return bool False whenever the status declares no maximum. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function isBreached(array $case, ?int $maximum, ?DateTimeImmutable $now = null): bool { if ($maximum === null || $maximum < 1) { @@ -165,7 +165,7 @@ public function isBreached(array $case, ?int $maximum, ?DateTimeImmutable $now = * * @return array{days: int, maximum: int|null, breached: bool, enteredAt: string} * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ public function snapshot(array $case, ?int $maximum, ?DateTimeImmutable $now = null): array { $entered = $this->enteredAt(case: $case); @@ -195,7 +195,7 @@ public function snapshot(array $case, ?int $maximum, ?DateTimeImmutable $now = n * * @return array Working days, keyed by statusType id. * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ public function totalsFor(array $case, ?DateTimeImmutable $now = null): array { $totals = []; @@ -230,7 +230,7 @@ public function totalsFor(array $case, ?DateTimeImmutable $now = null): array { * * @return DateTimeImmutable|null The moment, or null when nothing recorded one. * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ private function enteredAt(array $case): ?DateTimeImmutable { $recorded = $this->dates->tryParse(value: ($case['currentStatusEnteredAt'] ?? null)); @@ -250,7 +250,7 @@ private function enteredAt(array $case): ?DateTimeImmutable { * * @return array * - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ private function addTo(mixed $totals, string $statusTypeId, int $workingDays): array { $rebuilt = []; diff --git a/lib/Service/Status/StatusDwellTimer.php b/lib/Service/Status/StatusDwellTimer.php index 29777173a8..09f23f1377 100644 --- a/lib/Service/Status/StatusDwellTimer.php +++ b/lib/Service/Status/StatusDwellTimer.php @@ -31,7 +31,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -46,7 +46,7 @@ /** * Arms and cancels the engine timer behind a status's maximum dwell. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class StatusDwellTimer { @@ -102,7 +102,7 @@ public function __construct( * * @return string|null The armed timer uuid, or null when the engine is absent or refused. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function arm(string $caseId, string $statusTypeId, int $maximumDwell, string $statusName = ''): ?string { if ($caseId === '' || $maximumDwell < 1) { @@ -189,7 +189,7 @@ public function arm(string $caseId, string $statusTypeId, int $maximumDwell, str * * @return int How many timers were cancelled. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function cancel(string $caseId, string $reason = 'status changed'): int { if ($caseId === '') { @@ -220,7 +220,7 @@ public function cancel(string $caseId, string $reason = 'status changed'): int { * * @return object|null The FlowTimerService. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function engine(): ?object { return $this->settingsService->getOpenRegisterClass(TermijnTimerService::ENGINE_CLASS); @@ -239,7 +239,7 @@ private function engine(): ?object { * * @return void * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function degraded(string $operation, string $caseId, Throwable $error): void { $this->logger->warning( diff --git a/lib/Service/Status/StatusFieldRuleDeclaration.php b/lib/Service/Status/StatusFieldRuleDeclaration.php index 23511bdff0..55cde08a6a 100644 --- a/lib/Service/Status/StatusFieldRuleDeclaration.php +++ b/lib/Service/Status/StatusFieldRuleDeclaration.php @@ -34,7 +34,7 @@ * * @link https://conduction.nl * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ declare(strict_types=1); @@ -44,7 +44,7 @@ /** * Reads a status type's field rules and writes them in OpenRegister's shape. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ class StatusFieldRuleDeclaration { @@ -99,7 +99,7 @@ class StatusFieldRuleDeclaration { * * @return array>> The `fields` block, possibly empty. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function fieldsBlock(array $statusType): array { $declared = ($statusType['fieldRules'] ?? null); @@ -128,7 +128,7 @@ public function fieldsBlock(array $statusType): array { * * @return array{0: string, 1: array}|null The kind and the entry. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function entryOf(mixed $rule): ?array { if (is_array($rule) === false) { @@ -176,7 +176,7 @@ private function entryOf(mixed $rule): ?array { * * @return array The group names. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function groupsOf(array $rule): array { $declared = ($rule['groups'] ?? null); @@ -212,7 +212,7 @@ private function groupsOf(array $rule): array { * * @return array|null The JSONLogic node. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function conditionOf(mixed $condition): ?array { if (is_array($condition) === false) { @@ -248,7 +248,7 @@ private function conditionOf(mixed $condition): ?array { * * @return array>> The block, ordered. * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ private function inPublishedOrder(array $block): array { $ordered = []; diff --git a/lib/Service/StatusTransitionService.php b/lib/Service/StatusTransitionService.php index 0bf5bd3f9c..ce0d5623b2 100644 --- a/lib/Service/StatusTransitionService.php +++ b/lib/Service/StatusTransitionService.php @@ -13,10 +13,13 @@ * - dispatch automatic actions sequentially (via SideEffectDispatcher) * - replay transition history from the `statusRecord` chain * - * Three collaborators carry the concerns that are not transition decisions: - * {@see Transitions\CaseStatusStore} owns every OpenRegister read/write, - * {@see Transitions\TransitionAuthorizer} owns the OR-RBAC group gate, and - * {@see Transitions\TransitionSpecReader} owns the template dialects a + * The engine keeps the decision: which transition, in which order the checks + * run, what is refused and what the caller is told. It asks two collaborators + * for everything else. {@see Transitions\CaseDeclarations} reads what a case + * declares about its status and its moves. {@see Transitions\CaseMoveLedger} + * holds every read and write of the store, the closing result and the side + * effects. {@see Transitions\TransitionAuthorizer} owns the OR-RBAC group gate + * and {@see Transitions\TransitionSpecReader} the template dialects a * transition's guards and actions may be spelled in. * * Identity is ALWAYS derived from IUserSession when the caller does not pass @@ -44,16 +47,11 @@ use OCA\Dossiq\Exception\RefusedException; use OCA\Dossiq\Service\Lifecycle\ProcessOwnedStatusRule; -use OCA\Dossiq\Service\Status\StatusDeclarations; -use OCA\Dossiq\Service\Transitions\CaseResultWriter; -use OCA\Dossiq\Service\Transitions\CaseStatusStore; +use OCA\Dossiq\Service\Transitions\CaseDeclarations; +use OCA\Dossiq\Service\Transitions\CaseMoveLedger; use OCA\Dossiq\Service\Transitions\GuardFailedException; use OCA\Dossiq\Service\Transitions\GuardRegistry; -use OCA\Dossiq\Service\Transitions\SideEffectDispatcher; -use OCA\Dossiq\Service\Transitions\StatusChecklist; use OCA\Dossiq\Service\Transitions\TransitionAuthorizer; -use OCA\Dossiq\Service\Transitions\OfferedTransitions; -use OCA\Dossiq\Service\Transitions\TransitionDeclarations; use OCA\Dossiq\Service\Transitions\TransitionSpecReader; use OCP\IUserSession; use Psr\Log\LoggerInterface; @@ -62,14 +60,6 @@ /** * The status-transition engine. * - * @SuppressWarnings(PHPMD.CouplingBetweenObjects) — the collaborators ARE the - * decomposition: persistence, the group gate, the template dialects, the - * closing result and the status checklist each live in their own class, - * and folding any of them back in to satisfy the count would put the - * concern back where it was split from. - * @SuppressWarnings(PHPMD.ExcessiveParameterList) — same reason, counted at the - * constructor: every parameter is one of those collaborators, injected. - * * @spec openspec/changes/status-transition-engine/tasks.md#T10 */ class StatusTransitionService { @@ -89,34 +79,24 @@ class StatusTransitionService { * * @param WorkflowTemplateLoader $templateLoader Active workflowTemplate loader * @param GuardRegistry $guardRegistry Guard registry - * @param SideEffectDispatcher $sideEffectDispatcher Side-effect dispatcher - * @param CaseStatusStore $store OpenRegister persistence for the engine * @param TransitionAuthorizer $authorizer OR-RBAC group gate * @param TransitionSpecReader $specReader Guard/action shape reader * @param IUserSession $userSession Current session * @param LoggerInterface $logger Logger - * @param CaseResultWriter $resultWriter Closing-result reader/writer - * @param StatusChecklist $statusChecklist The checklist a status brings with it - * @param StatusDeclarations $declarations What a status declares about itself - * @param TransitionDeclarations $declaredMoves What a transition declares about itself - * @param OfferedTransitions $offered Which moves this case offers and which it withholds * @param ProcessOwnedStatusRule $processOwnedStatus Refuses a hand-set status where the case type gives it to the process + * @param CaseDeclarations $declarations What the case declares about its status and its moves + * @param CaseMoveLedger $ledger What a move writes, and what earlier moves wrote */ public function __construct( private readonly WorkflowTemplateLoader $templateLoader, private readonly GuardRegistry $guardRegistry, - private readonly SideEffectDispatcher $sideEffectDispatcher, - private readonly CaseStatusStore $store, private readonly TransitionAuthorizer $authorizer, private readonly TransitionSpecReader $specReader, private readonly IUserSession $userSession, private readonly LoggerInterface $logger, - private readonly CaseResultWriter $resultWriter, - private readonly StatusChecklist $statusChecklist, - private readonly StatusDeclarations $declarations, - private readonly TransitionDeclarations $declaredMoves, - private readonly OfferedTransitions $offered, private readonly ProcessOwnedStatusRule $processOwnedStatus, + private readonly CaseDeclarations $declarations, + private readonly CaseMoveLedger $ledger, ) { }//end __construct() @@ -132,7 +112,7 @@ public function __construct( */ public function getAvailableTransitions(string $caseId, ?string $userId = null): array { $userId = $this->resolveUserId(explicit: $userId); - $case = $this->store->loadCase(caseId: $caseId); + $case = $this->ledger->load(caseId: $caseId); if ($case === null) { return ['transitions' => [], 'current' => []]; } @@ -149,7 +129,7 @@ public function getAvailableTransitions(string $caseId, ?string $userId = null): // The handler asks both questions in the same breath — what can I do // here, and why is the thing I expected not on offer — so they are // answered in one round trip rather than two. - $declared = $this->declarations->panelFor(case: $case); + $declared = $this->declarations->panel(case: $case); $result = [ 'transitions' => [], @@ -157,17 +137,14 @@ public function getAvailableTransitions(string $caseId, ?string $userId = null): // to test for the key would read an absent list as "nothing is in // the way", which is the one wrong answer that looks right. 'withheld' => [], - 'current' => [ - 'statusId' => $currentId, - 'statusName' => $this->store->lookupStatusName(statusTypeId: $currentId), - 'statusColour' => $this->store->lookupStatusColour(statusTypeId: $currentId), - // What this status MEANS, written by an administrator and - // rendered on the case. Published beside the name because both - // come off the same row. - 'statusDescription' => $this->store->lookupStatusDescription(statusTypeId: $currentId), - 'waitingOn' => $declared['waitingOn'], - 'dwell' => $declared['dwell'], - ], + // The name, the colour and what this status MEANS, written by an + // administrator and rendered on the case: all three come off the + // same row. + 'current' => array_merge( + ['statusId' => $currentId], + $this->declarations->statusFacts(statusTypeId: $currentId), + ['waitingOn' => $declared['waitingOn'], 'dwell' => $declared['dwell']], + ), 'derivation' => $declared['derivation'], ]; @@ -184,7 +161,7 @@ public function getAvailableTransitions(string $caseId, ?string $userId = null): // decision about one transition asked five ways, and it lives in its // own class. This engine's job is what happens when a handler TAKES // one. - $sorted = $this->offered->from( + $sorted = $this->declarations->offered( transitions: $transitions, case: $case, currentId: $currentId, @@ -231,7 +208,7 @@ public function execute( ?string $resultTypeId = null, ): array { $userId = $this->resolveUserId(explicit: $userId); - $case = $this->store->loadCase(caseId: $caseId); + $case = $this->ledger->load(caseId: $caseId); if ($case === null) { throw new RuntimeException('case_not_found'); } @@ -263,7 +240,7 @@ public function execute( // H2: Optimistic concurrency guard — re-load the case immediately before writing // and abort if its status changed since we read it (concurrent transition executed // between our guard evaluation and our save). - $caseAtSave = $this->assertNoConcurrentChange( + $caseAtSave = $this->ledger->reloadUnchanged( caseId: $caseId, readVersion: $readVersion, currentId: $currentId, @@ -396,7 +373,7 @@ private function actionOutcome(array $dispatched, string $caseId): array { * @throws RuntimeException When a closing transition carries no result type. * * @spec openspec/specs/status-transition-engine/spec.md - * @spec openspec/changes/what-a-status-declares/specs/doorlooptijd-dashboard/spec.md + * @spec openspec/specs/doorlooptijd-dashboard/spec.md */ private function writeMove( array $caseAtSave, @@ -405,31 +382,16 @@ private function writeMove( ?string $resultTypeId, int $readVersion, ): array { - $caseAtSave = $this->applyClosingResult( + $caseAtSave = $this->ledger->close( case: $caseAtSave, caseId: $caseId, toStatus: $toStatus, resultTypeId: $resultTypeId, ); - $caseAtSave = $this->declarations->applyStatusChange(case: $caseAtSave, toStatus: $toStatus); - - $caseAtSave['status'] = $toStatus; - - // One branch, not two. `isset() === false || is_array() === false` is - // two decision points for one question, and this class sits on PHPMD's - // complexity ceiling: the compound spelling is what pushed it over. - // A missing key reads as null here, and null is not an array. - $self = ($caseAtSave['@self'] ?? null); - if (is_array($self) === false) { - $self = []; - } - - $self['version'] = $readVersion; - $caseAtSave['@self'] = $self; - $savedCase = $this->store->saveCase(case: $caseAtSave); + $caseAtSave = $this->declarations->stamp(case: $caseAtSave, toStatus: $toStatus); - return [$savedCase, (int)(($savedCase['@self']['version'] ?? ($savedCase['version'] ?? 0)))]; + return $this->ledger->save(case: $caseAtSave, toStatus: $toStatus, readVersion: $readVersion); }//end writeMove() /** @@ -461,117 +423,32 @@ private function recordAndDispatch( string $userId, array $evaluatedGuards, ): array { - $label = (string)($transition['label'] ?? ''); $toStatus = (string)($transition['toStatus'] ?? ''); - $record = $this->store->writeStatusRecord( - caseId: $caseId, - toStatus: $toStatus, - fromStatus: $currentId, - label: $label, - comment: $comment, - evaluatedGuards: $evaluatedGuards, - noWorkflowTemplate: false, - actor: $userId, - ); - - $statusRecordId = (string)($record['id'] ?? ''); - $context = [ + $move = [ 'fromStatus' => $currentId, 'toStatus' => $toStatus, - 'transitionLabel' => $label, - 'userId' => $userId, - 'statusRecordUuid' => $statusRecordId, + 'label' => (string)($transition['label'] ?? ''), + 'comment' => $comment, + 'actor' => $userId, + 'evaluatedGuards' => $evaluatedGuards, + 'noWorkflowTemplate' => false, ]; + $record = $this->ledger->record(caseId: $caseId, move: $move); // The status the case just entered brings its own work. The checklist // actions go FIRST, so the tasks a phase asks for exist before whatever - // the transition itself does with them — a notification that lists the + // the transition itself does with them: a notification that lists the // case's open tasks is otherwise sent one dispatch too early. $actions = array_merge( - $this->statusChecklist->actionsFor(statusTypeId: $toStatus, case: $case, actor: $userId), + $this->declarations->checklistActions(statusTypeId: $toStatus, case: $case, actor: $userId), $this->specReader->extractActions(transition: $transition), ); - $dispatched = $this->sideEffectDispatcher->dispatch(actions: $actions, case: $case, transitionContext: $context); + $dispatched = $this->ledger->dispatch(actions: $actions, case: $case, move: $move, record: $record); // Update the statusRecord with the actual dispatched-action results. - $record = $this->persistDispatchedActions( - record: $record, - dispatched: $dispatched, - statusRecordId: $statusRecordId, - ); - - return [$record, $dispatched]; + return [$this->ledger->writeBack(record: $record, dispatched: $dispatched), $dispatched]; }//end recordAndDispatch() - /** - * Write the result a closing transition records, onto the case payload. - * - * Returns the case unchanged when the target status is not final. When it - * is, a result type is REQUIRED: a case that closes with no result answers - * none of the questions an archivist, a citizen or a WOO request will ask - * of it later. - * - * A closing transition also settles the case's END DATE and its ARCHIVAL - * FUTURE, both in this same save. Neither used to happen here. `endDate` - * was written only by the ZGW path (zrc-007a), so a case closed at the desk - * read as still open to every consumer that asks the zaak whether it has an - * einddatum; and the zrc-021 nomination was derived only over the API, so - * the same case closed two ways ended in two different archival states. - * Doing both here, rather than in a job afterwards, is what keeps them - * true of every closed case rather than of most of them. - * - * @param array $case The case payload about to be saved - * @param string $caseId Case UUID - * @param string $toStatus The target statusType UUID - * @param string|null $resultTypeId The chosen resultType UUID, or null - * - * @return array The case payload, with `result`, `endDate` and - * the archival fields set when closing - * - * @throws RuntimeException When a closing transition carries no result type - * - * @spec openspec/specs/status-transition-engine/spec.md - */ - private function applyClosingResult( - array $case, - string $caseId, - string $toStatus, - ?string $resultTypeId, - ): array { - if ($this->resultWriter->isFinalStatus(statusTypeId: $toStatus) === false) { - return $case; - } - - // An end date the case already carries is left alone: a handler who - // backdated the close meant it, and zrc-021 derives from that date. - $endDate = substr((string)($case['endDate'] ?? ''), 0, 10); - if ($endDate === '') { - $endDate = date('Y-m-d'); - } - - $case['endDate'] = $endDate; - - $resultId = $this->resultWriter->resolveClosingResult( - caseId: $caseId, - caseTypeId: (string)($case['caseType'] ?? ''), - resultTypeId: $resultTypeId, - ); - if ($resultId === null) { - return $case; - } - - $case['result'] = $resultId; - - return array_merge( - $case, - $this->resultWriter->archivalFuture( - case: $case, - resultTypeId: (string)$resultTypeId, - endDate: $endDate, - ) - ); - }//end applyClosingResult() - /** * Re-evaluate every server-side precondition for a transition. * @@ -608,45 +485,8 @@ private function assertTransitionAllowed( // are: the list a browser holds was true when it was fetched, and the // advice request can have arrived since. Withholding is the courtesy; // this is the rule. - $withheld = $this->declaredMoves->withheldReasons(transition: $transition, case: $case); - if ($withheld !== []) { - throw new RefusedException( - rule: 'transition-dependency-open', - sentence: 'This move is waiting on ' . $withheld[0] . '.', - status: RefusedException::STATUS_REFUSED, - ); - } - - // 🔑 FOUR EYES IS A NEGATIVE RULE ABOUT AN ACT, NOT A ROLE. The same - // person legitimately approves other cases they did not prepare, so - // there is no role to withhold this from: the engine reads who - // performed the named earlier act ON THIS CASE and refuses them. - // It refuses rather than withholds, deliberately. A colleague opening - // the same case must see the move, and a list that hid it per reader - // would make two handlers disagree about what the case offers. - $refusal = $this->declaredMoves->fourEyesRefusal( - transition: $transition, - caseId: $caseId, - userId: $userId, - ); - if ($refusal !== null) { - throw new RefusedException( - rule: 'transition-four-eyes', - sentence: $this->declaredMoves->refusalSentence(refusal: $refusal), - status: RefusedException::STATUS_FORBIDDEN, - ); - } + $this->declarations->assertOpen(transition: $transition, case: $case, caseId: $caseId, userId: $userId); - // OR-RBAC role-routing gate (ADR-022). At publish time - // WorkflowDefinitionService resolves each transition's assignee role - // to its `roleType.ncGroupId` and freezes the literal group id(s) on - // the transition `authorization` list — the same OR PR #153 gate - // format OR enforces declaratively on schemas that carry an - // x-openregister-lifecycle. `case.status` is a per-caseType dynamic - // state machine with no static lifecycle table, so OR cannot enforce - // it on saveObject; this engine therefore enforces the SAME group - // model here using OR's single trusted membership check (IGroupManager), - // not a bespoke role-resolution scheme. An empty/absent list = open. if ($this->authorizer->isTransitionGroupAuthorized(transition: $transition, userId: $userId) === false) { throw new RefusedException( rule: 'transition-unauthorized', @@ -671,80 +511,6 @@ private function assertTransitionAllowed( return $eval; }//end assertTransitionAllowed() - /** - * Re-load the case immediately before writing and abort when another - * transition landed in the meantime (H2 optimistic concurrency guard). - * - * @param string $caseId Case UUID - * @param int $readVersion The @self.version captured at read time - * @param string $currentId The statusType UUID observed at read time - * - * @return array The freshly loaded case - * - * @throws RuntimeException When the case vanished or was concurrently changed - */ - private function assertNoConcurrentChange( - string $caseId, - int $readVersion, - string $currentId, - ): array { - $caseAtSave = $this->store->loadCase(caseId: $caseId); - if ($caseAtSave === null) { - throw new RuntimeException('case_not_found'); - } - - $versionAtSave = (int)(($caseAtSave['@self']['version'] ?? ($caseAtSave['version'] ?? 0))); - if ($versionAtSave !== $readVersion) { - throw new RefusedException( - rule: 'transition-conflict', - sentence: 'Another change reached this case first. Reload it and try again.', - status: RefusedException::STATUS_REFUSED, - ); - } - - $statusAtSave = (string)($caseAtSave['status'] ?? ''); - if ($statusAtSave !== $currentId) { - throw new RefusedException( - rule: 'transition-conflict', - sentence: 'Another change reached this case first. Reload it and try again.', - status: RefusedException::STATUS_REFUSED, - ); - } - - return $caseAtSave; - }//end assertNoConcurrentChange() - - /** - * Persist the dispatched-action results onto the statusRecord. - * - * @param array $record The statusRecord - * @param array> $dispatched Dispatch results - * @param string $statusRecordId The statusRecord UUID - * - * @return array The (possibly updated) statusRecord - */ - private function persistDispatchedActions( - array $record, - array $dispatched, - string $statusRecordId, - ): array { - if ($statusRecordId === '') { - return $record; - } - - $record['dispatchedActions'] = $dispatched; - try { - return $this->store->updateStatusRecord(record: $record); - } catch (\Throwable $e) { - $this->logger->error( - 'StatusTransitionService: dispatchedActions persist failed', - ['exception' => $e->getMessage(), 'statusRecord' => $statusRecordId], - ); - } - - return $record; - }//end persistDispatchedActions() - /** * Execute an admin-only free-form transition for caseTypes without an active workflow template. * @@ -771,7 +537,7 @@ public function executeFreeForm(string $caseId, string $toStatusId, ?string $com throw new RuntimeException('forbidden_admin_only'); } - $case = $this->store->loadCase(caseId: $caseId); + $case = $this->ledger->load(caseId: $caseId); if ($case === null) { throw new RuntimeException('case_not_found'); } @@ -790,44 +556,29 @@ public function executeFreeForm(string $caseId, string $toStatusId, ?string $com // would read complete and not be. $this->processOwnedStatus->requireHandSetAllowed(caseTypeId: $caseTypeId); - $this->store->assertStatusBelongsToCaseType(caseTypeId: $caseTypeId, statusTypeId: $toStatusId); - - $currentId = (string)($case['status'] ?? ''); - // The dwell bookkeeping belongs to the MOVE, not to the road into it. - // An admin free-form move is still a move, and a case whose dwell reset - // only on the guarded path would report months in a status it entered - // this morning. - $case = $this->declarations->applyStatusChange(case: $case, toStatus: $toStatusId); - $case['status'] = $toStatusId; - $case = $this->store->saveCase(case: $case); - $this->declarations->retime(caseId: $caseId, toStatus: $toStatusId); + $this->ledger->assertStatusOfCaseType(caseTypeId: $caseTypeId, statusTypeId: $toStatusId); - $record = $this->store->writeStatusRecord( - caseId: $caseId, + $move = [ + 'fromStatus' => (string)($case['status'] ?? ''), + 'toStatus' => $toStatusId, + 'label' => 'Free-form transition', + 'comment' => $comment, + 'actor' => $userId, + 'evaluatedGuards' => [], + 'noWorkflowTemplate' => true, + ]; + [$case] = $this->ledger->save( + case: $this->declarations->stamp(case: $case, toStatus: $toStatusId), toStatus: $toStatusId, - fromStatus: $currentId, - label: 'Free-form transition', - comment: $comment, - evaluatedGuards: [], - noWorkflowTemplate: true, - actor: $userId, ); + $this->declarations->retime(caseId: $caseId, toStatus: $toStatusId); - // A status brings its checklist however the case arrived. This path - // dispatched nothing at all before, because it has no transition to - // read actions off — but the work belongs to the phase, not to the road - // into it, so an admin's move brings the tasks too. Only the checklist - // actions run here: there is no transition whose actions could. - $dispatched = $this->sideEffectDispatcher->dispatch( - actions: $this->statusChecklist->actionsFor(statusTypeId: $toStatusId, case: $case, actor: $userId), + $record = $this->ledger->record(caseId: $caseId, move: $move); + $dispatched = $this->ledger->dispatch( + actions: $this->declarations->checklistActions(statusTypeId: $toStatusId, case: $case, actor: $userId), case: $case, - transitionContext: [ - 'fromStatus' => $currentId, - 'toStatus' => $toStatusId, - 'transitionLabel' => 'Free-form transition', - 'userId' => $userId, - 'statusRecordUuid' => (string)($record['id'] ?? ''), - ], + move: $move, + record: $record, ); $outcome = $this->actionOutcome(dispatched: $dispatched, caseId: $caseId); @@ -850,7 +601,7 @@ public function executeFreeForm(string $caseId, string $toStatusId, ?string $com * @spec openspec/specs/status-transition-engine/spec.md */ public function replay(string $caseId): array { - $list = $this->store->findStatusRecords(caseId: $caseId); + $list = $this->ledger->history(caseId: $caseId); if ($list === null) { return ['history' => [], 'replayable' => false]; } diff --git a/lib/Service/Transitions/CaseDeclarations.php b/lib/Service/Transitions/CaseDeclarations.php new file mode 100644 index 0000000000..8df5f7a252 --- /dev/null +++ b/lib/Service/Transitions/CaseDeclarations.php @@ -0,0 +1,187 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * + * @version GIT: + * + * @link https://conduction.nl + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + +declare(strict_types=1); + +namespace OCA\Dossiq\Service\Transitions; + +use OCA\Dossiq\Exception\RefusedException; +use OCA\Dossiq\Service\Status\StatusDeclarations; + +/** + * One reader over what a status, a transition and a checklist declare. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ +class CaseDeclarations { + /** + * Constructor. + * + * @param StatusDeclarations $statuses What a status declares about itself. + * @param TransitionDeclarations $moves What a transition declares about itself. + * @param OfferedTransitions $offered Which moves a case offers and which it withholds. + * @param StatusChecklist $checklist The checklist a status brings with it. + * @param CaseStatusStore $store Reads the row of a status. + */ + public function __construct( + private readonly StatusDeclarations $statuses, + private readonly TransitionDeclarations $moves, + private readonly OfferedTransitions $offered, + private readonly StatusChecklist $checklist, + private readonly CaseStatusStore $store, + ) { + }//end __construct() + + /** + * The name, the colour and the description of a status, as its row holds them. + * + * @param string $statusTypeId The status. + * + * @return array{statusName: string, statusColour: string, statusDescription: string} + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function statusFacts(string $statusTypeId): array { + return [ + 'statusName' => $this->store->lookupStatusName(statusTypeId: $statusTypeId), + 'statusColour' => $this->store->lookupStatusColour(statusTypeId: $statusTypeId), + 'statusDescription' => $this->store->lookupStatusDescription(statusTypeId: $statusTypeId), + ]; + }//end statusFacts() + + /** + * What the status the case is in declares: who it waits on, how long it has lasted, how it was derived. + * + * @param array $case The case. + * + * @return array `waitingOn`, `dwell` and `derivation`. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function panel(array $case): array { + return $this->statuses->panelFor(case: $case); + }//end panel() + + /** + * Which of a workflow's transitions this case offers this user, and which it withholds and why. + * + * @param array $transitions The transitions of the workflow the case runs on. + * @param array $case The case. + * @param string $currentId The status the case is in. + * @param string $userId The user asking. + * + * @return array{transitions: array>, withheld: array>} + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function offered(array $transitions, array $case, string $currentId, string $userId): array { + return $this->offered->from(transitions: $transitions, case: $case, currentId: $currentId, userId: $userId); + }//end offered() + + /** + * Refuse a move its own declaration withholds: an open dependency first, then four eyes. + * + * @param array $transition The transition. + * @param array $case The case. + * @param string $caseId The case uuid. + * @param string $userId The user taking the move. + * + * @return void + * + * @throws RefusedException When something the move waits on is open, or four eyes refuses the user. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function assertOpen(array $transition, array $case, string $caseId, string $userId): void { + $withheld = $this->moves->withheldReasons(transition: $transition, case: $case); + if ($withheld !== []) { + throw new RefusedException( + rule: 'transition-dependency-open', + sentence: 'This move is waiting on ' . $withheld[0] . '.', + status: RefusedException::STATUS_REFUSED, + ); + } + + $refusal = $this->moves->fourEyesRefusal(transition: $transition, caseId: $caseId, userId: $userId); + if ($refusal !== null) { + throw new RefusedException( + rule: 'transition-four-eyes', + sentence: $this->moves->refusalSentence(refusal: $refusal), + status: RefusedException::STATUS_FORBIDDEN, + ); + } + }//end assertOpen() + + /** + * The case payload with the bookkeeping of the status it enters. Saved by the caller. + * + * @param array $case The case payload about to be saved. + * @param string $toStatus The status being entered. + * + * @return array + * + * @spec openspec/specs/doorlooptijd-dashboard/spec.md + */ + public function stamp(array $case, string $toStatus): array { + return $this->statuses->applyStatusChange(case: $case, toStatus: $toStatus); + }//end stamp() + + /** + * Stop the clock on the status the case left and start one on the status it entered. + * + * Called after the save, never before it. + * + * @param string $caseId The case uuid. + * @param string $toStatus The status entered. + * + * @return void + * + * @spec openspec/specs/doorlooptijd-dashboard/spec.md + */ + public function retime(string $caseId, string $toStatus): void { + $this->statuses->retime(caseId: $caseId, toStatus: $toStatus); + }//end retime() + + /** + * The actions the entered status brings with it. + * + * @param string $statusTypeId The status entered. + * @param array $case The saved case. + * @param string $actor The acting user. + * + * @return array> + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function checklistActions(string $statusTypeId, array $case, string $actor): array { + return $this->checklist->actionsFor(statusTypeId: $statusTypeId, case: $case, actor: $actor); + }//end checklistActions() +}//end class diff --git a/lib/Service/Transitions/CaseMoveLedger.php b/lib/Service/Transitions/CaseMoveLedger.php new file mode 100644 index 0000000000..5f7e1ff7ee --- /dev/null +++ b/lib/Service/Transitions/CaseMoveLedger.php @@ -0,0 +1,291 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * + * @version GIT: + * + * @link https://conduction.nl + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + +declare(strict_types=1); + +namespace OCA\Dossiq\Service\Transitions; + +use OCA\Dossiq\Exception\RefusedException; +use Psr\Log\LoggerInterface; +use RuntimeException; + +/** + * The store, the closing result and the side effects of a move, behind one class. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ +class CaseMoveLedger { + /** + * Constructor. + * + * @param CaseStatusStore $store OpenRegister persistence for the engine. + * @param CaseResultWriter $resultWriter Closing-result reader and writer. + * @param SideEffectDispatcher $dispatcher Side-effect dispatcher. + * @param LoggerInterface $logger The logger. + */ + public function __construct( + private readonly CaseStatusStore $store, + private readonly CaseResultWriter $resultWriter, + private readonly SideEffectDispatcher $dispatcher, + private readonly LoggerInterface $logger, + ) { + }//end __construct() + + /** + * The case as stored, or null when it is not there. + * + * @param string $caseId The case uuid. + * + * @return array|null + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function load(string $caseId): ?array { + return $this->store->loadCase(caseId: $caseId); + }//end load() + + /** + * Re-load the case immediately before writing and refuse when another change landed in the meantime. + * + * @param string $caseId The case uuid. + * @param int $readVersion The `@self.version` captured at read time. + * @param string $currentId The status observed at read time. + * + * @return array The freshly loaded case. + * + * @throws RuntimeException When the case is gone. + * @throws RefusedException When its version or its status changed. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function reloadUnchanged(string $caseId, int $readVersion, string $currentId): array { + $caseAtSave = $this->store->loadCase(caseId: $caseId); + if ($caseAtSave === null) { + throw new RuntimeException('case_not_found'); + } + + $versionAtSave = (int)(($caseAtSave['@self']['version'] ?? ($caseAtSave['version'] ?? 0))); + $statusAtSave = (string)($caseAtSave['status'] ?? ''); + if ($versionAtSave !== $readVersion || $statusAtSave !== $currentId) { + throw new RefusedException( + rule: 'transition-conflict', + sentence: 'Another change reached this case first. Reload it and try again.', + status: RefusedException::STATUS_REFUSED, + ); + } + + return $caseAtSave; + }//end reloadUnchanged() + + /** + * Write the result a closing move records onto the case payload. Not saved here. + * + * Returns the case unchanged when the target status is not final. When it + * is, the end date is settled, and with a result the archival future too, + * so all of it lands in the one save of the move. + * + * @param array $case The case payload about to be saved. + * @param string $caseId The case uuid. + * @param string $toStatus The status being entered. + * @param string|null $resultTypeId The chosen result type, or null. + * + * @return array The payload, with `endDate`, `result` and the archival fields when closing. + * + * @throws RuntimeException When a closing move carries no result type. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function close(array $case, string $caseId, string $toStatus, ?string $resultTypeId): array { + if ($this->resultWriter->isFinalStatus(statusTypeId: $toStatus) === false) { + return $case; + } + + $endDate = substr((string)($case['endDate'] ?? ''), 0, 10); + if ($endDate === '') { + $endDate = date('Y-m-d'); + } + + $case['endDate'] = $endDate; + + $resultId = $this->resultWriter->resolveClosingResult( + caseId: $caseId, + caseTypeId: (string)($case['caseType'] ?? ''), + resultTypeId: $resultTypeId, + ); + if ($resultId === null) { + return $case; + } + + $case['result'] = $resultId; + + return array_merge( + $case, + $this->resultWriter->archivalFuture(case: $case, resultTypeId: (string)$resultTypeId, endDate: $endDate) + ); + }//end close() + + /** + * Save the case in the status it enters, once. + * + * With a read version the save carries it as `@self.version`, so the store + * refuses a write another move got in front of. + * + * @param array $case The settled case payload. + * @param string $toStatus The status being entered. + * @param int|null $readVersion The version captured at read time, or null to save without one. + * + * @return array{0: array, 1: int} The saved case and its new version. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function save(array $case, string $toStatus, ?int $readVersion = null): array { + $case['status'] = $toStatus; + if ($readVersion !== null) { + $self = ($case['@self'] ?? null); + if (is_array($self) === false) { + $self = []; + } + + $self['version'] = $readVersion; + $case['@self'] = $self; + } + + $saved = $this->store->saveCase(case: $case); + + return [$saved, (int)(($saved['@self']['version'] ?? ($saved['version'] ?? 0)))]; + }//end save() + + /** + * Refuse a status that is not one of the case type's. + * + * @param string $caseTypeId The case type. + * @param string $statusTypeId The status. + * + * @return void + * + * @throws RuntimeException When the status belongs to another case type. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function assertStatusOfCaseType(string $caseTypeId, string $statusTypeId): void { + $this->store->assertStatusBelongsToCaseType(caseTypeId: $caseTypeId, statusTypeId: $statusTypeId); + }//end assertStatusOfCaseType() + + /** + * Write the status record of a move. + * + * @param string $caseId The case uuid. + * @param array $move `fromStatus`, `toStatus`, `label`, `comment`, `actor`, `evaluatedGuards`, `noWorkflowTemplate`. + * + * @return array The status record. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function record(string $caseId, array $move): array { + return $this->store->writeStatusRecord( + caseId: $caseId, + toStatus: (string)$move['toStatus'], + fromStatus: (string)$move['fromStatus'], + label: (string)$move['label'], + comment: $move['comment'], + evaluatedGuards: $move['evaluatedGuards'], + noWorkflowTemplate: $move['noWorkflowTemplate'], + actor: (string)$move['actor'], + ); + }//end record() + + /** + * Run the actions of a move, in order, with the move and its record as their context. + * + * @param array> $actions The actions to run. + * @param array $case The saved case. + * @param array $move The move, as given to {@see self::record()}. + * @param array $record The status record of the move. + * + * @return array> One result row per action. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function dispatch(array $actions, array $case, array $move, array $record): array { + return $this->dispatcher->dispatch( + actions: $actions, + case: $case, + transitionContext: [ + 'fromStatus' => (string)$move['fromStatus'], + 'toStatus' => (string)$move['toStatus'], + 'transitionLabel' => (string)$move['label'], + 'userId' => (string)$move['actor'], + 'statusRecordUuid' => (string)($record['id'] ?? ''), + ] + ); + }//end dispatch() + + /** + * Write the results of the dispatched actions back onto the status record. + * + * A record without an id is left as it is. A write that fails is logged at + * error and the record is answered as it stood: the move has happened. + * + * @param array $record The status record. + * @param array> $dispatched The result rows. + * + * @return array The record, updated when it could be. + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function writeBack(array $record, array $dispatched): array { + $statusRecordId = (string)($record['id'] ?? ''); + if ($statusRecordId === '') { + return $record; + } + + $record['dispatchedActions'] = $dispatched; + try { + return $this->store->updateStatusRecord(record: $record); + } catch (\Throwable $e) { + $this->logger->error( + 'StatusTransitionService: dispatchedActions persist failed', + ['exception' => $e->getMessage(), 'statusRecord' => $statusRecordId], + ); + } + + return $record; + }//end writeBack() + + /** + * The status records of a case, unsorted, or null when they cannot be read. + * + * @param string $caseId The case uuid. + * + * @return array>|null + * + * @spec openspec/specs/status-transition-engine/spec.md + */ + public function history(string $caseId): ?array { + return $this->store->findStatusRecords(caseId: $caseId); + }//end history() +}//end class diff --git a/lib/Service/WorkQueueService.php b/lib/Service/WorkQueueService.php index 0b3724adca..364206d458 100644 --- a/lib/Service/WorkQueueService.php +++ b/lib/Service/WorkQueueService.php @@ -197,7 +197,7 @@ static function (array $a, array $b): int { * * @return array{ours: int, applicant: int, thirdParty: int, total: int} * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function countByWaitingOn(?string $userId = null): array { $objectService = $this->settingsService->getObjectService(); @@ -243,7 +243,7 @@ public function countByWaitingOn(?string $userId = null): array { * * @return array{ours: int, applicant: int, thirdParty: int, total: int} * - * @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + * @spec openspec/specs/status-transition-engine/spec.md */ public function tallyWaitingOn(array $cases): array { $counts = ['ours' => 0, 'applicant' => 0, 'thirdParty' => 0, 'total' => 0]; diff --git a/lib/Service/Zgw/BrcCrossRegisterSync.php b/lib/Service/Zgw/BrcCrossRegisterSync.php index 628a6b6432..e30a0b1400 100644 --- a/lib/Service/Zgw/BrcCrossRegisterSync.php +++ b/lib/Service/Zgw/BrcCrossRegisterSync.php @@ -115,8 +115,9 @@ public function syncCaseDecisionToZrc(string $caseUrl, string $decisionUrl): voi 'brc-006: Created zaakbesluit for zaak=' . $zaakUuid . ' besluit=' . $decisionUrl ); } catch (\Throwable $e) { - $this->zgwService->getLogger()->warning( - 'brc-006: Failed to create zaakbesluit: ' . $e->getMessage() + $this->zgwService->getLogger()->error( + 'brc-006: the besluit was stored, but its zaakbesluit was not: ' . $e->getMessage(), + ['rule' => 'brc-006', 'resource' => 'zaakbesluiten', 'case' => $zaakUuid, 'decision' => $decisionUrl, 'exception' => $e] ); } }//end syncZaakBesluitToZrc() @@ -155,8 +156,9 @@ public function createOioInDrc(string $decisionUrl, string $ioUrl): void { object: $oioData ); } catch (\Throwable $e) { - $this->zgwService->getLogger()->warning( - 'brc-005a: Failed to create OIO in DRC: ' . $e->getMessage() + $this->zgwService->getLogger()->error( + 'brc-005a: the besluitinformatieobject was stored, but its objectinformatieobject in the DRC was not: ' . $e->getMessage(), + ['rule' => 'brc-005a', 'resource' => 'objectinformatieobjecten', 'decision' => $decisionUrl, 'document' => $ioUrl, 'exception' => $e] ); } }//end createOioInDrc() @@ -217,8 +219,9 @@ public function deleteOiosForDecision(string $decisionUrl): void { } } } catch (\Throwable $e) { - $this->zgwService->getLogger()->warning( - 'BRC: Failed to delete OIOs for besluit: ' . $e->getMessage() + $this->zgwService->getLogger()->error( + 'brc-009: the objectinformatieobjecten of a besluit were not all deleted from the DRC: ' . $e->getMessage(), + ['rule' => 'brc-009', 'resource' => 'objectinformatieobjecten', 'decision' => $decisionUrl, 'exception' => $e] ); }//end try }//end deleteOiosForBesluit() @@ -280,8 +283,9 @@ public function deleteOioByDecisionAndIo(string $decisionUrl, string $ioUrl): vo } } } catch (\Throwable $e) { - $this->zgwService->getLogger()->warning( - 'brc-005b: Failed to delete OIO: ' . $e->getMessage() + $this->zgwService->getLogger()->error( + 'brc-005b: the besluitinformatieobject was deleted, but its objectinformatieobject in the DRC was not: ' . $e->getMessage(), + ['rule' => 'brc-005b', 'resource' => 'objectinformatieobjecten', 'decision' => $decisionUrl, 'document' => $ioUrl, 'exception' => $e] ); }//end try }//end deleteOioByBesluitAndIo() diff --git a/lib/Service/Zgw/BrcDocumentLinkHandler.php b/lib/Service/Zgw/BrcDocumentLinkHandler.php index 4b10853bc9..5b9d46b7b5 100644 --- a/lib/Service/Zgw/BrcDocumentLinkHandler.php +++ b/lib/Service/Zgw/BrcDocumentLinkHandler.php @@ -257,6 +257,7 @@ public function destroy(IRequest $request, string $uuid): JSONResponse { return $this->zgwService->mappingNotFoundResponse(self::ZGW_API, $resource); } + $object = $resource . ' ' . $uuid; try { // Read the BIO to get besluit URL before deletion. $bioObj = $objectService->find( @@ -264,6 +265,15 @@ public function destroy(IRequest $request, string $uuid): JSONResponse { register: $mappingConfig['sourceRegister'], schema: $mappingConfig['sourceSchema'] ); + } catch (\Throwable $e) { + return $this->zgwService->storeFailureResponse(error: $e, operation: 'BRC delete BIO', object: $object); + } + + if ($bioObj === null) { + return $this->zgwService->notFoundResponse(); + } + + try { $bioData = $this->objectToArray(row: $bioObj); // Build the besluit URL from the stored decision UUID. @@ -281,30 +291,18 @@ public function destroy(IRequest $request, string $uuid): JSONResponse { // Delete the BIO. $objectService->deleteObject(uuid: $uuid); + } catch (\Throwable $e) { + return $this->zgwService->storeFailureResponse(error: $e, operation: 'BRC delete BIO', object: $object); + }//end try - // Brc-005b: Delete matching OIO in DRC. - if ($decisionUrl !== '' && $ioUrl !== '') { - $this->sync->deleteOioByDecisionAndIo(decisionUrl: $decisionUrl, ioUrl: $ioUrl); - } + // The BIO is gone. Brc-005b: delete the matching OIO in the DRC; a failure there is + // logged at error by the sync, and the delete still answers 204. + if ($decisionUrl !== '' && $ioUrl !== '') { + $this->sync->deleteOioByDecisionAndIo(decisionUrl: $decisionUrl, ioUrl: $ioUrl); + } - $baseUrl = $this->zgwService->buildBaseUrl($request, self::ZGW_API, $resource); - $this->zgwService->publishNotification( - self::ZGW_API, - $resource, - $baseUrl . '/' . $uuid, - 'destroy' - ); + $this->zgwService->notifyDeleted(request: $request, zgwApi: self::ZGW_API, resource: $resource, uuid: $uuid); - return new JSONResponse(data: [], statusCode: Http::STATUS_NO_CONTENT); - } catch (\Throwable $e) { - $this->zgwService->getLogger()->error( - 'BRC delete BIO error: ' . $e->getMessage(), - ['exception' => $e] - ); - return new JSONResponse( - data: ['detail' => 'Not found'], - statusCode: Http::STATUS_NOT_FOUND - ); - }//end try + return new JSONResponse(data: [], statusCode: Http::STATUS_NO_CONTENT); }//end destroyBesluitInformatieObject() }//end class diff --git a/lib/Service/Zgw/DrcDocumentRelations.php b/lib/Service/Zgw/DrcDocumentRelations.php index 1691dea983..78a0d99ce1 100644 --- a/lib/Service/Zgw/DrcDocumentRelations.php +++ b/lib/Service/Zgw/DrcDocumentRelations.php @@ -31,7 +31,6 @@ use OCA\Dossiq\Service\FieldValidator; use OCA\Dossiq\Service\SettingsService; -use Psr\Log\LoggerInterface; /** * Reads the stored relations of a document. @@ -46,14 +45,12 @@ class DrcDocumentRelations { * @param ZgwRuleResults $results Builds the answers of a rule. * @param SettingsService $settingsService The settings service. * @param FieldValidator $fieldValidator The field validator. - * @param LoggerInterface $logger The logger. */ public function __construct( private readonly ZgwRuleLookups $lookups, private readonly ZgwRuleResults $results, private readonly SettingsService $settingsService, private readonly FieldValidator $fieldValidator, - private readonly LoggerInterface $logger, ) { }//end __construct() @@ -63,12 +60,17 @@ public function __construct( * OIO objects store the full informatieobject URL in the 'document' field. * This method searches by UUID first, then by partial URL match if needed. * + * An empty list means both searches ran and found none. A search that cannot + * run throws, so the caller refuses the delete instead of reading "no relations". + * * @param string $register The register ID * @param string $schema The OIO schema ID * @param string $docUuid The document UUID * * @return array Array of matching OIO UUIDs * + * @throws \Throwable When the store cannot be searched. + * * @spec openspec/specs/zgw-business-rules-compliance/spec.md */ public function findOioRelationsForDocument( @@ -77,7 +79,7 @@ public function findOioRelationsForDocument( string $docUuid, ): array { // First try by exact UUID (in case stored as UUID). - $results = $this->lookups->findAllObjectsByField( + $results = $this->lookups->idsByField( register: $register, schema: $schema, field: 'document', @@ -88,56 +90,50 @@ public function findOioRelationsForDocument( } // Also search by partial URL match (OIO may store full URL). - try { - $query = $this->lookups->objectService()->buildSearchQuery( - requestParams: ['document' => '%' . $docUuid . '%', '_limit' => 100], - register: $register, - schema: $schema - ); - $result = $this->lookups->objectService()->searchObjectsPaginated(query: $query); - - $ids = []; - foreach (($result['results'] ?? []) as $obj) { - $data = $obj; - if (is_array($obj) === false) { - $data = $obj->jsonSerialize(); - } - - $id = $data['id'] ?? ($data['@self']['id'] ?? null); - if ($id !== null) { - $ids[] = $id; - } - } - - return $ids; - } catch (\Throwable $e) { - $this->logger->warning( - 'Drc-007: OIO relation search with partial match failed: ' . $e->getMessage() - ); - return []; - }//end try + return $this->lookups->idsByField( + register: $register, + schema: $schema, + field: 'document', + value: '%' . $docUuid . '%' + ); }//end findOioRelationsForDocument() /** * Validate OIO uniqueness: object + informatieobject + objectType (drc-003 VNG). * + * A search that cannot run refuses with the "could not be checked" answer. + * It does not answer "duplicate": that would tell the caller a relation + * exists that nobody found. + * * @param string $ioUrl The informatieobject URL * @param string $objectUrl The object URL (zaak or besluit) * @param string $objectType The object type (zaak or besluit) * - * @return array|null Validation error if duplicate found, null if unique + * @return array|null The refusal, or null when the relation is new * * @spec openspec/specs/zgw-business-rules-compliance/spec.md */ public function checkOioUniqueness(string $ioUrl, string $objectUrl, string $objectType): ?array { $register = $this->lookups->register(); - $oioSchema = $this->settingsService->getConfigValue(key: 'document_link_schema'); - if ($register === '') { return null; } - $duplicateError = $this->results->error( + try { + $duplicate = $this->relationExists(register: $register, ioUrl: $ioUrl, objectUrl: $objectUrl, objectType: $objectType); + } catch (\Throwable $e) { + return $this->results->notVerified( + rule: 'drc-003', + object: 'document ' . $ioUrl . ' on ' . $objectUrl, + cause: $e->getMessage() + ); + } + + if ($duplicate === false) { + return null; + } + + return $this->results->error( status: 400, detail: 'De combinatie informatieobject + object + objectType bestaat al.', invalidParams: [ @@ -148,59 +144,55 @@ public function checkOioUniqueness(string $ioUrl, string $objectUrl, string $obj ), ] ); + }//end checkOioUniqueness() + + /** + * Whether the relation is stored already, as an OIO or as its ZIO or BIO. + * + * Drc-003a/b: a relationship created via ZRC (zaakinformatieobjecten) or BRC + * (besluitinformatieobjecten) counts as a duplicate for OIO creation. + * + * @param string $register The register ID + * @param string $ioUrl The informatieobject URL + * @param string $objectUrl The object URL (zaak or besluit) + * @param string $objectType The object type (zaak or besluit) + * + * @return bool True when a stored relation was found + * + * @throws \Throwable When the store cannot be searched. + */ + private function relationExists(string $register, string $ioUrl, string $objectUrl, string $objectType): bool { + $crossRegister = ['case' => ['case_document_schema', 'case'], 'decision' => ['decision_document_schema', 'decision']]; + $places = [['document_link_schema', 'object']]; + if (isset($crossRegister[$objectType]) === true) { + $places[] = $crossRegister[$objectType]; + } $ioUuid = $this->fieldValidator->extractUuid(url: $ioUrl); $objectUuid = $this->fieldValidator->extractUuid(url: $objectUrl); - // Check OIO schema for existing duplicates. - if ($oioSchema !== '') { + foreach ($places as [$schemaKey, $objectField]) { + $schema = $this->settingsService->getConfigValue(key: $schemaKey); + if ($schema === '') { + continue; + } + $found = $this->searchDuplicateRelation( register: $register, - schema: $oioSchema, + schema: $schema, ioUrl: $ioUrl, objectUrl: $objectUrl, ioUuid: $ioUuid, objectUuid: $objectUuid, - objectField: 'object' + objectField: $objectField ); if ($found === true) { - return $duplicateError; - } - } - - // Drc-003a/b: Also check ZIO/BIO schemas — a relationship created via - // ZRC (zaakinformatieobjecten) or BRC (besluitinformatieobjecten) counts - // as a duplicate for OIO creation. - $crossSchemaKey = ''; - $crossField = ''; - if ($objectType === 'case') { - $crossSchemaKey = 'case_document_schema'; - $crossField = 'case'; - } elseif ($objectType === 'decision') { - $crossSchemaKey = 'decision_document_schema'; - $crossField = 'decision'; - } - - if ($crossSchemaKey !== '') { - $crossSchema = $this->settingsService->getConfigValue(key: $crossSchemaKey); - if ($crossSchema !== '') { - $found = $this->searchDuplicateRelation( - register: $register, - schema: $crossSchema, - ioUrl: $ioUrl, - objectUrl: $objectUrl, - ioUuid: $ioUuid, - objectUuid: $objectUuid, - objectField: $crossField - ); - if ($found === true) { - return $duplicateError; - } + return true; } } - return null; - }//end checkOioUniqueness() + return false; + }//end relationExists() /** * Search for an existing relation in a schema by document+object combination. @@ -214,6 +206,8 @@ public function checkOioUniqueness(string $ioUrl, string $objectUrl, string $obj * @param string $objectField The field name for the object reference * * @return bool True if a duplicate was found + * + * @throws \Throwable When the store cannot be searched. */ private function searchDuplicateRelation( string $register, @@ -224,10 +218,21 @@ private function searchDuplicateRelation( ?string $objectUuid, string $objectField, ): bool { - try { - // Search by full URL. + // Search by full URL. + $query = $this->lookups->objectService()->buildSearchQuery( + requestParams: ['document' => $ioUrl, $objectField => $objectUrl, '_limit' => 1], + register: $register, + schema: $schema + ); + $result = $this->lookups->objectService()->searchObjectsPaginated(query: $query); + if (($result['total'] ?? count($result['results'] ?? [])) > 0) { + return true; + } + + // Fallback: search by UUID (stored data may use UUID instead of URL). + if ($ioUuid !== null && $objectUuid !== null) { $query = $this->lookups->objectService()->buildSearchQuery( - requestParams: ['document' => $ioUrl, $objectField => $objectUrl, '_limit' => 1], + requestParams: ['document' => $ioUuid, $objectField => $objectUuid, '_limit' => 1], register: $register, schema: $schema ); @@ -236,39 +241,18 @@ private function searchDuplicateRelation( return true; } - // Fallback: search by UUID (stored data may use UUID instead of URL). - if ($ioUuid !== null && $objectUuid !== null) { - $query = $this->lookups->objectService()->buildSearchQuery( - requestParams: ['document' => $ioUuid, $objectField => $objectUuid, '_limit' => 1], - register: $register, - schema: $schema - ); - $result = $this->lookups->objectService()->searchObjectsPaginated(query: $query); - if (($result['total'] ?? count($result['results'] ?? [])) > 0) { - return true; - } - - // Full-text search using both UUIDs (field-specific LIKE - // with % is not supported by MagicMapper). - $query = $this->lookups->objectService()->buildSearchQuery( - requestParams: ['_search' => $ioUuid . ' ' . $objectUuid, '_limit' => 1], - register: $register, - schema: $schema - ); - $result = $this->lookups->objectService()->searchObjectsPaginated(query: $query); - if (($result['total'] ?? count($result['results'] ?? [])) > 0) { - return true; - } - }//end if - } catch (\Throwable $e) { - // Fail-closed: treat an indeterminate check as a duplicate to prevent - // inadvertent duplicates when the storage backend is unavailable. - $this->logger->warning( - 'Drc-003: Uniqueness check failed for schema {schema}: {message}', - ['schema' => $schema, 'message' => $e->getMessage()] + // Full-text search using both UUIDs (field-specific LIKE + // with % is not supported by MagicMapper). + $query = $this->lookups->objectService()->buildSearchQuery( + requestParams: ['_search' => $ioUuid . ' ' . $objectUuid, '_limit' => 1], + register: $register, + schema: $schema ); - return true; - }//end try + $result = $this->lookups->objectService()->searchObjectsPaginated(query: $query); + if (($result['total'] ?? count($result['results'] ?? [])) > 0) { + return true; + } + }//end if return false; }//end searchDuplicateRelation() diff --git a/lib/Service/Zgw/DrcObjectLinkRules.php b/lib/Service/Zgw/DrcObjectLinkRules.php index cb11cc96c6..ae0372b6e0 100644 --- a/lib/Service/Zgw/DrcObjectLinkRules.php +++ b/lib/Service/Zgw/DrcObjectLinkRules.php @@ -31,7 +31,6 @@ use OCA\Dossiq\Service\FieldValidator; use OCA\Dossiq\Service\SettingsService; -use Psr\Log\LoggerInterface; /** * Create rules of an objectinformatieobject. @@ -46,7 +45,6 @@ class DrcObjectLinkRules { * @param ZgwRuleResults $results Builds the answers of a rule. * @param SettingsService $settingsService The settings service. * @param FieldValidator $fieldValidator The field validator. - * @param LoggerInterface $logger The logger. * @param DrcDocumentRelations $relations The stored relations of a document. */ public function __construct( @@ -54,7 +52,6 @@ public function __construct( private readonly ZgwRuleResults $results, private readonly SettingsService $settingsService, private readonly FieldValidator $fieldValidator, - private readonly LoggerInterface $logger, private readonly DrcDocumentRelations $relations, ) { }//end __construct() @@ -99,7 +96,7 @@ public function rulesObjectinformatieobjectenCreate(array $body): array { * @param mixed $objectUrl The object URL * @param mixed $objectType The objectType * - * @return array|null The refusal, or null when acceptable or nothing can be looked up + * @return array|null The refusal, or null when acceptable or when there is nothing to look up */ private function checkOioRelations(mixed $ioUrl, mixed $objectUrl, mixed $objectType): ?array { if ($ioUrl === '' || $objectUrl === '' || $this->lookups->objectService() === null) { @@ -189,7 +186,7 @@ private function validateObjectUrl(string $objectUrl, string $objectType): ?arra * @param string $objectUrl The object URL (zaak or besluit) * @param string $objectType The object type (zaak or besluit) * - * @return array|null Validation error if relation doesn't exist, null if valid + * @return array|null The refusal when the relation does not exist or could not be looked up, null if valid */ private function validateOioCrossRegister(string $ioUrl, string $objectUrl, string $objectType): ?array { $ioUuid = $this->fieldValidator->extractUuid(url: $ioUrl) ?? ''; @@ -251,8 +248,11 @@ private function validateOioCrossRegister(string $ioUrl, string $objectUrl, stri ); } } catch (\Throwable $e) { - $this->logger->warning( - 'Drc-004: Cross-register validation failed: ' . $e->getMessage() + // The lookup did not run, so nobody knows whether the counterpart exists. Refuse. + return $this->results->notVerified( + rule: 'drc-004', + object: $objectType . ' ' . $objectUuid . ' with document ' . $ioUuid, + cause: $e->getMessage() ); }//end try diff --git a/lib/Service/Zgw/LookupNotCompleted.php b/lib/Service/Zgw/LookupNotCompleted.php new file mode 100644 index 0000000000..6c7e434d1d --- /dev/null +++ b/lib/Service/Zgw/LookupNotCompleted.php @@ -0,0 +1,53 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * + * @version GIT: + * + * @link https://conduction.nl + * + * @spec openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md + */ + +declare(strict_types=1); + +namespace OCA\Dossiq\Service\Zgw; + +use RuntimeException; +use Throwable; + +/** + * Thrown by a rule lookup whose read failed, naming what it was looking up. + * + * @spec openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md + */ +final class LookupNotCompleted extends RuntimeException { + /** + * Constructor. + * + * @param string $lookup What the lookup was reading, for the log (`reference name "Brief"`). + * @param Throwable $cause Why the read failed. + */ + public function __construct( + public readonly string $lookup, + Throwable $cause, + ) { + parent::__construct(message: $cause->getMessage(), previous: $cause); + }//end __construct() +}//end class diff --git a/lib/Service/Zgw/StoreFailureAnswer.php b/lib/Service/Zgw/StoreFailureAnswer.php new file mode 100644 index 0000000000..ee066d2b08 --- /dev/null +++ b/lib/Service/Zgw/StoreFailureAnswer.php @@ -0,0 +1,123 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * + * @version GIT: + * + * @link https://conduction.nl + * + * @spec openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md + */ + +declare(strict_types=1); + +namespace OCA\Dossiq\Service\Zgw; + +use OCP\AppFramework\Db\DoesNotExistException; +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\JSONResponse; +use Psr\Log\LoggerInterface; +use Throwable; + +/** + * Sorts a store failure into not found (404), not allowed (403) and not reached (503). + * + * @spec openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md + */ +class StoreFailureAnswer { + /** + * The code of the answer when the store could not be reached. + */ + public const NOT_COMPLETED_CODE = 'store-not-reached'; + + /** + * OpenRegister's refusal of a permission. Named as a string: OpenRegister is optional at runtime. + */ + private const NOT_AUTHORIZED = 'OCA\\OpenRegister\\Exception\\NotAuthorizedException'; + + /** + * Constructor. + * + * @param LoggerInterface $logger The logger. + */ + public function __construct( + private readonly LoggerInterface $logger, + ) { + }//end __construct() + + /** + * The answer for a failed store call, logged with what was asked of the store. + * + * OpenRegister says "not found" with a DoesNotExistException (a null answer is + * read as one by the caller, {@see self::notFound()}), and "not allowed" with + * its NotAuthorizedException or any failure carrying code 403. Everything + * else means the store did not complete the call, and nothing may be assumed + * about the object. + * + * @param Throwable $error The failure. + * @param string $operation What was asked (`ZGW delete`). + * @param string $object Which object (`besluiten 1f0c...`). + * + * @return JSONResponse + * + * @spec openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md + */ + public function respond(Throwable $error, string $operation, string $object): JSONResponse { + $context = ['operation' => $operation, 'object' => $object, 'exception' => $error]; + + if ($error instanceof DoesNotExistException) { + $this->logger->info('{operation}: {object} not found', $context); + return $this->notFound(); + } + + if (is_a($error, self::NOT_AUTHORIZED) === true || $error->getCode() === Http::STATUS_FORBIDDEN) { + $this->logger->warning('{operation}: not allowed on {object}: ' . $error->getMessage(), $context); + return new JSONResponse( + data: [ + 'type' => 'PermissionDenied', + 'code' => 'permission_denied', + 'title' => 'U heeft geen toestemming voor deze actie.', + 'status' => Http::STATUS_FORBIDDEN, + 'detail' => 'U heeft geen toestemming voor deze actie.', + ], + statusCode: Http::STATUS_FORBIDDEN + ); + } + + $this->logger->error('{operation}: the store did not complete the call for {object}: ' . $error->getMessage(), $context); + return new JSONResponse( + data: [ + 'code' => self::NOT_COMPLETED_CODE, + 'detail' => 'De registratie kon de opdracht niet voltooien. Probeer het later opnieuw.', + ], + statusCode: ZgwRuleResults::NOT_VERIFIED_STATUS + ); + }//end respond() + + /** + * The answer for an object the store does not hold. + * + * @return JSONResponse + * + * @spec openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md + */ + public function notFound(): JSONResponse { + return new JSONResponse(data: ['detail' => 'Not found'], statusCode: Http::STATUS_NOT_FOUND); + }//end notFound() +}//end class diff --git a/lib/Service/Zgw/ZgwRuleLookups.php b/lib/Service/Zgw/ZgwRuleLookups.php index af9e6b1285..22dc492d86 100644 --- a/lib/Service/Zgw/ZgwRuleLookups.php +++ b/lib/Service/Zgw/ZgwRuleLookups.php @@ -29,7 +29,8 @@ namespace OCA\Dossiq\Service\Zgw; use OCA\Dossiq\Service\SettingsService; -use Psr\Log\LoggerInterface; +use OCP\AppFramework\Db\DoesNotExistException; +use RuntimeException; /** * Store lookups of the ZGW business rules, in the request context. @@ -54,11 +55,9 @@ class ZgwRuleLookups { /** * Constructor. * - * @param LoggerInterface $logger The logger * @param SettingsService $settingsService The settings service */ public function __construct( - private readonly LoggerInterface $logger, private readonly SettingsService $settingsService, ) { }//end __construct() @@ -108,9 +107,11 @@ public function register(): mixed { * @param string $field The field to search by * @param string $value The value to search for * - * @return string|null The object UUID, or null if not found + * @return string|null The object UUID, or null when none carries the value or there is no store * - * @spec openspec/specs/zgw-business-rules-compliance/spec.md + * @throws LookupNotCompleted When the search fails: a type that could not be read does not read as absent. + * + * @spec openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md */ public function findObjectByField( string $register, @@ -118,6 +119,10 @@ public function findObjectByField( string $field, string $value, ): ?string { + if ($this->objectService === null) { + return null; + } + try { $query = $this->objectService->buildSearchQuery( requestParams: [$field => $value, '_limit' => 1], @@ -139,17 +144,16 @@ public function findObjectByField( return $data['id'] ?? ($data['@self']['id'] ?? null); } catch (\Throwable $e) { - $this->logger->warning( - 'Reference resolution failed: ' . $e->getMessage(), - ['field' => $field, 'value' => $value] - ); - return null; + throw new LookupNotCompleted(lookup: "reference {$field} \"{$value}\"", cause: $e); }//end try }//end findObjectByField() /** * Find all objects matching a field value. * + * Without a store in the request context nothing is looked up and the list + * is empty. A search that fails throws. + * * @param string $register The register to search in * @param string $schema The schema to search in * @param string $field The field to match on @@ -157,7 +161,9 @@ public function findObjectByField( * * @return array Array of matching object UUIDs * - * @spec openspec/specs/zgw-business-rules-compliance/spec.md + * @throws LookupNotCompleted When the search fails. + * + * @spec openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md */ public function findAllObjectsByField( string $register, @@ -165,36 +171,65 @@ public function findAllObjectsByField( string $field, string $value, ): array { + if ($this->objectService === null) { + return []; + } + try { - $query = $this->objectService->buildSearchQuery( - requestParams: [$field => $value, '_limit' => 100], - register: $register, - schema: $schema - ); - $result = $this->objectService->searchObjectsPaginated(query: $query); + return $this->idsByField(register: $register, schema: $schema, field: $field, value: $value); + } catch (\Throwable $e) { + throw new LookupNotCompleted(lookup: "reference {$field} \"{$value}\"", cause: $e); + } + }//end findAllObjectsByField() - $ids = []; - foreach (($result['results'] ?? []) as $obj) { - $data = $obj; - if (is_array($obj) === false) { - $data = $obj->jsonSerialize(); - } + /** + * The ids of the objects that carry a field value. A search that cannot run throws. + * + * An empty list means the search ran and found none. + * + * @param string $register The register to search in + * @param string $schema The schema to search in + * @param string $field The field to match on + * @param string $value The field value to search for + * + * @return array The ids of the matching objects + * + * @throws \Throwable When the store cannot be searched. + * + * @spec openspec/specs/zgw-business-rules-compliance/spec.md + */ + public function idsByField( + string $register, + string $schema, + string $field, + string $value, + ): array { + if ($this->objectService === null) { + throw new RuntimeException('No object service in the request context'); + } - $id = $data['id'] ?? ($data['@self']['id'] ?? null); - if ($id !== null) { - $ids[] = $id; - } + $query = $this->objectService->buildSearchQuery( + requestParams: [$field => $value, '_limit' => 100], + register: $register, + schema: $schema + ); + $result = $this->objectService->searchObjectsPaginated(query: $query); + + $ids = []; + foreach (($result['results'] ?? []) as $obj) { + $data = $obj; + if (is_array($obj) === false) { + $data = $obj->jsonSerialize(); } - return $ids; - } catch (\Throwable $e) { - $this->logger->warning( - 'Reference resolution failed: ' . $e->getMessage(), - ['field' => $field, 'value' => $value] - ); - return []; - }//end try - }//end findAllObjectsByField() + $id = $data['id'] ?? ($data['@self']['id'] ?? null); + if ($id !== null) { + $ids[] = $id; + } + } + + return $ids; + }//end idsByField() /** * Look up an object in OpenRegister by UUID and schema key. @@ -202,9 +237,15 @@ public function findAllObjectsByField( * @param string $uuid The object UUID * @param string $schemaKey The settings config key for the schema * - * @return array|null The object data, or null on failure + * Nothing is looked up, and the answer is null, without a store, a register + * in the mapping or a schema setting for the key. An object the store does + * not hold, or does not show this caller, is null too. * - * @spec openspec/specs/zgw-business-rules-compliance/spec.md + * @return array|null The object data, or null when there is none to read + * + * @throws LookupNotCompleted When the read fails for any other reason. + * + * @spec openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md */ public function findBySchemaKey(string $uuid, string $schemaKey): ?array { if ($this->objectService === null) { @@ -224,22 +265,45 @@ public function findBySchemaKey(string $uuid, string $schemaKey): ?array { register: $register, schema: $schema ); - if (is_array($obj) === true) { - return $obj; + } catch (DoesNotExistException $e) { + return null; + } catch (\Throwable $e) { + if ($this->isRefusedRead(error: $e) === true) { + return null; } - return $obj->jsonSerialize(); - } catch (\Throwable $e) { - return null; + throw new LookupNotCompleted(lookup: "{$schemaKey} object {$uuid}", cause: $e); + }//end try + + if ($obj === null || is_array($obj) === true) { + return $obj; } + + return $obj->jsonSerialize(); }//end findBySchemaKey() + /** + * Whether a read failed because the caller may not see the object. + * + * OpenRegister refuses a read with its NotAuthorizedException (code 403), and + * hides an object of another organisation as not found. A rule treats both + * as an object it cannot use, as it did before; only a read that could not + * run is a reason to refuse the request. + * + * @param \Throwable $error The failure. + * + * @return bool + */ + private function isRefusedRead(\Throwable $error): bool { + return is_a($error, 'OCA\\OpenRegister\\Exception\\NotAuthorizedException'); + }//end isRefusedRead() + /** * Whether a stored object already carries this pair of values (identificatie + organisatie). * * Nothing is looked up, and the pair counts as free, without a first value, an * object service, or a register and schema in the mapping. A search that fails - * is logged and counts as free too. + * throws: a duplicate nobody could look for is not a free pair. * * @param string $field1Value First field value (e.g. identificatie) * @param string $field1Search OpenRegister field name to search @@ -248,7 +312,9 @@ public function findBySchemaKey(string $uuid, string $schemaKey): ?array { * * @return bool True when the pair is taken * - * @spec openspec/specs/zgw-business-rules-compliance/spec.md + * @throws LookupNotCompleted When the search fails. + * + * @spec openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md */ public function fieldPairExists( string $field1Value, @@ -301,12 +367,8 @@ public function fieldPairExists( return ($matchCount > 0); } catch (\Throwable $e) { - $this->logger->warning( - 'Uniqueness check failed: ' . $e->getMessage() - ); + throw new LookupNotCompleted(lookup: "uniqueness of {$field1Search} \"{$field1Value}\"", cause: $e); }//end try - - return false; }//end fieldPairExists() /** diff --git a/lib/Service/Zgw/ZgwRuleResults.php b/lib/Service/Zgw/ZgwRuleResults.php index 5c67b99999..43978799ac 100644 --- a/lib/Service/Zgw/ZgwRuleResults.php +++ b/lib/Service/Zgw/ZgwRuleResults.php @@ -4,8 +4,12 @@ * The answers a ZGW business rule gives * * A rule passes with the body it may have enriched, or refuses with a status, - * a detail and the fields it refuses. Stateless: every rules class and every - * collaborator builds the same shapes here. + * a detail and the fields it refuses. Every rules class and every collaborator + * builds the same shapes here. + * + * A rule that could not run its check refuses too, with its own answer: the + * request is not at fault, so it gets no field errors, and the log names the + * rule and the object. * * @category Service * @package OCA\Dossiq\Service\Zgw @@ -28,12 +32,79 @@ namespace OCA\Dossiq\Service\Zgw; +use Psr\Log\LoggerInterface; + /** * Builds the valid and the refused result of a rule. * * @spec openspec/specs/zgw-business-rules-compliance/spec.md */ class ZgwRuleResults { + /** + * The status of a refusal because the check could not run. + * + * The same status the ZGW endpoints answer when OpenRegister is absent. + */ + public const NOT_VERIFIED_STATUS = 503; + + /** + * The code of a refusal because the check could not run. + */ + public const NOT_VERIFIED_CODE = 'check-not-completed'; + + /** + * Constructor. + * + * @param LoggerInterface $logger The logger. + */ + public function __construct( + private readonly LoggerInterface $logger, + ) { + }//end __construct() + + /** + * Refuse because a rule could not run its check, and log which rule and which object. + * + * A lookup that ran and found nothing is an answer. A lookup that could not + * run is not, so the rule refuses instead of passing. The refusal carries no + * field errors: nothing in the request is wrong. + * + * @param string $rule The rule, as the standard names it (drc-004). + * @param string $object What the rule was checking. + * @param string $cause Why the check could not run. + * + * @return array{valid: bool, status: int, detail: string, invalidParams: array, enrichedBody: array} With `code` set. + * + * @spec openspec/specs/zgw-business-rules-compliance/spec.md + */ + public function notVerified(string $rule, string $object, string $cause): array { + $this->logNotVerified(rule: $rule, object: $object, cause: $cause); + + return $this->error( + status: self::NOT_VERIFIED_STATUS, + detail: "De controle van regel {$rule} kon niet worden uitgevoerd. Er is niets gewijzigd. Probeer het later opnieuw.", + code: self::NOT_VERIFIED_CODE + ); + }//end notVerified() + + /** + * Log that a rule could not run its check and that the request is refused for it. + * + * @param string $rule The rule. + * @param string $object What the rule was checking. + * @param string $cause Why the check could not run. + * + * @return void + * + * @spec openspec/specs/zgw-business-rules-compliance/spec.md + */ + public function logNotVerified(string $rule, string $object, string $cause): void { + $this->logger->error( + 'ZGW rule {rule} could not be checked for {object}, so the request is refused: {cause}', + ['rule' => $rule, 'object' => $object, 'cause' => $cause] + ); + }//end logNotVerified() + /** * Build a successful validation result (pass-through). * @@ -55,7 +126,7 @@ public function isValid(array $body): array { /** * Build a validation error result. * - * @param int $status HTTP status code (400 or 403) + * @param int $status HTTP status code (400, 403, or 503 when the check could not run) * @param string $detail Error detail message * @param array $invalidParams Invalid parameter entries * @param string $code Optional error code diff --git a/lib/Service/Zgw/ZtcPublishAndDeleteChecks.php b/lib/Service/Zgw/ZtcPublishAndDeleteChecks.php index 5d89273122..d946fe8b56 100644 --- a/lib/Service/Zgw/ZtcPublishAndDeleteChecks.php +++ b/lib/Service/Zgw/ZtcPublishAndDeleteChecks.php @@ -5,7 +5,8 @@ * * Publishing needs a status type, a final one among them, and a valid-from date. * Deleting is blocked while active cases use the type and asks for confirmation - * when only closed cases do. + * when only closed cases do. A delete check that cannot read the cases blocks + * too: "could not look" is not "no cases". * * @category Service * @package OCA\Dossiq\Service\Zgw @@ -31,6 +32,7 @@ use OCA\Dossiq\Service\SettingsService; use OCA\Dossiq\Service\Support\SearchesObjects; +use RuntimeException; /** * Publish and delete checks of a case type. @@ -40,14 +42,21 @@ class ZtcPublishAndDeleteChecks { use SearchesObjects; + /** + * The name the delete check goes by in the log. + */ + private const DELETE_CHECK = 'ztc case type delete check (REQ-CT-01d)'; + /** * Constructor. * * @param ZgwRuleLookups $lookups Store lookups in the request context. + * @param ZgwRuleResults $results Logs a check that could not run. * @param SettingsService $settingsService The settings service. */ public function __construct( private readonly ZgwRuleLookups $lookups, + private readonly ZgwRuleResults $results, private readonly SettingsService $settingsService, ) { }//end __construct() @@ -169,8 +178,11 @@ private function loadCaseTypeRow(object $objectService, string $register, string * ['blocked' => bool, 'requiresConfirmation' => bool, 'message' => string] * * - blocked=true → 409 Conflict; active cases exist. + * - blocked=true with unverified=true → the cases could not be read, so nobody + * knows whether the type is in use. The caller refuses with the + * "could not be checked" status, not with a conflict. * - requiresConfirmation=true → 200 OK with confirmation prompt (closed-only cases). - * - otherwise → safe to delete. + * - otherwise → the check ran and found no cases: safe to delete. * * @param string $register Register slug. * @param string $caseTypeId Case type id. @@ -181,32 +193,39 @@ private function loadCaseTypeRow(object $objectService, string $register, string */ public function validateDeletion(string $register, string $caseTypeId): array { $default = ['blocked' => false, 'requiresConfirmation' => false, 'message' => '']; - if ($this->lookups->objectService() === null || $caseTypeId === '') { + if ($caseTypeId === '') { return $default; } - $caseSchema = (string)$this->settingsService->getConfigValue(key: 'case_schema'); - try { + $objectService = $this->lookups->objectService(); + if ($objectService === null) { + throw new RuntimeException('No object service in the request context'); + } + $cases = $this->searchObjectsAsArrays( - objectService: $this->lookups->objectService(), + objectService: $objectService, register: $register, - schema: $caseSchema, + schema: (string)$this->settingsService->getConfigValue(key: 'case_schema'), filters: ['caseType' => $caseTypeId], ); - } catch (\Throwable $e) { - return $default; - } + if (count($cases) === 0) { + return $default; + } - if (count($cases) === 0) { - return $default; - } + $finalSlugs = $this->loadFinalStatusSlugs(objectService: $objectService, register: $register, caseTypeId: $caseTypeId); + } catch (\Throwable $e) { + // The cases or their final statuses could not be read. Refuse: the type may be in use. + $this->results->logNotVerified(rule: self::DELETE_CHECK, object: 'case type ' . $caseTypeId, cause: $e->getMessage()); - $finalSlugs = $this->loadFinalStatusSlugs( - objectService: $this->lookups->objectService(), - register: $register, - caseTypeId: $caseTypeId, - ); + return [ + 'blocked' => true, + 'requiresConfirmation' => false, + 'unverified' => true, + 'message' => 'Cannot delete case type: the check for cases that use this type could not be completed. ' + . 'Nothing was deleted. Try again later.', + ]; + }//end try $tally = $this->tallyCaseClosure(cases: $cases, finalSlugs: $finalSlugs); $activeCount = $tally['active']; @@ -229,26 +248,24 @@ public function validateDeletion(string $register, string $caseTypeId): array { }//end validateDeletion() /** - * Load the ids of the final statusTypes of a case type, or an empty list when unreadable. + * Load the ids of the final statusTypes of a case type. An unreadable list throws. * * @param object $objectService The OpenRegister object service. * @param string $register Register slug. * @param string $caseTypeId Case type id. * * @return array The final statusType ids. + * + * @throws \Throwable When the status types cannot be read. */ private function loadFinalStatusSlugs(object $objectService, string $register, string $caseTypeId): array { $statusSchema = (string)$this->settingsService->getConfigValue(key: 'status_type_schema'); - try { - $finalStatusTypes = $this->searchObjectsAsArrays( - objectService: $objectService, - register: $register, - schema: $statusSchema, - filters: ['caseType' => $caseTypeId, 'isFinal' => true], - ); - } catch (\Throwable $e) { - return []; - } + $finalStatusTypes = $this->searchObjectsAsArrays( + objectService: $objectService, + register: $register, + schema: $statusSchema, + filters: ['caseType' => $caseTypeId, 'isFinal' => true], + ); $finalSlugs = []; foreach ($finalStatusTypes as $row) { diff --git a/lib/Service/ZgwBusinessRulesService.php b/lib/Service/ZgwBusinessRulesService.php index f15d18550d..8238bf12ab 100644 --- a/lib/Service/ZgwBusinessRulesService.php +++ b/lib/Service/ZgwBusinessRulesService.php @@ -31,6 +31,8 @@ namespace OCA\Dossiq\Service; +use OCA\Dossiq\Service\Zgw\LookupNotCompleted; +use OCA\Dossiq\Service\Zgw\ZgwRuleResults; use OCA\Dossiq\Service\Zgw\ZgwRulesDispatcher; /** @@ -47,12 +49,14 @@ class ZgwBusinessRulesService { * * @param ZgwZtcRulesService $ztcRules ZTC (Catalogi) rules, used by the concept/publish/destroy guards * @param ZgwRulesDispatcher $dispatcher Routes a validated request to the rules service that owns it + * @param ZgwRuleResults $results Builds the refusal when a rule's read could not run * * @return void */ public function __construct( private readonly ZgwZtcRulesService $ztcRules, private readonly ZgwRulesDispatcher $dispatcher, + private readonly ZgwRuleResults $results, ) { }//end __construct() @@ -89,73 +93,83 @@ public function validate( ?bool $caseClosed = null, bool $hasGeforceerd = true, ): array { - // Set context on the ZTC rules this service guards with directly, and - // on every rules service the dispatcher can route to. - $this->ztcRules->setContext($objectService, $mappingConfig); - $this->dispatcher->setContext($objectService, $mappingConfig); + // A rule that read the store and could not finish the read throws, and every + // such read ends here: nothing decided on it is an answer (REQ-ZBR-022). + try { + // Set context on the ZTC rules this service guards with directly, and + // on every rules service the dispatcher can route to. + $this->ztcRules->setContext($objectService, $mappingConfig); + $this->dispatcher->setContext($objectService, $mappingConfig); - // ---- ZTC cross-cutting concerns (concept protection) ---- - if ($zgwApi === 'catalogi') { - $draftCheck = $this->applyCatalogiDraftRules( - resource: $resource, - action: $action, - body: $body, - existingObject: $existingObject, - parentCaseTypeDraft: $parentCaseTypeDraft - ); - if ($draftCheck !== null) { - return $draftCheck; - } + // ---- ZTC cross-cutting concerns (concept protection) ---- + if ($zgwApi === 'catalogi') { + $draftCheck = $this->applyCatalogiDraftRules( + resource: $resource, + action: $action, + body: $body, + existingObject: $existingObject, + parentCaseTypeDraft: $parentCaseTypeDraft + ); + if ($draftCheck !== null) { + return $draftCheck; + } - $publishGuard = $this->guardCaseTypePublish( - resource: $resource, - action: $action, - body: $body, - existingObject: $existingObject, - mappingConfig: $mappingConfig - ); - if ($publishGuard !== null) { - return $publishGuard; + $publishGuard = $this->guardCaseTypePublish( + resource: $resource, + action: $action, + body: $body, + existingObject: $existingObject, + mappingConfig: $mappingConfig + ); + if ($publishGuard !== null) { + return $publishGuard; + } + + $destroyGuard = $this->guardCaseTypeDestroy( + resource: $resource, + action: $action, + body: $body, + existingObject: $existingObject, + mappingConfig: $mappingConfig + ); + if ($destroyGuard !== null) { + return $destroyGuard; + } + }//end if + + // ---- ZRC cross-cutting concern: closed zaak protection (zrc-007) ---- + if ($caseClosed === true && $hasGeforceerd === false) { + return [ + 'valid' => false, + 'status' => 403, + 'detail' => 'Zaak is afgesloten. Wijzigingen zijn niet toegestaan zonder scope zaken.geforceerd-bijwerken.', + 'code' => 'permission_denied', + 'invalidParams' => [ + [ + 'name' => 'nonFieldErrors', + 'code' => 'zaak-closed', + 'reason' => 'De zaak is afgesloten.', + ], + ], + 'enrichedBody' => [], + ]; } - $destroyGuard = $this->guardCaseTypeDestroy( + // ---- Delegate to per-register rule services ---- + return $this->dispatcher->dispatch( + zgwApi: $zgwApi, resource: $resource, action: $action, body: $body, - existingObject: $existingObject, - mappingConfig: $mappingConfig + existingObject: $existingObject ); - if ($destroyGuard !== null) { - return $destroyGuard; - } - }//end if - - // ---- ZRC cross-cutting concern: closed zaak protection (zrc-007) ---- - if ($caseClosed === true && $hasGeforceerd === false) { - return [ - 'valid' => false, - 'status' => 403, - 'detail' => 'Zaak is afgesloten. Wijzigingen zijn niet toegestaan zonder scope zaken.geforceerd-bijwerken.', - 'code' => 'permission_denied', - 'invalidParams' => [ - [ - 'name' => 'nonFieldErrors', - 'code' => 'zaak-closed', - 'reason' => 'De zaak is afgesloten.', - ], - ], - 'enrichedBody' => [], - ]; - } - - // ---- Delegate to per-register rule services ---- - return $this->dispatcher->dispatch( - zgwApi: $zgwApi, - resource: $resource, - action: $action, - body: $body, - existingObject: $existingObject - ); + } catch (LookupNotCompleted $e) { + return $this->results->notVerified( + rule: "{$zgwApi}/{$resource} {$action}", + object: $e->lookup, + cause: $e->getMessage() + ); + }//end try }//end validate() /** @@ -282,6 +296,17 @@ private function guardCaseTypeDestroy( } $delGuard = $this->ztcRules->validateDeletion($register, $caseTypeId); + if (($delGuard['unverified'] ?? false) === true) { + // The check could not read the cases. Nothing says the request is wrong, so no conflict. + return [ + 'valid' => false, + 'status' => ZgwRuleResults::NOT_VERIFIED_STATUS, + 'detail' => (string)$delGuard['message'], + 'code' => 'destroy_check_not_completed', + 'enrichedBody' => $body, + ]; + } + if ($delGuard['blocked'] === true) { return [ 'valid' => false, diff --git a/lib/Service/ZgwDrcRulesService.php b/lib/Service/ZgwDrcRulesService.php index 6a727b95bb..7e1fc6f686 100644 --- a/lib/Service/ZgwDrcRulesService.php +++ b/lib/Service/ZgwDrcRulesService.php @@ -107,15 +107,13 @@ public function __construct(LoggerInterface $logger, SettingsService $settingsSe lookups: $this->lookups, results: $this->results, settingsService: $settingsService, - fieldValidator: $fieldValidator, - logger: $logger + fieldValidator: $fieldValidator ); $this->objectLinks = new DrcObjectLinkRules( lookups: $this->lookups, results: $this->results, settingsService: $settingsService, fieldValidator: $fieldValidator, - logger: $logger, relations: $this->relations ); }//end __construct() @@ -329,11 +327,17 @@ public function rulesEnkelvoudiginformatieobjectenDestroy( if ($register !== '' && $oioSchema !== '') { // OIO stores the full informatieobject URL in the 'document' field, // so search with both UUID and partial match to find any OIO relations. - $relatedIds = $this->relations->findOioRelationsForDocument( - register: $register, - schema: $oioSchema, - docUuid: $existingId - ); + try { + $relatedIds = $this->relations->findOioRelationsForDocument( + register: $register, + schema: $oioSchema, + docUuid: $existingId + ); + } catch (\Throwable $e) { + // The search did not run, so nobody knows whether relations point here. Refuse. + return $this->results->notVerified(rule: 'drc-007', object: 'document ' . $existingId, cause: $e->getMessage()); + } + if (empty($relatedIds) === false) { return $this->error( status: 400, diff --git a/lib/Service/ZgwRulesBase.php b/lib/Service/ZgwRulesBase.php index 38effd6ef9..d4273a40c0 100644 --- a/lib/Service/ZgwRulesBase.php +++ b/lib/Service/ZgwRulesBase.php @@ -111,8 +111,8 @@ public function __construct( protected readonly SettingsService $settingsService, protected readonly FieldValidator $fieldValidator, ) { - $this->results = new ZgwRuleResults(); - $this->lookups = new ZgwRuleLookups(logger: $logger, settingsService: $settingsService); + $this->results = new ZgwRuleResults(logger: $logger); + $this->lookups = new ZgwRuleLookups(settingsService: $settingsService); }//end __construct() /** diff --git a/lib/Service/ZgwService.php b/lib/Service/ZgwService.php index ec901bbd62..797888a0c9 100644 --- a/lib/Service/ZgwService.php +++ b/lib/Service/ZgwService.php @@ -31,6 +31,7 @@ use OCP\AppFramework\Http\JSONResponse; use OCP\IRequest; use OCA\Dossiq\Service\Zgw\ZgwParentStateResolver; +use OCA\Dossiq\Service\Zgw\StoreFailureAnswer; use OCA\Dossiq\Service\Zgw\ZgwPatchMerger; use Psr\Log\LoggerInterface; @@ -172,6 +173,13 @@ class ZgwService { */ private ?array $cachedRequestBody = null; + /** + * Sorts a failed store call into its answer (built on first use). + * + * @var StoreFailureAnswer|null + */ + private ?StoreFailureAnswer $storeFailures = null; + /** * The OpenRegister ConsumerMapper (loaded dynamically). * @@ -316,6 +324,48 @@ public function getLogger(): LoggerInterface { return $this->logger; }//end getLogger() + /** + * The answer for a store call that failed: 404 not found, 403 not allowed, or 503 not reached. + * + * Every ZGW read, update and delete of a stored object answers its failures through here, + * so a store that was down never reads as an object that is not there (REQ-ZBR-023). + * + * @param \Throwable $error The failure. + * @param string $operation What was asked of the store (`ZGW delete`). + * @param string $object Which object (`besluiten `). + * + * @return JSONResponse + * + * @spec openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md + */ + public function storeFailureResponse(\Throwable $error, string $operation, string $object): JSONResponse { + return $this->storeFailures()->respond(error: $error, operation: $operation, object: $object); + }//end storeFailureResponse() + + /** + * The answer for an object the store does not hold. + * + * @return JSONResponse + * + * @spec openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md + */ + public function notFoundResponse(): JSONResponse { + return $this->storeFailures()->notFound(); + }//end notFoundResponse() + + /** + * The store failure sorter, built on first use. + * + * @return StoreFailureAnswer + */ + private function storeFailures(): StoreFailureAnswer { + if ($this->storeFailures === null) { + $this->storeFailures = new StoreFailureAnswer(logger: $this->logger); + } + + return $this->storeFailures; + }//end storeFailures() + /** * Load ZGW mapping configuration. * @@ -1127,7 +1177,15 @@ public function handleShow( schema: $mappingConfig['sourceSchema'], id: $uuid ); + } catch (\Throwable $e) { + return $this->storeFailureResponse(error: $e, operation: 'ZGW show', object: $resource . ' ' . $uuid); + } + if ($object === null) { + return $this->notFoundResponse(); + } + + try { $baseUrl = $this->buildBaseUrl(request: $request, zgwApi: $zgwApi, resource: $resource); $outboundMapping = $this->createOutboundMapping(mappingConfig: $mappingConfig); @@ -1197,6 +1255,11 @@ public function handleUpdate( return $this->mappingNotFoundResponse(zgwApi: $zgwApi, resource: $resource); } + $existingData = $this->readExisting(uuid: $uuid, mappingConfig: $mappingConfig, operation: 'ZGW update', resource: $resource); + if ($existingData instanceof JSONResponse) { + return $existingData; + } + try { $body = $this->getRequestBody(request: $request); @@ -1210,7 +1273,7 @@ public function handleUpdate( resource: $resource, action: $action, body: $body, - existingObject: $this->findSerialized(uuid: $uuid, mappingConfig: $mappingConfig), + existingObject: $existingData, objectService: $this->objectService, mappingConfig: $mappingConfig, parentCaseTypeDraft: $parentZtDraft, @@ -1226,15 +1289,10 @@ public function handleUpdate( $englishData = $inbound['data']; $englishData['id'] = $uuid; - // For partial updates (PATCH), merge with existing object data. + // For partial updates (PATCH), merge with the existing object read above. if ($partial === true) { - $existing = $this->objectService->find( - $uuid, - register: $mappingConfig['sourceRegister'], - schema: $mappingConfig['sourceSchema'] - ); $englishData = (new ZgwPatchMerger())->merge( - existingData: $existing->jsonSerialize(), + existingData: $existingData, body: $body, englishData: $englishData, mappingConfig: $mappingConfig @@ -1288,20 +1346,44 @@ private function ruleRefusal(array $ruleResult): JSONResponse { ); }//end ruleRefusal() + /** + * The stored object an update or delete acts on, or the answer when it cannot be read. + * + * @param string $uuid The object uuid + * @param array $mappingConfig The ZGW mapping (`sourceRegister`, `sourceSchema`) + * @param string $operation What the request asks (`ZGW delete`), for the log + * @param string $resource The ZGW resource name, for the log + * + * @return array|JSONResponse The object as an array, or the 404, 403 or 503 answer + */ + private function readExisting(string $uuid, array $mappingConfig, string $operation, string $resource): array|JSONResponse { + try { + $existing = $this->findSerialized(uuid: $uuid, mappingConfig: $mappingConfig); + } catch (\Throwable $e) { + return $this->storeFailureResponse(error: $e, operation: $operation, object: $resource . ' ' . $uuid); + } + + return $existing ?? $this->notFoundResponse(); + }//end readExisting() + /** * Find a stored object and serialise it. * * @param string $uuid The object uuid * @param array $mappingConfig The ZGW mapping (`sourceRegister`, `sourceSchema`) * - * @return mixed The object as an array (or whatever the ObjectService returned as one) + * @return array|null The object as an array, or null when the store answered none */ - private function findSerialized(string $uuid, array $mappingConfig): mixed { + private function findSerialized(string $uuid, array $mappingConfig): ?array { $existingObj = $this->objectService->find( $uuid, register: $mappingConfig['sourceRegister'], schema: $mappingConfig['sourceSchema'] ); + if ($existingObj === null) { + return null; + } + if (is_array($existingObj) === false) { return $existingObj->jsonSerialize(); } @@ -1401,18 +1483,12 @@ public function handleDestroy( return $this->mappingNotFoundResponse(zgwApi: $zgwApi, resource: $resource); } - try { - $existingObj = $this->objectService->find( - $uuid, - register: $mappingConfig['sourceRegister'], - schema: $mappingConfig['sourceSchema'] - ); - - $existingData = $existingObj; - if (is_array($existingObj) === false) { - $existingData = $existingObj->jsonSerialize(); - } + $existingData = $this->readExisting(uuid: $uuid, mappingConfig: $mappingConfig, operation: 'ZGW delete', resource: $resource); + if ($existingData instanceof JSONResponse) { + return $existingData; + } + try { $ruleResult = $this->businessRulesService->validate( zgwApi: $zgwApi, resource: $resource, @@ -1433,7 +1509,30 @@ public function handleDestroy( } $this->objectService->deleteObject(uuid: $uuid); + } catch (\Throwable $e) { + return $this->storeFailureResponse(error: $e, operation: 'ZGW delete', object: $resource . ' ' . $uuid); + }//end try + // The object is gone. A notification that fails does not make the delete fail. + $this->notifyDeleted(request: $request, zgwApi: $zgwApi, resource: $resource, uuid: $uuid); + + return new JSONResponse(data: [], statusCode: Http::STATUS_NO_CONTENT); + }//end handleDestroy() + + /** + * Publish the notification of a delete that happened, logging a failure instead of answering it. + * + * @param IRequest $request The request. + * @param string $zgwApi The ZGW API group. + * @param string $resource The ZGW resource name. + * @param string $uuid The deleted object. + * + * @return void + * + * @spec openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md + */ + public function notifyDeleted(IRequest $request, string $zgwApi, string $resource, string $uuid): void { + try { $baseUrl = $this->buildBaseUrl(request: $request, zgwApi: $zgwApi, resource: $resource); $this->publishNotification( zgwApi: $zgwApi, @@ -1441,19 +1540,13 @@ public function handleDestroy( resourceUrl: $baseUrl . '/' . $uuid, action: 'destroy' ); - - return new JSONResponse(data: [], statusCode: Http::STATUS_NO_CONTENT); } catch (\Throwable $e) { $this->logger->error( - 'ZGW delete error: ' . $e->getMessage(), - ['exception' => $e] + 'ZGW delete of {resource} {uuid} happened, but its notification failed: ' . $e->getMessage(), + ['resource' => $resource, 'uuid' => $uuid, 'exception' => $e] ); - return new JSONResponse( - data: ['detail' => 'Not found'], - statusCode: Http::STATUS_NOT_FOUND - ); - }//end try - }//end handleDestroy() + } + }//end notifyDeleted() /** * Handle audit trail index — proxies to OpenRegister's audit trail. diff --git a/lib/Service/ZgwZtcRulesService.php b/lib/Service/ZgwZtcRulesService.php index 862ecda58c..599b77a5ad 100644 --- a/lib/Service/ZgwZtcRulesService.php +++ b/lib/Service/ZgwZtcRulesService.php @@ -117,7 +117,7 @@ public function __construct(LoggerInterface $logger, SettingsService $settingsSe parent::__construct(logger: $logger, settingsService: $settingsService, fieldValidator: $fieldValidator); $this->concept = new ZtcConceptRules(results: $this->results); $this->references = new ZtcTypeReferences(lookups: $this->lookups, settingsService: $settingsService, fieldValidator: $fieldValidator); - $this->lifecycle = new ZtcPublishAndDeleteChecks(lookups: $this->lookups, settingsService: $settingsService); + $this->lifecycle = new ZtcPublishAndDeleteChecks(lookups: $this->lookups, results: $this->results, settingsService: $settingsService); }//end __construct() /** diff --git a/openspec/changes/guards-refuse-when-unsure/proposal.md b/openspec/changes/guards-refuse-when-unsure/proposal.md new file mode 100644 index 0000000000..2d38ee75c5 --- /dev/null +++ b/openspec/changes/guards-refuse-when-unsure/proposal.md @@ -0,0 +1,50 @@ +--- +kind: code +depends_on: [] +--- + +# Proposal: guards-refuse-when-unsure + +## Summary + +A ZGW guard that cannot run its check refuses the request. It no longer passes, and it no longer answers something it did not find. + +- Decision: Ruben, 2026-10-12, decision 224. "Guards refuse when unsure: the case type delete check and document rules drc-004 and drc-007 refuse with a clear error when their check cannot run, like drc-003." + +## Why + +Three guards read the store to decide. When the read failed they let the request through: + +- the case type delete check deleted a case type whose cases it could not read; +- drc-004 stored a link between a document and a case or decision without knowing the counterpart exists; +- drc-007 deleted a document without knowing whether relations still point at it. + +A fourth, drc-003, did refuse, but said "this combination already exists" about a relation nobody had found. A client that reads that answer stops retrying a request that was never wrong. + +"The lookup ran and found nothing" and "the lookup could not run" are two answers. Only the first is a pass. + +## What changes + +1. One refusal for a check that could not run: HTTP 503, code `check-not-completed`, a detail that names the rule, and no `invalidParams` (REQ-ZBR-020). +2. drc-003, drc-004, drc-007 and the controller's own relation check before a document delete (drc-008a) answer it when their search fails. +3. The case type delete check blocks the delete as unverified when it cannot read the cases or the final statuses. The ZGW delete answers 503 with code `destroy_check_not_completed` (REQ-ZBR-021). +4. Every such refusal is logged at error with the rule and the object. + +## Why 503 + +The ZGW error model has two shapes. A `ValidatieFout` (400, with `invalidParams`) says the request is wrong and names the field to fix. A `Fout` covers everything else. A check that did not run says nothing about the request, so it is a `Fout`, and a 4xx would tell the client to change a body that was fine. + +Of the 5xx statuses the ZGW specifications declare 500 on every operation. These endpoints already answer 503 with a `detail` when OpenRegister is absent (`ZgwService::unavailableResponse`). A store that cannot be read is the same situation, so the guards answer the same status. It also tells a client the one useful thing: the request can be sent again unchanged. + +The status is one constant, `ZgwRuleResults::NOT_VERIFIED_STATUS`. + +## Out of scope + +- A setting that is not configured. A rule whose schema setting is empty still skips its lookup, as drc-003 does. +- The cross-register writes of the Besluiten API, which stay best effort, and the 404 a failed ZGW delete answers. Both are in QUESTIONS.md (Q-dossiq-L11-2). +- The other lookups in `ZgwRuleLookups` that answer "none" when a read fails (Q-dossiq-L11-3). + +## Release notes + +- A ZGW request is refused with 503 `check-not-completed` when dossiq could not check a business rule. Nothing is stored or deleted. Send the request again. +- Deleting a case type answers 503 `destroy_check_not_completed` when dossiq could not read the cases of that type. diff --git a/openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md b/openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md new file mode 100644 index 0000000000..ee8888a483 --- /dev/null +++ b/openspec/changes/guards-refuse-when-unsure/specs/zgw-business-rules-compliance/spec.md @@ -0,0 +1,125 @@ +# zgw-business-rules-compliance + +## ADDED Requirements + +### Requirement: A rule that cannot run its check refuses the request (REQ-ZBR-020) + +A ZGW business rule that reads the store to decide SHALL tell "the lookup ran and found nothing" from "the lookup could not run". The first is an answer and the rule decides on it. The second is not, and the rule SHALL refuse the request. + +The refusal SHALL be HTTP 503 with a `detail` that names the rule and says the check could not be completed, and the `code` `check-not-completed`. It SHALL carry no `invalidParams`: nothing in the request is wrong. Nothing SHALL be stored or deleted. + +Every such refusal SHALL be logged at error with the rule and the object it was checking. + +This holds for drc-003 (a relation is new), drc-004 (the ZaakInformatieObject or BesluitInformatieObject exists), drc-007 (no ObjectInformatieObject points at the document) and the relation check before a document delete (drc-008a). + +A rule SHALL NOT answer a finding it did not make. A duplicate search that failed is not a duplicate. + +#### Scenario: The counterpart lookup of drc-004 fails + +- **GIVEN** an objectinformatieobject for a case and a document +- **WHEN** the search for the ZaakInformatieObject throws +- **THEN** the create is refused with 503 and code `check-not-completed` +- **AND** the detail names drc-004 +- **AND** the log holds the rule, the case and the document + +#### Scenario: The counterpart lookup of drc-004 finds nothing + +- **GIVEN** the same objectinformatieobject +- **WHEN** the search runs and finds no ZaakInformatieObject +- **THEN** the create is refused with 400 and code `inconsistent-relation` + +#### Scenario: The relation search of a document delete fails + +- **GIVEN** a document +- **WHEN** a search for the relations that point at it throws +- **THEN** the delete is refused with 503 and code `check-not-completed` +- **AND** the document, its gebruiksrechten and its files stay + +#### Scenario: The duplicate search of drc-003 fails + +- **GIVEN** an objectinformatieobject nobody stored before +- **WHEN** the duplicate search throws +- **THEN** the create is refused with 503 and code `check-not-completed` +- **AND** the answer does not say the combination exists + +### Requirement: A case type is not deleted when its cases cannot be read (REQ-ZBR-021) + +The delete check of a case type SHALL block the delete when it cannot read the cases that use the type, or the final statuses that say which of them are closed. The ZGW delete SHALL answer 503 with the code `destroy_check_not_completed` and a detail that says the check could not be completed. A confirmation flag SHALL NOT override it. + +A check that ran and found no cases SHALL let the delete through, as before. Active cases stay a 409 `destroy_blocked_active_cases`, and closed cases stay a 409 `destroy_requires_confirmation` until confirmed. + +The refusal SHALL be logged at error with the check and the case type. + +#### Scenario: The cases cannot be read + +- **GIVEN** a case type +- **WHEN** the search for its cases throws +- **THEN** the delete is refused with 503 and code `destroy_check_not_completed` +- **AND** the case type stays + +#### Scenario: Nobody uses the case type + +- **GIVEN** a case type +- **WHEN** the search runs and finds no cases +- **THEN** the delete goes through + +### Requirement: A reference or uniqueness read that fails refuses the write (REQ-ZBR-022) + +The store reads every ZGW rule shares (`Zgw/ZgwRuleLookups`) SHALL tell "the read ran and found nothing" from "the read could not run". A read that could not run SHALL throw, and the entry point of the rules SHALL refuse the request with the answer of REQ-ZBR-020: HTTP 503, code `check-not-completed`, no `invalidParams`, logged at error with the rule and what it was reading. + +This holds for the identification uniqueness check (a duplicate nobody could search for is not a free identification) and for reference resolution (a zaaktype, informatieobjecttype or besluittype that could not be read is not a type that does not exist). + +An object the store does not hold, or does not show the caller, SHALL still read as none, as before. A lookup with nothing to read in, because the request has no store or the setting that names its schema is empty, SHALL still be skipped: an instance without a decision register cannot hold a decision either. + +#### Scenario: The uniqueness search fails + +- **GIVEN** a zaak with an identificatie and a bronorganisatie +- **WHEN** the search for another zaak with that identificatie throws +- **THEN** the create is refused with 503 and code `check-not-completed` +- **AND** the answer does not say the identificatie is taken + +#### Scenario: A referenced type cannot be read + +- **GIVEN** a write that refers to a zaaktype +- **WHEN** reading that zaaktype throws for another reason than "not found" or "not allowed" +- **THEN** the write is refused with 503 and code `check-not-completed` +- **AND** the log names the rule and the zaaktype + +#### Scenario: The referenced type does not exist + +- **GIVEN** a write that refers to a zaaktype the store does not hold +- **WHEN** the read answers "not found" +- **THEN** the rule decides as before, on a type that is not there + +### Requirement: A ZGW read, update or delete says why it failed (REQ-ZBR-023) + +A ZGW show, update or delete of a stored object SHALL sort a failure of the store into three answers, in one place for every resource: + +- **not found** (404, `detail` "Not found"): the store says the object is not there, by a DoesNotExistException or by answering no object; +- **not allowed** (403, code `permission_denied`): the store refuses the caller, by OpenRegister's NotAuthorizedException or any failure carrying code 403; +- **not completed** (503, code `store-not-reached`): any other failure. Nothing SHALL be assumed about the object, and the failure SHALL be logged at error with the operation and the object. + +A refused permission SHALL NOT answer 503. A store that was down SHALL NOT answer 404. + +A delete that happened SHALL answer 204 even when its notification fails afterwards; that failure SHALL be logged at error. + +The writes the Besluiten API makes in another register (the zaakbesluit of brc-006, the objectinformatieobject of brc-005a, brc-005b and brc-009) stay best effort, but a failure SHALL be logged at error with the rule, the resource and the objects involved. + +#### Scenario: The store is down when a besluit is deleted + +- **GIVEN** a stored besluit +- **WHEN** deleting it fails because the store cannot be reached +- **THEN** the delete answers 503 with code `store-not-reached` +- **AND** no notification goes out + +#### Scenario: The caller may not delete the object + +- **GIVEN** a stored object the caller may not delete +- **WHEN** OpenRegister refuses the delete +- **THEN** the delete answers 403 with code `permission_denied` + +#### Scenario: The object is not there + +- **GIVEN** an id the store does not hold +- **WHEN** it is deleted +- **THEN** the delete answers 404 "Not found" diff --git a/openspec/changes/guards-refuse-when-unsure/tasks.md b/openspec/changes/guards-refuse-when-unsure/tasks.md new file mode 100644 index 0000000000..84d8945da9 --- /dev/null +++ b/openspec/changes/guards-refuse-when-unsure/tasks.md @@ -0,0 +1,46 @@ +# Tasks: guards-refuse-when-unsure + +Decision 224. One PR, `--base development`, stacked on the method-decomposition slices 8e to 8g. + +## 1. One refusal for a check that could not run + +- [x] 1.1 (`lib/Service/Zgw/ZgwRuleResults.php`) `notVerified()` answers 503 `check-not-completed` with the rule in the detail and logs the rule and the object at error (REQ-ZBR-020). + - `tests/Unit/Service/Zgw/ZgwRuleResultsTest.php`: "a check that could not run refuses and is logged". Control: an ordinary refusal logs nothing. +- [x] 1.2 (`lib/Service/Zgw/ZgwRuleLookups.php`) `idsByField()` throws when the search cannot run. `findAllObjectsByField()` keeps its empty answer for the reference rules that are not part of this change. + +## 2. The document rules + +- [x] 2.1 (`lib/Service/Zgw/DrcObjectLinkRules.php`) drc-004 refuses when the counterpart lookup fails (REQ-ZBR-020). +- [x] 2.2 (`lib/Service/Zgw/DrcDocumentRelations.php`, `lib/Service/ZgwDrcRulesService.php`) drc-007 refuses when the relation search fails. +- [x] 2.3 (`lib/Service/Zgw/DrcDocumentRelations.php`) drc-003 refuses with the same answer and no longer claims a duplicate. +- [x] 2.4 (`lib/Controller/DrcController.php`) the relation check before a document delete (drc-008a) refuses when one of its three searches fails. + - Each with a control in the same test: the same search answering lets the request through, or answers the rule's own refusal. + +## 3. The case type delete check + +- [x] 3.1 (`lib/Service/Zgw/ZtcPublishAndDeleteChecks.php`, `lib/Service/ZgwBusinessRulesService.php`) cases or final statuses that cannot be read block the delete as unverified; the ZGW delete answers 503 `destroy_check_not_completed` (REQ-ZBR-021). + - `tests/Unit/Service/ZgwBusinessRulesServiceDestroyGuardTest.php` asserts it from the caller. + +## 4. The debt list + +- [x] 4.1 (`tests/Unit/Architecture/catch-return-null.allowlist.json`) two sites left, ceiling 256 to 254 (after train 5). + +## 5. Reference and uniqueness reads (Q-dossiq-L11-3, answered 12 Oct) + +- [x] 5.1 (`lib/Service/Zgw/LookupNotCompleted.php`, `lib/Service/Zgw/ZgwRuleLookups.php`) `findObjectByField()`, `findAllObjectsByField()`, `findBySchemaKey()` and `fieldPairExists()` throw `LookupNotCompleted` when their read fails. Not found and a refused read (OpenRegister's `NotAuthorizedException`) stay "none"; no store or an empty schema setting stays a skipped lookup (REQ-ZBR-022). + - `tests/Unit/Service/Zgw/ZgwRuleLookupsTest.php`, `tests/Unit/Service/ZgwRulesBaseUniquenessTest.php`. +- [x] 5.2 (`lib/Service/ZgwBusinessRulesService.php`) `validate()` catches it once and answers `ZgwRuleResults::notVerified()`, for the entry point's own guards and for every per-register rule. + - `tests/Unit/Service/ZgwBusinessRulesServiceLookupFailureTest.php`, with a control that rules which could read decide as before. +- [x] 5.3 (`tests/Unit/Architecture/catch-return-null.allowlist.json`) three sites left, ceiling 254 to 251. + +## 6. Store failures of a ZGW read, update and delete (Q-dossiq-L11-2, answered 12 Oct) + +- [x] 6.1 (`lib/Service/Zgw/StoreFailureAnswer.php`, `lib/Service/ZgwService.php`) one place sorts a store failure into 404, 403 and 503; `handleShow()`, `handleUpdate()` and `handleDestroy()` answer through it, and a delete that happened answers 204 when only its notification fails (REQ-ZBR-023). + - `tests/Unit/Service/ZgwServiceStoreFailureTest.php`, one test per signal: DoesNotExistException, a null answer, NotAuthorizedException, a failure with code 403, any other failure. +- [x] 6.2 (`lib/Controller/BrcController.php`, `lib/Service/Zgw/BrcDocumentLinkHandler.php`) the besluit delete and the besluitinformatieobject delete answer through the same place. + - `tests/Unit/Controller/BrcControllerTest.php`, `tests/Unit/Service/Zgw/BrcDocumentLinkHandlerTest.php`. +- [x] 6.3 (`lib/Service/Zgw/BrcCrossRegisterSync.php`) a failed cross-register write is logged at error with the rule, the resource and the objects; it stays best effort. + +## 7. Live + +- [ ] 7.1 (live pass, decision 139) With the store reachable: an objectinformatieobject create, a document delete and a case type delete answer as before. diff --git a/openspec/changes/method-decomposition/design.md b/openspec/changes/method-decomposition/design.md index 8a4af093a5..9cd862149d 100644 --- a/openspec/changes/method-decomposition/design.md +++ b/openspec/changes/method-decomposition/design.md @@ -68,6 +68,96 @@ Reduce constructor parameters by grouping related dependencies into composite se - **`lib/Service/MappingTransformService.php`** — Mapping data transformation (extracted from ZgwMappingService) - **`lib/Settings/zgw_mappings/*.json`** — Mapping configuration files (zaak.json, documents.json, besluit.json, catalogi.json) +## StatusTransitionService: two facades (slice 8h) + +Measured on the stack head (12 Oct): `StatusTransitionService` couples to 18 classes against a limit of 13 and takes 14 constructor arguments against 10. It carries two suppressions for it, `CouplingBetweenObjects` and `ExcessiveParameterList`. Its complexity is not the problem: the earlier split into `Transitions/*` already took the work out. What is left is an engine that holds every one of those classes itself. + +A move is not a design here. Folding two collaborators into a third to lower the count would put a concern back where it was split from, which is what the suppression says. The engine asks two kinds of question, so it gets two collaborators. + +### What the engine asks + +| The engine asks | Today it holds | After | +|---|---|---| +| What does this case declare about its status and its moves? | `StatusDeclarations`, `TransitionDeclarations`, `OfferedTransitions`, `StatusChecklist`, and the status row read of `CaseStatusStore` | `Transitions/CaseDeclarations` | +| What does a move write, and what did earlier moves write? | `CaseStatusStore`, `CaseResultWriter`, `SideEffectDispatcher` | `Transitions/CaseMoveLedger` | +| May this person take this move? | `TransitionAuthorizer`, `GuardRegistry`, `TransitionSpecReader`, `ProcessOwnedStatusRule` | unchanged, the engine keeps them | +| Which workflow does the case run on, and who is asking? | `WorkflowTemplateLoader`, `IUserSession`, `LoggerInterface` | unchanged | + +The engine keeps the decision: which transition, in which order the checks run, what is refused and what the caller is told. That order is the specification (REQ-STE-5-002, REQ-STE-12) and stays readable in one method. + +### `Transitions/CaseDeclarations` (the read side) + +One class over the four that read what an administrator declared, plus the store for one read: the row of a status. No write to the store goes through it. + +| Method | Takes over | Answers | +|---|---|---| +| `panel(array $case): array` | `StatusDeclarations::panelFor` | `waitingOn`, `dwell`, `derivation` of the status the case is in | +| `statusFacts(string $statusTypeId): array` | `CaseStatusStore::lookupStatusName`, `lookupStatusColour`, `lookupStatusDescription` | one call, keys `statusName`, `statusColour`, `statusDescription` | +| `offered(array $transitions, array $case, string $currentId, string $userId): array` | `OfferedTransitions::from` | `transitions` and `withheld` | +| `assertOpen(array $transition, array $case, string $caseId, string $userId): void` | `TransitionDeclarations::withheldReasons`, `fourEyesRefusal`, `refusalSentence` and the two refusals the engine throws on them | throws `RefusedException` `transition-dependency-open` or `transition-four-eyes`, in that order | +| `stamp(array $case, string $toStatus): array` | `StatusDeclarations::applyStatusChange` | the case payload with the dwell bookkeeping of the status it enters | +| `retime(string $caseId, string $toStatus): void` | `StatusDeclarations::retime` | nothing; stops and starts the status timers | +| `checklistActions(string $statusTypeId, array $case, string $actor): array` | `StatusChecklist::actionsFor` | the actions the entered status brings | + +`stamp()` and `retime()` are on the read side on purpose. They change nothing in the store themselves: `stamp()` returns a payload the ledger saves, and `retime()` talks to the term engine. They belong with the class that knows what a status declares. + +### `Transitions/CaseMoveLedger` (the write side) + +One class over the three that touch the store or the outside world. It takes whole steps from the engine, not single calls, so it is not a pass-through. + +| Method | Takes over | Notes | +|---|---|---| +| `load(string $caseId): ?array` | `CaseStatusStore::loadCase` | | +| `reloadUnchanged(string $caseId, int $readVersion, string $currentId): array` | the engine's `assertNoConcurrentChange()` | throws `case_not_found` or `RefusedException` `transition-conflict` | +| `close(array $case, string $caseId, string $toStatus, ?string $resultTypeId): array` | the engine's `applyClosingResult()` | end date, closing result and archival future on the PAYLOAD; saves nothing | +| `save(array $case, string $toStatus, ?int $readVersion = null): array` | the save of `writeMove()` and of `executeFreeForm()` | the status and, with a read version, `@self.version`, in ONE save; returns the saved case and its version | +| `assertStatusOfCaseType(string $caseTypeId, string $statusTypeId): void` | `CaseStatusStore::assertStatusBelongsToCaseType` | the free-form check | +| `record(string $caseId, array $move): array` | `CaseStatusStore::writeStatusRecord` | `$move` is `fromStatus`, `toStatus`, `label`, `comment`, `actor`, `evaluatedGuards`, `noWorkflowTemplate` | +| `dispatch(array $actions, array $case, array $move, array $record): array` | `SideEffectDispatcher::dispatch` | builds the transition context from the move and the record | +| `writeBack(array $record, array $dispatched): array` | the engine's `persistDispatchedActions()` | a failed write is logged at error, the record answered as it stood | +| `history(string $caseId): ?array` | `findStatusRecords` | `replay()` keeps the sort | + +The engine calls `close()`, then `CaseDeclarations::stamp()`, then `save()`, then `retime()`: the same order as before the split, and the only place the two facades meet. `close()` and `save()` are two methods because the stamp sits between them. A free-form move calls `record()` and `dispatch()` and never `writeBack()`, as before. + +As built, two things differ from the first draft of this design. The status row read (`statusFacts`) sits on the read side: it is a declaration, and with it the ledger stays at ten public methods, the PHPMD limit. And the ledger has no `saveMove()` or `recordAndDispatch()`: a single method each would have had to change the order of the stamp, or take a flag for the write-back. + +### The engine after the split + +Constructor, nine arguments: `WorkflowTemplateLoader`, `GuardRegistry`, `TransitionAuthorizer`, `TransitionSpecReader`, `IUserSession`, `LoggerInterface`, `ProcessOwnedStatusRule`, `CaseDeclarations`, `CaseMoveLedger`. With `RefusedException`, `GuardFailedException` and `RuntimeException` it couples to 12 classes (measured: whole-tree PHPMD is clean without the suppressions). Both suppressions go and `phpmd.suppressWarnings` in `.debt-baseline.json` goes down by 2. + +The public methods and what they return do not change: `getAvailableTransitions()`, `execute()`, `executeFreeForm()`, `replay()`, `isAdmin()`, `ADMIN_GROUP_ID`. The container builds both facades by type, so no registration changes. Every class that injects the engine (32 files under `lib/`) is untouched. + +The seven classes behind the facades keep their own constructors and their own tests. Other callers of them, such as `Lifecycle/CaseActionProvider` and `CaseDocumentGenerationService` on `CaseStatusStore`, keep calling them directly. + +### What can go wrong + +- **The order of the refusals.** `assertTransitionAllowed()` refuses in a fixed order: from-status, open dependency, four eyes, group, guards. A caller sees the first. `assertOpen()` holds the second and third and is called between the first and the fourth. A characterisation test with a transition that fails all five pins the order before the split. +- **One save, not two.** The closing result, the dwell bookkeeping and the status land in one save. `saveMove()` must not save before the stamp is on the payload. Pinned by asserting the single `saveCase` payload. +- **The timer after the save.** `retime()` runs after `saveMove()` returns and not when it throws. +- **Coverage metadata.** Every test that reaches the engine executes the two new classes. Each needs both in `@uses`, or CI marks it risky. No coverage driver runs locally by default: use the pcov image (`d179-logs/pcov.sh`) on `tests/Unit` before the push. +- **Whole-tree PHPMD only.** Coupling does not show in a per-file run. Measure with a fresh cache (`HOME` and `TMPDIR` on empty directories). + +### Characterisation first + +Five test classes already cover the engine through the real collaborators: the group gate, the closing result, failed actions, replay, and the route a case runs on. Before the split, in its own commit and green on the single class, one more (`StatusTransitionServiceOrderTest`) pins what none of them does: + +1. the order of the five refusals, and that nothing is saved on any of them; +2. `transition-conflict` when the version or the status changed between read and save; +3. `case_not_found`, `transition_not_found` and `transition_missing_to_status`; +4. the payload of the one save of a closing move, with the stamp and the version on it; +5. `retime()` after the save and not after a refused save; +6. a status record whose dispatched actions cannot be written back: logged at error, the move still answers; +7. the free-form refusals (not an administrator, a status the process owns, a status of another case type); +8. the shape of `getAvailableTransitions()` without a case, without a workflow, and with one: `withheld` always present. + +The test builds the engine through one helper, so the constructor change in the next commit touches that helper and the six existing constructions only. + +### Steps + +1. This design. +2. Characterisation test, green on the single class (own commit). +3. `CaseDeclarations` and `CaseMoveLedger` with their own unit tests; the engine delegates; both suppressions removed; baseline lowered by 2. The characterisation test and the five existing ones stay green without an edit beyond the constructor helper. + ## Implementation Order 1. **Phase 1 (Priority 1 controllers):** ZrcController, ZtcController, BrcController, DrcController diff --git a/openspec/changes/method-decomposition/tasks.md b/openspec/changes/method-decomposition/tasks.md index cfb1c1f902..0c266d83cd 100644 --- a/openspec/changes/method-decomposition/tasks.md +++ b/openspec/changes/method-decomposition/tasks.md @@ -493,3 +493,21 @@ characterisation test with a mapped ObjectService is a sufficient safety net; th active. Behaviour kept, not changed here. Own tests: `tests/Unit/Service/Zgw/ZtcConceptRulesTest` (6), `ZtcTypeReferencesTest` (4, every search asserted with its schema) and `ZtcPublishAndDeleteChecksTest` (6). + - [x] 8h `StatusTransitionService` (2: CouplingBetweenObjects 18 against 13, and + ExcessiveParameterList, 14 constructor arguments against 10). Not a move: the design is in + design.md, "StatusTransitionService: two facades". `Transitions/CaseDeclarations` reads + what a case declares about its status and its moves (over `StatusDeclarations`, + `TransitionDeclarations`, `OfferedTransitions`, `StatusChecklist` and the status row). + `Transitions/CaseMoveLedger` holds what a move writes (over `CaseStatusStore`, + `CaseResultWriter`, `SideEffectDispatcher`). The engine keeps the decision and the order + of the checks, takes nine arguments and couples to 12 classes. Both suppressions are gone + (183 to 181). The public methods and their answers are unchanged; the constructor changed. + Characterisation, green on the single class first (own commit): + `tests/Unit/Service/StatusTransitionServiceOrderTest.php`, 10 tests over the order of the + five refusals, the conflict check, the one save of a closing move with the version read, + the timer after the save, the record that cannot be written back, the free-form refusals + and the shape of the available transitions. Seven mutations of the engine turn it red. + Unchanged and green after the split apart from its one constructor helper, as are the + five existing engine tests. + Own tests: `tests/Unit/Service/Transitions/CaseDeclarationsTest` (4) and + `CaseMoveLedgerTest` (6). diff --git a/src/components/case/CaseStatusDeclarationPanel.vue b/src/components/case/CaseStatusDeclarationPanel.vue index 136d7515bd..67014b55fc 100644 --- a/src/components/case/CaseStatusDeclarationPanel.vue +++ b/src/components/case/CaseStatusDeclarationPanel.vue @@ -25,7 +25,7 @@ its status maximum, with no derivation pending, renders nothing at all rather than three empty lines. - @spec openspec/changes/what-a-status-declares/specs/status-transition-engine/spec.md + @spec openspec/specs/status-transition-engine/spec.md -->