From 7d1cf2ce5b0f7c95b6a63b8f243223aff419d2a0 Mon Sep 17 00:00:00 2001 From: Vaibhav Kamra Date: Mon, 26 Sep 2022 16:53:22 -0700 Subject: [PATCH] Use path based addressing to get OneDrive folder (#951) ## Description Addresses a follow-up from the folder lookup implementation where we were not iterating through all children (no pagination was implemented) when searching for a folder. Instead of implementing pagination, this uses OneDrive path based addressing to do a direct lookup for the folder. ## Type of change - [ ] :sunflower: Feature - [x] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Test - [ ] :computer: CI/Deployment - [ ] :hamster: Trivial/Minor ## Issue(s) * #875 ## Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [x] :green_heart: E2E --- src/internal/connector/onedrive/drive.go | 57 ++++++++++------ src/internal/connector/onedrive/item_test.go | 69 ++++++++++++-------- 2 files changed, 80 insertions(+), 46 deletions(-) diff --git a/src/internal/connector/onedrive/drive.go b/src/internal/connector/onedrive/drive.go index 8667a8273..974112650 100644 --- a/src/internal/connector/onedrive/drive.go +++ b/src/internal/connector/onedrive/drive.go @@ -4,9 +4,11 @@ import ( "context" "fmt" + "github.com/microsoftgraph/msgraph-sdk-go/drives/item/items" + "github.com/microsoftgraph/msgraph-sdk-go/drives/item/items/item" "github.com/microsoftgraph/msgraph-sdk-go/drives/item/root/delta" - "github.com/microsoftgraph/msgraph-sdk-go/me/drives/item/items" "github.com/microsoftgraph/msgraph-sdk-go/models" + "github.com/microsoftgraph/msgraph-sdk-go/models/odataerrors" "github.com/pkg/errors" "github.com/alcionai/corso/src/internal/connector/graph" @@ -21,6 +23,8 @@ const ( // graph response nextLinkKey = "@odata.nextLink" itemChildrenRawURLFmt = "https://graph.microsoft.com/v1.0/drives/%s/items/%s/children" + itemByPathRawURLFmt = "https://graph.microsoft.com/v1.0/drives/%s/items/%s:/%s" + itemNotFoundErrorCode = "itemNotFound" ) // Enumerates the drives for the specified user @@ -84,29 +88,44 @@ func collectItems( func getFolder(ctx context.Context, service graph.Service, driveID string, parentFolderID string, folderName string, ) (models.DriveItemable, error) { - children, err := service.Client().DrivesById(driveID).ItemsById(parentFolderID).Children().Get(ctx, nil) + // The `Children().Get()` API doesn't yet support $filter, so using that to find a folder + // will be sub-optimal. + // Instead, we leverage OneDrive path-based addressing - + // https://learn.microsoft.com/en-us/graph/onedrive-addressing-driveitems#path-based-addressing + // - which allows us to lookup an item by its path relative to the parent ID + rawURL := fmt.Sprintf(itemByPathRawURLFmt, driveID, parentFolderID, folderName) + + builder := item.NewDriveItemItemRequestBuilder(rawURL, service.Adapter()) + + foundItem, err := builder.Get(ctx, nil) if err != nil { - return nil, errors.Wrapf( - err, - "failed to get children. details: %s", + var oDataError *odataerrors.ODataError + if errors.As(err, &oDataError) && + oDataError.GetError() != nil && + oDataError.GetError().GetCode() != nil && + *oDataError.GetError().GetCode() == itemNotFoundErrorCode { + return nil, errors.WithStack(errFolderNotFound) + } + + return nil, errors.Wrapf(err, + "failed to get folder %s/%s. details: %s", + parentFolderID, + folderName, support.ConnectorStackErrorTrace(err), ) } - for _, item := range children.GetValue() { - if item.GetFolder() == nil || item.GetName() == nil || *item.GetName() != folderName { - continue - } - - return item, nil + // Check if the item found is a folder, fail the call if not + if foundItem.GetFolder() == nil { + return nil, errors.WithStack(errFolderNotFound) } - return nil, errors.WithStack(errFolderNotFound) + return foundItem, nil } // Create a new item in the specified folder func createItem(ctx context.Context, service graph.Service, driveID string, parentFolderID string, - item models.DriveItemable, + newItem models.DriveItemable, ) (models.DriveItemable, error) { // Graph SDK doesn't yet provide a POST method for `/children` so we set the `rawUrl` ourselves as recommended // here: https://github.com/microsoftgraph/msgraph-sdk-go/issues/155#issuecomment-1136254310 @@ -114,7 +133,7 @@ func createItem(ctx context.Context, service graph.Service, driveID string, pare builder := items.NewItemsRequestBuilder(rawURL, service.Adapter()) - newItem, err := builder.Post(ctx, item, nil) + newItem, err := builder.Post(ctx, newItem, nil) if err != nil { return nil, errors.Wrapf( err, @@ -128,14 +147,14 @@ func createItem(ctx context.Context, service graph.Service, driveID string, pare // newItem initializes a `models.DriveItemable` that can be used as input to `createItem` func newItem(name string, folder bool) models.DriveItemable { - item := models.NewDriveItem() - item.SetName(&name) + itemToCreate := models.NewDriveItem() + itemToCreate.SetName(&name) if folder { - item.SetFolder(models.NewFolder()) + itemToCreate.SetFolder(models.NewFolder()) } else { - item.SetFile(models.NewFile()) + itemToCreate.SetFile(models.NewFile()) } - return item + return itemToCreate } diff --git a/src/internal/connector/onedrive/item_test.go b/src/internal/connector/onedrive/item_test.go index 3e7f23e9e..19845634f 100644 --- a/src/internal/connector/onedrive/item_test.go +++ b/src/internal/connector/onedrive/item_test.go @@ -18,6 +18,8 @@ import ( type ItemIntegrationSuite struct { suite.Suite + user string + driveID string client *msgraphsdk.GraphServiceClient adapter *msgraphsdk.GraphRequestAdapter } @@ -58,6 +60,16 @@ func (suite *ItemIntegrationSuite) SetupSuite() { require.NoError(suite.T(), err) suite.client = msgraphsdk.NewGraphServiceClient(adapter) suite.adapter = adapter + + suite.user = tester.M365UserID(suite.T()) + + drives, err := drives(context.TODO(), suite, suite.user) + require.NoError(suite.T(), err) + // Test Requirement 1: Need a drive + require.Greaterf(suite.T(), len(drives), 0, "user %s does not have a drive", suite.user) + + // Pick the first drive + suite.driveID = *drives[0].GetId() } // TestItemReader is an integration test that makes a few assumptions @@ -67,15 +79,6 @@ func (suite *ItemIntegrationSuite) SetupSuite() { // The test checks these in below func (suite *ItemIntegrationSuite) TestItemReader() { ctx := context.TODO() - user := tester.M365UserID(suite.T()) - - drives, err := drives(ctx, suite, user) - require.NoError(suite.T(), err) - // Test Requirement 1: Need a drive - require.Greaterf(suite.T(), len(drives), 0, "user %s does not have a drive", user) - - // Pick the first drive - driveID := *drives[0].GetId() var driveItemID string // This item collector tries to find "a" drive item that is a file to test the reader function @@ -89,7 +92,7 @@ func (suite *ItemIntegrationSuite) TestItemReader() { return nil } - err = collectItems(ctx, suite, driveID, itemCollector) + err := collectItems(ctx, suite, suite.driveID, itemCollector) require.NoError(suite.T(), err) // Test Requirement 2: Need a file @@ -97,12 +100,13 @@ func (suite *ItemIntegrationSuite) TestItemReader() { suite.T(), driveItemID, "no file item found for user %s drive %s", - user, - driveID, + suite.user, + suite.driveID, ) // Read data for the file - itemInfo, itemData, err := driveItemReader(ctx, suite, driveID, driveItemID) + + itemInfo, itemData, err := driveItemReader(ctx, suite, suite.driveID, driveItemID) require.NoError(suite.T(), err) require.NotNil(suite.T(), itemInfo) require.NotEmpty(suite.T(), itemInfo.ItemName) @@ -119,27 +123,18 @@ func (suite *ItemIntegrationSuite) TestItemReader() { // testitem_ item and writes data to it func (suite *ItemIntegrationSuite) TestItemWriter() { ctx := context.TODO() - user := tester.M365UserID(suite.T()) - drives, err := drives(ctx, suite, user) - require.NoError(suite.T(), err) - // Test Requirement 1: Need a drive - require.Greaterf(suite.T(), len(drives), 0, "user %s does not have a drive", user) - - // Pick the first drive - driveID := *drives[0].GetId() - - root, err := suite.Client().DrivesById(driveID).Root().Get(ctx, nil) + root, err := suite.Client().DrivesById(suite.driveID).Root().Get(ctx, nil) require.NoError(suite.T(), err) // Test Requirement 2: "Test Folder" should exist - folder, err := getFolder(ctx, suite, driveID, *root.GetId(), "Test Folder") + folder, err := getFolder(ctx, suite, suite.driveID, *root.GetId(), "Test Folder") require.NoError(suite.T(), err) newFolderName := "testfolder_" + time.Now().Format("2006-01-02T15-04-05") suite.T().Logf("Test will create folder %s", newFolderName) - newFolder, err := createItem(ctx, suite, driveID, *folder.GetId(), newItem(newFolderName, true)) + newFolder, err := createItem(ctx, suite, suite.driveID, *folder.GetId(), newItem(newFolderName, true)) require.NoError(suite.T(), err) require.NotNil(suite.T(), newFolder.GetId()) @@ -147,15 +142,20 @@ func (suite *ItemIntegrationSuite) TestItemWriter() { newItemName := "testItem_" + time.Now().Format("2006-01-02T15-04-05") suite.T().Logf("Test will create item %s", newItemName) - newItem, err := createItem(ctx, suite, driveID, *newFolder.GetId(), newItem(newItemName, false)) + newItem, err := createItem(ctx, suite, suite.driveID, *newFolder.GetId(), newItem(newItemName, false)) require.NoError(suite.T(), err) require.NotNil(suite.T(), newItem.GetId()) + // HACK: Leveraging this to test getFolder behavior for a file. `getFolder()` on the + // newly created item should fail because it's a file not a folder + _, err = getFolder(ctx, suite, suite.driveID, *newFolder.GetId(), newItemName) + require.ErrorIs(suite.T(), err, errFolderNotFound) + // Initialize a 100KB mockDataProvider td, writeSize := mockDataReader(int64(100 * 1024)) - w, err := driveItemWriter(ctx, suite, driveID, *newItem.GetId(), writeSize) + w, err := driveItemWriter(ctx, suite, suite.driveID, *newItem.GetId(), writeSize) require.NoError(suite.T(), err) // Using a 32 KB buffer for the copy allows us to validate the @@ -173,3 +173,18 @@ func mockDataReader(size int64) (io.Reader, int64) { data := bytes.Repeat([]byte("D"), int(size)) return bytes.NewReader(data), size } + +func (suite *ItemIntegrationSuite) TestDriveGetFolder() { + ctx := context.TODO() + + root, err := suite.Client().DrivesById(suite.driveID).Root().Get(ctx, nil) + require.NoError(suite.T(), err) + + // Lookup a folder that doesn't exist + _, err = getFolder(ctx, suite, suite.driveID, *root.GetId(), "FolderDoesNotExist") + require.ErrorIs(suite.T(), err, errFolderNotFound) + + // Lookup a folder that does exist + _, err = getFolder(ctx, suite, suite.driveID, *root.GetId(), "") + require.NoError(suite.T(), err) +}