Skip to content

[server] Optimize getTeamMembers (N queries → 1 query) #4482

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[server] Optimize getTeamMembers (N queries → 1 query) #4482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions components/gitpod-db/src/team-db.spec.db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { TypeORM } from './typeorm/typeorm';
import { DBTeam } from './typeorm/entity/db-team';
import { DBTeamMembership } from './typeorm/entity/db-team-membership';
import { DBUser } from './typeorm/entity/db-user';
import { DBIdentity } from './typeorm/entity/db-identity';

@suite class TeamDBSpec {

Expand All @@ -35,6 +36,7 @@ import { DBUser } from './typeorm/entity/db-user';
await manager.getRepository(DBTeam).delete({});
await manager.getRepository(DBTeamMembership).delete({});
await manager.getRepository(DBUser).delete({});
await manager.getRepository(DBIdentity).delete({});
}

@test(timeout(10000))
Expand All @@ -48,5 +50,18 @@ import { DBUser } from './typeorm/entity/db-user';
expect(dbResult[0].name).to.eq('Ground Control');
}

@test(timeout(10000))
public async findTeamMembers() {
const user = await this.userDb.newUser();
user.identities.push({ authProviderId: 'GitHub', authId: '1234', authName: 'Major Tom', primaryEmail: '[email protected]' });
await this.userDb.storeUser(user);
const team = await this.db.createTeam(user.id, 'Flight Crew');
const members = await this.db.findMembersByTeam(team.id);
expect(members.length).to.eq(1);
expect(members[0].userId).to.eq(user.id);
expect(members[0].primaryEmail).to.eq('[email protected]');
}

}

module.exports = new TeamDBSpec()
5 changes: 2 additions & 3 deletions components/gitpod-db/src/team-db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@
* See License.enterprise.txt in the project root folder.
*/

import { Team } from "@gitpod/gitpod-protocol";
import { DBTeamMembership } from "./typeorm/entity/db-team-membership";
import { Team, TeamMemberInfo } from "@gitpod/gitpod-protocol";

export const TeamDB = Symbol('TeamDB');
export interface TeamDB {
findTeamById(teamId: string): Promise<Team | undefined>;
findMembershipsByTeam(teamId: string): Promise<DBTeamMembership[]>;
findMembersByTeam(teamId: string): Promise<TeamMemberInfo[]>;
findTeamsByUser(userId: string): Promise<Team[]>;
createTeam(userId: string, name: string): Promise<Team>;
}
20 changes: 17 additions & 3 deletions components/gitpod-db/src/typeorm/team-db-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@
* See License-AGPL.txt in the project root for license information.
*/

import { Team, TeamMemberInfo, User } from "@gitpod/gitpod-protocol";
import { inject, injectable } from "inversify";
import { TypeORM } from "./typeorm";
import { Repository } from "typeorm";
import * as uuidv4 from 'uuid/v4';
import { TeamDB } from "../team-db";
import { DBTeam } from "./entity/db-team";
import { DBTeamMembership } from "./entity/db-team-membership";
import { Team } from "@gitpod/gitpod-protocol";
import { DBUser } from "./entity/db-user";

