Skip to content

fix(settings): force the register import when the computed version differs — completes #391 #400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rubenvdlinde merged 1 commit into from
Jul 24, 2026
Merged

fix(settings): force the register import when the computed version differs — completes #391 #400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 133 additions & 11 deletions lib/Service/SettingsService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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)),
]
);

Expand All @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
24 changes: 22 additions & 2 deletions openspec/specs/settings-service/spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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.
Expand Down
Loading