From 17ae6155cda55628289f0c1b3d22ce8d3459d861 Mon Sep 17 00:00:00 2001 From: Sebastian Machuca Date: Fri, 2 Jan 2026 17:51:21 +1100 Subject: [PATCH] Using comma-ok pattern for rule evaluation --- .testcoverage.yml | 1 + codecov.yml | 1 + internal/flags/evaluator.go | 12 ++-- internal/flags/evaluator_test.go | 96 ++++++++++++++++---------------- internal/flags/service.go | 7 +-- internal/flags/service_test.go | 6 +- internal/handler/handler.go | 19 ------- internal/handler/routes.go | 25 +++++++++ 8 files changed, 86 insertions(+), 81 deletions(-) create mode 100644 internal/handler/routes.go diff --git a/.testcoverage.yml b/.testcoverage.yml index 8572f9f..0b4c497 100644 --- a/.testcoverage.yml +++ b/.testcoverage.yml @@ -15,3 +15,4 @@ exclude: # Main entry points and DI wiring paths: - ^cmd/server/main\.go$ + - ^internal/handler/routes\.go diff --git a/codecov.yml b/codecov.yml index 5a08687..4a136b6 100644 --- a/codecov.yml +++ b/codecov.yml @@ -6,3 +6,4 @@ coverage: ignore: - "cmd/**/*" + - "internal/handler/routes.go" \ No newline at end of file diff --git a/internal/flags/evaluator.go b/internal/flags/evaluator.go index 241d6cf..e135831 100644 --- a/internal/flags/evaluator.go +++ b/internal/flags/evaluator.go @@ -2,17 +2,17 @@ package flags import "strings" -type RuleMatcher func(rules []Rule, evalCtx EvalContext) *Rule +type RuleMatcher func(rules []Rule, evalCtx EvalContext) (Rule, bool) func DefaultRuleMatcher() RuleMatcher { - return func(rules []Rule, evalCtx EvalContext) *Rule { - for i := range rules { - if matchesRule(rules[i], evalCtx) { - return &rules[i] + return func(rules []Rule, evalCtx EvalContext) (Rule, bool) { + for _, rule := range rules { + if matchesRule(rule, evalCtx) { + return rule, true } } - return nil + return Rule{}, false } } diff --git a/internal/flags/evaluator_test.go b/internal/flags/evaluator_test.go index 3632b06..3045128 100644 --- a/internal/flags/evaluator_test.go +++ b/internal/flags/evaluator_test.go @@ -26,11 +26,11 @@ func TestRuleMatcher_ReturnsFirstMatch(t *testing.T) { }, } - result := matcher(rules, flags.EvalContext{ + result, ok := matcher(rules, flags.EvalContext{ Attrs: map[string]any{"plan": "premium"}, }) - require.NotNil(t, result) + require.True(t, ok) assert.Equal(t, "rule-2", result.ID) } @@ -46,11 +46,11 @@ func TestRuleMatcher_NoMatch(t *testing.T) { }, } - result := matcher(rules, flags.EvalContext{ + _, ok := matcher(rules, flags.EvalContext{ Attrs: map[string]any{"plan": "free"}, }) - assert.Nil(t, result) + assert.False(t, ok) } func TestRuleMatcher_EmptyRules(t *testing.T) { @@ -58,9 +58,9 @@ func TestRuleMatcher_EmptyRules(t *testing.T) { matcher := flags.DefaultRuleMatcher() - result := matcher([]flags.Rule{}, flags.EvalContext{}) + _, ok := matcher([]flags.Rule{}, flags.EvalContext{}) - assert.Nil(t, result) + assert.False(t, ok) } func TestRuleMatcher_OpEquals(t *testing.T) { @@ -75,15 +75,15 @@ func TestRuleMatcher_OpEquals(t *testing.T) { }, } - result := matcher(rules, flags.EvalContext{ + _, ok := matcher(rules, flags.EvalContext{ Attrs: map[string]any{"country": "US"}, }) - require.NotNil(t, result) + require.True(t, ok) - result = matcher(rules, flags.EvalContext{ + _, ok = matcher(rules, flags.EvalContext{ Attrs: map[string]any{"country": "UK"}, }) - assert.Nil(t, result) + assert.False(t, ok) } func TestRuleMatcher_OpNotEquals(t *testing.T) { @@ -98,15 +98,15 @@ func TestRuleMatcher_OpNotEquals(t *testing.T) { }, } - result := matcher(rules, flags.EvalContext{ + _, ok := matcher(rules, flags.EvalContext{ Attrs: map[string]any{"env": "staging"}, }) - require.NotNil(t, result) + require.True(t, ok) - result = matcher(rules, flags.EvalContext{ + _, ok = matcher(rules, flags.EvalContext{ Attrs: map[string]any{"env": "production"}, }) - assert.Nil(t, result) + assert.False(t, ok) } func TestRuleMatcher_OpIn(t *testing.T) { @@ -121,15 +121,15 @@ func TestRuleMatcher_OpIn(t *testing.T) { }, } - result := matcher(rules, flags.EvalContext{ + _, ok := matcher(rules, flags.EvalContext{ Attrs: map[string]any{"region": "us-east"}, }) - require.NotNil(t, result) + require.True(t, ok) - result = matcher(rules, flags.EvalContext{ + _, ok = matcher(rules, flags.EvalContext{ Attrs: map[string]any{"region": "eu-west"}, }) - assert.Nil(t, result) + assert.False(t, ok) } func TestRuleMatcher_OpNotIn(t *testing.T) { @@ -144,15 +144,15 @@ func TestRuleMatcher_OpNotIn(t *testing.T) { }, } - result := matcher(rules, flags.EvalContext{ + _, ok := matcher(rules, flags.EvalContext{ Attrs: map[string]any{"country": "US"}, }) - require.NotNil(t, result) + require.True(t, ok) - result = matcher(rules, flags.EvalContext{ + _, ok = matcher(rules, flags.EvalContext{ Attrs: map[string]any{"country": "CN"}, }) - assert.Nil(t, result) + assert.False(t, ok) } func TestRuleMatcher_OpExists(t *testing.T) { @@ -167,15 +167,15 @@ func TestRuleMatcher_OpExists(t *testing.T) { }, } - result := matcher(rules, flags.EvalContext{ + _, ok := matcher(rules, flags.EvalContext{ Attrs: map[string]any{"beta_enabled": true}, }) - require.NotNil(t, result) + require.True(t, ok) - result = matcher(rules, flags.EvalContext{ + _, ok = matcher(rules, flags.EvalContext{ Attrs: map[string]any{}, }) - assert.Nil(t, result) + assert.False(t, ok) } func TestRuleMatcher_OpStartsWith(t *testing.T) { @@ -190,15 +190,15 @@ func TestRuleMatcher_OpStartsWith(t *testing.T) { }, } - result := matcher(rules, flags.EvalContext{ + _, ok := matcher(rules, flags.EvalContext{ Attrs: map[string]any{"email": "@internal.company.com"}, }) - require.NotNil(t, result) + require.True(t, ok) - result = matcher(rules, flags.EvalContext{ + _, ok = matcher(rules, flags.EvalContext{ Attrs: map[string]any{"email": "user@external.com"}, }) - assert.Nil(t, result) + assert.False(t, ok) } func TestRuleMatcher_MultipleConditions_AllMustMatch(t *testing.T) { @@ -217,20 +217,20 @@ func TestRuleMatcher_MultipleConditions_AllMustMatch(t *testing.T) { }, } - result := matcher(rules, flags.EvalContext{ + _, ok := matcher(rules, flags.EvalContext{ Attrs: map[string]any{"plan": "premium", "country": "US", "verified": true}, }) - require.NotNil(t, result) + require.True(t, ok) - result = matcher(rules, flags.EvalContext{ + _, ok = matcher(rules, flags.EvalContext{ Attrs: map[string]any{"plan": "premium", "country": "US"}, }) - assert.Nil(t, result) + assert.False(t, ok) - result = matcher(rules, flags.EvalContext{ + _, ok = matcher(rules, flags.EvalContext{ Attrs: map[string]any{"plan": "free", "country": "US", "verified": true}, }) - assert.Nil(t, result) + assert.False(t, ok) } func TestRuleMatcher_BuiltInAttrs(t *testing.T) { @@ -249,16 +249,16 @@ func TestRuleMatcher_BuiltInAttrs(t *testing.T) { }, } - result := matcher(rules, flags.EvalContext{ + result, ok := matcher(rules, flags.EvalContext{ UserID: "user-123", }) - require.NotNil(t, result) + require.True(t, ok) assert.Equal(t, "user-rule", result.ID) - result = matcher(rules, flags.EvalContext{ + result, ok = matcher(rules, flags.EvalContext{ TenantID: "tenant-456", }) - require.NotNil(t, result) + require.True(t, ok) assert.Equal(t, "tenant-rule", result.ID) } @@ -274,10 +274,10 @@ func TestRuleMatcher_UnknownOp(t *testing.T) { }, } - result := matcher(rules, flags.EvalContext{ + _, ok := matcher(rules, flags.EvalContext{ Attrs: map[string]any{"x": "y"}, }) - assert.Nil(t, result) + assert.False(t, ok) } func TestRuleMatcher_NilAttrs(t *testing.T) { @@ -292,10 +292,10 @@ func TestRuleMatcher_NilAttrs(t *testing.T) { }, } - result := matcher(rules, flags.EvalContext{ + _, ok := matcher(rules, flags.EvalContext{ Attrs: nil, }) - assert.Nil(t, result) + assert.False(t, ok) } func TestRuleMatcher_OpIn_InvalidListType(t *testing.T) { @@ -310,10 +310,10 @@ func TestRuleMatcher_OpIn_InvalidListType(t *testing.T) { }, } - result := matcher(rules, flags.EvalContext{ + _, ok := matcher(rules, flags.EvalContext{ Attrs: map[string]any{"plan": "premium"}, }) - assert.Nil(t, result) + assert.False(t, ok) } func TestRuleMatcher_OpNotIn_InvalidListType(t *testing.T) { @@ -328,9 +328,9 @@ func TestRuleMatcher_OpNotIn_InvalidListType(t *testing.T) { }, } - result := matcher(rules, flags.EvalContext{ + result, ok := matcher(rules, flags.EvalContext{ Attrs: map[string]any{"plan": "premium"}, }) - require.NotNil(t, result) + require.True(t, ok) assert.Equal(t, "invalid-not-in", result.ID) } diff --git a/internal/flags/service.go b/internal/flags/service.go index 66cc3d2..f47b661 100644 --- a/internal/flags/service.go +++ b/internal/flags/service.go @@ -11,10 +11,7 @@ type Service struct { } func NewService(repo Repository) *Service { - return &Service{ - repo: repo, - ruleMatcher: DefaultRuleMatcher(), - } + return NewServiceWithMatcher(repo, DefaultRuleMatcher()) } func NewServiceWithMatcher(repo Repository, matcher RuleMatcher) *Service { @@ -48,7 +45,7 @@ func (s *Service) Evaluate(ctx context.Context, key string, evalCtx EvalContext) return result, nil } - if rule := s.ruleMatcher(flag.Rules, evalCtx); rule != nil { + if rule, ok := s.ruleMatcher(flag.Rules, evalCtx); ok { result.Value = rule.Value result.Reason = ReasonRuleMatch result.RuleID = rule.ID diff --git a/internal/flags/service_test.go b/internal/flags/service_test.go index 8b20e85..04c4057 100644 --- a/internal/flags/service_test.go +++ b/internal/flags/service_test.go @@ -365,11 +365,11 @@ func TestService_WithCustomMatcher(t *testing.T) { t.Parallel() repo := flags.NewMemoryRepository() - customMatcher := func(_ []flags.Rule, _ flags.EvalContext) *flags.Rule { - return &flags.Rule{ + customMatcher := func(_ []flags.Rule, _ flags.EvalContext) (flags.Rule, bool) { + return flags.Rule{ ID: "custom-rule", Value: flags.StringValue("custom-value"), - } + }, true } svc := flags.NewServiceWithMatcher(repo, customMatcher) ctx := context.Background() diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 97bfb15..4b7e304 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -3,7 +3,6 @@ package handler import ( "context" "errors" - "net/http" "github.com/danielgtaylor/huma/v2" "github.com/serroba/features/internal/flags" @@ -17,24 +16,6 @@ func New(service *flags.Service) *Handler { return &Handler{service: service} } -func (h *Handler) Register(api huma.API) { - huma.Register(api, huma.Operation{ - OperationID: "create-flag", - Method: http.MethodPost, - Path: "/flags", - Summary: "Create a new feature flag", - Tags: []string{"Flags"}, - }, h.CreateFlag) - - huma.Register(api, huma.Operation{ - OperationID: "evaluate-flag", - Method: http.MethodPost, - Path: "/flags/{key}/evaluate", - Summary: "Evaluate a feature flag", - Tags: []string{"Flags"}, - }, h.EvaluateFlag) -} - func (h *Handler) CreateFlag(ctx context.Context, req *CreateFlagRequest) (*CreateFlagResponse, error) { flag := ToFlag(req.Body) diff --git a/internal/handler/routes.go b/internal/handler/routes.go new file mode 100644 index 0000000..8c9667c --- /dev/null +++ b/internal/handler/routes.go @@ -0,0 +1,25 @@ +package handler + +import ( + "net/http" + + "github.com/danielgtaylor/huma/v2" +) + +func (h *Handler) Register(api huma.API) { + huma.Register(api, huma.Operation{ + OperationID: "create-flag", + Method: http.MethodPost, + Path: "/flags", + Summary: "Create a new feature flag", + Tags: []string{"Flags"}, + }, h.CreateFlag) + + huma.Register(api, huma.Operation{ + OperationID: "evaluate-flag", + Method: http.MethodPost, + Path: "/flags/{key}/evaluate", + Summary: "Evaluate a feature flag", + Tags: []string{"Flags"}, + }, h.EvaluateFlag) +}