Skip to content
This repository was archived by the owner on May 29, 2026. It is now read-only.

feat!: opt-in @self.files via _extend on rendered objects#1369

Merged
rjzondervan merged 2 commits into
developmentfrom
feature/1358/extend-files
May 1, 2026
Merged

feat!: opt-in @self.files via _extend on rendered objects#1369
rjzondervan merged 2 commits into
developmentfrom
feature/1358/extend-files

Conversation

@rjzondervan

Copy link
Copy Markdown
Member

BREAKING CHANGE: @self.files no longer contains full file metadata by
default. Default is now a lightweight list of integer file IDs (e.g.
[123, 456, 789]). Add _extend[]=@self.files (or the equivalent shorthand
_extend[]=_files) to receive the full metadata payload. Migration is one
query parameter on the consumer side.

Why: today, show endpoints that route through RenderObject::renderEntity
unconditionally pay the FileMapper + SystemTag lookup cost on every
response (even when the consumer never uses the file information), while
list endpoints that bypass renderEntity emit no @self.files field at
all. Consumers see different shapes across endpoints and cannot opt in
or out. The new contract is symmetric and explicit, and the lightweight
default is strictly cheaper than today's show behavior.

What changed

  • FileMapper::getFileIdsForObjects(string[] $uuids): new batched lookup
    (two queries regardless of input size) backing the lightweight default.
  • RenderObject::renderEntity now gates renderFiles() on whether _extend
    covers @self.files. Body of renderFiles() is unchanged. When extend is
    absent, setLightweightFileIds() attaches the integer ID list, reusing a
    transient batch cache populated by renderEntities() so list pages do not
    go N+1.
  • normalizeMap gains '_files' => '@self.files' so both spellings are
    equivalent, mirroring the existing _schema/_register normalization.
  • QueryHandler::searchObjectsPaginatedDatabase enriches cheap-path rows
    (no extend / fields / filter / propAuth) with lightweight IDs via a
    single batched query before returning. Complex-rendering rows inherit
    the new behavior automatically through renderEntities.
  • ObjectService::searchObjectsPaginated applies the same enrichment on
    the SOLR/index branch. (SOLR + _extend[]=@self.files falls back to IDs;
    pre-existing limitation that the SOLR path does not route rows through
    renderEntities, documented inline.)
  • attachLightweightFilesToRows() handles both ObjectEntity and array row
    shapes for tolerance across pipelines.

Performance

  • Default show: strictly cheaper (no file metadata work).
  • Default list: +1 batched query per request, regardless of page size.
  • Extend on show: unchanged.
  • Extend on list: per-row file/tag lookups; documented as heavily
    discouraged with a verbatim warning in docs/api/objects.md.

Tests

  • Unit tests for FileMapper::getFileIdsForObjects input validation.
  • Integration tests deferred to a follow-up (Newman fixture with
    attachments).

Out of scope: renderFileProperties (schema-property file hydration),
PublicationsController::attachments() (separate explicit endpoint), and
opencatalogi documentation (docs-only follow-up against opencatalogi).

OpenSpec change: openspec/changes/opt-in-files-extend
Refs #1358

A few options if you want it tighter:

  • One-liner subject only (matches several of your recent commits): feat!: opt-in @self.files via _extend
    on rendered objects
  • Drop the ! and use the feat: prefix only if you don't follow conventional-commits strictly here — the
    BREAKING CHANGE: block in the body still surfaces the semver impact.
  • Trim the body to just the BREAKING CHANGE block + Refs line if you prefer pull-request descriptions to
    carry the detail and commit messages to stay terse.

Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com

  BREAKING CHANGE: @self.files no longer contains full file metadata by
  default. Default is now a lightweight list of integer file IDs (e.g.
  [123, 456, 789]). Add _extend[]=@self.files (or the equivalent shorthand
  _extend[]=_files) to receive the full metadata payload. Migration is one
  query parameter on the consumer side.

  Why: today, show endpoints that route through RenderObject::renderEntity
  unconditionally pay the FileMapper + SystemTag lookup cost on every
  response (even when the consumer never uses the file information), while
  list endpoints that bypass renderEntity emit no @self.files field at
  all. Consumers see different shapes across endpoints and cannot opt in
  or out. The new contract is symmetric and explicit, and the lightweight
  default is strictly cheaper than today's show behavior.

  What changed
  - FileMapper::getFileIdsForObjects(string[] $uuids): new batched lookup
    (two queries regardless of input size) backing the lightweight default.
  - RenderObject::renderEntity now gates renderFiles() on whether _extend
    covers @self.files. Body of renderFiles() is unchanged. When extend is
    absent, setLightweightFileIds() attaches the integer ID list, reusing a
    transient batch cache populated by renderEntities() so list pages do not
    go N+1.
  - normalizeMap gains '_files' => '@self.files' so both spellings are
    equivalent, mirroring the existing _schema/_register normalization.
  - QueryHandler::searchObjectsPaginatedDatabase enriches cheap-path rows
    (no extend / fields / filter / propAuth) with lightweight IDs via a
    single batched query before returning. Complex-rendering rows inherit
    the new behavior automatically through renderEntities.
  - ObjectService::searchObjectsPaginated applies the same enrichment on
    the SOLR/index branch. (SOLR + _extend[]=@self.files falls back to IDs;
    pre-existing limitation that the SOLR path does not route rows through
    renderEntities, documented inline.)
  - attachLightweightFilesToRows() handles both ObjectEntity and array row
    shapes for tolerance across pipelines.

  Performance
  - Default show: strictly cheaper (no file metadata work).
  - Default list: +1 batched query per request, regardless of page size.
  - Extend on show: unchanged.
  - Extend on list: per-row file/tag lookups; documented as heavily
    discouraged with a verbatim warning in docs/api/objects.md.

  Tests
  - Unit tests for FileMapper::getFileIdsForObjects input validation.
  - Integration tests deferred to a follow-up (Newman fixture with
    attachments).

  Out of scope: renderFileProperties (schema-property file hydration),
  PublicationsController::attachments() (separate explicit endpoint), and
  opencatalogi documentation (docs-only follow-up against opencatalogi).

  OpenSpec change: openspec/changes/opt-in-files-extend
  Refs #1358

  A few options if you want it tighter:

  - One-liner subject only (matches several of your recent commits): feat!: opt-in @self.files via _extend
  on rendered objects
  - Drop the ! and use the feat: prefix only if you don't follow conventional-commits strictly here — the
  BREAKING CHANGE: block in the body still surfaces the semver impact.
  - Trim the body to just the BREAKING CHANGE block + Refs line if you prefer pull-request descriptions to
  carry the detail and commit messages to stay terse.

 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comment thread lib/Service/Object/RenderObject.php Outdated
