Adding EnsureBot plugin helper. (#10542)

* Adding EnsureBot plugin helper.

* Removing unessisary GetBot call.

* Moving to own file and error handling cleanup.

* Removing patch functionaliy. Plugins should manage their own bot account updates for now.

* Adding tests and cleaning up errors.

* Modify to not shadow err.

* Moving helpers to seperate interface.

* Feedback fixes

Makefile

@@ -338,6 +338,7 @@ plugin-mocks: ## Creates mock files for plugins.
 	go get -u github.com/vektra/mockery/...
 	$(GOPATH)/bin/mockery -dir plugin -name API -output plugin/plugintest -outpkg plugintest -case underscore -note 'Regenerate this file using `make plugin-mocks`.'
 	$(GOPATH)/bin/mockery -dir plugin -name Hooks -output plugin/plugintest -outpkg plugintest -case underscore -note 'Regenerate this file using `make plugin-mocks`.'
+	$(GOPATH)/bin/mockery -dir plugin -name Helpers -output plugin/plugintest -outpkg plugintest -case underscore -note 'Regenerate this file using `make plugin-mocks`.'
 
 pluginapi: ## Generates api and hooks glue code for plugins
 	go generate ./plugin

plugin/api.go

@@ -457,6 +457,7 @@ type API interface {
 	// KV Store Section
 
 	// KVSet will store a key-value pair, unique per plugin.
+	// Provided helper functions and internal plugin code will use the prefix `mmi_` before keys. Do not use this prefix.
 	KVSet(key string, value []byte) *model.AppError
 
 	// KVCompareAndSet will update a key-value pair,

plugin/client.go

@@ -7,16 +7,21 @@ import (
 	"github.com/hashicorp/go-plugin"
 )
 
+const (
+	INTERNAL_KEY_PREFIX = "mmi_"
+	BOT_USER_KEY        = INTERNAL_KEY_PREFIX + "botid"
+)
+
 // Starts the serving of a Mattermost plugin over net/rpc. gRPC is not yet supported.
 //
 // Call this when your plugin is ready to start.
 func ClientMain(pluginImplementation interface{}) {
 	if impl, ok := pluginImplementation.(interface {
-		SetAPI(api API)
+		SetAPI(api API, helpers Helpers)
 	}); !ok {
 		panic("Plugin implementation given must embed plugin.MattermostPlugin")
 	} else {
-		impl.SetAPI(nil)
+		impl.SetAPI(nil, nil)
 	}
 
 	pluginMap := map[string]plugin.Plugin{
@@ -31,11 +36,13 @@ func ClientMain(pluginImplementation interface{}) {
 
 type MattermostPlugin struct {
 	// API exposes the plugin api, and becomes available just prior to the OnActive hook.
-	API API
+	API     API
+	Helpers Helpers
 }
 
 // SetAPI persists the given API interface to the plugin. It is invoked just prior to the
 // OnActivate hook, exposing the API for use by the plugin.
-func (p *MattermostPlugin) SetAPI(api API) {
+func (p *MattermostPlugin) SetAPI(api API, helpers Helpers) {
 	p.API = api
+	p.Helpers = helpers
 }

plugin/client_rpc.go

@@ -196,9 +196,9 @@ func (s *hooksRPCServer) OnActivate(args *Z_OnActivateArgs, returns *Z_OnActivat
 	}
 
 	if mmplugin, ok := s.impl.(interface {
-		SetAPI(api API)
+		SetAPI(api API, helpers Helpers)
 	}); ok {
-		mmplugin.SetAPI(s.apiRPCClient)
+		mmplugin.SetAPI(s.apiRPCClient, &HelpersImpl{API: s.apiRPCClient})
 	}
 
 	if mmplugin, ok := s.impl.(interface {

plugin/helpers.go

@@ -0,0 +1,17 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package plugin
+
+import "github.com/mattermost/mattermost-server/model"
+
+type Helpers interface {
+	// EnsureBot ether returns an existing bot user or creates a bot user with
+	// the specifications of the passed bot.
+	// Returns the id of the bot created or existing.
+	EnsureBot(bot *model.Bot) (string, error)
+}
+
+type HelpersImpl struct {
+	API API
+}

plugin/helpers_bots.go

@@ -0,0 +1,66 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package plugin
+
+import (
+	"time"
+
+	"github.com/mattermost/mattermost-server/model"
+	"github.com/pkg/errors"
+)
+
+func (p *HelpersImpl) EnsureBot(bot *model.Bot) (retBotId string, retErr error) {
+	// Must provide a bot with a username
+	if bot == nil || len(bot.Username) < 1 {
+		return "", errors.New("passed a bad bot, nil or no username")
+	}
+
+	// If we fail for any reason, this could be a race between creation of bot and
+	// retreval from anouther EnsureBot. Just try the basic retrieve existing again.
+	defer func() {
+		if retBotId == "" || retErr != nil {
+			time.Sleep(time.Second)
+			botIdBytes, err := p.API.KVGet(BOT_USER_KEY)
+			if err == nil && botIdBytes != nil {
+				retBotId = string(botIdBytes)
+				retErr = nil
+			}
+		}
+	}()
+
+	botIdBytes, kvGetErr := p.API.KVGet(BOT_USER_KEY)
+	if kvGetErr != nil {
+		return "", errors.Wrap(kvGetErr, "failed to get bot")
+	}
+
+	// If the bot has already been created, there is nothing to do.
+	if botIdBytes != nil {
+		botId := string(botIdBytes)
+		return botId, nil
+	}
+
+	// Check for an existing bot user with that username. If one exists, then use that.
+	if user, userGetErr := p.API.GetUserByUsername(bot.Username); userGetErr == nil && user != nil {
+		if user.IsBot {
+			if kvSetErr := p.API.KVSet(BOT_USER_KEY, []byte(user.Id)); kvSetErr != nil {
+				p.API.LogWarn("Failed to set claimed bot user id.", "userid", user.Id, "err", kvSetErr)
+			}
+			return user.Id, nil
+		} else {
+			return "", errors.New("unable to create bot because user exists with the same name")
+		}
+	}
+
+	// Create a new bot user for the plugin
+	createdBot, createBotErr := p.API.CreateBot(bot)
+	if createBotErr != nil {
+		return "", errors.Wrap(createBotErr, "failed to create bot")
+	}
+
+	if kvSetErr := p.API.KVSet(BOT_USER_KEY, []byte(createdBot.UserId)); kvSetErr != nil {
+		p.API.LogWarn("Failed to set created bot user id.", "userid", createdBot.UserId, "err", kvSetErr)
+	}
+
+	return createdBot.UserId, nil
+}

plugin/helpers_bots_test.go

@@ -0,0 +1,154 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package plugin_test
+
+import (
+	"testing"
+
+	"github.com/mattermost/mattermost-server/model"
+	"github.com/mattermost/mattermost-server/plugin"
+	"github.com/mattermost/mattermost-server/plugin/plugintest"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestEnsureBot(t *testing.T) {
+	setupAPI := func() *plugintest.API {
+		return &plugintest.API{}
+	}
+
+	testbot := &model.Bot{
+		Username:    "testbot",
+		DisplayName: "Test Bot",
+		Description: "testbotdescription",
+	}
+
+	t.Run("bad parameters", func(t *testing.T) {
+		t.Run("no bot", func(t *testing.T) {
+			p := &plugin.HelpersImpl{}
+			botId, err := p.EnsureBot(nil)
+			assert.Equal(t, "", botId)
+			assert.NotNil(t, err)
+		})
+		t.Run("bad username", func(t *testing.T) {
+			p := &plugin.HelpersImpl{}
+			botId, err := p.EnsureBot(&model.Bot{
+				Username: "",
+			})
+			assert.Equal(t, "", botId)
+			assert.NotNil(t, err)
+		})
+	})
+
+	t.Run("if bot already exists", func(t *testing.T) {
+		t.Run("should find and return the existing bot ID", func(t *testing.T) {
+			expectedBotId := model.NewId()
+
+			api := setupAPI()
+			api.On("KVGet", plugin.BOT_USER_KEY).Return([]byte(expectedBotId), nil)
+			defer api.AssertExpectations(t)
+
+			p := &plugin.HelpersImpl{}
+			p.API = api
+
+			botId, err := p.EnsureBot(testbot)
+
+			assert.Equal(t, expectedBotId, botId)
+			assert.Nil(t, err)
+		})
+
+		t.Run("should return an error if unable to get bot", func(t *testing.T) {
+			api := setupAPI()
+			api.On("KVGet", plugin.BOT_USER_KEY).Return(nil, &model.AppError{})
+			defer api.AssertExpectations(t)
+
+			p := &plugin.HelpersImpl{}
+			p.API = api
+
+			botId, err := p.EnsureBot(testbot)
+
+			assert.Equal(t, "", botId)
+			assert.NotNil(t, err)
+		})
+	})
+
+	t.Run("if bot doesn't exist", func(t *testing.T) {
+		t.Run("should create the bot and return the ID", func(t *testing.T) {
+			expectedBotId := model.NewId()
+
+			api := setupAPI()
+			api.On("KVGet", plugin.BOT_USER_KEY).Return(nil, nil)
+			api.On("GetUserByUsername", testbot.Username).Return(nil, nil)
+			api.On("CreateBot", testbot).Return(&model.Bot{
+				UserId: expectedBotId,
+			}, nil)
+			api.On("KVSet", plugin.BOT_USER_KEY, []byte(expectedBotId)).Return(nil)
+			defer api.AssertExpectations(t)
+
+			p := &plugin.HelpersImpl{}
+			p.API = api
+
+			botId, err := p.EnsureBot(testbot)
+
+			assert.Equal(t, expectedBotId, botId)
+			assert.Nil(t, err)
+		})
+
+		t.Run("should claim existing bot and return the ID", func(t *testing.T) {
+			expectedBotId := model.NewId()
+
+			api := setupAPI()
+			api.On("KVGet", plugin.BOT_USER_KEY).Return(nil, nil)
+			api.On("GetUserByUsername", testbot.Username).Return(&model.User{
+				Id:    expectedBotId,
+				IsBot: true,
+			}, nil)
+			api.On("KVSet", plugin.BOT_USER_KEY, []byte(expectedBotId)).Return(nil)
+			defer api.AssertExpectations(t)
+
+			p := &plugin.HelpersImpl{}
+			p.API = api
+
+			botId, err := p.EnsureBot(testbot)
+
+			assert.Equal(t, expectedBotId, botId)
+			assert.Nil(t, err)
+		})
+
+		t.Run("should fail if user exists with the same name and is not a bot", func(t *testing.T) {
+			api := setupAPI()
+			api.On("KVGet", plugin.BOT_USER_KEY).Return(nil, nil)
+			api.On("GetUserByUsername", testbot.Username).Return(&model.User{
+				Id:    "conflictingid",
+				IsBot: false,
+			}, nil)
+			defer api.AssertExpectations(t)
+
+			p := &plugin.HelpersImpl{}
+			p.API = api
+
+			botId, err := p.EnsureBot(testbot)
+
+			t.Log(botId)
+			t.Log(err)
+			assert.Equal(t, "", botId)
+			assert.NotNil(t, err)
+		})
+
+		t.Run("shoudl fail if create bot fails", func(t *testing.T) {
+			api := setupAPI()
+			api.On("KVGet", plugin.BOT_USER_KEY).Return(nil, nil)
+			api.On("GetUserByUsername", testbot.Username).Return(nil, nil)
+			api.On("CreateBot", testbot).Return(nil, &model.AppError{})
+			defer api.AssertExpectations(t)
+
+			p := &plugin.HelpersImpl{}
+			p.API = api
+
+			botId, err := p.EnsureBot(testbot)
+
+			assert.Equal(t, "", botId)
+			assert.NotNil(t, err)
+		})
+	})
+}

plugin/plugintest/example_hello_user_test.go

@@ -42,8 +42,11 @@ func Example() {
 	api.On("GetUser", user.Id).Return(user, nil)
 	defer api.AssertExpectations(t)
 
+	helpers := &plugintest.Helpers{}
+	defer helpers.AssertExpectations(t)
+
 	p := &HelloUserPlugin{}
-	p.SetAPI(api)
+	p.SetAPI(api, helpers)
 
 	w := httptest.NewRecorder()
 	r := httptest.NewRequest("GET", "/", nil)

plugin/plugintest/helpers.go

@@ -0,0 +1,34 @@
+// Code generated by mockery v1.0.0. DO NOT EDIT.
+
+// Regenerate this file using `make plugin-mocks`.
+
+package plugintest
+
+import mock "github.com/stretchr/testify/mock"
+import model "github.com/mattermost/mattermost-server/model"
+
+// Helpers is an autogenerated mock type for the Helpers type
+type Helpers struct {
+	mock.Mock
+}
+
+// EnsureBot provides a mock function with given fields: bot
+func (_m *Helpers) EnsureBot(bot *model.Bot) (string, error) {
+	ret := _m.Called(bot)
+
+	var r0 string
+	if rf, ok := ret.Get(0).(func(*model.Bot) string); ok {
+		r0 = rf(bot)
+	} else {
+		r0 = ret.Get(0).(string)
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(*model.Bot) error); ok {
+		r1 = rf(bot)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}