Skip to content

Commit

Permalink
refactor: optimize keycloak queries, new resolver for group members t…
Browse files Browse the repository at this point in the history 
…o help reduce group only queries
  • Loading branch information
@shreddedbacon
shreddedbacon committed Feb 15, 2023
1 parent f56da27 commit dbaa6ef
Show file tree
Hide file tree
Showing 8 changed files with 293 additions and 211 deletions.
172 changes: 75 additions & 97 deletions services/api/src/models/group.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,9 @@ export const Group = (clients: {
groups: R.isEmpty(subGroups)
? []
: await transformKeycloakGroups(subGroups),
members: await getGroupMembership(group)
// retrieving members is a heavy operation
// this is now its own resolver
// members: await getGroupMembership(group)
});
}

Expand All @@ -130,7 +132,7 @@ export const Group = (clients: {

const loadGroupById = async (id: string): Promise<Group> => {
const keycloakGroup = await keycloakAdminClient.groups.findOne({
id
id,
});

if (R.isNil(keycloakGroup)) {
Expand Down Expand Up @@ -187,14 +189,9 @@ export const Group = (clients: {
};

const loadAllGroups = async (): Promise<Group[]> => {
const keycloakGroups = await keycloakAdminClient.groups.find();
const keycloakGroups = await keycloakAdminClient.groups.find({briefRepresentation: false});

let fullGroups: Group[] = [];
for (const group of keycloakGroups) {
const fullGroup = await loadGroupById(group.id);

fullGroups = [...fullGroups, fullGroup];
}
const fullGroups = transformKeycloakGroups(keycloakGroups);

return fullGroups;
};
Expand Down Expand Up @@ -230,18 +227,9 @@ export const Group = (clients: {
const loadGroupsByAttribute = async (
filterFn: AttributeFilterFn
): Promise<Group[]> => {
const keycloakGroups = await keycloakAdminClient.groups.find();

let fullGroups: Group[] = [];
for (const group of keycloakGroups) {
const fullGroup = await keycloakAdminClient.groups.findOne({
id: group.id
});

fullGroups = [...fullGroups, fullGroup];
}
const keycloakGroups = await keycloakAdminClient.groups.find({briefRepresentation: false});

const filteredGroups = filterGroupsByAttribute(fullGroups, filterFn);
const filteredGroups = filterGroupsByAttribute(keycloakGroups, filterFn);

const groups = await transformKeycloakGroups(filteredGroups);

Expand All @@ -260,39 +248,27 @@ export const Group = (clients: {
return false;
};

let groupIds = [];

// let groupIds = [];
// This function is called often and is expensive to compute so prefer
// performance over DRY
try {
groupIds = await redisClient.getProjectGroupsCache(projectId);
} catch (err) {
logger.warn(`Error loading project groups from cache: ${err.message}`);
groupIds = [];
}
// try {
// groupIds = await redisClient.getProjectGroupsCache(projectId);
// } catch (err) {
// logger.warn(`Error loading project groups from cache: ${err.message}`);
// groupIds = [];
// }

if (R.isEmpty(groupIds)) {
const keycloakGroups = await keycloakAdminClient.groups.find();
// @ts-ignore
groupIds = R.pluck('id', keycloakGroups);
}

let fullGroups = [];
for (const id of groupIds) {
const fullGroup = await keycloakAdminClient.groups.findOne({
id
});
// this request will be huge, but it is significantly faster than the alternative iteration that followed previously
let fullGroups = await keycloakAdminClient.groups.find({briefRepresentation: false});

fullGroups = [...fullGroups, fullGroup];
}

const filteredGroups = filterGroupsByAttribute(fullGroups, filterFn);
try {
const filteredGroupIds = R.pluck('id', filteredGroups);
await redisClient.saveProjectGroupsCache(projectId, filteredGroupIds);
} catch (err) {
logger.warn(`Error saving project groups to cache: ${err.message}`);
}
// try {
// const filteredGroupIds = R.pluck('id', filteredGroups);
// await redisClient.saveProjectGroupsCache(projectId, filteredGroupIds);
// } catch (err) {
// logger.warn(`Error saving project groups to cache: ${err.message}`);
// }

const groups = await transformKeycloakGroups(filteredGroups);

Expand Down Expand Up @@ -491,9 +467,9 @@ export const Group = (clients: {
};

const deleteGroup = async (id: string): Promise<void> => {
const group = loadGroupById(id);
// const group = loadGroupById(id);
// @ts-ignore
const projectIds = getProjectIdsFromGroup(group);
// const projectIds = getProjectIdsFromGroup(group);

try {
await keycloakAdminClient.groups.del({ id });
Expand All @@ -505,13 +481,13 @@ export const Group = (clients: {
}
}

for (const projectId of projectIds) {
try {
await redisClient.deleteProjectGroupsCache(projectId);
} catch (err) {
logger.warn(`Error deleting project groups cache: ${err.message}`);
}
}
// for (const projectId of projectIds) {
// try {
// await redisClient.deleteProjectGroupsCache(projectId);
// } catch (err) {
// logger.warn(`Error deleting project groups cache: ${err.message}`);
// }
// }
};

const addUserToGroup = async (
Expand Down Expand Up @@ -553,11 +529,11 @@ export const Group = (clients: {
throw new Error(`Could not add user to group: ${err.message}`);
}

try {
await redisClient.deleteRedisUserCache(user.id);
} catch (err) {
logger.warn(`Error deleting user cache ${user.id}: ${err}`);
}
// try {
// await redisClient.deleteRedisUserCache(user.id);
// } catch (err) {
// logger.warn(`Error deleting user cache ${user.id}: ${err}`);
// }

return await loadGroupById(group.id);
};
Expand All @@ -582,11 +558,11 @@ export const Group = (clients: {
throw new Error(`Could not remove user from group: ${err.message}`);
}

try {
await redisClient.deleteRedisUserCache(user.id);
} catch (err) {
logger.warn(`Error deleting user cache ${user.id}: ${err}`);
}
// try {
// await redisClient.deleteRedisUserCache(user.id);
// } catch (err) {
// logger.warn(`Error deleting user cache ${user.id}: ${err}`);
// }
}

return await loadGroupById(group.id);
Expand Down Expand Up @@ -626,21 +602,21 @@ export const Group = (clients: {
}

// Clear the cache for users that gained access to the project
const groupAndParentsMembers = await getMembersFromGroupAndParents(group);
const userIds = R.map(R.path(['user', 'id']), groupAndParentsMembers);
for (const userId of userIds) {
try {
await redisClient.deleteRedisUserCache(userId);
} catch (err) {
logger.warn(`Error deleting user cache ${userId}: ${err}`);
}
}

try {
await redisClient.deleteProjectGroupsCache(projectId);
} catch (err) {
logger.warn(`Error deleting project groups cache: ${err.message}`);
}
// const groupAndParentsMembers = await getMembersFromGroupAndParents(group);
// const userIds = R.map(R.path(['user', 'id']), groupAndParentsMembers);
// for (const userId of userIds) {
// try {
// await redisClient.deleteRedisUserCache(userId);
// } catch (err) {
// logger.warn(`Error deleting user cache ${userId}: ${err}`);
// }
// }

// try {
// await redisClient.deleteProjectGroupsCache(projectId);
// } catch (err) {
// logger.warn(`Error deleting project groups cache: ${err.message}`);
// }
};

const removeProjectFromGroup = async (
Expand Down Expand Up @@ -676,21 +652,21 @@ export const Group = (clients: {
}

// Clear the cache for users that lost access to the project
const groupAndParentsMembers = await getMembersFromGroupAndParents(group);
const userIds = R.map(R.path(['user', 'id']), groupAndParentsMembers);
for (const userId of userIds) {
try {
await redisClient.deleteRedisUserCache(userId);
} catch (err) {
logger.warn(`Error deleting user cache ${userId}: ${err}`);
}
}

try {
await redisClient.deleteProjectGroupsCache(projectId);
} catch (err) {
logger.warn(`Error deleting project groups cache: ${err.message}`);
}
// const groupAndParentsMembers = await getMembersFromGroupAndParents(group);
// const userIds = R.map(R.path(['user', 'id']), groupAndParentsMembers);
// for (const userId of userIds) {
// try {
// await redisClient.deleteRedisUserCache(userId);
// } catch (err) {
// logger.warn(`Error deleting user cache ${userId}: ${err}`);
// }
// }

// try {
// await redisClient.deleteProjectGroupsCache(projectId);
// } catch (err) {
// logger.warn(`Error deleting project groups cache: ${err.message}`);
// }
};

return {
Expand All @@ -709,6 +685,8 @@ export const Group = (clients: {
addUserToGroup,
removeUserFromGroup,
addProjectToGroup,
removeProjectFromGroup
removeProjectFromGroup,
transformKeycloakGroups,
getGroupMembership
};
};
39 changes: 19 additions & 20 deletions services/api/src/models/user.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import pickNonNil from '../util/pickNonNil';
import { logger } from '../loggers/logger';
import UserRepresentation from 'keycloak-admin/lib/defs/userRepresentation';
import { Group, isRoleSubgroup } from './group';
import { GroupInterface } from '../resolvers';

export interface User {
email: string;
Expand Down Expand Up @@ -30,10 +31,11 @@ interface UserModel {
loadUserByUsername: (username: string) => Promise<User>;
loadUserByIdOrUsername: (userInput: UserEdit) => Promise<User>;
getAllGroupsForUser: (userInput: User) => Promise<Group[]>;
getAllProjectsIdsForUser: (userInput: User) => Promise<number[]>;
getAllProjectsIdsForUser: (userInput: User) => Promise<[number[], Group[]]>;
getUserRolesForProject: (
userInput: User,
projectId: number
projectId: number,
userGroups: Group[]
) => Promise<string[]>;
addUser: (userInput: User) => Promise<User>;
updateUser: (userInput: UserEdit) => Promise<User>;
Expand Down Expand Up @@ -215,17 +217,15 @@ export const User = (clients: {
let groups = [];

const roleSubgroups = await keycloakAdminClient.users.listGroups({
id: userInput.id
id: userInput.id,
briefRepresentation: false
});

for (const roleSubgroup of roleSubgroups) {
const fullRoleSubgroup = await GroupModel.loadGroupById(roleSubgroup.id);
if (!isRoleSubgroup(fullRoleSubgroup)) {
continue;
}

const roleSubgroupParent = await GroupModel.loadParentGroup(
fullRoleSubgroup
// just look up the group with what we know the parent name to be with the regex of the role stripped
let regexp = /-(owner|maintainer|developer|reporter|guest)$/g;
const roleSubgroupParent = await GroupModel.loadGroupByName(
roleSubgroup.name.replace(regexp, "")
);

groups.push(roleSubgroupParent);
Expand All @@ -236,7 +236,7 @@ export const User = (clients: {

const getAllProjectsIdsForUser = async (
userInput: User
): Promise<number[]> => {
): Promise<[number[], Group[]]> => {
const GroupModel = Group(clients);
let projects = [];

Expand All @@ -249,17 +249,16 @@ export const User = (clients: {
projects = [...projects, ...projectIds];
}

return R.uniq(projects);
return [R.uniq(projects), userGroups];
};

const getUserRolesForProject = async (
userInput: User,
projectId: number
projectId: number,
userGroups: Group[]
): Promise<string[]> => {
const GroupModel = Group(clients);

const userGroups = await getAllGroupsForUser(userInput);

let roles = [];
for (const group of userGroups) {
const projectIds = await GroupModel.getProjectsFromGroupAndSubgroups(
Expand Down Expand Up @@ -365,11 +364,11 @@ export const User = (clients: {
throw new Error(`Error deleting user ${id}: ${err}`);
}
}
try {
await redisClient.deleteRedisUserCache(id);
} catch (err) {
logger.error(`Error deleting user cache ${id}: ${err}`);
}
// try {
// await redisClient.deleteRedisUserCache(id);
// } catch (err) {
// logger.error(`Error deleting user cache ${id}: ${err}`);
// }
};

return {
Expand Down
4 changes: 3 additions & 1 deletion services/api/src/resolvers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ const {
removeUserFromGroup,
addGroupsToProject,
removeGroupsFromProject,
getMembersByGroupId,
} = require('./resources/group/resolvers');

const {
Expand Down Expand Up @@ -362,7 +363,8 @@ const resolvers = {
},
},
Group: {
projects: getAllProjectsByGroupId
projects: getAllProjectsByGroupId,
members: getMembersByGroupId
},
DeployTargetConfig: {
project: getProjectById,
Expand Down
Loading

0 comments on commit dbaa6ef

Please sign in to comment.