Commit f2ddef91 authored by Jesse Hallam's avatar Jesse Hallam Committed by Christopher Speller

MM-11734: better plugin `error` handling (#9405)

* MM-11734: encode unregistered error implementations as an ErrorString

* MM-11734: test error string handling

* more idiomatic error handling
parent 8b17bf9e
......@@ -13,6 +13,8 @@ import (
"testing"
"time"
"github.com/pkg/errors"
"github.com/mattermost/mattermost-server/model"
"github.com/mattermost/mattermost-server/plugin"
"github.com/mattermost/mattermost-server/plugin/plugintest"
......@@ -33,7 +35,7 @@ func compileGo(t *testing.T, sourceCode, outputPath string) {
require.NoError(t, cmd.Run())
}
func SetAppEnvironmentWithPlugins(t *testing.T, pluginCode []string, app *App, apiFunc func(*model.Manifest) plugin.API) {
func SetAppEnvironmentWithPlugins(t *testing.T, pluginCode []string, app *App, apiFunc func(*model.Manifest) plugin.API) []error {
pluginDir, err := ioutil.TempDir("", "")
require.NoError(t, err)
webappPluginDir, err := ioutil.TempDir("", "")
......@@ -45,14 +47,18 @@ func SetAppEnvironmentWithPlugins(t *testing.T, pluginCode []string, app *App, a
require.NoError(t, err)
app.Plugins = env
activationErrors := []error{}
for _, code := range pluginCode {
pluginId := model.NewId()
backend := filepath.Join(pluginDir, pluginId, "backend.exe")
compileGo(t, code, backend)
ioutil.WriteFile(filepath.Join(pluginDir, pluginId, "plugin.json"), []byte(`{"id": "`+pluginId+`", "backend": {"executable": "backend.exe"}}`), 0600)
env.Activate(pluginId)
_, _, activationErr := env.Activate(pluginId)
activationErrors = append(activationErrors, activationErr)
}
return activationErrors
}
func TestHookMessageWillBePosted(t *testing.T) {
......@@ -791,3 +797,73 @@ func TestUserHasLoggedIn(t *testing.T) {
t.Errorf("Expected firstname overwrite, got default")
}
}
func TestErrorString(t *testing.T) {
th := Setup().InitBasic()
defer th.TearDown()
t.Run("errors.New", func(t *testing.T) {
activationErrors := SetAppEnvironmentWithPlugins(t,
[]string{
`
package main
import (
"github.com/pkg/errors"
"github.com/mattermost/mattermost-server/plugin"
)
type MyPlugin struct {
plugin.MattermostPlugin
}
func (p *MyPlugin) OnActivate() error {
return errors.New("simulate failure")
}
func main() {
plugin.ClientMain(&MyPlugin{})
}
`}, th.App, th.App.NewPluginAPI)
require.Len(t, activationErrors, 1)
require.NotNil(t, activationErrors[0])
require.Contains(t, activationErrors[0].Error(), "simulate failure")
})
t.Run("AppError", func(t *testing.T) {
activationErrors := SetAppEnvironmentWithPlugins(t,
[]string{
`
package main
import (
"github.com/mattermost/mattermost-server/plugin"
"github.com/mattermost/mattermost-server/model"
)
type MyPlugin struct {
plugin.MattermostPlugin
}
func (p *MyPlugin) OnActivate() error {
return model.NewAppError("where", "id", map[string]interface{}{"param": 1}, "details", 42)
}
func main() {
plugin.ClientMain(&MyPlugin{})
}
`}, th.App, th.App.NewPluginAPI)
require.Len(t, activationErrors, 1)
require.NotNil(t, activationErrors[0])
cause := errors.Cause(activationErrors[0])
require.IsType(t, &model.AppError{}, cause)
// params not expected, since not exported
expectedErr := model.NewAppError("where", "id", nil, "details", 42)
require.Equal(t, expectedErr, cause)
})
}
......@@ -62,12 +62,39 @@ type apiRPCServer struct {
impl API
}
// ErrorString is a fallback for sending unregistered implementations of the error interface across
// rpc. For example, the errorString type from the github.com/pkg/errors package cannot be
// registered since it is not exported, but this precludes common error handling paradigms.
// ErrorString merely preserves the string description of the error, while satisfying the error
// interface itself to allow other registered types (such as model.AppError) to be sent unmodified.
type ErrorString struct {
Err string
}
func (e ErrorString) Error() string {
return e.Err
}
func encodableError(err error) error {
if err == nil {
return nil
}
if _, ok := err.(*model.AppError); ok {
return err
}
return &ErrorString{
Err: err.Error(),
}
}
// Registering some types used by MM for encoding/gob used by rpc
func init() {
gob.Register([]*model.SlackAttachment{})
gob.Register([]interface{}{})
gob.Register(map[string]interface{}{})
gob.Register(&model.AppError{})
gob.Register(&ErrorString{})
}
// These enforce compile time checks to make sure types implement the interface
......@@ -128,7 +155,7 @@ func (s *hooksRPCServer) Implemented(args struct{}, reply *[]string) error {
methods = append(methods, method.Name)
}
*reply = methods
return nil
return encodableError(nil)
}
type Z_OnActivateArgs struct {
......@@ -182,7 +209,7 @@ func (s *hooksRPCServer) OnActivate(args *Z_OnActivateArgs, returns *Z_OnActivat
if hook, ok := s.impl.(interface {
OnActivate() error
}); ok {
returns.A = hook.OnActivate()
returns.A = encodableError(hook.OnActivate())
}
return nil
}
......
This diff is collapsed.
......@@ -69,6 +69,43 @@ func FieldListToNames(fieldList *ast.FieldList, fileset *token.FileSet) string {
return strings.Join(result, ", ")
}
func FieldListToEncodedErrors(structPrefix string, fieldList *ast.FieldList, fileset *token.FileSet) string {
result := []string{}
if fieldList == nil {
return ""
}
nextLetter := 'A'
for _, field := range fieldList.List {
typeNameBuffer := &bytes.Buffer{}
err := printer.Fprint(typeNameBuffer, fileset, field.Type)
if err != nil {
panic(err)
}
if typeNameBuffer.String() != "error" {
nextLetter += 1
continue
}
name := ""
if len(field.Names) == 0 {
name = string(nextLetter)
nextLetter += 1
} else {
for range field.Names {
name += string(nextLetter)
nextLetter += 1
}
}
result = append(result, structPrefix+name+" = encodableError("+structPrefix+name+")")
}
return strings.Join(result, "\n")
}
func FieldListDestruct(structPrefix string, fieldList *ast.FieldList, fileset *token.FileSet) string {
result := []string{}
if fieldList == nil || len(fieldList.List) == 0 {
......@@ -237,8 +274,9 @@ func (s *hooksRPCServer) {{.Name}}(args *{{.Name | obscure}}Args, returns *{{.Na
{{.Name}}{{funcStyle .Params}} {{funcStyle .Return}}
}); ok {
{{if .Return}}{{destruct "returns." .Return}} = {{end}}hook.{{.Name}}({{destruct "args." .Params}})
{{if .Return}}{{encodeErrors "returns." .Return}}{{end}}
} else {
return fmt.Errorf("Hook {{.Name}} called but not implemented.")
return encodableError(fmt.Errorf("Hook {{.Name}} called but not implemented."))
}
return nil
}
......@@ -269,7 +307,7 @@ func (s *apiRPCServer) {{.Name}}(args *{{.Name | obscure}}Args, returns *{{.Name
}); ok {
{{if .Return}}{{destruct "returns." .Return}} = {{end}}hook.{{.Name}}({{destruct "args." .Params}})
} else {
return fmt.Errorf("API {{.Name}} called but not implemented.")
return encodableError(fmt.Errorf("API {{.Name}} called but not implemented."))
}
return nil
}
......@@ -292,6 +330,9 @@ func generateGlue(info *PluginInterfaceInfo) {
"funcStyle": func(fields *ast.FieldList) string { return FieldListToFuncList(fields, info.FileSet) },
"structStyle": func(fields *ast.FieldList) string { return FieldListToStructList(fields, info.FileSet) },
"valuesOnly": func(fields *ast.FieldList) string { return FieldListToNames(fields, info.FileSet) },
"encodeErrors": func(structPrefix string, fields *ast.FieldList) string {
return FieldListToEncodedErrors(structPrefix, fields, info.FileSet)
},
"destruct": func(structPrefix string, fields *ast.FieldList) string {
return FieldListDestruct(structPrefix, fields, info.FileSet)
},
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment