Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If autolinking would make posts too long, don't #154

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

If autolinking would make posts too long, don't #154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c930fa
If autolinking would make posts too long, don't
elyscape Apr 1, 2021
3094f9a
Correctly handle updated posts
elyscape Apr 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion server/autolinkplugin/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"net/http"
"strings"
"sync"
"unicode/utf8"

"github.com/mattermost/mattermost-server/v5/model"
"github.com/mattermost/mattermost-server/v5/plugin"
Expand Down Expand Up @@ -208,6 +209,11 @@ func (p *Plugin) ProcessPost(c *plugin.Context, post *model.Post) (*model.Post,
return nil, ""
}

if runes := utf8.RuneCountInString(postText); runes > model.POST_MESSAGE_MAX_RUNES_V1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For newer Mattermost instances or the one that updates, this will be longer:

This release includes support for post messages longer than the default of 4000 characters, but may require a manual database migration. This migration is entirely optional, and need only be done if you want to enable post messages up to 16383 characters. For many installations, no migration will be required, or the old limit remains sufficient.

https://docs.mattermost.com/administration/important-upgrade-notes.html

I'm hesitant to cap at the old limit as that might render the plugin useless for some instances.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fair, but we currently have no way to find the actual limit from a plug-in, and that excerpt notes that upgrading from 4000 characters may require a manual migration, which we can't expect every deployment to have performed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From looking at the server code base the UpdatePost RPC method does return an model.post.is_valid.msg.app_error error. (https://github.com/mattermost/mattermost-server/blob/ee3f986da0d9fbc69769dcec0c66d8493589757a/model/post.go#L299) Can we check for the specific error and return the original message in that case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that this function is effectively the handler for the MessageWillBePosted() and MessageWillBeUpdated() hooks, meaning we can alter the Post object but need to allow persistence to be handled further down the chain.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the ideal solution is to first provide a way for plugins to get the result of GetMaxPostSize(), and then have the plugin:

  1. Retrieve that value
  2. Update the Post object
  3. Call Post.IsValid() with its value
  4. Revert the object if the result is false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a concern to me as it unexpectedly renders the plugin useless in some cases that previously worked. I'm 2/5 that it's not worth breaking that cases for the bug fix that the PR includes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hanzei What would you suggest as a path forward?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From looking at the server code base the UpdatePost RPC method does return an model.post.is_valid.msg.app_error error. (mattermost/mattermost-server@ee3f986/model/post.go#L299) Can we check for the specific error and return the original message in that case?

@elyscape Do you think it's feasible to refactor the code and use that approach?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not. This function is effectively the handler for the MessageWillBePosted and MessageWillBeUpdated hooks, meaning we can alter the Post object but need to allow persistence to be handled further down the chain. From the documentation:

To reject a post, return an non-empty string describing why the post was rejected. To modify the post, return the replacement, non-nil *model.Post and an empty string. To allow the post without modification, return a nil *model.Post and an empty string. To dismiss the post, return a nil *model.Post and the const DismissPostError string.

We could call UpdatePost from MessageHasBeenPosted or MessageHasBeenUpdated, because those are events that fire after a message occurs, but not here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hanzei As a side complication, even once we get access to the result of GetMaxPostSize(), we'll need a fallback path for versions of Mattermost that don't support it.

p.API.LogWarn("Rewritten message would be too long, skipping", "rewrittenLength", runes)
return nil, ""
}

post.Message = postText
}

Expand Down Expand Up @@ -235,5 +241,11 @@ func (p *Plugin) MessageWillBeUpdated(c *plugin.Context, post *model.Post, _ *mo
return post, ""
}

return p.ProcessPost(c, post)
modifiedPost, reason := p.ProcessPost(c, post)

if modifiedPost == nil && reason == "" {
return post, ""
}

return modifiedPost, reason
}
87 changes: 79 additions & 8 deletions server/autolinkplugin/plugin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"net/http"
"net/http/httptest"
"strings"
"testing"

