sentry icon indicating copy to clipboard operation
sentry copied to clipboard

fix(grouping): Fix project-deletion-triggered `GroupHash` deletion

Open lobsterkatie opened this issue 1 year ago • 2 comments

This is a follow-up to https://github.com/getsentry/sentry/pull/76312, fixing another bug with grouphash deletion. Currently, when a project is deleted, its associated GroupHash records are deleted using the BulkModelDeletionTask. Unfortunately, that task doesn't take into account any dependent tables, meaning that the corresponding GroupHashMetadata records aren't deleted as they should be.

This fixes that by no longer specifying which deletion task project deletion should use for GroupHash records, which then allows use of the GroupHash-specific deletion task registered in the deletion module's __init__.py.

Note that while the results of this change aren't tested in this PR, they are implicitly tested in https://github.com/getsentry/sentry/pull/76245, which will directly follow this one and which for the first time will actually create GroupHashMetadata records. Without the change in this PR, the presence of that PR's new GroupHashMetadata records causes the project deletion test to fail, but with this change, they pass.

lobsterkatie avatar Aug 26 '24 21:08 lobsterkatie

is the deletion of GroupHash still being done in bulk, and if not, is this a concern? I see that GroupHashDeletionTask extends ModelDeletionTask

jangjodi avatar Aug 27 '24 15:08 jangjodi

is the deletion of GroupHash still being done in bulk, and if not, is this a concern? I see that GroupHashDeletionTask extends ModelDeletionTask

So there are two ways a GroupHash deletion can be triggered - by deleting a group or by deleting a project. When it's a group that's deleted, it's already using the grouphash-specific deletion task (which, as you say, isn't bulk). And deleting a project deletes all the groups, so another option here would be to just remove GroupHash from the project deletion task, and let the GroupHash and GroupHashMetadata deletes cascade from the Group deletion.

In any case, no, I don't think it's an issue, because if you look at that __init__ file where all the deletion tasks are registered (that I linked in the PR description), lots of models have bespoke deletion tasks, and they all inherit from ModelDeletionTask. Also, we have no choice here, because (as is the case with many of the models which have custom tasks), the delete has to cascade, and the way that happens is by subclassing and overriding the get_child_relations method (which is called by ModelDeletionTask but not by BulkModelDeletionClass).

UPDATE: I decided it actually is cleaner to just let the groups handle the deletion, so I've switched to that approach and updated the PR description.

lobsterkatie avatar Aug 27 '24 17:08 lobsterkatie

are there appropriate tests to ensure that groups still get deleted when a project is?

JoshFerge avatar Aug 28 '24 17:08 JoshFerge

are there appropriate tests to ensure that groups still get deleted when a project is?

That groups get deleted? Yeah, we test that here. But why would changing what triggers GroupHash deletion change the deletion of groups?

lobsterkatie avatar Aug 28 '24 17:08 lobsterkatie

are there appropriate tests to ensure that groups still get deleted when a project is?

That groups get deleted? Yeah, we test that here. But why would changing what triggers GroupHash deletion change the deletion of groups?

sorry, meant Group

JoshFerge avatar Aug 28 '24 23:08 JoshFerge

sorry, meant Group

Yeah, no, I know. That's what you said originally. And we do test for it, in the spot I linked above. I'm just confused why that's a question here, since we're not changing anything about group deletion.

lobsterkatie avatar Aug 28 '24 23:08 lobsterkatie