Merge branch 'refactor-basic-diff' of https://github.com/walmartlabs/grafana into walmartlabs-refactor-basic-diff

pkg/components/dashdiffs/formatter_basic.go

@@ -71,6 +71,8 @@ func NewBasicFormatter(left interface{}) *BasicFormatter {
 	}
 }
 
+// Format takes the diff of two JSON documents, and returns the difference
+// between them summarized in an HTML document.
 func (b *BasicFormatter) Format(d diff.Diff) ([]byte, error) {
 	// calling jsonDiff.Format(d) populates the JSON diff's "Lines" value,
 	// which we use to compute the basic dif
@@ -90,67 +92,45 @@ func (b *BasicFormatter) Format(d diff.Diff) ([]byte, error) {
 	return buf.Bytes(), nil
 }
 
-// Basic is V2 of the basic diff
+// Basic transforms a slice of JSONLines into a slice of BasicBlocks.
 func (b *BasicDiff) Basic(lines []*JSONLine) []*BasicBlock {
 	// init an array you can append to for the basic "blocks"
 	blocks := make([]*BasicBlock, 0)
 
-	// iterate through each line
 	for _, line := range lines {
-		// TODO: this condition needs an explaination? what does it mean?
-		if b.LastIndent == 2 && line.Indent == 1 && line.Change == ChangeNil {
+		if b.returnToTopLevelKey(line) {
 			if b.Block != nil {
 				blocks = append(blocks, b.Block)
 			}
 		}
 
+		// Record the last indent level at each pass in case we need to
+		// check for a change in depth inside the JSON data structures.
 		b.LastIndent = line.Indent
 
-		// TODO: why special handling for indent 2?
 		if line.Indent == 1 {
-			switch line.Change {
-			case ChangeNil:
-				if line.Change == ChangeNil {
-					if line.Key != "" {
-						b.Block = &BasicBlock{
-							Title:  line.Key,
-							Change: line.Change,
-						}
-					}
-				}
-
-			case ChangeAdded, ChangeDeleted:
-				blocks = append(blocks, &BasicBlock{
-					Title:     line.Key,
-					Change:    line.Change,
-					New:       line.Val,
-					LineStart: line.LineNum,
-				})
-
-			case ChangeOld:
-				b.Block = &BasicBlock{
-					Title:     line.Key,
-					Old:       line.Val,
-					Change:    line.Change,
-					LineStart: line.LineNum,
-				}
-
-			case ChangeNew:
-				b.Block.New = line.Val
-				b.Block.LineEnd = line.LineNum
-
-				// then write out the change
-				blocks = append(blocks, b.Block)
-			default:
-				// ok
+			if block, ok := b.handleTopLevelChange(line); ok {
+				blocks = append(blocks, block)
 			}
 		}
 
-		// TODO: why special handling for indent > 2 ?
-		// Other Lines
+		// Here is where we handle changes for all types, appending each change
+		// to the current block based on the value.
+		//
+		// Values which only occupy a single line in JSON (like a string or
+		// int, for example) are treated as "Basic Changes" that we append to
+		// the current block as soon as they're detected.
+		//
+		// Values which occupy multiple lines (either slices or maps) are
+		// treated as "Basic Summaries". When we detect the "ChangeNil" type,
+		// we know we've encountered one of these types, so we record the
+		// starting position as well the type of the change, and stop
+		// performing comparisons until we find the end of that change. Upon
+		// finding the change, we append it to the current block, and begin
+		// performing comparisons again.
 		if line.Indent > 1 {
-			// Ensure single line change
-			if line.Key != "" && line.Val != nil && !b.writing {
+			// check to ensure a single line change
+			if b.isSingleLineChange(line) {
 				switch line.Change {
 				case ChangeAdded, ChangeDeleted:
 
@@ -178,13 +158,31 @@ func (b *BasicDiff) Basic(lines []*JSONLine) []*BasicBlock {
 					//ok
 				}
 
+				// otherwise, we're dealing with a change at a deeper level. We
+				// know there's a change somewhere in the JSON tree, but we
+				// don't know exactly where, so we go deeper.
 			} else {
+
+				// if the change is anything but unchanged, continue processing
+				//
+				// we keep "narrowing" the key as we go deeper, in order to
+				// correctly report the key name for changes found within an
+				// object or array.
 				if line.Change != ChangeUnchanged {
 					if line.Key != "" {
 						b.narrow = line.Key
 						b.keysIdent = line.Indent
 					}
 
+					// if the change isn't nil, and we're not already writing
+					// out a change, then we've found something.
+					//
+					// First, try to determine the title of the embedded JSON
+					// object. If it's an empty string, then we're in an object
+					// or array, so we default to using the "narrowed" key.
+					//
+					// We also start recording the basic summary, until we find
+					// the next `ChangeUnchanged`.
 					if line.Change != ChangeNil {
 						if !b.writing {
 							b.writing = true
@@ -204,6 +202,17 @@ func (b *BasicDiff) Basic(lines []*JSONLine) []*BasicBlock {
 							}
 						}
 					}
+					// if we find a `ChangeUnchanged`, we do one of two things:
+					//
+					// - if we're recording a change already, then we know
+					// we've come to the end of that change block, so we write
+					// that change out be recording the line number of where
+					// that change ends, and append it to the current block's
+					// summary.
+					//
+					// - if we're not recording a change, then we do nothing,
+					// since the BasicDiff doesn't report on unchanged JSON
+					// values.
 				} else {
 					if b.writing {
 						b.writing = false
@@ -218,6 +227,81 @@ func (b *BasicDiff) Basic(lines []*JSONLine) []*BasicBlock {
 	return blocks
 }
 
+// returnToTopLevelKey indicates that we've moved from a key at one level deep
+// in the JSON document to a top level key.
+//
+// In order to produce distinct "blocks" when rendering the basic diff,
+// we need a way to distinguish between differnt sections of data.
+// To do this, we consider the value(s) of each top-level JSON key to
+// represent a distinct block for Grafana's JSON data structure, so
+// we perform this check to see if we've entered a new "block". If we
+// have, we simply append the existing block to the array of blocks.
+func (b *BasicDiff) returnToTopLevelKey(line *JSONLine) bool {
+	return b.LastIndent == 2 && line.Indent == 1 && line.Change == ChangeNil
+}
+
+// handleTopLevelChange handles a change on one of the top-level keys on a JSON
+// document.
+//
+// If the line's indentation is at level 1, then we know it's a top
+// level key in the JSON document. As mentioned earlier, we treat these
+// specially as they indicate their values belong to distinct blocks.
+//
+// At level 1, we only record single-line changes, ie, the "added",
+// "deleted", "old" or "new" cases, since we know those values aren't
+// arrays or maps. We only handle these cases at level 2 or deeper,
+// since for those we either output a "change" or "summary". This is
+// done for formatting reasons only, so we have logical "blocks" to
+// display.
+func (b *BasicDiff) handleTopLevelChange(line *JSONLine) (*BasicBlock, bool) {
+	switch line.Change {
+	case ChangeNil:
+		if line.Change == ChangeNil {
+			if line.Key != "" {
+				b.Block = &BasicBlock{
+					Title:  line.Key,
+					Change: line.Change,
+				}
+			}
+		}
+
+	case ChangeAdded, ChangeDeleted:
+		return &BasicBlock{
+			Title:     line.Key,
+			Change:    line.Change,
+			New:       line.Val,
+			LineStart: line.LineNum,
+		}, true
+
+	case ChangeOld:
+		b.Block = &BasicBlock{
+			Title:     line.Key,
+			Old:       line.Val,
+			Change:    line.Change,
+			LineStart: line.LineNum,
+		}
+
+	case ChangeNew:
+		b.Block.New = line.Val
+		b.Block.LineEnd = line.LineNum
+
+		// For every "old" change there is a corresponding "new", which
+		// is why we wait until we detect the "new" change before
+		// appending the change.
+		return b.Block, true
+	default:
+		// ok
+	}
+
+	return nil, false
+}
+
+// isSingleLineChange ensures we're iterating over a single line change (ie,
+// either a single line or a old-new value pair was changed in the JSON file).
+func (b *BasicDiff) isSingleLineChange(line *JSONLine) bool {
+	return line.Key != "" && line.Val != nil && !b.writing
+}
+
 // encStateMap is used in the template helper
 var (
 	encStateMap = map[ChangeType]string{
@@ -240,7 +324,9 @@ var (
 )
 
 var (
-	// tplBlock is the whole thing
+	// tplBlock is the container for the basic diff. It iterates over each
+	// basic block, expanding each "change" and "summary" belonging to every
+	// block.
 	tplBlock = `{{ define "block" -}}
 {{ range . }}
 <div class="diff-group">
@@ -286,7 +372,7 @@ var (
 {{ end }}
 {{ end }}`
 
-	// tplChange is the template for changes
+	// tplChange is the template for basic changes.
 	tplChange = `{{ define "change" -}}
 <li class="diff-change-group">
 	<span class="bullet-position-container">
@@ -313,7 +399,7 @@ var (
 </li>
 {{ end }}`
 
-	// tplSummary is for basis summaries
+	// tplSummary is for basic summaries.
 	tplSummary = `{{ define "summary" -}}
 <div class="diff-group-name">
 	<i class="diff-circle diff-circle-{{ getChange .Change }} fa fa-circle-o diff-list-circle"></i>

pkg/components/dashdiffs/formatter_test.go

@@ -0,0 +1,131 @@
+package dashdiffs
+
+import (
+	"testing"
+
+	"github.com/grafana/grafana/pkg/components/simplejson"
+	. "github.com/smartystreets/goconvey/convey"
+)
+
+func TestDiff(t *testing.T) {
+	// Sample json docs for tests only
+	const (
+		leftJSON = `{
+			"key": "value",
+			"object": {
+				"key": "value",
+				"anotherObject": {
+					"same": "this field is the same in rightJSON",
+					"change": "this field should change in rightJSON",
+					"delete": "this field doesn't appear in rightJSON"
+				}
+			},
+			"array": [
+				"same",
+				"change",
+				"delete"
+			],
+			"embeddedArray": {
+				"array": [
+					"same",
+					"change",
+					"delete"
+				]
+			}
+		}`
+
+		rightJSON = `{
+			"key": "differentValue",
+			"object": {
+				"key": "value",
+				"newKey": "value",
+				"anotherObject": {
+					"same": "this field is the same in rightJSON",
+					"change": "this field should change in rightJSON",
+					"add": "this field is added"
+				}
+			},
+			"array": [
+				"same",
+				"changed!",
+				"add"
+			],
+			"embeddedArray": {
+				"array": [
+					"same",
+					"changed!",
+					"add"
+				]
+			}
+		}`
+	)
+
+	Convey("Testing dashboard diffs", t, func() {
+
+		// Compute the diff between the two JSON objects
+		baseData, err := simplejson.NewJson([]byte(leftJSON))
+		So(err, ShouldBeNil)
+
+		newData, err := simplejson.NewJson([]byte(rightJSON))
+		So(err, ShouldBeNil)
+
+		left, jsonDiff, err := getDiff(baseData, newData)
+		So(err, ShouldBeNil)
+
+		Convey("The JSONFormatter should produce the expected JSON tokens", func() {
+			f := NewJSONFormatter(left)
+			_, err := f.Format(jsonDiff)
+			So(err, ShouldBeNil)
+
+			// Total up the change types. If the number of different change
+			// types is correct, it means that the diff is producing correct
+			// output to the template rendered.
+			changeCounts := make(map[ChangeType]int)
+			for _, line := range f.Lines {
+				changeCounts[line.Change]++
+			}
+
+			// The expectedChangeCounts here were determined by manually
+			// looking at the JSON
+			expectedChangeCounts := map[ChangeType]int{
+				ChangeNil:       12,
+				ChangeAdded:     2,
+				ChangeDeleted:   1,
+				ChangeOld:       5,
+				ChangeNew:       5,
+				ChangeUnchanged: 5,
+			}
+			So(changeCounts, ShouldResemble, expectedChangeCounts)
+		})
+
+		Convey("The BasicFormatter should produce the expected BasicBlocks", func() {
+			f := NewBasicFormatter(left)
+			_, err := f.Format(jsonDiff)
+			So(err, ShouldBeNil)
+
+			bd := &BasicDiff{}
+			blocks := bd.Basic(f.jsonDiff.Lines)
+
+			changeCounts := make(map[ChangeType]int)
+			for _, block := range blocks {
+				for _, change := range block.Changes {
+					changeCounts[change.Change]++
+				}
+
+				for _, summary := range block.Summaries {
+					changeCounts[summary.Change]++
+				}
+
+				changeCounts[block.Change]++
+			}
+
+			expectedChangeCounts := map[ChangeType]int{
+				ChangeNil:     3,
+				ChangeAdded:   2,
+				ChangeDeleted: 1,
+				ChangeOld:     3,
+			}
+			So(changeCounts, ShouldResemble, expectedChangeCounts)
+		})
+	})
+}