"github.com/mattermost/mattermost-server/v5/model"
Expand Down Expand Up @@ -305,6 +306,9 @@ func TestSpecialCases(t *testing.T) {
Pattern: "(example)",
Template: "test",
Scope: []string{"other-team/town-square"},
}, autolink.Autolink{
Pattern: "(toolong)",
Template: strings.Repeat("1", model.POST_MESSAGE_MAX_RUNES_V1+1),
})
validConfig := Config{
EnableAdminCommand: false,
Expand All @@ -331,6 +335,8 @@ func TestSpecialCases(t *testing.T) {
api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil)
api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil)

api.On("LogWarn", mock.AnythingOfType("string"), mock.AnythingOfType("string"), mock.AnythingOfType("int"))

testUser := model.User{
IsBot: false,
}
Expand All @@ -344,115 +350,156 @@ func TestSpecialCases(t *testing.T) {
var tests = []struct {
inputMessage string
expectedMessage string
expectedIgnored bool
}{
{
"hello ``` Mattermost ``` goodbye",
"hello ``` Mattermost ``` goodbye",
false,
}, {
"hello\n```\nMattermost\n```\ngoodbye",
"hello\n```\nMattermost\n```\ngoodbye",
false,
}, {
"Mattermost ``` Mattermost ``` goodbye",
"[Mattermost](https://mattermost.com) ``` Mattermost ``` goodbye",
false,
}, {
"``` Mattermost ``` Mattermost",
"``` Mattermost ``` [Mattermost](https://mattermost.com)",
false,
}, {
"Mattermost ``` Mattermost ```",
"[Mattermost](https://mattermost.com) ``` Mattermost ```",
false,
}, {
"Mattermost ``` Mattermost ```\n\n",
"[Mattermost](https://mattermost.com) ``` Mattermost ```\n\n",
false,
}, {
"hello ` Mattermost ` goodbye",
"hello ` Mattermost ` goodbye",
false,
}, {
"hello\n`\nMattermost\n`\ngoodbye",
"hello\n`\nMattermost\n`\ngoodbye",
false,
}, {
"Mattermost ` Mattermost ` goodbye",
"[Mattermost](https://mattermost.com) ` Mattermost ` goodbye",
false,
}, {
"` Mattermost ` Mattermost",
"` Mattermost ` [Mattermost](https://mattermost.com)",
false,
}, {
"Mattermost ` Mattermost `",
"[Mattermost](https://mattermost.com) ` Mattermost `",
false,
}, {
"Mattermost ` Mattermost `\n\n",
"[Mattermost](https://mattermost.com) ` Mattermost `\n\n",
false,
}, {
"hello ``` Mattermost ``` goodbye ` Mattermost ` end",
"hello ``` Mattermost ``` goodbye ` Mattermost ` end",
false,
}, {
"hello\n```\nMattermost\n```\ngoodbye ` Mattermost ` end",
"hello\n```\nMattermost\n```\ngoodbye ` Mattermost ` end",
false,
}, {
"Mattermost ``` Mattermost ``` goodbye ` Mattermost ` end",
"[Mattermost](https://mattermost.com) ``` Mattermost ``` goodbye ` Mattermost ` end",
false,
}, {
"``` Mattermost ``` Mattermost",
"``` Mattermost ``` [Mattermost](https://mattermost.com)",
false,
}, {
"```\n` Mattermost `\n```\nMattermost",
"```\n` Mattermost `\n```\n[Mattermost](https://mattermost.com)",
false,
}, {
" Mattermost",
" [Mattermost](https://mattermost.com)",
false,
}, {
" Mattermost",
" Mattermost",
false,
}, {
" ```\nMattermost\n ```",
" ```\n[Mattermost](https://mattermost.com)\n ```",
false,
}, {
"` ``` `\nMattermost\n` ``` `",
"` ``` `\n[Mattermost](https://mattermost.com)\n` ``` `",
false,
}, {
"Mattermost \n Mattermost",
"[Mattermost](https://mattermost.com) \n [Mattermost](https://mattermost.com)",
false,
}, {
"[Mattermost](https://mattermost.com)",
"[Mattermost](https://mattermost.com)",
false,
}, {
"[ Mattermost ](https://mattermost.com)",
"[ Mattermost ](https://mattermost.com)",
false,
}, {
"[ Mattermost ][1]\n\n[1]: https://mattermost.com",
"[ Mattermost ][1]\n\n[1]: https://mattermost.com",
false,
}, {
"![ Mattermost ](https://mattermost.com/example.png)",
"![ Mattermost ](https://mattermost.com/example.png)",
false,
}, {
"![ Mattermost ][1]\n\n[1]: https://mattermost.com/example.png",
"![ Mattermost ][1]\n\n[1]: https://mattermost.com/example.png",
false,
}, {
"foo!bar\nExample\nfoo!bar Mattermost",
"fb\n[Example](https://example.com)\nfb [Mattermost](https://mattermost.com)",
false,
}, {
"foo!bar",
"fb",
false,
}, {
"foo!barfoo!bar",
"foo!barfoo!bar",
false,
}, {
"foo!bar & foo!bar",
"fb & fb",
false,
}, {
"foo!bar & foo!bar\nfoo!bar & foo!bar\nfoo!bar & foo!bar",
"fb & fb\nfb & fb\nfb & fb",
false,
}, {
"https://mattermost.atlassian.net/browse/MM-12345",
"[MM-12345](https://mattermost.atlassian.net/browse/MM-12345)",
false,
}, {
"Welcome https://mattermost.atlassian.net/browse/MM-12345",
"Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345)",
false,
}, {
"text https://mattermost.atlassian.net/browse/MM-12345 other text",
"text [MM-12345](https://mattermost.atlassian.net/browse/MM-12345) other text",
false,
}, {
"check out MM-12345 too",
"check out [MM-12345](https://mattermost.atlassian.net/browse/MM-12345) too",
false,
}, {
"toolong",
"toolong",
true,
},
}

Expand All @@ -465,9 +512,15 @@ func TestSpecialCases(t *testing.T) {
Message: tt.inputMessage,
}

rpost, _ := p.MessageWillBePosted(&plugin.Context{}, post)
rpost, err := p.MessageWillBePosted(&plugin.Context{}, post)

assert.Equal(t, tt.expectedMessage, rpost.Message)
if tt.expectedIgnored {
assert.Nil(t, rpost)
} else {
assert.Equal(t, tt.expectedMessage, rpost.Message)
}

assert.Equal(t, "", err)
}
{
// user updates the modified post but with no changes
Expand All @@ -476,9 +529,15 @@ func TestSpecialCases(t *testing.T) {
Message: tt.expectedMessage,
}

rpost, _ := p.MessageWillBeUpdated(&plugin.Context{}, post, post)
rpost, err := p.MessageWillBeUpdated(&plugin.Context{}, post, post)

assert.Equal(t, tt.expectedMessage, rpost.Message)
if tt.expectedIgnored {
assert.Same(t, post, rpost)
} else {
assert.Equal(t, tt.expectedMessage, rpost.Message)
}

assert.Equal(t, "", err)
}
{
// user updates the modified post and sets it back to the original text
Expand All @@ -490,9 +549,15 @@ func TestSpecialCases(t *testing.T) {
Message: tt.inputMessage,
}

rpost, _ := p.MessageWillBeUpdated(&plugin.Context{}, originalPost, post)
rpost, err := p.MessageWillBeUpdated(&plugin.Context{}, originalPost, post)

if tt.expectedIgnored {
assert.Same(t, originalPost, rpost)
} else {
assert.Equal(t, tt.expectedMessage, rpost.Message)
}

assert.Equal(t, tt.expectedMessage, rpost.Message)
assert.Equal(t, "", err)
}
{
// user updates an empty post to the original text
Expand All @@ -502,9 +567,15 @@ func TestSpecialCases(t *testing.T) {
Message: tt.inputMessage,
}

rpost, _ := p.MessageWillBeUpdated(&plugin.Context{}, post, emptyPost)
rpost, err := p.MessageWillBeUpdated(&plugin.Context{}, post, emptyPost)

if tt.expectedIgnored {
assert.Same(t, post, rpost)
} else {
assert.Equal(t, tt.expectedMessage, rpost.Message)
}

assert.Equal(t, tt.expectedMessage, rpost.Message)
assert.Equal(t, "", err)
}
})
}
Expand Down