Improve handling of new and removed OWNERS files in cache

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');
   }

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()));

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();

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'

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');