Skip to content

Commit 56b743e

Browse files
toddbaerttoddbaert
committed
fixup: more feedback
Signed-off-by: Todd Baert <[email protected]>
1 parent d3cbd27 commit 56b743e

File tree

9 files changed

+176
-56
lines changed

9 files changed

+176
-56
lines changed

‎core/pkg/evaluator/json.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ func (je *Resolver) ResolveAllValues(ctx context.Context, reqID string, context
156156
} else {
157157
selector = store.NewSelector("")
158158
}
159-
allFlags, flagSetMetadata, err := je.store.GetAll(ctx, selector)
159+
allFlags, flagSetMetadata, err := je.store.GetAll(ctx, &selector)
160160
if err != nil {
161161
return nil, flagSetMetadata, fmt.Errorf("error retreiving flags from the store: %w", err)
162162
}
@@ -318,7 +318,7 @@ func (je *Resolver) evaluateVariant(ctx context.Context, reqID string, flagKey s
318318
if s != nil {
319319
selector = s.(store.Selector)
320320
}
321-
flag, metadata, err := je.store.Get(ctx, flagKey, selector)
321+
flag, metadata, err := je.store.Get(ctx, flagKey, &selector)
322322
if err != nil {
323323
// flag not found
324324
je.Logger.DebugWithID(reqID, fmt.Sprintf("requested flag could not be found: %s", flagKey))

‎core/pkg/store/query.go

Lines changed: 8 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -27,25 +27,15 @@ const flagSetIdKeySourceCompoundIndex = flagSetIdIndex + "+" + keyIndex + "+" +
2727
// any flag without a "flagSetId" is assigned this one; it's never exposed externally
2828
var nilFlagSetId = uuid.New().String()
2929

30-
// A selector represents a set of constraints used to query the store.
31-
type Selector interface {
32-
SelectorMapToQuery() (indexId string, constraints []interface{})
33-
SelectorToMetadata() model.Metadata
34-
IsEmpty() bool
35-
WithIndex(key string, value string) Selector
36-
}
37-
38-
var _ Selector = (*selector)(nil)
39-
40-
type selector struct {
30+
type Selector struct {
4131
indexMap map[string]string
4232
}
4333

4434
// NewSelector creates a new Selector from a selector expression string.
4535
// For example, to select flags from source "./mySource" and flagSetId "1234", use the expression:
4636
// "source=./mySource,flagSetId=1234"
4737
func NewSelector(selectorExpression string) Selector {
48-
return selector{
38+
return Selector{
4939
indexMap: expressionToMap(selectorExpression),
5040
}
5141
}
@@ -70,22 +60,22 @@ func expressionToMap(selector string) map[string]string {
7060
return selectorMap
7161
}
7262

73-
func (s selector) WithIndex(key string, value string) Selector {
63+
func (s Selector) WithIndex(key string, value string) Selector {
7464
m := maps.Clone(s.indexMap)
7565
m[key] = value
76-
return selector{
66+
return Selector{
7767
indexMap: m,
7868
}
7969
}
8070

81-
func (s selector) IsEmpty() bool {
82-
return len(s.indexMap) == 0
71+
func (s *Selector) IsEmpty() bool {
72+
return s == nil || len(s.indexMap) == 0
8373
}
8474

8575
// SelectorMapToQuery converts the selector map to an indexId and constraints for querying the store.
8676
// For a given index, a specific order and number of constraints are required.
8777
// Both the indexId and constraints are generated based on the keys present in the selector's internal map.
88-
func (s selector) SelectorMapToQuery() (indexId string, constraints []interface{}) {
78+
func (s Selector) ToQuery() (indexId string, constraints []interface{}) {
8979

9080
if len(s.indexMap) == 2 && s.indexMap[flagSetIdIndex] != "" && s.indexMap[keyIndex] != "" {
9181
// special case for flagSetId and key (this is the "id" index)
@@ -118,7 +108,7 @@ func (s selector) SelectorMapToQuery() (indexId string, constraints []interface{
118108

119109
// SelectorToMetadata converts the selector's internal map to metadata for logging or tracing purposes.
120110
// Only includes known indices to avoid leaking sensitive information, and is usually returned as the "top level" metadata
121-
func (s selector) SelectorToMetadata() model.Metadata {
111+
func (s Selector) ToMetadata() model.Metadata {
122112
meta := model.Metadata{}
123113

124114
if s.indexMap[flagSetIdIndex] != "" {

‎core/pkg/store/query_test.go

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
package store
2+
3+
import (
4+
"reflect"
5+
"testing"
6+
)
7+
8+
func TestSelector_IsEmpty(t *testing.T) {
9+
tests := []struct {
10+
name string
11+
selector *Selector
12+
wantEmpty bool
13+
}{
14+
{
15+
name: "nil selector",
16+
selector: nil,
17+
wantEmpty: true,
18+
},
19+
{
20+
name: "nil indexMap",
21+
selector: &Selector{indexMap: nil},
22+
wantEmpty: true,
23+
},
24+
{
25+
name: "empty indexMap",
26+
selector: &Selector{indexMap: map[string]string{}},
27+
wantEmpty: true,
28+
},
29+
{
30+
name: "non-empty indexMap",
31+
selector: &Selector{indexMap: map[string]string{"source": "abc"}},
32+
wantEmpty: false,
33+
},
34+
}
35+
36+
for _, tt := range tests {
37+
t.Run(tt.name, func(t *testing.T) {
38+
got := tt.selector.IsEmpty()
39+
if got != tt.wantEmpty {
40+
t.Errorf("IsEmpty() = %v, want %v", got, tt.wantEmpty)
41+
}
42+
})
43+
}
44+
}
45+
46+
func TestSelector_WithIndex(t *testing.T) {
47+
oldS := Selector{indexMap: map[string]string{"source": "abc"}}
48+
newS := oldS.WithIndex("flagSetId", "1234")
49+
50+
if newS.indexMap["source"] != "abc" {
51+
t.Errorf("WithIndex did not preserve existing keys")
52+
}
53+
if newS.indexMap["flagSetId"] != "1234" {
54+
t.Errorf("WithIndex did not add new key")
55+
}
56+
// Ensure original is unchanged
57+
if _, ok := oldS.indexMap["flagSetId"]; ok {
58+
t.Errorf("WithIndex mutated original selector")
59+
}
60+
}
61+
62+
func TestSelector_ToQuery(t *testing.T) {
63+
tests := []struct {
64+
name string
65+
selector Selector
66+
wantIndex string
67+
wantConstr []interface{}
68+
}{
69+
{
70+
name: "flagSetId and key primary index special case",
71+
selector: Selector{indexMap: map[string]string{"flagSetId": "fsid", "key": "myKey"}},
72+
wantIndex: "id",
73+
wantConstr: []interface{}{"fsid", "myKey"},
74+
},
75+
{
76+
name: "multiple keys sorted",
77+
selector: Selector{indexMap: map[string]string{"source": "src", "flagSetId": "fsid"}},
78+
wantIndex: "flagSetId+source",
79+
wantConstr: []interface{}{"fsid", "src"},
80+
},
81+
{
82+
name: "single key",
83+
selector: Selector{indexMap: map[string]string{"source": "src"}},
84+
wantIndex: "source",
85+
wantConstr: []interface{}{"src"},
86+
},
87+
}
88+
89+
for _, tt := range tests {
90+
t.Run(tt.name, func(t *testing.T) {
91+
gotIndex, gotConstr := tt.selector.ToQuery()
92+
if gotIndex != tt.wantIndex {
93+
t.Errorf("ToQuery() index = %v, want %v", gotIndex, tt.wantIndex)
94+
}
95+
if !reflect.DeepEqual(gotConstr, tt.wantConstr) {
96+
t.Errorf("ToQuery() constraints = %v, want %v", gotConstr, tt.wantConstr)
97+
}
98+
})
99+
}
100+
}
101+
102+
func TestSelector_ToMetadata(t *testing.T) {
103+
s := Selector{indexMap: map[string]string{"flagSetId": "fsid", "source": "src", "key": "myKey"}}
104+
meta := s.ToMetadata()
105+
if meta["flagSetId"] != "fsid" {
106+
t.Errorf("ToMetadata missing flagSetId")
107+
}
108+
if meta["source"] != "src" {
109+
t.Errorf("ToMetadata missing source")
110+
}
111+
if _, ok := meta["key"]; ok {
112+
t.Errorf("ToMetadata should not include key")
113+
}
114+
}
115+
116+
func TestNewSelector(t *testing.T) {
117+
s := NewSelector("source=abc,flagSetId=1234")
118+
if s.indexMap["source"] != "abc" {
119+
t.Errorf("NewSelector did not parse source")
120+
}
121+
if s.indexMap["flagSetId"] != "1234" {
122+
t.Errorf("NewSelector did not parse flagSetId")
123+
}
124+
}

‎core/pkg/store/store.go

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,9 @@ type FlagQueryResult struct {
2222
}
2323

2424
type IStore interface {
25-
Get(ctx context.Context, key string, selector Selector) (model.Flag, model.Metadata, error)
26-
GetAll(ctx context.Context, selector Selector) (map[string]model.Flag, model.Metadata, error)
27-
Watch(ctx context.Context, selector Selector, watcher chan FlagQueryResult)
25+
Get(ctx context.Context, key string, selector *Selector) (model.Flag, model.Metadata, error)
26+
GetAll(ctx context.Context, selector *Selector) (map[string]model.Flag, model.Metadata, error)
27+
Watch(ctx context.Context, selector *Selector, watcher chan<- FlagQueryResult)
2828
}
2929

3030
var _ IStore = (*Store)(nil)
@@ -151,16 +151,16 @@ func NewFlags() *Store {
151151
return state
152152
}
153153

154-
func (s *Store) Get(_ context.Context, key string, selector Selector) (model.Flag, model.Metadata, error) {
154+
func (s *Store) Get(_ context.Context, key string, selector *Selector) (model.Flag, model.Metadata, error) {
155155
s.logger.Debug(fmt.Sprintf("getting flag %s", key))
156156
txn := s.db.Txn(false)
157157
queryMeta := model.Metadata{}
158158

159159
// if present, use the selector to query the flags
160-
if selector != nil && !selector.IsEmpty() {
161-
queryMeta = selector.SelectorToMetadata()
160+
if !selector.IsEmpty() {
161+
queryMeta = selector.ToMetadata()
162162
selector := selector.WithIndex("key", key)
163-
indexId, constraints := selector.SelectorMapToQuery()
163+
indexId, constraints := selector.ToQuery()
164164
s.logger.Debug(fmt.Sprintf("getting flag with query: %s, %v", indexId, constraints))
165165
raw, err := txn.First(flagsTable, indexId, constraints...)
166166
flag, ok := raw.(model.Flag)
@@ -216,11 +216,11 @@ func (f *Store) String() (string, error) {
216216
}
217217

218218
// GetAll returns a copy of the store's state (copy in order to be concurrency safe)
219-
func (s *Store) GetAll(ctx context.Context, selector Selector) (map[string]model.Flag, model.Metadata, error) {
219+
func (s *Store) GetAll(ctx context.Context, selector *Selector) (map[string]model.Flag, model.Metadata, error) {
220220
flags := make(map[string]model.Flag)
221221
queryMeta := model.Metadata{}
222222
if selector != nil && !selector.IsEmpty() {
223-
queryMeta = selector.SelectorToMetadata()
223+
queryMeta = selector.ToMetadata()
224224
}
225225
it, err := s.selectOrAll(selector)
226226

@@ -259,7 +259,7 @@ func (s *Store) Update(
259259

260260
// get all flags for the source we are updating
261261
selector := NewSelector(sourceIndex + "=" + source)
262-
oldFlags, _, _ := s.GetAll(context.Background(), selector)
262+
oldFlags, _, _ := s.GetAll(context.Background(), &selector)
263263

264264
s.mx.Lock()
265265
for key := range oldFlags {
@@ -326,11 +326,16 @@ func (s *Store) Update(
326326
}
327327

328328
// Watch the result-set of a selector for changes, sending updates to the watcher channel.
329-
func (s *Store) Watch(ctx context.Context, selector Selector, watcher chan FlagQueryResult) {
329+
func (s *Store) Watch(ctx context.Context, selector *Selector, watcher chan<- FlagQueryResult) {
330330
go func() {
331331
for {
332332
ws := memdb.NewWatchSet()
333333
it, err := s.selectOrAll(selector)
334+
if err != nil {
335+
s.logger.Error(fmt.Sprintf("error watching flags: %v", err))
336+
close(watcher)
337+
return
338+
}
334339
ws.Add(it.WatchCh())
335340

336341
flags := s.collect(it)
@@ -339,18 +344,19 @@ func (s *Store) Watch(ctx context.Context, selector Selector, watcher chan FlagQ
339344
}
340345

341346
if err = ws.WatchCtx(ctx); err != nil {
347+
s.logger.Error(fmt.Sprintf("error watching flags: %v", err))
342348
close(watcher)
343-
return // cancelled or deadline
349+
return
344350
}
345351
}
346352
}()
347353
}
348354

349355
// returns an iterator for the given selector, or all flags if the selector is nil or empty
350-
func (s *Store) selectOrAll(selector Selector) (it memdb.ResultIterator, err error) {
356+
func (s *Store) selectOrAll(selector *Selector) (it memdb.ResultIterator, err error) {
351357
txn := s.db.Txn(false)
352-
if selector != nil && !selector.IsEmpty() {
353-
indexId, constraints := selector.SelectorMapToQuery()
358+
if !selector.IsEmpty() {
359+
indexId, constraints := selector.ToQuery()
354360
s.logger.Debug(fmt.Sprintf("getting all flags with query: %s, %v", indexId, constraints))
355361
return txn.Get(flagsTable, indexId, constraints...)
356362
} else {

0 commit comments

Comments
 (0)