From 1b1dd95fc9babba78bbc5613a8f87dee8af557ac Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sun, 16 Aug 2026 12:06:02 +0200 Subject: [PATCH] fix(register): two dangling objectDescriptionField values detached the whole voorzieningen register MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The E2E (Playwright) job on `development` has not run a single test since 2026-08-14 19:29Z. It fails in the seed step, before Playwright starts, on softwarecatalog's own honest gate: softwarecatalog has no register/schema mapping for: ['organisatie_schema', 'contactpersoon_schema', 'module_schema', 'contract_schema'] That gate is correct and the register really is unusable. The chain, read from the failing job's own Nextcloud log (run 31937311499): 1. `view` and `bioMeasure` declare `objectDescriptionField: "summary"` while neither schema has a `summary` property. OpenRegister's `SchemaMapper::validateConfigField()` throws for exactly this, and `ImportHandler` logs `Failed to import schema: The value for objectDescriptionField ('summary') does not exist as a property in the schema.` 2. Both schemas are therefore absent from the import's `schemasMap`, so the `voorzieningen` and `vng-gemma` registers are imported without them — OpenRegister logs 18 `not found in schemasMap` warnings. 3. `SettingsService::configureVoorzieningen()` iterates the register's schemas to build the app-config map. With the links gone it writes `register` and nothing else, leaving every `*_schema` key empty. 4. `tests/e2e/ci-seed.sh` refuses to run Playwright against that. Correctly — the alternative is ~20 spec failures blaming the fixtures. Where the two values came from: commit 386771dc (#513, "translate 12 pre-existing Dutch property names") renamed the `view` schema's `summary` property KEY to `omschrijving` and left `objectDescriptionField` pointing at the old key, and separately rewrote `bioMeasure`'s `objectDescriptionField` VALUE from `omschrijving` to `summary` while its property key stayed `omschrijving`. Two dangling references, opposite directions, one commit. This points both at the property each schema actually declares. It does not rename anything: a property rename here is a data migration and belongs with the vocabulary programme, not with an E2E fix. Evidence, same instrument both sides — OpenRegister's own three acceptance forms applied to the shipped register file, 20 schemas / 38 configuration fields measured: before: 2 failures (view.objectDescriptionField, bioMeasure.objectDescriptionField) after: 0 failures The new test reproduces that measurement in PHPUnit and carries a positive control asserting the check can fail, so a future rename cannot silently detach the register again. Run in a php:8.3-cli container (the host is 8.2): before fix: Tests: 4, Assertions: 21, Failures: 1 (naming both schemas) after fix: OK (4 tests, 21 assertions) phpcs --standard=phpcs.xml on the new file: exit 0, 1 file measured The test also records the two `objectSummaryField` values that dangle today (`element`, `relation`). OpenRegister does not validate that key, so they are inert — asserted as a known set rather than zero, so adding a new one fails while the existing debt stays visible. --- lib/Settings/softwarecatalogus_register.json | 4 +- .../RegisterConfigFieldResolutionTest.php | 284 ++++++++++++++++++ 2 files changed, 286 insertions(+), 2 deletions(-) create mode 100644 tests/Unit/Settings/RegisterConfigFieldResolutionTest.php diff --git a/lib/Settings/softwarecatalogus_register.json b/lib/Settings/softwarecatalogus_register.json index 59ef03f4..6d43542c 100644 --- a/lib/Settings/softwarecatalogus_register.json +++ b/lib/Settings/softwarecatalogus_register.json @@ -5643,7 +5643,7 @@ "configuration": { "autoPublish": false, "objectNameField": "name", - "objectDescriptionField": "summary" + "objectDescriptionField": "omschrijving" } }, "model": { @@ -7603,7 +7603,7 @@ "configuration": { "objectNameField": "name", "objectSummaryField": "thema", - "objectDescriptionField": "summary", + "objectDescriptionField": "omschrijving", "autoPublish": true } }, diff --git a/tests/Unit/Settings/RegisterConfigFieldResolutionTest.php b/tests/Unit/Settings/RegisterConfigFieldResolutionTest.php new file mode 100644 index 00000000..b5f3eaba --- /dev/null +++ b/tests/Unit/Settings/RegisterConfigFieldResolutionTest.php @@ -0,0 +1,284 @@ + + * @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 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Settings; + +use PHPUnit\Framework\TestCase; + +/** + * Every configuration field in the shipped register must resolve to a declared property. + */ +class RegisterConfigFieldResolutionTest extends TestCase { + + /** + * The configuration keys OpenRegister validates against the property map. + * + * `objectSummaryField` is deliberately NOT in this list: OpenRegister + * validates only these two (SchemaMapper.php, the two + * `validateConfigField()` call sites), so adding it here would fail the + * build on references OpenRegister accepts today. It is reported by + * testDanglingSummaryFieldsAreReportedNotEnforced() instead. + * + * @var array + */ + private const VALIDATED_FIELDS = ['objectNameField', 'objectDescriptionField']; + + /** + * The decoded register file, loaded once in setUp(). + * + * @var array + */ + private array $register; + + /** + * Load and decode the register file once. + * + * @return void + */ + protected function setUp(): void { + $path = __DIR__ . '/../../../lib/Settings/softwarecatalogus_register.json'; + $this->assertFileExists(filename: $path); + $decoded = json_decode((string)file_get_contents($path), true); + $this->assertIsArray(actual: $decoded, message: 'register file must be valid JSON'); + $this->register = $decoded; + }//end setUp() + + /** + * Decide whether a configuration value resolves against a property map. + * + * A transcription of OpenRegister's `SchemaMapper::validateConfigField()`. + * The three accepted forms, in the order OpenRegister tests them: + * 1. a Twig template — every `{{ prop }}` reference must exist; + * 2. a pipe-separated fallback list — AT LEAST ONE entry must exist; + * 3. a plain property name — it must exist. + * + * @param string $value The configuration value. + * @param array $propertyKeys The schema's declared property keys. + * + * @return string|null The failure reason, or null when the value resolves. + */ + private function resolutionFailure(string $value, array $propertyKeys): ?string { + if (str_contains($value, '{{') === true && str_contains($value, '}}') === true) { + preg_match_all('/\{\{\s*([a-zA-Z0-9_-]+)\s*\}\}/', $value, $matches); + $templateProps = ($matches[1] ?? []); + if (empty($templateProps) === true) { + return null; + } + + foreach ($templateProps as $prop) { + if (in_array($prop, $propertyKeys, true) === false) { + return "template property '$prop' does not exist"; + } + } + + return null; + } + + if (str_contains($value, '|') === true) { + foreach (array_map('trim', explode('|', $value)) as $fallback) { + if (in_array($fallback, $propertyKeys, true) === true) { + return null; + } + } + + return "none of the fallback fields in '$value' exist as properties"; + } + + if (in_array($value, $propertyKeys, true) === false) { + return "'$value' does not exist as a property"; + } + + return null; + }//end resolutionFailure() + + /** + * Every validated configuration field resolves to a declared property. + * + * @return void + */ + public function testEveryValidatedConfigFieldResolvesToADeclaredProperty(): void { + $schemas = ($this->register['components']['schemas'] ?? []); + $this->assertNotEmpty(actual: $schemas, message: 'the register must declare schemas'); + + $checked = 0; + $failures = []; + foreach ($schemas as $slug => $schema) { + $propertyKeys = array_keys(($schema['properties'] ?? [])); + foreach (self::VALIDATED_FIELDS as $field) { + $value = (string)(($schema['configuration'] ?? [])[$field] ?? ''); + if ($value === '') { + continue; + } + + $checked++; + $failure = $this->resolutionFailure(value: $value, propertyKeys: $propertyKeys); + if ($failure !== null) { + $failures[] = "$slug.$field: $failure"; + } + } + } + + // COUNT WHAT WAS MEASURED. An empty `$failures` is also what a run over + // zero fields produces, and that is the shape this whole test exists to + // stop being mistaken for a pass. + $this->assertGreaterThan( + expected: 30, + actual: $checked, + message: 'far fewer configuration fields were checked than this register declares — the traversal is wrong, not the data' + ); + + $this->assertSame( + expected: [], + actual: $failures, + message: "a configuration field names a property its schema does not declare. OpenRegister REJECTS the whole schema for this, " + . "the register is then imported without that schema link, and every dependent app-config id is written empty:\n- " + . implode("\n- ", $failures) + ); + }//end testEveryValidatedConfigFieldResolvesToADeclaredProperty() + + /** + * POSITIVE CONTROL — the check above can actually fail. + * + * Replays the exact shape that broke `development` on 2026-08-14: a + * property key renamed out from under a configuration value that still + * names the old key. Without this, a traversal bug would make the test + * above green forever. + * + * @return void + */ + public function testTheResolutionCheckCanFail(): void { + $this->assertNotNull( + actual: $this->resolutionFailure(value: 'summary', propertyKeys: ['name', 'omschrijving']), + message: 'a plain value naming an absent property must be reported' + ); + $this->assertNotNull( + actual: $this->resolutionFailure(value: '{{ gone }}', propertyKeys: ['name']), + message: 'a template referencing an absent property must be reported' + ); + $this->assertNotNull( + actual: $this->resolutionFailure(value: 'gone | alsoGone', propertyKeys: ['name']), + message: 'a fallback list with no surviving entry must be reported' + ); + + // And the three forms that OpenRegister accepts must NOT be reported, + // or the test above would fail on data that imports perfectly well. + $this->assertNull(actual: $this->resolutionFailure(value: 'name', propertyKeys: ['name'])); + $this->assertNull(actual: $this->resolutionFailure(value: '{{ name }}', propertyKeys: ['name'])); + $this->assertNull(actual: $this->resolutionFailure(value: 'gone | name', propertyKeys: ['name'])); + }//end testTheResolutionCheckCanFail() + + /** + * Every schema a register references is declared in the same file. + * + * The second half of the same failure: `ImportHandler` resolves a + * register's `schemas` list against the slugs it imported in that session, + * so a reference to a slug this file does not declare silently produces a + * register with a missing link rather than an error. + * + * @return void + */ + public function testEveryRegisterSchemaReferenceIsDeclared(): void { + $schemas = ($this->register['components']['schemas'] ?? []); + $registers = ($this->register['components']['registers'] ?? []); + $this->assertNotEmpty(actual: $registers, message: 'the register file must declare registers'); + + $declared = []; + foreach ($schemas as $key => $schema) { + $declared[] = (string)($schema['slug'] ?? $key); + } + + $checked = 0; + $dangling = []; + foreach ($registers as $registerSlug => $register) { + foreach (($register['schemas'] ?? []) as $reference) { + $checked++; + if (in_array((string)$reference, $declared, true) === false) { + $dangling[] = "$registerSlug -> $reference"; + } + } + } + + $this->assertGreaterThan( + expected: 10, + actual: $checked, + message: 'almost no register->schema references were checked — the traversal is wrong' + ); + $this->assertSame(expected: [], actual: $dangling, message: 'a register references a schema this file does not declare'); + }//end testEveryRegisterSchemaReferenceIsDeclared() + + /** + * Records the `objectSummaryField` values that do not resolve. + * + * OpenRegister does NOT validate this key today, so a dangling value is + * inert rather than fatal — but it is the same defect one release away + * from being fatal, and leaving it undocumented is how the two + * `objectDescriptionField` references survived review. This asserts the + * known set rather than zero, so ADDING a new one fails the build while + * the existing debt stays visible and counted. + * + * @return void + */ + public function testDanglingSummaryFieldsAreReportedNotEnforced(): void { + $dangling = []; + foreach (($this->register['components']['schemas'] ?? []) as $slug => $schema) { + $value = (string)(($schema['configuration'] ?? [])['objectSummaryField'] ?? ''); + if ($value === '') { + continue; + } + + if ($this->resolutionFailure(value: $value, propertyKeys: array_keys(($schema['properties'] ?? []))) !== null) { + $dangling[] = "$slug: $value"; + } + } + + sort($dangling); + $this->assertSame( + expected: ['element: summary', 'relation: summary'], + actual: $dangling, + message: 'the set of dangling objectSummaryField references changed. OpenRegister ignores this key today; ' + . 'if you added one, point it at a declared property instead' + ); + }//end testDanglingSummaryFieldsAreReportedNotEnforced() +}//end class