-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore(group_history): Safe removal of prev_history #102663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #102663 +/- ##
===========================================
- Coverage 80.92% 80.91% -0.01%
===========================================
Files 8922 8922
Lines 390952 390971 +19
Branches 24860 24860
===========================================
+ Hits 316360 316367 +7
- Misses 74224 74236 +12
Partials 368 368 |
| name="prev_history", | ||
| field=FlexibleForeignKey( | ||
| # We want to remove the database-level foreign key constraint | ||
| db_constraint=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes integrity errors like this: https://sentry.sentry.io/issues/6994851757/
| SafeRemoveField( | ||
| model_name="grouphistory", | ||
| name="prev_history", | ||
| deletion_action=DeletionAction.MOVE_TO_PENDING, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This simply marks the column as not useable.
| prev_history_date = models.DateTimeField( | ||
| null=True | ||
| ) # This field is used to simplify query calculations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Removing prev_history from GroupHistory model before its SafeRemoveField migration runs causes a FieldDoesNotExist error during migration execution.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The SafeRemoveField.state_forwards() method, used in the migration, expects the prev_history field to exist in the GroupHistory model definition when the migration runs. Removing prev_history from src/sentry/models/grouphistory.py at lines 215-217 in the same change as the migration causes state.apps.get_model(...)._meta.get_field('prev_history') to raise a FieldDoesNotExist error, leading to a migration crash.
💡 Suggested Fix
Retain the prev_history field definition in src/sentry/models/grouphistory.py in this PR. Remove the field definition in a separate, subsequent PR after the migration has successfully deployed and executed in production.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/models/grouphistory.py#L215-L217
Potential issue: The `SafeRemoveField.state_forwards()` method, used in the migration,
expects the `prev_history` field to exist in the `GroupHistory` model definition when
the migration runs. Removing `prev_history` from `src/sentry/models/grouphistory.py` at
lines 215-217 in the same change as the migration causes
`state.apps.get_model(...)._meta.get_field('prev_history')` to raise a
`FieldDoesNotExist` error, leading to a migration crash.
Did we get this right? 👍 / 👎 to inform future reviews.
|
This PR has a migration; here is the generated SQL for for --
-- Alter field prev_history on grouphistory
--
SET CONSTRAINTS "sentry_grouphistory_prev_history_id_905ec402_fk_sentry_gr" IMMEDIATE; ALTER TABLE "sentry_grouphistory" DROP CONSTRAINT "sentry_grouphistory_prev_history_id_905ec402_fk_sentry_gr";
--
-- Moved grouphistory.prev_history field to pending deletion state
--
-- (no-op) |
|
I will open a new PR. What a mess. |
This is part of the required steps to remove a column from Sentry:
https://develop.sentry.dev/backend/application-domains/database-migrations/#deleting-columns.
This is a follow-up to #102474.