use-globalid #373
use-globalid #373
Conversation
…d update related logic
… search_vector triggers for searchable tables
There was a problem hiding this comment.
Pull request overview
This PR refactors the NMA_MinorTraceChemistry table to use GlobalID as the primary key instead of an auto-incremented id field, and updates the associated transfer logic and database migrations accordingly.
Changes:
- Changed primary key from auto-incremented
idtoGlobalID(UUID) in theNMAMinorTraceChemistrymodel - Updated upsert logic to use
GlobalIDfor conflict resolution instead of the composite unique constraint - Added two migration files: one to migrate the primary key change and one to add search vector triggers
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| db/nma_legacy.py | Changed primary key from id to global_id (GlobalID) |
| transfers/minor_trace_chemistry_transfer.py | Updated transfer logic to extract, validate, and dedupe by GlobalID; changed upsert conflict resolution |
| alembic/versions/d2f4c6a8b1c2_make_minor_trace_globalid_pk.py | Migration to replace auto-increment id with GlobalID as primary key |
| alembic/versions/95d8b982cd5d_add_nma_chemistry_lineage_relations.py | Updated initial table creation to use GlobalID instead of id |
| alembic/versions/e4f7a9c0b2d3_add_search_vector_triggers.py | New migration for adding search vector triggers (unrelated to GlobalID change) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dab067e0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| op.add_column( | ||
| "NMA_MinorTraceChemistry", | ||
| sa.Column("GlobalID", postgresql.UUID(as_uuid=True), nullable=False), | ||
| ) |
There was a problem hiding this comment.
The upgrade adds a new GlobalID column with nullable=False on an existing table, but there is no backfill or default before enforcing the constraint. On PostgreSQL, this migration will fail if NMA_MinorTraceChemistry already contains rows because the new column is NULL for existing records. To make this safe on populated databases, add the column as nullable (or with a server default), populate it, then alter it to NOT NULL.
Useful? React with 👍 / 👎.
| stmt = insert_stmt.values(chunk).on_conflict_do_update( | ||
| constraint="uq_minor_trace_chemistry_sample_analyte", | ||
| index_elements=["GlobalID"], | ||
| set_={ |
There was a problem hiding this comment.
The upsert now targets GlobalID only, but the table still enforces the unique constraint on (chemistry_sample_info_id, analyte) from the earlier migration/model. If the legacy source has two rows with the same SamplePtID/analyte but different GlobalIDs (which the transfer previously deduped by), this insert will raise a unique‑constraint error because the conflict target doesn’t match the constraint that’s being violated. Either drop/adjust the unique constraint or keep the ON CONFLICT target aligned with it.
Useful? React with 👍 / 👎.
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
Notes
Any special considerations, workarounds, or follow-up work to note?