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

app/plugin_api_test.go

@@ -76,16 +76,28 @@ func TestPluginAPILoadPluginConfiguration(t *testing.T) {
 			"fmt"
 		)
 
-		type MyPlugin struct {
-			plugin.MattermostPlugin
-			
+		type configuration struct {
 			MyStringSetting string
 			MyIntSetting int
 			MyBoolSetting bool
 		}
 
+		type MyPlugin struct {
+			plugin.MattermostPlugin	
+
+			configuration configuration
+		}
+
+		func (p *MyPlugin) OnConfigurationChange() error {
+			if err := p.API.LoadPluginConfiguration(&p.configuration); err != nil {
+				return err
+			}
+
+			return nil
+		}
+
 		func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) {
-			return nil, fmt.Sprintf("%v%v%v", p.MyStringSetting, p.MyIntSetting, p.MyBoolSetting)
+			return nil, fmt.Sprintf("%v%v%v", p.configuration.MyStringSetting, p.configuration.MyIntSetting, p.configuration.MyBoolSetting)
 		}
 
 		func main() {
@@ -135,16 +147,28 @@ func TestPluginAPILoadPluginConfigurationDefaults(t *testing.T) {
 			"fmt"
 		)
 
-		type MyPlugin struct {
-			plugin.MattermostPlugin
-			
+		type configuration struct {
 			MyStringSetting string
 			MyIntSetting int
 			MyBoolSetting bool
 		}
 
+		type MyPlugin struct {
+			plugin.MattermostPlugin
+
+			configuration configuration
+		}
+
+		func (p *MyPlugin) OnConfigurationChange() error {
+			if err := p.API.LoadPluginConfiguration(&p.configuration); err != nil {
+				return err
+			}
+
+			return nil
+		}
+
 		func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) {
-			return nil, fmt.Sprintf("%v%v%v", p.MyStringSetting, p.MyIntSetting, p.MyBoolSetting)
+			return nil, fmt.Sprintf("%v%v%v", p.configuration.MyStringSetting, p.configuration.MyIntSetting, p.configuration.MyBoolSetting)
 		}
 
 		func main() {

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
-}

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

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

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,

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