diff --git a/lib/Service/ArchiMateImportService.php b/lib/Service/ArchiMateImportService.php index 934fa958..af3dd45d 100644 --- a/lib/Service/ArchiMateImportService.php +++ b/lib/Service/ArchiMateImportService.php @@ -1955,6 +1955,48 @@ private function getCurrentOrganisation(): string } }//end getCurrentOrganisation() + /** + * Read a configured id, failing closed on the empty default. + * + * The legacy fallback used to read every id with `''` as its default, so + * an unconfigured instance produced a config array full of empty STRINGS. + * Its consumers guard with `=== null` — `getAmefRegisterId()` and + * `getAmefSchemaIdForType()` reject them with `is_numeric()` + `> 0` — and `'' === null` is + * false. Today nothing reaches a query only because that fallback writes + * PLURAL key names (`views_schema`) while every consumer reads SINGULAR + * ones (`view_schema`), so the lookups miss and fall back to `null`. That + * is an accident of naming, not a defence: adding the singular keys — the + * obvious "cleanup" — would send `register => ''` straight into + * `searchObjects()` as an UNPINNED query, and an unpinned query returns + * rows, which reads exactly like a correct result. + * + * Returning null instead of `''` makes `?? null` downstream yield null, + * which is what every consumer already checks for, and the warning names + * the missing key so a misconfigured import stops reporting "0 objects" + * with no explanation. + * + * @param string $key The app-config key holding the id. + * + * @return string|null The configured id, or null when it is unset. + * + * @spec openspec/specs/archimate-import/spec.md + */ + private function resolveConfiguredId(string $key): ?string + { + $value = $this->config->getValueString('softwarecatalog', $key, ''); + if (trim($value) === '') { + $this->logger->warning( + 'ArchiMate configuration is incomplete — this id is not configured, so it is omitted rather than passed on as an empty string', + ['key' => $key] + ); + + return null; + } + + return $value; + + }//end resolveConfiguredId() + /** * Get AMEF configuration from app config * @@ -1971,49 +2013,27 @@ public function getAmefConfig(): array $decoded = json_decode($config, true); if (is_array($decoded) === false) { - // Fallback to individual config values for backward compatibility. - $decoded = [ - 'register_id' => $this->config->getValueString( - 'softwarecatalog', - 'amef_register', - '' - ), - 'model_schema_id' => $this->config->getValueString( - 'softwarecatalog', - 'amef_model_schema', - '' - ), - 'elements_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_elements_schema', - '' - ), - 'relationships_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_relationships_schema', - '' - ), - 'views_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_views_schema', - '' - ), - 'organizations_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_organizations_schema', - '' - ), - 'folders_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_folders_schema', - '' - ), - 'property_definitions_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_property_definitions_schema', - '' - ), - ]; + // Fallback to individual config values for backward + // compatibility. Every id is read through + // resolveConfiguredId(), which guards the empty default at the + // point of the read and omits the key entirely when it is + // unset — so `?? null` downstream yields null, which is what + // the consumers already check for. + $decoded = array_filter( + [ + 'register_id' => $this->resolveConfiguredId(key: 'amef_register'), + 'model_schema_id' => $this->resolveConfiguredId(key: 'amef_model_schema'), + 'elements_schema' => $this->resolveConfiguredId(key: 'amef_elements_schema'), + 'relationships_schema' => $this->resolveConfiguredId(key: 'amef_relationships_schema'), + 'views_schema' => $this->resolveConfiguredId(key: 'amef_views_schema'), + 'organizations_schema' => $this->resolveConfiguredId(key: 'amef_organizations_schema'), + 'folders_schema' => $this->resolveConfiguredId(key: 'amef_folders_schema'), + 'property_definitions_schema' => $this->resolveConfiguredId(key: 'amef_property_definitions_schema'), + ], + static function ($value) { + return $value !== null; + } + ); }//end if return $decoded; diff --git a/lib/Service/ArchiMateService.php b/lib/Service/ArchiMateService.php index f4c1590c..1a64673f 100644 --- a/lib/Service/ArchiMateService.php +++ b/lib/Service/ArchiMateService.php @@ -1621,6 +1621,48 @@ private function createTempFile(string $content): string return $tempFile; }//end createTempFile() + /** + * Read a configured id, failing closed on the empty default. + * + * The legacy fallback used to read every id with `''` as its default, so + * an unconfigured instance produced a config array full of empty STRINGS. + * Its consumers guard with `=== null` — `ViewService::getViews()` and + * `getView()` throw only when a value `=== null` — and `'' === null` is + * false. Today nothing reaches a query only because that fallback writes + * PLURAL key names (`views_schema`) while every consumer reads SINGULAR + * ones (`view_schema`), so the lookups miss and fall back to `null`. That + * is an accident of naming, not a defence: adding the singular keys — the + * obvious "cleanup" — would send `register => ''` straight into + * `searchObjects()` as an UNPINNED query, and an unpinned query returns + * rows, which reads exactly like a correct result. + * + * Returning null instead of `''` makes `?? null` downstream yield null, + * which is what every consumer already checks for, and the warning names + * the missing key so a misconfigured import stops reporting "0 objects" + * with no explanation. + * + * @param string $key The app-config key holding the id. + * + * @return string|null The configured id, or null when it is unset. + * + * @spec openspec/specs/archimate-import/spec.md + */ + private function resolveConfiguredId(string $key): ?string + { + $value = $this->config->getValueString('softwarecatalog', $key, ''); + if (trim($value) === '') { + $this->logger->warning( + 'ArchiMate configuration is incomplete — this id is not configured, so it is omitted rather than passed on as an empty string', + ['key' => $key] + ); + + return null; + } + + return $value; + + }//end resolveConfiguredId() + /** * Get AMEF configuration from app config * @@ -1637,45 +1679,27 @@ public function getAmefConfig(): array $decoded = json_decode($config, true); if (is_array($decoded) === false) { - // Fallback to individual config values for backward compatibility. - $decoded = [ - 'register_id' => $this->config->getValueString('softwarecatalog', 'amef_register', ''), - 'model_schema_id' => $this->config->getValueString( - 'softwarecatalog', - 'amef_model_schema', - '' - ), - 'elements_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_elements_schema', - '' - ), - 'relationships_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_relationships_schema', - '' - ), - 'views_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_views_schema', - '' - ), - 'organizations_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_organizations_schema', - '' - ), - 'folders_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_folders_schema', - '' - ), - 'property_definitions_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_property_definitions_schema', - '' - ), - ]; + // Fallback to individual config values for backward + // compatibility. Every id is read through + // resolveConfiguredId(), which guards the empty default at the + // point of the read and omits the key entirely when it is + // unset — so `?? null` downstream yields null, which is what + // the consumers already check for. + $decoded = array_filter( + [ + 'register_id' => $this->resolveConfiguredId(key: 'amef_register'), + 'model_schema_id' => $this->resolveConfiguredId(key: 'amef_model_schema'), + 'elements_schema' => $this->resolveConfiguredId(key: 'amef_elements_schema'), + 'relationships_schema' => $this->resolveConfiguredId(key: 'amef_relationships_schema'), + 'views_schema' => $this->resolveConfiguredId(key: 'amef_views_schema'), + 'organizations_schema' => $this->resolveConfiguredId(key: 'amef_organizations_schema'), + 'folders_schema' => $this->resolveConfiguredId(key: 'amef_folders_schema'), + 'property_definitions_schema' => $this->resolveConfiguredId(key: 'amef_property_definitions_schema'), + ], + static function ($value) { + return $value !== null; + } + ); }//end if return $decoded; diff --git a/lib/Service/ViewService.php b/lib/Service/ViewService.php index d68bb8ce..e5a2a04c 100644 --- a/lib/Service/ViewService.php +++ b/lib/Service/ViewService.php @@ -711,6 +711,22 @@ private function getModulesData(): array $amefConfig = $this->settingsService->getAmefConfig(); $registerId = $amefConfig['register_id'] ?? null; + // Fail closed on an unconfigured register. This used to be the one + // read of `register_id` with no guard at all: the loop below only + // checks the SCHEMA, so an empty register would have been pinned + // into `@self` and OpenRegister asked for "any register" — an + // unpinned query returns rows, which reads exactly like a correct + // result. `empty()` rather than `=== null` because the legacy + // config fallback resolves unset ids to `''`, and `'' === null` is + // false. + if (empty($registerId) === true) { + $this->logger->warning( + 'ViewService: AMEF register is not configured; skipping the module lookup rather than issuing an unpinned query' + ); + + return []; + } + // Modules could be in various schemas - check common ones. $moduleSchemas = [ $amefConfig['module_schema'] ?? null, @@ -721,7 +737,7 @@ private function getModulesData(): array $allModules = []; foreach ($moduleSchemas as $schemaId) { - if ($schemaId === null) { + if (empty($schemaId) === true) { continue; } diff --git a/tests/Unit/Service/ArchiMateConfigFailModeTest.php b/tests/Unit/Service/ArchiMateConfigFailModeTest.php new file mode 100644 index 00000000..d532c5fa --- /dev/null +++ b/tests/Unit/Service/ArchiMateConfigFailModeTest.php @@ -0,0 +1,161 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @version GIT: + * @link https://codeberg.org/Conduction/SoftwareCatalog + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Service; + +use OCA\SoftwareCatalog\Service\ArchiMateImportService; +use PHPUnit\Framework\TestCase; + +/** + * The AMEF config resolvers must omit unresolved ids, never emit ''. + * + * @category Test + * @package OCA\SoftwareCatalog\Tests\Unit\Service + * @author Conduction b.v. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @version GIT: + * @link https://codeberg.org/Conduction/SoftwareCatalog + */ +class ArchiMateConfigFailModeTest extends TestCase +{ + + + /** + * Build the service with only the two collaborators the resolver touches. + * + * @param array $values Config key => stored value. + * + * @return ArchiMateImportService + */ + private function serviceWithConfig(array $values): ArchiMateImportService + { + // The legacy fallback is only reached when `amef_config` does not + // decode to an array. Its DEFAULT is '{}', which decodes to [] — so on + // an instance that never wrote the key, getAmefConfig() returns [] and + // the fallback never runs at all. Malformed JSON is the branch under + // test here, and callers must not be given empty ids on that path. + $values['amef_config'] = $values['amef_config'] ?? 'not-json'; + + $config = $this->createMock(\OCP\IAppConfig::class); + $config->method('getValueString')->willReturnCallback( + function (string $app, string $key, string $default = '') use ($values) { + return $values[$key] ?? $default; + } + ); + + $service = (new \ReflectionClass(ArchiMateImportService::class))->newInstanceWithoutConstructor(); + + foreach (['config' => $config, 'logger' => $this->createMock(\Psr\Log\LoggerInterface::class)] as $prop => $value) { + $property = new \ReflectionProperty(ArchiMateImportService::class, $prop); + $property->setValue($service, $value); + } + + return $service; + + }//end serviceWithConfig() + + + /** + * An unconfigured instance yields no id keys at all — not empty strings. + * + * @return void + */ + public function testUnconfiguredInstanceOmitsEveryIdRatherThanEmittingEmptyStrings(): void + { + $config = $this->serviceWithConfig([])->getAmefConfig(); + + // Guard against a vacuous pass: if the fallback had not run at all, + // the loop below would iterate nothing and prove nothing. + $this->assertSame([], $config, 'every id was unset, so none should survive'); + + foreach ($config as $key => $value) { + $this->assertNotSame('', $value, sprintf('"%s" was handed on as an empty string', $key)); + } + + // The property the consumers rely on: `?? null` must yield null. + $this->assertNull($config['register_id'] ?? null); + $this->assertNull($config['views_schema'] ?? null); + + }//end testUnconfiguredInstanceOmitsEveryIdRatherThanEmittingEmptyStrings() + + + /** + * A configured id still comes through untouched. + * + * @return void + */ + public function testConfiguredIdsAreReturnedUnchanged(): void + { + $config = $this->serviceWithConfig( + [ + 'amef_register' => '11', + 'amef_views_schema' => '42', + ] + )->getAmefConfig(); + + $this->assertSame('11', $config['register_id']); + $this->assertSame('42', $config['views_schema']); + + }//end testConfiguredIdsAreReturnedUnchanged() + + + /** + * A partially configured instance keeps what is set and drops what is not. + * + * This is the shape that makes the difference real: with `''` retained, a + * `=== null` guard on the missing half would pass. + * + * @return void + */ + public function testPartialConfigurationKeepsSetIdsAndDropsUnsetOnes(): void + { + $config = $this->serviceWithConfig(['amef_register' => '11'])->getAmefConfig(); + + $this->assertSame('11', $config['register_id']); + $this->assertArrayNotHasKey('views_schema', $config); + $this->assertArrayNotHasKey('elements_schema', $config); + + }//end testPartialConfigurationKeepsSetIdsAndDropsUnsetOnes() + + + /** + * A whitespace-only id counts as unset, not as a usable value. + * + * @return void + */ + public function testWhitespaceOnlyIdIsTreatedAsUnset(): void + { + $config = $this->serviceWithConfig(['amef_register' => ' '])->getAmefConfig(); + + $this->assertArrayNotHasKey('register_id', $config); + + }//end testWhitespaceOnlyIdIsTreatedAsUnset() + + +}//end class