From 1730d94dcfe938557c22069ba509c320264cee3e Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Mon, 25 Nov 2019 20:38:27 -0500 Subject: [PATCH 01/12] neq empty string and evaluation logic --- storage/evaluator.go | 15 ++++++-- storage/evaluator_test.go | 78 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 5 deletions(-) diff --git a/storage/evaluator.go b/storage/evaluator.go index 9649b344d1..7730f3d1d0 100644 --- a/storage/evaluator.go +++ b/storage/evaluator.go @@ -424,14 +424,21 @@ func matchesString(c constraint, v string) bool { value := c.Value switch c.Operator { - case opEQ: - return value == v - case opNEQ: - return value != v case opEmpty: return len(strings.TrimSpace(v)) == 0 case opNotEmpty: return len(strings.TrimSpace(v)) != 0 + } + + if v == "" { + return false + } + + switch c.Operator { + case opEQ: + return value == v + case opNEQ: + return value != v case opPrefix: return strings.HasPrefix(strings.TrimSpace(v), value) case opSuffix: diff --git a/storage/evaluator_test.go b/storage/evaluator_test.go index 8ffcf485cc..afab6bef34 100644 --- a/storage/evaluator_test.go +++ b/storage/evaluator_test.go @@ -773,6 +773,16 @@ func Test_matchesString(t *testing.T) { value: "bar", wantMatch: true, }, + { + name: "eq whitespace value", + constraint: constraint{ + Property: "foo", + Operator: "eq", + Value: " ", + }, + value: " ", + wantMatch: true, + }, { name: "negative eq", constraint: constraint{ @@ -782,6 +792,23 @@ func Test_matchesString(t *testing.T) { }, value: "baz", }, + { + name: "negative eq empty value", + constraint: constraint{ + Property: "foo", + Operator: "eq", + Value: "bar", + }, + }, + { + name: "negative eq whitespace value", + constraint: constraint{ + Property: "foo", + Operator: "eq", + Value: "bar", + }, + value: " ", + }, { name: "neq", constraint: constraint{ @@ -792,6 +819,24 @@ func Test_matchesString(t *testing.T) { value: "baz", wantMatch: true, }, + { + name: "neq whitespace value", + constraint: constraint{ + Property: "foo", + Operator: "neq", + Value: "bar", + }, + value: " ", + wantMatch: true, + }, + { + name: "neq empty value", + constraint: constraint{ + Property: "foo", + Operator: "neq", + Value: "bar", + }, + }, { name: "negative neq", constraint: constraint{ @@ -807,6 +852,14 @@ func Test_matchesString(t *testing.T) { Property: "foo", Operator: "empty", }, + wantMatch: true, + }, + { + name: "empty whitespace", + constraint: constraint{ + Property: "foo", + Operator: "empty", + }, value: " ", wantMatch: true, }, @@ -833,7 +886,14 @@ func Test_matchesString(t *testing.T) { Property: "foo", Operator: "notempty", }, - value: "", + }, + { + name: "negative not empty whitespace", + constraint: constraint{ + Property: "foo", + Operator: "notempty", + }, + value: " ", }, { name: "unknown operator", @@ -863,6 +923,14 @@ func Test_matchesString(t *testing.T) { }, value: "nope", }, + { + name: "negative prefix empty value", + constraint: constraint{ + Property: "foo", + Operator: "prefix", + Value: "bar", + }, + }, { name: "suffix", constraint: constraint{ @@ -882,6 +950,14 @@ func Test_matchesString(t *testing.T) { }, value: "nope", }, + { + name: "negative suffix empty value", + constraint: constraint{ + Property: "foo", + Operator: "suffix", + Value: "bar", + }, + }, } for _, tt := range tests { var ( From 782ee48ffeefbcd0f741b5dbbc56e1484ed1d2f1 Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Tue, 26 Nov 2019 09:48:11 -0500 Subject: [PATCH 02/12] WIP --- config/local.yml | 7 +++++++ server/segment.go | 4 ++++ storage/evaluator.go | 4 ++-- ui/src/components/Segments/Segment.vue | 29 +++++++++++++++++++++++--- ui/src/services/api.js | 7 ++++++- 5 files changed, 45 insertions(+), 6 deletions(-) diff --git a/config/local.yml b/config/local.yml index 944b8aed24..7138eca26c 100644 --- a/config/local.yml +++ b/config/local.yml @@ -5,3 +5,10 @@ db: url: file:flipt.db migrations: path: ./config/migrations + +ui: + enabled: false + +cors: + enabled: true + allowed_origins: "*" diff --git a/server/segment.go b/server/segment.go index 62fbeca99c..cc2a8281f0 100644 --- a/server/segment.go +++ b/server/segment.go @@ -85,6 +85,8 @@ func (s *Server) CreateConstraint(ctx context.Context, req *flipt.CreateConstrai return nil, emptyFieldError("operator") } + // TODO: test for empty value if operator ! [EMPTY, NOT_EMPTY, PRESENT, NOT_PRESENT] + return s.SegmentStore.CreateConstraint(ctx, req) } @@ -106,6 +108,8 @@ func (s *Server) UpdateConstraint(ctx context.Context, req *flipt.UpdateConstrai return nil, emptyFieldError("operator") } + // TODO: test for empty value if operator ! [EMPTY, NOT_EMPTY, PRESENT, NOT_PRESENT] + return s.SegmentStore.UpdateConstraint(ctx, req) } diff --git a/storage/evaluator.go b/storage/evaluator.go index 7730f3d1d0..a088993333 100644 --- a/storage/evaluator.go +++ b/storage/evaluator.go @@ -421,8 +421,6 @@ func validate(c constraint) error { } func matchesString(c constraint, v string) bool { - value := c.Value - switch c.Operator { case opEmpty: return len(strings.TrimSpace(v)) == 0 @@ -434,6 +432,8 @@ func matchesString(c constraint, v string) bool { return false } + value := c.Value + switch c.Operator { case opEQ: return value == v diff --git a/ui/src/components/Segments/Segment.vue b/ui/src/components/Segments/Segment.vue index c8313ee5fa..9de8bee5c4 100644 --- a/ui/src/components/Segments/Segment.vue +++ b/ui/src/components/Segments/Segment.vue @@ -164,7 +164,11 @@ - +
@@ -248,6 +252,7 @@
@@ -396,10 +401,28 @@ export default { return this.segment.key && this.segment.name; }, canAddConstraint() { - return this.newConstraint.property && this.newConstraint.type; + let valid = + this.newConstraint.property && + this.newConstraint.type && + this.newConstraint.operator; + + if (this.hasValue(this.newConstraint.operator)) { + return valid && this.newConstraint.value; + } + + return valid; }, canUpdateConstraint() { - return this.selectedConstraint.property && this.selectedConstraint.type; + let valid = + this.selectedConstraint.property && + this.selectedConstraint.type && + this.selectedConstraint.operator; + + if (this.hasValue(this.selectedConstraint.operator)) { + return valid && this.selectedConstraint.value; + } + + return valid; } }, mounted() { diff --git a/ui/src/services/api.js b/ui/src/services/api.js index b1046445cc..2d6da14fc3 100644 --- a/ui/src/services/api.js +++ b/ui/src/services/api.js @@ -1,5 +1,10 @@ import axios from "axios"; +let host = + process.env.NODE_ENV === "production" + ? window.location.host + : "localhost:8080"; + export const Api = axios.create({ - baseURL: "//" + window.location.host + "/api/v1/" + baseURL: "//" + host + "/api/v1/" }); From 569e90bba1a1d591cc9f84863cecc0ade07bb196 Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Tue, 26 Nov 2019 14:01:04 -0500 Subject: [PATCH 03/12] Move errors to own package --- errors/errors.go | 55 ++++++++++++++++++++++++++++++++++++++ rpc/validation.go | 3 +++ server/errors.go | 23 ---------------- server/evaluator.go | 5 ++-- server/evaluator_test.go | 7 ++--- server/flag.go | 28 ++++++++++--------- server/flag_test.go | 29 ++++++++++---------- server/rule.go | 53 ++++++++++++++++++------------------ server/rule_test.go | 55 +++++++++++++++++++------------------- server/segment.go | 31 ++++++++++----------- server/segment_test.go | 33 ++++++++++++----------- server/server.go | 7 ++--- server/server_test.go | 20 +++++++------- storage/cache/flag_test.go | 5 ++-- storage/errors.go | 27 ------------------- storage/evaluator.go | 9 ++++--- storage/flag.go | 23 ++++++++-------- storage/rule.go | 13 ++++----- storage/segment.go | 31 ++++++++++----------- storage/segment_test.go | 25 ++++++++--------- 20 files changed, 252 insertions(+), 230 deletions(-) create mode 100644 errors/errors.go create mode 100644 rpc/validation.go delete mode 100644 server/errors.go delete mode 100644 storage/errors.go diff --git a/errors/errors.go b/errors/errors.go new file mode 100644 index 0000000000..7434e5533b --- /dev/null +++ b/errors/errors.go @@ -0,0 +1,55 @@ +package errors + +import ( + "errors" + "fmt" +) + +// New creates a new error with errors.New +func New(s string) error { + return errors.New(s) +} + +// ErrNotFound represents a not found error +type ErrNotFound string + +// ErrNotFoundf creates an ErrNotFound using a custom format +func ErrNotFoundf(format string, args ...interface{}) error { + return ErrNotFound(fmt.Sprintf(format, args...)) +} + +func (e ErrNotFound) Error() string { + return fmt.Sprintf("%s not found", string(e)) +} + +// ErrInvalid represents an invalid error +type ErrInvalid string + +// ErrInvalidf creates an ErrInvalid using a custom format +func ErrInvalidf(format string, args ...interface{}) error { + return ErrInvalid(fmt.Sprintf(format, args...)) +} + +func (e ErrInvalid) Error() string { + return string(e) +} + +// ErrValidation is a validation error for a specific field and reason +type ErrValidation struct { + field string + reason string +} + +func (e ErrValidation) Error() string { + return fmt.Sprintf("invalid field %s: %s", e.field, e.reason) +} + +// InvalidFieldError creates an ErrInvalidField for a specific field and reason +func InvalidFieldError(field, reason string) error { + return ErrValidation{field, reason} +} + +// EmptyFieldError creates an ErrInvalidField for an empty field +func EmptyFieldError(field string) error { + return InvalidFieldError(field, "must not be empty") +} diff --git a/rpc/validation.go b/rpc/validation.go new file mode 100644 index 0000000000..9e98dd8e7e --- /dev/null +++ b/rpc/validation.go @@ -0,0 +1,3 @@ +package flipt + + diff --git a/server/errors.go b/server/errors.go deleted file mode 100644 index 3e8a6fa9d9..0000000000 --- a/server/errors.go +++ /dev/null @@ -1,23 +0,0 @@ -package server - -import "fmt" - -// errInvalidField represents a validation error -type errInvalidField struct { - field string - reason string -} - -func (e errInvalidField) Error() string { - return fmt.Sprintf("invalid field %s: %s", e.field, e.reason) -} - -// invalidFieldError creates an errInvalidField for a specific field and reason -func invalidFieldError(field, reason string) error { - return errInvalidField{field, reason} -} - -// emptyFieldError creates an errInvalidField for an empty field -func emptyFieldError(field string) error { - return invalidFieldError(field, "must not be empty") -} diff --git a/server/evaluator.go b/server/evaluator.go index 53ae707e37..7894cede44 100644 --- a/server/evaluator.go +++ b/server/evaluator.go @@ -5,17 +5,18 @@ import ( "time" "github.com/gofrs/uuid" + "github.com/markphelps/flipt/errors" flipt "github.com/markphelps/flipt/rpc" ) // Evaluate evaluates a request for a given flag and entity func (s *Server) Evaluate(ctx context.Context, req *flipt.EvaluationRequest) (*flipt.EvaluationResponse, error) { if req.FlagKey == "" { - return nil, emptyFieldError("flagKey") + return nil, errors.EmptyFieldError("flagKey") } if req.EntityId == "" { - return nil, emptyFieldError("entityId") + return nil, errors.EmptyFieldError("entityId") } startTime := time.Now() diff --git a/server/evaluator_test.go b/server/evaluator_test.go index 51e8ac0d5f..55bdc293b2 100644 --- a/server/evaluator_test.go +++ b/server/evaluator_test.go @@ -2,9 +2,10 @@ package server import ( "context" - "errors" "testing" + "github.com/markphelps/flipt/errors" + flipt "github.com/markphelps/flipt/rpc" "github.com/markphelps/flipt/storage" "github.com/stretchr/testify/assert" @@ -59,7 +60,7 @@ func TestEvaluate(t *testing.T) { EntityId: r.EntityId, }, nil }, - wantErr: emptyFieldError("flagKey"), + wantErr: errors.EmptyFieldError("flagKey"), }, { name: "emptyEntityId", @@ -74,7 +75,7 @@ func TestEvaluate(t *testing.T) { EntityId: "", }, nil }, - wantErr: emptyFieldError("entityId"), + wantErr: errors.EmptyFieldError("entityId"), }, { name: "error test", diff --git a/server/flag.go b/server/flag.go index 7ca4a7aced..46dd5ec17d 100644 --- a/server/flag.go +++ b/server/flag.go @@ -3,6 +3,8 @@ package server import ( "context" + "github.com/markphelps/flipt/errors" + "github.com/golang/protobuf/ptypes/empty" flipt "github.com/markphelps/flipt/rpc" ) @@ -10,7 +12,7 @@ import ( // GetFlag gets a flag func (s *Server) GetFlag(ctx context.Context, req *flipt.GetFlagRequest) (*flipt.Flag, error) { if req.Key == "" { - return nil, emptyFieldError("key") + return nil, errors.EmptyFieldError("key") } return s.FlagStore.GetFlag(ctx, req) @@ -35,11 +37,11 @@ func (s *Server) ListFlags(ctx context.Context, req *flipt.ListFlagRequest) (*fl // CreateFlag creates a flag func (s *Server) CreateFlag(ctx context.Context, req *flipt.CreateFlagRequest) (*flipt.Flag, error) { if req.Key == "" { - return nil, emptyFieldError("key") + return nil, errors.EmptyFieldError("key") } if req.Name == "" { - return nil, emptyFieldError("name") + return nil, errors.EmptyFieldError("name") } return s.FlagStore.CreateFlag(ctx, req) @@ -48,11 +50,11 @@ func (s *Server) CreateFlag(ctx context.Context, req *flipt.CreateFlagRequest) ( // UpdateFlag updates an existing flag func (s *Server) UpdateFlag(ctx context.Context, req *flipt.UpdateFlagRequest) (*flipt.Flag, error) { if req.Key == "" { - return nil, emptyFieldError("key") + return nil, errors.EmptyFieldError("key") } if req.Name == "" { - return nil, emptyFieldError("name") + return nil, errors.EmptyFieldError("name") } return s.FlagStore.UpdateFlag(ctx, req) @@ -61,7 +63,7 @@ func (s *Server) UpdateFlag(ctx context.Context, req *flipt.UpdateFlagRequest) ( // DeleteFlag deletes a flag func (s *Server) DeleteFlag(ctx context.Context, req *flipt.DeleteFlagRequest) (*empty.Empty, error) { if req.Key == "" { - return nil, emptyFieldError("key") + return nil, errors.EmptyFieldError("key") } if err := s.FlagStore.DeleteFlag(ctx, req); err != nil { @@ -74,11 +76,11 @@ func (s *Server) DeleteFlag(ctx context.Context, req *flipt.DeleteFlagRequest) ( // CreateVariant creates a variant func (s *Server) CreateVariant(ctx context.Context, req *flipt.CreateVariantRequest) (*flipt.Variant, error) { if req.FlagKey == "" { - return nil, emptyFieldError("flagKey") + return nil, errors.EmptyFieldError("flagKey") } if req.Key == "" { - return nil, emptyFieldError("key") + return nil, errors.EmptyFieldError("key") } return s.FlagStore.CreateVariant(ctx, req) @@ -87,15 +89,15 @@ func (s *Server) CreateVariant(ctx context.Context, req *flipt.CreateVariantRequ // UpdateVariant updates an existing variant func (s *Server) UpdateVariant(ctx context.Context, req *flipt.UpdateVariantRequest) (*flipt.Variant, error) { if req.Id == "" { - return nil, emptyFieldError("id") + return nil, errors.EmptyFieldError("id") } if req.FlagKey == "" { - return nil, emptyFieldError("flagKey") + return nil, errors.EmptyFieldError("flagKey") } if req.Key == "" { - return nil, emptyFieldError("key") + return nil, errors.EmptyFieldError("key") } return s.FlagStore.UpdateVariant(ctx, req) @@ -104,11 +106,11 @@ func (s *Server) UpdateVariant(ctx context.Context, req *flipt.UpdateVariantRequ // DeleteVariant deletes a variant func (s *Server) DeleteVariant(ctx context.Context, req *flipt.DeleteVariantRequest) (*empty.Empty, error) { if req.Id == "" { - return nil, emptyFieldError("id") + return nil, errors.EmptyFieldError("id") } if req.FlagKey == "" { - return nil, emptyFieldError("flagKey") + return nil, errors.EmptyFieldError("flagKey") } if err := s.FlagStore.DeleteVariant(ctx, req); err != nil { diff --git a/server/flag_test.go b/server/flag_test.go index 62bcf1403c..62efa0ab4b 100644 --- a/server/flag_test.go +++ b/server/flag_test.go @@ -2,9 +2,10 @@ package server import ( "context" - "errors" "testing" + "github.com/markphelps/flipt/errors" + "github.com/golang/protobuf/ptypes/empty" flipt "github.com/markphelps/flipt/rpc" "github.com/markphelps/flipt/storage" @@ -91,7 +92,7 @@ func TestGetFlag(t *testing.T) { }, nil }, flag: nil, - wantErr: emptyFieldError("key"), + wantErr: errors.EmptyFieldError("key"), }, } @@ -233,7 +234,7 @@ func TestCreateFlag(t *testing.T) { Enabled: r.Enabled, }, nil }, - wantErr: emptyFieldError("key"), + wantErr: errors.EmptyFieldError("key"), }, { name: "emptyName", @@ -257,7 +258,7 @@ func TestCreateFlag(t *testing.T) { Enabled: r.Enabled, }, nil }, - wantErr: emptyFieldError("name"), + wantErr: errors.EmptyFieldError("name"), }, } @@ -342,7 +343,7 @@ func TestUpdateFlag(t *testing.T) { Enabled: r.Enabled, }, nil }, - wantErr: emptyFieldError("key"), + wantErr: errors.EmptyFieldError("key"), }, { name: "emptyName", @@ -366,7 +367,7 @@ func TestUpdateFlag(t *testing.T) { Enabled: r.Enabled, }, nil }, - wantErr: emptyFieldError("name"), + wantErr: errors.EmptyFieldError("name"), }, } @@ -420,7 +421,7 @@ func TestDeleteFlag(t *testing.T) { return nil }, - wantErr: emptyFieldError("key"), + wantErr: errors.EmptyFieldError("key"), }, { name: "error", @@ -516,7 +517,7 @@ func TestCreateVariant(t *testing.T) { Description: r.Description, }, nil }, - wantErr: emptyFieldError("flagKey"), + wantErr: errors.EmptyFieldError("flagKey"), }, { name: "emptyKey", @@ -540,7 +541,7 @@ func TestCreateVariant(t *testing.T) { Description: r.Description, }, nil }, - wantErr: emptyFieldError("key"), + wantErr: errors.EmptyFieldError("key"), }, } @@ -620,7 +621,7 @@ func TestUpdateVariant(t *testing.T) { Description: r.Description, }, nil }, - wantErr: emptyFieldError("id"), + wantErr: errors.EmptyFieldError("id"), }, { name: "emptyFlagKey", @@ -641,7 +642,7 @@ func TestUpdateVariant(t *testing.T) { Description: r.Description, }, nil }, - wantErr: emptyFieldError("flagKey"), + wantErr: errors.EmptyFieldError("flagKey"), }, { name: "emptyKey", @@ -662,7 +663,7 @@ func TestUpdateVariant(t *testing.T) { Description: r.Description, }, nil }, - wantErr: emptyFieldError("key"), + wantErr: errors.EmptyFieldError("key"), }, } @@ -718,7 +719,7 @@ func TestDeleteVariant(t *testing.T) { return nil }, - wantErr: emptyFieldError("id"), + wantErr: errors.EmptyFieldError("id"), }, { name: "emptyFlagKey", @@ -730,7 +731,7 @@ func TestDeleteVariant(t *testing.T) { return nil }, - wantErr: emptyFieldError("flagKey"), + wantErr: errors.EmptyFieldError("flagKey"), }, { name: "error", diff --git a/server/rule.go b/server/rule.go index f42f48cfbd..69b455b103 100644 --- a/server/rule.go +++ b/server/rule.go @@ -4,6 +4,7 @@ import ( "context" "github.com/golang/protobuf/ptypes/empty" + "github.com/markphelps/flipt/errors" flipt "github.com/markphelps/flipt/rpc" ) @@ -15,7 +16,7 @@ func (s *Server) GetRule(ctx context.Context, req *flipt.GetRuleRequest) (*flipt // ListRules lists all rules func (s *Server) ListRules(ctx context.Context, req *flipt.ListRuleRequest) (*flipt.RuleList, error) { if req.FlagKey == "" { - return nil, emptyFieldError("flagKey") + return nil, errors.EmptyFieldError("flagKey") } rules, err := s.RuleStore.ListRules(ctx, req) @@ -35,15 +36,15 @@ func (s *Server) ListRules(ctx context.Context, req *flipt.ListRuleRequest) (*fl // CreateRule creates a rule func (s *Server) CreateRule(ctx context.Context, req *flipt.CreateRuleRequest) (*flipt.Rule, error) { if req.FlagKey == "" { - return nil, emptyFieldError("flagKey") + return nil, errors.EmptyFieldError("flagKey") } if req.SegmentKey == "" { - return nil, emptyFieldError("segmentKey") + return nil, errors.EmptyFieldError("segmentKey") } if req.Rank <= 0 { - return nil, invalidFieldError("rank", "must be greater than 0") + return nil, errors.InvalidFieldError("rank", "must be greater than 0") } return s.RuleStore.CreateRule(ctx, req) @@ -52,15 +53,15 @@ func (s *Server) CreateRule(ctx context.Context, req *flipt.CreateRuleRequest) ( // UpdateRule updates an existing rule func (s *Server) UpdateRule(ctx context.Context, req *flipt.UpdateRuleRequest) (*flipt.Rule, error) { if req.Id == "" { - return nil, emptyFieldError("id") + return nil, errors.EmptyFieldError("id") } if req.FlagKey == "" { - return nil, emptyFieldError("flagKey") + return nil, errors.EmptyFieldError("flagKey") } if req.SegmentKey == "" { - return nil, emptyFieldError("segmentKey") + return nil, errors.EmptyFieldError("segmentKey") } return s.RuleStore.UpdateRule(ctx, req) @@ -69,11 +70,11 @@ func (s *Server) UpdateRule(ctx context.Context, req *flipt.UpdateRuleRequest) ( // DeleteRule deletes a rule func (s *Server) DeleteRule(ctx context.Context, req *flipt.DeleteRuleRequest) (*empty.Empty, error) { if req.Id == "" { - return nil, emptyFieldError("id") + return nil, errors.EmptyFieldError("id") } if req.FlagKey == "" { - return nil, emptyFieldError("flagKey") + return nil, errors.EmptyFieldError("flagKey") } if err := s.RuleStore.DeleteRule(ctx, req); err != nil { @@ -86,11 +87,11 @@ func (s *Server) DeleteRule(ctx context.Context, req *flipt.DeleteRuleRequest) ( // OrderRules orders rules func (s *Server) OrderRules(ctx context.Context, req *flipt.OrderRulesRequest) (*empty.Empty, error) { if req.FlagKey == "" { - return nil, emptyFieldError("flagKey") + return nil, errors.EmptyFieldError("flagKey") } if len(req.RuleIds) < 2 { - return nil, invalidFieldError("ruleIds", "must contain atleast 2 elements") + return nil, errors.InvalidFieldError("ruleIds", "must contain atleast 2 elements") } if err := s.RuleStore.OrderRules(ctx, req); err != nil { @@ -103,23 +104,23 @@ func (s *Server) OrderRules(ctx context.Context, req *flipt.OrderRulesRequest) ( // CreateDistribution creates a distribution func (s *Server) CreateDistribution(ctx context.Context, req *flipt.CreateDistributionRequest) (*flipt.Distribution, error) { if req.FlagKey == "" { - return nil, emptyFieldError("flagKey") + return nil, errors.EmptyFieldError("flagKey") } if req.RuleId == "" { - return nil, emptyFieldError("ruleId") + return nil, errors.EmptyFieldError("ruleId") } if req.VariantId == "" { - return nil, emptyFieldError("variantId") + return nil, errors.EmptyFieldError("variantId") } if req.Rollout < 0 { - return nil, invalidFieldError("rollout", "must be greater than or equal to '0'") + return nil, errors.InvalidFieldError("rollout", "must be greater than or equal to '0'") } if req.Rollout > 100 { - return nil, invalidFieldError("rollout", "must be less than or equal to '100'") + return nil, errors.InvalidFieldError("rollout", "must be less than or equal to '100'") } return s.RuleStore.CreateDistribution(ctx, req) @@ -128,27 +129,27 @@ func (s *Server) CreateDistribution(ctx context.Context, req *flipt.CreateDistri // UpdateDistribution updates an existing distribution func (s *Server) UpdateDistribution(ctx context.Context, req *flipt.UpdateDistributionRequest) (*flipt.Distribution, error) { if req.Id == "" { - return nil, emptyFieldError("id") + return nil, errors.EmptyFieldError("id") } if req.FlagKey == "" { - return nil, emptyFieldError("flagKey") + return nil, errors.EmptyFieldError("flagKey") } if req.RuleId == "" { - return nil, emptyFieldError("ruleId") + return nil, errors.EmptyFieldError("ruleId") } if req.VariantId == "" { - return nil, emptyFieldError("variantId") + return nil, errors.EmptyFieldError("variantId") } if req.Rollout < 0 { - return nil, invalidFieldError("rollout", "must be greater than or equal to '0'") + return nil, errors.InvalidFieldError("rollout", "must be greater than or equal to '0'") } if req.Rollout > 100 { - return nil, invalidFieldError("rollout", "must be less than or equal to '100'") + return nil, errors.InvalidFieldError("rollout", "must be less than or equal to '100'") } return s.RuleStore.UpdateDistribution(ctx, req) @@ -157,19 +158,19 @@ func (s *Server) UpdateDistribution(ctx context.Context, req *flipt.UpdateDistri // DeleteDistribution deletes a distribution func (s *Server) DeleteDistribution(ctx context.Context, req *flipt.DeleteDistributionRequest) (*empty.Empty, error) { if req.Id == "" { - return nil, emptyFieldError("id") + return nil, errors.EmptyFieldError("id") } if req.FlagKey == "" { - return nil, emptyFieldError("flagKey") + return nil, errors.EmptyFieldError("flagKey") } if req.RuleId == "" { - return nil, emptyFieldError("ruleId") + return nil, errors.EmptyFieldError("ruleId") } if req.VariantId == "" { - return nil, emptyFieldError("variantId") + return nil, errors.EmptyFieldError("variantId") } if err := s.RuleStore.DeleteDistribution(ctx, req); err != nil { diff --git a/server/rule_test.go b/server/rule_test.go index 1018b9fb2d..50d9c6f28b 100644 --- a/server/rule_test.go +++ b/server/rule_test.go @@ -2,9 +2,10 @@ package server import ( "context" - "errors" "testing" + "github.com/markphelps/flipt/errors" + "github.com/golang/protobuf/ptypes/empty" flipt "github.com/markphelps/flipt/rpc" "github.com/markphelps/flipt/storage" @@ -142,7 +143,7 @@ func TestListRules(t *testing.T) { {FlagKey: ""}, }, nil }, - wantErr: emptyFieldError("flagKey"), + wantErr: errors.EmptyFieldError("flagKey"), }, { name: "error test", @@ -231,7 +232,7 @@ func TestCreateRule(t *testing.T) { Rank: r.Rank, }, nil }, - wantErr: emptyFieldError("flagKey"), + wantErr: errors.EmptyFieldError("flagKey"), }, { name: "emptySegmentKey", @@ -252,7 +253,7 @@ func TestCreateRule(t *testing.T) { Rank: r.Rank, }, nil }, - wantErr: emptyFieldError("segmentKey"), + wantErr: errors.EmptyFieldError("segmentKey"), }, { name: "rank_lesser_than_0", @@ -273,7 +274,7 @@ func TestCreateRule(t *testing.T) { Rank: r.Rank, }, nil }, - wantErr: invalidFieldError("rank", "must be greater than 0"), + wantErr: errors.InvalidFieldError("rank", "must be greater than 0"), }, } @@ -351,7 +352,7 @@ func TestUpdateRule(t *testing.T) { SegmentKey: r.SegmentKey, }, nil }, - wantErr: emptyFieldError("id"), + wantErr: errors.EmptyFieldError("id"), }, { name: "emptyFlagKey", @@ -372,7 +373,7 @@ func TestUpdateRule(t *testing.T) { SegmentKey: r.SegmentKey, }, nil }, - wantErr: emptyFieldError("flagKey"), + wantErr: errors.EmptyFieldError("flagKey"), }, { name: "emptySegmentKey", @@ -393,7 +394,7 @@ func TestUpdateRule(t *testing.T) { SegmentKey: "", }, nil }, - wantErr: emptyFieldError("segmentKey"), + wantErr: errors.EmptyFieldError("segmentKey"), }, } @@ -447,7 +448,7 @@ func TestDeleteRule(t *testing.T) { assert.Equal(t, "flagKey", r.FlagKey) return nil }, - wantErr: emptyFieldError("id"), + wantErr: errors.EmptyFieldError("id"), }, { name: "emptyFlagKey", @@ -458,7 +459,7 @@ func TestDeleteRule(t *testing.T) { assert.Equal(t, "", r.FlagKey) return nil }, - wantErr: emptyFieldError("flagKey"), + wantErr: errors.EmptyFieldError("flagKey"), }, { name: "error test", @@ -523,7 +524,7 @@ func TestOrderRules(t *testing.T) { assert.Equal(t, []string{"1", "2"}, r.RuleIds) return nil }, - wantErr: emptyFieldError("flagKey"), + wantErr: errors.EmptyFieldError("flagKey"), }, { name: "ruleIds length lesser than 2", @@ -535,7 +536,7 @@ func TestOrderRules(t *testing.T) { assert.Equal(t, 1, len(r.RuleIds)) return nil }, - wantErr: invalidFieldError("ruleIds", "must contain atleast 2 elements"), + wantErr: errors.InvalidFieldError("ruleIds", "must contain atleast 2 elements"), }, { name: "error test", @@ -613,7 +614,7 @@ func TestCreateDistribution(t *testing.T) { VariantId: "variantID", }, nil }, - wantErr: emptyFieldError("flagKey"), + wantErr: errors.EmptyFieldError("flagKey"), }, { name: "emptyRuleID", @@ -629,7 +630,7 @@ func TestCreateDistribution(t *testing.T) { VariantId: "variantID", }, nil }, - wantErr: emptyFieldError("ruleId"), + wantErr: errors.EmptyFieldError("ruleId"), }, { name: "emptyVariantID", @@ -645,7 +646,7 @@ func TestCreateDistribution(t *testing.T) { VariantId: "", }, nil }, - wantErr: emptyFieldError("variantId"), + wantErr: errors.EmptyFieldError("variantId"), }, { name: "rollout is less than 0", @@ -661,7 +662,7 @@ func TestCreateDistribution(t *testing.T) { VariantId: "variantID", }, nil }, - wantErr: invalidFieldError("rollout", "must be greater than or equal to '0'"), + wantErr: errors.InvalidFieldError("rollout", "must be greater than or equal to '0'"), }, { name: "rollout is more than 100", @@ -677,7 +678,7 @@ func TestCreateDistribution(t *testing.T) { VariantId: "variantID", }, nil }, - wantErr: invalidFieldError("rollout", "must be less than or equal to '100'"), + wantErr: errors.InvalidFieldError("rollout", "must be less than or equal to '100'"), }, } @@ -749,7 +750,7 @@ func TestUpdateDistribution(t *testing.T) { VariantId: r.VariantId, }, nil }, - wantErr: emptyFieldError("id"), + wantErr: errors.EmptyFieldError("id"), }, { name: "emptyFlagKey", @@ -767,7 +768,7 @@ func TestUpdateDistribution(t *testing.T) { VariantId: r.VariantId, }, nil }, - wantErr: emptyFieldError("flagKey"), + wantErr: errors.EmptyFieldError("flagKey"), }, { name: "emptyRuleID", @@ -785,7 +786,7 @@ func TestUpdateDistribution(t *testing.T) { VariantId: r.VariantId, }, nil }, - wantErr: emptyFieldError("ruleId"), + wantErr: errors.EmptyFieldError("ruleId"), }, { name: "emptyVariantID", @@ -803,7 +804,7 @@ func TestUpdateDistribution(t *testing.T) { VariantId: "", }, nil }, - wantErr: emptyFieldError("variantId"), + wantErr: errors.EmptyFieldError("variantId"), }, { name: "rollout is lesser than 0", @@ -821,7 +822,7 @@ func TestUpdateDistribution(t *testing.T) { VariantId: r.VariantId, }, nil }, - wantErr: invalidFieldError("rollout", "must be greater than or equal to '0'"), + wantErr: errors.InvalidFieldError("rollout", "must be greater than or equal to '0'"), }, { name: "rollout is greater than 100", @@ -839,7 +840,7 @@ func TestUpdateDistribution(t *testing.T) { VariantId: r.VariantId, }, nil }, - wantErr: invalidFieldError("rollout", "must be less than or equal to '100'"), + wantErr: errors.InvalidFieldError("rollout", "must be less than or equal to '100'"), }, } @@ -899,7 +900,7 @@ func TestDeleteDistribution(t *testing.T) { return nil }, - wantErr: emptyFieldError("id"), + wantErr: errors.EmptyFieldError("id"), }, { name: "emptyFlagKey", @@ -913,7 +914,7 @@ func TestDeleteDistribution(t *testing.T) { return nil }, - wantErr: emptyFieldError("flagKey"), + wantErr: errors.EmptyFieldError("flagKey"), }, { name: "emptyRuleID", @@ -927,7 +928,7 @@ func TestDeleteDistribution(t *testing.T) { return nil }, - wantErr: emptyFieldError("ruleId"), + wantErr: errors.EmptyFieldError("ruleId"), }, { name: "emptyVariantID", @@ -941,7 +942,7 @@ func TestDeleteDistribution(t *testing.T) { return nil }, - wantErr: emptyFieldError("variantId"), + wantErr: errors.EmptyFieldError("variantId"), }, { name: "error test", diff --git a/server/segment.go b/server/segment.go index cc2a8281f0..d0e5f14fb4 100644 --- a/server/segment.go +++ b/server/segment.go @@ -4,13 +4,14 @@ import ( "context" "github.com/golang/protobuf/ptypes/empty" + "github.com/markphelps/flipt/errors" flipt "github.com/markphelps/flipt/rpc" ) // GetSegment gets a segment func (s *Server) GetSegment(ctx context.Context, req *flipt.GetSegmentRequest) (*flipt.Segment, error) { if req.Key == "" { - return nil, emptyFieldError("key") + return nil, errors.EmptyFieldError("key") } return s.SegmentStore.GetSegment(ctx, req) @@ -35,11 +36,11 @@ func (s *Server) ListSegments(ctx context.Context, req *flipt.ListSegmentRequest // CreateSegment creates a segment func (s *Server) CreateSegment(ctx context.Context, req *flipt.CreateSegmentRequest) (*flipt.Segment, error) { if req.Key == "" { - return nil, emptyFieldError("key") + return nil, errors.EmptyFieldError("key") } if req.Name == "" { - return nil, emptyFieldError("name") + return nil, errors.EmptyFieldError("name") } return s.SegmentStore.CreateSegment(ctx, req) @@ -48,11 +49,11 @@ func (s *Server) CreateSegment(ctx context.Context, req *flipt.CreateSegmentRequ // UpdateSegment updates an existing segment func (s *Server) UpdateSegment(ctx context.Context, req *flipt.UpdateSegmentRequest) (*flipt.Segment, error) { if req.Key == "" { - return nil, emptyFieldError("key") + return nil, errors.EmptyFieldError("key") } if req.Name == "" { - return nil, emptyFieldError("name") + return nil, errors.EmptyFieldError("name") } return s.SegmentStore.UpdateSegment(ctx, req) @@ -61,7 +62,7 @@ func (s *Server) UpdateSegment(ctx context.Context, req *flipt.UpdateSegmentRequ // DeleteSegment deletes a segment func (s *Server) DeleteSegment(ctx context.Context, req *flipt.DeleteSegmentRequest) (*empty.Empty, error) { if req.Key == "" { - return nil, emptyFieldError("key") + return nil, errors.EmptyFieldError("key") } if err := s.SegmentStore.DeleteSegment(ctx, req); err != nil { @@ -74,15 +75,15 @@ func (s *Server) DeleteSegment(ctx context.Context, req *flipt.DeleteSegmentRequ // CreateConstraint creates a constraint func (s *Server) CreateConstraint(ctx context.Context, req *flipt.CreateConstraintRequest) (*flipt.Constraint, error) { if req.SegmentKey == "" { - return nil, emptyFieldError("segmentKey") + return nil, errors.EmptyFieldError("segmentKey") } if req.Property == "" { - return nil, emptyFieldError("property") + return nil, errors.EmptyFieldError("property") } if req.Operator == "" { - return nil, emptyFieldError("operator") + return nil, errors.EmptyFieldError("operator") } // TODO: test for empty value if operator ! [EMPTY, NOT_EMPTY, PRESENT, NOT_PRESENT] @@ -93,19 +94,19 @@ func (s *Server) CreateConstraint(ctx context.Context, req *flipt.CreateConstrai // UpdateConstraint updates an existing constraint func (s *Server) UpdateConstraint(ctx context.Context, req *flipt.UpdateConstraintRequest) (*flipt.Constraint, error) { if req.Id == "" { - return nil, emptyFieldError("id") + return nil, errors.EmptyFieldError("id") } if req.SegmentKey == "" { - return nil, emptyFieldError("segmentKey") + return nil, errors.EmptyFieldError("segmentKey") } if req.Property == "" { - return nil, emptyFieldError("property") + return nil, errors.EmptyFieldError("property") } if req.Operator == "" { - return nil, emptyFieldError("operator") + return nil, errors.EmptyFieldError("operator") } // TODO: test for empty value if operator ! [EMPTY, NOT_EMPTY, PRESENT, NOT_PRESENT] @@ -116,11 +117,11 @@ func (s *Server) UpdateConstraint(ctx context.Context, req *flipt.UpdateConstrai // DeleteConstraint deletes a constraint func (s *Server) DeleteConstraint(ctx context.Context, req *flipt.DeleteConstraintRequest) (*empty.Empty, error) { if req.Id == "" { - return nil, emptyFieldError("id") + return nil, errors.EmptyFieldError("id") } if req.SegmentKey == "" { - return nil, emptyFieldError("segmentKey") + return nil, errors.EmptyFieldError("segmentKey") } if err := s.SegmentStore.DeleteConstraint(ctx, req); err != nil { diff --git a/server/segment_test.go b/server/segment_test.go index aaff0c7d45..bbfe7d1c26 100644 --- a/server/segment_test.go +++ b/server/segment_test.go @@ -2,9 +2,10 @@ package server import ( "context" - "errors" "testing" + "github.com/markphelps/flipt/errors" + "github.com/golang/protobuf/ptypes/empty" flipt "github.com/markphelps/flipt/rpc" "github.com/markphelps/flipt/storage" @@ -90,7 +91,7 @@ func TestGetSegment(t *testing.T) { Key: "", }, nil }, - wantErr: emptyFieldError("key"), + wantErr: errors.EmptyFieldError("key"), }, } @@ -224,7 +225,7 @@ func TestCreateSegment(t *testing.T) { Description: r.Description, }, nil }, - wantErr: emptyFieldError("key"), + wantErr: errors.EmptyFieldError("key"), }, { name: "emptyName", @@ -245,7 +246,7 @@ func TestCreateSegment(t *testing.T) { Description: r.Description, }, nil }, - wantErr: emptyFieldError("name"), + wantErr: errors.EmptyFieldError("name"), }, } @@ -323,7 +324,7 @@ func TestUpdateSegment(t *testing.T) { Description: r.Description, }, nil }, - wantErr: emptyFieldError("key"), + wantErr: errors.EmptyFieldError("key"), }, { name: "emptyName", @@ -344,7 +345,7 @@ func TestUpdateSegment(t *testing.T) { Description: r.Description, }, nil }, - wantErr: emptyFieldError("name"), + wantErr: errors.EmptyFieldError("name"), }, } @@ -396,7 +397,7 @@ func TestDeleteSegment(t *testing.T) { assert.Equal(t, "", r.Key) return nil }, - wantErr: emptyFieldError("key"), + wantErr: errors.EmptyFieldError("key"), }, { name: "error test", @@ -498,7 +499,7 @@ func TestCreateConstraint(t *testing.T) { Value: r.Value, }, nil }, - wantErr: emptyFieldError("segmentKey"), + wantErr: errors.EmptyFieldError("segmentKey"), }, { name: "emptyProperty", @@ -525,7 +526,7 @@ func TestCreateConstraint(t *testing.T) { Value: r.Value, }, nil }, - wantErr: emptyFieldError("property"), + wantErr: errors.EmptyFieldError("property"), }, { name: "emptyOperator", @@ -552,7 +553,7 @@ func TestCreateConstraint(t *testing.T) { Value: r.Value, }, nil }, - wantErr: emptyFieldError("operator"), + wantErr: errors.EmptyFieldError("operator"), }, } @@ -651,7 +652,7 @@ func TestUpdateConstraint(t *testing.T) { Value: r.Value, }, nil }, - wantErr: emptyFieldError("id"), + wantErr: errors.EmptyFieldError("id"), }, { name: "emptySegmentKey", @@ -681,7 +682,7 @@ func TestUpdateConstraint(t *testing.T) { Value: r.Value, }, nil }, - wantErr: emptyFieldError("segmentKey"), + wantErr: errors.EmptyFieldError("segmentKey"), }, { name: "emptyProperty", @@ -711,7 +712,7 @@ func TestUpdateConstraint(t *testing.T) { Value: r.Value, }, nil }, - wantErr: emptyFieldError("property"), + wantErr: errors.EmptyFieldError("property"), }, { name: "emptyOperator", @@ -741,7 +742,7 @@ func TestUpdateConstraint(t *testing.T) { Value: r.Value, }, nil }, - wantErr: emptyFieldError("operator"), + wantErr: errors.EmptyFieldError("operator"), }, } @@ -795,7 +796,7 @@ func TestDeleteConstraint(t *testing.T) { assert.Equal(t, "segmentKey", r.SegmentKey) return nil }, - wantErr: emptyFieldError("id"), + wantErr: errors.EmptyFieldError("id"), }, { name: "emptySegmentKey", @@ -806,7 +807,7 @@ func TestDeleteConstraint(t *testing.T) { assert.Equal(t, "", r.SegmentKey) return nil }, - wantErr: emptyFieldError("segmentKey"), + wantErr: errors.EmptyFieldError("segmentKey"), }, { name: "error test", diff --git a/server/server.go b/server/server.go index 367c9f65b6..64f6e4b939 100644 --- a/server/server.go +++ b/server/server.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" + "github.com/markphelps/flipt/errors" pb "github.com/markphelps/flipt/rpc" "github.com/markphelps/flipt/storage" "github.com/markphelps/flipt/storage/cache" @@ -67,11 +68,11 @@ func (s *Server) ErrorUnaryInterceptor(ctx context.Context, req interface{}, _ * errorsTotal.Inc() switch err.(type) { - case storage.ErrNotFound: + case errors.ErrNotFound: err = status.Error(codes.NotFound, err.Error()) - case storage.ErrInvalid: + case errors.ErrInvalid: err = status.Error(codes.InvalidArgument, err.Error()) - case errInvalidField: + case errors.ErrValidation: err = status.Error(codes.InvalidArgument, err.Error()) default: err = status.Error(codes.Internal, err.Error()) diff --git a/server/server_test.go b/server/server_test.go index c07b58d3fe..7c30e0e21a 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -3,11 +3,11 @@ package server import ( "context" "database/sql" - "errors" "testing" + "github.com/markphelps/flipt/errors" + sq "github.com/Masterminds/squirrel" - "github.com/markphelps/flipt/storage" "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -34,23 +34,23 @@ func TestErrorUnaryInterceptor(t *testing.T) { wantCode codes.Code }{ { - name: "storage not found error", - wantErr: storage.ErrNotFound("foo"), + name: "not found error", + wantErr: errors.ErrNotFound("foo"), wantCode: codes.NotFound, }, { - name: "storage invalid error", - wantErr: storage.ErrInvalid("foo"), + name: "invalid error", + wantErr: errors.ErrInvalid("foo"), wantCode: codes.InvalidArgument, }, { - name: "server invalid field", - wantErr: invalidFieldError("bar", "is wrong"), + name: "invalid field", + wantErr: errors.InvalidFieldError("bar", "is wrong"), wantCode: codes.InvalidArgument, }, { - name: "server empty field", - wantErr: emptyFieldError("bar"), + name: "empty field", + wantErr: errors.EmptyFieldError("bar"), wantCode: codes.InvalidArgument, }, { diff --git a/storage/cache/flag_test.go b/storage/cache/flag_test.go index cd17380fcc..ef7644909e 100644 --- a/storage/cache/flag_test.go +++ b/storage/cache/flag_test.go @@ -2,12 +2,11 @@ package cache import ( "context" - "errors" "testing" lru "github.com/hashicorp/golang-lru" + "github.com/markphelps/flipt/errors" flipt "github.com/markphelps/flipt/rpc" - "github.com/markphelps/flipt/storage" "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -48,7 +47,7 @@ func TestGetFlagNotFound(t *testing.T) { logger, _ = test.NewNullLogger() store = &flagStoreMock{ getFlagFn: func(context.Context, *flipt.GetFlagRequest) (*flipt.Flag, error) { - return nil, storage.ErrNotFound("foo") + return nil, errors.ErrNotFound("foo") }, } cache, _ = lru.New(1) diff --git a/storage/errors.go b/storage/errors.go deleted file mode 100644 index b117bc3233..0000000000 --- a/storage/errors.go +++ /dev/null @@ -1,27 +0,0 @@ -package storage - -import "fmt" - -// ErrNotFound represents a not found error -type ErrNotFound string - -// ErrNotFoundf creates an ErrNotFound using a custom format -func ErrNotFoundf(format string, args ...interface{}) error { - return ErrNotFound(fmt.Sprintf(format, args...)) -} - -func (e ErrNotFound) Error() string { - return fmt.Sprintf("%s not found", string(e)) -} - -// ErrInvalid represents an invalid error -type ErrInvalid string - -// ErrInvalidf creates an ErrInvalid using a custom format -func ErrInvalidf(format string, args ...interface{}) error { - return ErrInvalid(fmt.Sprintf(format, args...)) -} - -func (e ErrInvalid) Error() string { - return string(e) -} diff --git a/storage/evaluator.go b/storage/evaluator.go index a088993333..b9c4c52206 100644 --- a/storage/evaluator.go +++ b/storage/evaluator.go @@ -3,7 +3,6 @@ package storage import ( "context" "database/sql" - "errors" "fmt" "hash/crc32" "sort" @@ -11,6 +10,8 @@ import ( "strings" "time" + "github.com/markphelps/flipt/errors" + sq "github.com/Masterminds/squirrel" "github.com/golang/protobuf/ptypes" flipt "github.com/markphelps/flipt/rpc" @@ -94,14 +95,14 @@ func (s *EvaluatorStorage) Evaluate(ctx context.Context, r *flipt.EvaluationRequ if err != nil { if err == sql.ErrNoRows { - return resp, ErrNotFoundf("flag %q", r.FlagKey) + return resp, errors.ErrNotFoundf("flag %q", r.FlagKey) } return resp, err } if !enabled { - return resp, ErrInvalidf("flag %q is disabled", r.FlagKey) + return resp, errors.ErrInvalidf("flag %q is disabled", r.FlagKey) } // get all rules for flag with their constraints if any @@ -204,7 +205,7 @@ func (s *EvaluatorStorage) Evaluate(ctx context.Context, r *flipt.EvaluationRequ case flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE: match, err = matchesBool(c, v) default: - return resp, ErrInvalid("unknown constraint type") + return resp, errors.ErrInvalid("unknown constraint type") } if err != nil { diff --git a/storage/flag.go b/storage/flag.go index d44545596c..0a300c0ae4 100644 --- a/storage/flag.go +++ b/storage/flag.go @@ -9,6 +9,7 @@ import ( "github.com/lib/pq" proto "github.com/golang/protobuf/ptypes" + "github.com/markphelps/flipt/errors" flipt "github.com/markphelps/flipt/rpc" sqlite3 "github.com/mattn/go-sqlite3" "github.com/sirupsen/logrus" @@ -73,7 +74,7 @@ func (s *FlagStorage) flag(ctx context.Context, key string) (*flipt.Flag, error) if err != nil { if err == sql.ErrNoRows { - return nil, ErrNotFoundf("flag %q", key) + return nil, errors.ErrNotFoundf("flag %q", key) } return nil, err @@ -176,11 +177,11 @@ func (s *FlagStorage) CreateFlag(ctx context.Context, r *flipt.CreateFlagRequest switch ierr := err.(type) { case sqlite3.Error: if ierr.Code == sqlite3.ErrConstraint { - return nil, ErrInvalidf("flag %q is not unique", r.Key) + return nil, errors.ErrInvalidf("flag %q is not unique", r.Key) } case *pq.Error: if ierr.Code.Name() == pgConstraintUnique { - return nil, ErrInvalidf("flag %q is not unique", r.Key) + return nil, errors.ErrInvalidf("flag %q is not unique", r.Key) } } @@ -214,7 +215,7 @@ func (s *FlagStorage) UpdateFlag(ctx context.Context, r *flipt.UpdateFlagRequest } if count != 1 { - return nil, ErrNotFoundf("flag %q", r.Key) + return nil, errors.ErrNotFoundf("flag %q", r.Key) } flag, err := s.flag(ctx, r.Key) @@ -260,16 +261,16 @@ func (s *FlagStorage) CreateVariant(ctx context.Context, r *flipt.CreateVariantR case sqlite3.Error: if ierr.Code == sqlite3.ErrConstraint { if ierr.ExtendedCode == sqlite3.ErrConstraintForeignKey { - return nil, ErrNotFoundf("flag %q", r.FlagKey) + return nil, errors.ErrNotFoundf("flag %q", r.FlagKey) } else if ierr.ExtendedCode == sqlite3.ErrConstraintUnique { - return nil, ErrInvalidf("variant %q is not unique", r.Key) + return nil, errors.ErrInvalidf("variant %q is not unique", r.Key) } } case *pq.Error: if ierr.Code.Name() == pgConstraintForeignKey { - return nil, ErrNotFoundf("flag %q", r.FlagKey) + return nil, errors.ErrNotFoundf("flag %q", r.FlagKey) } else if ierr.Code.Name() == pgConstraintUnique { - return nil, ErrInvalidf("variant %q is not unique", r.Key) + return nil, errors.ErrInvalidf("variant %q is not unique", r.Key) } } @@ -297,12 +298,12 @@ func (s *FlagStorage) UpdateVariant(ctx context.Context, r *flipt.UpdateVariantR case sqlite3.Error: if ierr.Code == sqlite3.ErrConstraint { if ierr.ExtendedCode == sqlite3.ErrConstraintUnique { - return nil, ErrInvalidf("variant %q is not unique", r.Key) + return nil, errors.ErrInvalidf("variant %q is not unique", r.Key) } } case *pq.Error: if ierr.Code.Name() == pgConstraintUnique { - return nil, ErrInvalidf("variant %q is not unique", r.Key) + return nil, errors.ErrInvalidf("variant %q is not unique", r.Key) } } @@ -315,7 +316,7 @@ func (s *FlagStorage) UpdateVariant(ctx context.Context, r *flipt.UpdateVariantR } if count != 1 { - return nil, ErrNotFoundf("variant %q", r.Key) + return nil, errors.ErrNotFoundf("variant %q", r.Key) } var ( diff --git a/storage/rule.go b/storage/rule.go index 46c8f56ff5..6d516ae428 100644 --- a/storage/rule.go +++ b/storage/rule.go @@ -8,6 +8,7 @@ import ( "github.com/gofrs/uuid" proto "github.com/golang/protobuf/ptypes" "github.com/lib/pq" + "github.com/markphelps/flipt/errors" flipt "github.com/markphelps/flipt/rpc" sqlite3 "github.com/mattn/go-sqlite3" "github.com/sirupsen/logrus" @@ -171,11 +172,11 @@ func (s *RuleStorage) CreateRule(ctx context.Context, r *flipt.CreateRuleRequest switch ierr := err.(type) { case sqlite3.Error: if ierr.Code == sqlite3.ErrConstraint { - return nil, ErrNotFoundf("flag %q or segment %q", r.FlagKey, r.SegmentKey) + return nil, errors.ErrNotFoundf("flag %q or segment %q", r.FlagKey, r.SegmentKey) } case *pq.Error: if ierr.Code.Name() == pgConstraintForeignKey { - return nil, ErrNotFoundf("flag %q or segment %q", r.FlagKey, r.SegmentKey) + return nil, errors.ErrNotFoundf("flag %q or segment %q", r.FlagKey, r.SegmentKey) } } @@ -207,7 +208,7 @@ func (s *RuleStorage) UpdateRule(ctx context.Context, r *flipt.UpdateRuleRequest } if count != 1 { - return nil, ErrNotFoundf("rule %q", r.Id) + return nil, errors.ErrNotFoundf("rule %q", r.Id) } rule, err := s.rule(ctx, r.Id, r.FlagKey) @@ -335,11 +336,11 @@ func (s *RuleStorage) CreateDistribution(ctx context.Context, r *flipt.CreateDis switch ierr := err.(type) { case sqlite3.Error: if ierr.Code == sqlite3.ErrConstraint { - return nil, ErrNotFoundf("rule %q", r.RuleId) + return nil, errors.ErrNotFoundf("rule %q", r.RuleId) } case *pq.Error: if ierr.Code.Name() == pgConstraintForeignKey { - return nil, ErrNotFoundf("rule %q", r.RuleId) + return nil, errors.ErrNotFoundf("rule %q", r.RuleId) } } @@ -371,7 +372,7 @@ func (s *RuleStorage) UpdateDistribution(ctx context.Context, r *flipt.UpdateDis } if count != 1 { - return nil, ErrNotFoundf("distribution %q", r.Id) + return nil, errors.ErrNotFoundf("distribution %q", r.Id) } var ( diff --git a/storage/segment.go b/storage/segment.go index fda5cc1290..dad44df363 100644 --- a/storage/segment.go +++ b/storage/segment.go @@ -10,6 +10,7 @@ import ( "github.com/lib/pq" proto "github.com/golang/protobuf/ptypes" + "github.com/markphelps/flipt/errors" flipt "github.com/markphelps/flipt/rpc" sqlite3 "github.com/mattn/go-sqlite3" "github.com/sirupsen/logrus" @@ -72,7 +73,7 @@ func (s SegmentStorage) segment(ctx context.Context, key string) (*flipt.Segment if err != nil { if err == sql.ErrNoRows { - return nil, ErrNotFoundf("segment %q", key) + return nil, errors.ErrNotFoundf("segment %q", key) } return nil, err @@ -173,11 +174,11 @@ func (s *SegmentStorage) CreateSegment(ctx context.Context, r *flipt.CreateSegme switch ierr := err.(type) { case sqlite3.Error: if ierr.Code == sqlite3.ErrConstraint { - return nil, ErrInvalidf("segment %q is not unique", r.Key) + return nil, errors.ErrInvalidf("segment %q is not unique", r.Key) } case *pq.Error: if ierr.Code.Name() == pgConstraintUnique { - return nil, ErrInvalidf("segment %q is not unique", r.Key) + return nil, errors.ErrInvalidf("segment %q is not unique", r.Key) } } @@ -210,7 +211,7 @@ func (s *SegmentStorage) UpdateSegment(ctx context.Context, r *flipt.UpdateSegme } if count != 1 { - return nil, ErrNotFoundf("segment %q", r.Key) + return nil, errors.ErrNotFoundf("segment %q", r.Key) } segment, err := s.segment(ctx, r.Key) @@ -254,18 +255,18 @@ func (s *SegmentStorage) CreateConstraint(ctx context.Context, r *flipt.CreateCo switch c.Type { case flipt.ComparisonType_STRING_COMPARISON_TYPE: if _, ok := stringOperators[c.Operator]; !ok { - return nil, ErrInvalidf("constraint operator %q is not valid for type string", r.Operator) + return nil, errors.ErrInvalidf("constraint operator %q is not valid for type string", r.Operator) } case flipt.ComparisonType_NUMBER_COMPARISON_TYPE: if _, ok := numberOperators[c.Operator]; !ok { - return nil, ErrInvalidf("constraint operator %q is not valid for type number", r.Operator) + return nil, errors.ErrInvalidf("constraint operator %q is not valid for type number", r.Operator) } case flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE: if _, ok := booleanOperators[c.Operator]; !ok { - return nil, ErrInvalidf("constraint operator %q is not valid for type boolean", r.Operator) + return nil, errors.ErrInvalidf("constraint operator %q is not valid for type boolean", r.Operator) } default: - return nil, ErrInvalidf("invalid constraint type: %q", c.Type.String()) + return nil, errors.ErrInvalidf("invalid constraint type: %q", c.Type.String()) } // unset value if operator does not require it @@ -281,11 +282,11 @@ func (s *SegmentStorage) CreateConstraint(ctx context.Context, r *flipt.CreateCo switch ierr := err.(type) { case sqlite3.Error: if ierr.Code == sqlite3.ErrConstraint { - return nil, ErrNotFoundf("segment %q", r.SegmentKey) + return nil, errors.ErrNotFoundf("segment %q", r.SegmentKey) } case *pq.Error: if ierr.Code.Name() == pgConstraintForeignKey { - return nil, ErrNotFoundf("segment %q", r.SegmentKey) + return nil, errors.ErrNotFoundf("segment %q", r.SegmentKey) } } @@ -306,18 +307,18 @@ func (s *SegmentStorage) UpdateConstraint(ctx context.Context, r *flipt.UpdateCo switch r.Type { case flipt.ComparisonType_STRING_COMPARISON_TYPE: if _, ok := stringOperators[operator]; !ok { - return nil, ErrInvalidf("constraint operator %q is not valid for type string", r.Operator) + return nil, errors.ErrInvalidf("constraint operator %q is not valid for type string", r.Operator) } case flipt.ComparisonType_NUMBER_COMPARISON_TYPE: if _, ok := numberOperators[operator]; !ok { - return nil, ErrInvalidf("constraint operator %q is not valid for type number", r.Operator) + return nil, errors.ErrInvalidf("constraint operator %q is not valid for type number", r.Operator) } case flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE: if _, ok := booleanOperators[operator]; !ok { - return nil, ErrInvalidf("constraint operator %q is not valid for type boolean", r.Operator) + return nil, errors.ErrInvalidf("constraint operator %q is not valid for type boolean", r.Operator) } default: - return nil, ErrInvalidf("invalid constraint type: %q", r.Type.String()) + return nil, errors.ErrInvalidf("invalid constraint type: %q", r.Type.String()) } // unset value if operator does not require it @@ -343,7 +344,7 @@ func (s *SegmentStorage) UpdateConstraint(ctx context.Context, r *flipt.UpdateCo } if count != 1 { - return nil, ErrNotFoundf("constraint %q", r.Id) + return nil, errors.ErrNotFoundf("constraint %q", r.Id) } var ( diff --git a/storage/segment_test.go b/storage/segment_test.go index 0196d6bb14..acbf16e972 100644 --- a/storage/segment_test.go +++ b/storage/segment_test.go @@ -4,6 +4,7 @@ import ( "context" "testing" + "github.com/markphelps/flipt/errors" flipt "github.com/markphelps/flipt/rpc" "github.com/gofrs/uuid" @@ -285,11 +286,11 @@ func TestCreateConstraint(t *testing.T) { assert.Len(t, segment.Constraints, 1) } -func TestCreateConstraint_ErrInvalid(t *testing.T) { +func TestCreateConstraint_Invalid(t *testing.T) { tests := []struct { name string req *flipt.CreateConstraintRequest - wantErr ErrInvalid + wantErr errors.ErrInvalid }{ { name: "invalid for type string", @@ -300,7 +301,7 @@ func TestCreateConstraint_ErrInvalid(t *testing.T) { Operator: "LT", Value: "baz", }, - wantErr: ErrInvalid("constraint operator \"LT\" is not valid for type string"), + wantErr: errors.ErrInvalid("constraint operator \"LT\" is not valid for type string"), }, { name: "invalid for type number", @@ -311,7 +312,7 @@ func TestCreateConstraint_ErrInvalid(t *testing.T) { Operator: "Empty", Value: "2", }, - wantErr: ErrInvalid("constraint operator \"Empty\" is not valid for type number"), + wantErr: errors.ErrInvalid("constraint operator \"Empty\" is not valid for type number"), }, { name: "invalid for type boolean", @@ -322,7 +323,7 @@ func TestCreateConstraint_ErrInvalid(t *testing.T) { Operator: "GT", Value: "false", }, - wantErr: ErrInvalid("constraint operator \"GT\" is not valid for type boolean"), + wantErr: errors.ErrInvalid("constraint operator \"GT\" is not valid for type boolean"), }, { name: "invalid for type unknown", @@ -333,7 +334,7 @@ func TestCreateConstraint_ErrInvalid(t *testing.T) { Operator: "EQ", Value: "+", }, - wantErr: ErrInvalid("invalid constraint type: \"UNKNOWN_COMPARISON_TYPE\""), + wantErr: errors.ErrInvalid("invalid constraint type: \"UNKNOWN_COMPARISON_TYPE\""), }, } @@ -422,11 +423,11 @@ func TestUpdateConstraint(t *testing.T) { assert.Len(t, segment.Constraints, 1) } -func TestUpdateConstraint_ErrInvalid(t *testing.T) { +func TestUpdateConstraint_Invalid(t *testing.T) { tests := []struct { name string req *flipt.UpdateConstraintRequest - wantErr ErrInvalid + wantErr errors.ErrInvalid }{ { name: "invalid for type string", @@ -438,7 +439,7 @@ func TestUpdateConstraint_ErrInvalid(t *testing.T) { Operator: "LT", Value: "baz", }, - wantErr: ErrInvalid("constraint operator \"LT\" is not valid for type string"), + wantErr: errors.ErrInvalid("constraint operator \"LT\" is not valid for type string"), }, { name: "invalid for type number", @@ -450,7 +451,7 @@ func TestUpdateConstraint_ErrInvalid(t *testing.T) { Operator: "Empty", Value: "2", }, - wantErr: ErrInvalid("constraint operator \"Empty\" is not valid for type number"), + wantErr: errors.ErrInvalid("constraint operator \"Empty\" is not valid for type number"), }, { name: "invalid for type boolean", @@ -462,7 +463,7 @@ func TestUpdateConstraint_ErrInvalid(t *testing.T) { Operator: "GT", Value: "false", }, - wantErr: ErrInvalid("constraint operator \"GT\" is not valid for type boolean"), + wantErr: errors.ErrInvalid("constraint operator \"GT\" is not valid for type boolean"), }, { name: "invalid for type unknown", @@ -474,7 +475,7 @@ func TestUpdateConstraint_ErrInvalid(t *testing.T) { Operator: "EQ", Value: "+", }, - wantErr: ErrInvalid("invalid constraint type: \"UNKNOWN_COMPARISON_TYPE\""), + wantErr: errors.ErrInvalid("invalid constraint type: \"UNKNOWN_COMPARISON_TYPE\""), }, } From 1c1a5420386c974430118363d7830e4d1d8adad1 Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Wed, 27 Nov 2019 10:48:48 -0500 Subject: [PATCH 04/12] WIP move validation to rpc package --- Makefile | 2 +- cmd/flipt/main.go | 3 +- codecov.yml | 2 + rpc/flipt.pb.go | 3 +- rpc/validation.go | 97 ++++++++++++ rpc/validation_test.go | 292 ++++++++++++++++++++++++++++++++++++ script/test/api | 26 +++- server/segment.go | 65 -------- server/segment_test.go | 330 ----------------------------------------- server/server.go | 12 ++ 10 files changed, 431 insertions(+), 401 deletions(-) create mode 100644 codecov.yml create mode 100644 rpc/validation_test.go diff --git a/Makefile b/Makefile index ba90c58bbe..dd52e7e2e7 100644 --- a/Makefile +++ b/Makefile @@ -51,7 +51,7 @@ cover: test ## Run all the tests and opens the coverage report .PHONY: fmt fmt: ## Run gofmt and goimports on all go files @echo ">> running gofmt" - @find . -name '*.go' -not -wholename './rpc/*' -not -wholename './ui/*' -not -wholename './swagger/*' | while read -r file; do gofmt -w -s "$$file"; goimports -w "$$file"; done + @find . -name '*.go' | while read -r file; do gofmt -w -s "$$file"; goimports -w "$$file"; done .PHONY: lint lint: ## Run all the linters diff --git a/cmd/flipt/main.go b/cmd/flipt/main.go index 37ee2d4b01..631a9f3303 100644 --- a/cmd/flipt/main.go +++ b/cmd/flipt/main.go @@ -286,11 +286,12 @@ func execute() error { srv = server.New(logger, builder, db, serverOpts...) grpcOpts = append(grpcOpts, grpc_middleware.WithUnaryServerChain( + grpc_recovery.UnaryServerInterceptor(), grpc_ctxtags.UnaryServerInterceptor(), grpc_logrus.UnaryServerInterceptor(logger), grpc_prometheus.UnaryServerInterceptor, srv.ErrorUnaryInterceptor, - grpc_recovery.UnaryServerInterceptor(), + srv.ValidationUnaryInterceptor, )) if cfg.Server.Protocol == config.HTTPS { diff --git a/codecov.yml b/codecov.yml new file mode 100644 index 0000000000..05c9e5dc10 --- /dev/null +++ b/codecov.yml @@ -0,0 +1,2 @@ +ignore: + - "rpc/flipt.pb.*" diff --git a/rpc/flipt.pb.go b/rpc/flipt.pb.go index 62bfdfd2a0..cf51e60147 100644 --- a/rpc/flipt.pb.go +++ b/rpc/flipt.pb.go @@ -6,6 +6,8 @@ package flipt import ( context "context" fmt "fmt" + math "math" + proto "github.com/golang/protobuf/proto" empty "github.com/golang/protobuf/ptypes/empty" timestamp "github.com/golang/protobuf/ptypes/timestamp" @@ -13,7 +15,6 @@ import ( grpc "google.golang.org/grpc" codes "google.golang.org/grpc/codes" status "google.golang.org/grpc/status" - math "math" ) // Reference imports to suppress errors if they are not otherwise used. diff --git a/rpc/validation.go b/rpc/validation.go index 9e98dd8e7e..da4f3a2ef8 100644 --- a/rpc/validation.go +++ b/rpc/validation.go @@ -1,3 +1,100 @@ package flipt +import "github.com/markphelps/flipt/errors" +// Validator validates types +type Validator interface { + Validate() error +} + +// Segments + +func (req *GetSegmentRequest) Validate() error { + if req.Key == "" { + return errors.EmptyFieldError("key") + } + + return nil +} + +func (req *CreateSegmentRequest) Validate() error { + if req.Key == "" { + return errors.EmptyFieldError("key") + } + + if req.Name == "" { + return errors.EmptyFieldError("name") + } + + return nil +} + +func (req *UpdateSegmentRequest) Validate() error { + if req.Key == "" { + return errors.EmptyFieldError("key") + } + + if req.Name == "" { + return errors.EmptyFieldError("name") + } + + return nil +} + +func (req *DeleteSegmentRequest) Validate() error { + if req.Key == "" { + return errors.EmptyFieldError("key") + } + + return nil +} + +func (req *CreateConstraintRequest) Validate() error { + if req.SegmentKey == "" { + return errors.EmptyFieldError("segmentKey") + } + + if req.Property == "" { + return errors.EmptyFieldError("property") + } + + if req.Operator == "" { + return errors.EmptyFieldError("operator") + } + + // TODO: test for empty value if operator ! [EMPTY, NOT_EMPTY, PRESENT, NOT_PRESENT] + return nil +} + +func (req *UpdateConstraintRequest) Validate() error { + if req.Id == "" { + return errors.EmptyFieldError("id") + } + + if req.SegmentKey == "" { + return errors.EmptyFieldError("segmentKey") + } + + if req.Property == "" { + return errors.EmptyFieldError("property") + } + + if req.Operator == "" { + return errors.EmptyFieldError("operator") + } + + // TODO: test for empty value if operator ! [EMPTY, NOT_EMPTY, PRESENT, NOT_PRESENT] + return nil +} + +func (req *DeleteConstraintRequest) Validate() error { + if req.Id == "" { + return errors.EmptyFieldError("id") + } + + if req.SegmentKey == "" { + return errors.EmptyFieldError("segmentKey") + } + + return nil +} diff --git a/rpc/validation_test.go b/rpc/validation_test.go new file mode 100644 index 0000000000..1c2d2feea0 --- /dev/null +++ b/rpc/validation_test.go @@ -0,0 +1,292 @@ +package flipt + +import ( + "testing" + + "github.com/markphelps/flipt/errors" + "github.com/stretchr/testify/assert" +) + +func TestValidate_GetSegmentRequest(t *testing.T) { + tests := []struct { + name string + req *GetSegmentRequest + wantErr error + }{ + { + name: "emptyKey", + req: &GetSegmentRequest{Key: ""}, + wantErr: errors.EmptyFieldError("key"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_CreateSegmentRequest(t *testing.T) { + tests := []struct { + name string + req *CreateSegmentRequest + wantErr error + }{ + { + name: "emptyKey", + req: &CreateSegmentRequest{ + Key: "", + Name: "name", + Description: "desc", + }, + wantErr: errors.EmptyFieldError("key"), + }, + { + name: "emptyName", + req: &CreateSegmentRequest{ + Key: "key", + Name: "", + Description: "desc", + }, + wantErr: errors.EmptyFieldError("name"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_UpdateSegmentRequest(t *testing.T) { + tests := []struct { + name string + req *UpdateSegmentRequest + wantErr error + }{ + { + name: "emptyKey", + req: &UpdateSegmentRequest{ + Key: "", + Name: "name", + Description: "desc", + }, + wantErr: errors.EmptyFieldError("key"), + }, + { + name: "emptyName", + req: &UpdateSegmentRequest{ + Key: "key", + Name: "", + Description: "desc", + }, + wantErr: errors.EmptyFieldError("name"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_DeleteSegmentRequest(t *testing.T) { + tests := []struct { + name string + req *DeleteSegmentRequest + wantErr error + }{ + { + name: "emptyKey", + req: &DeleteSegmentRequest{Key: ""}, + wantErr: errors.EmptyFieldError("key"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_CreateConstraintRequest(t *testing.T) { + tests := []struct { + name string + req *CreateConstraintRequest + wantErr error + }{ + { + name: "emptySegmentKey", + req: &CreateConstraintRequest{ + SegmentKey: "", + Type: ComparisonType_BOOLEAN_COMPARISON_TYPE, + Property: "foo", + Operator: "EQ", + Value: "bar", + }, + wantErr: errors.EmptyFieldError("segmentKey"), + }, + { + name: "emptyProperty", + req: &CreateConstraintRequest{ + SegmentKey: "segmentKey", + Type: ComparisonType_BOOLEAN_COMPARISON_TYPE, + Property: "", + Operator: "EQ", + Value: "bar", + }, + wantErr: errors.EmptyFieldError("property"), + }, + { + name: "emptyOperator", + req: &CreateConstraintRequest{ + SegmentKey: "segmentKey", + Type: ComparisonType_BOOLEAN_COMPARISON_TYPE, + Property: "foo", + Operator: "", + Value: "bar", + }, + wantErr: errors.EmptyFieldError("operator"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_UpdateConstraintRequest(t *testing.T) { + tests := []struct { + name string + req *UpdateConstraintRequest + wantErr error + }{ + { + name: "emptyID", + req: &UpdateConstraintRequest{ + Id: "", + SegmentKey: "segmentKey", + Type: ComparisonType_BOOLEAN_COMPARISON_TYPE, + Property: "foo", + Operator: "EQ", + Value: "bar", + }, + wantErr: errors.EmptyFieldError("id"), + }, + { + name: "emptySegmentKey", + req: &UpdateConstraintRequest{ + Id: "1", + SegmentKey: "", + Type: ComparisonType_BOOLEAN_COMPARISON_TYPE, + Property: "foo", + Operator: "EQ", + Value: "bar", + }, + wantErr: errors.EmptyFieldError("segmentKey"), + }, + { + name: "emptyProperty", + req: &UpdateConstraintRequest{ + Id: "1", + SegmentKey: "segmentKey", + Type: ComparisonType_BOOLEAN_COMPARISON_TYPE, + Property: "", + Operator: "EQ", + Value: "bar", + }, + wantErr: errors.EmptyFieldError("property"), + }, + { + name: "emptyOperator", + req: &UpdateConstraintRequest{ + Id: "1", + SegmentKey: "segmentKey", + Type: ComparisonType_BOOLEAN_COMPARISON_TYPE, + Property: "foo", + Operator: "", + Value: "bar", + }, + wantErr: errors.EmptyFieldError("operator"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_DeleteConstraintRequest(t *testing.T) { + tests := []struct { + name string + req *DeleteConstraintRequest + wantErr error + }{ + { + name: "emptyID", + req: &DeleteConstraintRequest{Id: "", SegmentKey: "segmentKey"}, + wantErr: errors.EmptyFieldError("id"), + }, + { + name: "emptySegmentKey", + req: &DeleteConstraintRequest{Id: "id", SegmentKey: ""}, + wantErr: errors.EmptyFieldError("segmentKey"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} diff --git a/script/test/api b/script/test/api index d8c5305118..75527dda0a 100755 --- a/script/test/api +++ b/script/test/api @@ -80,10 +80,20 @@ step_2_test_flags_and_variants() step_3_test_segments_and_constraints() { - # create segment segment_key=$(uuid_str) segment_name_1=$(uuid_str) + # create segment - missing key + shakedown POST "$flipt_api/segments" -H 'Content-Type:application/json' -d "{\"name\":\"$segment_name_1\",\"description\":\"description\"}" + status 400 + matches "\"error\":\"invalid field key: must not be empty\"" + + # create segment - missing name + shakedown POST "$flipt_api/segments" -H 'Content-Type:application/json' -d "{\"key\":\"$segment_key\",\"description\":\"description\"}" + status 400 + matches "\"error\":\"invalid field name: must not be empty\"" + + # create segment shakedown POST "$flipt_api/segments" -H 'Content-Type:application/json' -d "{\"key\":\"$segment_key\",\"name\":\"$segment_name_1\",\"description\":\"description\"}" status 200 matches "\"key\":\"$segment_key\"" @@ -95,9 +105,19 @@ step_3_test_segments_and_constraints() matches "\"key\":\"$segment_key\"" matches "\"name\":\"$segment_name_1\"" - # update segment segment_name_2=$(uuid_str) + # update segment - missing key + shakedown PUT "$flipt_api/segments/" -H 'Content-Type:application/json' -d "{\"name\":\"$segment_name_2\",\"description\":\"description\"}" + status 400 + matches "\"error\":\"invalid field key: must not be empty\"" + + # update segment - missing name + shakedown PUT "$flipt_api/segments/$segment_key" -H 'Content-Type:application/json' -d "{\"key\":\"$segment_key\",\"description\":\"description\"}" + status 400 + matches "\"error\":\"invalid field name: must not be empty\"" + + # update segment shakedown PUT "$flipt_api/segments/$segment_key" -H 'Content-Type:application/json' -d "{\"key\":\"$segment_key\",\"name\":\"$segment_name_2\",\"description\":\"description\"}" status 200 matches "\"key\":\"$segment_key\"" @@ -178,7 +198,7 @@ step_4_test_rules_and_distributions() step_5_test_evaluation() { # evaluate - shakedown POST "$flipt_api/evaluate" -H 'Content-Type:application/json' -d "{\"flag_key\":\"$flag_key\",\"entity_id\":\"$(uuid_str)\",\"context\":{\"foo\":\"baz\"}}" + shakedown POST "$flipt_api/evaluate" -H 'Content-Type:application/json' -d "{\"flag_key\":\"$flag_key\",\"entity_id\":\"$(uuid_str)\",\"context\":{\"foo\":\"baz\",\"fizz\":\"bozz\"}}" status 200 matches "\"flagKey\":\"$flag_key\"" matches "\"segmentKey\":\"$segment_key\"" diff --git a/server/segment.go b/server/segment.go index d0e5f14fb4..fe528e04c5 100644 --- a/server/segment.go +++ b/server/segment.go @@ -4,16 +4,11 @@ import ( "context" "github.com/golang/protobuf/ptypes/empty" - "github.com/markphelps/flipt/errors" flipt "github.com/markphelps/flipt/rpc" ) // GetSegment gets a segment func (s *Server) GetSegment(ctx context.Context, req *flipt.GetSegmentRequest) (*flipt.Segment, error) { - if req.Key == "" { - return nil, errors.EmptyFieldError("key") - } - return s.SegmentStore.GetSegment(ctx, req) } @@ -35,36 +30,16 @@ func (s *Server) ListSegments(ctx context.Context, req *flipt.ListSegmentRequest // CreateSegment creates a segment func (s *Server) CreateSegment(ctx context.Context, req *flipt.CreateSegmentRequest) (*flipt.Segment, error) { - if req.Key == "" { - return nil, errors.EmptyFieldError("key") - } - - if req.Name == "" { - return nil, errors.EmptyFieldError("name") - } - return s.SegmentStore.CreateSegment(ctx, req) } // UpdateSegment updates an existing segment func (s *Server) UpdateSegment(ctx context.Context, req *flipt.UpdateSegmentRequest) (*flipt.Segment, error) { - if req.Key == "" { - return nil, errors.EmptyFieldError("key") - } - - if req.Name == "" { - return nil, errors.EmptyFieldError("name") - } - return s.SegmentStore.UpdateSegment(ctx, req) } // DeleteSegment deletes a segment func (s *Server) DeleteSegment(ctx context.Context, req *flipt.DeleteSegmentRequest) (*empty.Empty, error) { - if req.Key == "" { - return nil, errors.EmptyFieldError("key") - } - if err := s.SegmentStore.DeleteSegment(ctx, req); err != nil { return nil, err } @@ -74,56 +49,16 @@ func (s *Server) DeleteSegment(ctx context.Context, req *flipt.DeleteSegmentRequ // CreateConstraint creates a constraint func (s *Server) CreateConstraint(ctx context.Context, req *flipt.CreateConstraintRequest) (*flipt.Constraint, error) { - if req.SegmentKey == "" { - return nil, errors.EmptyFieldError("segmentKey") - } - - if req.Property == "" { - return nil, errors.EmptyFieldError("property") - } - - if req.Operator == "" { - return nil, errors.EmptyFieldError("operator") - } - - // TODO: test for empty value if operator ! [EMPTY, NOT_EMPTY, PRESENT, NOT_PRESENT] - return s.SegmentStore.CreateConstraint(ctx, req) } // UpdateConstraint updates an existing constraint func (s *Server) UpdateConstraint(ctx context.Context, req *flipt.UpdateConstraintRequest) (*flipt.Constraint, error) { - if req.Id == "" { - return nil, errors.EmptyFieldError("id") - } - - if req.SegmentKey == "" { - return nil, errors.EmptyFieldError("segmentKey") - } - - if req.Property == "" { - return nil, errors.EmptyFieldError("property") - } - - if req.Operator == "" { - return nil, errors.EmptyFieldError("operator") - } - - // TODO: test for empty value if operator ! [EMPTY, NOT_EMPTY, PRESENT, NOT_PRESENT] - return s.SegmentStore.UpdateConstraint(ctx, req) } // DeleteConstraint deletes a constraint func (s *Server) DeleteConstraint(ctx context.Context, req *flipt.DeleteConstraintRequest) (*empty.Empty, error) { - if req.Id == "" { - return nil, errors.EmptyFieldError("id") - } - - if req.SegmentKey == "" { - return nil, errors.EmptyFieldError("segmentKey") - } - if err := s.SegmentStore.DeleteConstraint(ctx, req); err != nil { return nil, err } diff --git a/server/segment_test.go b/server/segment_test.go index bbfe7d1c26..9a135f0069 100644 --- a/server/segment_test.go +++ b/server/segment_test.go @@ -80,19 +80,6 @@ func TestGetSegment(t *testing.T) { Key: "key", }, }, - { - name: "emptyKey", - req: &flipt.GetSegmentRequest{Key: ""}, - f: func(_ context.Context, r *flipt.GetSegmentRequest) (*flipt.Segment, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.Key) - - return &flipt.Segment{ - Key: "", - }, nil - }, - wantErr: errors.EmptyFieldError("key"), - }, } for _, tt := range tests { @@ -206,48 +193,6 @@ func TestCreateSegment(t *testing.T) { Description: "desc", }, }, - { - name: "emptyKey", - req: &flipt.CreateSegmentRequest{ - Key: "", - Name: "name", - Description: "desc", - }, - f: func(_ context.Context, r *flipt.CreateSegmentRequest) (*flipt.Segment, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.Key) - assert.Equal(t, "name", r.Name) - assert.Equal(t, "desc", r.Description) - - return &flipt.Segment{ - Key: "", - Name: r.Name, - Description: r.Description, - }, nil - }, - wantErr: errors.EmptyFieldError("key"), - }, - { - name: "emptyName", - req: &flipt.CreateSegmentRequest{ - Key: "key", - Name: "", - Description: "desc", - }, - f: func(_ context.Context, r *flipt.CreateSegmentRequest) (*flipt.Segment, error) { - assert.NotNil(t, r) - assert.Equal(t, "key", r.Key) - assert.Equal(t, "", r.Name) - assert.Equal(t, "desc", r.Description) - - return &flipt.Segment{ - Key: r.Key, - Name: "", - Description: r.Description, - }, nil - }, - wantErr: errors.EmptyFieldError("name"), - }, } for _, tt := range tests { @@ -305,48 +250,6 @@ func TestUpdateSegment(t *testing.T) { Description: "desc", }, }, - { - name: "emptyKey", - req: &flipt.UpdateSegmentRequest{ - Key: "", - Name: "name", - Description: "desc", - }, - f: func(_ context.Context, r *flipt.UpdateSegmentRequest) (*flipt.Segment, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.Key) - assert.Equal(t, "name", r.Name) - assert.Equal(t, "desc", r.Description) - - return &flipt.Segment{ - Key: "", - Name: r.Name, - Description: r.Description, - }, nil - }, - wantErr: errors.EmptyFieldError("key"), - }, - { - name: "emptyName", - req: &flipt.UpdateSegmentRequest{ - Key: "key", - Name: "", - Description: "desc", - }, - f: func(_ context.Context, r *flipt.UpdateSegmentRequest) (*flipt.Segment, error) { - assert.NotNil(t, r) - assert.Equal(t, "key", r.Key) - assert.Equal(t, "name", r.Name) - assert.Equal(t, "desc", r.Description) - - return &flipt.Segment{ - Key: r.Key, - Name: "", - Description: r.Description, - }, nil - }, - wantErr: errors.EmptyFieldError("name"), - }, } for _, tt := range tests { @@ -389,16 +292,6 @@ func TestDeleteSegment(t *testing.T) { }, empty: &empty.Empty{}, }, - { - name: "emptyKey", - req: &flipt.DeleteSegmentRequest{Key: ""}, - f: func(_ context.Context, r *flipt.DeleteSegmentRequest) error { - assert.NotNil(t, r) - assert.Equal(t, "", r.Key) - return nil - }, - wantErr: errors.EmptyFieldError("key"), - }, { name: "error test", req: &flipt.DeleteSegmentRequest{Key: "key"}, @@ -474,87 +367,6 @@ func TestCreateConstraint(t *testing.T) { Value: "bar", }, }, - { - name: "emptySegmentKey", - req: &flipt.CreateConstraintRequest{ - SegmentKey: "", - Type: flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, - Property: "foo", - Operator: "EQ", - Value: "bar", - }, - f: func(_ context.Context, r *flipt.CreateConstraintRequest) (*flipt.Constraint, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.SegmentKey) - assert.Equal(t, flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, r.Type) - assert.Equal(t, "foo", r.Property) - assert.Equal(t, "EQ", r.Operator) - assert.Equal(t, "bar", r.Value) - - return &flipt.Constraint{ - SegmentKey: "", - Type: r.Type, - Property: r.Property, - Operator: r.Operator, - Value: r.Value, - }, nil - }, - wantErr: errors.EmptyFieldError("segmentKey"), - }, - { - name: "emptyProperty", - req: &flipt.CreateConstraintRequest{ - SegmentKey: "segmentKey", - Type: flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, - Property: "", - Operator: "EQ", - Value: "bar", - }, - f: func(_ context.Context, r *flipt.CreateConstraintRequest) (*flipt.Constraint, error) { - assert.NotNil(t, r) - assert.Equal(t, "segmentKey", r.SegmentKey) - assert.Equal(t, flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, r.Type) - assert.Equal(t, "", r.Property) - assert.Equal(t, "EQ", r.Operator) - assert.Equal(t, "bar", r.Value) - - return &flipt.Constraint{ - SegmentKey: r.SegmentKey, - Type: r.Type, - Property: "", - Operator: r.Operator, - Value: r.Value, - }, nil - }, - wantErr: errors.EmptyFieldError("property"), - }, - { - name: "emptyOperator", - req: &flipt.CreateConstraintRequest{ - SegmentKey: "segmentKey", - Type: flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, - Property: "foo", - Operator: "", - Value: "bar", - }, - f: func(_ context.Context, r *flipt.CreateConstraintRequest) (*flipt.Constraint, error) { - assert.NotNil(t, r) - assert.Equal(t, "segmentKey", r.SegmentKey) - assert.Equal(t, flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, r.Type) - assert.Equal(t, "foo", r.Property) - assert.Equal(t, "", r.Operator) - assert.Equal(t, "bar", r.Value) - - return &flipt.Constraint{ - SegmentKey: r.SegmentKey, - Type: r.Type, - Property: r.Property, - Operator: "", - Value: r.Value, - }, nil - }, - wantErr: errors.EmptyFieldError("operator"), - }, } for _, tt := range tests { @@ -624,126 +436,6 @@ func TestUpdateConstraint(t *testing.T) { Value: "bar", }, }, - { - name: "emptyID", - req: &flipt.UpdateConstraintRequest{ - Id: "", - SegmentKey: "segmentKey", - Type: flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, - Property: "foo", - Operator: "EQ", - Value: "bar", - }, - f: func(_ context.Context, r *flipt.UpdateConstraintRequest) (*flipt.Constraint, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.Id) - assert.Equal(t, "segmentKey", r.SegmentKey) - assert.Equal(t, flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, r.Type) - assert.Equal(t, "foo", r.Property) - assert.Equal(t, "EQ", r.Operator) - assert.Equal(t, "bar", r.Value) - - return &flipt.Constraint{ - Id: "", - SegmentKey: r.SegmentKey, - Type: r.Type, - Property: r.Property, - Operator: r.Operator, - Value: r.Value, - }, nil - }, - wantErr: errors.EmptyFieldError("id"), - }, - { - name: "emptySegmentKey", - req: &flipt.UpdateConstraintRequest{ - Id: "1", - SegmentKey: "", - Type: flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, - Property: "foo", - Operator: "EQ", - Value: "bar", - }, - f: func(_ context.Context, r *flipt.UpdateConstraintRequest) (*flipt.Constraint, error) { - assert.NotNil(t, r) - assert.Equal(t, "1", r.Id) - assert.Equal(t, "", r.SegmentKey) - assert.Equal(t, flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, r.Type) - assert.Equal(t, "foo", r.Property) - assert.Equal(t, "EQ", r.Operator) - assert.Equal(t, "bar", r.Value) - - return &flipt.Constraint{ - Id: r.Id, - SegmentKey: "", - Type: r.Type, - Property: r.Property, - Operator: r.Operator, - Value: r.Value, - }, nil - }, - wantErr: errors.EmptyFieldError("segmentKey"), - }, - { - name: "emptyProperty", - req: &flipt.UpdateConstraintRequest{ - Id: "1", - SegmentKey: "segmentKey", - Type: flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, - Property: "", - Operator: "EQ", - Value: "bar", - }, - f: func(_ context.Context, r *flipt.UpdateConstraintRequest) (*flipt.Constraint, error) { - assert.NotNil(t, r) - assert.Equal(t, "1", r.Id) - assert.Equal(t, "segmentKey", r.SegmentKey) - assert.Equal(t, flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, r.Type) - assert.Equal(t, "", r.Property) - assert.Equal(t, "EQ", r.Operator) - assert.Equal(t, "bar", r.Value) - - return &flipt.Constraint{ - Id: r.Id, - SegmentKey: r.SegmentKey, - Type: r.Type, - Property: "", - Operator: r.Operator, - Value: r.Value, - }, nil - }, - wantErr: errors.EmptyFieldError("property"), - }, - { - name: "emptyOperator", - req: &flipt.UpdateConstraintRequest{ - Id: "1", - SegmentKey: "segmentKey", - Type: flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, - Property: "foo", - Operator: "", - Value: "bar", - }, - f: func(_ context.Context, r *flipt.UpdateConstraintRequest) (*flipt.Constraint, error) { - assert.NotNil(t, r) - assert.Equal(t, "1", r.Id) - assert.Equal(t, "segmentKey", r.SegmentKey) - assert.Equal(t, flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, r.Type) - assert.Equal(t, "foo", r.Property) - assert.Equal(t, "", r.Operator) - assert.Equal(t, "bar", r.Value) - - return &flipt.Constraint{ - Id: r.Id, - SegmentKey: r.SegmentKey, - Type: r.Type, - Property: r.Property, - Operator: "", - Value: r.Value, - }, nil - }, - wantErr: errors.EmptyFieldError("operator"), - }, } for _, tt := range tests { @@ -787,28 +479,6 @@ func TestDeleteConstraint(t *testing.T) { }, empty: &empty.Empty{}, }, - { - name: "emptyID", - req: &flipt.DeleteConstraintRequest{Id: "", SegmentKey: "segmentKey"}, - f: func(_ context.Context, r *flipt.DeleteConstraintRequest) error { - assert.NotNil(t, r) - assert.Equal(t, "", r.Id) - assert.Equal(t, "segmentKey", r.SegmentKey) - return nil - }, - wantErr: errors.EmptyFieldError("id"), - }, - { - name: "emptySegmentKey", - req: &flipt.DeleteConstraintRequest{Id: "id", SegmentKey: ""}, - f: func(_ context.Context, r *flipt.DeleteConstraintRequest) error { - assert.NotNil(t, r) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "", r.SegmentKey) - return nil - }, - wantErr: errors.EmptyFieldError("segmentKey"), - }, { name: "error test", req: &flipt.DeleteConstraintRequest{Id: "id", SegmentKey: "segmentKey"}, diff --git a/server/server.go b/server/server.go index 64f6e4b939..96de1f184c 100644 --- a/server/server.go +++ b/server/server.go @@ -5,6 +5,7 @@ import ( "database/sql" "github.com/markphelps/flipt/errors" + flipt "github.com/markphelps/flipt/rpc" pb "github.com/markphelps/flipt/rpc" "github.com/markphelps/flipt/storage" "github.com/markphelps/flipt/storage/cache" @@ -58,6 +59,17 @@ func New(logger logrus.FieldLogger, builder sq.StatementBuilderType, db *sql.DB, return s } +// ValidationUnaryInterceptor validates incomming requests +func (s *Server) ValidationUnaryInterceptor(ctx context.Context, req interface{}, _ *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) { + if v, ok := req.(flipt.Validator); ok { + if err := v.Validate(); err != nil { + return nil, err + } + } + + return handler(ctx, req) +} + // ErrorUnaryInterceptor intercepts known errors and returns the appropriate GRPC status code func (s *Server) ErrorUnaryInterceptor(ctx context.Context, req interface{}, _ *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) { resp, err = handler(ctx, req) From 939c5d25e5c0922644c356ec1d6555169a45ebb2 Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Thu, 28 Nov 2019 09:15:47 -0500 Subject: [PATCH 05/12] More tests --- rpc/validation.go | 150 +++++++++++++++ rpc/validation_test.go | 359 ++++++++++++++++++++++++++++++++++ server/rule.go | 105 ---------- server/rule_test.go | 428 ----------------------------------------- 4 files changed, 509 insertions(+), 533 deletions(-) diff --git a/rpc/validation.go b/rpc/validation.go index da4f3a2ef8..546a8823ab 100644 --- a/rpc/validation.go +++ b/rpc/validation.go @@ -7,6 +7,156 @@ type Validator interface { Validate() error } +// Rules + +func (req *ListRuleRequest) Validate() error { + if req.FlagKey == "" { + return errors.EmptyFieldError("flagKey") + } + + return nil +} + +func (req *GetRuleRequest) Validate() error { + if req.Id == "" { + return errors.EmptyFieldError("id") + } + + if req.FlagKey == "" { + return errors.EmptyFieldError("flagKey") + } + + return nil +} + +func (req *CreateRuleRequest) Validate() error { + if req.FlagKey == "" { + return errors.EmptyFieldError("flagKey") + } + + if req.SegmentKey == "" { + return errors.EmptyFieldError("segmentKey") + } + + if req.Rank <= 0 { + return errors.InvalidFieldError("rank", "must be greater than 0") + } + + return nil +} + +func (req *DeleteRuleRequest) Validate() error { + if req.Id == "" { + return errors.EmptyFieldError("id") + } + + if req.FlagKey == "" { + return errors.EmptyFieldError("flagKey") + } + + return nil +} + +func (req *UpdateRuleRequest) Validate() error { + if req.Id == "" { + return errors.EmptyFieldError("id") + } + + if req.FlagKey == "" { + return errors.EmptyFieldError("flagKey") + } + + if req.SegmentKey == "" { + return errors.EmptyFieldError("segmentKey") + } + + return nil +} + +func (req *OrderRulesRequest) Validate() error { + if req.FlagKey == "" { + return errors.EmptyFieldError("flagKey") + } + + if len(req.RuleIds) < 2 { + return errors.InvalidFieldError("ruleIds", "must contain atleast 2 elements") + } + + return nil +} + +func (req *CreateDistributionRequest) Validate() error { + if req.FlagKey == "" { + return errors.EmptyFieldError("flagKey") + } + + if req.RuleId == "" { + return errors.EmptyFieldError("ruleId") + } + + if req.VariantId == "" { + return errors.EmptyFieldError("variantId") + } + + if req.Rollout < 0 { + return errors.InvalidFieldError("rollout", "must be greater than or equal to '0'") + } + + if req.Rollout > 100 { + return errors.InvalidFieldError("rollout", "must be less than or equal to '100'") + } + + return nil +} + +func (req *UpdateDistributionRequest) Validate() error { + if req.Id == "" { + return errors.EmptyFieldError("id") + } + + if req.FlagKey == "" { + return errors.EmptyFieldError("flagKey") + } + + if req.RuleId == "" { + return errors.EmptyFieldError("ruleId") + } + + if req.VariantId == "" { + return errors.EmptyFieldError("variantId") + } + + if req.Rollout < 0 { + return errors.InvalidFieldError("rollout", "must be greater than or equal to '0'") + } + + if req.Rollout > 100 { + return errors.InvalidFieldError("rollout", "must be less than or equal to '100'") + } + + return nil +} + +func (req *DeleteDistributionRequest) Validate() error { + if req.Id == "" { + return errors.EmptyFieldError("id") + } + + if req.FlagKey == "" { + return errors.EmptyFieldError("flagKey") + } + + if req.RuleId == "" { + return errors.EmptyFieldError("ruleId") + } + + if req.VariantId == "" { + return errors.EmptyFieldError("variantId") + } + + return nil +} + // Segments func (req *GetSegmentRequest) Validate() error { diff --git a/rpc/validation_test.go b/rpc/validation_test.go index 1c2d2feea0..12b4e9c2cb 100644 --- a/rpc/validation_test.go +++ b/rpc/validation_test.go @@ -7,6 +7,365 @@ import ( "github.com/stretchr/testify/assert" ) +func TestValidate_ListRuleRequest(t *testing.T) { + tests := []struct { + name string + req *ListRuleRequest + wantErr error + }{ + { + name: "emptyFlagKey", + req: &ListRuleRequest{FlagKey: ""}, + wantErr: errors.EmptyFieldError("flagKey"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_GetRuleRequest(t *testing.T) { + tests := []struct { + name string + req *GetRuleRequest + wantErr error + }{ + { + name: "emptyId", + req: &GetRuleRequest{Id: ""}, + wantErr: errors.EmptyFieldError("id"), + }, + { + name: "emptyFlagKey", + req: &GetRuleRequest{Id: "id", FlagKey: ""}, + wantErr: errors.EmptyFieldError("flagKey"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_CreateRuleRequest(t *testing.T) { + tests := []struct { + name string + req *CreateRuleRequest + wantErr error + }{ + { + name: "emptyFlagKey", + req: &CreateRuleRequest{ + FlagKey: "", + SegmentKey: "segmentKey", + Rank: 1, + }, + wantErr: errors.EmptyFieldError("flagKey"), + }, + { + name: "emptySegmentKey", + req: &CreateRuleRequest{ + FlagKey: "flagKey", + SegmentKey: "", + Rank: 1, + }, + wantErr: errors.EmptyFieldError("segmentKey"), + }, + { + name: "rankLessThanZero", + req: &CreateRuleRequest{ + FlagKey: "flagKey", + SegmentKey: "segmentKey", + Rank: -1, + }, + wantErr: errors.InvalidFieldError("rank", "must be greater than 0"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_UpdateRuleRequest(t *testing.T) { + tests := []struct { + name string + req *UpdateRuleRequest + wantErr error + }{ + { + name: "emptyID", + req: &UpdateRuleRequest{ + Id: "", + FlagKey: "flagKey", + SegmentKey: "segmentKey", + }, + wantErr: errors.EmptyFieldError("id"), + }, + { + name: "emptyFlagKey", + req: &UpdateRuleRequest{ + Id: "id", + FlagKey: "", + SegmentKey: "segmentKey", + }, + wantErr: errors.EmptyFieldError("flagKey"), + }, + { + name: "emptySegmentKey", + req: &UpdateRuleRequest{ + Id: "id", + FlagKey: "flagKey", + SegmentKey: "", + }, + wantErr: errors.EmptyFieldError("segmentKey"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_DeleteRuleRequest(t *testing.T) { + tests := []struct { + name string + req *DeleteRuleRequest + wantErr error + }{ + { + name: "emptyID", + req: &DeleteRuleRequest{ + Id: "", + FlagKey: "flagKey", + }, + wantErr: errors.EmptyFieldError("id"), + }, + { + name: "emptyFlagKey", + req: &DeleteRuleRequest{ + Id: "id", + FlagKey: "", + }, + wantErr: errors.EmptyFieldError("flagKey"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_OrderRulesRequest(t *testing.T) { + tests := []struct { + name string + req *OrderRulesRequest + wantErr error + }{ + { + name: "emptyFlagKey", + req: &OrderRulesRequest{FlagKey: "", RuleIds: []string{"1", "2"}}, + wantErr: errors.EmptyFieldError("flagKey"), + }, + { + name: "ruleIds length lesser than 2", + req: &OrderRulesRequest{FlagKey: "flagKey", RuleIds: []string{"1"}}, + wantErr: errors.InvalidFieldError("ruleIds", "must contain atleast 2 elements"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_CreateDistributionRequest(t *testing.T) { + tests := []struct { + name string + req *CreateDistributionRequest + wantErr error + }{ + { + name: "emptyFlagKey", + req: &CreateDistributionRequest{FlagKey: "", RuleId: "ruleID", VariantId: "variantID"}, + wantErr: errors.EmptyFieldError("flagKey"), + }, + { + name: "emptyRuleID", + req: &CreateDistributionRequest{FlagKey: "flagKey", RuleId: "", VariantId: "variantID"}, + wantErr: errors.EmptyFieldError("ruleId"), + }, + { + name: "emptyVariantID", + req: &CreateDistributionRequest{FlagKey: "flagKey", RuleId: "ruleID", VariantId: ""}, + wantErr: errors.EmptyFieldError("variantId"), + }, + { + name: "rollout is less than 0", + req: &CreateDistributionRequest{FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID", Rollout: -1}, + wantErr: errors.InvalidFieldError("rollout", "must be greater than or equal to '0'"), + }, + { + name: "rollout is more than 100", + req: &CreateDistributionRequest{FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID", Rollout: 101}, + wantErr: errors.InvalidFieldError("rollout", "must be less than or equal to '100'"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_UpdateDistributionRequest(t *testing.T) { + tests := []struct { + name string + req *UpdateDistributionRequest + wantErr error + }{ + { + name: "emptyID", + req: &UpdateDistributionRequest{Id: "", FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID"}, + wantErr: errors.EmptyFieldError("id"), + }, + { + name: "emptyFlagKey", + req: &UpdateDistributionRequest{Id: "id", FlagKey: "", RuleId: "ruleID", VariantId: "variantID"}, + wantErr: errors.EmptyFieldError("flagKey"), + }, + { + name: "emptyRuleID", + req: &UpdateDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "", VariantId: "variantID"}, + wantErr: errors.EmptyFieldError("ruleId"), + }, + { + name: "emptyVariantID", + req: &UpdateDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "ruleID", VariantId: ""}, + wantErr: errors.EmptyFieldError("variantId"), + }, + { + name: "rollout is less than 0", + req: &UpdateDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID", Rollout: -1}, + wantErr: errors.InvalidFieldError("rollout", "must be greater than or equal to '0'"), + }, + { + name: "rollout is more than 100", + req: &UpdateDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID", Rollout: 101}, + wantErr: errors.InvalidFieldError("rollout", "must be less than or equal to '100'"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_DeleteDistributionRequest(t *testing.T) { + tests := []struct { + name string + req *DeleteDistributionRequest + wantErr error + }{ + { + name: "emptyID", + req: &DeleteDistributionRequest{Id: "", FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID"}, + wantErr: errors.EmptyFieldError("id"), + }, + { + name: "emptyFlagKey", + req: &DeleteDistributionRequest{Id: "id", FlagKey: "", RuleId: "ruleID", VariantId: "variantID"}, + wantErr: errors.EmptyFieldError("flagKey"), + }, + { + name: "emptyRuleID", + req: &DeleteDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "", VariantId: "variantID"}, + wantErr: errors.EmptyFieldError("ruleId"), + }, + { + name: "emptyVariantID", + req: &DeleteDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "ruleID", VariantId: ""}, + wantErr: errors.EmptyFieldError("variantId"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + func TestValidate_GetSegmentRequest(t *testing.T) { tests := []struct { name string diff --git a/server/rule.go b/server/rule.go index 69b455b103..5852c40ce3 100644 --- a/server/rule.go +++ b/server/rule.go @@ -4,7 +4,6 @@ import ( "context" "github.com/golang/protobuf/ptypes/empty" - "github.com/markphelps/flipt/errors" flipt "github.com/markphelps/flipt/rpc" ) @@ -15,10 +14,6 @@ func (s *Server) GetRule(ctx context.Context, req *flipt.GetRuleRequest) (*flipt // ListRules lists all rules func (s *Server) ListRules(ctx context.Context, req *flipt.ListRuleRequest) (*flipt.RuleList, error) { - if req.FlagKey == "" { - return nil, errors.EmptyFieldError("flagKey") - } - rules, err := s.RuleStore.ListRules(ctx, req) if err != nil { return nil, err @@ -35,48 +30,16 @@ func (s *Server) ListRules(ctx context.Context, req *flipt.ListRuleRequest) (*fl // CreateRule creates a rule func (s *Server) CreateRule(ctx context.Context, req *flipt.CreateRuleRequest) (*flipt.Rule, error) { - if req.FlagKey == "" { - return nil, errors.EmptyFieldError("flagKey") - } - - if req.SegmentKey == "" { - return nil, errors.EmptyFieldError("segmentKey") - } - - if req.Rank <= 0 { - return nil, errors.InvalidFieldError("rank", "must be greater than 0") - } - return s.RuleStore.CreateRule(ctx, req) } // UpdateRule updates an existing rule func (s *Server) UpdateRule(ctx context.Context, req *flipt.UpdateRuleRequest) (*flipt.Rule, error) { - if req.Id == "" { - return nil, errors.EmptyFieldError("id") - } - - if req.FlagKey == "" { - return nil, errors.EmptyFieldError("flagKey") - } - - if req.SegmentKey == "" { - return nil, errors.EmptyFieldError("segmentKey") - } - return s.RuleStore.UpdateRule(ctx, req) } // DeleteRule deletes a rule func (s *Server) DeleteRule(ctx context.Context, req *flipt.DeleteRuleRequest) (*empty.Empty, error) { - if req.Id == "" { - return nil, errors.EmptyFieldError("id") - } - - if req.FlagKey == "" { - return nil, errors.EmptyFieldError("flagKey") - } - if err := s.RuleStore.DeleteRule(ctx, req); err != nil { return nil, err } @@ -86,14 +49,6 @@ func (s *Server) DeleteRule(ctx context.Context, req *flipt.DeleteRuleRequest) ( // OrderRules orders rules func (s *Server) OrderRules(ctx context.Context, req *flipt.OrderRulesRequest) (*empty.Empty, error) { - if req.FlagKey == "" { - return nil, errors.EmptyFieldError("flagKey") - } - - if len(req.RuleIds) < 2 { - return nil, errors.InvalidFieldError("ruleIds", "must contain atleast 2 elements") - } - if err := s.RuleStore.OrderRules(ctx, req); err != nil { return nil, err } @@ -103,76 +58,16 @@ func (s *Server) OrderRules(ctx context.Context, req *flipt.OrderRulesRequest) ( // CreateDistribution creates a distribution func (s *Server) CreateDistribution(ctx context.Context, req *flipt.CreateDistributionRequest) (*flipt.Distribution, error) { - if req.FlagKey == "" { - return nil, errors.EmptyFieldError("flagKey") - } - - if req.RuleId == "" { - return nil, errors.EmptyFieldError("ruleId") - } - - if req.VariantId == "" { - return nil, errors.EmptyFieldError("variantId") - } - - if req.Rollout < 0 { - return nil, errors.InvalidFieldError("rollout", "must be greater than or equal to '0'") - } - - if req.Rollout > 100 { - return nil, errors.InvalidFieldError("rollout", "must be less than or equal to '100'") - } - return s.RuleStore.CreateDistribution(ctx, req) } // UpdateDistribution updates an existing distribution func (s *Server) UpdateDistribution(ctx context.Context, req *flipt.UpdateDistributionRequest) (*flipt.Distribution, error) { - if req.Id == "" { - return nil, errors.EmptyFieldError("id") - } - - if req.FlagKey == "" { - return nil, errors.EmptyFieldError("flagKey") - } - - if req.RuleId == "" { - return nil, errors.EmptyFieldError("ruleId") - } - - if req.VariantId == "" { - return nil, errors.EmptyFieldError("variantId") - } - - if req.Rollout < 0 { - return nil, errors.InvalidFieldError("rollout", "must be greater than or equal to '0'") - } - - if req.Rollout > 100 { - return nil, errors.InvalidFieldError("rollout", "must be less than or equal to '100'") - } - return s.RuleStore.UpdateDistribution(ctx, req) } // DeleteDistribution deletes a distribution func (s *Server) DeleteDistribution(ctx context.Context, req *flipt.DeleteDistributionRequest) (*empty.Empty, error) { - if req.Id == "" { - return nil, errors.EmptyFieldError("id") - } - - if req.FlagKey == "" { - return nil, errors.EmptyFieldError("flagKey") - } - - if req.RuleId == "" { - return nil, errors.EmptyFieldError("ruleId") - } - - if req.VariantId == "" { - return nil, errors.EmptyFieldError("variantId") - } - if err := s.RuleStore.DeleteDistribution(ctx, req); err != nil { return nil, err } diff --git a/server/rule_test.go b/server/rule_test.go index 50d9c6f28b..19e7eedf8e 100644 --- a/server/rule_test.go +++ b/server/rule_test.go @@ -132,19 +132,6 @@ func TestListRules(t *testing.T) { }, }, }, - { - name: "emptyFlagKey", - req: &flipt.ListRuleRequest{FlagKey: ""}, - f: func(ctx context.Context, r *flipt.ListRuleRequest) ([]*flipt.Rule, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.FlagKey) - - return []*flipt.Rule{ - {FlagKey: ""}, - }, nil - }, - wantErr: errors.EmptyFieldError("flagKey"), - }, { name: "error test", req: &flipt.ListRuleRequest{FlagKey: "flagKey"}, @@ -213,69 +200,6 @@ func TestCreateRule(t *testing.T) { Rank: int32(1), }, }, - { - name: "emptyFlagKey", - req: &flipt.CreateRuleRequest{ - FlagKey: "", - SegmentKey: "segmentKey", - Rank: 1, - }, - f: func(_ context.Context, r *flipt.CreateRuleRequest) (*flipt.Rule, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.FlagKey) - assert.Equal(t, "segmentKey", r.SegmentKey) - assert.Equal(t, int32(1), r.Rank) - - return &flipt.Rule{ - FlagKey: "", - SegmentKey: r.SegmentKey, - Rank: r.Rank, - }, nil - }, - wantErr: errors.EmptyFieldError("flagKey"), - }, - { - name: "emptySegmentKey", - req: &flipt.CreateRuleRequest{ - FlagKey: "flagKey", - SegmentKey: "", - Rank: 1, - }, - f: func(_ context.Context, r *flipt.CreateRuleRequest) (*flipt.Rule, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "", r.SegmentKey) - assert.Equal(t, int32(1), r.Rank) - - return &flipt.Rule{ - FlagKey: r.FlagKey, - SegmentKey: "", - Rank: r.Rank, - }, nil - }, - wantErr: errors.EmptyFieldError("segmentKey"), - }, - { - name: "rank_lesser_than_0", - req: &flipt.CreateRuleRequest{ - FlagKey: "flagKey", - SegmentKey: "segmentKey", - Rank: -1, - }, - f: func(_ context.Context, r *flipt.CreateRuleRequest) (*flipt.Rule, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "segmentKey", r.SegmentKey) - assert.Equal(t, int32(-1), r.Rank) - - return &flipt.Rule{ - FlagKey: r.FlagKey, - SegmentKey: r.SegmentKey, - Rank: r.Rank, - }, nil - }, - wantErr: errors.InvalidFieldError("rank", "must be greater than 0"), - }, } for _, tt := range tests { @@ -333,69 +257,6 @@ func TestUpdateRule(t *testing.T) { SegmentKey: "segmentKey", }, }, - { - name: "emptyID", - req: &flipt.UpdateRuleRequest{ - Id: "", - FlagKey: "flagKey", - SegmentKey: "segmentKey", - }, - f: func(_ context.Context, r *flipt.UpdateRuleRequest) (*flipt.Rule, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.Id) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "segmentKey", r.SegmentKey) - - return &flipt.Rule{ - Id: "", - FlagKey: r.FlagKey, - SegmentKey: r.SegmentKey, - }, nil - }, - wantErr: errors.EmptyFieldError("id"), - }, - { - name: "emptyFlagKey", - req: &flipt.UpdateRuleRequest{ - Id: "id", - FlagKey: "", - SegmentKey: "segmentKey", - }, - f: func(_ context.Context, r *flipt.UpdateRuleRequest) (*flipt.Rule, error) { - assert.NotNil(t, r) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "", r.FlagKey) - assert.Equal(t, "segmentKey", r.SegmentKey) - - return &flipt.Rule{ - Id: r.Id, - FlagKey: "", - SegmentKey: r.SegmentKey, - }, nil - }, - wantErr: errors.EmptyFieldError("flagKey"), - }, - { - name: "emptySegmentKey", - req: &flipt.UpdateRuleRequest{ - Id: "id", - FlagKey: "flagKey", - SegmentKey: "", - }, - f: func(_ context.Context, r *flipt.UpdateRuleRequest) (*flipt.Rule, error) { - assert.NotNil(t, r) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "", r.SegmentKey) - - return &flipt.Rule{ - Id: r.Id, - FlagKey: r.FlagKey, - SegmentKey: "", - }, nil - }, - wantErr: errors.EmptyFieldError("segmentKey"), - }, } for _, tt := range tests { @@ -439,28 +300,6 @@ func TestDeleteRule(t *testing.T) { }, empty: &empty.Empty{}, }, - { - name: "emptyID", - req: &flipt.DeleteRuleRequest{Id: "", FlagKey: "flagKey"}, - f: func(_ context.Context, r *flipt.DeleteRuleRequest) error { - assert.NotNil(t, r) - assert.Equal(t, "", r.Id) - assert.Equal(t, "flagKey", r.FlagKey) - return nil - }, - wantErr: errors.EmptyFieldError("id"), - }, - { - name: "emptyFlagKey", - req: &flipt.DeleteRuleRequest{Id: "id", FlagKey: ""}, - f: func(_ context.Context, r *flipt.DeleteRuleRequest) error { - assert.NotNil(t, r) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "", r.FlagKey) - return nil - }, - wantErr: errors.EmptyFieldError("flagKey"), - }, { name: "error test", req: &flipt.DeleteRuleRequest{Id: "id", FlagKey: "flagKey"}, @@ -515,29 +354,6 @@ func TestOrderRules(t *testing.T) { }, empty: &empty.Empty{}, }, - { - name: "emptyFlagKey", - req: &flipt.OrderRulesRequest{FlagKey: "", RuleIds: []string{"1", "2"}}, - f: func(_ context.Context, r *flipt.OrderRulesRequest) error { - assert.NotNil(t, r) - assert.Equal(t, "", r.FlagKey) - assert.Equal(t, []string{"1", "2"}, r.RuleIds) - return nil - }, - wantErr: errors.EmptyFieldError("flagKey"), - }, - { - name: "ruleIds length lesser than 2", - req: &flipt.OrderRulesRequest{FlagKey: "flagKey", RuleIds: []string{"1"}}, - f: func(_ context.Context, r *flipt.OrderRulesRequest) error { - assert.NotNil(t, r) - assert.Equal(t, "", r.FlagKey) - assert.Equal(t, []string{"1"}, r.RuleIds) - assert.Equal(t, 1, len(r.RuleIds)) - return nil - }, - wantErr: errors.InvalidFieldError("ruleIds", "must contain atleast 2 elements"), - }, { name: "error test", req: &flipt.OrderRulesRequest{FlagKey: "flagKey", RuleIds: []string{"1", "2"}}, @@ -600,86 +416,6 @@ func TestCreateDistribution(t *testing.T) { VariantId: "variantID", }, }, - { - name: "emptyFlagKey", - req: &flipt.CreateDistributionRequest{FlagKey: "", RuleId: "ruleID", VariantId: "variantID"}, - f: func(ctx context.Context, r *flipt.CreateDistributionRequest) (*flipt.Distribution, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.FlagKey) - assert.Equal(t, "ruleID", r.RuleId) - assert.Equal(t, "variantID", r.VariantId) - - return &flipt.Distribution{ - RuleId: "ruleID", - VariantId: "variantID", - }, nil - }, - wantErr: errors.EmptyFieldError("flagKey"), - }, - { - name: "emptyRuleID", - req: &flipt.CreateDistributionRequest{FlagKey: "flagKey", RuleId: "", VariantId: "variantID"}, - f: func(ctx context.Context, r *flipt.CreateDistributionRequest) (*flipt.Distribution, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "", r.RuleId) - assert.Equal(t, "variantID", r.VariantId) - - return &flipt.Distribution{ - RuleId: "", - VariantId: "variantID", - }, nil - }, - wantErr: errors.EmptyFieldError("ruleId"), - }, - { - name: "emptyVariantID", - req: &flipt.CreateDistributionRequest{FlagKey: "flagKey", RuleId: "ruleID", VariantId: ""}, - f: func(ctx context.Context, r *flipt.CreateDistributionRequest) (*flipt.Distribution, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "ruleID", r.RuleId) - assert.Equal(t, "", r.VariantId) - - return &flipt.Distribution{ - RuleId: "ruleID", - VariantId: "", - }, nil - }, - wantErr: errors.EmptyFieldError("variantId"), - }, - { - name: "rollout is less than 0", - req: &flipt.CreateDistributionRequest{FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID", Rollout: -1}, - f: func(ctx context.Context, r *flipt.CreateDistributionRequest) (*flipt.Distribution, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "ruleID", r.RuleId) - assert.Equal(t, "variantID", r.VariantId) - - return &flipt.Distribution{ - RuleId: "ruleID", - VariantId: "variantID", - }, nil - }, - wantErr: errors.InvalidFieldError("rollout", "must be greater than or equal to '0'"), - }, - { - name: "rollout is more than 100", - req: &flipt.CreateDistributionRequest{FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID", Rollout: 101}, - f: func(ctx context.Context, r *flipt.CreateDistributionRequest) (*flipt.Distribution, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "ruleID", r.RuleId) - assert.Equal(t, "variantID", r.VariantId) - - return &flipt.Distribution{ - RuleId: "ruleID", - VariantId: "variantID", - }, nil - }, - wantErr: errors.InvalidFieldError("rollout", "must be less than or equal to '100'"), - }, } for _, tt := range tests { @@ -734,114 +470,6 @@ func TestUpdateDistribution(t *testing.T) { VariantId: "variantID", }, }, - { - name: "emptyID", - req: &flipt.UpdateDistributionRequest{Id: "", FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID"}, - f: func(_ context.Context, r *flipt.UpdateDistributionRequest) (*flipt.Distribution, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "", r.Id) - assert.Equal(t, "ruleID", r.RuleId) - assert.Equal(t, "variantID", r.VariantId) - - return &flipt.Distribution{ - Id: "", - RuleId: r.RuleId, - VariantId: r.VariantId, - }, nil - }, - wantErr: errors.EmptyFieldError("id"), - }, - { - name: "emptyFlagKey", - req: &flipt.UpdateDistributionRequest{Id: "id", FlagKey: "", RuleId: "ruleID", VariantId: "variantID"}, - f: func(_ context.Context, r *flipt.UpdateDistributionRequest) (*flipt.Distribution, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.FlagKey) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "ruleID", r.RuleId) - assert.Equal(t, "variantID", r.VariantId) - - return &flipt.Distribution{ - Id: r.Id, - RuleId: r.RuleId, - VariantId: r.VariantId, - }, nil - }, - wantErr: errors.EmptyFieldError("flagKey"), - }, - { - name: "emptyRuleID", - req: &flipt.UpdateDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "", VariantId: "variantID"}, - f: func(_ context.Context, r *flipt.UpdateDistributionRequest) (*flipt.Distribution, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "", r.RuleId) - assert.Equal(t, "variantID", r.VariantId) - - return &flipt.Distribution{ - Id: r.Id, - RuleId: "", - VariantId: r.VariantId, - }, nil - }, - wantErr: errors.EmptyFieldError("ruleId"), - }, - { - name: "emptyVariantID", - req: &flipt.UpdateDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "ruleID", VariantId: ""}, - f: func(_ context.Context, r *flipt.UpdateDistributionRequest) (*flipt.Distribution, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "ruleID", r.RuleId) - assert.Equal(t, "", r.VariantId) - - return &flipt.Distribution{ - Id: r.Id, - RuleId: r.RuleId, - VariantId: "", - }, nil - }, - wantErr: errors.EmptyFieldError("variantId"), - }, - { - name: "rollout is lesser than 0", - req: &flipt.UpdateDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID", Rollout: -1}, - f: func(_ context.Context, r *flipt.UpdateDistributionRequest) (*flipt.Distribution, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "ruleID", r.RuleId) - assert.Equal(t, "variantID", r.VariantId) - - return &flipt.Distribution{ - Id: r.Id, - RuleId: r.RuleId, - VariantId: r.VariantId, - }, nil - }, - wantErr: errors.InvalidFieldError("rollout", "must be greater than or equal to '0'"), - }, - { - name: "rollout is greater than 100", - req: &flipt.UpdateDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID", Rollout: 101}, - f: func(_ context.Context, r *flipt.UpdateDistributionRequest) (*flipt.Distribution, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "ruleID", r.RuleId) - assert.Equal(t, "variantID", r.VariantId) - - return &flipt.Distribution{ - Id: r.Id, - RuleId: r.RuleId, - VariantId: r.VariantId, - }, nil - }, - wantErr: errors.InvalidFieldError("rollout", "must be less than or equal to '100'"), - }, } for _, tt := range tests { @@ -888,62 +516,6 @@ func TestDeleteDistribution(t *testing.T) { }, empty: &empty.Empty{}, }, - { - name: "emptyID", - req: &flipt.DeleteDistributionRequest{Id: "", FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID"}, - f: func(_ context.Context, r *flipt.DeleteDistributionRequest) error { - assert.NotNil(t, r) - assert.Equal(t, "", r.Id) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "ruleID", r.RuleId) - assert.Equal(t, "variantID", r.VariantId) - - return nil - }, - wantErr: errors.EmptyFieldError("id"), - }, - { - name: "emptyFlagKey", - req: &flipt.DeleteDistributionRequest{Id: "id", FlagKey: "", RuleId: "ruleID", VariantId: "variantID"}, - f: func(_ context.Context, r *flipt.DeleteDistributionRequest) error { - assert.NotNil(t, r) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "", r.FlagKey) - assert.Equal(t, "ruleID", r.RuleId) - assert.Equal(t, "variantID", r.VariantId) - - return nil - }, - wantErr: errors.EmptyFieldError("flagKey"), - }, - { - name: "emptyRuleID", - req: &flipt.DeleteDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "", VariantId: "variantID"}, - f: func(_ context.Context, r *flipt.DeleteDistributionRequest) error { - assert.NotNil(t, r) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "", r.RuleId) - assert.Equal(t, "variantID", r.VariantId) - - return nil - }, - wantErr: errors.EmptyFieldError("ruleId"), - }, - { - name: "emptyVariantID", - req: &flipt.DeleteDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "ruleID", VariantId: ""}, - f: func(_ context.Context, r *flipt.DeleteDistributionRequest) error { - assert.NotNil(t, r) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "ruleID", r.RuleId) - assert.Equal(t, "", r.VariantId) - - return nil - }, - wantErr: errors.EmptyFieldError("variantId"), - }, { name: "error test", req: &flipt.DeleteDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID"}, From 1502fddd997f12a7254f20b5577d27937a4d45da Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Thu, 28 Nov 2019 11:21:49 -0500 Subject: [PATCH 06/12] Move operators --- rpc/operators.go | 67 ++++++++++++++++++ rpc/validation.go | 43 +++++++++++- rpc/validation_test.go | 92 ++++++++++++++++++++++++ storage/evaluator.go | 122 +++++--------------------------- storage/evaluator_test.go | 76 +++----------------- storage/segment.go | 39 +---------- storage/segment_test.go | 143 +------------------------------------- 7 files changed, 232 insertions(+), 350 deletions(-) create mode 100644 rpc/operators.go diff --git a/rpc/operators.go b/rpc/operators.go new file mode 100644 index 0000000000..4a1f260ad6 --- /dev/null +++ b/rpc/operators.go @@ -0,0 +1,67 @@ +package flipt + +const ( + OpEQ = "eq" + OpNEQ = "neq" + OpLT = "lt" + OpLTE = "lte" + OpGT = "gt" + OpGTE = "gte" + OpEmpty = "empty" + OpNotEmpty = "notempty" + OpTrue = "true" + OpFalse = "false" + OpPresent = "present" + OpNotPresent = "notpresent" + OpPrefix = "prefix" + OpSuffix = "suffix" +) + +var ( + ValidOperators = map[string]struct{}{ + OpEQ: {}, + OpNEQ: {}, + OpLT: {}, + OpLTE: {}, + OpGT: {}, + OpGTE: {}, + OpEmpty: {}, + OpNotEmpty: {}, + OpTrue: {}, + OpFalse: {}, + OpPresent: {}, + OpNotPresent: {}, + OpPrefix: {}, + OpSuffix: {}, + } + NoValueOperators = map[string]struct{}{ + OpEmpty: {}, + OpNotEmpty: {}, + OpPresent: {}, + OpNotPresent: {}, + } + StringOperators = map[string]struct{}{ + OpEQ: {}, + OpNEQ: {}, + OpEmpty: {}, + OpNotEmpty: {}, + OpPrefix: {}, + OpSuffix: {}, + } + NumberOperators = map[string]struct{}{ + OpEQ: {}, + OpNEQ: {}, + OpLT: {}, + OpLTE: {}, + OpGT: {}, + OpGTE: {}, + OpPresent: {}, + OpNotPresent: {}, + } + BooleanOperators = map[string]struct{}{ + OpTrue: {}, + OpFalse: {}, + OpPresent: {}, + OpNotPresent: {}, + } +) diff --git a/rpc/validation.go b/rpc/validation.go index 546a8823ab..76229e9c14 100644 --- a/rpc/validation.go +++ b/rpc/validation.go @@ -1,6 +1,9 @@ package flipt -import "github.com/markphelps/flipt/errors" +import ( + "github.com/markphelps/flipt/errors" + "strings" +) // Validator validates types type Validator interface { @@ -212,6 +215,25 @@ func (req *CreateConstraintRequest) Validate() error { return errors.EmptyFieldError("operator") } + operator := strings.ToLower(req.Operator) + // validate operator works for this constraint type + switch req.Type { + case ComparisonType_STRING_COMPARISON_TYPE: + if _, ok := StringOperators[operator]; !ok { + return errors.ErrInvalidf("constraint operator %q is not valid for type string", req.Operator) + } + case ComparisonType_NUMBER_COMPARISON_TYPE: + if _, ok := NumberOperators[operator]; !ok { + return errors.ErrInvalidf("constraint operator %q is not valid for type number", req.Operator) + } + case ComparisonType_BOOLEAN_COMPARISON_TYPE: + if _, ok := BooleanOperators[operator]; !ok { + return errors.ErrInvalidf("constraint operator %q is not valid for type boolean", req.Operator) + } + default: + return errors.ErrInvalidf("invalid constraint type: %q", req.Type.String()) + } + // TODO: test for empty value if operator ! [EMPTY, NOT_EMPTY, PRESENT, NOT_PRESENT] return nil } @@ -233,6 +255,25 @@ func (req *UpdateConstraintRequest) Validate() error { return errors.EmptyFieldError("operator") } + operator := strings.ToLower(req.Operator) + // validate operator works for this constraint type + switch req.Type { + case ComparisonType_STRING_COMPARISON_TYPE: + if _, ok := StringOperators[operator]; !ok { + return errors.ErrInvalidf("constraint operator %q is not valid for type string", req.Operator) + } + case ComparisonType_NUMBER_COMPARISON_TYPE: + if _, ok := NumberOperators[operator]; !ok { + return errors.ErrInvalidf("constraint operator %q is not valid for type number", req.Operator) + } + case ComparisonType_BOOLEAN_COMPARISON_TYPE: + if _, ok := BooleanOperators[operator]; !ok { + return errors.ErrInvalidf("constraint operator %q is not valid for type boolean", req.Operator) + } + default: + return errors.ErrInvalidf("invalid constraint type: %q", req.Type.String()) + } + // TODO: test for empty value if operator ! [EMPTY, NOT_EMPTY, PRESENT, NOT_PRESENT] return nil } diff --git a/rpc/validation_test.go b/rpc/validation_test.go index 12b4e9c2cb..95402ddf0d 100644 --- a/rpc/validation_test.go +++ b/rpc/validation_test.go @@ -535,6 +535,50 @@ func TestValidate_CreateConstraintRequest(t *testing.T) { }, wantErr: errors.EmptyFieldError("operator"), }, + { + name: "invalidStringType", + req: &CreateConstraintRequest{ + SegmentKey: "segmentKey", + Type: ComparisonType_STRING_COMPARISON_TYPE, + Property: "foo", + Operator: "false", + Value: "bar", + }, + wantErr: errors.ErrInvalid("constraint operator \"false\" is not valid for type string"), + }, + { + name: "invalidNumberType", + req: &CreateConstraintRequest{ + SegmentKey: "segmentKey", + Type: ComparisonType_NUMBER_COMPARISON_TYPE, + Property: "foo", + Operator: "false", + Value: "bar", + }, + wantErr: errors.ErrInvalid("constraint operator \"false\" is not valid for type number"), + }, + { + name: "invalidBooleanType", + req: &CreateConstraintRequest{ + SegmentKey: "segmentKey", + Type: ComparisonType_BOOLEAN_COMPARISON_TYPE, + Property: "foo", + Operator: "eq", + Value: "bar", + }, + wantErr: errors.ErrInvalid("constraint operator \"eq\" is not valid for type boolean"), + }, + { + name: "invalidType", + req: &CreateConstraintRequest{ + SegmentKey: "segmentKey", + Type: ComparisonType_UNKNOWN_COMPARISON_TYPE, + Property: "foo", + Operator: "eq", + Value: "bar", + }, + wantErr: errors.ErrInvalid("invalid constraint type: \"UNKNOWN_COMPARISON_TYPE\""), + }, } for _, tt := range tests { @@ -604,6 +648,54 @@ func TestValidate_UpdateConstraintRequest(t *testing.T) { }, wantErr: errors.EmptyFieldError("operator"), }, + { + name: "invalidStringType", + req: &UpdateConstraintRequest{ + Id: "1", + SegmentKey: "segmentKey", + Type: ComparisonType_STRING_COMPARISON_TYPE, + Property: "foo", + Operator: "false", + Value: "bar", + }, + wantErr: errors.ErrInvalid("constraint operator \"false\" is not valid for type string"), + }, + { + name: "invalidNumberType", + req: &UpdateConstraintRequest{ + Id: "1", + SegmentKey: "segmentKey", + Type: ComparisonType_NUMBER_COMPARISON_TYPE, + Property: "foo", + Operator: "false", + Value: "bar", + }, + wantErr: errors.ErrInvalid("constraint operator \"false\" is not valid for type number"), + }, + { + name: "invalidBooleanType", + req: &UpdateConstraintRequest{ + Id: "1", + SegmentKey: "segmentKey", + Type: ComparisonType_BOOLEAN_COMPARISON_TYPE, + Property: "foo", + Operator: "eq", + Value: "bar", + }, + wantErr: errors.ErrInvalid("constraint operator \"eq\" is not valid for type boolean"), + }, + { + name: "invalidType", + req: &UpdateConstraintRequest{ + Id: "1", + SegmentKey: "segmentKey", + Type: ComparisonType_UNKNOWN_COMPARISON_TYPE, + Property: "foo", + Operator: "eq", + Value: "bar", + }, + wantErr: errors.ErrInvalid("invalid constraint type: \"UNKNOWN_COMPARISON_TYPE\""), + }, } for _, tt := range tests { diff --git a/storage/evaluator.go b/storage/evaluator.go index b9c4c52206..4bfb91ccdb 100644 --- a/storage/evaluator.go +++ b/storage/evaluator.go @@ -186,9 +186,6 @@ func (s *EvaluatorStorage) Evaluate(ctx context.Context, r *flipt.EvaluationRequ constraintMatches := 0 for _, c := range rule.Constraints { - if err := validate(c); err != nil { - return resp, err - } v := r.Context[c.Property] @@ -329,72 +326,6 @@ func crc32Num(entityID string, salt string) uint { return uint(crc32.ChecksumIEEE([]byte(salt+entityID))) % totalBucketNum } -const ( - opEQ = "eq" - opNEQ = "neq" - opLT = "lt" - opLTE = "lte" - opGT = "gt" - opGTE = "gte" - opEmpty = "empty" - opNotEmpty = "notempty" - opTrue = "true" - opFalse = "false" - opPresent = "present" - opNotPresent = "notpresent" - opPrefix = "prefix" - opSuffix = "suffix" -) - -var ( - validOperators = map[string]struct{}{ - opEQ: {}, - opNEQ: {}, - opLT: {}, - opLTE: {}, - opGT: {}, - opGTE: {}, - opEmpty: {}, - opNotEmpty: {}, - opTrue: {}, - opFalse: {}, - opPresent: {}, - opNotPresent: {}, - opPrefix: {}, - opSuffix: {}, - } - noValueOperators = map[string]struct{}{ - opEmpty: {}, - opNotEmpty: {}, - opPresent: {}, - opNotPresent: {}, - } - stringOperators = map[string]struct{}{ - opEQ: {}, - opNEQ: {}, - opEmpty: {}, - opNotEmpty: {}, - opPrefix: {}, - opSuffix: {}, - } - numberOperators = map[string]struct{}{ - opEQ: {}, - opNEQ: {}, - opLT: {}, - opLTE: {}, - opGT: {}, - opGTE: {}, - opPresent: {}, - opNotPresent: {}, - } - booleanOperators = map[string]struct{}{ - opTrue: {}, - opFalse: {}, - opPresent: {}, - opNotPresent: {}, - } -) - const ( // totalBucketNum represents how many buckets we can use to determine the consistent hashing // distribution and rollout @@ -404,28 +335,11 @@ const ( percentMultiplier float32 = float32(totalBucketNum) / 100 ) -func validate(c constraint) error { - if c.Property == "" { - return errors.New("empty property") - } - - if c.Operator == "" { - return errors.New("empty operator") - } - - op := strings.ToLower(c.Operator) - if _, ok := validOperators[op]; !ok { - return fmt.Errorf("unsupported operator: %q", op) - } - - return nil -} - func matchesString(c constraint, v string) bool { switch c.Operator { - case opEmpty: + case flipt.OpEmpty: return len(strings.TrimSpace(v)) == 0 - case opNotEmpty: + case flipt.OpNotEmpty: return len(strings.TrimSpace(v)) != 0 } @@ -436,13 +350,13 @@ func matchesString(c constraint, v string) bool { value := c.Value switch c.Operator { - case opEQ: + case flipt.OpEQ: return value == v - case opNEQ: + case flipt.OpNEQ: return value != v - case opPrefix: + case flipt.OpPrefix: return strings.HasPrefix(strings.TrimSpace(v), value) - case opSuffix: + case flipt.OpSuffix: return strings.HasSuffix(strings.TrimSpace(v), value) } @@ -451,9 +365,9 @@ func matchesString(c constraint, v string) bool { func matchesNumber(c constraint, v string) (bool, error) { switch c.Operator { - case opNotPresent: + case flipt.OpNotPresent: return len(strings.TrimSpace(v)) == 0, nil - case opPresent: + case flipt.OpPresent: return len(strings.TrimSpace(v)) != 0, nil } @@ -473,17 +387,17 @@ func matchesNumber(c constraint, v string) (bool, error) { } switch c.Operator { - case opEQ: + case flipt.OpEQ: return value == n, nil - case opNEQ: + case flipt.OpNEQ: return value != n, nil - case opLT: + case flipt.OpLT: return n < value, nil - case opLTE: + case flipt.OpLTE: return n <= value, nil - case opGT: + case flipt.OpGT: return n > value, nil - case opGTE: + case flipt.OpGTE: return n >= value, nil } @@ -492,9 +406,9 @@ func matchesNumber(c constraint, v string) (bool, error) { func matchesBool(c constraint, v string) (bool, error) { switch c.Operator { - case opNotPresent: + case flipt.OpNotPresent: return len(strings.TrimSpace(v)) == 0, nil - case opPresent: + case flipt.OpPresent: return len(strings.TrimSpace(v)) != 0, nil } @@ -509,9 +423,9 @@ func matchesBool(c constraint, v string) (bool, error) { } switch c.Operator { - case opTrue: + case flipt.OpTrue: return value, nil - case opFalse: + case flipt.OpFalse: return !value, nil } diff --git a/storage/evaluator_test.go b/storage/evaluator_test.go index afab6bef34..8c6da82ca4 100644 --- a/storage/evaluator_test.go +++ b/storage/evaluator_test.go @@ -97,7 +97,7 @@ func TestEvaluate_NoVariants_NoDistributions(t *testing.T) { SegmentKey: segment.Key, Type: flipt.ComparisonType_STRING_COMPARISON_TYPE, Property: "bar", - Operator: opEQ, + Operator: flipt.OpEQ, Value: "baz", }) @@ -205,7 +205,7 @@ func TestEvaluate_SingleVariantDistribution(t *testing.T) { SegmentKey: segment.Key, Type: flipt.ComparisonType_STRING_COMPARISON_TYPE, Property: "bar", - Operator: opEQ, + Operator: flipt.OpEQ, Value: "baz", }) @@ -216,7 +216,7 @@ func TestEvaluate_SingleVariantDistribution(t *testing.T) { SegmentKey: segment.Key, Type: flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, Property: "admin", - Operator: opTrue, + Operator: flipt.OpTrue, }) require.NoError(t, err) @@ -353,7 +353,7 @@ func TestEvaluate_RolloutDistribution(t *testing.T) { SegmentKey: segment.Key, Type: flipt.ComparisonType_STRING_COMPARISON_TYPE, Property: "bar", - Operator: opEQ, + Operator: flipt.OpEQ, Value: "baz", }) @@ -494,7 +494,7 @@ func TestEvaluate_RolloutDistribution_WithMatchAll(t *testing.T) { SegmentKey: subscriberSegment.Key, Type: flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, Property: "premium_user", - Operator: opTrue, + Operator: flipt.OpTrue, }) require.NoError(t, err) @@ -696,66 +696,6 @@ func TestEvaluate_NoConstraints(t *testing.T) { } } -func Test_validate(t *testing.T) { - tests := []struct { - name string - constraint constraint - wantErr bool - }{ - { - name: "missing property", - constraint: constraint{ - Operator: "eq", - Value: "bar", - }, - wantErr: true, - }, - { - name: "missing operator", - constraint: constraint{ - Property: "foo", - Value: "bar", - }, - wantErr: true, - }, - { - name: "invalid operator", - constraint: constraint{ - Property: "foo", - Operator: "?", - Value: "bar", - }, - wantErr: true, - }, - { - name: "valid", - constraint: constraint{ - Property: "foo", - Operator: "eq", - Value: "bar", - }, - }, - } - - for _, tt := range tests { - var ( - constraint = tt.constraint - wantErr = tt.wantErr - ) - - t.Run(tt.name, func(t *testing.T) { - err := validate(constraint) - - if wantErr { - require.Error(t, err) - return - } - - require.NoError(t, err) - }) - } -} - func Test_matchesString(t *testing.T) { tests := []struct { name string @@ -1434,7 +1374,7 @@ func BenchmarkEvaluate_SingleVariantDistribution(b *testing.B) { SegmentKey: segment.Key, Type: flipt.ComparisonType_STRING_COMPARISON_TYPE, Property: "bar", - Operator: opEQ, + Operator: flipt.OpEQ, Value: "baz", }) @@ -1442,7 +1382,7 @@ func BenchmarkEvaluate_SingleVariantDistribution(b *testing.B) { SegmentKey: segment.Key, Type: flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, Property: "admin", - Operator: opTrue, + Operator: flipt.OpTrue, }) rule, _ := ruleStore.CreateRule(context.TODO(), &flipt.CreateRuleRequest{ @@ -1552,7 +1492,7 @@ func BenchmarkEvaluate_RolloutDistribution(b *testing.B) { SegmentKey: segment.Key, Type: flipt.ComparisonType_STRING_COMPARISON_TYPE, Property: "bar", - Operator: opEQ, + Operator: flipt.OpEQ, Value: "baz", }) diff --git a/storage/segment.go b/storage/segment.go index dad44df363..e2f9a7117a 100644 --- a/storage/segment.go +++ b/storage/segment.go @@ -251,26 +251,8 @@ func (s *SegmentStorage) CreateConstraint(ctx context.Context, r *flipt.CreateCo } ) - // validate operator works for this constraint type - switch c.Type { - case flipt.ComparisonType_STRING_COMPARISON_TYPE: - if _, ok := stringOperators[c.Operator]; !ok { - return nil, errors.ErrInvalidf("constraint operator %q is not valid for type string", r.Operator) - } - case flipt.ComparisonType_NUMBER_COMPARISON_TYPE: - if _, ok := numberOperators[c.Operator]; !ok { - return nil, errors.ErrInvalidf("constraint operator %q is not valid for type number", r.Operator) - } - case flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE: - if _, ok := booleanOperators[c.Operator]; !ok { - return nil, errors.ErrInvalidf("constraint operator %q is not valid for type boolean", r.Operator) - } - default: - return nil, errors.ErrInvalidf("invalid constraint type: %q", c.Type.String()) - } - // unset value if operator does not require it - if _, ok := noValueOperators[c.Operator]; ok { + if _, ok := flipt.NoValueOperators[c.Operator]; ok { c.Value = "" } @@ -303,26 +285,9 @@ func (s *SegmentStorage) UpdateConstraint(ctx context.Context, r *flipt.UpdateCo s.logger.WithField("request", r).Debug("update constraint") operator := strings.ToLower(r.Operator) - // validate operator works for this constraint type - switch r.Type { - case flipt.ComparisonType_STRING_COMPARISON_TYPE: - if _, ok := stringOperators[operator]; !ok { - return nil, errors.ErrInvalidf("constraint operator %q is not valid for type string", r.Operator) - } - case flipt.ComparisonType_NUMBER_COMPARISON_TYPE: - if _, ok := numberOperators[operator]; !ok { - return nil, errors.ErrInvalidf("constraint operator %q is not valid for type number", r.Operator) - } - case flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE: - if _, ok := booleanOperators[operator]; !ok { - return nil, errors.ErrInvalidf("constraint operator %q is not valid for type boolean", r.Operator) - } - default: - return nil, errors.ErrInvalidf("invalid constraint type: %q", r.Type.String()) - } // unset value if operator does not require it - if _, ok := noValueOperators[operator]; ok { + if _, ok := flipt.NoValueOperators[operator]; ok { r.Value = "" } diff --git a/storage/segment_test.go b/storage/segment_test.go index acbf16e972..be10a1eb1f 100644 --- a/storage/segment_test.go +++ b/storage/segment_test.go @@ -4,7 +4,6 @@ import ( "context" "testing" - "github.com/markphelps/flipt/errors" flipt "github.com/markphelps/flipt/rpc" "github.com/gofrs/uuid" @@ -272,7 +271,7 @@ func TestCreateConstraint(t *testing.T) { assert.Equal(t, segment.Key, constraint.SegmentKey) assert.Equal(t, flipt.ComparisonType_STRING_COMPARISON_TYPE, constraint.Type) assert.Equal(t, "foo", constraint.Property) - assert.Equal(t, opEQ, constraint.Operator) + assert.Equal(t, flipt.OpEQ, constraint.Operator) assert.Equal(t, "bar", constraint.Value) assert.NotZero(t, constraint.CreatedAt) assert.Equal(t, constraint.CreatedAt.Seconds, constraint.UpdatedAt.Seconds) @@ -286,72 +285,6 @@ func TestCreateConstraint(t *testing.T) { assert.Len(t, segment.Constraints, 1) } -func TestCreateConstraint_Invalid(t *testing.T) { - tests := []struct { - name string - req *flipt.CreateConstraintRequest - wantErr errors.ErrInvalid - }{ - { - name: "invalid for type string", - req: &flipt.CreateConstraintRequest{ - SegmentKey: "foo", - Type: flipt.ComparisonType_STRING_COMPARISON_TYPE, - Property: "foo", - Operator: "LT", - Value: "baz", - }, - wantErr: errors.ErrInvalid("constraint operator \"LT\" is not valid for type string"), - }, - { - name: "invalid for type number", - req: &flipt.CreateConstraintRequest{ - SegmentKey: "foo", - Type: flipt.ComparisonType_NUMBER_COMPARISON_TYPE, - Property: "1", - Operator: "Empty", - Value: "2", - }, - wantErr: errors.ErrInvalid("constraint operator \"Empty\" is not valid for type number"), - }, - { - name: "invalid for type boolean", - req: &flipt.CreateConstraintRequest{ - SegmentKey: "foo", - Type: flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, - Property: "true", - Operator: "GT", - Value: "false", - }, - wantErr: errors.ErrInvalid("constraint operator \"GT\" is not valid for type boolean"), - }, - { - name: "invalid for type unknown", - req: &flipt.CreateConstraintRequest{ - SegmentKey: "foo", - Type: flipt.ComparisonType_UNKNOWN_COMPARISON_TYPE, - Property: "-", - Operator: "EQ", - Value: "+", - }, - wantErr: errors.ErrInvalid("invalid constraint type: \"UNKNOWN_COMPARISON_TYPE\""), - }, - } - - for _, tt := range tests { - var ( - req = tt.req - wantErr = tt.wantErr - ) - - t.Run(tt.name, func(t *testing.T) { - constraint, err := segmentStore.CreateConstraint(context.TODO(), req) - assert.Equal(t, wantErr, err) - assert.Nil(t, constraint) - }) - } -} - func TestCreateConstraint_SegmentNotFound(t *testing.T) { _, err := segmentStore.CreateConstraint(context.TODO(), &flipt.CreateConstraintRequest{ SegmentKey: "foo", @@ -389,7 +322,7 @@ func TestUpdateConstraint(t *testing.T) { assert.Equal(t, segment.Key, constraint.SegmentKey) assert.Equal(t, flipt.ComparisonType_STRING_COMPARISON_TYPE, constraint.Type) assert.Equal(t, "foo", constraint.Property) - assert.Equal(t, opEQ, constraint.Operator) + assert.Equal(t, flipt.OpEQ, constraint.Operator) assert.Equal(t, "bar", constraint.Value) assert.NotZero(t, constraint.CreatedAt) assert.Equal(t, constraint.CreatedAt.Seconds, constraint.UpdatedAt.Seconds) @@ -409,7 +342,7 @@ func TestUpdateConstraint(t *testing.T) { assert.Equal(t, constraint.SegmentKey, updated.SegmentKey) assert.Equal(t, constraint.Type, updated.Type) assert.Equal(t, constraint.Property, updated.Property) - assert.Equal(t, opEmpty, updated.Operator) + assert.Equal(t, flipt.OpEmpty, updated.Operator) assert.Empty(t, updated.Value) assert.NotZero(t, updated.CreatedAt) assert.NotEqual(t, updated.CreatedAt, updated.UpdatedAt) @@ -423,76 +356,6 @@ func TestUpdateConstraint(t *testing.T) { assert.Len(t, segment.Constraints, 1) } -func TestUpdateConstraint_Invalid(t *testing.T) { - tests := []struct { - name string - req *flipt.UpdateConstraintRequest - wantErr errors.ErrInvalid - }{ - { - name: "invalid for type string", - req: &flipt.UpdateConstraintRequest{ - Id: "1", - SegmentKey: "foo", - Type: flipt.ComparisonType_STRING_COMPARISON_TYPE, - Property: "foo", - Operator: "LT", - Value: "baz", - }, - wantErr: errors.ErrInvalid("constraint operator \"LT\" is not valid for type string"), - }, - { - name: "invalid for type number", - req: &flipt.UpdateConstraintRequest{ - Id: "1", - SegmentKey: "foo", - Type: flipt.ComparisonType_NUMBER_COMPARISON_TYPE, - Property: "1", - Operator: "Empty", - Value: "2", - }, - wantErr: errors.ErrInvalid("constraint operator \"Empty\" is not valid for type number"), - }, - { - name: "invalid for type boolean", - req: &flipt.UpdateConstraintRequest{ - Id: "1", - SegmentKey: "foo", - Type: flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, - Property: "true", - Operator: "GT", - Value: "false", - }, - wantErr: errors.ErrInvalid("constraint operator \"GT\" is not valid for type boolean"), - }, - { - name: "invalid for type unknown", - req: &flipt.UpdateConstraintRequest{ - Id: "1", - SegmentKey: "foo", - Type: flipt.ComparisonType_UNKNOWN_COMPARISON_TYPE, - Property: "-", - Operator: "EQ", - Value: "+", - }, - wantErr: errors.ErrInvalid("invalid constraint type: \"UNKNOWN_COMPARISON_TYPE\""), - }, - } - - for _, tt := range tests { - var ( - req = tt.req - wantErr = tt.wantErr - ) - - t.Run(tt.name, func(t *testing.T) { - constraint, err := segmentStore.UpdateConstraint(context.TODO(), req) - assert.Equal(t, wantErr, err) - assert.Nil(t, constraint) - }) - } -} - func TestUpdateConstraint_NotFound(t *testing.T) { segment, err := segmentStore.CreateSegment(context.TODO(), &flipt.CreateSegmentRequest{ Key: t.Name(), From 4aa990a8e26f2b9ee9b164d9bc1d7787790f7c7f Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Thu, 28 Nov 2019 12:28:33 -0500 Subject: [PATCH 07/12] Flags --- rpc/validation.go | 97 ++++++++++++- rpc/validation_test.go | 308 +++++++++++++++++++++++++++++++++++++++++ server/flag.go | 54 -------- server/flag_test.go | 258 +--------------------------------- server/rule_test.go | 8 +- server/segment_test.go | 6 +- 6 files changed, 410 insertions(+), 321 deletions(-) diff --git a/rpc/validation.go b/rpc/validation.go index 76229e9c14..ba15f151d2 100644 --- a/rpc/validation.go +++ b/rpc/validation.go @@ -1,8 +1,9 @@ package flipt import ( - "github.com/markphelps/flipt/errors" "strings" + + "github.com/markphelps/flipt/errors" ) // Validator validates types @@ -10,6 +11,88 @@ type Validator interface { Validate() error } +// Flags + +func (req *GetFlagRequest) Validate() error { + if req.Key == "" { + return errors.EmptyFieldError("key") + } + + return nil +} + +func (req *CreateFlagRequest) Validate() error { + if req.Key == "" { + return errors.EmptyFieldError("key") + } + + if req.Name == "" { + return errors.EmptyFieldError("name") + } + + return nil +} + +func (req *UpdateFlagRequest) Validate() error { + if req.Key == "" { + return errors.EmptyFieldError("key") + } + + if req.Name == "" { + return errors.EmptyFieldError("name") + } + + return nil +} + +func (req *DeleteFlagRequest) Validate() error { + if req.Key == "" { + return errors.EmptyFieldError("key") + } + + return nil +} + +func (req *CreateVariantRequest) Validate() error { + if req.FlagKey == "" { + return errors.EmptyFieldError("flagKey") + } + + if req.Key == "" { + return errors.EmptyFieldError("key") + } + + return nil +} + +func (req *UpdateVariantRequest) Validate() error { + if req.Id == "" { + return errors.EmptyFieldError("id") + } + + if req.FlagKey == "" { + return errors.EmptyFieldError("flagKey") + } + + if req.Key == "" { + return errors.EmptyFieldError("key") + } + + return nil +} + +func (req *DeleteVariantRequest) Validate() error { + if req.Id == "" { + return errors.EmptyFieldError("id") + } + + if req.FlagKey == "" { + return errors.EmptyFieldError("flagKey") + } + + return nil +} + // Rules func (req *ListRuleRequest) Validate() error { @@ -234,7 +317,11 @@ func (req *CreateConstraintRequest) Validate() error { return errors.ErrInvalidf("invalid constraint type: %q", req.Type.String()) } - // TODO: test for empty value if operator ! [EMPTY, NOT_EMPTY, PRESENT, NOT_PRESENT] + // check if value is required + if _, ok := NoValueOperators[operator]; !ok { + return errors.EmptyFieldError("value") + } + return nil } @@ -274,7 +361,11 @@ func (req *UpdateConstraintRequest) Validate() error { return errors.ErrInvalidf("invalid constraint type: %q", req.Type.String()) } - // TODO: test for empty value if operator ! [EMPTY, NOT_EMPTY, PRESENT, NOT_PRESENT] + // check if value is required + if _, ok := NoValueOperators[operator]; !ok { + return errors.EmptyFieldError("value") + } + return nil } diff --git a/rpc/validation_test.go b/rpc/validation_test.go index 95402ddf0d..2fbd4e2ff1 100644 --- a/rpc/validation_test.go +++ b/rpc/validation_test.go @@ -7,6 +7,274 @@ import ( "github.com/stretchr/testify/assert" ) +func TestValidate_GetFlagRequest(t *testing.T) { + tests := []struct { + name string + req *GetFlagRequest + wantErr error + }{ + { + name: "emptyKey", + req: &GetFlagRequest{Key: ""}, + wantErr: errors.EmptyFieldError("key"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_CreateFlagRequest(t *testing.T) { + tests := []struct { + name string + req *CreateFlagRequest + wantErr error + }{ + { + name: "emptyKey", + req: &CreateFlagRequest{ + Key: "", + Name: "name", + Description: "desc", + Enabled: true, + }, + wantErr: errors.EmptyFieldError("key"), + }, + { + name: "emptyName", + req: &CreateFlagRequest{ + Key: "key", + Name: "", + Description: "desc", + Enabled: true, + }, + wantErr: errors.EmptyFieldError("name"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_UpdateFlagRequest(t *testing.T) { + tests := []struct { + name string + req *UpdateFlagRequest + wantErr error + }{ + { + name: "emptyKey", + req: &UpdateFlagRequest{ + Key: "", + Name: "name", + Description: "desc", + Enabled: true, + }, + wantErr: errors.EmptyFieldError("key"), + }, + { + name: "emptyName", + req: &UpdateFlagRequest{ + Key: "key", + Name: "", + Description: "desc", + Enabled: true, + }, + wantErr: errors.EmptyFieldError("name"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_DeleteFlagRequest(t *testing.T) { + tests := []struct { + name string + req *DeleteFlagRequest + wantErr error + }{ + { + name: "emptyKey", + req: &DeleteFlagRequest{ + Key: "", + }, + wantErr: errors.EmptyFieldError("key"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_CreateVariantRequest(t *testing.T) { + tests := []struct { + name string + req *CreateVariantRequest + wantErr error + }{ + { + name: "emptyFlagKey", + req: &CreateVariantRequest{ + FlagKey: "", + Key: "key", + Name: "name", + Description: "desc", + }, + wantErr: errors.EmptyFieldError("flagKey"), + }, + { + name: "emptyKey", + req: &CreateVariantRequest{ + FlagKey: "flagKey", + Key: "", + Name: "name", + Description: "desc", + }, + wantErr: errors.EmptyFieldError("key"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_UpdateVariantRequest(t *testing.T) { + tests := []struct { + name string + req *UpdateVariantRequest + wantErr error + }{ + { + name: "emptyId", + req: &UpdateVariantRequest{ + Id: "", + FlagKey: "flagKey", + Key: "key", + Name: "name", + Description: "desc", + }, + wantErr: errors.EmptyFieldError("id"), + }, + { + name: "emptyFlagKey", + req: &UpdateVariantRequest{ + Id: "id", + FlagKey: "", + Key: "key", + Name: "name", + Description: "desc", + }, + wantErr: errors.EmptyFieldError("flagKey"), + }, + { + name: "emptyKey", + req: &UpdateVariantRequest{ + Id: "id", + FlagKey: "flagKey", + Key: "", + Name: "name", + Description: "desc", + }, + wantErr: errors.EmptyFieldError("key"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + +func TestValidate_DeleteVariantRequest(t *testing.T) { + tests := []struct { + name string + req *DeleteVariantRequest + wantErr error + }{ + { + name: "emptyId", + req: &DeleteVariantRequest{ + Id: "", + FlagKey: "flagKey", + }, + wantErr: errors.EmptyFieldError("id"), + }, + { + name: "emptyFlagKey", + req: &DeleteVariantRequest{ + Id: "id", + FlagKey: "", + }, + wantErr: errors.EmptyFieldError("flagKey"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + func TestValidate_ListRuleRequest(t *testing.T) { tests := []struct { name string @@ -579,6 +847,25 @@ func TestValidate_CreateConstraintRequest(t *testing.T) { }, wantErr: errors.ErrInvalid("invalid constraint type: \"UNKNOWN_COMPARISON_TYPE\""), }, + { + name: "emptyValue ok", + req: &CreateConstraintRequest{ + SegmentKey: "segmentKey", + Type: ComparisonType_STRING_COMPARISON_TYPE, + Property: "foo", + Operator: "notempty", + }, + }, + { + name: "emptyValue not allowed", + req: &CreateConstraintRequest{ + SegmentKey: "segmentKey", + Type: ComparisonType_STRING_COMPARISON_TYPE, + Property: "foo", + Operator: "eq", + }, + wantErr: errors.EmptyFieldError("value"), + }, } for _, tt := range tests { @@ -696,6 +983,27 @@ func TestValidate_UpdateConstraintRequest(t *testing.T) { }, wantErr: errors.ErrInvalid("invalid constraint type: \"UNKNOWN_COMPARISON_TYPE\""), }, + { + name: "emptyValue ok", + req: &UpdateConstraintRequest{ + Id: "1", + SegmentKey: "segmentKey", + Type: ComparisonType_STRING_COMPARISON_TYPE, + Property: "foo", + Operator: "notempty", + }, + }, + { + name: "emptyValue not allowed", + req: &UpdateConstraintRequest{ + Id: "1", + SegmentKey: "segmentKey", + Type: ComparisonType_STRING_COMPARISON_TYPE, + Property: "foo", + Operator: "eq", + }, + wantErr: errors.EmptyFieldError("value"), + }, } for _, tt := range tests { diff --git a/server/flag.go b/server/flag.go index 46dd5ec17d..59e54fb5f2 100644 --- a/server/flag.go +++ b/server/flag.go @@ -3,18 +3,12 @@ package server import ( "context" - "github.com/markphelps/flipt/errors" - "github.com/golang/protobuf/ptypes/empty" flipt "github.com/markphelps/flipt/rpc" ) // GetFlag gets a flag func (s *Server) GetFlag(ctx context.Context, req *flipt.GetFlagRequest) (*flipt.Flag, error) { - if req.Key == "" { - return nil, errors.EmptyFieldError("key") - } - return s.FlagStore.GetFlag(ctx, req) } @@ -36,36 +30,16 @@ func (s *Server) ListFlags(ctx context.Context, req *flipt.ListFlagRequest) (*fl // CreateFlag creates a flag func (s *Server) CreateFlag(ctx context.Context, req *flipt.CreateFlagRequest) (*flipt.Flag, error) { - if req.Key == "" { - return nil, errors.EmptyFieldError("key") - } - - if req.Name == "" { - return nil, errors.EmptyFieldError("name") - } - return s.FlagStore.CreateFlag(ctx, req) } // UpdateFlag updates an existing flag func (s *Server) UpdateFlag(ctx context.Context, req *flipt.UpdateFlagRequest) (*flipt.Flag, error) { - if req.Key == "" { - return nil, errors.EmptyFieldError("key") - } - - if req.Name == "" { - return nil, errors.EmptyFieldError("name") - } - return s.FlagStore.UpdateFlag(ctx, req) } // DeleteFlag deletes a flag func (s *Server) DeleteFlag(ctx context.Context, req *flipt.DeleteFlagRequest) (*empty.Empty, error) { - if req.Key == "" { - return nil, errors.EmptyFieldError("key") - } - if err := s.FlagStore.DeleteFlag(ctx, req); err != nil { return nil, err } @@ -75,44 +49,16 @@ func (s *Server) DeleteFlag(ctx context.Context, req *flipt.DeleteFlagRequest) ( // CreateVariant creates a variant func (s *Server) CreateVariant(ctx context.Context, req *flipt.CreateVariantRequest) (*flipt.Variant, error) { - if req.FlagKey == "" { - return nil, errors.EmptyFieldError("flagKey") - } - - if req.Key == "" { - return nil, errors.EmptyFieldError("key") - } - return s.FlagStore.CreateVariant(ctx, req) } // UpdateVariant updates an existing variant func (s *Server) UpdateVariant(ctx context.Context, req *flipt.UpdateVariantRequest) (*flipt.Variant, error) { - if req.Id == "" { - return nil, errors.EmptyFieldError("id") - } - - if req.FlagKey == "" { - return nil, errors.EmptyFieldError("flagKey") - } - - if req.Key == "" { - return nil, errors.EmptyFieldError("key") - } - return s.FlagStore.UpdateVariant(ctx, req) } // DeleteVariant deletes a variant func (s *Server) DeleteVariant(ctx context.Context, req *flipt.DeleteVariantRequest) (*empty.Empty, error) { - if req.Id == "" { - return nil, errors.EmptyFieldError("id") - } - - if req.FlagKey == "" { - return nil, errors.EmptyFieldError("flagKey") - } - if err := s.FlagStore.DeleteVariant(ctx, req); err != nil { return nil, err } diff --git a/server/flag_test.go b/server/flag_test.go index 62efa0ab4b..fefa53230e 100644 --- a/server/flag_test.go +++ b/server/flag_test.go @@ -80,20 +80,6 @@ func TestGetFlag(t *testing.T) { Key: "key", }, }, - { - name: "emptyKey", - req: &flipt.GetFlagRequest{Key: ""}, - f: func(_ context.Context, r *flipt.GetFlagRequest) (*flipt.Flag, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.Key) - - return &flipt.Flag{ - Key: "", - }, nil - }, - flag: nil, - wantErr: errors.EmptyFieldError("key"), - }, } for _, tt := range tests { @@ -143,7 +129,7 @@ func TestListFlags(t *testing.T) { }, }, { - name: "err", + name: "error", req: &flipt.ListFlagRequest{}, f: func(context.Context, *flipt.ListFlagRequest) ([]*flipt.Flag, error) { return nil, errors.New("test error") @@ -212,54 +198,6 @@ func TestCreateFlag(t *testing.T) { Enabled: true, }, }, - { - name: "emptyKey", - req: &flipt.CreateFlagRequest{ - Key: "", - Name: "name", - Description: "desc", - Enabled: true, - }, - f: func(_ context.Context, r *flipt.CreateFlagRequest) (*flipt.Flag, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.Key) - assert.Equal(t, "name", r.Name) - assert.Equal(t, "desc", r.Description) - assert.True(t, r.Enabled) - - return &flipt.Flag{ - Key: "", - Name: r.Name, - Description: r.Description, - Enabled: r.Enabled, - }, nil - }, - wantErr: errors.EmptyFieldError("key"), - }, - { - name: "emptyName", - req: &flipt.CreateFlagRequest{ - Key: "key", - Name: "", - Description: "desc", - Enabled: true, - }, - f: func(_ context.Context, r *flipt.CreateFlagRequest) (*flipt.Flag, error) { - assert.NotNil(t, r) - assert.Equal(t, "key", r.Key) - assert.Equal(t, "", r.Name) - assert.Equal(t, "desc", r.Description) - assert.True(t, r.Enabled) - - return &flipt.Flag{ - Key: r.Key, - Name: "", - Description: r.Description, - Enabled: r.Enabled, - }, nil - }, - wantErr: errors.EmptyFieldError("name"), - }, } for _, tt := range tests { @@ -321,54 +259,6 @@ func TestUpdateFlag(t *testing.T) { Enabled: true, }, }, - { - name: "emptyKey", - req: &flipt.UpdateFlagRequest{ - Key: "", - Name: "name", - Description: "desc", - Enabled: true, - }, - f: func(_ context.Context, r *flipt.UpdateFlagRequest) (*flipt.Flag, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.Key) - assert.Equal(t, "name", r.Name) - assert.Equal(t, "desc", r.Description) - assert.True(t, r.Enabled) - - return &flipt.Flag{ - Key: "", - Name: r.Name, - Description: r.Description, - Enabled: r.Enabled, - }, nil - }, - wantErr: errors.EmptyFieldError("key"), - }, - { - name: "emptyName", - req: &flipt.UpdateFlagRequest{ - Key: "key", - Name: "", - Description: "desc", - Enabled: true, - }, - f: func(_ context.Context, r *flipt.UpdateFlagRequest) (*flipt.Flag, error) { - assert.NotNil(t, r) - assert.Equal(t, "key", r.Key) - assert.Equal(t, "", r.Name) - assert.Equal(t, "desc", r.Description) - assert.True(t, r.Enabled) - - return &flipt.Flag{ - Key: r.Key, - Name: "", - Description: r.Description, - Enabled: r.Enabled, - }, nil - }, - wantErr: errors.EmptyFieldError("name"), - }, } for _, tt := range tests { @@ -412,17 +302,6 @@ func TestDeleteFlag(t *testing.T) { }, empty: &empty.Empty{}, }, - { - name: "emptyKey", - req: &flipt.DeleteFlagRequest{Key: ""}, - f: func(_ context.Context, r *flipt.DeleteFlagRequest) error { - assert.NotNil(t, r) - assert.Equal(t, "", r.Key) - - return nil - }, - wantErr: errors.EmptyFieldError("key"), - }, { name: "error", req: &flipt.DeleteFlagRequest{Key: "key"}, @@ -495,54 +374,6 @@ func TestCreateVariant(t *testing.T) { Description: "desc", }, }, - { - name: "emptyFlagKey", - req: &flipt.CreateVariantRequest{ - FlagKey: "", - Key: "key", - Name: "name", - Description: "desc", - }, - f: func(_ context.Context, r *flipt.CreateVariantRequest) (*flipt.Variant, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.FlagKey) - assert.Equal(t, "key", r.Key) - assert.Equal(t, "name", r.Name) - assert.Equal(t, "desc", r.Description) - - return &flipt.Variant{ - FlagKey: "", - Key: r.Key, - Name: r.Name, - Description: r.Description, - }, nil - }, - wantErr: errors.EmptyFieldError("flagKey"), - }, - { - name: "emptyKey", - req: &flipt.CreateVariantRequest{ - FlagKey: "flagKey", - Key: "", - Name: "name", - Description: "desc", - }, - f: func(_ context.Context, r *flipt.CreateVariantRequest) (*flipt.Variant, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "", r.Key) - assert.Equal(t, "name", r.Name) - assert.Equal(t, "desc", r.Description) - - return &flipt.Variant{ - FlagKey: r.FlagKey, - Key: "", - Name: r.Name, - Description: r.Description, - }, nil - }, - wantErr: errors.EmptyFieldError("key"), - }, } for _, tt := range tests { @@ -602,69 +433,6 @@ func TestUpdateVariant(t *testing.T) { Description: "desc", }, }, - { - name: "emptyID", - req: &flipt.UpdateVariantRequest{Id: "", FlagKey: "flagKey", Key: "key", Name: "name", Description: "desc"}, - f: func(_ context.Context, r *flipt.UpdateVariantRequest) (*flipt.Variant, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "", r.Id) - assert.Equal(t, "key", r.Key) - assert.Equal(t, "name", r.Name) - assert.Equal(t, "desc", r.Description) - - return &flipt.Variant{ - Id: "", - FlagKey: r.FlagKey, - Key: r.Key, - Name: r.Name, - Description: r.Description, - }, nil - }, - wantErr: errors.EmptyFieldError("id"), - }, - { - name: "emptyFlagKey", - req: &flipt.UpdateVariantRequest{Id: "id", FlagKey: "", Key: "key", Name: "name", Description: "desc"}, - f: func(_ context.Context, r *flipt.UpdateVariantRequest) (*flipt.Variant, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.FlagKey) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "key", r.Key) - assert.Equal(t, "name", r.Name) - assert.Equal(t, "desc", r.Description) - - return &flipt.Variant{ - Id: r.Id, - FlagKey: "", - Key: r.Key, - Name: r.Name, - Description: r.Description, - }, nil - }, - wantErr: errors.EmptyFieldError("flagKey"), - }, - { - name: "emptyKey", - req: &flipt.UpdateVariantRequest{Id: "id", FlagKey: "flagKey", Key: "", Name: "name", Description: "desc"}, - f: func(_ context.Context, r *flipt.UpdateVariantRequest) (*flipt.Variant, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "", r.Key) - assert.Equal(t, "name", r.Name) - assert.Equal(t, "desc", r.Description) - - return &flipt.Variant{ - Id: r.Id, - FlagKey: r.FlagKey, - Key: "", - Name: r.Name, - Description: r.Description, - }, nil - }, - wantErr: errors.EmptyFieldError("key"), - }, } for _, tt := range tests { @@ -709,30 +477,6 @@ func TestDeleteVariant(t *testing.T) { }, empty: &empty.Empty{}, }, - { - name: "emptyID", - req: &flipt.DeleteVariantRequest{Id: "", FlagKey: "flagKey"}, - f: func(_ context.Context, r *flipt.DeleteVariantRequest) error { - assert.NotNil(t, r) - assert.Equal(t, "", r.Id) - assert.Equal(t, "flagKey", r.FlagKey) - - return nil - }, - wantErr: errors.EmptyFieldError("id"), - }, - { - name: "emptyFlagKey", - req: &flipt.DeleteVariantRequest{Id: "id", FlagKey: ""}, - f: func(_ context.Context, r *flipt.DeleteVariantRequest) error { - assert.NotNil(t, r) - assert.Equal(t, "id", r.Id) - assert.Equal(t, "", r.FlagKey) - - return nil - }, - wantErr: errors.EmptyFieldError("flagKey"), - }, { name: "error", req: &flipt.DeleteVariantRequest{Id: "id", FlagKey: "flagKey"}, diff --git a/server/rule_test.go b/server/rule_test.go index 19e7eedf8e..fbbd263d2c 100644 --- a/server/rule_test.go +++ b/server/rule_test.go @@ -133,7 +133,7 @@ func TestListRules(t *testing.T) { }, }, { - name: "error test", + name: "error", req: &flipt.ListRuleRequest{FlagKey: "flagKey"}, f: func(ctx context.Context, r *flipt.ListRuleRequest) ([]*flipt.Rule, error) { assert.NotNil(t, r) @@ -301,7 +301,7 @@ func TestDeleteRule(t *testing.T) { empty: &empty.Empty{}, }, { - name: "error test", + name: "error", req: &flipt.DeleteRuleRequest{Id: "id", FlagKey: "flagKey"}, f: func(_ context.Context, r *flipt.DeleteRuleRequest) error { assert.NotNil(t, r) @@ -355,7 +355,7 @@ func TestOrderRules(t *testing.T) { empty: &empty.Empty{}, }, { - name: "error test", + name: "error", req: &flipt.OrderRulesRequest{FlagKey: "flagKey", RuleIds: []string{"1", "2"}}, f: func(_ context.Context, r *flipt.OrderRulesRequest) error { assert.NotNil(t, r) @@ -517,7 +517,7 @@ func TestDeleteDistribution(t *testing.T) { empty: &empty.Empty{}, }, { - name: "error test", + name: "error", req: &flipt.DeleteDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID"}, f: func(_ context.Context, r *flipt.DeleteDistributionRequest) error { assert.NotNil(t, r) diff --git a/server/segment_test.go b/server/segment_test.go index 9a135f0069..1f19437a7a 100644 --- a/server/segment_test.go +++ b/server/segment_test.go @@ -129,7 +129,7 @@ func TestListSegments(t *testing.T) { }, }, { - name: "error test", + name: "error", req: &flipt.ListSegmentRequest{}, f: func(context.Context, *flipt.ListSegmentRequest) ([]*flipt.Segment, error) { return nil, errors.New("error test") @@ -293,7 +293,7 @@ func TestDeleteSegment(t *testing.T) { empty: &empty.Empty{}, }, { - name: "error test", + name: "error", req: &flipt.DeleteSegmentRequest{Key: "key"}, f: func(_ context.Context, r *flipt.DeleteSegmentRequest) error { assert.NotNil(t, r) @@ -480,7 +480,7 @@ func TestDeleteConstraint(t *testing.T) { empty: &empty.Empty{}, }, { - name: "error test", + name: "error", req: &flipt.DeleteConstraintRequest{Id: "id", SegmentKey: "segmentKey"}, f: func(_ context.Context, r *flipt.DeleteConstraintRequest) error { assert.NotNil(t, r) From de54922f83d4b1a7d53bfe0b66a95135952639e7 Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Thu, 28 Nov 2019 12:35:02 -0500 Subject: [PATCH 08/12] Linter --- config/local.yml | 7 ------- script/test/api | 20 -------------------- storage/evaluator.go | 1 - 3 files changed, 28 deletions(-) diff --git a/config/local.yml b/config/local.yml index 7138eca26c..944b8aed24 100644 --- a/config/local.yml +++ b/config/local.yml @@ -5,10 +5,3 @@ db: url: file:flipt.db migrations: path: ./config/migrations - -ui: - enabled: false - -cors: - enabled: true - allowed_origins: "*" diff --git a/script/test/api b/script/test/api index 75527dda0a..dcd24998a3 100755 --- a/script/test/api +++ b/script/test/api @@ -83,16 +83,6 @@ step_3_test_segments_and_constraints() segment_key=$(uuid_str) segment_name_1=$(uuid_str) - # create segment - missing key - shakedown POST "$flipt_api/segments" -H 'Content-Type:application/json' -d "{\"name\":\"$segment_name_1\",\"description\":\"description\"}" - status 400 - matches "\"error\":\"invalid field key: must not be empty\"" - - # create segment - missing name - shakedown POST "$flipt_api/segments" -H 'Content-Type:application/json' -d "{\"key\":\"$segment_key\",\"description\":\"description\"}" - status 400 - matches "\"error\":\"invalid field name: must not be empty\"" - # create segment shakedown POST "$flipt_api/segments" -H 'Content-Type:application/json' -d "{\"key\":\"$segment_key\",\"name\":\"$segment_name_1\",\"description\":\"description\"}" status 200 @@ -107,16 +97,6 @@ step_3_test_segments_and_constraints() segment_name_2=$(uuid_str) - # update segment - missing key - shakedown PUT "$flipt_api/segments/" -H 'Content-Type:application/json' -d "{\"name\":\"$segment_name_2\",\"description\":\"description\"}" - status 400 - matches "\"error\":\"invalid field key: must not be empty\"" - - # update segment - missing name - shakedown PUT "$flipt_api/segments/$segment_key" -H 'Content-Type:application/json' -d "{\"key\":\"$segment_key\",\"description\":\"description\"}" - status 400 - matches "\"error\":\"invalid field name: must not be empty\"" - # update segment shakedown PUT "$flipt_api/segments/$segment_key" -H 'Content-Type:application/json' -d "{\"key\":\"$segment_key\",\"name\":\"$segment_name_2\",\"description\":\"description\"}" status 200 diff --git a/storage/evaluator.go b/storage/evaluator.go index 4bfb91ccdb..6ae2216f1f 100644 --- a/storage/evaluator.go +++ b/storage/evaluator.go @@ -186,7 +186,6 @@ func (s *EvaluatorStorage) Evaluate(ctx context.Context, r *flipt.EvaluationRequ constraintMatches := 0 for _, c := range rule.Constraints { - v := r.Context[c.Property] var ( From 28d5dde2d72b4318c37ff68f7f5aacc51d110983 Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Thu, 28 Nov 2019 13:23:59 -0500 Subject: [PATCH 09/12] Evaluator --- rpc/validation.go | 14 ++++++++++++++ rpc/validation_test.go | 31 +++++++++++++++++++++++++++++++ server/evaluator.go | 9 --------- server/evaluator_test.go | 32 +------------------------------- 4 files changed, 46 insertions(+), 40 deletions(-) diff --git a/rpc/validation.go b/rpc/validation.go index ba15f151d2..c518bddbcb 100644 --- a/rpc/validation.go +++ b/rpc/validation.go @@ -11,6 +11,20 @@ type Validator interface { Validate() error } +// Evaluate + +func (req *EvaluationRequest) Validate() error { + if req.FlagKey == "" { + return errors.EmptyFieldError("flagKey") + } + + if req.EntityId == "" { + return errors.EmptyFieldError("entityId") + } + + return nil +} + // Flags func (req *GetFlagRequest) Validate() error { diff --git a/rpc/validation_test.go b/rpc/validation_test.go index 2fbd4e2ff1..062ab9f884 100644 --- a/rpc/validation_test.go +++ b/rpc/validation_test.go @@ -7,6 +7,37 @@ import ( "github.com/stretchr/testify/assert" ) +func TestValidate_EvaluationRequest(t *testing.T) { + tests := []struct { + name string + req *EvaluationRequest + wantErr error + }{ + { + name: "emptyFlagKey", + req: &EvaluationRequest{FlagKey: "", EntityId: "entityID"}, + wantErr: errors.EmptyFieldError("flagKey"), + }, + { + name: "emptyEntityId", + req: &EvaluationRequest{FlagKey: "flagKey", EntityId: ""}, + wantErr: errors.EmptyFieldError("entityId"), + }, + } + + for _, tt := range tests { + var ( + req = tt.req + wantErr = tt.wantErr + ) + + t.Run(tt.name, func(t *testing.T) { + err := req.Validate() + assert.Equal(t, wantErr, err) + }) + } +} + func TestValidate_GetFlagRequest(t *testing.T) { tests := []struct { name string diff --git a/server/evaluator.go b/server/evaluator.go index 7894cede44..2eba4601fe 100644 --- a/server/evaluator.go +++ b/server/evaluator.go @@ -5,20 +5,11 @@ import ( "time" "github.com/gofrs/uuid" - "github.com/markphelps/flipt/errors" flipt "github.com/markphelps/flipt/rpc" ) // Evaluate evaluates a request for a given flag and entity func (s *Server) Evaluate(ctx context.Context, req *flipt.EvaluationRequest) (*flipt.EvaluationResponse, error) { - if req.FlagKey == "" { - return nil, errors.EmptyFieldError("flagKey") - } - - if req.EntityId == "" { - return nil, errors.EmptyFieldError("entityId") - } - startTime := time.Now() // set request ID if not present diff --git a/server/evaluator_test.go b/server/evaluator_test.go index 55bdc293b2..6a53d91d49 100644 --- a/server/evaluator_test.go +++ b/server/evaluator_test.go @@ -48,37 +48,7 @@ func TestEvaluate(t *testing.T) { }, }, { - name: "emptyFlagKey", - req: &flipt.EvaluationRequest{FlagKey: "", EntityId: "entityID"}, - f: func(_ context.Context, r *flipt.EvaluationRequest) (*flipt.EvaluationResponse, error) { - assert.NotNil(t, r) - assert.Equal(t, "", r.FlagKey) - assert.Equal(t, "entityID", r.EntityId) - - return &flipt.EvaluationResponse{ - FlagKey: "", - EntityId: r.EntityId, - }, nil - }, - wantErr: errors.EmptyFieldError("flagKey"), - }, - { - name: "emptyEntityId", - req: &flipt.EvaluationRequest{FlagKey: "flagKey", EntityId: ""}, - f: func(_ context.Context, r *flipt.EvaluationRequest) (*flipt.EvaluationResponse, error) { - assert.NotNil(t, r) - assert.Equal(t, "flagKey", r.FlagKey) - assert.Equal(t, "", r.EntityId) - - return &flipt.EvaluationResponse{ - FlagKey: r.FlagKey, - EntityId: "", - }, nil - }, - wantErr: errors.EmptyFieldError("entityId"), - }, - { - name: "error test", + name: "error", req: &flipt.EvaluationRequest{FlagKey: "flagKey", EntityId: "entityID"}, f: func(_ context.Context, r *flipt.EvaluationRequest) (*flipt.EvaluationResponse, error) { assert.NotNil(t, r) From 4a2eff498f362c9765b7955855844b81123eba24 Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Thu, 28 Nov 2019 13:37:28 -0500 Subject: [PATCH 10/12] Fix constraint value --- rpc/validation.go | 16 ++++++++++------ rpc/validation_test.go | 21 +++++++++++++++++++++ script/test/api | 4 ++-- 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/rpc/validation.go b/rpc/validation.go index c518bddbcb..cb746754df 100644 --- a/rpc/validation.go +++ b/rpc/validation.go @@ -331,9 +331,11 @@ func (req *CreateConstraintRequest) Validate() error { return errors.ErrInvalidf("invalid constraint type: %q", req.Type.String()) } - // check if value is required - if _, ok := NoValueOperators[operator]; !ok { - return errors.EmptyFieldError("value") + if req.Value == "" { + // check if value is required + if _, ok := NoValueOperators[operator]; !ok { + return errors.EmptyFieldError("value") + } } return nil @@ -375,9 +377,11 @@ func (req *UpdateConstraintRequest) Validate() error { return errors.ErrInvalidf("invalid constraint type: %q", req.Type.String()) } - // check if value is required - if _, ok := NoValueOperators[operator]; !ok { - return errors.EmptyFieldError("value") + if req.Value == "" { + // check if value is required + if _, ok := NoValueOperators[operator]; !ok { + return errors.EmptyFieldError("value") + } } return nil diff --git a/rpc/validation_test.go b/rpc/validation_test.go index 062ab9f884..e887027c95 100644 --- a/rpc/validation_test.go +++ b/rpc/validation_test.go @@ -878,6 +878,16 @@ func TestValidate_CreateConstraintRequest(t *testing.T) { }, wantErr: errors.ErrInvalid("invalid constraint type: \"UNKNOWN_COMPARISON_TYPE\""), }, + { + name: "ok", + req: &CreateConstraintRequest{ + SegmentKey: "segmentKey", + Type: ComparisonType_STRING_COMPARISON_TYPE, + Property: "foo", + Operator: "eq", + Value: "bar", + }, + }, { name: "emptyValue ok", req: &CreateConstraintRequest{ @@ -1014,6 +1024,17 @@ func TestValidate_UpdateConstraintRequest(t *testing.T) { }, wantErr: errors.ErrInvalid("invalid constraint type: \"UNKNOWN_COMPARISON_TYPE\""), }, + { + name: "ok", + req: &UpdateConstraintRequest{ + Id: "1", + SegmentKey: "segmentKey", + Type: ComparisonType_STRING_COMPARISON_TYPE, + Property: "foo", + Operator: "eq", + Value: "bar", + }, + }, { name: "emptyValue ok", req: &UpdateConstraintRequest{ diff --git a/script/test/api b/script/test/api index dcd24998a3..60991930c9 100755 --- a/script/test/api +++ b/script/test/api @@ -80,10 +80,10 @@ step_2_test_flags_and_variants() step_3_test_segments_and_constraints() { + # create segment segment_key=$(uuid_str) segment_name_1=$(uuid_str) - # create segment shakedown POST "$flipt_api/segments" -H 'Content-Type:application/json' -d "{\"key\":\"$segment_key\",\"name\":\"$segment_name_1\",\"description\":\"description\"}" status 200 matches "\"key\":\"$segment_key\"" @@ -95,9 +95,9 @@ step_3_test_segments_and_constraints() matches "\"key\":\"$segment_key\"" matches "\"name\":\"$segment_name_1\"" + # update segment segment_name_2=$(uuid_str) - # update segment shakedown PUT "$flipt_api/segments/$segment_key" -H 'Content-Type:application/json' -d "{\"key\":\"$segment_key\",\"name\":\"$segment_name_2\",\"description\":\"description\"}" status 200 matches "\"key\":\"$segment_key\"" From 0baed2f3f2c18f263b6ad740e04d0e656790ffee Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Thu, 28 Nov 2019 14:49:34 -0500 Subject: [PATCH 11/12] More tests --- rpc/validation_test.go | 141 +++++++++++++++++++++++++++++++++++++++-- server/server_test.go | 52 +++++++++++++++ 2 files changed, 189 insertions(+), 4 deletions(-) diff --git a/rpc/validation_test.go b/rpc/validation_test.go index e887027c95..3a3cc1f871 100644 --- a/rpc/validation_test.go +++ b/rpc/validation_test.go @@ -23,6 +23,10 @@ func TestValidate_EvaluationRequest(t *testing.T) { req: &EvaluationRequest{FlagKey: "flagKey", EntityId: ""}, wantErr: errors.EmptyFieldError("entityId"), }, + { + name: "valid", + req: &EvaluationRequest{FlagKey: "flagKey", EntityId: "entityId"}, + }, } for _, tt := range tests { @@ -49,6 +53,10 @@ func TestValidate_GetFlagRequest(t *testing.T) { req: &GetFlagRequest{Key: ""}, wantErr: errors.EmptyFieldError("key"), }, + { + name: "valid", + req: &GetFlagRequest{Key: "key"}, + }, } for _, tt := range tests { @@ -90,6 +98,15 @@ func TestValidate_CreateFlagRequest(t *testing.T) { }, wantErr: errors.EmptyFieldError("name"), }, + { + name: "valid", + req: &CreateFlagRequest{ + Key: "key", + Name: "name", + Description: "desc", + Enabled: true, + }, + }, } for _, tt := range tests { @@ -131,6 +148,15 @@ func TestValidate_UpdateFlagRequest(t *testing.T) { }, wantErr: errors.EmptyFieldError("name"), }, + { + name: "valid", + req: &UpdateFlagRequest{ + Key: "key", + Name: "name", + Description: "desc", + Enabled: true, + }, + }, } for _, tt := range tests { @@ -159,6 +185,12 @@ func TestValidate_DeleteFlagRequest(t *testing.T) { }, wantErr: errors.EmptyFieldError("key"), }, + { + name: "valid", + req: &DeleteFlagRequest{ + Key: "key", + }, + }, } for _, tt := range tests { @@ -200,6 +232,15 @@ func TestValidate_CreateVariantRequest(t *testing.T) { }, wantErr: errors.EmptyFieldError("key"), }, + { + name: "valid", + req: &CreateVariantRequest{ + FlagKey: "flagKey", + Key: "key", + Name: "name", + Description: "desc", + }, + }, } for _, tt := range tests { @@ -254,6 +295,16 @@ func TestValidate_UpdateVariantRequest(t *testing.T) { }, wantErr: errors.EmptyFieldError("key"), }, + { + name: "valid", + req: &UpdateVariantRequest{ + Id: "id", + FlagKey: "flagKey", + Key: "key", + Name: "name", + Description: "desc", + }, + }, } for _, tt := range tests { @@ -291,6 +342,13 @@ func TestValidate_DeleteVariantRequest(t *testing.T) { }, wantErr: errors.EmptyFieldError("flagKey"), }, + { + name: "valid", + req: &DeleteVariantRequest{ + Id: "id", + FlagKey: "flagKey", + }, + }, } for _, tt := range tests { @@ -317,6 +375,10 @@ func TestValidate_ListRuleRequest(t *testing.T) { req: &ListRuleRequest{FlagKey: ""}, wantErr: errors.EmptyFieldError("flagKey"), }, + { + name: "valid", + req: &ListRuleRequest{FlagKey: "flagKey"}, + }, } for _, tt := range tests { @@ -348,6 +410,10 @@ func TestValidate_GetRuleRequest(t *testing.T) { req: &GetRuleRequest{Id: "id", FlagKey: ""}, wantErr: errors.EmptyFieldError("flagKey"), }, + { + name: "valid", + req: &GetRuleRequest{Id: "id", FlagKey: "flagKey"}, + }, } for _, tt := range tests { @@ -396,6 +462,14 @@ func TestValidate_CreateRuleRequest(t *testing.T) { }, wantErr: errors.InvalidFieldError("rank", "must be greater than 0"), }, + { + name: "valid", + req: &CreateRuleRequest{ + FlagKey: "flagKey", + SegmentKey: "segmentKey", + Rank: 1, + }, + }, } for _, tt := range tests { @@ -444,6 +518,14 @@ func TestValidate_UpdateRuleRequest(t *testing.T) { }, wantErr: errors.EmptyFieldError("segmentKey"), }, + { + name: "valid", + req: &UpdateRuleRequest{ + Id: "id", + FlagKey: "flagKey", + SegmentKey: "segmentKey", + }, + }, } for _, tt := range tests { @@ -481,6 +563,13 @@ func TestValidate_DeleteRuleRequest(t *testing.T) { }, wantErr: errors.EmptyFieldError("flagKey"), }, + { + name: "valid", + req: &DeleteRuleRequest{ + Id: "id", + FlagKey: "flagKey", + }, + }, } for _, tt := range tests { @@ -512,6 +601,10 @@ func TestValidate_OrderRulesRequest(t *testing.T) { req: &OrderRulesRequest{FlagKey: "flagKey", RuleIds: []string{"1"}}, wantErr: errors.InvalidFieldError("ruleIds", "must contain atleast 2 elements"), }, + { + name: "valid", + req: &OrderRulesRequest{FlagKey: "flagKey", RuleIds: []string{"1", "2"}}, + }, } for _, tt := range tests { @@ -558,6 +651,10 @@ func TestValidate_CreateDistributionRequest(t *testing.T) { req: &CreateDistributionRequest{FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID", Rollout: 101}, wantErr: errors.InvalidFieldError("rollout", "must be less than or equal to '100'"), }, + { + name: "valid", + req: &CreateDistributionRequest{FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID", Rollout: 100}, + }, } for _, tt := range tests { @@ -609,6 +706,10 @@ func TestValidate_UpdateDistributionRequest(t *testing.T) { req: &UpdateDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID", Rollout: 101}, wantErr: errors.InvalidFieldError("rollout", "must be less than or equal to '100'"), }, + { + name: "valid", + req: &UpdateDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID", Rollout: 100}, + }, } for _, tt := range tests { @@ -650,6 +751,10 @@ func TestValidate_DeleteDistributionRequest(t *testing.T) { req: &DeleteDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "ruleID", VariantId: ""}, wantErr: errors.EmptyFieldError("variantId"), }, + { + name: "emptyVariantID", + req: &DeleteDistributionRequest{Id: "id", FlagKey: "flagKey", RuleId: "ruleID", VariantId: "variantID"}, + }, } for _, tt := range tests { @@ -676,6 +781,10 @@ func TestValidate_GetSegmentRequest(t *testing.T) { req: &GetSegmentRequest{Key: ""}, wantErr: errors.EmptyFieldError("key"), }, + { + name: "valid", + req: &GetSegmentRequest{Key: "key"}, + }, } for _, tt := range tests { @@ -715,6 +824,14 @@ func TestValidate_CreateSegmentRequest(t *testing.T) { }, wantErr: errors.EmptyFieldError("name"), }, + { + name: "valid", + req: &CreateSegmentRequest{ + Key: "key", + Name: "name", + Description: "desc", + }, + }, } for _, tt := range tests { @@ -754,6 +871,14 @@ func TestValidate_UpdateSegmentRequest(t *testing.T) { }, wantErr: errors.EmptyFieldError("name"), }, + { + name: "valid", + req: &UpdateSegmentRequest{ + Key: "key", + Name: "name", + Description: "desc", + }, + }, } for _, tt := range tests { @@ -780,6 +905,10 @@ func TestValidate_DeleteSegmentRequest(t *testing.T) { req: &DeleteSegmentRequest{Key: ""}, wantErr: errors.EmptyFieldError("key"), }, + { + name: "valid", + req: &DeleteSegmentRequest{Key: "key"}, + }, } for _, tt := range tests { @@ -879,7 +1008,7 @@ func TestValidate_CreateConstraintRequest(t *testing.T) { wantErr: errors.ErrInvalid("invalid constraint type: \"UNKNOWN_COMPARISON_TYPE\""), }, { - name: "ok", + name: "valid", req: &CreateConstraintRequest{ SegmentKey: "segmentKey", Type: ComparisonType_STRING_COMPARISON_TYPE, @@ -889,7 +1018,7 @@ func TestValidate_CreateConstraintRequest(t *testing.T) { }, }, { - name: "emptyValue ok", + name: "emptyValue valid", req: &CreateConstraintRequest{ SegmentKey: "segmentKey", Type: ComparisonType_STRING_COMPARISON_TYPE, @@ -1025,7 +1154,7 @@ func TestValidate_UpdateConstraintRequest(t *testing.T) { wantErr: errors.ErrInvalid("invalid constraint type: \"UNKNOWN_COMPARISON_TYPE\""), }, { - name: "ok", + name: "valid", req: &UpdateConstraintRequest{ Id: "1", SegmentKey: "segmentKey", @@ -1036,7 +1165,7 @@ func TestValidate_UpdateConstraintRequest(t *testing.T) { }, }, { - name: "emptyValue ok", + name: "emptyValue valid", req: &UpdateConstraintRequest{ Id: "1", SegmentKey: "segmentKey", @@ -1087,6 +1216,10 @@ func TestValidate_DeleteConstraintRequest(t *testing.T) { req: &DeleteConstraintRequest{Id: "id", SegmentKey: ""}, wantErr: errors.EmptyFieldError("segmentKey"), }, + { + name: "valid", + req: &DeleteConstraintRequest{Id: "id", SegmentKey: "segmentKey"}, + }, } for _, tt := range tests { diff --git a/server/server_test.go b/server/server_test.go index 7c30e0e21a..25ef81df3f 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -27,6 +27,58 @@ func TestNew(t *testing.T) { assert.NotNil(t, server) } +type validatable struct { + err error +} + +func (v *validatable) Validate() error { + return v.err +} + +func TestValidationUnaryInterceptor(t *testing.T) { + tests := []struct { + name string + req interface{} + wantCalled int + }{ + { + name: "does not implement Validate", + req: struct{}{}, + wantCalled: 1, + }, + { + name: "implements validate no error", + req: &validatable{}, + wantCalled: 1, + }, + { + name: "implements validate error", + req: &validatable{err: errors.New("invalid")}, + }, + } + + for _, tt := range tests { + var ( + req = tt.req + called int + ) + + t.Run(tt.name, func(t *testing.T) { + var ( + subject = &Server{} + + spyHandler = grpc.UnaryHandler(func(ctx context.Context, req interface{}) (interface{}, error) { + called++ + return nil, nil + }) + ) + + _, _ = subject.ValidationUnaryInterceptor(context.Background(), req, nil, spyHandler) + assert.Equal(t, tt.wantCalled, called) + }) + } +} + func TestErrorUnaryInterceptor(t *testing.T) { tests := []struct { name string From 57e21557ed0b121cf575c97e462c07b572750527 Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Thu, 28 Nov 2019 14:56:53 -0500 Subject: [PATCH 12/12] lint --- server/server_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/server/server_test.go b/server/server_test.go index 25ef81df3f..6672c17c73 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -59,8 +59,9 @@ func TestValidationUnaryInterceptor(t *testing.T) { for _, tt := range tests { var ( - req = tt.req - called int + req = tt.req + wantCalled = tt.wantCalled + called int ) t.Run(tt.name, func(t *testing.T) { @@ -74,7 +75,7 @@ func TestValidationUnaryInterceptor(t *testing.T) { ) _, _ = subject.ValidationUnaryInterceptor(context.Background(), req, nil, spyHandler) - assert.Equal(t, tt.wantCalled, called) + assert.Equal(t, wantCalled, called) }) } }