feat(diff): Better diff of random objects

Use a unified diff format to render differences in arbitrary values, making it easier to understand what is changing in possibly large JSON structures, for example.
aws · Jan 7, 2019 · 63200c3 · 63200c3
1 parent 5b24583
commit 63200c3
Show file tree

Hide file tree

Showing 6 changed files with 127 additions and 22 deletions.
diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts
@@ -32,7 +32,7 @@ const DIFF_HANDLERS: HandlerRegistry = {
  * Compare two CloudFormation templates and return semantic differences between them.
  *
  * @param currentTemplate the current state of the stack.
- * @param newTemplate   the target state of the stack.
+ * @param newTemplate     the target state of the stack.
  *
  * @returns a +types.TemplateDiff+ object that represents the changes that will happen if
  *      a stack which current state is described by +currentTemplate+ is updated with
@@ -144,4 +144,4 @@ function deepCopy(x: any): any {
   }
 
   return x;
-}
+}
diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts
@@ -8,15 +8,23 @@ import { deepEqual } from './diff/util';
 import { IamChanges } from './iam/iam-changes';
 import { SecurityGroupChanges } from './network/security-group-changes';
 
+// tslint:disable-next-line:no-var-requires
+const { structuredPatch } = require('diff');
+
 /**
  * Renders template differences to the process' console.
  *
- * @param templateDiff TemplateDiff to be rendered to the console.
+ * @param string           The IO stream where to output the rendered diff.
+ * @param templateDiff     TemplateDiff to be rendered to the console.
  * @param logicalToPathMap A map from logical ID to construct path. Useful in
  *                         case there is no aws:cdk:path metadata in the template.
+ * @param context          the number of context lines to use in arbitrary JSON diff.
  */
