Skip to content

Commit

Permalink
refactor: favor permission name over id (#5409)
Browse files Browse the repository at this point in the history 
https://linear.app/unleash/issue/2-1664/create-db-migration-that-favors-the-name-column-over-id-for

Similar to #5398, but
non-breaking (semver).
This keeps the permissions `id` column intact, however favors the
permission name whenever possible.
  • Loading branch information
@nunogois
nunogois authored Nov 27, 2023
1 parent b021e7c commit 023db4e
Show file tree
Hide file tree
Showing 9 changed files with 1,217 additions and 93 deletions.
2 changes: 1 addition & 1 deletion frontend/src/component/common/RoleDescription/RoleDescription.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export const RoleDescription = ({
</StyledDescriptionHeader>
<StyledPermissionsList>
{permissions.map((permission) => (
<li key={permission.id}>
<li key={permission.name}>
{permission.displayName}
</li>
))}
Expand Down
1 change: 0 additions & 1 deletion frontend/src/interfaces/permissions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ export type PermissionType =
| typeof ENVIRONMENT_PERMISSION_TYPE;

export interface IPermission {
id: number;
name: string;
displayName: string;
type: PermissionType;
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/utils/permissions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import cloneDeep from 'lodash.clonedeep';

export const getRoleKey = (permission: IPermission): string => {
return permission.environment
? `${permission.id}-${permission.environment}`
: `${permission.id}`;
? `${permission.name}-${permission.environment}`
: `${permission.name}`;
};

export const permissionsToCheckedPermissions = (
Expand Down
42 changes: 19 additions & 23 deletions src/lib/db/access-store.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,14 @@ test('resolvePermissions returns empty list if empty list', async () => {
});

test.each([
args([{ id: 1 }]),
args([{ id: 4, environment: 'development' }]),
args([{ id: 4, name: 'should keep the id' }]),
args([{ name: 'CREATE_CONTEXT_FIELD' }]),
args([{ name: 'CREATE_FEATURE', environment: 'development' }]),
args([
{ id: 1, environment: 'development' },
{ id: 2, name: 'ignore this name' },
{ name: 'CREATE_CONTEXT_FIELD' },
{ name: 'CREATE_FEATURE', environment: 'development' },
]),
])(
'resolvePermissions with permission ids (%o) returns the list unmodified',
'resolvePermissions with permission names (%o) will return the list unmodified',
async (permissions) => {
const access = db.stores.accessStore as AccessStore;
const result = await access.resolvePermissions(permissions);
Expand All @@ -56,26 +55,23 @@ test.each([
);

test.each([
args([{ id: 1 }], [{ id: 1, name: 'ADMIN' }]),
args(
[{ name: 'CREATE_CONTEXT_FIELD' }],
[{ id: 18, name: 'CREATE_CONTEXT_FIELD' }],
),
args(
[{ name: 'CREATE_FEATURE', environment: 'development' }],
[{ id: 2, name: 'CREATE_FEATURE', environment: 'development' }],
[{ id: 4, environment: 'development' }],
[{ id: 4, name: 'CREATE_ADDON', environment: 'development' }],
),
args(
[
{ name: 'CREATE_CONTEXT_FIELD' },
{ name: 'CREATE_FEATURE', environment: 'development' },
{ id: 1, environment: 'development' },
{ id: 2, name: 'ignore this name' },
],
[
{ id: 18, name: 'CREATE_CONTEXT_FIELD' },
{ id: 2, name: 'CREATE_FEATURE', environment: 'development' },
{ id: 1, name: 'ADMIN', environment: 'development' },
{ id: 2, name: 'ignore this name' },
],
),
])(
'resolvePermissions with permission names (%o) will inject the ids',
'resolvePermissions with only permission ids (%o) will resolve to named permissions without an id',
async (permissions, expected) => {
const access = db.stores.accessStore as AccessStore;
const result = await access.resolvePermissions(permissions);
Expand All @@ -93,15 +89,15 @@ test.each([
{ name: 'UPDATE_FEATURE', environment: 'development' },
],
[
{ id: 18, name: 'CREATE_CONTEXT_FIELD' },
{ id: 3 },
{ id: 2, name: 'CREATE_FEATURE', environment: 'development' },
{ id: 15, environment: 'development' },
{ id: 7, name: 'UPDATE_FEATURE', environment: 'development' },
{ name: 'CREATE_CONTEXT_FIELD' },
{ id: 3, name: 'CREATE_STRATEGY' },
{ name: 'CREATE_FEATURE', environment: 'development' },
{ id: 15, name: 'UPDATE_STRATEGY', environment: 'development' },
{ name: 'UPDATE_FEATURE', environment: 'development' },
],
),
])(
'resolvePermissions mixed ids and names (%o) will inject the ids where they are missing',
'resolvePermissions mixed ids and names (%o) will inject the names where they are missing',
async (permissions, expected) => {
const access = db.stores.accessStore as AccessStore;
const result = await access.resolvePermissions(permissions);
Expand Down
120 changes: 55 additions & 65 deletions src/lib/db/access-store.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,7 @@ interface IPermissionRow {
role_id: number;
}

type ResolvedPermission = {
id: number;
name?: string;
environment?: string;
};
type NameAndIdPermission = NamePermissionRef & IdPermissionRef;

export class AccessStore implements IAccessStore {
private logger: Logger;
Expand All @@ -74,70 +70,71 @@ export class AccessStore implements IAccessStore {
});
}

private permissionHasId = (permission: PermissionRef): boolean => {
return (permission as IdPermissionRef).id !== undefined;
private permissionHasName = (permission: PermissionRef): boolean => {
return (permission as NamePermissionRef).name !== undefined;
};

private permissionNamesToIds = async (
permissions: NamePermissionRef[],
): Promise<ResolvedPermission[]> => {
const permissionNames = (permissions ?? [])
.filter((p) => p.name !== undefined)
.map((p) => p.name);
private permissionIdsToNames = async (
permissions: IdPermissionRef[],
): Promise<NameAndIdPermission[]> => {
const permissionIds = (permissions ?? [])
.filter((p) => p.id !== undefined)
.map((p) => p.id);

if (permissionNames.length === 0) {
if (permissionIds.length === 0) {
return [];
}

const stopTimer = this.timer('permissionNamesToIds');
const stopTimer = this.timer('permissionIdsToNames');

const rows = await this.db
.select('id', 'permission')
.from(T.PERMISSIONS)
.whereIn('permission', permissionNames);
.whereIn('id', permissionIds);

const rowByPermissionName = rows.reduce((acc, row) => {
acc[row.permission] = row;
const rowByPermissionId = rows.reduce((acc, row) => {
acc[row.id] = row;
return acc;
}, {} as Map<string, IPermissionRow>);

const permissionsWithIds = permissions.map((permission) => ({
id: rowByPermissionName[permission.name].id,
const permissionsWithNames = permissions.map((permission) => ({
name: rowByPermissionId[permission.id].permission,
...permission,
}));

stopTimer();
return permissionsWithIds;
return permissionsWithNames;
};

resolvePermissions = async (
permissions: PermissionRef[],
): Promise<ResolvedPermission[]> => {
): Promise<NamePermissionRef[]> => {
if (permissions === undefined || permissions.length === 0) {
return [];
}
// permissions without ids (just names)
const permissionsWithoutIds = permissions.filter(
(p) => !this.permissionHasId(p),
) as NamePermissionRef[];
const idPermissionsFromNamed = await this.permissionNamesToIds(
permissionsWithoutIds,
);

if (permissionsWithoutIds.length === permissions.length) {
// all named permissions without ids
return idPermissionsFromNamed;
} else if (permissionsWithoutIds.length === 0) {
// all permissions have ids
return permissions as ResolvedPermission[];
// permissions without names (just ids)
const permissionsWithoutNames = permissions.filter(
(p) => !this.permissionHasName(p),
) as IdPermissionRef[];

if (permissionsWithoutNames.length === permissions.length) {
// all permissions without names
return await this.permissionIdsToNames(permissionsWithoutNames);
} else if (permissionsWithoutNames.length === 0) {
// all permissions have names
return permissions as NamePermissionRef[];
}
// some permissions have ids, some don't (should not happen!)

// some permissions have names, some don't (should not happen!)
const namedPermissionsFromIds = await this.permissionIdsToNames(
permissionsWithoutNames,
);
return permissions.map((permission) => {
if (this.permissionHasId(permission)) {
return permission as ResolvedPermission;
if (this.permissionHasName(permission)) {
return permission as NamePermissionRef;
} else {
return idPermissionsFromNamed.find(
(p) => p.name === (permission as NamePermissionRef).name,
return namedPermissionsFromIds.find(
(p) => p.id === (permission as IdPermissionRef).id,
)!;
}
});
Expand Down Expand Up @@ -204,20 +201,20 @@ export class AccessStore implements IAccessStore {
let userPermissionQuery = this.db
.select(
'project',
'permission',
'rp.permission',
'environment',
'type',
'ur.role_id',
)
.from<IPermissionRow>(`${T.ROLE_PERMISSION} AS rp`)
.join(`${T.ROLE_USER} AS ur`, 'ur.role_id', 'rp.role_id')
.join(`${T.PERMISSIONS} AS p`, 'p.id', 'rp.permission_id')
.join(`${T.PERMISSIONS} AS p`, 'p.permission', 'rp.permission')
.where('ur.user_id', '=', userId);

userPermissionQuery = userPermissionQuery.union((db) => {
db.select(
'project',
'permission',
'rp.permission',
'environment',
'p.type',
'gr.role_id',
Expand All @@ -226,14 +223,14 @@ export class AccessStore implements IAccessStore {
.join(`${T.GROUPS} AS g`, 'g.id', 'gu.group_id')
.join(`${T.GROUP_ROLE} AS gr`, 'gu.group_id', 'gr.group_id')
.join(`${T.ROLE_PERMISSION} AS rp`, 'rp.role_id', 'gr.role_id')
.join(`${T.PERMISSIONS} AS p`, 'p.id', 'rp.permission_id')
.join(`${T.PERMISSIONS} AS p`, 'p.permission', 'rp.permission')
.andWhere('gu.user_id', '=', userId);
});

userPermissionQuery = userPermissionQuery.union((db) => {
db.select(
this.db.raw("'default' as project"),
'permission',
'rp.permission',
'environment',
'p.type',
'g.root_role_id as role_id',
Expand All @@ -245,7 +242,7 @@ export class AccessStore implements IAccessStore {
'rp.role_id',
'g.root_role_id',
)
.join(`${T.PERMISSIONS} as p`, 'p.id', 'rp.permission_id')
.join(`${T.PERMISSIONS} as p`, 'p.permission', 'rp.permission')
.whereNotNull('g.root_role_id')
.andWhere('gu.user_id', '=', userId);
});
Expand Down Expand Up @@ -280,13 +277,13 @@ export class AccessStore implements IAccessStore {
const rows = await this.db
.select(
'p.id',
'p.permission',
'rp.permission',
'rp.environment',
'p.display_name',
'p.type',
)
.from<IPermission>(`${T.ROLE_PERMISSION} as rp`)
.join(`${T.PERMISSIONS} as p`, 'p.id', 'rp.permission_id')
.join(`${T.PERMISSIONS} as p`, 'p.permission', 'rp.permission')
.where('rp.role_id', '=', roleId);
stopTimer();
return rows.map((permission) => {
Expand All @@ -304,12 +301,12 @@ export class AccessStore implements IAccessStore {
role_id: number,
permissions: PermissionRef[],
): Promise<void> {
const resolvedPermission = await this.resolvePermissions(permissions);
const resolvedPermissions = await this.resolvePermissions(permissions);

const rows = resolvedPermission.map((permission) => {
const rows = resolvedPermissions.map((permission) => {
return {
role_id,
permission_id: permission.id,
permission: permission.name,
environment: permission.environment,
};
});
Expand Down Expand Up @@ -808,14 +805,14 @@ export class AccessStore implements IAccessStore {
}
});
// no need to pass down the environment in this particular case because it'll be overriden
const permissionsWithIds = await this.resolvePermissions(
const permissionsWithNames = await this.resolvePermissions(
permissionsAsRefs,
);

const newRoles = permissionsWithIds.map((p) => ({
const newRoles = permissionsWithNames.map((p) => ({
role_id,
environment,
permission_id: p.id,
permission: p.name,
}));

return this.db.batchInsert(T.ROLE_PERMISSION, newRoles);
Expand All @@ -826,17 +823,10 @@ export class AccessStore implements IAccessStore {
permission: string,
environment?: string,
): Promise<void> {
const rows = await this.db
.select('id as permissionId')
.from<number>(T.PERMISSIONS)
.where('permission', permission);

const permissionId = rows[0].permissionId;

return this.db(T.ROLE_PERMISSION)
.where({
role_id,
permission_id: permissionId,
permission,
environment,
})
.delete();
Expand All @@ -856,8 +846,8 @@ export class AccessStore implements IAccessStore {
): Promise<void> {
return this.db.raw(
`insert into role_permission
(role_id, permission_id, environment)
(select role_id, permission_id, ?
(role_id, permission, environment)
(select role_id, permission, ?
from ${T.ROLE_PERMISSION} where environment = ?)`,
[destinationEnvironment, sourceEnvironment],
);
Expand Down
Loading

1 comment on commit 023db4e

@vercel
Copy link

@vercel vercel bot commented on 023db4e Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Successfully deployed to the following URLs:

unleash-monorepo-frontend – ./frontend

unleash-monorepo-frontend-unleash-team.vercel.app
unleash-monorepo-frontend-git-main-unleash-team.vercel.app
unleash-monorepo-frontend.vercel.app

Please sign in to comment.