Skip to content
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(grouping): Fix project-deletion-triggered GroupHash deletion #76572

Merged

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Aug 26, 2024

This is a follow-up to #76312, fixing another bug with grouphash deletion. Currently, when a project is deleted, its associated GroupHash records are deleted directly by the project deletion task, 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 allowing GroupHash deletions to cascade from group deletions (which themselves are set off by project deletion), rather than having the project deletion task delete the grouphashes directly. (Group deletions already handle GroupHash/GroupHashMetadata deletion correctly, using 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 #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.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 26, 2024
@lobsterkatie lobsterkatie marked this pull request as ready for review August 27, 2024 04:15
@jangjodi
Copy link
Member

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

@lobsterkatie
Copy link
Member Author

lobsterkatie commented Aug 27, 2024

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.

@JoshFerge
Copy link
Member

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

@lobsterkatie
Copy link
Member Author

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?

@JoshFerge
Copy link
Member

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

@lobsterkatie
Copy link
Member Author

lobsterkatie commented Aug 28, 2024

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 lobsterkatie merged commit b380dd5 into master Sep 3, 2024
50 checks passed
@lobsterkatie lobsterkatie deleted the kmclb-fix-grouphash-deletion-via-project-deletion branch September 3, 2024 18:45
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants