fix: add postgres triggers to remove deleted users from user_links

[Emyrk]

What this does

• Removes ability to un-delete a user. We never exposed this outside the db layer, doing this to make that decision something that takes more thought than just changing an api param.
• User_links should not longer exist for deleted users.

[Emyrk]

• fix: add postgres triggers to remove deleted users from user_links #12117 👈
• fix: do not query user_link for deleted accounts #12112
• chore: add database test fixture to insert non-unique linked_ids #12111
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Emyrk and the rest of your teammates on Graphite

[kylecarbs]

Why are we removing the ability to undelete a user?

[Emyrk]

@kylecarbs

Why are we removing the ability to undelete a user?

We never exposed this via the CLI or the API. It only exists in the DB query.

However, un-deleting a user is more work than just the query that I removed. It can now collide with existing users, their user_links were not correct, and now deleted.

Essentially undeleting a user can cause issues (if it can succeed at all), so we never supported it. And we should remove the query that will very likely fail in prod. I fear someone would just add a query param, use the existing query, merge it (because we do not test deleted users very much in tests). Then almost immediately it'll cause issues in an actual database.

[kylecarbs]

I know we never supported it, but it feels odd to keep the soft delete and a real delete. Can we change it to completely do one or the other?

[Emyrk]

I know we never supported it, but it feels odd to keep the soft delete and a real delete. Can we change it to completely do one or the other?

We do not support hard deletes at all on most resources. For auditing purposes, we need to keep these objects around.

[kylecarbs]

@Emyrk I'm confused why we should then on users. It seems like a fork in the road. It seems better to either leave this and not permit undeleting a user (e.g. actually blocking it), or making it work instead of allowing it for a single resource type.

[Emyrk]

I do not totally follow.

The product feature set is unchanged. We previously only supported soft deleting users from the API. I just made the DB query reflect this, so now you cannot un-delete users even from a DB query. So just trying to prevent developer error.

The user_links cleanup is because a deleted user should not be able to authenticate into Coder anymore. This becomes problematic if this is allowed because then a single github user can have multiple accounts linked to it. Maybe this is ok? Just felt strange from a data POV, so I implemented this to clean it up in the same way we do api keys.

[kylecarbs]

Even though it is idiomatic with our data structure, I'm a bit nervous about having a term "soft delete" in one resource, even though I know we have it in others.

We've coined the term deleted as a soft-delete already. And since we called it UpdateDeleted I feel like it's a bit more confusing to call this SoftDelete now.

A few questions:

1. Can we just fix this in the Go code? Do we need to have the triggers added? It seems harder to test, and more spread complexity.
2. Is it alright if we continue to use UpdateDeleted instead of SoftDelete? Does my rationale make sense in this context?

[Emyrk]

2. Is it alright if we continue to use UpdateDeleted instead of SoftDelete? Does my rationale make sense in this context?

How about I return the name to UpdateUserDeletedByID but always make it do deleted = true? So there is no argument to ever revert the deleted state?

[kylecarbs]

That sounds good to me!