Skip to content

Commit 421053c

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

File tree

9 files changed

+169
-54
lines changed

9 files changed

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

‎core/pkg/store/store.go

Lines changed: 19 additions & 13 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
160160
if selector != nil && !selector.IsEmpty() {
161-
queryMeta = selector.SelectorToMetadata()
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)
352358
if selector != nil && !selector.IsEmpty() {
353-
indexId, constraints := selector.SelectorMapToQuery()
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)