From c9860dd916fb0b3a1ae7015d3b5f57f3d65fe141 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Fri, 24 Jul 2026 15:54:41 +0200 Subject: [PATCH] fix(settings): force importFromApp when computed version differs from stored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenRegister's importFromApp(force: false) advances the stored configuration version whenever any registers/schemas/objects come back from the import, but does not apply property/authorization changes to schemas that already exist — only newly-created schemas get the full payload (ConductionNL/openregister#2075). A register edit that only touches an existing schema (e.g. catalog-ratings adding auteur/status/ authorization.read to the pre-existing beoordeeling schema) therefore advanced the stored version, made the instance look up to date, and left the schema stale — worse than a plain no-op, since the newly-written version also gated off every later non-forced import. Add SettingsService::resolveImportForce(): reads back the version OpenRegister already has stored for this app via ConfigurationService::getConfiguredAppVersion() (a like-for-like comparison against our own content-derived computeConfigVersion() signature) and forces importFromApp() whenever the two differ. Matching versions keep the existing cheap no-op path. An explicit caller-supplied force=true still always forces. Live-verified end-to-end via the non-forced GET /api/settings/load path (no force:true endpoint used): a throwaway probe property added to catalog-ratings.json landed on the live beoordeeling schema with force:false/effective_force:true logged, and removing the probe and re-running the same non-forced path removed it again and restored the exact original stored version string. Adds a ConfigurationService test stub and resolveImportForce() unit tests, and updates the REQ-003 spec with the new force-when-stale behavior. --- lib/Service/SettingsService.php | 144 ++++++++++- openspec/specs/settings-service/spec.md | 24 +- tests/Stubs/Service/ConfigurationService.php | 52 ++++ .../SettingsServiceResolveImportForceTest.php | 226 ++++++++++++++++++ 4 files changed, 433 insertions(+), 13 deletions(-) create mode 100644 tests/Stubs/Service/ConfigurationService.php create mode 100644 tests/Unit/Service/SettingsServiceResolveImportForceTest.php diff --git a/lib/Service/SettingsService.php b/lib/Service/SettingsService.php index a3aa467e..969adf38 100644 --- a/lib/Service/SettingsService.php +++ b/lib/Service/SettingsService.php @@ -1580,6 +1580,11 @@ public function loadSettings(bool $force=false): array // Import via configuration service if available with version checking. try { + // Default to the caller-supplied $force in case an exception is + // thrown below (e.g. from getConfigurationService()) before + // resolveImportForce() runs — the catch block's error-surfacing + // check below still needs a defined value. + $effectiveForce = $force; $configurationService = $this->getConfigurationService(); // Content-derived version signature (register-import-reliability): @@ -1595,14 +1600,54 @@ public function loadSettings(bool $force=false): array fragmentSig: $fragmentSig ); + $appId = \OCA\SoftwareCatalog\AppInfo\Application::APP_ID; + + // Force-when-stale workaround (register-import-reliability, + // https://github.com/ConductionNL/openregister/issues/2075): + // OpenRegister's importFromApp(force: false) advances the + // STORED configuration version whenever any + // registers/schemas/objects come back from the import, but + // does NOT apply property/authorization changes to schemas + // that already exist — only newly-created schemas get the + // full payload. A monolith or fragment edit to an EXISTING + // schema (e.g. a fragment adding a property + an + // authorization rule to an already-shipped schema) + // therefore advances the version, makes the instance LOOK + // up to date, and leaves the schema stale — strictly worse + // than the pre-computeConfigVersion() no-op, because the + // very version this call just wrote now also gates off + // every later non-forced import. Verified live: a version + // that legitimately advanced across an `occ upgrade` still + // left the pre-existing schema unchanged until a + // force:true import was run. + // + // Work around it here, entirely on the consumer side: use + // this app's own content-derived $configVersion as the + // authority for "something changed" (resolveImportForce() + // reads back the version OpenRegister already stored via + // the same content-derived scheme, so this is a + // like-for-like comparison — unlike the removed + // app-semver-vs-content-version comparison documented on + // shouldLoadSettings()) and force the import whenever it + // differs, so the change actually applies instead of just + // being recorded. When the versions match we keep today's + // cheap no-op path — do not import on every request. + $effectiveForce = $this->resolveImportForce( + configurationService: $configurationService, + appId: $appId, + configVersion: $configVersion, + force: $force + ); + // Log the import attempt for debugging. $this->logger->info( 'SettingsService: Attempting to import softwarecatalogus_register.json', [ - 'force' => $force, - 'app_id' => \OCA\SoftwareCatalog\AppInfo\Application::APP_ID, - 'config_version' => $configVersion, - 'data_size' => strlen(json_encode($softwareCatalogSettings)), + 'force' => $force, + 'effective_force' => $effectiveForce, + 'app_id' => $appId, + 'config_version' => $configVersion, + 'data_size' => strlen(json_encode($softwareCatalogSettings)), ] ); @@ -1619,10 +1664,10 @@ public function loadSettings(bool $force=false): array // 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, + appId: $appId, data: $softwareCatalogSettings, version: $configVersion, - force: $force + force: $effectiveForce ); $this->logger->info( @@ -1651,15 +1696,19 @@ public function loadSettings(bool $force=false): array $this->logger->error( 'Failed to import softwarecatalog settings: '.$e->getMessage(), [ - 'exception' => $e, - 'trace' => $e->getTraceAsString(), - 'force_flag' => $force, - 'app_id' => \OCA\SoftwareCatalog\AppInfo\Application::APP_ID, + 'exception' => $e, + 'trace' => $e->getTraceAsString(), + 'force_flag' => $force, + 'effective_force' => $effectiveForce, + 'app_id' => \OCA\SoftwareCatalog\AppInfo\Application::APP_ID, ] ); // In force mode, we want to surface import errors more prominently. - if ($force === true) { + // Uses $effectiveForce (not just the caller-supplied $force) so a + // failure while forcing because of a detected version mismatch is + // surfaced just as loudly as an explicit caller force:true. + if ($effectiveForce === true) { throw new \RuntimeException('Force import failed: '.$e->getMessage(), 0, $e); } }//end try @@ -1717,6 +1766,79 @@ private static function computeConfigVersion(string $baseVersion, string $monoli return $configVersion; }//end computeConfigVersion() + /** + * Decides whether `importFromApp()` should be forced for this + * `loadSettings()` call. + * + * Workaround for https://github.com/ConductionNL/openregister/issues/2075: + * `ConfigurationService::importFromApp(force: false)` advances the + * STORED configuration version whenever any registers/schemas/objects + * come back from the import, but does NOT apply property or + * authorization changes to schemas that already exist — only + * newly-created schemas receive the full payload. A register edit that + * only touches an EXISTING schema therefore advances the version, + * makes the instance LOOK up to date, and leaves the schema itself + * stale — worse than a plain no-op, because the version this call just + * wrote also gates off every later non-forced import attempt. + * + * This method treats the content-derived `$configVersion` computed by + * `computeConfigVersion()` as the authority for "something changed": + * it reads back the version OpenRegister already has stored for this + * app via `ConfigurationService::getConfiguredAppVersion()` — the same + * content-derived scheme, so this is a like-for-like comparison + * (unlike the app-semver-vs-content-version comparison removed from + * `shouldLoadSettings()`, see that method's docblock) — and forces the + * import whenever the two differ, so the change actually applies + * instead of merely being recorded. When they match, the caller's + * `$force` is passed through unchanged, preserving the existing cheap + * no-op path (this method MUST NOT force an import on every request). + * + * An explicit caller-supplied `$force=true` always forces, regardless + * of the version comparison. + * + * A stored version of `null` — either nothing has ever been imported + * for this app, or `getConfiguredAppVersion()` itself could not + * determine one (it swallows its own exceptions and returns `null`, + * see its docblock) — is treated as "differs". For a first-ever + * import there is nothing existing to skip, so forcing is harmless. + * For an undeterminable lookup, this mirrors `importFromApp()`'s own + * internal `findByApp()`/organisation-scope lookup (see the + * register-import-reliability note above the `importFromApp()` call + * in `loadSettings()`): a miss there already causes OpenRegister to + * treat the call as a fresh import today, so this does not introduce + * a new failure mode. + * + * @param \OCA\OpenRegister\Service\ConfigurationService $configurationService The resolved OpenRegister configuration service. + * @param string $appId The app id to look up the stored version for. + * @param string $configVersion The version this call just computed via `computeConfigVersion()`. + * @param bool $force The caller-supplied `$force` argument to `loadSettings()`. + * + * @return bool Whether `importFromApp()` should be called with `force=true`. + * + * @spec openspec/specs/settings-service/spec.md#requirement-the-system-shall-run-auto-configuration-import-and-configuration-maintenance-req-003 + */ + private function resolveImportForce( + \OCA\OpenRegister\Service\ConfigurationService $configurationService, + string $appId, + string $configVersion, + bool $force + ): bool { + if ($force === true) { + return true; + } + + try { + $storedConfigVersion = $configurationService->getConfiguredAppVersion($appId); + } catch (\Exception $e) { + // Defensive only — getConfiguredAppVersion() already catches its + // own exceptions and returns null. Treat as "unknown", same as a + // null return below. + $storedConfigVersion = null; + } + + return $storedConfigVersion !== $configVersion; + }//end resolveImportForce() + /** * Verifies the live OpenRegister schema set against the register this * app just (attempted to) import, so a no-op or partial import is diff --git a/openspec/specs/settings-service/spec.md b/openspec/specs/settings-service/spec.md index 73db2931..d410e889 100644 --- a/openspec/specs/settings-service/spec.md +++ b/openspec/specs/settings-service/spec.md @@ -45,9 +45,9 @@ The service MUST expose get/set (and focused get/update) pairs for voorzieningen ### 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. +`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. Before calling `importFromApp`, `loadSettings` MUST compare its freshly computed content-derived version against the version OpenRegister already has stored for this app (via `ConfigurationService::getConfiguredAppVersion`) and MUST call `importFromApp` with `force=true` whenever the two differ — even when the caller's own `$force` argument is `false` — because `importFromApp(force: false)` only records a changed version without applying property/authorization changes to already-existing schemas (see https://github.com/ConductionNL/openregister/issues/2075). When the computed and stored versions match, `loadSettings` MUST NOT force the import, preserving the existing cheap no-op path; an explicit caller-supplied `force=true` MUST continue to force the import regardless of this comparison. 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.) +(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. After the content-derived version fix (case 3 below) shipped, live verification showed the stored version now legitimately advances on register content changes, but `importFromApp(force: false)` still does not apply the corresponding changes to schemas that already exist — so the instance looked up to date while the schema was stale, and the newly-advanced version additionally gated off any later non-forced retry. `loadSettings` did not yet force the import based on its own version comparison.) #### Scenario: REQ-003 case 1 @@ -80,6 +80,26 @@ The service MUST expose get/set (and focused get/update) pairs for voorzieningen - 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 +#### Scenario: REQ-003 case 6 — a stale stored version forces the import so changes to existing schemas apply + +- GIVEN `loadSettings()`'s freshly computed content-derived version differs from the version `ConfigurationService::getConfiguredAppVersion` reports as already stored for this app +- AND the caller invoked `loadSettings()` with `force=false` +- WHEN `loadSettings()` calls `importFromApp` +- THEN `importFromApp` MUST be called with `force=true`, so changes to already-existing schemas are applied rather than only recorded on the stored version marker + +#### Scenario: REQ-003 case 7 — matching versions preserve the cheap no-op path + +- GIVEN `loadSettings()`'s freshly computed content-derived version matches the version `ConfigurationService::getConfiguredAppVersion` reports as already stored for this app +- AND the caller invoked `loadSettings()` with `force=false` +- WHEN `loadSettings()` calls `importFromApp` +- THEN `importFromApp` MUST be called with `force=false`, so an unchanged register still short-circuits at OpenRegister's version gate instead of re-importing on every call + +#### Scenario: REQ-003 case 8 — an explicit caller force always forces + +- GIVEN the caller invoked `loadSettings()` with `force=true` +- WHEN `loadSettings()` calls `importFromApp` +- THEN `importFromApp` MUST be called with `force=true` regardless of how the computed and stored versions compare + ### 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. diff --git a/tests/Stubs/Service/ConfigurationService.php b/tests/Stubs/Service/ConfigurationService.php new file mode 100644 index 00000000..db4c5e1e --- /dev/null +++ b/tests/Stubs/Service/ConfigurationService.php @@ -0,0 +1,52 @@ + + * @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\OpenRegister\Service\ConfigurationService; +use OCA\SoftwareCatalog\Service\SettingsService; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use ReflectionMethod; + +/** + * Covers the version-comparison decision `loadSettings()` uses to decide + * whether `importFromApp()` should be called with `force=true`. + * + * Background: OpenRegister's `importFromApp(force: false)` advances the + * STORED configuration version whenever any registers/schemas/objects come + * back from the import, but does NOT apply property/authorization changes + * to schemas that already exist — only newly-created schemas receive the + * full payload. Verified live: a `catalog-ratings` fragment adding + * `auteur`/`status`/`authorization.read` to the pre-existing `beoordeeling` + * schema advanced the stored configuration version across an `occ upgrade` + * (proving the content-derived version signature itself works), yet the + * schema was left unchanged until a subsequent `force: true` import ran. + * `resolveImportForce()` closes that gap on the consumer side by comparing + * the freshly computed content-derived version against the version + * OpenRegister already has stored for this app and forcing whenever they + * differ. + */ +final class SettingsServiceResolveImportForceTest extends TestCase +{ + + + /** + * Build a SettingsService instance without running its constructor, and + * a reflection handle to the private resolveImportForce() method. + * + * @return array{0: SettingsService, 1: ReflectionMethod} + */ + private function makeSubject(): array + { + $service = $this->getMockBuilder(SettingsService::class) + ->disableOriginalConstructor() + ->getMock(); + + $method = new ReflectionMethod($service, 'resolveImportForce'); + $method->setAccessible(true); + + return [$service, $method]; + }//end makeSubject() + + + /** + * (a) Computed and stored versions differ, caller did not ask for + * force: resolveImportForce() MUST return true, so loadSettings() + * passes force=true to importFromApp() and the change to an existing + * schema actually applies instead of only advancing the stored marker. + * + * @return void + */ + public function testVersionsDifferForcesImportEvenWhenCallerDidNotRequestForce(): void + { + [$service, $method] = $this->makeSubject(); + + /** @var ConfigurationService|MockObject $configurationService */ + $configurationService = $this->createMock(ConfigurationService::class); + $configurationService->method('getConfiguredAppVersion') + ->with('softwarecatalog') + ->willReturn('2.4.0+base.9003c029'); + + $result = $method->invoke( + $service, + $configurationService, + 'softwarecatalog', + '2.4.0+base.f6e72fc8+frag.92299b19', + false + ); + + $this->assertTrue( + $result, + 'A stale stored version must force the import so changes to already-existing schemas apply ' + .'(ConductionNL/openregister#2075) — not just re-record the new version.' + ); + }//end testVersionsDifferForcesImportEvenWhenCallerDidNotRequestForce() + + + /** + * (b) Computed and stored versions match, caller did not ask for + * force: resolveImportForce() MUST return false, preserving the + * existing cheap no-op path — an unchanged register must not trigger a + * forced import on every call. + * + * @return void + */ + public function testVersionsMatchDoesNotForceImport(): void + { + [$service, $method] = $this->makeSubject(); + + /** @var ConfigurationService|MockObject $configurationService */ + $configurationService = $this->createMock(ConfigurationService::class); + $configurationService->method('getConfiguredAppVersion') + ->with('softwarecatalog') + ->willReturn('2.4.0+base.f6e72fc8+frag.92299b19'); + + $result = $method->invoke( + $service, + $configurationService, + 'softwarecatalog', + '2.4.0+base.f6e72fc8+frag.92299b19', + false + ); + + $this->assertFalse( + $result, + 'Matching computed/stored versions must NOT force the import — otherwise every loadSettings() ' + .'call would re-import unconditionally, a performance regression.' + ); + }//end testVersionsMatchDoesNotForceImport() + + + /** + * (c) An explicit caller-supplied force=true MUST still force the + * import, regardless of how the computed and stored versions compare + * — resolveImportForce() must not weaken the pre-existing explicit + * force semantics. + * + * @return void + */ + public function testExplicitCallerForceAlwaysForcesRegardlessOfVersionMatch(): void + { + [$service, $method] = $this->makeSubject(); + + /** @var ConfigurationService|MockObject $configurationService */ + $configurationService = $this->createMock(ConfigurationService::class); + $configurationService->expects($this->never())->method('getConfiguredAppVersion'); + + $result = $method->invoke( + $service, + $configurationService, + 'softwarecatalog', + '2.4.0+base.f6e72fc8+frag.92299b19', + true + ); + + $this->assertTrue( + $result, + 'An explicit caller force=true must always force the import, without even needing to read ' + .'back the stored version.' + ); + }//end testExplicitCallerForceAlwaysForcesRegardlessOfVersionMatch() + + + /** + * A stored version of null (nothing imported yet for this app, or + * getConfiguredAppVersion() could not determine one) is treated as + * "differs" — resolveImportForce() MUST return true, since there is + * either nothing to skip (first import) or the safe default is to + * force. + * + * @return void + */ + public function testNullStoredVersionIsTreatedAsDifferingAndForcesImport(): void + { + [$service, $method] = $this->makeSubject(); + + /** @var ConfigurationService|MockObject $configurationService */ + $configurationService = $this->createMock(ConfigurationService::class); + $configurationService->method('getConfiguredAppVersion')->willReturn(null); + + $result = $method->invoke( + $service, + $configurationService, + 'softwarecatalog', + '2.4.0+base.f6e72fc8+frag.92299b19', + false + ); + + $this->assertTrue($result); + }//end testNullStoredVersionIsTreatedAsDifferingAndForcesImport() + + + /** + * If getConfiguredAppVersion() itself throws (defensive path — the real + * implementation already catches its own exceptions, but the caller + * must not blow up if that ever changes), resolveImportForce() MUST + * treat the lookup as "unknown" and still return a boolean (forcing), + * not propagate the exception. + * + * @return void + */ + public function testExceptionFromStoredVersionLookupIsTreatedAsUnknownAndForcesImport(): void + { + [$service, $method] = $this->makeSubject(); + + /** @var ConfigurationService|MockObject $configurationService */ + $configurationService = $this->createMock(ConfigurationService::class); + $configurationService->method('getConfiguredAppVersion') + ->willThrowException(new \RuntimeException('lookup failed')); + + $result = $method->invoke( + $service, + $configurationService, + 'softwarecatalog', + '2.4.0+base.f6e72fc8+frag.92299b19', + false + ); + + $this->assertTrue($result); + }//end testExceptionFromStoredVersionLookupIsTreatedAsUnknownAndForcesImport() +}//end class