feat: incorporate review changes

Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>

plugins/catalog-backend-module-gitlab/src/__testUtils__/handlers.ts

@@ -210,6 +210,22 @@ const graphqlHandlers = [
           }),
         );
       }
+      // group with no associated members
+      if (group === 'group3' && relations.includes('DIRECT')) {
+        return res(
+          ctx.data({
+            group: {
+              groupMembers: {
+                nodes: [{}],
+                pageInfo: {
+                  endCursor: 'end',
+                  hasNextPage: false,
+                },
+              },
+            },
+          }),
+        );
+      }
       if (group === 'saas-multi-user-group') {
         return res(
           ctx.data({

plugins/catalog-backend-module-gitlab/src/__testUtils__/mocks.ts

@@ -139,7 +139,7 @@ export const config_saas_no_group: MockObject = {
   },
 };
 
-export const config_non_gitlab_host: MockObject = {
+export const config_github_host: MockObject = {
   integrations: {
     github: [
       {
@@ -910,9 +910,9 @@ export const group_destroy_event: EventParams = {
     event_name: 'group_destroy',
     created_at: '2024-02-02T10:53:09Z',
     updated_at: '2024-02-02T10:53:09Z',
-    name: 'group1',
-    path: 'group1',
-    full_path: 'group1',
+    name: 'group3',
+    path: 'group3',
+    full_path: 'group3',
     group_id: 123,
   },
 };
@@ -951,10 +951,10 @@ export const group_create_event: EventParams = {
     event_name: 'group_create',
     created_at: '2024-02-02T10:53:09Z',
     updated_at: '2024-02-02T10:53:09Z',
-    name: 'group1',
-    path: 'group1',
-    full_path: 'group1',
-    group_id: 1,
+    name: 'group3',
+    path: 'group3',
+    full_path: 'group3',
+    group_id: 3,
   },
 };
 export const group_create_event_unmatched: EventParams = {
@@ -1038,9 +1038,9 @@ export const user_remove_from_group_event: EventParams = {
   eventPayload: {
     created_at: '2024-02-02T10:53:09Z',
     updated_at: '2024-02-02T10:53:09Z',
-    group_name: 'group1',
-    group_path: 'my-groups/group1',
-    group_id: 1,
+    group_name: 'group3',
+    group_path: 'my-groups/group3',
+    group_id: 3,
     user_username: 'user1',
     user_name: 'John Doe',
     user_email: 'john.doe@company.com',
@@ -1382,17 +1382,17 @@ export const expected_group_entity: MockObject[] = [
       kind: 'Group',
       metadata: {
         annotations: {
-          'backstage.io/managed-by-location': 'url:https://example.com/group1',
+          'backstage.io/managed-by-location': 'url:https://example.com/group3',
           'backstage.io/managed-by-origin-location':
-            'url:https://example.com/group1',
-          'example.com/team-path': 'group1',
+            'url:https://example.com/group3',
+          'example.com/team-path': 'group3',
         },
-        name: 'group1',
+        name: 'group3',
       },
       spec: {
         children: [],
         profile: {
-          displayName: 'group1',
+          displayName: 'group3',
         },
         type: 'team',
       },
@@ -1489,17 +1489,44 @@ export const expected_transformed_group_entity: MockObject[] = [
       kind: 'Group',
       metadata: {
         annotations: {
-          'backstage.io/managed-by-location': 'url:https://example.com/group1',
+          'backstage.io/managed-by-location': 'url:https://example.com/group3',
           'backstage.io/managed-by-origin-location':
-            'url:https://example.com/group1',
-          'example.com/team-path': 'group1',
+            'url:https://example.com/group3',
+          'example.com/team-path': 'group3',
         },
-        name: '1',
+        name: '3',
       },
       spec: {
         children: [],
         profile: {
-          displayName: 'group1',
+          displayName: 'group3',
+        },
+        type: 'team',
+      },
+    },
+    locationKey: 'GitlabOrgDiscoveryEntityProvider:test-id',
+  },
+];
+
+export const expected_removed_user_entity: MockObject[] = [
+  {
+    entity: {
+      apiVersion: 'backstage.io/v1alpha1',
+      kind: 'Group',
+      metadata: {
+        annotations: {
+          'backstage.io/managed-by-location': 'url:https://example.com/group3',
+          'backstage.io/managed-by-origin-location':
+            'url:https://example.com/group3',
+          'example.com/team-path': 'group3',
+        },
+        name: '3',
+      },
+      spec: {
+        children: [],
+        members: [],
+        profile: {
+          displayName: 'group3',
         },
         type: 'team',
       },
@@ -1661,8 +1688,29 @@ export const expected_full_org_scan_entities: MockObject[] = [
     },
     locationKey: 'GitlabOrgDiscoveryEntityProvider:test-id',
   },
-
-  expected_group_entity[0],
+  {
+    entity: {
+      apiVersion: 'backstage.io/v1alpha1',
+      kind: 'Group',
+      metadata: {
+        annotations: {
+          'backstage.io/managed-by-location': 'url:https://example.com/group1',
+          'backstage.io/managed-by-origin-location':
+            'url:https://example.com/group1',
+          'example.com/team-path': 'group1',
+        },
+        name: 'group1',
+      },
+      spec: {
+        children: [],
+        profile: {
+          displayName: 'group1',
+        },
+        type: 'team',
+      },
+    },
+    locationKey: 'GitlabOrgDiscoveryEntityProvider:test-id',
+  },
 ];
 
 export const expected_full_org_scan_entities_saas: MockObject[] = [

plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts

@@ -87,7 +87,7 @@ describe('GitlabDiscoveryEntityProvider - configuration', () => {
   });
   it('should throw error when no matching GitLab integration config found', () => {
     const schedule = new PersistingTaskRunner();
-    const config = new ConfigReader(mock.config_non_gitlab_host);
+    const config = new ConfigReader(mock.config_github_host);
 
     expect(() => {
       GitlabDiscoveryEntityProvider.fromConfig(config, {

plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts

@@ -61,6 +61,7 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider {
   private readonly scheduleFn: () => Promise<void>;
   private connection?: EntityProviderConnection;
   private readonly events?: EventsService;
+  private readonly gitLabClient: GitLabClient;
 
   static fromConfig(
     config: Config,
@@ -129,6 +130,10 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider {
     });
     this.scheduleFn = this.createScheduleFn(options.taskRunner);
     this.events = options.events;
+    this.gitLabClient = new GitLabClient({
+      config: this.integration.config,
+      logger: this.logger,
+    });
   }
 
   getProviderName(): string {
@@ -196,13 +201,9 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider {
         `Gitlab discovery connection not initialized for ${this.getProviderName()}`,
       );
     }
-    const client = new GitLabClient({
-      config: this.integration.config,
-      logger: logger,
-    });
 
     const projects = paginated<GitLabProject>(
-      options => client.listProjects(options),
+      options => this.gitLabClient.listProjects(options),
       {
         archived: false,
         group: this.config.group,
@@ -217,13 +218,17 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider {
     };
 
     for await (const project of projects) {
-      if (await this.shouldProcessProject(project, client)) {
+      if (await this.shouldProcessProject(project, this.gitLabClient)) {
         res.scanned++;
         res.matches.push(project);
       }
     }
 
     const locations = res.matches.map(p => this.createLocationSpec(p));
+
+    logger.info(
+      `Processed ${locations.length} from scanned ${res.scanned} projects.`,
+    );
     await this.connection.applyMutation({
       type: 'full',
       entities: locations.map(location => ({
@@ -261,12 +266,7 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider {
       `Received push event for ${event.project.path_with_namespace}`,
     );
 
-    const client = new GitLabClient({
-      config: this.integration.config,
-      logger: this.logger,
-    });
-
-    const project = await client.getProjectById(event.project_id);
+    const project = await this.gitLabClient.getProjectById(event.project_id);
 
     if (!project) {
       this.logger.debug(
@@ -276,7 +276,7 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider {
       return;
     }
 
-    if (!(await this.shouldProcessProject(project, client))) {
+    if (!(await this.shouldProcessProject(project, this.gitLabClient))) {
       this.logger.debug(`Skipping event ${event.project.path_with_namespace}`);
       return;
     }

plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.test.ts

@@ -63,7 +63,7 @@ describe('GitlabOrgDiscoveryEntityProvider - configuration', () => {
 
   it('should throw error when no matching GitLab integration config found', () => {
     const schedule = new PersistingTaskRunner();
-    const config = new ConfigReader(mock.config_non_gitlab_host);
+    const config = new ConfigReader(mock.config_github_host);
 
     expect(() => {
       GitlabOrgDiscoveryEntityProvider.fromConfig(config, {
@@ -625,7 +625,7 @@ describe('GitlabOrgDiscoveryEntityProvider with events support', () => {
     expect(entityProviderConnection.applyMutation).toHaveBeenCalledTimes(1);
     expect(entityProviderConnection.applyMutation).toHaveBeenCalledWith({
       type: 'delta',
-      added: mock.expected_transformed_group_entity,
+      added: mock.expected_removed_user_entity,
       removed: [],
     });
   });

plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.ts

@@ -107,6 +107,7 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
   private userTransformer: UserTransformer;
   private groupEntitiesTransformer: GroupEntitiesTransformer;
   private groupNameTransformer: GroupNameTransformer;
+  private readonly gitLabClient: GitLabClient;
 
   static fromConfig(
     config: Config,
@@ -192,6 +193,11 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
       options.groupEntitiesTransformer ?? defaultGroupEntitiesTransformer;
     this.groupNameTransformer =
       options.groupNameTransformer ?? defaultGroupNameTransformer;
+
+    this.gitLabClient = new GitLabClient({
+      config: this.integration.config,
+      logger: this.logger,
+    });
   }
 
   getProviderName(): string {
@@ -346,31 +352,33 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
       );
     }
 
-    const client = new GitLabClient({
-      config: this.integration.config,
-      logger: logger,
-    });
-
     let groups;
     let users;
 
-    if (client.isSelfManaged()) {
-      groups = paginated<GitLabGroup>(options => client.listGroups(options), {
-        page: 1,
-        per_page: 100,
-        all_available: true,
-      });
+    if (this.gitLabClient.isSelfManaged()) {
+      groups = paginated<GitLabGroup>(
+        options => this.gitLabClient.listGroups(options),
+        {
+          page: 1,
+          per_page: 100,
+          all_available: true,
+        },
+      );
 
-      users = paginated<GitLabUser>(options => client.listUsers(options), {
-        page: 1,
-        per_page: 100,
-        active: true,
-      });
+      users = paginated<GitLabUser>(
+        options => this.gitLabClient.listUsers(options),
+        {
+          page: 1,
+          per_page: 100,
+          active: true,
+        },
+      );
     } else {
-      groups = (await client.listDescendantGroups(this.config.group)).items;
+      groups = (await this.gitLabClient.listDescendantGroups(this.config.group))
+        .items;
       const rootGroup = this.config.group.split('/')[0];
       users = paginated<GitLabUser>(
-        options => client.listSaaSUsers(rootGroup, options),
+        options => this.gitLabClient.listSaaSUsers(rootGroup, options),
         {
           page: 1,
           per_page: 100,
@@ -418,7 +426,10 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
         const relations = this.config.allowInherited
           ? ['DIRECT', 'INHERITED']
           : ['DIRECT'];
-        groupUsers = await client.getGroupMembers(group.full_path, relations);
+        groupUsers = await this.gitLabClient.getGroupMembers(
+          group.full_path,
+          relations,
+        );
       } catch (e) {
         logger.error(
           `Failed fetching users for group '${group.full_path}': ${e}`,
@@ -488,12 +499,6 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
       );
     }
 
-    const getClient = () =>
-      new GitLabClient({
-        config: this.integration.config,
-        logger: this.logger,
-      });
-
     let group: GitLabGroup | undefined;
     if (event.event_name === 'group_destroy') {
       group = {
@@ -504,8 +509,7 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
         parent_id: 0,
       };
     } else {
-      const client = getClient();
-      group = await client.getGroupById(event.group_id);
+      group = await this.gitLabClient.getGroupById(event.group_id);
     }
 
     if (!this.shouldProcessGroup(group)) {
@@ -522,12 +526,7 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
 
     // we need to fetch the parent group's object because its representation might be changed by the groupTransformer
     if (group.parent_id) {
-      const client = new GitLabClient({
-        config: this.integration.config,
-        logger: this.logger,
-      });
-
-      const parentGroup = await client.getGroupById(group.parent_id);
+      const parentGroup = await this.gitLabClient.getGroupById(group.parent_id);
 
       groupEntity[0].spec.parent = this.groupNameTransformer({
         group: parentGroup,
@@ -575,12 +574,7 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
       groupNameTransformer: this.groupNameTransformer,
     });
 
-    const client = new GitLabClient({
-      config: this.integration.config,
-      logger: this.logger,
-    });
-
-    const groupToAdd = await client.getGroupById(event.group_id);
+    const groupToAdd = await this.gitLabClient.getGroupById(event.group_id);
 
     if (!this.shouldProcessGroup(groupToAdd)) {
       this.logger.debug(`Skipped group ${groupToAdd.full_path}.`);
@@ -594,7 +588,9 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
     });
 
     if (groupToAdd.parent_id) {
-      const parentGroup = await client.getGroupById(groupToAdd.parent_id);
+      const parentGroup = await this.gitLabClient.getGroupById(
+        groupToAdd.parent_id,
+      );
 
       groupEntityToAdd[0].spec.parent = this.groupNameTransformer({
         group: parentGroup,
@@ -602,8 +598,8 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
       });
     }
 
-    const { added } = createDeltaOperation([...groupEntityToAdd]);
-    const { removed } = createDeltaOperation([...groupEntityToRemove]);
+    const { added } = createDeltaOperation(groupEntityToAdd);
+    const { removed } = createDeltaOperation(groupEntityToRemove);
 
     this.logger.debug(`Applying mutation for group ${groupToAdd.full_path}.`);
     await this.connection.applyMutation({
@@ -626,7 +622,9 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
       );
     }
 
-    let user: GitLabUser;
+    let user: GitLabUser | undefined = undefined;
+
+    // if user destroy event is received, retrieve user data from the event itself
     if (event.event_name === 'user_destroy') {
       user = {
         id: event.user_id,
@@ -637,13 +635,18 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
         web_url: '',
         avatar_url: '',
       };
-    } else {
-      const client = new GitLabClient({
-        config: this.integration.config,
-        logger: this.logger,
-      });
+    }
 
-      user = await client.getUserById(event.user_id);
+    // if user create event received fetch data from gitlab
+    if (event.event_name === 'user_create') {
+      user = await this.gitLabClient.getUserById(event.user_id);
+    }
+
+    if (!user) {
+      this.logger.debug(
+        `Couldn't retrieve user data. Skipped ${event.event_name} event processing for user ${event.username}`,
+      );
+      return;
     }
 
     if (!this.shouldProcessUser(user)) {
@@ -681,16 +684,19 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
       );
     }
 
-    const client = new GitLabClient({
-      config: this.integration.config,
-      logger: this.logger,
-    });
-
-    const groupToRebuild: GitLabGroup = await client.getGroupById(
+    // fetch group data from GitLab
+    const groupToRebuild: GitLabGroup = await this.gitLabClient.getGroupById(
       event.group_id,
     );
 
-    // If the group is outside the scope there is no point creating anything related to it.
+    if (!groupToRebuild) {
+      this.logger.debug(
+        `Couldn't retrieve group data. Skipped ${event.event_name} event processing.`,
+      );
+      return;
+    }
+
+    // if the group is outside the scope there is no point creating anything related to it.
     if (!this.shouldProcessGroup(groupToRebuild)) {
       this.logger.debug(`Skipped group ${groupToRebuild.full_path}.`);
       return;
@@ -700,16 +706,12 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
       ? ['DIRECT', 'INHERITED']
       : ['DIRECT'];
 
-    const groupMembers = await client.getGroupMembers(
+    // fetch group members from GitLab
+    const groupMembers = await this.gitLabClient.getGroupMembers(
       groupToRebuild.full_path,
       relations,
     );
-
-    // new members of the group
-    const usersToBeAdded =
-      event.event_name === 'user_remove_from_group'
-        ? groupMembers.items.filter(e => e.username !== event.user_username)
-        : groupMembers.items;
+    const usersToBeAdded = groupMembers.items;
 
     const groupEntityToModify = await this.groupEntitiesTransformer({
       groups: [groupToRebuild],
@@ -719,19 +721,22 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
 
     // we need to fetch the parent group's object because its representation might be changed by the groupTransformer
     if (groupToRebuild.parent_id) {
-      const parentGroup = await client.getGroupById(groupToRebuild.parent_id);
+      const parentGroup = await this.gitLabClient.getGroupById(
+        groupToRebuild.parent_id,
+      );
 
+      // update parent of the group entity
       groupEntityToModify[0].spec.parent = this.groupNameTransformer({
         group: parentGroup,
         providerConfig: this.config,
       });
     }
 
-    if (usersToBeAdded.length !== 0) {
-      groupEntityToModify[0].spec.members = usersToBeAdded.map(e => e.username);
-    }
+    // update members of the group entity
+    groupEntityToModify[0].spec.members =
+      usersToBeAdded.length !== 0 ? usersToBeAdded.map(e => e.username) : [];
 
-    const { added, removed } = createDeltaOperation([...groupEntityToModify]);
+    const { added, removed } = createDeltaOperation(groupEntityToModify);
 
     this.logger.debug(
       `Applying mutation for group ${groupToRebuild.full_path}.`,