Merge pull request #1197 from danger/fb/fix-moved-json-crash

Fix: [git] JSONDiffForFile when passed the path of a JSON File that was moved, crashes
danger · Jan 15, 2022 · a7355a3 · a7355a3
2 parents d896baf + 3411074
commit a7355a3
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 211 deletions.
diff --git a/.prettierignore b/.prettierignore
@@ -2,3 +2,4 @@ source/danger-outgoing-process-schema.json
 source/danger-incoming-process-schema.json
 source/platforms/_tests/fixtures/bbs-dsl-input.json
 source/platforms/_tests/fixtures/bbc-dsl-input.json
+CHANGELOG.md
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,7 @@
 <!-- Your comment below this -->
 
 - Fix: [Github] Multiple Inline Comments on the same file/line should all be posted [#1176](https://github.com/danger/danger-js/pull/1176) [@Rouby]
+- Fix: [git] JSONDiffForFile when passed the path of a JSON File that was moved, crashes [#1193](https://github.com/danger/danger-js/pull/1193) [@fbartho]
 
 <!-- Your comment above this -->
 

diff --git a/package.json b/package.json
@@ -100,6 +100,7 @@
     "@types/http-proxy-agent": "^2.0.1",
     "@types/jest": "^24.0.11",
     "@types/json5": "^0.0.30",
+    "@types/jsonpointer": "^4.0.0",
     "@types/jsonwebtoken": "^7.2.8",
     "@types/lodash.includes": "^4.3.4",
     "@types/lodash.mapvalues": "^4.6.6",

diff --git a/source/ambient.d.ts b/source/ambient.d.ts
@@ -7,7 +7,6 @@ declare module "jest-runtime"
 declare module "jest-haste-map"
 declare module "jest-environment-node"
 declare module "jest-config"
-declare module "jsonpointer"
 declare module "parse-link-header"
 declare module "pinpoint"
 declare module "prettyjson"

diff --git a/source/platforms/git/gitJSONToGitDSL.ts b/source/platforms/git/gitJSONToGitDSL.ts
@@ -65,6 +65,9 @@ export const gitJSONToGitDSL = (gitJSONRep: GitJSONDSL, config: GitJSONToGitDSLC
    * Takes a filename, and pulls from the PR the two versions of a file
    * where we then pass that off to the rfc6902 JSON patch generator.
    *
+   * If you provide the current filename:
+   *  - If a file was renamed then you'll see { before: null }
+   *
    * @param filename The path of the file
    */
   const JSONPatchForFile = async (filename: string) => {
@@ -110,16 +113,18 @@ export const gitJSONToGitDSL = (gitJSONRep: GitJSONDSL, config: GitJSONToGitDSLC
     // Thanks to @wtgtybhertgeghgtwtg for getting this started in #175
     // The idea is to loop through all the JSON patches, then pull out the before and after from those changes.
 
-    const { diff, before, after } = patchObject
-    return diff.reduce((accumulator, { path }) => {
+    const { diff: outerDiff, before, after } = patchObject
+    return outerDiff.reduce((accumulator, { path }) => {
       // We don't want to show the last root object, as these tend to just go directly
       // to a single value in the patch. This is fine, but not useful when showing a before/after
       const pathSteps = path.split("/")
       const backAStepPath = pathSteps.length <= 2 ? path : pathSteps.slice(0, pathSteps.length - 1).join("/")
 
       const diff: any = {
-        after: jsonpointer.get(after, backAStepPath) || null,
-        before: jsonpointer.get(before, backAStepPath) || null,
+        // If a file is moved/renamed, the file will be in "danger.git.modified_files"
+        // JSONPatchForFile will return null for the old file, so we need to check for that
+        before: (before && jsonpointer.get(before, backAStepPath)) || null,
+        after: (after && jsonpointer.get(after, backAStepPath)) || null,
       }
 
       const emptyValueOfCounterpart = (other: any) => {