diff --git a/owners/src/api/github.js b/owners/src/api/github.js index b629fe238..0915c881e 100644 --- a/owners/src/api/github.js +++ b/owners/src/api/github.js @@ -412,18 +412,18 @@ class GitHub { /** * Fetches the contents of a file from GitHub. * - * @param {!FileRef} file file ref to fetch. + * @param {string} file file ref to fetch. * @return {string} file contents as a string. */ - async getFileContents(file) { - this.logger.info( - `Fetching contents of file ${file.filename} at ref ${file.sha}` - ); + async getFileContents(filename) { + this.logger.info(`Fetching contents of file ${filename}`); - const response = await this.client.git.getBlob( - this.repo({'file_sha': file.sha}) - ); - this.logger.debug('[getFileContents]', file, response.data); + const response = await this.client.repos.getContent({ + owner: this.owner, + repo: this.repository, + path: filename, + }); + this.logger.debug('[getFileContents]', filename, response.data); return Buffer.from(response.data.content, 'base64').toString('utf8'); } diff --git a/owners/src/ownership/parser.js b/owners/src/ownership/parser.js index 68f7255dd..2e2e73f6d 100644 --- a/owners/src/ownership/parser.js +++ b/owners/src/ownership/parser.js @@ -276,10 +276,10 @@ class OwnersParser { */ async parseOwnersFile(ownersPath) { const errors = []; - const contents = await this.repo.readFile(ownersPath); let file; try { + const contents = await this.repo.readFile(ownersPath); file = JSON5.parse(contents); } catch (error) { errors.push(new OwnersParserError(ownersPath, error.toString())); diff --git a/owners/src/repo/virtual_repo.js b/owners/src/repo/virtual_repo.js index 599d6bdab..24a3fbfc0 100644 --- a/owners/src/repo/virtual_repo.js +++ b/owners/src/repo/virtual_repo.js @@ -40,11 +40,13 @@ module.exports = class VirtualRepository extends Repository { */ async sync() { let fileChanged = false; + const ownersFilesKnown = new Set(this._fileRefs.keys()); const ownersFiles = await this.github.searchFilename('OWNERS'); for (const {filename, sha} of ownersFiles) { const repoPath = this.repoPath(filename); const fileSha = this._fileRefs.get(repoPath); + ownersFilesKnown.delete(repoPath); if (!fileSha) { // File has never been fetched and should be added to the cache. @@ -66,6 +68,20 @@ module.exports = class VirtualRepository extends Repository { } } + // Force-invalidate any files that we know should exist but didn't come back + // in the GitHub search results. + for (const filename of ownersFilesKnown) { + const repoPath = this.repoPath(filename); + // File has been updated and needs to be re-fetched. + this.logger.info( + `Updating SHA and clearing cache for file "${repoPath}"` + ); + fileChanged = true; + + this._fileRefs.set(repoPath, 'UNKNOWN'); + await this.cache.invalidate(repoPath); + } + if (fileChanged) { const fileRefsPath = this.repoPath('__fileRefs__'); await this.cache.invalidate(fileRefsPath); @@ -126,10 +142,19 @@ module.exports = class VirtualRepository extends Repository { } return await this.cache.readFile(repoPath, async () => { - const contents = await this.github.getFileContents({ - filename: relativePath, - sha: fileSha, - }); + let contents = ''; + try { + contents = await this.github.getFileContents(relativePath); + // If fetching a file that didn't come back in search results, refresh its + // recorded SHA. + if (fileSha == 'UNKNOWN') { + this._fileRefs.set(repoPath, contents.data.sha); + } + } catch (e) { + // If the file no longer exists, remove its cache entry. + this._fileRefs.delete(repoPath); + throw e; + } if (cacheMissCallback) { await cacheMissCallback(); diff --git a/owners/test/api/github.test.js b/owners/test/api/github.test.js index dfae75c17..72b60e357 100644 --- a/owners/test/api/github.test.js +++ b/owners/test/api/github.test.js @@ -519,13 +519,12 @@ describe('GitHub API', () => { expect.assertions(1); nock('https://api.github.com') .get( - '/repos/test_owner/test_repo/git/blobs/eeae1593f4ecbae3f4453c9ceee2940a0e98ddca' + '/repos/test_owner/test_repo/contents/third_party%2Fsubscriptions-project%2FOWNERS' ) .reply(200, getFileResponse); - const contents = await github.getFileContents({ - filename: 'third_party/subscriptions-project/OWNERS', - sha: 'eeae1593f4ecbae3f4453c9ceee2940a0e98ddca', - }); + const contents = await github.getFileContents( + 'third_party/subscriptions-project/OWNERS' + ); expect(contents).toEqual( '- otherperson\n- auser\n- otheruser\n- programmer\n- someperson\n' diff --git a/owners/test/repo/virtual_repo.test.js b/owners/test/repo/virtual_repo.test.js index 437b6ca0c..47dac2c00 100644 --- a/owners/test/repo/virtual_repo.test.js +++ b/owners/test/repo/virtual_repo.test.js @@ -158,14 +158,11 @@ describe('virtual repository', () => { it('fetches file refs from GitHub', async () => { await repo.warmCache(); - sandbox.assert.calledWith(github.getFileContents.getCall(0), { - filename: 'OWNERS', - sha: 'sha_1', - }); - sandbox.assert.calledWith(github.getFileContents.getCall(1), { - filename: 'foo/OWNERS', - sha: 'sha_2', - }); + sandbox.assert.calledWith(github.getFileContents.getCall(0), 'OWNERS'); + sandbox.assert.calledWith( + github.getFileContents.getCall(1), + 'foo/OWNERS' + ); }); describe('when file refs are in the cache', () => { @@ -239,10 +236,7 @@ describe('virtual repository', () => { await repo.sync(); const contents = await repo.readFile('OWNERS'); - sandbox.assert.calledWith(github.getFileContents, { - filename: 'OWNERS', - sha: 'sha_1', - }); + sandbox.assert.calledWith(github.getFileContents, 'OWNERS'); expect(contents).toEqual('contents'); }); @@ -264,10 +258,7 @@ describe('virtual repository', () => { await repo.sync(); await repo.readFile('OWNERS'); - sandbox.assert.calledWith(github.getFileContents, { - filename: 'OWNERS', - sha: 'sha_1', - }); + sandbox.assert.calledWith(github.getFileContents, 'OWNERS'); await repo.cache.invalidate('test_repo/OWNERS'); const contents = await repo.readFile('OWNERS');