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

feat(schema): add appendOnly schema flag (closes scholiq deps #1)#1482

Merged
1 commit merged into
developmentfrom
feat/scholiq-deps/append-only
May 15, 2026
Merged

feat(schema): add appendOnly schema flag (closes scholiq deps #1)#1482
1 commit merged into
developmentfrom
feat/scholiq-deps/append-only

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Summary

  • Adds appendOnly: true top-level schema flag. When set, INSERT (new objects) is permitted but UPDATE and DELETE are rejected with HTTP 405 + structured error SCHEMA_APPEND_ONLY.
  • Implements DB migration (Version1Date20260511100000) adding the append_only boolean column (default false, backward-compatible).
  • New AppendOnlyException class with toResponseBody() returning { error, message, schema, operation }.
  • Enforcement in ObjectService::saveObject() (UUID present = update path) and ObjectService::deleteObject().
  • ObjectsController handles AppendOnlyException in update(), patch(), postPatch(), and destroy() with HTTP 405.
  • 7 PHPUnit tests: insert allowed, update rejected, delete rejected, ordinary schema unchanged, exception response body structure, Schema entity getter.

Closes #1470 (scholiq deps: XapiStatement + Attestation schemas need audit-log immutability).

Test plan

  • composer check:strict passes (PHPCS clean on all lib/ files)
  • PHPUnit: AppendOnlyTest — 7 tests pass in Docker environment
  • Manual: create object on appendOnly schema → 201; update → 405 SCHEMA_APPEND_ONLY; delete → 405
  • Schemas without appendOnly flag are completely unaffected

Schemas with appendOnly: true permit INSERT operations but reject
UPDATE and DELETE with HTTP 405 + structured error SCHEMA_APPEND_ONLY.

Changes:
- Schema entity: appendOnly boolean field, DB type registration,
  isAppendOnly() accessor, jsonSerialize() output
- Migration Version1Date20260511100000: adds append_only column (default false)
- AppendOnlyException: structured 405 exception with toResponseBody()
- ObjectService::saveObject(): rejects when UUID present + schema isAppendOnly
- ObjectService::deleteObject(): rejects when schema isAppendOnly
- ObjectsController: catches AppendOnlyException in update/patch/postPatch/destroy
  and returns HTTP 405 with SCHEMA_APPEND_ONLY error body
- AppendOnlyTest: 7 unit tests covering insert allowed, update rejected,
  delete rejected, ordinary schema unchanged, exception structure, entity getter
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 7788b96

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

Quality workflow — 2026-05-11 20:29 UTC

Download the full PDF report from the workflow artifacts.

@@ -0,0 +1,123 @@
<?php

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.

[BLOCKER] Missing SPDX-License-Identifier header (Gate 1)

Both lib/Exception/AppendOnlyException.php and lib/Migration/Version1Date20260511100000.php use a @license PHPDoc tag instead of the required // SPDX-License-Identifier: EUPL-1.2 comment. Compare with other recent files which carry the machine-readable SPDX identifier.

@@ -0,0 +1,88 @@
<?php

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.

[BLOCKER] Migration has no down() method — irreversible

SimpleMigrationStep supports down(). Without it the append_only column cannot be dropped during downgrade. All recent migrations in this codebase include a down(). Add public function down(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper that drops the column when present.

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.

[BLOCKER] deleteObjects() bulk path bypasses appendOnly guard entirely

ObjectService::deleteObjects() iterates UUIDs and calls $this->deleteHandler->deleteObject(...) directly, completely skipping ObjectService::deleteObject() and its new appendOnly check. An admin with bulk-delete access can wipe every record from an append-only schema. Add an appendOnly check at the top of deleteObjects() that throws AppendOnlyException before the foreach loop.

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.

[BLOCKER] saveObjects() bulk path bypasses appendOnly guard for UPDATE

ObjectService::saveObjects() delegates to SaveObjectsHandler::saveObjects() which calls MagicMapper bulk-upsert directly and handles create-vs-update internally. This path never calls ObjectService::saveObject() so never hits the appendOnly check. Any object with an existing UUID passed through bulk save will be silently updated on an append-only schema. Add an appendOnly guard at the start of saveObjects() that rejects update-intent objects.

@@ -2436,6 +2446,9 @@ public function destroy(string $id, string $register, string $schema, ObjectServ

// Return 204 No Content for successful delete (REST convention).

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.

[BLOCKER] AppendOnly rejection uses HTTP 405 — wrong semantics

The destroy catch returns Http::STATUS_METHOD_NOT_ALLOWED (405). REST semantics for 'this resource cannot be deleted because the schema is append-only' map to 409 Conflict or 403 Forbidden; 405 normally means 'this HTTP method is not supported on this endpoint at all' and will confuse clients and OpenAPI consumers. The exception's constructor hard-codes code: 405, coupling HTTP status to the domain model. Use 403 or 409 and document the choice.

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.

[CONCERN] appendOnly check placed after CascadingHandler

The appendOnly guard fires before handleCascadingWithContextPreservation(). If cascading sets a UUID on a previously null-UUID object (creating a ghost sub-object pointing to an existing record), the guard has already passed with $uuid === null. Worth a comment explaining the ordering is intentional, or move the guard after cascading.

@@ -0,0 +1,438 @@
<?php

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.

[CONCERN] PHPUnit failing in CI — coverage for rejection paths uncertain

CI shows PHPUnit failing on PHP 8.3 + 8.4. None of the visible tests cover: (a) bulk delete via deleteObjects(), (b) bulk save-update via saveObjects(), (c) toggling appendOnly from true to false mid-flight, (d) schema context being null when the guard fires. Add tests for these bypass paths.

Comment thread lib/Db/Schema.php

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.

[CONCERN] No interaction test between appendOnly and immutable flags

Schema has both $immutable and $appendOnly. With appendOnly: true and immutable: false, objects can't be mutated but the schema itself can still be updated to set appendOnly: false, re-enabling retroactive mutations. There is no guard, documentation, or test for this combined-state behaviour. If retroactive mutation is forbidden for compliance, a guard on the schema controller is needed.

@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.

Review

🔴 Blockers (5)

🟡 Concerns (3)

🟢 Minor (1)

  • Notes on NOT NULL boolean column on large tables (lib/Migration/Version1Date20260511100000.php:337)
    notnull: true, default: false maps to integer 0. Existing rows will be backfilled to 0 which is the desired behaviour and hasColumn guard makes the migration idempotent. Worth a comment noting default: false is backward-compatible for MySQL strict mode.

Reviewed by WilcoLouwerse via automated batch review.

@WilcoLouwerse WilcoLouwerse closed this pull request by merging all changes into development in f86e83f May 15, 2026
@WilcoLouwerse WilcoLouwerse deleted the feat/scholiq-deps/append-only branch May 15, 2026 09:40
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