Skip to content

Commit 3b45729

Browse files
ericpromislowericpromislow
authored
Generate field names with brackets when needed. (#477)
* Generate field names with brackets when needed. * Stop hard-wiring complex selectors as `["field1", "field2[sub-field3]"]` and instead represent them as a more consistent `["field1", "field2", "sub-field3"]` * Convert all filter strings ending with square brackets to an array. Stop special-casing 'metadata.labels[X]' and handle any query string that ends with '[...]'. * Stop checking for pre-bracketed terms in constant field-accessor arrays. In this commit we stop converting string arrays like `["metadata", "labels[k8s.io/deepcode]"]` into the database field `"metadata.labels[k8s.io/deepcode]"` and instead will do a naive `join` to give `metadata[labels[k8s.io/deepcode]]`. The solution is to never express the above terms in separate fields, like `["metadata", "labels", "k8s.io/deepcode"]`. This also better reflects the stucture of the referenced object. * gofmt changes * Simplify comment about 'smartJoin'.
1 parent 527d44a commit 3b45729

9 files changed

Lines changed: 228 additions & 49 deletions

File tree

‎pkg/sqlcache/informer/listoption_indexer.go‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -742,9 +742,28 @@ func formatMatchTargetWithFormatter(match string, format string) string {
742742
return fmt.Sprintf(format, match)
743743
}
744744

745+
// There are two kinds of string arrays to turn into a string, based on the last value in the array
746+
// simple: ["a", "b", "conformsToIdentifier"] => "a.b.conformsToIdentifier"
747+
// complex: ["a", "b", "foo.io/stuff"] => "a.b[foo.io/stuff]"
748+
749+
func smartJoin(s []string) string {
750+
if len(s) == 0 {
751+
return ""
752+
}
753+
if len(s) == 1 {
754+
return s[0]
755+
}
756+
lastBit := s[len(s)-1]
757+
simpleName := regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`)
758+
if simpleName.MatchString(lastBit) {
759+
return strings.Join(s, ".")
760+
}
761+
return fmt.Sprintf("%s[%s]", strings.Join(s[0:len(s)-1], "."), lastBit)
762+
}
763+
745764
// toColumnName returns the column name corresponding to a field expressed as string slice
746765
func toColumnName(s []string) string {
747-
return db.Sanitize(strings.Join(s, "."))
766+
return db.Sanitize(smartJoin(s))
748767
}
749768

750769
// getField extracts the value of a field expressed as a string path from an unstructured object

‎pkg/sqlcache/informer/listoption_indexer_test.go‎

Lines changed: 86 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,7 @@ func TestListByOptions(t *testing.T) {
262262
expectedCountStmtArgs []any
263263
expectedStmt string
264264
expectedStmtArgs []any
265+
extraIndexedFields []string
265266
expectedList *unstructured.UnstructuredList
266267
returnList []any
267268
expectedContToken string
@@ -688,6 +689,27 @@ func TestListByOptions(t *testing.T) {
688689
expectedErr: nil,
689690
})
690691

692+
tests = append(tests, testCase{
693+
description: "ListByOptions sorting on two complex fields should sort on the first field in ascending order first and then sort on the second field in ascending order in prepared sql.Stmt",
694+
listOptions: ListOptions{
695+
Sort: Sort{
696+
Fields: [][]string{{"metadata", "fields", "3"}, {"metadata", "labels", "stub.io/candy"}},
697+
Orders: []SortOrder{ASC, ASC},
698+
},
699+
},
700+
extraIndexedFields: []string{"metadata.fields[3]", "metadata.labels[stub.io/candy]"},
701+
partitions: []partition.Partition{},
702+
ns: "",
703+
expectedStmt: `SELECT o.object, o.objectnonce, o.dekid FROM "something" o
704+
JOIN "something_fields" f ON o.key = f.key
705+
WHERE
706+
(FALSE)
707+
ORDER BY f."metadata.fields[3]" ASC, f."metadata.labels[stub.io/candy]" ASC`,
708+
returnList: []any{&unstructured.Unstructured{Object: unstrTestObjectMap}, &unstructured.Unstructured{Object: unstrTestObjectMap}},
709+
expectedList: &unstructured.UnstructuredList{Object: map[string]interface{}{"items": []map[string]interface{}{unstrTestObjectMap, unstrTestObjectMap}}, Items: []unstructured.Unstructured{{Object: unstrTestObjectMap}, {Object: unstrTestObjectMap}}},
710+
expectedContToken: "",
711+
expectedErr: nil,
712+
})
691713
tests = append(tests, testCase{
692714
description: "ListByOptions sorting on two fields should sort on the first field in ascending order first and then sort on the second field in ascending order in prepared sql.Stmt",
693715
listOptions: ListOptions{
@@ -909,8 +931,8 @@ func TestListByOptions(t *testing.T) {
909931
Indexer: i,
910932
indexedFields: []string{"metadata.somefield", "status.someotherfield"},
911933
}
912-
if test.description == "ListByOptions with multiple OrFilters set should select where all OrFilters contain one filter that is true in prepared sql.Stmt" {
913-
fmt.Printf("stop here")
934+
if len(test.extraIndexedFields) > 0 {
935+
lii.indexedFields = append(lii.indexedFields, test.extraIndexedFields...)
914936
}
915937
queryInfo, err := lii.constructQuery(test.listOptions, test.partitions, test.ns, "something")
916938
if test.expectedErr != nil {
@@ -1536,3 +1558,65 @@ func TestConstructQuery(t *testing.T) {
15361558
})
15371559
}
15381560
}
1561+
1562+
func TestSmartJoin(t *testing.T) {
1563+
type testCase struct {
1564+
description string
1565+
fieldArray []string
1566+
expectedFieldName string
1567+
}
1568+
1569+
var tests []testCase
1570+
tests = append(tests, testCase{
1571+
description: "single-letter names should be dotted",
1572+
fieldArray: []string{"metadata", "labels", "a"},
1573+
expectedFieldName: "metadata.labels.a",
1574+
})
1575+
tests = append(tests, testCase{
1576+
description: "underscore should be dotted",
1577+
fieldArray: []string{"metadata", "labels", "_"},
1578+
expectedFieldName: "metadata.labels._",
1579+
})
1580+
tests = append(tests, testCase{
1581+
description: "simple names should be dotted",
1582+
fieldArray: []string{"metadata", "labels", "queryField2"},
1583+
expectedFieldName: "metadata.labels.queryField2",
1584+
})
1585+
tests = append(tests, testCase{
1586+
description: "a numeric field should be bracketed",
1587+
fieldArray: []string{"metadata", "fields", "43"},
1588+
expectedFieldName: "metadata.fields[43]",
1589+
})
1590+
tests = append(tests, testCase{
1591+
description: "a field starting with a number should be bracketed",
1592+
fieldArray: []string{"metadata", "fields", "46days"},
1593+
expectedFieldName: "metadata.fields[46days]",
1594+
})
1595+
tests = append(tests, testCase{
1596+
description: "compound names should be bracketed",
1597+
fieldArray: []string{"metadata", "labels", "rancher.cattle.io/moo"},
1598+
expectedFieldName: "metadata.labels[rancher.cattle.io/moo]",
1599+
})
1600+
tests = append(tests, testCase{
1601+
description: "space-separated names should be bracketed",
1602+
fieldArray: []string{"metadata", "labels", "space here"},
1603+
expectedFieldName: "metadata.labels[space here]",
1604+
})
1605+
tests = append(tests, testCase{
1606+
description: "already-bracketed terms cause double-bracketing and should never be used",
1607+
fieldArray: []string{"metadata", "labels[k8s.io/deepcode]"},
1608+
expectedFieldName: "metadata[labels[k8s.io/deepcode]]",
1609+
})
1610+
tests = append(tests, testCase{
1611+
description: "an empty array should be an empty string",
1612+
fieldArray: []string{},
1613+
expectedFieldName: "",
1614+
})
1615+
t.Parallel()
1616+
for _, test := range tests {
1617+
t.Run(test.description, func(t *testing.T) {
1618+
gotFieldName := smartJoin(test.fieldArray)
1619+
assert.Equal(t, test.expectedFieldName, gotFieldName)
1620+
})
1621+
}
1622+
}

‎pkg/sqlcache/integration_test.go‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ func (i *IntegrationSuite) TearDownSuite() {
6161
}
6262

6363
func (i *IntegrationSuite) TestSQLCacheFilters() {
64-
fields := [][]string{{`metadata`, `annotations[somekey]`}}
64+
fields := [][]string{{"metadata", "annotations", "somekey"}}
6565
require := i.Require()
6666
configMapWithAnnotations := func(name string, annotations map[string]string) v1.ConfigMap {
6767
return v1.ConfigMap{
@@ -122,7 +122,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
122122
{
123123
name: "matches filter",
124124
filters: orFiltersForFilters(informer.Filter{
125-
Field: []string{`metadata`, `annotations[somekey]`},
125+
Field: []string{"metadata", "annotations", "somekey"},
126126
Matches: []string{"somevalue"},
127127
Op: informer.Eq,
128128
Partial: false,
@@ -132,7 +132,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
132132
{
133133
name: "partial matches filter",
134134
filters: orFiltersForFilters(informer.Filter{
135-
Field: []string{`metadata`, `annotations[somekey]`},
135+
Field: []string{"metadata", "annotations", "somekey"},
136136
Matches: []string{"somevalue"},
137137
Op: informer.Eq,
138138
Partial: true,
@@ -142,7 +142,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
142142
{
143143
name: "no matches for filter with underscore as it is interpreted literally",
144144
filters: orFiltersForFilters(informer.Filter{
145-
Field: []string{`metadata`, `annotations[somekey]`},
145+
Field: []string{"metadata", "annotations", "somekey"},
146146
Matches: []string{"somevalu_"},
147147
Op: informer.Eq,
148148
Partial: true,
@@ -152,7 +152,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
152152
{
153153
name: "no matches for filter with percent sign as it is interpreted literally",
154154
filters: orFiltersForFilters(informer.Filter{
155-
Field: []string{`metadata`, `annotations[somekey]`},
155+
Field: []string{"metadata", "annotations", "somekey"},
156156
Matches: []string{"somevalu%"},
157157
Op: informer.Eq,
158158
Partial: true,
@@ -162,7 +162,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
162162
{
163163
name: "match with special characters",
164164
filters: orFiltersForFilters(informer.Filter{
165-
Field: []string{`metadata`, `annotations[somekey]`},
165+
Field: []string{"metadata", "annotations", "somekey"},
166166
Matches: []string{"c%%l_value"},
167167
Op: informer.Eq,
168168
Partial: true,
@@ -172,7 +172,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
172172
{
173173
name: "match with literal backslash character",
174174
filters: orFiltersForFilters(informer.Filter{
175-
Field: []string{`metadata`, `annotations[somekey]`},
175+
Field: []string{"metadata", "annotations", "somekey"},
176176
Matches: []string{`my\windows\path`},
177177
Op: informer.Eq,
178178
Partial: true,
@@ -182,7 +182,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
182182
{
183183
name: "not eq filter",
184184
filters: orFiltersForFilters(informer.Filter{
185-
Field: []string{`metadata`, `annotations[somekey]`},
185+
Field: []string{"metadata", "annotations", "somekey"},
186186
Matches: []string{"somevalue"},
187187
Op: informer.NotEq,
188188
Partial: false,
@@ -192,7 +192,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
192192
{
193193
name: "partial not eq filter",
194194
filters: orFiltersForFilters(informer.Filter{
195-
Field: []string{`metadata`, `annotations[somekey]`},
195+
Field: []string{"metadata", "annotations", "somekey"},
196196
Matches: []string{"somevalue"},
197197
Op: informer.NotEq,
198198
Partial: true,
@@ -203,13 +203,13 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
203203
name: "multiple or filters match",
204204
filters: orFiltersForFilters(
205205
informer.Filter{
206-
Field: []string{`metadata`, `annotations[somekey]`},
206+
Field: []string{"metadata", "annotations", "somekey"},
207207
Matches: []string{"somevalue"},
208208
Op: informer.Eq,
209209
Partial: true,
210210
},
211211
informer.Filter{
212-
Field: []string{`metadata`, `annotations[somekey]`},
212+
Field: []string{"metadata", "annotations", "somekey"},
213213
Matches: []string{"notequal"},
214214
Op: informer.Eq,
215215
Partial: false,
@@ -221,7 +221,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
221221
name: "or filters on different fields",
222222
filters: orFiltersForFilters(
223223
informer.Filter{
224-
Field: []string{`metadata`, `annotations[somekey]`},
224+
Field: []string{"metadata", "annotations", "somekey"},
225225
Matches: []string{"somevalue"},
226226
Op: informer.Eq,
227227
Partial: true,
@@ -241,7 +241,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
241241
{
242242
Filters: []informer.Filter{
243243
{
244-
Field: []string{`metadata`, `annotations[somekey]`},
244+
Field: []string{"metadata", "annotations", "somekey"},
245245
Matches: []string{"somevalue"},
246246
Op: informer.Eq,
247247
Partial: true,
@@ -265,7 +265,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
265265
name: "no matches",
266266
filters: orFiltersForFilters(
267267
informer.Filter{
268-
Field: []string{`metadata`, `annotations[somekey]`},
268+
Field: []string{"metadata", "annotations", "somekey"},
269269
Matches: []string{"valueNotRepresented"},
270270
Op: informer.Eq,
271271
Partial: false,

‎pkg/stores/sqlpartition/listprocessor/processor.go‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ const (
3535
notOp = "!"
3636
)
3737

38-
var labelsRegex = regexp.MustCompile(`^(metadata)\.(labels)\[(.+)\]$`)
38+
var endsWithBracket = regexp.MustCompile(`^(.+)\[(.+)]$`)
3939
var mapK8sOpToRancherOp = map[selection.Operator]informer.Op{
4040
selection.Equals: informer.Eq,
4141
selection.DoubleEquals: informer.Eq,
@@ -191,17 +191,16 @@ func getLimit(apiOp *types.APIRequest) int {
191191
return limit
192192
}
193193

194-
// splitQuery takes a single-string metadata-labels filter and converts it into an array of 3 accessor strings,
195-
// where the first two strings are always "metadata" and "labels", and the third is the label name.
196-
// This is more complex than doing something like `strings.Split(".", "metadata.labels.fieldName")
197-
// because the fieldName can be more complex - in particular it can contain "."s) and needs to be
198-
// bracketed, as in `metadata.labels[rancher.io/cattle.and.beef]".
199-
// The `labelsRegex` looks for the bracketed form.
194+
// splitQuery takes a single-string k8s object accessor and returns its separate fields in a slice.
195+
// "Simple" accessors of the form `metadata.labels.foo` => ["metadata", "labels", "foo"]
196+
// but accessors with square brackets need to be broken on the brackets, as in
197+
// "metadata.annotations[k8s.io/this-is-fun]" => ["metadata", "annotations", "k8s.io/this-is-fun"]
198+
// We assume in the kubernetes/rancher world json keys are always alphanumeric-underscorish, so
199+
// we only look for square brackets at the end of the string.
200200
func splitQuery(query string) []string {
201-
m := labelsRegex.FindStringSubmatch(query)
202-
if m != nil && len(m) == 4 {
203-
// m[0] contains the entire string, so just return all but that first item in `m`
204-
return m[1:]
201+
m := endsWithBracket.FindStringSubmatch(query)
202+
if m != nil {
203+
return append(strings.Split(m[1], "."), m[2])
205204
}
206205
return strings.Split(query, ".")
207206
}
@@ -219,7 +218,7 @@ func parseNamespaceOrProjectFilters(ctx context.Context, projOrNS string, op inf
219218
Op: informer.Eq,
220219
},
221220
{
222-
Field: []string{"metadata", "labels[field.cattle.io/projectId]"},
221+
Field: []string{"metadata", "labels", "field.cattle.io/projectId"},
223222
Matches: []string{pn},
224223
Op: informer.Eq,
225224
},

0 commit comments

Comments
 (0)