Skip to content

Commit

Permalink
Use service-handler for restore operations (#4714)
Browse files Browse the repository at this point in the history
Switch restore code path to use service-level handlers. Does a few
things:
* switches existing service-level functions to be methods on
  service-level handlers
* update interfaces as necessary
* moves some logic from old controller-level restore function to either
  the new handlers or the operation-level function
* removes old code

May be easiest to review by commit

---

#### 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

- [ ] 🌻 Feature
- [ ] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [x] 🧹 Tech Debt/Cleanup

#### Issue(s)

* #4254

#### Test Plan

- [ ] 💪 Manual
- [x] ⚡ Unit test
- [x] 💚 E2E
  • Loading branch information
ashmrtn committed Nov 28, 2023
1 parent 1be4c4d commit d366362
Show file tree
Hide file tree
Showing 19 changed files with 208 additions and 163 deletions.
21 changes: 12 additions & 9 deletions src/cmd/factory/impl/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,12 @@ func generateAndRestoreItems(
Selector: sel,
}

deets, _, err := ctrl.ConsumeRestoreCollections(
handler, err := ctrl.NewServiceHandler(opts, service)
if err != nil {
return nil, clues.Stack(err)
}

deets, _, err := handler.ConsumeRestoreCollections(
ctx,
rcc,
dataColls,
Expand Down Expand Up @@ -428,13 +433,6 @@ func generateAndRestoreDriveItems(
return nil, err
}

// collections := getCollections(
// service,
// tenantID,
// []string{resourceOwner},
// input,
// version.Backup)

opts := control.DefaultOptions()
restoreCfg.IncludePermissions = true

Expand Down Expand Up @@ -462,7 +460,12 @@ func generateAndRestoreDriveItems(
Selector: sel,
}

deets, _, err := ctrl.ConsumeRestoreCollections(
handler, err := ctrl.NewServiceHandler(opts, service)
if err != nil {
return nil, clues.Stack(err)
}

deets, _, err := handler.ConsumeRestoreCollections(
ctx,
rcc,
collections,
Expand Down
5 changes: 4 additions & 1 deletion src/internal/data/implementations.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ import (
"github.com/alcionai/corso/src/pkg/path"
)

var ErrNotFound = clues.New("not found")
var (
ErrNotFound = clues.New("not found")
ErrNoData = clues.New("no data")
)

type CollectionState int

Expand Down
1 change: 0 additions & 1 deletion src/internal/m365/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ var ErrNoResourceLookup = clues.New("missing resource lookup client")
// must comply with BackupProducer and RestoreConsumer
var (
_ inject.BackupProducer = &Controller{}
_ inject.RestoreConsumer = &Controller{}
_ inject.ToServiceHandler = &Controller{}
)

Expand Down
21 changes: 18 additions & 3 deletions src/internal/m365/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,12 @@ func (suite *ControllerIntegrationSuite) TestEmptyCollections() {
Selector: test.sel,
}

deets, _, err := suite.ctrl.ConsumeRestoreCollections(
handler, err := suite.ctrl.NewServiceHandler(
control.DefaultOptions(),
test.sel.PathService())
require.NoError(t, err, clues.ToCore(err))

deets, _, err := handler.ConsumeRestoreCollections(
ctx,
rcc,
test.col,
Expand Down Expand Up @@ -562,7 +567,12 @@ func runRestore(
Selector: restoreSel,
}

deets, status, err := restoreCtrl.ConsumeRestoreCollections(
handler, err := restoreCtrl.NewServiceHandler(
control.DefaultOptions(),
sci.Service)
require.NoError(t, err, clues.ToCore(err))

deets, status, err := handler.ConsumeRestoreCollections(
ctx,
rcc,
collections,
Expand Down Expand Up @@ -1194,7 +1204,12 @@ func (suite *ControllerIntegrationSuite) TestMultiFolderBackupDifferentNames() {
Selector: restoreSel,
}

deets, status, err := restoreCtrl.ConsumeRestoreCollections(
handler, err := restoreCtrl.NewServiceHandler(
control.DefaultOptions(),
test.service)
require.NoError(t, err, clues.ToCore(err))

deets, status, err := handler.ConsumeRestoreCollections(
ctx,
rcc,
collections,
Expand Down
42 changes: 42 additions & 0 deletions src/internal/m365/mock/connector.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,45 @@ func (ctrl Controller) SetRateLimiter(
) context.Context {
return ctx
}

var _ inject.RestoreConsumer = &RestoreConsumer{}

type RestoreConsumer struct {
Deets *details.Details

Err error

Stats data.CollectionStats

ProtectedResourceID string
ProtectedResourceName string
ProtectedResourceErr error
}

func (rc RestoreConsumer) IsServiceEnabled(
context.Context,
string,
) (bool, error) {
return true, rc.Err
}

func (rc RestoreConsumer) PopulateProtectedResourceIDAndName(
ctx context.Context,
protectedResource string, // input value, can be either id or name
ins idname.Cacher,
) (idname.Provider, error) {
return idname.NewProvider(rc.ProtectedResourceID, rc.ProtectedResourceName),
rc.ProtectedResourceErr
}

func (rc RestoreConsumer) CacheItemInfo(dii details.ItemInfo) {}

func (rc RestoreConsumer) ConsumeRestoreCollections(
ctx context.Context,
rcc inject.RestoreConsumerConfig,
dcs []data.RestoreCollection,
errs *fault.Bus,
ctr *count.Bus,
) (*details.Details, *data.CollectionStats, error) {
return rc.Deets, &rc.Stats, rc.Err
}
91 changes: 0 additions & 91 deletions src/internal/m365/restore.go

This file was deleted.

2 changes: 1 addition & 1 deletion src/internal/m365/service/exchange/export.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
"github.com/alcionai/corso/src/pkg/services/m365/api"
)

var _ inject.ServiceHandler = &baseExchangeHandler{}
var _ inject.ServiceHandler = &exchangeHandler{}

func NewExchangeHandler(
opts control.Options,
Expand Down
19 changes: 14 additions & 5 deletions src/internal/m365/service/exchange/restore.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,29 +13,38 @@ import (
"github.com/alcionai/corso/src/pkg/count"
"github.com/alcionai/corso/src/pkg/fault"
"github.com/alcionai/corso/src/pkg/path"
"github.com/alcionai/corso/src/pkg/services/m365/api"
"github.com/alcionai/corso/src/pkg/services/m365/api/graph"
)

// ConsumeRestoreCollections restores M365 objects in data.RestoreCollection to MSFT
// store through GraphAPI.
func ConsumeRestoreCollections(
func (h *exchangeHandler) ConsumeRestoreCollections(
ctx context.Context,
ac api.Client,
rcc inject.RestoreConsumerConfig,
dcs []data.RestoreCollection,
errs *fault.Bus,
ctr *count.Bus,
) (*details.Details, *data.CollectionStats, error) {
if len(dcs) == 0 {
return nil, nil, clues.New("no data collections to restore")
return nil, nil, clues.WrapWC(ctx, data.ErrNoData, "performing restore")
}

// TODO(ashmrtn): We should stop relying on the context for rate limiter stuff
// and instead configure this when we make the handler instance. We can't
// initialize it in the NewHandler call right now because those functions
// aren't (and shouldn't be) returning a context along with the handler. Since
// that call isn't directly calling into this function even if we did
// initialize the rate limiter there it would be lost because it wouldn't get
// stored in an ancestor of the context passed to this function.
ctx = graph.BindRateLimiterConfig(
ctx,
graph.LimiterCfg{Service: path.ExchangeService})

var (
deets = &details.Builder{}
resourceID = rcc.ProtectedResource.ID()
directoryCache = make(map[path.CategoryType]graph.ContainerResolver)
handlers = exchange.RestoreHandlers(ac)
handlers = exchange.RestoreHandlers(h.apiClient)
metrics support.CollectionMetrics
el = errs.Local()
)
Expand Down
34 changes: 24 additions & 10 deletions src/internal/m365/service/groups/restore.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,35 @@ import (
)

// ConsumeRestoreCollections will restore the specified data collections into OneDrive
func ConsumeRestoreCollections(
func (h *groupsHandler) ConsumeRestoreCollections(
ctx context.Context,
rcc inject.RestoreConsumerConfig,
ac api.Client,
backupDriveIDNames idname.Cacher,
backupSiteIDWebURL idname.Cacher,
dcs []data.RestoreCollection,
errs *fault.Bus,
ctr *count.Bus,
) (*details.Details, *data.CollectionStats, error) {
if len(dcs) == 0 {
return nil, nil, clues.WrapWC(ctx, data.ErrNoData, "performing restore")
}

// TODO(ashmrtn): We should stop relying on the context for rate limiter stuff
// and instead configure this when we make the handler instance. We can't
// initialize it in the NewHandler call right now because those functions
// aren't (and shouldn't be) returning a context along with the handler. Since
// that call isn't directly calling into this function even if we did
// initialize the rate limiter there it would be lost because it wouldn't get
// stored in an ancestor of the context passed to this function.
ctx = graph.BindRateLimiterConfig(
ctx,
graph.LimiterCfg{Service: path.GroupsService})

var (
deets = &details.Builder{}
restoreMetrics support.CollectionMetrics
caches = drive.NewRestoreCaches(backupDriveIDNames)
lrh = drive.NewSiteRestoreHandler(ac, rcc.Selector.PathService())
deets = &details.Builder{}
restoreMetrics support.CollectionMetrics
caches = drive.NewRestoreCaches(h.backupDriveIDNames)
lrh = drive.NewSiteRestoreHandler(
h.apiClient,
rcc.Selector.PathService())
el = errs.Local()
webURLToSiteNames = map[string]string{}
)
Expand Down Expand Up @@ -70,13 +84,13 @@ func ConsumeRestoreCollections(
case path.LibrariesCategory:
siteID := dc.FullPath().Folders()[1]

webURL, ok := backupSiteIDWebURL.NameOf(siteID)
webURL, ok := h.backupSiteIDWebURL.NameOf(siteID)
if !ok {
// This should not happen, but just in case
logger.Ctx(ictx).With("site_id", siteID).Info("site weburl not found, using site id")
}

siteName, err = getSiteName(ictx, siteID, webURL, ac.Sites(), webURLToSiteNames)
siteName, err = getSiteName(ictx, siteID, webURL, h.apiClient.Sites(), webURLToSiteNames)
if err != nil {
el.AddRecoverable(ictx, clues.Wrap(err, "getting site").
With("web_url", webURL, "site_id", siteID))
Expand Down
17 changes: 7 additions & 10 deletions src/internal/m365/service/groups/restore_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"github.com/stretchr/testify/suite"
"golang.org/x/exp/slices"

"github.com/alcionai/corso/src/internal/common/idname"
"github.com/alcionai/corso/src/internal/common/ptr"
"github.com/alcionai/corso/src/internal/data"
"github.com/alcionai/corso/src/internal/data/mock"
Expand Down Expand Up @@ -53,15 +52,13 @@ func (suite *GroupsUnitSuite) TestConsumeRestoreCollections_noErrorOnGroups() {
mock.Collection{Path: pth},
}

_, _, err = ConsumeRestoreCollections(
ctx,
rcc,
api.Client{},
idname.NewCache(map[string]string{}),
idname.NewCache(map[string]string{}),
dcs,
fault.New(false),
nil)
_, _, err = NewGroupsHandler(control.DefaultOptions(), api.Client{}, nil).
ConsumeRestoreCollections(
ctx,
rcc,
dcs,
fault.New(false),
nil)
assert.NoError(t, err, "Groups Channels restore")
}

Expand Down

0 comments on commit d366362

Please sign in to comment.