From f5ed88501ba3c687a1e2f75ec340ae7e02c951fc Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Sun, 24 Jan 2021 16:54:01 +0100 Subject: [PATCH] backend: URL Reader readTree should not expect the extracted directory name If readTree can work without it receiving the top level directory name, it will be a relief. We won't have to deduce the directory name from the API responses or by any other means. --- .../src/reading/BitbucketUrlReader.ts | 24 +------------- .../src/reading/GithubUrlReader.ts | 28 +--------------- .../src/reading/GitlabUrlReader.ts | 26 +-------------- .../reading/tree/ReadTreeResponseFactory.ts | 9 +++--- .../reading/tree/TarArchiveResponse.test.ts | 32 ++++++------------- .../src/reading/tree/TarArchiveResponse.ts | 26 ++++++++++++--- .../reading/tree/ZipArchiveResponse.test.ts | 31 ++++++------------ .../src/reading/tree/ZipArchiveResponse.ts | 29 +++++++++++++---- packages/backend-common/src/reading/types.ts | 3 ++ 9 files changed, 72 insertions(+), 136 deletions(-) diff --git a/packages/backend-common/src/reading/BitbucketUrlReader.ts b/packages/backend-common/src/reading/BitbucketUrlReader.ts index e9727e04cf..bbccfe4794 100644 --- a/packages/backend-common/src/reading/BitbucketUrlReader.ts +++ b/packages/backend-common/src/reading/BitbucketUrlReader.ts @@ -121,31 +121,9 @@ export class BitbucketUrlReader implements UrlReader { throw new Error(message); } - // Get the filename of archive from the header of the response - const contentDispositionHeader = archiveBitbucketResponse.headers.get( - 'content-disposition', - ) as string; - if (!contentDispositionHeader) { - throw new Error( - `Failed to read tree from ${url}. ` + - 'Bitbucket API response for downloading archive does not contain content-disposition header ', - ); - } - const fileNameRegEx = new RegExp( - /^attachment; filename=(?.*).zip$/, - ); - const archiveFileName = contentDispositionHeader.match(fileNameRegEx) - ?.groups?.fileName; - if (!archiveFileName) { - throw new Error( - `Failed to read tree from ${url}. Bitbucket API response for downloading archive has an unexpected ` + - `format of content-disposition header ${contentDispositionHeader} `, - ); - } - return await this.treeResponseFactory.fromZipArchive({ stream: (archiveBitbucketResponse.body as unknown) as Readable, - path: `${archiveFileName}/${filepath}`, + subpath: filepath, etag: lastCommitShortHash, filter: options?.filter, }); diff --git a/packages/backend-common/src/reading/GithubUrlReader.ts b/packages/backend-common/src/reading/GithubUrlReader.ts index 6c7cefe2ef..4f1aa7c07b 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.ts @@ -166,37 +166,11 @@ export class GithubUrlReader implements UrlReader { throw new Error(message); } - // Get the filename of archive from the header of the response - const contentDispositionHeader = archive.headers.get( - 'content-disposition', - ) as string; - if (!contentDispositionHeader) { - throw new Error( - `Failed to read tree from ${url}. ` + - 'GitHub API response for downloading archive does not contain content-disposition header ', - ); - } - const fileNameRegEx = new RegExp( - /^attachment; filename=(?.*).tar.gz$/, - ); - const archiveFileName = contentDispositionHeader.match(fileNameRegEx) - ?.groups?.fileName; - if (!archiveFileName) { - throw new Error( - `Failed to read tree from ${url}. GitHub API response for downloading archive has an unexpected ` + - `format of content-disposition header ${contentDispositionHeader} `, - ); - } - - // The path includes the name of the directory inside the tarball and a sub path - // if requested in readTree. - const path = `${archiveFileName}/${filepath}`; - 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, + subpath: filepath, etag: commitSha, filter: options?.filter, }); diff --git a/packages/backend-common/src/reading/GitlabUrlReader.ts b/packages/backend-common/src/reading/GitlabUrlReader.ts index 654f4f9a85..22316f6895 100644 --- a/packages/backend-common/src/reading/GitlabUrlReader.ts +++ b/packages/backend-common/src/reading/GitlabUrlReader.ts @@ -140,33 +140,9 @@ export class GitlabUrlReader implements UrlReader { throw new Error(message); } - // Get the filename of archive from the header of the response - const contentDispositionHeader = archiveGitLabResponse.headers.get( - 'content-disposition', - ) as string; - if (!contentDispositionHeader) { - throw new Error( - `Failed to read tree from ${url}. ` + - 'GitLab API response for downloading archive does not contain content-disposition header ', - ); - } - const fileNameRegEx = new RegExp( - /^attachment; filename="(?.*).zip"$/, - ); - const archiveFileName = contentDispositionHeader.match(fileNameRegEx) - ?.groups?.fileName; - if (!archiveFileName) { - throw new Error( - `Failed to read tree from ${url}. GitLab API response for downloading archive has an unexpected ` + - `format of content-disposition header ${contentDispositionHeader} `, - ); - } - - const path = filepath ? `${archiveFileName}/${filepath}/` : ''; - return await this.treeResponseFactory.fromZipArchive({ stream: (archiveGitLabResponse.body as unknown) as Readable, - path, + subpath: filepath, etag: commitSha, filter: options?.filter, }); diff --git a/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts b/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts index 7332154d09..05ecdb0fc3 100644 --- a/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts +++ b/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts @@ -24,8 +24,9 @@ import { ZipArchiveResponse } from './ZipArchiveResponse'; type FromArchiveOptions = { // A binary stream of a tar archive. stream: Readable; - // If set, the root of the tree will be set to the given directory path. - path?: string; + // If unset, the files at the root of the tree will be read. + // subpath must not contain the name of the top level directory. + subpath?: string; // etag of the blob etag: string; // Filter passed on from the ReadTreeOptions @@ -45,7 +46,7 @@ export class ReadTreeResponseFactory { async fromTarArchive(options: FromArchiveOptions): Promise { return new TarArchiveResponse( options.stream, - options.path ?? '', + options.subpath ?? '', this.workDir, options.etag, options.filter, @@ -55,7 +56,7 @@ export class ReadTreeResponseFactory { async fromZipArchive(options: FromArchiveOptions): Promise { return new ZipArchiveResponse( options.stream, - options.path ?? '', + options.subpath ?? '', this.workDir, options.etag, options.filter, diff --git a/packages/backend-common/src/reading/tree/TarArchiveResponse.test.ts b/packages/backend-common/src/reading/tree/TarArchiveResponse.test.ts index 2cbfc4a89e..2b76bea9e3 100644 --- a/packages/backend-common/src/reading/tree/TarArchiveResponse.test.ts +++ b/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', 'etag'); + const res = new TarArchiveResponse(stream, '', '/tmp', 'etag'); const files = await res.files(); expect(files).toEqual([ @@ -61,12 +61,8 @@ 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', - 'etag', - path => path.endsWith('.yml'), + const res = new TarArchiveResponse(stream, '', '/tmp', 'etag', path => + path.endsWith('.yml'), ); const files = await res.files(); @@ -83,7 +79,7 @@ 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', 'etag'); + const res = new TarArchiveResponse(stream, '', '/tmp', 'etag'); const buffer = await res.archive(); await expect(res.archive()).rejects.toThrow( @@ -115,24 +111,18 @@ describe('TarArchiveResponse', () => { const res = new TarArchiveResponse(stream, '', '/tmp', 'etag'); const dir = await res.dir(); - await expect( - fs.readFile(resolvePath(dir, 'mock-main/mkdocs.yml'), 'utf8'), + fs.readFile(resolvePath(dir, 'mkdocs.yml'), 'utf8'), ).resolves.toBe('site_name: Test\n'); await expect( - fs.readFile(resolvePath(dir, 'mock-main/docs/index.md'), 'utf8'), + fs.readFile(resolvePath(dir, 'docs/index.md'), 'utf8'), ).resolves.toBe('# Test\n'); }); 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', - 'etag', - ); + const res = new TarArchiveResponse(stream, 'docs', '/tmp', 'etag'); const dir = await res.dir(); expect(dir).toMatch(/^[\/\\]tmp[\/\\].*$/); @@ -144,12 +134,8 @@ 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', - 'etag', - path => path.endsWith('.yml'), + const res = new TarArchiveResponse(stream, '', '/tmp', 'etag', path => + path.endsWith('.yml'), ); const dir = await res.dir({ targetDir: '/tmp' }); diff --git a/packages/backend-common/src/reading/tree/TarArchiveResponse.ts b/packages/backend-common/src/reading/tree/TarArchiveResponse.ts index 5927eb75a1..96d3a5d495 100644 --- a/packages/backend-common/src/reading/tree/TarArchiveResponse.ts +++ b/packages/backend-common/src/reading/tree/TarArchiveResponse.ts @@ -30,6 +30,10 @@ import { const TarParseStream = (Parse as unknown) as { new (): ParseStream }; const pipeline = promisify(pipelineCb); +// Matches a directory name + one `/` at the start of any string, +// containing any character except `/` one or more times, and ending with a `/` +// e.g. Will match `dirA/` in `dirA/dirB/file.ext` +const directoryNameRegex = /^[^\/]+\//; /** * Wraps a tar archive stream into a tree response reader. @@ -78,14 +82,18 @@ export class TarArchiveResponse implements ReadTreeResponse { return; } + // File path relative to the root extracted directory. Will remove the + // top level dir name from the path since its name is hard to predetermine. + const relativePath = entry.path.replace(directoryNameRegex, ''); + if (this.subPath) { - if (!entry.path.startsWith(this.subPath)) { + if (!relativePath.startsWith(this.subPath)) { entry.resume(); return; } } - const path = entry.path.slice(this.subPath.length); + const path = relativePath.slice(this.subPath.length); if (this.filter) { if (!this.filter(path)) { entry.resume(); @@ -97,7 +105,10 @@ export class TarArchiveResponse implements ReadTreeResponse { await pipeline(entry, concatStream(resolve)); }); - files.push({ path, content: () => content }); + files.push({ + path, + content: () => content, + }); entry.resume(); }); @@ -138,7 +149,9 @@ export class TarArchiveResponse implements ReadTreeResponse { options?.targetDir ?? (await fs.mkdtemp(path.join(this.workDir, 'backstage-'))); - const strip = this.subPath ? this.subPath.split('/').length - 1 : 0; + // Equivalent of tar --strip-components=N + // When no subPath is given, remove just 1 top level directory + const strip = this.subPath ? this.subPath.split('/').length : 1; await pipeline( this.stream, @@ -146,7 +159,10 @@ export class TarArchiveResponse implements ReadTreeResponse { strip, cwd: dir, filter: path => { - if (this.subPath && !path.startsWith(this.subPath)) { + // File path relative to the root extracted directory. Will remove the + // top level dir name from the path since its name is hard to predetermine. + const relativePath = path.replace(directoryNameRegex, ''); + if (this.subPath && !relativePath.startsWith(this.subPath)) { return false; } if (this.filter) { diff --git a/packages/backend-common/src/reading/tree/ZipArchiveResponse.test.ts b/packages/backend-common/src/reading/tree/ZipArchiveResponse.test.ts index b42ec79d81..3bcaa5e0e3 100644 --- a/packages/backend-common/src/reading/tree/ZipArchiveResponse.test.ts +++ b/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', 'etag'); + const res = new ZipArchiveResponse(stream, '', '/tmp', 'etag'); const files = await res.files(); expect(files).toEqual([ @@ -61,12 +61,8 @@ describe('ZipArchiveResponse', () => { it('should read files with filter', async () => { const stream = fs.createReadStream('/test-archive.zip'); - const res = new ZipArchiveResponse( - stream, - 'mock-main/', - '/tmp', - 'etag', - path => path.endsWith('.yml'), + const res = new ZipArchiveResponse(stream, '', '/tmp', 'etag', path => + path.endsWith('.yml'), ); const files = await res.files(); @@ -83,7 +79,7 @@ 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', 'etag'); + const res = new ZipArchiveResponse(stream, '', '/tmp', 'etag'); const buffer = await res.archive(); await expect(res.archive()).rejects.toThrow( @@ -117,22 +113,17 @@ describe('ZipArchiveResponse', () => { const dir = await res.dir(); await expect( - fs.readFile(resolvePath(dir, 'mock-main/mkdocs.yml'), 'utf8'), + fs.readFile(resolvePath(dir, 'mkdocs.yml'), 'utf8'), ).resolves.toBe('site_name: Test\n'); await expect( - fs.readFile(resolvePath(dir, 'mock-main/docs/index.md'), 'utf8'), + fs.readFile(resolvePath(dir, 'docs/index.md'), 'utf8'), ).resolves.toBe('# Test\n'); }); 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', - 'etag', - ); + const res = new ZipArchiveResponse(stream, 'docs/', '/tmp', 'etag'); const dir = await res.dir(); expect(dir).toMatch(/^[\/\\]tmp[\/\\].*$/); @@ -144,12 +135,8 @@ 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', - 'etag', - path => path.endsWith('.yml'), + const res = new ZipArchiveResponse(stream, '', '/tmp', 'etag', path => + path.endsWith('.yml'), ); const dir = await res.dir({ targetDir: '/tmp' }); diff --git a/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts b/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts index 07d34faaa3..37ff32741f 100644 --- a/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts +++ b/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts @@ -25,6 +25,11 @@ import { ReadTreeResponseDirOptions, } from '../types'; +// Matches a directory name + one `/` at the start of any string, +// containing any character except / one or more times, and ending with a `/` +// e.g. Will match `dirA/` in `dirA/dirB/file.ext` +const directoryNameRegex = /^[^\/]+\//; + /** * Wraps a zip archive stream into a tree response reader. */ @@ -60,18 +65,26 @@ export class ZipArchiveResponse implements ReadTreeResponse { this.read = true; } - private getPath(entry: Entry): string { - return entry.path.slice(this.subPath.length); + // Will remove the top level dir name from the path since its name is hard to predetermine. + private stripTopDirectory(path: string): string { + return path.replace(directoryNameRegex, ''); + } + + // File path relative to the root extracted directory or a sub directory if subpath is set. + private getInnerPath(path: string): string { + return path.slice(this.subPath.length); } private shouldBeIncluded(entry: Entry): boolean { + const strippedPath = this.stripTopDirectory(entry.path); + if (this.subPath) { - if (!entry.path.startsWith(this.subPath)) { + if (!strippedPath.startsWith(this.subPath)) { return false; } } if (this.filter) { - return this.filter(this.getPath(entry)); + return this.filter(this.getInnerPath(entry.path)); } return true; } @@ -91,7 +104,7 @@ export class ZipArchiveResponse implements ReadTreeResponse { if (this.shouldBeIncluded(entry)) { files.push({ - path: this.getPath(entry), + path: this.getInnerPath(this.stripTopDirectory(entry.path)), content: () => entry.buffer(), }); } else { @@ -115,7 +128,7 @@ export class ZipArchiveResponse implements ReadTreeResponse { .pipe(unzipper.Parse()) .on('entry', (entry: Entry) => { if (entry.type === 'File' && this.shouldBeIncluded(entry)) { - archive.append(entry, { name: this.getPath(entry) }); + archive.append(entry, { name: this.getInnerPath(entry.path) }); } else { entry.autodrain(); } @@ -139,7 +152,9 @@ export class ZipArchiveResponse implements ReadTreeResponse { // Ignore directory entries since we handle that with the file entries // as a zip can have files with directories without directory entries if (entry.type === 'File' && this.shouldBeIncluded(entry)) { - const entryPath = this.getPath(entry); + const entryPath = this.getInnerPath( + this.stripTopDirectory(entry.path), + ); const dirname = path.dirname(entryPath); if (dirname) { await fs.mkdirp(path.join(dir, dirname)); diff --git a/packages/backend-common/src/reading/types.ts b/packages/backend-common/src/reading/types.ts index e98f760d8f..f9dfbe9aff 100644 --- a/packages/backend-common/src/reading/types.ts +++ b/packages/backend-common/src/reading/types.ts @@ -81,6 +81,9 @@ export type ReadTreeResponseDirOptions = { }; export type ReadTreeResponse = { + /** + * files() returns an array of all the files inside the tree and corresponding functions to read their content. + */ files(): Promise; archive(): Promise;