From 2167b7eab09bea146fc71816d409883e4465b858 Mon Sep 17 00:00:00 2001
From: Robert Bunning <rbunning@webstaurantstore.com>
Date: Mon, 31 Jul 2023 12:51:30 -0400
Subject: [PATCH 1/9] Add pagination support to newrelic plugin

Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
---
 .changeset/khaki-camels-rush.md        |   5 +
 app-config.yaml                        |   2 +
 plugins/newrelic/README.md             |   4 +
 plugins/newrelic/src/api/index.test.ts | 312 +++++++++++++++++++++++++
 plugins/newrelic/src/api/index.ts      |  91 ++++++--
 plugins/newrelic/src/plugin.ts         |   9 +-
 6 files changed, 408 insertions(+), 15 deletions(-)
 create mode 100644 .changeset/khaki-camels-rush.md
 create mode 100644 plugins/newrelic/src/api/index.test.ts

diff --git a/.changeset/khaki-camels-rush.md b/.changeset/khaki-camels-rush.md
new file mode 100644
index 0000000000..a38600abf3
--- /dev/null
+++ b/.changeset/khaki-camels-rush.md
@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-newrelic': patch
+---
+
+The newrelic plugin now supports pagination when retrieving results from newrelic. It will no longer truncate results. To see all applications, the link header will need to be allowed through the proxy (see the newrelic plugin readme).
diff --git a/app-config.yaml b/app-config.yaml
index 2b562ee689..da505d390b 100644
--- a/app-config.yaml
+++ b/app-config.yaml
@@ -70,6 +70,8 @@ proxy:
     target: https://api.newrelic.com/v2
     headers:
       X-Api-Key: ${NEW_RELIC_REST_API_KEY}
+    allowedHeaders:
+      - link
 
   '/newrelic/api':
     target: https://api.newrelic.com
diff --git a/plugins/newrelic/README.md b/plugins/newrelic/README.md
index 8ac7ed2c72..14049ba39c 100644
--- a/plugins/newrelic/README.md
+++ b/plugins/newrelic/README.md
@@ -18,6 +18,8 @@ APIs.
         target: https://api.newrelic.com/v2
         headers:
           X-Api-Key: ${NEW_RELIC_REST_API_KEY}
+        allowedHeaders:
+          - link
     ```
 
     There is some types of api key on new relic, to this use must be `User` type of key, In your production deployment of Backstage, you would also need to ensure that
@@ -33,6 +35,8 @@ APIs.
       '/newrelic/apm/api':
         headers:
           X-Api-Key: NRRA-YourActualApiKey
+        allowedHeaders:
+          - link
     ```
 
     Read more about how to find or generate this key in
diff --git a/plugins/newrelic/src/api/index.test.ts b/plugins/newrelic/src/api/index.test.ts
new file mode 100644
index 0000000000..13d9c5304d
--- /dev/null
+++ b/plugins/newrelic/src/api/index.test.ts
@@ -0,0 +1,312 @@
+/*
+ * Copyright 2023 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { NewRelicClient } from '.';
+import { FetchApi } from '@backstage/core-plugin-api';
+import { DiscoveryApi } from '@backstage/core-plugin-api';
+
+beforeEach(() => {
+  jest.resetAllMocks();
+});
+
+describe('NewRelicClient', () => {
+  test.each([
+    ['https://test.test/BASEPATH/apm/api/applications.json', '/BASEPATH'],
+    ['https://test.test/BASEPATH2/apm/api/applications.json', '/BASEPATH2'],
+    ['https://test.testBASEPATH3/apm/api/applications.json', 'BASEPATH3'],
+    ['https://test.test/newrelic/apm/api/applications.json', undefined],
+  ])(
+    'It correctly forms the request url (%p) when proxyPathBase is %p',
+    async (expectedUrl, basePathOverride) => {
+      const mockedDiscoveryApi: DiscoveryApi = {
+        getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
+      };
+
+      const mockedFetchApi: FetchApi = {
+        fetch: jest.fn().mockResolvedValueOnce({
+          ok: true,
+          json: async () => [],
+          headers: new Map<string, string | null>(),
+        }),
+      };
+
+      const client = new NewRelicClient({
+        discoveryApi: mockedDiscoveryApi,
+        fetchApi: mockedFetchApi,
+        proxyPathBase: basePathOverride,
+      });
+      await client.getApplications();
+
+      expect(mockedFetchApi.fetch).toHaveBeenCalledWith(expectedUrl);
+    },
+  );
+
+  it('Correctly reads all pages of results and returns the expected results', async () => {
+    const mockedDiscoveryApi: DiscoveryApi = {
+      getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
+    };
+
+    const mockedApplicationOne = {
+      id: 1,
+      application_summary: {
+        apdex_score: 0,
+        error_rate: 100,
+        host_count: 500,
+        instance_count: 5000,
+        response_time: 20,
+        throughput: 500000,
+      },
+      name: 'Testing Application #1',
+      language: 'en-us',
+      health_status: 'Failing',
+      reporting: true,
+      settings: {
+        app_apdex_threshold: 0,
+        end_user_apdex_threshold: 0,
+        enable_real_user_monitoring: true,
+        use_server_side_config: true,
+      },
+    };
+
+    const mockedApplicationTwo = {
+      id: 2,
+      name: 'Testing Application #2',
+      language: 'en-us',
+      health_status: 'Working',
+      reporting: true,
+      settings: {
+        app_apdex_threshold: 0,
+        end_user_apdex_threshold: 0,
+        enable_real_user_monitoring: true,
+        use_server_side_config: true,
+      },
+    };
+
+    const mockedApplicationThree = {
+      id: 3,
+      application_summary: {
+        apdex_score: -900,
+        error_rate: 0,
+        host_count: 0,
+        instance_count: 0,
+        response_time: 0,
+        throughput: 0,
+      },
+      name: 'Testing Application #3',
+      language: 'en-us',
+      health_status: 'Waiting',
+      reporting: false,
+      settings: {
+        app_apdex_threshold: 1000,
+        end_user_apdex_threshold: 500,
+        enable_real_user_monitoring: false,
+        use_server_side_config: false,
+      },
+    };
+
+    const mockedFetchApi: FetchApi = {
+      fetch: jest
+        .fn()
+        .mockResolvedValueOnce({
+          ok: true,
+          json: async () => ({ applications: [mockedApplicationOne] }),
+          headers: new Map<string, string | null>([
+            [
+              'link',
+              '<https://next.page/page2>; rel="next", <https://badsite.dontgohere>; rel="next"',
+            ],
+            ['otherheader', 'otherValue'],
+          ]),
+        })
+        .mockResolvedValueOnce({
+          ok: true,
+          json: async () => ({ applications: [] }),
+          headers: new Map<string, string | null>([
+            ['Link', '<https://next.page/page3>; rel="next",'],
+          ]),
+        })
+        .mockResolvedValueOnce({
+          ok: true,
+          json: async () => ({
+            applications: [mockedApplicationTwo, mockedApplicationThree],
+          }),
+          headers: new Map<string, string | null>([
+            [
+              'link',
+              '<https://first.page/page1>; rel="first", <https://next.page/page2>; rel="next"',
+            ],
+          ]),
+        }),
+    };
+
+    const client = new NewRelicClient({
+      discoveryApi: mockedDiscoveryApi,
+      fetchApi: mockedFetchApi,
+    });
+    const actual = await client.getApplications();
+    const expected = {
+      applications: [
+        mockedApplicationOne,
+        mockedApplicationTwo,
+        mockedApplicationThree,
+      ],
+    };
+
+    expect(mockedFetchApi.fetch).toHaveBeenCalledTimes(3);
+    expect(mockedFetchApi.fetch).toHaveBeenCalledWith(
+      'https://test.test/newrelic/apm/api/applications.json',
+    );
+    expect(mockedFetchApi.fetch).toHaveBeenCalledWith(
+      'https://next.page/page2',
+    );
+    expect(mockedFetchApi.fetch).toHaveBeenCalledWith(
+      'https://next.page/page3',
+    );
+    expect(actual).toStrictEqual(expected);
+  });
+
+  test.each([['LINK'], ['lINK']])(
+    'It does not attempt pagination when the link header name is invalid (%p)',
+    async linkHeaderName => {
+      const mockedDiscoveryApi: DiscoveryApi = {
+        getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
+      };
+
+      const mockedFetchApi: FetchApi = {
+        fetch: jest.fn().mockResolvedValueOnce({
+          ok: true,
+          json: async () => ({ applications: [] }),
+          headers: new Map<string, string | null>([
+            [
+              linkHeaderName,
+              '<https://next.page/page2>; rel="next", <https://badsite.dontgohere>; rel="next"',
+            ],
+            ['otherheader', 'otherValue'],
+          ]),
+        }),
+      };
+
+      const client = new NewRelicClient({
+        discoveryApi: mockedDiscoveryApi,
+        fetchApi: mockedFetchApi,
+      });
+      await client.getApplications();
+
+      expect(mockedFetchApi.fetch).toHaveBeenCalledTimes(1);
+      expect(mockedFetchApi.fetch).toHaveBeenCalledWith(
+        'https://test.test/newrelic/apm/api/applications.json',
+      );
+    },
+  );
+
+  test.each([
+    [''],
+    ['<> rel=""'],
+    ['<>; rel=""'],
+    ['<https://next.page>; rel=""'],
+    ['<>; rel:"value"'],
+    ['<https://next.page>; rel: "next"'],
+    ['ABCDE'],
+  ])(
+    'It does not attempt pagination when the link header value is invalid (%p)',
+    async linkHeaderValue => {
+      const mockedDiscoveryApi: DiscoveryApi = {
+        getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
+      };
+
+      const mockedFetchApi: FetchApi = {
+        fetch: jest.fn().mockResolvedValueOnce({
+          ok: true,
+          json: async () => ({ applications: [] }),
+          headers: new Map<string, string | null>([
+            ['Link', linkHeaderValue],
+            ['otherheader', 'otherValue'],
+          ]),
+        }),
+      };
+
+      const client = new NewRelicClient({
+        discoveryApi: mockedDiscoveryApi,
+        fetchApi: mockedFetchApi,
+      });
+      await client.getApplications();
+
+      expect(mockedFetchApi.fetch).toHaveBeenCalledTimes(1);
+      expect(mockedFetchApi.fetch).toHaveBeenCalledWith(
+        'https://test.test/newrelic/apm/api/applications.json',
+      );
+    },
+  );
+
+  test.each([
+    [
+      {
+        ok: false,
+        statusText: 'statusText',
+        json: async () => ({ error: { title: 'TESTING' } }),
+      },
+    ],
+    [
+      {
+        ok: false,
+        statusText: 'statusText',
+        json: async () => ({}),
+      },
+    ],
+  ])(
+    'It returns an empty array of applications when the fetch is not okay',
+    async fetchResult => {
+      const mockedDiscoveryApi: DiscoveryApi = {
+        getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
+      };
+
+      const mockedFetchApi: FetchApi = {
+        fetch: jest.fn().mockResolvedValueOnce({
+          ok: true,
+          json: async () => fetchResult,
+        }),
+      };
+
+      const client = new NewRelicClient({
+        discoveryApi: mockedDiscoveryApi,
+        fetchApi: mockedFetchApi,
+      });
+      const actual = await client.getApplications();
+
+      expect(actual).toStrictEqual({ applications: [] });
+    },
+  );
+
+  it('Returns an empty array when the fetch itself throws an error', async () => {
+    const mockedDiscoveryApi: DiscoveryApi = {
+      getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
+    };
+
+    const mockedFetchApi: FetchApi = {
+      fetch: () => {
+        throw new Error('TESTING');
+      },
+    };
+
+    const client = new NewRelicClient({
+      discoveryApi: mockedDiscoveryApi,
+      fetchApi: mockedFetchApi,
+    });
+    const actual = await client.getApplications();
+
+    expect(actual).toStrictEqual({ applications: [] });
+  });
+});
diff --git a/plugins/newrelic/src/api/index.ts b/plugins/newrelic/src/api/index.ts
index 2734dd2b4f..9c95f3fe1f 100644
--- a/plugins/newrelic/src/api/index.ts
+++ b/plugins/newrelic/src/api/index.ts
@@ -14,7 +14,11 @@
  * limitations under the License.
  */
 
-import { createApiRef, DiscoveryApi } from '@backstage/core-plugin-api';
+import {
+  createApiRef,
+  DiscoveryApi,
+  FetchApi,
+} from '@backstage/core-plugin-api';
 
 export type NewRelicApplication = {
   id: number;
@@ -61,6 +65,7 @@ const DEFAULT_PROXY_PATH_BASE = '/newrelic';
 
 type Options = {
   discoveryApi: DiscoveryApi;
+  fetchApi: FetchApi;
   /**
    * Path to use for requests via the proxy, defaults to /newrelic
    */
@@ -71,27 +76,58 @@ export interface NewRelicApi {
   getApplications(): Promise<NewRelicApplications>;
 }
 
+interface PaginationInformation {
+  nextPageLink: string;
+  relation: string;
+}
+
+interface NewRelicPageReadResult {
+  hasMoreApplications: boolean;
+  pagination: PaginationInformation | undefined;
+  applicationsFromReadPage: NewRelicApplication[];
+}
+
 export class NewRelicClient implements NewRelicApi {
   private readonly discoveryApi: DiscoveryApi;
+  private readonly fetchApi: FetchApi;
   private readonly proxyPathBase: string;
 
   constructor(options: Options) {
     this.discoveryApi = options.discoveryApi;
+    this.fetchApi = options.fetchApi;
     this.proxyPathBase = options.proxyPathBase ?? DEFAULT_PROXY_PATH_BASE;
   }
 
   async getApplications(): Promise<NewRelicApplications> {
-    const url = await this.getApiUrl('apm', 'applications.json');
-    const response = await fetch(url);
-    let responseJson;
+    const proxyUrl = await this.discoveryApi.getBaseUrl('proxy');
+    let targetUrl = `${proxyUrl}${this.proxyPathBase}/apm/api/applications.json`;
+    let hasNextPage = true;
 
     try {
-      responseJson = await response.json();
-    } catch (e) {
-      responseJson = { applications: [] };
-    }
+      let applications: NewRelicApplication[] = [];
 
-    if (response.status !== 200) {
+      do {
+        const { hasMoreApplications, pagination, applicationsFromReadPage } =
+          await this.fetchNewRelic(targetUrl);
+
+        hasNextPage = hasMoreApplications;
+        targetUrl = pagination!.nextPageLink;
+        applications = applications.concat(applicationsFromReadPage);
+      } while (hasNextPage);
+
+      return { applications };
+    } catch (e) {
+      return { applications: [] };
+    }
+  }
+
+  private async fetchNewRelic(
+    targetUrl: string,
+  ): Promise<NewRelicPageReadResult> {
+    const response = await this.fetchApi.fetch(targetUrl);
+    const responseJson = await response.json();
+
+    if (!response.ok) {
       throw new Error(
         `Error communicating with New Relic: ${
           responseJson?.error?.title || response.statusText
@@ -99,11 +135,40 @@ export class NewRelicClient implements NewRelicApi {
       );
     }
 
-    return responseJson;
+    const readResponse = responseJson as NewRelicApplications;
+    const linkHeader =
+      response.headers.get('Link') || response.headers.get('link');
+    const pagination = this.parseLinkHeader(linkHeader);
+
+    return {
+      hasMoreApplications: !!(pagination && pagination.relation === 'next'),
+      pagination,
+      applicationsFromReadPage: readResponse.applications,
+    };
   }
 
-  private async getApiUrl(product: string, path: string) {
-    const proxyUrl = await this.discoveryApi.getBaseUrl('proxy');
-    return `${proxyUrl}${this.proxyPathBase}/${product}/api/${path}`;
+  private parseLinkHeader(
+    linkHeader: string | null,
+  ): PaginationInformation | undefined {
+    if (!linkHeader) {
+      return undefined;
+    }
+
+    const nextRelevantLink = linkHeader.replaceAll(' ', '').split(',')[0];
+
+    // Link should be of the format <nextPageLink>;rel="relation"
+    const linkParts = nextRelevantLink.match(/^<(.+)>;rel="(.+)"$/);
+    const nextPageLink = linkParts?.[1];
+    const relation = linkParts?.[2];
+    const isValidLink = !!(!!linkParts && nextPageLink && relation);
+
+    if (!nextRelevantLink || !isValidLink) {
+      return undefined;
+    }
+
+    return {
+      nextPageLink,
+      relation,
+    };
   }
 }
diff --git a/plugins/newrelic/src/plugin.ts b/plugins/newrelic/src/plugin.ts
index 0387c50aa9..da7cae9b93 100644
--- a/plugins/newrelic/src/plugin.ts
+++ b/plugins/newrelic/src/plugin.ts
@@ -20,6 +20,7 @@ import {
   createPlugin,
   createRouteRef,
   discoveryApiRef,
+  fetchApiRef,
   createRoutableExtension,
 } from '@backstage/core-plugin-api';
 
@@ -33,8 +34,12 @@ export const newRelicPlugin = createPlugin({
   apis: [
     createApiFactory({
       api: newRelicApiRef,
-      deps: { discoveryApi: discoveryApiRef },
-      factory: ({ discoveryApi }) => new NewRelicClient({ discoveryApi }),
+      deps: {
+        discoveryApi: discoveryApiRef,
+        fetchApi: fetchApiRef,
+      },
+      factory: ({ discoveryApi, fetchApi }) =>
+        new NewRelicClient({ discoveryApi, fetchApi }),
     }),
   ],
   routes: {

From f55b6fd1e28e0dd67a6974a2acb507e2b3538221 Mon Sep 17 00:00:00 2001
From: Robert Bunning <rbunning@webstaurantstore.com>
Date: Tue, 1 Aug 2023 11:20:31 -0400
Subject: [PATCH 2/9] Ensure subsequent page reads are run through the proxy

Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
---
 plugins/newrelic/src/api/index.test.ts | 34 +++++++++++++++++++++++---
 plugins/newrelic/src/api/index.ts      | 22 +++++++++++------
 2 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/plugins/newrelic/src/api/index.test.ts b/plugins/newrelic/src/api/index.test.ts
index 13d9c5304d..69cf74066f 100644
--- a/plugins/newrelic/src/api/index.test.ts
+++ b/plugins/newrelic/src/api/index.test.ts
@@ -126,7 +126,7 @@ describe('NewRelicClient', () => {
           headers: new Map<string, string | null>([
             [
               'link',
-              '<https://next.page/page2>; rel="next", <https://badsite.dontgohere>; rel="next"',
+              '<https://next.page/page2?page=2>; rel="next", <https://badsite.dontgohere>; rel="next"',
             ],
             ['otherheader', 'otherValue'],
           ]),
@@ -135,7 +135,7 @@ describe('NewRelicClient', () => {
           ok: true,
           json: async () => ({ applications: [] }),
           headers: new Map<string, string | null>([
-            ['Link', '<https://next.page/page3>; rel="next",'],
+            ['Link', '<https://next.page/page3?page=3>; rel="next",'],
           ]),
         })
         .mockResolvedValueOnce({
@@ -170,10 +170,10 @@ describe('NewRelicClient', () => {
       'https://test.test/newrelic/apm/api/applications.json',
     );
     expect(mockedFetchApi.fetch).toHaveBeenCalledWith(
-      'https://next.page/page2',
+      'https://test.test/newrelic/apm/api/applications.json?page=2',
     );
     expect(mockedFetchApi.fetch).toHaveBeenCalledWith(
-      'https://next.page/page3',
+      'https://test.test/newrelic/apm/api/applications.json?page=3',
     );
     expect(actual).toStrictEqual(expected);
   });
@@ -220,6 +220,7 @@ describe('NewRelicClient', () => {
     ['<>; rel:"value"'],
     ['<https://next.page>; rel: "next"'],
     ['ABCDE'],
+    ['<https://next.page/page3?page=>; rel="next",'],
   ])(
     'It does not attempt pagination when the link header value is invalid (%p)',
     async linkHeaderValue => {
@@ -309,4 +310,29 @@ describe('NewRelicClient', () => {
 
     expect(actual).toStrictEqual({ applications: [] });
   });
+
+  it('Generates the base url only once', async () => {
+    const mockedDiscoveryApi: DiscoveryApi = {
+      getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
+    };
+
+    const mockedFetchApi: FetchApi = {
+      fetch: jest.fn().mockResolvedValue({
+        ok: true,
+        json: async () => ({ applications: [] }),
+      }),
+    };
+
+    const client = new NewRelicClient({
+      discoveryApi: mockedDiscoveryApi,
+      fetchApi: mockedFetchApi,
+    });
+
+    await client.getApplications();
+    await client.getApplications();
+    await client.getApplications();
+    await client.getApplications();
+
+    expect(mockedDiscoveryApi.getBaseUrl).toHaveBeenCalledTimes(1);
+  });
 });
diff --git a/plugins/newrelic/src/api/index.ts b/plugins/newrelic/src/api/index.ts
index 9c95f3fe1f..b471104139 100644
--- a/plugins/newrelic/src/api/index.ts
+++ b/plugins/newrelic/src/api/index.ts
@@ -91,16 +91,22 @@ export class NewRelicClient implements NewRelicApi {
   private readonly discoveryApi: DiscoveryApi;
   private readonly fetchApi: FetchApi;
   private readonly proxyPathBase: string;
+  private baseUrl: string;
 
   constructor(options: Options) {
     this.discoveryApi = options.discoveryApi;
     this.fetchApi = options.fetchApi;
     this.proxyPathBase = options.proxyPathBase ?? DEFAULT_PROXY_PATH_BASE;
+    this.baseUrl = '';
   }
 
   async getApplications(): Promise<NewRelicApplications> {
-    const proxyUrl = await this.discoveryApi.getBaseUrl('proxy');
-    let targetUrl = `${proxyUrl}${this.proxyPathBase}/apm/api/applications.json`;
+    if (!this.baseUrl) {
+      const proxyUrl = await this.discoveryApi.getBaseUrl('proxy');
+      this.baseUrl = `${proxyUrl}${this.proxyPathBase}/apm/api/applications.json`;
+    }
+
+    let targetUrl = this.baseUrl;
     let hasNextPage = true;
 
     try {
@@ -111,7 +117,7 @@ export class NewRelicClient implements NewRelicApi {
           await this.fetchNewRelic(targetUrl);
 
         hasNextPage = hasMoreApplications;
-        targetUrl = pagination!.nextPageLink;
+        targetUrl = hasNextPage ? pagination!.nextPageLink : '';
         applications = applications.concat(applicationsFromReadPage);
       } while (hasNextPage);
 
@@ -155,12 +161,12 @@ export class NewRelicClient implements NewRelicApi {
     }
 
     const nextRelevantLink = linkHeader.replaceAll(' ', '').split(',')[0];
-
-    // Link should be of the format <nextPageLink>;rel="relation"
-    const linkParts = nextRelevantLink.match(/^<(.+)>;rel="(.+)"$/);
-    const nextPageLink = linkParts?.[1];
+    const linkParts = nextRelevantLink.match(/^<.+(\?page=.+)>;rel="(.+)"$/);
+    const nextPageNumber = linkParts?.[1];
     const relation = linkParts?.[2];
-    const isValidLink = !!(!!linkParts && nextPageLink && relation);
+
+    const nextPageLink = `${this.baseUrl}${nextPageNumber}`;
+    const isValidLink = !!(!!linkParts && nextPageNumber && relation);
 
     if (!nextRelevantLink || !isValidLink) {
       return undefined;

From a6f2f70515ab7131ee8a777f1f0f21c66d86eb1c Mon Sep 17 00:00:00 2001
From: Robert Bunning <129326788+wss-rbunning@users.noreply.github.com>
Date: Thu, 3 Aug 2023 14:47:25 -0400
Subject: [PATCH 3/9] Update plugins/newrelic/src/api/index.ts
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Co-authored-by: Fredrik Adelöw <freben@gmail.com>
Signed-off-by: Robert Bunning <129326788+wss-rbunning@users.noreply.github.com>
---
 plugins/newrelic/src/api/index.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/newrelic/src/api/index.ts b/plugins/newrelic/src/api/index.ts
index b471104139..9ce6f2c111 100644
--- a/plugins/newrelic/src/api/index.ts
+++ b/plugins/newrelic/src/api/index.ts
@@ -166,7 +166,7 @@ export class NewRelicClient implements NewRelicApi {
     const relation = linkParts?.[2];
 
     const nextPageLink = `${this.baseUrl}${nextPageNumber}`;
-    const isValidLink = !!(!!linkParts && nextPageNumber && relation);
+    const isValidLink = !!(linkParts && nextPageNumber && relation);
 
     if (!nextRelevantLink || !isValidLink) {
       return undefined;

From 96846456b10dd34dfc1c7e175803f68c559844e9 Mon Sep 17 00:00:00 2001
From: Robert Bunning <rbunning@webstaurantstore.com>
Date: Fri, 4 Aug 2023 11:46:19 -0400
Subject: [PATCH 4/9] Only get lowercased link header

Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
---
 plugins/newrelic/src/api/index.test.ts | 4 ++--
 plugins/newrelic/src/api/index.ts      | 3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/plugins/newrelic/src/api/index.test.ts b/plugins/newrelic/src/api/index.test.ts
index 69cf74066f..70fda9606d 100644
--- a/plugins/newrelic/src/api/index.test.ts
+++ b/plugins/newrelic/src/api/index.test.ts
@@ -135,7 +135,7 @@ describe('NewRelicClient', () => {
           ok: true,
           json: async () => ({ applications: [] }),
           headers: new Map<string, string | null>([
-            ['Link', '<https://next.page/page3?page=3>; rel="next",'],
+            ['link', '<https://next.page/page3?page=3>; rel="next",'],
           ]),
         })
         .mockResolvedValueOnce({
@@ -178,7 +178,7 @@ describe('NewRelicClient', () => {
     expect(actual).toStrictEqual(expected);
   });
 
-  test.each([['LINK'], ['lINK']])(
+  test.each([['Link'], ['LINK'], ['lINK']])(
     'It does not attempt pagination when the link header name is invalid (%p)',
     async linkHeaderName => {
       const mockedDiscoveryApi: DiscoveryApi = {
diff --git a/plugins/newrelic/src/api/index.ts b/plugins/newrelic/src/api/index.ts
index 9ce6f2c111..237fe95fde 100644
--- a/plugins/newrelic/src/api/index.ts
+++ b/plugins/newrelic/src/api/index.ts
@@ -142,8 +142,7 @@ export class NewRelicClient implements NewRelicApi {
     }
 
     const readResponse = responseJson as NewRelicApplications;
-    const linkHeader =
-      response.headers.get('Link') || response.headers.get('link');
+    const linkHeader = response.headers.get('link');
     const pagination = this.parseLinkHeader(linkHeader);
 
     return {

From 4c99b8fca63ff69e0c4ecebf2e004f0ab7b18ee1 Mon Sep 17 00:00:00 2001
From: Robert Bunning <rbunning@webstaurantstore.com>
Date: Fri, 4 Aug 2023 15:19:43 -0400
Subject: [PATCH 5/9] Switch to using other link header parser

Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
---
 plugins/newrelic/package.json          |  2 +
 plugins/newrelic/src/api/index.test.ts |  2 +-
 plugins/newrelic/src/api/index.ts      | 52 +++++---------------------
 yarn.lock                              | 20 +++++++++-
 4 files changed, 32 insertions(+), 44 deletions(-)

diff --git a/plugins/newrelic/package.json b/plugins/newrelic/package.json
index b09050c599..7e42a26d9f 100644
--- a/plugins/newrelic/package.json
+++ b/plugins/newrelic/package.json
@@ -39,6 +39,8 @@
     "@material-ui/core": "^4.12.2",
     "@material-ui/icons": "^4.9.1",
     "@material-ui/lab": "4.0.0-alpha.61",
+    "@types/parse-link-header": "^2.0.1",
+    "parse-link-header": "^2.0.0",
     "react-use": "^17.2.4"
   },
   "peerDependencies": {
diff --git a/plugins/newrelic/src/api/index.test.ts b/plugins/newrelic/src/api/index.test.ts
index 70fda9606d..94bdc8ff94 100644
--- a/plugins/newrelic/src/api/index.test.ts
+++ b/plugins/newrelic/src/api/index.test.ts
@@ -126,7 +126,7 @@ describe('NewRelicClient', () => {
           headers: new Map<string, string | null>([
             [
               'link',
-              '<https://next.page/page2?page=2>; rel="next", <https://badsite.dontgohere>; rel="next"',
+              '<https://next.page/page2?page=2>; rel="next", <https://badsite.dontgohere>; rel="first"',
             ],
             ['otherheader', 'otherValue'],
           ]),
diff --git a/plugins/newrelic/src/api/index.ts b/plugins/newrelic/src/api/index.ts
index 237fe95fde..ede225dc05 100644
--- a/plugins/newrelic/src/api/index.ts
+++ b/plugins/newrelic/src/api/index.ts
@@ -20,6 +20,8 @@ import {
   FetchApi,
 } from '@backstage/core-plugin-api';
 
+import parseLinkHeader from 'parse-link-header';
+
 export type NewRelicApplication = {
   id: number;
   application_summary: NewRelicApplicationSummary;
@@ -76,14 +78,8 @@ export interface NewRelicApi {
   getApplications(): Promise<NewRelicApplications>;
 }
 
-interface PaginationInformation {
-  nextPageLink: string;
-  relation: string;
-}
-
 interface NewRelicPageReadResult {
-  hasMoreApplications: boolean;
-  pagination: PaginationInformation | undefined;
+  nextPageUrl: string | undefined;
   applicationsFromReadPage: NewRelicApplication[];
 }
 
@@ -106,20 +102,17 @@ export class NewRelicClient implements NewRelicApi {
       this.baseUrl = `${proxyUrl}${this.proxyPathBase}/apm/api/applications.json`;
     }
 
-    let targetUrl = this.baseUrl;
-    let hasNextPage = true;
-
     try {
       let applications: NewRelicApplication[] = [];
+      let targetUrl = this.baseUrl;
 
       do {
-        const { hasMoreApplications, pagination, applicationsFromReadPage } =
+        const { nextPageUrl, applicationsFromReadPage } =
           await this.fetchNewRelic(targetUrl);
 
-        hasNextPage = hasMoreApplications;
-        targetUrl = hasNextPage ? pagination!.nextPageLink : '';
+        targetUrl = nextPageUrl ?? '';
         applications = applications.concat(applicationsFromReadPage);
-      } while (hasNextPage);
+      } while (!!targetUrl);
 
       return { applications };
     } catch (e) {
@@ -143,37 +136,12 @@ export class NewRelicClient implements NewRelicApi {
 
     const readResponse = responseJson as NewRelicApplications;
     const linkHeader = response.headers.get('link');
-    const pagination = this.parseLinkHeader(linkHeader);
+    const parseResult = parseLinkHeader(linkHeader);
+    const nextPageNumber = parseResult?.next?.page;
 
     return {
-      hasMoreApplications: !!(pagination && pagination.relation === 'next'),
-      pagination,
+      nextPageUrl: nextPageNumber && `${this.baseUrl}?page=${nextPageNumber}`,
       applicationsFromReadPage: readResponse.applications,
     };
   }
-
-  private parseLinkHeader(
-    linkHeader: string | null,
-  ): PaginationInformation | undefined {
-    if (!linkHeader) {
-      return undefined;
-    }
-
-    const nextRelevantLink = linkHeader.replaceAll(' ', '').split(',')[0];
-    const linkParts = nextRelevantLink.match(/^<.+(\?page=.+)>;rel="(.+)"$/);
-    const nextPageNumber = linkParts?.[1];
-    const relation = linkParts?.[2];
-
-    const nextPageLink = `${this.baseUrl}${nextPageNumber}`;
-    const isValidLink = !!(linkParts && nextPageNumber && relation);
-
-    if (!nextRelevantLink || !isValidLink) {
-      return undefined;
-    }
-
-    return {
-      nextPageLink,
-      relation,
-    };
-  }
 }
diff --git a/yarn.lock b/yarn.lock
index 3f9b0ee1bc..1399f1d622 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -7605,9 +7605,11 @@ __metadata:
     "@testing-library/react": ^12.1.3
     "@testing-library/user-event": ^14.0.0
     "@types/node": ^16.11.26
+    "@types/parse-link-header": ^2.0.1
     "@types/react": ^16.13.1 || ^17.0.0
     cross-fetch: ^3.1.5
     msw: ^1.0.0
+    parse-link-header: ^2.0.0
     react-use: ^17.2.4
   peerDependencies:
     react: ^16.13.1 || ^17.0.0
@@ -17606,6 +17608,13 @@ __metadata:
   languageName: node
   linkType: hard
 
+"@types/parse-link-header@npm:^2.0.1":
+  version: 2.0.1
+  resolution: "@types/parse-link-header@npm:2.0.1"
+  checksum: f76678612511365aefc23704f00f3262fcb1d6bd9c4dd83f783a38e50e3ccf26615f9a62c757952cab5af6f2bd8b3b1763ce391376458afce5fe47c6fe2b80e1
+  languageName: node
+  linkType: hard
+
 "@types/passport-auth0@npm:^1.0.5":
   version: 1.0.5
   resolution: "@types/passport-auth0@npm:1.0.5"
@@ -34337,6 +34346,15 @@ __metadata:
   languageName: node
   linkType: hard
 
+"parse-link-header@npm:^2.0.0":
+  version: 2.0.0
+  resolution: "parse-link-header@npm:2.0.0"
+  dependencies:
+    xtend: ~4.0.1
+  checksum: 0e96c6af9910e8f92084b49b8dc6a10dd58db470847d1499f562576180c1ac5e49d18007697f0d538e5f3efdc8ce1d8777641f3ae225302b74af0dd0578b628e
+  languageName: node
+  linkType: hard
+
 "parse-path@npm:^7.0.0":
   version: 7.0.0
   resolution: "parse-path@npm:7.0.0"
@@ -42745,7 +42763,7 @@ __metadata:
   languageName: node
   linkType: hard
 
-"xtend@npm:^4.0.0":
+"xtend@npm:^4.0.0, xtend@npm:~4.0.1":
   version: 4.0.2
   resolution: "xtend@npm:4.0.2"
   checksum: ac5dfa738b21f6e7f0dd6e65e1b3155036d68104e67e5d5d1bde74892e327d7e5636a076f625599dc394330a731861e87343ff184b0047fef1360a7ec0a5a36a

From 7c8e57af3dcfc2fd92a85b36fbbbde946523b89e Mon Sep 17 00:00:00 2001
From: Robert Bunning <rbunning@webstaurantstore.com>
Date: Mon, 7 Aug 2023 15:39:18 -0400
Subject: [PATCH 6/9] Convert tests to use msw instead of mocking fetch

Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
---
 plugins/newrelic/package.json          |   3 +-
 plugins/newrelic/src/api/index.test.ts | 266 ++++++++++++++-----------
 yarn.lock                              |  11 +-
 3 files changed, 163 insertions(+), 117 deletions(-)

diff --git a/plugins/newrelic/package.json b/plugins/newrelic/package.json
index 7e42a26d9f..b510e9cbbe 100644
--- a/plugins/newrelic/package.json
+++ b/plugins/newrelic/package.json
@@ -49,6 +49,7 @@
     "react-router-dom": "6.0.0-beta.0 || ^6.3.0"
   },
   "devDependencies": {
+    "@backstage/backend-test-utils": "workspace:^",
     "@backstage/cli": "workspace:^",
     "@backstage/core-app-api": "workspace:^",
     "@backstage/dev-utils": "workspace:^",
@@ -60,7 +61,7 @@
     "@types/node": "^16.11.26",
     "@types/react": "^16.13.1 || ^17.0.0",
     "cross-fetch": "^3.1.5",
-    "msw": "^1.0.0"
+    "msw": "^1.2.3"
   },
   "files": [
     "dist"
diff --git a/plugins/newrelic/src/api/index.test.ts b/plugins/newrelic/src/api/index.test.ts
index 94bdc8ff94..f45482be87 100644
--- a/plugins/newrelic/src/api/index.test.ts
+++ b/plugins/newrelic/src/api/index.test.ts
@@ -14,15 +14,24 @@
  * limitations under the License.
  */
 
-import { NewRelicClient } from '.';
-import { FetchApi } from '@backstage/core-plugin-api';
+import { NewRelicApplication, NewRelicClient } from '.';
 import { DiscoveryApi } from '@backstage/core-plugin-api';
+import { rest } from 'msw';
+import { setupServer } from 'msw/node';
+import { MockFetchApi, setupRequestMockHandlers } from '@backstage/test-utils';
 
 beforeEach(() => {
   jest.resetAllMocks();
 });
 
 describe('NewRelicClient', () => {
+  const server = setupServer();
+  setupRequestMockHandlers(server);
+
+  beforeEach(() => {
+    server.resetHandlers();
+  });
+
   test.each([
     ['https://test.test/BASEPATH/apm/api/applications.json', '/BASEPATH'],
     ['https://test.test/BASEPATH2/apm/api/applications.json', '/BASEPATH2'],
@@ -31,17 +40,18 @@ describe('NewRelicClient', () => {
   ])(
     'It correctly forms the request url (%p) when proxyPathBase is %p',
     async (expectedUrl, basePathOverride) => {
+      server.use(
+        rest.get(expectedUrl, (_, res, ctx) =>
+          res(ctx.status(200), ctx.json({ applications: [] })),
+        ),
+      );
+
       const mockedDiscoveryApi: DiscoveryApi = {
         getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
       };
 
-      const mockedFetchApi: FetchApi = {
-        fetch: jest.fn().mockResolvedValueOnce({
-          ok: true,
-          json: async () => [],
-          headers: new Map<string, string | null>(),
-        }),
-      };
+      const mockedFetchApi = new MockFetchApi();
+      const fetchSpy = jest.spyOn(mockedFetchApi, 'fetch');
 
       const client = new NewRelicClient({
         discoveryApi: mockedDiscoveryApi,
@@ -50,7 +60,7 @@ describe('NewRelicClient', () => {
       });
       await client.getApplications();
 
-      expect(mockedFetchApi.fetch).toHaveBeenCalledWith(expectedUrl);
+      expect(fetchSpy).toHaveBeenCalledWith(expectedUrl);
     },
   );
 
@@ -59,7 +69,7 @@ describe('NewRelicClient', () => {
       getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
     };
 
-    const mockedApplicationOne = {
+    const mockedApplicationOne: NewRelicApplication = {
       id: 1,
       application_summary: {
         apdex_score: 0,
@@ -81,8 +91,16 @@ describe('NewRelicClient', () => {
       },
     };
 
-    const mockedApplicationTwo = {
+    const mockedApplicationTwo: NewRelicApplication = {
       id: 2,
+      application_summary: {
+        apdex_score: -900,
+        error_rate: 0,
+        host_count: 0,
+        instance_count: 0,
+        response_time: 0,
+        throughput: 0,
+      },
       name: 'Testing Application #2',
       language: 'en-us',
       health_status: 'Working',
@@ -95,7 +113,7 @@ describe('NewRelicClient', () => {
       },
     };
 
-    const mockedApplicationThree = {
+    const mockedApplicationThree: NewRelicApplication = {
       id: 3,
       application_summary: {
         apdex_score: -900,
@@ -117,45 +135,65 @@ describe('NewRelicClient', () => {
       },
     };
 
-    const mockedFetchApi: FetchApi = {
-      fetch: jest
-        .fn()
-        .mockResolvedValueOnce({
-          ok: true,
-          json: async () => ({ applications: [mockedApplicationOne] }),
-          headers: new Map<string, string | null>([
-            [
-              'link',
-              '<https://next.page/page2?page=2>; rel="next", <https://badsite.dontgohere>; rel="first"',
-            ],
-            ['otherheader', 'otherValue'],
-          ]),
-        })
-        .mockResolvedValueOnce({
-          ok: true,
-          json: async () => ({ applications: [] }),
-          headers: new Map<string, string | null>([
-            ['link', '<https://next.page/page3?page=3>; rel="next",'],
-          ]),
-        })
-        .mockResolvedValueOnce({
-          ok: true,
-          json: async () => ({
-            applications: [mockedApplicationTwo, mockedApplicationThree],
-          }),
-          headers: new Map<string, string | null>([
-            [
-              'link',
-              '<https://first.page/page1>; rel="first", <https://next.page/page2>; rel="next"',
-            ],
-          ]),
-        }),
-    };
+    const queryToRequestData = new Map<
+      string | null,
+      { link?: string | string[]; apps: NewRelicApplication[] }
+    >([
+      [
+        null,
+        {
+          link: [
+            '<https://next.page/page2?page=2>; rel="next"',
+            '<https://badsite.dontgohere>; rel="bad"',
+          ],
+          apps: [mockedApplicationOne],
+        },
+      ],
+      [
+        '2',
+        {
+          link: '<https://next.page/page3?page=3>; rel="next",',
+          apps: [],
+        },
+      ],
+      [
+        '3',
+        {
+          apps: [mockedApplicationTwo, mockedApplicationThree],
+        },
+      ],
+    ]);
+
+    const mockedFetchApi = new MockFetchApi();
+    const fetchSpy = jest.spyOn(mockedFetchApi, 'fetch');
+
+    server.use(
+      rest.get(
+        'https://test.test/newrelic/apm/api/applications.json',
+        (req, res, ctx) => {
+          const nextPageNumber = req.url.searchParams.get('page');
+          const requestData = queryToRequestData.get(nextPageNumber) ?? {
+            apps: [],
+          };
+
+          const { link, apps: applications } = requestData;
+          const statusTransform = ctx.status(200);
+          const responseBody = ctx.json({ applications });
+
+          if (!!link) {
+            return res(statusTransform, ctx.set({ link }), responseBody);
+          }
+
+          return res(statusTransform, responseBody);
+        },
+      ),
+    );
 
     const client = new NewRelicClient({
       discoveryApi: mockedDiscoveryApi,
       fetchApi: mockedFetchApi,
     });
+
     const actual = await client.getApplications();
     const expected = {
       applications: [
@@ -165,14 +203,14 @@ describe('NewRelicClient', () => {
       ],
     };
 
-    expect(mockedFetchApi.fetch).toHaveBeenCalledTimes(3);
-    expect(mockedFetchApi.fetch).toHaveBeenCalledWith(
+    expect(fetchSpy).toHaveBeenCalledTimes(3);
+    expect(fetchSpy).toHaveBeenCalledWith(
       'https://test.test/newrelic/apm/api/applications.json',
     );
-    expect(mockedFetchApi.fetch).toHaveBeenCalledWith(
+    expect(fetchSpy).toHaveBeenCalledWith(
       'https://test.test/newrelic/apm/api/applications.json?page=2',
     );
-    expect(mockedFetchApi.fetch).toHaveBeenCalledWith(
+    expect(fetchSpy).toHaveBeenCalledWith(
       'https://test.test/newrelic/apm/api/applications.json?page=3',
     );
     expect(actual).toStrictEqual(expected);
@@ -185,19 +223,28 @@ describe('NewRelicClient', () => {
         getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
       };
 
-      const mockedFetchApi: FetchApi = {
-        fetch: jest.fn().mockResolvedValueOnce({
-          ok: true,
-          json: async () => ({ applications: [] }),
-          headers: new Map<string, string | null>([
-            [
-              linkHeaderName,
-              '<https://next.page/page2>; rel="next", <https://badsite.dontgohere>; rel="next"',
-            ],
-            ['otherheader', 'otherValue'],
-          ]),
+      const mockedFetchApi = new MockFetchApi();
+      const fetchSpy = jest.spyOn(mockedFetchApi, 'fetch');
+
+      server.use(
+        rest.get(
+          'https://test.test/newrelic/apm/api/applications.json',
+          (_, res, ctx) =>
+            res(
+              ctx.status(200),
+              ctx.set(
+                linkHeaderName,
+                '<https://test.test/badroute>; rel="next"',
+              ),
+              ctx.json({ applications: [] }),
+            ),
+        ),
+        rest.get('https://test.test/badroute', () => {
+          throw new Error(
+            'NewRelicClient attempted to paginate when it should not have',
+          );
         }),
-      };
+      );
 
       const client = new NewRelicClient({
         discoveryApi: mockedDiscoveryApi,
@@ -205,8 +252,8 @@ describe('NewRelicClient', () => {
       });
       await client.getApplications();
 
-      expect(mockedFetchApi.fetch).toHaveBeenCalledTimes(1);
-      expect(mockedFetchApi.fetch).toHaveBeenCalledWith(
+      expect(fetchSpy).toHaveBeenCalledTimes(1);
+      expect(fetchSpy).toHaveBeenCalledWith(
         'https://test.test/newrelic/apm/api/applications.json',
       );
     },
@@ -228,16 +275,25 @@ describe('NewRelicClient', () => {
         getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
       };
 
-      const mockedFetchApi: FetchApi = {
-        fetch: jest.fn().mockResolvedValueOnce({
-          ok: true,
-          json: async () => ({ applications: [] }),
-          headers: new Map<string, string | null>([
-            ['Link', linkHeaderValue],
-            ['otherheader', 'otherValue'],
-          ]),
+      const mockedFetchApi = new MockFetchApi();
+      const fetchSpy = jest.spyOn(mockedFetchApi, 'fetch');
+
+      server.use(
+        rest.get(
+          'https://test.test/newrelic/apm/api/applications.json',
+          (_, res, ctx) =>
+            res(
+              ctx.status(200),
+              ctx.set('link', linkHeaderValue),
+              ctx.json({ applications: [] }),
+            ),
+        ),
+        rest.get('https://test.test/badroute', () => {
+          throw new Error(
+            'NewRelicClient attempted to paginate when it should not have',
+          );
         }),
-      };
+      );
 
       const client = new NewRelicClient({
         discoveryApi: mockedDiscoveryApi,
@@ -245,45 +301,33 @@ describe('NewRelicClient', () => {
       });
       await client.getApplications();
 
-      expect(mockedFetchApi.fetch).toHaveBeenCalledTimes(1);
-      expect(mockedFetchApi.fetch).toHaveBeenCalledWith(
+      expect(fetchSpy).toHaveBeenCalledTimes(1);
+      expect(fetchSpy).toHaveBeenCalledWith(
         'https://test.test/newrelic/apm/api/applications.json',
       );
     },
   );
 
   test.each([
-    [
-      {
-        ok: false,
-        statusText: 'statusText',
-        json: async () => ({ error: { title: 'TESTING' } }),
-      },
-    ],
-    [
-      {
-        ok: false,
-        statusText: 'statusText',
-        json: async () => ({}),
-      },
-    ],
+    [404, { error: { title: 'TESTING' } }],
+    [500, {}],
   ])(
     'It returns an empty array of applications when the fetch is not okay',
-    async fetchResult => {
+    async (statusCode, jsonBody) => {
       const mockedDiscoveryApi: DiscoveryApi = {
         getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
       };
 
-      const mockedFetchApi: FetchApi = {
-        fetch: jest.fn().mockResolvedValueOnce({
-          ok: true,
-          json: async () => fetchResult,
-        }),
-      };
+      server.use(
+        rest.get(
+          'https://test.test/newrelic/apm/api/applications.json',
+          (_, res, ctx) => res(ctx.status(statusCode), ctx.json(jsonBody)),
+        ),
+      );
 
       const client = new NewRelicClient({
         discoveryApi: mockedDiscoveryApi,
-        fetchApi: mockedFetchApi,
+        fetchApi: new MockFetchApi(),
       });
       const actual = await client.getApplications();
 
@@ -296,15 +340,15 @@ describe('NewRelicClient', () => {
       getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
     };
 
-    const mockedFetchApi: FetchApi = {
-      fetch: () => {
-        throw new Error('TESTING');
-      },
-    };
+    server.use(
+      rest.get('https://test.test/newrelic/apm/api/applications.json', () => {
+        throw new Error('Network Error');
+      }),
+    );
 
     const client = new NewRelicClient({
       discoveryApi: mockedDiscoveryApi,
-      fetchApi: mockedFetchApi,
+      fetchApi: new MockFetchApi(),
     });
     const actual = await client.getApplications();
 
@@ -316,16 +360,16 @@ describe('NewRelicClient', () => {
       getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
     };
 
-    const mockedFetchApi: FetchApi = {
-      fetch: jest.fn().mockResolvedValue({
-        ok: true,
-        json: async () => ({ applications: [] }),
-      }),
-    };
+    server.use(
+      rest.get(
+        'https://test.test/newrelic/apm/api/applications.json',
+        (_, res, ctx) => res(ctx.status(200), ctx.json({ applications: [] })),
+      ),
+    );
 
     const client = new NewRelicClient({
       discoveryApi: mockedDiscoveryApi,
-      fetchApi: mockedFetchApi,
+      fetchApi: new MockFetchApi(),
     });
 
     await client.getApplications();
diff --git a/yarn.lock b/yarn.lock
index 1399f1d622..b7d49886f5 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -7590,6 +7590,7 @@ __metadata:
   version: 0.0.0-use.local
   resolution: "@backstage/plugin-newrelic@workspace:plugins/newrelic"
   dependencies:
+    "@backstage/backend-test-utils": "workspace:^"
     "@backstage/cli": "workspace:^"
     "@backstage/core-app-api": "workspace:^"
     "@backstage/core-components": "workspace:^"
@@ -7608,7 +7609,7 @@ __metadata:
     "@types/parse-link-header": ^2.0.1
     "@types/react": ^16.13.1 || ^17.0.0
     cross-fetch: ^3.1.5
-    msw: ^1.0.0
+    msw: ^1.2.3
     parse-link-header: ^2.0.0
     react-use: ^17.2.4
   peerDependencies:
@@ -32846,9 +32847,9 @@ __metadata:
   languageName: node
   linkType: hard
 
-"msw@npm:^1.0.0, msw@npm:^1.0.1, msw@npm:^1.2.1":
-  version: 1.2.2
-  resolution: "msw@npm:1.2.2"
+"msw@npm:^1.0.0, msw@npm:^1.0.1, msw@npm:^1.2.1, msw@npm:^1.2.3":
+  version: 1.2.3
+  resolution: "msw@npm:1.2.3"
   dependencies:
     "@mswjs/cookies": ^0.2.2
     "@mswjs/interceptors": ^0.17.5
@@ -32876,7 +32877,7 @@ __metadata:
       optional: true
   bin:
     msw: cli/index.js
-  checksum: e42cec8f5523663020bdecf6a7977a10aa86a4718d1920def3fbde0ff3734391873668cc6e3996d6790add3c74dac95a952f8560ce2543697280125eb55138e8
+  checksum: 832a6fc3973726a97e03ce2690c3b6e1774b5156d064a478657a1b33f9933e4834af833cc8a859e1b717fa9b71f1271b3e66a1ec6eed7f76bfe79406e9ec3986
   languageName: node
   linkType: hard
 

From b6ee85d1e7e930d8d992cd3f10b6c815a0435925 Mon Sep 17 00:00:00 2001
From: Robert Bunning <rbunning@webstaurantstore.com>
Date: Mon, 7 Aug 2023 15:41:55 -0400
Subject: [PATCH 7/9] Switch to using push to add on read applications

Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
---
 plugins/newrelic/src/api/index.ts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/plugins/newrelic/src/api/index.ts b/plugins/newrelic/src/api/index.ts
index ede225dc05..3fdebd1494 100644
--- a/plugins/newrelic/src/api/index.ts
+++ b/plugins/newrelic/src/api/index.ts
@@ -103,7 +103,7 @@ export class NewRelicClient implements NewRelicApi {
     }
 
     try {
-      let applications: NewRelicApplication[] = [];
+      const applications: NewRelicApplication[] = [];
       let targetUrl = this.baseUrl;
 
       do {
@@ -111,7 +111,7 @@ export class NewRelicClient implements NewRelicApi {
           await this.fetchNewRelic(targetUrl);
 
         targetUrl = nextPageUrl ?? '';
-        applications = applications.concat(applicationsFromReadPage);
+        applications.push(...applicationsFromReadPage);
       } while (!!targetUrl);
 
       return { applications };

From 89435f72405aa5ab6a11389bb578dd4bbd1e99eb Mon Sep 17 00:00:00 2001
From: Robert Bunning <rbunning@webstaurantstore.com>
Date: Tue, 8 Aug 2023 13:50:27 -0400
Subject: [PATCH 8/9] Display http errors first. Allow json parsing errors to
 escape and be displayed

Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
---
 plugins/newrelic/src/api/index.test.ts | 84 +++++++++++++-------------
 plugins/newrelic/src/api/index.ts      | 34 ++++++-----
 2 files changed, 61 insertions(+), 57 deletions(-)

diff --git a/plugins/newrelic/src/api/index.test.ts b/plugins/newrelic/src/api/index.test.ts
index f45482be87..d4ed4a1691 100644
--- a/plugins/newrelic/src/api/index.test.ts
+++ b/plugins/newrelic/src/api/index.test.ts
@@ -20,6 +20,10 @@ import { rest } from 'msw';
 import { setupServer } from 'msw/node';
 import { MockFetchApi, setupRequestMockHandlers } from '@backstage/test-utils';
 
+const mockedDiscoveryApi: DiscoveryApi = {
+  getBaseUrl: async () => 'https://test.test',
+};
+
 beforeEach(() => {
   jest.resetAllMocks();
 });
@@ -46,10 +50,6 @@ describe('NewRelicClient', () => {
         ),
       );
 
-      const mockedDiscoveryApi: DiscoveryApi = {
-        getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
-      };
-
       const mockedFetchApi = new MockFetchApi();
       const fetchSpy = jest.spyOn(mockedFetchApi, 'fetch');
 
@@ -65,10 +65,6 @@ describe('NewRelicClient', () => {
   );
 
   it('Correctly reads all pages of results and returns the expected results', async () => {
-    const mockedDiscoveryApi: DiscoveryApi = {
-      getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
-    };
-
     const mockedApplicationOne: NewRelicApplication = {
       id: 1,
       application_summary: {
@@ -219,10 +215,6 @@ describe('NewRelicClient', () => {
   test.each([['Link'], ['LINK'], ['lINK']])(
     'It does not attempt pagination when the link header name is invalid (%p)',
     async linkHeaderName => {
-      const mockedDiscoveryApi: DiscoveryApi = {
-        getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
-      };
-
       const mockedFetchApi = new MockFetchApi();
       const fetchSpy = jest.spyOn(mockedFetchApi, 'fetch');
 
@@ -271,10 +263,6 @@ describe('NewRelicClient', () => {
   ])(
     'It does not attempt pagination when the link header value is invalid (%p)',
     async linkHeaderValue => {
-      const mockedDiscoveryApi: DiscoveryApi = {
-        getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
-      };
-
       const mockedFetchApi = new MockFetchApi();
       const fetchSpy = jest.spyOn(mockedFetchApi, 'fetch');
 
@@ -309,19 +297,38 @@ describe('NewRelicClient', () => {
   );
 
   test.each([
-    [404, { error: { title: 'TESTING' } }],
-    [500, {}],
+    ['Error communicating with New Relic: Not Found', 404, JSON.stringify({})],
+    [
+      'Error communicating with New Relic: ERROR TITLE',
+      404,
+      JSON.stringify({
+        error: {
+          title: 'ERROR TITLE',
+        },
+      }),
+    ],
+    [
+      'Error communicating with New Relic: Internal Server Error',
+      500,
+      JSON.stringify(undefined),
+    ],
+    [
+      'Error communicating with New Relic: Internal Server Error',
+      500,
+      JSON.stringify(null),
+    ],
+    [
+      'Error communicating with New Relic: Internal Server Error',
+      500,
+      '<invalid></invalid',
+    ],
   ])(
-    'It returns an empty array of applications when the fetch is not okay',
-    async (statusCode, jsonBody) => {
-      const mockedDiscoveryApi: DiscoveryApi = {
-        getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
-      };
-
+    'It throws this error: %p when the status code is %p and the body is %j',
+    async (expectedErrorMessage, statusCode, body) => {
       server.use(
         rest.get(
           'https://test.test/newrelic/apm/api/applications.json',
-          (_, res, ctx) => res(ctx.status(statusCode), ctx.json(jsonBody)),
+          (_, res, ctx) => res(ctx.status(statusCode), ctx.body(body)),
         ),
       );
 
@@ -329,36 +336,31 @@ describe('NewRelicClient', () => {
         discoveryApi: mockedDiscoveryApi,
         fetchApi: new MockFetchApi(),
       });
-      const actual = await client.getApplications();
 
-      expect(actual).toStrictEqual({ applications: [] });
+      await expect(client.getApplications()).rejects.toThrow(
+        expectedErrorMessage,
+      );
     },
   );
 
-  it('Returns an empty array when the fetch itself throws an error', async () => {
-    const mockedDiscoveryApi: DiscoveryApi = {
-      getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
-    };
-
+  it('Throws an error when the body is invalid json but the status code is 200', async () => {
     server.use(
-      rest.get('https://test.test/newrelic/apm/api/applications.json', () => {
-        throw new Error('Network Error');
-      }),
+      rest.get(
+        'https://test.test/newrelic/apm/api/applications.json',
+        (_, res, ctx) => res(ctx.status(200), ctx.body('<Invalid></Invalid')),
+      ),
     );
 
     const client = new NewRelicClient({
       discoveryApi: mockedDiscoveryApi,
       fetchApi: new MockFetchApi(),
     });
-    const actual = await client.getApplications();
 
-    expect(actual).toStrictEqual({ applications: [] });
+    await expect(client.getApplications()).rejects.toThrow();
   });
 
   it('Generates the base url only once', async () => {
-    const mockedDiscoveryApi: DiscoveryApi = {
-      getBaseUrl: jest.fn().mockResolvedValueOnce('https://test.test'),
-    };
+    const getBaseUrlSpy = jest.spyOn(mockedDiscoveryApi, 'getBaseUrl');
 
     server.use(
       rest.get(
@@ -377,6 +379,6 @@ describe('NewRelicClient', () => {
     await client.getApplications();
     await client.getApplications();
 
-    expect(mockedDiscoveryApi.getBaseUrl).toHaveBeenCalledTimes(1);
+    expect(getBaseUrlSpy).toHaveBeenCalledTimes(1);
   });
 });
diff --git a/plugins/newrelic/src/api/index.ts b/plugins/newrelic/src/api/index.ts
index 3fdebd1494..b37575b163 100644
--- a/plugins/newrelic/src/api/index.ts
+++ b/plugins/newrelic/src/api/index.ts
@@ -102,39 +102,41 @@ export class NewRelicClient implements NewRelicApi {
       this.baseUrl = `${proxyUrl}${this.proxyPathBase}/apm/api/applications.json`;
     }
 
-    try {
-      const applications: NewRelicApplication[] = [];
-      let targetUrl = this.baseUrl;
+    const applications: NewRelicApplication[] = [];
+    let targetUrl = this.baseUrl;
 
-      do {
-        const { nextPageUrl, applicationsFromReadPage } =
-          await this.fetchNewRelic(targetUrl);
+    do {
+      const { nextPageUrl, applicationsFromReadPage } =
+        await this.fetchNewRelic(targetUrl);
 
-        targetUrl = nextPageUrl ?? '';
-        applications.push(...applicationsFromReadPage);
-      } while (!!targetUrl);
+      targetUrl = nextPageUrl ?? '';
+      applications.push(...applicationsFromReadPage);
+    } while (!!targetUrl);
 
-      return { applications };
-    } catch (e) {
-      return { applications: [] };
-    }
+    return { applications };
   }
 
   private async fetchNewRelic(
     targetUrl: string,
   ): Promise<NewRelicPageReadResult> {
     const response = await this.fetchApi.fetch(targetUrl);
-    const responseJson = await response.json();
 
     if (!response.ok) {
+      let specificErrorTitle = undefined;
+      try {
+        specificErrorTitle = (await response.json())?.error?.title;
+      } catch (e) {
+        /* empty */
+      }
+
       throw new Error(
         `Error communicating with New Relic: ${
-          responseJson?.error?.title || response.statusText
+          specificErrorTitle || response.statusText
         }`,
       );
     }
 
-    const readResponse = responseJson as NewRelicApplications;
+    const readResponse = (await response.json()) as NewRelicApplications;
     const linkHeader = response.headers.get('link');
     const parseResult = parseLinkHeader(linkHeader);
     const nextPageNumber = parseResult?.next?.page;

From 58398c759988a9a2ab55fd6b214b810b1c8f2967 Mon Sep 17 00:00:00 2001
From: Robert Bunning <rbunning@webstaurantstore.com>
Date: Wed, 9 Aug 2023 11:56:01 -0400
Subject: [PATCH 9/9] fix linter errors

Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
---
 plugins/newrelic/package.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/newrelic/package.json b/plugins/newrelic/package.json
index b510e9cbbe..c70a06dee8 100644
--- a/plugins/newrelic/package.json
+++ b/plugins/newrelic/package.json
@@ -39,7 +39,6 @@
     "@material-ui/core": "^4.12.2",
     "@material-ui/icons": "^4.9.1",
     "@material-ui/lab": "4.0.0-alpha.61",
-    "@types/parse-link-header": "^2.0.1",
     "parse-link-header": "^2.0.0",
     "react-use": "^17.2.4"
   },
@@ -59,6 +58,7 @@
     "@testing-library/react": "^12.1.3",
     "@testing-library/user-event": "^14.0.0",
     "@types/node": "^16.11.26",
+    "@types/parse-link-header": "^2.0.1",
     "@types/react": "^16.13.1 || ^17.0.0",
     "cross-fetch": "^3.1.5",
     "msw": "^1.2.3"