fix: group cleanup (#4334)

src/lib/db/group-store.ts

@@ -1,16 +1,15 @@
 import { IGroupStore, IStoreGroup } from '../types/stores/group-store';
-import { Knex } from 'knex';
 import NotFoundError from '../error/notfound-error';
 import Group, {
+    ICreateGroupModel,
+    ICreateGroupUserModel,
     IGroup,
-    IGroupModel,
     IGroupProject,
     IGroupRole,
     IGroupUser,
-    IGroupUserModel,
 } from '../types/group';
-import Transaction = Knex.Transaction;
 import { Db } from './db';
+import { BadDataError, FOREIGN_KEY_VIOLATION } from '../error';
 
 const T = {
     GROUPS: 'groups',
@@ -81,13 +80,23 @@ export default class GroupStore implements IGroupStore {
         return groups.map(rowToGroup);
     }
 
-    async update(group: IGroupModel): Promise<IGroup> {
-        const rows = await this.db(T.GROUPS)
-            .where({ id: group.id })
-            .update(groupToRow(group))
-            .returning(GROUP_COLUMNS);
+    async update(group: ICreateGroupModel): Promise<IGroup> {
+        try {
+            const rows = await this.db(T.GROUPS)
+                .where({ id: group.id })
+                .update(groupToRow(group))
+                .returning(GROUP_COLUMNS);
 
-        return rowToGroup(rows[0]);
+            return rowToGroup(rows[0]);
+        } catch (error) {
+            if (
+                error.code === FOREIGN_KEY_VIOLATION &&
+                error.constraint === 'fk_group_role_id'
+            ) {
+                throw new BadDataError(`Incorrect role id ${group.rootRole}`);
+            }
+            throw error;
+        }
     }
 
     async getProjectGroupRoles(projectId: string): Promise<IGroupRole[]> {
@@ -176,10 +185,20 @@ export default class GroupStore implements IGroupStore {
     }
 
     async create(group: IStoreGroup): Promise<Group> {
-        const row = await this.db(T.GROUPS)
-            .insert(groupToRow(group))
-            .returning('*');
-        return rowToGroup(row[0]);
+        try {
+            const row = await this.db(T.GROUPS)
+                .insert(groupToRow(group))
+                .returning('*');
+            return rowToGroup(row[0]);
+        } catch (error) {
+            if (
+                error.code === FOREIGN_KEY_VIOLATION &&
+                error.constraint === 'fk_group_role_id'
+            ) {
+                throw new BadDataError(`Incorrect role id ${group.rootRole}`);
+            }
+            throw error;
+        }
     }
 
     async count(): Promise<number> {
@@ -190,25 +209,31 @@ export default class GroupStore implements IGroupStore {
 
     async addUsersToGroup(
         groupId: number,
-        users: IGroupUserModel[],
+        users: ICreateGroupUserModel[],
         userName: string,
-        transaction?: Transaction,
     ): Promise<void> {
-        const rows = (users || []).map((user) => {
-            return {
-                group_id: groupId,
-                user_id: user.user.id,
-                created_by: userName,
-            };
-        });
-        return (transaction || this.db).batchInsert(T.GROUP_USER, rows);
+        try {
+            const rows = (users || []).map((user) => {
+                return {
+                    group_id: groupId,
+                    user_id: user.user.id,
+                    created_by: userName,
+                };
+            });
+            return await this.db.batchInsert(T.GROUP_USER, rows);
+        } catch (error) {
+            if (
+                error.code === FOREIGN_KEY_VIOLATION &&
+                error.constraint === 'group_user_user_id_fkey'
+            ) {
+                throw new BadDataError('Incorrect user id in the users group');
+            }
+            throw error;
+        }
     }
 
-    async deleteUsersFromGroup(
-        deletableUsers: IGroupUser[],
-        transaction?: Transaction,
-    ): Promise<void> {
-        return (transaction || this.db)(T.GROUP_USER)
+    async deleteUsersFromGroup(deletableUsers: IGroupUser[]): Promise<void> {
+        return this.db(T.GROUP_USER)
             .whereIn(
                 ['group_id', 'user_id'],
                 deletableUsers.map((user) => [user.groupId, user.userId]),
@@ -218,14 +243,12 @@ export default class GroupStore implements IGroupStore {
 
     async updateGroupUsers(
         groupId: number,
-        newUsers: IGroupUserModel[],
+        newUsers: ICreateGroupUserModel[],
         deletableUsers: IGroupUser[],
         userName: string,
     ): Promise<void> {
-        await this.db.transaction(async (tx) => {
-            await this.addUsersToGroup(groupId, newUsers, userName, tx);
-            await this.deleteUsersFromGroup(deletableUsers, tx);
-        });
+        await this.addUsersToGroup(groupId, newUsers, userName);
+        await this.deleteUsersFromGroup(deletableUsers);
     }
 
     async getNewGroupsForExternalUser(

src/lib/features/group/createGroupService.ts

@@ -0,0 +1,21 @@
+import { IUnleashConfig } from '../../types';
+import { GroupService } from '../../services';
+import { Db } from '../../db/db';
+import GroupStore from '../../db/group-store';
+import { AccountStore } from '../../db/account-store';
+import EventStore from '../../db/event-store';
+
+export const createGroupService = (
+    db: Db,
+    config: IUnleashConfig,
+): GroupService => {
+    const { getLogger } = config;
+    const groupStore = new GroupStore(db);
+    const accountStore = new AccountStore(db, getLogger);
+    const eventStore = new EventStore(db, getLogger);
+    const groupService = new GroupService(
+        { groupStore, eventStore, accountStore },
+        { getLogger },
+    );
+    return groupService;
+};

src/lib/openapi/index.ts

@@ -152,6 +152,7 @@ import {
     strategyVariantSchema,
     createStrategyVariantSchema,
     clientSegmentSchema,
+    createGroupSchema,
 } from './spec';
 import { IServerOption } from '../types';
 import { mapValues, omitKeys } from '../util';
@@ -361,6 +362,7 @@ export const schemas: UnleashSchemas = {
     strategyVariantSchema,
     createStrategyVariantSchema,
     clientSegmentSchema,
+    createGroupSchema,
 };
 
 // Remove JSONSchema keys that would result in an invalid OpenAPI spec.

src/lib/openapi/spec/create-group-schema.ts

@@ -0,0 +1,43 @@
+import { FromSchema } from 'json-schema-to-ts';
+import { groupSchema } from './group-schema';
+
+export const createGroupSchema = {
+    $id: '#/components/schemas/createGroupSchema',
+    type: 'object',
+    additionalProperties: true,
+    required: ['name'],
+    description: 'A detailed information about a user group',
+    properties: {
+        name: groupSchema.properties.name,
+        description: groupSchema.properties.description,
+        mappingsSSO: groupSchema.properties.mappingsSSO,
+        rootRole: groupSchema.properties.rootRole,
+        users: {
+            type: 'array',
+            description: 'A list of users belonging to this group',
+            items: {
+                type: 'object',
+                description: 'A minimal user object',
+                required: ['user'],
+                properties: {
+                    user: {
+                        type: 'object',
+                        description: 'A minimal user object',
+                        required: ['id'],
+                        properties: {
+                            id: {
+                                description: 'The user id',
+                                type: 'integer',
+                                minimum: 0,
+                                example: 123,
+                            },
+                        },
+                    },
+                },
+            },
+        },
+    },
+    components: {},
+} as const;
+
+export type CreateGroupSchema = FromSchema<typeof createGroupSchema>;

src/lib/openapi/spec/group-schema.ts

@@ -5,7 +5,7 @@ import { userSchema } from './user-schema';
 export const groupSchema = {
     $id: '#/components/schemas/groupSchema',
     type: 'object',
-    additionalProperties: true,
+    additionalProperties: false,
     required: ['name'],
     description: 'A detailed information about a user group',
     properties: {

src/lib/openapi/spec/index.ts

@@ -151,3 +151,4 @@ export * from './create-strategy-variant-schema';
 export * from './strategy-variant-schema';
 export * from './client-segment-schema';
 export * from './update-feature-type-lifetime-schema';
+export * from './create-group-schema';

src/lib/services/group-service.ts

@@ -1,4 +1,5 @@
 import {
+    ICreateGroupModel,
     IGroup,
     IGroupModel,
     IGroupModelWithProjectRole,
@@ -81,7 +82,10 @@ export class GroupService {
         return this.mapGroupWithUsers(group, groupUsers, users);
     }
 
-    async createGroup(group: IGroupModel, userName: string): Promise<IGroup> {
+    async createGroup(
+        group: ICreateGroupModel,
+        userName: string,
+    ): Promise<IGroup> {
         await this.validateGroup(group);
 
         const newGroup = await this.groupStore.create(group);
@@ -101,7 +105,10 @@ export class GroupService {
         return newGroup;
     }
 
-    async updateGroup(group: IGroupModel, userName: string): Promise<IGroup> {
+    async updateGroup(
+        group: ICreateGroupModel,
+        userName: string,
+    ): Promise<IGroup> {
         const preData = await this.groupStore.get(group.id);
 
         await this.validateGroup(group, preData);
@@ -173,7 +180,7 @@ export class GroupService {
     }
 
     async validateGroup(
-        group: IGroupModel,
+        group: ICreateGroupModel,
         existingGroup?: IGroup,
     ): Promise<void> {
         if (!group.name) {

src/lib/services/index.ts

@@ -59,6 +59,7 @@ import {
 import ConfigurationRevisionService from '../features/feature-toggle/configuration-revision-service';
 import { createFeatureToggleService } from '../features';
 import EventAnnouncerService from './event-announcer-service';
+import { createGroupService } from '../features/group/createGroupService';
 
 // TODO: will be moved to scheduler feature directory
 export const scheduleServices = async (
@@ -211,6 +212,8 @@ export const createServices = (
         createExportImportTogglesService(txDb, config);
     const transactionalFeatureToggleService = (txDb: Knex.Transaction) =>
         createFeatureToggleService(txDb, config);
+    const transactionalGroupService = (txDb: Knex.Transaction) =>
+        createGroupService(txDb, config);
     const userSplashService = new UserSplashService(stores, config);
     const openApiService = new OpenApiService(config);
     const clientSpecService = new ClientSpecService(config);
@@ -307,6 +310,7 @@ export const createServices = (
         schedulerService,
         configurationRevisionService,
         transactionalFeatureToggleService,
+        transactionalGroupService,
     };
 };
 

src/lib/types/group.ts

@@ -33,6 +33,11 @@ export interface IGroupModel extends IGroup {
     projects?: string[];
 }
 
+export interface ICreateGroupModel extends IGroup {
+    users?: ICreateGroupUserModel[];
+    projects?: string[];
+}
+
 export interface IGroupProject {
     groupId: number;
     project: string;
@@ -44,6 +49,10 @@ export interface IGroupUserModel {
     createdBy?: string;
 }
 
+export interface ICreateGroupUserModel {
+    user: Pick<IUser, 'id'>;
+}
+
 export interface IGroupModelWithProjectRole extends IGroupModel {
     roleId: number;
     addedAt: Date;

src/lib/types/services.ts

@@ -96,4 +96,5 @@ export interface IUnleashServices {
     transactionalFeatureToggleService: (
         db: Knex.Transaction,
     ) => FeatureToggleService;
+    transactionalGroupService: (db: Knex.Transaction) => GroupService;
 }

src/lib/types/stores/group-store.ts

@@ -1,11 +1,11 @@
 import { Store } from './store';
 import Group, {
+    ICreateGroupModel,
+    ICreateGroupUserModel,
     IGroup,
-    IGroupModel,
     IGroupProject,
     IGroupRole,
     IGroupUser,
-    IGroupUserModel,
 } from '../group';
 
 export interface IStoreGroup {
@@ -38,20 +38,20 @@ export interface IGroupStore extends Store<IGroup, number> {
 
     updateGroupUsers(
         groupId: number,
-        newUsers: IGroupUserModel[],
+        newUsers: ICreateGroupUserModel[],
         deletableUsers: IGroupUser[],
         userName: string,
     ): Promise<void>;
 
     deleteUsersFromGroup(deletableUsers: IGroupUser[]): Promise<void>;
 
-    update(group: IGroupModel): Promise<IGroup>;
+    update(group: ICreateGroupModel): Promise<IGroup>;
 
     getAllUsersByGroups(groupIds: number[]): Promise<IGroupUser[]>;
 
     addUsersToGroup(
         groupId: number,
-        users: IGroupUserModel[],
+        users: ICreateGroupUserModel[],
         userName: string,
     ): Promise<void>;
 

src/test/e2e/services/group-service.e2e.test.ts

@@ -124,3 +124,26 @@ test('adding a root role to a group with a project role should fail', async () =
 
     expect.assertions(1);
 });
+
+test('adding a nonexistent role to a group should fail', async () => {
+    const group = await groupStore.create({
+        name: 'root_group',
+        description: 'root_group',
+    });
+
+    await expect(() => {
+        return groupService.updateGroup(
+            {
+                id: group.id,
+                name: group.name,
+                users: [],
+                rootRole: 100,
+                createdAt: new Date(),
+                createdBy: 'test',
+            },
+            'test',
+        );
+    }).rejects.toThrow(
+        'Request validation failed: your request body or params contain invalid data: Incorrect role id 100',
+    );
+});

src/test/fixtures/fake-group-store.ts

@@ -1,11 +1,11 @@
 import { IGroupStore, IStoreGroup } from '../../lib/types/stores/group-store';
 import Group, {
+    ICreateGroupModel,
+    ICreateGroupUserModel,
     IGroup,
-    IGroupModel,
     IGroupProject,
     IGroupRole,
     IGroupUser,
-    IGroupUserModel,
 } from '../../lib/types/group';
 /* eslint-disable @typescript-eslint/no-unused-vars */
 export default class FakeGroupStore implements IGroupStore {
@@ -48,7 +48,7 @@ export default class FakeGroupStore implements IGroupStore {
 
     addUsersToGroup(
         id: number,
-        users: IGroupUserModel[],
+        users: ICreateGroupUserModel[],
         userName: string,
     ): Promise<void> {
         throw new Error('Method not implemented.');
@@ -62,13 +62,13 @@ export default class FakeGroupStore implements IGroupStore {
         throw new Error('Method not implemented.');
     }
 
-    update(group: IGroupModel): Promise<IGroup> {
+    update(group: ICreateGroupModel): Promise<IGroup> {
         throw new Error('Method not implemented.');
     }
 
     updateGroupUsers(
         groupId: number,
-        newUsers: IGroupUserModel[],
+        newUsers: ICreateGroupUserModel[],
         deletableUsers: IGroupUser[],
         userName: string,
     ): Promise<void> {