From e74422b4d844b43172b76216fa5e79b7be528dd8 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Nov 2020 10:37:04 +0100 Subject: [PATCH 1/9] backend-common: make ReadTreeResponse.files async --- packages/backend-common/src/reading/GithubUrlReader.test.ts | 2 +- packages/backend-common/src/reading/GithubUrlReader.ts | 2 +- packages/backend-common/src/reading/types.ts | 2 +- plugins/techdocs-backend/src/helpers.test.ts | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/backend-common/src/reading/GithubUrlReader.test.ts b/packages/backend-common/src/reading/GithubUrlReader.test.ts index 1e7326af2b..7552547924 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.test.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.test.ts @@ -272,7 +272,7 @@ describe('GithubUrlReader', () => { ['mkdocs.yml', 'docs'], ); - const files = response.files(); + const files = await response.files(); const mkDocsFile = await files[0].content(); const indexMarkdownFile = await files[1].content(); diff --git a/packages/backend-common/src/reading/GithubUrlReader.ts b/packages/backend-common/src/reading/GithubUrlReader.ts index 906076a3c2..41db541f7f 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.ts @@ -286,7 +286,7 @@ export class GithubUrlReader implements UrlReader { // @ts-ignore Typescript doesn't consider .pipe a method on ReadableStream. Don't know why. repoArchive.body?.pipe(parser).on('finish', () => { resolve({ - files: () => { + files: async () => { return files; }, archive: () => { diff --git a/packages/backend-common/src/reading/types.ts b/packages/backend-common/src/reading/types.ts index 26ac4af45e..d9fa5d839e 100644 --- a/packages/backend-common/src/reading/types.ts +++ b/packages/backend-common/src/reading/types.ts @@ -49,7 +49,7 @@ export type File = { }; export type ReadTreeResponse = { - files(): File[]; + files(): Promise; archive(): Promise; dir(outDir?: string): Promise; }; diff --git a/plugins/techdocs-backend/src/helpers.test.ts b/plugins/techdocs-backend/src/helpers.test.ts index 8d4d69ba26..c3f2261637 100644 --- a/plugins/techdocs-backend/src/helpers.test.ts +++ b/plugins/techdocs-backend/src/helpers.test.ts @@ -30,7 +30,7 @@ describe('getDocFilesFromRepository', () => { dir: async () => { return '/tmp/testfolder'; }, - files: () => { + files: async () => { return []; }, archive: async () => { @@ -78,7 +78,7 @@ describe('getDocFilesFromRepository', () => { dir: async () => { return '/tmp/testfolder'; }, - files: () => { + files: async () => { return []; }, archive: async () => { From 1b632b5aa71f695a3d265be6ec6327c4158babb5 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 10 Nov 2020 10:47:24 +0100 Subject: [PATCH 2/9] backend-common: add archive tree reading utilities --- .../src/reading/tree/ArchiveResponse.test.ts | 93 +++++++++++++ .../src/reading/tree/ArchiveResponse.ts | 124 ++++++++++++++++++ .../reading/tree/ReadTreeResponseFactory.ts | 47 +++++++ .../backend-common/src/reading/tree/index.ts | 17 +++ 4 files changed, 281 insertions(+) create mode 100644 packages/backend-common/src/reading/tree/ArchiveResponse.test.ts create mode 100644 packages/backend-common/src/reading/tree/ArchiveResponse.ts create mode 100644 packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts create mode 100644 packages/backend-common/src/reading/tree/index.ts diff --git a/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts b/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts new file mode 100644 index 0000000000..57a490650c --- /dev/null +++ b/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts @@ -0,0 +1,93 @@ +/* + * Copyright 2020 Spotify AB + * + * 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 fs from 'fs-extra'; +import mockFs from 'mock-fs'; +import { Readable } from 'stream'; +import { resolve as resolvePath } from 'path'; +import { ArchiveResponse } from './ArchiveResponse'; + +const archiveData = fs.readFileSync( + resolvePath(__filename, '../../__fixtures__/repo.tar.gz'), +); + +describe('ArchiveResponse', () => { + beforeEach(() => { + mockFs({ + '/test-archive.tar.gz': archiveData, + '/tmp': mockFs.directory(), + }); + }); + + afterEach(() => { + mockFs.restore(); + }); + + it('should read as archive and files', async () => { + const stream = fs.createReadStream('/test-archive.tar.gz'); + + const res = new ArchiveResponse(stream, 'mock-repo', '/tmp'); + const buffer = await res.archive(); + + await expect(res.archive()).rejects.toThrow( + 'Response has already been read', + ); + + const res2 = new ArchiveResponse(Readable.from(buffer), '', '/tmp'); + const files = await res2.files(); + + expect(files).toEqual([ + { + path: './mkdocs.yml', + content: expect.any(Function), + }, + { + path: './docs/index.md', + content: expect.any(Function), + }, + ]); + const contents = await Promise.all(files.map(f => f.content())); + expect(contents.map(c => c.toString('utf8').trim())).toEqual([ + 'site_name: Test', + '# Test', + ]); + }); + + it('should extract entire archive into directory', async () => { + const stream = fs.createReadStream('/test-archive.tar.gz'); + + const res = new ArchiveResponse(stream, '', '/tmp'); + const dir = await res.dir(); + + await expect( + fs.readFile(resolvePath(dir, 'mock-repo/mkdocs.yml'), 'utf8'), + ).resolves.toBe('site_name: Test\n'); + await expect( + fs.readFile(resolvePath(dir, 'mock-repo/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 ArchiveResponse(stream, 'mock-repo/docs', '/tmp'); + const dir = await res.dir(); + + await expect( + fs.readFile(resolvePath(dir, 'index.md'), 'utf8'), + ).resolves.toBe('# Test\n'); + }); +}); diff --git a/packages/backend-common/src/reading/tree/ArchiveResponse.ts b/packages/backend-common/src/reading/tree/ArchiveResponse.ts new file mode 100644 index 0000000000..65ad8c18fe --- /dev/null +++ b/packages/backend-common/src/reading/tree/ArchiveResponse.ts @@ -0,0 +1,124 @@ +/* + * Copyright 2020 Spotify AB + * + * 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 os from 'os'; +import tar, { Parse, ParseStream, ReadEntry } from 'tar'; +import path from 'path'; +import fs from 'fs-extra'; +import { Readable, pipeline as pipelineCb } from 'stream'; +import { promisify } from 'util'; +import concatStream from 'concat-stream'; +import { ReadTreeResponse, File } from '../types'; + +// Tar types for `Parse` is not a proper constructor, but it should be +const TarParseStream = (Parse as unknown) as { new (): ParseStream }; + +const pipeline = promisify(pipelineCb); + +/** + * Wraps a tar archive stream into a tree response reader. + */ +export class ArchiveResponse implements ReadTreeResponse { + private read = false; + + constructor( + private readonly stream: Readable, + private readonly subPath: string, + private readonly workDir: string = os.tmpdir(), + ) {} + + // Make sure the input stream is only read once + private onlyOnce() { + if (this.read) { + throw new Error('Response has already been read'); + } + this.read = true; + } + + async files(): Promise { + this.onlyOnce(); + + const files: File[] = []; + const parser = new TarParseStream(); + + parser.on('entry', (entry: ReadEntry & Readable) => { + if (entry.type === 'Directory') { + entry.resume(); + return; + } + + const contentPromise = new Promise(async resolve => { + await pipeline(entry, concatStream(resolve)); + }); + + files.push({ + path: entry.path, + content: () => contentPromise, + }); + + entry.resume(); + }); + + await pipeline(this.stream, parser); + + return files; + } + + async archive(): Promise { + if (!this.subPath) { + this.onlyOnce(); + + return new Promise(resolve => + pipeline(this.stream, concatStream(resolve)), + ); + } + + // TODO(Rugvip): method for repacking a tar with a subpath is to simply extract into a + // tmp dir and recreate the archive. Would be nicer to stream things instead. + const tmpDir = await this.dir(); + + try { + return await new Promise(async resolve => { + await pipeline( + tar.create({ cwd: tmpDir }, ['.']), + concatStream(resolve), + ); + }); + } finally { + await fs.remove(tmpDir); + } + } + + async dir(outDir?: string): Promise { + this.onlyOnce(); + + const dir = + outDir ?? (await fs.mkdtemp(path.join(this.workDir, 'backstage-'))); + + const strip = this.subPath ? this.subPath.split('/').length : 0; + + await pipeline( + this.stream, + tar.extract({ + strip, + cwd: dir, + filter: path => path.startsWith(this.subPath), + }), + ); + + return dir; + } +} diff --git a/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts b/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts new file mode 100644 index 0000000000..0cdf926720 --- /dev/null +++ b/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts @@ -0,0 +1,47 @@ +/* + * Copyright 2020 Spotify AB + * + * 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 os from 'os'; +import { Readable } from 'stream'; +import { Config } from '@backstage/config'; +import { ReadTreeResponse } from '../types'; +import { ArchiveResponse } from './ArchiveResponse'; + +type FromArchiveOptions = { + // A binary stream of a tar archive. + stream: Readable; + // If set, root of the tree will be set to the given path. Should not have a trailing `/`. + path?: string; +}; + +export class ReadTreeResponseFactory { + static create(options: { config: Config }): ReadTreeResponseFactory { + return new ReadTreeResponseFactory( + options.config.getOptionalString('backend.workingDirectory') ?? + os.tmpdir(), + ); + } + + constructor(private readonly workDir: string) {} + + async fromArchive(options: FromArchiveOptions): Promise { + return new ArchiveResponse( + options.stream, + options.path ?? '', + this.workDir, + ); + } +} diff --git a/packages/backend-common/src/reading/tree/index.ts b/packages/backend-common/src/reading/tree/index.ts new file mode 100644 index 0000000000..858cf15877 --- /dev/null +++ b/packages/backend-common/src/reading/tree/index.ts @@ -0,0 +1,17 @@ +/* + * Copyright 2020 Spotify AB + * + * 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. + */ + +export { ReadTreeResponseFactory } from './ReadTreeResponseFactory'; From 73231427e138a7aa983a22a5d84e725805500d10 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Nov 2020 12:20:07 +0100 Subject: [PATCH 3/9] backend-common: add top-level archive filtering to ArchiveResponse + fix files() --- .../src/reading/tree/ArchiveResponse.test.ts | 67 +++++++++++++++++-- .../src/reading/tree/ArchiveResponse.ts | 58 ++++++++++++---- .../reading/tree/ReadTreeResponseFactory.ts | 3 + 3 files changed, 112 insertions(+), 16 deletions(-) diff --git a/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts b/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts index 57a490650c..6b8f29de8c 100644 --- a/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts +++ b/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts @@ -36,10 +36,51 @@ describe('ArchiveResponse', () => { mockFs.restore(); }); + it('should read files', async () => { + const stream = fs.createReadStream('/test-archive.tar.gz'); + + const res = new ArchiveResponse(stream, 'mock-repo/', '/tmp'); + const files = await res.files(); + + expect(files).toEqual([ + { + path: 'mkdocs.yml', + content: expect.any(Function), + }, + { + path: 'docs/index.md', + content: expect.any(Function), + }, + ]); + const contents = await Promise.all(files.map(f => f.content())); + expect(contents.map(c => c.toString('utf8').trim())).toEqual([ + 'site_name: Test', + '# Test', + ]); + }); + + it('should read files with filter', async () => { + const stream = fs.createReadStream('/test-archive.tar.gz'); + + const res = new ArchiveResponse(stream, 'mock-repo/', '/tmp', path => + path.endsWith('.yml'), + ); + const files = await res.files(); + + expect(files).toEqual([ + { + path: 'mkdocs.yml', + content: expect.any(Function), + }, + ]); + const content = await files[0].content(); + expect(content.toString('utf8').trim()).toEqual('site_name: Test'); + }); + it('should read as archive and files', async () => { const stream = fs.createReadStream('/test-archive.tar.gz'); - const res = new ArchiveResponse(stream, 'mock-repo', '/tmp'); + const res = new ArchiveResponse(stream, 'mock-repo/', '/tmp'); const buffer = await res.archive(); await expect(res.archive()).rejects.toThrow( @@ -51,11 +92,11 @@ describe('ArchiveResponse', () => { expect(files).toEqual([ { - path: './mkdocs.yml', + path: 'mkdocs.yml', content: expect.any(Function), }, { - path: './docs/index.md', + path: 'docs/index.md', content: expect.any(Function), }, ]); @@ -83,11 +124,29 @@ describe('ArchiveResponse', () => { it('should extract archive into directory with a subpath', async () => { const stream = fs.createReadStream('/test-archive.tar.gz'); - const res = new ArchiveResponse(stream, 'mock-repo/docs', '/tmp'); + const res = new ArchiveResponse(stream, 'mock-repo/docs/', '/tmp'); const dir = await res.dir(); + expect(dir).toMatch(/^\/tmp\/.*$/); await expect( fs.readFile(resolvePath(dir, 'index.md'), 'utf8'), ).resolves.toBe('# Test\n'); }); + + it('should extract archive into directory with a subpath and filter', async () => { + const stream = fs.createReadStream('/test-archive.tar.gz'); + + const res = new ArchiveResponse(stream, 'mock-repo/', '/tmp', path => + path.endsWith('.yml'), + ); + const dir = await res.dir({ targetDir: '/tmp' }); + + expect(dir).toBe('/tmp'); + await expect(fs.pathExists(resolvePath(dir, 'mkdocs.yml'))).resolves.toBe( + true, + ); + await expect( + fs.pathExists(resolvePath(dir, 'docs/index.md')), + ).resolves.toBe(false); + }); }); diff --git a/packages/backend-common/src/reading/tree/ArchiveResponse.ts b/packages/backend-common/src/reading/tree/ArchiveResponse.ts index 65ad8c18fe..83f6f6a8e5 100644 --- a/packages/backend-common/src/reading/tree/ArchiveResponse.ts +++ b/packages/backend-common/src/reading/tree/ArchiveResponse.ts @@ -21,7 +21,7 @@ import fs from 'fs-extra'; import { Readable, pipeline as pipelineCb } from 'stream'; import { promisify } from 'util'; import concatStream from 'concat-stream'; -import { ReadTreeResponse, File } from '../types'; +import { ReadTreeResponse, File, ReadTreeResponseDirOptions } from '../types'; // Tar types for `Parse` is not a proper constructor, but it should be const TarParseStream = (Parse as unknown) as { new (): ParseStream }; @@ -38,7 +38,17 @@ export class ArchiveResponse implements ReadTreeResponse { private readonly stream: Readable, private readonly subPath: string, private readonly workDir: string = os.tmpdir(), - ) {} + private readonly filter?: (path: string) => boolean, + ) { + if (subPath) { + if (!subPath.endsWith('/')) { + throw new TypeError('ArchiveResponse subPath must end with a /'); + } + if (subPath.startsWith('/')) { + throw new TypeError('ArchiveResponse subPath must not start with a /'); + } + } + } // Make sure the input stream is only read once private onlyOnce() { @@ -60,14 +70,28 @@ export class ArchiveResponse implements ReadTreeResponse { return; } - const contentPromise = new Promise(async resolve => { + if (this.subPath) { + if (!entry.path.startsWith(this.subPath)) { + entry.resume(); + return; + } + } + + const path = this.subPath + ? entry.path.replace(this.subPath, '') + : entry.path; + if (this.filter) { + if (!this.filter(path)) { + entry.resume(); + return; + } + } + + const content = new Promise(async resolve => { await pipeline(entry, concatStream(resolve)); }); - files.push({ - path: entry.path, - content: () => contentPromise, - }); + files.push({ path, content: () => content }); entry.resume(); }); @@ -93,7 +117,7 @@ export class ArchiveResponse implements ReadTreeResponse { try { return await new Promise(async resolve => { await pipeline( - tar.create({ cwd: tmpDir }, ['.']), + tar.create({ cwd: tmpDir }, ['']), concatStream(resolve), ); }); @@ -102,20 +126,30 @@ export class ArchiveResponse implements ReadTreeResponse { } } - async dir(outDir?: string): Promise { + async dir(options?: ReadTreeResponseDirOptions): Promise { this.onlyOnce(); const dir = - outDir ?? (await fs.mkdtemp(path.join(this.workDir, 'backstage-'))); + options?.targetDir ?? + (await fs.mkdtemp(path.join(this.workDir, 'backstage-'))); - const strip = this.subPath ? this.subPath.split('/').length : 0; + const strip = this.subPath ? this.subPath.split('/').length - 1 : 0; await pipeline( this.stream, tar.extract({ strip, cwd: dir, - filter: path => path.startsWith(this.subPath), + filter: path => { + if (this.subPath && !path.startsWith(this.subPath)) { + return false; + } + if (this.filter) { + const innerPath = path.split('/').slice(strip).join('/'); + return this.filter(innerPath); + } + return true; + }, }), ); diff --git a/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts b/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts index 0cdf926720..5044cdabdc 100644 --- a/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts +++ b/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts @@ -25,6 +25,8 @@ type FromArchiveOptions = { stream: Readable; // If set, root of the tree will be set to the given path. Should not have a trailing `/`. path?: string; + // Filter passed on from the ReadTreeOptions + filter?: (path: string) => boolean; }; export class ReadTreeResponseFactory { @@ -42,6 +44,7 @@ export class ReadTreeResponseFactory { options.stream, options.path ?? '', this.workDir, + options.filter, ); } } From 5bc30d262f928955776763a1f217da2811858aee Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Nov 2020 12:31:23 +0100 Subject: [PATCH 4/9] backend-common: make ReadTreeResponse.dir() accept an options object instead --- .../src/reading/GithubUrlReader.test.ts | 2 +- .../backend-common/src/reading/GithubUrlReader.ts | 13 ++++++++++--- packages/backend-common/src/reading/types.ts | 12 +++++++++++- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/packages/backend-common/src/reading/GithubUrlReader.test.ts b/packages/backend-common/src/reading/GithubUrlReader.test.ts index 7552547924..e165745790 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.test.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.test.ts @@ -293,7 +293,7 @@ describe('GithubUrlReader', () => { ); mockfs(); - const directory = await response.dir('/tmp/fs'); + const directory = await response.dir({ targetDir: '/tmp/fs' }); const writtenToDirectory = fs.existsSync(directory); const paths = await recursive(directory); diff --git a/packages/backend-common/src/reading/GithubUrlReader.ts b/packages/backend-common/src/reading/GithubUrlReader.ts index 41db541f7f..e3a47ee420 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.ts @@ -18,7 +18,13 @@ import { Config } from '@backstage/config'; import parseGitUri from 'git-url-parse'; import fetch from 'cross-fetch'; import { NotFoundError } from '../errors'; -import { ReaderFactory, ReadTreeResponse, UrlReader, File } from './types'; +import { + ReaderFactory, + ReadTreeResponse, + UrlReader, + File, + ReadTreeResponseDirOptions, +} from './types'; import tar from 'tar'; import fs from 'fs-extra'; import concatStream from 'concat-stream'; @@ -294,9 +300,10 @@ export class GithubUrlReader implements UrlReader { resolve(Buffer.from('Archive is not yet implemented')), ); }, - dir: (outDir: string | undefined) => { + dir: (options?: ReadTreeResponseDirOptions) => { const targetDirectory = - outDir || fs.mkdtempSync(path.join(os.tmpdir(), 'backstage-')); + options?.targetDir || + fs.mkdtempSync(path.join(os.tmpdir(), 'backstage-')); return new Promise((res, rej) => { Promise.all( diff --git a/packages/backend-common/src/reading/types.ts b/packages/backend-common/src/reading/types.ts index d9fa5d839e..8a78e9d65f 100644 --- a/packages/backend-common/src/reading/types.ts +++ b/packages/backend-common/src/reading/types.ts @@ -17,6 +17,11 @@ import { Logger } from 'winston'; import { Config } from '@backstage/config'; +export type ReadTreeOptions = { + /** A filter that can be used to select which files should be extracted. By default all files are extracted */ + filter?(path: string): boolean; +}; + /** * A generic interface for fetching plain data from URLs. */ @@ -48,8 +53,13 @@ export type File = { content(): Promise; }; +export type ReadTreeResponseDirOptions = { + /** The directory to write files to. Defaults to the OS tmpdir or `backend.workingDirectory` if set in config */ + targetDir?: string; +}; + export type ReadTreeResponse = { files(): Promise; archive(): Promise; - dir(outDir?: string): Promise; + dir(options?: ReadTreeResponseDirOptions): Promise; }; From e1057d9a506d7763ffdad4d5aaf1c84c4c6414eb Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Nov 2020 12:33:58 +0100 Subject: [PATCH 5/9] backend-common: provide a ReadTreeResponseFactory instance to all ReaderFactories --- .../src/reading/AzureUrlReader.test.ts | 17 +++++++++++++++-- .../src/reading/GitlabUrlReader.test.ts | 17 +++++++++++++++-- .../backend-common/src/reading/UrlReaders.ts | 4 +++- packages/backend-common/src/reading/types.ts | 2 ++ 4 files changed, 35 insertions(+), 5 deletions(-) diff --git a/packages/backend-common/src/reading/AzureUrlReader.test.ts b/packages/backend-common/src/reading/AzureUrlReader.test.ts index 24b435006a..6efc7641b8 100644 --- a/packages/backend-common/src/reading/AzureUrlReader.test.ts +++ b/packages/backend-common/src/reading/AzureUrlReader.test.ts @@ -20,9 +20,14 @@ import { ConfigReader } from '@backstage/config'; import { getVoidLogger } from '../logging'; import { AzureUrlReader } from './AzureUrlReader'; import { msw } from '@backstage/test-utils'; +import { ReadTreeResponseFactory } from './tree'; const logger = getVoidLogger(); +const treeResponseFactory = ReadTreeResponseFactory.create({ + config: new ConfigReader({}), +}); + describe('AzureUrlReader', () => { const worker = setupServer(); msw.setupDefaultHandlers(worker); @@ -87,7 +92,11 @@ describe('AzureUrlReader', () => { }), }, ])('should handle happy path %#', async ({ url, config, response }) => { - const [{ reader }] = AzureUrlReader.factory({ config, logger }); + const [{ reader }] = AzureUrlReader.factory({ + config, + logger, + treeResponseFactory, + }); const data = await reader.read(url); const res = await JSON.parse(data.toString('utf-8')); @@ -115,7 +124,11 @@ describe('AzureUrlReader', () => { }, ])('should handle error path %#', async ({ url, config, error }) => { await expect(async () => { - const [{ reader }] = AzureUrlReader.factory({ config, logger }); + const [{ reader }] = AzureUrlReader.factory({ + config, + logger, + treeResponseFactory, + }); await reader.read(url); }).rejects.toThrow(error); }); diff --git a/packages/backend-common/src/reading/GitlabUrlReader.test.ts b/packages/backend-common/src/reading/GitlabUrlReader.test.ts index 4b3aa471c3..3a76c0631b 100644 --- a/packages/backend-common/src/reading/GitlabUrlReader.test.ts +++ b/packages/backend-common/src/reading/GitlabUrlReader.test.ts @@ -20,9 +20,14 @@ import { ConfigReader } from '@backstage/config'; import { getVoidLogger } from '../logging'; import { GitlabUrlReader } from './GitlabUrlReader'; import { msw } from '@backstage/test-utils'; +import { ReadTreeResponseFactory } from './tree'; const logger = getVoidLogger(); +const treeResponseFactory = ReadTreeResponseFactory.create({ + config: new ConfigReader({}), +}); + describe('GitlabUrlReader', () => { const worker = setupServer(); @@ -98,7 +103,11 @@ describe('GitlabUrlReader', () => { }), }, ])('should handle happy path %#', async ({ url, config, response }) => { - const [{ reader }] = GitlabUrlReader.factory({ config, logger }); + const [{ reader }] = GitlabUrlReader.factory({ + config, + logger, + treeResponseFactory, + }); const data = await reader.read(url); const res = await JSON.parse(data.toString('utf-8')); @@ -114,7 +123,11 @@ describe('GitlabUrlReader', () => { }, ])('should handle error path %#', async ({ url, config, error }) => { await expect(async () => { - const [{ reader }] = GitlabUrlReader.factory({ config, logger }); + const [{ reader }] = GitlabUrlReader.factory({ + config, + logger, + treeResponseFactory, + }); await reader.read(url); }).rejects.toThrow(error); }); diff --git a/packages/backend-common/src/reading/UrlReaders.ts b/packages/backend-common/src/reading/UrlReaders.ts index e1d99a2c49..2bb5617907 100644 --- a/packages/backend-common/src/reading/UrlReaders.ts +++ b/packages/backend-common/src/reading/UrlReaders.ts @@ -23,6 +23,7 @@ import { BitbucketUrlReader } from './BitbucketUrlReader'; import { GithubUrlReader } from './GithubUrlReader'; import { GitlabUrlReader } from './GitlabUrlReader'; import { FetchUrlReader } from './FetchUrlReader'; +import { ReadTreeResponseFactory } from './tree'; type CreateOptions = { /** Root config object */ @@ -49,9 +50,10 @@ export class UrlReaders { fallback, }: CreateOptions): UrlReader { const mux = new UrlReaderPredicateMux({ fallback: fallback }); + const treeResponseFactory = ReadTreeResponseFactory.create({ config }); for (const factory of factories ?? []) { - const tuples = factory({ config, logger: logger }); + const tuples = factory({ config, logger: logger, treeResponseFactory }); for (const tuple of tuples) { mux.register(tuple); diff --git a/packages/backend-common/src/reading/types.ts b/packages/backend-common/src/reading/types.ts index 8a78e9d65f..bf53e98ef2 100644 --- a/packages/backend-common/src/reading/types.ts +++ b/packages/backend-common/src/reading/types.ts @@ -16,6 +16,7 @@ import { Logger } from 'winston'; import { Config } from '@backstage/config'; +import { ReadTreeResponseFactory } from './tree'; export type ReadTreeOptions = { /** A filter that can be used to select which files should be extracted. By default all files are extracted */ @@ -46,6 +47,7 @@ export type UrlReaderPredicateTuple = { export type ReaderFactory = (options: { config: Config; logger: Logger; + treeResponseFactory: ReadTreeResponseFactory; }) => UrlReaderPredicateTuple[]; export type File = { From 6417d966bb4117093883f6f64d15d7c6f339ec08 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Nov 2020 13:53:22 +0100 Subject: [PATCH 6/9] backend-common,techdocs-backend: refactor .readTree to accept options and implement GitHub reader with response utils --- .../src/reading/GithubUrlReader.test.ts | 82 ++++++----- .../src/reading/GithubUrlReader.ts | 139 ++++++------------ .../src/reading/UrlReaderPredicateMux.ts | 14 +- .../src/reading/tree/ArchiveResponse.ts | 6 +- packages/backend-common/src/reading/types.ts | 6 +- plugins/techdocs-backend/src/helpers.test.ts | 54 +------ plugins/techdocs-backend/src/helpers.ts | 15 +- 7 files changed, 113 insertions(+), 203 deletions(-) diff --git a/packages/backend-common/src/reading/GithubUrlReader.test.ts b/packages/backend-common/src/reading/GithubUrlReader.test.ts index e165745790..658d908f84 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.test.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.test.ts @@ -29,8 +29,11 @@ import { } from './GithubUrlReader'; import fs from 'fs'; import path from 'path'; -import mockfs from 'mock-fs'; -import recursive from 'recursive-readdir'; +import { ReadTreeResponseFactory } from './tree'; + +const treeResponseFactory = ReadTreeResponseFactory.create({ + config: new ConfigReader({}), +}); describe('GithubUrlReader', () => { describe('getApiRequestOptions', () => { @@ -226,10 +229,13 @@ describe('GithubUrlReader', () => { describe('implementation', () => { it('rejects unknown targets', async () => { - const processor = new GithubUrlReader({ - host: 'github.com', - apiBaseUrl: 'https://api.github.com', - }); + const processor = new GithubUrlReader( + { + host: 'github.com', + apiBaseUrl: 'https://api.github.com', + }, + { treeResponseFactory }, + ); await expect( processor.read('https://not.github.com/apa'), ).rejects.toThrow( @@ -250,7 +256,7 @@ describe('GithubUrlReader', () => { beforeEach(() => { worker.use( rest.get( - 'https://github.com/spotify/mock/archive/repo.tar.gz', + 'https://github.com/backstage/mock/archive/repo.tar.gz', (_, res, ctx) => res( ctx.status(200), @@ -262,18 +268,20 @@ describe('GithubUrlReader', () => { }); it('returns the wanted files from an archive', async () => { - const processor = new GithubUrlReader({ - host: 'github.com', - }); + const processor = new GithubUrlReader( + { + host: 'github.com', + }, + { treeResponseFactory }, + ); const response = await processor.readTree( - 'https://github.com/spotify/mock', - 'repo', - ['mkdocs.yml', 'docs'], + 'https://github.com/backstage/mock/tree/repo', ); const files = await response.files(); + expect(files.length).toBe(2); const mkDocsFile = await files[0].content(); const indexMarkdownFile = await files[1].content(); @@ -281,33 +289,39 @@ describe('GithubUrlReader', () => { expect(indexMarkdownFile.toString()).toBe('# Test\n'); }); - it('returns a folder path from an archive', async () => { - const processor = new GithubUrlReader({ - host: 'github.com', - }); + it('must specify a branch', async () => { + const processor = new GithubUrlReader( + { + host: 'github.com', + }, + { treeResponseFactory }, + ); + + await expect( + processor.readTree('https://github.com/backstage/mock'), + ).rejects.toThrow( + 'GitHub URL must contain branch to be able to fetch tree', + ); + }); + + it('returns the wanted files from an archive with a subpath', async () => { + const processor = new GithubUrlReader( + { + host: 'github.com', + }, + { treeResponseFactory }, + ); const response = await processor.readTree( - 'https://github.com/spotify/mock', - 'repo', - ['mkdocs.yml', 'docs'], + 'https://github.com/backstage/mock/tree/repo/docs', ); - mockfs(); - const directory = await response.dir({ targetDir: '/tmp/fs' }); + const files = await response.files(); - const writtenToDirectory = fs.existsSync(directory); - const paths = await recursive(directory); - mockfs.restore(); + expect(files.length).toBe(1); + const indexMarkdownFile = await files[0].content(); - expect(writtenToDirectory).toBe(true); - expect(paths.sort()).toEqual( - [ - '/tmp/fs/mock-repo/docs/index.md', - '/tmp/fs/mock-repo/mkdocs.yml', - ].sort(), - ); - - worker.resetHandlers(); + expect(indexMarkdownFile.toString()).toBe('# Test\n'); }); }); }); diff --git a/packages/backend-common/src/reading/GithubUrlReader.ts b/packages/backend-common/src/reading/GithubUrlReader.ts index e3a47ee420..ca67a6cfcb 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.ts @@ -17,19 +17,15 @@ import { Config } from '@backstage/config'; import parseGitUri from 'git-url-parse'; import fetch from 'cross-fetch'; -import { NotFoundError } from '../errors'; +import { Readable } from 'stream'; +import { InputError, NotFoundError } from '../errors'; import { ReaderFactory, ReadTreeResponse, UrlReader, - File, - ReadTreeResponseDirOptions, + ReadTreeOptions, } from './types'; -import tar from 'tar'; -import fs from 'fs-extra'; -import concatStream from 'concat-stream'; -import path from 'path'; -import os from 'os'; +import { ReadTreeResponseFactory } from './tree'; /** * The configuration parameters for a single GitHub API provider. @@ -198,19 +194,18 @@ export function readConfig(config: Config): ProviderConfig[] { * the one exposed by GitHub itself. */ export class GithubUrlReader implements UrlReader { - private config: ProviderConfig; - - static factory: ReaderFactory = ({ config }) => { + static factory: ReaderFactory = ({ config, treeResponseFactory }) => { return readConfig(config).map(provider => { - const reader = new GithubUrlReader(provider); + const reader = new GithubUrlReader(provider, { treeResponseFactory }); const predicate = (url: URL) => url.host === provider.host; return { reader, predicate }; }); }; - constructor(config: ProviderConfig) { - this.config = config; - } + constructor( + private readonly config: ProviderConfig, + private readonly deps: { treeResponseFactory: ReadTreeResponseFactory }, + ) {} async read(url: string): Promise { const useApi = @@ -240,90 +235,48 @@ export class GithubUrlReader implements UrlReader { throw new Error(message); } - private async getRepositoryArchive( - repoUrl: string, - branchName: string, - ): Promise { - return fetch(new URL(`${repoUrl}/archive/${branchName}.tar.gz`).toString()); - } - - private async writeBufferToFile( - filePath: string, - content: Buffer, - ): Promise { - await fs.outputFile(filePath, content.toString()); - } - async readTree( - repoUrl: string, - branchName: string, - paths: Array, + url: string, + options?: ReadTreeOptions, ): Promise { - const { name: repoName } = parseGitUri(repoUrl); + const { + name: repoName, + ref, + protocol, + source, + full_name, + filepath, + } = parseGitUri(url); - const repoArchive = await this.getRepositoryArchive(repoUrl, branchName); + if (!ref) { + // TODO(Rugvip): We should add support for defaulting to the default branch + throw new InputError( + 'GitHub URL must contain branch to be able to fetch tree', + ); + } - const files: File[] = []; - return new Promise(resolve => { - const parser = new (tar.Parse as any)({ - filter: (path: string) => - !!paths.filter(file => { - return path.startsWith(`${repoName}-${branchName}/${file}`); - }).length, - onentry: (entry: tar.ReadEntry) => { - if (entry.type === 'Directory') { - entry.resume(); - return; - } + // TODO(Rugvip): use API to fetch URL instead + const response = await fetch( + new URL( + `${protocol}://${source}/${full_name}/archive/${ref}.tar.gz`, + ).toString(), + ); + if (!response.ok) { + const message = `Failed to read tree from ${url}, ${response.status} ${response.statusText}`; + if (response.status === 404) { + throw new NotFoundError(message); + } + throw new Error(message); + } - const contentPromise: Promise = new Promise(res => { - entry.pipe(concatStream(res)); - }); + const path = `${repoName}-${ref}/${filepath}`; - files.push({ - path: entry.path, - content: () => contentPromise, - }); - - entry.resume(); - }, - }); - - // @ts-ignore Typescript doesn't consider .pipe a method on ReadableStream. Don't know why. - repoArchive.body?.pipe(parser).on('finish', () => { - resolve({ - files: async () => { - return files; - }, - archive: () => { - return new Promise(resolve => - resolve(Buffer.from('Archive is not yet implemented')), - ); - }, - dir: (options?: ReadTreeResponseDirOptions) => { - const targetDirectory = - options?.targetDir || - fs.mkdtempSync(path.join(os.tmpdir(), 'backstage-')); - - return new Promise((res, rej) => { - Promise.all( - files.map(async file => { - return this.writeBufferToFile( - `${targetDirectory}/${file.path}`, - await file.content(), - ); - }), - ) - .then(() => { - res(`${targetDirectory}/${repoName}-${branchName}`); - }) - .catch(err => { - rej(err); - }); - }); - }, - }); - }); + return this.deps.treeResponseFactory.fromArchive({ + // TODO(Rugvip): Underlying implementation of fetch will be node-fetch, we probably want + // to stick to using that in exclusively backend code. + stream: (response.body as unknown) as Readable, + path, + filter: options?.filter, }); } diff --git a/packages/backend-common/src/reading/UrlReaderPredicateMux.ts b/packages/backend-common/src/reading/UrlReaderPredicateMux.ts index 74f94f0295..9b0566106a 100644 --- a/packages/backend-common/src/reading/UrlReaderPredicateMux.ts +++ b/packages/backend-common/src/reading/UrlReaderPredicateMux.ts @@ -14,7 +14,12 @@ * limitations under the License. */ -import { ReadTreeResponse, UrlReader, UrlReaderPredicateTuple } from './types'; +import { + ReadTreeOptions, + ReadTreeResponse, + UrlReader, + UrlReaderPredicateTuple, +} from './types'; type Options = { // UrlReader to fall back to if no other reader is matched @@ -55,14 +60,13 @@ export class UrlReaderPredicateMux implements UrlReader { readTree( repoUrl: string, - branchName: string, - paths: Array, + options?: ReadTreeOptions, ): Promise { const parsed = new URL(repoUrl); for (const { predicate, reader } of this.readers) { if (predicate(parsed)) { - if (reader.readTree) return reader.readTree(repoUrl, branchName, paths); + if (reader.readTree) return reader.readTree(repoUrl, options); throw new Error( `Trying to call readTree on UrlReader which does not support the feature.`, ); @@ -71,7 +75,7 @@ export class UrlReaderPredicateMux implements UrlReader { if (this.fallback) { if (this.fallback.readTree) - return this.fallback.readTree(repoUrl, branchName, paths); + return this.fallback.readTree(repoUrl, options); throw new Error( `Trying to call readTree on UrlReader which does not support the feature.`, ); diff --git a/packages/backend-common/src/reading/tree/ArchiveResponse.ts b/packages/backend-common/src/reading/tree/ArchiveResponse.ts index 83f6f6a8e5..0e06ff3804 100644 --- a/packages/backend-common/src/reading/tree/ArchiveResponse.ts +++ b/packages/backend-common/src/reading/tree/ArchiveResponse.ts @@ -42,10 +42,12 @@ export class ArchiveResponse implements ReadTreeResponse { ) { if (subPath) { if (!subPath.endsWith('/')) { - throw new TypeError('ArchiveResponse subPath must end with a /'); + this.subPath += '/'; } if (subPath.startsWith('/')) { - throw new TypeError('ArchiveResponse subPath must not start with a /'); + throw new TypeError( + `ArchiveResponse subPath must not start with a /, got '${subPath}'`, + ); } } } diff --git a/packages/backend-common/src/reading/types.ts b/packages/backend-common/src/reading/types.ts index bf53e98ef2..c7e5915401 100644 --- a/packages/backend-common/src/reading/types.ts +++ b/packages/backend-common/src/reading/types.ts @@ -28,11 +28,7 @@ export type ReadTreeOptions = { */ export type UrlReader = { read(url: string): Promise; - readTree?( - repoUrl: string, - branchName: string, - paths: Array, - ): Promise; + readTree?(url: string, options?: ReadTreeOptions): Promise; }; export type UrlReaderPredicateTuple = { diff --git a/plugins/techdocs-backend/src/helpers.test.ts b/plugins/techdocs-backend/src/helpers.test.ts index c3f2261637..5e98ab51c8 100644 --- a/plugins/techdocs-backend/src/helpers.test.ts +++ b/plugins/techdocs-backend/src/helpers.test.ts @@ -19,7 +19,7 @@ import { UrlReader, ReadTreeResponse } from '@backstage/backend-common'; import { Entity } from '@backstage/catalog-model'; describe('getDocFilesFromRepository', () => { - it('should take the directory from UrlReader.readTree and add the docs path when mkdocs.yml is in root', async () => { + it('should read a remote directory using UrlReader.readTree', async () => { class MockUrlReader implements UrlReader { async read() { return Buffer.from('mock'); @@ -45,7 +45,7 @@ describe('getDocFilesFromRepository', () => { namespace: 'default', annotations: { 'backstage.io/techdocs-ref': - 'url:https://github.com/backstage/backstage/blob/master/mkdocs.yml', + 'url:https://github.com/backstage/backstage/blob/master/subfolder/', }, name: 'mytestcomponent', description: 'A component for testing', @@ -64,54 +64,6 @@ describe('getDocFilesFromRepository', () => { mockEntity, ); - expect(output).toBe('/tmp/testfolder/.'); - }); - - it('should take the directory from UrlReader.readTree and add the docs path when mkdocs.yml is in a subfolder', async () => { - class MockUrlReader implements UrlReader { - async read() { - return Buffer.from('mock'); - } - - async readTree(): Promise { - return { - dir: async () => { - return '/tmp/testfolder'; - }, - files: async () => { - return []; - }, - archive: async () => { - return Buffer.from(''); - }, - }; - } - } - - const mockEntity: Entity = { - metadata: { - namespace: 'default', - annotations: { - 'backstage.io/techdocs-ref': - 'url:https://github.com/backstage/backstage/blob/master/subfolder/mkdocs.yml', - }, - name: 'mytestcomponent', - description: 'A component for testing', - }, - apiVersion: 'backstage.io/v1alpha1', - kind: 'Component', - spec: { - type: 'documentation', - lifecycle: 'experimental', - owner: 'testuser', - }, - }; - - const output = await getDocFilesFromRepository( - new MockUrlReader(), - mockEntity, - ); - - expect(output).toBe('/tmp/testfolder/subfolder'); + expect(output).toBe('/tmp/testfolder'); }); }); diff --git a/plugins/techdocs-backend/src/helpers.ts b/plugins/techdocs-backend/src/helpers.ts index f861d4375e..97b087558f 100644 --- a/plugins/techdocs-backend/src/helpers.ts +++ b/plugins/techdocs-backend/src/helpers.ts @@ -179,21 +179,10 @@ export const getDocFilesFromRepository = async ( entity, ); - const { ref, filepath: mkdocsPath } = parseGitUrl(target); - - const docsRootPath = path.dirname(mkdocsPath); - const docsFolderPath = path.join(docsRootPath, 'docs'); - if (reader.readTree) { - const readTreeResponse = await reader.readTree( - parseGitUrl(target).toString(), - ref, - [mkdocsPath, docsFolderPath], - ); + const response = await reader.readTree(target); - const tmpDir = await readTreeResponse.dir(); - - return `${tmpDir}/${docsRootPath}`; + return response.dir(); } throw new Error( From b672492e2164a58b0224cfbdbd26f3e29479d7d1 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Nov 2020 13:59:10 +0100 Subject: [PATCH 7/9] backend-common: make .readTree required --- .../backend-common/src/reading/AzureUrlReader.ts | 6 +++++- .../backend-common/src/reading/BitbucketUrlReader.ts | 6 +++++- .../backend-common/src/reading/FetchUrlReader.ts | 6 +++++- .../backend-common/src/reading/GitlabUrlReader.ts | 6 +++++- .../src/reading/UrlReaderPredicateMux.ts | 11 ++--------- packages/backend-common/src/reading/types.ts | 2 +- .../ingestion/processors/CodeOwnersProcessor.test.ts | 12 ++++++------ .../processors/PlaceholderProcessor.test.ts | 2 +- .../src/service/CatalogBuilder.test.ts | 5 ++++- plugins/techdocs-backend/src/helpers.ts | 10 ++-------- 10 files changed, 36 insertions(+), 30 deletions(-) diff --git a/packages/backend-common/src/reading/AzureUrlReader.ts b/packages/backend-common/src/reading/AzureUrlReader.ts index 8e6e6a75df..79058c79a9 100644 --- a/packages/backend-common/src/reading/AzureUrlReader.ts +++ b/packages/backend-common/src/reading/AzureUrlReader.ts @@ -17,7 +17,7 @@ import fetch from 'cross-fetch'; import { Config } from '@backstage/config'; import { NotFoundError } from '../errors'; -import { ReaderFactory, UrlReader } from './types'; +import { ReaderFactory, ReadTreeResponse, UrlReader } from './types'; type Options = { // TODO: added here for future support, but we only allow dev.azure.com for now @@ -86,6 +86,10 @@ export class AzureUrlReader implements UrlReader { throw new Error(message); } + readTree(): Promise { + throw new Error('AzureUrlReader does not implement readTree'); + } + // Converts // from: https://dev.azure.com/{organization}/{project}/_git/reponame?path={path}&version=GB{commitOrBranch}&_a=contents // to: https://dev.azure.com/{organization}/{project}/_apis/git/repositories/reponame/items?path={path}&version={commitOrBranch} diff --git a/packages/backend-common/src/reading/BitbucketUrlReader.ts b/packages/backend-common/src/reading/BitbucketUrlReader.ts index 348426fd34..b2fb9fa309 100644 --- a/packages/backend-common/src/reading/BitbucketUrlReader.ts +++ b/packages/backend-common/src/reading/BitbucketUrlReader.ts @@ -18,7 +18,7 @@ import { Config } from '@backstage/config'; import parseGitUri from 'git-url-parse'; import fetch from 'cross-fetch'; import { NotFoundError } from '../errors'; -import { ReaderFactory, UrlReader } from './types'; +import { ReaderFactory, ReadTreeResponse, UrlReader } from './types'; const DEFAULT_BASE_URL = 'https://api.bitbucket.org/2.0'; @@ -209,6 +209,10 @@ export class BitbucketUrlReader implements UrlReader { throw new Error(message); } + readTree(): Promise { + throw new Error('BitbucketUrlReader does not implement readTree'); + } + toString() { const { host, token, username, appPassword } = this.config; let authed = Boolean(token); diff --git a/packages/backend-common/src/reading/FetchUrlReader.ts b/packages/backend-common/src/reading/FetchUrlReader.ts index d2e5c45620..1d1784590c 100644 --- a/packages/backend-common/src/reading/FetchUrlReader.ts +++ b/packages/backend-common/src/reading/FetchUrlReader.ts @@ -16,7 +16,7 @@ import fetch from 'cross-fetch'; import { NotFoundError } from '../errors'; -import { UrlReader } from './types'; +import { ReadTreeResponse, UrlReader } from './types'; /** * A UrlReader that does a plain fetch of the URL. @@ -41,6 +41,10 @@ export class FetchUrlReader implements UrlReader { throw new Error(message); } + readTree(): Promise { + throw new Error('FetchUrlReader does not implement readTree'); + } + toString() { return 'fetch{}'; } diff --git a/packages/backend-common/src/reading/GitlabUrlReader.ts b/packages/backend-common/src/reading/GitlabUrlReader.ts index 378e76fb61..c621e41338 100644 --- a/packages/backend-common/src/reading/GitlabUrlReader.ts +++ b/packages/backend-common/src/reading/GitlabUrlReader.ts @@ -17,7 +17,7 @@ import fetch from 'cross-fetch'; import { Config } from '@backstage/config'; import { NotFoundError } from '../errors'; -import { ReaderFactory, UrlReader } from './types'; +import { ReaderFactory, ReadTreeResponse, UrlReader } from './types'; type Options = { host: string; @@ -87,6 +87,10 @@ export class GitlabUrlReader implements UrlReader { throw new Error(message); } + readTree(): Promise { + throw new Error('GitlabUrlReader does not implement readTree'); + } + // Converts // from: https://gitlab.example.com/a/b/blob/master/c.yaml // to: https://gitlab.example.com/a/b/raw/master/c.yaml diff --git a/packages/backend-common/src/reading/UrlReaderPredicateMux.ts b/packages/backend-common/src/reading/UrlReaderPredicateMux.ts index 9b0566106a..9a81fdc9ab 100644 --- a/packages/backend-common/src/reading/UrlReaderPredicateMux.ts +++ b/packages/backend-common/src/reading/UrlReaderPredicateMux.ts @@ -66,19 +66,12 @@ export class UrlReaderPredicateMux implements UrlReader { for (const { predicate, reader } of this.readers) { if (predicate(parsed)) { - if (reader.readTree) return reader.readTree(repoUrl, options); - throw new Error( - `Trying to call readTree on UrlReader which does not support the feature.`, - ); + return reader.readTree(repoUrl, options); } } if (this.fallback) { - if (this.fallback.readTree) - return this.fallback.readTree(repoUrl, options); - throw new Error( - `Trying to call readTree on UrlReader which does not support the feature.`, - ); + return this.fallback.readTree(repoUrl, options); } throw new Error(`No reader found that could handle '${repoUrl}'`); diff --git a/packages/backend-common/src/reading/types.ts b/packages/backend-common/src/reading/types.ts index c7e5915401..101502fed7 100644 --- a/packages/backend-common/src/reading/types.ts +++ b/packages/backend-common/src/reading/types.ts @@ -28,7 +28,7 @@ export type ReadTreeOptions = { */ export type UrlReader = { read(url: string): Promise; - readTree?(url: string, options?: ReadTreeOptions): Promise; + readTree(url: string, options?: ReadTreeOptions): Promise; }; export type UrlReaderPredicateTuple = { diff --git a/plugins/catalog-backend/src/ingestion/processors/CodeOwnersProcessor.test.ts b/plugins/catalog-backend/src/ingestion/processors/CodeOwnersProcessor.test.ts index e4ddb249d6..dfb470fa21 100644 --- a/plugins/catalog-backend/src/ingestion/processors/CodeOwnersProcessor.test.ts +++ b/plugins/catalog-backend/src/ingestion/processors/CodeOwnersProcessor.test.ts @@ -157,14 +157,14 @@ describe('CodeOwnersProcessor', () => { const read = jest .fn() .mockResolvedValue(mockReadResult({ data: ownersText })); - const reader = { read }; + const reader = { read, readTree: jest.fn() }; const result = await findRawCodeOwners(mockLocation(), reader); expect(result).toEqual(ownersText); }); it('should raise error when no codeowner', async () => { const read = jest.fn().mockRejectedValue(mockReadResult()); - const reader = { read }; + const reader = { read, readTree: jest.fn() }; await expect( findRawCodeOwners(mockLocation(), reader), @@ -178,7 +178,7 @@ describe('CodeOwnersProcessor', () => { .mockImplementationOnce(() => mockReadResult({ error: 'foo' })) .mockImplementationOnce(() => mockReadResult({ error: 'bar' })) .mockResolvedValue(mockReadResult({ data: ownersText })); - const reader = { read }; + const reader = { read, readTree: jest.fn() }; const result = await findRawCodeOwners(mockLocation(), reader); @@ -197,7 +197,7 @@ describe('CodeOwnersProcessor', () => { const read = jest .fn() .mockResolvedValue(mockReadResult({ data: mockCodeOwnersText() })); - const reader = { read }; + const reader = { read, readTree: jest.fn() }; const owner = await resolveCodeOwner(mockLocation(), reader); expect(owner).toBe('backstage-core'); @@ -207,7 +207,7 @@ describe('CodeOwnersProcessor', () => { const read = jest .fn() .mockImplementation(() => mockReadResult({ error: 'error: foo' })); - const reader = { read }; + const reader = { read, readTree: jest.fn() }; await expect( resolveCodeOwner(mockLocation(), reader), @@ -221,7 +221,7 @@ describe('CodeOwnersProcessor', () => { const read = jest .fn() .mockResolvedValue(mockReadResult({ data: mockCodeOwnersText() })); - const reader = { read }; + const reader = { read, readTree: jest.fn() }; const processor = new CodeOwnersProcessor({ reader }); return { entity, processor, read }; diff --git a/plugins/catalog-backend/src/ingestion/processors/PlaceholderProcessor.test.ts b/plugins/catalog-backend/src/ingestion/processors/PlaceholderProcessor.test.ts index 48cd2dae01..5a3511af6d 100644 --- a/plugins/catalog-backend/src/ingestion/processors/PlaceholderProcessor.test.ts +++ b/plugins/catalog-backend/src/ingestion/processors/PlaceholderProcessor.test.ts @@ -27,7 +27,7 @@ import { describe('PlaceholderProcessor', () => { const read: jest.MockedFunction = jest.fn(); - const reader: UrlReader = { read }; + const reader: UrlReader = { read, readTree: jest.fn() }; beforeEach(() => { jest.resetAllMocks(); diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.test.ts b/plugins/catalog-backend/src/service/CatalogBuilder.test.ts index 45d92e6555..ee7c6f2246 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.test.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.test.ts @@ -40,7 +40,10 @@ const dummyEntityYaml = yaml.stringify(dummyEntity); describe('CatalogBuilder', () => { let db: Knex; - const reader: jest.Mocked = { read: jest.fn() }; + const reader: jest.Mocked = { + read: jest.fn(), + readTree: jest.fn(), + }; const env: CatalogEnvironment = { logger: getVoidLogger(), database: { getClient: async () => db }, diff --git a/plugins/techdocs-backend/src/helpers.ts b/plugins/techdocs-backend/src/helpers.ts index 97b087558f..901aea533a 100644 --- a/plugins/techdocs-backend/src/helpers.ts +++ b/plugins/techdocs-backend/src/helpers.ts @@ -179,13 +179,7 @@ export const getDocFilesFromRepository = async ( entity, ); - if (reader.readTree) { - const response = await reader.readTree(target); + const response = await reader.readTree(target); - return response.dir(); - } - - throw new Error( - `No readTree method available on the UrlReader for ${target}`, - ); + return await response.dir(); }; From 8e2effb531a862c0415c6adf4dfb352421ccc07c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Nov 2020 14:01:32 +0100 Subject: [PATCH 8/9] changesets: added changeset for UrlReader readTree refactoring --- .changeset/odd-camels-begin.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/odd-camels-begin.md diff --git a/.changeset/odd-camels-begin.md b/.changeset/odd-camels-begin.md new file mode 100644 index 0000000000..d28d729609 --- /dev/null +++ b/.changeset/odd-camels-begin.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': minor +--- + +Refactored UrlReader.readTree to be required and accept (url, options) From bd1cef1847bb2f2c75d7eda147a68a868f06b31e Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 13 Nov 2020 00:43:20 +0100 Subject: [PATCH 9/9] backend-common: review comments for readTree + switch archive return type to a stream --- .../src/reading/UrlReaderPredicateMux.ts | 13 ++++------ .../src/reading/tree/ArchiveResponse.test.ts | 3 +-- .../src/reading/tree/ArchiveResponse.ts | 26 +++++++++---------- .../reading/tree/ReadTreeResponseFactory.ts | 2 +- packages/backend-common/src/reading/types.ts | 19 +++++++++++--- plugins/techdocs-backend/src/helpers.test.ts | 3 ++- 6 files changed, 37 insertions(+), 29 deletions(-) diff --git a/packages/backend-common/src/reading/UrlReaderPredicateMux.ts b/packages/backend-common/src/reading/UrlReaderPredicateMux.ts index 9a81fdc9ab..465c125fda 100644 --- a/packages/backend-common/src/reading/UrlReaderPredicateMux.ts +++ b/packages/backend-common/src/reading/UrlReaderPredicateMux.ts @@ -58,23 +58,20 @@ export class UrlReaderPredicateMux implements UrlReader { throw new Error(`No reader found that could handle '${url}'`); } - readTree( - repoUrl: string, - options?: ReadTreeOptions, - ): Promise { - const parsed = new URL(repoUrl); + readTree(url: string, options?: ReadTreeOptions): Promise { + const parsed = new URL(url); for (const { predicate, reader } of this.readers) { if (predicate(parsed)) { - return reader.readTree(repoUrl, options); + return reader.readTree(url, options); } } if (this.fallback) { - return this.fallback.readTree(repoUrl, options); + return this.fallback.readTree(url, options); } - throw new Error(`No reader found that could handle '${repoUrl}'`); + throw new Error(`No reader found that could handle '${url}'`); } toString() { diff --git a/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts b/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts index 6b8f29de8c..fd351863b2 100644 --- a/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts +++ b/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts @@ -16,7 +16,6 @@ import fs from 'fs-extra'; import mockFs from 'mock-fs'; -import { Readable } from 'stream'; import { resolve as resolvePath } from 'path'; import { ArchiveResponse } from './ArchiveResponse'; @@ -87,7 +86,7 @@ describe('ArchiveResponse', () => { 'Response has already been read', ); - const res2 = new ArchiveResponse(Readable.from(buffer), '', '/tmp'); + const res2 = new ArchiveResponse(buffer, '', '/tmp'); const files = await res2.files(); expect(files).toEqual([ diff --git a/packages/backend-common/src/reading/tree/ArchiveResponse.ts b/packages/backend-common/src/reading/tree/ArchiveResponse.ts index 0e06ff3804..e16be63127 100644 --- a/packages/backend-common/src/reading/tree/ArchiveResponse.ts +++ b/packages/backend-common/src/reading/tree/ArchiveResponse.ts @@ -14,14 +14,17 @@ * limitations under the License. */ -import os from 'os'; import tar, { Parse, ParseStream, ReadEntry } from 'tar'; import path from 'path'; import fs from 'fs-extra'; import { Readable, pipeline as pipelineCb } from 'stream'; import { promisify } from 'util'; import concatStream from 'concat-stream'; -import { ReadTreeResponse, File, ReadTreeResponseDirOptions } from '../types'; +import { + ReadTreeResponse, + ReadTreeResponseFile, + ReadTreeResponseDirOptions, +} from '../types'; // Tar types for `Parse` is not a proper constructor, but it should be const TarParseStream = (Parse as unknown) as { new (): ParseStream }; @@ -37,7 +40,7 @@ export class ArchiveResponse implements ReadTreeResponse { constructor( private readonly stream: Readable, private readonly subPath: string, - private readonly workDir: string = os.tmpdir(), + private readonly workDir: string, private readonly filter?: (path: string) => boolean, ) { if (subPath) { @@ -60,10 +63,10 @@ export class ArchiveResponse implements ReadTreeResponse { this.read = true; } - async files(): Promise { + async files(): Promise { this.onlyOnce(); - const files: File[] = []; + const files = Array(); const parser = new TarParseStream(); parser.on('entry', (entry: ReadEntry & Readable) => { @@ -79,9 +82,7 @@ export class ArchiveResponse implements ReadTreeResponse { } } - const path = this.subPath - ? entry.path.replace(this.subPath, '') - : entry.path; + const path = entry.path.slice(this.subPath.length); if (this.filter) { if (!this.filter(path)) { entry.resume(); @@ -103,13 +104,11 @@ export class ArchiveResponse implements ReadTreeResponse { return files; } - async archive(): Promise { + async archive(): Promise { if (!this.subPath) { this.onlyOnce(); - return new Promise(resolve => - pipeline(this.stream, concatStream(resolve)), - ); + return this.stream; } // TODO(Rugvip): method for repacking a tar with a subpath is to simply extract into a @@ -117,12 +116,13 @@ export class ArchiveResponse implements ReadTreeResponse { const tmpDir = await this.dir(); try { - return await new Promise(async resolve => { + const data = await new Promise(async resolve => { await pipeline( tar.create({ cwd: tmpDir }, ['']), concatStream(resolve), ); }); + return Readable.from(data); } finally { await fs.remove(tmpDir); } diff --git a/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts b/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts index 5044cdabdc..a134d185f3 100644 --- a/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts +++ b/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts @@ -23,7 +23,7 @@ import { ArchiveResponse } from './ArchiveResponse'; type FromArchiveOptions = { // A binary stream of a tar archive. stream: Readable; - // If set, root of the tree will be set to the given path. Should not have a trailing `/`. + // If set, the root of the tree will be set to the given directory path. path?: string; // Filter passed on from the ReadTreeOptions filter?: (path: string) => boolean; diff --git a/packages/backend-common/src/reading/types.ts b/packages/backend-common/src/reading/types.ts index 101502fed7..f9dca3e1d5 100644 --- a/packages/backend-common/src/reading/types.ts +++ b/packages/backend-common/src/reading/types.ts @@ -19,7 +19,18 @@ import { Config } from '@backstage/config'; import { ReadTreeResponseFactory } from './tree'; export type ReadTreeOptions = { - /** A filter that can be used to select which files should be extracted. By default all files are extracted */ + /** + * A filter that can be used to select which files should be included. + * + * The path passed to the filter function is the relative path from the URL + * that the file tree is fetched from, without any leading '/'. + * + * For example, given the URL https://github.com/my/repo/tree/master/my-dir, a file + * at https://github.com/my/repo/blob/master/my-dir/my-subdir/my-file.txt will + * be represented as my-subdir/my-file.txt + * + * If no filter is provided all files are extracted. + */ filter?(path: string): boolean; }; @@ -46,7 +57,7 @@ export type ReaderFactory = (options: { treeResponseFactory: ReadTreeResponseFactory; }) => UrlReaderPredicateTuple[]; -export type File = { +export type ReadTreeResponseFile = { path: string; content(): Promise; }; @@ -57,7 +68,7 @@ export type ReadTreeResponseDirOptions = { }; export type ReadTreeResponse = { - files(): Promise; - archive(): Promise; + files(): Promise; + archive(): Promise; dir(options?: ReadTreeResponseDirOptions): Promise; }; diff --git a/plugins/techdocs-backend/src/helpers.test.ts b/plugins/techdocs-backend/src/helpers.test.ts index 5e98ab51c8..10518df74f 100644 --- a/plugins/techdocs-backend/src/helpers.test.ts +++ b/plugins/techdocs-backend/src/helpers.test.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { Readable } from 'stream'; import { getDocFilesFromRepository } from './helpers'; import { UrlReader, ReadTreeResponse } from '@backstage/backend-common'; import { Entity } from '@backstage/catalog-model'; @@ -34,7 +35,7 @@ describe('getDocFilesFromRepository', () => { return []; }, archive: async () => { - return Buffer.from(''); + return Readable.from(''); }, }; }