DiamondLightSource / DiamondLightSource/smartem-devtools

Schema and data integrity: re-enable the drift check, add natural-key constraints

Aperta
#82 1 commento 0 reazioni 0 assegnatari Vedi su GitHub
Lingua principale
TypeScript
Stelle
0
Fork
0
Metriche di merge delle PR
Nessuna PR unita negli ultimi 30g

Descrizione

Narrowed. This issue was originally a broad automated "production readiness assessment"
covering eleven areas. Re-verifying its claims against the current codebase, two are true,
actionable, and tracked nowhere else; the rest are stale, overstated, or describe
infrastructure this project does not operate. The discarded items are listed at the bottom
with reasons, so the same assessment does not get re-run and refiled.

## In scope

### 1. The schema drift check is bypassed in CI

`.github/workflows/_schema_drift.yml` still carries `continue-on-error: true`. The job runs and
reports, but cannot fail a build, so drift between the SQLModel definitions and the migration
history can accumulate silently and indefinitely.

This is the more important of the two items, because it is the reason schema problems persist
without being noticed.

Work: establish whether the check currently passes on a clean run; fix whatever it reports; then
remove `continue-on-error` so it is enforced. Re-enabling it before the backlog of drift is
cleared would simply block every PR, so the order matters.

### 2. No unique constraints on natural keys

The following have no unique constraint, despite each being a natural key:

- `foilhole` - `(foilhole_id, gridsquare_uuid)`
- `gridsquare` - `(gridsquare_id, grid_uuid)`
- `micrograph` - `(micrograph_id, foilhole_uuid)`

The only unique constraints in `model/database.py` are on `session_id`, `instruction_id` and
`connection_id`, which are unrelated.

This is not theoretical. `tools/find_foilhole_duplicates.py` exists in the repository, which
means duplicates have been observed and needed hunting.

Work: deduplicate existing data first, then add the constraints in a migration. The
deduplication is the awkward part and needs a decision on which row wins when duplicates differ.

Note the interaction with the agent's filesystem intake: if the ingest path can legitimately
re-observe the same entity, the constraint needs to be paired with an upsert rather than simply
rejecting the second write.

## Explicitly discarded

Re-checked and not carried forward:

- **"Only 13 test files, none for database operations"** - no longer true. There are 32 test
files, including `test_acquisitions.py`, `test_foilholes.py`, `test_gridsquares.py`,
`test_grids.py` and `test_atlases.py`, plus an async database stub in `conftest.py`.
- **"Index created on the wrong table (line 602)"** - unverifiable. The migration it cites is
548 lines long, so that line does not exist. The `qualitypredictionmodelparameter` references
in that file are foreign-key operations, not a misplaced index.
- **"147 indexes causing write overhead"** - that figure counts `create_index` calls across the
whole migration history, including indexes subsequently dropped. It is not a count of live
indexes, and no performance problem attributable to indexing has been observed.
- **"Migration removed CASCADE DELETE"** - overstated. `ondelete="CASCADE"` appears in the same
migration the claim is made about.
- **Backups, WAL archiving, retention and archival** - deferred, and likely not this project's
responsibility. DLS may have a facility-wide strategy; that needs confirming before any work
is scoped here.
- **PostgreSQL configuration tuning, database user separation, row-level security,
table partitioning** - these describe operating our own PostgreSQL instance. In staging and
production, PostgreSQL is a managed service provided to the project. This advice was derived
from the development k3s cluster, where the database is self-hosted, and does not transfer.
- **Connection pool monitoring** - not dismissed on merit, but it belongs with observability
rather than schema integrity, and should be scoped alongside metrics collection generally.

## On the original framing

The "72% production readiness - NOT READY" headline and the "2-3 weeks to production" estimate
were derived from the above, including the items now shown to be false or inapplicable. Neither
figure should be quoted.

Guida per i contributori

Nessuna guida per i contributori indicizzata per questo repository

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.