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 <!--- Please check the type of change your PR introduces: ---> - [ ] 🌻 Feature - [x] 🐛 Bugfix - [ ] 🗺️ Documentation - [ ] 🤖 Test - [ ] 💻 CI/Deployment - [ ] 🐹 Trivial/Minor ## Issue(s) * #875 ## Test Plan <!-- How will this be tested prior to merging.--> - [ ] 💪 Manual - [x] ⚡ Unit test - [x] 💚 E2E
This commit is contained in:
parent
984df8fcfb
commit
7d1cf2ce5b
@ -4,9 +4,11 @@ import (
|
|||||||
"context"
|
"context"
|
||||||
"fmt"
|
"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/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"
|
||||||
|
"github.com/microsoftgraph/msgraph-sdk-go/models/odataerrors"
|
||||||
"github.com/pkg/errors"
|
"github.com/pkg/errors"
|
||||||
|
|
||||||
"github.com/alcionai/corso/src/internal/connector/graph"
|
"github.com/alcionai/corso/src/internal/connector/graph"
|
||||||
@ -21,6 +23,8 @@ const (
|
|||||||
// graph response
|
// graph response
|
||||||
nextLinkKey = "@odata.nextLink"
|
nextLinkKey = "@odata.nextLink"
|
||||||
itemChildrenRawURLFmt = "https://graph.microsoft.com/v1.0/drives/%s/items/%s/children"
|
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
|
// 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,
|
func getFolder(ctx context.Context, service graph.Service, driveID string, parentFolderID string,
|
||||||
folderName string,
|
folderName string,
|
||||||
) (models.DriveItemable, error) {
|
) (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 {
|
if err != nil {
|
||||||
return nil, errors.Wrapf(
|
var oDataError *odataerrors.ODataError
|
||||||
err,
|
if errors.As(err, &oDataError) &&
|
||||||
"failed to get children. details: %s",
|
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),
|
support.ConnectorStackErrorTrace(err),
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
for _, item := range children.GetValue() {
|
// Check if the item found is a folder, fail the call if not
|
||||||
if item.GetFolder() == nil || item.GetName() == nil || *item.GetName() != folderName {
|
if foundItem.GetFolder() == nil {
|
||||||
continue
|
return nil, errors.WithStack(errFolderNotFound)
|
||||||
}
|
|
||||||
|
|
||||||
return item, nil
|
|
||||||
}
|
}
|
||||||
|
|
||||||
return nil, errors.WithStack(errFolderNotFound)
|
return foundItem, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// Create a new item in the specified folder
|
// Create a new item in the specified folder
|
||||||
func createItem(ctx context.Context, service graph.Service, driveID string, parentFolderID string,
|
func createItem(ctx context.Context, service graph.Service, driveID string, parentFolderID string,
|
||||||
item models.DriveItemable,
|
newItem models.DriveItemable,
|
||||||
) (models.DriveItemable, error) {
|
) (models.DriveItemable, error) {
|
||||||
// Graph SDK doesn't yet provide a POST method for `/children` so we set the `rawUrl` ourselves as recommended
|
// 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
|
// 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())
|
builder := items.NewItemsRequestBuilder(rawURL, service.Adapter())
|
||||||
|
|
||||||
newItem, err := builder.Post(ctx, item, nil)
|
newItem, err := builder.Post(ctx, newItem, nil)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, errors.Wrapf(
|
return nil, errors.Wrapf(
|
||||||
err,
|
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`
|
// newItem initializes a `models.DriveItemable` that can be used as input to `createItem`
|
||||||
func newItem(name string, folder bool) models.DriveItemable {
|
func newItem(name string, folder bool) models.DriveItemable {
|
||||||
item := models.NewDriveItem()
|
itemToCreate := models.NewDriveItem()
|
||||||
item.SetName(&name)
|
itemToCreate.SetName(&name)
|
||||||
|
|
||||||
if folder {
|
if folder {
|
||||||
item.SetFolder(models.NewFolder())
|
itemToCreate.SetFolder(models.NewFolder())
|
||||||
} else {
|
} else {
|
||||||
item.SetFile(models.NewFile())
|
itemToCreate.SetFile(models.NewFile())
|
||||||
}
|
}
|
||||||
|
|
||||||
return item
|
return itemToCreate
|
||||||
}
|
}
|
||||||
|
|||||||
@ -18,6 +18,8 @@ import (
|
|||||||
|
|
||||||
type ItemIntegrationSuite struct {
|
type ItemIntegrationSuite struct {
|
||||||
suite.Suite
|
suite.Suite
|
||||||
|
user string
|
||||||
|
driveID string
|
||||||
client *msgraphsdk.GraphServiceClient
|
client *msgraphsdk.GraphServiceClient
|
||||||
adapter *msgraphsdk.GraphRequestAdapter
|
adapter *msgraphsdk.GraphRequestAdapter
|
||||||
}
|
}
|
||||||
@ -58,6 +60,16 @@ func (suite *ItemIntegrationSuite) SetupSuite() {
|
|||||||
require.NoError(suite.T(), err)
|
require.NoError(suite.T(), err)
|
||||||
suite.client = msgraphsdk.NewGraphServiceClient(adapter)
|
suite.client = msgraphsdk.NewGraphServiceClient(adapter)
|
||||||
suite.adapter = 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
|
// TestItemReader is an integration test that makes a few assumptions
|
||||||
@ -67,15 +79,6 @@ func (suite *ItemIntegrationSuite) SetupSuite() {
|
|||||||
// The test checks these in below
|
// The test checks these in below
|
||||||
func (suite *ItemIntegrationSuite) TestItemReader() {
|
func (suite *ItemIntegrationSuite) TestItemReader() {
|
||||||
ctx := context.TODO()
|
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
|
var driveItemID string
|
||||||
// This item collector tries to find "a" drive item that is a file to test the reader function
|
// 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
|
return nil
|
||||||
}
|
}
|
||||||
err = collectItems(ctx, suite, driveID, itemCollector)
|
err := collectItems(ctx, suite, suite.driveID, itemCollector)
|
||||||
require.NoError(suite.T(), err)
|
require.NoError(suite.T(), err)
|
||||||
|
|
||||||
// Test Requirement 2: Need a file
|
// Test Requirement 2: Need a file
|
||||||
@ -97,12 +100,13 @@ func (suite *ItemIntegrationSuite) TestItemReader() {
|
|||||||
suite.T(),
|
suite.T(),
|
||||||
driveItemID,
|
driveItemID,
|
||||||
"no file item found for user %s drive %s",
|
"no file item found for user %s drive %s",
|
||||||
user,
|
suite.user,
|
||||||
driveID,
|
suite.driveID,
|
||||||
)
|
)
|
||||||
|
|
||||||
// Read data for the file
|
// 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.NoError(suite.T(), err)
|
||||||
require.NotNil(suite.T(), itemInfo)
|
require.NotNil(suite.T(), itemInfo)
|
||||||
require.NotEmpty(suite.T(), itemInfo.ItemName)
|
require.NotEmpty(suite.T(), itemInfo.ItemName)
|
||||||
@ -119,27 +123,18 @@ func (suite *ItemIntegrationSuite) TestItemReader() {
|
|||||||
// testitem_<timestamp> item and writes data to it
|
// testitem_<timestamp> item and writes data to it
|
||||||
func (suite *ItemIntegrationSuite) TestItemWriter() {
|
func (suite *ItemIntegrationSuite) TestItemWriter() {
|
||||||
ctx := context.TODO()
|
ctx := context.TODO()
|
||||||
user := tester.M365UserID(suite.T())
|
|
||||||
|
|
||||||
drives, err := drives(ctx, suite, user)
|
root, err := suite.Client().DrivesById(suite.driveID).Root().Get(ctx, nil)
|
||||||
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)
|
|
||||||
require.NoError(suite.T(), err)
|
require.NoError(suite.T(), err)
|
||||||
|
|
||||||
// Test Requirement 2: "Test Folder" should exist
|
// 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)
|
require.NoError(suite.T(), err)
|
||||||
|
|
||||||
newFolderName := "testfolder_" + time.Now().Format("2006-01-02T15-04-05")
|
newFolderName := "testfolder_" + time.Now().Format("2006-01-02T15-04-05")
|
||||||
suite.T().Logf("Test will create folder %s", newFolderName)
|
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.NoError(suite.T(), err)
|
||||||
|
|
||||||
require.NotNil(suite.T(), newFolder.GetId())
|
require.NotNil(suite.T(), newFolder.GetId())
|
||||||
@ -147,15 +142,20 @@ func (suite *ItemIntegrationSuite) TestItemWriter() {
|
|||||||
newItemName := "testItem_" + time.Now().Format("2006-01-02T15-04-05")
|
newItemName := "testItem_" + time.Now().Format("2006-01-02T15-04-05")
|
||||||
suite.T().Logf("Test will create item %s", newItemName)
|
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.NoError(suite.T(), err)
|
||||||
|
|
||||||
require.NotNil(suite.T(), newItem.GetId())
|
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
|
// Initialize a 100KB mockDataProvider
|
||||||
td, writeSize := mockDataReader(int64(100 * 1024))
|
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)
|
require.NoError(suite.T(), err)
|
||||||
|
|
||||||
// Using a 32 KB buffer for the copy allows us to validate the
|
// 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))
|
data := bytes.Repeat([]byte("D"), int(size))
|
||||||
return bytes.NewReader(data), 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)
|
||||||
|
}
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user