Merge pull request #5 from Polymer/github-filter-bad-references

filter out pattern matches missing the #ref
Polymer · Oct 5, 2017 · 5d4c7ae · 5d4c7ae
2 parents 37043f4 + befd8b3
commit 5d4c7ae
Show file tree

Hide file tree

Showing 5 changed files with 119 additions and 67 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@
 <!-- Add new, unreleased items here. -->
 - Fix gitRepo.fetch() for when no branchName is given
 - Fix gitRepo.getHeadSha() & add tests
+- When expanding repo patterns, Filter out repos that don't match the reference branch
 
 ## v1.0.1 [10-05-2017]
 - Update package.json description

diff --git a/src/github.ts b/src/github.ts
@@ -47,8 +47,19 @@ export interface GitHubRepo extends GitHubRepoData { ref?: string; }
 /**
  * Returns true if the response is a redirect to another repo
  */
-function isRedirect(response: any): boolean {
-  return !!(response.meta && response.meta.status.match(/^301\b/));
+function isSuccessResponse(response: any): boolean {
+  return !!(
+      response.meta && response.meta.status &&
+      response.meta.status.match(/^200\b/));
+}
+
+/**
+ * Returns true if the response is a redirect to another repo
+ */
+function isRedirectResponse(response: any): boolean {
+  return !!(
+      response.meta && response.meta.status &&
+      response.meta.status.match(/^301\b/));
 }
 
 /**
@@ -158,7 +169,7 @@ export class GitHubConnection {
     // details in error messaging.  This was encountered because we
     // tried to request Polymer/hydrolysis which has been renamed to
     // Polymer/polymer-analyzer and the API doesn't auto-follow this.
-    if (isRedirect(response)) {
+    if (isRedirectResponse(response)) {
       console.log('Repo ${owner}/${repo} has moved permanently.');
       console.log(response);
       throw new Error(`Repo ${repoReference.owner}/${
@@ -258,14 +269,38 @@ export class GitHubConnection {
             pattern.substring(0, pattern.indexOf('*')).toLowerCase();
         const ref = pattern.includes('#') &&
             pattern.substring(pattern.indexOf('#') + 1);
-        (await this.getOwnerRepos(owner))
-            .filter((cachedRepo) => {
-              return cachedRepo.fullName.toLowerCase().startsWith(namePattern);
-            })
-            .forEach((cachedRepo) => {
-              allGitHubRepos.push(createGitHubRepoReferenceFromDataAndReference(
-                  cachedRepo, ref || cachedRepo.defaultBranch));
-            });
+        const allOwnerRepos = await this.getOwnerRepos(owner);
+        // Filter all of this owner's repos for possible matches:
+        for (const possibleMatch of allOwnerRepos) {
+          // If the repo's fullName doesn't match the pattern prefix, ignore it.
+          if (!possibleMatch.fullName.toLowerCase().startsWith(namePattern)) {
+            continue;
+          }
+          // If a branch was defined after the wildcard but this repo doesn't
+          // have a ref with that name, ignore it.
+          if (ref && ref !== possibleMatch.defaultBranch) {
+            try {
+              const response = await this._github.gitdata.getReference({
+                owner: possibleMatch.owner,
+                repo: possibleMatch.name,
+                ref: 'heads/' + ref,
+              });
+              // GitHub API peculiarity: if ref isn't an exact match, GitHub
+              // switches behavior and returns all references that have `ref` as
+              // a prefix. Since we only want exact matches, add an extra check
+              // that the API did not return an array.
+              if (!isSuccessResponse(response) ||
+                  Array.isArray(response.data)) {
+                continue;
+              }
+            } catch (err) {
+              continue;
+            }
+          }
+          // Otherwise, it's a match! Add it to allGitHubRepos to be returned.
+          allGitHubRepos.push(createGitHubRepoReferenceFromDataAndReference(
+              possibleMatch, ref || possibleMatch.defaultBranch));
+        }
       })();
     }));
 

diff --git a/src/test/_util/mock-api.ts b/src/test/_util/mock-api.ts
@@ -65,9 +65,27 @@ export function setup() {
 
   nock('https://api.github.com')
       .persist()
-      .get('/orgs/polymerelements/repos')
-      .query({page: 1, per_page: 50, access_token: testApiToken})
-      .reply(200, [], {Status: '200 OK'});
+      .get('/repos/PolymerElements/paper-appbar/git/refs/heads/ABCDEFGH')
+      .query({access_token: testApiToken})
+      .reply(200, {ref: 'refs/heads/ABCDEFGH'}, {Status: '200 OK'});
+
+  nock('https://api.github.com')
+      .persist()
+      .get('/repos/PolymerElements/paper-button/git/refs/heads/ABCDEFGH')
+      .query({access_token: testApiToken})
+      .reply(
+          200,
+          [
+            {ref: 'refs/heads/ABCDEFGH-MATCH-1'},
+            {ref: 'refs/heads/ABCDEFGH-MATCH-2'}
+          ],
+          {Status: '200 OK'});
+
+  nock('https://api.github.com')
+      .persist()
+      .get('/repos/PolymerElements/iron-ajax/git/refs/heads/ABCDEFGH')
+      .query({access_token: testApiToken})
+      .reply(200, '', {Status: '404 NOT FOUND'});
 }
 
 export function teardown() {

diff --git a/src/test/github_test.ts b/src/test/github_test.ts
@@ -88,64 +88,61 @@ suite('src/github', () => {
       test('handles dynamic owner/*#ref pattern', async () => {
         const references = await githubConnection.expandRepoPatterns(
             ['polymerelements/*#ABCDEFGH']);
-        assert.deepEqual(references, [
-          {
-            owner: 'PolymerElements',
-            name: 'paper-appbar',
-            fullName: 'PolymerElements/paper-appbar',
-            ref: 'ABCDEFGH'
-          },
-          {
-            owner: 'PolymerElements',
-            name: 'paper-button',
-            fullName: 'PolymerElements/paper-button',
-            ref: 'ABCDEFGH'
-          },
-          {
-            owner: 'PolymerElements',
-            name: 'iron-ajax',
-            fullName: 'PolymerElements/iron-ajax',
-            ref: 'ABCDEFGH'
-          }
-        ]);
+        assert.deepEqual(references, [{
+                           owner: 'PolymerElements',
+                           name: 'paper-appbar',
+                           fullName: 'PolymerElements/paper-appbar',
+                           ref: 'ABCDEFGH'
+                         }]);
       });
 
       test('handles dynamic owner/partial-name-* pattern', async () => {
-        const references = await githubConnection.expandRepoPatterns(
-            ['polymerelements/paper-*']);
-        assert.deepEqual(references, [
-          {
-            owner: 'PolymerElements',
-            name: 'paper-appbar',
-            fullName: 'PolymerElements/paper-appbar',
-            ref: undefined
-          },
-          {
-            owner: 'PolymerElements',
-            name: 'paper-button',
-            fullName: 'PolymerElements/paper-button',
-            ref: undefined
-          },
-        ]);
+        assert.deepEqual(
+            await githubConnection.expandRepoPatterns(
+                ['polymerelements/paper-*']),
+            [
+              {
+                owner: 'PolymerElements',
+                name: 'paper-appbar',
+                fullName: 'PolymerElements/paper-appbar',
+                ref: undefined
+              },
+              {
+                owner: 'PolymerElements',
+                name: 'paper-button',
+                fullName: 'PolymerElements/paper-button',
+                ref: undefined
+              },
+            ]);
+        assert.deepEqual(
+            await githubConnection.expandRepoPatterns(
+                ['polymerelements/iron-*']),
+            [
+              {
+                owner: 'PolymerElements',
+                name: 'iron-ajax',
+                fullName: 'PolymerElements/iron-ajax',
+                ref: undefined
+              },
+            ]);
       });
 
       test('handles dynamic owner/partial-name-*#ref pattern', async () => {
-        const references = await githubConnection.expandRepoPatterns(
-            ['polymerelements/paper-*#ABCDEFGH']);
-        assert.deepEqual(references, [
-          {
-            owner: 'PolymerElements',
-            name: 'paper-appbar',
-            fullName: 'PolymerElements/paper-appbar',
-            ref: 'ABCDEFGH'
-          },
-          {
-            owner: 'PolymerElements',
-            name: 'paper-button',
-            fullName: 'PolymerElements/paper-button',
-            ref: 'ABCDEFGH'
-          },
-        ]);
+        assert.deepEqual(
+            await githubConnection.expandRepoPatterns(
+                ['polymerelements/paper-*#ABCDEFGH']),
+            [
+              {
+                owner: 'PolymerElements',
+                name: 'paper-appbar',
+                fullName: 'PolymerElements/paper-appbar',
+                ref: 'ABCDEFGH'
+              },
+            ]);
+        assert.deepEqual(
+            await githubConnection.expandRepoPatterns(
+                ['polymerelements/iron-*#ABCDEFGH']),
+            []);
       });
 
     });

diff --git a/src/util/bower.ts b/src/util/bower.ts
@@ -12,10 +12,11 @@
  * http://polymer.github.io/PATENTS.txt
  */
 
-import {existsSync} from './fs';
 import * as fs from 'fs';
 import * as path from 'path';
 
+import {existsSync} from './fs';
+
 /**
  * @returns a dictionary object of dev dependencies from the bower.json
  * entries in all workspace repos that are marked for test, suitable for