From 9c0575ce6ef662c18ad7eb91bf6084c6fac1b7ae Mon Sep 17 00:00:00 2001 From: Chris Date: Tue, 24 Oct 2017 18:36:31 -0500 Subject: PLT-7599: webhook post splitting (#7707) * webhook post splitting * style fix * update old webhook test --- api/webhook_test.go | 4 +- app/webhook.go | 118 ++++++++++++++++++++++++++++++++++++++-------------- app/webhook_test.go | 96 ++++++++++++++++++++++++++++++++++++++++++ i18n/en.json | 4 +- model/post.go | 1 + 5 files changed, 187 insertions(+), 36 deletions(-) diff --git a/api/webhook_test.go b/api/webhook_test.go index 285650b5c..68dc4f4e7 100644 --- a/api/webhook_test.go +++ b/api/webhook_test.go @@ -1065,8 +1065,8 @@ func TestIncomingWebhooks(t *testing.T) { ] }` - if _, err := Client.DoPost(url, attachmentPayload, "application/json"); err == nil || err.StatusCode != http.StatusBadRequest { - t.Fatal("should have failed with bad request - attachment too long") + if _, err := Client.DoPost(url, attachmentPayload, "application/json"); err != nil { + t.Fatal(err) } th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableIncomingWebhooks = false }) diff --git a/app/webhook.go b/app/webhook.go index e8f9936f8..56f1f0875 100644 --- a/app/webhook.go +++ b/app/webhook.go @@ -123,6 +123,86 @@ func (a *App) TriggerWebhook(payload *model.OutgoingWebhookPayload, hook *model. } } +func SplitWebhookPost(post *model.Post) ([]*model.Post, *model.AppError) { + splits := make([]*model.Post, 0) + remainingText := post.Message + + base := *post + base.Message = "" + base.Props = make(map[string]interface{}) + for k, v := range post.Props { + if k != "attachments" { + base.Props[k] = v + } + } + if utf8.RuneCountInString(model.StringInterfaceToJson(base.Props)) > model.POST_PROPS_MAX_USER_RUNES { + return nil, model.NewAppError("SplitWebhookPost", "web.incoming_webhook.split_props_length.app_error", map[string]interface{}{"Max": model.POST_PROPS_MAX_USER_RUNES}, "", http.StatusBadRequest) + } + + for utf8.RuneCountInString(remainingText) > model.POST_MESSAGE_MAX_RUNES { + split := base + x := 0 + for index := range remainingText { + x++ + if x > model.POST_MESSAGE_MAX_RUNES { + split.Message = remainingText[:index] + remainingText = remainingText[index:] + break + } + } + splits = append(splits, &split) + } + + split := base + split.Message = remainingText + splits = append(splits, &split) + + attachments, _ := post.Props["attachments"].([]*model.SlackAttachment) + for _, attachment := range attachments { + newAttachment := *attachment + for { + lastSplit := splits[len(splits)-1] + newProps := make(map[string]interface{}) + for k, v := range lastSplit.Props { + newProps[k] = v + } + origAttachments, _ := newProps["attachments"].([]*model.SlackAttachment) + newProps["attachments"] = append(origAttachments, &newAttachment) + newPropsString := model.StringInterfaceToJson(newProps) + runeCount := utf8.RuneCountInString(newPropsString) + + if runeCount <= model.POST_PROPS_MAX_USER_RUNES { + lastSplit.Props = newProps + break + } + + if len(origAttachments) > 0 { + newSplit := base + splits = append(splits, &newSplit) + continue + } + + truncationNeeded := runeCount - model.POST_PROPS_MAX_USER_RUNES + textRuneCount := utf8.RuneCountInString(attachment.Text) + if textRuneCount < truncationNeeded { + return nil, model.NewAppError("SplitWebhookPost", "web.incoming_webhook.split_props_length.app_error", map[string]interface{}{"Max": model.POST_PROPS_MAX_USER_RUNES}, "", http.StatusBadRequest) + } + x := 0 + for index := range attachment.Text { + x++ + if x > textRuneCount-truncationNeeded { + newAttachment.Text = newAttachment.Text[:index] + break + } + } + lastSplit.Props = newProps + break + } + } + + return splits, nil +} + func (a *App) CreateWebhookPost(userId string, channel *model.Channel, text, overrideUsername, overrideIconUrl string, props model.StringInterface, postType string) (*model.Post, *model.AppError) { // parse links into Markdown format linkWithTextRegex := regexp.MustCompile(`<([^<\|]+)\|([^>]+)>`) @@ -166,37 +246,18 @@ func (a *App) CreateWebhookPost(userId string, channel *model.Channel, text, ove } } - splits := make([]string, 0) - remainingText := post.Message - - for len(remainingText) > model.POST_MESSAGE_MAX_RUNES { - splits = append(splits, remainingText[:model.POST_MESSAGE_MAX_RUNES]) - remainingText = remainingText[model.POST_MESSAGE_MAX_RUNES:] + splits, err := SplitWebhookPost(post) + if err != nil { + return nil, err } - splits = append(splits, remainingText) - - var firstPost *model.Post = nil - - for _, txt := range splits { - post.Id = "" - post.UpdateAt = 0 - post.CreateAt = 0 - post.Message = txt - if _, err := a.CreatePostMissingChannel(post, false); err != nil { + for _, split := range splits { + if _, err := a.CreatePostMissingChannel(split, false); err != nil { return nil, model.NewAppError("CreateWebhookPost", "api.post.create_webhook_post.creating.app_error", nil, "err="+err.Message, http.StatusInternalServerError) } - - if firstPost == nil { - if len(splits) > 1 { - firstPost = model.PostFromJson(strings.NewReader(post.ToJson())) - } else { - firstPost = post - } - } } - return firstPost, nil + return splits[0], nil } func (a *App) CreateIncomingWebhookForChannel(creatorId string, channel *model.Channel, hook *model.IncomingWebhook) (*model.IncomingWebhook, *model.AppError) { @@ -482,13 +543,6 @@ func (a *App) HandleIncomingWebhook(hookId string, req *model.IncomingWebhookReq req.Props = make(model.StringInterface) } req.Props["attachments"] = req.Attachments - - attachmentSize := utf8.RuneCountInString(model.StringInterfaceToJson(req.Props)) - // Minus 100 to leave room for setting post type in the Props - if attachmentSize > model.POST_PROPS_MAX_RUNES-100 { - return model.NewAppError("HandleIncomingWebhook", "web.incoming_webhook.attachment.app_error", map[string]interface{}{"Max": model.POST_PROPS_MAX_RUNES - 100, "Actual": attachmentSize}, "", http.StatusBadRequest) - } - webhookType = model.POST_SLACK_ATTACHMENT } diff --git a/app/webhook_test.go b/app/webhook_test.go index d6293e2b7..8dc90b49b 100644 --- a/app/webhook_test.go +++ b/app/webhook_test.go @@ -4,8 +4,12 @@ package app import ( + "strings" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/utils" ) @@ -50,3 +54,95 @@ func TestCreateWebhookPost(t *testing.T) { t.Fatal("should have failed - bad post type") } } + +func TestSplitWebhookPost(t *testing.T) { + type TestCase struct { + Post *model.Post + Expected []*model.Post + } + + for name, tc := range map[string]TestCase{ + "LongPost": { + Post: &model.Post{ + Message: strings.Repeat("本", model.POST_MESSAGE_MAX_RUNES*3/2), + }, + Expected: []*model.Post{ + { + Message: strings.Repeat("本", model.POST_MESSAGE_MAX_RUNES), + }, + { + Message: strings.Repeat("本", model.POST_MESSAGE_MAX_RUNES/2), + }, + }, + }, + "LongPostAndMultipleAttachments": { + Post: &model.Post{ + Message: strings.Repeat("本", model.POST_MESSAGE_MAX_RUNES*3/2), + Props: map[string]interface{}{ + "attachments": []*model.SlackAttachment{ + &model.SlackAttachment{ + Text: strings.Repeat("本", 1000), + }, + &model.SlackAttachment{ + Text: strings.Repeat("本", 2000), + }, + &model.SlackAttachment{ + Text: strings.Repeat("本", model.POST_PROPS_MAX_USER_RUNES-1000), + }, + }, + }, + }, + Expected: []*model.Post{ + { + Message: strings.Repeat("本", model.POST_MESSAGE_MAX_RUNES), + }, + { + Message: strings.Repeat("本", model.POST_MESSAGE_MAX_RUNES/2), + Props: map[string]interface{}{ + "attachments": []*model.SlackAttachment{ + &model.SlackAttachment{ + Text: strings.Repeat("本", 1000), + }, + &model.SlackAttachment{ + Text: strings.Repeat("本", 2000), + }, + }, + }, + }, + { + Props: map[string]interface{}{ + "attachments": []*model.SlackAttachment{ + &model.SlackAttachment{ + Text: strings.Repeat("本", model.POST_PROPS_MAX_USER_RUNES-1000), + }, + }, + }, + }, + }, + }, + "UnsplittableProps": { + Post: &model.Post{ + Message: "foo", + Props: map[string]interface{}{ + "foo": strings.Repeat("x", model.POST_PROPS_MAX_USER_RUNES*2), + }, + }, + }, + } { + t.Run(name, func(t *testing.T) { + splits, err := SplitWebhookPost(tc.Post) + if tc.Expected == nil { + require.NotNil(t, err) + } else { + require.Nil(t, err) + } + assert.Equal(t, len(tc.Expected), len(splits)) + for i, split := range splits { + if i < len(tc.Expected) { + assert.Equal(t, tc.Expected[i].Message, split.Message) + assert.Equal(t, tc.Expected[i].Props["attachments"], split.Props["attachments"]) + } + } + }) + } +} diff --git a/i18n/en.json b/i18n/en.json index 7a8c8f4c1..e20edf2f2 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -6860,8 +6860,8 @@ "translation": "Back" }, { - "id": "web.incoming_webhook.attachment.app_error", - "translation": "Maximum attachments length is {{.Max}} characters, received size is {{.Actual}}" + "id": "web.incoming_webhook.split_props_length.app_error", + "translation": "Unable to split webhook props into {{.Max}} character parts." }, { "id": "web.incoming_webhook.channel.app_error", diff --git a/model/post.go b/model/post.go index a5ae0b82a..a7be63432 100644 --- a/model/post.go +++ b/model/post.go @@ -32,6 +32,7 @@ const ( POST_HASHTAGS_MAX_RUNES = 1000 POST_MESSAGE_MAX_RUNES = 4000 POST_PROPS_MAX_RUNES = 8000 + POST_PROPS_MAX_USER_RUNES = POST_PROPS_MAX_RUNES - 400 // Leave some room for system / pre-save modifications POST_CUSTOM_TYPE_PREFIX = "custom_" PROPS_ADD_CHANNEL_MEMBER = "add_channel_member" ) -- cgit v1.2.3-1-g7c22