Repository: mattermost/mattermost-plugin-github
Pull request #349 (2020-08-10 11:04:43)

Add recovery middleware
#### Summary
Add recovery middleware to avoid crashing the whole plugin if one API call panics.

Credit for the code goes to @agnivade 


#### Ticket Link
Fixes https://github.com/mattermost/mattermost-plugin-github/issues/348

codecov-commenter: # [Codecov](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/349?src=pr&el=h1) Report
> Merging [#349](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/349?src=pr&el=desc) into [master](https://codecov.io/gh/mattermost/mattermost-plugin-github/commit/585a08e81ec265e2bf65bb2ab33cbbc69b0d06c5&el=desc) will **increase** coverage by `0.29%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/349/graphs/tree.svg?width=650&height=150&src=pr&token=0uHZTjTrgl)](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/349?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #349      +/-   ##
==========================================
+ Coverage   21.42%   21.72%   +0.29%     
==========================================
  Files          10       10              
  Lines        2614     2624      +10     
==========================================
+ Hits          560      570      +10     
  Misses       2017     2017              
  Partials       37       37              
```


| [Impacted Files](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/349?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [server/plugin/api.go](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/349/diff?src=pr&el=tree#diff-c2VydmVyL3BsdWdpbi9hcGkuZ28=) | `8.82% <100.00%> (+1.21%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/349?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/349?src=pr&el=footer). Last update [585a08e...f197a74](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/349?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/server/plugin/api.go b/server/plugin/api.go
index fe58b14..ddcce42 100644
--- a/server/plugin/api.go
+++ b/server/plugin/api.go
@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"runtime/debug"
 	"strconv"
 	"strings"
 	"sync"
@@ -98,6 +99,7 @@ func (p *Plugin) writeAPIError(w http.ResponseWriter, apiErr *APIErrorResponse)
 
 func (p *Plugin) initializeAPI() {
 	p.router = mux.NewRouter()
+	p.router.Use(p.withRecovery)
 
 	oauthRouter := p.router.PathPrefix("/oauth").Subrouter()
 	apiRouter := p.router.PathPrefix("/api/v1").Subrouter()
@@ -128,6 +130,21 @@ func (p *Plugin) initializeAPI() {
 	apiRouter.HandleFunc("/token", checkPluginRequest(p.getToken)).Methods(http.MethodGet)
 }
 
+func (p *Plugin) withRecovery(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		defer func() {
+			if x := recover(); x != nil {
+				p.API.LogError("Recovered from a panic",
+					"url", r.URL.String(),
+					"error", x,
+					"stack", string(debug.Stack()))
+			}
+		}()
+
+		next.ServeHTTP(w, r)
+	})
+}
+
 func (p *Plugin) extractUserMiddleWare(handler HTTPHandlerFuncWithUser, responseType ResponseType) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		userID := r.Header.Get("Mattermost-User-ID")
@@ -520,9 +537,17 @@ func (p *Plugin) getUnreads(w http.ResponseWriter, r *http.Request, userID strin
 			continue
 		}
 
+		issueURL := n.GetSubject().GetURL()
+		issueNumIndex := strings.LastIndex(issueURL, "/")
+		issueNum := issueURL[issueNumIndex+1:]
+		subjectURL := n.GetSubject().GetURL()
+		if n.GetSubject().GetLatestCommentURL() != "" {
+			subjectURL = n.GetSubject().GetLatestCommentURL()
+		}
+
 		filteredNotifications = append(filteredNotifications, &filteredNotification{
 			Notification: *n,
-			HTMLUrl:      fixGithubNotificationSubjectURL(n.GetSubject().GetURL()),
+			HTMLUrl:      fixGithubNotificationSubjectURL(subjectURL, issueNum),
 		})
 	}
 
diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go
index 85c6ab1..1b5c078 100644
--- a/server/plugin/plugin.go
+++ b/server/plugin/plugin.go
@@ -422,11 +422,19 @@ func (p *Plugin) GetToDo(ctx context.Context, username string, githubClient *git
 		notificationType := notificationSubject.GetType()
 		switch notificationType {
 		case "RepositoryVulnerabilityAlert":
-			message := fmt.Sprintf("[Vulnerability Alert for %v](%v)", n.GetRepository().GetFullName(), fixGithubNotificationSubjectURL(n.GetSubject().GetURL()))
+			message := fmt.Sprintf("[Vulnerability Alert for %v](%v)", n.GetRepository().GetFullName(), fixGithubNotificationSubjectURL(n.GetSubject().GetURL(), ""))
 			notificationContent += fmt.Sprintf("* %v\n", message)
 		default:
+			issueURL := n.GetSubject().GetURL()
+			issueNumIndex := strings.LastIndex(issueURL, "/")
+			issueNum := issueURL[issueNumIndex+1:]
+			subjectURL := n.GetSubject().GetURL()
+			if n.GetSubject().GetLatestCommentURL() != "" {
+				subjectURL = n.GetSubject().GetLatestCommentURL()
+			}
+
 			notificationTitle := notificationSubject.GetTitle()
-			notificationURL := fixGithubNotificationSubjectURL(notificationSubject.GetURL())
+			notificationURL := fixGithubNotificationSubjectURL(subjectURL, issueNum)
 			notificationContent += getToDoDisplayText(baseURL, notificationTitle, notificationURL, notificationType)
 		}
 
diff --git a/server/plugin/template.go b/server/plugin/template.go
index 0cef875..50cdb34 100644
--- a/server/plugin/template.go
+++ b/server/plugin/template.go
@@ -332,7 +332,8 @@ Assignees: {{range $i, $el := .Assignees -}} {{- if $i}}, {{end}}{{template "use
 		"    * `deletes` - includes branch and tag deletions\n" +
 		"    * `issue_comments` - includes new issue comments\n" +
 		"    * `pull_reviews` - includes pull request reviews\n" +
-		"    * `label:<labelname>` - must include `pulls` or `issues` in feature list when using a label. Defaults to `pulls,issues,creates,deletes`\n" +
+		"    * `label:<labelname>` - limit pull request and issue events to only this label. Must include `pulls` or `issues` in feature list when using a label.\n" +
+		"    * Defaults to `pulls,issues,creates,deletes`\n" +
 		"  * `flags` currently supported:\n" +
 		"    * `--exclude-org-member` - events triggered by organization members will not be delivered (the GitHub organization config should be set, otherwise this flag has not effect)\n" +
 		"* `/github unsubscribe owner/repo` - Unsubscribe the current channel from a repository\n" +
diff --git a/server/plugin/utils.go b/server/plugin/utils.go
index ca40843..5ed0b47 100644
--- a/server/plugin/utils.go
+++ b/server/plugin/utils.go
@@ -160,12 +160,13 @@ func parseGitHubUsernamesFromText(text string) []string {
 	return usernames
 }
 
-func fixGithubNotificationSubjectURL(url string) string {
+func fixGithubNotificationSubjectURL(url, issueNum string) string {
 	url = strings.Replace(url, "api.", "", 1)
 	url = strings.Replace(url, "repos/", "", 1)
 	url = strings.Replace(url, "/pulls/", "/pull/", 1)
 	url = strings.Replace(url, "/commits/", "/commit/", 1)
 	url = strings.Replace(url, "/api/v3", "", 1)
+	url = strings.Replace(url, "comments/", issueNum+"#issuecomment-", 1)
 	return url
 }
 


Test patch:
diff --git a/server/plugin/api_test.go b/server/plugin/api_test.go
index b15a1a9..82910de 100644
--- a/server/plugin/api_test.go
+++ b/server/plugin/api_test.go
@@ -1,16 +1,58 @@
 package plugin
 
 import (
+	"io"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"testing"
 
 	"github.com/mattermost/mattermost-server/v5/plugin"
 	"github.com/mattermost/mattermost-server/v5/plugin/plugintest"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 
 	"github.com/mattermost/mattermost-plugin-github/server/testutils"
 )
 
+type panicHandler struct {
+}
+
+func (ph panicHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	panic("bad handler")
+}
+
+func TestWithRecovery(t *testing.T) {
+	defer func() {
+		if x := recover(); x != nil {
+			require.Fail(t, "got panic")
+		}
+	}()
+
+	p := NewPlugin()
+	api := &plugintest.API{}
+	api.On("LogError",
+		"Recovered from a panic",
+		"url", "http://random",
+		"error", "bad handler",
+		"stack", mock.Anything)
+	p.SetAPI(api)
+
+	ph := panicHandler{}
+	handler := p.withRecovery(ph)
+
+	req := httptest.NewRequest(http.MethodGet, "http://random", nil)
+	w := httptest.NewRecorder()
+	handler.ServeHTTP(w, req)
+
+	resp := w.Result()
+	if resp.Body != nil {
+		defer resp.Body.Close()
+		_, err := io.Copy(ioutil.Discard, resp.Body)
+		require.NoError(t, err)
+	}
+}
+
 func TestPlugin_ServeHTTP(t *testing.T) {
 	httpTestJSON := testutils.HTTPTest{
 		T:       t,
diff --git a/server/plugin/utils_test.go b/server/plugin/utils_test.go
index b6bd976..46ca183 100644
--- a/server/plugin/utils_test.go
+++ b/server/plugin/utils_test.go
@@ -39,16 +39,18 @@ func TestFixGithubNotificationSubjectURL(t *testing.T) {
 	tcs := []struct {
 		Text     string
 		Expected string
+		IssueNum string
 	}{
 		{Text: "https://api.github.com/repos/jwilander/mattermost-webapp/issues/123", Expected: "https://github.com/jwilander/mattermost-webapp/issues/123"},
 		{Text: "https://api.github.com/repos/jwilander/mattermost-webapp/pulls/123", Expected: "https://github.com/jwilander/mattermost-webapp/pull/123"},
 		{Text: "https://enterprise.github.com/api/v3/jwilander/mattermost-webapp/issues/123", Expected: "https://enterprise.github.com/jwilander/mattermost-webapp/issues/123"},
 		{Text: "https://enterprise.github.com/api/v3/jwilander/mattermost-webapp/pull/123", Expected: "https://enterprise.github.com/jwilander/mattermost-webapp/pull/123"},
 		{Text: "https://api.github.com/repos/mattermost/mattermost-server/commits/cc6c385d3e8903546fc6fc856bf468ad09b70913", Expected: "https://github.com/mattermost/mattermost-server/commit/cc6c385d3e8903546fc6fc856bf468ad09b70913"},
+		{Text: "https://api.github.com/repos/user/rate_my_cakes/issues/comments/655139214", Expected: "https://github.com/user/rate_my_cakes/issues/4#issuecomment-655139214", IssueNum: "4"},
 	}
 
 	for _, tc := range tcs {
-		assert.Equal(t, tc.Expected, fixGithubNotificationSubjectURL(tc.Text))
+		assert.Equal(t, tc.Expected, fixGithubNotificationSubjectURL(tc.Text, tc.IssueNum))
 	}
 }