From 9c1d04e3f11c690de4a162ce74e8f0b98945b062 Mon Sep 17 00:00:00 2001
From: Vaibhav Kamra <vkamra@alcion.ai>
Date: Tue, 7 Mar 2023 20:05:29 -0800
Subject: [PATCH] Remove Graph call that could cause throttling issues (#2727)

The logic added in [2632](https://github.com/alcionai/corso/pull/2632) introduces an extra Graph call for
every item that is being backed up - which can lead to throttling errors.

Removing this till we figure out the right solution.

This commit also adds some extra logging to help debug throttling errors.

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

- [ ] :white_check_mark: Yes, it's included
- [ ] :clock1: Yes, but in a later PR
- [x] :no_entry: No

#### Type of change

<!--- Please check the type of change your PR introduces: --->
- [ ] :sunflower: Feature
- [x] :bug: Bugfix
- [ ] :world_map: Documentation
- [ ] :robot: Test
- [ ] :computer: CI/Deployment
- [ ] :broom: Tech Debt/Cleanup

#### Issue(s)

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

#### Test Plan

<!-- How will this be tested prior to merging.-->
- [x] :muscle: Manual
- [ ] :zap: Unit test
- [ ] :green_heart: E2E
---
 src/internal/connector/graph/service.go       | 10 +++-
 src/internal/connector/onedrive/collection.go | 16 ++++---
 src/internal/connector/onedrive/item.go       | 48 +++++++++----------
 3 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/src/internal/connector/graph/service.go b/src/internal/connector/graph/service.go
index 1c189a6b3..14050decd 100644
--- a/src/internal/connector/graph/service.go
+++ b/src/internal/connector/graph/service.go
@@ -29,6 +29,9 @@ const (
 	defaultMaxRetries       = 3
 	defaultDelay            = 3 * time.Second
 	absoluteMaxDelaySeconds = 180
+	rateLimitHeader         = "RateLimit-Limit"
+	rateRemainingHeader     = "RateLimit-Remaining"
+	rateResetHeader         = "RateLimit-Reset"
 )
 
 // AllMetadataFileNames produces the standard set of filenames used to store graph
@@ -312,7 +315,12 @@ func (handler *LoggingMiddleware) Intercept(
 	} else {
 		// special case for supportability: log all throttling cases.
 		if resp.StatusCode == http.StatusTooManyRequests {
-			logger.Ctx(ctx).Infow("graph api throttling", "method", req.Method, "url", req.URL)
+			logger.Ctx(ctx).Infow("graph api throttling",
+				"method", req.Method,
+				"url", req.URL,
+				"limit", resp.Header.Get(rateLimitHeader),
+				"remaining", resp.Header.Get(rateRemainingHeader),
+				"reset", resp.Header.Get(rateResetHeader))
 		} else if resp.StatusCode == http.StatusBadRequest {
 			respDump, _ := httputil.DumpResponse(resp, true)
 			logger.Ctx(ctx).Infow(
diff --git a/src/internal/connector/onedrive/collection.go b/src/internal/connector/onedrive/collection.go
index 8c05dc2e7..ca7a5d64a 100644
--- a/src/internal/connector/onedrive/collection.go
+++ b/src/internal/connector/onedrive/collection.go
@@ -334,13 +334,17 @@ func (oc *Collection) populateItems(ctx context.Context, errs *fault.Bus) {
 				"backup_item_size", itemSize,
 			)
 
-			pr, err := fetchParentReference(ctx, oc.service, item.GetParentReference())
-			if err != nil {
-				el.AddRecoverable(clues.Wrap(err, "getting parent reference").Label(fault.LabelForceNoBackupCreation))
-				return
-			}
+			// TODO: Removing the logic below because it introduces an extra Graph API call for
+			// every item being backed up. This can lead to throttling errors.
+			//
+			// pr, err := fetchParentReference(ctx, oc.service, item.GetParentReference())
+			// if err != nil {
+			// 	el.AddRecoverable(clues.Wrap(err, "getting parent reference").Label(fault.LabelForceNoBackupCreation))
+			// 	return
+			// }
+
+			// item.SetParentReference(pr)
 
-			item.SetParentReference(pr)
 			isFile := item.GetFile() != nil
 
 			if isFile {
diff --git a/src/internal/connector/onedrive/item.go b/src/internal/connector/onedrive/item.go
index 59ce4849b..dacd26af6 100644
--- a/src/internal/connector/onedrive/item.go
+++ b/src/internal/connector/onedrive/item.go
@@ -400,33 +400,33 @@ func constructWebURL(adtl map[string]any) string {
 	return url
 }
 
-func fetchParentReference(
-	ctx context.Context,
-	service graph.Servicer,
-	orig models.ItemReferenceable,
-) (models.ItemReferenceable, error) {
-	if orig == nil || service == nil || ptr.Val(orig.GetName()) != "" {
-		return orig, nil
-	}
+// func fetchParentReference(
+// 	ctx context.Context,
+// 	service graph.Servicer,
+// 	orig models.ItemReferenceable,
+// ) (models.ItemReferenceable, error) {
+// 	if orig == nil || service == nil || ptr.Val(orig.GetName()) != "" {
+// 		return orig, nil
+// 	}
 
-	options := &msdrives.DriveItemRequestBuilderGetRequestConfiguration{
-		QueryParameters: &msdrives.DriveItemRequestBuilderGetQueryParameters{
-			Select: []string{"name"},
-		},
-	}
+// 	options := &msdrives.DriveItemRequestBuilderGetRequestConfiguration{
+// 		QueryParameters: &msdrives.DriveItemRequestBuilderGetQueryParameters{
+// 			Select: []string{"name"},
+// 		},
+// 	}
 
-	driveID := ptr.Val(orig.GetDriveId())
+// 	driveID := ptr.Val(orig.GetDriveId())
 
-	if driveID == "" {
-		return orig, nil
-	}
+// 	if driveID == "" {
+// 		return orig, nil
+// 	}
 
-	drive, err := service.Client().DrivesById(driveID).Get(ctx, options)
-	if err != nil {
-		return nil, graph.Stack(ctx, err)
-	}
+// 	drive, err := service.Client().DrivesById(driveID).Get(ctx, options)
+// 	if err != nil {
+// 		return nil, graph.Stack(ctx, err)
+// 	}
 
-	orig.SetName(drive.GetName())
+// 	orig.SetName(drive.GetName())
 
-	return orig, nil
-}
+// 	return orig, nil
+// }