feat(diff): Better diff of random objects (#1488)

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. The number of context lines used when rendering the JSON differences can be customized using the `--context-lines` option of `cdk diff`, which has a default value of `3`.
aws · Jan 7, 2019 · 607f997 · 607f997
1 parent 5b24583
commit 607f997
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 stream           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 (defaults to 3).
  */
-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 = 3) {
+  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 = 3) {
     // 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('context-lines', { type: 'number', desc: 'number of context lines to include in arbitrary JSON diff rendering', default: 3 })
       .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.contextLines);
 
       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'));
   }