From 7625219e14a33b64669b50e7f028991ffdce9740 Mon Sep 17 00:00:00 2001 From: ashmrtn <3891298+ashmrtn@users.noreply.github.com> Date: Wed, 11 Oct 2023 12:03:17 -0700 Subject: [PATCH] Refactor drive handler logic (#4477) Do a couple of high-level things with drive handlers: * pull out generic functionality into a base handler * use handler-specific implementations of AugmentItemInfo * remove old switch-based augmentItemInfo Overall goal is to reduce places we use service type comparisons to handle behavior differences --- #### 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 - [ ] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [x] :broom: Tech Debt/Cleanup #### Issue(s) * #4254 #### Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [x] :green_heart: E2E --- .../m365/collection/drive/collections_test.go | 16 ++- .../m365/collection/drive/group_handler.go | 41 ++++++- .../m365/collection/drive/handler_utils.go | 108 ++++-------------- .../collection/drive/item_collector_test.go | 8 +- .../m365/collection/drive/item_handler.go | 103 ++++++++++------- .../m365/collection/drive/library_handler.go | 99 ++++++++++------ 6 files changed, 210 insertions(+), 165 deletions(-) diff --git a/src/internal/m365/collection/drive/collections_test.go b/src/internal/m365/collection/drive/collections_test.go index bae8019a8..af801e07c 100644 --- a/src/internal/m365/collection/drive/collections_test.go +++ b/src/internal/m365/collection/drive/collections_test.go @@ -740,7 +740,13 @@ func (suite *OneDriveCollectionsUnitSuite) TestUpdateCollections() { maps.Copy(currPrevPaths, test.inputFolderMap) c := NewCollections( - &itemBackupHandler{api.Drives{}, user, test.scope}, + &itemBackupHandler{ + baseItemHandler: baseItemHandler{ + ac: api.Drives{}, + }, + userID: user, + scope: test.scope, + }, tenant, idname.NewProvider(user, user), nil, @@ -2462,7 +2468,13 @@ func (suite *OneDriveCollectionsUnitSuite) TestAddURLCacheToDriveCollections() { // Add a few collections for i := 0; i < collCount; i++ { coll, err := NewCollection( - &itemBackupHandler{api.Drives{}, "test-user", anyFolder}, + &itemBackupHandler{ + baseItemHandler: baseItemHandler{ + ac: api.Drives{}, + }, + userID: "test-user", + scope: anyFolder, + }, idname.NewProvider("", ""), nil, nil, diff --git a/src/internal/m365/collection/drive/group_handler.go b/src/internal/m365/collection/drive/group_handler.go index 21768c5c3..2b445e960 100644 --- a/src/internal/m365/collection/drive/group_handler.go +++ b/src/internal/m365/collection/drive/group_handler.go @@ -2,8 +2,12 @@ package drive import ( "github.com/alcionai/clues" + "github.com/microsoftgraph/msgraph-sdk-go/models" + "github.com/alcionai/corso/src/internal/common/idname" + "github.com/alcionai/corso/src/internal/common/ptr" odConsts "github.com/alcionai/corso/src/internal/m365/service/onedrive/consts" + "github.com/alcionai/corso/src/pkg/backup/details" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/selectors" "github.com/alcionai/corso/src/pkg/services/m365/api" @@ -24,7 +28,9 @@ func NewGroupBackupHandler( ) groupBackupHandler { return groupBackupHandler{ libraryBackupHandler{ - ac: ac, + baseLibraryHandler: baseLibraryHandler{ + ac: ac, + }, siteID: siteID, // Not adding scope here. Anything that needs scope has to // be from group handler @@ -96,6 +102,39 @@ func (h groupBackupHandler) SitePathPrefix(tenantID string) (path.Path, error) { h.siteID) } +func (h groupBackupHandler) AugmentItemInfo( + dii details.ItemInfo, + resource idname.Provider, + item models.DriveItemable, + size int64, + parentPath *path.Builder, +) details.ItemInfo { + var pps string + + if parentPath != nil { + pps = parentPath.String() + } + + driveName, driveID := getItemDriveInfo(item) + + dii.Extension = &details.ExtensionData{} + dii.Groups = &details.GroupsInfo{ + Created: ptr.Val(item.GetCreatedDateTime()), + DriveID: driveID, + DriveName: driveName, + ItemName: ptr.Val(item.GetName()), + ItemType: details.SharePointLibrary, + Modified: ptr.Val(item.GetLastModifiedDateTime()), + Owner: getItemCreator(item), + ParentPath: pps, + SiteID: resource.ID(), + Size: size, + WebURL: resource.Name(), + } + + return dii +} + func (h groupBackupHandler) IsAllPass() bool { return h.scope.IsAny(selectors.GroupsLibraryFolder) } diff --git a/src/internal/m365/collection/drive/handler_utils.go b/src/internal/m365/collection/drive/handler_utils.go index d637d1a8a..dd1e415b4 100644 --- a/src/internal/m365/collection/drive/handler_utils.go +++ b/src/internal/m365/collection/drive/handler_utils.go @@ -5,96 +5,38 @@ import ( "github.com/microsoftgraph/msgraph-sdk-go/models" - "github.com/alcionai/corso/src/internal/common/idname" "github.com/alcionai/corso/src/internal/common/ptr" - "github.com/alcionai/corso/src/pkg/backup/details" - "github.com/alcionai/corso/src/pkg/path" ) -func augmentItemInfo( - dii details.ItemInfo, - resource idname.Provider, - service path.ServiceType, - item models.DriveItemable, - size int64, - parentPath *path.Builder, -) details.ItemInfo { - var driveName, driveID, creatorEmail string - - // TODO: we rely on this info for details/restore lookups, - // so if it's nil we have an issue, and will need an alternative - // way to source the data. - - if item.GetCreatedBy() != nil && item.GetCreatedBy().GetUser() != nil { - // User is sometimes not available when created via some - // external applications (like backup/restore solutions) - additionalData := item.GetCreatedBy().GetUser().GetAdditionalData() - - ed, ok := additionalData["email"] - if !ok { - ed = additionalData["displayName"] - } - - if ed != nil { - creatorEmail = *ed.(*string) - } +func getItemCreator(item models.DriveItemable) string { + if item.GetCreatedBy() == nil || item.GetCreatedBy().GetUser() == nil { + return "" } - if item.GetParentReference() != nil { - driveID = ptr.Val(item.GetParentReference().GetDriveId()) - driveName = strings.TrimSpace(ptr.Val(item.GetParentReference().GetName())) + // User is sometimes not available when created via some + // external applications (like backup/restore solutions) + additionalData := item.GetCreatedBy().GetUser().GetAdditionalData() + + ed, ok := additionalData["email"] + if !ok { + ed = additionalData["displayName"] } - var pps string - if parentPath != nil { - pps = parentPath.String() + if ed == nil { + return "" } - switch service { - case path.OneDriveService: - dii.OneDrive = &details.OneDriveInfo{ - Created: ptr.Val(item.GetCreatedDateTime()), - DriveID: driveID, - DriveName: driveName, - ItemName: ptr.Val(item.GetName()), - ItemType: details.OneDriveItem, - Modified: ptr.Val(item.GetLastModifiedDateTime()), - Owner: creatorEmail, - ParentPath: pps, - Size: size, - } - case path.SharePointService: - dii.SharePoint = &details.SharePointInfo{ - Created: ptr.Val(item.GetCreatedDateTime()), - DriveID: driveID, - DriveName: driveName, - ItemName: ptr.Val(item.GetName()), - ItemType: details.SharePointLibrary, - Modified: ptr.Val(item.GetLastModifiedDateTime()), - Owner: creatorEmail, - ParentPath: pps, - SiteID: resource.ID(), - Size: size, - WebURL: resource.Name(), - } - - case path.GroupsService: - dii.Groups = &details.GroupsInfo{ - Created: ptr.Val(item.GetCreatedDateTime()), - DriveID: driveID, - DriveName: driveName, - ItemName: ptr.Val(item.GetName()), - ItemType: details.SharePointLibrary, - Modified: ptr.Val(item.GetLastModifiedDateTime()), - Owner: creatorEmail, - ParentPath: pps, - SiteID: resource.ID(), - Size: size, - WebURL: resource.Name(), - } - } - - dii.Extension = &details.ExtensionData{} - - return dii + // TODO(ashmrtn): Replace with str package with fallbacks. + return *ed.(*string) +} + +func getItemDriveInfo(item models.DriveItemable) (string, string) { + if item.GetParentReference() == nil { + return "", "" + } + + driveName := strings.TrimSpace(ptr.Val(item.GetParentReference().GetName())) + driveID := ptr.Val(item.GetParentReference().GetDriveId()) + + return driveName, driveID } diff --git a/src/internal/m365/collection/drive/item_collector_test.go b/src/internal/m365/collection/drive/item_collector_test.go index b6f32a5bc..762e5c10f 100644 --- a/src/internal/m365/collection/drive/item_collector_test.go +++ b/src/internal/m365/collection/drive/item_collector_test.go @@ -266,7 +266,13 @@ func (suite *OneDriveIntgSuite) TestOneDriveNewCollections() { ) colls := NewCollections( - &itemBackupHandler{suite.ac.Drives(), test.user, scope}, + &itemBackupHandler{ + baseItemHandler: baseItemHandler{ + ac: suite.ac.Drives(), + }, + userID: test.user, + scope: scope, + }, creds.AzureTenantID, idname.NewProvider(test.user, test.user), service.updateStatus, diff --git a/src/internal/m365/collection/drive/item_handler.go b/src/internal/m365/collection/drive/item_handler.go index 4804db187..c89062dd7 100644 --- a/src/internal/m365/collection/drive/item_handler.go +++ b/src/internal/m365/collection/drive/item_handler.go @@ -9,6 +9,7 @@ import ( "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/alcionai/corso/src/internal/common/idname" + "github.com/alcionai/corso/src/internal/common/ptr" odConsts "github.com/alcionai/corso/src/internal/m365/service/onedrive/consts" "github.com/alcionai/corso/src/pkg/backup/details" "github.com/alcionai/corso/src/pkg/control" @@ -21,16 +22,69 @@ import ( // backup // --------------------------------------------------------------------------- +type baseItemHandler struct { + ac api.Drives +} + +func (h baseItemHandler) NewDrivePager( + resourceOwner string, + fields []string, +) api.Pager[models.Driveable] { + return h.ac.NewUserDrivePager(resourceOwner, fields) +} + +// AugmentItemInfo will populate a details.OneDriveInfo struct +// with properties from the drive item. ItemSize is specified +// separately for restore processes because the local itemable +// doesn't have its size value updated as a side effect of creation, +// and kiota drops any SetSize update. +func (h baseItemHandler) AugmentItemInfo( + dii details.ItemInfo, + resource idname.Provider, + item models.DriveItemable, + size int64, + parentPath *path.Builder, +) details.ItemInfo { + var pps string + + if parentPath != nil { + pps = parentPath.String() + } + + driveName, driveID := getItemDriveInfo(item) + + dii.Extension = &details.ExtensionData{} + dii.OneDrive = &details.OneDriveInfo{ + Created: ptr.Val(item.GetCreatedDateTime()), + DriveID: driveID, + DriveName: driveName, + ItemName: ptr.Val(item.GetName()), + ItemType: details.OneDriveItem, + Modified: ptr.Val(item.GetLastModifiedDateTime()), + Owner: getItemCreator(item), + ParentPath: pps, + Size: size, + } + + return dii +} + var _ BackupHandler = &itemBackupHandler{} type itemBackupHandler struct { - ac api.Drives + baseItemHandler userID string scope selectors.OneDriveScope } func NewItemBackupHandler(ac api.Drives, userID string, scope selectors.OneDriveScope) *itemBackupHandler { - return &itemBackupHandler{ac, userID, scope} + return &itemBackupHandler{ + baseItemHandler: baseItemHandler{ + ac: ac, + }, + userID: userID, + scope: scope, + } } func (h itemBackupHandler) Get( @@ -82,22 +136,6 @@ func (h itemBackupHandler) ServiceCat() (path.ServiceType, path.CategoryType) { return path.OneDriveService, path.FilesCategory } -func (h itemBackupHandler) NewDrivePager( - resourceOwner string, fields []string, -) api.Pager[models.Driveable] { - return h.ac.NewUserDrivePager(resourceOwner, fields) -} - -func (h itemBackupHandler) AugmentItemInfo( - dii details.ItemInfo, - resource idname.Provider, - item models.DriveItemable, - size int64, - parentPath *path.Builder, -) details.ItemInfo { - return augmentItemInfo(dii, resource, path.OneDriveService, item, size, parentPath) -} - func (h itemBackupHandler) FormatDisplayPath( _ string, // drive name not displayed for onedrive pb *path.Builder, @@ -149,11 +187,15 @@ func (h itemBackupHandler) EnumerateDriveItemsDelta( var _ RestoreHandler = &itemRestoreHandler{} type itemRestoreHandler struct { - ac api.Drives + baseItemHandler } func NewRestoreHandler(ac api.Client) *itemRestoreHandler { - return &itemRestoreHandler{ac.Drives()} + return &itemRestoreHandler{ + baseItemHandler: baseItemHandler{ + ac: ac.Drives(), + }, + } } func (h itemRestoreHandler) PostDrive( @@ -163,27 +205,6 @@ func (h itemRestoreHandler) PostDrive( return nil, clues.New("creating drives in oneDrive is not supported") } -func (h itemRestoreHandler) NewDrivePager( - resourceOwner string, fields []string, -) api.Pager[models.Driveable] { - return h.ac.NewUserDrivePager(resourceOwner, fields) -} - -// AugmentItemInfo will populate a details.OneDriveInfo struct -// with properties from the drive item. ItemSize is specified -// separately for restore processes because the local itemable -// doesn't have its size value updated as a side effect of creation, -// and kiota drops any SetSize update. -func (h itemRestoreHandler) AugmentItemInfo( - dii details.ItemInfo, - resource idname.Provider, - item models.DriveItemable, - size int64, - parentPath *path.Builder, -) details.ItemInfo { - return augmentItemInfo(dii, resource, path.OneDriveService, item, size, parentPath) -} - func (h itemRestoreHandler) DeleteItem( ctx context.Context, driveID, itemID string, diff --git a/src/internal/m365/collection/drive/library_handler.go b/src/internal/m365/collection/drive/library_handler.go index b64eaaee7..95bebb0bd 100644 --- a/src/internal/m365/collection/drive/library_handler.go +++ b/src/internal/m365/collection/drive/library_handler.go @@ -9,6 +9,7 @@ import ( "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/alcionai/corso/src/internal/common/idname" + "github.com/alcionai/corso/src/internal/common/ptr" odConsts "github.com/alcionai/corso/src/internal/m365/service/onedrive/consts" "github.com/alcionai/corso/src/pkg/backup/details" "github.com/alcionai/corso/src/pkg/control" @@ -17,10 +18,54 @@ import ( "github.com/alcionai/corso/src/pkg/services/m365/api" ) +type baseLibraryHandler struct { + ac api.Drives +} + +func (h baseLibraryHandler) NewDrivePager( + resourceOwner string, + fields []string, +) api.Pager[models.Driveable] { + return h.ac.NewSiteDrivePager(resourceOwner, fields) +} + +func (h baseLibraryHandler) AugmentItemInfo( + dii details.ItemInfo, + resource idname.Provider, + item models.DriveItemable, + size int64, + parentPath *path.Builder, +) details.ItemInfo { + var pps string + + if parentPath != nil { + pps = parentPath.String() + } + + driveName, driveID := getItemDriveInfo(item) + + dii.Extension = &details.ExtensionData{} + dii.SharePoint = &details.SharePointInfo{ + Created: ptr.Val(item.GetCreatedDateTime()), + DriveID: driveID, + DriveName: driveName, + ItemName: ptr.Val(item.GetName()), + ItemType: details.SharePointLibrary, + Modified: ptr.Val(item.GetLastModifiedDateTime()), + Owner: getItemCreator(item), + ParentPath: pps, + SiteID: resource.ID(), + Size: size, + WebURL: resource.Name(), + } + + return dii +} + var _ BackupHandler = &libraryBackupHandler{} type libraryBackupHandler struct { - ac api.Drives + baseLibraryHandler siteID string scope selectors.SharePointScope service path.ServiceType @@ -32,7 +77,14 @@ func NewLibraryBackupHandler( scope selectors.SharePointScope, service path.ServiceType, ) libraryBackupHandler { - return libraryBackupHandler{ac, siteID, scope, service} + return libraryBackupHandler{ + baseLibraryHandler: baseLibraryHandler{ + ac: ac, + }, + siteID: siteID, + scope: scope, + service: service, + } } func (h libraryBackupHandler) Get( @@ -84,23 +136,6 @@ func (h libraryBackupHandler) ServiceCat() (path.ServiceType, path.CategoryType) return h.service, path.LibrariesCategory } -func (h libraryBackupHandler) NewDrivePager( - resourceOwner string, - fields []string, -) api.Pager[models.Driveable] { - return h.ac.NewSiteDrivePager(resourceOwner, fields) -} - -func (h libraryBackupHandler) AugmentItemInfo( - dii details.ItemInfo, - resource idname.Provider, - item models.DriveItemable, - size int64, - parentPath *path.Builder, -) details.ItemInfo { - return augmentItemInfo(dii, resource, h.service, item, size, parentPath) -} - func (h libraryBackupHandler) FormatDisplayPath( driveName string, pb *path.Builder, @@ -152,12 +187,19 @@ func (h libraryBackupHandler) EnumerateDriveItemsDelta( var _ RestoreHandler = &libraryRestoreHandler{} type libraryRestoreHandler struct { + baseLibraryHandler ac api.Client service path.ServiceType } func NewLibraryRestoreHandler(ac api.Client, service path.ServiceType) libraryRestoreHandler { - return libraryRestoreHandler{ac, service} + return libraryRestoreHandler{ + baseLibraryHandler: baseLibraryHandler{ + ac: ac.Drives(), + }, + ac: ac, + service: service, + } } func (h libraryRestoreHandler) PostDrive( @@ -167,23 +209,6 @@ func (h libraryRestoreHandler) PostDrive( return h.ac.Lists().PostDrive(ctx, siteID, driveName) } -func (h libraryRestoreHandler) NewDrivePager( - resourceOwner string, - fields []string, -) api.Pager[models.Driveable] { - return h.ac.Drives().NewSiteDrivePager(resourceOwner, fields) -} - -func (h libraryRestoreHandler) AugmentItemInfo( - dii details.ItemInfo, - resource idname.Provider, - item models.DriveItemable, - size int64, - parentPath *path.Builder, -) details.ItemInfo { - return augmentItemInfo(dii, resource, h.service, item, size, parentPath) -} - func (h libraryRestoreHandler) DeleteItem( ctx context.Context, driveID, itemID string,