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

1 files changed, 60 insertions, 17 deletions

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,