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

Performance: do not fetch extra post data unless needed #105

Merged
merged 9 commits into from
Apr 25, 2020
Merged

Performance: do not fetch extra post data unless needed #105

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
59e501a
Performance: do not fetch extra post data unless needed
mo2menelzeiny Apr 7, 2020
aeb4256
simplify the scope condition
mo2menelzeiny Apr 8, 2020
c7cd289
move api calls outside markdown inspect
mo2menelzeiny Apr 8, 2020
b8e9a56
check if there is scope defined before making any api calls
mo2menelzeiny Apr 8, 2020
c06b80f
remove redundant log error calls
mo2menelzeiny Apr 8, 2020
dd73214
Merge branch 'master' into perf-process-post
mo2menelzeiny Apr 24, 2020
ec50d32
fix: remove duplicate import
mo2menelzeiny Apr 24, 2020
8f206be
fix: linting errors
mo2menelzeiny Apr 24, 2020
806a1d8
fix: linting errors
mo2menelzeiny Apr 24, 2020
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
104 changes: 70 additions & 34 deletions server/autolinkplugin/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (

"github.com/mattermost/mattermost-plugin-autolink/server/api"

"github.com/mattermost/mattermost-server/v5/mlog"
"github.com/mattermost/mattermost-server/v5/model"
"github.com/mattermost/mattermost-server/v5/plugin"
"github.com/mattermost/mattermost-server/v5/utils/markdown"
Expand Down Expand Up @@ -59,22 +58,55 @@ func (p *Plugin) IsAuthorizedAdmin(userId string) (bool, error) {
return false, nil
}

func contains(team string, channel string, list []string) bool {
for _, channelTeam := range list {
channelTeamSplit := strings.Split(channelTeam, "/")
if len(channelTeamSplit) == 2 {
if strings.EqualFold(channelTeamSplit[0], team) && strings.EqualFold(channelTeamSplit[1], channel) {
return true
}
} else if len(channelTeamSplit) == 1 {
if strings.EqualFold(channelTeamSplit[0], team) {
return true
}
} else {
mlog.Error("error splitting channel & team combination.")
func (p *Plugin) resolveScope(channelId string) (string, string, *model.AppError) {
channel, cErr := p.API.GetChannel(channelId)
if cErr != nil {
p.API.LogError("Failed to get Channel", "error", cErr.Error())
mo2menelzeiny marked this conversation as resolved.
Show resolved Hide resolved
return "", "", cErr
}

if channel.TeamId == "" {
return channel.Name, "", nil
}

team, tErr := p.API.GetTeam(channel.TeamId)
if tErr != nil {
p.API.LogError("Failed to get Team", "error", tErr.Error())
mo2menelzeiny marked this conversation as resolved.
Show resolved Hide resolved
return "", "", tErr
}

return channel.Name, team.Name, nil
}

func (p *Plugin) inScope(scope []string, channelName string, teamName string) bool {

if len(scope) == 0 {
return true
}

if teamName == "" {
return false
}

for _, teamChannel := range scope {
split := strings.Split(teamChannel, "/")

splitLength := len(split)

if splitLength == 1 && split[0] == "" {
return false
}

if splitLength == 1 && strings.EqualFold(split[0], teamName) {
return true
}

scopeMatch := strings.EqualFold(split[0], teamName) && strings.EqualFold(split[1], channelName)
if splitLength == 2 && scopeMatch {
return true
}
}

return false
}

Expand All @@ -92,6 +124,27 @@ func (p *Plugin) ProcessPost(c *plugin.Context, post *model.Post) (*model.Post,
conf := p.getConfig()
postText := post.Message
offset := 0

hasOneOrMoreScopes := false
for _, link := range conf.Links {
if len(link.Scope) > 0 {
hasOneOrMoreScopes = true
break
}
}

channelName := ""
teamName := ""
if hasOneOrMoreScopes {
cn, tn, rsErr := p.resolveScope(post.ChannelId)
channelName = cn
teamName = tn

if rsErr != nil {
p.API.LogError("Failed to resolve scope", "error", rsErr.Error())
}
}

mo2menelzeiny marked this conversation as resolved.
Show resolved Hide resolved
markdown.Inspect(post.Message, func(node interface{}) bool {
switch node.(type) {
// never descend into the text content of a link/image
Expand Down Expand Up @@ -130,26 +183,9 @@ func (p *Plugin) ProcessPost(c *plugin.Context, post *model.Post) (*model.Post,
if origText != "" {
newText := origText

channel, cErr := p.API.GetChannel(post.ChannelId)
if cErr != nil {
p.API.LogError("Failed to get Channel", "error", cErr.Error())
return false
}
teamName := ""
if channel.TeamId != "" {
team, tErr := p.API.GetTeam(channel.TeamId)
if tErr != nil {
p.API.LogError("Failed to get Team", "error", tErr.Error())
return false
}
teamName = team.Name
}

for _, l := range conf.Links {
if len(l.Scope) == 0 {
newText = l.Replace(newText)
} else if teamName != "" && contains(teamName, channel.Name, l.Scope) {
newText = l.Replace(newText)
for _, link := range conf.Links {
if p.inScope(link.Scope, channelName, teamName) {
newText = link.Replace(newText)
}
}
if origText != newText {
Expand Down
259 changes: 259 additions & 0 deletions server/autolinkplugin/plugin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,3 +733,262 @@ func TestAPI(t *testing.T) {
require.Len(t, p.conf.Links, 2)
assert.Equal(t, "new", p.conf.Links[1].Name)
}

func TestResolveScope(t *testing.T) {
t.Run("resolve channel name and team name", func(t *testing.T) {
testChannel := model.Channel{
Name: "TestChannel",
TeamId: "TestId",
}

testTeam := model.Team{
Name: "TestTeam",
}

api := &plugintest.API{}
api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil)
api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil)

p := Plugin{}
p.SetAPI(api)
channelName, teamName, _ := p.resolveScope("TestId")
assert.Equal(t, "TestChannel", channelName)
assert.Equal(t, "TestTeam", teamName)
})

t.Run("resolve channel name and returns empty team name", func(t *testing.T) {
testChannel := model.Channel{
Name: "TestChannel",
}

api := &plugintest.API{}
api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil)

p := Plugin{}
p.SetAPI(api)

channelName, teamName, _ := p.resolveScope("TestId")
assert.Equal(t, "TestChannel", channelName)
assert.Equal(t, "", teamName)
})

t.Run("error when api fails to get channel", func(t *testing.T) {
api := &plugintest.API{}

api.On("GetChannel",
mock.AnythingOfType("string")).Return(nil, &model.AppError{})
api.On("LogError",
mock.AnythingOfType("string"),
mock.AnythingOfType("string"),
mock.AnythingOfType("string")).Return(nil)

p := Plugin{}
p.SetAPI(api)

channelName, teamName, err := p.resolveScope("TestId")
assert.Error(t, err)
assert.Equal(t, teamName, "")
assert.Equal(t, channelName, "")
})

t.Run("error when api fails to get team", func(t *testing.T) {
testChannel := model.Channel{
Name: "TestChannel",
TeamId: "TestId",
}

api := &plugintest.API{}

api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil)
api.On("GetTeam", mock.AnythingOfType("string")).Return(nil, &model.AppError{})
api.On("LogError",
mock.AnythingOfType("string"),
mock.AnythingOfType("string"),
mock.AnythingOfType("string")).Return(nil)

p := Plugin{}
p.SetAPI(api)

channelName, teamName, err := p.resolveScope("TestId")
assert.Error(t, err)
assert.Equal(t, channelName, "")
assert.Equal(t, teamName, "")
})
}

func TestProcessPost(t *testing.T) {
t.Run("cannot resolve scope", func(t *testing.T) {
conf := Config{
Links: []autolink.Autolink{
autolink.Autolink{
Pattern: "(Mattermost)",
Template: "[Mattermost](https://mattermost.com)",
Scope: []string{"TestTeam/TestChannel"},
},
},
}

api := &plugintest.API{}

api.On("LoadPluginConfiguration",
mock.AnythingOfType("*autolinkplugin.Config")).Return(func(dest interface{}) error {
*dest.(*Config) = conf
return nil
})
api.On("UnregisterCommand", mock.AnythingOfType("string"),
mock.AnythingOfType("string")).Return((*model.AppError)(nil))

api.On("GetChannel", mock.AnythingOfType("string")).Return(nil, &model.AppError{})

api.On("LogError",
mock.AnythingOfType("string"),
mock.AnythingOfType("string"),
mock.AnythingOfType("string")).Return(nil)

testUser := model.User{
IsBot: false,
}
api.On("GetUser", mock.AnythingOfType("string")).Return(&testUser, nil)

p := New()
p.SetAPI(api)
p.OnConfigurationChange()

post := &model.Post{Message: "Welcome to Mattermost!"}
rpost, _ := p.ProcessPost(&plugin.Context{}, post)

assert.Equal(t, "Welcome to Mattermost!", rpost.Message)
})

t.Run("team name is empty", func(t *testing.T) {
conf := Config{
Links: []autolink.Autolink{
autolink.Autolink{
Pattern: "(Mattermost)",
Template: "[Mattermost](https://mattermost.com)",
Scope: []string{"TestTeam/TestChannel"},
},
},
}

testChannel := model.Channel{
Name: "TestChannel",
}

api := &plugintest.API{}

api.On("LoadPluginConfiguration",
mock.AnythingOfType("*autolinkplugin.Config")).Return(func(dest interface{}) error {
*dest.(*Config) = conf
return nil
})
api.On("UnregisterCommand", mock.AnythingOfType("string"),
mock.AnythingOfType("string")).Return((*model.AppError)(nil))
api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil)
api.On("LogError",
mock.AnythingOfType("string"),
mock.AnythingOfType("string"),
mock.AnythingOfType("string")).Return(nil)

testUser := model.User{
IsBot: false,
}
api.On("GetUser", mock.AnythingOfType("string")).Return(&testUser, nil)

p := New()
p.SetAPI(api)
p.OnConfigurationChange()

post := &model.Post{Message: "Welcome to Mattermost!"}
rpost, _ := p.ProcessPost(&plugin.Context{}, post)

assert.Equal(t, "Welcome to Mattermost!", rpost.Message)
})

t.Run("valid scope replaces text", func(t *testing.T) {
conf := Config{
Links: []autolink.Autolink{
autolink.Autolink{
Pattern: "(Mattermost)",
Template: "[Mattermost](https://mattermost.com)",
Scope: []string{"TestTeam/TestChannel"},
},
},
}

testChannel := model.Channel{
Name: "TestChannel",
TeamId: "TestId",
}

testTeam := model.Team{
Name: "TestTeam",
}

api := &plugintest.API{}

api.On("LoadPluginConfiguration",
mock.AnythingOfType("*autolinkplugin.Config")).Return(func(dest interface{}) error {
*dest.(*Config) = conf
return nil
})
api.On("UnregisterCommand", mock.AnythingOfType("string"),
mock.AnythingOfType("string")).Return((*model.AppError)(nil))

api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil)
api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil)

testUser := model.User{
IsBot: false,
}
api.On("GetUser", mock.AnythingOfType("string")).Return(&testUser, nil)

p := New()
p.SetAPI(api)
p.OnConfigurationChange()

post := &model.Post{Message: "Welcome to Mattermost!"}
rpost, _ := p.ProcessPost(&plugin.Context{}, post)

assert.Equal(t, "Welcome to [Mattermost](https://mattermost.com)!", rpost.Message)
})
}

func TestInScope(t *testing.T) {

t.Run("returns true if scope array is empty", func(t *testing.T) {
p := &Plugin{}
result := p.inScope([]string{}, "TestChannel", "TestTeam")
assert.Equal(t, true, result)
})

t.Run("returns true when team and channels are valid", func(t *testing.T) {
p := &Plugin{}
result := p.inScope([]string{"TestTeam/TestChannel"}, "TestChannel", "TestTeam")
assert.Equal(t, true, result)
})

t.Run("returns false when channel is empty", func(t *testing.T) {
p := &Plugin{}
result := p.inScope([]string{"TestTeam/"}, "TestChannel", "TestTeam")
assert.Equal(t, false, result)
})

t.Run("returns false when team is empty", func(t *testing.T) {
p := &Plugin{}
result := p.inScope([]string{"TestTeam/TestChannel"}, "TestChannel", "")
assert.Equal(t, false, result)
})

t.Run("returns false on empty scope", func(t *testing.T) {
p := &Plugin{}
result := p.inScope([]string{""}, "TestChannel", "TestTeam")
assert.Equal(t, false, result)
})

t.Run("returns true on team scope only", func(t *testing.T) {
p := &Plugin{}
result := p.inScope([]string{"TestTeam"}, "TestChannel", "TestTeam")
assert.Equal(t, true, result)
})
}