diff --git a/api/config.go b/api/config.go index 29055ae71..0b1671e15 100644 --- a/api/config.go +++ b/api/config.go @@ -71,6 +71,8 @@ const ( type LimitsConfig struct { // Trigger contains limits for triggers. Trigger TriggerLimits + // Trigger contains limits for teams. + Team TeamLimits } // TriggerLimits contains all limits applied for triggers. @@ -85,5 +87,22 @@ func GetTestLimitsConfig() LimitsConfig { Trigger: TriggerLimits{ MaxNameSize: DefaultTriggerNameMaxSize, }, + Team: TeamLimits{ + MaxNameSize: DefaultTeamNameMaxSize, + MaxDescriptionSize: DefaultTeamDescriptionMaxSize, + }, } } + +const ( + DefaultTeamNameMaxSize = 100 + DefaultTeamDescriptionMaxSize = 1000 +) + +// TeamLimits contains all limits applied for triggers. +type TeamLimits struct { + // MaxNameSize is the amount of characters allowed in team name. + MaxNameSize int + // MaxNameSize is the amount of characters allowed in team description. + MaxDescriptionSize int +} diff --git a/api/controller/team.go b/api/controller/team.go index 3c35ac0e2..5de8e9f6b 100644 --- a/api/controller/team.go +++ b/api/controller/team.go @@ -49,8 +49,13 @@ func CreateTeam(dataBase moira.Database, team dto.TeamModel, userID string) (dto return dto.SaveTeamResponse{}, api.ErrorInternalServer(fmt.Errorf("cannot generate unique id for team")) } } + err := dataBase.SaveTeam(teamID, team.ToMoiraTeam()) if err != nil { + if errors.Is(err, database.ErrTeamWithNameAlreadyExists) { + return dto.SaveTeamResponse{}, api.ErrorInvalidRequest(fmt.Errorf("cannot save team: %w", err)) + } + return dto.SaveTeamResponse{}, api.ErrorInternalServer(fmt.Errorf("cannot save team: %w", err)) } @@ -295,6 +300,10 @@ func AddTeamUsers(dataBase moira.Database, teamID string, newUsers []string) (dt func UpdateTeam(dataBase moira.Database, teamID string, team dto.TeamModel) (dto.SaveTeamResponse, *api.ErrorResponse) { err := dataBase.SaveTeam(teamID, team.ToMoiraTeam()) if err != nil { + if errors.Is(err, database.ErrTeamWithNameAlreadyExists) { + return dto.SaveTeamResponse{}, api.ErrorInvalidRequest(fmt.Errorf("cannot save team: %w", err)) + } + return dto.SaveTeamResponse{}, api.ErrorInternalServer(fmt.Errorf("cannot save team: %w", err)) } return dto.SaveTeamResponse{ID: teamID}, nil diff --git a/api/controller/team_test.go b/api/controller/team_test.go index 0eb937e4f..eda2eafeb 100644 --- a/api/controller/team_test.go +++ b/api/controller/team_test.go @@ -82,6 +82,14 @@ func TestCreateTeam(t *testing.T) { So(response, ShouldResemble, dto.SaveTeamResponse{}) So(err, ShouldResemble, api.ErrorInternalServer(fmt.Errorf("cannot save team: %w", returnErr))) }) + + Convey("team with name already exists error, while saving", func() { + dataBase.EXPECT().GetTeam(gomock.Any()).Return(moira.Team{}, database.ErrNil) + dataBase.EXPECT().SaveTeam(gomock.Any(), team.ToMoiraTeam()).Return(database.ErrTeamWithNameAlreadyExists) + response, err := CreateTeam(dataBase, team, user) + So(response, ShouldResemble, dto.SaveTeamResponse{}) + So(err, ShouldResemble, api.ErrorInvalidRequest(fmt.Errorf("cannot save team: %w", database.ErrTeamWithNameAlreadyExists))) + }) }) } diff --git a/api/dto/team.go b/api/dto/team.go index 9b08cb7ee..eafaf4aa8 100644 --- a/api/dto/team.go +++ b/api/dto/team.go @@ -1,17 +1,17 @@ package dto import ( + "errors" "fmt" "net/http" "unicode/utf8" + "github.com/moira-alert/moira/api/middleware" + "github.com/moira-alert/moira" ) -const ( - teamNameLimit = 100 - teamDescriptionLimit = 1000 -) +var errEmptyTeamName = errors.New("team name cannot be empty") // TeamModel is a structure that represents team entity in HTTP transfer. type TeamModel struct { @@ -31,15 +31,20 @@ func NewTeamModel(team moira.Team) TeamModel { // Bind is a method that implements Binder interface from chi and checks that validity of data in request. func (t TeamModel) Bind(request *http.Request) error { + limits := middleware.GetLimits(request) + if t.Name == "" { - return fmt.Errorf("team name cannot be empty") + return errEmptyTeamName } - if utf8.RuneCountInString(t.Name) > teamNameLimit { - return fmt.Errorf("team name cannot be longer than %d characters", teamNameLimit) + + if utf8.RuneCountInString(t.Name) > limits.Team.MaxNameSize { + return fmt.Errorf("team name cannot be longer than %d characters", limits.Team.MaxNameSize) } - if utf8.RuneCountInString(t.Description) > teamDescriptionLimit { - return fmt.Errorf("team description cannot be longer than %d characters", teamNameLimit) + + if utf8.RuneCountInString(t.Description) > limits.Team.MaxDescriptionSize { + return fmt.Errorf("team description cannot be longer than %d characters", limits.Team.MaxDescriptionSize) } + return nil } diff --git a/api/dto/team_test.go b/api/dto/team_test.go new file mode 100644 index 000000000..2886f6945 --- /dev/null +++ b/api/dto/team_test.go @@ -0,0 +1,57 @@ +package dto + +import ( + "fmt" + "net/http" + "strings" + "testing" + + "github.com/moira-alert/moira/api" + "github.com/moira-alert/moira/api/middleware" + + . "github.com/smartystreets/goconvey/convey" +) + +func TestTeamValidation(t *testing.T) { + Convey("Test team validation", t, func() { + teamModel := TeamModel{} + + limits := api.GetTestLimitsConfig() + + request, _ := http.NewRequest("POST", "/api/teams", nil) + request.Header.Set("Content-Type", "application/json") + request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "limits", limits)) + + Convey("with empty team.Name", func() { + err := teamModel.Bind(request) + + So(err, ShouldResemble, errEmptyTeamName) + }) + + Convey("with team.Name has characters more than in limit", func() { + teamModel.Name = strings.Repeat("ё", limits.Team.MaxNameSize+1) + + err := teamModel.Bind(request) + + So(err, ShouldResemble, fmt.Errorf("team name cannot be longer than %d characters", limits.Team.MaxNameSize)) + }) + + Convey("with team.Description has characters more than in limit", func() { + teamModel.Name = strings.Repeat("ё", limits.Team.MaxNameSize) + teamModel.Description = strings.Repeat("ё", limits.Team.MaxDescriptionSize+1) + + err := teamModel.Bind(request) + + So(err, ShouldResemble, fmt.Errorf("team description cannot be longer than %d characters", limits.Team.MaxDescriptionSize)) + }) + + Convey("with valid team", func() { + teamModel.Name = strings.Repeat("ё", limits.Team.MaxNameSize) + teamModel.Description = strings.Repeat("ё", limits.Team.MaxDescriptionSize) + + err := teamModel.Bind(request) + + So(err, ShouldBeNil) + }) + }) +} diff --git a/cmd/api/config.go b/cmd/api/config.go index b63770873..41b998b37 100644 --- a/cmd/api/config.go +++ b/cmd/api/config.go @@ -56,6 +56,8 @@ type apiConfig struct { type LimitsConfig struct { // Trigger contains the limits applied to triggers. Trigger TriggerLimitsConfig `yaml:"trigger"` + // Team contains the limits applied to teams. + Team TeamLimitsConfig `yaml:"team"` } // TriggerLimitsConfig represents the limits which will be applied to all triggers. @@ -64,12 +66,24 @@ type TriggerLimitsConfig struct { MaxNameSize int `yaml:"max_name_size"` } +// TeamLimitsConfig represents the limits which will be applied to all teams. +type TeamLimitsConfig struct { + // MaxNameSize is the max amount of characters allowed in team name. + MaxNameSize int `yaml:"max_name_size"` + // MaxDescriptionSize is the max amount of characters allowed in team description. + MaxDescriptionSize int `yaml:"max_description_size"` +} + // ToLimits converts LimitsConfig to api.LimitsConfig. func (conf LimitsConfig) ToLimits() api.LimitsConfig { return api.LimitsConfig{ Trigger: api.TriggerLimits{ MaxNameSize: conf.Trigger.MaxNameSize, }, + Team: api.TeamLimits{ + MaxNameSize: conf.Team.MaxNameSize, + MaxDescriptionSize: conf.Team.MaxDescriptionSize, + }, } } @@ -259,6 +273,10 @@ func getDefault() config { Trigger: TriggerLimitsConfig{ MaxNameSize: api.DefaultTriggerNameMaxSize, }, + Team: TeamLimitsConfig{ + MaxNameSize: api.DefaultTeamNameMaxSize, + MaxDescriptionSize: api.DefaultTeamDescriptionMaxSize, + }, }, }, Web: webConfig{ diff --git a/cmd/api/config_test.go b/cmd/api/config_test.go index 4d7f86a2b..2892ab5db 100644 --- a/cmd/api/config_test.go +++ b/cmd/api/config_test.go @@ -92,6 +92,10 @@ func Test_webConfig_getDefault(t *testing.T) { Trigger: TriggerLimitsConfig{ MaxNameSize: api.DefaultTriggerNameMaxSize, }, + Team: TeamLimitsConfig{ + MaxNameSize: api.DefaultTeamNameMaxSize, + MaxDescriptionSize: api.DefaultTeamDescriptionMaxSize, + }, }, }, Web: webConfig{ diff --git a/database/database.go b/database/database.go index 825dab56d..459e25b31 100644 --- a/database/database.go +++ b/database/database.go @@ -20,3 +20,6 @@ type ErrLockNotAcquired struct { func (e *ErrLockNotAcquired) Error() string { return fmt.Sprintf("lock was not acquired: %v", e.Err) } + +// ErrTeamWithNameAlreadyExists may be returned from SaveTeam method. +var ErrTeamWithNameAlreadyExists = fmt.Errorf("team with such name alredy exists") diff --git a/database/redis/database.go b/database/redis/database.go index 03cf0cd56..de7df50b3 100644 --- a/database/redis/database.go +++ b/database/redis/database.go @@ -33,6 +33,8 @@ const ( testSource DBSource = "test" ) +var _ moira.Database = &DbConnector{} + // DbConnector contains redis client. type DbConnector struct { client *redis.UniversalClient diff --git a/database/redis/teams.go b/database/redis/teams.go index be94b4c58..ce47a2230 100644 --- a/database/redis/teams.go +++ b/database/redis/teams.go @@ -1,26 +1,116 @@ package redis import ( + "errors" "fmt" + "strings" + "github.com/go-redis/redis/v8" "github.com/moira-alert/moira" + "github.com/moira-alert/moira/database" "github.com/moira-alert/moira/database/redis/reply" ) +const teamSaveAttempts = 3 + // SaveTeam saves team into redis. func (connector *DbConnector) SaveTeam(teamID string, team moira.Team) error { c := *connector.client teamBytes, err := reply.MarshallTeam(team) if err != nil { - return fmt.Errorf("save team error: %w", err) + return fmt.Errorf("failed to marshal team: %w", err) + } + + existedTeam, err := connector.GetTeam(teamID) + if err != nil && !errors.Is(err, database.ErrNil) { + return fmt.Errorf("failed to get team: %w", err) } + for i := 0; i < teamSaveAttempts; i++ { + // need to use watch here because if team name is updated + // we also need to change name in moira-teams-names set + err = c.Watch( + connector.context, + func(tx *redis.Tx) error { + return connector.saveTeamNameInTx(tx, teamID, team.Name, existedTeam.Name) + }, + teamsByNamesKey) + + if err == nil { + break + } + + if !errors.Is(err, redis.TxFailedErr) { + return err + } + } + + // save team err = c.HSet(connector.context, teamsKey, teamID, teamBytes).Err() if err != nil { - return fmt.Errorf("save team redis error: %w", err) + return fmt.Errorf("failed to save team metadata: %w", err) } - return nil + + return err +} + +func (connector *DbConnector) saveTeamNameInTx( + tx *redis.Tx, + teamID string, + newTeamName string, + existedTeamName string, +) error { + teamWithSuchNameID, err := connector.getTeamIDByNameInTx(tx, newTeamName) + if err != nil && !errors.Is(err, database.ErrNil) { + return err + } + + // team with such id does not exist but another team with such name exists + if teamWithSuchNameID != "" && teamWithSuchNameID != teamID { + return database.ErrTeamWithNameAlreadyExists + } + + newTeamLowercaseName := strings.ToLower(newTeamName) + existedTeamLowercaseName := strings.ToLower(existedTeamName) + + _, err = tx.TxPipelined( + connector.context, + func(pipe redis.Pipeliner) error { + updateTeamName := existedTeamName != "" && existedTeamLowercaseName != newTeamLowercaseName + + // if team.Name is changed + if updateTeamName { + // remove old team.Name from team names redis hash + err = pipe.HDel(connector.context, teamsByNamesKey, existedTeamLowercaseName).Err() + if err != nil { + return fmt.Errorf("failed to update team name: %w", err) + } + } + + // save new team.Name to team names redis hash + err = pipe.HSet(connector.context, teamsByNamesKey, newTeamLowercaseName, teamID).Err() + if err != nil { + return fmt.Errorf("failed to save team name: %w", err) + } + + return nil + }) + + return err +} + +func (connector *DbConnector) getTeamIDByNameInTx(tx *redis.Tx, teamName string) (string, error) { + teamID, err := tx.HGet(connector.context, teamsByNamesKey, strings.ToLower(teamName)).Result() + if err != nil { + if errors.Is(err, redis.Nil) { + return "", database.ErrNil + } + + return "", fmt.Errorf("failed to check team name existence: %w", err) + } + + return teamID, nil } // GetTeam retrieves team from redis by it's id. @@ -37,6 +127,22 @@ func (connector *DbConnector) GetTeam(teamID string) (moira.Team, error) { return team, nil } +// GetTeamByName retrieves team from redis by its name. +func (connector *DbConnector) GetTeamByName(name string) (moira.Team, error) { + c := *connector.client + + teamID, err := c.HGet(connector.context, teamsByNamesKey, strings.ToLower(name)).Result() + if err != nil { + if errors.Is(err, redis.Nil) { + return moira.Team{}, database.ErrNil + } + + return moira.Team{}, fmt.Errorf("failed to get team by name: %w", err) + } + + return connector.GetTeam(teamID) +} + // SaveTeamsAndUsers is a function that saves users for one team and teams for bunch of users in one transaction. func (connector *DbConnector) SaveTeamsAndUsers(teamID string, users []string, teams map[string][]string) error { c := *connector.client @@ -110,32 +216,48 @@ func (connector *DbConnector) IsTeamContainUser(teamID, userID string) (bool, er func (connector *DbConnector) DeleteTeam(teamID, userID string) error { c := *connector.client - pipe := c.TxPipeline() - - err := pipe.SRem(connector.context, userTeamsKey(userID), teamID).Err() + team, err := connector.GetTeam(teamID) if err != nil { - return fmt.Errorf("failed to remove team from user's teams: %w", err) - } + if errors.Is(err, database.ErrNil) { + return nil + } - err = pipe.Del(connector.context, teamUsersKey(teamID)).Err() - if err != nil { - return fmt.Errorf("failed to remove team users: %w", err) + return fmt.Errorf("failed to get team to delete: %w", err) } - err = pipe.HDel(connector.context, teamsKey, teamID).Err() + err = c.HDel(connector.context, teamsByNamesKey, strings.ToLower(team.Name)).Err() if err != nil { - return fmt.Errorf("failed to remove team metadata: %w", err) + return fmt.Errorf("failed to remove team name: %w", err) } - _, err = pipe.Exec(connector.context) - if err != nil { - return fmt.Errorf("cannot commit transaction and delete team: %w", err) - } + _, err = c.TxPipelined( + connector.context, + func(pipe redis.Pipeliner) error { + err = pipe.SRem(connector.context, userTeamsKey(userID), teamID).Err() + if err != nil { + return fmt.Errorf("failed to remove team from user's teams: %w", err) + } - return nil + err = pipe.Del(connector.context, teamUsersKey(teamID)).Err() + if err != nil { + return fmt.Errorf("failed to remove team users: %w", err) + } + + err = pipe.HDel(connector.context, teamsKey, teamID).Err() + if err != nil { + return fmt.Errorf("failed to remove team metadata: %w", err) + } + + return nil + }) + + return err } -const teamsKey = "moira-teams" +const ( + teamsKey = "moira-teams" + teamsByNamesKey = "moira-teams-by-names" +) func userTeamsKey(userID string) string { return fmt.Sprintf("moira-userTeams:%s", userID) diff --git a/database/redis/teams_test.go b/database/redis/teams_test.go index d3db66077..cb4093ba9 100644 --- a/database/redis/teams_test.go +++ b/database/redis/teams_test.go @@ -1,6 +1,7 @@ package redis import ( + "strings" "testing" "github.com/moira-alert/moira" @@ -36,6 +37,10 @@ func TestTeamStoring(t *testing.T) { So(err, ShouldBeNil) So(actualTeam, ShouldResemble, team) + actualTeam, err = dataBase.GetTeamByName(team.Name) + So(err, ShouldBeNil) + So(actualTeam, ShouldResemble, team) + actualTeam, err = dataBase.GetTeam("nonExistentTeam") So(err, ShouldResemble, database.ErrNil) So(actualTeam, ShouldResemble, moira.Team{}) @@ -130,20 +135,141 @@ func TestTeamStoring(t *testing.T) { Name: "TeamName", Description: "Team Description", } + err = dataBase.SaveTeam(teamToDeleteID, teamToDelete) So(err, ShouldBeNil) + err = dataBase.SaveTeamsAndUsers(teamToDeleteID, []string{userOfTeamToDeleteID}, map[string][]string{teamToDeleteID: {userOfTeamToDeleteID}}) So(err, ShouldBeNil) + err = dataBase.DeleteTeam(teamToDeleteID, userOfTeamToDeleteID) So(err, ShouldBeNil) + actualTeam, err = dataBase.GetTeam(teamToDeleteID) So(err, ShouldResemble, database.ErrNil) So(actualTeam, ShouldResemble, moira.Team{}) + + actualTeam, err = dataBase.GetTeamByName(teamToDelete.Name) + So(err, ShouldResemble, database.ErrNil) + So(actualTeam, ShouldResemble, moira.Team{}) + actualTeams, err = dataBase.GetUserTeams(userOfTeamToDeleteID) So(err, ShouldBeNil) So(actualTeams, ShouldHaveLength, 0) + actualUsers, err = dataBase.GetTeamUsers(teamToDeleteID) So(err, ShouldBeNil) So(actualUsers, ShouldHaveLength, 0) }) } + +func TestSaveAndGetTeam(t *testing.T) { + Convey("Test saving team", t, func() { + logger, _ := logging.GetLogger("dataBase") + dataBase := NewTestDatabase(logger) + dataBase.Flush() + defer dataBase.Flush() + + team := moira.Team{ + ID: "someTeamID", + Name: "Test team name", + Description: "Test description", + } + + Convey("when no team, get returns database.ErrNil", func() { + gotTeam, err := dataBase.GetTeam(team.ID) + + So(err, ShouldResemble, database.ErrNil) + So(gotTeam, ShouldResemble, moira.Team{}) + }) + + Convey("when no team, get by name returns database.ErrNil", func() { + gotTeam, err := dataBase.GetTeamByName(team.Name) + + So(err, ShouldResemble, database.ErrNil) + So(gotTeam, ShouldResemble, moira.Team{}) + }) + + Convey("when no team with such name, saved ok", func() { + err := dataBase.SaveTeam(team.ID, team) + + So(err, ShouldBeNil) + + Convey("and getting team by id returns saved team", func() { + gotTeam, err := dataBase.GetTeam(team.ID) + + So(err, ShouldBeNil) + So(gotTeam, ShouldResemble, team) + }) + + Convey("and getting team by name returns saved team", func() { + gotTeam, err := dataBase.GetTeamByName(team.Name) + + So(err, ShouldBeNil) + So(gotTeam, ShouldResemble, team) + }) + + Convey("and updating team ok", func() { + team.Name = strings.ToUpper(team.Name) + + err := dataBase.SaveTeam(team.ID, team) + So(err, ShouldBeNil) + + gotTeam, err := dataBase.GetTeam(team.ID) + So(err, ShouldBeNil) + So(gotTeam, ShouldResemble, team) + + gotTeam, err = dataBase.GetTeamByName(team.Name) + So(err, ShouldBeNil) + So(gotTeam, ShouldResemble, team) + }) + }) + + Convey("with changing name of existing team", func() { + err := dataBase.SaveTeam(team.ID, team) + So(err, ShouldBeNil) + + otherTeam := moira.Team{ + ID: "otherTeamID", + Name: "Other team name", + Description: "others description", + } + + err = dataBase.SaveTeam(otherTeam.ID, otherTeam) + So(err, ShouldBeNil) + + prevName := otherTeam.Name + + Convey("to name of existed team (no matter upper/lower-case) returns error", func() { + otherTeam.Name = team.Name + + err = dataBase.SaveTeam(otherTeam.ID, otherTeam) + So(err, ShouldResemble, database.ErrTeamWithNameAlreadyExists) + + otherTeam.Name = strings.ToLower(team.Name) + + err = dataBase.SaveTeam(otherTeam.ID, otherTeam) + So(err, ShouldResemble, database.ErrTeamWithNameAlreadyExists) + }) + + Convey("to new name, no team with prev name exist", func() { + otherTeam.Name = team.Name + "1" + + err = dataBase.SaveTeam(otherTeam.ID, otherTeam) + So(err, ShouldBeNil) + + gotTeam, err := dataBase.GetTeam(otherTeam.ID) + So(err, ShouldBeNil) + So(gotTeam, ShouldResemble, otherTeam) + + gotTeam, err = dataBase.GetTeamByName(prevName) + So(err, ShouldResemble, database.ErrNil) + So(gotTeam, ShouldResemble, moira.Team{}) + + gotTeam, err = dataBase.GetTeamByName(otherTeam.Name) + So(err, ShouldBeNil) + So(gotTeam, ShouldResemble, otherTeam) + }) + }) + }) +} diff --git a/interfaces.go b/interfaces.go index 94fc776bd..137d96afc 100644 --- a/interfaces.go +++ b/interfaces.go @@ -143,6 +143,7 @@ type Database interface { // Teams management SaveTeam(teamID string, team Team) error GetTeam(teamID string) (Team, error) + GetTeamByName(name string) (Team, error) SaveTeamsAndUsers(teamID string, users []string, usersTeams map[string][]string) error GetUserTeams(userID string) ([]string, error) GetTeamUsers(teamID string) ([]string, error) diff --git a/local/api.yml b/local/api.yml index d852a09b5..1f47563d3 100644 --- a/local/api.yml +++ b/local/api.yml @@ -53,6 +53,9 @@ api: limits: trigger: max_name_size: 200 + team: + max_name_size: 100 + max_description_size: 1000 web: contacts_template: - type: mail diff --git a/mock/moira-alert/database.go b/mock/moira-alert/database.go index a9953446c..2eddd945a 100644 --- a/mock/moira-alert/database.go +++ b/mock/moira-alert/database.go @@ -715,6 +715,21 @@ func (mr *MockDatabaseMockRecorder) GetTeam(arg0 any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetTeam", reflect.TypeOf((*MockDatabase)(nil).GetTeam), arg0) } +// GetTeamByName mocks base method. +func (m *MockDatabase) GetTeamByName(arg0 string) (moira.Team, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetTeamByName", arg0) + ret0, _ := ret[0].(moira.Team) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetTeamByName indicates an expected call of GetTeamByName. +func (mr *MockDatabaseMockRecorder) GetTeamByName(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetTeamByName", reflect.TypeOf((*MockDatabase)(nil).GetTeamByName), arg0) +} + // GetTeamContactIDs mocks base method. func (m *MockDatabase) GetTeamContactIDs(arg0 string) ([]string, error) { m.ctrl.T.Helper()