From 680ec2b7510c654c719c28ebd51c2830e618cdb7 Mon Sep 17 00:00:00 2001 From: Keepers Date: Mon, 13 Feb 2023 09:54:09 -0700 Subject: [PATCH] adding fault/clues to non-service connector pkgs (#2380) ## 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/discovery/api/users.go | 34 ++++++++++--------- src/internal/connector/discovery/discovery.go | 7 ++-- .../connector/graph/cache_container.go | 5 +-- src/internal/connector/graph_connector.go | 6 ++-- .../connector/graph_connector_test.go | 18 ++++------ src/internal/connector/support/status.go | 27 ++++----------- .../connector/uploadsession/uploadsession.go | 16 ++++----- src/pkg/selectors/scopes_test.go | 2 +- src/pkg/services/m365/m365.go | 2 +- src/pkg/services/m365/m365_test.go | 5 +-- 10 files changed, 52 insertions(+), 70 deletions(-) diff --git a/src/internal/connector/discovery/api/users.go b/src/internal/connector/discovery/api/users.go index c08297a9e..b1a37b683 100644 --- a/src/internal/connector/discovery/api/users.go +++ b/src/internal/connector/discovery/api/users.go @@ -3,6 +3,7 @@ package api import ( "context" + "github.com/alcionai/clues" absser "github.com/microsoft/kiota-abstractions-go" msgraphgocore "github.com/microsoftgraph/msgraph-sdk-go-core" "github.com/microsoftgraph/msgraph-sdk-go/models" @@ -10,7 +11,7 @@ import ( "github.com/pkg/errors" "github.com/alcionai/corso/src/internal/connector/graph" - "github.com/alcionai/corso/src/internal/connector/support" + "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/path" ) @@ -85,7 +86,7 @@ func userOptions(fs *string) *users.UsersRequestBuilderGetRequestConfiguration { } // GetAll retrieves all users. -func (c Users) GetAll(ctx context.Context) ([]models.Userable, error) { +func (c Users) GetAll(ctx context.Context, errs *fault.Errors) ([]models.Userable, error) { service, err := c.service() if err != nil { return nil, err @@ -99,7 +100,7 @@ func (c Users) GetAll(ctx context.Context) ([]models.Userable, error) { }) if err != nil { - return nil, support.ConnectorStackErrorTraceWrap(err, "getting all users") + return nil, clues.Wrap(err, "getting all users").WithClues(ctx).WithAll(graph.ErrData(err)...) } iter, err := msgraphgocore.NewPageIterator( @@ -107,18 +108,19 @@ func (c Users) GetAll(ctx context.Context) ([]models.Userable, error) { service.Adapter(), models.CreateUserCollectionResponseFromDiscriminatorValue) if err != nil { - return nil, support.ConnectorStackErrorTraceWrap(err, "constructing user iterator") + return nil, clues.Wrap(err, "creating users iterator").WithClues(ctx).WithAll(graph.ErrData(err)...) } - var ( - iterErrs error - us = make([]models.Userable, 0) - ) + us := make([]models.Userable, 0) iterator := func(item any) bool { + if errs.Failed() { + return false + } + u, err := validateUser(item) if err != nil { - iterErrs = support.WrapAndAppend("validating user", err, iterErrs) + errs.Add(clues.Wrap(err, "validating user").WithClues(ctx).WithAll(graph.ErrData(err)...)) } else { us = append(us, u) } @@ -127,10 +129,10 @@ func (c Users) GetAll(ctx context.Context) ([]models.Userable, error) { } if err := iter.Iterate(ctx, iterator); err != nil { - return nil, support.ConnectorStackErrorTraceWrap(err, "iterating all users") + return nil, clues.Wrap(err, "iterating all users").WithClues(ctx).WithAll(graph.ErrData(err)...) } - return us, iterErrs + return us, errs.Err() } func (c Users) GetByID(ctx context.Context, userID string) (models.Userable, error) { @@ -145,7 +147,7 @@ func (c Users) GetByID(ctx context.Context, userID string) (models.Userable, err }) if err != nil { - return nil, support.ConnectorStackErrorTraceWrap(err, "getting user by id") + return nil, clues.Wrap(err, "getting user").WithClues(ctx).WithAll(graph.ErrData(err)...) } return resp, err @@ -167,7 +169,7 @@ func (c Users) GetInfo(ctx context.Context, userID string) (*UserInfo, error) { if err != nil { if !graph.IsErrExchangeMailFolderNotFound(err) { - return nil, support.ConnectorStackErrorTraceWrap(err, "getting user's exchange mailfolders") + return nil, clues.Wrap(err, "getting user's mail folder").WithClues(ctx).WithAll(graph.ErrData(err)...) } delete(userInfo.DiscoveredServices, path.ExchangeService) @@ -186,15 +188,15 @@ func (c Users) GetInfo(ctx context.Context, userID string) (*UserInfo, error) { func validateUser(item any) (models.Userable, error) { m, ok := item.(models.Userable) if !ok { - return nil, errors.Errorf("expected Userable, got %T", item) + return nil, clues.Stack(clues.New("unexpected model"), errors.Errorf("%T", item)) } if m.GetId() == nil { - return nil, errors.Errorf("missing ID") + return nil, clues.New("missing ID") } if m.GetUserPrincipalName() == nil { - return nil, errors.New("missing principalName") + return nil, clues.New("missing principalName") } return m, nil diff --git a/src/internal/connector/discovery/discovery.go b/src/internal/connector/discovery/discovery.go index 3f75f74c4..f8a6f27c7 100644 --- a/src/internal/connector/discovery/discovery.go +++ b/src/internal/connector/discovery/discovery.go @@ -7,6 +7,7 @@ import ( "github.com/pkg/errors" "github.com/alcionai/corso/src/internal/connector/discovery/api" + "github.com/alcionai/corso/src/pkg/fault" ) // --------------------------------------------------------------------------- @@ -14,7 +15,7 @@ import ( // --------------------------------------------------------------------------- type getAller interface { - GetAll(context.Context) ([]models.Userable, error) + GetAll(context.Context, *fault.Errors) ([]models.Userable, error) } type getter interface { @@ -35,8 +36,8 @@ type getWithInfoer interface { // --------------------------------------------------------------------------- // Users fetches all users in the tenant. -func Users(ctx context.Context, ga getAller) ([]models.Userable, error) { - return ga.GetAll(ctx) +func Users(ctx context.Context, ga getAller, errs *fault.Errors) ([]models.Userable, error) { + return ga.GetAll(ctx, errs) } func User(ctx context.Context, gwi getWithInfoer, userID string) (models.Userable, *api.UserInfo, error) { diff --git a/src/internal/connector/graph/cache_container.go b/src/internal/connector/graph/cache_container.go index e792c235e..f88032fa6 100644 --- a/src/internal/connector/graph/cache_container.go +++ b/src/internal/connector/graph/cache_container.go @@ -1,6 +1,7 @@ package graph import ( + "github.com/alcionai/clues" "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/pkg/errors" @@ -26,12 +27,12 @@ func CheckRequiredValues(c Container) error { ptr := c.GetDisplayName() if ptr == nil || len(*ptr) == 0 { - return errors.Errorf("folder %s without display name", *idPtr) + return clues.New("folder missing display name").With("container_id", *idPtr) } ptr = c.GetParentFolderId() if ptr == nil || len(*ptr) == 0 { - return errors.Errorf("folder %s without parent ID", *idPtr) + return clues.New("folder missing parent ID").With("container_parent_id", *idPtr) } return nil diff --git a/src/internal/connector/graph_connector.go b/src/internal/connector/graph_connector.go index 06fb86188..45461e6d1 100644 --- a/src/internal/connector/graph_connector.go +++ b/src/internal/connector/graph_connector.go @@ -98,7 +98,7 @@ func NewGraphConnector( // For now this keeps things functioning if callers do pass in a selector like // "*" instead of. if r == AllResources || r == Users { - if err = gc.setTenantUsers(ctx); err != nil { + if err = gc.setTenantUsers(ctx, errs); err != nil { return nil, errors.Wrap(err, "retrieving tenant user list") } } @@ -129,11 +129,11 @@ func (gc *GraphConnector) createService() (*graph.Service, error) { // setTenantUsers queries the M365 to identify the users in the // workspace. The users field is updated during this method // iff the returned error is nil -func (gc *GraphConnector) setTenantUsers(ctx context.Context) error { +func (gc *GraphConnector) setTenantUsers(ctx context.Context, errs *fault.Errors) error { ctx, end := D.Span(ctx, "gc:setTenantUsers") defer end() - users, err := discovery.Users(ctx, gc.Owners.Users()) + users, err := discovery.Users(ctx, gc.Owners.Users(), errs) if err != nil { return err } diff --git a/src/internal/connector/graph_connector_test.go b/src/internal/connector/graph_connector_test.go index 8a2a2a39e..1840d7473 100644 --- a/src/internal/connector/graph_connector_test.go +++ b/src/internal/connector/graph_connector_test.go @@ -129,12 +129,8 @@ func (suite *GraphConnectorUnitSuite) TestUnionSiteIDsAndWebURLs() { ctx, flush := tester.NewContext() defer flush() - errs := fault.New(true) - - result, err := gc.UnionSiteIDsAndWebURLs(ctx, test.ids, test.urls, errs) + result, err := gc.UnionSiteIDsAndWebURLs(ctx, test.ids, test.urls, fault.New(true)) assert.NoError(t, err) - assert.NoError(t, errs.Err()) - assert.Empty(t, errs.Errs()) assert.ElementsMatch(t, test.expect, result) }) } @@ -192,9 +188,11 @@ func (suite *GraphConnectorIntegrationSuite) TestSetTenantUsers() { require.NoError(suite.T(), err) newConnector.Owners = owners - suite.Empty(len(newConnector.Users)) - err = newConnector.setTenantUsers(ctx) + + errs := fault.New(true) + + err = newConnector.setTenantUsers(ctx, errs) suite.NoError(err) suite.Less(0, len(newConnector.Users)) } @@ -219,12 +217,8 @@ func (suite *GraphConnectorIntegrationSuite) TestSetTenantSites() { newConnector.Service = service assert.Equal(t, 0, len(newConnector.Sites)) - errs := fault.New(true) - - err = newConnector.setTenantSites(ctx, errs) + err = newConnector.setTenantSites(ctx, fault.New(true)) assert.NoError(t, err) - assert.NoError(t, errs.Err()) - assert.Empty(t, errs.Errs()) assert.Less(t, 0, len(newConnector.Sites)) for _, site := range newConnector.Sites { diff --git a/src/internal/connector/support/status.go b/src/internal/connector/support/status.go index dcf5f32c5..849fee9bc 100644 --- a/src/internal/connector/support/status.go +++ b/src/internal/connector/support/status.go @@ -6,8 +6,6 @@ import ( "github.com/dustin/go-humanize" multierror "github.com/hashicorp/go-multierror" - - "github.com/alcionai/corso/src/pkg/logger" ) // ConnectorOperationStatus is a data type used to describe the state of @@ -80,15 +78,6 @@ func CreateStatus( additionalDetails: details, } - if status.ObjectCount != status.ErrorCount+status.Successful { - logger.Ctx(ctx).Errorw( - "status object count does not match errors + successes", - "objects", cm.Objects, - "successes", cm.Successes, - "numErrors", numErr, - "errors", err) - } - return &status } @@ -114,10 +103,11 @@ func MergeStatus(one, two ConnectorOperationStatus) ConnectorOperationStatus { } status := ConnectorOperationStatus{ - lastOperation: one.lastOperation, - ObjectCount: one.ObjectCount + two.ObjectCount, - FolderCount: one.FolderCount + two.FolderCount, - Successful: one.Successful + two.Successful, + lastOperation: one.lastOperation, + ObjectCount: one.ObjectCount + two.ObjectCount, + FolderCount: one.FolderCount + two.FolderCount, + Successful: one.Successful + two.Successful, + // TODO: remove in favor of fault.Errors ErrorCount: one.ErrorCount + two.ErrorCount, Err: multierror.Append(one.Err, two.Err).ErrorOrNil(), bytes: one.bytes + two.bytes, @@ -144,14 +134,11 @@ func (cos *ConnectorOperationStatus) String() string { cos.Successful, cos.ObjectCount, humanize.Bytes(uint64(cos.bytes)), - cos.FolderCount, - ) + cos.FolderCount) if cos.incomplete { message += " " + cos.incompleteReason } - message += " " + operationStatement + cos.additionalDetails + "\n" - - return message + return message + " " + operationStatement + cos.additionalDetails } diff --git a/src/internal/connector/uploadsession/uploadsession.go b/src/internal/connector/uploadsession/uploadsession.go index 818159e0b..5dc04388c 100644 --- a/src/internal/connector/uploadsession/uploadsession.go +++ b/src/internal/connector/uploadsession/uploadsession.go @@ -5,7 +5,7 @@ import ( "context" "fmt" - "github.com/pkg/errors" + "github.com/alcionai/clues" "gopkg.in/resty.v1" "github.com/alcionai/corso/src/pkg/logger" @@ -38,7 +38,7 @@ func NewWriter(id, url string, size int64) *writer { // Write will upload the provided data to M365. It sets the `Content-Length` and `Content-Range` headers based on // https://docs.microsoft.com/en-us/graph/api/driveitem-createuploadsession -func (iw *writer) Write(p []byte) (n int, err error) { +func (iw *writer) Write(p []byte) (int, error) { rangeLength := len(p) logger.Ctx(context.Background()).Debugf("WRITE for %s. Size:%d, Offset: %d, TotalSize: %d", iw.id, rangeLength, iw.lastWrittenOffset, iw.contentLength) @@ -47,7 +47,7 @@ func (iw *writer) Write(p []byte) (n int, err error) { // PUT the request - set headers `Content-Range`to describe total size and `Content-Length` to describe size of // data in the current request - resp, err := iw.client.R(). + _, err := iw.client.R(). SetHeaders(map[string]string{ contentRangeHeaderKey: fmt.Sprintf(contentRangeHeaderValueFmt, iw.lastWrittenOffset, @@ -57,15 +57,15 @@ func (iw *writer) Write(p []byte) (n int, err error) { }). SetBody(bytes.NewReader(p)).Put(iw.url) if err != nil { - return 0, errors.Wrapf(err, - "failed to upload item %s. Upload failed at Size:%d, Offset: %d, TotalSize: %d ", - iw.id, rangeLength, iw.lastWrittenOffset, iw.contentLength) + return 0, clues.Wrap(err, "uploading item").WithAll( + "upload_id", iw.id, + "upload_chunk_size", rangeLength, + "upload_offset", iw.lastWrittenOffset, + "upload_size", iw.contentLength) } // Update last offset iw.lastWrittenOffset = endOffset - logger.Ctx(context.Background()).Debugf("Response: %s", resp.String()) - return rangeLength, nil } diff --git a/src/pkg/selectors/scopes_test.go b/src/pkg/selectors/scopes_test.go index 273b1b774..a4598821a 100644 --- a/src/pkg/selectors/scopes_test.go +++ b/src/pkg/selectors/scopes_test.go @@ -284,7 +284,7 @@ func (suite *SelectorScopesSuite) TestReduce() { dataCats, errs) require.NotNil(t, result) - require.Empty(t, errs.Errs(), "recoverable errors") + require.NoError(t, errs.Err(), "no recoverable errors") assert.Len(t, result.Entries, test.expectLen) }) } diff --git a/src/pkg/services/m365/m365.go b/src/pkg/services/m365/m365.go index 9379cc028..b6564ea4d 100644 --- a/src/pkg/services/m365/m365.go +++ b/src/pkg/services/m365/m365.go @@ -28,7 +28,7 @@ func Users(ctx context.Context, acct account.Account, errs *fault.Errors) ([]*Us return nil, errors.Wrap(err, "initializing M365 graph connection") } - users, err := discovery.Users(ctx, gc.Owners.Users()) + users, err := discovery.Users(ctx, gc.Owners.Users(), errs) if err != nil { return nil, err } diff --git a/src/pkg/services/m365/m365_test.go b/src/pkg/services/m365/m365_test.go index f5f040dfa..503e220fe 100644 --- a/src/pkg/services/m365/m365_test.go +++ b/src/pkg/services/m365/m365_test.go @@ -31,13 +31,10 @@ func (suite *M365IntegrationSuite) TestUsers() { var ( t = suite.T() acct = tester.NewM365Account(suite.T()) - errs = fault.New(true) ) - users, err := Users(ctx, acct, errs) + users, err := Users(ctx, acct, fault.New(true)) require.NoError(t, err) - require.NoError(t, errs.Err()) - require.Empty(t, errs.Errs()) require.NotNil(t, users) require.Greater(t, len(users), 0)