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

fix: resolve HTTP 500 on object delete#1331

Merged
bbrands02 merged 5 commits into
developmentfrom
fix/delete-object
Apr 23, 2026
Merged

fix: resolve HTTP 500 on object delete#1331
bbrands02 merged 5 commits into
developmentfrom
fix/delete-object

Conversation

@bbrands02

@bbrands02 bbrands02 commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • updateObjectEntity missing ensureTableForRegisterSchema call — the soft-delete path (which is the default for all API DELETE requests) updates the dynamic table without first checking for missing columns. Columns added to getMetadataColumns() after a table was created (e.g. _tmlo) caused SQLSTATE[42S22]: Unknown column '_tmlo' in 'SET'. Added the same ensureTableForRegisterSchema call that insertObjectEntity already has.
  • array_flip() on column map in MagicTableHandlergetExistingTableColumns() returns a map keyed by column name; array_flip() silently dropped all entries because the values are arrays, not strings. This made array_diff_key report every column as missing and triggered a spurious full table sync on every single request. Removed the erroneous array_flip().
  • ISO 8601 datetime strings rejected by MySQL — schema properties with format: date-time or date were passed raw (e.g. 2024-03-21T10:15:30+00:00) to MySQL DATETIME columns, which only accept Y-m-d H:i:s. Applied DateTimeNormalizer::formatForDatabase() in prepareObjectDataForTable for these property types, matching the normalisation already in place for system metadata fields (created, updated, expires).

Test plan

  • DELETE an object whose dynamic table was created before _tmlo was added — should return 200, not 500
  • Verify no array_flip PHP warnings in nextcloud.log during any object operation
  • DELETE an object that has a schema property with format: date-time containing an ISO 8601 value — should soft-delete cleanly
  • Confirm ensureTableForRegisterSchema is a no-op (cache hit) on subsequent updates once the table is already up to date

Implementation plan: column-verified cache

Problem

ensureTableForRegisterSchema currently hits information_schema on every object
write (insert, update, soft-delete) to verify the dynamic table has all required
columns. This query is a no-op in the happy path but runs unconditionally.

Solution

Add a static boolean cache ($tableColumnsVerifiedCache) to MagicMapper that
records whether a given register+schema table has been verified in the current
PHP process. Once verified, subsequent requests skip the information_schema
query entirely.

Design decisions

  • Boolean flag, not full column list — a cache miss still runs one
    information_schema query, which is acceptable since misses are rare (first
    request after deploy or schema change). Caching the full column list would add
    complexity to keep it in sync after ALTERs without meaningful gain.
  • Lazy table sync is kept as-is — schema saves do not trigger an immediate
    ALTER TABLE. The sync happens on the next object write via
    ensureTableForRegisterSchema. hasRegisterSchemaChanged() already detects
    the version mismatch and triggers sync correctly, so no hook into schema save
    is needed.

Changes

MagicMapper.php

  • Added static $tableColumnsVerifiedCache = [] alongside existing static caches
  • Added setTableColumnsVerified(string $cacheKey) static accessor
  • Added isTableColumnsVerified(string $cacheKey): bool static accessor
  • Updated invalidateTableCache($cacheKey) to also unset the verified flag
  • Updated clearAllStaticCaches() to also clear $tableColumnsVerifiedCache

MagicMapper/MagicTableHandler.php

  • ensureTableForRegisterSchema: returns immediately if verified flag is set,
    skipping buildTableColumnsFromSchema + getExistingTableColumns entirely.
    Sets flag to true after a passing sanity check.
  • syncTableForRegisterSchema: sets flag to true after a successful sync.
    Covers both the automatic path (via ensureTableForRegisterSchema) and the
    manual path (via TablesController frontend sync button).

Invalidation triggers

Event Action
Schema changed (version hash mismatch on next write) syncTableForRegisterSchema runs, sets flag to true with new state
Table synced via frontend button syncTableForRegisterSchema runs, sets flag to true
Table dropped or force-recreated invalidateTableCache clears flag (already happened)
App update / migration clearAllStaticCaches clears all flags (already happened)

