[LIB][BUG] (*node.Tree).Sync() is not working properly.

Summary:
There was a few bugs detected by adding a simple integration test:
- The scanner created from the response body was not working properly, it was never making it inside of the for loop.
- The checkpoint is not necessarily present on every line, so by updating it on every line, we were ending up removing the Checkpoint completely and thus getting stuck in a sync loop.
- Forcing a complete reset if the parents have changed was not optimal nor the right thing to do.

fixes T14

Test Plan:
Travis-CI is happy.

AND

STR:
- Delete the current cache located at `~/Library/Caches/com.appspot.go-acd.cache` on Mac.
- Run the `ls` command implemented in T5, this should create the cache
- Upload a new folder using the [Amazon Cloud Drive Web UI](https://www.amazon.com/clouddrive)
- Run the `ls` command to list the recently uploaded folder

Expected:
The files in the newly created folder are listed.

Actual:
The library is returning `node not found`

```
kalbasit@cratos ~/code/src/gopkg.in/acd.v0 [T5 *] ± % go run cmd/acd/main.go ls acd:///pictures
panic: node not found

goroutine 1 [running]:
gopkg.in/acd.v0/cli.lsAction(0xc2082037a0)
        /Users/kalbasit/code/src/gopkg.in/acd.v0/cli/ls.go:31 +0x126
github.com/codegangsta/cli.(*App).RunAsSubcommand(0xc20806d400, 0xc208080000, 0x0, 0x0)
        /Users/kalbasit/code/src/github.com/codegangsta/cli/app.go:262 +0xc8c
github.com/codegangsta/cli.Command.startApp(0x39c8b0, 0x2, 0x0, 0x0, 0x0, 0x0, 0x0, 0x3d2590, 0x17, 0x444190, ...)
        /Users/kalbasit/code/src/github.com/codegangsta/cli/command.go:183 +0x505
github.com/codegangsta/cli.Command.Run(0x39c8b0, 0x2, 0x0, 0x0, 0x0, 0x0, 0x0, 0x3d2590, 0x17, 0x444190, ...)
        /Users/kalbasit/code/src/github.com/codegangsta/cli/command.go:45 +0x14ee
github.com/codegangsta/cli.(*App).Run(0xc20806c200, 0xc20800a000, 0x3, 0x3, 0x0, 0x0)
        /Users/kalbasit/code/src/github.com/codegangsta/cli/app.go:160 +0xd0c
github.com/codegangsta/cli.(*App).RunAndExitOnError(0xc20806c200)
        /Users/kalbasit/code/src/github.com/codegangsta/cli/app.go:171 +0x57
main.main()
        /Users/kalbasit/code/src/gopkg.in/acd.v0/cmd/acd/main.go:7 +0x2c

goroutine 17 [syscall, locked to thread]:
runtime.goexit()
        /usr/local/Cellar/go/1.4.2/libexec/src/runtime/asm_amd64.s:2232 +0x1
exit status 2
```

Reviewers: #go_amazon_cloud_drive, kalbasit

Reviewed By: #go_amazon_cloud_drive, kalbasit

Maniphest Tasks: T14

Differential Revision: http://phabricator.nasreddine.com/D11

Author: kalbasit

integrationtest/fixtures/syncfolder/README.syncfolder

@@ -0,0 +1,22 @@
+The Amazon Cloud Drive API lets your customers access the photos, videos, and
+documents they have saved in Amazon Cloud Drive, and gives you the ability to
+interact with millions of Amazon customers. With access to the free Amazon
+Cloud Drive API you can put your own creative spin on how they upload, view,
+edit, download, and organize their digital content using your app.
+
+Built on the highly reliable and scalable AWS platform, the Amazon Cloud Drive
+API puts the power of a large-scale cloud services platform in your hands. You
+can offer confidence and peace of mind to your customer, as their content is
+safe and always accessible from Amazon Cloud Drive. Quit worrying about the
+cost of storage and unleash your creativity to build the best experience for
+your customers. For example, you could develop a mobile app to let customers
+edit all of their photos and videos into a single movie, or a web app that
+allows customers to organize their content around geolocation, or a desktop app
+that reimagines the way customers interact locally with their data in the
+cloud.
+
+If you're new to the Amazon Cloud Drive API, see our Developer Guide and tour
+Getting Started.
+
+https://developer.amazon.com/public/apis/experience/cloud-drive
+syncfolder/README.syncfolder

integrationtest/simple_upload_test.go

@@ -45,10 +45,10 @@ func TestSimpleUpload(t *testing.T) {
 	// test the NodeTree is updated
 	node, err := c.NodeTree.FindNode(remoteReadmeFile)
 	if err != nil {
-		t.Errorf("c.NodeTree.FindNode(%s): got error %s", remoteReadmeFile, err)
+		t.Errorf("c.NodeTree.FindNode(%q): got error %s", remoteReadmeFile, err)
 	}
 	if want, got := inmd5, node.ContentProperties.MD5; want != got {
-		t.Errorf("c.NodeTree.FindNode(%s).ContentProperties.MD5: want %s got %s", remoteReadmeFile, want, got)
+		t.Errorf("c.NodeTree.FindNode(%q).ContentProperties.MD5: want %s got %s", remoteReadmeFile, want, got)
 	}
 
 	// test the cache is being saved updated
@@ -62,10 +62,10 @@ func TestSimpleUpload(t *testing.T) {
 	}
 	node, err = c.NodeTree.FindNode(remoteReadmeFile)
 	if err != nil {
-		t.Errorf("reloaded cache, c.NodeTree.FindNode(%s): got error %s", remoteReadmeFile, err)
+		t.Errorf("reloaded cache, c.NodeTree.FindNode(%q): got error %s", remoteReadmeFile, err)
 	}
 	if want, got := inmd5, node.ContentProperties.MD5; want != got {
-		t.Errorf("reloaded cache, c.NodeTree.FindNode(%s).ContentProperties.MD5: want %s got %s", remoteReadmeFile, want, got)
+		t.Errorf("reloaded cache, c.NodeTree.FindNode(%q).ContentProperties.MD5: want %s got %s", remoteReadmeFile, want, got)
 	}
 
 	// check the file exists on the server

integrationtest/tree_sync_test.go

@@ -0,0 +1,63 @@
+package integrationtest
+
+import (
+	"crypto/md5"
+	"encoding/hex"
+	"io"
+	"os"
+	"testing"
+)
+
+func TestTreeSync(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping integration test")
+	}
+	needCleaning = true
+
+	var (
+		readmeFile       = "fixtures/syncfolder/README.syncfolder"
+		remoteReadmeFile = remotePath(readmeFile)
+	)
+
+	// create two cached clients
+	c1, err := newCachedClient(true)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := c1.FetchNodeTree(); err != nil {
+		t.Fatal(err)
+	}
+	c2, err := newCachedClient(true)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := c2.FetchNodeTree(); err != nil {
+		t.Fatal(err)
+	}
+
+	// using the first client upload the README file
+	in, err := os.Open(readmeFile)
+	if err != nil {
+		t.Fatal(err)
+	}
+	inhash := md5.New()
+	in.Seek(0, 0)
+	io.Copy(inhash, in)
+	inmd5 := hex.EncodeToString(inhash.Sum(nil))
+	in.Seek(0, 0)
+	if err := c1.Upload(remoteReadmeFile, in); err != nil {
+		t.Errorf("error uploading %s to %s: %s", readmeFile, remoteReadmeFile, err)
+	}
+
+	// using the second client, sync and find the node
+	if err := c2.NodeTree.Sync(); err != nil {
+		t.Fatal(err)
+	}
+	readmeNode, err := c2.NodeTree.FindNode(remoteReadmeFile)
+	if err != nil {
+		t.Fatalf("c2.NodeTree.FindNode(%q) error: %s", remoteReadmeFile, err)
+	}
+	if want, got := inmd5, readmeNode.ContentProperties.MD5; want != got {
+		t.Errorf("c.NodeTree.FindNode(%q).ContentProperties.MD5: want %s got %s", remoteReadmeFile, want, got)
+	}
+}

internal/constants/errors.go

@@ -25,6 +25,8 @@ var (
 	// ErrJSONDecodingResponseBody is returned if there was an error decoding the
 	// response body.
 	ErrJSONDecodingResponseBody = errors.New("error while JSON-decoding the response body")
+	// ErrReadingResponseBody is returned if ioutil.ReadAll() has failed.
+	ErrReadingResponseBody = errors.New("error reading the entire response body")
 
 	// Request errors
 

node/sync.go

@@ -1,17 +1,32 @@
 package node
 
 import (
-	"bufio"
 	"bytes"
 	"encoding/json"
+	"io/ioutil"
 	"net/http"
-	"reflect"
 	"sort"
 
 	"gopkg.in/acd.v0/internal/constants"
 	"gopkg.in/acd.v0/internal/log"
 )
 
+type (
+	changes struct {
+		Checkpoint    string `json:"checkpoint,omitempty"`
+		Chunksize     int    `json:"chunkSize,omitempty"`
+		MaxNodes      int    `json:"maxNodes,omitempty"`
+		IncludePurged string `json:"includePurged,omitempty"`
+	}
+
+	changesResponse struct {
+		Checkpoint string  `json:"checkpoint,omitempty"`
+		Nodes      []*Node `json:"nodes,omitempty"`
+		Reset      bool    `json:"reset,omitempty"`
+		End        bool    `json:"end,omitempty"`
+	}
+)
+
 // Sync syncs the tree with the server.
 func (nt *Tree) Sync() error {
 	postURL := nt.client.GetMetadataURL("changes")
@@ -23,17 +38,11 @@ func (nt *Tree) Sync() error {
 		log.Errorf("%s: %s", constants.ErrJSONEncoding, err)
 		return constants.ErrJSONEncoding
 	}
-
-	// return format should be:
-	// {"checkpoint": str, "reset": bool, "nodes": []}
-	// {"checkpoint": str, "reset": false, "nodes": []}
-	// {"end": true}
 	req, err := http.NewRequest("POST", postURL, bytes.NewBuffer(jsonBytes))
 	if err != nil {
 		log.Errorf("%s: %s", constants.ErrCreatingHTTPRequest, err)
 		return constants.ErrCreatingHTTPRequest
 	}
-
 	req.Header.Set("Content-Type", "application/json")
 	res, err := nt.client.Do(req)
 	if err != nil {
@@ -44,15 +53,26 @@ func (nt *Tree) Sync() error {
 		return err
 	}
 
+	// return format should be:
+	// {"checkpoint": str, "reset": bool, "nodes": []}
+	// {"checkpoint": str, "reset": false, "nodes": []}
+	// {"end": true}
 	defer res.Body.Close()
-	s := bufio.NewScanner(res.Body)
-	for s.Scan() {
+	bodyBytes, err := ioutil.ReadAll(res.Body)
+	if err != nil {
+		log.Errorf("%s: %s", constants.ErrReadingResponseBody, err)
+		return constants.ErrReadingResponseBody
+	}
+	for _, lineBytes := range bytes.Split(bodyBytes, []byte("\n")) {
 		var cr changesResponse
-		if err := json.Unmarshal(s.Bytes(), &cr); err != nil {
+		if err := json.Unmarshal(lineBytes, &cr); err != nil {
 			log.Errorf("%s: %s", constants.ErrJSONDecodingResponseBody, err)
 			return constants.ErrJSONDecodingResponseBody
 		}
-		nt.Checkpoint = cr.Checkpoint
+		if cr.Checkpoint != "" {
+			log.Debugf("changes returned Checkpoint: %s", cr.Checkpoint)
+			nt.Checkpoint = cr.Checkpoint
+		}
 		if cr.Reset {
 			log.Debug("reset is required")
 			return constants.ErrMustFetchFresh
@@ -71,8 +91,14 @@ func (nt *Tree) Sync() error {
 func (nt *Tree) updateNodes(nodes []*Node) error {
 	// first make sure our nodeMap is up to date
 	for _, node := range nodes {
+		log.Debugf("node %s ID %s has changed.", node.Name, node.ID)
 		if _, found := nt.nodeMap[node.ID]; !found {
-			nt.nodeMap[node.ID] = node
+			// remove the parents from the node we are inserting so the next section
+			// will detect the added parents and add those.
+			newNode := &Node{}
+			(*newNode) = *node
+			newNode.Parents = []string{}
+			nt.nodeMap[node.ID] = newNode
 		}
 	}
 
@@ -93,7 +119,7 @@ func (nt *Tree) updateNodes(nodes []*Node) error {
 				if err != nil {
 					continue
 				}
-				parent.RemoveChild(node)
+				parent.RemoveChild(nt.nodeMap[node.ID])
 			}
 
 			// remove the node itself from the nodemap
@@ -102,12 +128,28 @@ func (nt *Tree) updateNodes(nodes []*Node) error {
 			continue
 		}
 
-		// TODO: Handle change in parent IDs gracefully!
-		sort.Strings(node.Parents)
+		// add/remove parents
+		sort.Strings(nt.nodeMap[node.ID].Parents)
 		sort.Strings(newNode.Parents)
-		if !reflect.DeepEqual(node.Parents, newNode.Parents) {
-			log.Debugf("The parents of the node %s have changed. %v => %v", node.ID, node.Parents, newNode.Parents)
-			return constants.ErrMustFetchFresh
+		if parentIDs := diffSliceStr(nt.nodeMap[node.ID].Parents, newNode.Parents); len(parentIDs) > 0 {
+			for _, parentID := range parentIDs {
+				log.Debugf("ParentID %s has been removed from %s ID %s", parentID, node.Name, node.ID)
+				parent, err := nt.FindByID(parentID)
+				if err != nil {
+					continue
+				}
+				parent.RemoveChild(nt.nodeMap[node.ID])
+			}
+		}
+		if parentIDs := diffSliceStr(newNode.Parents, nt.nodeMap[node.ID].Parents); len(parentIDs) > 0 {
+			for _, parentID := range parentIDs {
+				log.Debugf("ParentID %s has been added to %s ID %s", parentID, node.Name, node.ID)
+				parent, err := nt.FindByID(parentID)
+				if err != nil {
+					continue
+				}
+				parent.AddChild(nt.nodeMap[node.ID])
+			}
 		}
 
 		// finally update the node itself

node/tree.go

@@ -31,20 +31,6 @@ type (
 		NextToken    string  `json:"nextToken,omitempty"`
 		Nodes        []*Node `json:"data,omitempty"`
 	}
-
-	changes struct {
-		Checkpoint    string `json:"checkpoint,omitempty"`
-		Chunksize     int    `json:"chunkSize,omitempty"`
-		MaxNodes      int    `json:"maxNodes,omitempty"`
-		IncludePurged string `json:"includePurged,omitempty"`
-	}
-
-	changesResponse struct {
-		Checkpoint string  `json:"checkpoint,omitempty"`
-		Nodes      []*Node `json:"nodes,omitempty"`
-		Reset      bool    `json:"reset,omitempty"`
-		End        bool    `json:"end,omitempty"`
-	}
 )
 
 // RemoveNode removes this node from the server and from the NodeTree.

node/util.go

@@ -0,0 +1,23 @@
+package node
+
+// diff returns all of the strings that exist in s1 but do not exist in s2
+func diffSliceStr(s1, s2 []string) []string {
+	var vs []string
+	var found bool
+
+	for _, v1 := range s1 {
+		found = false
+		for _, v2 := range s2 {
+			if v1 == v2 {
+				found = true
+				break
+			}
+		}
+
+		if !found {
+			vs = append(vs, v1)
+		}
+	}
+
+	return vs
+}

node/util_test.go

@@ -0,0 +1,44 @@
+package node
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestDiffSliceStr(t *testing.T) {
+	type sliceString []string
+	slices := [][][]string{
+		[][]string{
+			[]string{"a", "b", "c"},
+			[]string{"a", "b"},
+		},
+		[][]string{
+			[]string{"a", "b", "c"},
+			[]string{"a", "b", "c"},
+		},
+		[][]string{
+			[]string{"b", "c"},
+			[]string{"a", "b", "c"},
+		},
+	}
+	diffs := [][]string{
+		[]string{"c"},
+		[]string{},
+		[]string{},
+	}
+
+	for i, ss := range slices {
+		want, got := diffs[i], diffSliceStr(ss[0], ss[1])
+
+		// when we get an empty slice, we are actually getting an uninitialized one
+		// for reflect.DeepEqual an initialized and an uninitialized slices are not
+		// equal so we must initialize got so it reflect does not bark
+		if len(got) == 0 {
+			got = make([]string, 0)
+		}
+
+		if !reflect.DeepEqual(want, got) {
+			t.Errorf("diffSliceStr(%v, %v): want %v, got %v", ss[0], ss[1], want, got)
+		}
+	}
+}