feat(datastore): repository Update and Delete (S17 Phase 1 Step 7)

João Pereirarequested to merge
jdrpereira/s17-phase1-repository-crud-step-7 into main

What

Adds RepositoryStore.Update and RepositoryStore.Delete to the datastore layer, Step 7 of the S17 Phase 1 plan, behind the S17 spec Update and Delete sections.

  • Update mutates only description and visibility, leaving last_updated_at and server-managed columns untouched.
  • Delete is a single DELETE FROM repositories. A migration (20260626094030) flips the per-format child and collection-link foreign keys to ON DELETE CASCADE, so Postgres removes the child row and every collection link; the artifact-to-child FKs stay NO ACTION, so a still-referenced artifact (live or soft-deleted-only) aborts the whole delete and surfaces as ErrRepositoryNotEmpty (-> 409). A miss is ErrNotFound (-> 404); anything else wraps to 500.

Design notes

  • Cascade-based delete. The decision (cascade where children are structure, reject where they are user data) is documented in ADR-007 via handbook MR gitlab-com/content-sites/handbook!20218 (merged). The migration flips four FKs to ON DELETE CASCADE, so Delete no longer hand-rolls the child/link/parent teardown.
  • FK-abort classification keys on the SQLSTATE, not the constraint name. Every FK into repositories and the collection link is ON DELETE CASCADE, so the only 23503 a DELETE FROM repositories can raise is an artifact-to-child NO ACTION FK; that maps to ErrRepositoryNotEmpty. Matching the SQLSTATE alone keeps it independent of the per-partition constraint name, which differs across PG versions.
  • ErrRepositoryNotEmpty is returned bare (not %w-wrapped) so PgError.Detail/Where cannot leak the violating key to the client.

Spec coverage

# Criterion Tests
AC-10 Update mutates description+visibility, leaves last_updated_at TestRepositoryStore_Update/mutates_only_..., .../clears_description...
AC-11 Delete removes child, link, parent; frees the name TestRepositoryStore_Delete/removes_child,_link,_and_parent... (container/npm/maven), .../removes_every_collection_link...
AC-11 Artifacts present -> ErrRepositoryNotEmpty, repo intact .../returns_ErrRepositoryNotEmpty... (per format), .../soft-deleted (npm/maven)
AC-11 Missing -> ErrNotFound .../returns_ErrNotFound_when_no_parent_row_matches
AC-12 Cross-namespace 404 (tenant isolation) TestRepositoryStore_Update/...across_namespaces, TestRepositoryStore_Delete/...across_namespaces
Error FK-abort classification (not-empty vs unrelated 23503) TestMapRepositoryDeleteError (unit)
Guards nil ctx / zero namespace / non-positive id reject before any DB work TestRepositoryStore_{FindByID,Update,Delete}_Guards (unit)

Handler-layer rows (HTTP codes, request validation) are Step 10.

Notes

  • The diff covers the cascade migration + regenerated structure.sql, the Update/Delete methods and their errRepository* per-store argument guards, and unit + integration tests. Test-heavy by nature of a datastore step.
  • Reviewed via /review-branch and the pr-review-toolkit; AppSec findings (the argument guards) and Duo findings addressed in-branch.

Related to #172 (closed)

Database Review Evidence

Migrations

Note

Timings are from CI (db:migrate matrix, goose verbose) against an empty database, in apply / rollback order per PG version. Production-scale validation via Database Lab is not yet available. See Database review evidence for the matrix rationale and how to read the numbers.

Migration PG 16 PG 17 PG 18
20260626094030_repositories_cascade_delete_to_children.sql OK (1.83s / 1.87s) OK (1.38s / 1.45s) OK (1.5s / 1.5s)

Migration notes:

  • The ~1.4-1.9s apply/rollback is the cost of dropping and re-adding four foreign keys, each propagated across the 64 hash partitions of its table (256 partition-level constraint operations). It is partition-DDL overhead on empty tables, not a data scan: NOT VALID is unsupported on partitioned tables, so each ADD CONSTRAINT validates, but the tables are empty so the scan finds nothing. The time is independent of row count and stays well within the 5-minute per-migration boot budget.

Queries

Note

Plans are from EXPLAIN (ANALYZE, BUFFERS) against an ephemeral PostgreSQL 17 container (matching GL_PG_CURR_VERSION from .gitlab-ci-other-versions.yml), with synthesized seed data rolled back per query and the container torn down at the end of the run. Numbers reflect moderate cardinality and do not capture production-scale effects. See Database review evidence for seed sizing, methodology, and the anomalies the skill flags. Expand each row's details for the seed shape, rendered SQL, bound args, and raw plan.

Method Plan node Index Rows (plan / actual) Cost Time Buffers (hit / read) Partitions
datastore.Update Update → Seq Scan n/a (50-row seed; pk_repositories at scale) 1 / 1 1.75 0.571ms 32 / 2 1
datastore.Delete Delete → Seq Scan n/a (50-row seed; pk_repositories at scale) 1 / 1 1.75 6.508ms 21 / 0 1