Comment thread lib/Service/ObjectService.php
Comment thread lib/Service/Object/RenderObject.php
Comment thread lib/Service/Object/RenderObject.php Outdated
Comment thread tests/Unit/Db/FileMapperGetFileIdsForObjectsTest.php
Comment thread CHANGELOG.md Outdated

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 blocker requires a fix before merge: batchFileIdsCache cache-miss returns [] for linked sub-entities, causing silent data corruption on any list+relation-extend request. Architecture and batching approach are sound; also flagged: SOLR path ignores _extend[]=@self.files without signal, zero automated test coverage of the renderEntity gate, and undocumented all token in shouldExtendFiles.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 5a3a7ad

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-04-30 14:48 UTC

Download the full PDF report from the workflow artifacts.

Blockers / concerns from @WilcoLouwerse 2026-04-30:

- 🔴 setLightweightFileIds cache-miss returned [] for sub-entities
  (RenderObject.php). The batchFileIdsCache is populated by
  renderEntities() with TOP-LEVEL page UUIDs only; sub-entities reached
  via relation extends are absent from it. Previous `?? []` short-circuit
  silently emitted `@self.files: []` on every linked sub-object that
  had attached files. Fix: use `array_key_exists` to distinguish "in
  the batch with zero files" from "not in the batch at all"; on miss,
  fall through to a single-UUID FileMapper lookup. Top-level N+1
  prevention is preserved.

- 🟡 'all' token removed from shouldExtendFiles. `_extend[]=all`
  propagates to sub-entity renders via array_merge(['all'], $keyExtends),
  which would silently pay full file-metadata cost on every linked
  sub-object whenever a top-level request used the wildcard. The
  proposal pins @self.files as a strict explicit opt-in via
  @self.files / _files only — the wildcard exception was not in spec.

- 🟡 SOLR-path extend_unsupported signal added (ObjectService.php).
  `_extend[]=@self.files` (or `_files`) on a SOLR-backed list endpoint
  cannot deliver full metadata (the SOLR path does not route rows
  through renderEntities). Response now carries
  `@self.extend_unsupported: ['@self.files']` so consumers can detect
  the shape mismatch programmatically. Also documented in CHANGELOG.

- 🟡 Render-contract gate tests added (RenderObjectTest.php). Five
  new test methods cover (a) shouldExtendFiles dispatch including the
  intentional 'all' rejection, (b) lightweight default fires
  getFileIdsForObjects but NOT getFilesForObject when extend is empty,
  (c) explicit opt-in routes through renderFiles, (d) cache-miss
  fall-through (regression test for the blocker), (e) cache-hit
  preserves cached value (no spurious FileMapper calls). The five
  pre-existing testRenderFiles* methods were updated to pass
  `_extend: ['@self.files']` — they exercise the heavy renderFiles
  path and were silently going through the lightweight path after the
  gate landed. Was the source of the PHPUnit ❌ in CI.

- 🟢 FileMapperGetFileIdsForObjectsTest: deliberate off-contract input
  test annotated with @psalm-suppress InvalidArgument so static
  analysis doesn't flag the contract-violating call site. The guard
  is intentional defence against sloppy callers.

- 🟢 CHANGELOG: SOLR limitation explicitly mentioned alongside the
  perf-discouragement note for list+extend.

