Refactor downloadItem (#3534)

This refactor is being done to allow calling `downloadFile` with an URL obtained from URL cache. 


---

#### Does this PR need a docs update or release note?

- [ ] ✅ Yes, it's included
- [ ] 🕐 Yes, but in a later PR
- [x] ⛔ No

#### Type of change

<!--- Please check the type of change your PR introduces: --->
- [ ] 🌻 Feature
- [ ] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [x] 🧹 Tech Debt/Cleanup

#### Issue(s)

<!-- Can reference multiple issues. Use one of the following "magic words" - "closes, fixes" to auto-close the Github issue. -->
* #<issue>

#### Test Plan

<!-- How will this be tested prior to merging.-->
- [ ] 💪 Manual
- [x] ⚡ Unit test
- [ ] 💚 E2E

src/internal/connector/onedrive/item.go

@@ -27,9 +27,14 @@ func downloadItem(
 	ag api.Getter,
 	item models.DriveItemable,
 ) (io.ReadCloser, error) {
+	if item == nil {
+		return nil, clues.New("nil item")
+	}
+
 	var (
 		rc     io.ReadCloser
 		isFile = item.GetFile() != nil
+		err    error
 	)
 
 	if isFile {
@@ -45,31 +50,42 @@ func downloadItem(
 			}
 		}
 
-		if len(url) == 0 {
-			return nil, clues.New("extracting file url")
-		}
-
-		resp, err := ag.Get(ctx, url, nil)
+		rc, err = downloadFile(ctx, ag, url)
 		if err != nil {
-			return nil, clues.Wrap(err, "getting item")
+			return nil, clues.Stack(err)
 		}
+	}
 
-		if graph.IsMalwareResp(ctx, resp) {
-			return nil, clues.New("malware detected").Label(graph.LabelsMalware)
-		}
+	return rc, nil
+}
 
-		if (resp.StatusCode / 100) != 2 {
-			// upstream error checks can compare the status with
-			// clues.HasLabel(err, graph.LabelStatus(http.KnownStatusCode))
-			return nil, clues.
-				Wrap(clues.New(resp.Status), "non-2xx http response").
-				Label(graph.LabelStatus(resp.StatusCode))
-		}
+func downloadFile(
+	ctx context.Context,
+	ag api.Getter,
+	url string,
+) (io.ReadCloser, error) {
+	if len(url) == 0 {
+		return nil, clues.New("empty file url")
+	}
 
-		rc = resp.Body
+	resp, err := ag.Get(ctx, url, nil)
+	if err != nil {
+		return nil, clues.Wrap(err, "getting file")
 	}
 
-	return rc, nil
+	if graph.IsMalwareResp(ctx, resp) {
+		return nil, clues.New("malware detected").Label(graph.LabelsMalware)
+	}
+
+	if (resp.StatusCode / 100) != 2 {
+		// upstream error checks can compare the status with
+		// clues.HasLabel(err, graph.LabelStatus(http.KnownStatusCode))
+		return nil, clues.
+			Wrap(clues.New(resp.Status), "non-2xx http response").
+			Label(graph.LabelStatus(resp.StatusCode))
+	}
+
+	return resp.Body, nil
 }
 
 func downloadItemMeta(

src/internal/connector/onedrive/item_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"io"
+	"net/http"
 	"testing"
 
 	"github.com/alcionai/clues"
@@ -256,3 +257,176 @@ func (suite *ItemIntegrationSuite) TestDriveGetFolder() {
 		})
 	}
 }
