From 5b9cd69e292ca4c3be4ed7cb2d775b8fa5261ea9 Mon Sep 17 00:00:00 2001 From: Abhishek Pandey Date: Fri, 5 May 2023 12:43:07 -0700 Subject: [PATCH] Fetch calendar by name on 409 ErrorFolderExist (#3318) Handle 409, `ErrorFolderExists` error in restore path while creating destination calendar. We need to fix this for all m365 services. This PR is focused on calendar only. Context: `CreateCalendar()` may fail with ErrorFolderExists under certain error conditions. For e.g. consider below scenario. 1. `CreateCalendar()` does a POST to graph to create restore destination calendar but this fails with 5xx. 2. It's possible that step 1 may have left some dirty state in graph. For e.g. it's possible that the destination folder in step 1 was actually created, but 5xx was returned due to other reasons. 3. So when we reattempt POST in such a scenario, we sometimes observe ErrorFolderExists error . 4. Corso should be resilient to such errors. To fix this, when we encounter such an error, we will do a GET to fetch the restore destination folder and add it to folder cache. --- #### 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 - [x] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup #### Issue(s) * # #### Test Plan Integration tests and mock tests will be added in a follow up PR. - [x] :muscle: Manual - [ ] :zap: Unit test - [ ] :green_heart: E2E --- src/internal/connector/exchange/api/events.go | 39 ++++++++++++++++ .../connector/exchange/service_restore.go | 12 ++++- src/internal/connector/graph/errors.go | 8 ++++ src/internal/connector/graph/errors_test.go | 45 +++++++++++++++++++ 4 files changed, 103 insertions(+), 1 deletion(-) diff --git a/src/internal/connector/exchange/api/events.go b/src/internal/connector/exchange/api/events.go index 8ec6c758b..cdf05d778 100644 --- a/src/internal/connector/exchange/api/events.go +++ b/src/internal/connector/exchange/api/events.go @@ -99,6 +99,45 @@ func (c Events) GetContainerByID( return graph.CalendarDisplayable{Calendarable: cal}, nil } +// GetContainerByName fetches a calendar by name +func (c Events) GetContainerByName( + ctx context.Context, + userID, name string, +) (models.Calendarable, error) { + filter := fmt.Sprintf("name eq '%s'", name) + options := &users.ItemCalendarsRequestBuilderGetRequestConfiguration{ + QueryParameters: &users.ItemCalendarsRequestBuilderGetQueryParameters{ + Filter: &filter, + }, + } + + ctx = clues.Add(ctx, "calendar_name", name) + + resp, err := c.Stable.Client().UsersById(userID).Calendars().Get(ctx, options) + if err != nil { + return nil, graph.Stack(ctx, err).WithClues(ctx) + } + + // We only allow the api to match one calendar with provided name. + // Return an error if multiple calendars exist (unlikely) or if no calendar + // is found. + if len(resp.GetValue()) != 1 { + err = clues.New("unexpected number of calendars returned"). + With("returned_calendar_count", len(resp.GetValue())) + return nil, err + } + + // Sanity check ID and name + cal := resp.GetValue()[0] + cd := CalendarDisplayable{Calendarable: cal} + + if err := checkIDAndName(cd); err != nil { + return nil, err + } + + return cal, nil +} + // GetItem retrieves an Eventable item. func (c Events) GetItem( ctx context.Context, diff --git a/src/internal/connector/exchange/service_restore.go b/src/internal/connector/exchange/service_restore.go index 82a0f0fca..4d49e3df9 100644 --- a/src/internal/connector/exchange/service_restore.go +++ b/src/internal/connector/exchange/service_restore.go @@ -689,10 +689,20 @@ func establishEventsRestoreLocation( ctx = clues.Add(ctx, "is_new_cache", isNewCache) temp, err := ac.Events().CreateCalendar(ctx, user, folders[0]) - if err != nil { + if err != nil && !graph.IsErrFolderExists(err) { return "", err } + // 409 handling: Fetch folder if it exists and add to cache. + // This is rare, but may happen if CreateCalendar() POST fails with 5xx, + // potentially leaving dirty state in graph. + if graph.IsErrFolderExists(err) { + temp, err = ac.Events().GetContainerByName(ctx, user, folders[0]) + if err != nil { + return "", err + } + } + folderID := ptr.Val(temp.GetId()) if isNewCache { diff --git a/src/internal/connector/graph/errors.go b/src/internal/connector/graph/errors.go index 527465621..70f2dd416 100644 --- a/src/internal/connector/graph/errors.go +++ b/src/internal/connector/graph/errors.go @@ -41,6 +41,10 @@ const ( syncFolderNotFound errorCode = "ErrorSyncFolderNotFound" syncStateInvalid errorCode = "SyncStateInvalid" syncStateNotFound errorCode = "SyncStateNotFound" + // This error occurs when an attempt is made to create a folder that has + // the same name as another folder in the same parent. Such duplicate folder + // names are not allowed by graph. + folderExists errorCode = "ErrorFolderExists" ) type errorMessage string @@ -178,6 +182,10 @@ func IsMalwareResp(ctx context.Context, resp *http.Response) bool { return false } +func IsErrFolderExists(err error) bool { + return hasErrorCode(err, folderExists) +} + // --------------------------------------------------------------------------- // error parsers // --------------------------------------------------------------------------- diff --git a/src/internal/connector/graph/errors_test.go b/src/internal/connector/graph/errors_test.go index 271b66717..8706834e7 100644 --- a/src/internal/connector/graph/errors_test.go +++ b/src/internal/connector/graph/errors_test.go @@ -300,3 +300,48 @@ func (suite *GraphErrorsUnitSuite) TestMalwareInfo() { assert.Equal(suite.T(), expect, ItemInfo(&i)) } + +func (suite *GraphErrorsUnitSuite) TestIsErrFolderExists() { + 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: "non-matching oDataErr", + err: odErr("folder doesn't exist"), + expect: assert.False, + }, + { + name: "matching oDataErr", + err: odErr(string(folderExists)), + expect: assert.True, + }, + // next two tests are to make sure the checks are case insensitive + { + name: "oDataErr camelcase", + err: odErr("ErrorFolderExists"), + expect: assert.True, + }, + { + name: "oDataErr lowercase", + err: odErr("errorfolderexists"), + expect: assert.True, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + test.expect(suite.T(), IsErrFolderExists(test.err)) + }) + } +}