-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
fix(migrations): Fix DoesNotExist exception handling in migration 0024 #23484
Conversation
Reported here: https://forum.sentry.io/t/error-importing-events-during-upgrade-from-9-1-2-to-20-11-and-up/12683/6?u=byk. I think this is either related to a Django upgrade or a Python upgrade. My code now covers both types of exceptions but not sure if that's the best way.
@@ -79,7 +79,7 @@ def _attach_related(_events): | |||
|
|||
try: | |||
group = event.group | |||
except Group.DoesNotExist: | |||
except (Group.DoesNotExist, NewGroup.DoesNotExist): |
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.
Doesn't that actually mean that e.group_id is None
?
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.
Yeah, I think it does. Might be able to just check on that before setting group
.
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.
Are you sure this is the only case? What if they have an invalid group id set, which is entirely possible?
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 migration is a bit tricky in general since it uses so many external functions.
I think what I'd do here instead would be group = Group.objects.get(id=event.group_id)
and not import NewGroup
at all. The logic in Event.group
really just handles caching which isn't going to be a thing here. We could also check that group_id is not None
before making the query.
You'll also need to use group
down below on insert
, rather than event.group
.
Edit: Actually, reading this more we do actually use event.group
for caching, we fetch it in advance in _attach_related
. The issue here is that if the group didn't exist, then we actually make another database query to try and fetch it again, then throw NewGroup.DoesNotExist
.
What we should actually do is set event.group = e.group
, then we'll be fine. We already fetched it from the database above, so there's no need to do this check again. We can then totally remove this try/catch, and only use event.group
. Then we don't need to import NewGroup
at all, and we should actually speed this migration up a bit, since we're not double fetching all groups.
Might also be worth setting event.project = e.project
, since that caching also gets dropped when we create event
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.
@wedamija updated based on your suggestions. Rereview? <3
This PR has a migration; here is the generated SQL BEGIN;
--
-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:
-- Raw Python operation
--
COMMIT; |
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 looks fine to me.
@@ -79,7 +79,7 @@ def _attach_related(_events): | |||
|
|||
try: | |||
group = event.group | |||
except Group.DoesNotExist: | |||
except (Group.DoesNotExist, NewGroup.DoesNotExist): |
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.
Yeah, I think it does. Might be able to just check on that before setting group
.
Reported here: forum.sentry.io/t/error-importing-events-during-upgrade-from-9-1-2-to-20-11-and-up/12683/6?u=byk. I think this is either related to a Django upgrade or a Python upgrade. We have change the code to be more efficient and clever now that we don't even need the error handling there.