From fbd389bb361cd5d2337d92a37d2c969b2bd18568 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Fri, 24 Jul 2026 14:44:06 +0200 Subject: [PATCH 1/3] fix(settings): fold monolith content into import version + fix broken re-import gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SettingsService::loadSettings() computed the OpenRegister import version from the register JSON's own info.version plus a hash of ADR-037 fragment files only. A change that edited the monolith softwarecatalogus_register.json directly without also bumping info.version by hand produced a byte-identical version, and OpenRegister's version-gated importFromApp() silently skipped the import — eight merged market-gap changes went dead on an upgraded instance while CI and `occ upgrade` both reported success. Fold an md5 of the monolith file's own content into the computed version (+base., alongside the existing +frag.) so ANY register edit forces a re-import, without weakening the gate into always-import. Also fix a second, independently sufficient defect found while reading this code path: SettingsService::shouldLoadSettings() compared this app's own semantic version against ConfigurationService::getConfiguredAppVersion() — but that value is the register-content version this same service passes to importFromApp(), not an app semver. version_compare("0.2.17", "2.3.1+frag.9003c029", ">") is false (verified), so once any import ever stored such a value, loadSettings() could never run again for any future app version bump. This is the confirmed mechanism behind the live evidence's "versions DID differ ... yet nothing imported and no import log line appeared". shouldLoadSettings() now always returns true; the actual (potentially expensive) import remains gated by importFromApp()'s own comparison of two like-for-like content-derived versions. Add post-import verification: after importFromApp() runs, confirm every schema slug in the effective (monolith + fragments) register resolves in OpenRegister, and that the schema ids this app tracks for its own object types are non-null. Mismatches are logged as warnings and persisted so getConfigurationStatus() surfaces a translated warning instead of a no-op import looking identical to full success. Investigated the three duplicate "Software Catalog Register" configuration rows: this app's only importFromApp() call site always passes the same constant appId, so the duplication is not attributable to this app. Code review of OpenRegister's ImportHandler/ ConfigurationMapper found findByApp()/findBySourceUrl() organisation- scope their lookup, so a caller whose active-organisation context differs from an existing row's can fail to find it and create a duplicate. Filed ConductionNL/openregister#2072 with the full mechanism; referenced from a code comment rather than hacked around. --- lib/AppInfo/Application.php | 3 +- lib/Service/SettingsService.php | 371 ++++++++++++++---- tests/Stubs/Db/SchemaMapper.php | 43 ++ .../SettingsServiceConfigVersionTest.php | 170 ++++++++ .../SettingsServiceDecompositionTest.php | 13 +- .../Service/SettingsServiceEolConfigTest.php | 4 +- ...ettingsServiceRegisterVerificationTest.php | 296 ++++++++++++++ .../SettingsServiceResolverWiringTest.php | 6 + 8 files changed, 829 insertions(+), 77 deletions(-) create mode 100644 tests/Stubs/Db/SchemaMapper.php create mode 100644 tests/Unit/Service/SettingsServiceConfigVersionTest.php create mode 100644 tests/Unit/Service/SettingsServiceRegisterVerificationTest.php diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 6f573e7c..bf53779a 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -314,7 +314,8 @@ function ($container) { container: $container, appManager: $container->get('OCP\App\IAppManager'), logger: $container->get('Psr\Log\LoggerInterface'), - groupManager: $container->get(IGroupManager::class) + groupManager: $container->get(IGroupManager::class), + l10n: $container->get('OCP\IL10N') ); } ); diff --git a/lib/Service/SettingsService.php b/lib/Service/SettingsService.php index ec853fc7..66dba0c6 100644 --- a/lib/Service/SettingsService.php +++ b/lib/Service/SettingsService.php @@ -20,6 +20,7 @@ use OCP\IAppConfig; use OCP\IGroupManager; +use OCP\IL10N; use OCP\IRequest; use OCP\App\IAppManager; use Psr\Container\ContainerInterface; @@ -111,6 +112,9 @@ class SettingsService * @param IAppManager $appManager App manager interface * @param LoggerInterface $logger Logger interface * @param IGroupManager $groupManager Group manager interface + * @param IL10N $l10n Localization service, used for the + * user-facing register-verification warning text + * surfaced via getConfigurationStatus(). */ public function __construct( private readonly IAppConfig $config, @@ -118,7 +122,8 @@ public function __construct( private readonly ContainerInterface $container, private readonly IAppManager $appManager, private readonly LoggerInterface $logger, - private readonly IGroupManager $groupManager + private readonly IGroupManager $groupManager, + private readonly IL10N $l10n ) { $this->appName = 'softwarecatalog'; }//end __construct() @@ -1235,11 +1240,77 @@ public function isFullyConfigured(): bool public function getConfigurationStatus(): array { return [ - 'organization' => $this->buildObjectTypeStatusEntry(objectType: 'organization'), - 'contact' => $this->buildObjectTypeStatusEntry(objectType: 'contactpersoon'), + 'organization' => $this->buildObjectTypeStatusEntry(objectType: 'organization'), + 'contact' => $this->buildObjectTypeStatusEntry(objectType: 'contactpersoon'), + 'registerVerification' => $this->getRegisterVerificationStatus(), ]; }//end getConfigurationStatus() + /** + * Reads back the most recent register-verification result persisted by + * persistRegisterVerificationStatus() (register-import-reliability), + * so a no-op or partial import is visible in the settings status + * payload rather than looking identical to a fully successful one. + * + * @return array{ + * ok: bool, + * checked: bool, + * missingSchemas: array, + * unresolvedObjectTypes: array, + * message: string|null + * } + * + * @spec openspec/specs/settings-service/spec.md#requirement-the-system-shall-read-and-persist-every-configuration-domain-req-002 + */ + protected function getRegisterVerificationStatus(): array + { + $unchecked = [ + 'ok' => true, + 'checked' => false, + 'missingSchemas' => [], + 'unresolvedObjectTypes' => [], + 'message' => null, + ]; + + $raw = $this->config->getValueString($this->appName, 'register_verification_status', ''); + if ($raw === '') { + return $unchecked; + } + + $decoded = json_decode($raw, true); + if (is_array($decoded) === false) { + return $unchecked; + } + + $ok = ($decoded['ok'] ?? true) === true; + + $missingSchemas = []; + if (is_array($decoded['missingSchemas'] ?? null) === true) { + $missingSchemas = $decoded['missingSchemas']; + } + + $unresolvedObjectTypes = []; + if (is_array($decoded['unresolvedObjectTypes'] ?? null) === true) { + $unresolvedObjectTypes = $decoded['unresolvedObjectTypes']; + } + + $message = null; + if ($ok === false) { + $message = $this->l10n->t( + 'The most recent register import did not fully reach OpenRegister — some schemas or ' + .'object types could not be verified. Re-run the import or check the server log for details.' + ); + } + + return [ + 'ok' => $ok, + 'checked' => true, + 'missingSchemas' => $missingSchemas, + 'unresolvedObjectTypes' => $unresolvedObjectTypes, + 'message' => $message, + ]; + }//end getRegisterVerificationStatus() + /** * Builds a single object-type status entry (configured/schemaId/registerId). * @@ -1507,15 +1578,18 @@ public function loadSettings(bool $force=false): array try { $configurationService = $this->getConfigurationService(); - // Use the configuration file's own version (from info.version) for change detection. - // This ensures changes to the JSON file trigger re-import even if app version is unchanged. - $configVersion = $softwareCatalogSettings['info']['version'] ?? '0.0.0'; - - // Fold the fragment signature into the version so OpenRegister's - // version-gated importFromApp re-imports whenever fragments change. - if ($fragmentSig !== '') { - $configVersion .= '+frag.'.substr(md5($fragmentSig), 0, 8); - } + // Content-derived version signature (register-import-reliability): + // folds a hash of the monolith's OWN content (+base.) alongside + // the existing fragment-file hash (+frag.) so ANY register edit — + // monolith or fragment — produces a version OpenRegister has not seen + // before and therefore re-imports, instead of relying on a human + // remembering to bump info.version by hand. See computeConfigVersion()'s + // own docblock for the full @spec anchor. + $configVersion = self::computeConfigVersion( + baseVersion: (string) ($softwareCatalogSettings['info']['version'] ?? '0.0.0'), + monolithContent: $softwareCatalogContent, + fragmentSig: $fragmentSig + ); // Log the import attempt for debugging. $this->logger->info( @@ -1529,6 +1603,17 @@ public function loadSettings(bool $force=false): array ); // Use importFromApp which handles Configuration entity creation automatically. + // NOTE (register-import-reliability): this app's own call site is the only + // place it calls importFromApp() and always passes the same appId, so it + // cannot itself cause duplicate Configuration rows. If more than one + // "Software Catalog Register" configuration row is ever observed in + // oc_openregister_configurations, the cause is upstream: OpenRegister's + // ConfigurationMapper::findByApp()/findBySourceUrl() organisation-scope + // their lookup, so a caller whose active-organisation context differs from + // an existing row's can fail to find it and create a duplicate instead. See + // https://github.com/ConductionNL/openregister/issues/2072 (filed with the + // full mechanism) — do not attempt to de-duplicate rows or change lookup + // behavior from this app; the fix belongs in OpenRegister. $importResult = $configurationService->importFromApp( appId: \OCA\SoftwareCatalog\AppInfo\Application::APP_ID, data: $softwareCatalogSettings, @@ -1545,6 +1630,18 @@ public function loadSettings(bool $force=false): array $results['softwarecatalog_imported'] = true; $results['import_result'] = $importResult; + + // Post-import verification (register-import-reliability): a version-gate + // skip, a partial import, or a duplicate-configuration-row resolution + // mistake on OpenRegister's side can all make an import look + // successful here while the live schema set never actually changed. + // Walk the effective (monolith + fragments) merged register and confirm + // every schema slug it declares, and every schema id this app resolves + // for its own object-type lookups, actually resolves in OpenRegister. See + // verifyRegisterAgainstEffectiveConfig()'s own docblock for the @spec anchor. + $verification = $this->verifyRegisterAgainstEffectiveConfig(effectiveRegister: $softwareCatalogSettings); + $results['registerVerification'] = $verification; + $this->persistRegisterVerificationStatus(verification: $verification); } catch (\Exception $e) { $results['softwarecatalog_import_error'] = $e->getMessage(); $this->logger->error( @@ -1575,6 +1672,158 @@ public function loadSettings(bool $force=false): array }//end try }//end loadSettings() + /** + * Computes the content-derived import version passed to + * ConfigurationService::importFromApp()'s version gate. + * + * Folds an md5 of the monolith register file's OWN raw content into the + * signature (`+base.`) alongside the existing ADR-037 fragment + * signature (`+frag.`), so ANY register change — whether it + * edits the monolith directly or lands as a fragment file — produces a + * version string OpenRegister has not seen before and therefore + * re-imports. Before this, the signature was derived only from + * `info.version` + the fragment hash: a monolith edit that did not also + * bump `info.version` by hand produced a byte-identical version and the + * import was silently skipped. + * + * Deliberately content-derived rather than "always re-import" — the + * hash is cheap (one extra md5() call on a string already read into + * memory) and only changes when the shipped register content actually + * changes, so an unchanged register still short-circuits at + * OpenRegister's version gate. + * + * @param string $baseVersion The register JSON's own `info.version` field. + * @param string $monolithContent The raw (unparsed) content of the monolith register file. + * @param string $fragmentSig The accumulated `filename:md5;` signature of merged + * ADR-037 fragment files, or an empty string if none exist. + * + * @return string The content-derived version, e.g. `2.4.0+base.1a2b3c4d+frag.9003c029`. + * + * @spec openspec/specs/settings-service/spec.md#requirement-the-system-shall-run-auto-configuration-import-and-configuration-maintenance-req-003 + */ + private static function computeConfigVersion(string $baseVersion, string $monolithContent, string $fragmentSig): string + { + $configVersion = $baseVersion; + $configVersion .= '+base.'.substr(md5($monolithContent), 0, 8); + + if ($fragmentSig !== '') { + $configVersion .= '+frag.'.substr(md5($fragmentSig), 0, 8); + } + + return $configVersion; + }//end computeConfigVersion() + + /** + * Verifies the live OpenRegister schema set against the register this + * app just (attempted to) import, so a no-op or partial import is + * observable instead of looking identical to a full success. + * + * Walks every schema slug declared in the effective (monolith + + * merged fragments) register and confirms it resolves in OpenRegister, + * and confirms every schema id this app resolves for its own tracked + * object types (per getConfigurationStatus()) is non-null. Any miss is + * logged as a WARNING and recorded in the returned summary rather than + * failing the request — verification is a diagnostic, not a gate. + * + * @param array $effectiveRegister The merged register data + * (monolith + fragments) that was just imported. + * + * @return array{ok: bool, missingSchemas: array, unresolvedObjectTypes: array} + * + * @spec openspec/specs/settings-service/spec.md#requirement-the-system-shall-run-auto-configuration-import-and-configuration-maintenance-req-003 + */ + private function verifyRegisterAgainstEffectiveConfig(array $effectiveRegister): array + { + $verification = [ + 'ok' => true, + 'missingSchemas' => [], + 'unresolvedObjectTypes' => [], + ]; + + $schemas = $effectiveRegister['components']['schemas'] ?? []; + if (is_array($schemas) === false || empty($schemas) === true) { + return $verification; + } + + try { + $schemaMapper = $this->container->get(\OCA\OpenRegister\Db\SchemaMapper::class); + } catch (\Throwable $e) { + // Cannot verify without the mapper — do not fail the import over a diagnostic. + $this->logger->warning( + 'SettingsService: could not resolve SchemaMapper for register verification, skipping', + ['exception' => $e->getMessage()] + ); + return $verification; + } + + foreach (array_keys($schemas) as $slug) { + if (is_string($slug) === false || $slug === '') { + continue; + } + + try { + $matches = $schemaMapper->findBySlug(slug: $slug, limit: 1); + } catch (\Throwable $e) { + $matches = []; + } + + if (empty($matches) === true) { + $verification['ok'] = false; + $verification['missingSchemas'][] = $slug; + $this->logger->warning( + 'SettingsService: register verification found a schema slug from the shipped ' + .'register that does not resolve in OpenRegister — the import may not have ' + .'reached this instance.', + ['schemaSlug' => $slug] + ); + } + }//end foreach + + // Also confirm the object types this app's own status reporting tracks + // (see getConfigurationStatus()) resolve to a schema id post-import. + foreach (['organization', 'contactpersoon'] as $objectType) { + if ($this->getSchemaIdForObjectType(objectType: $objectType) === null) { + $verification['ok'] = false; + $verification['unresolvedObjectTypes'][] = $objectType; + $this->logger->warning( + 'SettingsService: register verification found an object type this app tracks ' + .'that does not resolve to a configured schema id after import.', + ['objectType' => $objectType] + ); + } + } + + return $verification; + }//end verifyRegisterAgainstEffectiveConfig() + + /** + * Persists the most recent register-verification result to app config + * so getConfigurationStatus() can surface it without re-running an + * import — verification only runs when loadSettings() actually + * attempts an import, while status can be polled independently. + * + * @param array $verification The summary from verifyRegisterAgainstEffectiveConfig(). + * + * @return void + * + * @spec openspec/specs/settings-service/spec.md#requirement-the-system-shall-read-and-persist-every-configuration-domain-req-002 + */ + private function persistRegisterVerificationStatus(array $verification): void + { + try { + $this->config->setValueString( + $this->appName, + 'register_verification_status', + json_encode($verification, JSON_THROW_ON_ERROR) + ); + } catch (\Throwable $e) { + $this->logger->warning( + 'SettingsService: failed to persist register verification status', + ['exception' => $e->getMessage()] + ); + } + }//end persistRegisterVerificationStatus() + /** * Gets the list of generic user groups from configuration * @@ -3057,71 +3306,45 @@ private function createSmtpTransport(array $settings): \Symfony\Component\Mailer }//end createSmtpTransport() /** - * Check if settings should be loaded based on version comparison. - * - * This method compares the current app version with the stored configuration - * version to determine if a settings import is needed. - * - * @return bool True if settings should be loaded, false otherwise. + * Whether initialize() should attempt loadSettings(). + * + * ALWAYS true (register-import-reliability). This previously compared + * this app's own semantic version (`appManager->getAppVersion()`, e.g. + * "0.2.17") against `ConfigurationService::getConfiguredAppVersion()` — + * but that value is not an app semver at all: it is exactly the + * register-content version string this same service computes and + * passes as the `version` argument to `importFromApp()` (e.g. + * "2.3.1+frag.9003c029" — see computeConfigVersion()). Those are two + * unrelated versioning schemes sharing one stored slot. + * `version_compare("0.2.17", "2.3.1+frag.9003c029", ">")` evaluates to + * `false` (verified) because the leading numeral of an app semver here + * (0) is always less than the leading numeral of a register-content + * version (2). Once any import has ever stored such a value, this + * comparison could never return true again for any future app + * version bump — permanently preventing loadSettings() from being + * invoked, regardless of subsequent register changes. This is the + * confirmed mechanism behind the live evidence's "versions DID differ + * ... yet nothing imported and no import log line appeared": the + * "Attempting to import" log line in loadSettings() never fired + * because loadSettings() was never entered. + * + * loadSettings() is only ever reached from an explicit admin-triggered + * controller action or the install/upgrade repair step + * (InitializeSettings::run(), which has its own + * last_initialized_version gate against repeated runs within the same + * app version) — never from a per-request code path — so always + * attempting it here is cheap (a couple of file reads plus md5()). The + * actual, potentially expensive, schema/register write remains gated + * by importFromApp()'s own comparison of two like-for-like + * content-derived versions. + * + * @return bool Always true — see above. + * + * @spec openspec/specs/settings-service/spec.md#requirement-the-system-shall-run-auto-configuration-import-and-configuration-maintenance-req-003 */ private function shouldLoadSettings(): bool { - try { - // Get the current app version. - $currentAppVersion = $this->appManager->getAppVersion(\OCA\SoftwareCatalog\AppInfo\Application::APP_ID); - - $this->logger->info( - 'SettingsService: Checking if settings should be loaded', - [ - 'current_app_version' => $currentAppVersion, - ] - ); - - // Get the configuration service to check stored version. - $configurationService = $this->getConfigurationService(); - $appId = \OCA\SoftwareCatalog\AppInfo\Application::APP_ID; - $storedVersion = $configurationService->getConfiguredAppVersion($appId); - - $this->logger->info( - 'SettingsService: Version comparison details', - [ - 'current_app_version' => $currentAppVersion, - 'stored_config_version' => $storedVersion, - 'stored_version_is_null' => $storedVersion === null, - ] - ); - - // If no stored version exists, we need to load settings. - if ($storedVersion === null) { - $this->logger->info('SettingsService: No stored version found, settings should be loaded'); - return true; - } - - // Compare versions using semantic versioning. - // Load settings if current version is newer than stored version. - $shouldLoad = version_compare($currentAppVersion, $storedVersion, '>'); - - $this->logger->info( - 'SettingsService: Version comparison result', - [ - 'current_version' => $currentAppVersion, - 'stored_version' => $storedVersion, - 'should_load' => $shouldLoad, - 'version_compare_result' => version_compare($currentAppVersion, $storedVersion), - ] - ); - - return $shouldLoad; - } catch (\Exception $e) { - // If we can't determine versions, err on the side of loading settings. - $this->logger->warning( - 'Failed to check if settings should be loaded: '.$e->getMessage(), - [ - 'exception' => $e, - ] - ); - return true; - }//end try + return true; }//end shouldLoadSettings() /** diff --git a/tests/Stubs/Db/SchemaMapper.php b/tests/Stubs/Db/SchemaMapper.php new file mode 100644 index 00000000..79687d16 --- /dev/null +++ b/tests/Stubs/Db/SchemaMapper.php @@ -0,0 +1,43 @@ + Array of matching schema entities. + */ + abstract public function findBySlug(string $slug, int $limit = 10, int $offset = 0): array; + + +}//end class diff --git a/tests/Unit/Service/SettingsServiceConfigVersionTest.php b/tests/Unit/Service/SettingsServiceConfigVersionTest.php new file mode 100644 index 00000000..d22985ab --- /dev/null +++ b/tests/Unit/Service/SettingsServiceConfigVersionTest.php @@ -0,0 +1,170 @@ + + * @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#requirement-the-system-shall-run-auto-configuration-import-and-configuration-maintenance-req-003 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Service; + +use OCA\SoftwareCatalog\Service\SettingsService; +use PHPUnit\Framework\TestCase; +use ReflectionMethod; + +/** + * Regression coverage for the original defect: the import version passed + * to OpenRegister's version-gated importFromApp() was derived only from + * the register JSON's own `info.version` field plus a hash of the + * ADR-037 fragment files — never from the monolith register file's own + * content. A change that edited the monolith directly without also + * bumping `info.version` by hand produced a byte-identical version, and + * OpenRegister's version gate silently skipped the import. These tests + * exercise `computeConfigVersion()` directly (a pure function of three + * strings, invoked via reflection since it is private) so the defect + * class can be asserted against without needing a live OpenRegister + * container or filesystem fixtures. + * + * @spec openspec/specs/settings-service/spec.md#requirement-the-system-shall-run-auto-configuration-import-and-configuration-maintenance-req-003 + */ +final class SettingsServiceConfigVersionTest extends TestCase +{ + + + /** + * Invoke the private static SettingsService::computeConfigVersion(). + * + * @param string $baseVersion The register JSON's own info.version. + * @param string $monolithContent The raw monolith file content. + * @param string $fragmentSig The accumulated fragment signature (may be empty). + * + * @return string The computed content-derived version. + */ + private function computeConfigVersion(string $baseVersion, string $monolithContent, string $fragmentSig): string + { + $m = new ReflectionMethod(SettingsService::class, 'computeConfigVersion'); + $m->setAccessible(true); + return $m->invoke(null, $baseVersion, $monolithContent, $fragmentSig); + }//end computeConfigVersion() + + + /** + * THE REGRESSION TEST: a monolith-content-only edit — info.version and + * every fragment file byte-identical — MUST still change the computed + * version. A signature derived only from info.version + the fragment + * hash (the original, defective behavior) would keep this identical, + * which is exactly how eight merged market-gap changes went dead on an + * upgraded instance while CI and `occ upgrade` both reported success. + * + * @return void + */ + public function testMonolithContentChangeAloneChangesComputedVersion(): void + { + $baseVersion = '2.4.0'; + $fragmentSig = 'contract-approval.json:'.md5('{"paths":{}}').';'; + + $before = $this->computeConfigVersion( + baseVersion: $baseVersion, + monolithContent: '{"info":{"version":"2.4.0"},"components":{"schemas":{"module":{}}}}', + fragmentSig: $fragmentSig + ); + + // Simulate a wave change editing the monolith directly (new schema + // added) WITHOUT bumping info.version — the exact scenario from the + // live reproduction — and with every fragment file untouched. + $after = $this->computeConfigVersion( + baseVersion: $baseVersion, + monolithContent: '{"info":{"version":"2.4.0"},"components":{"schemas":{"module":{},"bioMaatregel":{}}}}', + fragmentSig: $fragmentSig + ); + + $this->assertNotSame( + $before, + $after, + 'A monolith content change with an unchanged info.version and unchanged fragments ' + .'must still change the computed configVersion, otherwise OpenRegister\'s version-gated ' + .'importFromApp() silently skips the re-import.' + ); + }//end testMonolithContentChangeAloneChangesComputedVersion() + + + /** + * Conversely: identical inputs (no monolith change, no fragment + * change, no info.version change) MUST produce an identical version, + * so an unchanged register still short-circuits at OpenRegister's + * version gate rather than re-importing on every repair-step run + * (performance regression the design explicitly guards against). + * + * @return void + */ + public function testUnchangedInputsProduceIdenticalVersion(): void + { + $args = [ + 'baseVersion' => '2.4.0', + 'monolithContent' => '{"info":{"version":"2.4.0"},"components":{"schemas":{"module":{}}}}', + 'fragmentSig' => 'contract-approval.json:'.md5('{}').';', + ]; + + $first = $this->computeConfigVersion(...$args); + $second = $this->computeConfigVersion(...$args); + + $this->assertSame($first, $second); + }//end testUnchangedInputsProduceIdenticalVersion() + + + /** + * A fragment-content-only change (monolith and info.version untouched) + * must also change the computed version — the pre-existing, already + * correct half of the signature must keep working after this fix. + * + * @return void + */ + public function testFragmentContentChangeAloneChangesComputedVersion(): void + { + $monolithContent = '{"info":{"version":"2.4.0"},"components":{"schemas":{"module":{}}}}'; + + $before = $this->computeConfigVersion( + baseVersion: '2.4.0', + monolithContent: $monolithContent, + fragmentSig: 'contract-approval.json:'.md5('{"paths":{}}').';' + ); + $after = $this->computeConfigVersion( + baseVersion: '2.4.0', + monolithContent: $monolithContent, + fragmentSig: 'contract-approval.json:'.md5('{"paths":{"/foo":{}}}').';' + ); + + $this->assertNotSame($before, $after); + }//end testFragmentContentChangeAloneChangesComputedVersion() + + + /** + * The computed version carries a `+base.` component whenever a + * monolith is present, and only appends `+frag.` when fragment + * files actually exist — an app with no fragments yet must not carry a + * stray, empty `+frag.` suffix. + * + * @return void + */ + public function testVersionFormatOmitsFragmentSuffixWhenNoFragmentsExist(): void + { + $version = $this->computeConfigVersion( + baseVersion: '2.4.0', + monolithContent: '{"info":{"version":"2.4.0"}}', + fragmentSig: '' + ); + + $this->assertStringStartsWith('2.4.0+base.', $version); + $this->assertStringNotContainsString('+frag.', $version); + }//end testVersionFormatOmitsFragmentSuffixWhenNoFragmentsExist() +}//end class diff --git a/tests/Unit/Service/SettingsServiceDecompositionTest.php b/tests/Unit/Service/SettingsServiceDecompositionTest.php index dfe16bbe..5575d855 100644 --- a/tests/Unit/Service/SettingsServiceDecompositionTest.php +++ b/tests/Unit/Service/SettingsServiceDecompositionTest.php @@ -113,7 +113,7 @@ public function testGetConfigurationStatusDelegatesToHelperForBothObjectTypes(): { $service = $this->getMockBuilder(SettingsService::class) ->disableOriginalConstructor() - ->onlyMethods(['getSchemaIdForObjectType', 'getRegisterIdForObjectType']) + ->onlyMethods(['getSchemaIdForObjectType', 'getRegisterIdForObjectType', 'getRegisterVerificationStatus']) ->getMock(); $service->method('getSchemaIdForObjectType')->willReturnMap( @@ -128,17 +128,28 @@ public function testGetConfigurationStatusDelegatesToHelperForBothObjectTypes(): ['contactpersoon', 22], ] ); + $service->method('getRegisterVerificationStatus')->willReturn( + [ + 'ok' => true, + 'checked' => false, + 'missingSchemas' => [], + 'unresolvedObjectTypes' => [], + 'message' => null, + ] + ); $status = $service->getConfigurationStatus(); $this->assertArrayHasKey('organization', $status); $this->assertArrayHasKey('contact', $status); + $this->assertArrayHasKey('registerVerification', $status); $this->assertTrue($status['organization']['configured']); $this->assertSame(11, $status['organization']['schemaId']); $this->assertSame(21, $status['organization']['registerId']); $this->assertTrue($status['contact']['configured']); $this->assertSame(12, $status['contact']['schemaId']); $this->assertSame(22, $status['contact']['registerId']); + $this->assertTrue($status['registerVerification']['ok']); }//end testGetConfigurationStatusDelegatesToHelperForBothObjectTypes() diff --git a/tests/Unit/Service/SettingsServiceEolConfigTest.php b/tests/Unit/Service/SettingsServiceEolConfigTest.php index 2e8e5497..6cd89d73 100644 --- a/tests/Unit/Service/SettingsServiceEolConfigTest.php +++ b/tests/Unit/Service/SettingsServiceEolConfigTest.php @@ -24,6 +24,7 @@ use OCP\App\IAppManager; use OCP\IAppConfig; use OCP\IGroupManager; +use OCP\IL10N; use OCP\IRequest; use PHPUnit\Framework\TestCase; use Psr\Container\ContainerInterface; @@ -65,7 +66,8 @@ function (string $app, string $key, string $value) use (&$store): bool { container: $this->createMock(ContainerInterface::class), appManager: $this->createMock(IAppManager::class), logger: $this->createMock(LoggerInterface::class), - groupManager: $this->createMock(IGroupManager::class) + groupManager: $this->createMock(IGroupManager::class), + l10n: $this->createMock(IL10N::class) ); }//end makeService() diff --git a/tests/Unit/Service/SettingsServiceRegisterVerificationTest.php b/tests/Unit/Service/SettingsServiceRegisterVerificationTest.php new file mode 100644 index 00000000..981b8cdf --- /dev/null +++ b/tests/Unit/Service/SettingsServiceRegisterVerificationTest.php @@ -0,0 +1,296 @@ + + * @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#requirement-the-system-shall-run-auto-configuration-import-and-configuration-maintenance-req-003 + * @spec openspec/specs/settings-service/spec.md#requirement-the-system-shall-read-and-persist-every-configuration-domain-req-002 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Service; + +use OCA\OpenRegister\Db\SchemaMapper; +use OCA\SoftwareCatalog\Service\SettingsService; +use OCP\App\IAppManager; +use OCP\IAppConfig; +use OCP\IGroupManager; +use OCP\IL10N; +use OCP\IRequest; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; +use ReflectionMethod; + +/** + * Covers verifyRegisterAgainstEffectiveConfig(), getRegisterVerificationStatus(), + * and shouldLoadSettings() — the three pieces that together turn a no-op or + * silently-blocked register import into a visible, diagnosable state. + */ +final class SettingsServiceRegisterVerificationTest extends TestCase +{ + + + /** + * Build a SettingsService with a real (mocked-dependency) constructor + * so verifyRegisterAgainstEffectiveConfig()'s container->get() call + * resolves, while getSchemaIdForObjectType() is overridden so the test + * does not need to also fake the voorzieningen/AMEF config lookups it + * would otherwise read through IAppConfig. + * + * @param SchemaMapper|MockObject $schemaMapper Mock returned for SchemaMapper::class. + * @param int|null $resolvedObjectType Value getSchemaIdForObjectType() returns. + * + * @return array{0: SettingsService, 1: LoggerInterface|MockObject} + */ + private function makeServiceForVerification($schemaMapper, ?int $resolvedObjectType): array + { + $container = $this->createMock(ContainerInterface::class); + $container->method('get')->willReturnCallback( + function (string $id) use ($schemaMapper) { + if ($id === SchemaMapper::class) { + return $schemaMapper; + } + + throw new \RuntimeException("Unexpected container->get({$id}) in test"); + } + ); + + $logger = $this->createMock(LoggerInterface::class); + + $service = $this->getMockBuilder(SettingsService::class) + ->setConstructorArgs( + [ + $this->createMock(IAppConfig::class), + $this->createMock(IRequest::class), + $container, + $this->createMock(IAppManager::class), + $logger, + $this->createMock(IGroupManager::class), + $this->createMock(IL10N::class), + ] + ) + ->onlyMethods(['getSchemaIdForObjectType']) + ->getMock(); + + $service->method('getSchemaIdForObjectType')->willReturn($resolvedObjectType); + + return [$service, $logger]; + }//end makeServiceForVerification() + + + /** + * Invoke the private verifyRegisterAgainstEffectiveConfig() method. + * + * @param SettingsService $service The service under test. + * @param array $effectiveRegister The merged register data. + * + * @return array{ok: bool, missingSchemas: array, unresolvedObjectTypes: array} + */ + private function verify(SettingsService $service, array $effectiveRegister): array + { + $m = new ReflectionMethod($service, 'verifyRegisterAgainstEffectiveConfig'); + $m->setAccessible(true); + return $m->invoke($service, $effectiveRegister); + }//end verify() + + + /** + * When every schema slug resolves and both tracked object types + * resolve to a schema id, verification reports ok with empty misses. + * + * @return void + */ + public function testAllSchemasAndObjectTypesResolveReportsOk(): void + { + $schemaMapper = $this->createMock(SchemaMapper::class); + $schemaMapper->method('findBySlug')->willReturn([new \stdClass()]); + + [$service] = $this->makeServiceForVerification($schemaMapper, resolvedObjectType: 42); + + $result = $this->verify( + $service, + ['components' => ['schemas' => ['module' => [], 'gebruik' => []]]] + ); + + $this->assertTrue($result['ok']); + $this->assertSame([], $result['missingSchemas']); + $this->assertSame([], $result['unresolvedObjectTypes']); + }//end testAllSchemasAndObjectTypesResolveReportsOk() + + + /** + * A schema slug present in the effective register that does not + * resolve in OpenRegister is reported as a missing schema and a + * WARNING is logged — this is the exact "eight merged features were + * dead on an upgraded instance" symptom made visible. + * + * @return void + */ + public function testUnresolvedSchemaSlugIsReportedAndLogged(): void + { + $schemaMapper = $this->createMock(SchemaMapper::class); + $schemaMapper->method('findBySlug')->willReturn([]); + + [$service, $logger] = $this->makeServiceForVerification($schemaMapper, resolvedObjectType: 42); + + $logger->expects($this->atLeastOnce())->method('warning'); + + $result = $this->verify( + $service, + ['components' => ['schemas' => ['bioMaatregel' => []]]] + ); + + $this->assertFalse($result['ok']); + $this->assertSame(['bioMaatregel'], $result['missingSchemas']); + }//end testUnresolvedSchemaSlugIsReportedAndLogged() + + + /** + * A tracked object type (organization/contactpersoon) that fails to + * resolve to a schema id after import is reported as unresolved. + * + * @return void + */ + public function testUnresolvedObjectTypeIsReported(): void + { + $schemaMapper = $this->createMock(SchemaMapper::class); + $schemaMapper->method('findBySlug')->willReturn([new \stdClass()]); + + [$service] = $this->makeServiceForVerification($schemaMapper, resolvedObjectType: null); + + $result = $this->verify($service, ['components' => ['schemas' => ['module' => []]]]); + + $this->assertFalse($result['ok']); + $this->assertSame(['organization', 'contactpersoon'], $result['unresolvedObjectTypes']); + }//end testUnresolvedObjectTypeIsReported() + + + /** + * An effective register with no schemas at all is a no-op verification + * that reports ok (nothing to check), not a false failure. + * + * @return void + */ + public function testEmptySchemasReportsOkWithoutTouchingSchemaMapper(): void + { + $schemaMapper = $this->createMock(SchemaMapper::class); + $schemaMapper->expects($this->never())->method('findBySlug'); + + [$service] = $this->makeServiceForVerification($schemaMapper, resolvedObjectType: 42); + + $result = $this->verify($service, ['components' => ['schemas' => []]]); + + $this->assertTrue($result['ok']); + }//end testEmptySchemasReportsOkWithoutTouchingSchemaMapper() + + + /** + * getRegisterVerificationStatus() with nothing persisted yet reports + * an "unchecked" status rather than a false "ok" that looks the same + * as a verified-clean import. + * + * @return void + */ + public function testGetRegisterVerificationStatusReportsUncheckedWhenNothingPersisted(): void + { + $config = $this->createMock(IAppConfig::class); + $config->method('getValueString')->willReturn(''); + + $service = new SettingsService( + $config, + $this->createMock(IRequest::class), + $this->createMock(ContainerInterface::class), + $this->createMock(IAppManager::class), + $this->createMock(LoggerInterface::class), + $this->createMock(IGroupManager::class), + $this->createMock(IL10N::class) + ); + + $reflection = new ReflectionMethod($service, 'getRegisterVerificationStatus'); + $reflection->setAccessible(true); + $status = $reflection->invoke($service); + + $this->assertTrue($status['ok']); + $this->assertFalse($status['checked']); + $this->assertNull($status['message']); + }//end testGetRegisterVerificationStatusReportsUncheckedWhenNothingPersisted() + + + /** + * getRegisterVerificationStatus() surfaces a persisted mismatch with a + * translated message, so a no-op import is visible in the settings + * status payload rather than looking identical to full success. + * + * @return void + */ + public function testGetRegisterVerificationStatusSurfacesPersistedMismatch(): void + { + $persisted = json_encode( + [ + 'ok' => false, + 'missingSchemas' => ['bioMaatregel'], + 'unresolvedObjectTypes' => [], + ] + ); + + $config = $this->createMock(IAppConfig::class); + $config->method('getValueString')->willReturn($persisted); + + $l10n = $this->createMock(IL10N::class); + $l10n->method('t')->willReturnCallback(static fn (string $text) => $text); + + $service = new SettingsService( + $config, + $this->createMock(IRequest::class), + $this->createMock(ContainerInterface::class), + $this->createMock(IAppManager::class), + $this->createMock(LoggerInterface::class), + $this->createMock(IGroupManager::class), + $l10n + ); + + $reflection = new ReflectionMethod($service, 'getRegisterVerificationStatus'); + $reflection->setAccessible(true); + $status = $reflection->invoke($service); + + $this->assertFalse($status['ok']); + $this->assertTrue($status['checked']); + $this->assertSame(['bioMaatregel'], $status['missingSchemas']); + $this->assertNotNull($status['message']); + }//end testGetRegisterVerificationStatusSurfacesPersistedMismatch() + + + /** + * shouldLoadSettings() always returns true (register-import-reliability): + * comparing this app's own semver against the register-content version + * ConfigurationService::getConfiguredAppVersion() returns is comparing + * two unrelated versioning schemes and could permanently block + * loadSettings() from ever running again. The only correct gate is + * importFromApp()'s own content-derived version comparison. + * + * @return void + */ + public function testShouldLoadSettingsAlwaysReturnsTrue(): void + { + $service = $this->getMockBuilder(SettingsService::class) + ->disableOriginalConstructor() + ->getMock(); + + $reflection = new ReflectionMethod($service, 'shouldLoadSettings'); + $reflection->setAccessible(true); + + $this->assertTrue($reflection->invoke($service)); + }//end testShouldLoadSettingsAlwaysReturnsTrue() +}//end class diff --git a/tests/Unit/Service/SettingsServiceResolverWiringTest.php b/tests/Unit/Service/SettingsServiceResolverWiringTest.php index 402c1b48..45c9bf35 100644 --- a/tests/Unit/Service/SettingsServiceResolverWiringTest.php +++ b/tests/Unit/Service/SettingsServiceResolverWiringTest.php @@ -29,6 +29,7 @@ use OCP\App\IAppManager; use OCP\IAppConfig; use OCP\IGroupManager; +use OCP\IL10N; use OCP\IRequest; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -67,6 +68,9 @@ class SettingsServiceResolverWiringTest extends TestCase /** @var IGroupManager|MockObject */ private IGroupManager|MockObject $groupManager; + /** @var IL10N|MockObject */ + private IL10N|MockObject $l10n; + /** * Build collaborator mocks shared across cases. @@ -82,6 +86,7 @@ protected function setUp(): void $this->appManager = $this->createMock(IAppManager::class); $this->logger = $this->createMock(LoggerInterface::class); $this->groupManager = $this->createMock(IGroupManager::class); + $this->l10n = $this->createMock(IL10N::class); }//end setUp() @@ -100,6 +105,7 @@ private function makeService(): SettingsService $this->appManager, $this->logger, $this->groupManager, + $this->l10n, ); }//end makeService() From e7ea4e9ae66f845f7bec36110bfe8408852ee72f Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Fri, 24 Jul 2026 14:44:12 +0200 Subject: [PATCH 2/3] docs(settings): document register delivery path + i18n for verification warning Add a CONFIGURATION.md section explaining how register/schema changes reach an installed instance: the ADR-037 fragment-file preference, and the content-hash mechanism that now forces a re-import on any monolith edit. Includes guidance for diagnosing a no-op import via the new registerVerification status field. Add Dutch and English l10n entries for the new user-facing register-verification warning message surfaced through getConfigurationStatus(). --- docs/CONFIGURATION.md | 16 ++++++++++++++++ l10n/en_US.js | 3 ++- l10n/en_US.json | 3 ++- l10n/nl.js | 3 ++- l10n/nl.json | 3 ++- 5 files changed, 24 insertions(+), 4 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index a61646de..e1351bba 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -84,6 +84,22 @@ After configuration: 2. Verify schema IDs are correctly mapped 3. Test with a sample object creation +## How Register and Schema Changes Reach an Installed Instance + +The app's register/schema definitions live in `lib/Settings/softwarecatalogus_register.json` (the "monolith"). On install and upgrade, the `InitializeSettings` repair step calls `SettingsService::loadSettings()`, which reads that file, merges any fragment files (see below), computes a version string, and hands both to OpenRegister's `ConfigurationService::importFromApp()`. OpenRegister only actually writes registers/schemas/objects when that version string is **newer** than the one already stored for this app — so the version string is the single thing that decides whether your register/schema change ever reaches a running instance. + +### Preferred: drop an ADR-037 fragment file + +Add your change as its own file under `lib/Settings/register.d/.json` instead of editing the monolith directly (see `lib/Settings/register.d/README.md`). Fragments are OpenAPI `components.schemas` / `paths` objects that get deep-merged onto the monolith at load time. Because each change owns a disjoint file, concurrent builds never conflict, and there is no need to remember to bump anything by hand — every fragment's own content is automatically folded into the import version. + +### If you must edit the monolith directly + +You can still edit `softwarecatalogus_register.json` directly (several changes have). As of the `register-import-reliability` fix, `loadSettings()` folds an md5 hash of the monolith file's own raw content into the computed version (`+base.`), alongside the existing fragment-file hash (`+frag.`). This means **any** change to the monolith — not just a fragment addition — now produces a version OpenRegister has not seen before and triggers a re-import automatically. You no longer need to remember to bump `info.version` by hand for the change to reach an instance, though doing so is still good practice for human-readable changelogs. + +### If an import looks successful but nothing changed + +After every import attempt, `loadSettings()` verifies that every schema slug declared in the effective (monolith + fragments) register actually resolves in OpenRegister, and that the schema ids this app tracks for its own object types (`organization`, `contactpersoon`) are non-null. Any mismatch is logged as a WARNING and recorded — check the settings status payload's `registerVerification` field (`GET /api/settings/status` equivalent, surfaced via `SettingsService::getConfigurationStatus()`) for `ok: false` and a `message` explaining that the most recent import did not fully reach OpenRegister. If you see this, re-run the import (`POST /api/settings/import {"force": true}`) and check the server log for the `SettingsService: register verification found...` warning lines naming the specific schema slugs or object types involved. + ## Object Schema Requirements ### Contactgegevens Object diff --git a/l10n/en_US.js b/l10n/en_US.js index cad5b86d..18aede8a 100644 --- a/l10n/en_US.js +++ b/l10n/en_US.js @@ -347,7 +347,8 @@ OC.L10N.register( "BBN1" : "BBN1", "BBN2" : "BBN2", "BBN3" : "BBN3", - "Without DPIA (BBN2+)" : "Without DPIA (BBN2+)" + "Without DPIA (BBN2+)" : "Without DPIA (BBN2+)", + "The most recent register import did not fully reach OpenRegister — some schemas or object types could not be verified. Re-run the import or check the server log for details." : "The most recent register import did not fully reach OpenRegister — some schemas or object types could not be verified. Re-run the import or check the server log for details." }, "nplurals=2; plural=(n != 1);" ); diff --git a/l10n/en_US.json b/l10n/en_US.json index 8a8f080b..944cf67e 100644 --- a/l10n/en_US.json +++ b/l10n/en_US.json @@ -406,6 +406,7 @@ "BBN1": "BBN1", "BBN2": "BBN2", "BBN3": "BBN3", - "Without DPIA (BBN2+)": "Without DPIA (BBN2+)" + "Without DPIA (BBN2+)": "Without DPIA (BBN2+)", + "The most recent register import did not fully reach OpenRegister — some schemas or object types could not be verified. Re-run the import or check the server log for details.": "The most recent register import did not fully reach OpenRegister — some schemas or object types could not be verified. Re-run the import or check the server log for details." } } diff --git a/l10n/nl.js b/l10n/nl.js index bd83671a..445f73f1 100644 --- a/l10n/nl.js +++ b/l10n/nl.js @@ -384,7 +384,8 @@ OC.L10N.register( "BBN1" : "BBN1", "BBN2" : "BBN2", "BBN3" : "BBN3", - "Without DPIA (BBN2+)" : "Zonder DPIA (BBN2+)" + "Without DPIA (BBN2+)" : "Zonder DPIA (BBN2+)", + "The most recent register import did not fully reach OpenRegister — some schemas or object types could not be verified. Re-run the import or check the server log for details." : "De meest recente registerimport heeft OpenRegister niet volledig bereikt — sommige schema's of objecttypen konden niet worden geverifieerd. Voer de import opnieuw uit of controleer het serverlogboek voor details." }, "nplurals=2; plural=(n != 1);" ); diff --git a/l10n/nl.json b/l10n/nl.json index 3360d7f6..c06e5f6f 100644 --- a/l10n/nl.json +++ b/l10n/nl.json @@ -550,6 +550,7 @@ "BBN1": "BBN1", "BBN2": "BBN2", "BBN3": "BBN3", - "Without DPIA (BBN2+)": "Zonder DPIA (BBN2+)" + "Without DPIA (BBN2+)": "Zonder DPIA (BBN2+)", + "The most recent register import did not fully reach OpenRegister — some schemas or object types could not be verified. Re-run the import or check the server log for details.": "De meest recente registerimport heeft OpenRegister niet volledig bereikt — sommige schema's of objecttypen konden niet worden geverifieerd. Voer de import opnieuw uit of controleer het serverlogboek voor details." } } From 79ea681f937111b1ba2b5c9e4741b22182723cf0 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Fri, 24 Jul 2026 14:44:18 +0200 Subject: [PATCH 3/3] docs(openspec): archive register-import-reliability change Merges the settings-service spec deltas (content-derived version signature, register-verification status surface, and the call-site-determinism requirement for the OpenRegister configuration row) into the canonical spec and archives the change artifacts. --- .../.openspec.yaml | 2 + .../context-brief.md | 46 +++++++++++ .../design.md | 69 ++++++++++++++++ .../proposal.md | 59 +++++++++++++ .../specs/settings-service/spec.md | 82 +++++++++++++++++++ .../tasks.md | 77 +++++++++++++++++ openspec/specs/settings-service/spec.md | 58 ++++++++++++- 7 files changed, 390 insertions(+), 3 deletions(-) create mode 100644 openspec/changes/archive/2026-07-24-register-import-reliability/.openspec.yaml create mode 100644 openspec/changes/archive/2026-07-24-register-import-reliability/context-brief.md create mode 100644 openspec/changes/archive/2026-07-24-register-import-reliability/design.md create mode 100644 openspec/changes/archive/2026-07-24-register-import-reliability/proposal.md create mode 100644 openspec/changes/archive/2026-07-24-register-import-reliability/specs/settings-service/spec.md create mode 100644 openspec/changes/archive/2026-07-24-register-import-reliability/tasks.md diff --git a/openspec/changes/archive/2026-07-24-register-import-reliability/.openspec.yaml b/openspec/changes/archive/2026-07-24-register-import-reliability/.openspec.yaml new file mode 100644 index 00000000..f4dd94eb --- /dev/null +++ b/openspec/changes/archive/2026-07-24-register-import-reliability/.openspec.yaml @@ -0,0 +1,2 @@ +schema: conduction +created: 2026-07-24 diff --git a/openspec/changes/archive/2026-07-24-register-import-reliability/context-brief.md b/openspec/changes/archive/2026-07-24-register-import-reliability/context-brief.md new file mode 100644 index 00000000..00ecc069 --- /dev/null +++ b/openspec/changes/archive/2026-07-24-register-import-reliability/context-brief.md @@ -0,0 +1,46 @@ +# Context Brief: register-import-reliability + +## What +Make register/schema changes **actually reach an installed instance**, and make a failure to do so **loud instead of silent**. Closes softwarecatalog#391. + +## Why (evidence — reproduced live 2026-07-24 on NC 34 / :8080) +Deployed the `development` head with all 8 market-gap changes, bumped `appinfo/info.xml` 0.2.17→0.2.18, ran `occ upgrade`. Upgrade reported **success** and ran `InitializeSettings`. Yet in OpenRegister: +- `bioMaatregel` / `sbomComponent` schemas — **not created** +- `module` — missing all 7 new properties (`bbnLevel`, `dpiaStatus`, `dpiaDate`, `dpiaVolgendeBeoordeling`, `dpiaDocumentRef`, `verwerkingsregisterRef`, `eolProductSlug`) +- `gebruik` — missing all 3 TIME fields (`timeClassification`, `timeRationale`, `timeReviewDate`) + +Eight merged features were **dead on an upgraded instance while green in CI**. Only `POST /api/settings/import {"force": true}` applied them. + +## Root cause (confirmed by reading the code) +`SettingsService::loadSettings()` (~lines 1462-1550) computes the import version as: +``` +$configVersion = $softwareCatalogSettings['info']['version'] // register JSON's OWN info.version + . ('+frag.' . substr(md5($fragmentSig), 0, 8)) // md5 of Settings/register.d/*.json only +``` +and passes it to `ConfigurationService::importFromApp(..., version: $configVersion, force: $force)`, which is version-gated. + +**The monolith's own content is NOT part of that signature.** Per ADR-037 each change should drop a `register.d/.json` fragment, but all 8 wave changes edited the monolith `lib/Settings/softwarecatalogus_register.json` directly — so unless a human also remembers to bump `info.version`, the computed version is byte-identical and the import is a **silent no-op**. (An independent session hit exactly this on opencatalogi.) + +Secondary: `oc_openregister_configurations` holds **three** rows titled `Software Catalog Register` (ids 7 and 117 at `2.3.1+frag.9003c029`, id 81 at `2.3.0`), which may also confuse the gate — on this instance the versions DID differ (JSON was `2.4.0`) yet nothing imported and no import log line appeared, so the duplicate rows are a live suspect. + +## Scope +IN: +1. **Fold the monolith into the signature** — include a hash of `softwarecatalogus_register.json`'s own content in `$configVersion` (e.g. `+base.`), so ANY register change (monolith or fragment) produces a new version and re-imports. This fixes the defect class permanently rather than relying on humans bumping `info.version`. +2. **Investigate + handle the duplicate configuration rows** — determine how three rows for one app arose, make `importFromApp` resolution deterministic from this app's side (and/or de-duplicate), and document the finding. If the dedupe must happen in OpenRegister, file an issue there instead of hacking around it here. +3. **Make silence impossible** — after import, verify the live schema set matches the shipped register (every schema slug in the effective register exists in OpenRegister, and configured schema ids resolve). On mismatch: log a WARNING and surface it in the admin settings status payload. A no-op import must never look like success. +4. **Regression test** — a test that would have caught this: assert the computed `$configVersion` CHANGES when the monolith content changes (not just when a fragment changes). +5. Docs note in `docs/` on how register changes reach an instance + the ADR-037 fragment preference. + +OUT: rewriting OpenRegister's `ConfigurationService`; migrating the 8 already-merged wave schemas into fragments (they are already live via the forced import — a separate cleanup if wanted); any UI beyond the status/warning surface. + +## Current state (read first) +- `lib/Service/SettingsService.php` — `loadSettings()` (signature computation + `importFromApp` call), `initialize()`, `getVoorzieningenConfig()`, `normalizeVoorzieningenConfig()`. +- `lib/Repair/InitializeSettings.php` — early-returns when `last_initialized_version === $currentAppVersion`; calls `SettingsService::initialize()`. +- `lib/Settings/register.d/README.md` — the ADR-037 fragment contract. +- `openspec/specs/settings-service/spec.md`, `openspec/specs/repair-init/spec.md` — the specs this change extends. + +## Design constraints +- ADR-001: no custom tables. ADR-008 Controller→Service. ADR-009 tests. ADR-005 i18n for any new user-facing string. +- **Do not weaken the version gate into "always re-import"** — importing on every request would be a performance regression. The signature must be content-derived and cheap (md5 of an already-read string). +- Spec deltas MUST use `### Requirement: ` headers, and the MUST/SHALL must be on the requirement's **first physical line** (validator only reads line 1). Avoid angle brackets in requirement bodies (validator false-positives). +- `@spec` anchors must point at the CANONICAL `openspec/specs//spec.md#requirement-` — never `openspec/changes/...`, because `openspec archive` moves the change dir and breaks those anchors. diff --git a/openspec/changes/archive/2026-07-24-register-import-reliability/design.md b/openspec/changes/archive/2026-07-24-register-import-reliability/design.md new file mode 100644 index 00000000..5954bf55 --- /dev/null +++ b/openspec/changes/archive/2026-07-24-register-import-reliability/design.md @@ -0,0 +1,69 @@ +# Design: register-import-reliability + +## Architecture Overview +`SettingsService::loadSettings()` reads the monolith `lib/Settings/softwarecatalogus_register.json`, deep-merges any `lib/Settings/register.d/*.json` fragments on top, computes a `$configVersion` string, and calls OpenRegister's `ConfigurationService::importFromApp(appId, data, version, force)`. `importFromApp` is version-gated: it skips the import when the passed `version` matches the version already stored on the `Configuration` entity for that app. Today `$configVersion` is derived from `info.version` (the monolith's self-declared version field) plus a hash of the fragment files only — never from the monolith body itself. This design closes that gap by making the signature fully content-derived, adds a post-import verification pass, and documents the duplicate-configuration-row investigation and its resolution. + +``` +loadSettings() + ├─ read softwarecatalogus_register.json (monolith) + ├─ merge register.d/*.json fragments (ADR-037) + ├─ compute $configVersion = info.version + │ + '+base.' + md5(monolith raw content)[0:8] <- NEW + │ + '+frag.' + md5(fragmentSig)[0:8] <- existing + ├─ importFromApp(appId, mergedData, $configVersion, $force) + └─ verifyRegisterAgainstEffectiveConfig(mergedData) <- NEW + ├─ every schema slug in mergedData.components.schemas exists in OpenRegister + ├─ every configured schema id (getSchemaIdForObjectType) resolves + └─ on mismatch: log WARNING + attach to $results['registerVerification'] +``` + +## Goals / Non-Goals +**Goals:** +- Any edit to the monolith OR a fragment produces a different `$configVersion`, guaranteeing `importFromApp` never silently no-ops on real content changes. +- A no-op or partially-applied import becomes observable: a WARNING in the log and a field in the settings status payload that an admin (or a future automated check) can act on. +- Understand and resolve (or escalate) the duplicate `Software Catalog Register` configuration rows. +- Keep `loadSettings()` cheap — it already reads the monolith and fragment files into memory; the new hash reuses that same string, no extra I/O. + +**Non-Goals:** +- Do not weaken the version gate into "always re-import" — that would be a performance regression on every repair-step run. +- Do not patch OpenRegister's `ConfigurationService` in this repo. +- Do not migrate the 8 already-merged wave schemas into fragment files. +- Do not build new admin UI; the warning surfaces through the existing settings status payload/response only. + +## Decisions + +### Decision 1: Fold a monolith content hash into `$configVersion` as `+base.` +`$configVersion` becomes `info.version . '+base.' . substr(md5($softwareCatalogContent), 0, 8) . ('+frag.' . substr(md5($fragmentSig), 0, 8) if fragments exist)`. `$softwareCatalogContent` is the raw file string already read at the top of `loadSettings()` (`file_get_contents($softwareCatalogPath)`), so this costs one extra `md5()` call on a string already in memory — negligible, and it only runs during the repair step (install/upgrade), not per-request. + +**Alternative considered:** hash the parsed+merged `$softwareCatalogSettings` array (post-merge) instead of the raw monolith string. Rejected: hashing an array requires stable serialization (`json_encode` with fixed key order) to be deterministic across PHP versions/opcache runs, and conflates "monolith changed" with "merge result changed" — the raw string hash is simpler, deterministic by construction, and keeps the `+base`/`+frag` split legible for debugging (an admin can see from the version string alone whether the base file or a fragment changed). + +### Decision 2: Post-import verification walks the *effective* merged register, not a hardcoded expectation +After `importFromApp` returns (success path), iterate `$softwareCatalogSettings['components']['schemas']` (post-merge, i.e. monolith + all fragments — the same data structure just imported) and check each schema's slug exists in OpenRegister via `getSchemaIdForObjectType()` / the register/schema service already injected into `SettingsService`. Separately, for the object types this app hardcodes lookups for, confirm the resolved schema id is non-null. Record misses into `$results['registerVerification'] = ['ok' => bool, 'missingSchemas' => [...], 'unresolvedObjectTypes' => [...]]`, log one WARNING per miss, and let `initialize()` propagate `registerVerification` into its own `$results` (already has an `errors`/`warnings` array pattern used elsewhere in the file) so `InitializeSettings::run()` surfaces it via `$output->warning()` exactly like other partial-failure paths already do. + +**Alternative considered:** compare against a static list of "expected" schemas maintained by hand. Rejected: it drifts (this defect *is* about drift) and duplicates information already present in the register JSON itself — verifying against the effective merged register is self-updating as fragments/monolith evolve. + +### Decision 3: Duplicate configuration rows — investigate via read-only query, resolve app-side if safely attributable, else file upstream issue +Read `oc_openregister_configurations` (via OpenRegister's existing configuration lookup service, not a raw custom query) filtered by this app's id/title to characterize the three rows (ids 7, 117, 81 observed live). If the duplication is explained by this app's own call pattern (e.g. `importFromApp` being invoked with an app-identifying key that isn't stable, or multiple call sites creating rows independently), fix the call site so this app always resolves/updates the same row deterministically. If the duplication instead stems from `ConfigurationService`'s own row-matching logic (e.g. matching on title rather than a stable appId+slug key), do not patch OpenRegister from this worktree — file `gh issue create -R ConductionNL/openregister` describing the reproduction (three rows for one app, two different version strings) and reference the issue number in this app's code comment and in `docs/`. + +**Alternative considered:** directly `DELETE` the stale rows via a repair step. Rejected as a first move — ADR-001/safety: deleting configuration rows without confirming which one is authoritative risks orphaning whatever OpenRegister-side state points at a given row id; investigate and attribute before any destructive action, per the task's explicit instruction and the workspace's "never range-delete" safety rule. + +**Outcome:** code review of `OCA\OpenRegister\Service\Configuration\ImportHandler::importFromApp()`, `ConfigurationMapper::findByApp()`/`findBySourceUrl()`, and `MultiTenancyTrait::applyOrganisationFilter()` shows both lookup paths organisation-scope their query (`allowNullOrg: false` by default), so an app-owned configuration row can be invisible to a caller whose active-organisation context differs from the row's — `importFromApp()` then concludes "no existing configuration found" and creates a duplicate. This is not attributable to a defect in this app's own call site (there is only one call site, `SettingsService::loadSettings()` → `getConfigurationService()->importFromApp()`, and it always passes the same `appId`); the true fix is upstream. Filed [openregister#2072](https://github.com/ConductionNL/openregister/issues/2072) with the full mechanism and a suggested fix (don't organisation-scope `is_local` app-owned rows, or add a DB-level uniqueness constraint on `app`). Live DB confirmation of the 3 existing rows' `organisation` column was blocked by the shared Postgres instance being in recovery mode at investigation time — the issue asks a maintainer with DB access to close that loop. + +### Decision 4: Remove `shouldLoadSettings()`'s broken app-semver-vs-content-version pre-gate +Code reading during design confirmed a second, independently sufficient cause of "no import log line appeared" in the live evidence: `initialize()` only calls `loadSettings()` when `shouldLoadSettings()` returns true, and that method runs `version_compare($currentAppVersion, $storedVersion, '>')` where `$currentAppVersion` is this app's own semver (`"0.2.17"`) and `$storedVersion` is `ConfigurationService::getConfiguredAppVersion($appId)` — which, per OpenRegister's `ImportHandler::importFromApp()`, stores exactly the `version` argument `loadSettings()` itself passed on the *previous* call, i.e. the register-content version (`"2.3.1+frag.9003c029"`). Verified: `version_compare("0.2.17", "2.3.1+frag.9003c029", ">")` returns `false`. Because register-content versions here start at `"2."` and the app's own semver is `"0.x"`, this comparison can never return true once any import has run — `shouldLoadSettings()` returns `false` forever afterward, and `loadSettings()` is never invoked again by any future upgrade. This makes Decision 1's fix (the content hash) permanently unreachable in exactly the scenario the live evidence describes. + +The fix: `shouldLoadSettings()` always returns `true`. The dead app-semver-vs-content-version comparison is removed and replaced with a docblock explaining why. `loadSettings()` is only ever reached from an explicit admin-triggered controller action or the install/upgrade repair step (`InitializeSettings::run()`, which has its own `last_initialized_version` gate against repeated runs within the same app version) — never from a per-request code path — so always attempting it is cheap: a couple of file reads plus `md5()` calls. The actual (potentially expensive) schema/register write remains gated by `importFromApp`'s own comparison of two like-for-like content versions, which Decision 1 makes correct. + +**Alternative considered:** keep an outer gate but fix its comparison to be like-for-like (compare freshly-computed `$configVersion` against `getConfiguredAppVersion()`). Rejected: that duplicates the exact comparison `importFromApp` already performs internally, doubling the places that must independently agree on "is this newer" semantics — a strict superset of Decision 1's fix with no added correctness, and more surface area to drift out of sync again in the future (which is precisely the defect class this change closes). + +## Risks / Trade-offs +- [Content-hash bump forces one re-import per instance on first deploy of this change] → Mitigation: intended; this is exactly the corrective effect needed for already-drifted instances, and `importFromApp` is idempotent per its existing contract (re-importing identical data is a safe no-op at the data level, just not at the version-gate level). +- [Verification adds a schema-resolution round trip after every real import] → Mitigation: it only runs on the (rare) path where the version-gate decided a re-import was needed, not on every request; cost is bounded by the number of schemas in the register (dozens, not thousands). +- [Duplicate-row root cause may turn out to be entirely OpenRegister-side, leaving three rows in place until upstream fixes it] → Mitigation: filing the issue plus documenting the deterministic resolution on this app's side (so we always know which row is "ours") is the correct scope boundary per the proposal; it doesn't block the primary defect fix (Decision 1) from shipping. +- [Removing `shouldLoadSettings()`'s gate makes `loadSettings()` run on every `initialize()` call instead of being skipped] → Mitigation: `initialize()` is already bounded to explicit admin action or the install/upgrade repair step (never per-request), and the work `loadSettings()` now always does when skipped previously — reading two small files and hashing them — is negligible next to the HTTP/DB round trips already in that path; the actual expensive write remains gated by `importFromApp`. + +## Migration Plan +No database schema changes (ADR-001 — no custom tables). Deployment is a normal app release: bump `info.xml` version, ship, `occ upgrade` runs the existing `InitializeSettings` repair step, which now computes a version signature that differs from any prior stored value (because the monolith content hash is new), forcing exactly one re-import that reconciles the instance to the current register state. Rollback is a `git revert` — reverting drops the `+base.` suffix, and the next repair run's version string again matches the last-imported value already recorded on the instance (no further action needed). + +## Open Questions +None outstanding — resolved during design: hash source (raw monolith string, not merged array), verification scope (effective merged register, not static list), and duplicate-row handling posture (investigate + deterministic resolution or upstream issue, never blind delete). diff --git a/openspec/changes/archive/2026-07-24-register-import-reliability/proposal.md b/openspec/changes/archive/2026-07-24-register-import-reliability/proposal.md new file mode 100644 index 00000000..94fc809c --- /dev/null +++ b/openspec/changes/archive/2026-07-24-register-import-reliability/proposal.md @@ -0,0 +1,59 @@ +# Proposal: register-import-reliability + +## Summary +`SettingsService::loadSettings()` computes the OpenRegister import version from `info.version` in `softwarecatalogus_register.json` plus a hash of the ADR-037 fragment files only — it never hashes the monolith's own body. When a change edits the monolith directly (as all 8 recent market-gap changes did) without also bumping `info.version`, the computed version is byte-identical to the last import and OpenRegister's version-gated `importFromApp` silently no-ops: schemas and properties that were merged and shipped never reach an installed instance, while CI and `occ upgrade` both report success. This change folds a content hash of the monolith into the version signature so any register edit forces a re-import, investigates and resolves the duplicate `Software Catalog Register` configuration rows that may also be confusing import resolution, and adds a post-import verification pass that turns a no-op import into a loud, visible warning instead of silence. + +## Motivation +Reproduced live on 2026-07-24 on NC 34 / :8080: after deploying `development` HEAD with all 8 merged market-gap changes and running `occ upgrade`, three schemas/property sets (`bioMaatregel`, `sbomComponent`, 7 `module` properties, 3 `gebruik` TIME fields) were absent from OpenRegister despite the upgrade reporting success. Only a manually-triggered `POST /api/settings/import {"force":true}` applied them. This is a silent data-loss-of-functionality defect: features that are "done" in every visible signal (merged PR, green CI, successful upgrade log) are dead on the instance that matters. It has now been observed independently on two apps (softwarecatalog here, opencatalogi in a separate session), indicating a defect class rather than a one-off mistake, and it must be fixed at the root (content-derived versioning) rather than by process discipline (remembering to bump `info.version`). + +## Affected Projects +- [x] Project: `softwarecatalog` — `SettingsService::loadSettings()` version signature, duplicate configuration row handling, post-import verification + status surface, regression test, docs note + +## Scope + +### In Scope +1. Fold an md5 of the monolith `softwarecatalogus_register.json` file content into the computed `$configVersion` (e.g. `+base.`) alongside the existing fragment signature, so any register change — monolith or fragment — produces a new version and triggers re-import. Also fix `SettingsService::shouldLoadSettings()`, whose comparison of the app's own semver against the register-content version stored by `importFromApp` is a confirmed apples-to-oranges defect that can permanently prevent `loadSettings()` from ever running again — making item 1's signature fix unreachable in the exact scenario reproduced live. +2. Investigate how three `Software Catalog Register` configuration rows (ids 7, 117, 81) arose in `oc_openregister_configurations`; make this app's resolution of "the" configuration deterministic; de-duplicate where safe from this app's side, or file an issue against OpenRegister's `ConfigurationService` if the true fix belongs there. **Filed:** [openregister#2072](https://github.com/ConductionNL/openregister/issues/2072) — code review of `ImportHandler::importFromApp()` found that `ConfigurationMapper::findByApp()`/`findBySourceUrl()` organisation-scope their lookup (`applyOrganisationFilter`, default `allowNullOrg: false`), so an app-owned configuration row can become invisible to a caller whose active-organisation context differs from the row's, causing "no existing configuration found, will create new one" and a duplicate row. Live DB confirmation of the 3 rows' `organisation` values was blocked by the shared Postgres instance being in recovery mode at investigation time; the issue documents the mechanism from code review and asks a maintainer with DB access to confirm. +3. Post-import verification: after `importFromApp` returns, confirm every schema slug in the effective (merged) register exists in OpenRegister and that every configured schema id resolves. On mismatch, log a WARNING and surface the mismatch in the settings status payload (`getConfigurationStatus()` / equivalent) so an admin can see a no-op import instead of it looking identical to success. +4. Regression test asserting the computed `$configVersion` changes when the monolith's own content changes (not only when a fragment changes) — the test that would have caught the original defect. +5. Docs note describing how register/schema changes reach an installed instance, and reiterating the ADR-037 fragment-file preference for future changes. + +### Out of Scope +- Rewriting or patching OpenRegister's `ConfigurationService` import/version-gate logic itself (file an issue there instead if a true fix belongs upstream). +- Migrating the 8 already-merged market-gap schema changes from the monolith into ADR-037 fragment files — they are already live via the forced import; a separate cleanup if desired later. +- Any admin UI beyond surfacing the mismatch/warning in the existing settings status payload (no new settings screens or widgets). + +## Approach +Compute a second content hash (`md5` of the raw monolith file contents, truncated to 8 hex chars) inside `loadSettings()`, append it to `$configVersion` as `+base.` before the existing `+frag.` suffix, so the full signature changes whenever the monolith OR any fragment changes. After `importFromApp` succeeds, walk the merged `components.schemas` keys from the effective register, resolve each against OpenRegister's schema list by slug, and record any misses; also resolve each schema id referenced in this app's own configuration (e.g. `getSchemaIdForObjectType`) and record unresolved ids. Persist a `registerVerification` block (ok/warnings/mismatched slugs) into the settings status result and log a WARNING per mismatch. Separately, query `oc_openregister_configurations` for rows matching this app's title/appId to characterize the duplicates, and either tighten `importFromApp`'s resolution to be appId+title deterministic from this app's call site, or (if the duplication is caused by OpenRegister-side logic) file a `ConductionNL/openregister` issue documenting the reproduction and reference it in the design doc and code comment. + +**Additional confirmed finding (code-reading, not just hypothesis):** `SettingsService::initialize()` only calls `loadSettings()` at all when its own private `shouldLoadSettings()` returns true, and that method compares this app's own semver (`appManager->getAppVersion()`, e.g. `"0.2.17"`) against `ConfigurationService::getConfiguredAppVersion($appId)` — which returns whatever value was last passed as the `version` argument to `importFromApp()`, i.e. the **register-content** version this same service computes (e.g. `"2.3.1+frag.9003c029"`). These are two unrelated versioning schemes on the same stored slot. `version_compare("0.2.17", "2.3.1+frag.9003c029", ">")` is `false` (verified), so once any import has ever stored a content version whose leading numeral is `>=` the app's own leading numeral (true here — content versions are `2.x`, the app is `0.x`), `shouldLoadSettings()` returns `false` **permanently**, and `loadSettings()` is never invoked again by any future upgrade, regardless of the fix in item 1. This is the exact, confirmed mechanism behind the live evidence's "versions DID differ ... yet nothing imported and no import log line appeared" — `loadSettings()`'s own "Attempting to import" log line never fires because the method is never entered. Fixing item 1 alone would ship a correct signature that is permanently unreachable. This change therefore also removes the broken pre-gate so `loadSettings()` always runs when `initialize()` runs (bounded to explicit admin action or the install/upgrade repair step — never per-request), leaving the now-correct, content-derived `importFromApp` version comparison as the sole and cheap gate on whether an actual import happens. + +## New Dependencies +None. + +## Impact +- `lib/Service/SettingsService.php` — `loadSettings()` version computation, new post-import verification method(s), status payload additions. +- `lib/Repair/InitializeSettings.php` — no signature change expected, but its logged/surfaced warnings will now include verification mismatches bubbling up from `initialize()` → `loadSettings()`. +- Settings status API/payload consumed by the admin settings screen (additive field only). +- `docs/` — new note on register-change delivery. +- Test suite — new/extended PHPUnit coverage in whatever `tests/` path already covers `SettingsService`. + +## Cross-Project Dependencies +Potentially OpenRegister (`ConductionNL/openregister`), if the duplicate-configuration-row root cause is confirmed to live in `ConfigurationService`'s resolution logic — handled by filing an issue there, not by patching OpenRegister in this worktree. + +## Risks + +### Risk 1: Verification false positives on legitimately-optional schemas +**Severity:** Medium — **Mitigation:** scope verification to schema slugs actually declared in the effective merged register (monolith + fragments), not to a hardcoded expected list, so the check tracks whatever the register currently claims to ship. + +### Risk 2: Duplicate-row cleanup accidentally deletes a row another app or process depends on +**Severity:** Medium — **Mitigation:** investigate and document first; only de-duplicate rows conclusively identified as this app's own stale/duplicate configuration entries, and prefer OpenRegister-side deterministic resolution (or an upstream issue) over destructive local cleanup. + +### Risk 3: Content-hash version bump causes a one-time re-import storm across many upgraded instances +**Severity:** Low — **Mitigation:** this is intended and desired (it is exactly how the fix corrects already-drifted instances); `importFromApp` remains idempotent per its existing contract, and the hash is only recomputed on repair-step runs (install/upgrade), not on every request. + +## Rollback Strategy +The change is additive to a single service method and a settings status payload field. Revert is a straightforward `git revert` of the commits on `wip/register-import-reliability`; no schema/data migrations are introduced, so no data rollback is needed. If the OpenRegister issue results in an upstream PR later, this app-side workaround can be removed independently once that lands. + +## Open Questions +None — root cause and remediation approach are already confirmed in `context-brief.md` from live reproduction. diff --git a/openspec/changes/archive/2026-07-24-register-import-reliability/specs/settings-service/spec.md b/openspec/changes/archive/2026-07-24-register-import-reliability/specs/settings-service/spec.md new file mode 100644 index 00000000..0a29ccc2 --- /dev/null +++ b/openspec/changes/archive/2026-07-24-register-import-reliability/specs/settings-service/spec.md @@ -0,0 +1,82 @@ +# settings-service Specification (delta) + +## MODIFIED Requirements + +### Requirement: The system SHALL run auto-configuration, import and configuration maintenance (REQ-003) + +`autoConfigure`, `autoConfigureAfterImport`, `configureOpenCatalogi`, `initialize`, `loadSettings`, `performConsolidatedAutoConfiguration`, `manualImport`, `forceUpdate`, `resetAutoConfiguration`, `compactToJsonConfiguration`, `cleanupOldConfiguration`, and `clearConfigurationCache` MUST create/repair the register-schema configuration in OpenRegister, import seed data, and maintain the cached configuration, returning a result summary. `loadSettings` MUST compute the import version passed to `importFromApp` from the content of both the monolith register file and any merged ADR-037 fragment files, so that a change to either one produces a different version and forces a re-import rather than being silently skipped by the version gate. `initialize` MUST NOT rely on any comparison between this app's own semantic version and the register-content version string stored by a previous `importFromApp` call to decide whether to invoke `loadSettings` at all — those are two unrelated versioning schemes on the same stored value, and comparing them can permanently prevent `loadSettings` from ever running again regardless of subsequent register changes. After a successful import, `loadSettings`/`initialize` MUST verify that every schema slug present in the effective (monolith + fragments) merged register resolves in OpenRegister, and that every schema id this app resolves via its own object-type lookups is non-null, recording any mismatch as a warning rather than allowing a partial or no-op import to be reported as full success. + +(Previously: the import version was derived only from the register JSON's own `info.version` field plus a hash of the fragment files — a monolith edit that did not also bump `info.version` produced a byte-identical version string and `importFromApp` silently skipped the import. `initialize` additionally gated entry into `loadSettings` on comparing this app's own semver against the stored register-content version, which could permanently block `loadSettings` from running again at all. No post-import verification existed.) + +#### Scenario: REQ-003 case 1 + +- WHEN `autoConfigure(force=true)` is called +- THEN the registers/schemas MUST be (re)configured and a result summary returned + +#### Scenario: REQ-003 case 2 + +- WHEN `clearConfigurationCache()` is called +- THEN the cached configuration MUST be invalidated + +#### Scenario: REQ-003 case 3 — monolith edit alone changes the computed version + +- GIVEN the monolith `softwarecatalogus_register.json` content changes but its `info.version` field and all `register.d/*.json` fragment files are unchanged +- WHEN `loadSettings()` computes the import version +- THEN the computed version string MUST differ from the version computed before the monolith content changed +- AND `importFromApp` MUST therefore be invoked with a version that is not already stored for this app, triggering a re-import + +#### Scenario: REQ-003 case 4 — no-op import surfaces a warning instead of silent success + +- GIVEN an import completes where a schema slug present in the effective merged register does not resolve in OpenRegister +- WHEN `loadSettings()` runs its post-import verification +- THEN a WARNING MUST be logged identifying the unresolved schema slug +- AND the mismatch MUST be included in the result payload returned by `loadSettings()`/`initialize()` + +#### Scenario: REQ-003 case 5 — a prior import never blocks re-attempting a later one + +- GIVEN a prior successful import stored a register-content version on the app's `Configuration` entity, and this app's own semantic version has since been bumped for an upgrade +- WHEN `initialize()` runs during that upgrade +- THEN `loadSettings()` MUST be invoked regardless of how the stored register-content version compares to the app's own semantic version +- AND the decision of whether an actual re-import occurs MUST come only from `importFromApp`'s comparison of the newly computed content-derived version against the stored one + +### Requirement: The system SHALL read and persist every configuration domain (REQ-002) + +The service MUST expose get/set (and focused get/update) pairs for voorzieningen, AMEF, email, ArchiMate, user-groups (generic/org-admin/super), cronjob, and catalog location, plus aggregate readers (`getSettings`, `getAllSettings`, `getConsolidatedConfiguration`, `getConfigurationStatus`, `isFullyConfigured`) and writers (`updateSettings`). Each MUST read from / write to app config and return the current or updated values. `getConfigurationStatus` MUST include the outcome of the most recent register-import verification (whether the live schema set matched the effective register, and which schema slugs or object-type lookups, if any, did not resolve) so a no-op or partial import is visible to an admin inspecting settings status rather than looking identical to a fully successful one. + +(Previously: `getConfigurationStatus` reported general configuration completeness only; it carried no information about whether the most recent register import had actually reached OpenRegister.) + +#### Scenario: REQ-002 case 1 + +- WHEN `setVoorzieningenConfig(config)` then `getVoorzieningenConfig()` is called +- THEN the persisted config MUST be returned + +#### Scenario: REQ-002 case 2 + +- WHEN `isFullyConfigured()` is called with all required config present +- THEN it MUST return true + +#### Scenario: REQ-002 case 3 — status payload surfaces a register verification mismatch + +- GIVEN the most recent `loadSettings()` run recorded a schema slug that failed to resolve in OpenRegister +- WHEN `getConfigurationStatus()` is called +- THEN the returned payload MUST include that mismatch so it is visible without inspecting server logs + +## ADDED Requirements + +### Requirement: The system SHALL call its own OpenRegister configuration import deterministically and account for any duplicate rows found (REQ-006) + +The service MUST call `importFromApp` from a single call site with the same, stable app-identifying key on every invocation, so this app's own code can never itself be the cause of multiple `Configuration` rows existing for it. Row-level resolution of "the" configuration for a given `appId` (matching an existing row versus creating a new one) is performed by OpenRegister's `ConfigurationService`, outside this app's control. Where duplicate configuration rows are found to already exist for this app, the service's documentation MUST record how those rows were characterized (root-cause analysis, not just their existence) and either resolve them from this app's side, if the app's own call site is conclusively the cause, or reference a filed upstream issue against the owning system when the true fix belongs there. + +#### Scenario: REQ-006 case 1 — this app's own call site is single and stable + +- GIVEN `loadSettings()` runs an import +- WHEN the call to `getConfigurationService()->importFromApp()` is inspected +- THEN it MUST always pass this app's own constant `Application::APP_ID` as the `appId` argument from the same single call site, so no duplication can originate from this app varying its own identity across calls + +#### Scenario: REQ-006 case 2 — duplicate rows are documented, not silently ignored + +- GIVEN more than one configuration row is found to already exist for this app's title/appId +- WHEN the duplication is investigated +- THEN the root-cause finding MUST be recorded (in code comments and docs) rather than left unexplained +- AND WHEN the true fix is determined to belong in OpenRegister rather than this app +- THEN an issue MUST be filed against the owning repository documenting the mechanism, and referenced from this app's code and docs diff --git a/openspec/changes/archive/2026-07-24-register-import-reliability/tasks.md b/openspec/changes/archive/2026-07-24-register-import-reliability/tasks.md new file mode 100644 index 00000000..6443a612 --- /dev/null +++ b/openspec/changes/archive/2026-07-24-register-import-reliability/tasks.md @@ -0,0 +1,77 @@ +# Tasks: register-import-reliability + +## Implementation Tasks + +### Task 1: Fold monolith content hash into the computed import version +- **spec_ref**: `openspec/changes/register-import-reliability/specs/settings-service/spec.md#requirement-the-system-shall-run-auto-configuration-import-and-configuration-maintenance-req-003` +- **files**: `lib/Service/SettingsService.php` +- **acceptance_criteria**: + - GIVEN the monolith `softwarecatalogus_register.json` content changes but `info.version` and all fragment files are unchanged WHEN `loadSettings()` computes `$configVersion` THEN the resulting string differs from the previously computed one (new `+base.` component) + - GIVEN neither the monolith nor any fragment changed WHEN `loadSettings()` runs twice THEN the computed `$configVersion` is identical both times (no spurious re-import) +- [x] Implement +- [x] Test + +### Task 2: Remove the broken app-semver-vs-content-version pre-gate in `initialize()` +- **spec_ref**: `openspec/changes/register-import-reliability/specs/settings-service/spec.md#requirement-the-system-shall-run-auto-configuration-import-and-configuration-maintenance-req-003` +- **files**: `lib/Service/SettingsService.php` +- **acceptance_criteria**: + - GIVEN a prior import stored a register-content version (e.g. `2.3.1+frag.9003c029`) and the app's own semantic version has since changed WHEN `initialize()` runs THEN `loadSettings()` is invoked regardless of how those two unrelated version strings compare + - GIVEN `shouldLoadSettings()` is called directly WHEN inspected THEN it no longer compares the app's own semver against the register-content version stored by `importFromApp` +- [x] Implement +- [x] Test + +### Task 3: Post-import verification of effective register against OpenRegister +- **spec_ref**: `openspec/changes/register-import-reliability/specs/settings-service/spec.md#requirement-the-system-shall-run-auto-configuration-import-and-configuration-maintenance-req-003` +- **files**: `lib/Service/SettingsService.php` +- **acceptance_criteria**: + - GIVEN an import completes and a schema slug in the merged effective register does not resolve in OpenRegister WHEN the verification pass runs THEN a WARNING is logged naming the schema slug + - GIVEN an import completes and every schema slug resolves WHEN the verification pass runs THEN no warning is logged and the result records success +- [x] Implement +- [x] Test + +### Task 4: Surface register verification result in the settings status payload +- **spec_ref**: `openspec/changes/register-import-reliability/specs/settings-service/spec.md#requirement-the-system-shall-read-and-persist-every-configuration-domain-req-002` +- **files**: `lib/Service/SettingsService.php` +- **acceptance_criteria**: + - GIVEN the most recent import recorded a verification mismatch WHEN `getConfigurationStatus()` is called THEN the returned payload includes that mismatch +- [x] Implement +- [x] Test + +### Task 5: Investigate duplicate `Software Catalog Register` configuration rows and account for the finding +- **spec_ref**: `openspec/changes/register-import-reliability/specs/settings-service/spec.md#requirement-the-system-shall-call-its-own-openregister-configuration-import-deterministically-and-account-for-any-duplicate-rows-found-req-006` +- **files**: `lib/Service/SettingsService.php`, `docs/` +- **acceptance_criteria**: + - GIVEN this app's single `importFromApp` call site WHEN inspected THEN it always passes the same constant `Application::APP_ID`, ruling out this app as the source of duplicate rows + - GIVEN the duplication mechanism (root cause, not just row ids) WHEN it is characterized by code review of the owning system THEN the finding is documented; if attributable to this app's call site the fix ships here, otherwise a `ConductionNL/openregister` issue is filed and referenced in code/docs +- [x] Implement +- [x] Test + +### Task 6: Regression test — configVersion changes on monolith-only edits +- **spec_ref**: `openspec/changes/register-import-reliability/specs/settings-service/spec.md#requirement-the-system-shall-run-auto-configuration-import-and-configuration-maintenance-req-003` +- **files**: `tests/Unit/Service/SettingsServiceConfigVersionTest.php`, `tests/Unit/Service/SettingsServiceRegisterVerificationTest.php`, `tests/Stubs/Db/SchemaMapper.php` +- **acceptance_criteria**: + - GIVEN a test fixture where only the monolith register file content is mutated between two `loadSettings()` calls WHEN the computed version is compared THEN the test asserts the versions differ (this is the test that would have caught the original defect — a fragment-only signature keeps the version unchanged on a monolith edit) +- [x] Implement +- [x] Test + +### Task 7: Docs note + i18n for the register-verification warning +- **spec_ref**: `openspec/changes/register-import-reliability/specs/settings-service/spec.md#requirement-the-system-shall-read-and-persist-every-configuration-domain-req-002` +- **files**: `docs/`, `l10n/nl.js`, `l10n/nl.json`, `l10n/en_US.js`, `l10n/en_US.json` +- **acceptance_criteria**: + - GIVEN a developer reads `docs/` WHEN looking for how register/schema changes reach an installed instance THEN they find the ADR-037 fragment-file preference and the content-hash re-import mechanism explained + - GIVEN the verification warning message is user-facing WHEN it is rendered in the settings status payload THEN its English source string has matching Dutch and English translation entries +- [x] Implement +- [x] Test + +## Quality checklist + + + +- All new/changed business logic covered by PHPUnit unit tests (`tests/Unit/`) +- New/changed API endpoints covered by Newman/Postman tests (none new — status payload field is additive to an existing endpoint) +- UI changes covered by Playwright browser tests (none — no new UI surface, only an existing status payload field) +- All tests pass (`vendor/bin/phpunit -c phpunit-unit.xml`) +- Feature documentation updated in `docs/` (ADR-010) +- Dutch (`nl_NL`) and English (`en_US`) translation strings added for the new warning message (ADR-005) +- `openspec validate` passes diff --git a/openspec/specs/settings-service/spec.md b/openspec/specs/settings-service/spec.md index e4cd6411..73db2931 100644 --- a/openspec/specs/settings-service/spec.md +++ b/openspec/specs/settings-service/spec.md @@ -8,7 +8,6 @@ status: done Provides the backend service that detects OpenRegister availability and resolves its service handles and register-schema ids, then reads and persists every configuration domain (voorzieningen, AMEF, email, ArchiMate, user-groups, cronjob, catalog location). It runs auto-configuration, seed import, and configuration maintenance, manages email settings, templates, and connectivity tests, and handles user groups, ArchiMate operation status, statistics, and organisation sync. @e2e exclude PHP SettingsService backend (config persistence, validation, OpenRegister/email/group resolution) — no UI surface; covered by PHPUnit service tests and Newman REST collections. - ## Requirements ### Requirement: The system SHALL detect and resolve OpenRegister availability and services (REQ-001) @@ -24,28 +23,63 @@ Provides the backend service that detects OpenRegister availability and resolves ### Requirement: The system SHALL read and persist every configuration domain (REQ-002) -The service MUST expose get/set (and focused get/update) pairs for voorzieningen, AMEF, email, ArchiMate, user-groups (generic/org-admin/super), cronjob, and catalog location, plus aggregate readers (`getSettings`, `getAllSettings`, `getConsolidatedConfiguration`, `getConfigurationStatus`, `isFullyConfigured`) and writers (`updateSettings`). Each MUST read from / write to app config and return the current or updated values. +The service MUST expose get/set (and focused get/update) pairs for voorzieningen, AMEF, email, ArchiMate, user-groups (generic/org-admin/super), cronjob, and catalog location, plus aggregate readers (`getSettings`, `getAllSettings`, `getConsolidatedConfiguration`, `getConfigurationStatus`, `isFullyConfigured`) and writers (`updateSettings`). Each MUST read from / write to app config and return the current or updated values. `getConfigurationStatus` MUST include the outcome of the most recent register-import verification (whether the live schema set matched the effective register, and which schema slugs or object-type lookups, if any, did not resolve) so a no-op or partial import is visible to an admin inspecting settings status rather than looking identical to a fully successful one. + +(Previously: `getConfigurationStatus` reported general configuration completeness only; it carried no information about whether the most recent register import had actually reached OpenRegister.) #### Scenario: REQ-002 case 1 + - WHEN `setVoorzieningenConfig(config)` then `getVoorzieningenConfig()` is called - THEN the persisted config MUST be returned #### Scenario: REQ-002 case 2 + - WHEN `isFullyConfigured()` is called with all required config present - THEN it MUST return true +#### Scenario: REQ-002 case 3 — status payload surfaces a register verification mismatch + +- GIVEN the most recent `loadSettings()` run recorded a schema slug that failed to resolve in OpenRegister +- WHEN `getConfigurationStatus()` is called +- THEN the returned payload MUST include that mismatch so it is visible without inspecting server logs + ### Requirement: The system SHALL run auto-configuration, import and configuration maintenance (REQ-003) -`autoConfigure`, `autoConfigureAfterImport`, `configureOpenCatalogi`, `initialize`, `loadSettings`, `performConsolidatedAutoConfiguration`, `manualImport`, `forceUpdate`, `resetAutoConfiguration`, `compactToJsonConfiguration`, `cleanupOldConfiguration`, and `clearConfigurationCache` MUST create/repair the register-schema configuration in OpenRegister, import seed data, and maintain the cached configuration, returning a result summary. +`autoConfigure`, `autoConfigureAfterImport`, `configureOpenCatalogi`, `initialize`, `loadSettings`, `performConsolidatedAutoConfiguration`, `manualImport`, `forceUpdate`, `resetAutoConfiguration`, `compactToJsonConfiguration`, `cleanupOldConfiguration`, and `clearConfigurationCache` MUST create/repair the register-schema configuration in OpenRegister, import seed data, and maintain the cached configuration, returning a result summary. `loadSettings` MUST compute the import version passed to `importFromApp` from the content of both the monolith register file and any merged ADR-037 fragment files, so that a change to either one produces a different version and forces a re-import rather than being silently skipped by the version gate. `initialize` MUST NOT rely on any comparison between this app's own semantic version and the register-content version string stored by a previous `importFromApp` call to decide whether to invoke `loadSettings` at all — those are two unrelated versioning schemes on the same stored value, and comparing them can permanently prevent `loadSettings` from ever running again regardless of subsequent register changes. After a successful import, `loadSettings`/`initialize` MUST verify that every schema slug present in the effective (monolith + fragments) merged register resolves in OpenRegister, and that every schema id this app resolves via its own object-type lookups is non-null, recording any mismatch as a warning rather than allowing a partial or no-op import to be reported as full success. + +(Previously: the import version was derived only from the register JSON's own `info.version` field plus a hash of the fragment files — a monolith edit that did not also bump `info.version` produced a byte-identical version string and `importFromApp` silently skipped the import. `initialize` additionally gated entry into `loadSettings` on comparing this app's own semver against the stored register-content version, which could permanently block `loadSettings` from running again at all. No post-import verification existed.) #### Scenario: REQ-003 case 1 + - WHEN `autoConfigure(force=true)` is called - THEN the registers/schemas MUST be (re)configured and a result summary returned #### Scenario: REQ-003 case 2 + - WHEN `clearConfigurationCache()` is called - THEN the cached configuration MUST be invalidated +#### Scenario: REQ-003 case 3 — monolith edit alone changes the computed version + +- GIVEN the monolith `softwarecatalogus_register.json` content changes but its `info.version` field and all `register.d/*.json` fragment files are unchanged +- WHEN `loadSettings()` computes the import version +- THEN the computed version string MUST differ from the version computed before the monolith content changed +- AND `importFromApp` MUST therefore be invoked with a version that is not already stored for this app, triggering a re-import + +#### Scenario: REQ-003 case 4 — no-op import surfaces a warning instead of silent success + +- GIVEN an import completes where a schema slug present in the effective merged register does not resolve in OpenRegister +- WHEN `loadSettings()` runs its post-import verification +- THEN a WARNING MUST be logged identifying the unresolved schema slug +- AND the mismatch MUST be included in the result payload returned by `loadSettings()`/`initialize()` + +#### Scenario: REQ-003 case 5 — a prior import never blocks re-attempting a later one + +- GIVEN a prior successful import stored a register-content version on the app's `Configuration` entity, and this app's own semantic version has since been bumped for an upgrade +- WHEN `initialize()` runs during that upgrade +- THEN `loadSettings()` MUST be invoked regardless of how the stored register-content version compares to the app's own semantic version +- AND the decision of whether an actual re-import occurs MUST come only from `importFromApp`'s comparison of the newly computed content-derived version against the stored one + ### Requirement: The system SHALL manage email settings, templates and connectivity tests (REQ-004) `getEmailSettings`/`updateEmailSettings`, `getEmailConfig`/`setEmailConfig`/`getEmailConfigFocused`/`updateEmailConfig`, `getEmailTemplate`/`updateEmailTemplate`/`getDefaultEmailTemplate`/`getAllEmailTemplates`/`getEmailTemplateVariables`, `sendTestEmail`, and `testEmailConnection` MUST manage the email transport configuration + templates and run connectivity/test-send diagnostics. @@ -70,3 +104,21 @@ User-group helpers (`getGenericUserGroups`/`set...`/`update...`, org-admin, supe - WHEN `getArchiMateStatus()` is called during an import - THEN it MUST report the in-progress import status +### Requirement: The system SHALL call its own OpenRegister configuration import deterministically and account for any duplicate rows found (REQ-006) + +The service MUST call `importFromApp` from a single call site with the same, stable app-identifying key on every invocation, so this app's own code can never itself be the cause of multiple `Configuration` rows existing for it. Row-level resolution of "the" configuration for a given `appId` (matching an existing row versus creating a new one) is performed by OpenRegister's `ConfigurationService`, outside this app's control. Where duplicate configuration rows are found to already exist for this app, the service's documentation MUST record how those rows were characterized (root-cause analysis, not just their existence) and either resolve them from this app's side, if the app's own call site is conclusively the cause, or reference a filed upstream issue against the owning system when the true fix belongs there. + +#### Scenario: REQ-006 case 1 — this app's own call site is single and stable + +- GIVEN `loadSettings()` runs an import +- WHEN the call to `getConfigurationService()->importFromApp()` is inspected +- THEN it MUST always pass this app's own constant `Application::APP_ID` as the `appId` argument from the same single call site, so no duplication can originate from this app varying its own identity across calls + +#### Scenario: REQ-006 case 2 — duplicate rows are documented, not silently ignored + +- GIVEN more than one configuration row is found to already exist for this app's title/appId +- WHEN the duplication is investigated +- THEN the root-cause finding MUST be recorded (in code comments and docs) rather than left unexplained +- AND WHEN the true fix is determined to belong in OpenRegister rather than this app +- THEN an issue MUST be filed against the owning repository documenting the mechanism, and referenced from this app's code and docs +