Status

✅ Implemented and tested — delete returns 204 with zero MagicTableHandler
warnings in logs.

🤖 Generated with Claude Code

remko48 and others added 3 commits April 22, 2026 14:38
- updateObjectEntity never called ensureTableForRegisterSchema, so
  columns added to getMetadataColumns() after a table was created (e.g.
  _tmlo) were missing when a soft-delete UPDATE ran against older tables.
  Added the same ensure call that insertObjectEntity already has.

- array_diff_key in MagicTableHandler used array_flip() on the result of
  getExistingTableColumns(), which returns a column-name-keyed map, not a
  flat list. The flip silently dropped every entry, making every column
  appear missing and triggering a spurious full sync on every request.
  Removed the erroneous array_flip().

- Schema properties with format date-time/date were written to MySQL
  DATETIME columns as raw ISO 8601 strings (e.g. 2024-03-21T10:15:30+00:00)
  which MySQL rejects. Applied DateTimeNormalizer::formatForDatabase() in
  prepareObjectDataForTable for these property types, matching the
  normalisation already in place for system metadata fields.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bbrands02

Copy link
Copy Markdown
Contributor Author

All three fixes are sound. The only actionable follow-up is caching getExistingTableColumns results to avoid the information_schema hit on every update.

How big of a problem is it to ignore that right now?

Not a problem right now. information_schema.columns is a fast metadata query — no row data involved, and MySQL/MariaDB keeps it in memory. On a dev instance with a handful of schemas it'll be sub-millisecond. It only becomes noticeable at scale: many concurrent users, high update frequency, or dozens of dynamic tables.

Worth a ticket for later but not blocking this PR.

Comment thread lib/Db/MagicMapper.php Outdated
Comment thread lib/Db/MagicMapper/MagicTableHandler.php
WilcoLouwerse
WilcoLouwerse previously approved these changes Apr 23, 2026
…ema queries

Added $tableColumnsVerifiedCache static property to MagicMapper that records
whether a given register+schema table's columns have been verified in the
current PHP process. Once verified, ensureTableForRegisterSchema returns
immediately without hitting information_schema on subsequent writes.

- Fast path in ensureTableForRegisterSchema: if verified flag is set, skip
  buildTableColumnsFromSchema + getExistingTableColumns entirely
- Flag is set to true after a passing sanity check (no missing columns)
- Flag is set to true after a successful syncTableForRegisterSchema, covering
  both the automatic path and the manual frontend sync button (TablesController)
- Flag is cleared in invalidateTableCache() alongside existing cache entries
- Flag is cleared in clearAllStaticCaches() alongside existing caches
- Added setTableColumnsVerified() and isTableColumnsVerified() static accessors

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 1cf759f

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

Quality workflow — 2026-04-23 11:15 UTC

Download the full PDF report from the workflow artifacts.

…tion

DateTime misses DateTimeImmutable — if upstream passes a DateTimeImmutable
the value falls through both branches and reaches MySQL unformatted. Using
DateTimeInterface covers both concrete types.

Fixed in two locations:
- System metadata fields (created/updated/expires) around the existing
  normalisation block
- Schema property date-time/date fields added in the previous commit

Also adds MagicTableHandlerTest covering the array_flip regression and the
$tableColumnsVerifiedCache fast path, as suggested in code review.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 4ebb271

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

Quality workflow — 2026-04-23 11:21 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 2d535d4

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

Quality workflow — 2026-04-23 11:28 UTC

Download the full PDF report from the workflow artifacts.

@WilcoLouwerse

Copy link
Copy Markdown
Contributor

Nog kleine nitpick opmerking: je Test plan checklist is niet afgevinkt in de beschrijving

@bbrands02 bbrands02 merged commit 60998e2 into development Apr 23, 2026
16 of 21 checks passed
@bbrands02 bbrands02 deleted the fix/delete-object branch April 23, 2026 11:31
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.

4 participants