Quality: 312/312 tests pass in RenderObjectTest, +5 new gate tests.
PHPCS clean on touched lib files. php -l clean on all touched files.
@rjzondervan

Copy link
Copy Markdown
Member Author

@WilcoLouwerse — review fixes pushed in 2feace9. Mapping fixes to your items:

Blocker

  • 🔴 setLightweightFileIds cache-miss returning [] for sub-entities (RenderObject.php:482)
    Fixed exactly as you suggested — switched the ?? [] short-circuit to an array_key_exists check, falling through to a single-UUID getFileIdsForObjects lookup on a cache miss. Top-level N+1 prevention is preserved (top-level UUIDs are in the cache → cache hit → no fall-through), and sub-entities reached via relation extends now correctly populate @self.files instead of silently emitting []. Regression test landed (testSetLightweightFileIdsCacheMissFallsThroughToSingleLookup).

Concerns

  • 🟡 shouldExtendFiles recognising the undocumented all token (RenderObject.php:454)
    Went with your option (a): removed 'all' from shouldExtendFiles. The proposal pins @self.files as a strict explicit opt-in (canonical @self.files + shorthand _files) and the wildcard token was not spec'd. Crucially, the array_merge(['all'], $keyExtends) propagation would have silently paid full file-metadata cost on every linked sub-object whenever a consumer used _extend[]=all — a real perf footgun. Documented the rejection in the helper's docblock and updated tasks.md §2.2. Test testShouldExtendFilesRecognizesCanonicalAndShorthand pins this behaviour.

  • 🟡 SOLR path silently ignoring _extend[]=@self.files (ObjectService.php:1980)
    Both your suggested mitigations applied: (1) machine-readable signal — when @self.files (or _files) is in _extend on a SOLR-backed list response, the response carries @self.extend_unsupported: ['@self.files'] so consumers detect the shape mismatch programmatically; (2) CHANGELOG entry explicitly mentions the SOLR limitation alongside the existing perf-discouragement note.

  • 🟡 Render-contract gate has zero automated coverage (RenderObject.php:482)
    Added five new test methods to RenderObjectTest.php:

    1. testShouldExtendFilesRecognizesCanonicalAndShorthand — gates @self.files / _files opt-in and asserts 'all' is rejected.
    2. testRenderEntityEmitsLightweightFileIdsWhenExtendOmitsFiles — proves getFileIdsForObjects IS called and getFilesForObject is NOT, when extend is empty.
    3. testRenderEntityRoutesThroughRenderFilesWhenExtendCoversFiles — proves getFilesForObject IS called and getFileIdsForObjects is NOT, when _extend[]=@self.files is set.
    4. testSetLightweightFileIdsCacheMissFallsThroughToSingleLookup — pins the blocker fix above.
    5. testSetLightweightFileIdsCacheHitReturnsCachedValue — pins the cache-hit path; getFileIdsForObjects is never called when the UUID is in the batch (including the empty-array case).

    Also fixed five pre-existing testRenderFiles* tests that were calling renderEntity($entity) without extend and asserting full-metadata file shapes — they had been silently failing in CI since the gate landed. They now correctly call renderEntity(entity: $entity, _extend: ['@self.files']). This is what was causing the PHPUnit ❌ in the quality report.

    Integration tests (4.4–4.6 in tasks.md) remain deferred — they need running stack + seed data with attachments. The unit-level gate is now protected.

Minors

  • 🟢 Type-contract violation in FileMapperGetFileIdsForObjectsTest:105 — annotated the deliberate off-contract call with @psalm-suppress InvalidArgument and documented the intent. Restricting input to [''] alone would lose coverage of the non-string paths (int, null, bool) — the guard exists exactly to defend against those, and that defence deserves coverage.

  • 🟢 CHANGELOG omitting SOLR limitation — added a sentence at the end of the breaking-change entry covering both the limitation and the new @self.extend_unsupported signal.

Quality

  • php -l clean on all touched files.
  • PHPCS clean on touched lib/ files.
  • RenderObjectTest: 312/312 tests pass (was 5/312 failing in CI). 5 new gate tests added.
  • FileMapperGetFileIdsForObjectsTest: still passes.

CI re-run should turn the PHPUnit ❌ into ✅ on the next quality workflow.

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All 6 prior findings addressed in 2feace9 (1 🔴, 3 🟡, 2 🟢) — fixes verified against the diff and pinned by new regression tests. The cache-miss fall-through, SOLR extend_unsupported signal, and 5-test gate suite all match what was suggested.

Note (non-blocking): Newman CI is failing due to pre-existing infrastructure issues (Solr host resolution + missing OPENAI_API_KEY) — confirmed also failing on development HEAD, not introduced by this PR. PHPUnit jobs were cancelled in this run; a re-run should turn the suite green now that the 5 testRenderFiles* tests have been corrected.

@rjzondervan rjzondervan merged commit b948f67 into development May 1, 2026
17 of 21 checks passed
@rjzondervan rjzondervan deleted the feature/1358/extend-files branch May 1, 2026 09:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants