Merge pull request #22223 from josecosta-shell/master

Fix :: Prevent Entity Providers from eliminating Users and Groups if synchronisation fails
2024-01-25 16:34:03 +01:00
parent 206c6367a8 4134450da3
commit 8cea347e80
4 changed files with 146 additions and 133 deletions
@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-catalog-backend-module-github': minor
+---
+
+Prevent Entity Providers from eliminating Users and Groups from the DB when the synchronisation fails
@@ -17,10 +17,7 @@
 import { getVoidLogger } from '@backstage/backend-common';
 import { GroupEntity, UserEntity } from '@backstage/catalog-model';
 import { ConfigReader } from '@backstage/config';
-import {
-  GithubCredentialsProvider,
-  GithubIntegrationConfig,
-} from '@backstage/integration';
+import { GithubCredentialsProvider } from '@backstage/integration';
 import { EntityProviderConnection } from '@backstage/plugin-catalog-node';
 import { EventSubscriber } from '@backstage/plugin-events-node';
 import { graphql } from '@octokit/graphql';
@@ -28,6 +25,7 @@ import {
  GithubMultiOrgEntityProvider,
  withLocations,
 } from './GithubMultiOrgEntityProvider';
+import { Logger } from 'winston';

 jest.mock('@octokit/graphql');

@@ -43,11 +41,50 @@ jest.mock('@backstage/integration', () => ({

 describe('GithubMultiOrgEntityProvider', () => {
  describe('read', () => {
+    let mockClient: jest.Mock<any, any, any>;
+    let entityProviderConnection: EntityProviderConnection;
+    let logger: Logger;
+    let gitHubConfig: { host: string };
+    let mockGetCredentials: jest.Mock<any, any, any>;
+    let entityProvider: GithubMultiOrgEntityProvider;
+
+    beforeEach(() => {
+      mockClient = jest.fn();
+      (graphql.defaults as jest.Mock).mockReturnValue(mockClient);
+
+      entityProviderConnection = {
+        applyMutation: jest.fn(),
+        refresh: jest.fn(),
+      };
+
+      logger = getVoidLogger();
+
+      gitHubConfig = { host: 'github.com' };
+
+      mockGetCredentials = jest.fn().mockReturnValue({
+        headers: { token: 'blah' },
+        type: 'app',
+      });
+
+      const githubCredentialsProvider: GithubCredentialsProvider = {
+        getCredentials: mockGetCredentials,
+      };
+
+      entityProvider = new GithubMultiOrgEntityProvider({
+        id: 'my-id',
+        gitHubConfig,
+        githubCredentialsProvider,
+        githubUrl: 'https://github.com',
+        logger,
+        orgs: ['orgA', 'orgB'], // only include for tests that require it
+      });
+
+      entityProvider.connect(entityProviderConnection);
+    });
+
    afterEach(() => jest.resetAllMocks());

    it('should read specified orgs', async () => {
-      const mockClient = jest.fn();
-
      mockClient
        .mockResolvedValueOnce({
          organization: {
@@ -155,36 +192,6 @@ describe('GithubMultiOrgEntityProvider', () => {

      (graphql.defaults as jest.Mock).mockReturnValue(mockClient);

-      const entityProviderConnection: EntityProviderConnection = {
-        applyMutation: jest.fn(),
-        refresh: jest.fn(),
-      };
-
-      const logger = getVoidLogger();
-      const gitHubConfig: GithubIntegrationConfig = {
-        host: 'github.com',
-      };
-
-      const mockGetCredentials = jest.fn().mockReturnValue({
-        headers: { token: 'blah' },
-        type: 'app',
-      });
-
-      const githubCredentialsProvider: GithubCredentialsProvider = {
-        getCredentials: mockGetCredentials,
-      };
-
-      const entityProvider = new GithubMultiOrgEntityProvider({
-        id: 'my-id',
-        gitHubConfig,
-        githubCredentialsProvider,
-        githubUrl: 'https://github.com',
-        logger,
-        orgs: ['orgA', 'orgB'],
-      });
-
-      entityProvider.connect(entityProviderConnection);
-
      await entityProvider.read();

      expect(mockGetCredentials).toHaveBeenCalledWith({
@@ -353,8 +360,6 @@ describe('GithubMultiOrgEntityProvider', () => {
        },
      ]);

-      const mockClient = jest.fn();
-
      mockClient
        .mockResolvedValueOnce({
          organization: {
@@ -462,26 +467,11 @@ describe('GithubMultiOrgEntityProvider', () => {

      (graphql.defaults as jest.Mock).mockReturnValue(mockClient);

-      const entityProviderConnection: EntityProviderConnection = {
-        applyMutation: jest.fn(),
-        refresh: jest.fn(),
-      };
-
-      const logger = getVoidLogger();
-      const gitHubConfig: GithubIntegrationConfig = {
-        host: 'github.com',
-      };
-
-      const mockGetCredentials = jest.fn().mockReturnValue({
-        headers: { token: 'blah' },
-        type: 'app',
-      });
-
      const githubCredentialsProvider: GithubCredentialsProvider = {
        getCredentials: mockGetCredentials,
      };

-      const entityProvider = new GithubMultiOrgEntityProvider({
+      entityProvider = new GithubMultiOrgEntityProvider({
        id: 'my-id',
        gitHubConfig,
        githubCredentialsProvider,
@@ -642,6 +632,16 @@ describe('GithubMultiOrgEntityProvider', () => {
        type: 'full',
      });
    });
+
+    it('should not call applyMutation if an error is thrown', async () => {
+      mockClient.mockImplementationOnce(() => {
+        throw new Error('Network Error');
+      });
+
+      await expect(entityProvider.read()).rejects.toThrow('Network Error');
+
+      expect(entityProviderConnection.applyMutation).not.toHaveBeenCalled();
+    });
  });

  describe('withLocations', () => {
@@ -248,53 +248,49 @@ export class GithubMultiOrgEntityProvider implements EntityProvider {
      : await this.getAllOrgs(this.options.gitHubConfig);

    for (const org of orgsToProcess) {
-      try {
-        const { headers, type: tokenType } =
-          await this.options.githubCredentialsProvider.getCredentials({
-            url: `${this.options.githubUrl}/${org}`,
-          });
-        const client = graphql.defaults({
-          baseUrl: this.options.gitHubConfig.apiBaseUrl,
-          headers,
+      const { headers, type: tokenType } =
+        await this.options.githubCredentialsProvider.getCredentials({
+          url: `${this.options.githubUrl}/${org}`,
        });
+      const client = graphql.defaults({
+        baseUrl: this.options.gitHubConfig.apiBaseUrl,
+        headers,
+      });

-        logger.info(`Reading GitHub users and teams for org: ${org}`);
+      logger.info(`Reading GitHub users and teams for org: ${org}`);

-        const { users } = await getOrganizationUsers(
-          client,
-          org,
-          tokenType,
-          this.options.userTransformer,
-        );
+      const { users } = await getOrganizationUsers(
+        client,
+        org,
+        tokenType,
+        this.options.userTransformer,
+      );

-        const { teams } = await getOrganizationTeams(
-          client,
-          org,
-          this.defaultMultiOrgTeamTransformer.bind(this),
-        );
+      const { teams } = await getOrganizationTeams(
+        client,
+        org,
+        this.defaultMultiOrgTeamTransformer.bind(this),
+      );

-        // Grab current users from `allUsersMap` if they already exist in our
-        // pending users so we can append to their group membership relations
-        const pendingUsers = users.map(u => {
-          const userRef = stringifyEntityRef(u);
-          if (!allUsersMap.has(userRef)) {
-            allUsersMap.set(userRef, u);
-          }
-
-          return allUsersMap.get(userRef);
-        });
-
-        if (areGroupEntities(teams)) {
-          buildOrgHierarchy(teams);
-          if (areUserEntities(pendingUsers)) {
-            assignGroupsToUsers(pendingUsers, teams);
-          }
+      // Grab current users from `allUsersMap` if they already exist in our
+      // pending users so we can append to their group membership relations
+      const pendingUsers = users.map(u => {
+        const userRef = stringifyEntityRef(u);
+        if (!allUsersMap.has(userRef)) {
+          allUsersMap.set(userRef, u);
        }

-        allTeams.push(...teams);
-      } catch (e) {
-        logger.error(`Failed to read GitHub org data for ${org}: ${e}`);
+        return allUsersMap.get(userRef);
+      });
+
+      if (areGroupEntities(teams)) {
+        buildOrgHierarchy(teams);
+        if (areUserEntities(pendingUsers)) {
+          assignGroupsToUsers(pendingUsers, teams);
+        }
      }
+
+      allTeams.push(...teams);
    }

    const allUsers = Array.from(allUsersMap.values());
@@ -30,13 +30,51 @@ jest.mock('@octokit/graphql');

 describe('GithubOrgEntityProvider', () => {
  describe('read', () => {
+    let mockClient;
+    let entityProviderConnection: EntityProviderConnection;
+    let entityProvider: GithubOrgEntityProvider;
+
+    const setupMocks = (response: ((...args: any) => any) | undefined) => {
+      mockClient = jest.fn().mockImplementation(response);
+      (graphql.defaults as jest.Mock).mockReturnValue(mockClient);
+    };
+
+    beforeEach(() => {
+      entityProviderConnection = {
+        applyMutation: jest.fn(),
+        refresh: jest.fn(),
+      };
+
+      const logger = getVoidLogger();
+      const gitHubConfig = {
+        host: 'https://github.com',
+      };
+
+      const mockGetCredentials = jest.fn().mockReturnValue({
+        headers: { token: 'blah' },
+        type: 'app',
+      });
+
+      const githubCredentialsProvider = {
+        getCredentials: mockGetCredentials,
+      };
+
+      entityProvider = new GithubOrgEntityProvider({
+        id: 'my-id',
+        githubCredentialsProvider,
+        orgUrl: 'https://github.com/backstage',
+        gitHubConfig,
+        logger,
+      });
+
+      entityProvider.connect(entityProviderConnection);
+    });
+
    afterEach(() => jest.resetAllMocks());

    it('should read org data and apply mutation', async () => {
-      const mockClient = jest.fn();
-
-      mockClient
-        .mockResolvedValueOnce({
+      setupMocks(() =>
+        Promise.resolve({
          organization: {
            membersWithRole: {
              pageInfo: { hasNextPage: false },
@@ -50,10 +88,6 @@ describe('GithubOrgEntityProvider', () => {
                },
              ],
            },
-          },
-        })
-        .mockResolvedValueOnce({
-          organization: {
            teams: {
              pageInfo: { hasNextPage: false },
              nodes: [
@@ -76,38 +110,8 @@ describe('GithubOrgEntityProvider', () => {
              ],
            },
          },
-        });
-
-      (graphql.defaults as jest.Mock).mockReturnValue(mockClient);
-
-      const entityProviderConnection: EntityProviderConnection = {
-        applyMutation: jest.fn(),
-        refresh: jest.fn(),
-      };
-
-      const logger = getVoidLogger();
-      const gitHubConfig: GithubIntegrationConfig = {
-        host: 'https://github.com',
-      };
-
-      const mockGetCredentials = jest.fn().mockReturnValue({
-        headers: { token: 'blah' },
-        type: 'app',
-      });
-
-      const githubCredentialsProvider: GithubCredentialsProvider = {
-        getCredentials: mockGetCredentials,
-      };
-
-      const entityProvider = new GithubOrgEntityProvider({
-        id: 'my-id',
-        githubCredentialsProvider,
-        orgUrl: 'https://github.com/backstage',
-        gitHubConfig,
-        logger,
-      });
-
-      entityProvider.connect(entityProviderConnection);
+        }),
+      );

      await entityProvider.read();

@@ -171,6 +175,14 @@ describe('GithubOrgEntityProvider', () => {
        type: 'full',
      });
    });
+
+    it('should not apply mutation if a request fails', async () => {
+      setupMocks(() => Promise.reject(new Error('Network error')));
+
+      await expect(entityProvider.read()).rejects.toThrow('Network error');
+
+      expect(entityProviderConnection.applyMutation).not.toHaveBeenCalled();
+    });
  });

  describe('withLocations', () => {