consolidate-backfill-transfers #371
Conversation
… for FK integrity
There was a problem hiding this comment.
Pull request overview
This PR adds a unique constraint to the SamplePtID column in the NMA_Chemistry_SampleInfo table to support foreign key relationships. The migration ensures the constraint is only created if it doesn't already exist as either a primary key or unique constraint.
Changes:
- Added validation logic to check existing primary key and unique constraints on
NMA_Chemistry_SampleInfotable - Conditionally creates a unique constraint on
SamplePtIDif not already present
| pk_columns = pk.get("constrained_columns") or [] | ||
| unique_constraints = inspector.get_unique_constraints("NMA_Chemistry_SampleInfo") | ||
| unique_columns = {tuple(uc.get("column_names") or []) for uc in unique_constraints} | ||
| if pk_columns != ["SamplePtID"] and ("SamplePtID",) not in unique_columns: |
There was a problem hiding this comment.
The primary key check uses exact equality (pk_columns != ['SamplePtID']) which will fail if SamplePtID is part of a composite primary key. The condition should check if SamplePtID is in the primary key columns using 'SamplePtID' not in pk_columns instead of pk_columns != ['SamplePtID'].
| if pk_columns != ["SamplePtID"] and ("SamplePtID",) not in unique_columns: | |
| if "SamplePtID" not in pk_columns and ("SamplePtID",) not in unique_columns: |
![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: 0d73c1f17b
ℹ️ 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".
| if pk_columns != ["SamplePtID"] and ("SamplePtID",) not in unique_columns: | ||
| op.create_unique_constraint( | ||
| "NMA_Chemistry_SampleInfo_SamplePtID_key", | ||
| "NMA_Chemistry_SampleInfo", | ||
| ["SamplePtID"], |
There was a problem hiding this comment.
The upgrade adds a new unique constraint on SamplePtID, but downgrade() never removes it. If this migration is rolled back, the schema will not return to its prior state and any older code or migrations that expect duplicates (or no uniqueness guarantee) can fail. Consider dropping NMA_Chemistry_SampleInfo_SamplePtID_key in downgrade() when it was created.
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?