+
+// Unit tests
+
+type mockGetter struct {
+	GetFunc func(ctx context.Context, url string) (*http.Response, error)
+}
+
+func (m mockGetter) Get(
+	ctx context.Context,
+	url string,
+	headers map[string]string,
+) (*http.Response, error) {
+	return m.GetFunc(ctx, url)
+}
+
+type ItemUnitTestSuite struct {
+	tester.Suite
+}
+
+func TestItemUnitTestSuite(t *testing.T) {
+	suite.Run(t, &ItemUnitTestSuite{Suite: tester.NewUnitSuite(t)})
+}
+
+func (suite *ItemUnitTestSuite) TestDownloadItem() {
+	testRc := io.NopCloser(bytes.NewReader([]byte("test")))
+	url := "https://example.com"
+
+	table := []struct {
+		name          string
+		itemFunc      func() models.DriveItemable
+		GetFunc       func(ctx context.Context, url string) (*http.Response, error)
+		errorExpected require.ErrorAssertionFunc
+		rcExpected    require.ValueAssertionFunc
+		label         string
+	}{
+		{
+			name: "nil item",
+			itemFunc: func() models.DriveItemable {
+				return nil
+			},
+			GetFunc: func(ctx context.Context, url string) (*http.Response, error) {
+				return nil, nil
+			},
+			errorExpected: require.Error,
+			rcExpected:    require.Nil,
+		},
+		{
+			name: "success",
+			itemFunc: func() models.DriveItemable {
+				di := newItem("test", false)
+				di.SetAdditionalData(map[string]interface{}{
+					"@microsoft.graph.downloadUrl": url,
+				})
+
+				return di
+			},
+			GetFunc: func(ctx context.Context, url string) (*http.Response, error) {
+				return &http.Response{
+					StatusCode: http.StatusOK,
+					Body:       testRc,
+				}, nil
+			},
+			errorExpected: require.NoError,
+			rcExpected:    require.NotNil,
+		},
+		{
+			name: "success, content url set instead of download url",
+			itemFunc: func() models.DriveItemable {
+				di := newItem("test", false)
+				di.SetAdditionalData(map[string]interface{}{
+					"@content.downloadUrl": url,
+				})
+
+				return di
+			},
+			GetFunc: func(ctx context.Context, url string) (*http.Response, error) {
+				return &http.Response{
+					StatusCode: http.StatusOK,
+					Body:       testRc,
+				}, nil
+			},
+			errorExpected: require.NoError,
+			rcExpected:    require.NotNil,
+		},
+		{
+			name: "api getter returns error",
+			itemFunc: func() models.DriveItemable {
+				di := newItem("test", false)
+				di.SetAdditionalData(map[string]interface{}{
+					"@microsoft.graph.downloadUrl": url,
+				})
+
+				return di
+			},
+			GetFunc: func(ctx context.Context, url string) (*http.Response, error) {
+				return nil, clues.New("test error")
+			},
+			errorExpected: require.Error,
+			rcExpected:    require.Nil,
+		},
+		{
+			name: "download url is empty",
+			itemFunc: func() models.DriveItemable {
+				di := newItem("test", false)
+				return di
+			},
+			GetFunc: func(ctx context.Context, url string) (*http.Response, error) {
+				return &http.Response{
+					StatusCode: http.StatusOK,
+					Body:       testRc,
+				}, nil
+			},
+			errorExpected: require.Error,
+			rcExpected:    require.Nil,
+		},
+		{
+			name: "malware",
+			itemFunc: func() models.DriveItemable {
+				di := newItem("test", false)
+				di.SetAdditionalData(map[string]interface{}{
+					"@microsoft.graph.downloadUrl": url,
+				})
+
+				return di
+			},
+			GetFunc: func(ctx context.Context, url string) (*http.Response, error) {
+				return &http.Response{
+					Header: http.Header{
+						"X-Virus-Infected": []string{"true"},
+					},
+					StatusCode: http.StatusOK,
+					Body:       testRc,
+				}, nil
+			},
+			errorExpected: require.Error,
+			rcExpected:    require.Nil,
+		},
+		{
+			name: "non-2xx http response",
+			itemFunc: func() models.DriveItemable {
+				di := newItem("test", false)
+				di.SetAdditionalData(map[string]interface{}{
+					"@microsoft.graph.downloadUrl": url,
+				})
+
+				return di
+			},
+			GetFunc: func(ctx context.Context, url string) (*http.Response, error) {
+				return &http.Response{
+					StatusCode: http.StatusNotFound,
+					Body:       nil,
+				}, nil
+			},
+			errorExpected: require.Error,
+			rcExpected:    require.Nil,
+		},
+	}
+
+	for _, test := range table {
+		suite.Run(test.name, func() {
+			t := suite.T()
+			ctx, flush := tester.NewContext(t)
+			defer flush()
+
+			mg := mockGetter{
+				GetFunc: test.GetFunc,
+			}
+			rc, err := downloadItem(ctx, mg, test.itemFunc())
+			test.errorExpected(t, err, clues.ToCore(err))
+			test.rcExpected(t, rc)
+		})
+	}
+}