chore: handle deleted organizations in idp sync #17405
Merged
+242
−33
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
f1878af to
9bdf31d
Compare
Emyrk
commented
Apr 15, 2025
Comment on lines
+2360
to
+2366
| deleted := false | ||
| q.data.organizationMembers = slices.DeleteFunc(q.data.organizationMembers, func(member database.OrganizationMember) bool { | ||
| match := member.OrganizationID == arg.OrganizationID && member.UserID == arg.UserID | ||
| deleted = deleted || match | ||
| return match | ||
| }) | ||
| if len(deleted) == 0 { | ||
| if !deleted { |
There was a problem hiding this comment.
oh this is nasty, took me a minute to realize what the bug was. 💀
if you want, rather than track deleted yourself, you could do something like
newMembers := slices.DeleteFunc(q.data.organizationMembers, func(member database.OrganizationMember) bool {
return member.OrganizationID == arg.OrganizationID && member.UserID == arg.UserID
})
if len(newMembers) == len(q.data.organizationMembers) {
return sql.ErrNoRows
}
q.data.organizationMembers = newMembersThere was a problem hiding this comment.
I had thought about that as well. That felt a bit less direct and less intuitive to me though, so I went with the crude and simple lol
Comment on lines
+58
to
+64
| -- Optionally provide a filter for deleted organizations. | ||
| CASE WHEN | ||
| sqlc.narg('deleted') :: boolean IS NULL THEN | ||
| true | ||
| ELSE | ||
| deleted = sqlc.narg('deleted') | ||
| END AND |
There was a problem hiding this comment.
Adds the ability to query for all orgs, regardless of deletion
Deleted organizations are still attempting to sync members. This causes an error on inserting the member, and would likely cause issues later in the sync process even if that member is inserted. Deleted orgs should be skipped.
Idp sync should exclude deleted organizations and auto remove members. They should not be members in the first place.
7b0cb6c to
909c895
Compare
aslilac
approved these changes
Apr 15, 2025
Comment on lines
+118
to
+120
| // If you pass in an empty slice to the db arg, you get all orgs. So the slice | ||
| // has to be non-empty to get the expected set. Logically it also does not make | ||
| // sense to fetch an empty set from the db. |
There was a problem hiding this comment.
yeah but this turns it into our problem. wouldn't this usually be the result of a configuration error?
There was a problem hiding this comment.
I don't follow. You can configure it such that a user is in 0 orgs. Even if it's a configuration mistake on the admin, it is a valid configuration.
Co-authored-by: ケイラ <mckayla@hey.com>
|
/cherry-pick release/2.20 |
gcp-cherry-pick-bot bot
pushed a commit
that referenced
this pull request
Apr 16, 2025
…17405) Deleted organizations are still attempting to sync members. This causes an error on inserting the member, and would likely cause issues later in the sync process even if that member is inserted. Deleted orgs should be skipped.
|
/cherry-pick release/2.21 |
gcp-cherry-pick-bot bot
pushed a commit
that referenced
this pull request
Apr 16, 2025
…17405) Deleted organizations are still attempting to sync members. This causes an error on inserting the member, and would likely cause issues later in the sync process even if that member is inserted. Deleted orgs should be skipped.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Deleted organizations are still attempting to sync members. This causes an error on inserting the member, and would likely cause issues later in the sync process even if that member is inserted. Deleted orgs should be skipped.
The error that arises: