diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f987eca45c..34bcb21e25c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ 1. [15592](https://github.com/influxdata/influxdb/pull/15592): Changed task runs success status code from 200 to 201 to match Swagger documentation. 1. [15634](https://github.com/influxdata/influxdb/pull/15634): TextAreas have the correct height 1. [15647](https://github.com/influxdata/influxdb/pull/15647): Ensures labels are unique by organization in the kv store +1. [15695](https://github.com/influxdata/influxdb/pull/15695): Ensures variable names are unique by organization ## v2.0.0-alpha.18 [2019-09-26] diff --git a/bolt/variable_test.go b/bolt/variable_test.go deleted file mode 100644 index f8e1e15b79d..00000000000 --- a/bolt/variable_test.go +++ /dev/null @@ -1,45 +0,0 @@ -package bolt_test - -import ( - "context" - platform "github.com/influxdata/influxdb" - "github.com/influxdata/influxdb/bolt" - "github.com/influxdata/influxdb/inmem" - platformtesting "github.com/influxdata/influxdb/testing" - "testing" -) - -func initVariableService(f platformtesting.VariableFields, t *testing.T) (platform.VariableService, string, func()) { - c := inmem.NewService() - - if f.TimeGenerator == nil { - c.TimeGenerator = platform.RealTimeGenerator{} - } - c.IDGenerator = f.IDGenerator - c.TimeGenerator = f.TimeGenerator - if f.TimeGenerator == nil { - c.TimeGenerator = platform.RealTimeGenerator{} - } - - ctx := context.Background() - for _, variable := range f.Variables { - if err := c.ReplaceVariable(ctx, variable); err != nil { - t.Fatalf("failed to populate test variables: %v", err) - } - } - - done := func() { - for _, variable := range f.Variables { - if err := c.DeleteVariable(ctx, variable.ID); err != nil { - t.Fatalf("failed to clean up variables bolt test: %v", err) - } - } - } - - return c, bolt.OpPrefix, done -} - -func TestVariableService(t *testing.T) { - t.Parallel() - platformtesting.VariableService(initVariableService, t) -} diff --git a/http/variable_test.go b/http/variable_test.go index 3e6a03952d0..c8dbdd79513 100644 --- a/http/variable_test.go +++ b/http/variable_test.go @@ -14,7 +14,6 @@ import ( "go.uber.org/zap" platform "github.com/influxdata/influxdb" - "github.com/influxdata/influxdb/inmem" "github.com/influxdata/influxdb/mock" platformtesting "github.com/influxdata/influxdb/testing" "github.com/julienschmidt/httprouter" @@ -887,37 +886,3 @@ func TestService_handlePostVariableLabel(t *testing.T) { }) } } - -func initVariableService(f platformtesting.VariableFields, t *testing.T) (platform.VariableService, string, func()) { - t.Helper() - - svc := inmem.NewService() - svc.IDGenerator = f.IDGenerator - svc.TimeGenerator = f.TimeGenerator - if f.TimeGenerator == nil { - svc.TimeGenerator = platform.RealTimeGenerator{} - } - - ctx := context.Background() - for _, variable := range f.Variables { - if err := svc.ReplaceVariable(ctx, variable); err != nil { - t.Fatalf("failed to populate variables") - } - } - - variableBackend := NewMockVariableBackend() - variableBackend.HTTPErrorHandler = ErrorHandler(0) - variableBackend.VariableService = svc - handler := NewVariableHandler(variableBackend) - server := httptest.NewServer(handler) - client := VariableService{ - Addr: server.URL, - } - done := server.Close - - return &client, inmem.OpPrefix, done -} - -func TestVariableService(t *testing.T) { - platformtesting.VariableService(initVariableService, t) -} diff --git a/inmem/variable_test.go b/inmem/variable_test.go deleted file mode 100644 index 86897c8913d..00000000000 --- a/inmem/variable_test.go +++ /dev/null @@ -1,35 +0,0 @@ -package inmem - -import ( - "context" - "testing" - - platform "github.com/influxdata/influxdb" - platformtesting "github.com/influxdata/influxdb/testing" -) - -func initVariableService(f platformtesting.VariableFields, t *testing.T) (platform.VariableService, string, func()) { - s := NewService() - s.IDGenerator = f.IDGenerator - s.TimeGenerator = f.TimeGenerator - - ctx := context.TODO() - for _, variable := range f.Variables { - if err := s.ReplaceVariable(ctx, variable); err != nil { - t.Fatalf("failed to populate variables") - } - } - - done := func() { - for _, variable := range f.Variables { - if err := s.DeleteVariable(ctx, variable.ID); err != nil { - t.Fatalf("failed to clean up variables bolt test: %v", err) - } - } - } - return s, OpPrefix, done -} - -func TestVariableService(t *testing.T) { - platformtesting.VariableService(initVariableService, t) -} diff --git a/kv/variable.go b/kv/variable.go index 5d18b0469a5..29553e7e9c3 100644 --- a/kv/variable.go +++ b/kv/variable.go @@ -4,6 +4,8 @@ import ( "bytes" "context" "encoding/json" + "fmt" + "strings" "github.com/influxdata/influxdb" ) @@ -11,6 +13,7 @@ import ( var ( variableBucket = []byte("variablesv1") variableOrgsIndex = []byte("variableorgsv1") + variablesIndex = []byte("variablesindexv1") ) func (s *Service) initializeVariables(ctx context.Context, tx Tx) error { @@ -237,6 +240,19 @@ func (s *Service) findVariableByID(ctx context.Context, tx Tx, id influxdb.ID) ( // CreateVariable creates a new variable and assigns it an ID func (s *Service) CreateVariable(ctx context.Context, variable *influxdb.Variable) error { return s.kv.Update(ctx, func(tx Tx) error { + if err := variable.Valid(); err != nil { + return &influxdb.Error{ + Code: influxdb.EInvalid, + Err: err, + } + } + + variable.Name = strings.TrimSpace(variable.Name) + + if err := s.uniqueVariableName(ctx, tx, variable); err != nil { + return err + } + variable.ID = s.IDGenerator.ID() if err := s.putVariableOrgsIndex(ctx, tx, variable); err != nil { @@ -263,6 +279,14 @@ func (s *Service) ReplaceVariable(ctx context.Context, variable *influxdb.Variab Err: err, } } + + err := s.uniqueVariableName(ctx, tx, variable) + if err != nil { + return &influxdb.Error{ + Err: err, + } + } + return s.putVariable(ctx, tx, variable) }) } @@ -345,6 +369,13 @@ func (s *Service) putVariable(ctx context.Context, tx Tx, variable *influxdb.Var } } + err = s.createVariableIndex(ctx, tx, variable) + if err != nil { + return &influxdb.Error{ + Err: err, + } + } + b, err := tx.Bucket(variableBucket) if err != nil { return err @@ -363,23 +394,43 @@ func (s *Service) putVariable(ctx context.Context, tx Tx, variable *influxdb.Var func (s *Service) UpdateVariable(ctx context.Context, id influxdb.ID, update *influxdb.VariableUpdate) (*influxdb.Variable, error) { var variable *influxdb.Variable err := s.kv.Update(ctx, func(tx Tx) error { - m, pe := s.findVariableByID(ctx, tx, id) - if pe != nil { + m, err := s.findVariableByID(ctx, tx, id) + if err != nil { return &influxdb.Error{ - Err: pe, + Err: err, } } m.UpdatedAt = s.Now() + + variable = m + + if update.Name != "" { + update.Name = strings.TrimSpace(update.Name) + + err = s.deleteVariableIndex(ctx, tx, variable) + if err != nil { + return &influxdb.Error{ + Err: err, + } + } + + variable.Name = update.Name + if err := s.uniqueVariableName(ctx, tx, variable); err != nil { + return &influxdb.Error{ + Err: err, + } + } + } + if err := update.Apply(m); err != nil { return &influxdb.Error{ Err: err, } } - variable = m - if pe = s.putVariable(ctx, tx, variable); pe != nil { + if err = s.putVariable(ctx, tx, variable); err != nil { return &influxdb.Error{ - Err: pe, + Err: err, } } return nil @@ -391,10 +442,10 @@ func (s *Service) UpdateVariable(ctx context.Context, id influxdb.ID, update *in // DeleteVariable removes a single variable from the store by its ID func (s *Service) DeleteVariable(ctx context.Context, id influxdb.ID) error { return s.kv.Update(ctx, func(tx Tx) error { - m, pe := s.findVariableByID(ctx, tx, id) - if pe != nil { + v, err := s.findVariableByID(ctx, tx, id) + if err != nil { return &influxdb.Error{ - Err: pe, + Err: err, } } @@ -405,7 +456,7 @@ func (s *Service) DeleteVariable(ctx context.Context, id influxdb.ID) error { } } - if err := s.removeVariableOrgsIndex(ctx, tx, m); err != nil { + if err := s.removeVariableOrgsIndex(ctx, tx, v); err != nil { return &influxdb.Error{ Err: err, } @@ -416,6 +467,12 @@ func (s *Service) DeleteVariable(ctx context.Context, id influxdb.ID) error { return err } + if err := s.deleteVariableIndex(ctx, tx, v); err != nil { + return &influxdb.Error{ + Err: err, + } + } + if err := b.Delete(encID); err != nil { return &influxdb.Error{ Err: err, @@ -425,3 +482,93 @@ func (s *Service) DeleteVariable(ctx context.Context, id influxdb.ID) error { return nil }) } + +func variableAlreadyExistsError(v *influxdb.Variable) error { + return &influxdb.Error{ + Code: influxdb.EConflict, + Msg: fmt.Sprintf("variable with name %s already exists", v.Name), + } +} + +func variableIndexKey(v *influxdb.Variable) ([]byte, error) { + orgID, err := v.OrganizationID.Encode() + if err != nil { + return nil, &influxdb.Error{ + Code: influxdb.EInvalid, + Err: err, + } + } + + k := make([]byte, influxdb.IDLength+len(v.Name)) + copy(k, orgID) + copy(k[influxdb.IDLength:], []byte(strings.ToLower((v.Name)))) + return k, nil +} + +func (s *Service) deleteVariableIndex(ctx context.Context, tx Tx, v *influxdb.Variable) error { + idx, err := tx.Bucket(variablesIndex) + if err != nil { + return &influxdb.Error{ + Err: err, + } + } + + idxKey, err := variableIndexKey(v) + if err != nil { + return &influxdb.Error{ + Err: err, + } + } + + if err := idx.Delete(idxKey); err != nil { + return &influxdb.Error{ + Err: err, + } + } + return nil +} + +func (s *Service) createVariableIndex(ctx context.Context, tx Tx, v *influxdb.Variable) error { + encID, err := v.OrganizationID.Encode() + if err != nil { + return &influxdb.Error{ + Err: err, + } + } + + idxBkt, err := tx.Bucket(variablesIndex) + if err != nil { + return &influxdb.Error{ + Err: err, + } + } + + idxKey, err := variableIndexKey(v) + if err != nil { + return &influxdb.Error{ + Err: err, + } + } + + if err := idxBkt.Put([]byte(idxKey), encID); err != nil { + return &influxdb.Error{ + Err: err, + } + } + + return nil +} + +func (s *Service) uniqueVariableName(ctx context.Context, tx Tx, v *influxdb.Variable) error { + key, err := variableIndexKey(v) + if err != nil { + return err + } + + err = s.unique(ctx, tx, variablesIndex, key) + if err == NotUniqueError { + return variableAlreadyExistsError(v) + } + + return nil +} diff --git a/testing/variable.go b/testing/variable.go index 9a511aca415..4d130cb2ab3 100644 --- a/testing/variable.go +++ b/testing/variable.go @@ -216,6 +216,234 @@ func CreateVariable(init func(VariableFields, *testing.T) (platform.VariableServ }, }, }, + { + name: "cant create a new variable with a name that exists", + fields: VariableFields{ + IDGenerator: &mock.IDGenerator{ + IDFn: func() platform.ID { + return MustIDBase16(idA) + }, + }, + TimeGenerator: fakeGenerator, + Variables: []*platform.Variable{ + { + ID: MustIDBase16(idB), + OrganizationID: MustIDBase16(idD), + Name: "existing-variable", + Selected: []string{"b"}, + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{"b"}, + }, + }, + }, + }, + args: args{ + variable: &platform.Variable{ + ID: MustIDBase16(idA), + OrganizationID: MustIDBase16(idD), + Name: "existing-variable", + Selected: []string{"a"}, + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{"a"}, + }, + CRUDLog: platform.CRUDLog{ + CreatedAt: fakeDate, + UpdatedAt: fakeDate, + }, + }, + }, + wants: wants{ + err: &platform.Error{ + Code: platform.EConflict, + Msg: "variable with name existing-variable already exists", + }, + variables: []*platform.Variable{ + { + ID: MustIDBase16(idB), + OrganizationID: MustIDBase16(idD), + Name: "existing-variable", + Selected: []string{"b"}, + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{"b"}, + }, + }, + }, + }, + }, + { + name: "variable names should be unique and case-insensitive", + fields: VariableFields{ + IDGenerator: &mock.IDGenerator{ + IDFn: func() platform.ID { + return MustIDBase16(idA) + }, + }, + TimeGenerator: fakeGenerator, + Variables: []*platform.Variable{ + { + ID: MustIDBase16(idB), + OrganizationID: platform.ID(3), + Name: "existing-variable", + Selected: []string{"b"}, + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{"b"}, + }, + }, + }, + }, + args: args{ + variable: &platform.Variable{ + ID: MustIDBase16(idA), + OrganizationID: platform.ID(3), + Name: "EXISTING-variable", + Selected: []string{"a"}, + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{"a"}, + }, + CRUDLog: platform.CRUDLog{ + CreatedAt: fakeDate, + UpdatedAt: fakeDate, + }, + }, + }, + wants: wants{ + err: &platform.Error{ + Code: platform.EConflict, + Msg: "variable with name EXISTING-variable already exists", + }, + variables: []*platform.Variable{ + { + ID: MustIDBase16(idB), + OrganizationID: platform.ID(3), + Name: "existing-variable", + Selected: []string{"b"}, + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{"b"}, + }, + }, + }, + }, + }, + { + name: "cant create a new variable when variable name exists with a different type", + fields: VariableFields{ + IDGenerator: &mock.IDGenerator{ + IDFn: func() platform.ID { + return MustIDBase16(idA) + }, + }, + TimeGenerator: fakeGenerator, + Variables: []*platform.Variable{ + { + ID: MustIDBase16(idB), + OrganizationID: platform.ID(3), + Name: "existing-variable", + Selected: []string{"b"}, + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{"b"}, + }, + }, + }, + }, + args: args{ + variable: &platform.Variable{ + ID: MustIDBase16(idA), + OrganizationID: platform.ID(3), + Name: "existing-variable", + Selected: []string{"a"}, + Arguments: &platform.VariableArguments{ + Type: "query", + Values: platform.VariableConstantValues{"a"}, + }, + CRUDLog: platform.CRUDLog{ + CreatedAt: fakeDate, + UpdatedAt: fakeDate, + }, + }, + }, + wants: wants{ + err: &platform.Error{ + Code: platform.EConflict, + Msg: "variable with name existing-variable already exists", + }, + variables: []*platform.Variable{ + { + ID: MustIDBase16(idB), + OrganizationID: platform.ID(3), + Name: "existing-variable", + Selected: []string{"b"}, + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{"b"}, + }, + }, + }, + }, + }, + { + name: "trims white space, but fails when variable name already exists", + fields: VariableFields{ + IDGenerator: &mock.IDGenerator{ + IDFn: func() platform.ID { + return MustIDBase16(idA) + }, + }, + TimeGenerator: fakeGenerator, + Variables: []*platform.Variable{ + { + ID: MustIDBase16(idB), + OrganizationID: platform.ID(3), + Name: "existing-variable", + Selected: []string{"b"}, + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{"b"}, + }, + }, + }, + }, + args: args{ + variable: &platform.Variable{ + ID: MustIDBase16(idA), + OrganizationID: platform.ID(3), + Name: " existing-variable ", + Selected: []string{"a"}, + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{"a"}, + }, + CRUDLog: platform.CRUDLog{ + CreatedAt: fakeDate, + UpdatedAt: fakeDate, + }, + }, + }, + wants: wants{ + err: &platform.Error{ + Code: platform.EConflict, + Msg: "variable with name existing-variable already exists", + }, + variables: []*platform.Variable{ + { + ID: MustIDBase16(idB), + OrganizationID: platform.ID(3), + Name: "existing-variable", + Selected: []string{"b"}, + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{"b"}, + }, + }, + }, + }, + }, } for _, tt := range tests { @@ -654,6 +882,156 @@ func UpdateVariable(init func(VariableFields, *testing.T) (platform.VariableServ variables: []*platform.Variable{}, }, }, + { + name: "updating fails when variable name already exists", + fields: VariableFields{ + TimeGenerator: fakeGenerator, + Variables: []*platform.Variable{ + { + ID: MustIDBase16(idA), + OrganizationID: platform.ID(7), + Name: "variable-a", + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{}, + }, + CRUDLog: platform.CRUDLog{ + CreatedAt: oldFakeDate, + UpdatedAt: fakeDate, + }, + }, + { + ID: MustIDBase16(idB), + OrganizationID: platform.ID(7), + Name: "variable-b", + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{}, + }, + CRUDLog: platform.CRUDLog{ + CreatedAt: oldFakeDate, + UpdatedAt: fakeDate, + }, + }, + }, + }, + args: args{ + id: MustIDBase16(idB), + update: &platform.VariableUpdate{ + Name: "variable-a", + }, + }, + wants: wants{ + err: &platform.Error{ + Code: platform.EConflict, + Op: platform.OpUpdateVariable, + Msg: "variable with name variable-a already exists", + }, + variables: []*platform.Variable{ + { + ID: MustIDBase16(idA), + OrganizationID: platform.ID(7), + Name: "variable-a", + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{}, + }, + CRUDLog: platform.CRUDLog{ + CreatedAt: oldFakeDate, + UpdatedAt: fakeDate, + }, + }, + { + ID: MustIDBase16(idB), + OrganizationID: platform.ID(7), + Name: "variable-b", + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{}, + }, + CRUDLog: platform.CRUDLog{ + CreatedAt: oldFakeDate, + UpdatedAt: fakeDate, + }, + }, + }, + }, + }, + { + name: "trims the variable name, but updating fails when variable name already exists", + fields: VariableFields{ + TimeGenerator: fakeGenerator, + Variables: []*platform.Variable{ + { + ID: MustIDBase16(idA), + OrganizationID: platform.ID(7), + Name: "variable-a", + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{}, + }, + CRUDLog: platform.CRUDLog{ + CreatedAt: oldFakeDate, + UpdatedAt: fakeDate, + }, + }, + { + ID: MustIDBase16(idB), + OrganizationID: platform.ID(7), + Name: "variable-b", + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{}, + }, + CRUDLog: platform.CRUDLog{ + CreatedAt: oldFakeDate, + UpdatedAt: fakeDate, + }, + }, + }, + }, + args: args{ + id: MustIDBase16(idB), + update: &platform.VariableUpdate{ + Name: " variable-a ", + }, + }, + wants: wants{ + err: &platform.Error{ + Code: platform.EConflict, + Op: platform.OpUpdateVariable, + Msg: "variable with name variable-a already exists", + }, + variables: []*platform.Variable{ + { + ID: MustIDBase16(idA), + OrganizationID: platform.ID(7), + Name: "variable-a", + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{}, + }, + CRUDLog: platform.CRUDLog{ + CreatedAt: oldFakeDate, + UpdatedAt: fakeDate, + }, + }, + { + ID: MustIDBase16(idB), + OrganizationID: platform.ID(7), + Name: "variable-b", + Arguments: &platform.VariableArguments{ + Type: "constant", + Values: platform.VariableConstantValues{}, + }, + CRUDLog: platform.CRUDLog{ + CreatedAt: oldFakeDate, + UpdatedAt: fakeDate, + }, + }, + }, + }, + }, } for _, tt := range tests {