mirror of
https://github.com/samsonjs/immich.git
synced 2026-03-25 09:15:56 +00:00
Add to-do list for non-destructive edits
This commit is contained in:
parent
ff7dca35f5
commit
b31418c569
1 changed files with 218 additions and 0 deletions
218
NON_DESTRUCTIVE_EDITS_MODEL_TODO.md
Normal file
218
NON_DESTRUCTIVE_EDITS_MODEL_TODO.md
Normal file
|
|
@ -0,0 +1,218 @@
|
|||
# Non-Destructive Edits: Model Layer To-Do
|
||||
|
||||
## Scope (Model Layer Only)
|
||||
- Build a durable schema for non-destructive edits that preserves originals and edit history.
|
||||
- Include only database/model work in `server/src/schema/*` (+ migration files).
|
||||
- Do not include controller, service, job orchestration, or UI changes in this pass.
|
||||
|
||||
## Execution Classification (Required)
|
||||
- `[TDD]` means red/green automated verification is expected (migration/integration/schema tests).
|
||||
- `[HUMAN]` means this cannot be fully confirmed functionally via automated tests and requires manual human completion/sign-off.
|
||||
- Policy: every `[HUMAN]` checkbox must be completed by a human before this work is considered done.
|
||||
|
||||
## Current Baseline (Already in Repo)
|
||||
- `asset_edit` stores per-asset edit operations (`action`, `parameters`, `sequence`).
|
||||
- `asset.isEdited` is maintained by DB triggers (`asset_edit_insert`, `asset_edit_delete`).
|
||||
- `asset_file` stores separate edited renditions using `isEdited` and unique (`assetId`, `type`, `isEdited`).
|
||||
|
||||
## Why Model Work Is Still Needed
|
||||
- Edit history is currently destructive: replacing edits deletes prior `asset_edit` rows.
|
||||
- There is no first-class revision entity for versioning, rollback lineage, or concurrency control.
|
||||
- Edited files are not explicitly tied to a specific edit revision.
|
||||
- Some important invariants are only enforced in application code (not at DB level).
|
||||
|
||||
## Target Model (End State)
|
||||
- Add `asset_edit_revision` table as the source of truth for edit versions per asset.
|
||||
- Keep `asset_edit` as operation rows, but attach each row to a revision (`revisionId`).
|
||||
- Add `asset.currentEditRevisionId` so active edits are explicit and fast to query.
|
||||
- Add `asset_file.editRevisionId` to link derived files to the exact revision they were generated from.
|
||||
- Keep compatibility columns (`asset.isEdited`, `asset_file.isEdited`) during transition; drive them from triggers.
|
||||
|
||||
## Required Data Invariants
|
||||
- Original file path/checksum in `asset` remains immutable by edit operations.
|
||||
- At most one active revision per asset at a time.
|
||||
- `asset.currentEditRevisionId` must reference a revision belonging to the same `asset.id`.
|
||||
- Revision operation order must be deterministic and gapless from `sequence = 0`.
|
||||
- Edited files (`asset_file`) must reference a valid revision for the same asset.
|
||||
- `asset.isEdited = true` iff `asset.currentEditRevisionId IS NOT NULL` (during compatibility phase).
|
||||
|
||||
## Implementation To-Do
|
||||
|
||||
### 1) Schema Contracts and Naming Freeze
|
||||
- [ ] [HUMAN] Finalize canonical names:
|
||||
- [ ] [HUMAN] `asset_edit_revision` (new table)
|
||||
- [ ] [HUMAN] `asset.currentEditRevisionId` (new nullable FK)
|
||||
- [ ] [HUMAN] `asset_file.editRevisionId` (new nullable FK)
|
||||
- [ ] [HUMAN] `asset_edit.revisionId` (new nullable -> then required FK)
|
||||
- [ ] [HUMAN] Decide lifecycle states for revisions (recommended: `pending`, `active`, `archived`, `failed`).
|
||||
- [ ] [HUMAN] Write a short ADR note in this file before coding migrations (1-2 paragraphs).
|
||||
|
||||
### 2) Add New Schema Objects
|
||||
- [ ] [TDD] Update `server/src/schema/enums.ts`:
|
||||
- [ ] [TDD] Add revision-state enum registration.
|
||||
- [ ] [TDD] Add `server/src/schema/tables/asset-edit-revision.table.ts`:
|
||||
- [ ] [TDD] `id` (uuid PK)
|
||||
- [ ] [TDD] `assetId` (FK -> `asset.id`, cascade on delete/update)
|
||||
- [ ] [TDD] `createdAt` (timestamp)
|
||||
- [ ] [TDD] `createdById` (nullable FK -> `user.id`, set null on delete)
|
||||
- [ ] [TDD] `state` (enum, default `pending`)
|
||||
- [ ] [TDD] `activatedAt` (nullable timestamp)
|
||||
- [ ] [TDD] `supersededAt` (nullable timestamp)
|
||||
- [ ] [TDD] `sourceWidth` / `sourceHeight` (nullable int, optional but recommended)
|
||||
- [ ] [TDD] `outputWidth` / `outputHeight` (nullable int, optional but recommended)
|
||||
- [ ] [TDD] `operationCount` (int default `0`)
|
||||
- [ ] [TDD] `hash` (nullable text for dedupe/idempotency, optional)
|
||||
- [ ] [TDD] Update `server/src/schema/tables/asset-edit.table.ts`:
|
||||
- [ ] [TDD] Add `revisionId` FK -> `asset_edit_revision.id` (nullable during transition).
|
||||
- [ ] [TDD] Keep `assetId` during transition for easier backfill; remove later only after service refactor.
|
||||
- [ ] [TDD] Keep unique (`assetId`, `sequence`) for compatibility phase.
|
||||
- [ ] [TDD] Add unique (`revisionId`, `sequence`) as new canonical order key.
|
||||
- [ ] [TDD] Update `server/src/schema/tables/asset.table.ts`:
|
||||
- [ ] [TDD] Add nullable FK `currentEditRevisionId` -> `asset_edit_revision.id` (`SET NULL` on delete).
|
||||
- [ ] [TDD] Keep `isEdited` column for backward compatibility.
|
||||
- [ ] [TDD] Update `server/src/schema/tables/asset-file.table.ts`:
|
||||
- [ ] [TDD] Add nullable FK `editRevisionId` -> `asset_edit_revision.id` (`CASCADE` update, `SET NULL` delete).
|
||||
- [ ] [TDD] Keep `isEdited` for compatibility.
|
||||
- [ ] [TDD] Update `server/src/schema/index.ts`:
|
||||
- [ ] [TDD] Register new table in `tables`.
|
||||
- [ ] [TDD] Add `asset_edit_revision` to `DB` interface.
|
||||
|
||||
### 3) DB-Level Constraints and Indexes
|
||||
- [ ] [TDD] Add constraints to `asset_edit_revision`:
|
||||
- [ ] [TDD] Check `operationCount >= 0`.
|
||||
- [ ] [TDD] Check timestamps are consistent (`activatedAt <= supersededAt` when both are present).
|
||||
- [ ] [TDD] Add partial unique index for one active revision per asset:
|
||||
- [ ] [TDD] unique (`assetId`) where `state = 'active'`.
|
||||
- [ ] [TDD] Add constraints to `asset_edit`:
|
||||
- [ ] [TDD] Check `sequence >= 0`.
|
||||
- [ ] [TDD] Check `jsonb_typeof(parameters) = 'object'`.
|
||||
- [ ] [TDD] Add deferred constraint trigger that enforces per-revision gapless sequence (`0..N-1`) at commit time.
|
||||
- [ ] [HUMAN] Decide whether action-specific parameter checks are too rigid for long-term evolution.
|
||||
- [ ] [TDD] If approved, add action-specific parameter shape checks (or trigger validation) for critical actions only.
|
||||
- [ ] [TDD] Add constraints to `asset_file`:
|
||||
- [ ] [TDD] Check `isEdited = (editRevisionId IS NOT NULL)` during compatibility phase.
|
||||
- [ ] [TDD] Keep existing uniqueness on (`assetId`, `type`, `isEdited`) until downstream code is updated.
|
||||
- [ ] [TDD] Add helpful indexes:
|
||||
- [ ] [TDD] `asset_edit_revision (assetId, state, createdAt desc)`
|
||||
- [ ] [TDD] `asset_edit (revisionId, sequence)`
|
||||
- [ ] [TDD] `asset_file (assetId, editRevisionId, type)`
|
||||
- [ ] [TDD] `asset (currentEditRevisionId)` (if not auto-indexed by FK tooling)
|
||||
|
||||
### 4) Trigger/Function Updates (Model-Side Logic)
|
||||
- [ ] [TDD] Update `server/src/schema/functions.ts` with revision-aware trigger functions:
|
||||
- [ ] [TDD] On revision activation, auto-supersede prior active revision for the same asset.
|
||||
- [ ] [TDD] Keep `asset.isEdited` synchronized from `asset.currentEditRevisionId`.
|
||||
- [ ] [TDD] Maintain `asset_edit_revision.operationCount` on `asset_edit` insert/delete.
|
||||
- [ ] [TDD] Validate `asset.currentEditRevisionId` belongs to the same asset (`asset.id`).
|
||||
- [ ] [TDD] Validate `asset_file.editRevisionId` belongs to the same asset (`asset_file.assetId`).
|
||||
- [ ] [TDD] Add write-compatibility trigger for legacy inserts:
|
||||
- [ ] [TDD] Before insert on `asset_edit`, if `revisionId` is null, assign from `asset.currentEditRevisionId`.
|
||||
- [ ] [TDD] If `asset.currentEditRevisionId` is null, create and activate a revision, then assign it.
|
||||
- [ ] [TDD] Preserve existing behavior for compatibility:
|
||||
- [ ] [TDD] `asset_edit_insert`/`asset_edit_delete` must continue to produce correct `asset.isEdited` while service layer catches up.
|
||||
- [ ] [TDD] Add migration override entries when function SQL is intentionally managed as overrides.
|
||||
|
||||
### 5) Migration Plan (Order Matters)
|
||||
- [ ] [TDD] Create migration `A`: add enum + `asset_edit_revision` + nullable new FK columns + indexes (no hard checks yet).
|
||||
- [ ] [TDD] Create migration `B`: backfill revisions from existing data:
|
||||
- [ ] [TDD] One `active` revision per asset that currently has rows in `asset_edit`.
|
||||
- [ ] [TDD] Populate `asset.currentEditRevisionId`.
|
||||
- [ ] [TDD] Populate `asset_edit.revisionId` by asset mapping.
|
||||
- [ ] [TDD] Populate `asset_file.editRevisionId` for rows where `asset_file.isEdited = true`.
|
||||
- [ ] [TDD] Set `operationCount`.
|
||||
- [ ] [TDD] Create migration `C`: add triggers/functions (including legacy `revisionId` assignment and cross-asset guards) and add constraints as `NOT VALID` where possible.
|
||||
- [ ] [TDD] Create migration `D`: validate constraints; make `asset_edit.revisionId` `NOT NULL` only after compatibility-trigger tests are green.
|
||||
- [ ] [TDD] Keep rollback safety:
|
||||
- [ ] [TDD] Each migration `down` should reverse schema and trigger changes without data loss for original asset files.
|
||||
- [ ] [HUMAN] Run rollout sequence on staging/prod in change window and sign off on operational safety.
|
||||
|
||||
### 6) Data Verification Queries (Run After Backfill)
|
||||
- [ ] [TDD] Add this SQL block to migration notes and assert all counts are zero in migration/integration tests.
|
||||
- [ ] [HUMAN] Run the same queries against staging/prod after migration and record results in rollout notes.
|
||||
|
||||
```sql
|
||||
-- 1) Edited flag consistency
|
||||
select count(*) as mismatches
|
||||
from asset
|
||||
where ("isEdited" = true and "currentEditRevisionId" is null)
|
||||
or ("isEdited" = false and "currentEditRevisionId" is not null);
|
||||
|
||||
-- 2) Current revision belongs to same asset
|
||||
select count(*) as bad_current_links
|
||||
from asset a
|
||||
join asset_edit_revision r on r.id = a."currentEditRevisionId"
|
||||
where r."assetId" <> a.id;
|
||||
|
||||
-- 3) Edited files are linked to a revision
|
||||
select count(*) as missing_revision_links
|
||||
from asset_file f
|
||||
where f."isEdited" = true and f."editRevisionId" is null;
|
||||
|
||||
-- 4) Edited files reference revision for same asset
|
||||
select count(*) as cross_asset_file_links
|
||||
from asset_file f
|
||||
join asset_edit_revision r on r.id = f."editRevisionId"
|
||||
where r."assetId" <> f."assetId";
|
||||
|
||||
-- 5) More than one active revision per asset
|
||||
select count(*) as multi_active_assets
|
||||
from (
|
||||
select "assetId"
|
||||
from asset_edit_revision
|
||||
where "state" = 'active'
|
||||
group by "assetId"
|
||||
having count(*) > 1
|
||||
) t;
|
||||
|
||||
-- 6) Operation ordering gaps (per revision)
|
||||
with ordered as (
|
||||
select "revisionId", "sequence",
|
||||
row_number() over (partition by "revisionId" order by "sequence") - 1 as expected
|
||||
from asset_edit
|
||||
)
|
||||
select count(*) as sequence_gaps
|
||||
from ordered
|
||||
where "sequence" <> expected;
|
||||
|
||||
-- 7) operationCount mismatch
|
||||
select count(*) as operation_count_mismatch
|
||||
from asset_edit_revision r
|
||||
left join (
|
||||
select "revisionId", count(*)::int as cnt
|
||||
from asset_edit
|
||||
group by "revisionId"
|
||||
) e on e."revisionId" = r.id
|
||||
where coalesce(e.cnt, 0) <> r."operationCount";
|
||||
```
|
||||
|
||||
### 7) Model-Focused Test Coverage
|
||||
- [ ] [TDD] Add/extend migration tests for:
|
||||
- [ ] [TDD] clean DB -> up migrations.
|
||||
- [ ] [TDD] existing edit data -> backfill correctness.
|
||||
- [ ] [TDD] down migration sanity.
|
||||
- [ ] [TDD] Add schema drift guard:
|
||||
- [ ] [TDD] run `schema-check` and ensure no unexpected drift.
|
||||
- [ ] [TDD] Add trigger behavior tests (integration-level):
|
||||
- [ ] [TDD] activate revision updates asset flags.
|
||||
- [ ] [TDD] deleting last edit operation does not orphan active state incorrectly.
|
||||
- [ ] [TDD] edited file links reject cross-asset revision references.
|
||||
- [ ] [TDD] legacy insert path without `revisionId` remains functional.
|
||||
- [ ] [TDD] sequence gap creation is rejected at transaction commit.
|
||||
- [ ] [TDD] concurrent activation attempts cannot produce two active revisions for one asset.
|
||||
|
||||
### 8) Deferred Cleanup (After Service Layer Is Updated)
|
||||
- [ ] [HUMAN] Confirm production traffic is fully revision-based before removing compatibility paths.
|
||||
- [ ] [TDD] Remove transitional compatibility constraints/triggers that only support old code paths.
|
||||
- [ ] [TDD] Remove legacy auto-assignment trigger for `asset_edit.revisionId` after service rollout.
|
||||
- [ ] [TDD] Consider dropping `asset_edit.assetId` once all reads/writes are revision-based.
|
||||
- [ ] [TDD] Consider replacing `asset_file.isEdited` with computed semantics from `editRevisionId`.
|
||||
- [ ] [HUMAN] Decide whether `asset.isEdited` remains as denormalized cache or is dropped.
|
||||
|
||||
## Definition of Done (Model Layer)
|
||||
- [ ] [TDD] Schema supports multiple preserved edit revisions per asset without deleting old revisions.
|
||||
- [ ] [TDD] Active revision is explicit, constrained, and query-efficient.
|
||||
- [ ] [TDD] Edited file rows are revision-linked and integrity-checked (including same-asset validation).
|
||||
- [ ] [TDD] Backfill is complete and verification queries return zero mismatches in automated tests.
|
||||
- [ ] [TDD] Migrations are reversible and `schema-check` shows no drift.
|
||||
- [ ] [HUMAN] Staging/prod verification queries are run and signed off by a human.
|
||||
- [ ] [HUMAN] All `[HUMAN]` tasks in this file are checked off by a human owner.
|
||||
Loading…
Reference in a new issue