From eb772d62ecc9d1ad61df7bbf08a572fda689c5be Mon Sep 17 00:00:00 2001
From: Lucas Di Pentima <ldipentima@veritasgenetics.com>
Date: Mon, 30 Oct 2017 19:42:06 -0300
Subject: [PATCH] 12018: Enhanced error message when having a parse error.
 Added test cleanup. Added test to confirm records with empty fields are
 ignored.

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <ldipentima@veritasgenetics.com>
---
 tools/arv-sync-groups/arv-sync-groups.go      |  4 +-
 tools/arv-sync-groups/arv-sync-groups_test.go | 86 +++++++++++++++++++
 2 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/tools/arv-sync-groups/arv-sync-groups.go b/tools/arv-sync-groups/arv-sync-groups.go
index a1a73ca923..40c6934602 100644
--- a/tools/arv-sync-groups/arv-sync-groups.go
+++ b/tools/arv-sync-groups/arv-sync-groups.go
@@ -336,14 +336,16 @@ func doMain(cfg *ConfigParams) error {
 
 // ProcessFile reads the CSV file and process every record
 func ProcessFile(cfg *ConfigParams, f *os.File, userIDToUUID map[string]string, groupNameToUUID map[string]string, remoteGroups map[string]*GroupInfo, allUsers map[string]arvados.User) (groupsCreated, membersAdded, membersSkipped int, err error) {
+	lineNo := 0
 	csvReader := csv.NewReader(f)
 	for {
 		record, e := csvReader.Read()
 		if e == io.EOF {
 			break
 		}
+		lineNo++
 		if e != nil {
-			err = fmt.Errorf("error reading %q: %s", cfg.Path, err)
+			err = fmt.Errorf("error parsing %q, line %d", cfg.Path, lineNo)
 			return
 		}
 		groupName := strings.TrimSpace(record[0])
diff --git a/tools/arv-sync-groups/arv-sync-groups_test.go b/tools/arv-sync-groups/arv-sync-groups_test.go
index d4dd8543f4..e41ad88632 100644
--- a/tools/arv-sync-groups/arv-sync-groups_test.go
+++ b/tools/arv-sync-groups/arv-sync-groups_test.go
@@ -75,6 +75,65 @@ func (s *TestSuite) SetUpSuite(c *C) {
 	c.Assert(len(s.users), Not(Equals), 0)
 }
 
+// Clean any membership link and remote group created by the test
+func (s *TestSuite) TearDownTest(c *C) {
+	gl := arvados.GroupList{}
+	params := arvados.ResourceListParams{
+		Filters: []arvados.Filter{{
+			Attr:     "group_class",
+			Operator: "=",
+			Operand:  "role",
+		}, {
+			Attr:     "owner_uuid",
+			Operator: "=",
+			Operand:  s.cfg.ParentGroupUUID,
+		}},
+	}
+	err := s.cfg.Client.RequestAndDecode(&gl, "GET", "/arvados/v1/groups", nil, params)
+	c.Assert(err, IsNil)
+	for _, group := range gl.Items {
+		ll := arvados.LinkList{}
+		// Delete user->group links
+		params = arvados.ResourceListParams{
+			Filters: []arvados.Filter{{
+				Attr:     "link_class",
+				Operator: "=",
+				Operand:  "permission",
+			}, {
+				Attr:     "head_uuid",
+				Operator: "=",
+				Operand:  group.UUID,
+			}},
+		}
+		err = s.cfg.Client.RequestAndDecode(&ll, "GET", "/arvados/v1/links", nil, params)
+		c.Assert(err, IsNil)
+		for _, link := range ll.Items {
+			err = DeleteLink(s.cfg, link.UUID)
+			c.Assert(err, IsNil)
+		}
+		// Delete group->user links
+		params = arvados.ResourceListParams{
+			Filters: []arvados.Filter{{
+				Attr:     "link_class",
+				Operator: "=",
+				Operand:  "permission",
+			}, {
+				Attr:     "tail_uuid",
+				Operator: "=",
+				Operand:  group.UUID,
+			}},
+		}
+		s.cfg.Client.RequestAndDecode(&ll, "GET", "/arvados/v1/links", nil, params)
+		for _, link := range ll.Items {
+			err = DeleteLink(s.cfg, link.UUID)
+			c.Assert(err, IsNil)
+		}
+		// Delete group
+		err = s.cfg.Client.RequestAndDecode(&arvados.Group{}, "DELETE", "/arvados/v1/groups/"+group.UUID, nil, nil)
+		c.Assert(err, IsNil)
+	}
+}
+
 var _ = Suite(&TestSuite{})
 
 // MakeTempCVSFile creates a temp file with data as comma separated values
@@ -341,3 +400,30 @@ func (s *TestSuite) TestIgnoreNonexistantUsers(c *C) {
 	c.Assert(groupUUID, Not(Equals), "")
 	c.Assert(GroupMembershipExists(s.cfg.Client, activeUserUUID, groupUUID), Equals, true)
 }
+
+// Users listed on the file that don't exist on the system are ignored
+func (s *TestSuite) TestIgnoreEmptyFields(c *C) {
+	activeUserEmail := s.users[arvadostest.ActiveUserUUID].Email
+	activeUserUUID := s.users[arvadostest.ActiveUserUUID].UUID
+	// Confirm that group doesn't exist
+	groupUUID, err := RemoteGroupExists(s.cfg, "TestGroup4")
+	c.Assert(err, IsNil)
+	c.Assert(groupUUID, Equals, "")
+	// Create file & run command
+	data := [][]string{
+		{"", activeUserEmail}, // Empty field
+		{"TestGroup5", ""},    // Empty field
+		{"TestGroup4", activeUserEmail},
+	}
+	tmpfile, err := MakeTempCSVFile(data)
+	c.Assert(err, IsNil)
+	defer os.Remove(tmpfile.Name()) // clean up
+	s.cfg.Path = tmpfile.Name()
+	err = doMain(s.cfg)
+	c.Assert(err, IsNil)
+	// Confirm that memberships exist
+	groupUUID, err = RemoteGroupExists(s.cfg, "TestGroup4")
+	c.Assert(err, IsNil)
+	c.Assert(groupUUID, Not(Equals), "")
+	c.Assert(GroupMembershipExists(s.cfg.Client, activeUserUUID, groupUUID), Equals, true)
+}
-- 
2.39.5