From 95300fdcd98c1a1f5bd5d1f5dcbc8f96922096f8 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 18 Aug 2020 06:22:17 -0700 Subject: [PATCH] fix: prevent duplicate and self-refs (#478) --- lib/links.js | 12 +++--- lib/metadata_gen.js | 2 +- test/fixtures/data.js | 6 ++- test/fixtures/pr_with_duplicate_refs.json | 20 +++++++++ test/fixtures/pr_with_self_ref.json | 20 +++++++++ test/unit/metadata_gen.test.js | 51 ++++++++++++++++++----- 6 files changed, 93 insertions(+), 18 deletions(-) create mode 100644 test/fixtures/pr_with_duplicate_refs.json create mode 100644 test/fixtures/pr_with_self_ref.json diff --git a/lib/links.js b/lib/links.js index 9bbbe0a8..d21cd9b1 100644 --- a/lib/links.js +++ b/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. diff --git a/lib/metadata_gen.js b/lib/metadata_gen.js index e2a02d78..3214adb5 100644 --- a/lib/metadata_gen.js +++ b/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 = []; diff --git a/test/fixtures/data.js b/test/fixtures/data.js index 8c6f17ed..115c56a9 100644 --- a/test/fixtures/data.js +++ b/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 }; diff --git a/test/fixtures/pr_with_duplicate_refs.json b/test/fixtures/pr_with_duplicate_refs.json new file mode 100644 index 00000000..a38272f5 --- /dev/null +++ b/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": + "

Refs: #16439\nRefs: #16439

", + "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" + } + ] + } +} diff --git a/test/fixtures/pr_with_self_ref.json b/test/fixtures/pr_with_self_ref.json new file mode 100644 index 00000000..b075aaf5 --- /dev/null +++ b/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": + "

Refs: #16438

", + "bodyText": "Refs: https://github.com/nodejs/node/pull/16438", + "labels": { + "nodes": [ + { + "name": "build" + }, + { + "name": "npm" + }, + { + "name": "python" + } + ] + } +} diff --git a/test/unit/metadata_gen.test.js b/test/unit/metadata_gen.test.js index d8dbede9..07520c54 100644 --- a/test/unit/metadata_gen.test.js +++ b/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 +Reviewed-By: Quux User +Reviewed-By: Baz User +Reviewed-By: Bar User +`; + const backportArgv = { argv: { owner: 'nodejs', @@ -29,17 +40,31 @@ 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 +Reviewed-By: Quux User +Reviewed-By: Baz User +Reviewed-By: Bar User +`; + +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 Reviewed-By: Quux User Reviewed-By: Baz User Reviewed-By: Bar User `; + +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 @@ -47,11 +72,7 @@ Reviewed-By: Quux User Reviewed-By: Baz User Reviewed-By: Bar User `; -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 Reviewed-By: Quux User @@ -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);