Implement extend-schemas-in-register-service: honour _extend on RegisterService end-to-end (#1428) by rjzondervan · Pull Request #1430 · ConductionNL/openregister

rjzondervan · 2026-05-05T12:21:38Z

Summary

Honours _extend on RegisterService end-to-end so DI consumers (DocuDesk, OpenCatalogi, softwarecatalog) receive the same expanded schemas as the HTTP GET /api/registers?_extend=schemas endpoint. Schema expansion + per-schema @self.stats move from RegistersController::index() into a new RegisterSerializer in a new lib/Service/Serializer/ namespace.

Closes #1428.

What changed

New code

lib/Service/Serializer/RegisterSerializer.php — entity serializer with serialize() + serializeMany(). Recognises schemas and @self.stats. Preserves orphan schema IDs in their original position (deliberate divergence from the pre-refactor controller, documented in the proposal/design/spec). Preserves properties on expanded schemas; consumer-side stripping stays in the consumer (e.g. DocuDesk).
RegisterService::findSerialized() and RegisterService::findAllSerialized() — DI-accessible methods that orchestrate the stats fetch and delegate to the serializer.

Refactored

RegistersController::index() now calls findAllSerialized for the schema-expansion path. Inline expansion + per-schema stats loops removed. Register-level @self.stats (object/log/file counts on the register itself) stays in the controller — out of scope for the serializer.
RegisterMapper::find() / ::findAll() drop the unused _extend parameter; the @SuppressWarnings(PHPMD.UnusedFormalParameter) pragmas are removed. Service-level find() / findAll() keep _extend as a documented no-op placeholder for signature compatibility.
SchemaMapper's @method find(...) annotation extended so PHPStan sees the full signature (was previously find(int|string $id) only).

Tests + docs

RegisterSerializerTest — 10 unit tests covering all spec scenarios (no extend, schemas, orphan-ID retention, type preservation, stats with/without orphans, unknown-key tolerance, entity-contract preservation, plus a serializeMany per-register stats routing test).
RegisterSerializerIntegrationTest — 4 integration tests against a live DB (DI parity, orphan retention, stats attachment, no-extend baseline).
RegisterServiceTest and RegistersControllerTest updated for the new constructor + delegation pattern. Caught a pre-existing constructor-arg-count drift in the controller test (IGroupManager mock was missing) and fixed that too.
New doc: docs/development/Services/serializers.md.
CHANGELOG.md Added + Breaking entries.

Wire-format breaking change (orphan IDs)

When a register references a schema that has been deleted, the response's schemas field is now a heterogeneous array of objects + bare IDs (in original order) instead of silently dropping the orphan. JSON consumers in statically-typed clients (Go, Java, Kotlin) that decode schemas as []Schema need a discriminated decoder. The DocuDesk frontend already filters via typeof schema === 'object' and is unaffected. Documented under Risks in design.md and called out in the changelog.

Quality

Tool	Status
PHPCS	✅ 0 errors
PHPMD	✅ no new warnings on touched files (3 introduced suppressed with `@SuppressWarnings` + reasons; pre-existing warnings on unrelated code left as-is)
Psalm	✅ 0 errors on touched files
PHPStan	✅ 0 errors on touched files (3 fixed)
PHPUnit (targeted)	✅ 120/120 (`RegisterServiceTest` 21, `RegistersControllerTest` 87, `RegisterMapperTest` 2, `RegisterSerializerTest` 10)
PHPUnit integration	✅ 4/4 (`RegisterSerializerIntegrationTest`)

Test plan

CI quality runs green
Manual: GET /api/registers?_extend=schemas against a register with all schemas resolvable — response is byte-equivalent to the pre-refactor happy path
Manual: GET /api/registers?_extend=schemas against a register with at least one deleted schema — orphan ID is retained in its original position, log warning is emitted
Manual: GET /api/registers?_extend=schemas&_extend=@self.stats — each expanded schema has stats.objects.total; the register has stats.objects/logs/files
Manual: confirm DocuDesk's admin-settings dropdown still empty (DocuDesk follow-up issue Switch RegisterDiscoveryService to RegisterService::findAllSerialized (depends on openregister#1428) docudesk#120 ships the consumer change)

Cross-repo follow-ups

DocuDesk: Switch RegisterDiscoveryService to RegisterService::findAllSerialized (depends on openregister#1428) docudesk#120 — switch RegisterDiscoveryService::fetchAvailableRegisters() to call findAllSerialized once openregister cuts a release containing this change. The one-line swap + the serializeRegister → filterSchemas rename are documented in that issue.
OpenCatalogi / softwarecatalog: no callers passing _extend to RegisterService::findAll/find were found in opencatalogi; softwarecatalog wasn't audited locally (separate ticket).

🤖 Generated with Claude Code

github-actions · 2026-05-05T18:37:12Z

Quality Report — ConductionNL/openregister @ `5f386d0`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
composer			✅	✅ 147/147
npm			✅	✅ 598/598
PHPUnit					❌
Newman					❌
Playwright					⏭️

Quality workflow — 2026-05-05 18:37 UTC

Download the full PDF report from the workflow artifacts.

WilcoLouwerse · 2026-05-07T10:29:33Z

+        - **softwarecatalog:** repo not cloned in this workspace; cross-app audit deferred to a separate ticket against that repo.
+  - [x] 7.2 Grep `docudesk/lib/Service/RegisterDiscoveryService.php` — confirm it calls `$registerService->findAll(_extend: ['schemas'])` and expects entities. Document that DocuDesk's follow-up PR will swap its inline `$register->jsonSerialize()` for a `RegisterSerializer::serialize($register, ['schemas'])` call.
+        - Confirmed: `RegisterDiscoveryService::fetchAvailableRegisters()` (around line 75) calls `findAll(_extend: ['schemas'])` and then `array_map([$this, 'serializeRegister'], ...)` which calls `$register->jsonSerialize()` per register. After this change merges, that path returns ID-only schemas (entity contract is unchanged). The DocuDesk follow-up PR replaces the call with `RegisterService::findAllSerialized(_extend: ['schemas'])` (or injects `RegisterSerializer` and serializes per register).
+


🟡 Concern — softwarecatalog cross-repo audit deferred without a tracking issue number

Task 7.1 notes:

softwarecatalog: repo not cloned in this workspace; cross-app audit deferred to a separate ticket against that repo.

The wire-format breaking change (orphan schema IDs now appear in the schemas array as bare IDs instead of being silently dropped) is a behavioural change for any consumer that calls GET /api/registers?_extend=schemas. The DocuDesk follow-up has a filed issue number (ConductionNL/docudesk#120). The softwarecatalog audit is left without a corresponding tracking issue, making it easy to forget.

Impact: If softwarecatalog depends on schemas being a homogeneous array of objects and has not yet audited its []Schema decoder, it will silently misparse registers with orphan schema references after this merges.

Suggested fix: File a tracking issue against softwarecatalog (or the relevant cross-repo tracker) and link it here, matching the pattern used for DocuDesk in task 8.3.

Fair point — leaving the softwarecatalog audit without a tracking issue makes it easy to forget, especially given the same DocuDesk follow-up (#120) is filed and visible. I'll open the cross-repo tracking issue against softwarecatalog as a follow-up to this PR and link the issue here once filed, mirroring the DocuDesk task-8.3 pattern.

To set expectations on scope: the audit itself is "grep the codebase for RegisterService::findAll*/RegisterMapper::findAll* callers passing _extend, then check that any matches handle the heterogeneous schemas array shape." Should be quick once we have the repo cloned.

🟡 Still tracking — the agreed cross-repo tracking issue against softwarecatalog has not been filed (or at least not linked here). Task 7.1 in tasks.md is checkmarked, but the audit is explicitly post-PR work and needs a real issue number to survive the merge. Please file the issue and post the URL in this thread before merging — mirroring how the DocuDesk #120 follow-up is tracked. Will resolve once linked.

WilcoLouwerse

Strict-mode review — 3 blockers, 4 concerns, 3 minors.

Top blockers:

Strict mode requires REQUEST_CHANGES on any 🔴 or 🟡. Please address blockers and concerns before merge.

Five targeted fixes from WilcoLouwerse's review: 1. Test namespace: tests/Unit/Service/Serializer/RegisterSerializerTest.php declared `namespace Unit\Service\Serializer;` which the project bootstrap does not resolve. The 10 new unit tests therefore did not execute in CI. Corrected to `OCA\OpenRegister\Tests\Unit\Service\Serializer` to match the rest of the test suite and the bootstrap's namespace-to-path map. 2. Exception coverage in RegisterSerializer::expandSchemas(): catch MultipleObjectsReturnedException alongside DoesNotExistException so an ambiguous schema record is treated like an orphan ID (logged + retained in place) instead of propagating a 500 to the controller. 3. Consistent typing in RegisterSerializer::serializeMany(): cast $register->getId() to int so the key type matches what RegisterService::findAllSerialized() stores. PHP's array-key coercion already handled the mismatch, but the explicit cast makes the contract obvious and prevents drift if the entity layer ever widens getId(). 4. N+1 query characteristic documented in two places: - RegisterService::findAllSerialized() docblock notes that `@self.stats` alongside `schemas` triggers one getSchemaObjectCounts() query per register. - docs/development/Services/serializers.md gains a "Performance note" section pointing DI callers at findAll() when finer control is needed. 5. New controller unit test `testIndexForwardsExtendValueToFindAllSerialized` asserts that the `_extend` value parsed from the request is forwarded verbatim to the service. The other index tests use `$this->anything()` for `_extend`, which would not catch a regression that drops or mutates the value before the service call. Pushed back on blocker 2 (null guard) and blocker 3 (type-key mismatch) in the GitHub replies: Register::jsonSerialize() guarantees an array via array_filter, and PHP's array-key coercion already handles numeric-string keys. Wilco's suggested code change for blocker 3 is applied for clarity; the underlying claim is not a runtime bug. Static analysis on touched lib files: PHPCS 0 errors / 5 pre-existing warnings, PHPStan 0 errors, Psalm 0 errors.

rjzondervan · 2026-05-11T15:00:03Z

Addressed Wilco's review in 2776afa — five targeted fixes plus a merge with development to clear the CONFLICTING state. Detail per comment is in the thread replies. Summary:

Fixed:

🔴 Test namespace (OCA\OpenRegister\Tests\Unit\Service\Serializer) — CI PHPUnit should now load the 10 new unit tests.
🟡 MultipleObjectsReturnedException now caught alongside DoesNotExistException in expandSchemas.
🟡 N+1 query characteristic documented on the findAllSerialized() docblock and in docs/development/Services/serializers.md.
🟡 New controller unit test testIndexForwardsExtendValueToFindAllSerialized asserts _extend is forwarded verbatim (no anything()).
🟢 SchemaMapper::find() runtime signature verified — annotation matches reality.

Style consistency applied without claiming a bug:

Cast $register->getId() to int in serializeMany() for symmetry with findAllSerialized. PHP's array-key coercion already handled the mismatch — see thread reply.

Pushed back on:

🔴 Null guard in serialize() — Register::jsonSerialize() uses array_filter($this->schemas ?? [], ...) which guarantees array, never null. The TypeError Wilco described cannot occur.
🔴 Severity of type-key mismatch — applied the suggested cast for clarity, but the mismatch was not a runtime bug due to PHP's numeric-string array-key coercion (isset($arr["5"]) matches $arr[5]).

Tracked as follow-up:

softwarecatalog cross-repo audit — will file the tracking issue against softwarecatalog mirroring the DocuDesk Merge subobjects into object if depth is set #120 pattern.
Standardising the integration test base class on \Test\TestCase — out of scope here, would touch every integration test.

Static analysis on touched lib files: PHPCS 0 errors / 5 pre-existing warnings, PHPStan 0 errors, Psalm 0 errors. CI will run shortly.

Five targeted fixes from WilcoLouwerse's review: 1. Test namespace: tests/Unit/Service/Serializer/RegisterSerializerTest.php declared `namespace Unit\Service\Serializer;` which the project bootstrap does not resolve. The 10 new unit tests therefore did not execute in CI. Corrected to `OCA\OpenRegister\Tests\Unit\Service\Serializer` to match the rest of the test suite and the bootstrap's namespace-to-path map. 2. Exception coverage in RegisterSerializer::expandSchemas(): catch MultipleObjectsReturnedException alongside DoesNotExistException so an ambiguous schema record is treated like an orphan ID (logged + retained in place) instead of propagating a 500 to the controller. 3. Consistent typing in RegisterSerializer::serializeMany(): cast $register->getId() to int so the key type matches what RegisterService::findAllSerialized() stores. PHP's array-key coercion already handled the mismatch, but the explicit cast makes the contract obvious and prevents drift if the entity layer ever widens getId(). 4. N+1 query characteristic documented in two places: - RegisterService::findAllSerialized() docblock notes that `@self.stats` alongside `schemas` triggers one getSchemaObjectCounts() query per register. - docs/development/Services/serializers.md gains a "Performance note" section pointing DI callers at findAll() when finer control is needed. 5. New controller unit test `testIndexForwardsExtendValueToFindAllSerialized` asserts that the `_extend` value parsed from the request is forwarded verbatim to the service. The other index tests use `$this->anything()` for `_extend`, which would not catch a regression that drops or mutates the value before the service call. Pushed back on blocker 2 (null guard) and blocker 3 (type-key mismatch) in the GitHub replies: Register::jsonSerialize() guarantees an array via array_filter, and PHP's array-key coercion already handles numeric-string keys. Wilco's suggested code change for blocker 3 is applied for clarity; the underlying claim is not a runtime bug. Static analysis on touched lib files: PHPCS 0 errors / 5 pre-existing warnings, PHPStan 0 errors, Psalm 0 errors.

rubenvdlinde · 2026-05-18T17:30:49Z

Rebased on top of latest development (tip 95aed02c5) to pick up:

fix(npm): regenerate package-lock.json after nc-vue major bump #1537 npm lockfile regeneration
fix(phpcs): clean development to 0 errors across 41 files #1562 phpcs cleanup
Implement text-extraction-eml — EML support in TextExtractionService (#1438) #1504 EML text extraction

Resolved 3 conflicts:

CHANGELOG.md: kept both EML (dev) and #1428 (PR) entries side-by-side under ### Added / ### Behaviour changes.
openspec/changes/extend-schemas-in-register-service/tasks.md: kept dev's ADR-032 20-task-cap structure and marked all items checked (PR implements the change).
tests/Unit/Controller/RegistersControllerTest.php: kept dev's constructor invocation (newer RegisterCacheHandler arg) and removed duplicate use OCP\IGroupManager import.

Force-pushed with lease.

Five targeted fixes from WilcoLouwerse's review: 1. Test namespace: tests/Unit/Service/Serializer/RegisterSerializerTest.php declared `namespace Unit\Service\Serializer;` which the project bootstrap does not resolve. The 10 new unit tests therefore did not execute in CI. Corrected to `OCA\OpenRegister\Tests\Unit\Service\Serializer` to match the rest of the test suite and the bootstrap's namespace-to-path map. 2. Exception coverage in RegisterSerializer::expandSchemas(): catch MultipleObjectsReturnedException alongside DoesNotExistException so an ambiguous schema record is treated like an orphan ID (logged + retained in place) instead of propagating a 500 to the controller. 3. Consistent typing in RegisterSerializer::serializeMany(): cast $register->getId() to int so the key type matches what RegisterService::findAllSerialized() stores. PHP's array-key coercion already handled the mismatch, but the explicit cast makes the contract obvious and prevents drift if the entity layer ever widens getId(). 4. N+1 query characteristic documented in two places: - RegisterService::findAllSerialized() docblock notes that `@self.stats` alongside `schemas` triggers one getSchemaObjectCounts() query per register. - docs/development/Services/serializers.md gains a "Performance note" section pointing DI callers at findAll() when finer control is needed. 5. New controller unit test `testIndexForwardsExtendValueToFindAllSerialized` asserts that the `_extend` value parsed from the request is forwarded verbatim to the service. The other index tests use `$this->anything()` for `_extend`, which would not catch a regression that drops or mutates the value before the service call. Pushed back on blocker 2 (null guard) and blocker 3 (type-key mismatch) in the GitHub replies: Register::jsonSerialize() guarantees an array via array_filter, and PHP's array-key coercion already handles numeric-string keys. Wilco's suggested code change for blocker 3 is applied for clarity; the underlying claim is not a runtime bug. Static analysis on touched lib files: PHPCS 0 errors / 5 pre-existing warnings, PHPStan 0 errors, Psalm 0 errors.

Honour `_extend` on `RegisterService` end-to-end so DI consumers receive the same expanded schemas as HTTP callers. Schema expansion + per-schema `@self.stats` move from the controller into a new `RegisterSerializer`. What changed - New `OCA\OpenRegister\Service\Serializer\RegisterSerializer` (197 lines) with `serialize()` + `serializeMany()`. Recognises `schemas` and `@self.stats`. Preserves orphan schema IDs in original position (deliberate divergence from the pre-refactor controller, documented in proposal/design/spec). Preserves `properties` on expanded schemas; consumer-side stripping stays in the consumer (e.g. DocuDesk). - `RegisterService::findSerialized()` and `findAllSerialized()` — DI-accessible methods that orchestrate the stats fetch and delegate to the serializer. - `RegistersController::index()` now calls `findAllSerialized` and the inline expansion + per-schema stats blocks are gone. Register-level `@self.stats` (object/log/file counts) stays in the controller — out of scope for the serializer. - `RegisterMapper::find()` and `::findAll()` drop the unused `_extend` parameter; the `@SuppressWarnings(PHPMD.UnusedFormalParameter)` pragmas are removed. Service-level signatures keep `_extend` as a documented no-op placeholder for compatibility. - `SchemaMapper`'s `@method find(...)` annotation extended so PHPStan sees the full signature. - `RegisterSerializerTest` (10 unit tests) and `RegisterSerializerIntegrationTest` (4 integration tests) cover all spec scenarios. `RegisterServiceTest` and `RegistersControllerTest` updated for the new constructor + delegation pattern. - Docs: new `docs/development/Services/serializers.md`, CHANGELOG entry (Added + Breaking). Quality - PHPCS / Psalm / PHPStan clean on all touched files - Unit suite for affected files: 120/120 green inside the dev container - Integration tests: 4/4 green Follow-up - DocuDesk follow-up issue filed at ConductionNL/docudesk#120 — opens once openregister cuts a release containing this change. Closes openregister#1428

Five targeted fixes from WilcoLouwerse's review: 1. Test namespace: tests/Unit/Service/Serializer/RegisterSerializerTest.php declared `namespace Unit\Service\Serializer;` which the project bootstrap does not resolve. The 10 new unit tests therefore did not execute in CI. Corrected to `OCA\OpenRegister\Tests\Unit\Service\Serializer` to match the rest of the test suite and the bootstrap's namespace-to-path map. 2. Exception coverage in RegisterSerializer::expandSchemas(): catch MultipleObjectsReturnedException alongside DoesNotExistException so an ambiguous schema record is treated like an orphan ID (logged + retained in place) instead of propagating a 500 to the controller. 3. Consistent typing in RegisterSerializer::serializeMany(): cast $register->getId() to int so the key type matches what RegisterService::findAllSerialized() stores. PHP's array-key coercion already handled the mismatch, but the explicit cast makes the contract obvious and prevents drift if the entity layer ever widens getId(). 4. N+1 query characteristic documented in two places: - RegisterService::findAllSerialized() docblock notes that `@self.stats` alongside `schemas` triggers one getSchemaObjectCounts() query per register. - docs/development/Services/serializers.md gains a "Performance note" section pointing DI callers at findAll() when finer control is needed. 5. New controller unit test `testIndexForwardsExtendValueToFindAllSerialized` asserts that the `_extend` value parsed from the request is forwarded verbatim to the service. The other index tests use `$this->anything()` for `_extend`, which would not catch a regression that drops or mutates the value before the service call. Pushed back on blocker 2 (null guard) and blocker 3 (type-key mismatch) in the GitHub replies: Register::jsonSerialize() guarantees an array via array_filter, and PHP's array-key coercion already handles numeric-string keys. Wilco's suggested code change for blocker 3 is applied for clarity; the underlying claim is not a runtime bug. Static analysis on touched lib files: PHPCS 0 errors / 5 pre-existing warnings, PHPStan 0 errors, Psalm 0 errors.

- EntityOrganisationAssignmentTest: add RegisterSerializer to RegisterService ctor (7th arg added by PR 1430). - RegisterSerializerTest: assert new warning message 'Schema unresolvable for expansion' (production was updated). Note: 28 'Unknown named parameter $_extend' errors remain — these trace to a production-code bug in lib/Service/Configuration/ImportHandler.php where line 645 still calls registerMapper->find(_extend: []) but PR 1430 removed _extend from RegisterMapper::find()'s signature. Not fixed here because per agent instructions production code is off-limits.

The PR's review-feedback commit (41b6bad) removed `_extend` from `RegisterMapper::find()` / `findAll()` signatures as a no-op param, but missed this call site in ImportHandler. Result: 24 unit-test errors + 4 expectException failures all stack-tracing to ImportHandler.php:645 with "Unknown named parameter \$_extend". The companion call at line 3138 uses `schemaMapper->find(_extend: [])` which is fine — SchemaMapper still has the param. Verified locally: ImportHandler tests 231/231 pass, full PHPUnit suite 12,226 tests / 25,949 assertions / 0 errors / 0 failures.

This PR's review-feedback commit (41b6bad) removed `\$_extend` from `RegisterMapper::find()` / `findAll()` as a no-op param. But one positional call site in MagicMapper at line 6426 still passed the old 5-arg shape `(id, [], null, true, false)` — the `[]` (former `\$_extend`) now lands on `\$published: ?bool` and triggers a TypeError at runtime. That broke `POST /api/objects/{register}/{schema}/{id}/files` (Advanced File Operations Newman tests 1-6) which routes through MagicMapper's `findRegisterAndSchemaForEntity` helper. File 1 returned 500; tests 2-6 cascaded as 404. Switched the call to named arguments matching the new signature: \$this->registerMapper->find(id: (int) \$entity->getRegister(), _multitenancy: false); Verified locally: file POST against /api/objects/.../files now returns HTTP 200 with the created file row.

WilcoLouwerse · 2026-05-19T14:59:21Z

🟡 CI heads-up — deploy / Build and validate is red on pre-existing issues, not on this PR's code

The failing job is the docs build. The two failures are in files this PR doesn't touch:

docs/Patterns/collaborative-editing.md:111 — MDX compilation fails on an unclosed <expiry> tag (it's being parsed as a JSX element).
docs/src/pages/index.js:238 — SyntaxError: a JSX comment {/* … */} placed where a spread operator is expected.

Neither file appears in this PR's diff. The check is not marked required (Newman API Test Suite passes), so this isn't a blocker for this PR — but it's worth knowing the red ❌ is unrelated. Whoever owns the documentation base branch will need to fix those two files in a separate PR.

WilcoLouwerse

No blockers. All three 🔴 prior findings fixed; pushback on the TypeError concern verified correct against Register::jsonSerialize()'s upstream contract.

One 🟡 remains: please file (or link) the softwarecatalog cross-repo tracking issue in that thread before merge — task 7.1 is checkmarked but the audit is post-PR work that needs a real follow-up. The failing deploy / Build and validate check is on pre-existing docs MDX issues unrelated to this PR.

…ng issue per PR #1430 review Resolves the outstanding 🟡 from Wilco's approval of PR #1430: > One 🟡 remains: please file (or link) the softwarecatalog cross-repo > tracking issue in that thread before merge — task 7.1 is checkmarked > but the audit is post-PR work that needs a real follow-up. Filed against the softwarecatalog repo as ConductionNL/softwarecatalog#272 with the full wire-format breaking- change context: orphan schema IDs now appear in the `schemas` array as bare integers alongside the resolved Schema objects (was: silently dropped). Statically-typed clients decoding `schemas` as a homogeneous `[]Schema` need to switch to a discriminated decoder. Linked from tasks.md task 7.1 next to the existing DocuDesk follow-up (`ConductionNL/docudesk#120`) so both downstream audits live in the same place.

rjzondervan requested review from Rem-Dam, WilcoLouwerse, bbrands02 and rubenvdlinde as code owners May 5, 2026 12:21

This was referenced May 5, 2026

Switch RegisterDiscoveryService to RegisterService::findAllSerialized (depends on openregister#1428) ConductionNL/docudesk#120

Open

fix: switch RegisterDiscoveryService to findAllSerialized to populate schema dropdown (#120) ConductionNL/docudesk#121

Merged

WilcoLouwerse mentioned this pull request May 7, 2026

Openstaande OpenRegister PR's verwerken ConductionNL/.github#42

Open

22 tasks