diff --git a/mattermost-plugin/server/plugin.go b/mattermost-plugin/server/plugin.go index 2ecfc8339ee..cffe04726e8 100644 --- a/mattermost-plugin/server/plugin.go +++ b/mattermost-plugin/server/plugin.go @@ -17,6 +17,7 @@ import ( "github.com/mattermost/focalboard/server/services/store" "github.com/mattermost/focalboard/server/services/store/mattermostauthlayer" "github.com/mattermost/focalboard/server/services/store/sqlstore" + "github.com/mattermost/focalboard/server/utils" "github.com/mattermost/focalboard/server/ws" pluginapi "github.com/mattermost/mattermost-plugin-api" @@ -154,15 +155,17 @@ func (p *Plugin) OnActivate() error { return err } - limits, err := p.API.GetCloudLimits() - if err != nil { - fmt.Println("ERROR FETCHING CLOUD LIMITS WHEN STARTING THE PLUGIN", err) - return err - } + if utils.IsCloudLicense(p.API.GetLicense()) { + limits, err := p.API.GetCloudLimits() + if err != nil { + fmt.Println("ERROR FETCHING CLOUD LIMITS WHEN STARTING THE PLUGIN", err) + return err + } - if err := server.App().SetCloudLimits(limits); err != nil { - fmt.Println("ERROR SETTING CLOUD LIMITS WHEN STARTING THE PLUGIN", err) - return err + if err := server.App().SetCloudLimits(limits); err != nil { + fmt.Println("ERROR SETTING CLOUD LIMITS WHEN STARTING THE PLUGIN", err) + return err + } } p.server = server diff --git a/server/api/api.go b/server/api/api.go index b9751144a00..d3893258657 100644 --- a/server/api/api.go +++ b/server/api/api.go @@ -336,7 +336,7 @@ func (a *API) handleGetBlocks(w http.ResponseWriter, r *http.Request) { ) var bErr error - blocks, bErr = a.app.ApplyCloudLimits(blocks) + blocks, bErr = a.app.ApplyCloudLimits(*container, blocks) if bErr != nil { a.errorResponse(w, r.URL.Path, http.StatusInternalServerError, "", bErr) return @@ -1003,7 +1003,7 @@ func (a *API) handleGetSubTree(w http.ResponseWriter, r *http.Request) { ) var bErr error - blocks, bErr = a.app.ApplyCloudLimits(blocks) + blocks, bErr = a.app.ApplyCloudLimits(*container, blocks) if bErr != nil { a.errorResponse(w, r.URL.Path, http.StatusInternalServerError, "", bErr) return diff --git a/server/app/cloud.go b/server/app/cloud.go index 9d4a18569d9..320bb1ef747 100644 --- a/server/app/cloud.go +++ b/server/app/cloud.go @@ -7,12 +7,12 @@ import ( "errors" "fmt" - "github.com/mattermost/focalboard/server/utils" - + mmModel "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-server/v6/shared/mlog" "github.com/mattermost/focalboard/server/model" - mmModel "github.com/mattermost/mattermost-server/v6/model" + "github.com/mattermost/focalboard/server/services/store" + "github.com/mattermost/focalboard/server/utils" ) var ErrNilPluginAPI = errors.New("server not running in plugin mode") @@ -107,7 +107,66 @@ func (a *App) UpdateCardLimitTimestamp() error { return a.doUpdateCardLimitTimestamp() } -func (a *App) ApplyCloudLimits(blocks []model.Block) ([]model.Block, error) { +// getTemplateMapForBlocks gets all board ids for the blocks, directly +// from the list if the boards are present or fetching them if +// necessary, and builds a map with the board IDs as the key and their +// isTemplate field as the value. +func (a *App) getTemplateMapForBlocks(c store.Container, blocks []model.Block) (map[string]bool, error) { + boards := []model.Block{} + boardIDMap := map[string]bool{} + for _, block := range blocks { + if block.Type == model.TypeBoard { + boards = append(boards, block) + } else { + boardIDMap[block.RootID] = true + } + } + + boardIDs := []string{} + // if the board is already part of the block set, we don't need to + // fetch it from the database + for boardID := range boardIDMap { + alreadyPresent := false + for _, board := range boards { + if board.ID == boardID { + alreadyPresent = true + break + } + } + + if !alreadyPresent { + boardIDs = append(boardIDs, boardID) + } + } + + if len(boardIDs) != 0 { + fetchedBoards, err := a.store.GetBlocksByIDs(c, boardIDs) + if err != nil { + return nil, err + } + boards = append(boards, fetchedBoards...) + } + + templateMap := map[string]bool{} + for _, board := range boards { + if isTemplateStr, ok := board.Fields["isTemplate"]; ok { + isTemplate, ok := isTemplateStr.(bool) + if !ok { + return nil, newErrInvalidIsTemplate(board.ID) + } + templateMap[board.ID] = isTemplate + } else { + templateMap[board.ID] = false + } + } + + return templateMap, nil +} + +// ApplyCloudLimits takes a set of blocks and, if the server is cloud +// limited, limits those that are outside of the card limit and don't +// belong to a template. +func (a *App) ApplyCloudLimits(c store.Container, blocks []model.Block) ([]model.Block, error) { // if there is no limit currently being applied, return if !a.IsCloudLimited() { return blocks, nil @@ -118,15 +177,32 @@ func (a *App) ApplyCloudLimits(blocks []model.Block) ([]model.Block, error) { return nil, err } - // ToDo: - // 1-get limited cards only on a map - // 2-iterate through all the blocks, limiting those that either - // are limited cards or are linked to them + templateMap, err := a.getTemplateMapForBlocks(c, blocks) + if err != nil { + return nil, err + } limitedBlocks := make([]model.Block, len(blocks)) for i, block := range blocks { - if block.Type != model.TypeBoard && - block.Type != model.TypeView && + // boards are never limited + if block.Type == model.TypeBoard { + limitedBlocks[i] = block + continue + } + + isTemplate, ok := templateMap[block.RootID] + if !ok { + return nil, newErrBoardNotFoundInTemplateMap(block.RootID) + } + + // if the block belongs to a template, it will never be + // limited + if isTemplate { + limitedBlocks[i] = block + continue + } + + if block.Type == model.TypeCard && block.UpdateAt < cardLimitTimestamp { limitedBlocks[i] = block.GetLimited() } else { @@ -188,3 +264,27 @@ func (a *App) NotifyPortalAdminsUpgradeRequest(workspaceID string) error { return nil } + +type errInvalidIsTemplate struct { + id string +} + +func newErrInvalidIsTemplate(id string) *errInvalidIsTemplate { + return &errInvalidIsTemplate{id} +} + +func (ei *errInvalidIsTemplate) Error() string { + return fmt.Sprintf("invalid isTemplate field value for board %q", ei.id) +} + +type errBoardNotFoundInTemplateMap struct { + id string +} + +func newErrBoardNotFoundInTemplateMap(id string) *errBoardNotFoundInTemplateMap { + return &errBoardNotFoundInTemplateMap{id} +} + +func (eb *errBoardNotFoundInTemplateMap) Error() string { + return fmt.Sprintf("board %q not found in template map", eb.id) +} diff --git a/server/app/cloud_test.go b/server/app/cloud_test.go index 1a43c763299..9172ba16e0a 100644 --- a/server/app/cloud_test.go +++ b/server/app/cloud_test.go @@ -9,6 +9,9 @@ import ( mmModel "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-server/v6/plugin/plugintest" + + "github.com/mattermost/focalboard/server/model" + "github.com/mattermost/focalboard/server/services/store" ) func TestIsCloud(t *testing.T) { @@ -312,3 +315,223 @@ func TestNotifyPortalAdminsUpgradeRequest(t *testing.T) { assert.NoError(t, err) }) } + +func TestGetTemplateMapForBlocks(t *testing.T) { + container := store.Container{ + WorkspaceID: "0", + } + + t.Run("should not access the database if all boards are present already in the blocks", func(t *testing.T) { + th, tearDown := SetupTestHelper(t) + defer tearDown() + + blocks := []model.Block{ + { + ID: "board1", + Type: model.TypeBoard, + ParentID: "board1", + RootID: "board1", + Fields: map[string]interface{}{"isTemplate": true}, + }, + { + ID: "card1", + Type: model.TypeCard, + ParentID: "board1", + RootID: "board1", + }, + { + ID: "board2", + Type: model.TypeBoard, + ParentID: "board2", + RootID: "board2", + Fields: map[string]interface{}{"isTemplate": false}, + }, + { + ID: "card2", + Type: model.TypeCard, + ParentID: "board2", + RootID: "board2", + }, + { + ID: "text2", + Type: model.TypeText, + ParentID: "card2", + RootID: "board2", + }, + } + + templateMap, err := th.App.getTemplateMapForBlocks(container, blocks) + require.NoError(t, err) + require.Len(t, templateMap, 2) + require.Contains(t, templateMap, "board1") + require.True(t, templateMap["board1"]) + require.Contains(t, templateMap, "board2") + require.False(t, templateMap["board2"]) + }) + + t.Run("should fetch boards from the database if not present", func(t *testing.T) { + th, tearDown := SetupTestHelper(t) + defer tearDown() + + blocks := []model.Block{ + { + ID: "board1", + Type: model.TypeBoard, + ParentID: "board1", + RootID: "board1", + Fields: map[string]interface{}{"isTemplate": true}, + }, + { + ID: "card1", + Type: model.TypeCard, + ParentID: "board1", + RootID: "board1", + }, + { + ID: "card2", + Type: model.TypeCard, + ParentID: "board2", + RootID: "board2", + }, + { + ID: "text3", + Type: model.TypeText, + ParentID: "card3", + RootID: "board3", + }, + } + + // doesn't have Fields, so it should be treated as not + // template + board2 := model.Block{ + ID: "board2", + Type: model.TypeBoard, + ParentID: "board2", + RootID: "board2", + } + + board3 := model.Block{ + ID: "board3", + Type: model.TypeBoard, + ParentID: "board3", + RootID: "board3", + Fields: map[string]interface{}{"isTemplate": true}, + } + + th.Store.EXPECT(). + GetBlocksByIDs(container, gomock.InAnyOrder([]string{"board2", "board3"})). + Return([]model.Block{board2, board3}, nil) + + templateMap, err := th.App.getTemplateMapForBlocks(container, blocks) + require.NoError(t, err) + require.Len(t, templateMap, 3) + require.Contains(t, templateMap, "board1") + require.True(t, templateMap["board1"]) + require.Contains(t, templateMap, "board2") + require.False(t, templateMap["board2"]) + require.Contains(t, templateMap, "board3") + require.True(t, templateMap["board3"]) + }) +} + +func TestApplyCloudLimits(t *testing.T) { + container := store.Container{ + WorkspaceID: "0", + } + + fakeLicense := &mmModel.License{ + Features: &mmModel.Features{Cloud: mmModel.NewBool(true)}, + } + + blocks := []model.Block{ + { + ID: "board1", + Type: model.TypeBoard, + ParentID: "board1", + RootID: "board1", + UpdateAt: 100, + }, + { + ID: "card1", + Type: model.TypeCard, + ParentID: "board1", + RootID: "board1", + UpdateAt: 100, + }, + { + ID: "text1", + Type: model.TypeText, + ParentID: "card1", + RootID: "board1", + UpdateAt: 100, + }, + { + ID: "card2", + Type: model.TypeCard, + ParentID: "board1", + RootID: "board1", + UpdateAt: 200, + }, + { + ID: "template", + Type: model.TypeBoard, + ParentID: "template", + RootID: "template", + UpdateAt: 1, + Fields: map[string]interface{}{"isTemplate": true}, + }, + { + ID: "card-from-template", + Type: model.TypeCard, + ParentID: "template", + RootID: "template", + UpdateAt: 1, + }, + } + + t.Run("if the server is not limited, it should return the blocks untouched", func(t *testing.T) { + th, tearDown := SetupTestHelper(t) + defer tearDown() + + require.Zero(t, th.App.CardLimit) + + newBlocks, err := th.App.ApplyCloudLimits(container, blocks) + require.NoError(t, err) + require.ElementsMatch(t, blocks, newBlocks) + }) + + t.Run("if the server is limited, it should limit the blocks that are beyond the card limit timestamp", func(t *testing.T) { + findBlock := func(blocks []model.Block, id string) model.Block { + for _, block := range blocks { + if block.ID == id { + return block + } + } + require.FailNow(t, "block %s not found", id) + return model.Block{} // this should never be reached + } + + th, tearDown := SetupTestHelper(t) + defer tearDown() + + th.App.CardLimit = 5 + + th.Store.EXPECT().GetLicense().Return(fakeLicense) + th.Store.EXPECT().GetCardLimitTimestamp().Return(int64(150), nil) + + newBlocks, err := th.App.ApplyCloudLimits(container, blocks) + require.NoError(t, err) + + // boards are never limited + require.False(t, findBlock(newBlocks, "board1").Limited) + // should be limited as it's beyond the threshold + require.True(t, findBlock(newBlocks, "card1").Limited) + // only cards are limited + require.False(t, findBlock(newBlocks, "text1").Limited) + // should not be limited as it's not beyond the threshold + require.False(t, findBlock(newBlocks, "card2").Limited) + // cards belonging to templates are never limited + require.False(t, findBlock(newBlocks, "template").Limited) + require.False(t, findBlock(newBlocks, "card-from-template").Limited) + }) +} diff --git a/server/services/store/mockstore/mockstore.go b/server/services/store/mockstore/mockstore.go index 12b0d24b602..9c03a904820 100644 --- a/server/services/store/mockstore/mockstore.go +++ b/server/services/store/mockstore/mockstore.go @@ -269,6 +269,21 @@ func (mr *MockStoreMockRecorder) GetBlockHistoryDescendants(arg0, arg1 interface return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetBlockHistoryDescendants", reflect.TypeOf((*MockStore)(nil).GetBlockHistoryDescendants), arg0, arg1) } +// GetBlocksByIDs mocks base method. +func (m *MockStore) GetBlocksByIDs(arg0 store.Container, arg1 []string) ([]model.Block, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetBlocksByIDs", arg0, arg1) + ret0, _ := ret[0].([]model.Block) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetBlocksByIDs indicates an expected call of GetBlocksByIDs. +func (mr *MockStoreMockRecorder) GetBlocksByIDs(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetBlocksByIDs", reflect.TypeOf((*MockStore)(nil).GetBlocksByIDs), arg0, arg1) +} + // GetBlocksWithParent mocks base method. func (m *MockStore) GetBlocksWithParent(arg0 store.Container, arg1 string) ([]model.Block, error) { m.ctrl.T.Helper() diff --git a/server/services/store/sqlstore/blocks.go b/server/services/store/sqlstore/blocks.go index 3464c86bbf0..5fadeab02f9 100644 --- a/server/services/store/sqlstore/blocks.go +++ b/server/services/store/sqlstore/blocks.go @@ -102,6 +102,33 @@ func (s *SQLStore) getBlocksWithParent(db sq.BaseRunner, c store.Container, pare return s.blocksFromRows(rows) } +func (s *SQLStore) getBlocksByIDs(db sq.BaseRunner, c store.Container, ids []string) ([]model.Block, error) { + query := s.getQueryBuilder(db). + Select(s.blockFields()...). + From(s.tablePrefix + "blocks"). + Where(sq.Eq{"coalesce(workspace_id, '0')": c.WorkspaceID}). + Where(sq.Eq{"id": ids}) + + rows, err := query.Query() + if err != nil { + s.logger.Error(`GetBlocksByIDs ERROR`, mlog.Err(err)) + + return nil, err + } + defer s.CloseRows(rows) + + blocks, err := s.blocksFromRows(rows) + if err != nil { + return nil, err + } + + if len(blocks) != len(ids) { + return nil, store.NewErrNotAllFound(ids) + } + + return blocks, nil +} + func (s *SQLStore) getBlocksWithRootID(db sq.BaseRunner, c store.Container, rootID string) ([]model.Block, error) { query := s.getQueryBuilder(db). Select(s.blockFields()...). diff --git a/server/services/store/sqlstore/public_methods.go b/server/services/store/sqlstore/public_methods.go index 8fd3b11f9b2..1356115c4d6 100644 --- a/server/services/store/sqlstore/public_methods.go +++ b/server/services/store/sqlstore/public_methods.go @@ -117,6 +117,11 @@ func (s *SQLStore) GetBlockHistoryDescendants(boardID string, opts model.QueryBl } +func (s *SQLStore) GetBlocksByIDs(c store.Container, ids []string) ([]model.Block, error) { + return s.getBlocksByIDs(s.db, c, ids) + +} + func (s *SQLStore) GetBlocksWithParent(c store.Container, parentID string) ([]model.Block, error) { return s.getBlocksWithParent(s.db, c, parentID) diff --git a/server/services/store/store.go b/server/services/store/store.go index e4ba0c637de..c0eb749a5b4 100644 --- a/server/services/store/store.go +++ b/server/services/store/store.go @@ -5,6 +5,7 @@ package store import ( "errors" "fmt" + "strings" "time" mmModel "github.com/mattermost/mattermost-server/v6/model" @@ -24,6 +25,7 @@ type Container struct { type Store interface { GetBlocksWithParentAndType(c Container, parentID string, blockType string) ([]model.Block, error) GetBlocksWithParent(c Container, parentID string) ([]model.Block, error) + GetBlocksByIDs(c Container, ids []string) ([]model.Block, error) GetBlocksWithRootID(c Container, rootID string) ([]model.Block, error) GetBlocksWithType(c Container, blockType string) ([]model.Block, error) GetSubTree2(c Container, blockID string, opts model.QuerySubtreeOptions) ([]model.Block, error) @@ -143,3 +145,30 @@ func IsErrNotFound(err error) bool { var nf *ErrNotFound return errors.As(err, &nf) } + +// ErrNotAllFound is an error type that can be returned by store APIs +// when a query that should fetch a certain amount of records +// unexpectedly fetches less. +type ErrNotAllFound struct { + resources []string +} + +func NewErrNotAllFound(resources []string) *ErrNotAllFound { + return &ErrNotAllFound{ + resources: resources, + } +} + +func (na *ErrNotAllFound) Error() string { + return fmt.Sprintf("not all instances in {%s} found", strings.Join(na.resources, ", ")) +} + +// IsErrNotAllFound returns true if `err` is or wraps a ErrNotAllFound. +func IsErrNotAllFound(err error) bool { + if err == nil { + return false + } + + var na *ErrNotAllFound + return errors.As(err, &na) +} diff --git a/server/services/store/storetests/blocks.go b/server/services/store/storetests/blocks.go index 95332a05636..096c92e36c7 100644 --- a/server/services/store/storetests/blocks.go +++ b/server/services/store/storetests/blocks.go @@ -742,8 +742,8 @@ func testUndeleteBlock(t *testing.T, store store.Store, container store.Containe }) } -func testGetBlocks(t *testing.T, store store.Store, container store.Container) { - blocks, err := store.GetAllBlocks(container) +func testGetBlocks(t *testing.T, storeInstance store.Store, container store.Container) { + blocks, err := storeInstance.GetAllBlocks(container) require.NoError(t, err) blocksToInsert := []model.Block{ @@ -783,68 +783,82 @@ func testGetBlocks(t *testing.T, store store.Store, container store.Container) { Type: "test", }, } - InsertBlocks(t, store, container, blocksToInsert, "user-id-1") - defer DeleteBlocks(t, store, container, blocksToInsert, "test") + InsertBlocks(t, storeInstance, container, blocksToInsert, "user-id-1") + defer DeleteBlocks(t, storeInstance, container, blocksToInsert, "test") t.Run("not existing parent", func(t *testing.T) { time.Sleep(1 * time.Millisecond) - blocks, err = store.GetBlocksWithParentAndType(container, "not-exists", "test") + blocks, err = storeInstance.GetBlocksWithParentAndType(container, "not-exists", "test") require.NoError(t, err) require.Len(t, blocks, 0) }) t.Run("not existing type", func(t *testing.T) { time.Sleep(1 * time.Millisecond) - blocks, err = store.GetBlocksWithParentAndType(container, "block1", "not-existing") + blocks, err = storeInstance.GetBlocksWithParentAndType(container, "block1", "not-existing") require.NoError(t, err) require.Len(t, blocks, 0) }) t.Run("valid parent and type", func(t *testing.T) { time.Sleep(1 * time.Millisecond) - blocks, err = store.GetBlocksWithParentAndType(container, "block1", "test") + blocks, err = storeInstance.GetBlocksWithParentAndType(container, "block1", "test") require.NoError(t, err) require.Len(t, blocks, 2) }) t.Run("not existing parent", func(t *testing.T) { time.Sleep(1 * time.Millisecond) - blocks, err = store.GetBlocksWithParent(container, "not-exists") + blocks, err = storeInstance.GetBlocksWithParent(container, "not-exists") require.NoError(t, err) require.Len(t, blocks, 0) }) t.Run("valid parent", func(t *testing.T) { time.Sleep(1 * time.Millisecond) - blocks, err = store.GetBlocksWithParent(container, "block1") + blocks, err = storeInstance.GetBlocksWithParent(container, "block1") require.NoError(t, err) require.Len(t, blocks, 3) }) + t.Run("by ids, all existing", func(t *testing.T) { + time.Sleep(1 * time.Millisecond) + blocks, err = storeInstance.GetBlocksByIDs(container, []string{"block1", "block3"}) + require.NoError(t, err) + require.Len(t, blocks, 2) + }) + + t.Run("by ids, some existing", func(t *testing.T) { + time.Sleep(1 * time.Millisecond) + blocks, err = storeInstance.GetBlocksByIDs(container, []string{"not-exists", "block3"}) + require.Error(t, err) + require.True(t, store.IsErrNotAllFound(err)) + }) + t.Run("not existing type", func(t *testing.T) { time.Sleep(1 * time.Millisecond) - blocks, err = store.GetBlocksWithType(container, "not-exists") + blocks, err = storeInstance.GetBlocksWithType(container, "not-exists") require.NoError(t, err) require.Len(t, blocks, 0) }) t.Run("valid type", func(t *testing.T) { time.Sleep(1 * time.Millisecond) - blocks, err = store.GetBlocksWithType(container, "test") + blocks, err = storeInstance.GetBlocksWithType(container, "test") require.NoError(t, err) require.Len(t, blocks, 4) }) t.Run("not existing parent", func(t *testing.T) { time.Sleep(1 * time.Millisecond) - blocks, err = store.GetBlocksWithRootID(container, "not-exists") + blocks, err = storeInstance.GetBlocksWithRootID(container, "not-exists") require.NoError(t, err) require.Len(t, blocks, 0) }) t.Run("valid parent", func(t *testing.T) { time.Sleep(1 * time.Millisecond) - blocks, err = store.GetBlocksWithRootID(container, "block1") + blocks, err = storeInstance.GetBlocksWithRootID(container, "block1") require.NoError(t, err) require.Len(t, blocks, 4) })