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'.

pkg/sqlcache/informer/listoption_indexer.go

@@ -742,9 +742,28 @@ func formatMatchTargetWithFormatter(match string, format string) string {
 	return fmt.Sprintf(format, match)
 }
 
+// There are two kinds of string arrays to turn into a string, based on the last value in the array
+// simple: ["a", "b", "conformsToIdentifier"] => "a.b.conformsToIdentifier"
+// complex: ["a", "b", "foo.io/stuff"] => "a.b[foo.io/stuff]"
+
+func smartJoin(s []string) string {
+	if len(s) == 0 {
+		return ""
+	}
+	if len(s) == 1 {
+		return s[0]
+	}
+	lastBit := s[len(s)-1]
+	simpleName := regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`)
+	if simpleName.MatchString(lastBit) {
+		return strings.Join(s, ".")
+	}
+	return fmt.Sprintf("%s[%s]", strings.Join(s[0:len(s)-1], "."), lastBit)
+}
+
 // toColumnName returns the column name corresponding to a field expressed as string slice
 func toColumnName(s []string) string {
-	return db.Sanitize(strings.Join(s, "."))
+	return db.Sanitize(smartJoin(s))
 }
 
 // getField extracts the value of a field expressed as a string path from an unstructured object

pkg/sqlcache/informer/listoption_indexer_test.go

@@ -262,6 +262,7 @@ func TestListByOptions(t *testing.T) {
 		expectedCountStmtArgs []any
 		expectedStmt          string
 		expectedStmtArgs      []any
+		extraIndexedFields    []string
 		expectedList          *unstructured.UnstructuredList
 		returnList            []any
 		expectedContToken     string
@@ -688,6 +689,27 @@ func TestListByOptions(t *testing.T) {
 		expectedErr:       nil,
 	})
 
+	tests = append(tests, testCase{
+		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",
+		listOptions: ListOptions{
+			Sort: Sort{
+				Fields: [][]string{{"metadata", "fields", "3"}, {"metadata", "labels", "stub.io/candy"}},
+				Orders: []SortOrder{ASC, ASC},
+			},
+		},
+		extraIndexedFields: []string{"metadata.fields[3]", "metadata.labels[stub.io/candy]"},
+		partitions:         []partition.Partition{},
+		ns:                 "",
+		expectedStmt: `SELECT o.object, o.objectnonce, o.dekid FROM "something" o
+  JOIN "something_fields" f ON o.key = f.key
+  WHERE
+    (FALSE)
+  ORDER BY f."metadata.fields[3]" ASC, f."metadata.labels[stub.io/candy]" ASC`,
+		returnList:        []any{&unstructured.Unstructured{Object: unstrTestObjectMap}, &unstructured.Unstructured{Object: unstrTestObjectMap}},
+		expectedList:      &unstructured.UnstructuredList{Object: map[string]interface{}{"items": []map[string]interface{}{unstrTestObjectMap, unstrTestObjectMap}}, Items: []unstructured.Unstructured{{Object: unstrTestObjectMap}, {Object: unstrTestObjectMap}}},
+		expectedContToken: "",
+		expectedErr:       nil,
+	})
 	tests = append(tests, testCase{
 		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",
 		listOptions: ListOptions{
@@ -909,8 +931,8 @@ func TestListByOptions(t *testing.T) {
 				Indexer:       i,
 				indexedFields: []string{"metadata.somefield", "status.someotherfield"},
 			}
-			if test.description == "ListByOptions with multiple OrFilters set should select where all OrFilters contain one filter that is true in prepared sql.Stmt" {
-				fmt.Printf("stop here")
+			if len(test.extraIndexedFields) > 0 {
+				lii.indexedFields = append(lii.indexedFields, test.extraIndexedFields...)
 			}
 			queryInfo, err := lii.constructQuery(test.listOptions, test.partitions, test.ns, "something")
 			if test.expectedErr != nil {
@@ -1536,3 +1558,65 @@ func TestConstructQuery(t *testing.T) {
 		})
 	}
 }
+
+func TestSmartJoin(t *testing.T) {
+	type testCase struct {
+		description       string
+		fieldArray        []string
+		expectedFieldName string
+	}
+
+	var tests []testCase
+	tests = append(tests, testCase{
+		description:       "single-letter names should be dotted",
+		fieldArray:        []string{"metadata", "labels", "a"},
+		expectedFieldName: "metadata.labels.a",
+	})
+	tests = append(tests, testCase{
+		description:       "underscore should be dotted",
+		fieldArray:        []string{"metadata", "labels", "_"},
+		expectedFieldName: "metadata.labels._",
+	})
+	tests = append(tests, testCase{
+		description:       "simple names should be dotted",
+		fieldArray:        []string{"metadata", "labels", "queryField2"},
+		expectedFieldName: "metadata.labels.queryField2",
+	})
+	tests = append(tests, testCase{
+		description:       "a numeric field should be bracketed",
+		fieldArray:        []string{"metadata", "fields", "43"},
+		expectedFieldName: "metadata.fields[43]",
+	})
+	tests = append(tests, testCase{
+		description:       "a field starting with a number should be bracketed",
+		fieldArray:        []string{"metadata", "fields", "46days"},
+		expectedFieldName: "metadata.fields[46days]",
+	})
+	tests = append(tests, testCase{
+		description:       "compound names should be bracketed",
+		fieldArray:        []string{"metadata", "labels", "rancher.cattle.io/moo"},
+		expectedFieldName: "metadata.labels[rancher.cattle.io/moo]",
+	})
+	tests = append(tests, testCase{
+		description:       "space-separated names should be bracketed",
+		fieldArray:        []string{"metadata", "labels", "space here"},
+		expectedFieldName: "metadata.labels[space here]",
+	})
+	tests = append(tests, testCase{
+		description:       "already-bracketed terms cause double-bracketing and should never be used",
+		fieldArray:        []string{"metadata", "labels[k8s.io/deepcode]"},
+		expectedFieldName: "metadata[labels[k8s.io/deepcode]]",
+	})
+	tests = append(tests, testCase{
+		description:       "an empty array should be an empty string",
+		fieldArray:        []string{},
+		expectedFieldName: "",
+	})
+	t.Parallel()
+	for _, test := range tests {
+		t.Run(test.description, func(t *testing.T) {
+			gotFieldName := smartJoin(test.fieldArray)
+			assert.Equal(t, test.expectedFieldName, gotFieldName)
+		})
+	}
+}

pkg/sqlcache/integration_test.go

@@ -61,7 +61,7 @@ func (i *IntegrationSuite) TearDownSuite() {
 }
 
 func (i *IntegrationSuite) TestSQLCacheFilters() {
-	fields := [][]string{{`metadata`, `annotations[somekey]`}}
+	fields := [][]string{{"metadata", "annotations", "somekey"}}
 	require := i.Require()
 	configMapWithAnnotations := func(name string, annotations map[string]string) v1.ConfigMap {
 		return v1.ConfigMap{
@@ -122,7 +122,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
 		{
 			name: "matches filter",
 			filters: orFiltersForFilters(informer.Filter{
-				Field:   []string{`metadata`, `annotations[somekey]`},
+				Field:   []string{"metadata", "annotations", "somekey"},
 				Matches: []string{"somevalue"},
 				Op:      informer.Eq,
 				Partial: false,
@@ -132,7 +132,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
 		{
 			name: "partial matches filter",
 			filters: orFiltersForFilters(informer.Filter{
-				Field:   []string{`metadata`, `annotations[somekey]`},
+				Field:   []string{"metadata", "annotations", "somekey"},
 				Matches: []string{"somevalue"},
 				Op:      informer.Eq,
 				Partial: true,
@@ -142,7 +142,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
 		{
 			name: "no matches for filter with underscore as it is interpreted literally",
 			filters: orFiltersForFilters(informer.Filter{
-				Field:   []string{`metadata`, `annotations[somekey]`},
+				Field:   []string{"metadata", "annotations", "somekey"},
 				Matches: []string{"somevalu_"},
 				Op:      informer.Eq,
 				Partial: true,
@@ -152,7 +152,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
 		{
 			name: "no matches for filter with percent sign as it is interpreted literally",
 			filters: orFiltersForFilters(informer.Filter{
-				Field:   []string{`metadata`, `annotations[somekey]`},
+				Field:   []string{"metadata", "annotations", "somekey"},
 				Matches: []string{"somevalu%"},
 				Op:      informer.Eq,
 				Partial: true,
@@ -162,7 +162,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
 		{
 			name: "match with special characters",
 			filters: orFiltersForFilters(informer.Filter{
-				Field:   []string{`metadata`, `annotations[somekey]`},
+				Field:   []string{"metadata", "annotations", "somekey"},
 				Matches: []string{"c%%l_value"},
 				Op:      informer.Eq,
 				Partial: true,
@@ -172,7 +172,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
 		{
 			name: "match with literal backslash character",
 			filters: orFiltersForFilters(informer.Filter{
-				Field:   []string{`metadata`, `annotations[somekey]`},
+				Field:   []string{"metadata", "annotations", "somekey"},
 				Matches: []string{`my\windows\path`},
 				Op:      informer.Eq,
 				Partial: true,
@@ -182,7 +182,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
 		{
 			name: "not eq filter",
 			filters: orFiltersForFilters(informer.Filter{
-				Field:   []string{`metadata`, `annotations[somekey]`},
+				Field:   []string{"metadata", "annotations", "somekey"},
 				Matches: []string{"somevalue"},
 				Op:      informer.NotEq,
 				Partial: false,
@@ -192,7 +192,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
 		{
 			name: "partial not eq filter",
 			filters: orFiltersForFilters(informer.Filter{
-				Field:   []string{`metadata`, `annotations[somekey]`},
+				Field:   []string{"metadata", "annotations", "somekey"},
 				Matches: []string{"somevalue"},
 				Op:      informer.NotEq,
 				Partial: true,
@@ -203,13 +203,13 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
 			name: "multiple or filters match",
 			filters: orFiltersForFilters(
 				informer.Filter{
-					Field:   []string{`metadata`, `annotations[somekey]`},
+					Field:   []string{"metadata", "annotations", "somekey"},
 					Matches: []string{"somevalue"},
 					Op:      informer.Eq,
 					Partial: true,
 				},
 				informer.Filter{
-					Field:   []string{`metadata`, `annotations[somekey]`},
+					Field:   []string{"metadata", "annotations", "somekey"},
 					Matches: []string{"notequal"},
 					Op:      informer.Eq,
 					Partial: false,
@@ -221,7 +221,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
 			name: "or filters on different fields",
 			filters: orFiltersForFilters(
 				informer.Filter{
-					Field:   []string{`metadata`, `annotations[somekey]`},
+					Field:   []string{"metadata", "annotations", "somekey"},
 					Matches: []string{"somevalue"},
 					Op:      informer.Eq,
 					Partial: true,
@@ -241,7 +241,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
 				{
 					Filters: []informer.Filter{
 						{
-							Field:   []string{`metadata`, `annotations[somekey]`},
+							Field:   []string{"metadata", "annotations", "somekey"},
 							Matches: []string{"somevalue"},
 							Op:      informer.Eq,
 							Partial: true,
@@ -265,7 +265,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() {
 			name: "no matches",
 			filters: orFiltersForFilters(
 				informer.Filter{
-					Field:   []string{`metadata`, `annotations[somekey]`},
+					Field:   []string{"metadata", "annotations", "somekey"},
 					Matches: []string{"valueNotRepresented"},
 					Op:      informer.Eq,
 					Partial: false,