1. Use etag over sha for the identifier

2. Re-combine Resonse types into one

packages/backend-common/src/reading/AzureUrlReader.test.ts

@@ -188,7 +188,7 @@ describe('AzureUrlReader', () => {
         'https://dev.azure.com/organization/project/_git/repository',
       );
 
-      expect(response.sha).toBe('123abc2');
+      expect(response.etag).toBe('123abc2');
 
       const files = await response.files();
 
@@ -200,24 +200,24 @@ describe('AzureUrlReader', () => {
       expect(indexMarkdownFile.toString()).toBe('# Test\n');
     });
 
-    it('throws a NotModifiedError when given a sha in options', async () => {
+    it('throws a NotModifiedError when given a etag in options', async () => {
       const fnAzure = async () => {
         await processor.readTree(
           'https://dev.azure.com/organization/project/_git/repository',
-          { sha: '123abc2' },
+          { etag: '123abc2' },
         );
       };
 
       await expect(fnAzure).rejects.toThrow(NotModifiedError);
     });
 
-    it('should not throw a NotModifiedError when given an outdated sha in options', async () => {
+    it('should not throw a NotModifiedError when given an outdated etag in options', async () => {
       const response = await processor.readTree(
         'https://dev.azure.com/organization/project/_git/repository',
-        { sha: 'outdated123abc' },
+        { etag: 'outdated123abc' },
       );
 
-      expect(response.sha).toBe('123abc2');
+      expect(response.etag).toBe('123abc2');
       const files = await response.files();
 
       expect(files.length).toBe(2);

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

@@ -91,7 +91,7 @@ export class AzureUrlReader implements UrlReader {
     }
 
     const commitSha = (await commitsAzureResponse.json()).value[0].commitId;
-    if (options?.sha && options.sha === commitSha) {
+    if (options?.etag && options.etag === commitSha) {
       throw new NotModifiedError();
     }
 
@@ -107,14 +107,11 @@ export class AzureUrlReader implements UrlReader {
       throw new Error(message);
     }
 
-    const archiveResponse = await this.deps.treeResponseFactory.fromZipArchive({
+    return await this.deps.treeResponseFactory.fromZipArchive({
       stream: (archiveAzureResponse.body as unknown) as Readable,
+      etag: commitSha,
       filter: options?.filter,
     });
-
-    const response = archiveResponse as ReadTreeResponse;
-    response.sha = commitSha;
-    return response;
   }
 
   toString() {

packages/backend-common/src/reading/BitbucketUrlReader.test.ts

@@ -135,7 +135,7 @@ describe('BitbucketUrlReader', () => {
         'https://bitbucket.org/backstage/mock/src/master',
       );
 
-      expect(response.sha).toBe('12ab34cd56ef');
+      expect(response.etag).toBe('12ab34cd56ef');
 
       const files = await response.files();
 
@@ -152,7 +152,7 @@ describe('BitbucketUrlReader', () => {
         'https://bitbucket.mycompany.net/projects/backstage/repos/mock/browse/docs?at=some-branch',
       );
 
-      expect(response.sha).toBe('12ab34cd56ef');
+      expect(response.etag).toBe('12ab34cd56ef');
 
       const files = await response.files();
 
@@ -167,7 +167,7 @@ describe('BitbucketUrlReader', () => {
         'https://bitbucket.org/backstage/mock/src/master/docs',
       );
 
-      expect(response.sha).toBe('12ab34cd56ef');
+      expect(response.etag).toBe('12ab34cd56ef');
 
       const files = await response.files();
 
@@ -177,24 +177,24 @@ describe('BitbucketUrlReader', () => {
       expect(indexMarkdownFile.toString()).toBe('# Test\n');
     });
 
-    it('throws a NotModifiedError when given a sha in options', async () => {
+    it('throws a NotModifiedError when given a etag in options', async () => {
       const fnBitbucket = async () => {
         await bitbucketProcessor.readTree(
           'https://bitbucket.org/backstage/mock',
-          { sha: '12ab34cd56ef' },
+          { etag: '12ab34cd56ef' },
         );
       };
 
       await expect(fnBitbucket).rejects.toThrow(NotModifiedError);
     });
 
-    it('should not throw a NotModifiedError when given an outdated sha in options', async () => {
+    it('should not throw a NotModifiedError when given an outdated etag in options', async () => {
       const response = await bitbucketProcessor.readTree(
         'https://bitbucket.org/backstage/mock',
-        { sha: 'outdatedSha123abc' },
+        { etag: 'outdatedetag123abc' },
       );
 
-      expect(response.sha).toBe('12ab34cd56ef');
+      expect(response.etag).toBe('12ab34cd56ef');
     });
   });
 });

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

@@ -106,7 +106,7 @@ export class BitbucketUrlReader implements UrlReader {
     );
 
     const lastCommitShortHash = await this.getLastCommitShortHash(url);
-    if (options?.sha && options.sha === lastCommitShortHash) {
+    if (options?.etag && options.etag === lastCommitShortHash) {
       throw new NotModifiedError();
     }
 
@@ -130,15 +130,12 @@ export class BitbucketUrlReader implements UrlReader {
       folderPath = `${project}-${repoName}-${lastCommitShortHash}`;
     }
 
-    const archiveResponse = await this.treeResponseFactory.fromZipArchive({
+    return await this.treeResponseFactory.fromZipArchive({
       stream: (archiveBitbucketResponse.body as unknown) as Readable,
       path: `${folderPath}/${filepath}`,
+      etag: lastCommitShortHash,
       filter: options?.filter,
     });
-
-    const response = archiveResponse as ReadTreeResponse;
-    response.sha = lastCommitShortHash;
-    return response;
   }
 
   toString() {

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

@@ -128,7 +128,7 @@ describe('GithubUrlReader', () => {
     const branchesApiResponse = {
       name: 'main',
       commit: {
-        sha: 'sha123abc',
+        sha: 'etag123abc',
       },
     };
 
@@ -198,7 +198,7 @@ describe('GithubUrlReader', () => {
         'https://github.com/backstage/mock/tree/main',
       );
 
-      expect(response.sha).toBe('sha123abc');
+      expect(response.etag).toBe('etag123abc');
 
       const files = await response.files();
 
@@ -272,10 +272,10 @@ describe('GithubUrlReader', () => {
       expect(indexMarkdownFile.toString()).toBe('# Test\n');
     });
 
-    it('throws a NotModifiedError when given a sha in options', async () => {
+    it('throws a NotModifiedError when given a etag in options', async () => {
       const fnGithub = async () => {
         await githubProcessor.readTree('https://github.com/backstage/mock', {
-          sha: 'sha123abc',
+          etag: 'etag123abc',
         });
       };
 
@@ -283,7 +283,7 @@ describe('GithubUrlReader', () => {
         await gheProcessor.readTree(
           'https://ghe.github.com/backstage/mock/tree/main/docs',
           {
-            sha: 'sha123abc',
+            etag: 'etag123abc',
           },
         );
       };
@@ -292,11 +292,11 @@ describe('GithubUrlReader', () => {
       await expect(fnGhe).rejects.toThrow(NotModifiedError);
     });
 
-    it('should not throw error when given an outdated sha in options', async () => {
+    it('should not throw error when given an outdated etag in options', async () => {
       const response = await githubProcessor.readTree(
         'https://github.com/backstage/mock/tree/main',
         {
-          sha: 'outdatedSha123abc',
+          etag: 'outdatedetag123abc',
         },
       );
       expect((await response.files()).length).toBe(2);

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

@@ -151,7 +151,7 @@ export class GithubUrlReader implements UrlReader {
     }
     const commitSha = (await branchGitHubResponse.json()).commit.sha;
 
-    if (options?.sha && options.sha === commitSha) {
+    if (options?.etag && options.etag === commitSha) {
       throw new NotModifiedError();
     }
 
@@ -179,17 +179,14 @@ export class GithubUrlReader implements UrlReader {
 
     const path = `${repoName}-${branch}/${filepath}`;
 
-    const archiveResponse = await this.deps.treeResponseFactory.fromTarArchive({
+    return await this.deps.treeResponseFactory.fromTarArchive({
       // TODO(Rugvip): Underlying implementation of fetch will be node-fetch, we probably want
       //               to stick to using that in exclusively backend code.
       stream: (archive.body as unknown) as Readable,
       path,
+      etag: commitSha,
       filter: options?.filter,
     });
-
-    const response = archiveResponse as ReadTreeResponse;
-    response.sha = commitSha;
-    return response;
   }
 
   toString() {

packages/backend-common/src/reading/GitlabUrlReader.test.ts

@@ -284,10 +284,10 @@ describe('GitlabUrlReader', () => {
       expect(indexMarkdownFile.toString()).toBe('# Test\n');
     });
 
-    it('throws a NotModifiedError when given a sha in options', async () => {
+    it('throws a NotModifiedError when given a etag in options', async () => {
       const fnGitlab = async () => {
         await gitlabProcessor.readTree('https://gitlab.com/backstage/mock', {
-          sha: 'sha123abc',
+          etag: 'sha123abc',
         });
       };
 
@@ -295,7 +295,7 @@ describe('GitlabUrlReader', () => {
         await hostedGitlabProcessor.readTree(
           'https://gitlab.mycompany.com/backstage/mock',
           {
-            sha: 'sha123abc',
+            etag: 'sha123abc',
           },
         );
       };
@@ -304,11 +304,11 @@ describe('GitlabUrlReader', () => {
       await expect(fnHostedGitlab).rejects.toThrow(NotModifiedError);
     });
 
-    it('should not throw error when given an outdated sha in options', async () => {
+    it('should not throw error when given an outdated etag in options', async () => {
       const response = await gitlabProcessor.readTree(
         'https://gitlab.com/backstage/mock/tree/main',
         {
-          sha: 'outdatedSha123abc',
+          etag: 'outdatedsha123abc',
         },
       );
       expect((await response.files()).length).toBe(2);

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

@@ -121,7 +121,7 @@ export class GitlabUrlReader implements UrlReader {
 
     const commitSha = (await branchGitlabResponse.json()).commit.id;
 
-    if (options?.sha && options.sha === commitSha) {
+    if (options?.etag && options.etag === commitSha) {
       throw new NotModifiedError();
     }
 
@@ -144,15 +144,12 @@ export class GitlabUrlReader implements UrlReader {
       ? `${repoName}-${branch}-${commitSha}/${filepath}/`
       : '';
 
-    const archiveResponse = await this.treeResponseFactory.fromZipArchive({
+    return await this.treeResponseFactory.fromZipArchive({
       stream: (archiveGitLabResponse.body as unknown) as Readable,
       path,
+      etag: commitSha,
       filter: options?.filter,
     });
-
-    const response = archiveResponse as ReadTreeResponse;
-    response.sha = commitSha;
-    return response;
   }
 
   toString() {

packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts

@@ -17,7 +17,7 @@
 import os from 'os';
 import { Readable } from 'stream';
 import { Config } from '@backstage/config';
-import { ReadTreeArchiveResponse } from '../types';
+import { ReadTreeResponse } from '../types';
 import { TarArchiveResponse } from './TarArchiveResponse';
 import { ZipArchiveResponse } from './ZipArchiveResponse';
 
@@ -26,6 +26,8 @@ type FromArchiveOptions = {
   stream: Readable;
   // If set, the root of the tree will be set to the given directory path.
   path?: string;
+  // etag of the blob
+  etag: string;
   // Filter passed on from the ReadTreeOptions
   filter?: (path: string) => boolean;
 };
@@ -40,24 +42,22 @@ export class ReadTreeResponseFactory {
 
   constructor(private readonly workDir: string) {}
 
-  async fromTarArchive(
-    options: FromArchiveOptions,
-  ): Promise<ReadTreeArchiveResponse> {
+  async fromTarArchive(options: FromArchiveOptions): Promise<ReadTreeResponse> {
     return new TarArchiveResponse(
       options.stream,
       options.path ?? '',
       this.workDir,
+      options.etag,
       options.filter,
     );
   }
 
-  async fromZipArchive(
-    options: FromArchiveOptions,
-  ): Promise<ReadTreeArchiveResponse> {
+  async fromZipArchive(options: FromArchiveOptions): Promise<ReadTreeResponse> {
     return new ZipArchiveResponse(
       options.stream,
       options.path ?? '',
       this.workDir,
+      options.etag,
       options.filter,
     );
   }

packages/backend-common/src/reading/tree/TarArchiveResponse.test.ts

@@ -38,7 +38,7 @@ describe('TarArchiveResponse', () => {
   it('should read files', async () => {
     const stream = fs.createReadStream('/test-archive.tar.gz');
 
-    const res = new TarArchiveResponse(stream, 'mock-main/', '/tmp');
+    const res = new TarArchiveResponse(stream, 'mock-main/', '/tmp', 'etag');
     const files = await res.files();
 
     expect(files).toEqual([
@@ -61,8 +61,12 @@ describe('TarArchiveResponse', () => {
   it('should read files with filter', async () => {
     const stream = fs.createReadStream('/test-archive.tar.gz');
 
-    const res = new TarArchiveResponse(stream, 'mock-main/', '/tmp', path =>
-      path.endsWith('.yml'),
+    const res = new TarArchiveResponse(
+      stream,
+      'mock-main/',
+      '/tmp',
+      'etag',
+      path => path.endsWith('.yml'),
     );
     const files = await res.files();
 
@@ -79,14 +83,14 @@ describe('TarArchiveResponse', () => {
   it('should read as archive and files', async () => {
     const stream = fs.createReadStream('/test-archive.tar.gz');
 
-    const res = new TarArchiveResponse(stream, 'mock-main/', '/tmp');
+    const res = new TarArchiveResponse(stream, 'mock-main/', '/tmp', 'etag');
     const buffer = await res.archive();
 
     await expect(res.archive()).rejects.toThrow(
       'Response has already been read',
     );
 
-    const res2 = new TarArchiveResponse(buffer, '', '/tmp');
+    const res2 = new TarArchiveResponse(buffer, '', '/tmp', 'etag');
     const files = await res2.files();
 
     expect(files).toEqual([
@@ -109,7 +113,7 @@ describe('TarArchiveResponse', () => {
   it('should extract entire archive into directory', async () => {
     const stream = fs.createReadStream('/test-archive.tar.gz');
 
-    const res = new TarArchiveResponse(stream, '', '/tmp');
+    const res = new TarArchiveResponse(stream, '', '/tmp', 'etag');
     const dir = await res.dir();
 
     await expect(
@@ -123,7 +127,12 @@ describe('TarArchiveResponse', () => {
   it('should extract archive into directory with a subpath', async () => {
     const stream = fs.createReadStream('/test-archive.tar.gz');
 
-    const res = new TarArchiveResponse(stream, 'mock-main/docs/', '/tmp');
+    const res = new TarArchiveResponse(
+      stream,
+      'mock-main/docs/',
+      '/tmp',
+      'etag',
+    );
     const dir = await res.dir();
 
     expect(dir).toMatch(/^[\/\\]tmp[\/\\].*$/);
@@ -135,8 +144,12 @@ describe('TarArchiveResponse', () => {
   it('should extract archive into directory with a subpath and filter', async () => {
     const stream = fs.createReadStream('/test-archive.tar.gz');
 
-    const res = new TarArchiveResponse(stream, 'mock-main/', '/tmp', path =>
-      path.endsWith('.yml'),
+    const res = new TarArchiveResponse(
+      stream,
+      'mock-main/',
+      '/tmp',
+      'etag',
+      path => path.endsWith('.yml'),
     );
     const dir = await res.dir({ targetDir: '/tmp' });
 

packages/backend-common/src/reading/tree/TarArchiveResponse.ts

@@ -21,7 +21,7 @@ import { Readable, pipeline as pipelineCb } from 'stream';
 import { promisify } from 'util';
 import concatStream from 'concat-stream';
 import {
-  ReadTreeArchiveResponse,
+  ReadTreeResponse,
   ReadTreeResponseFile,
   ReadTreeResponseDirOptions,
 } from '../types';
@@ -34,13 +34,15 @@ const pipeline = promisify(pipelineCb);
 /**
  * Wraps a tar archive stream into a tree response reader.
  */
-export class TarArchiveResponse implements ReadTreeArchiveResponse {
+export class TarArchiveResponse implements ReadTreeResponse {
   private read = false;
+  public readonly etag;
 
   constructor(
     private readonly stream: Readable,
     private readonly subPath: string,
     private readonly workDir: string,
+    etag: string,
     private readonly filter?: (path: string) => boolean,
   ) {
     if (subPath) {
@@ -53,6 +55,8 @@ export class TarArchiveResponse implements ReadTreeArchiveResponse {
         );
       }
     }
+
+    this.etag = etag;
   }
 
   // Make sure the input stream is only read once

packages/backend-common/src/reading/tree/ZipArchiveResponse.test.ts

@@ -38,7 +38,7 @@ describe('ZipArchiveResponse', () => {
   it('should read files', async () => {
     const stream = fs.createReadStream('/test-archive.zip');
 
-    const res = new ZipArchiveResponse(stream, 'mock-main/', '/tmp');
+    const res = new ZipArchiveResponse(stream, 'mock-main/', '/tmp', 'etag');
     const files = await res.files();
 
     expect(files).toEqual([
@@ -61,8 +61,12 @@ describe('ZipArchiveResponse', () => {
   it('should read files with filter', async () => {
     const stream = fs.createReadStream('/test-archive.zip');
 
-    const res = new ZipArchiveResponse(stream, 'mock-main/', '/tmp', path =>
-      path.endsWith('.yml'),
+    const res = new ZipArchiveResponse(
+      stream,
+      'mock-main/',
+      '/tmp',
+      'etag',
+      path => path.endsWith('.yml'),
     );
     const files = await res.files();
 
@@ -79,14 +83,14 @@ describe('ZipArchiveResponse', () => {
   it('should read as archive and files', async () => {
     const stream = fs.createReadStream('/test-archive.zip');
 
-    const res = new ZipArchiveResponse(stream, 'mock-main/', '/tmp');
+    const res = new ZipArchiveResponse(stream, 'mock-main/', '/tmp', 'etag');
     const buffer = await res.archive();
 
     await expect(res.archive()).rejects.toThrow(
       'Response has already been read',
     );
 
-    const res2 = new ZipArchiveResponse(buffer, '', '/tmp');
+    const res2 = new ZipArchiveResponse(buffer, '', '/tmp', 'etag');
     const files = await res2.files();
 
     expect(files).toEqual([
@@ -109,7 +113,7 @@ describe('ZipArchiveResponse', () => {
   it('should extract entire archive into directory', async () => {
     const stream = fs.createReadStream('/test-archive.zip');
 
-    const res = new ZipArchiveResponse(stream, '', '/tmp');
+    const res = new ZipArchiveResponse(stream, '', '/tmp', 'etag');
     const dir = await res.dir();
 
     await expect(
@@ -123,7 +127,12 @@ describe('ZipArchiveResponse', () => {
   it('should extract archive into directory with a subpath', async () => {
     const stream = fs.createReadStream('/test-archive.zip');
 
-    const res = new ZipArchiveResponse(stream, 'mock-main/docs/', '/tmp');
+    const res = new ZipArchiveResponse(
+      stream,
+      'mock-main/docs/',
+      '/tmp',
+      'etag',
+    );
     const dir = await res.dir();
 
     expect(dir).toMatch(/^[\/\\]tmp[\/\\].*$/);
@@ -135,8 +144,12 @@ describe('ZipArchiveResponse', () => {
   it('should extract archive into directory with a subpath and filter', async () => {
     const stream = fs.createReadStream('/test-archive.zip');
 
-    const res = new ZipArchiveResponse(stream, 'mock-main/', '/tmp', path =>
-      path.endsWith('.yml'),
+    const res = new ZipArchiveResponse(
+      stream,
+      'mock-main/',
+      '/tmp',
+      'etag',
+      path => path.endsWith('.yml'),
     );
     const dir = await res.dir({ targetDir: '/tmp' });
 

packages/backend-common/src/reading/tree/ZipArchiveResponse.ts

@@ -20,7 +20,7 @@ import unzipper, { Entry } from 'unzipper';
 import archiver from 'archiver';
 import { Readable } from 'stream';
 import {
-  ReadTreeArchiveResponse,
+  ReadTreeResponse,
   ReadTreeResponseFile,
   ReadTreeResponseDirOptions,
 } from '../types';
@@ -28,13 +28,15 @@ import {
 /**
  * Wraps a zip archive stream into a tree response reader.
  */
-export class ZipArchiveResponse implements ReadTreeArchiveResponse {
+export class ZipArchiveResponse implements ReadTreeResponse {
   private read = false;
+  public readonly etag;
 
   constructor(
     private readonly stream: Readable,
     private readonly subPath: string,
     private readonly workDir: string,
+    etag: string,
     private readonly filter?: (path: string) => boolean,
   ) {
     if (subPath) {
@@ -47,6 +49,8 @@ export class ZipArchiveResponse implements ReadTreeArchiveResponse {
         );
       }
     }
+
+    this.etag = etag;
   }
 
   // Make sure the input stream is only read once

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

@@ -34,17 +34,17 @@ export type ReadTreeOptions = {
   filter?(path: string): boolean;
 
   /**
-   * A commit SHA can be provided to check whether readTree's response has changed from a previous execution.
+   * An etag can be provided to check whether readTree's response has changed from a previous execution.
    *
-   * In the readTree() response, a SHA is returned along with the tree blob. The SHA belongs to the
-   * latest commit on the target repository's branch that was used to read the blob.
+   * In the readTree() response, an etag is returned along with the tree blob. The etag is a unique identifer
+   * of the tree blob, usually the commit SHA or etag from the target.
    *
-   * When a SHA is given in ReadTreeOptions, readTree will first compare the SHA against the latest commit
-   * on the target branch. If they match, it will throw a NotModifiedError indicating that the readTree
-   * response will not differ from the previous response which included this particular SHA. If they mismatch,
-   * readTree will return a new SHA along with the rest of ReadTreeResponse.
+   * When a etag is given in ReadTreeOptions, readTree will first compare the etag against the etag
+   * on the target branch. If they match, readTree will throw a NotModifiedError indicating that the readTree
+   * response will not differ from the previous response which included this particular etag. If they mismatch,
+   * readTree will return the rest of ReadTreeResponse along with a new etag.
    */
-  sha?: string;
+  etag?: string;
 };
 
 /**
@@ -80,7 +80,7 @@ export type ReadTreeResponseDirOptions = {
   targetDir?: string;
 };
 
-export type ReadTreeArchiveResponse = {
+export type ReadTreeResponse = {
   files(): Promise<ReadTreeResponseFile[]>;
   archive(): Promise<NodeJS.ReadableStream>;
 
@@ -88,8 +88,9 @@ export type ReadTreeArchiveResponse = {
    * dir() extracts the tree response into a directory and returns the path of the directory.
    */
   dir(options?: ReadTreeResponseDirOptions): Promise<string>;
-};
 
-export interface ReadTreeResponse extends ReadTreeArchiveResponse {
-  sha: string;
-}
+  /**
+   * A unique identifer of the tree blob, usually the commit SHA or etag from the target.
+   */
+  etag: string;
+};

packages/techdocs-common/src/helpers.test.ts

@@ -135,7 +135,7 @@ describe('getDocFilesFromRepository', () => {
           archive: async () => {
             return Readable.from('');
           },
-          sha: '',
+          etag: '',
         };
       }
     }