From 2132b3c789b7314a606f4dde47bdd1c0df5a5d36 Mon Sep 17 00:00:00 2001 From: Keepers Date: Tue, 27 Jun 2023 10:17:37 -0600 Subject: [PATCH] properly handle collision replace behavior (#3636) completes the item collision handling behavior in exchange by turning replace handling into a post-delete process. --- #### Does this PR need a docs update or release note? - [x] :no_entry: No #### Type of change - [x] :sunflower: Feature #### Issue(s) * #3562 #### Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- .../m365/exchange/contacts_restore.go | 37 ++++++-- .../m365/exchange/contacts_restore_test.go | 68 +++++++++++---- src/internal/m365/exchange/events_restore.go | 28 ++++-- .../m365/exchange/events_restore_test.go | 85 +++++++++++++------ src/internal/m365/exchange/handlers.go | 11 +++ src/internal/m365/exchange/mail_restore.go | 27 +++++- .../m365/exchange/mail_restore_test.go | 70 +++++++++++---- 7 files changed, 252 insertions(+), 74 deletions(-) diff --git a/src/internal/m365/exchange/contacts_restore.go b/src/internal/m365/exchange/contacts_restore.go index 076cd4a22..c4b2c7681 100644 --- a/src/internal/m365/exchange/contacts_restore.go +++ b/src/internal/m365/exchange/contacts_restore.go @@ -79,9 +79,14 @@ func (h contactRestoreHandler) restore( errs) } +type contactRestorer interface { + postItemer[models.Contactable] + deleteItemer +} + func restoreContact( ctx context.Context, - pi postItemer[models.Contactable], + cr contactRestorer, body []byte, userID, destinationID string, collisionKeyToItemID map[string]string, @@ -94,22 +99,40 @@ func restoreContact( } ctx = clues.Add(ctx, "item_id", ptr.Val(contact.GetId())) - collisionKey := api.ContactCollisionKey(contact) - if _, ok := collisionKeyToItemID[collisionKey]; ok { + var ( + collisionKey = api.ContactCollisionKey(contact) + collisionID string + shouldDeleteOriginal bool + ) + + if id, ok := collisionKeyToItemID[collisionKey]; ok { log := logger.Ctx(ctx).With("collision_key", clues.Hide(collisionKey)) log.Debug("item collision") - // TODO(rkeepers): Replace probably shouldn't no-op. Just a starting point. - if collisionPolicy == control.Skip || collisionPolicy == control.Replace { + if collisionPolicy == control.Skip { log.Debug("skipping item with collision") return nil, graph.ErrItemAlreadyExistsConflict } + + collisionID = id + shouldDeleteOriginal = collisionPolicy == control.Replace } - item, err := pi.PostItem(ctx, userID, destinationID, contact) + item, err := cr.PostItem(ctx, userID, destinationID, contact) if err != nil { - return nil, graph.Wrap(ctx, err, "restoring mail message") + return nil, graph.Wrap(ctx, err, "restoring contact") + } + + // contacts have no PUT request, and PATCH could retain data that's not + // associated with the backup item state. Instead of updating, we + // post first, then delete. In case of failure between the two calls, + // at least we'll have accidentally over-produced data instead of deleting + // the user's data. + if shouldDeleteOriginal { + if err := cr.DeleteItem(ctx, userID, collisionID); err != nil { + return nil, graph.Wrap(ctx, err, "deleting colliding contact") + } } info := api.ContactInfo(item) diff --git a/src/internal/m365/exchange/contacts_restore_test.go b/src/internal/m365/exchange/contacts_restore_test.go index 6ed6a3c5e..7bd3fd5b0 100644 --- a/src/internal/m365/exchange/contacts_restore_test.go +++ b/src/internal/m365/exchange/contacts_restore_test.go @@ -20,20 +20,32 @@ import ( "github.com/alcionai/corso/src/pkg/services/m365/api" ) -var _ postItemer[models.Contactable] = &mockContactRestorer{} +var _ contactRestorer = &contactRestoreMock{} -type mockContactRestorer struct { - postItemErr error +type contactRestoreMock struct { + postItemErr error + calledPost bool + deleteItemErr error + calledDelete bool } -func (m mockContactRestorer) PostItem( - ctx context.Context, - userID, containerID string, - body models.Contactable, +func (m *contactRestoreMock) PostItem( + _ context.Context, + _, _ string, + _ models.Contactable, ) (models.Contactable, error) { + m.calledPost = true return models.NewContact(), m.postItemErr } +func (m *contactRestoreMock) DeleteItem( + _ context.Context, + _, _ string, +) error { + m.calledDelete = true + return m.deleteItemErr +} + // --------------------------------------------------------------------------- // tests // --------------------------------------------------------------------------- @@ -78,63 +90,88 @@ func (suite *ContactsRestoreIntgSuite) TestRestoreContact() { table := []struct { name string - apiMock postItemer[models.Contactable] + apiMock *contactRestoreMock collisionMap map[string]string onCollision control.CollisionPolicy expectErr func(*testing.T, error) + expectMock func(*testing.T, *contactRestoreMock) }{ { name: "no collision: skip", - apiMock: mockContactRestorer{}, + apiMock: &contactRestoreMock{}, collisionMap: map[string]string{}, onCollision: control.Copy, expectErr: func(t *testing.T, err error) { assert.NoError(t, err, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *contactRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "no collision: copy", - apiMock: mockContactRestorer{}, + apiMock: &contactRestoreMock{}, collisionMap: map[string]string{}, onCollision: control.Skip, expectErr: func(t *testing.T, err error) { assert.NoError(t, err, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *contactRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "no collision: replace", - apiMock: mockContactRestorer{}, + apiMock: &contactRestoreMock{}, collisionMap: map[string]string{}, onCollision: control.Replace, expectErr: func(t *testing.T, err error) { assert.NoError(t, err, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *contactRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "collision: skip", - apiMock: mockContactRestorer{}, + apiMock: &contactRestoreMock{}, collisionMap: map[string]string{collisionKey: "smarf"}, onCollision: control.Skip, expectErr: func(t *testing.T, err error) { assert.ErrorIs(t, err, graph.ErrItemAlreadyExistsConflict, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *contactRestoreMock) { + assert.False(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "collision: copy", - apiMock: mockContactRestorer{}, + apiMock: &contactRestoreMock{}, collisionMap: map[string]string{collisionKey: "smarf"}, onCollision: control.Copy, expectErr: func(t *testing.T, err error) { assert.NoError(t, err, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *contactRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "collision: replace", - apiMock: mockContactRestorer{}, + apiMock: &contactRestoreMock{}, collisionMap: map[string]string{collisionKey: "smarf"}, onCollision: control.Replace, expectErr: func(t *testing.T, err error) { - assert.ErrorIs(t, err, graph.ErrItemAlreadyExistsConflict, clues.ToCore(err)) + assert.NoError(t, err, clues.ToCore(err)) + }, + expectMock: func(t *testing.T, m *contactRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.True(t, m.calledDelete, "old item deleted") }, }, } @@ -156,6 +193,7 @@ func (suite *ContactsRestoreIntgSuite) TestRestoreContact() { fault.New(true)) test.expectErr(t, err) + test.expectMock(t, test.apiMock) }) } } diff --git a/src/internal/m365/exchange/events_restore.go b/src/internal/m365/exchange/events_restore.go index 8ccb1232c..fde55110b 100644 --- a/src/internal/m365/exchange/events_restore.go +++ b/src/internal/m365/exchange/events_restore.go @@ -105,17 +105,24 @@ func restoreEvent( } ctx = clues.Add(ctx, "item_id", ptr.Val(event.GetId())) - collisionKey := api.EventCollisionKey(event) - if _, ok := collisionKeyToItemID[collisionKey]; ok { + var ( + collisionKey = api.EventCollisionKey(event) + collisionID string + shouldDeleteOriginal bool + ) + + if id, ok := collisionKeyToItemID[collisionKey]; ok { log := logger.Ctx(ctx).With("collision_key", clues.Hide(collisionKey)) log.Debug("item collision") - // TODO(rkeepers): Replace probably shouldn't no-op. Just a starting point. - if collisionPolicy == control.Skip || collisionPolicy == control.Replace { + if collisionPolicy == control.Skip { log.Debug("skipping item with collision") return nil, graph.ErrItemAlreadyExistsConflict } + + collisionID = id + shouldDeleteOriginal = collisionPolicy == control.Replace } event = toEventSimplified(event) @@ -131,7 +138,18 @@ func restoreEvent( item, err := er.PostItem(ctx, userID, destinationID, event) if err != nil { - return nil, graph.Wrap(ctx, err, "restoring calendar item") + return nil, graph.Wrap(ctx, err, "restoring event") + } + + // events have no PUT request, and PATCH could retain data that's not + // associated with the backup item state. Instead of updating, we + // post first, then delete. In case of failure between the two calls, + // at least we'll have accidentally over-produced data instead of deleting + // the user's data. + if shouldDeleteOriginal { + if err := er.DeleteItem(ctx, userID, collisionID); err != nil { + return nil, graph.Wrap(ctx, err, "deleting colliding event") + } } err = uploadAttachments( diff --git a/src/internal/m365/exchange/events_restore_test.go b/src/internal/m365/exchange/events_restore_test.go index ddd9983b8..489e2c416 100644 --- a/src/internal/m365/exchange/events_restore_test.go +++ b/src/internal/m365/exchange/events_restore_test.go @@ -21,22 +21,34 @@ import ( "github.com/alcionai/corso/src/pkg/services/m365/api" ) -var _ eventRestorer = &mockEventRestorer{} +var _ eventRestorer = &eventRestoreMock{} -type mockEventRestorer struct { +type eventRestoreMock struct { postItemErr error + calledPost bool + deleteItemErr error + calledDelete bool postAttachmentErr error } -func (m mockEventRestorer) PostItem( - ctx context.Context, - userID, containerID string, - body models.Eventable, +func (m *eventRestoreMock) PostItem( + _ context.Context, + _, _ string, + _ models.Eventable, ) (models.Eventable, error) { + m.calledPost = true return models.NewEvent(), m.postItemErr } -func (m mockEventRestorer) PostSmallAttachment( +func (m *eventRestoreMock) DeleteItem( + _ context.Context, + _, _ string, +) error { + m.calledDelete = true + return m.deleteItemErr +} + +func (m *eventRestoreMock) PostSmallAttachment( _ context.Context, _, _, _ string, _ models.Attachmentable, @@ -44,7 +56,7 @@ func (m mockEventRestorer) PostSmallAttachment( return m.postAttachmentErr } -func (m mockEventRestorer) PostLargeAttachment( +func (m *eventRestoreMock) PostLargeAttachment( _ context.Context, _, _, _, _ string, _ []byte, @@ -52,21 +64,14 @@ func (m mockEventRestorer) PostLargeAttachment( return uuid.NewString(), m.postAttachmentErr } -func (m mockEventRestorer) DeleteAttachment( +func (m *eventRestoreMock) DeleteAttachment( ctx context.Context, userID, calendarID, eventID, attachmentID string, ) error { return nil } -func (m mockEventRestorer) DeleteItem( - ctx context.Context, - userID, itemID string, -) error { - return nil -} - -func (m mockEventRestorer) GetAttachments( +func (m *eventRestoreMock) GetAttachments( _ context.Context, _ bool, _, _ string, @@ -74,14 +79,14 @@ func (m mockEventRestorer) GetAttachments( return []models.Attachmentable{}, nil } -func (m mockEventRestorer) GetItemInstances( +func (m *eventRestoreMock) GetItemInstances( _ context.Context, _, _, _, _ string, ) ([]models.Eventable, error) { return []models.Eventable{}, nil } -func (m mockEventRestorer) PatchItem( +func (m *eventRestoreMock) PatchItem( _ context.Context, _, _ string, _ models.Eventable, @@ -133,63 +138,88 @@ func (suite *EventsRestoreIntgSuite) TestRestoreEvent() { table := []struct { name string - apiMock eventRestorer + apiMock *eventRestoreMock collisionMap map[string]string onCollision control.CollisionPolicy expectErr func(*testing.T, error) + expectMock func(*testing.T, *eventRestoreMock) }{ { name: "no collision: skip", - apiMock: mockEventRestorer{}, + apiMock: &eventRestoreMock{}, collisionMap: map[string]string{}, onCollision: control.Copy, expectErr: func(t *testing.T, err error) { assert.NoError(t, err, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *eventRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "no collision: copy", - apiMock: mockEventRestorer{}, + apiMock: &eventRestoreMock{}, collisionMap: map[string]string{}, onCollision: control.Skip, expectErr: func(t *testing.T, err error) { assert.NoError(t, err, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *eventRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "no collision: replace", - apiMock: mockEventRestorer{}, + apiMock: &eventRestoreMock{}, collisionMap: map[string]string{}, onCollision: control.Replace, expectErr: func(t *testing.T, err error) { assert.NoError(t, err, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *eventRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "collision: skip", - apiMock: mockEventRestorer{}, + apiMock: &eventRestoreMock{}, collisionMap: map[string]string{collisionKey: "smarf"}, onCollision: control.Skip, expectErr: func(t *testing.T, err error) { assert.ErrorIs(t, err, graph.ErrItemAlreadyExistsConflict, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *eventRestoreMock) { + assert.False(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "collision: copy", - apiMock: mockEventRestorer{}, + apiMock: &eventRestoreMock{}, collisionMap: map[string]string{collisionKey: "smarf"}, onCollision: control.Copy, expectErr: func(t *testing.T, err error) { assert.NoError(t, err, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *eventRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "collision: replace", - apiMock: mockEventRestorer{}, + apiMock: &eventRestoreMock{}, collisionMap: map[string]string{collisionKey: "smarf"}, onCollision: control.Replace, expectErr: func(t *testing.T, err error) { - assert.ErrorIs(t, err, graph.ErrItemAlreadyExistsConflict, clues.ToCore(err)) + assert.NoError(t, err, clues.ToCore(err)) + }, + expectMock: func(t *testing.T, m *eventRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.True(t, m.calledDelete, "old item deleted") }, }, } @@ -211,6 +241,7 @@ func (suite *EventsRestoreIntgSuite) TestRestoreEvent() { fault.New(true)) test.expectErr(t, err) + test.expectMock(t, test.apiMock) }) } } diff --git a/src/internal/m365/exchange/handlers.go b/src/internal/m365/exchange/handlers.go index 243ad11fd..ccc8b2c2e 100644 --- a/src/internal/m365/exchange/handlers.go +++ b/src/internal/m365/exchange/handlers.go @@ -138,6 +138,10 @@ type getItemsByCollisionKeyser interface { ) (map[string]string, error) } +// --------------------------------------------------------------------------- +// other interfaces +// --------------------------------------------------------------------------- + type postItemer[T any] interface { PostItem( ctx context.Context, @@ -145,3 +149,10 @@ type postItemer[T any] interface { body T, ) (T, error) } + +type deleteItemer interface { + DeleteItem( + ctx context.Context, + userID, itemID string, + ) error +} diff --git a/src/internal/m365/exchange/mail_restore.go b/src/internal/m365/exchange/mail_restore.go index 1ddb3e52b..cba904df8 100644 --- a/src/internal/m365/exchange/mail_restore.go +++ b/src/internal/m365/exchange/mail_restore.go @@ -86,6 +86,7 @@ func (h mailRestoreHandler) restore( type mailRestorer interface { postItemer[models.Messageable] + deleteItemer attachmentPoster } @@ -104,17 +105,24 @@ func restoreMail( } ctx = clues.Add(ctx, "item_id", ptr.Val(msg.GetId())) - collisionKey := api.MailCollisionKey(msg) - if _, ok := collisionKeyToItemID[collisionKey]; ok { + var ( + collisionKey = api.MailCollisionKey(msg) + collisionID string + shouldDeleteOriginal bool + ) + + if id, ok := collisionKeyToItemID[collisionKey]; ok { log := logger.Ctx(ctx).With("collision_key", clues.Hide(collisionKey)) log.Debug("item collision") - // TODO(rkeepers): Replace probably shouldn't no-op. Just a starting point. - if collisionPolicy == control.Skip || collisionPolicy == control.Replace { + if collisionPolicy == control.Skip { log.Debug("skipping item with collision") return nil, graph.ErrItemAlreadyExistsConflict } + + collisionID = id + shouldDeleteOriginal = collisionPolicy == control.Replace } msg = setMessageSVEPs(toMessage(msg)) @@ -128,6 +136,17 @@ func restoreMail( return nil, graph.Wrap(ctx, err, "restoring mail message") } + // mails have no PUT request, and PATCH could retain data that's not + // associated with the backup item state. Instead of updating, we + // post first, then delete. In case of failure between the two calls, + // at least we'll have accidentally over-produced data instead of deleting + // the user's data. + if shouldDeleteOriginal { + if err := mr.DeleteItem(ctx, userID, collisionID); err != nil { + return nil, graph.Wrap(ctx, err, "deleting colliding mail message") + } + } + err = uploadAttachments( ctx, mr, diff --git a/src/internal/m365/exchange/mail_restore_test.go b/src/internal/m365/exchange/mail_restore_test.go index fc2837c85..43791704c 100644 --- a/src/internal/m365/exchange/mail_restore_test.go +++ b/src/internal/m365/exchange/mail_restore_test.go @@ -21,22 +21,34 @@ import ( "github.com/alcionai/corso/src/pkg/services/m365/api" ) -var _ mailRestorer = &mockMailRestorer{} +var _ mailRestorer = &mailRestoreMock{} -type mockMailRestorer struct { +type mailRestoreMock struct { postItemErr error + calledPost bool + deleteItemErr error + calledDelete bool postAttachmentErr error } -func (m mockMailRestorer) PostItem( - ctx context.Context, - userID, containerID string, - body models.Messageable, +func (m *mailRestoreMock) PostItem( + _ context.Context, + _, _ string, + _ models.Messageable, ) (models.Messageable, error) { + m.calledPost = true return models.NewMessage(), m.postItemErr } -func (m mockMailRestorer) PostSmallAttachment( +func (m *mailRestoreMock) DeleteItem( + _ context.Context, + _, _ string, +) error { + m.calledDelete = true + return m.deleteItemErr +} + +func (m *mailRestoreMock) PostSmallAttachment( _ context.Context, _, _, _ string, _ models.Attachmentable, @@ -44,7 +56,7 @@ func (m mockMailRestorer) PostSmallAttachment( return m.postAttachmentErr } -func (m mockMailRestorer) PostLargeAttachment( +func (m *mailRestoreMock) PostLargeAttachment( _ context.Context, _, _, _, _ string, _ []byte, @@ -95,63 +107,88 @@ func (suite *MailRestoreIntgSuite) TestRestoreMail() { table := []struct { name string - apiMock mailRestorer + apiMock *mailRestoreMock collisionMap map[string]string onCollision control.CollisionPolicy expectErr func(*testing.T, error) + expectMock func(*testing.T, *mailRestoreMock) }{ { name: "no collision: skip", - apiMock: mockMailRestorer{}, + apiMock: &mailRestoreMock{}, collisionMap: map[string]string{}, onCollision: control.Copy, expectErr: func(t *testing.T, err error) { assert.NoError(t, err, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *mailRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "no collision: copy", - apiMock: mockMailRestorer{}, + apiMock: &mailRestoreMock{}, collisionMap: map[string]string{}, onCollision: control.Skip, expectErr: func(t *testing.T, err error) { assert.NoError(t, err, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *mailRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "no collision: replace", - apiMock: mockMailRestorer{}, + apiMock: &mailRestoreMock{}, collisionMap: map[string]string{}, onCollision: control.Replace, expectErr: func(t *testing.T, err error) { assert.NoError(t, err, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *mailRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "collision: skip", - apiMock: mockMailRestorer{}, + apiMock: &mailRestoreMock{}, collisionMap: map[string]string{collisionKey: "smarf"}, onCollision: control.Skip, expectErr: func(t *testing.T, err error) { assert.ErrorIs(t, err, graph.ErrItemAlreadyExistsConflict, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *mailRestoreMock) { + assert.False(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "collision: copy", - apiMock: mockMailRestorer{}, + apiMock: &mailRestoreMock{}, collisionMap: map[string]string{collisionKey: "smarf"}, onCollision: control.Copy, expectErr: func(t *testing.T, err error) { assert.NoError(t, err, clues.ToCore(err)) }, + expectMock: func(t *testing.T, m *mailRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.False(t, m.calledDelete, "old item deleted") + }, }, { name: "collision: replace", - apiMock: mockMailRestorer{}, + apiMock: &mailRestoreMock{}, collisionMap: map[string]string{collisionKey: "smarf"}, onCollision: control.Replace, expectErr: func(t *testing.T, err error) { - assert.ErrorIs(t, err, graph.ErrItemAlreadyExistsConflict, clues.ToCore(err)) + assert.NoError(t, err, clues.ToCore(err)) + }, + expectMock: func(t *testing.T, m *mailRestoreMock) { + assert.True(t, m.calledPost, "new item posted") + assert.True(t, m.calledDelete, "old item deleted") }, }, } @@ -173,6 +210,7 @@ func (suite *MailRestoreIntgSuite) TestRestoreMail() { fault.New(true)) test.expectErr(t, err) + test.expectMock(t, test.apiMock) }) } }