diff --git a/src/internal/m365/graph/errors.go b/src/internal/m365/graph/errors.go index 4c3f8a3cb..ca70df38d 100644 --- a/src/internal/m365/graph/errors.go +++ b/src/internal/m365/graph/errors.go @@ -29,8 +29,8 @@ const ( activityLimitReached errorCode = "activityLimitReached" emailFolderNotFound errorCode = "ErrorSyncFolderNotFound" errorAccessDenied errorCode = "ErrorAccessDenied" - itemNotFound errorCode = "ErrorItemNotFound" - itemNotFoundShort errorCode = "itemNotFound" + errorItemNotFound errorCode = "ErrorItemNotFound" + itemNotFound errorCode = "itemNotFound" MailboxNotEnabledForRESTAPI errorCode = "MailboxNotEnabledForRESTAPI" malwareDetected errorCode = "malwareDetected" // nameAlreadyExists occurs when a request with @@ -111,8 +111,8 @@ func IsErrDeletedInFlight(err error) bool { if hasErrorCode( err, + errorItemNotFound, itemNotFound, - itemNotFoundShort, syncFolderNotFound, ) { return true @@ -121,6 +121,10 @@ func IsErrDeletedInFlight(err error) bool { return false } +func IsErrItemNotFound(err error) bool { + return hasErrorCode(err, itemNotFound) +} + func IsErrInvalidDelta(err error) bool { return hasErrorCode(err, syncStateNotFound, resyncRequired, syncStateInvalid) || errors.Is(err, ErrInvalidDelta) @@ -133,7 +137,7 @@ func IsErrQuotaExceeded(err error) bool { func IsErrExchangeMailFolderNotFound(err error) bool { // Not sure if we can actually see a resourceNotFound error here. I've only // seen the latter two. - return hasErrorCode(err, ResourceNotFound, itemNotFound, MailboxNotEnabledForRESTAPI) + return hasErrorCode(err, ResourceNotFound, errorItemNotFound, MailboxNotEnabledForRESTAPI) } func IsErrUserNotFound(err error) bool { diff --git a/src/internal/m365/graph/errors_test.go b/src/internal/m365/graph/errors_test.go index 72daef7a4..eea3b7ddd 100644 --- a/src/internal/m365/graph/errors_test.go +++ b/src/internal/m365/graph/errors_test.go @@ -101,7 +101,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrDeletedInFlight() { }, { name: "not-found oDataErr", - err: odErr(string(itemNotFound)), + err: odErr(string(errorItemNotFound)), expect: assert.True, }, { @@ -531,3 +531,42 @@ func (suite *GraphErrorsUnitSuite) TestGraphStack_labels() { }) } } + +func (suite *GraphErrorsUnitSuite) TestIsErrItemNotFound() { + table := []struct { + name string + err error + expect assert.BoolAssertionFunc + }{ + { + name: "nil", + err: nil, + expect: assert.False, + }, + { + name: "non-matching", + err: assert.AnError, + expect: assert.False, + }, + { + name: "as", + err: ErrInvalidDelta, + expect: assert.False, + }, + { + name: "non-matching oDataErr", + err: odErr("fnords"), + expect: assert.False, + }, + { + name: "item nott found oDataErr", + err: odErr(string(itemNotFound)), + expect: assert.True, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + test.expect(suite.T(), IsErrItemNotFound(test.err)) + }) + } +} diff --git a/src/pkg/services/m365/api/sites.go b/src/pkg/services/m365/api/sites.go index 4a43c87b6..e573cfc07 100644 --- a/src/pkg/services/m365/api/sites.go +++ b/src/pkg/services/m365/api/sites.go @@ -117,9 +117,21 @@ func (c Sites) GetByID(ctx context.Context, identifier string) (models.Siteable, ctx = clues.Add(ctx, "given_site_id", identifier) if siteIDRE.MatchString(identifier) { - resp, err = c.Stable.Client().Sites().BySiteId(identifier).Get(ctx, nil) + resp, err = c.Stable. + Client(). + Sites(). + BySiteId(identifier). + Get(ctx, nil) if err != nil { - return nil, graph.Wrap(ctx, err, "getting site by id") + err := graph.Wrap(ctx, err, "getting site by id") + + // a 404 when getting sites by ID returns an itemNotFound + // error code, instead of something more sensible. + if graph.IsErrItemNotFound(err) { + err = clues.Stack(graph.ErrResourceOwnerNotFound, err) + } + + return nil, err } return resp, err @@ -147,7 +159,15 @@ func (c Sites) GetByID(ctx context.Context, identifier string) (models.Siteable, NewItemSitesSiteItemRequestBuilder(rawURL, c.Stable.Adapter()). Get(ctx, nil) if err != nil { - return nil, graph.Wrap(ctx, err, "getting site by weburl") + err := graph.Wrap(ctx, err, "getting site by weburl") + + // a 404 when getting sites by ID returns an itemNotFound + // error code, instead of something more sensible. + if graph.IsErrItemNotFound(err) { + err = clues.Stack(graph.ErrResourceOwnerNotFound, err) + } + + return nil, err } return resp, err diff --git a/src/pkg/services/m365/api/sites_test.go b/src/pkg/services/m365/api/sites_test.go index d9f16a324..a99ff9cdc 100644 --- a/src/pkg/services/m365/api/sites_test.go +++ b/src/pkg/services/m365/api/sites_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/suite" "github.com/alcionai/corso/src/internal/common/ptr" + "github.com/alcionai/corso/src/internal/m365/graph" "github.com/alcionai/corso/src/internal/tester" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/services/m365/api" @@ -112,7 +113,7 @@ func TestSitesIntgSuite(t *testing.T) { suite.Run(t, &SitesIntgSuite{ Suite: tester.NewIntegrationSuite( t, - [][]string{tester.M365AcctCredEnvs, tester.AWSStorageCredEnvs}), + [][]string{tester.M365AcctCredEnvs}), }) } @@ -126,7 +127,9 @@ func (suite *SitesIntgSuite) TestGetAll() { ctx, flush := tester.NewContext(t) defer flush() - sites, err := suite.its.ac.Sites().GetAll(ctx, fault.New(true)) + sites, err := suite.its.ac. + Sites(). + GetAll(ctx, fault.New(true)) require.NoError(t, err) require.NotZero(t, len(sites), "must have at least one site") @@ -137,11 +140,12 @@ func (suite *SitesIntgSuite) TestGetAll() { func (suite *SitesIntgSuite) TestSites_GetByID() { var ( - t = suite.T() - siteID = tester.M365SiteID(t) - host = strings.Split(siteID, ",")[0] - shortID = strings.TrimPrefix(siteID, host+",") - siteURL = tester.M365SiteURL(t) + t = suite.T() + siteID = tester.M365SiteID(t) + host = strings.Split(siteID, ",")[0] + shortID = strings.TrimPrefix(siteID, host+",") + siteURL = tester.M365SiteURL(t) + modifiedSiteURL = siteURL + "foo" ) sitesAPI := suite.its.ac.Sites() @@ -149,26 +153,81 @@ func (suite *SitesIntgSuite) TestSites_GetByID() { table := []struct { name string id string - expectErr assert.ErrorAssertionFunc + expectErr func(*testing.T, error) }{ - {"3 part id", siteID, assert.NoError}, - {"2 part id", shortID, assert.NoError}, - {"malformed id", uuid.NewString(), assert.Error}, - {"random id", uuid.NewString() + "," + uuid.NewString(), assert.Error}, - {"url", siteURL, assert.NoError}, - {"host only", host, assert.NoError}, - {"malformed url", "barunihlda", assert.Error}, - {"non-matching url", "https://test/sites/testing", assert.Error}, + { + name: "3 part id", + id: siteID, + expectErr: func(t *testing.T, err error) { + assert.NoError(t, err, clues.ToCore(err)) + }, + }, + { + name: "2 part id", + id: shortID, + expectErr: func(t *testing.T, err error) { + assert.NoError(t, err, clues.ToCore(err)) + }, + }, + { + name: "malformed id", + id: uuid.NewString(), + expectErr: func(t *testing.T, err error) { + assert.Error(t, err, clues.ToCore(err)) + }, + }, + { + name: "random id", + id: uuid.NewString() + "," + uuid.NewString(), + expectErr: func(t *testing.T, err error) { + assert.ErrorIs(t, err, graph.ErrResourceOwnerNotFound, clues.ToCore(err)) + }, + }, + { + name: "url", + id: siteURL, + expectErr: func(t *testing.T, err error) { + assert.NoError(t, err, clues.ToCore(err)) + }, + }, + { + name: "host only", + id: host, + expectErr: func(t *testing.T, err error) { + assert.NoError(t, err, clues.ToCore(err)) + }, + }, + { + name: "malformed url", + id: "barunihlda", + expectErr: func(t *testing.T, err error) { + assert.Error(t, err, clues.ToCore(err)) + }, + }, + { + name: "well formed url, invalid hostname", + id: "https://test/sites/testing", + expectErr: func(t *testing.T, err error) { + assert.Error(t, err, clues.ToCore(err)) + }, + }, + { + name: "well formed url, no sites match", + id: modifiedSiteURL, + expectErr: func(t *testing.T, err error) { + assert.ErrorIs(t, err, graph.ErrResourceOwnerNotFound, clues.ToCore(err)) + }, + }, } for _, test := range table { suite.Run(test.name, func() { + t := suite.T() + ctx, flush := tester.NewContext(t) defer flush() - t := suite.T() - _, err := sitesAPI.GetByID(ctx, test.id) - test.expectErr(t, err, clues.ToCore(err)) + test.expectErr(t, err) }) } }