Skip to content

Commit c12a0ae

Browse files
alexandraoberaigneralexandraoberaigner
authored
feat: allow null flagSetId Selector, restrict Selector to single key-value-pairs (#1708) (#1811)
## This PR This pull request updates the `Selector` logic in the flag store to allow selecting flags without a flagSetId (`flagSetId=`). It also restricts selector expressions to a single key-value pair, and updates / skips related tests. This is a temporary limitation pending a decision on the selector syntax (#1708). ### Related Issues <!-- add here the GitHub issue that this PR resolves if applicable --> #1708 ### Follow-up Tasks TODO: we should decide on the selector syntax ### How to test * run `make run-flagd-selector-demo` * experiment with queries & flag configurations with/without flagSetIds (be aware that the top level flagSetId is setting the flagSetId for all flags without already exisiting flagSetId): ``` grpcurl -d '{"selector":"flagSetId=example"}' -import-path schemas/protobuf/flagd/sync/v1/ -proto sync.proto -plaintext localhost:8015 flagd.sync.v1.FlagSyncService/SyncFlags | jq grpcurl -d '{"selector":"flagSetId="}' -import-path schemas/protobuf/flagd/sync/v1/ -proto sync.proto -plaintext localhost:8015 flagd.sync.v1.FlagSyncService/SyncFlags | jq ``` Signed-off-by: Alexandra Oberaigner <[email protected]>
1 parent 5f291f8 commit c12a0ae

File tree

3 files changed

+95
-57
lines changed

3 files changed

+95
-57
lines changed

‎core/pkg/store/query.go‎

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -27,14 +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.
30+
// A Selector represents a set of constraints used to query the store.
3131
type Selector struct {
3232
indexMap map[string]string
3333
}
3434

3535
// NewSelector creates a new Selector from a selector expression string.
36-
// For example, to select flags from source "./mySource" and flagSetId "1234", use the expression:
37-
// "source=./mySource,flagSetId=1234"
36+
// #1708 Until we decide on the Selector syntax, only a single key=value pair is supported
37+
// For example, to select flags from source "./mySource" or flagSetId "1234", use the expressions:
38+
// "source=./mySource" or "flagSetId=1234"
3839
func NewSelector(selectorExpression string) Selector {
3940
return Selector{
4041
indexMap: expressionToMap(selectorExpression),
@@ -47,27 +48,28 @@ func expressionToMap(sExp string) map[string]string {
4748
return selectorMap
4849
}
4950

50-
if strings.Index(sExp, "=") == -1 {
51-
// if no '=' is found, treat the whole string as as source (backwards compatibility)
52-
// we may may support interpreting this as a flagSetId in the future as an option
51+
delimiterIdx := strings.Index(sExp, "=")
52+
if delimiterIdx == -1 {
53+
// if no '=' is found, treat the whole string as source (backwards compatibility)
54+
// we may support interpreting this as a flagSetId in the future as an option
5355
selectorMap[sourceIndex] = sExp
5456
return selectorMap
5557
}
5658

57-
// Split the selector by commas
58-
pairs := strings.Split(sExp, ",")
59-
for _, pair := range pairs {
60-
// Split each pair by the first equal sign
61-
parts := strings.Split(pair, "=")
62-
if len(parts) == 2 {
63-
key := parts[0]
64-
value := parts[1]
65-
selectorMap[key] = value
66-
}
59+
// split the selector by the first equal sign
60+
key := sExp[:delimiterIdx]
61+
value := sExp[delimiterIdx+1:]
62+
63+
// handle empty flagSetId as nilFlagSetId to query all flags without flagSetId
64+
if key == "flagSetId" && value == "" {
65+
value = nilFlagSetId
6766
}
67+
selectorMap[key] = value
68+
6869
return selectorMap
6970
}
7071

72+
// WithIndex creates a new Selector from the current Selector and adds the given key-value-pair
7173
func (s Selector) WithIndex(key string, value string) Selector {
7274
m := maps.Clone(s.indexMap)
7375
m[key] = value
@@ -80,7 +82,7 @@ func (s *Selector) IsEmpty() bool {
8082
return s == nil || len(s.indexMap) == 0
8183
}
8284

83-
// SelectorMapToQuery converts the selector map to an indexId and constraints for querying the store.
85+
// ToQuery converts the Selector map to an indexId and constraints for querying the Store.
8486
// For a given index, a specific order and number of constraints are required.
8587
// Both the indexId and constraints are generated based on the keys present in the selector's internal map.
8688
func (s Selector) ToQuery() (indexId string, constraints []interface{}) {
@@ -114,7 +116,7 @@ func (s Selector) ToQuery() (indexId string, constraints []interface{}) {
114116
return indexId, constraints
115117
}
116118

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

‎core/pkg/store/query_test.go‎

Lines changed: 52 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ func TestSelector_IsEmpty(t *testing.T) {
3333
selector: &Selector{indexMap: map[string]string{"source": "abc"}},
3434
wantEmpty: false,
3535
},
36+
{
37+
name: "non-empty indexMap, empty value",
38+
selector: &Selector{indexMap: map[string]string{"flagSetId": ""}},
39+
wantEmpty: false,
40+
},
3641
}
3742

3843
for _, tt := range tests {
@@ -68,24 +73,33 @@ func TestSelector_ToQuery(t *testing.T) {
6873
wantIndex string
6974
wantConstr []interface{}
7075
}{
71-
{
72-
name: "flagSetId and key primary index special case",
73-
selector: Selector{indexMap: map[string]string{"flagSetId": "fsid", "key": "myKey"}},
74-
wantIndex: "id",
75-
wantConstr: []interface{}{"fsid", "myKey"},
76-
},
77-
{
78-
name: "multiple keys sorted",
79-
selector: Selector{indexMap: map[string]string{"source": "src", "flagSetId": "fsid"}},
80-
wantIndex: "flagSetId+source",
81-
wantConstr: []interface{}{"fsid", "src"},
82-
},
76+
// #1708 Until we decide on the Selector syntax, only a single key=value pair is supported
77+
/*
78+
{
79+
name: "flagSetId and key primary index special case",
80+
selector: Selector{indexMap: map[string]string{"flagSetId": "fsid", "key": "myKey"}},
81+
wantIndex: "id",
82+
wantConstr: []interface{}{"fsid", "myKey"},
83+
},
84+
{
85+
name: "multiple keys sorted",
86+
selector: Selector{indexMap: map[string]string{"source": "src", "flagSetId": "fsid"}},
87+
wantIndex: "flagSetId+source",
88+
wantConstr: []interface{}{"fsid", "src"},
89+
},
90+
*/
8391
{
8492
name: "single key",
8593
selector: Selector{indexMap: map[string]string{"source": "src"}},
8694
wantIndex: "source",
8795
wantConstr: []interface{}{"src"},
8896
},
97+
{
98+
name: "flagSetId null",
99+
selector: Selector{indexMap: map[string]string{"flagSetId": ""}},
100+
wantIndex: "flagSetId",
101+
wantConstr: []interface{}{""},
102+
},
89103
}
90104

91105
for _, tt := range tests {
@@ -132,16 +146,19 @@ func TestSelector_ToMetadata(t *testing.T) {
132146
selector: &Selector{indexMap: map[string]string{"source": "src"}},
133147
want: model.Metadata{"source": "src"},
134148
},
135-
{
136-
name: "flagSetId and source",
137-
selector: &Selector{indexMap: map[string]string{"flagSetId": "fsid", "source": "src"}},
138-
want: model.Metadata{"flagSetId": "fsid", "source": "src"},
139-
},
140-
{
141-
name: "flagSetId, source, and key (key should be ignored)",
142-
selector: &Selector{indexMap: map[string]string{"flagSetId": "fsid", "source": "src", "key": "myKey"}},
143-
want: model.Metadata{"flagSetId": "fsid", "source": "src"},
144-
},
149+
// #1708 Until we decide on the Selector syntax, only a single key=value pair is supported
150+
/*
151+
{
152+
name: "flagSetId and source",
153+
selector: &Selector{indexMap: map[string]string{"flagSetId": "fsid", "source": "src"}},
154+
want: model.Metadata{"flagSetId": "fsid", "source": "src"},
155+
},
156+
{
157+
name: "flagSetId, source, and key (key should be ignored)",
158+
selector: &Selector{indexMap: map[string]string{"flagSetId": "fsid", "source": "src", "key": "myKey"}},
159+
want: model.Metadata{"flagSetId": "fsid", "source": "src"},
160+
},
161+
*/
145162
}
146163

147164
for _, tt := range tests {
@@ -160,11 +177,14 @@ func TestNewSelector(t *testing.T) {
160177
input string
161178
wantMap map[string]string
162179
}{
163-
{
164-
name: "source and flagSetId",
165-
input: "source=abc,flagSetId=1234",
166-
wantMap: map[string]string{"source": "abc", "flagSetId": "1234"},
167-
},
180+
// #1708 Until we decide on the Selector syntax, only a single key=value pair is supported
181+
/*
182+
{
183+
name: "source and flagSetId",
184+
input: "source=abc,flagSetId=1234",
185+
wantMap: map[string]string{"source": "abc", "flagSetId": "1234"},
186+
},
187+
*/
168188
{
169189
name: "source",
170190
input: "source=abc",
@@ -175,6 +195,11 @@ func TestNewSelector(t *testing.T) {
175195
input: "mysource",
176196
wantMap: map[string]string{"source": "mysource"},
177197
},
198+
{
199+
name: "null flagSetId",
200+
input: "flagSetId=",
201+
wantMap: map[string]string{"flagSetId": nilFlagSetId},
202+
},
178203
{
179204
name: "empty string",
180205
input: "",

‎core/pkg/store/store_test.go‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,8 @@ func TestGetAllNoWatcher(t *testing.T) {
379379

380380
sourceASelector := NewSelector("source=" + sourceA.Name)
381381
flagSetIdCSelector := NewSelector("flagSetId=" + flagSetIdC)
382-
flagSetIdAndCSelector := NewSelector("flagSetId=" + flagSetIdC + ",source=" + sourceC.Name)
382+
// #1708 Until we decide on the Selector syntax, only a single key=value pair is supported
383+
//flagSetIdAndCSelector := NewSelector("flagSetId=" + flagSetIdC + ",source=" + sourceC.Name)
383384

384385
t.Parallel()
385386
tests := []struct {
@@ -420,14 +421,17 @@ func TestGetAllNoWatcher(t *testing.T) {
420421
{Key: "dupe", DefaultVariant: "off", Source: sourceC.Name, FlagSetId: flagSetIdC, Priority: 2, Metadata: model.Metadata{"flagSetId": flagSetIdC}},
421422
},
422423
},
423-
{
424-
name: "flagSetId and source selector",
425-
selector: &flagSetIdAndCSelector,
426-
wantFlags: []model.Flag{
427-
{Key: "dupeMultiSource", DefaultVariant: "both", Source: sourceC.Name, FlagSetId: flagSetIdC, Metadata: model.Metadata{"flagSetId": flagSetIdC}, Priority: 2},
428-
{Key: "dupe", DefaultVariant: "off", Source: sourceC.Name, FlagSetId: flagSetIdC, Priority: 2, Metadata: model.Metadata{"flagSetId": flagSetIdC}},
424+
// #1708 Until we decide on the Selector syntax, only a single key=value pair is supported
425+
/*
426+
{
427+
name: "flagSetId and source selector",
428+
selector: &flagSetIdAndCSelector,
429+
wantFlags: []model.Flag{
430+
{Key: "dupeMultiSource", DefaultVariant: "both", Source: sourceC.Name, FlagSetId: flagSetIdC, Metadata: model.Metadata{"flagSetId": flagSetIdC}, Priority: 2},
431+
{Key: "dupe", DefaultVariant: "off", Source: sourceC.Name, FlagSetId: flagSetIdC, Priority: 2, Metadata: model.Metadata{"flagSetId": flagSetIdC}},
432+
},
429433
},
430-
},
434+
*/
431435
}
432436

433437
sourceOrder := []struct {
@@ -633,11 +637,18 @@ func TestQueryMetadata(t *testing.T) {
633637
// setup initial flags
634638
store.Update(sourceA, sourceAFlags, model.Metadata{})
635639

636-
selector := NewSelector("source=" + otherSource + ",flagSetId=" + nonExistingFlagSetId)
640+
// #1708 Until we decide on the Selector syntax, only a single key=value pair is supported
641+
// these tests should then also cover more complex selectors
642+
643+
selector := NewSelector("flagSetId=" + nonExistingFlagSetId)
637644
_, metadata, _ := store.GetAll(context.Background(), &selector)
638-
assert.Equal(t, metadata, model.Metadata{"source": otherSource, "flagSetId": nonExistingFlagSetId}, "metadata did not match expected")
645+
assert.Equal(t, metadata, model.Metadata{"flagSetId": nonExistingFlagSetId}, "metadata did not match expected")
646+
647+
selector = NewSelector("flagSetId=" + nonExistingFlagSetId)
648+
_, metadata, _ = store.Get(context.Background(), "key", &selector)
649+
assert.Equal(t, metadata, model.Metadata{"flagSetId": nonExistingFlagSetId}, "metadata did not match expected")
639650

640-
selector = NewSelector("source=" + otherSource + ",flagSetId=" + nonExistingFlagSetId)
651+
selector = NewSelector("source=" + otherSource)
641652
_, metadata, _ = store.Get(context.Background(), "key", &selector)
642-
assert.Equal(t, metadata, model.Metadata{"source": otherSource, "flagSetId": nonExistingFlagSetId}, "metadata did not match expected")
653+
assert.Equal(t, metadata, model.Metadata{"source": otherSource}, "metadata did not match expected")
643654
}

0 commit comments

Comments
 (0)