From fad3dfe769a6028642b07659fb6b24e5a07bd864 Mon Sep 17 00:00:00 2001 From: Abhishek Pandey Date: Tue, 9 Jan 2024 20:49:04 -0800 Subject: [PATCH] Download attachments for group mailbox posts (#4992) * Attachment download needed an expand operation. Added that. * Added test coverage with gock. Tested with manual backup of posts which contain attachments(embedded/non-embedded). We can't add e2e tests with attachments, since API to create new conversations requires delegated access. * Note that https://github.com/alcionai/corso/issues/4991 is still an open item. We don't have a resolution for it right now, since attachment endpoint requires delegated token. Defaulting to let the backup fail for "too many attachments" error case. We don't know yet if we'd see that with group mailboxes, and whether it'd be the same error that we saw with exchange (recurring 503s). --- #### Does this PR need a docs update or release note? - [x] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [ ] :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 - [x] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- CHANGELOG.md | 4 + src/pkg/services/m365/api/conversations.go | 88 ++++++++++++++++--- .../services/m365/api/conversations_test.go | 84 +++++++++++++++++- website/docs/support/known-issues.md | 4 +- 4 files changed, 163 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d60c0e01..f849a2758 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,10 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Retry transient 400 "invalidRequest" errors during onedrive & sharepoint backup. +- Backup attachments associated with group mailbox items. ### Changed - When running `backup details` on an empty backup returns a more helpful error message. +### Known issues +- Backing up a group mailbox item may fail if it has a very large number of attachments (500+). + ## [v0.18.0] (beta) - 2024-01-02 ### Fixed diff --git a/src/pkg/services/m365/api/conversations.go b/src/pkg/services/m365/api/conversations.go index f58fbec81..ebfafe619 100644 --- a/src/pkg/services/m365/api/conversations.go +++ b/src/pkg/services/m365/api/conversations.go @@ -11,6 +11,7 @@ import ( "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/common/str" "github.com/alcionai/corso/src/pkg/backup/details" + "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" ) @@ -63,7 +64,38 @@ func (c Conversations) GetConversationPost( return nil, nil, graph.Stack(ctx, err) } - return post, conversationPostInfo(post), graph.Stack(ctx, err).OrNil() + preview, contentLen, err := getConversationPostContentPreview(post) + if err != nil { + preview = "malformed or unparseable content body: " + preview + } + + if !ptr.Val(post.GetHasAttachments()) && !HasAttachments(post.GetBody()) { + return post, conversationPostInfo(post, contentLen, preview), nil + } + + attachments, totalSize, err := c.getAttachments( + ctx, + groupID, + conversationID, + threadID, + postID) + if err != nil { + // Similar to exchange, a failure can happen if a post has a lot of attachments. + // We don't have a fallback option here to fetch attachments one by one. See + // issue #4991. + // + // Resort to failing the post backup for now since we don't know yet how this + // error might manifest itself for posts. + logger.CtxErr(ctx, err).Info("failed to get post attachments") + + return nil, nil, clues.Stack(err) + } + + contentLen += totalSize + + post.SetAttachments(attachments) + + return post, conversationPostInfo(post, contentLen, preview), graph.Stack(ctx, err).OrNil() } // --------------------------------------------------------------------------- @@ -72,27 +104,18 @@ func (c Conversations) GetConversationPost( func conversationPostInfo( post models.Postable, + size int64, + preview string, ) *details.GroupsInfo { if post == nil { return nil } - preview, contentLen, err := getConversationPostContentPreview(post) - if err != nil { - preview = "malformed or unparseable html" + preview - } - var sender string if post.GetSender() != nil && post.GetSender().GetEmailAddress() != nil { sender = ptr.Val(post.GetSender().GetEmailAddress().GetAddress()) } - size := contentLen - - for _, a := range post.GetAttachments() { - size += int64(ptr.Val(a.GetSize())) - } - cpi := details.ConversationPostInfo{ CreatedAt: ptr.Val(post.GetCreatedDateTime()), Creator: sender, @@ -128,3 +151,44 @@ func stripConversationPostHTML(post models.Postable) (string, int64, error) { return content, origSize, clues.Stack(err).OrNil() } + +// getAttachments attempts to get all attachments, including their content, in a singe query. +func (c Conversations) getAttachments( + ctx context.Context, + groupID, conversationID, threadID, postID string, +) ([]models.Attachmentable, int64, error) { + var ( + result = []models.Attachmentable{} + totalSize int64 + ) + + cfg := &groups.ItemConversationsItemThreadsItemPostsPostItemRequestBuilderGetRequestConfiguration{ + QueryParameters: &groups.ItemConversationsItemThreadsItemPostsPostItemRequestBuilderGetQueryParameters{ + Expand: []string{"attachments"}, + }, + } + + post, err := c.LargeItem. + Client(). + Groups(). + ByGroupId(groupID). + Conversations(). + ByConversationId(conversationID). + Threads(). + ByConversationThreadId(threadID). + Posts(). + ByPostId(postID). + Get(ctx, cfg) + if err != nil { + return nil, 0, graph.Stack(ctx, err) + } + + attachments := post.GetAttachments() + + for _, a := range attachments { + totalSize += int64(ptr.Val(a.GetSize())) + result = append(result, a) + } + + return result, totalSize, nil +} diff --git a/src/pkg/services/m365/api/conversations_test.go b/src/pkg/services/m365/api/conversations_test.go index da63b9b8d..a5ac24de7 100644 --- a/src/pkg/services/m365/api/conversations_test.go +++ b/src/pkg/services/m365/api/conversations_test.go @@ -108,8 +108,8 @@ func (suite *ConversationsAPIUnitSuite) TestConversationPostInfo() { suite.Run(test.name, func() { t := suite.T() - chMsg, expected := test.postAndInfo() - result := conversationPostInfo(chMsg) + post, expected := test.postAndInfo() + result := conversationPostInfo(post, 0, "") assert.Equal(t, expected, result) }) @@ -138,6 +138,7 @@ func (suite *ConversationAPIIntgSuite) SetupSuite() { func (suite *ConversationAPIIntgSuite) TestConversations_attachmentListDownload() { pid := "fake-post-id" aid := "fake-attachment-id" + contentWithAttachment := "" tests := []struct { name string @@ -173,6 +174,19 @@ func (suite *ConversationAPIIntgSuite) TestConversations_attachmentListDownload( itm.SetId(&pid) itm.SetHasAttachments(ptr.To(true)) + // First call to get the post will not expand attachments. + interceptV1Path( + "groups", + "group", + "conversations", + "conv", + "threads", + "thread", + "posts", + pid). + Reply(200). + JSON(graphTD.ParseableToMap(suite.T(), itm)) + attch := models.NewAttachment() attch.SetSize(ptr.To[int32](50)) @@ -187,6 +201,7 @@ func (suite *ConversationAPIIntgSuite) TestConversations_attachmentListDownload( "thread", "posts", pid). + MatchParam("$expand", "attachments"). Reply(200). JSON(graphTD.ParseableToMap(suite.T(), itm)) }, @@ -197,10 +212,22 @@ func (suite *ConversationAPIIntgSuite) TestConversations_attachmentListDownload( { name: "fetch multiple individual attachments", setupf: func() { - truthy := true itm := models.NewPost() + itm.SetId(&pid) - itm.SetHasAttachments(&truthy) + itm.SetHasAttachments(ptr.To(true)) + + interceptV1Path( + "groups", + "group", + "conversations", + "conv", + "threads", + "thread", + "posts", + pid). + Reply(200). + JSON(graphTD.ParseableToMap(suite.T(), itm)) attch := models.NewAttachment() attch.SetId(&aid) @@ -217,6 +244,7 @@ func (suite *ConversationAPIIntgSuite) TestConversations_attachmentListDownload( "thread", "posts", pid). + MatchParam("$expand", "attachments"). Reply(200). JSON(graphTD.ParseableToMap(suite.T(), itm)) }, @@ -224,6 +252,54 @@ func (suite *ConversationAPIIntgSuite) TestConversations_attachmentListDownload( size: 1000, expect: assert.NoError, }, + { + name: "embedded attachment", + setupf: func() { + itm := models.NewPost() + itm.SetId(&pid) + + body := models.NewItemBody() + body.SetContentType(ptr.To(models.HTML_BODYTYPE)) + + // Test html content with embedded attachment. + + body.SetContent(ptr.To(contentWithAttachment)) + + itm.SetBody(body) + + interceptV1Path( + "groups", + "group", + "conversations", + "conv", + "threads", + "thread", + "posts", + pid). + Reply(200). + JSON(graphTD.ParseableToMap(suite.T(), itm)) + + attch := models.NewAttachment() + attch.SetSize(ptr.To[int32](50)) + itm.SetAttachments([]models.Attachmentable{attch}) + + interceptV1Path( + "groups", + "group", + "conversations", + "conv", + "threads", + "thread", + "posts", + pid). + MatchParam("$expand", "attachments"). + Reply(200). + JSON(graphTD.ParseableToMap(suite.T(), itm)) + }, + attachmentCount: 1, + size: 50 + int64(len(contentWithAttachment)), + expect: assert.NoError, + }, } for _, test := range tests { diff --git a/website/docs/support/known-issues.md b/website/docs/support/known-issues.md index ae56f8db4..52ac0ccd3 100644 --- a/website/docs/support/known-issues.md +++ b/website/docs/support/known-issues.md @@ -34,4 +34,6 @@ Below is a list of known Corso issues and limitations: * Groups and Teams support is available in an early-access status, and may be subject to breaking changes. -* Restoring the data into a different Group from the one it was backed up from isn't currently supported +* Restoring the data into a different Group from the one it was backed up from isn't currently supported. + +* Backing up a group mailbox item may fail if it has a large number of attachments (500+).