fix: prevent duplicate and self-refs (#478)

Author: codebytere

lib/links.js

@@ -32,27 +32,27 @@ class LinkParser {
   }
 
   getRefsUrlsFromArray(arr) {
-    const result = [];
+    const result = new Set();
     for (const item of arr) {
       const m = item.match(REF_RE);
       if (!m) continue;
       const ref = m[1];
       const url = this.getUrlFromOP(ref);
-      if (url) result.push(url);
+      if (url) result.add(url);
     }
-    return result;
+    return Array.from(result);
   }
 
   getPRUrlsFromArray(arr) {
-    const result = [];
+    const result = new Set();
     for (const item of arr) {
       const m = item.match(PR_RE);
       if (!m) continue;
       const prUrl = m[1];
       const url = this.getUrlFromOP(prUrl);
-      if (url) result.push(url);
+      if (url) result.add(url);
     }
-    return result;
+    return Array.from(result);
   }
 
   // Do this so we can reliably get the correct url.

lib/metadata_gen.js

@@ -32,7 +32,7 @@ class MetadataGenerator {
 
     const parser = new LinkParser(owner, repo, op);
     const fixes = parser.getFixes();
-    const refs = parser.getRefs();
+    const refs = parser.getRefs().filter(f => f !== prUrl);
     const altPrUrl = parser.getAltPrUrl();
 
     const meta = [];

test/fixtures/data.js

@@ -77,6 +77,8 @@ const firstTimerPrivatePR = readJSON('first_timer_pr_with_private_email.json');
 const semverMajorPR = readJSON('semver_major_pr.json');
 const fixAndRefPR = readJSON('pr_with_fixes_and_refs.json');
 const fixCrossPR = readJSON('pr_with_fixes_cross.json');
+const duplicateRefPR = readJSON('pr_with_duplicate_refs.json');
+const selfRefPR = readJSON('pr_with_self_ref.json');
 const backportPR = readJSON('pr_with_backport.json');
 const conflictingPR = readJSON('conflicting_pr.json');
 const emptyProfilePR = readJSON('empty_profile_pr.json');
@@ -137,5 +139,7 @@ module.exports = {
   readmeNoCollaboratorE,
   readmeUnordered,
   closedPR,
-  mergedPR
+  mergedPR,
+  selfRefPR,
+  duplicateRefPR
 };

test/fixtures/pr_with_duplicate_refs.json

@@ -0,0 +1,20 @@
+{
+  "createdAt": "2017-10-24T11:13:43Z",
+  "url": "https://github.com/nodejs/node/pull/16438",
+  "bodyHTML":
+    "<p>Refs: <a href=\"https://github.com/nodejs/node/pull/16439\" class=\"issue-link js-issue-link\" data-error-text=\"Failed to load issue title\" data-id=\"271243248\" data-permission-text=\"Issue title is private\" data-url=\"https://github.com/nodejs/node/pull/16439\">#16439</a>\nRefs: <a href=\"https://github.com/nodejs/node/pull/16439\" class=\"issue-link js-issue-link\" data-error-text=\"Failed to load issue title\" data-id=\"271243248\" data-permission-text=\"Issue title is private\" data-url=\"https://github.com/nodejs/node/pull/16439\">#16439</a></p>",
+  "bodyText": "Refs: https://github.com/nodejs/node/pull/16439\nRefs: https://github.com/nodejs/node/pull/16439",
+  "labels": {
+    "nodes": [
+      {
+        "name": "build"
+      },
+      {
+        "name": "npm"
+      },
+      {
+        "name": "python"
+      }
+    ]
+  }
+}

test/fixtures/pr_with_self_ref.json

@@ -0,0 +1,20 @@
+{
+  "createdAt": "2017-10-24T11:13:43Z",
+  "url": "https://github.com/nodejs/node/pull/16438",
+  "bodyHTML":
+    "<p>Refs: <a href=\"https://github.com/nodejs/node/pull/16438\" class=\"issue-link js-issue-link\" data-error-text=\"Failed to load issue title\" data-id=\"271243248\" data-permission-text=\"Issue title is private\" data-url=\"https://github.com/nodejs/node/pull/16438\">#16438</a></p>",
+  "bodyText": "Refs: https://github.com/nodejs/node/pull/16438",
+  "labels": {
+    "nodes": [
+      {
+        "name": "build"
+      },
+      {
+        "name": "npm"
+      },
+      {
+        "name": "python"
+      }
+    ]
+  }
+}

test/unit/metadata_gen.test.js

@@ -3,19 +3,30 @@
 const MetadataGenerator = require('../../lib/metadata_gen');
 const {
   fixAndRefPR,
+  selfRefPR,
+  duplicateRefPR,
   fixCrossPR,
   backportPR,
   allGreenReviewers
 } = require('../fixtures/data');
 
 const assert = require('assert');
+
 const data = {
   owner: 'nodejs',
   repo: 'node',
   pr: fixAndRefPR,
   reviewers: allGreenReviewers
 };
-const crossData = Object.assign({}, data, { pr: fixCrossPR });
+const expected = `PR-URL: https://github.com/nodejs/node/pull/16438
+Fixes: https://github.com/nodejs/node/issues/16437
+Refs: https://github.com/nodejs/node/pull/15148
+Reviewed-By: Foo User <[email protected]>
+Reviewed-By: Quux User <[email protected]>
+Reviewed-By: Baz User <[email protected]>
+Reviewed-By: Bar User <[email protected]>
+`;
+
 const backportArgv = {
   argv: {
     owner: 'nodejs',
@@ -29,29 +40,39 @@ const backportArgv = {
     backport: true
   }
 };
-
 const backportData = Object.assign({}, data, { pr: backportPR }, backportArgv);
+const backportExpected = `PR-URL: https://github.com/nodejs/node/pull/29995
+Backport-PR-URL: https://github.com/nodejs/node/pull/30072
+Fixes: https://github.com/nodejs/build/issues/1961
+Refs: https://github.com/nodejs/node/commit/53ca0b9ae145c430842bf78e553e3b6cbd2823aa#commitcomment-35494896
+`;
 
-const expected = `PR-URL: https://github.com/nodejs/node/pull/16438
-Fixes: https://github.com/nodejs/node/issues/16437
-Refs: https://github.com/nodejs/node/pull/15148
+const selfRefData = Object.assign({}, data, { pr: selfRefPR });
+const selfRefExpected = `PR-URL: https://github.com/nodejs/node/pull/16438
+Reviewed-By: Foo User <[email protected]>
+Reviewed-By: Quux User <[email protected]>
+Reviewed-By: Baz User <[email protected]>
+Reviewed-By: Bar User <[email protected]>
+`;
+
+const duplicateRefData = Object.assign({}, data, { pr: duplicateRefPR });
+const duplicateRefExpected = `PR-URL: https://github.com/nodejs/node/pull/16438
+Refs: https://github.com/nodejs/node/pull/16439
 Reviewed-By: Foo User <[email protected]>
 Reviewed-By: Quux User <[email protected]>
 Reviewed-By: Baz User <[email protected]>
 Reviewed-By: Bar User <[email protected]>
 `;
+
+const crossData = Object.assign({}, data, { pr: fixCrossPR });
 const crossExpected = `PR-URL: https://github.com/nodejs/node/pull/16438
 Fixes: https://github.com/joyeecheung/node-core-utils/issues/123
 Reviewed-By: Foo User <[email protected]>
 Reviewed-By: Quux User <[email protected]>
 Reviewed-By: Baz User <[email protected]>
 Reviewed-By: Bar User <[email protected]>
 `;
-const backportExpected = `PR-URL: https://github.com/nodejs/node/pull/29995
-Backport-PR-URL: https://github.com/nodejs/node/pull/30072
-Fixes: https://github.com/nodejs/build/issues/1961
-Refs: https://github.com/nodejs/node/commit/53ca0b9ae145c430842bf78e553e3b6cbd2823aa#commitcomment-35494896
-`;
+
 const skipRefsExpected = `PR-URL: https://github.com/nodejs/node/pull/16438
 Reviewed-By: Foo User <[email protected]>
 Reviewed-By: Quux User <[email protected]>
@@ -65,6 +86,16 @@ describe('MetadataGenerator', () => {
     assert.strictEqual(expected, results);
   });
 
+  it('should prevent duplicate references', () => {
+    const results = new MetadataGenerator(duplicateRefData).getMetadata();
+    assert.strictEqual(duplicateRefExpected, results);
+  });
+
+  it('should prevent self-referential references', () => {
+    const results = new MetadataGenerator(selfRefData).getMetadata();
+    assert.strictEqual(selfRefExpected, results);
+  });
+
   it('should handle cross-owner and cross-repo fixes properly', () => {
     const results = new MetadataGenerator(crossData).getMetadata();
     assert.strictEqual(crossExpected, results);