Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: (sqlite) eval storage namespace support #1372

Merged
merged 3 commits into from
Mar 3, 2023
Merged

feat: (sqlite) eval storage namespace support #1372

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6747a28
feat: (sqlite) add namespace support for rules storage
markphelps Mar 2, 2023
185680f
feat: (sqlite) eval storage namespace support
markphelps Mar 2, 2023
9772be6
chore: namespace error suggestion if namespaces dont match
markphelps Mar 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions internal/ext/exporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const defaultBatchSize = 25
type lister interface {
ListFlags(ctx context.Context, namespaceKey string, opts ...storage.QueryOption) (storage.ResultSet[*flipt.Flag], error)
ListSegments(ctx context.Context, namespaceKey string, opts ...storage.QueryOption) (storage.ResultSet[*flipt.Segment], error)
ListRules(ctx context.Context, flagKey string, opts ...storage.QueryOption) (storage.ResultSet[*flipt.Rule], error)
ListRules(ctx context.Context, namespaceKey, flagKey string, opts ...storage.QueryOption) (storage.ResultSet[*flipt.Rule], error)
}

type Exporter struct {
Expand Down Expand Up @@ -88,8 +88,10 @@ func (e *Exporter) Export(ctx context.Context, w io.Writer) error {
variantKeys[v.Id] = v.Key
}

// TODO: support all namespaces

// export rules for flag
resp, err := e.store.ListRules(ctx, flag.Key)
resp, err := e.store.ListRules(ctx, storage.DefaultNamespace, flag.Key)
if err != nil {
return fmt.Errorf("getting rules for flag %q: %w", flag.Key, err)
}
Expand Down
2 changes: 1 addition & 1 deletion internal/ext/exporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func (m mockLister) ListSegments(ctx context.Context, namespaceKey string, opts
}, m.segmentErr
}

