From 610fb89830a9392958ae69af61c1d312e6a12430 Mon Sep 17 00:00:00 2001 From: Lev Brouk Date: Tue, 9 Jul 2019 14:08:16 -0700 Subject: [PATCH 1/8] MM-15965: cleaned up word boundary checks - Using `\b` instead of a homebrewed word separator list - Since `\b` does not consume, removed the second pass ReplaceAll - Made tests more realistic by running MessageWillBePosted, to cover markdown inspection - Added word boundary tests - Cleanup --- .vscode/.gitignore | 1 + server/autolinker.go | 31 +++----- server/autolinker_test.go | 148 ++++++++++++++++++++++++++++++++++---- server/plugin.go | 2 +- 4 files changed, 146 insertions(+), 36 deletions(-) create mode 100644 .vscode/.gitignore diff --git a/.vscode/.gitignore b/.vscode/.gitignore new file mode 100644 index 00000000..e38da200 --- /dev/null +++ b/.vscode/.gitignore @@ -0,0 +1 @@ +settings.json diff --git a/server/autolinker.go b/server/autolinker.go index de446348..e6695dbd 100644 --- a/server/autolinker.go +++ b/server/autolinker.go @@ -7,49 +7,40 @@ import ( // AutoLinker helper for replace regex with links type AutoLinker struct { - link *Link - pattern *regexp.Regexp + link *Link + re *regexp.Regexp } // NewAutoLinker create and initialize a AutoLinker -func NewAutoLinker(link *Link) (*AutoLinker, error) { - if link == nil || len(link.Pattern) == 0 || len(link.Template) == 0 { +func NewAutoLinker(link Link) (*AutoLinker, error) { + if len(link.Pattern) == 0 || len(link.Template) == 0 { return nil, errors.New("Pattern or template was empty") } if !link.DisableNonWordPrefix { - link.Pattern = "(?P^|\\s)" + link.Pattern - link.Template = "${MMDisableNonWordPrefix}" + link.Template + link.Pattern = `\b` + link.Pattern } if !link.DisableNonWordSuffix { - link.Pattern = link.Pattern + "(?P$|\\s|\\.|\\!|\\?|\\,|\\))" - link.Template = link.Template + "${DisableNonWordSuffix}" + link.Pattern = link.Pattern + `\b` } - p, err := regexp.Compile(link.Pattern) + re, err := regexp.Compile(link.Pattern) if err != nil { return nil, err } return &AutoLinker{ - link: link, - pattern: p, + link: &link, + re: re, }, nil } // Replace will subsitute the regex's with the supplied links func (l *AutoLinker) Replace(message string) string { - if l.pattern == nil { + if l.re == nil { return message } - // beacuse MMDisableNonWordPrefix DisableNonWordSuffix are greedy then - // two matches back to back won't get found. So we need to run the - // replace all twice - if !l.link.DisableNonWordPrefix && !l.link.DisableNonWordSuffix { - message = string(l.pattern.ReplaceAll([]byte(message), []byte(l.link.Template))) - } - - return string(l.pattern.ReplaceAll([]byte(message), []byte(l.link.Template))) + return l.re.ReplaceAllString(message, l.link.Template) } diff --git a/server/autolinker_test.go b/server/autolinker_test.go index a8168d25..3a0be73d 100644 --- a/server/autolinker_test.go +++ b/server/autolinker_test.go @@ -1,17 +1,41 @@ package main import ( + "fmt" "testing" + "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/plugin/plugintest" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) +func setupTestPlugin(t *testing.T, link Link) *Plugin { + p := &Plugin{} + api := &plugintest.API{} + + api.On("GetChannel", mock.AnythingOfType("string")).Run(func(args mock.Arguments) { + api.On("GetTeam", mock.AnythingOfType("string")).Return(&model.Team{}, (*model.AppError)(nil)) + }).Return(&model.Channel{ + Id: "thechannel_id0123456789012", + TeamId: "theteam_id0123456789012345", + Name: "thechannel_name", + }, (*model.AppError)(nil)) + p.SetAPI(api) + + al, err := NewAutoLinker(link) + require.Nil(t, err) + p.links.Store([]*AutoLinker{al}) + return p +} + func TestAutolink(t *testing.T) { - var tests = []struct { + for _, tc := range []struct { Name string Link *Link - inputMessage string - expectedMessage string + Message string + ExpectedMessage string }{ { "Simple pattern", @@ -156,14 +180,111 @@ func TestAutolink(t *testing.T) { "Welcome https://mattermost.atlassian.net/browse/MM-12345", "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345)", }, + } { + + t.Run(tc.Name, func(t *testing.T) { + p := setupTestPlugin(t, *tc.Link) + post, _ := p.MessageWillBePosted(nil, &model.Post{ + Message: tc.Message, + }) + + assert.Equal(t, tc.ExpectedMessage, post.Message) + }) + } +} - for _, tt := range tests { - t.Run(tt.Name, func(t *testing.T) { - al, _ := NewAutoLinker(tt.Link) - actual := al.Replace(tt.inputMessage) +func TestAutolinkWordBoundaries(t *testing.T) { + const pattern = "(KEY)(-)(?P\\d+)" + const template = "[KEY-$ID](someurl/KEY-$ID)" + const ref = "KEY-12345" + const ID = "12345" + const markdown = "[KEY-12345](someurl/KEY-12345)" + + var defaultLink = Link{ + Pattern: pattern, + Template: template, + } + + linkNoPrefix := defaultLink + linkNoPrefix.DisableNonWordPrefix = true - assert.Equal(t, tt.expectedMessage, actual) + linkNoSuffix := defaultLink + linkNoSuffix.DisableNonWordSuffix = true + + linkNoPrefixNoSuffix := defaultLink + linkNoPrefixNoSuffix.DisableNonWordSuffix = true + linkNoPrefixNoSuffix.DisableNonWordPrefix = true + + for _, tc := range []struct { + Name string + Sep string + Link *Link + Prefix string + Suffix string + ExpectFail bool + }{ + {Name: "space both sides both breaks required", Prefix: " ", Suffix: " "}, + {Name: "space both sides left break not required", Prefix: " ", Suffix: " ", Link: &linkNoPrefix}, + {Name: "space both sides right break not required", Prefix: " ", Suffix: " ", Link: &linkNoSuffix}, + {Name: "space both sides neither break required", Prefix: " ", Suffix: " ", Link: &linkNoPrefixNoSuffix}, + + {Name: "space left side both breaks required", Prefix: " ", ExpectFail: true}, + {Name: "space left side left break not required", Prefix: " ", Link: &linkNoPrefix, ExpectFail: true}, + {Name: "space left side right break not required", Prefix: " ", Link: &linkNoSuffix}, + {Name: "space left side neither break required", Prefix: " ", Link: &linkNoPrefixNoSuffix}, + + {Name: "space right side both breaks required", Suffix: " ", ExpectFail: true}, + {Name: "space right side left break not required", Suffix: " ", Link: &linkNoPrefix}, + {Name: "space right side right break not required", Suffix: " ", Link: &linkNoSuffix, ExpectFail: true}, + {Name: "space right side neither break required", Prefix: " ", Link: &linkNoPrefixNoSuffix}, + + {Name: "none both breaks required", ExpectFail: true}, + {Name: "none left break not required", Link: &linkNoPrefix, ExpectFail: true}, + {Name: "none right break not required", Link: &linkNoSuffix, ExpectFail: true}, + {Name: "none neither break required", Link: &linkNoPrefixNoSuffix}, + + {Sep: "paren", Name: "2 parens", Prefix: "(", Suffix: ")"}, + {Sep: "paren", Name: "left paren", Prefix: "(", Link: &linkNoSuffix}, + {Sep: "paren", Name: "right paren", Suffix: ")", Link: &linkNoPrefix}, + {Sep: "sbracket", Name: "2 brackets", Prefix: "[", Suffix: "]"}, + {Sep: "lsbracket", Name: "both breaks", Prefix: "[", ExpectFail: true}, + {Sep: "lsbracket", Name: "bracket no prefix", Prefix: "[", Link: &linkNoPrefix, ExpectFail: true}, + {Sep: "lsbracket", Name: "bracket no suffix", Prefix: "[", Link: &linkNoSuffix}, + {Sep: "lsbracket", Name: "bracket neither prefix suffix", Prefix: "[", Link: &linkNoPrefixNoSuffix}, + {Sep: "rsbracket", Name: "bracket", Suffix: "]", Link: &linkNoPrefix}, + {Sep: "rand", Name: "random separators", Prefix: "% (", Suffix: "-- $%^&"}, + } { + + orig := fmt.Sprintf("word1%s%s%sword2", tc.Prefix, ref, tc.Suffix) + expected := fmt.Sprintf("word1%s%s%sword2", tc.Prefix, markdown, tc.Suffix) + + pref := tc.Prefix + suff := tc.Suffix + if tc.Sep != "" { + pref = "_" + tc.Sep + "_" + suff = "_" + tc.Sep + "_" + } + name := fmt.Sprintf("word1%s%s%sword2", pref, ref, suff) + if tc.Name != "" { + name = tc.Name + " " + name + } + + t.Run(name, func(t *testing.T) { + l := tc.Link + if l == nil { + l = &defaultLink + } + p := setupTestPlugin(t, *l) + + post, _ := p.MessageWillBePosted(nil, &model.Post{ + Message: orig, + }) + if tc.ExpectFail { + assert.Equal(t, orig, post.Message) + return + } + assert.Equal(t, expected, post.Message) }) } } @@ -171,23 +292,20 @@ func TestAutolink(t *testing.T) { func TestAutolinkErrors(t *testing.T) { var tests = []struct { Name string - Link *Link + Link Link }{ { - "No Link at all", - nil, - }, { "Empty Link", - &Link{}, + Link{}, }, { "No pattern", - &Link{ + Link{ Pattern: "", Template: "blah", }, }, { "No template", - &Link{ + Link{ Pattern: "blah", Template: "", }, diff --git a/server/plugin.go b/server/plugin.go index 5f7c271b..da8c50c1 100644 --- a/server/plugin.go +++ b/server/plugin.go @@ -28,7 +28,7 @@ func (p *Plugin) OnConfigurationChange() error { links := make([]*AutoLinker, 0) for _, l := range c.Links { - al, lerr := NewAutoLinker(l) + al, lerr := NewAutoLinker(*l) if lerr != nil { mlog.Error("Error creating autolinker: ") } From 7e99caf6bf802f3b73d6ba062b1b1118675a22ce Mon Sep 17 00:00:00 2001 From: Lev Brouk Date: Thu, 1 Aug 2019 00:26:43 -0700 Subject: [PATCH 2/8] added config commands --- server/autolinker.go | 46 ---- server/autolinker_test.go | 490 -------------------------------------- server/config.go | 80 ++++++- server/plugin.go | 41 ++-- server/plugin_test.go | 46 ++-- 5 files changed, 111 insertions(+), 592 deletions(-) delete mode 100644 server/autolinker.go delete mode 100644 server/autolinker_test.go diff --git a/server/autolinker.go b/server/autolinker.go deleted file mode 100644 index e6695dbd..00000000 --- a/server/autolinker.go +++ /dev/null @@ -1,46 +0,0 @@ -package main - -import ( - "errors" - "regexp" -) - -// AutoLinker helper for replace regex with links -type AutoLinker struct { - link *Link - re *regexp.Regexp -} - -// NewAutoLinker create and initialize a AutoLinker -func NewAutoLinker(link Link) (*AutoLinker, error) { - if len(link.Pattern) == 0 || len(link.Template) == 0 { - return nil, errors.New("Pattern or template was empty") - } - - if !link.DisableNonWordPrefix { - link.Pattern = `\b` + link.Pattern - } - - if !link.DisableNonWordSuffix { - link.Pattern = link.Pattern + `\b` - } - - re, err := regexp.Compile(link.Pattern) - if err != nil { - return nil, err - } - - return &AutoLinker{ - link: &link, - re: re, - }, nil -} - -// Replace will subsitute the regex's with the supplied links -func (l *AutoLinker) Replace(message string) string { - if l.re == nil { - return message - } - - return l.re.ReplaceAllString(message, l.link.Template) -} diff --git a/server/autolinker_test.go b/server/autolinker_test.go deleted file mode 100644 index 0670848d..00000000 --- a/server/autolinker_test.go +++ /dev/null @@ -1,490 +0,0 @@ -package main - -import ( - "fmt" - "regexp" - "testing" - - "github.com/mattermost/mattermost-server/model" - "github.com/mattermost/mattermost-server/plugin/plugintest" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" -) - -func setupTestPlugin(t *testing.T, link Link) *Plugin { - p := &Plugin{} - api := &plugintest.API{} - - api.On("GetChannel", mock.AnythingOfType("string")).Run(func(args mock.Arguments) { - api.On("GetTeam", mock.AnythingOfType("string")).Return(&model.Team{}, (*model.AppError)(nil)) - }).Return(&model.Channel{ - Id: "thechannel_id0123456789012", - TeamId: "theteam_id0123456789012345", - Name: "thechannel_name", - }, (*model.AppError)(nil)) - p.SetAPI(api) - - al, err := NewAutoLinker(link) - require.Nil(t, err) - p.links.Store([]*AutoLinker{al}) - return p -} - -const ( - reLastFour = `(?P[0-9]{4})` - reVISA = `(?P` + `(?P4\d{3})[ -]?(?P\d{4})[ -]?(?P\d{4})[ -]?` + reLastFour + `)` - reMasterCard = `(?P` + `(?P5[1-5]\d{2})[ -]?(?P\d{4})[ -]?(?P\d{4})[ -]?` + reLastFour + `)` - reSwitchSolo = `(?P` + `(?P67\d{2})[ -]?(?P\d{4})[ -]?(?P\d{4})[ -]?` + reLastFour + `)` - reDiscover = `(?P` + `(?P6011)[ -]?(?P\d{4})[ -]?(?P\d{4})[ -]?` + reLastFour + `)` - reAMEX = `(?P` + `(?P3[47]\d{2})[ -]?(?P\d{6})[ -]?(?P\d)` + reLastFour + `)` - - replace4444 = `XXXX-XXXX-XXXX-$LastFour` - replace465 = `XXXX-XXXXXX-X$LastFour` - - replaceVISA = "VISA " + replace4444 - replaceMasterCard = "MasterCard " + replace4444 - replaceSwitchSolo = "Switch/Solo " + replace4444 - replaceDiscover = "Discover " + replace4444 - replaceAMEX = "American Express " + replace465 -) - -func TestCCRegex(t *testing.T) { - for _, tc := range []struct { - Name string - RE string - Replace string - In string - Out string - }{ - {"Visa happy spaces", reVISA, replaceVISA, " abc 4111 1111 1111 1234 def", " abc VISA XXXX-XXXX-XXXX-1234 def"}, - {"Visa happy dashes", reVISA, replaceVISA, "4111-1111-1111-1234", "VISA XXXX-XXXX-XXXX-1234"}, - {"Visa happy mixed", reVISA, replaceVISA, "41111111 1111-1234", "VISA XXXX-XXXX-XXXX-1234"}, - {"Visa happy digits", reVISA, replaceVISA, "abc 4111111111111234 def", "abc VISA XXXX-XXXX-XXXX-1234 def"}, - {"Visa non-match start", reVISA, replaceVISA, "3111111111111234", ""}, - {"Visa non-match num digits", reVISA, replaceVISA, " 4111-1111-1111-123", ""}, - {"Visa non-match sep", reVISA, replaceVISA, "4111=1111=1111_1234", ""}, - {"Visa non-match no break before", reVISA, replaceVISA, "abc4111-1111-1111-1234", "abcVISA XXXX-XXXX-XXXX-1234"}, - {"Visa non-match no break after", reVISA, replaceVISA, "4111-1111-1111-1234def", "VISA XXXX-XXXX-XXXX-1234def"}, - - {"MasterCard happy spaces", reMasterCard, replaceMasterCard, " abc 5111 1111 1111 1234 def", " abc MasterCard XXXX-XXXX-XXXX-1234 def"}, - {"MasterCard happy dashes", reMasterCard, replaceMasterCard, "5211-1111-1111-1234", "MasterCard XXXX-XXXX-XXXX-1234"}, - {"MasterCard happy mixed", reMasterCard, replaceMasterCard, "53111111 1111-1234", "MasterCard XXXX-XXXX-XXXX-1234"}, - {"MasterCard happy digits", reMasterCard, replaceMasterCard, "abc 5411111111111234 def", "abc MasterCard XXXX-XXXX-XXXX-1234 def"}, - {"MasterCard non-match start", reMasterCard, replaceMasterCard, "3111111111111234", ""}, - {"MasterCard non-match num digits", reMasterCard, replaceMasterCard, " 5111-1111-1111-123", ""}, - {"MasterCard non-match sep", reMasterCard, replaceMasterCard, "5111=1111=1111_1234", ""}, - {"MasterCard non-match no break before", reMasterCard, replaceMasterCard, "abc5511-1111-1111-1234", "abcMasterCard XXXX-XXXX-XXXX-1234"}, - {"MasterCard non-match no break after", reMasterCard, replaceMasterCard, "5111-1111-1111-1234def", "MasterCard XXXX-XXXX-XXXX-1234def"}, - - {"SwitchSolo happy spaces", reSwitchSolo, replaceSwitchSolo, " abc 6711 1111 1111 1234 def", " abc Switch/Solo XXXX-XXXX-XXXX-1234 def"}, - {"SwitchSolo happy dashes", reSwitchSolo, replaceSwitchSolo, "6711-1111-1111-1234", "Switch/Solo XXXX-XXXX-XXXX-1234"}, - {"SwitchSolo happy mixed", reSwitchSolo, replaceSwitchSolo, "67111111 1111-1234", "Switch/Solo XXXX-XXXX-XXXX-1234"}, - {"SwitchSolo happy digits", reSwitchSolo, replaceSwitchSolo, "abc 6711111111111234 def", "abc Switch/Solo XXXX-XXXX-XXXX-1234 def"}, - {"SwitchSolo non-match start", reSwitchSolo, replaceSwitchSolo, "3111111111111234", ""}, - {"SwitchSolo non-match num digits", reSwitchSolo, replaceSwitchSolo, " 6711-1111-1111-123", ""}, - {"SwitchSolo non-match sep", reSwitchSolo, replaceSwitchSolo, "6711=1111=1111_1234", ""}, - {"SwitchSolo non-match no break before", reSwitchSolo, replaceSwitchSolo, "abc6711-1111-1111-1234", "abcSwitch/Solo XXXX-XXXX-XXXX-1234"}, - {"SwitchSolo non-match no break after", reSwitchSolo, replaceSwitchSolo, "6711-1111-1111-1234def", "Switch/Solo XXXX-XXXX-XXXX-1234def"}, - - {"Discover happy spaces", reDiscover, replaceDiscover, " abc 6011 1111 1111 1234 def", " abc Discover XXXX-XXXX-XXXX-1234 def"}, - {"Discover happy dashes", reDiscover, replaceDiscover, "6011-1111-1111-1234", "Discover XXXX-XXXX-XXXX-1234"}, - {"Discover happy mixed", reDiscover, replaceDiscover, "60111111 1111-1234", "Discover XXXX-XXXX-XXXX-1234"}, - {"Discover happy digits", reDiscover, replaceDiscover, "abc 6011111111111234 def", "abc Discover XXXX-XXXX-XXXX-1234 def"}, - {"Discover non-match start", reDiscover, replaceDiscover, "3111111111111234", ""}, - {"Discover non-match num digits", reDiscover, replaceDiscover, " 6011-1111-1111-123", ""}, - {"Discover non-match sep", reDiscover, replaceDiscover, "6011=1111=1111_1234", ""}, - {"Discover non-match no break before", reDiscover, replaceDiscover, "abc6011-1111-1111-1234", "abcDiscover XXXX-XXXX-XXXX-1234"}, - {"Discover non-match no break after", reDiscover, replaceDiscover, "6011-1111-1111-1234def", "Discover XXXX-XXXX-XXXX-1234def"}, - - {"AMEX happy spaces", reAMEX, replaceAMEX, " abc 3411 123456 12345 def", " abc American Express XXXX-XXXXXX-X2345 def"}, - {"AMEX happy dashes", reAMEX, replaceAMEX, "3711-123456-12345", "American Express XXXX-XXXXXX-X2345"}, - {"AMEX happy mixed", reAMEX, replaceAMEX, "3411-123456 12345", "American Express XXXX-XXXXXX-X2345"}, - {"AMEX happy digits", reAMEX, replaceAMEX, "abc 371112345612345 def", "abc American Express XXXX-XXXXXX-X2345 def"}, - {"AMEX non-match start 41", reAMEX, replaceAMEX, "411112345612345", ""}, - {"AMEX non-match start 31", reAMEX, replaceAMEX, "3111111111111234", ""}, - {"AMEX non-match num digits", reAMEX, replaceAMEX, " 4111-1111-1111-123", ""}, - {"AMEX non-match sep", reAMEX, replaceAMEX, "4111-1111=1111-1234", ""}, - {"AMEX non-match no break before", reAMEX, replaceAMEX, "abc3711-123456-12345", "abcAmerican Express XXXX-XXXXXX-X2345"}, - {"AMEX non-match no break after", reAMEX, replaceAMEX, "3711-123456-12345def", "American Express XXXX-XXXXXX-X2345def"}, - } { - t.Run(tc.Name, func(t *testing.T) { - re := regexp.MustCompile(tc.RE) - result := re.ReplaceAllString(tc.In, tc.Replace) - if tc.Out != "" { - assert.Equal(t, tc.Out, result) - } else { - assert.Equal(t, tc.In, result) - } - }) - } -} - -const ( - reSSN = `(?P(?P\d{3})[ -]?(?P\d{2})[ -]?` + reLastFour + `)` - replaceSSN = `XXX-XX-$LastFour` -) - -func TestSSNRegex(t *testing.T) { - for _, tc := range []struct { - Name string - RE string - Replace string - In string - Out string - }{ - {"SSN happy spaces", reSSN, replaceSSN, " abc 652 47 3356 def", " abc XXX-XX-3356 def"}, - {"SSN happy dashes", reSSN, replaceSSN, " abc 652-47-3356 def", " abc XXX-XX-3356 def"}, - {"SSN happy digits", reSSN, replaceSSN, " abc 652473356 def", " abc XXX-XX-3356 def"}, - {"SSN happy mixed1", reSSN, replaceSSN, " abc 65247-3356 def", " abc XXX-XX-3356 def"}, - {"SSN happy mixed2", reSSN, replaceSSN, " abc 652 47-3356 def", " abc XXX-XX-3356 def"}, - {"SSN non-match 19-09-9999", reSSN, replaceSSN, " abc 19-09-9999 def", " abc 19-09-9999 def"}, - {"SSN non-match 652_47-3356", reSSN, replaceSSN, " abc 652_47-3356 def", " abc 652_47-3356 def"}, - } { - t.Run(tc.Name, func(t *testing.T) { - re := regexp.MustCompile(tc.RE) - result := re.ReplaceAllString(tc.In, tc.Replace) - if tc.Out != "" { - assert.Equal(t, tc.Out, result) - } else { - assert.Equal(t, tc.In, result) - } - }) - } -} - -func TestCreditCard(t *testing.T) { - var tests = []struct { - Name string - Link *Link - inputMessage string - expectedMessage string - }{ - { - "VISA happy", - &Link{ - Pattern: reVISA, - Template: replaceVISA, - }, - "A credit card 4111-1111-2222-1234 mentioned", - "A credit card VISA XXXX-XXXX-XXXX-1234 mentioned", - }, { - "VISA", - &Link{ - Pattern: reVISA, - Template: replaceVISA, - DisableNonWordPrefix: true, - DisableNonWordSuffix: true, - }, - "A credit card4111-1111-2222-3333mentioned", - "A credit cardVISA XXXX-XXXX-XXXX-3333mentioned", - }, { - "Multiple VISA replacements", - &Link{ - Pattern: reVISA, - Template: replaceVISA, - }, - "Credit cards 4111-1111-2222-3333 and 4222-3333-4444-5678 mentioned", - "Credit cards VISA XXXX-XXXX-XXXX-3333 and VISA XXXX-XXXX-XXXX-5678 mentioned", - }, - } - - for _, tt := range tests { - t.Run(tt.Name, func(t *testing.T) { - al, _ := NewAutoLinker(*tt.Link) - actual := al.Replace(tt.inputMessage) - - assert.Equal(t, tt.expectedMessage, actual) - }) - } -} - -func TestAutolink(t *testing.T) { - for _, tc := range []struct { - Name string - Link *Link - Message string - ExpectedMessage string - }{ - { - "Simple pattern", - &Link{ - Pattern: "(Mattermost)", - Template: "[Mattermost](https://mattermost.com)", - }, - "Welcome to Mattermost!", - "Welcome to [Mattermost](https://mattermost.com)!", - }, { - "Pattern with variable name accessed using $variable", - &Link{ - Pattern: "(?PMattermost)", - Template: "[$key](https://mattermost.com)", - }, - "Welcome to Mattermost!", - "Welcome to [Mattermost](https://mattermost.com)!", - }, { - "Multiple replacments", - &Link{ - Pattern: "(?PMattermost)", - Template: "[$key](https://mattermost.com)", - }, - "Welcome to Mattermost and have fun with Mattermost!", - "Welcome to [Mattermost](https://mattermost.com) and have fun with [Mattermost](https://mattermost.com)!", - }, { - "Pattern with variable name accessed using ${variable}", - &Link{ - Pattern: "(?PMattermost)", - Template: "[${key}](https://mattermost.com)", - }, - "Welcome to Mattermost!", - "Welcome to [Mattermost](https://mattermost.com)!", - }, { - "Jira example", - &Link{ - Pattern: "(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "Welcome MM-12345 should link!", - "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345) should link!", - }, { - "Jira example 2 (within a ())", - &Link{ - Pattern: "(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "Link in brackets should link (see MM-12345)", - "Link in brackets should link (see [MM-12345](https://mattermost.atlassian.net/browse/MM-12345))", - }, { - "Jira example 3 (before ,)", - &Link{ - Pattern: "(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "Link a ticket MM-12345, before a comma", - "Link a ticket [MM-12345](https://mattermost.atlassian.net/browse/MM-12345), before a comma", - }, { - "Jira example 3 (at begin of the message)", - &Link{ - Pattern: "(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "MM-12345 should link!", - "[MM-12345](https://mattermost.atlassian.net/browse/MM-12345) should link!", - }, { - "Pattern word prefix and suffix disabled", - &Link{ - Pattern: "(?P^|\\s)(MM)(-)(?P\\d+)", - Template: "${previous}[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - DisableNonWordPrefix: true, - DisableNonWordSuffix: true, - }, - "Welcome MM-12345 should link!", - "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345) should link!", - }, { - "Pattern word prefix and suffix disabled (at begin of the message)", - &Link{ - Pattern: "(?P^|\\s)(MM)(-)(?P\\d+)", - Template: "${previous}[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - DisableNonWordPrefix: true, - DisableNonWordSuffix: true, - }, - "MM-12345 should link!", - "[MM-12345](https://mattermost.atlassian.net/browse/MM-12345) should link!", - }, { - "Pattern word prefix and suffix enable (in the middle of other text)", - &Link{ - Pattern: "(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "WelcomeMM-12345should not link!", - "WelcomeMM-12345should not link!", - }, { - "Pattern word prefix and suffix disabled (in the middle of other text)", - &Link{ - Pattern: "(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - DisableNonWordPrefix: true, - DisableNonWordSuffix: true, - }, - "WelcomeMM-12345should link!", - "Welcome[MM-12345](https://mattermost.atlassian.net/browse/MM-12345)should link!", - }, { - "Not relinking", - &Link{ - Pattern: "(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345) should not re-link!", - "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345) should not re-link!", - }, { - "Url replacement", - &Link{ - Pattern: "(https://mattermost.atlassian.net/browse/)(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "Welcome https://mattermost.atlassian.net/browse/MM-12345 should link!", - "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345) should link!", - }, { - "Url replacement multiple times", - &Link{ - Pattern: "(https://mattermost.atlassian.net/browse/)(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "Welcome https://mattermost.atlassian.net/browse/MM-12345. should link https://mattermost.atlassian.net/browse/MM-12346 !", - "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345). should link [MM-12346](https://mattermost.atlassian.net/browse/MM-12346) !", - }, { - "Url replacement multiple times and at beginning", - &Link{ - Pattern: "(https:\\/\\/mattermost.atlassian.net\\/browse\\/)(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "https://mattermost.atlassian.net/browse/MM-12345 https://mattermost.atlassian.net/browse/MM-12345", - "[MM-12345](https://mattermost.atlassian.net/browse/MM-12345) [MM-12345](https://mattermost.atlassian.net/browse/MM-12345)", - }, { - "Url replacement at end", - &Link{ - Pattern: "(https://mattermost.atlassian.net/browse/)(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "Welcome https://mattermost.atlassian.net/browse/MM-12345", - "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345)", - }, - } { - - t.Run(tc.Name, func(t *testing.T) { - p := setupTestPlugin(t, *tc.Link) - post, _ := p.MessageWillBePosted(nil, &model.Post{ - Message: tc.Message, - }) - - assert.Equal(t, tc.ExpectedMessage, post.Message) - }) - - } -} - -func TestAutolinkWordBoundaries(t *testing.T) { - const pattern = "(KEY)(-)(?P\\d+)" - const template = "[KEY-$ID](someurl/KEY-$ID)" - const ref = "KEY-12345" - const ID = "12345" - const markdown = "[KEY-12345](someurl/KEY-12345)" - - var defaultLink = Link{ - Pattern: pattern, - Template: template, - } - - linkNoPrefix := defaultLink - linkNoPrefix.DisableNonWordPrefix = true - - linkNoSuffix := defaultLink - linkNoSuffix.DisableNonWordSuffix = true - - linkNoPrefixNoSuffix := defaultLink - linkNoPrefixNoSuffix.DisableNonWordSuffix = true - linkNoPrefixNoSuffix.DisableNonWordPrefix = true - - for _, tc := range []struct { - Name string - Sep string - Link *Link - Prefix string - Suffix string - ExpectFail bool - }{ - {Name: "space both sides both breaks required", Prefix: " ", Suffix: " "}, - {Name: "space both sides left break not required", Prefix: " ", Suffix: " ", Link: &linkNoPrefix}, - {Name: "space both sides right break not required", Prefix: " ", Suffix: " ", Link: &linkNoSuffix}, - {Name: "space both sides neither break required", Prefix: " ", Suffix: " ", Link: &linkNoPrefixNoSuffix}, - - {Name: "space left side both breaks required", Prefix: " ", ExpectFail: true}, - {Name: "space left side left break not required", Prefix: " ", Link: &linkNoPrefix, ExpectFail: true}, - {Name: "space left side right break not required", Prefix: " ", Link: &linkNoSuffix}, - {Name: "space left side neither break required", Prefix: " ", Link: &linkNoPrefixNoSuffix}, - - {Name: "space right side both breaks required", Suffix: " ", ExpectFail: true}, - {Name: "space right side left break not required", Suffix: " ", Link: &linkNoPrefix}, - {Name: "space right side right break not required", Suffix: " ", Link: &linkNoSuffix, ExpectFail: true}, - {Name: "space right side neither break required", Prefix: " ", Link: &linkNoPrefixNoSuffix}, - - {Name: "none both breaks required", ExpectFail: true}, - {Name: "none left break not required", Link: &linkNoPrefix, ExpectFail: true}, - {Name: "none right break not required", Link: &linkNoSuffix, ExpectFail: true}, - {Name: "none neither break required", Link: &linkNoPrefixNoSuffix}, - - {Sep: "paren", Name: "2 parens", Prefix: "(", Suffix: ")"}, - {Sep: "paren", Name: "left paren", Prefix: "(", Link: &linkNoSuffix}, - {Sep: "paren", Name: "right paren", Suffix: ")", Link: &linkNoPrefix}, - {Sep: "sbracket", Name: "2 brackets", Prefix: "[", Suffix: "]"}, - {Sep: "lsbracket", Name: "both breaks", Prefix: "[", ExpectFail: true}, - {Sep: "lsbracket", Name: "bracket no prefix", Prefix: "[", Link: &linkNoPrefix, ExpectFail: true}, - {Sep: "lsbracket", Name: "bracket no suffix", Prefix: "[", Link: &linkNoSuffix}, - {Sep: "lsbracket", Name: "bracket neither prefix suffix", Prefix: "[", Link: &linkNoPrefixNoSuffix}, - {Sep: "rsbracket", Name: "bracket", Suffix: "]", Link: &linkNoPrefix}, - {Sep: "rand", Name: "random separators", Prefix: "% (", Suffix: "-- $%^&"}, - } { - - orig := fmt.Sprintf("word1%s%s%sword2", tc.Prefix, ref, tc.Suffix) - expected := fmt.Sprintf("word1%s%s%sword2", tc.Prefix, markdown, tc.Suffix) - - pref := tc.Prefix - suff := tc.Suffix - if tc.Sep != "" { - pref = "_" + tc.Sep + "_" - suff = "_" + tc.Sep + "_" - } - name := fmt.Sprintf("word1%s%s%sword2", pref, ref, suff) - if tc.Name != "" { - name = tc.Name + " " + name - } - - t.Run(name, func(t *testing.T) { - l := tc.Link - if l == nil { - l = &defaultLink - } - p := setupTestPlugin(t, *l) - - post, _ := p.MessageWillBePosted(nil, &model.Post{ - Message: orig, - }) - if tc.ExpectFail { - assert.Equal(t, orig, post.Message) - return - } - assert.Equal(t, expected, post.Message) - }) - } -} - -func TestAutolinkErrors(t *testing.T) { - var tests = []struct { - Name string - Link Link - }{ - { - "Empty Link", - Link{}, - }, { - "No pattern", - Link{ - Pattern: "", - Template: "blah", - }, - }, { - "No template", - Link{ - Pattern: "blah", - Template: "", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.Name, func(t *testing.T) { - _, err := NewAutoLinker(tt.Link) - assert.NotNil(t, err) - }) - } -} diff --git a/server/config.go b/server/config.go index c6431d94..d2adfaa2 100755 --- a/server/config.go +++ b/server/config.go @@ -1,15 +1,71 @@ package main -// Link represents a pattern to autolink -type Link struct { - Pattern string - Template string - Scope []string - DisableNonWordPrefix bool - DisableNonWordSuffix bool -} - -// Configuration from config.json -type Configuration struct { - Links []*Link +import ( + "fmt" + "sort" + "strings" + + "github.com/mattermost/mattermost-server/mlog" +) + +// Config from config.json +type Config struct { + Links []Link +} + +// OnConfigurationChange is invoked when configuration changes may have been made. +func (p *Plugin) OnConfigurationChange() error { + var c Config + err := p.API.LoadPluginConfiguration(&c) + if err != nil { + return err + } + + for i := range c.Links { + err = c.Links[i].Compile() + if err != nil { + mlog.Error(fmt.Sprintf("Error creating autolinker: %+v: %v", c.Links[i], err)) + } + } + + p.updateConfig(func(conf *Config) { + *conf = c + }) + return nil +} + +func (p *Plugin) getConfig() Config { + p.confLock.RLock() + defer p.confLock.RUnlock() + return p.conf +} + +func (p *Plugin) updateConfig(f func(conf *Config)) Config { + p.confLock.Lock() + defer p.confLock.Unlock() + + f(&p.conf) + return p.conf +} + +// ToConfig marshals Config into a tree of map[string]interface{} to pass down +// to p.API.SavePluginConfig, otherwise RPC/gob barfs at the unknown type. +func (conf Config) ToConfig() map[string]interface{} { + links := []interface{}{} + for _, l := range conf.Links { + links = append(links, l.ToConfig()) + } + return map[string]interface{}{ + "Links": links, + } +} + +// Sorted returns a clone of the Config, with links sorted alphabetically +func (conf Config) Sorted() Config { + sorted := conf + sorted.Links = append([]Link{}, conf.Links...) + sort.Slice(conf.Links, func(i, j int) bool { + return strings.Compare(conf.Links[i].DisplayName(), conf.Links[j].DisplayName()) < 0 + }) + return conf } diff --git a/server/plugin.go b/server/plugin.go index da8c50c1..732aa45e 100644 --- a/server/plugin.go +++ b/server/plugin.go @@ -2,9 +2,10 @@ package main import ( "fmt" - "sync/atomic" + "sync" "github.com/mattermost/mattermost-server/mlog" + "github.com/pkg/errors" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/plugin" @@ -15,35 +16,31 @@ import ( type Plugin struct { plugin.MattermostPlugin - links atomic.Value + // configuration and a muttex to control concurrent access + conf Config + confLock sync.RWMutex } -// OnConfigurationChange is invoked when configuration changes may have been made. -func (p *Plugin) OnConfigurationChange() error { - var c Configuration - err := p.API.LoadPluginConfiguration(&c) +func (p *Plugin) OnActivate() error { + err := p.API.RegisterCommand(&model.Command{ + Trigger: "autolink", + DisplayName: "Autolink", + Description: "Autolink administration.", + AutoComplete: true, + AutoCompleteDesc: "Available commands: config", + AutoCompleteHint: "[command]", + }) if err != nil { - return err - } - links := make([]*AutoLinker, 0) - - for _, l := range c.Links { - al, lerr := NewAutoLinker(*l) - if lerr != nil { - mlog.Error("Error creating autolinker: ") - } - - links = append(links, al) + return errors.WithMessage(err, "OnActivate: failed to register command") } - p.links.Store(links) return nil } // MessageWillBePosted is invoked when a message is posted by a user before it is committed // to the database. func (p *Plugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) { - links := p.links.Load().([]*AutoLinker) + conf := p.getConfig() postText := post.Message offset := 0 markdown.Inspect(post.Message, func(node interface{}) bool { @@ -97,10 +94,10 @@ func (p *Plugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*mode teamName = team.Name } - for _, l := range links { - if len(l.link.Scope) == 0 { + for _, l := range conf.Links { + if len(l.Scope) == 0 { newText = l.Replace(newText) - } else if teamName != "" && contains(teamName, channel.Name, l.link.Scope) { + } else if teamName != "" && contains(teamName, channel.Name, l.Scope) { newText = l.Replace(newText) } } diff --git a/server/plugin_test.go b/server/plugin_test.go index a2440a80..0860757d 100644 --- a/server/plugin_test.go +++ b/server/plugin_test.go @@ -11,12 +11,14 @@ import ( ) func TestPlugin(t *testing.T) { - links := make([]*Link, 0) - links = append(links, &Link{ - Pattern: "(Mattermost)", - Template: "[Mattermost](https://mattermost.com)", - }) - validConfiguration := Configuration{links} + conf := Config{ + Links: []Link{ + Link{ + Pattern: "(Mattermost)", + Template: "[Mattermost](https://mattermost.com)", + }, + }, + } testChannel := model.Channel{ Name: "TestChanel", @@ -28,8 +30,8 @@ func TestPlugin(t *testing.T) { api := &plugintest.API{} - api.On("LoadPluginConfiguration", mock.AnythingOfType("*main.Configuration")).Return(func(dest interface{}) error { - *dest.(*Configuration) = validConfiguration + api.On("LoadPluginConfiguration", mock.AnythingOfType("*main.Config")).Return(func(dest interface{}) error { + *dest.(*Config) = conf return nil }) @@ -47,29 +49,29 @@ func TestPlugin(t *testing.T) { } func TestSpecialCases(t *testing.T) { - links := make([]*Link, 0) - links = append(links, &Link{ + links := make([]Link, 0) + links = append(links, Link{ Pattern: "(Mattermost)", Template: "[Mattermost](https://mattermost.com)", - }, &Link{ + }, Link{ Pattern: "(Example)", Template: "[Example](https://example.com)", - }, &Link{ + }, Link{ Pattern: "(https://mattermost.atlassian.net/browse/)(MM)(-)(?P\\d+)", Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, &Link{ + }, Link{ Pattern: "(foo!bar)", Template: "fb", - }, &Link{ - Pattern: "(example)", + }, Link{ + Pattern: "(example)", Template: "test", - Scope: []string{"team/off-topic"}, - }, &Link{ - Pattern: "(example)", + Scope: []string{"team/off-topic"}, + }, Link{ + Pattern: "(example)", Template: "test", - Scope: []string{"other-team/town-square"}, + Scope: []string{"other-team/town-square"}, }) - validConfiguration := Configuration{links} + validConfig := Config{links} testChannel := model.Channel{ Name: "TestChanel", @@ -81,8 +83,8 @@ func TestSpecialCases(t *testing.T) { api := &plugintest.API{} - api.On("LoadPluginConfiguration", mock.AnythingOfType("*main.Configuration")).Return(func(dest interface{}) error { - *dest.(*Configuration) = validConfiguration + api.On("LoadPluginConfiguration", mock.AnythingOfType("*main.Config")).Return(func(dest interface{}) error { + *dest.(*Config) = validConfig return nil }) From c6c3c7e07bba01bdff798e309764afe8a1325d9c Mon Sep 17 00:00:00 2001 From: Lev Brouk Date: Thu, 1 Aug 2019 06:18:37 -0700 Subject: [PATCH 3/8] fixed paths --- server/autolinker.go | 46 ---- server/autolinker_test.go | 490 -------------------------------------- 2 files changed, 536 deletions(-) delete mode 100644 server/autolinker.go delete mode 100644 server/autolinker_test.go diff --git a/server/autolinker.go b/server/autolinker.go deleted file mode 100644 index e6695dbd..00000000 --- a/server/autolinker.go +++ /dev/null @@ -1,46 +0,0 @@ -package main - -import ( - "errors" - "regexp" -) - -// AutoLinker helper for replace regex with links -type AutoLinker struct { - link *Link - re *regexp.Regexp -} - -// NewAutoLinker create and initialize a AutoLinker -func NewAutoLinker(link Link) (*AutoLinker, error) { - if len(link.Pattern) == 0 || len(link.Template) == 0 { - return nil, errors.New("Pattern or template was empty") - } - - if !link.DisableNonWordPrefix { - link.Pattern = `\b` + link.Pattern - } - - if !link.DisableNonWordSuffix { - link.Pattern = link.Pattern + `\b` - } - - re, err := regexp.Compile(link.Pattern) - if err != nil { - return nil, err - } - - return &AutoLinker{ - link: &link, - re: re, - }, nil -} - -// Replace will subsitute the regex's with the supplied links -func (l *AutoLinker) Replace(message string) string { - if l.re == nil { - return message - } - - return l.re.ReplaceAllString(message, l.link.Template) -} diff --git a/server/autolinker_test.go b/server/autolinker_test.go deleted file mode 100644 index 0670848d..00000000 --- a/server/autolinker_test.go +++ /dev/null @@ -1,490 +0,0 @@ -package main - -import ( - "fmt" - "regexp" - "testing" - - "github.com/mattermost/mattermost-server/model" - "github.com/mattermost/mattermost-server/plugin/plugintest" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" -) - -func setupTestPlugin(t *testing.T, link Link) *Plugin { - p := &Plugin{} - api := &plugintest.API{} - - api.On("GetChannel", mock.AnythingOfType("string")).Run(func(args mock.Arguments) { - api.On("GetTeam", mock.AnythingOfType("string")).Return(&model.Team{}, (*model.AppError)(nil)) - }).Return(&model.Channel{ - Id: "thechannel_id0123456789012", - TeamId: "theteam_id0123456789012345", - Name: "thechannel_name", - }, (*model.AppError)(nil)) - p.SetAPI(api) - - al, err := NewAutoLinker(link) - require.Nil(t, err) - p.links.Store([]*AutoLinker{al}) - return p -} - -const ( - reLastFour = `(?P[0-9]{4})` - reVISA = `(?P` + `(?P4\d{3})[ -]?(?P\d{4})[ -]?(?P\d{4})[ -]?` + reLastFour + `)` - reMasterCard = `(?P` + `(?P5[1-5]\d{2})[ -]?(?P\d{4})[ -]?(?P\d{4})[ -]?` + reLastFour + `)` - reSwitchSolo = `(?P` + `(?P67\d{2})[ -]?(?P\d{4})[ -]?(?P\d{4})[ -]?` + reLastFour + `)` - reDiscover = `(?P` + `(?P6011)[ -]?(?P\d{4})[ -]?(?P\d{4})[ -]?` + reLastFour + `)` - reAMEX = `(?P` + `(?P3[47]\d{2})[ -]?(?P\d{6})[ -]?(?P\d)` + reLastFour + `)` - - replace4444 = `XXXX-XXXX-XXXX-$LastFour` - replace465 = `XXXX-XXXXXX-X$LastFour` - - replaceVISA = "VISA " + replace4444 - replaceMasterCard = "MasterCard " + replace4444 - replaceSwitchSolo = "Switch/Solo " + replace4444 - replaceDiscover = "Discover " + replace4444 - replaceAMEX = "American Express " + replace465 -) - -func TestCCRegex(t *testing.T) { - for _, tc := range []struct { - Name string - RE string - Replace string - In string - Out string - }{ - {"Visa happy spaces", reVISA, replaceVISA, " abc 4111 1111 1111 1234 def", " abc VISA XXXX-XXXX-XXXX-1234 def"}, - {"Visa happy dashes", reVISA, replaceVISA, "4111-1111-1111-1234", "VISA XXXX-XXXX-XXXX-1234"}, - {"Visa happy mixed", reVISA, replaceVISA, "41111111 1111-1234", "VISA XXXX-XXXX-XXXX-1234"}, - {"Visa happy digits", reVISA, replaceVISA, "abc 4111111111111234 def", "abc VISA XXXX-XXXX-XXXX-1234 def"}, - {"Visa non-match start", reVISA, replaceVISA, "3111111111111234", ""}, - {"Visa non-match num digits", reVISA, replaceVISA, " 4111-1111-1111-123", ""}, - {"Visa non-match sep", reVISA, replaceVISA, "4111=1111=1111_1234", ""}, - {"Visa non-match no break before", reVISA, replaceVISA, "abc4111-1111-1111-1234", "abcVISA XXXX-XXXX-XXXX-1234"}, - {"Visa non-match no break after", reVISA, replaceVISA, "4111-1111-1111-1234def", "VISA XXXX-XXXX-XXXX-1234def"}, - - {"MasterCard happy spaces", reMasterCard, replaceMasterCard, " abc 5111 1111 1111 1234 def", " abc MasterCard XXXX-XXXX-XXXX-1234 def"}, - {"MasterCard happy dashes", reMasterCard, replaceMasterCard, "5211-1111-1111-1234", "MasterCard XXXX-XXXX-XXXX-1234"}, - {"MasterCard happy mixed", reMasterCard, replaceMasterCard, "53111111 1111-1234", "MasterCard XXXX-XXXX-XXXX-1234"}, - {"MasterCard happy digits", reMasterCard, replaceMasterCard, "abc 5411111111111234 def", "abc MasterCard XXXX-XXXX-XXXX-1234 def"}, - {"MasterCard non-match start", reMasterCard, replaceMasterCard, "3111111111111234", ""}, - {"MasterCard non-match num digits", reMasterCard, replaceMasterCard, " 5111-1111-1111-123", ""}, - {"MasterCard non-match sep", reMasterCard, replaceMasterCard, "5111=1111=1111_1234", ""}, - {"MasterCard non-match no break before", reMasterCard, replaceMasterCard, "abc5511-1111-1111-1234", "abcMasterCard XXXX-XXXX-XXXX-1234"}, - {"MasterCard non-match no break after", reMasterCard, replaceMasterCard, "5111-1111-1111-1234def", "MasterCard XXXX-XXXX-XXXX-1234def"}, - - {"SwitchSolo happy spaces", reSwitchSolo, replaceSwitchSolo, " abc 6711 1111 1111 1234 def", " abc Switch/Solo XXXX-XXXX-XXXX-1234 def"}, - {"SwitchSolo happy dashes", reSwitchSolo, replaceSwitchSolo, "6711-1111-1111-1234", "Switch/Solo XXXX-XXXX-XXXX-1234"}, - {"SwitchSolo happy mixed", reSwitchSolo, replaceSwitchSolo, "67111111 1111-1234", "Switch/Solo XXXX-XXXX-XXXX-1234"}, - {"SwitchSolo happy digits", reSwitchSolo, replaceSwitchSolo, "abc 6711111111111234 def", "abc Switch/Solo XXXX-XXXX-XXXX-1234 def"}, - {"SwitchSolo non-match start", reSwitchSolo, replaceSwitchSolo, "3111111111111234", ""}, - {"SwitchSolo non-match num digits", reSwitchSolo, replaceSwitchSolo, " 6711-1111-1111-123", ""}, - {"SwitchSolo non-match sep", reSwitchSolo, replaceSwitchSolo, "6711=1111=1111_1234", ""}, - {"SwitchSolo non-match no break before", reSwitchSolo, replaceSwitchSolo, "abc6711-1111-1111-1234", "abcSwitch/Solo XXXX-XXXX-XXXX-1234"}, - {"SwitchSolo non-match no break after", reSwitchSolo, replaceSwitchSolo, "6711-1111-1111-1234def", "Switch/Solo XXXX-XXXX-XXXX-1234def"}, - - {"Discover happy spaces", reDiscover, replaceDiscover, " abc 6011 1111 1111 1234 def", " abc Discover XXXX-XXXX-XXXX-1234 def"}, - {"Discover happy dashes", reDiscover, replaceDiscover, "6011-1111-1111-1234", "Discover XXXX-XXXX-XXXX-1234"}, - {"Discover happy mixed", reDiscover, replaceDiscover, "60111111 1111-1234", "Discover XXXX-XXXX-XXXX-1234"}, - {"Discover happy digits", reDiscover, replaceDiscover, "abc 6011111111111234 def", "abc Discover XXXX-XXXX-XXXX-1234 def"}, - {"Discover non-match start", reDiscover, replaceDiscover, "3111111111111234", ""}, - {"Discover non-match num digits", reDiscover, replaceDiscover, " 6011-1111-1111-123", ""}, - {"Discover non-match sep", reDiscover, replaceDiscover, "6011=1111=1111_1234", ""}, - {"Discover non-match no break before", reDiscover, replaceDiscover, "abc6011-1111-1111-1234", "abcDiscover XXXX-XXXX-XXXX-1234"}, - {"Discover non-match no break after", reDiscover, replaceDiscover, "6011-1111-1111-1234def", "Discover XXXX-XXXX-XXXX-1234def"}, - - {"AMEX happy spaces", reAMEX, replaceAMEX, " abc 3411 123456 12345 def", " abc American Express XXXX-XXXXXX-X2345 def"}, - {"AMEX happy dashes", reAMEX, replaceAMEX, "3711-123456-12345", "American Express XXXX-XXXXXX-X2345"}, - {"AMEX happy mixed", reAMEX, replaceAMEX, "3411-123456 12345", "American Express XXXX-XXXXXX-X2345"}, - {"AMEX happy digits", reAMEX, replaceAMEX, "abc 371112345612345 def", "abc American Express XXXX-XXXXXX-X2345 def"}, - {"AMEX non-match start 41", reAMEX, replaceAMEX, "411112345612345", ""}, - {"AMEX non-match start 31", reAMEX, replaceAMEX, "3111111111111234", ""}, - {"AMEX non-match num digits", reAMEX, replaceAMEX, " 4111-1111-1111-123", ""}, - {"AMEX non-match sep", reAMEX, replaceAMEX, "4111-1111=1111-1234", ""}, - {"AMEX non-match no break before", reAMEX, replaceAMEX, "abc3711-123456-12345", "abcAmerican Express XXXX-XXXXXX-X2345"}, - {"AMEX non-match no break after", reAMEX, replaceAMEX, "3711-123456-12345def", "American Express XXXX-XXXXXX-X2345def"}, - } { - t.Run(tc.Name, func(t *testing.T) { - re := regexp.MustCompile(tc.RE) - result := re.ReplaceAllString(tc.In, tc.Replace) - if tc.Out != "" { - assert.Equal(t, tc.Out, result) - } else { - assert.Equal(t, tc.In, result) - } - }) - } -} - -const ( - reSSN = `(?P(?P\d{3})[ -]?(?P\d{2})[ -]?` + reLastFour + `)` - replaceSSN = `XXX-XX-$LastFour` -) - -func TestSSNRegex(t *testing.T) { - for _, tc := range []struct { - Name string - RE string - Replace string - In string - Out string - }{ - {"SSN happy spaces", reSSN, replaceSSN, " abc 652 47 3356 def", " abc XXX-XX-3356 def"}, - {"SSN happy dashes", reSSN, replaceSSN, " abc 652-47-3356 def", " abc XXX-XX-3356 def"}, - {"SSN happy digits", reSSN, replaceSSN, " abc 652473356 def", " abc XXX-XX-3356 def"}, - {"SSN happy mixed1", reSSN, replaceSSN, " abc 65247-3356 def", " abc XXX-XX-3356 def"}, - {"SSN happy mixed2", reSSN, replaceSSN, " abc 652 47-3356 def", " abc XXX-XX-3356 def"}, - {"SSN non-match 19-09-9999", reSSN, replaceSSN, " abc 19-09-9999 def", " abc 19-09-9999 def"}, - {"SSN non-match 652_47-3356", reSSN, replaceSSN, " abc 652_47-3356 def", " abc 652_47-3356 def"}, - } { - t.Run(tc.Name, func(t *testing.T) { - re := regexp.MustCompile(tc.RE) - result := re.ReplaceAllString(tc.In, tc.Replace) - if tc.Out != "" { - assert.Equal(t, tc.Out, result) - } else { - assert.Equal(t, tc.In, result) - } - }) - } -} - -func TestCreditCard(t *testing.T) { - var tests = []struct { - Name string - Link *Link - inputMessage string - expectedMessage string - }{ - { - "VISA happy", - &Link{ - Pattern: reVISA, - Template: replaceVISA, - }, - "A credit card 4111-1111-2222-1234 mentioned", - "A credit card VISA XXXX-XXXX-XXXX-1234 mentioned", - }, { - "VISA", - &Link{ - Pattern: reVISA, - Template: replaceVISA, - DisableNonWordPrefix: true, - DisableNonWordSuffix: true, - }, - "A credit card4111-1111-2222-3333mentioned", - "A credit cardVISA XXXX-XXXX-XXXX-3333mentioned", - }, { - "Multiple VISA replacements", - &Link{ - Pattern: reVISA, - Template: replaceVISA, - }, - "Credit cards 4111-1111-2222-3333 and 4222-3333-4444-5678 mentioned", - "Credit cards VISA XXXX-XXXX-XXXX-3333 and VISA XXXX-XXXX-XXXX-5678 mentioned", - }, - } - - for _, tt := range tests { - t.Run(tt.Name, func(t *testing.T) { - al, _ := NewAutoLinker(*tt.Link) - actual := al.Replace(tt.inputMessage) - - assert.Equal(t, tt.expectedMessage, actual) - }) - } -} - -func TestAutolink(t *testing.T) { - for _, tc := range []struct { - Name string - Link *Link - Message string - ExpectedMessage string - }{ - { - "Simple pattern", - &Link{ - Pattern: "(Mattermost)", - Template: "[Mattermost](https://mattermost.com)", - }, - "Welcome to Mattermost!", - "Welcome to [Mattermost](https://mattermost.com)!", - }, { - "Pattern with variable name accessed using $variable", - &Link{ - Pattern: "(?PMattermost)", - Template: "[$key](https://mattermost.com)", - }, - "Welcome to Mattermost!", - "Welcome to [Mattermost](https://mattermost.com)!", - }, { - "Multiple replacments", - &Link{ - Pattern: "(?PMattermost)", - Template: "[$key](https://mattermost.com)", - }, - "Welcome to Mattermost and have fun with Mattermost!", - "Welcome to [Mattermost](https://mattermost.com) and have fun with [Mattermost](https://mattermost.com)!", - }, { - "Pattern with variable name accessed using ${variable}", - &Link{ - Pattern: "(?PMattermost)", - Template: "[${key}](https://mattermost.com)", - }, - "Welcome to Mattermost!", - "Welcome to [Mattermost](https://mattermost.com)!", - }, { - "Jira example", - &Link{ - Pattern: "(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "Welcome MM-12345 should link!", - "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345) should link!", - }, { - "Jira example 2 (within a ())", - &Link{ - Pattern: "(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "Link in brackets should link (see MM-12345)", - "Link in brackets should link (see [MM-12345](https://mattermost.atlassian.net/browse/MM-12345))", - }, { - "Jira example 3 (before ,)", - &Link{ - Pattern: "(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "Link a ticket MM-12345, before a comma", - "Link a ticket [MM-12345](https://mattermost.atlassian.net/browse/MM-12345), before a comma", - }, { - "Jira example 3 (at begin of the message)", - &Link{ - Pattern: "(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "MM-12345 should link!", - "[MM-12345](https://mattermost.atlassian.net/browse/MM-12345) should link!", - }, { - "Pattern word prefix and suffix disabled", - &Link{ - Pattern: "(?P^|\\s)(MM)(-)(?P\\d+)", - Template: "${previous}[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - DisableNonWordPrefix: true, - DisableNonWordSuffix: true, - }, - "Welcome MM-12345 should link!", - "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345) should link!", - }, { - "Pattern word prefix and suffix disabled (at begin of the message)", - &Link{ - Pattern: "(?P^|\\s)(MM)(-)(?P\\d+)", - Template: "${previous}[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - DisableNonWordPrefix: true, - DisableNonWordSuffix: true, - }, - "MM-12345 should link!", - "[MM-12345](https://mattermost.atlassian.net/browse/MM-12345) should link!", - }, { - "Pattern word prefix and suffix enable (in the middle of other text)", - &Link{ - Pattern: "(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "WelcomeMM-12345should not link!", - "WelcomeMM-12345should not link!", - }, { - "Pattern word prefix and suffix disabled (in the middle of other text)", - &Link{ - Pattern: "(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - DisableNonWordPrefix: true, - DisableNonWordSuffix: true, - }, - "WelcomeMM-12345should link!", - "Welcome[MM-12345](https://mattermost.atlassian.net/browse/MM-12345)should link!", - }, { - "Not relinking", - &Link{ - Pattern: "(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345) should not re-link!", - "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345) should not re-link!", - }, { - "Url replacement", - &Link{ - Pattern: "(https://mattermost.atlassian.net/browse/)(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "Welcome https://mattermost.atlassian.net/browse/MM-12345 should link!", - "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345) should link!", - }, { - "Url replacement multiple times", - &Link{ - Pattern: "(https://mattermost.atlassian.net/browse/)(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "Welcome https://mattermost.atlassian.net/browse/MM-12345. should link https://mattermost.atlassian.net/browse/MM-12346 !", - "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345). should link [MM-12346](https://mattermost.atlassian.net/browse/MM-12346) !", - }, { - "Url replacement multiple times and at beginning", - &Link{ - Pattern: "(https:\\/\\/mattermost.atlassian.net\\/browse\\/)(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "https://mattermost.atlassian.net/browse/MM-12345 https://mattermost.atlassian.net/browse/MM-12345", - "[MM-12345](https://mattermost.atlassian.net/browse/MM-12345) [MM-12345](https://mattermost.atlassian.net/browse/MM-12345)", - }, { - "Url replacement at end", - &Link{ - Pattern: "(https://mattermost.atlassian.net/browse/)(MM)(-)(?P\\d+)", - Template: "[MM-$jira_id](https://mattermost.atlassian.net/browse/MM-$jira_id)", - }, - "Welcome https://mattermost.atlassian.net/browse/MM-12345", - "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345)", - }, - } { - - t.Run(tc.Name, func(t *testing.T) { - p := setupTestPlugin(t, *tc.Link) - post, _ := p.MessageWillBePosted(nil, &model.Post{ - Message: tc.Message, - }) - - assert.Equal(t, tc.ExpectedMessage, post.Message) - }) - - } -} - -func TestAutolinkWordBoundaries(t *testing.T) { - const pattern = "(KEY)(-)(?P\\d+)" - const template = "[KEY-$ID](someurl/KEY-$ID)" - const ref = "KEY-12345" - const ID = "12345" - const markdown = "[KEY-12345](someurl/KEY-12345)" - - var defaultLink = Link{ - Pattern: pattern, - Template: template, - } - - linkNoPrefix := defaultLink - linkNoPrefix.DisableNonWordPrefix = true - - linkNoSuffix := defaultLink - linkNoSuffix.DisableNonWordSuffix = true - - linkNoPrefixNoSuffix := defaultLink - linkNoPrefixNoSuffix.DisableNonWordSuffix = true - linkNoPrefixNoSuffix.DisableNonWordPrefix = true - - for _, tc := range []struct { - Name string - Sep string - Link *Link - Prefix string - Suffix string - ExpectFail bool - }{ - {Name: "space both sides both breaks required", Prefix: " ", Suffix: " "}, - {Name: "space both sides left break not required", Prefix: " ", Suffix: " ", Link: &linkNoPrefix}, - {Name: "space both sides right break not required", Prefix: " ", Suffix: " ", Link: &linkNoSuffix}, - {Name: "space both sides neither break required", Prefix: " ", Suffix: " ", Link: &linkNoPrefixNoSuffix}, - - {Name: "space left side both breaks required", Prefix: " ", ExpectFail: true}, - {Name: "space left side left break not required", Prefix: " ", Link: &linkNoPrefix, ExpectFail: true}, - {Name: "space left side right break not required", Prefix: " ", Link: &linkNoSuffix}, - {Name: "space left side neither break required", Prefix: " ", Link: &linkNoPrefixNoSuffix}, - - {Name: "space right side both breaks required", Suffix: " ", ExpectFail: true}, - {Name: "space right side left break not required", Suffix: " ", Link: &linkNoPrefix}, - {Name: "space right side right break not required", Suffix: " ", Link: &linkNoSuffix, ExpectFail: true}, - {Name: "space right side neither break required", Prefix: " ", Link: &linkNoPrefixNoSuffix}, - - {Name: "none both breaks required", ExpectFail: true}, - {Name: "none left break not required", Link: &linkNoPrefix, ExpectFail: true}, - {Name: "none right break not required", Link: &linkNoSuffix, ExpectFail: true}, - {Name: "none neither break required", Link: &linkNoPrefixNoSuffix}, - - {Sep: "paren", Name: "2 parens", Prefix: "(", Suffix: ")"}, - {Sep: "paren", Name: "left paren", Prefix: "(", Link: &linkNoSuffix}, - {Sep: "paren", Name: "right paren", Suffix: ")", Link: &linkNoPrefix}, - {Sep: "sbracket", Name: "2 brackets", Prefix: "[", Suffix: "]"}, - {Sep: "lsbracket", Name: "both breaks", Prefix: "[", ExpectFail: true}, - {Sep: "lsbracket", Name: "bracket no prefix", Prefix: "[", Link: &linkNoPrefix, ExpectFail: true}, - {Sep: "lsbracket", Name: "bracket no suffix", Prefix: "[", Link: &linkNoSuffix}, - {Sep: "lsbracket", Name: "bracket neither prefix suffix", Prefix: "[", Link: &linkNoPrefixNoSuffix}, - {Sep: "rsbracket", Name: "bracket", Suffix: "]", Link: &linkNoPrefix}, - {Sep: "rand", Name: "random separators", Prefix: "% (", Suffix: "-- $%^&"}, - } { - - orig := fmt.Sprintf("word1%s%s%sword2", tc.Prefix, ref, tc.Suffix) - expected := fmt.Sprintf("word1%s%s%sword2", tc.Prefix, markdown, tc.Suffix) - - pref := tc.Prefix - suff := tc.Suffix - if tc.Sep != "" { - pref = "_" + tc.Sep + "_" - suff = "_" + tc.Sep + "_" - } - name := fmt.Sprintf("word1%s%s%sword2", pref, ref, suff) - if tc.Name != "" { - name = tc.Name + " " + name - } - - t.Run(name, func(t *testing.T) { - l := tc.Link - if l == nil { - l = &defaultLink - } - p := setupTestPlugin(t, *l) - - post, _ := p.MessageWillBePosted(nil, &model.Post{ - Message: orig, - }) - if tc.ExpectFail { - assert.Equal(t, orig, post.Message) - return - } - assert.Equal(t, expected, post.Message) - }) - } -} - -func TestAutolinkErrors(t *testing.T) { - var tests = []struct { - Name string - Link Link - }{ - { - "Empty Link", - Link{}, - }, { - "No pattern", - Link{ - Pattern: "", - Template: "blah", - }, - }, { - "No template", - Link{ - Pattern: "blah", - Template: "", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.Name, func(t *testing.T) { - _, err := NewAutoLinker(tt.Link) - assert.NotNil(t, err) - }) - } -} From 923a3c9a94b125e053ea716500cbcc4d1b2b6f39 Mon Sep 17 00:00:00 2001 From: Lev Brouk Date: Thu, 1 Aug 2019 14:21:50 -0700 Subject: [PATCH 4/8] Legacy word boundaries and tests --- server/link.go | 47 +++++++++++++-- server/link_test.go | 141 +++++++++++++++++++++++++++++++++----------- 2 files changed, 149 insertions(+), 39 deletions(-) diff --git a/server/link.go b/server/link.go index b56fc524..60723930 100755 --- a/server/link.go +++ b/server/link.go @@ -15,7 +15,10 @@ type Link struct { WordMatch bool DisableNonWordPrefix bool DisableNonWordSuffix bool - re *regexp.Regexp + + template string + re *regexp.Regexp + multipass bool } // DisplayName returns a display name for the link. @@ -33,11 +36,25 @@ func (l *Link) Compile() error { } pattern := l.Pattern + template := l.Template + multipass := false if !l.DisableNonWordPrefix { - pattern = `\b` + pattern + if l.WordMatch { + pattern = `\b` + pattern + } else { + pattern = `(?P(^|\s))` + pattern + template = `${MattermostNonWordPrefix}` + template + multipass = true + } } if !l.DisableNonWordSuffix { - pattern = pattern + `\b` + if l.WordMatch { + pattern = pattern + `\b` + } else { + pattern = pattern + `(?P$|[\s\.\!\?\,\)])` + template = template + `${MattermostNonWordSuffix}` + multipass = true + } } re, err := regexp.Compile(pattern) @@ -45,6 +62,9 @@ func (l *Link) Compile() error { return err } l.re = re + l.template = template + l.multipass = multipass + return nil } @@ -53,7 +73,26 @@ func (l Link) Replace(message string) string { if l.re == nil { return message } - return l.re.ReplaceAllString(message, l.Template) + + // Since they don't consume, `\b`s require no special handling, can just ReplaceAll + if !l.multipass { + return l.re.ReplaceAllString(message, l.template) + } + + // Replace one at a time + in := []byte(message) + out := []byte{} + for { + submatch := l.re.FindSubmatchIndex(in) + if submatch == nil { + break + } + out = append(out, in[:submatch[0]]...) + out = l.re.Expand(out, []byte(l.template), in, submatch) + in = in[submatch[1]:] + } + out = append(out, in...) + return string(out) } // ToMarkdown prints a Link as a markdown list element diff --git a/server/link_test.go b/server/link_test.go index 5a59a9d6..70501212 100644 --- a/server/link_test.go +++ b/server/link_test.go @@ -180,8 +180,8 @@ func TestCreditCard(t *testing.T) { Pattern: reVISA, Template: replaceVISA, }, - "Credit cards 4111-1111-2222-3333 and 4222-3333-4444-5678 mentioned", - "Credit cards VISA XXXX-XXXX-XXXX-3333 and VISA XXXX-XXXX-XXXX-5678 mentioned", + "Credit cards 4111-1111-2222-3333 4222-3333-4444-5678 mentioned", + "Credit cards VISA XXXX-XXXX-XXXX-3333 VISA XXXX-XXXX-XXXX-5678 mentioned", }, } @@ -194,7 +194,7 @@ func TestCreditCard(t *testing.T) { } } -func TestAutolink(t *testing.T) { +func TestLink(t *testing.T) { for _, tc := range []struct { Name string Link Link @@ -357,7 +357,7 @@ func TestAutolink(t *testing.T) { } } -func TestAutolinkWordBoundaries(t *testing.T) { +func TestLegacyWordBoundaries(t *testing.T) { const pattern = "(KEY)(-)(?P\\d+)" const template = "[KEY-$ID](someurl/KEY-$ID)" const ref = "KEY-12345" @@ -379,6 +379,108 @@ func TestAutolinkWordBoundaries(t *testing.T) { linkNoPrefixNoSuffix.DisableNonWordSuffix = true linkNoPrefixNoSuffix.DisableNonWordPrefix = true + for _, tc := range []struct { + Name string + Sep string + Link Link + Prefix string + Suffix string + ExpectFail bool + }{ + {Name: "space both sides both breaks required", Prefix: " ", Suffix: " "}, + {Name: "space both sides left break not required", Prefix: " ", Suffix: " ", Link: linkNoPrefix}, + {Name: "space both sides right break not required", Prefix: " ", Suffix: " ", Link: linkNoSuffix}, + {Name: "space both sides neither break required", Prefix: " ", Suffix: " ", Link: linkNoPrefixNoSuffix}, + + {Name: "space left side both breaks required", Prefix: " ", ExpectFail: true}, + {Name: "space left side left break not required", Prefix: " ", Link: linkNoPrefix, ExpectFail: true}, + {Name: "space left side right break not required", Prefix: " ", Link: linkNoSuffix}, + {Name: "space left side neither break required", Prefix: " ", Link: linkNoPrefixNoSuffix}, + + {Name: "space right side both breaks required", Suffix: " ", ExpectFail: true}, + {Name: "space right side left break not required", Suffix: " ", Link: linkNoPrefix}, + {Name: "space right side right break not required", Suffix: " ", Link: linkNoSuffix, ExpectFail: true}, + {Name: "space right side neither break required", Prefix: " ", Link: linkNoPrefixNoSuffix}, + + {Name: "none both breaks required", ExpectFail: true}, + {Name: "none left break not required", Link: linkNoPrefix, ExpectFail: true}, + {Name: "none right break not required", Link: linkNoSuffix, ExpectFail: true}, + {Name: "none neither break required", Link: linkNoPrefixNoSuffix}, + + // '(', '[' are not start separators + {Sep: "paren", Name: "2 parens", Prefix: "(", Suffix: ")", ExpectFail: true}, + {Sep: "paren", Name: "2 parens no suffix", Prefix: "(", Suffix: ")", Link: linkNoSuffix, ExpectFail: true}, + {Sep: "paren", Name: "left paren", Prefix: "(", Link: linkNoSuffix, ExpectFail: true}, + {Sep: "sbracket", Name: "2 brackets", Prefix: "[", Suffix: "]", ExpectFail: true}, + {Sep: "lsbracket", Name: "bracket no prefix", Prefix: "[", Link: linkNoPrefix, ExpectFail: true}, + {Sep: "lsbracket", Name: "both breaks", Prefix: "[", ExpectFail: true}, + {Sep: "lsbracket", Name: "bracket no suffix", Prefix: "[", Link: linkNoSuffix, ExpectFail: true}, + + // ']' is not a finish separator + {Sep: "rsbracket", Name: "bracket", Suffix: "]", Link: linkNoPrefix, ExpectFail: true}, + + {Sep: "paren", Name: "2 parens no prefix", Prefix: "(", Suffix: ")", Link: linkNoPrefix}, + {Sep: "paren", Name: "right paren", Suffix: ")", Link: linkNoPrefix}, + {Sep: "lsbracket", Name: "bracket neither prefix suffix", Prefix: "[", Link: linkNoPrefixNoSuffix}, + {Sep: "rand", Name: "random separators", Prefix: "%() ", Suffix: "?! $%^&"}, + } { + + orig := fmt.Sprintf("word1%s%s%sword2", tc.Prefix, ref, tc.Suffix) + expected := fmt.Sprintf("word1%s%s%sword2", tc.Prefix, markdown, tc.Suffix) + + pref := tc.Prefix + suff := tc.Suffix + if tc.Sep != "" { + pref = "_" + tc.Sep + "_" + suff = "_" + tc.Sep + "_" + } + name := fmt.Sprintf("word1%s%s%sword2", pref, ref, suff) + if tc.Name != "" { + name = tc.Name + " " + name + } + + t.Run(name, func(t *testing.T) { + l := tc.Link + if l.Pattern == "" { + l = defaultLink + } + p := setupTestPlugin(t, l) + + post, _ := p.MessageWillBePosted(nil, &model.Post{ + Message: orig, + }) + if tc.ExpectFail { + assert.Equal(t, orig, post.Message) + return + } + assert.Equal(t, expected, post.Message) + }) + } +} + +func TestWordMatch(t *testing.T) { + const pattern = "(KEY)(-)(?P\\d+)" + const template = "[KEY-$ID](someurl/KEY-$ID)" + const ref = "KEY-12345" + const ID = "12345" + const markdown = "[KEY-12345](someurl/KEY-12345)" + + var defaultLink = Link{ + Pattern: pattern, + Template: template, + WordMatch: true, + } + + linkNoPrefix := defaultLink + linkNoPrefix.DisableNonWordPrefix = true + + linkNoSuffix := defaultLink + linkNoSuffix.DisableNonWordSuffix = true + + linkNoPrefixNoSuffix := defaultLink + linkNoPrefixNoSuffix.DisableNonWordSuffix = true + linkNoPrefixNoSuffix.DisableNonWordPrefix = true + for _, tc := range []struct { Name string Sep string @@ -451,34 +553,3 @@ func TestAutolinkWordBoundaries(t *testing.T) { }) } } - -func TestAutolinkErrors(t *testing.T) { - var tests = []struct { - Name string - Link Link - }{ - { - "Empty Link", - Link{}, - }, { - "No pattern", - Link{ - Pattern: "", - Template: "blah", - }, - }, { - "No template", - Link{ - Pattern: "blah", - Template: "", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.Name, func(t *testing.T) { - err := tt.Link.Compile() - assert.NotNil(t, err) - }) - } -} From b8b3f0c343bed2749457d977c12fc1ebe86df682 Mon Sep 17 00:00:00 2001 From: Lev Brouk Date: Thu, 1 Aug 2019 15:37:46 -0700 Subject: [PATCH 5/8] Added EnableAdminCommand config setting Also - fixed the help message - fixed incorrect parsing of "false" - moved command [un-]registration OnConfigurationChanged --- plugin.json | 10 +++++++++- server/command.go | 11 ++++++----- server/config.go | 21 ++++++++++++++++++++- server/plugin.go | 17 ----------------- 4 files changed, 35 insertions(+), 24 deletions(-) diff --git a/plugin.json b/plugin.json index a704b7e5..d855b3b5 100755 --- a/plugin.json +++ b/plugin.json @@ -11,6 +11,14 @@ } }, "settings_schema": { - "header": "Configure this plugin directly in the config.json file. Learn more [in our documentation](https://github.com/mattermost/mattermost-plugin-autolink/blob/master/README.md).\n\nTo report an issue, make a suggestion or a contribution, [check the plugin repository](https://github.com/mattermost/mattermost-plugin-autolink)." + "header": "Configure this plugin directly in the config.json file. Learn more [in our documentation](https://github.com/mattermost/mattermost-plugin-autolink/blob/master/README.md).\n\nTo report an issue, make a suggestion or a contribution, [check the plugin repository](https://github.com/mattermost/mattermost-plugin-autolink).", + "settings": [ + { + "key": "EnableAdminCommand", + "display_name": "Enable administration with /autolink command", + "type": "bool", + "default": false + } + ] } } diff --git a/server/command.go b/server/command.go index 96baa30e..be5da95b 100644 --- a/server/command.go +++ b/server/command.go @@ -11,20 +11,21 @@ import ( ) const helpText = "###### Mattermost Autolink Plugin Administration\n" + - " is either the Name of a link, or its number in the `/autolink list` output.\n" + + " is either the Name of a link, or its number in the `/autolink list` output. A partial Name can be specified, but some commands require it to be uniquely resolved.\n" + "* `/autolink list` - list all configured links.\n" + "* `/autolink list ` - list a specific link.\n" + "* `/autolink enable ` - enable a link.\n" + "* `/autolink disable ` - disable a link.\n" + "* `/autolink add ` - add a new link, named .\n" + - "* `/autolink set value...` - sets a field to a value.\n" + + "* `/autolink delete ` - delete a link.\n" + + "* `/autolink set value...` - sets a link's field to a value. The entire command line after is used for the value, unescaped, leading/trailing whitespace trimmed.\n" + "\n" + "Example:\n" + "```\n" + "/autolink add Visa\n" + "/autolink disable Visa\n" + - `/autolink set Visa Template (?P(?P4\d{3})[ -]?(?P\d{4})[ -]?(?P\d{4})[ -]?(?P[0-9]{4}))` + "\n" + - "/autolink set Visa Pattern VISA XXXX-XXXX-XXXX-$LastFour\n" + + `/autolink set Visa Pattern (?P(?P4\d{3})[ -]?(?P\d{4})[ -]?(?P\d{4})[ -]?(?P[0-9]{4}))` + "\n" + + "/autolink set Visa Template VISA XXXX-XXXX-XXXX-$LastFour\n" + "/autolink set Visa WordMatch true\n" + "/autolink enable Visa\n" + "```\n" + @@ -336,7 +337,7 @@ func parseBoolArg(arg string) (bool, error) { case "true", "on": return true, nil case "false", "off": - return true, nil + return false, nil } return false, errors.Errorf("Not a bool, %q", arg) } diff --git a/server/config.go b/server/config.go index d2adfaa2..24a2b507 100755 --- a/server/config.go +++ b/server/config.go @@ -6,11 +6,14 @@ import ( "strings" "github.com/mattermost/mattermost-server/mlog" + "github.com/mattermost/mattermost-server/model" + "github.com/pkg/errors" ) // Config from config.json type Config struct { - Links []Link + EnableAdminCommand bool + Links []Link } // OnConfigurationChange is invoked when configuration changes may have been made. @@ -31,6 +34,22 @@ func (p *Plugin) OnConfigurationChange() error { p.updateConfig(func(conf *Config) { *conf = c }) + + if c.EnableAdminCommand { + err := p.API.RegisterCommand(&model.Command{ + Trigger: "autolink", + DisplayName: "Autolink", + Description: "Autolink administration.", + AutoComplete: true, + AutoCompleteHint: "[command]", + }) + if err != nil { + return errors.WithMessage(err, "failed to register /autolink command") + } + } else { + _ = p.API.UnregisterCommand("", "autolink") + } + return nil } diff --git a/server/plugin.go b/server/plugin.go index 732aa45e..134c48c0 100644 --- a/server/plugin.go +++ b/server/plugin.go @@ -5,7 +5,6 @@ import ( "sync" "github.com/mattermost/mattermost-server/mlog" - "github.com/pkg/errors" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/plugin" @@ -21,22 +20,6 @@ type Plugin struct { confLock sync.RWMutex } -func (p *Plugin) OnActivate() error { - err := p.API.RegisterCommand(&model.Command{ - Trigger: "autolink", - DisplayName: "Autolink", - Description: "Autolink administration.", - AutoComplete: true, - AutoCompleteDesc: "Available commands: config", - AutoCompleteHint: "[command]", - }) - if err != nil { - return errors.WithMessage(err, "OnActivate: failed to register command") - } - - return nil -} - // MessageWillBePosted is invoked when a message is posted by a user before it is committed // to the database. func (p *Plugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) { From 7453c479485f273ede0b4f800d2d5267256bf70a Mon Sep 17 00:00:00 2001 From: Lev Brouk Date: Thu, 1 Aug 2019 18:57:37 -0700 Subject: [PATCH 6/8] Added /autolink test, fixed deadlock, --- server/command.go | 90 ++++++++++++++++++++----------------------- server/config.go | 29 +++++++------- server/plugin_test.go | 7 +++- 3 files changed, 61 insertions(+), 65 deletions(-) diff --git a/server/command.go b/server/command.go index be5da95b..180ccae9 100644 --- a/server/command.go +++ b/server/command.go @@ -14,6 +14,7 @@ const helpText = "###### Mattermost Autolink Plugin Administration\n" + " is either the Name of a link, or its number in the `/autolink list` output. A partial Name can be specified, but some commands require it to be uniquely resolved.\n" + "* `/autolink list` - list all configured links.\n" + "* `/autolink list ` - list a specific link.\n" + + "* `/autolink test test-text...` - test a link on a sample.\n" + "* `/autolink enable ` - enable a link.\n" + "* `/autolink disable ` - disable a link.\n" + "* `/autolink add ` - add a new link, named .\n" + @@ -27,6 +28,7 @@ const helpText = "###### Mattermost Autolink Plugin Administration\n" + `/autolink set Visa Pattern (?P(?P4\d{3})[ -]?(?P\d{4})[ -]?(?P\d{4})[ -]?(?P[0-9]{4}))` + "\n" + "/autolink set Visa Template VISA XXXX-XXXX-XXXX-$LastFour\n" + "/autolink set Visa WordMatch true\n" + + "/autolink test Visa 4356-7891-2345-1111 -- (4111222233334444)\n" + "/autolink enable Visa\n" + "```\n" + "" @@ -47,6 +49,7 @@ var autolinkCommandHandler = CommandHandler{ "enable": executeEnable, "add": executeAdd, "set": executeSet, + "test": executeTest, }, defaultHandler: commandHelp, } @@ -66,6 +69,13 @@ func commandHelp(p *Plugin, c *plugin.Context, header *model.CommandArgs, args . } func (p *Plugin) ExecuteCommand(c *plugin.Context, commandArgs *model.CommandArgs) (*model.CommandResponse, *model.AppError) { + user, appErr := p.API.GetUser(commandArgs.UserId) + if appErr != nil { + return responsef("%v", appErr.Error()), nil + } + if !strings.Contains(user.Roles, "system_admin") { + return responsef("`/autolink` can only be executed by a system administrator."), nil + } args := strings.Fields(commandArgs.Command) if len(args) == 0 || args[0] != "/autolink" { return responsef(helpText), nil @@ -74,14 +84,6 @@ func (p *Plugin) ExecuteCommand(c *plugin.Context, commandArgs *model.CommandArg } func executeList(p *Plugin, c *plugin.Context, header *model.CommandArgs, args ...string) *model.CommandResponse { - authorized, err := authorizedSysAdmin(p, header.UserId) - if err != nil { - return responsef("%v", err) - } - if !authorized { - return responsef("`/autolink link list` can only be run by a system administrator.") - } - links, refs, err := parseLinkRef(p, false, args...) if err != nil { return responsef("%v", err) @@ -101,13 +103,6 @@ func executeList(p *Plugin, c *plugin.Context, header *model.CommandArgs, args . } func executeDelete(p *Plugin, c *plugin.Context, header *model.CommandArgs, args ...string) *model.CommandResponse { - authorized, err := authorizedSysAdmin(p, header.UserId) - if err != nil { - return responsef("%v", err) - } - if !authorized { - return responsef("`/autolink link delete` can only be run by a system administrator.") - } if len(args) != 1 { return responsef(helpText) } @@ -132,13 +127,6 @@ func executeDelete(p *Plugin, c *plugin.Context, header *model.CommandArgs, args } func executeSet(p *Plugin, c *plugin.Context, header *model.CommandArgs, args ...string) *model.CommandResponse { - authorized, err := authorizedSysAdmin(p, header.UserId) - if err != nil { - return responsef("%v", err) - } - if !authorized { - return responsef("`/autolink link delete` can only be run by a system administrator.") - } if len(args) < 3 { return responsef(helpText) } @@ -205,6 +193,36 @@ func executeSet(p *Plugin, c *plugin.Context, header *model.CommandArgs, args .. return executeList(p, c, header, ref) } +func executeTest(p *Plugin, c *plugin.Context, header *model.CommandArgs, args ...string) *model.CommandResponse { + if len(args) < 2 { + return responsef(helpText) + } + + links, refs, err := parseLinkRef(p, false, args...) + if err != nil { + return responsef("%v", err) + } + + restOfCommand := header.Command[10:] // "/autolink " + restOfCommand = restOfCommand[strings.Index(restOfCommand, args[0])+len(args[0]):] + orig := strings.TrimSpace(restOfCommand) + out := "" + + for _, ref := range refs { + l := links[ref] + l.Disabled = false + replaced := l.Replace(orig) + if replaced == orig { + out += fmt.Sprintf("- %s: _no change_\n", l.DisplayName()) + } else { + out += fmt.Sprintf("- %s: `%s`\n", l.DisplayName(), replaced) + orig = replaced + } + } + + return responsef(out) +} + func executeEnable(p *Plugin, c *plugin.Context, header *model.CommandArgs, args ...string) *model.CommandResponse { if len(args) != 1 { return responsef(helpText) @@ -220,14 +238,6 @@ func executeDisable(p *Plugin, c *plugin.Context, header *model.CommandArgs, arg } func executeEnableImpl(p *Plugin, c *plugin.Context, header *model.CommandArgs, ref string, enabled bool) *model.CommandResponse { - authorized, err := authorizedSysAdmin(p, header.UserId) - if err != nil { - return responsef("%v", err) - } - if !authorized { - return responsef("`/autolink link delete` can only be run by a system administrator.") - } - links, refs, err := parseLinkRef(p, true, ref) if err != nil { return responsef("%v", err) @@ -247,13 +257,6 @@ func executeEnableImpl(p *Plugin, c *plugin.Context, header *model.CommandArgs, } func executeAdd(p *Plugin, c *plugin.Context, header *model.CommandArgs, args ...string) *model.CommandResponse { - authorized, err := authorizedSysAdmin(p, header.UserId) - if err != nil { - return responsef("%v", err) - } - if !authorized { - return responsef("`/autolink link add` can only be run by a system administrator.") - } if len(args) > 1 { return responsef(helpText) } @@ -262,7 +265,7 @@ func executeAdd(p *Plugin, c *plugin.Context, header *model.CommandArgs, args .. name = args[0] } - err = saveConfigLinks(p, append(p.getConfig().Links, Link{ + err := saveConfigLinks(p, append(p.getConfig().Links, Link{ Name: name, })) if err != nil { @@ -275,17 +278,6 @@ func executeAdd(p *Plugin, c *plugin.Context, header *model.CommandArgs, args .. return executeList(p, c, header, name) } -func authorizedSysAdmin(p *Plugin, userId string) (bool, error) { - user, err := p.API.GetUser(userId) - if err != nil { - return false, err - } - if !strings.Contains(user.Roles, "system_admin") { - return false, nil - } - return true, nil -} - func responsef(format string, args ...interface{}) *model.CommandResponse { return &model.CommandResponse{ ResponseType: model.COMMAND_RESPONSE_TYPE_EPHEMERAL, diff --git a/server/config.go b/server/config.go index 24a2b507..a8292694 100755 --- a/server/config.go +++ b/server/config.go @@ -7,7 +7,6 @@ import ( "github.com/mattermost/mattermost-server/mlog" "github.com/mattermost/mattermost-server/model" - "github.com/pkg/errors" ) // Config from config.json @@ -35,20 +34,19 @@ func (p *Plugin) OnConfigurationChange() error { *conf = c }) - if c.EnableAdminCommand { - err := p.API.RegisterCommand(&model.Command{ - Trigger: "autolink", - DisplayName: "Autolink", - Description: "Autolink administration.", - AutoComplete: true, - AutoCompleteHint: "[command]", - }) - if err != nil { - return errors.WithMessage(err, "failed to register /autolink command") + go func() { + if c.EnableAdminCommand { + _ = p.API.RegisterCommand(&model.Command{ + Trigger: "autolink", + DisplayName: "Autolink", + Description: "Autolink administration.", + AutoComplete: true, + AutoCompleteHint: "[command]", + }) + } else { + _ = p.API.UnregisterCommand("", "autolink") } - } else { - _ = p.API.UnregisterCommand("", "autolink") - } + }() return nil } @@ -75,7 +73,8 @@ func (conf Config) ToConfig() map[string]interface{} { links = append(links, l.ToConfig()) } return map[string]interface{}{ - "Links": links, + "EnableAdminCommand": conf.EnableAdminCommand, + "Links": links, } } diff --git a/server/plugin_test.go b/server/plugin_test.go index 0860757d..3d272873 100644 --- a/server/plugin_test.go +++ b/server/plugin_test.go @@ -34,6 +34,7 @@ func TestPlugin(t *testing.T) { *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) @@ -71,7 +72,10 @@ func TestSpecialCases(t *testing.T) { Template: "test", Scope: []string{"other-team/town-square"}, }) - validConfig := Config{links} + validConfig := Config{ + EnableAdminCommand: false, + Links: links, + } testChannel := model.Channel{ Name: "TestChanel", @@ -87,6 +91,7 @@ func TestSpecialCases(t *testing.T) { *dest.(*Config) = validConfig 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) From 4e74314e0872e2bb1d609fae9590febcbabb23d6 Mon Sep 17 00:00:00 2001 From: Lev Brouk Date: Mon, 12 Aug 2019 18:11:53 -0700 Subject: [PATCH 7/8] /autolink help formatting, style --- server/command.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/server/command.go b/server/command.go index 180ccae9..6826c339 100644 --- a/server/command.go +++ b/server/command.go @@ -12,14 +12,14 @@ import ( const helpText = "###### Mattermost Autolink Plugin Administration\n" + " is either the Name of a link, or its number in the `/autolink list` output. A partial Name can be specified, but some commands require it to be uniquely resolved.\n" + - "* `/autolink list` - list all configured links.\n" + - "* `/autolink list ` - list a specific link.\n" + - "* `/autolink test test-text...` - test a link on a sample.\n" + - "* `/autolink enable ` - enable a link.\n" + - "* `/autolink disable ` - disable a link.\n" + "* `/autolink add ` - add a new link, named .\n" + "* `/autolink delete ` - delete a link.\n" + + "* `/autolink disable ` - disable a link.\n" + + "* `/autolink enable ` - enable a link.\n" + + "* `/autolink list ` - list a specific link.\n" + + "* `/autolink list` - list all configured links.\n" + "* `/autolink set value...` - sets a link's field to a value. The entire command line after is used for the value, unescaped, leading/trailing whitespace trimmed.\n" + + "* `/autolink test test-text...` - test a link on a sample.\n" + "\n" + "Example:\n" + "```\n" + @@ -28,7 +28,7 @@ const helpText = "###### Mattermost Autolink Plugin Administration\n" + `/autolink set Visa Pattern (?P(?P4\d{3})[ -]?(?P\d{4})[ -]?(?P\d{4})[ -]?(?P[0-9]{4}))` + "\n" + "/autolink set Visa Template VISA XXXX-XXXX-XXXX-$LastFour\n" + "/autolink set Visa WordMatch true\n" + - "/autolink test Visa 4356-7891-2345-1111 -- (4111222233334444)\n" + + "/autolink test Vi 4356-7891-2345-1111 -- (4111222233334444)\n" + "/autolink enable Visa\n" + "```\n" + "" @@ -42,7 +42,7 @@ type CommandHandler struct { var autolinkCommandHandler = CommandHandler{ handlers: map[string]CommandHandlerFunc{ - "help": commandHelp, + "help": executeHelp, "list": executeList, "delete": executeDelete, "disable": executeDisable, @@ -51,7 +51,7 @@ var autolinkCommandHandler = CommandHandler{ "set": executeSet, "test": executeTest, }, - defaultHandler: commandHelp, + defaultHandler: executeHelp, } func (ch CommandHandler) Handle(p *Plugin, c *plugin.Context, header *model.CommandArgs, args ...string) *model.CommandResponse { @@ -64,10 +64,6 @@ func (ch CommandHandler) Handle(p *Plugin, c *plugin.Context, header *model.Comm return ch.defaultHandler(p, c, header, args...) } -func commandHelp(p *Plugin, c *plugin.Context, header *model.CommandArgs, args ...string) *model.CommandResponse { - return responsef(helpText) -} - func (p *Plugin) ExecuteCommand(c *plugin.Context, commandArgs *model.CommandArgs) (*model.CommandResponse, *model.AppError) { user, appErr := p.API.GetUser(commandArgs.UserId) if appErr != nil { @@ -278,6 +274,10 @@ func executeAdd(p *Plugin, c *plugin.Context, header *model.CommandArgs, args .. return executeList(p, c, header, name) } +func executeHelp(p *Plugin, c *plugin.Context, header *model.CommandArgs, args ...string) *model.CommandResponse { + return responsef(helpText) +} + func responsef(format string, args ...interface{}) *model.CommandResponse { return &model.CommandResponse{ ResponseType: model.COMMAND_RESPONSE_TYPE_EPHEMERAL, From be37ac968fc45b613d71c7991250bbb6c6d8fbc8 Mon Sep 17 00:00:00 2001 From: Lev Brouk Date: Mon, 12 Aug 2019 18:14:44 -0700 Subject: [PATCH 8/8] PR feedback: renamed multipass to canReplaceAll, commented --- server/link.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/server/link.go b/server/link.go index 60723930..3fe73b97 100755 --- a/server/link.go +++ b/server/link.go @@ -16,9 +16,9 @@ type Link struct { DisableNonWordPrefix bool DisableNonWordSuffix bool - template string - re *regexp.Regexp - multipass bool + template string + re *regexp.Regexp + canReplaceAll bool } // DisplayName returns a display name for the link. @@ -35,25 +35,27 @@ func (l *Link) Compile() error { return nil } + // `\b` can be used with ReplaceAll since it does not consume characters, + // custom patterns can not and need to be processed one at a time. + canReplaceAll := false pattern := l.Pattern template := l.Template - multipass := false if !l.DisableNonWordPrefix { if l.WordMatch { pattern = `\b` + pattern + canReplaceAll = true } else { pattern = `(?P(^|\s))` + pattern template = `${MattermostNonWordPrefix}` + template - multipass = true } } if !l.DisableNonWordSuffix { if l.WordMatch { pattern = pattern + `\b` + canReplaceAll = true } else { pattern = pattern + `(?P$|[\s\.\!\?\,\)])` template = template + `${MattermostNonWordSuffix}` - multipass = true } } @@ -63,7 +65,7 @@ func (l *Link) Compile() error { } l.re = re l.template = template - l.multipass = multipass + l.canReplaceAll = canReplaceAll return nil } @@ -75,7 +77,7 @@ func (l Link) Replace(message string) string { } // Since they don't consume, `\b`s require no special handling, can just ReplaceAll - if !l.multipass { + if l.canReplaceAll { return l.re.ReplaceAllString(message, l.template) }