From c92b70e00011c482e8ed0166453fd07920ca3801 Mon Sep 17 00:00:00 2001 From: Keepers Date: Fri, 17 Feb 2023 10:08:08 -0700 Subject: [PATCH] wrap up clues and fault additions to exchange (#2504) ## Does this PR need a docs update or release note? - [x] :no_entry: No ## Type of change - [x] :broom: Tech Debt/Cleanup ## Issue(s) * #1970 ## Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- src/internal/connector/data_collections.go | 27 ++++++++++--------- .../connector/exchange/api/contacts.go | 2 +- src/internal/connector/exchange/api/events.go | 4 +-- src/internal/connector/exchange/api/mail.go | 6 ++--- src/internal/connector/exchange/attachment.go | 23 ++++++++-------- .../connector/exchange/cache_container.go | 18 ++++++------- .../exchange/{exchange_vars.go => consts.go} | 0 .../exchange/container_resolver_test.go | 6 ++--- src/internal/connector/graph/api/api.go | 18 +++---------- .../connector/graph/cache_container.go | 18 ++++++------- src/internal/connector/graph/service.go | 21 ++++++++++----- src/internal/connector/graph_connector.go | 2 +- .../operations/backup_integration_test.go | 3 +-- 13 files changed, 73 insertions(+), 75 deletions(-) rename src/internal/connector/exchange/{exchange_vars.go => consts.go} (100%) diff --git a/src/internal/connector/data_collections.go b/src/internal/connector/data_collections.go index 22c29b1b1..2e82c53d9 100644 --- a/src/internal/connector/data_collections.go +++ b/src/internal/connector/data_collections.go @@ -2,12 +2,12 @@ package connector import ( "context" - "fmt" "strings" "github.com/pkg/errors" "golang.org/x/exp/maps" + "github.com/alcionai/clues" "github.com/alcionai/corso/src/internal/connector/discovery" "github.com/alcionai/corso/src/internal/connector/discovery/api" "github.com/alcionai/corso/src/internal/connector/exchange" @@ -46,10 +46,14 @@ func (gc *GraphConnector) DataCollections( err := verifyBackupInputs(sels, gc.GetUsers(), gc.GetSiteIDs()) if err != nil { - return nil, nil, err + return nil, nil, clues.Stack(err).WithClues(ctx) } - serviceEnabled, err := checkServiceEnabled(ctx, gc.Owners.Users(), path.ServiceType(sels.Service), sels.DiscreteOwner) + serviceEnabled, err := checkServiceEnabled( + ctx, + gc.Owners.Users(), + path.ServiceType(sels.Service), + sels.DiscreteOwner) if err != nil { return nil, nil, err } @@ -65,7 +69,6 @@ func (gc *GraphConnector) DataCollections( sels, metadata, gc.credentials, - // gc.Service, gc.UpdateStatus, ctrlOpts, errs) @@ -110,7 +113,7 @@ func (gc *GraphConnector) DataCollections( return colls, excludes, nil default: - return nil, nil, errors.Errorf("service %s not supported", sels.Service.String()) + return nil, nil, clues.Wrap(clues.New(sels.Service.String()), "service not supported").WithClues(ctx) } } @@ -137,7 +140,7 @@ func verifyBackupInputs(sels selectors.Selector, userPNs, siteIDs []string) erro } if !found { - return fmt.Errorf("resource owner [%s] not found within tenant", sels.DiscreteOwner) + return clues.New("resource owner not found within tenant").With("missing_resource_owner", sels.DiscreteOwner) } return nil @@ -192,19 +195,18 @@ func (gc *GraphConnector) OneDriveDataCollections( ) ([]data.BackupCollection, map[string]struct{}, error) { odb, err := selector.ToOneDriveBackup() if err != nil { - return nil, nil, errors.Wrap(err, "oneDriveDataCollection: parsing selector") + return nil, nil, clues.Wrap(err, "parsing selector").WithClues(ctx) } var ( user = selector.DiscreteOwner collections = []data.BackupCollection{} allExcludes = map[string]struct{}{} - errs error ) // for each scope that includes oneDrive items, get all for _, scope := range odb.Scopes() { - logger.Ctx(ctx).With("user", user).Debug("Creating OneDrive collections") + logger.Ctx(ctx).Debug("creating OneDrive collections") odcs, excludes, err := onedrive.NewCollections( gc.itemClient, @@ -217,7 +219,7 @@ func (gc *GraphConnector) OneDriveDataCollections( ctrlOpts, ).Get(ctx, metadata) if err != nil { - return nil, nil, support.WrapAndAppend(user, err, errs) + return nil, nil, err } collections = append(collections, odcs...) @@ -232,7 +234,7 @@ func (gc *GraphConnector) OneDriveDataCollections( } } - return collections, allExcludes, errs + return collections, allExcludes, nil } // RestoreDataCollections restores data from the specified collections @@ -253,7 +255,6 @@ func (gc *GraphConnector) RestoreDataCollections( var ( status *support.ConnectorOperationStatus - err error deets = &details.Builder{} ) @@ -270,7 +271,7 @@ func (gc *GraphConnector) RestoreDataCollections( case selectors.ServiceSharePoint: status, err = sharepoint.RestoreCollections(ctx, backupVersion, creds, gc.Service, dest, dcs, deets) default: - err = errors.Errorf("restore data from service %s not supported", selector.Service.String()) + err = clues.Wrap(clues.New(selector.Service.String()), "service not supported") } gc.incrementAwaitingMessages() diff --git a/src/internal/connector/exchange/api/contacts.go b/src/internal/connector/exchange/api/contacts.go index ab680355d..4a44f1e67 100644 --- a/src/internal/connector/exchange/api/contacts.go +++ b/src/internal/connector/exchange/api/contacts.go @@ -47,7 +47,7 @@ func (c Contacts) CreateContactFolder( mdl, err := c.stable.Client().UsersById(user).ContactFolders().Post(ctx, requestBody, nil) if err != nil { - return nil, clues.Stack(err).WithClues(ctx).WithAll(graph.ErrData(err)...) + return nil, clues.Wrap(err, "creating contact folder").WithClues(ctx).WithAll(graph.ErrData(err)...) } return mdl, nil diff --git a/src/internal/connector/exchange/api/events.go b/src/internal/connector/exchange/api/events.go index d6cfc1bbf..a35d266d1 100644 --- a/src/internal/connector/exchange/api/events.go +++ b/src/internal/connector/exchange/api/events.go @@ -48,7 +48,7 @@ func (c Events) CreateCalendar( mdl, err := c.stable.Client().UsersById(user).Calendars().Post(ctx, requestbody, nil) if err != nil { - return nil, clues.Stack(err).WithClues(ctx).WithAll(graph.ErrData(err)...) + return nil, clues.Wrap(err, "creating calendar").WithClues(ctx).WithAll(graph.ErrData(err)...) } return mdl, nil @@ -238,7 +238,7 @@ func (c Events) GetAddedAndRemovedItemIDs( ctx = clues.AddAll( ctx, - "calendar_id", calendarID) + "container_id", calendarID) if len(oldDelta) > 0 { var ( diff --git a/src/internal/connector/exchange/api/mail.go b/src/internal/connector/exchange/api/mail.go index ed768459a..7845b42d7 100644 --- a/src/internal/connector/exchange/api/mail.go +++ b/src/internal/connector/exchange/api/mail.go @@ -48,7 +48,7 @@ func (c Mail) CreateMailFolder( mdl, err := c.stable.Client().UsersById(user).MailFolders().Post(ctx, requestBody, nil) if err != nil { - return nil, clues.Stack(err).WithClues(ctx).WithAll(graph.ErrData(err)...) + return nil, clues.Wrap(err, "creating mail folder").WithClues(ctx).WithAll(graph.ErrData(err)...) } return mdl, nil @@ -75,7 +75,7 @@ func (c Mail) CreateMailFolderWithParent( ChildFolders(). Post(ctx, requestBody, nil) if err != nil { - return nil, clues.Stack(err).WithClues(ctx).WithAll(graph.ErrData(err)...) + return nil, clues.Wrap(err, "creating nested mail folder").WithClues(ctx).WithAll(graph.ErrData(err)...) } return mdl, nil @@ -250,7 +250,7 @@ func (c Mail) GetAddedAndRemovedItemIDs( ctx = clues.AddAll( ctx, "category", selectors.ExchangeMail, - "folder_id", directoryID) + "container_id", directoryID) options, err := optionsForFolderMessagesDelta([]string{"isRead"}) if err != nil { diff --git a/src/internal/connector/exchange/attachment.go b/src/internal/connector/exchange/attachment.go index 831a42fcf..bb8756bbe 100644 --- a/src/internal/connector/exchange/attachment.go +++ b/src/internal/connector/exchange/attachment.go @@ -6,7 +6,6 @@ import ( "io" "github.com/microsoftgraph/msgraph-sdk-go/models" - "github.com/pkg/errors" "github.com/alcionai/clues" "github.com/alcionai/corso/src/internal/common/ptr" @@ -58,11 +57,11 @@ func uploadAttachment( "internal_item_type", getItemAttachmentItemType(attachment), "uploader_item_id", uploader.getItemID()) - logger.Ctx(ctx).Debugw("uploading attachment") + logger.Ctx(ctx).Debug("uploading attachment") // Reference attachments that are inline() do not need to be recreated. The contents are part of the body. if attachmentType == models.REFERENCE_ATTACHMENTTYPE && ptr.Val(attachment.GetIsInline()) { - logger.Ctx(ctx).Debug("skip uploading inline reference attachment: ", ptr.Val(attachment.GetName())) + logger.Ctx(ctx).Debug("skip uploading inline reference attachment") return nil } @@ -95,24 +94,26 @@ func uploadLargeAttachment( uploader attachmentUploadable, attachment models.Attachmentable, ) error { - ab := attachmentBytes(attachment) - size := int64(len(ab)) + var ( + bs = attachmentBytes(attachment) + size = int64(len(bs)) + ) - session, err := uploader.uploadSession(ctx, *attachment.GetName(), size) + session, err := uploader.uploadSession(ctx, ptr.Val(attachment.GetName()), size) if err != nil { - return err + return clues.Stack(err).WithClues(ctx) } - url := *session.GetUploadUrl() + url := ptr.Val(session.GetUploadUrl()) aw := uploadsession.NewWriter(uploader.getItemID(), url, size) - logger.Ctx(ctx).Debugf("Created an upload session for item %s. URL: %s", uploader.getItemID(), url) + logger.Ctx(ctx).Debugw("uploading large attachment", "attachment_url", url) // TODO: url pii // Upload the stream data copyBuffer := make([]byte, attachmentChunkSize) - _, err = io.CopyBuffer(aw, bytes.NewReader(ab), copyBuffer) + _, err = io.CopyBuffer(aw, bytes.NewReader(bs), copyBuffer) if err != nil { - return errors.Wrapf(err, "failed to upload attachment: item %s", uploader.getItemID()) + return clues.Wrap(err, "uploading large attachment").WithClues(ctx) } return nil diff --git a/src/internal/connector/exchange/cache_container.go b/src/internal/connector/exchange/cache_container.go index 8f968a507..1da43d994 100644 --- a/src/internal/connector/exchange/cache_container.go +++ b/src/internal/connector/exchange/cache_container.go @@ -3,20 +3,21 @@ package exchange import ( "github.com/pkg/errors" + "github.com/alcionai/clues" + "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/connector/graph" ) // checkIDAndName is a helper function to ensure that // the ID and name pointers are set prior to being called. func checkIDAndName(c graph.Container) error { - idPtr := c.GetId() - if idPtr == nil || len(*idPtr) == 0 { - return errors.New("folder without ID") + id, ok := ptr.ValOK(c.GetId()) + if !ok { + return errors.New("container missing ID") } - ptr := c.GetDisplayName() - if ptr == nil || len(*ptr) == 0 { - return errors.Errorf("folder %s without display name", *idPtr) + if _, ok := ptr.ValOK(c.GetDisplayName()); !ok { + return clues.New("container missing display name").With("container_id", id) } return nil @@ -29,9 +30,8 @@ func checkRequiredValues(c graph.Container) error { return err } - ptr := c.GetParentFolderId() - if ptr == nil || len(*ptr) == 0 { - return errors.Errorf("folder %s without parent ID", *c.GetId()) + if _, ok := ptr.ValOK(c.GetParentFolderId()); !ok { + return clues.New("container missing parent ID").With("container_id", ptr.Val(c.GetId())) } return nil diff --git a/src/internal/connector/exchange/exchange_vars.go b/src/internal/connector/exchange/consts.go similarity index 100% rename from src/internal/connector/exchange/exchange_vars.go rename to src/internal/connector/exchange/consts.go diff --git a/src/internal/connector/exchange/container_resolver_test.go b/src/internal/connector/exchange/container_resolver_test.go index 601fec0b2..aa3bb67d6 100644 --- a/src/internal/connector/exchange/container_resolver_test.go +++ b/src/internal/connector/exchange/container_resolver_test.go @@ -96,7 +96,7 @@ var ( displayName: &testName, parentID: &testParentID, }, - check: assert.Error, + check: assert.NoError, }, { name: "EmptyDisplayName", @@ -105,7 +105,7 @@ var ( displayName: &emptyString, parentID: &testParentID, }, - check: assert.Error, + check: assert.NoError, }, { name: "AllValues", @@ -145,7 +145,7 @@ func (suite *FolderCacheUnitSuite) TestCheckRequiredValues() { displayName: &testName, parentID: &emptyString, }, - check: assert.Error, + check: assert.NoError, }, } diff --git a/src/internal/connector/graph/api/api.go b/src/internal/connector/graph/api/api.go index abcf29a24..dff30bc4b 100644 --- a/src/internal/connector/graph/api/api.go +++ b/src/internal/connector/graph/api/api.go @@ -1,5 +1,7 @@ package api +import "github.com/alcionai/corso/src/internal/common/ptr" + type PageLinker interface { GetOdataNextLink() *string } @@ -10,21 +12,9 @@ type DeltaPageLinker interface { } func NextLink(pl PageLinker) string { - next := pl.GetOdataNextLink() - if next == nil || len(*next) == 0 { - return "" - } - - return *next + return ptr.Val(pl.GetOdataNextLink()) } func NextAndDeltaLink(pl DeltaPageLinker) (string, string) { - next := NextLink(pl) - - delta := pl.GetOdataDeltaLink() - if delta == nil || len(*delta) == 0 { - return next, "" - } - - return next, *delta + return NextLink(pl), ptr.Val(pl.GetOdataDeltaLink()) } diff --git a/src/internal/connector/graph/cache_container.go b/src/internal/connector/graph/cache_container.go index 89210a728..89b40c25a 100644 --- a/src/internal/connector/graph/cache_container.go +++ b/src/internal/connector/graph/cache_container.go @@ -3,9 +3,11 @@ package graph import ( "context" + "github.com/alcionai/clues" "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/pkg/errors" + "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/path" ) @@ -171,19 +173,17 @@ func CreateCalendarDisplayable(entry any, parentID string) *CalendarDisplayable // checkRequiredValues is a helper function to ensure that // all the pointers are set prior to being called. func CheckRequiredValues(c Container) error { - idPtr := c.GetId() - if idPtr == nil || len(*idPtr) == 0 { - return errors.New("folder without ID") + id, ok := ptr.ValOK(c.GetId()) + if !ok { + return errors.New("container missing ID") } - ptr := c.GetDisplayName() - if ptr == nil || len(*ptr) == 0 { - return errors.Errorf("folder %s without display name", *idPtr) + if _, ok := ptr.ValOK(c.GetDisplayName()); !ok { + return clues.New("container missing display name").With("container_id", id) } - ptr = c.GetParentFolderId() - if ptr == nil || len(*ptr) == 0 { - return errors.Errorf("folder %s without parent ID", *idPtr) + if _, ok := ptr.ValOK(c.GetParentFolderId()); !ok { + return clues.New("container missing parent ID").With("container_id", id) } return nil diff --git a/src/internal/connector/graph/service.go b/src/internal/connector/graph/service.go index 3d2cacef9..c1320d836 100644 --- a/src/internal/connector/graph/service.go +++ b/src/internal/connector/graph/service.go @@ -7,6 +7,7 @@ import ( "time" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" + "github.com/alcionai/clues" backoff "github.com/cenkalti/backoff/v4" "github.com/microsoft/kiota-abstractions-go/serialization" ka "github.com/microsoft/kiota-authentication-azure-go" @@ -15,7 +16,6 @@ import ( msgraphgocore "github.com/microsoftgraph/msgraph-sdk-go-core" "github.com/pkg/errors" - "github.com/alcionai/corso/src/internal/connector/support" "github.com/alcionai/corso/src/pkg/account" "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/path" @@ -79,7 +79,7 @@ func (s Service) Serialize(object serialization.Parsable) ([]byte, error) { err = writer.WriteObjectValue("", object) if err != nil { - return nil, errors.Wrap(err, "writeObjecValue serialization") + return nil, errors.Wrap(err, "serializing object") } return writer.GetSerializedContent() @@ -161,7 +161,7 @@ func CreateAdapter(tenant, client, secret string, opts ...option) (*msgraphsdk.G // Client Provider: Uses Secret for access to tenant-level data cred, err := azidentity.NewClientSecretCredential(tenant, client, secret, nil) if err != nil { - return nil, errors.Wrap(err, "creating m365 client secret credentials") + return nil, errors.Wrap(err, "creating m365 client identity") } auth, err := ka.NewAzureIdentityAuthenticationProviderWithScopes( @@ -169,13 +169,15 @@ func CreateAdapter(tenant, client, secret string, opts ...option) (*msgraphsdk.G []string{"https://graph.microsoft.com/.default"}, ) if err != nil { - return nil, errors.Wrap(err, "creating new AzureIdentityAuthentication") + return nil, errors.Wrap(err, "creating azure authentication") } httpClient := HTTPClient(opts...) return msgraphsdk.NewGraphRequestAdapterWithParseNodeFactoryAndSerializationWriterFactoryAndHttpClient( - auth, nil, nil, httpClient) + auth, + nil, nil, + httpClient) } // HTTPClient creates the httpClient with middlewares and timeout configured @@ -327,14 +329,14 @@ func (middleware RetryHandler) Intercept( response, err := pipeline.Next(req, middlewareIndex) if err != nil && !IsErrTimeout(err) { - return response, support.ConnectorStackErrorTraceWrap(err, "maximum retries or unretryable") + return nil, clues.Stack(err).WithClues(ctx).WithAll(ErrData(err)...) } exponentialBackOff := backoff.NewExponentialBackOff() exponentialBackOff.InitialInterval = middleware.Delay exponentialBackOff.Reset() - return middleware.retryRequest( + response, err = middleware.retryRequest( ctx, pipeline, middlewareIndex, @@ -344,4 +346,9 @@ func (middleware RetryHandler) Intercept( 0, exponentialBackOff, err) + if err != nil { + return nil, clues.Stack(err).WithClues(ctx).WithAll(ErrData(err)...) + } + + return response, nil } diff --git a/src/internal/connector/graph_connector.go b/src/internal/connector/graph_connector.go index a60e76e0c..8e8bb824b 100644 --- a/src/internal/connector/graph_connector.go +++ b/src/internal/connector/graph_connector.go @@ -192,7 +192,7 @@ const personalSitePath = "sharepoint.com/personal/" func identifySite(item any) (string, string, error) { m, ok := item.(models.Siteable) if !ok { - return "", "", clues.New("iteration retrieved non-Site item").With("item_type", fmt.Sprintf("%T", item)) + return "", "", clues.New("non-Siteable item").With("item_type", fmt.Sprintf("%T", item)) } if m.GetName() == nil { diff --git a/src/internal/operations/backup_integration_test.go b/src/internal/operations/backup_integration_test.go index 521e87a80..899befb74 100644 --- a/src/internal/operations/backup_integration_test.go +++ b/src/internal/operations/backup_integration_test.go @@ -146,8 +146,7 @@ func runAndCheckBackup( Completed, bo.Status, "backup status should be Completed, got %s", - bo.Status, - ) + bo.Status) require.Less(t, 0, bo.Results.ItemsWritten) assert.Less(t, 0, bo.Results.ItemsRead, "count of items read")