Skip to content

Commit

Permalink
feat: (sqlite) eval storage namespace support (#1372)
Browse files Browse the repository at this point in the history
  • Loading branch information
@markphelps
markphelps authored Mar 3, 2023
1 parent d8482a3 commit 37062fb
Show file tree
Hide file tree
Showing 10 changed files with 789 additions and 721 deletions.
25 changes: 22 additions & 3 deletions internal/server/evaluator.go
View file
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,16 @@ 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
if req.NamespaceKey == "" {
req.NamespaceKey = r.NamespaceKey
} else if req.NamespaceKey != r.NamespaceKey {
return &res, errs.InvalidFieldError("namespace_key", "must be the same for all requests if specified")
}

// 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 +107,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 +155,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
67 changes: 48 additions & 19 deletions internal/server/evaluator_test.go
View file
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 All @@ -68,6 +68,35 @@ func TestBatchEvaluate(t *testing.T) {
assert.False(t, resp.Responses[0].Match)
}

func TestBatchEvaluate_NamespaceMismatch(t *testing.T) {
var (
store = &storeMock{}
logger = zaptest.NewLogger(t)
s = &Server{
logger: logger,
store: store,
}
)

_, err := s.BatchEvaluate(context.TODO(), &flipt.BatchEvaluationRequest{
NamespaceKey: "foo",
RequestId: "12345",
ExcludeNotFound: true,
Requests: []*flipt.EvaluationRequest{
{
NamespaceKey: "bar",
EntityId: "1",
FlagKey: "foo",
Context: map[string]string{
"bar": "boz",
},
},
},
})

assert.EqualError(t, err, "invalid field namespace_key: must be the same for all requests if specified")
}

func TestBatchEvaluate_FlagNotFoundExcluded(t *testing.T) {
var (
store = &storeMock{}
Expand All @@ -86,7 +115,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 +163,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 +254,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 +281,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 +308,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 +370,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 +418,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 +507,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 +637,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 +758,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 +833,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 +944,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 +1033,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 +1195,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 +1316,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 +1391,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 +1502,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 +1602,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
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
4 changes: 2 additions & 2 deletions internal/server/middleware/grpc/support_test.go
View file
Original file line number Diff line number Diff line change
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
4 changes: 2 additions & 2 deletions internal/server/support_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,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
15 changes: 10 additions & 5 deletions internal/storage/sql/common/evaluation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,16 @@ import (
flipt "go.flipt.io/flipt/rpc/flipt"
)

func (s *Store) GetEvaluationRules(ctx context.Context, flagKey string) ([]*storage.EvaluationRule, error) {
func (s *Store) GetEvaluationRules(ctx context.Context, namespaceKey, flagKey string) ([]*storage.EvaluationRule, error) {
if namespaceKey == "" {
namespaceKey = storage.DefaultNamespace
}
// get all rules for flag with their constraints if any
rows, err := s.builder.Select("r.id, r.flag_key, r.segment_key, s.match_type, r.\"rank\", c.id, c.type, c.property, c.operator, c.value").
rows, err := s.builder.Select("r.id, r.namespace_key, r.flag_key, r.segment_key, s.match_type, r.\"rank\", c.id, c.type, c.property, c.operator, c.value").
From("rules r").
Join("segments s on (r.segment_key = s.\"key\")").
LeftJoin("constraints c ON (s.\"key\" = c.segment_key)").
Where(sq.Eq{"r.flag_key": flagKey}).
Join("segments s ON (r.segment_key = s.\"key\" AND r.namespace_key = s.namespace_key)").
LeftJoin("constraints c ON (s.\"key\" = c.segment_key AND s.namespace_key = c.namespace_key)").
Where(sq.And{sq.Eq{"r.flag_key": flagKey}, sq.Eq{"r.namespace_key": namespaceKey}}).
OrderBy("r.\"rank\" ASC").
GroupBy("r.id, c.id, s.match_type").
QueryContext(ctx)
Expand All @@ -42,6 +45,7 @@ func (s *Store) GetEvaluationRules(ctx context.Context, flagKey string) ([]*stor

if err := rows.Scan(
&tempRule.ID,
&tempRule.NamespaceKey,
&tempRule.FlagKey,
&tempRule.SegmentKey,
&tempRule.SegmentMatchType,
Expand Down Expand Up @@ -70,6 +74,7 @@ func (s *Store) GetEvaluationRules(ctx context.Context, flagKey string) ([]*stor
// haven't seen this rule before
newRule := &storage.EvaluationRule{
ID: tempRule.ID,
NamespaceKey: tempRule.NamespaceKey,
FlagKey: tempRule.FlagKey,
SegmentKey: tempRule.SegmentKey,
SegmentMatchType: tempRule.SegmentMatchType,
Expand Down
5 changes: 4 additions & 1 deletion internal/storage/sql/evaluation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.flipt.io/flipt/internal/storage"
flipt "go.flipt.io/flipt/rpc/flipt"
)

Expand Down Expand Up @@ -70,20 +71,22 @@ func (s *DBTestSuite) TestGetEvaluationRules() {

require.NoError(t, err)

evaluationRules, err := s.store.GetEvaluationRules(context.TODO(), flag.Key)
evaluationRules, err := s.store.GetEvaluationRules(context.TODO(), storage.DefaultNamespace, flag.Key)
require.NoError(t, err)

assert.NotEmpty(t, evaluationRules)
assert.Equal(t, 2, len(evaluationRules))

assert.Equal(t, rule1.Id, evaluationRules[0].ID)
assert.Equal(t, storage.DefaultNamespace, evaluationRules[0].NamespaceKey)
assert.Equal(t, rule1.FlagKey, evaluationRules[0].FlagKey)
assert.Equal(t, rule1.SegmentKey, evaluationRules[0].SegmentKey)
assert.Equal(t, segment.MatchType, evaluationRules[0].SegmentMatchType)
assert.Equal(t, rule1.Rank, evaluationRules[0].Rank)
assert.Equal(t, 2, len(evaluationRules[0].Constraints))

assert.Equal(t, rule2.Id, evaluationRules[1].ID)
assert.Equal(t, storage.DefaultNamespace, evaluationRules[1].NamespaceKey)
assert.Equal(t, rule2.FlagKey, evaluationRules[1].FlagKey)
assert.Equal(t, rule2.SegmentKey, evaluationRules[1].SegmentKey)
assert.Equal(t, segment.MatchType, evaluationRules[1].SegmentMatchType)
Expand Down
3 changes: 2 additions & 1 deletion internal/storage/storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const (
// given flagKey matches a segment
type EvaluationRule struct {
ID string
NamespaceKey string
FlagKey string
SegmentKey string
SegmentMatchType flipt.MatchType
Expand Down Expand Up @@ -127,7 +128,7 @@ const DefaultNamespace = "default"
type EvaluationStore interface {
// GetEvaluationRules returns rules applicable to flagKey provided
// Note: Rules MUST be returned in order by Rank
GetEvaluationRules(ctx context.Context, flagKey string) ([]*EvaluationRule, error)
GetEvaluationRules(ctx context.Context, namespaceKey, flagKey string) ([]*EvaluationRule, error)
GetEvaluationDistributions(ctx context.Context, ruleID string) ([]*EvaluationDistribution, error)
}

Expand Down
Loading

0 comments on commit 37062fb

Please sign in to comment.