Both are point operations on the (namespace_id, id) key that prune 64 partitions to one. At the 50-row write-target seed the planner Seq-Scans the single pruned partition; the PK covers the predicate at production cardinality. Delete is one DELETE FROM repositories: the plan shows the four ON DELETE CASCADE triggers that remove the per-format child row and the collection link, plus the container_images NO ACTION FK check on the cascaded child (the "repository not empty" guard, which passes here because the seeded repository holds no artifacts). No anomalies.

datastore.Update

Summary: Point UPDATE on (namespace_id, id). The namespace_id literal prunes to one partition (repositories_p02); the Seq Scan over the 50-row pruned partition is a function of the seed size, pk_repositories covers the predicate at scale. Estimate matches actual (1/1). Only visibility and description are in the SET list. No anomalies.

Seed shape: namespaces=1, repositories=50

Rendered SQL:

UPDATE public.repositories
SET (visibility, description) = ($1, NULL)
WHERE (repositories.namespace_id = $2::uuid) AND (repositories.id = $3)
RETURNING repositories.namespace_id, repositories.id, repositories.artifacts_count,
          repositories.downloads_count, repositories.size_bytes, repositories.created_at,
          repositories.last_updated_at, repositories.soft_deleted_at, repositories.format,
          repositories.kind, repositories.visibility, repositories.name, repositories.description,
          repositories.gitlab_created_by_user_id, repositories.gitlab_last_updated_by_user_id

Bound args: [1 (visibility), <namespace_id>, 26 (id)]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Update on repositories  (cost=0.00..1.75 rows=1 width=44) (actual time=0.197..0.198 rows=1 loops=1)
   Update on repositories_p02 repositories_1
   Buffers: shared hit=32 read=2
   ->  Seq Scan on repositories_p02 repositories_1  (cost=0.00..1.75 rows=1 width=44) (actual time=0.005..0.007 rows=1 loops=1)
         Filter: ((namespace_id = '7b1f683e-2ff4-4598-bb3a-62469770bee9'::uuid) AND (id = '26'::bigint))
         Rows Removed by Filter: 49
         Buffers: shared hit=1
 Planning:
   Buffers: shared hit=359
 Planning Time: 1.137 ms
 Trigger for constraint fk_repositories_namespace_id_namespaces on repositories_p02: time=0.233 calls=1
 Execution Time: 0.571 ms

Timings: planning 1.137ms, execution 0.571ms, total 1.708ms.

datastore.Delete

Summary: Point DELETE on (namespace_id, id), pruned to one partition (repositories_p41). The plan shows the cascade design directly: the container/npm/maven_repositories and repository_collection_repositories ON DELETE CASCADE FK triggers fire (removing the seeded child row and collection link), and the container_images NO ACTION FK check fires on the cascaded child row, the guard that aborts the delete when artifacts remain (here it passes, the repository is empty). Execution (6.5ms) is dominated by the five FK-trigger checks; each is a point operation on the pruned partition at scale. Estimate matches actual (1/1). No anomalies.

Seed shape: namespaces=1, repository_collections=1, repositories=50, container_repositories=1, repository_collection_repositories=1

Rendered SQL:

DELETE FROM public.repositories
WHERE (repositories.namespace_id = $1::uuid) AND (repositories.id = $2)

Bound args: [<namespace_id>, 76 (id)]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Delete on repositories  (cost=0.00..1.75 rows=0 width=0) (actual time=0.087..0.088 rows=0 loops=1)
   Delete on repositories_p41 repositories_1
   Buffers: shared hit=21
   ->  Seq Scan on repositories_p41 repositories_1  (cost=0.00..1.75 rows=1 width=10) (actual time=0.006..0.008 rows=1 loops=1)
         Filter: ((namespace_id = '33f682c0-9b6f-45d3-ba61-fc3451940dc9'::uuid) AND (id = '76'::bigint))
         Rows Removed by Filter: 49
         Buffers: shared hit=1
 Planning:
   Buffers: shared hit=144
 Planning Time: 0.751 ms
 Trigger for constraint container_repositories_repository_id_namespace_id_fkey41 on repositories_p41: time=0.886 calls=1
 Trigger for constraint npm_repositories_repository_id_namespace_id_fkey41 on repositories_p41: time=1.899 calls=1
 Trigger for constraint maven_repositories_repository_id_namespace_id_fkey41 on repositories_p41: time=1.075 calls=1
 Trigger for constraint repository_collection_reposit_repository_id_namespace_id_fkey41 on repositories_p41: time=0.673 calls=1
 Trigger for constraint container_images_container_repository_id_namespace_id_fkey41 on container_repositories_p41: time=1.673 calls=1
 Execution Time: 6.508 ms

Timings: planning 0.751ms, execution 6.508ms, total 7.259ms.

Edited by João Pereira

Merge request reports