-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[server] Optimize getTeamMembers (N queries → 1 query) #4482
Conversation
@@ -36,6 +36,7 @@ import { DBWorkspaceInstance } from './typeorm/entity/db-workspace-instance'; | |||
const typeorm = testContainer.get<TypeORM>(TypeORM); | |||
const mnr = await typeorm.getConnection(); | |||
await mnr.getRepository(DBUser).delete({}); | |||
await mnr.getRepository(DBIdentity).delete({}); |
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.
@geropl On a side note, while manipulating identities in TeamDBSpec
, I stumbled on a weird failure in UserDBSpec
:
1) UserDBSpec
createUserAndFindById:
AssertionError: expected { Object (id, creationDate, ...) } to have deep property 'identities' of [ Array(1) ], but got [ Array(1) ]
+ expected - actual
"authId": "1234"
"authName": "gero"
"authProviderId": "GitHub"
"deleted": false
- "primaryEmail": "[email protected]"
+ "primaryEmail": [undefined]
"readonly": false
"tokens": []
}
]
See: https://werft.gitpod-dev.com/job/gitpod-build-jx-optimize-get-team-members.2
That's when I realized that identities are not deleted when users are deleted:
gitpod/components/gitpod-db/src/typeorm/entity/db-user.ts
Lines 41 to 49 in a6c5481
@OneToMany(type => DBIdentity, | |
identity => identity.user, | |
{ | |
eager: true, | |
cascadeInsert: true, | |
cascadeUpdate: true | |
// , cascadeRemove: true // In the docs but not the code??? | |
} | |
) |
My drive-by fix here was to also clean up the DBIdentity
repo after cleaning up DBUser
(in both UserDBSpec
and TeamDBSpec
). But that pre-existing comment makes a good point -- maybe we should cascadeRemove identities?
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.
Wasn't the reason that we are not cascade deleting related items that it needs to go through dbsync? i.e. we need to enable soft-deletion for that.
/werft run 👍 started the job as gitpod-build-jx-optimize-get-team-members.5 |
1a09a88
to
4218d75
Compare
Anyone please approve. Can be safely merged any time. |
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.
LGTM
Fixes #4470
I've also added a test (passed! ✅) and getting team member infos still works fine: