make hooks optional, short circuit if unimplemented (#7263)

5 files changed, 156 insertions, 49 deletions

diff --git a/plugin/rpcplugin/hooks.go b/plugin/rpcplugin/hooks.go
index 008730402..5b97742aa 100644
--- a/plugin/rpcplugin/hooks.go
+++ b/plugin/rpcplugin/hooks.go
@@ -3,41 +3,87 @@ package rpcplugin
 import (
 	"io"
 	"net/rpc"
+	"reflect"
 
 	"github.com/mattermost/platform/plugin"
 )
 
 type LocalHooks struct {
-	hooks     plugin.Hooks
+	hooks     interface{}
 	muxer     *Muxer
 	remoteAPI *RemoteAPI
 }
 
+// Implemented replies with the names of the hooks that are implemented.
+func (h *LocalHooks) Implemented(args struct{}, reply *[]string) error {
+	ifaceType := reflect.TypeOf((*plugin.Hooks)(nil)).Elem()
+	implType := reflect.TypeOf(h.hooks)
+	selfType := reflect.TypeOf(h)
+	var methods []string
+	for i := 0; i < ifaceType.NumMethod(); i++ {
+		method := ifaceType.Method(i)
+		if m, ok := implType.MethodByName(method.Name); !ok {
+			continue
+		} else if m.Type.NumIn() != method.Type.NumIn()+1 {
+			continue
+		} else if m.Type.NumOut() != method.Type.NumOut() {
+			continue
+		} else {
+			match := true
+			for j := 0; j < method.Type.NumIn(); j++ {
+				if m.Type.In(j+1) != method.Type.In(j) {
+					match = false
+					break
+				}
+			}
+			for j := 0; j < method.Type.NumOut(); j++ {
+				if m.Type.Out(j) != method.Type.Out(j) {
+					match = false
+					break
+				}
+			}
+			if !match {
+				continue
+			}
+		}
+		if _, ok := selfType.MethodByName(method.Name); !ok {
+			continue
+		}
+		methods = append(methods, method.Name)
+	}
+	*reply = methods
+	return nil
+}
+
 func (h *LocalHooks) OnActivate(args int64, reply *struct{}) error {
-	stream := h.muxer.Connect(args)
 	if h.remoteAPI != nil {
 		h.remoteAPI.Close()
+		h.remoteAPI = nil
+	}
+	if hook, ok := h.hooks.(interface {
+		OnActivate(plugin.API) error
+	}); ok {
+		stream := h.muxer.Connect(args)
+		h.remoteAPI = ConnectAPI(stream, h.muxer)
+		return hook.OnActivate(h.remoteAPI)
 	}
-	h.remoteAPI = ConnectAPI(stream, h.muxer)
-	return h.hooks.OnActivate(h.remoteAPI)
+	return nil
 }
 
-func (h *LocalHooks) OnDeactivate(args, reply *struct{}) error {
-	err := h.hooks.OnDeactivate()
+func (h *LocalHooks) OnDeactivate(args, reply *struct{}) (err error) {
+	if hook, ok := h.hooks.(interface {
+		OnDeactivate() error
+	}); ok {
+		err = hook.OnDeactivate()
+	}
 	if h.remoteAPI != nil {
 		h.remoteAPI.Close()
 		h.remoteAPI = nil
 	}
-	return err
+	return
 }
 
-type RemoteHooks struct {
-	client    *rpc.Client
-	muxer     *Muxer
-	apiCloser io.Closer
-}
-
-func ServeHooks(hooks plugin.Hooks, conn io.ReadWriteCloser, muxer *Muxer) {
+func ServeHooks(hooks interface{}, conn io.ReadWriteCloser, muxer *Muxer) {
 	server := rpc.NewServer()
 	server.Register(&LocalHooks{
 		hooks: hooks,
@@ -46,32 +92,72 @@ func ServeHooks(hooks plugin.Hooks, conn io.ReadWriteCloser, muxer *Muxer) {
 	server.ServeConn(conn)
 }
 
+const (
+	remoteOnActivate = iota
+	remoteOnDeactivate
+	maxRemoteHookCount
+)
+
+type RemoteHooks struct {
+	client      *rpc.Client
+	muxer       *Muxer
+	apiCloser   io.Closer
+	implemented [maxRemoteHookCount]bool
+}
+
 var _ plugin.Hooks = (*RemoteHooks)(nil)
 
+func (h *RemoteHooks) Implemented() (impl []string, err error) {
+	err = h.client.Call("LocalHooks.Implemented", struct{}{}, &impl)
+	return
+}
+
 func (h *RemoteHooks) OnActivate(api plugin.API) error {
-	id, stream := h.muxer.Serve()
 	if h.apiCloser != nil {
 		h.apiCloser.Close()
+		h.apiCloser = nil
 	}
+	if !h.implemented[remoteOnActivate] {
+		return nil
+	}
+	id, stream := h.muxer.Serve()
 	h.apiCloser = stream
 	go ServeAPI(api, stream, h.muxer)
 	return h.client.Call("LocalHooks.OnActivate", id, nil)
 }
 
 func (h *RemoteHooks) OnDeactivate() error {
+	if !h.implemented[remoteOnDeactivate] {
+		return nil
+	}
 	return h.client.Call("LocalHooks.OnDeactivate", struct{}{}, nil)
 }
 
 func (h *RemoteHooks) Close() error {
 	if h.apiCloser != nil {
 		h.apiCloser.Close()
+		h.apiCloser = nil
 	}
 	return h.client.Close()
 }
 
-func ConnectHooks(conn io.ReadWriteCloser, muxer *Muxer) *RemoteHooks {
-	return &RemoteHooks{
+func ConnectHooks(conn io.ReadWriteCloser, muxer *Muxer) (*RemoteHooks, error) {
+	remote := &RemoteHooks{
 		client: rpc.NewClient(conn),
 		muxer:  muxer,
 	}
+	implemented, err := remote.Implemented()
+	if err != nil {
+		remote.Close()
+		return nil, err
+	}
+	for _, method := range implemented {
+		switch method {
+		case "OnActivate":
+			remote.implemented[remoteOnActivate] = true
+		case "OnDeactivate":
+			remote.implemented[remoteOnDeactivate] = true
+		}
+	}
+	return remote, nil
 }
diff --git a/plugin/rpcplugin/hooks_test.go b/plugin/rpcplugin/hooks_test.go
index fbbbbedeb..6cd7ff547 100644
--- a/plugin/rpcplugin/hooks_test.go
+++ b/plugin/rpcplugin/hooks_test.go
@@ -11,7 +11,7 @@ import (
 	"github.com/mattermost/platform/plugin/plugintest"
 )
 
-func testHooksRPC(hooks plugin.Hooks, f func(plugin.Hooks)) {
+func testHooksRPC(hooks interface{}, f func(*RemoteHooks)) error {
 	r1, w1 := io.Pipe()
 	r2, w2 := io.Pipe()
 
@@ -24,10 +24,14 @@ func testHooksRPC(hooks plugin.Hooks, f func(plugin.Hooks)) {
 	id, server := c1.Serve()
 	go ServeHooks(hooks, server, c1)
 
-	remote := ConnectHooks(c2.Connect(id), c2)
+	remote, err := ConnectHooks(c2.Connect(id), c2)
+	if err != nil {
+		return err
+	}
 	defer remote.Close()
 
 	f(remote)
+	return nil
 }
 
 func TestHooks(t *testing.T) {
@@ -35,24 +39,65 @@ func TestHooks(t *testing.T) {
 	var hooks plugintest.Hooks
 	defer hooks.AssertExpectations(t)
 
-	testHooksRPC(&hooks, func(remote plugin.Hooks) {
+	assert.NoError(t, testHooksRPC(&hooks, func(remote *RemoteHooks) {
 		hooks.On("OnActivate", mock.AnythingOfType("*rpcplugin.RemoteAPI")).Return(nil)
 		assert.NoError(t, remote.OnActivate(&api))
 
 		hooks.On("OnDeactivate").Return(nil)
 		assert.NoError(t, remote.OnDeactivate())
-	})
+	}))
+}
+
+type testHooks struct {
+	mock.Mock
+}
+
+func (h *testHooks) OnActivate(api plugin.API) error {
+	return h.Called(api).Error(0)
+}
+
+func TestHooks_PartiallyImplemented(t *testing.T) {
+	var api plugintest.API
+	var hooks testHooks
+	defer hooks.AssertExpectations(t)
+
+	assert.NoError(t, testHooksRPC(&hooks, func(remote *RemoteHooks) {
+		implemented, err := remote.Implemented()
+		assert.NoError(t, err)
+		assert.Equal(t, []string{"OnActivate"}, implemented)
+
+		hooks.On("OnActivate", mock.AnythingOfType("*rpcplugin.RemoteAPI")).Return(nil)
+		assert.NoError(t, remote.OnActivate(&api))
+
+		assert.NoError(t, remote.OnDeactivate())
+	}))
 }
 
 func BenchmarkOnDeactivate(b *testing.B) {
 	var hooks plugintest.Hooks
 	hooks.On("OnDeactivate").Return(nil)
 
-	testHooksRPC(&hooks, func(remote plugin.Hooks) {
+	if err := testHooksRPC(&hooks, func(remote *RemoteHooks) {
+		b.ResetTimer()
+		for n := 0; n < b.N; n++ {
+			remote.OnDeactivate()
+		}
+		b.StopTimer()
+	}); err != nil {
+		b.Fatal(err.Error())
+	}
+}
+
+func BenchmarkOnDeactivate_Unimplemented(b *testing.B) {
+	var hooks testHooks
+
+	if err := testHooksRPC(&hooks, func(remote *RemoteHooks) {
 		b.ResetTimer()
 		for n := 0; n < b.N; n++ {
 			remote.OnDeactivate()
 		}
 		b.StopTimer()
-	})
+	}); err != nil {
+		b.Fatal(err.Error())
+	}
 }
diff --git a/plugin/rpcplugin/main.go b/plugin/rpcplugin/main.go
index 36177954b..241d70bc9 100644
--- a/plugin/rpcplugin/main.go
+++ b/plugin/rpcplugin/main.go
@@ -6,12 +6,10 @@ import (
 	"fmt"
 	"log"
 	"os"
-
-	"github.com/mattermost/platform/plugin"
 )
 
 // Makes a set of hooks available via RPC. This function never returns.
-func Main(hooks plugin.Hooks) {
+func Main(hooks interface{}) {
 	ipc, err := InheritedProcessIPC()
 	if err != nil {
 		log.Fatal(err.Error())
@@ -42,5 +40,5 @@ func ConnectMain(muxer *Muxer) (*RemoteHooks, error) {
 		return nil, err
 	}
 
-	return ConnectHooks(muxer.Connect(id), muxer), nil
+	return ConnectHooks(muxer.Connect(id), muxer)
 }
diff --git a/plugin/rpcplugin/main_test.go b/plugin/rpcplugin/main_test.go
index a796516e7..cc388787f 100644
--- a/plugin/rpcplugin/main_test.go
+++ b/plugin/rpcplugin/main_test.go
@@ -23,20 +23,11 @@ func TestMain(t *testing.T) {
 		package main
 
 		import (
-			"github.com/mattermost/platform/plugin"
 			"github.com/mattermost/platform/plugin/rpcplugin"
 		)
 
 		type MyPlugin struct {}
 
-		func (p *MyPlugin) OnActivate(api plugin.API) error {
-			return nil
-		}
-
-		func (p *MyPlugin) OnDeactivate() error {
-			return nil
-		}
-
 		func main() {
 			rpcplugin.Main(&MyPlugin{})
 		}
diff --git a/plugin/rpcplugin/supervisor_test.go b/plugin/rpcplugin/supervisor_test.go
index 438ebe02a..c43fd3dc9 100644
--- a/plugin/rpcplugin/supervisor_test.go
+++ b/plugin/rpcplugin/supervisor_test.go
@@ -23,20 +23,11 @@ func TestSupervisor(t *testing.T) {
 		package main
 
 		import (
-			"github.com/mattermost/platform/plugin"
 			"github.com/mattermost/platform/plugin/rpcplugin"
 		)
 
 		type MyPlugin struct {}
 
-		func (p *MyPlugin) OnActivate(api plugin.API) error {
-			return nil
-		}
-
-		func (p *MyPlugin) OnDeactivate() error {
-			return nil
-		}
-
 		func main() {
 			rpcplugin.Main(&MyPlugin{})
 		}
@@ -100,10 +91,6 @@ func TestSupervisor_PluginCrash(t *testing.T) {
 			return nil
 		}
 
-		func (p *MyPlugin) OnDeactivate() error {
-			return nil
-		}
-
 		func main() {
 			rpcplugin.Main(&MyPlugin{})
 		}