Merge pull request #21822 from secustor/refactor/githuburlreader-centralize-fetching

refactor(backend-common/GithubUrlReader): only call fetch inside fetchResponse

.changeset/eight-rice-cross.md

@@ -0,0 +1,5 @@
+---
+'@backstage/backend-common': patch
+---
+
+Do not call fetch directly but rather use `fetchResponse` facility

packages/backend-common/src/reading/GithubUrlReader.ts

@@ -105,58 +105,28 @@ export class GithubUrlReader implements UrlReader {
       credentials,
     );
 
-    let response: Response;
-    try {
-      response = await fetch(ghUrl, {
-        headers: {
-          ...credentials?.headers,
-          ...(options?.etag && { 'If-None-Match': options.etag }),
-          ...(options?.lastModifiedAfter && {
-            'If-Modified-Since': options.lastModifiedAfter.toUTCString(),
-          }),
-          Accept: 'application/vnd.github.v3.raw',
-        },
-        // TODO(freben): The signal cast is there because pre-3.x versions of
-        // node-fetch have a very slightly deviating AbortSignal type signature.
-        // The difference does not affect us in practice however. The cast can
-        // be removed after we support ESM for CLI dependencies and migrate to
-        // version 3 of node-fetch.
-        // https://github.com/backstage/backstage/issues/8242
-        signal: options?.signal as any,
-      });
-    } catch (e) {
-      throw new Error(`Unable to read ${url}, ${e}`);
-    }
+    const response = await this.fetchResponse(ghUrl, {
+      headers: {
+        ...credentials?.headers,
+        ...(options?.etag && { 'If-None-Match': options.etag }),
+        ...(options?.lastModifiedAfter && {
+          'If-Modified-Since': options.lastModifiedAfter.toUTCString(),
+        }),
+        Accept: 'application/vnd.github.v3.raw',
+      },
+      // TODO(freben): The signal cast is there because pre-3.x versions of
+      // node-fetch have a very slightly deviating AbortSignal type signature.
+      // The difference does not affect us in practice however. The cast can
+      // be removed after we support ESM for CLI dependencies and migrate to
+      // version 3 of node-fetch.
+      // https://github.com/backstage/backstage/issues/8242
+      signal: options?.signal as any,
+    });
 
-    if (response.status === 304) {
-      throw new NotModifiedError();
-    }
-
-    if (response.ok) {
-      return ReadUrlResponseFactory.fromNodeJSReadable(response.body, {
-        etag: response.headers.get('ETag') ?? undefined,
-        lastModifiedAt: parseLastModified(
-          response.headers.get('Last-Modified'),
-        ),
-      });
-    }
-
-    let message = `${url} could not be read as ${ghUrl}, ${response.status} ${response.statusText}`;
-    if (response.status === 404) {
-      throw new NotFoundError(message);
-    }
-
-    // GitHub returns a 403 response with a couple of headers indicating rate
-    // limit status. See more in the GitHub docs:
-    // https://docs.github.com/en/rest/overview/resources-in-the-rest-api#rate-limiting
-    if (
-      response.status === 403 &&
-      response.headers.get('X-RateLimit-Remaining') === '0'
-    ) {
-      message += ' (rate limit exceeded)';
-    }
-
-    throw new Error(message);
+    return ReadUrlResponseFactory.fromNodeJSReadable(response.body, {
+      etag: response.headers.get('ETag') ?? undefined,
+      lastModifiedAt: parseLastModified(response.headers.get('Last-Modified')),
+    });
   }
 
   async readTree(
@@ -350,10 +320,26 @@ export class GithubUrlReader implements UrlReader {
     const response = await fetch(urlAsString, init);
 
     if (!response.ok) {
-      const message = `Request failed for ${urlAsString}, ${response.status} ${response.statusText}`;
+      let message = `Request failed for ${urlAsString}, ${response.status} ${response.statusText}`;
+
+      if (response.status === 304) {
+        throw new NotModifiedError();
+      }
+
       if (response.status === 404) {
         throw new NotFoundError(message);
       }
+
+      // GitHub returns a 403 response with a couple of headers indicating rate
+      // limit status. See more in the GitHub docs:
+      // https://docs.github.com/en/rest/overview/resources-in-the-rest-api#rate-limiting
+      if (
+        response.status === 403 &&
+        response.headers.get('X-RateLimit-Remaining') === '0'
+      ) {
+        message += ' (rate limit exceeded)';
+      }
+
       throw new Error(message);
     }