Skip to content

Commit

Permalink
Performance: do not fetch extra post data unless needed (#105)
Browse files Browse the repository at this point in the history 
* Performance: do not fetch extra post data unless needed

* simplify the scope condition

* move api calls outside markdown inspect

refactor contains function and make it more readable

* check if there is scope defined before making any api calls

refactor duplicated replace calls

* remove redundant log error calls

* fix: remove duplicate import

* fix: linting errors

* fix: linting errors
  • Loading branch information
@mo2menelzeiny
mo2menelzeiny authored Apr 25, 2020
1 parent 384f220 commit 6e6b5df
Show file tree
Hide file tree
Showing 2 changed files with 317 additions and 35 deletions.
102 changes: 67 additions & 35 deletions server/autolinkplugin/plugin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"strings"
"sync"

"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,20 +58,49 @@ 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, "/")
switch len(channelTeamSplit) {
case 1:
if strings.EqualFold(channelTeamSplit[0], team) {
return true
}
case 2:
if strings.EqualFold(channelTeamSplit[0], team) && strings.EqualFold(channelTeamSplit[1], channel) {
return true
}
default:
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 {
return "", "", cErr
}

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

team, tErr := p.API.GetTeam(channel.TeamId)
if tErr != nil {
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
}
}

Expand All @@ -93,6 +121,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())
}
}

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 @@ -131,26 +180,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
250 changes: 250 additions & 0 deletions server/autolinkplugin/plugin_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -732,3 +732,253 @@ func TestAPI(t *testing.T) {
assert.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{})

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{})

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{
{
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{
{
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{
{
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)
})
}

0 comments on commit 6e6b5df

Please sign in to comment.