@injectable()
export class TeamDBImpl implements TeamDB {
Expand All @@ -29,14 +30,27 @@ export class TeamDBImpl implements TeamDB {
return (await this.getEntityManager()).getRepository<DBTeamMembership>(DBTeamMembership);
}

protected async getUserRepo(): Promise<Repository<DBUser>> {
return (await this.getEntityManager()).getRepository<DBUser>(DBUser);
}

public async findTeamById(teamId: string): Promise<Team | undefined> {
const teamRepo = await this.getTeamRepo();
return teamRepo.findOne({ id: teamId });
}

public async findMembershipsByTeam(teamId: string): Promise<DBTeamMembership[]> {
public async findMembersByTeam(teamId: string): Promise<TeamMemberInfo[]> {
const membershipRepo = await this.getMembershipRepo();
return membershipRepo.find({ teamId });
const userRepo = await this.getUserRepo();
const memberships = await membershipRepo.find({ teamId });
const users = await userRepo.findByIds(memberships.map(m => m.userId));
return users.map(u => ({
userId: u.id,
fullName: u.fullName || u.name,
primaryEmail: User.getPrimaryEmail(u),
avatarUrl: u.avatarUrl,
memberSince: u.creationDate,
}));
}

public async findTeamsByUser(userId: string): Promise<Team[]> {
Expand Down
4 changes: 2 additions & 2 deletions components/gitpod-db/src/typeorm/user-db-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,8 @@ export class TypeORMUserDBImpl implements UserDB {
}

public async findIdentitiesByName(identity: Identity): Promise<Identity[]> {
const userRepo = await this.getIdentitiesRepo();
const qBuilder = userRepo.createQueryBuilder('identity')
const repo = await this.getIdentitiesRepo();
const qBuilder = repo.createQueryBuilder('identity')
.where(`identity.authProviderId = :authProviderId`, { authProviderId: identity.authProviderId })
.andWhere(`identity.deleted != true`)
.andWhere(`identity.authName = :authName`, { authName: identity.authName });
Expand Down
1 change: 1 addition & 0 deletions components/gitpod-db/src/user-db.spec.db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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({});
Copy link
Contributor Author

@jankeromnes jankeromnes Jun 11, 2021 •
edited
Loading

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?

Copy link
Member

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.

await mnr.getRepository(DBWorkspace).delete({});
await mnr.getRepository(DBWorkspaceInstance).delete({});
}
Expand Down
7 changes: 3 additions & 4 deletions components/server/src/auth/resource-access.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@
* See License-AGPL.txt in the project root for license information.
*/

import { GitpodToken, Snapshot, Team, Token, User, UserEnvVar, Workspace, WorkspaceInstance } from "@gitpod/gitpod-protocol";
import { DBTeamMembership } from '@gitpod/gitpod-db/lib/typeorm/entity/db-team-membership';
import { GitpodToken, Snapshot, Team, TeamMemberInfo, Token, User, UserEnvVar, Workspace, WorkspaceInstance } from "@gitpod/gitpod-protocol";

declare var resourceInstance: GuardedResource;
export type GuardedResourceKind = typeof resourceInstance.kind;
Expand Down Expand Up @@ -83,7 +82,7 @@ export interface GuardEnvVar {
export interface GuardedTeam {
kind: "team";
subject: Team;
memberships: DBTeamMembership[];
members: TeamMemberInfo[];
}

export interface GuardedGitpodToken {
Expand Down Expand Up @@ -157,7 +156,7 @@ export class OwnerResourceGuard implements ResourceAccessGuard {
case "envVar":
return resource.subject.userId === this.userId;
case "team":
return resource.memberships.some(membership => membership.userId === this.userId);
return resource.members.some(m => m.userId === this.userId);
}
}

Expand Down
16 changes: 3 additions & 13 deletions components/server/src/workspace/gitpod-server-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ import { MessageBusIntegration } from './messagebus-integration';
import { WorkspaceDeletionService } from './workspace-deletion-service';
import { WorkspaceFactory } from './workspace-factory';
import { WorkspaceStarter } from './workspace-starter';
import { DBTeamMembership } from "@gitpod/gitpod-db/lib/typeorm/entity/db-team-membership";


@injectable()
Expand Down Expand Up @@ -1396,18 +1395,9 @@ export class GitpodServerImpl<Client extends GitpodClient, Server extends Gitpod
if (!team) {
throw new ResponseError(ErrorCodes.NOT_FOUND, "Team not found");
}
const memberships = await this.teamDB.findMembershipsByTeam(team.id);
await this.guardAccess({ kind: "team", subject: team, memberships }, "get");
return Promise.all(memberships.map(async (m: DBTeamMembership): Promise<TeamMemberInfo> => {
const member = await this.userDB.findUserById(m.userId);
return {
userId: m.userId,
fullName: member?.fullName || member?.name,
primaryEmail: !!member ? User.getPrimaryEmail(member) : undefined,
avatarUrl: member?.avatarUrl,
memberSince: m.creationTime,
};
}));
const members = await this.teamDB.findMembersByTeam(team.id);
await this.guardAccess({ kind: "team", subject: team, members }, "get");
return members;
}

public async createTeam(name: string): Promise<Team> {
Expand Down