func (m mockLister) ListRules(ctx context.Context, flagKey string, opts ...storage.QueryOption) (storage.ResultSet[*flipt.Rule], error) {
func (m mockLister) ListRules(ctx context.Context, namespaceKey string, flagKey string, opts ...storage.QueryOption) (storage.ResultSet[*flipt.Rule], error) {
return storage.ResultSet[*flipt.Rule]{
Results: m.rules,
}, m.ruleErr
Expand Down
21 changes: 18 additions & 3 deletions internal/server/evaluator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ import (
// Evaluate evaluates a request for a given flag and entity
func (s *Server) Evaluate(ctx context.Context, r *flipt.EvaluationRequest) (*flipt.EvaluationResponse, error) {
s.logger.Debug("evaluate", zap.Stringer("request", r))

if r.NamespaceKey == "" {
r.NamespaceKey = storage.DefaultNamespace
}

resp, err := s.evaluate(ctx, r)
if err != nil {
return resp, err
Expand Down Expand Up @@ -53,6 +58,11 @@ func (s *Server) Evaluate(ctx context.Context, r *flipt.EvaluationRequest) (*fli
// BatchEvaluate evaluates a request for multiple flags and entities
func (s *Server) BatchEvaluate(ctx context.Context, r *flipt.BatchEvaluationRequest) (*flipt.BatchEvaluationResponse, error) {
s.logger.Debug("batch-evaluate", zap.Stringer("request", r))

if r.NamespaceKey == "" {
r.NamespaceKey = storage.DefaultNamespace
}

resp, err := s.batchEvaluate(ctx, r)
if err != nil {
return nil, err
Expand All @@ -68,9 +78,12 @@ func (s *Server) batchEvaluate(ctx context.Context, r *flipt.BatchEvaluationRequ

// TODO: we should change this to a native batch query instead of looping through
// each request individually
for _, flag := range r.GetRequests() {
for _, req := range r.GetRequests() {
// ensure all requests have the same namespace
req.NamespaceKey = r.NamespaceKey
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My gut says we probably should just reject a batch request if it contains get requests with namespaces which don't match the batch payload namespace.

Maybe we could at-least support implying a blank namespace in a child evaluate means inherits the parent batch request namespace?

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that makes sense. in retrospect I should have made the batch API use it's own set of request objects which don't share the same properties with the EvaluateRequest.. but live and learn I guess. something to think about if we do a v2 API

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense.


// TODO: we also need to validate each request, we should likely do this in the validation middleware
f, err := s.evaluate(ctx, flag)
f, err := s.evaluate(ctx, req)
if err != nil {
var errnf errs.ErrNotFound
if r.GetExcludeNotFound() && errors.As(err, &errnf) {
Expand All @@ -90,7 +103,9 @@ func (s *Server) evaluate(ctx context.Context, r *flipt.EvaluationRequest) (resp
startTime = time.Now().UTC()
flagAttr = metrics.AttributeFlag.String(r.FlagKey)
)

metrics.EvaluationsTotal.Add(ctx, 1, flagAttr)

defer func() {
if err == nil {
metrics.EvaluationResultsTotal.Add(ctx, 1,
Expand Down Expand Up @@ -136,7 +151,7 @@ func (s *Server) evaluate(ctx context.Context, r *flipt.EvaluationRequest) (resp
return resp, nil
}

rules, err := s.store.GetEvaluationRules(ctx, r.FlagKey)
rules, err := s.store.GetEvaluationRules(ctx, r.NamespaceKey, r.FlagKey)
if err != nil {
resp.Reason = flipt.EvaluationReason_ERROR_EVALUATION_REASON
return resp, err
Expand Down
38 changes: 19 additions & 19 deletions internal/server/evaluator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func TestBatchEvaluate(t *testing.T) {
store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)
store.On("GetFlag", mock.Anything, mock.Anything, "bar").Return(disabled, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return([]*storage.EvaluationRule{}, nil)
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return([]*storage.EvaluationRule{}, nil)

resp, err := s.BatchEvaluate(context.TODO(), &flipt.BatchEvaluationRequest{
RequestId: "12345",
Expand Down Expand Up @@ -86,7 +86,7 @@ func TestBatchEvaluate_FlagNotFoundExcluded(t *testing.T) {
store.On("GetFlag", mock.Anything, mock.Anything, "bar").Return(disabled, nil)
store.On("GetFlag", mock.Anything, mock.Anything, "NotFoundFlag").Return(&flipt.Flag{}, errs.ErrNotFoundf("flag %q", "NotFoundFlag"))

store.On("GetEvaluationRules", mock.Anything, "foo").Return([]*storage.EvaluationRule{}, nil)
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return([]*storage.EvaluationRule{}, nil)

resp, err := s.BatchEvaluate(context.TODO(), &flipt.BatchEvaluationRequest{
RequestId: "12345",
Expand Down Expand Up @@ -134,7 +134,7 @@ func TestBatchEvaluate_FlagNotFound(t *testing.T) {
store.On("GetFlag", mock.Anything, mock.Anything, "bar").Return(disabled, nil)
store.On("GetFlag", mock.Anything, mock.Anything, "NotFoundFlag").Return(&flipt.Flag{}, errs.ErrNotFoundf("flag %q", "NotFoundFlag"))

store.On("GetEvaluationRules", mock.Anything, "foo").Return([]*storage.EvaluationRule{}, nil)
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return([]*storage.EvaluationRule{}, nil)

_, err := s.BatchEvaluate(context.TODO(), &flipt.BatchEvaluationRequest{
RequestId: "12345",
Expand Down Expand Up @@ -225,7 +225,7 @@ func TestEvaluate_FlagNoRules(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return([]*storage.EvaluationRule{}, nil)
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return([]*storage.EvaluationRule{}, nil)

resp, err := s.Evaluate(context.TODO(), &flipt.EvaluationRequest{
EntityId: "1",
Expand All @@ -252,7 +252,7 @@ func TestEvaluate_ErrorGettingRules(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return([]*storage.EvaluationRule{}, errors.New("error getting rules!"))
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return([]*storage.EvaluationRule{}, errors.New("error getting rules!"))

resp, err := s.Evaluate(context.TODO(), &flipt.EvaluationRequest{
EntityId: "1",
Expand All @@ -279,7 +279,7 @@ func TestEvaluate_RulesOutOfOrder(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down Expand Up @@ -341,7 +341,7 @@ func TestEvaluate_ErrorGettingDistributions(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down Expand Up @@ -389,7 +389,7 @@ func TestEvaluate_MatchAll_NoVariants_NoDistributions(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down Expand Up @@ -478,7 +478,7 @@ func TestEvaluate_MatchAll_SingleVariantDistribution(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down Expand Up @@ -608,7 +608,7 @@ func TestEvaluate_MatchAll_RolloutDistribution(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down Expand Up @@ -729,7 +729,7 @@ func TestEvaluate_MatchAll_RolloutDistribution_MultiRule(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down Expand Up @@ -804,7 +804,7 @@ func TestEvaluate_MatchAll_NoConstraints(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down Expand Up @@ -915,7 +915,7 @@ func TestEvaluate_MatchAny_NoVariants_NoDistributions(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down Expand Up @@ -1004,7 +1004,7 @@ func TestEvaluate_MatchAny_SingleVariantDistribution(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down Expand Up @@ -1166,7 +1166,7 @@ func TestEvaluate_MatchAny_RolloutDistribution(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down Expand Up @@ -1287,7 +1287,7 @@ func TestEvaluate_MatchAny_RolloutDistribution_MultiRule(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down Expand Up @@ -1362,7 +1362,7 @@ func TestEvaluate_MatchAny_NoConstraints(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down Expand Up @@ -1473,7 +1473,7 @@ func TestEvaluate_FirstRolloutRuleIsZero(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down Expand Up @@ -1573,7 +1573,7 @@ func TestEvaluate_MultipleZeroRolloutDistributions(t *testing.T) {

store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(enabledFlag, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down
2 changes: 1 addition & 1 deletion internal/server/middleware/grpc/middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ func TestCacheUnaryInterceptor_Evaluate(t *testing.T) {
Enabled: true,
}, nil)

store.On("GetEvaluationRules", mock.Anything, "foo").Return(
store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return(
[]*storage.EvaluationRule{
{
ID: "1",
Expand Down
16 changes: 8 additions & 8 deletions internal/server/middleware/grpc/support_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,18 +109,18 @@ func (m *storeMock) DeleteConstraint(ctx context.Context, r *flipt.DeleteConstra
return args.Error(0)
}

func (m *storeMock) GetRule(ctx context.Context, id string) (*flipt.Rule, error) {
args := m.Called(ctx, id)
func (m *storeMock) GetRule(ctx context.Context, namespaceKey string, id string) (*flipt.Rule, error) {
args := m.Called(ctx, namespaceKey, id)
return args.Get(0).(*flipt.Rule), args.Error(1)
}

func (m *storeMock) ListRules(ctx context.Context, flagKey string, opts ...storage.QueryOption) (storage.ResultSet[*flipt.Rule], error) {
args := m.Called(ctx, flagKey, opts)
func (m *storeMock) ListRules(ctx context.Context, namespaceKey string, flagKey string, opts ...storage.QueryOption) (storage.ResultSet[*flipt.Rule], error) {
args := m.Called(ctx, namespaceKey, flagKey, opts)
return args.Get(0).(storage.ResultSet[*flipt.Rule]), args.Error(1)
}

func (m *storeMock) CountRules(ctx context.Context) (uint64, error) {
args := m.Called(ctx)
func (m *storeMock) CountRules(ctx context.Context, namespaceKey string) (uint64, error) {
args := m.Called(ctx, namespaceKey)
return args.Get(0).(uint64), args.Error(1)
}

Expand Down Expand Up @@ -159,8 +159,8 @@ func (m *storeMock) DeleteDistribution(ctx context.Context, r *flipt.DeleteDistr
return args.Error(0)
}

func (m *storeMock) GetEvaluationRules(ctx context.Context, flagKey string) ([]*storage.EvaluationRule, error) {
args := m.Called(ctx, flagKey)
func (m *storeMock) GetEvaluationRules(ctx context.Context, namespaceKey string, flagKey string) ([]*storage.EvaluationRule, error) {
args := m.Called(ctx, namespaceKey, flagKey)
return args.Get(0).([]*storage.EvaluationRule), args.Error(1)
}

Expand Down
6 changes: 3 additions & 3 deletions internal/server/rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
// GetRule gets a rule
func (s *Server) GetRule(ctx context.Context, r *flipt.GetRuleRequest) (*flipt.Rule, error) {
s.logger.Debug("get rule", zap.Stringer("request", r))
rule, err := s.store.GetRule(ctx, r.Id)
rule, err := s.store.GetRule(ctx, r.NamespaceKey, r.Id)
s.logger.Debug("get rule", zap.Stringer("response", rule))
return rule, err
}
Expand All @@ -41,7 +41,7 @@ func (s *Server) ListRules(ctx context.Context, r *flipt.ListRuleRequest) (*flip
opts = append(opts, storage.WithOffset(uint64(r.Offset)))
}

results, err := s.store.ListRules(ctx, r.FlagKey, opts...)
results, err := s.store.ListRules(ctx, r.NamespaceKey, r.FlagKey, opts...)
if err != nil {
return nil, err
}
Expand All @@ -50,7 +50,7 @@ func (s *Server) ListRules(ctx context.Context, r *flipt.ListRuleRequest) (*flip

resp.Rules = append(resp.Rules, results.Results...)

total, err := s.store.CountRules(ctx)
total, err := s.store.CountRules(ctx, r.NamespaceKey)
if err != nil {
return nil, err
}
Expand Down
10 changes: 5 additions & 5 deletions internal/server/rule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func TestGetRule(t *testing.T) {
req = &flipt.GetRuleRequest{Id: "id", FlagKey: "flagKey"}
)

store.On("GetRule", mock.Anything, "id").Return(&flipt.Rule{
store.On("GetRule", mock.Anything, mock.Anything, "id").Return(&flipt.Rule{
Id: "1",
}, nil)

Expand All @@ -47,7 +47,7 @@ func TestListRules_PaginationOffset(t *testing.T) {
defer store.AssertExpectations(t)

params := storage.QueryParams{}
store.On("ListRules", mock.Anything, "flagKey", mock.MatchedBy(func(opts []storage.QueryOption) bool {
store.On("ListRules", mock.Anything, mock.Anything, "flagKey", mock.MatchedBy(func(opts []storage.QueryOption) bool {
for _, opt := range opts {
opt(&params)
}
Expand All @@ -64,7 +64,7 @@ func TestListRules_PaginationOffset(t *testing.T) {
NextPageToken: "bar",
}, nil)

store.On("CountRules", mock.Anything).Return(uint64(1), nil)
store.On("CountRules", mock.Anything, mock.Anything).Return(uint64(1), nil)

got, err := s.ListRules(context.TODO(), &flipt.ListRuleRequest{FlagKey: "flagKey",
Offset: 10,
Expand All @@ -90,7 +90,7 @@ func TestListRules_PaginationPageToken(t *testing.T) {
defer store.AssertExpectations(t)

params := storage.QueryParams{}
store.On("ListRules", mock.Anything, "flagKey", mock.MatchedBy(func(opts []storage.QueryOption) bool {
store.On("ListRules", mock.Anything, mock.Anything, "flagKey", mock.MatchedBy(func(opts []storage.QueryOption) bool {
for _, opt := range opts {
opt(&params)
}
Expand All @@ -107,7 +107,7 @@ func TestListRules_PaginationPageToken(t *testing.T) {
NextPageToken: "bar",
}, nil)

store.On("CountRules", mock.Anything).Return(uint64(1), nil)
store.On("CountRules", mock.Anything, mock.Anything).Return(uint64(1), nil)

got, err := s.ListRules(context.TODO(), &flipt.ListRuleRequest{FlagKey: "flagKey",
PageToken: "Zm9v",
Expand Down
Loading