-export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: TemplateDiff, logicalToPathMap: { [logicalId: string]: string } = { }) {
-  const formatter = new Formatter(stream, logicalToPathMap, templateDiff);
+export function formatDifferences(stream: NodeJS.WriteStream,
+                                  templateDiff: TemplateDiff,
+                                  logicalToPathMap: { [logicalId: string]: string } = { },
+                                  context?: number) {
+  const formatter = new Formatter(stream, logicalToPathMap, templateDiff, context);
 
   if (templateDiff.awsTemplateFormatVersion || templateDiff.transform || templateDiff.description) {
     formatter.printSectionHeader('Template');
@@ -40,8 +48,11 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp
 /**
  * Renders a diff of security changes to the given stream
  */
-export function formatSecurityChanges(stream: NodeJS.WriteStream, templateDiff: TemplateDiff, logicalToPathMap: {[logicalId: string]: string} = {}) {
-  const formatter = new Formatter(stream, logicalToPathMap, templateDiff);
+export function formatSecurityChanges(stream: NodeJS.WriteStream,
+                                      templateDiff: TemplateDiff,
+                                      logicalToPathMap: {[logicalId: string]: string} = {},
+                                      context?: number) {
+  const formatter = new Formatter(stream, logicalToPathMap, templateDiff, context);
 
   formatSecurityChangesWithBanner(formatter, templateDiff);
 }
@@ -56,11 +67,15 @@ function formatSecurityChangesWithBanner(formatter: Formatter, templateDiff: Tem
 }
 
 const ADDITION = colors.green('[+]');
+const CONTEXT  = colors.grey('[ ]');
 const UPDATE   = colors.yellow('[~]');
 const REMOVAL  = colors.red('[-]');
 
 class Formatter {
-  constructor(private readonly stream: NodeJS.WriteStream, private readonly logicalToPathMap: { [logicalId: string]: string }, diff?: TemplateDiff) {
+  constructor(private readonly stream: NodeJS.WriteStream,
+              private readonly logicalToPathMap: { [logicalId: string]: string },
+              diff?: TemplateDiff,
+              private readonly context: number = 5) {
     // Read additional construct paths from the diff if it is supplied
     if (diff) {
       this.readConstructPathsFrom(diff);
@@ -126,7 +141,7 @@ class Formatter {
    * Print a resource difference for a given logical ID.
    *
    * @param logicalId the logical ID of the resource that changed.
-   * @param diff    the change to be rendered.
+   * @param diff      the change to be rendered.
    */
   public formatResourceDifference(_type: string, logicalId: string, diff: ResourceDifference) {
     const resourceType = diff.isRemoval ? diff.oldResourceType : diff.newResourceType;
@@ -184,9 +199,9 @@ class Formatter {
 
   /**
    * Renders a tree of differences under a particular name.
-   * @param name the name of the root of the tree.
-   * @param diff the difference on the tree.
-   * @param last whether this is the last node of a parent tree.
+   * @param name    the name of the root of the tree.
+   * @param diff    the difference on the tree.
+   * @param last    whether this is the last node of a parent tree.
    */
   public formatTreeDiff(name: string, diff: Difference<any>, last: boolean) {
     let additionalInfo = '';
@@ -210,10 +225,19 @@ class Formatter {
    * @param linePrefix a prefix (indent-like) to be used on every line.
    */
   public formatObjectDiff(oldObject: any, newObject: any, linePrefix: string) {
-    if ((typeof oldObject !== typeof newObject) || Array.isArray(oldObject) || typeof oldObject === 'string' || typeof oldObject === 'number') {
+    if ((typeof oldObject !== typeof newObject) || Array.isArray(oldObject) || typeof oldObject === 'string' || typeof oldObject === 'number') {
       if (oldObject !== undefined && newObject !== undefined) {
-        this.print('%s   ├─ %s %s', linePrefix, REMOVAL, this.formatValue(oldObject, colors.red));
-        this.print('%s   └─ %s %s', linePrefix, ADDITION, this.formatValue(newObject, colors.green));
+        if (typeof oldObject === 'object' || typeof newObject === 'object') {
+          const oldStr = JSON.stringify(oldObject, null, 2);
+          const newStr = JSON.stringify(newObject, null, 2);
+          const diff = _diffStrings(oldStr, newStr, this.context);
+          for (let i = 0 ; i < diff.length ; i++) {
+            this.print('%s   %s %s', linePrefix, i === 0 ? '└─' : '  ', diff[i]);
+          }
+        } else {
+          this.print('%s   ├─ %s %s', linePrefix, REMOVAL, this.formatValue(oldObject, colors.red));
+          this.print('%s   └─ %s %s', linePrefix, ADDITION, this.formatValue(newObject, colors.green));
+        }
       } else if (oldObject !== undefined /* && newObject === undefined */) {
         this.print('%s   └─ %s', linePrefix, this.formatValue(oldObject, colors.red));
       } else /* if (oldObject === undefined && newObject !== undefined) */ {
@@ -398,3 +422,75 @@ function stripHorizontalLines(tableRendering: string) {
     return cols[1];
   }
 }
+
+/**
+ * A patch as returned by ``diff.structuredPatch``.
+ */
+interface Patch {
+  /**
+   * Hunks in the patch.
+   */
+  hunks: ReadonlyArray<PatchHunk>;
+}
+
+/**
+ * A hunk in a patch produced by ``diff.structuredPatch``.
+ */
+interface PatchHunk {
+  oldStart: number;
+  oldLines: number;
+  newStart: number;
+  newLines: number;
+  lines: string[];
+}
+
+/**
+ * Creates a unified diff of two strings.
+ *
+ * @param oldStr  the "old" version of the string.
+ * @param newStr  the "new" version of the string.
+ * @param context the number of context lines to use in arbitrary JSON diff.
+ *
+ * @returns an array of diff lines.
+ */
+function _diffStrings(oldStr: string, newStr: string, context: number): string[] {
+  const patch: Patch = structuredPatch(null, null, oldStr, newStr, null, null, { context });
+  const result = new Array<string>();
+  for (const hunk of patch.hunks) {
+    result.push(colors.magenta(`@@ -${hunk.oldStart},${hunk.oldLines} +${hunk.newStart},${hunk.newLines} @@`));
+    const baseIndent = _findIndent(hunk.lines);
+    for (const line of hunk.lines) {
+      // Don't care about termination newline.
+      if (line === '\\ No newline at end of file') { continue; }
+      const marker = line.charAt(0);
+      const text = line.slice(1 + baseIndent);
+      switch (marker) {
+      case ' ':
+        result.push(`${CONTEXT} ${text}`);
+        break;
+      case '+':
+        result.push(colors.bold(`${ADDITION} ${colors.green(text)}`));
+        break;
+      case '-':
+        result.push(colors.bold(`${REMOVAL} ${colors.red(text)}`));
+        break;
+      default:
+        throw new Error(`Unexpected diff marker: ${marker} (full line: ${line})`);
+      }
+    }
+  }
+  return result;
+
+  function _findIndent(lines: string[]): number {
+    let indent = Number.MAX_SAFE_INTEGER;
+    for (const line of lines) {
+      for (let i = 1 ; i < line.length ; i++) {
+        if (line.charAt(i) !== ' ') {
+          indent = indent > i - 1 ? i - 1 : indent;
+          break;
+        }
+      }
+    }
+    return indent;
+  }
+}
diff --git a/packages/@aws-cdk/cloudformation-diff/package-lock.json b/packages/@aws-cdk/cloudformation-diff/package-lock.json
diff --git a/packages/@aws-cdk/cloudformation-diff/package.json b/packages/@aws-cdk/cloudformation-diff/package.json
@@ -27,6 +27,7 @@
     "@aws-cdk/cx-api": "^0.21.0",
     "cli-table": "^0.3.1",
     "colors": "^1.2.1",
+    "diff": "^4.0.1",
     "fast-deep-equal": "^2.0.1",
     "source-map-support": "^0.5.6"
   },

diff --git a/packages/aws-cdk/bin/cdk.ts b/packages/aws-cdk/bin/cdk.ts
@@ -60,6 +60,7 @@ async function parseCommandLineArguments() {
     .command('destroy [STACKS..]', 'Destroy the stack(s) named STACKS', yargs => yargs
       .option('force', { type: 'boolean', alias: 'f', desc: 'Do not ask for confirmation before destroying the stacks' }))
     .command('diff [STACK]', 'Compares the specified stack with the deployed stack or a local template file, and returns with status 1 if any difference is found', yargs => yargs
+      .option('json-context', { type: 'number', desc: 'number of context lines to include in arbitrary JSON diff rendering', default: 5 })
       .option('template', { type: 'string', desc: 'the path to the CloudFormation template to compare with' })
       .option('strict', { type: 'boolean', desc: 'do not filter out AWS::CDK::Metadata resources', default: false }))
     .command('metadata [STACK]', 'Returns all metadata associated with this stack')
@@ -142,7 +143,7 @@ async function initCommandLine() {
     return returnValue;
   }
 
-  async function main(command: string, args: any): Promise<number | string | {} | void> {
+  async function main(command: string, args: any): Promise<number | string | {} | void> {
     const toolkitStackName: string = configuration.combined.get(['toolkitStackName']) || DEFAULT_TOOLKIT_STACK_NAME;
 
     if (toolkitStackName !== DEFAULT_TOOLKIT_STACK_NAME) {
@@ -158,7 +159,7 @@ async function initCommandLine() {
         return await cliList({ long: args.long });
 
       case 'diff':
-        return await diffStack(await findStack(args.STACK), args.template, args.strict);
+        return await diffStack(await findStack(args.STACK), args.template, args.strict, args.jsonContext);
 
       case 'bootstrap':
         return await cliBootstrap(args.ENVIRONMENTS, toolkitStackName, args.roleArn);
@@ -383,10 +384,10 @@ async function initCommandLine() {
     }
   }
 
-  async function diffStack(stackName: string, templatePath: string | undefined, strict: boolean): Promise<number> {
+  async function diffStack(stackName: string, templatePath: string | undefined, strict: boolean, context: number): Promise<number> {
     const stack = await appStacks.synthesizeStack(stackName);
     const currentTemplate = await readCurrentTemplate(stack, templatePath);
-    if (printStackDiff(currentTemplate, stack, strict) === 0) {
+    if (printStackDiff(currentTemplate, stack, strict, context) === 0) {
       return 0;
     } else {
       return 1;

diff --git a/packages/aws-cdk/lib/diff.ts b/packages/aws-cdk/lib/diff.ts
@@ -8,10 +8,12 @@ import { print, warning } from './logging';
  *
  * @param oldTemplate the old/current state of the stack.
  * @param newTemplate the new/target state of the stack.
+ * @param strict      do not filter out AWS::CDK::Metadata
+ * @param context     lines of context to use in arbitrary JSON diff
  *
  * @returns the count of differences that were rendered.
  */
-export function printStackDiff(oldTemplate: any, newTemplate: cxapi.SynthesizedStack, strict: boolean): number {
+export function printStackDiff(oldTemplate: any, newTemplate: cxapi.SynthesizedStack, strict: boolean, context: number): number {
   if (_hasAssets(newTemplate)) {
     const issue = 'https://github.com/awslabs/aws-cdk/issues/395';
     warning(`The ${newTemplate.name} stack uses assets, which are currently not accounted for in the diff output! See ${issue}`);
@@ -30,7 +32,7 @@ export function printStackDiff(oldTemplate: any, newTemplate: cxapi.SynthesizedS
   }
 
   if (!diff.isEmpty) {
-    cfnDiff.formatDifferences(process.stderr, diff, buildLogicalToPathMap(newTemplate));
+    cfnDiff.formatDifferences(process.stderr, diff, buildLogicalToPathMap(newTemplate), context);
   } else {
     print(colors.green('There were no differences'));
   }