From b995ed3e01b81d86bc2982efaaa76a8aa5ba09ea Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sat, 8 Aug 2026 12:39:26 +0200 Subject: [PATCH] fix: restore the 16 branch bodies commit 651a055f deleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 651a055f ("refactor: Replace else clauses with early returns — remove ElseExpression suppressions", 2026-03-19) rewrote ~206 else expressions mechanically. On the default-then-override shape it applied: if (C) { $x = A; } else { $x = B; } -> $x = B; if (C) { } It hoisted the else-body out and deleted BOTH the `else` keyword AND the if-body. The condition survived; the only statement it guarded did not. PHP parses it, PHPCS and PHPMD are satisfied (no `else` is left to object to), and the call site silently collapses to its default branch. Sixteen of these survived to `development`. What each one cost: AangebodenGebruikController 145, 235 — `$statusCode` never became 500, so a service-level failure was returned as HTTP 200 with an `error` key in the body. Every client that branches on `response.ok` — which is what this app's own Pinia stores do — read a failed request as a successful one with zero results. SettingsService 4808, 4812 — the ArchiMate import/export status the admin panel renders was pinned to []. A finished import and an import that never ran looked identical. AangebodenGebruikService 406 — the ambtenaar `?organisation=` filter was dropped on the floor. $organisationFilter is read 3 lines later and assigned into $searchQuery['@self']['organisation'], so the filter silently did nothing. SymfonyEmailService 691, 787 — the recipient's name in two outbound e-mails was pinned to the literal "Gebruiker". SettingsService 503 — a string value was cast with (string) rather than passed through. SettingsService 4182, 4186 — the schema-slug mapping diagnostic always logged 'NO', which is the log you read when the mapping is broken. ModuleComplianceService 409 — an array standaardversie was rendered with (string), i.e. "Array" plus a PHP warning, instead of json_encode(). AanbodService 194 and AangebodenGebruikService 203, 825, 1351, 1381 — five `is_array($x)` guards became unconditional method calls on a value that may be an array: a latent "Call to a member function on array" fatal. The repairs use default-then-override, never `else` and never an inline ternary, so PHPCS (Inline IF statements are not allowed) and PHPMD (ElseExpression) both stay green: 0 errors / 9 warnings on these six files before and after, identical. Adds three tests: AangebodenGebruikControllerStatusCodeTest asserts the STATUS CODE on both endpoints, with a 200-on-success arm so a hardcoded 500 cannot satisfy it. Reverting the controller turns exactly the two error arms red ("Failed asserting that 200 is identical to 500"). SettingsServiceArchiMateStatusFallbackTest asserts on the ITEM inside the envelope (import.status, import.processed) rather than on the envelope, which was always well-formed and is why the defect survived. Reverting SettingsService turns both arms red. NoEmptyControlFlowBlockTest is the detector none of the 64 Hydra gates provides: a token-stream scan of lib/ for empty if/elseif/else/for/ foreach/while/switch bodies. Empty `catch` is excluded and only that — eight deliberate swallow-and-continue catches predate the refactor. It carries its own positive controls: the scanner must fire on the mangled shape, must NOT fire on the repaired shape, and the tree scan asserts it read >50 files before treating zero findings as clean. Reverting the four service files makes it name all nine sites. Full unit suite: 490 tests green (481 before, 9 added). --- .../AangebodenGebruikController.php | 6 +- lib/Service/AanbodService.php | 3 +- lib/Service/AangebodenGebruikService.php | 15 +- lib/Service/ModuleComplianceService.php | 3 +- lib/Service/SettingsService.php | 15 +- lib/Service/SymfonyEmailService.php | 6 +- ...gebodenGebruikControllerStatusCodeTest.php | 225 ++++++++++++++ tests/Unit/NoEmptyControlFlowBlockTest.php | 281 ++++++++++++++++++ ...ingsServiceArchiMateStatusFallbackTest.php | 173 +++++++++++ 9 files changed, 711 insertions(+), 16 deletions(-) create mode 100644 tests/Unit/Controller/AangebodenGebruikControllerStatusCodeTest.php create mode 100644 tests/Unit/NoEmptyControlFlowBlockTest.php create mode 100644 tests/Unit/Service/SettingsServiceArchiMateStatusFallbackTest.php diff --git a/lib/Controller/AangebodenGebruikController.php b/lib/Controller/AangebodenGebruikController.php index 7a722bf4..4d08f2dc 100644 --- a/lib/Controller/AangebodenGebruikController.php +++ b/lib/Controller/AangebodenGebruikController.php @@ -141,8 +141,9 @@ public function getGebruiksWhereAfnemer(): JSONResponse $result = $this->gebruikSvc->getGebruiksWhereAfnemer($options); // Determine HTTP status code based on whether there's an error. - $statusCode = 200; + $statusCode = 200; if (isset($result['error']) === true) { + $statusCode = 500; } $this->logger->info( @@ -231,8 +232,9 @@ public function getKoppelingenGebruikByUuid(string $uuid): JSONResponse ); // Determine HTTP status code based on whether there's an error. - $statusCode = 200; + $statusCode = 200; if (isset($result['error']) === true) { + $statusCode = 500; } $this->logger->info( diff --git a/lib/Service/AanbodService.php b/lib/Service/AanbodService.php index 2acae7b6..8ff2fb82 100644 --- a/lib/Service/AanbodService.php +++ b/lib/Service/AanbodService.php @@ -191,8 +191,9 @@ public function getAanbod(array $options=[]): array foreach ($searchResult['results'] ?? [] as $result) { // Use jsonSerialize() instead of getObject() to include @self metadata. // GetObject() only returns raw object data without @self.organisation. + $resultData = $result; + if (is_array($result) === false) { $resultData = $result->jsonSerialize(); - if (is_array($result) === true) { } $selfOrg = $resultData['@self']['organisation'] ?? null; diff --git a/lib/Service/AangebodenGebruikService.php b/lib/Service/AangebodenGebruikService.php index 51f94868..c6edff50 100644 --- a/lib/Service/AangebodenGebruikService.php +++ b/lib/Service/AangebodenGebruikService.php @@ -201,8 +201,9 @@ public function getGebruiksWhereAfnemer(array $options=[]): array $filteredResults = []; foreach ($searchResult['results'] ?? [] as $result) { // Convert ObjectEntity to array if needed. + $resultData = $result; + if (is_array(value: $result) === false) { $resultData = $result->getObject(); - if (is_array(value: $result) === true) { } $selfOrg = $resultData['@self']['organisation'] ?? null; @@ -405,8 +406,9 @@ public function getKoppelingenGebruikByUuid(string $uuid, array $options=[], boo } // Get organization filter if provided (for ambtenaar). - $organisationFilter = null; + $organisationFilter = null; if ($isAmbtenaar === true && isset($options['organisation']) === true) { + $organisationFilter = $options['organisation']; } // Build search query using ObjectService's buildSearchQuery. @@ -823,8 +825,9 @@ public function getGebruiksWhereDeelnemers(array $options=[]): array // Process and add to results. foreach ($gebruikItems as $gebruik) { + $gebruikData = $gebruik; + if (is_array(value: $gebruik) === false) { $gebruikData = $gebruik->jsonSerialize(); - if (is_array(value: $gebruik) === true) { } $gebruikData['_filter_type'] = 'deelnemers'; @@ -1345,8 +1348,9 @@ private function getApplicationsOwnedByOrganisation( ); foreach ($suites as $suite) { + $suiteData = $suite; + if (is_array(value: $suite) === false) { $suiteData = $suite->getObject(); - if (is_array(value: $suite) === true) { } $appUuids[] = $suiteData['uuid'] ?? $suiteData['id'] ?? null; @@ -1372,8 +1376,9 @@ private function getApplicationsOwnedByOrganisation( ); foreach ($modules as $module) { + $moduleData = $module; + if (is_array(value: $module) === false) { $moduleData = $module->getObject(); - if (is_array(value: $module) === true) { } $appUuids[] = $moduleData['uuid'] ?? $moduleData['id'] ?? null; diff --git a/lib/Service/ModuleComplianceService.php b/lib/Service/ModuleComplianceService.php index 4527a68a..016dceef 100644 --- a/lib/Service/ModuleComplianceService.php +++ b/lib/Service/ModuleComplianceService.php @@ -406,8 +406,9 @@ private function extractStandaardversieUuids(array $complianceObjects): array && (is_object($standaardversie) === false || isset($standaardversie->uuid) === false) ) { $tracking['invalidType']++; - $standaardversieValue = (string) $standaardversie; + $standaardversieValue = (string) $standaardversie; if (is_array($standaardversie) === true) { + $standaardversieValue = json_encode($standaardversie); } $this->logger->warning( diff --git a/lib/Service/SettingsService.php b/lib/Service/SettingsService.php index c8e76e69..f2d5c333 100644 --- a/lib/Service/SettingsService.php +++ b/lib/Service/SettingsService.php @@ -500,8 +500,9 @@ public function updateSettings(array $data): array $stringValue = json_encode($value); } else { // Ensure value is converted to string as required by setValueString. - $stringValue = (string) $value; + $stringValue = (string) $value; if (is_string($value) === true) { + $stringValue = $value; } } @@ -4179,12 +4180,14 @@ private function configureVoorzieningen(): array $originalSlug = $schema['slug'] ?? ''; $lowercaseSlug = strtolower($originalSlug); - $hasMappingOriginalValue = 'NO'; + $hasMappingOriginalValue = 'NO'; if (isset($slugToKey[$originalSlug]) === true) { + $hasMappingOriginalValue = 'YES'; } - $hasMappingLowercaseValue = 'NO'; + $hasMappingLowercaseValue = 'NO'; if (isset($slugToKey[$lowercaseSlug]) === true) { + $hasMappingLowercaseValue = 'YES'; } $this->logger->info( @@ -4802,12 +4805,14 @@ public function getArchiMateStatus(): array // Get AMEF object counts. $amefObjectCounts = $this->getAmefObjectCounts(); - $importValue = []; + $importValue = []; if (is_array($importDecoded) === true) { + $importValue = $importDecoded; } - $exportValue = []; + $exportValue = []; if (is_array($exportDecoded) === true) { + $exportValue = $exportDecoded; } return [ diff --git a/lib/Service/SymfonyEmailService.php b/lib/Service/SymfonyEmailService.php index 913118d6..04d4b14a 100644 --- a/lib/Service/SymfonyEmailService.php +++ b/lib/Service/SymfonyEmailService.php @@ -688,8 +688,9 @@ public function sendUserCreationEmail(array $user, array $organization=[]): bool ); // Prepare template data. - $displayName = 'Gebruiker'; + $displayName = 'Gebruiker'; if (empty($userName) === false) { + $displayName = $userName; } $templateData = [ @@ -784,8 +785,9 @@ public function sendUserUpdateEmail(array $user, array $organization=[]): bool ); // Prepare template data. - $displayName = 'Gebruiker'; + $displayName = 'Gebruiker'; if (empty($userName) === false) { + $displayName = $userName; } $templateData = [ diff --git a/tests/Unit/Controller/AangebodenGebruikControllerStatusCodeTest.php b/tests/Unit/Controller/AangebodenGebruikControllerStatusCodeTest.php new file mode 100644 index 00000000..88828286 --- /dev/null +++ b/tests/Unit/Controller/AangebodenGebruikControllerStatusCodeTest.php @@ -0,0 +1,225 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://codeberg.org/Conduction/SoftwareCatalog + * + * @spec openspec/specs/vendor-visibility-rbac/spec.md#requirement-the-offered-usage-afnemer-endpoint-must-require-authentication-explicitly-not-implicitly-req-004 + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Controller; + +use OCA\SoftwareCatalog\Controller\AangebodenGebruikController; +use OCA\SoftwareCatalog\Service\AangebodenGebruikService; +use OCP\IGroupManager; +use OCP\IRequest; +use OCP\IUser; +use OCP\IUserSession; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; + +/** + * THE DEFECT UNDER TEST. + * + * Both endpoints below documented their intent in a comment — "Determine + * HTTP status code based on whether there's an error" — and then shipped + * + * $statusCode = 200; + * if (isset($result['error']) === true) { + * } + * + * an EMPTY if body. Commit 651a055f ("refactor: Replace else clauses with + * early returns") rewrote `if (err) { $s = 500; } else { $s = 200; }` by + * hoisting the else-body out and deleting the if-body along with the + * `else` keyword. The condition survived; the only statement it guarded + * did not. + * + * The consequence is not cosmetic: a service-level failure was returned to + * the caller as **HTTP 200** with an `error` key in the body. Every client + * that branches on `response.ok` — which is what this app's own Pinia + * stores do — read a failed request as a successful one with zero results. + * A "no results" screen and a "the backend blew up" screen became + * indistinguishable over the wire. + * + * These tests assert the STATUS CODE, not the envelope, because the + * envelope was always right and is exactly what made the defect invisible. + */ +final class AangebodenGebruikControllerStatusCodeTest extends TestCase +{ + + /** + * The service double the controller under test delegates to. + * + * @var AangebodenGebruikService|MockObject + */ + private AangebodenGebruikService|MockObject $gebruikSvc; + + /** + * The session double, always populated with an authenticated user so + * the controller's own auth guard is not what these tests measure. + * + * @var IUserSession|MockObject + */ + private IUserSession|MockObject $userSession; + + + /** + * Build the controller with an authenticated caller in session. + * + * @return AangebodenGebruikController The controller under test. + */ + private function makeController(): AangebodenGebruikController + { + $request = $this->createMock(IRequest::class); + $request->method('getParams')->willReturn([]); + $request->method('getParam')->willReturn(null); + + $this->gebruikSvc = $this->createMock(AangebodenGebruikService::class); + $this->userSession = $this->createMock(IUserSession::class); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('caller-uid'); + $this->userSession->method('getUser')->willReturn($user); + + return new AangebodenGebruikController( + 'softwarecatalog', + $request, + $this->userSession, + $this->gebruikSvc, + $this->createMock(LoggerInterface::class), + $this->createMock(IGroupManager::class) + ); + + }//end makeController() + + + /** + * The error envelope produced by the service layer on failure. + * + * @param string|null $error The error message, or null for the success shape. + * + * @return array The service return value. + */ + private function envelope(?string $error): array + { + $envelope = [ + 'results' => [], + 'total' => 0, + 'page' => 1, + 'pages' => 0, + 'limit' => 20, + 'offset' => 0, + ]; + + if ($error !== null) { + $envelope['error'] = $error; + } + + return $envelope; + + }//end envelope() + + + /** + * A service-reported error on the afnemer listing MUST surface as 500, + * not as a 200 carrying an `error` key. + * + * @return void + */ + public function testAfnemerListingReturns500WhenTheServiceReportsAnError(): void + { + $controller = $this->makeController(); + + $this->gebruikSvc->method('getGebruiksWhereAfnemer') + ->willReturn($this->envelope('Voorzieningen configuration not found')); + + $response = $controller->getGebruiksWhereAfnemer(); + + $this->assertSame( + 500, + $response->getStatus(), + 'A service error must be reported as HTTP 500. Returning 200 makes a ' + .'backend failure indistinguishable from an empty result set for every ' + .'client that branches on response.ok.' + ); + + }//end testAfnemerListingReturns500WhenTheServiceReportsAnError() + + + /** + * The success path must stay 200 — the fix must not turn every + * response into a 500. Without this arm the test above would also pass + * against a hardcoded `$statusCode = 500`. + * + * @return void + */ + public function testAfnemerListingReturns200OnSuccess(): void + { + $controller = $this->makeController(); + + $this->gebruikSvc->method('getGebruiksWhereAfnemer') + ->willReturn($this->envelope(null)); + + $response = $controller->getGebruiksWhereAfnemer(); + + $this->assertSame(200, $response->getStatus()); + + }//end testAfnemerListingReturns200OnSuccess() + + + /** + * Same defect, second site: the koppelingen-by-UUID endpoint. + * + * @return void + */ + public function testKoppelingenByUuidReturns500WhenTheServiceReportsAnError(): void + { + $controller = $this->makeController(); + + $this->gebruikSvc->method('getKoppelingenGebruikByUuid') + ->willReturn($this->envelope('Voorzieningen configuration not found')); + + $response = $controller->getKoppelingenGebruikByUuid('some-uuid'); + + $this->assertSame( + 500, + $response->getStatus(), + 'A service error must be reported as HTTP 500 on the koppelingen-by-UUID endpoint too.' + ); + + }//end testKoppelingenByUuidReturns500WhenTheServiceReportsAnError() + + + /** + * And its success arm. + * + * @return void + */ + public function testKoppelingenByUuidReturns200OnSuccess(): void + { + $controller = $this->makeController(); + + $this->gebruikSvc->method('getKoppelingenGebruikByUuid') + ->willReturn($this->envelope(null)); + + $response = $controller->getKoppelingenGebruikByUuid('some-uuid'); + + $this->assertSame(200, $response->getStatus()); + + }//end testKoppelingenByUuidReturns200OnSuccess() + + +}//end class diff --git a/tests/Unit/NoEmptyControlFlowBlockTest.php b/tests/Unit/NoEmptyControlFlowBlockTest.php new file mode 100644 index 00000000..7fac1751 --- /dev/null +++ b/tests/Unit/NoEmptyControlFlowBlockTest.php @@ -0,0 +1,281 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://codeberg.org/Conduction/SoftwareCatalog + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit; + +use PHPUnit\Framework\TestCase; +use RecursiveDirectoryIterator; +use RecursiveIteratorIterator; + +/** + * WHY THIS TEST EXISTS. + * + * Commit 651a055f ("refactor: Replace else clauses with early returns — + * remove ElseExpression suppressions") rewrote ~206 else-expressions + * mechanically. The rewrite it applied to the default-then-override shape + * was: + * + * if (C) { $x = A; } else { $x = B; } -> $x = B; + * if (C) { } + * + * It hoisted the else-body out and deleted BOTH the `else` keyword AND the + * if-body. The condition survived; the statement it guarded did not. PHP + * parses the result without complaint, PHPCS and PHPMD are satisfied + * (there is no `else` left to object to), and every affected call site + * silently collapsed to its default branch. + * + * Sixteen of these survived to `development`, including: + * + * - two controller endpoints that returned HTTP 200 on a service error; + * - the ArchiMate import/export status the admin panel renders, pinned + * to an empty array; + * - the ambtenaar `?organisation=` filter, silently dropped; + * - the recipient's name in three outbound e-mails, pinned to + * "Gebruiker"; + * - five `is_array($x) ? $x : $x->getObject()` guards, each of which now + * calls a method unconditionally on a value that may be an array — a + * latent fatal. + * + * None of the 64 Hydra gates detects an empty block, so this test is the + * detector. It is deliberately structural rather than behavioural: the + * defect class is "a branch body went missing", which is visible in the + * token stream and cheap to assert over the whole tree, whereas + * behavioural coverage of all sixteen sites would need live OpenRegister + * and mail transports. + * + * EMPTY `catch` BLOCKS ARE EXCLUDED, and only those. `catch (\Throwable) {}` + * is a deliberate, readable "swallow and continue" idiom and eight of them + * predate the refactor. Every other construct — if / elseif / else / for / + * foreach / while / switch — is in scope. + */ +final class NoEmptyControlFlowBlockTest extends TestCase +{ + + + /** + * Constructs whose body must never be empty, keyed by token id. + * + * @return array Token id to human-readable keyword. + */ + private function guardedConstructs(): array + { + return [ + T_IF => 'if', + T_ELSEIF => 'elseif', + T_ELSE => 'else', + T_FOR => 'for', + T_FOREACH => 'foreach', + T_WHILE => 'while', + T_SWITCH => 'switch', + ]; + + }//end guardedConstructs() + + + /** + * Find every empty guarded block in a single PHP source string. + * + * Works on the token stream rather than on text so that multi-line + * conditions, comments between the condition and the brace, and + * arbitrary indentation cannot hide a finding — the original + * hand-rolled grep for this defect missed sites for exactly those + * reasons. + * + * @param string $source The PHP source to scan. + * + * @return array The findings. + */ + private function findEmptyBlocks(string $source): array + { + $significant = []; + foreach (token_get_all($source) as $token) { + if (is_array($token) === true) { + if (in_array($token[0], [T_WHITESPACE, T_COMMENT, T_DOC_COMMENT], true) === true) { + continue; + } + + $significant[] = [ + 'id' => $token[0], + 'text' => $token[1], + 'line' => $token[2], + ]; + continue; + } + + $significant[] = [ + 'id' => null, + 'text' => $token, + 'line' => 0, + ]; + } + + $constructs = $this->guardedConstructs(); + $findings = []; + $count = count($significant); + + for ($i = 0; $i < ($count - 1); $i++) { + if ($significant[$i]['text'] !== '{' || $significant[$i + 1]['text'] !== '}') { + continue; + } + + // Walk back over the balanced condition parentheses, if any. + $j = ($i - 1); + if ($j >= 0 && $significant[$j]['text'] === ')') { + $depth = 0; + while ($j >= 0) { + if ($significant[$j]['text'] === ')') { + $depth++; + } + + if ($significant[$j]['text'] === '(') { + $depth--; + if ($depth === 0) { + $j--; + break; + } + } + + $j--; + } + } + + if ($j < 0 || isset($constructs[$significant[$j]['id']]) === false) { + continue; + } + + $line = 0; + for ($k = $j; $k <= $i; $k++) { + if ($significant[$k]['line'] > 0) { + $line = $significant[$k]['line']; + break; + } + } + + $findings[] = [ + 'line' => $line, + 'keyword' => $constructs[$significant[$j]['id']], + ]; + }//end for + + return $findings; + + }//end findEmptyBlocks() + + + /** + * The scanner must be able to report a finding. Without this arm a + * broken scanner and a clean tree produce byte-identical output, and + * the assertion below would be permanently, silently green. + * + * @return void + */ + public function testTheScannerDetectsTheExactShapeTheRefactorLeftBehind(): void + { + $mangled = <<<'PHP' +findEmptyBlocks($mangled); + + $this->assertCount(1, $findings, 'The scanner must find the empty if body.'); + $this->assertSame('if', $findings[0]['keyword']); + + // And it must NOT fire on the repaired shape, or it would report + // every fixed site as still broken. + $repaired = <<<'PHP' +assertSame([], $this->findEmptyBlocks($repaired)); + + // An empty catch is explicitly out of scope. + $emptyCatch = <<<'PHP' +assertSame([], $this->findEmptyBlocks($emptyCatch)); + + }//end testTheScannerDetectsTheExactShapeTheRefactorLeftBehind() + + + /** + * No shipped file under lib/ may contain an empty guarded block. + * + * @return void + */ + public function testNoShippedFileContainsAnEmptyGuardedBlock(): void + { + $libDir = dirname(__DIR__, 2).'/lib'; + $this->assertDirectoryExists($libDir, 'lib/ must exist for this scan to mean anything.'); + + $iterator = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($libDir)); + $scanned = 0; + $findings = []; + + foreach ($iterator as $file) { + if ($file->isDir() === true || $file->getExtension() !== 'php') { + continue; + } + + $scanned++; + $relative = substr($file->getPathname(), (strlen($libDir) - 3)); + + foreach ($this->findEmptyBlocks(file_get_contents($file->getPathname())) as $finding) { + $findings[] = $relative.':'.$finding['line'].' — empty '.$finding['keyword'].' body'; + } + } + + // Positive control on the INPUT: a zero finding count is only + // meaningful if the scan actually read files. + $this->assertGreaterThan( + 50, + $scanned, + 'Fewer than 50 PHP files were scanned under lib/ — the scan did not run over the real tree, ' + .'so a clean result says nothing.' + ); + + $this->assertSame( + [], + $findings, + "Empty control-flow bodies found in lib/. Each one is a branch whose only statement was " + ."deleted, so the code always takes its default path:\n ".implode("\n ", $findings) + ); + + }//end testNoShippedFileContainsAnEmptyGuardedBlock() + + +}//end class diff --git a/tests/Unit/Service/SettingsServiceArchiMateStatusFallbackTest.php b/tests/Unit/Service/SettingsServiceArchiMateStatusFallbackTest.php new file mode 100644 index 00000000..455b5da3 --- /dev/null +++ b/tests/Unit/Service/SettingsServiceArchiMateStatusFallbackTest.php @@ -0,0 +1,173 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://codeberg.org/Conduction/SoftwareCatalog + * + * @spec openspec/specs/settings-service/spec.md + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Service; + +use OCA\SoftwareCatalog\Service\SettingsService; +use OCP\App\IAppManager; +use OCP\IAppConfig; +use OCP\IGroupManager; +use OCP\IL10N; +use OCP\IRequest; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; + +/** + * THE DEFECT UNDER TEST. + * + * getArchiMateStatus() falls back to reading the persisted import/export + * status blobs directly whenever ArchiMateService cannot be resolved from + * the container. That fallback read the config, json_decode()d it, and + * then shipped: + * + * $importValue = []; + * if (is_array($importDecoded) === true) { + * } + * + * — an empty if body left behind by commit 651a055f, which hoisted the + * else-branch out of `if (is_array($d)) { $v = $d; } else { $v = []; }` + * and deleted the if-branch with the `else` keyword. + * + * The decoded status was therefore discarded and the admin panel was + * handed `import => []` and `export => []` unconditionally. A finished + * import and an import that never ran rendered identically, which is the + * failure mode the status blob exists to prevent. + * + * The test asserts on the ITEM inside the envelope (`import.processed`), + * not on the envelope: `getArchiMateStatus()` always returned a + * well-formed array with `import` and `export` keys, and that is precisely + * why the defect survived. + */ +final class SettingsServiceArchiMateStatusFallbackTest extends TestCase +{ + + + /** + * Build a SettingsService whose container ALWAYS throws, so + * getArchiMateStatus() is forced down its config-reading fallback — + * the branch that carried the defect. + * + * @param array $store Reference to the backing key/value store. + * + * @return SettingsService The service under test. + */ + private function makeService(array &$store): SettingsService + { + $config = $this->createMock(IAppConfig::class); + $config->method('getValueString')->willReturnCallback( + function (string $app, string $key, string $default='') use (&$store): string { + return $store[$key] ?? $default; + } + ); + + $container = $this->createMock(ContainerInterface::class); + $container->method('get')->willThrowException( + new \Exception('ArchiMateService not resolvable in a unit context') + ); + + return new SettingsService( + config: $config, + request: $this->createMock(IRequest::class), + container: $container, + appManager: $this->createMock(IAppManager::class), + logger: $this->createMock(LoggerInterface::class), + groupManager: $this->createMock(IGroupManager::class), + l10n: $this->createMock(IL10N::class) + ); + + }//end makeService() + + + /** + * A persisted import status MUST reach the caller. Before the fix this + * returned [] no matter what had been stored. + * + * @return void + */ + public function testPersistedImportStatusIsReturnedNotDiscarded(): void + { + $store = [ + 'archimate_import_status' => json_encode( + [ + 'status' => 'completed', + 'processed' => 42, + ] + ), + ]; + + $status = $this->makeService($store)->getArchiMateStatus(); + + $this->assertSame( + 'completed', + $status['import']['status'] ?? null, + 'The persisted ArchiMate import status must be returned. An empty array here means ' + .'a finished import and an import that never ran are indistinguishable in the admin panel.' + ); + $this->assertSame(42, $status['import']['processed'] ?? null); + + }//end testPersistedImportStatusIsReturnedNotDiscarded() + + + /** + * Same for the export half — two separate sites carried the same + * defect, so both need their own arm. + * + * @return void + */ + public function testPersistedExportStatusIsReturnedNotDiscarded(): void + { + $store = [ + 'archimate_export_status' => json_encode( + [ + 'status' => 'running', + 'exported' => 7, + ] + ), + ]; + + $status = $this->makeService($store)->getArchiMateStatus(); + + $this->assertSame('running', $status['export']['status'] ?? null); + $this->assertSame(7, $status['export']['exported'] ?? null); + + }//end testPersistedExportStatusIsReturnedNotDiscarded() + + + /** + * The positive control for the negative case: when nothing is stored + * the default '{}' decodes to an empty array and an empty array is the + * correct answer. Without this arm the assertions above could be + * satisfied by code that never consults the config at all. + * + * @return void + */ + public function testUnsetStatusStillYieldsAnEmptyArray(): void + { + $store = []; + $status = $this->makeService($store)->getArchiMateStatus(); + + $this->assertSame([], $status['import']); + $this->assertSame([], $status['export']); + + }//end testUnsetStatusStillYieldsAnEmptyArray() + + +}//end class