MM-12193: remove auto unmarshalling (#9519)

* MM-12193: remove auto configuration unmarshalling

Since plugin hook events are called concurrently, there's no way for the plugin framework to coordinate safe access to the automatically unmarshalled configuration fields. Remove this functionality, and update documentation to illustrate a safe way to do this.

* better Fprint example

* fix unit tests

* log when OnConfigurationChange fails through OnActivate

* clarify lifecycle when OnConfigurationChange returns an error

* call SetAPI even if OnConfigurationChange not implemented

5 files changed, 73 insertions, 45 deletions

diff --git a/plugin/client.go b/plugin/client.go
index 457a16cf4..63cedfbce 100644
--- a/plugin/client.go
+++ b/plugin/client.go
@@ -13,12 +13,10 @@ import (
 func ClientMain(pluginImplementation interface{}) {
 	if impl, ok := pluginImplementation.(interface {
 		SetAPI(api API)
-		SetSelfRef(ref interface{})
 	}); !ok {
 		panic("Plugin implementation given must embed plugin.MattermostPlugin")
 	} else {
 		impl.SetAPI(nil)
-		impl.SetSelfRef(pluginImplementation)
 	}
 
 	pluginMap := map[string]plugin.Plugin{
@@ -34,8 +32,6 @@ func ClientMain(pluginImplementation interface{}) {
 type MattermostPlugin struct {
 	// API exposes the plugin api, and becomes available just prior to the OnActive hook.
 	API API
-
-	selfRef interface{} // This is so we can unmarshal into our parent
 }
 
 // SetAPI persists the given API interface to the plugin. It is invoked just prior to the
@@ -43,21 +39,3 @@ type MattermostPlugin struct {
 func (p *MattermostPlugin) SetAPI(api API) {
 	p.API = api
 }
-
-// SetSelfRef is called by ClientMain to maintain a pointer to the plugin interface originally
-// registered. This allows for the default implementation of OnConfigurationChange.
-func (p *MattermostPlugin) SetSelfRef(ref interface{}) {
-	p.selfRef = ref
-}
-
-// OnConfigurationChange provides a default implementation of this hook event that unmarshals the
-// plugin configuration directly onto the plugin struct.
-//
-// Feel free to implement your own version of OnConfigurationChange if you need more advanced
-// configuration handling.
-func (p *MattermostPlugin) OnConfigurationChange() error {
-	if p.selfRef != nil {
-		return p.API.LoadPluginConfiguration(p.selfRef)
-	}
-	return nil
-}
diff --git a/plugin/client_rpc.go b/plugin/client_rpc.go
index 72bd41f68..2e85466d7 100644
--- a/plugin/client_rpc.go
+++ b/plugin/client_rpc.go
@@ -195,11 +195,16 @@ func (s *hooksRPCServer) OnActivate(args *Z_OnActivateArgs, returns *Z_OnActivat
 
 	if mmplugin, ok := s.impl.(interface {
 		SetAPI(api API)
-		OnConfigurationChange() error
-	}); !ok {
-	} else {
+	}); ok {
 		mmplugin.SetAPI(s.apiRPCClient)
-		mmplugin.OnConfigurationChange()
+	}
+
+	if mmplugin, ok := s.impl.(interface {
+		OnConfigurationChange() error
+	}); ok {
+		if err := mmplugin.OnConfigurationChange(); err != nil {
+			fmt.Fprintf(os.Stderr, "[ERROR] call to OnConfigurationChange failed, error: %v", err.Error())
+		}
 	}
 
 	// Capture output of standard logger because go-plugin
diff --git a/plugin/example_hello_world_test.go b/plugin/example_hello_world_test.go
index 0da171eb7..26f25c91a 100644
--- a/plugin/example_hello_world_test.go
+++ b/plugin/example_hello_world_test.go
@@ -12,7 +12,7 @@ type HelloWorldPlugin struct {
 }
 
 func (p *HelloWorldPlugin) ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Request) {
-	fmt.Fprintf(w, "Hello, world!")
+	fmt.Fprint(w, "Hello, world!")
 }
 
 // This example demonstrates a plugin that handles HTTP requests which respond by greeting the
diff --git a/plugin/example_help_test.go b/plugin/example_help_test.go
index 175b6588e..e4aa9dec3 100644
--- a/plugin/example_help_test.go
+++ b/plugin/example_help_test.go
@@ -2,48 +2,91 @@ package plugin_test
 
 import (
 	"strings"
+	"sync"
+
+	"github.com/pkg/errors"
 
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/plugin"
 )
 
-type HelpPlugin struct {
-	plugin.MattermostPlugin
-
+// configuration represents the configuration for this plugin as exposed via the Mattermost
+// server configuration.
+type configuration struct {
 	TeamName    string
 	ChannelName string
 
+	// channelId is resolved when the public configuration fields above change
 	channelId string
 }
 
+type HelpPlugin struct {
+	plugin.MattermostPlugin
+
+	// configurationLock synchronizes access to the configuration.
+	configurationLock sync.RWMutex
+
+	// configuration is the active plugin configuration. Consult getConfiguration and
+	// setConfiguration for usage.
+	configuration *configuration
+}
+
+// getConfiguration retrieves the active configuration under lock, making it safe to use
+// concurrently. The active configuration may change underneath the client of this method, but
+// the struct returned by this API call is considered immutable.
+func (p *HelpPlugin) getConfiguration() *configuration {
+	p.configurationLock.RLock()
+	defer p.configurationLock.RUnlock()
+
+	if p.configuration == nil {
+		return &configuration{}
+	}
+
+	return p.configuration
+}
+
+// setConfiguration replaces the active configuration under lock.
+//
+// Do not call setConfiguration while holding the configurationLock, as sync.Mutex is not
+// reentrant.
+func (p *HelpPlugin) setConfiguration(configuration *configuration) {
+	// Replace the active configuration under lock.
+	p.configurationLock.Lock()
+	defer p.configurationLock.Unlock()
+	p.configuration = configuration
+}
+
+// OnConfigurationChange updates the active configuration for this plugin under lock.
 func (p *HelpPlugin) OnConfigurationChange() error {
-	// Reuse the default implementation of OnConfigurationChange to automatically load the
-	// required TeamName and ChannelName.
-	if err := p.MattermostPlugin.OnConfigurationChange(); err != nil {
-		p.API.LogError(err.Error())
-		return nil
+	var configuration = new(configuration)
+
+	// Load the public configuration fields from the Mattermost server configuration.
+	if err := p.API.LoadPluginConfiguration(configuration); err != nil {
+		return errors.Wrap(err, "failed to load plugin configuration")
 	}
 
-	team, err := p.API.GetTeamByName(p.TeamName)
+	team, err := p.API.GetTeamByName(configuration.TeamName)
 	if err != nil {
-		p.API.LogError("failed to find team", "team_name", p.TeamName)
-		return nil
+		return errors.Wrapf(err, "failed to find team %s", configuration.TeamName)
 	}
 
-	channel, err := p.API.GetChannelByName(p.ChannelName, team.Id, false)
+	channel, err := p.API.GetChannelByName(configuration.ChannelName, team.Id, false)
 	if err != nil {
-		p.API.LogError("failed to find channel", "channel_name", p.ChannelName)
-		return nil
+		return errors.Wrapf(err, "failed to find channel %s", configuration.ChannelName)
 	}
 
-	p.channelId = channel.Id
+	configuration.channelId = channel.Id
+
+	p.setConfiguration(configuration)
 
 	return nil
 }
 
 func (p *HelpPlugin) MessageHasBeenPosted(c *plugin.Context, post *model.Post) {
+	configuration := p.getConfiguration()
+
 	// Ignore posts not in the configured channel
-	if post.ChannelId != p.channelId {
+	if post.ChannelId != configuration.channelId {
 		return
 	}
 
@@ -58,7 +101,7 @@ func (p *HelpPlugin) MessageHasBeenPosted(c *plugin.Context, post *model.Post) {
 	}
 
 	p.API.SendEphemeralPost(post.UserId, &model.Post{
-		ChannelId: p.channelId,
+		ChannelId: configuration.channelId,
 		Message:   "You asked for help? Checkout https://about.mattermost.com/help/",
 		Props: map[string]interface{}{
 			"sent_by_plugin": true,
diff --git a/plugin/hooks.go b/plugin/hooks.go
index 4af177c0d..b5cff6f6b 100644
--- a/plugin/hooks.go
+++ b/plugin/hooks.go
@@ -55,7 +55,9 @@ type Hooks interface {
 	// will stop receiving hooks just prior to this method being called.
 	OnDeactivate() error
 
-	// OnConfigurationChange is invoked when configuration changes may have been made.
+	// OnConfigurationChange is invoked when configuration changes may have been made. Any
+	// returned error is logged, but does not stop the plugin. You must be prepared to handle
+	// a configuration failure gracefully.
 	OnConfigurationChange() error
 
 	// ServeHTTP allows the plugin to implement the http.Handler interface. Requests destined for