Skip to content

Commit b3f7ab6

Browse files
michelle192837michelle192837
committed
Allow label/target search for ResultStore sources.
Instead of requiring `label:"prow"` in queries, allow any number of label or target atoms in a query. (This still limits the queries we accept, but it should work for most simple uses of ResultStore sources). Test: Unit tests, and verified with a local instance of TestGrid that a ResultStore source config will pick up results from ResultStore correctly.
1 parent 79d4a04 commit b3f7ab6

File tree

4 files changed

+185
-173
lines changed

4 files changed

+185
-173
lines changed

‎pkg/updater/resultstore/query/query.go‎

Lines changed: 50 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -17,47 +17,76 @@ limitations under the License.
1717
package query
1818

1919
import (
20+
"errors"
2021
"fmt"
2122
"regexp"
2223
"strings"
2324
)
2425

25-
func translateAtom(simpleAtom string) (string, error) {
26-
if simpleAtom == "" {
27-
return "", nil
26+
type keyValue struct {
27+
key string
28+
value string
29+
}
30+
31+
func translateAtom(simpleAtom keyValue, queryTarget bool) (string, error) {
32+
if simpleAtom.key == "" {
33+
return "", errors.New("missing key")
2834
}
29-
// For now, we expect an atom with the exact form `target:"<target>"`
30-
// Split the `key:value` atom.
31-
parts := strings.SplitN(simpleAtom, ":", 2)
32-
if len(parts) != 2 {
33-
return "", fmt.Errorf("unrecognized atom %q", simpleAtom)
35+
if simpleAtom.value == "" {
36+
return "", errors.New("missing value")
3437
}
35-
key := strings.TrimSpace(parts[0])
36-
val := strings.Trim(strings.TrimSpace(parts[1]), `"`)
37-
3838
switch {
39-
case key == "target":
40-
return fmt.Sprintf(`id.target_id="%s"`, val), nil
39+
case simpleAtom.key == "label" && queryTarget:
40+
return fmt.Sprintf(`invocation.invocation_attributes.labels:"%s"`, simpleAtom.value), nil
41+
case simpleAtom.key == "label":
42+
return fmt.Sprintf(`invocation_attributes.labels:"%s"`, simpleAtom.value), nil
43+
case simpleAtom.key == "target":
44+
return fmt.Sprintf(`id.target_id="%s"`, simpleAtom.value), nil
4145
default:
42-
return "", fmt.Errorf("unrecognized atom key %q", key)
46+
return "", fmt.Errorf("unknown type of atom %q", simpleAtom.key)
4347
}
4448
}
4549

4650
var (
47-
queryRe = regexp.MustCompile(`^target:".*"$`)
51+
// Captures any atoms of the form `label:"<label>"` or `target:"<target>"`.
52+
atomReStr = `(?P<atom>(?P<key>label|target):"(?P<value>.+?)")`
53+
atomRe = regexp.MustCompile(atomReStr)
54+
// A query can only have atoms (above), separated by spaces.
55+
queryRe = regexp.MustCompile(`^(` + atomReStr + ` *)+$`)
4856
)
4957

58+
// TranslateQuery translates a simple query (similar to the syntax of searching invocations in the
59+
// UI) to a query for searching via API.
60+
// More at https://github.com/googleapis/googleapis/blob/master/google/devtools/resultstore/v2/resultstore_download.proto.
61+
//
62+
// This expects a query consisting of any number of space-separated `label:"<label>"` or
63+
// `target:"<target>"` atoms.
5064
func TranslateQuery(simpleQuery string) (string, error) {
5165
if simpleQuery == "" {
5266
return "", nil
5367
}
54-
// For now, we expect a query with a single atom, with the exact form `target:"<target>"`
5568
if !queryRe.MatchString(simpleQuery) {
56-
return "", fmt.Errorf("invalid query %q: must match %q", simpleQuery, queryRe.String())
69+
return "", fmt.Errorf("query must consist of only space-separated `label:\"<label>\"` or `target:\"<target>\"` atoms")
70+
}
71+
var simpleAtoms []keyValue
72+
var queryTarget bool
73+
matches := atomRe.FindAllStringSubmatch(simpleQuery, -1)
74+
for _, match := range matches {
75+
if len(match) != 4 {
76+
return "", fmt.Errorf("atom %v: want 4 submatches (full match, atom, key, value), but got %d", match, len(match))
77+
}
78+
simpleAtoms = append(simpleAtoms, keyValue{match[2], match[3]})
79+
if match[2] == "target" {
80+
queryTarget = true
81+
}
5782
}
58-
query, err := translateAtom(simpleQuery)
59-
if err != nil {
60-
return "", fmt.Errorf("invalid query %q: %v", simpleQuery, err)
83+
var atoms []string
84+
for _, simpleAtom := range simpleAtoms {
85+
atom, err := translateAtom(simpleAtom, queryTarget)
86+
if err != nil {
87+
return "", fmt.Errorf("atom %v: %v", simpleAtom, err)
88+
}
89+
atoms = append(atoms, atom)
6190
}
62-
return query, nil
91+
return strings.Join(atoms, " "), nil
6392
}

‎pkg/updater/resultstore/query/query_test.go‎

Lines changed: 111 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -22,127 +22,173 @@ import (
2222

2323
func TestTranslateAtom(t *testing.T) {
2424
cases := []struct {
25-
name string
26-
atom string
27-
want string
28-
wantError bool
25+
name string
26+
atom keyValue
27+
want string
28+
wantTarget string
2929
}{
3030
{
31-
name: "empty",
32-
atom: "",
33-
want: "",
34-
},
35-
{
36-
name: "basic",
37-
atom: `target:"//my-target"`,
38-
want: `id.target_id="//my-target"`,
31+
name: "label atom",
32+
atom: keyValue{"label", "foo"},
33+
want: `invocation_attributes.labels:"foo"`,
34+
wantTarget: `invocation.invocation_attributes.labels:"foo"`,
3935
},
4036
{
41-
name: "case-sensitive key",
42-
atom: `TARGET:"//MY-TARGET"`,
43-
wantError: true,
37+
name: "target atom",
38+
atom: keyValue{"target", "//my-target"},
39+
want: `id.target_id="//my-target"`,
40+
wantTarget: `id.target_id="//my-target"`,
4441
},
42+
}
43+
44+
for _, tc := range cases {
45+
t.Run(tc.name, func(t *testing.T) {
46+
// Invocation queries (queryTarget = false)
47+
got, err := translateAtom(tc.atom, false)
48+
if err != nil {
49+
t.Fatalf("translateAtom(%q, %t) errored: %v", tc.atom, false, err)
50+
}
51+
if tc.want != got {
52+
t.Errorf("translateAtom(%q, %t) differed; got %q, want %q", tc.atom, false, got, tc.want)
53+
}
54+
// Configured Target queries (queryTarget = true)
55+
gotTarget, err := translateAtom(tc.atom, true)
56+
if err != nil {
57+
t.Fatalf("translateAtom(%q, %t) errored: %v", tc.atom, true, err)
58+
}
59+
if tc.wantTarget != gotTarget {
60+
t.Errorf("translateAtom(%q, %t) differed; got %q, want %q", tc.atom, true, gotTarget, tc.wantTarget)
61+
}
62+
})
63+
}
64+
}
65+
66+
func TestTranslateAtom_Error(t *testing.T) {
67+
cases := []struct {
68+
name string
69+
atom keyValue
70+
}{
4571
{
46-
name: "multiple colons",
47-
atom: `target:"//path/to:my-target"`,
48-
want: `id.target_id="//path/to:my-target"`,
72+
name: "empty",
73+
atom: keyValue{"", ""},
4974
},
5075
{
51-
name: "unquoted",
52-
atom: `target://my-target`,
53-
want: `id.target_id="//my-target"`,
76+
name: "case-sensitive key",
77+
atom: keyValue{"TARGET", "//MY-TARGET"},
5478
},
5579
{
56-
name: "partial quotes",
57-
atom: `target://my-target"`,
58-
want: `id.target_id="//my-target"`,
80+
name: "missing key",
81+
atom: keyValue{"", "//path/to:my-target"},
5982
},
6083
{
61-
name: "not enough parts",
62-
atom: "target",
63-
wantError: true,
84+
name: "missing value",
85+
atom: keyValue{"target", ""},
6486
},
6587
{
66-
name: "unknown atom",
67-
atom: "label:foo",
68-
wantError: true,
88+
name: "unknown atom",
89+
atom: keyValue{"custom-property", "foo"},
6990
},
7091
}
7192

7293
for _, tc := range cases {
7394
t.Run(tc.name, func(t *testing.T) {
74-
got, err := translateAtom(tc.atom)
75-
if tc.want != got {
76-
t.Errorf("translateAtom(%q) differed; got %q, want %q", tc.atom, got, tc.want)
95+
// Invocation queries (queryTarget = false)
96+
_, err := translateAtom(tc.atom, false)
97+
if err == nil {
98+
t.Fatalf("translateAtom(%q, %t): want error but got none", tc.atom, false)
7799
}
78-
if err == nil && tc.wantError {
79-
t.Errorf("translateAtom(%q) did not error as expected", tc.atom)
80-
} else if err != nil && !tc.wantError {
81-
t.Errorf("translateAtom(%q) errored unexpectedly: %v", tc.atom, err)
100+
// Configured Target queries (queryTarget = true)
101+
_, err = translateAtom(tc.atom, true)
102+
if err == nil {
103+
t.Fatalf("translateAtom(%q, %t): want error but got none", tc.atom, true)
82104
}
83105
})
84106
}
85107
}
86108

87109
func TestTranslateQuery(t *testing.T) {
88110
cases := []struct {
89-
name string
90-
query string
91-
want string
92-
wantError bool
111+
name string
112+
query string
113+
want string
93114
}{
94115
{
95116
name: "empty",
96117
query: "",
97118
want: "",
98119
},
99120
{
100-
name: "basic",
121+
name: "label",
122+
query: `label:"foo"`,
123+
want: `invocation_attributes.labels:"foo"`,
124+
},
125+
{
126+
name: "target",
101127
query: `target:"//my-target"`,
102128
want: `id.target_id="//my-target"`,
103129
},
104130
{
105-
name: "case-sensitive key",
106-
query: `TARGET:"//MY-TARGET"`,
107-
wantError: true,
131+
name: "multiple labels",
132+
query: `label:"foo" label:"bar"`,
133+
want: `invocation_attributes.labels:"foo" invocation_attributes.labels:"bar"`,
134+
},
135+
{
136+
name: "mixed",
137+
query: `label:"foo" target:"//my-target" label:"bar"`,
138+
want: `invocation.invocation_attributes.labels:"foo" id.target_id="//my-target" invocation.invocation_attributes.labels:"bar"`,
108139
},
109140
{
110141
name: "multiple colons",
111142
query: `target:"//path/to:my-target"`,
112143
want: `id.target_id="//path/to:my-target"`,
113144
},
145+
}
146+
147+
for _, tc := range cases {
148+
t.Run(tc.name, func(t *testing.T) {
149+
got, err := TranslateQuery(tc.query)
150+
if err != nil {
151+
t.Fatalf("translateQuery(%q) errored: %v", tc.query, err)
152+
}
153+
if tc.want != got {
154+
t.Errorf("translateQuery(%q) differed; got %q, want %q", tc.query, got, tc.want)
155+
}
156+
})
157+
}
158+
}
159+
160+
func TestTranslateQuery_Error(t *testing.T) {
161+
cases := []struct {
162+
name string
163+
query string
164+
}{
165+
{
166+
name: "case-sensitive key",
167+
query: `TARGET:"//MY-TARGET"`,
168+
},
114169
{
115-
name: "unquoted",
116-
query: `target://my-target`,
117-
wantError: true,
170+
name: "unquoted",
171+
query: `target://my-target`,
118172
},
119173
{
120-
name: "partial quotes",
121-
query: `target://my-target"`,
122-
wantError: true,
174+
name: "partial quotes",
175+
query: `target://my-target"`,
123176
},
124177
{
125-
name: "invalid query",
126-
query: `label:foo`,
127-
wantError: true,
178+
name: "invalid query",
179+
query: `label:foo`,
128180
},
129181
{
130-
name: "partial match",
131-
query: `some_target:foo`,
132-
wantError: true,
182+
name: "partial match",
183+
query: `some_target:foo`,
133184
},
134185
}
135186

136187
for _, tc := range cases {
137188
t.Run(tc.name, func(t *testing.T) {
138-
got, err := TranslateQuery(tc.query)
139-
if tc.want != got {
140-
t.Errorf("translateQuery(%q) differed; got %q, want %q", tc.query, got, tc.want)
141-
}
142-
if tc.wantError && err == nil {
143-
t.Errorf("translateQuery(%q) did not error as expected", tc.query)
144-
} else if !tc.wantError && err != nil {
145-
t.Errorf("translateQuery(%q) errored unexpectedly: %v", tc.query, err)
189+
_, err := TranslateQuery(tc.query)
190+
if err == nil {
191+
t.Fatalf("translateQuery(%q): want error, got none", tc.query)
146192
}
147193
})
148194
}

‎pkg/updater/resultstore/resultstore.go‎

Lines changed: 8 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -887,36 +887,23 @@ func queryAfter(query string, when time.Time) string {
887887
if query == "" {
888888
return ""
889889
}
890-
return fmt.Sprintf("%s timing.start_time>=\"%s\"", query, when.UTC().Format(time.RFC3339))
891-
}
892-
893-
const (
894-
// Use this when searching invocations, e.g. if query does not search for a target.
895-
prowLabel = `invocation_attributes.labels:"prow"`
896-
// Use this when searching for a configured target, e.g. if query contains `target:"<target>"`.
897-
prowTargetLabel = `invocation.invocation_attributes.labels:"prow"`
898-
)
899-
900-
func queryProw(baseQuery string, stop time.Time) (string, error) {
901-
// TODO: ResultStore use is assumed to be Prow-only at the moment. Make this more flexible in future.
902-
if baseQuery == "" {
903-
return queryAfter(prowLabel, stop), nil
904-
}
905-
query, err := query.TranslateQuery(baseQuery)
906-
if err != nil {
907-
return "", err
890+
// Note: time arguments are different for invocation vs. configured target searches.
891+
timeAtom := fmt.Sprintf(`timing.start_time>="%s"`, when.UTC().Format(time.RFC3339))
892+
if strings.Contains(query, "id.target_id=") {
893+
timeAtom = fmt.Sprintf(`invocation.timing.start_time>="%s"`, when.UTC().Format(time.RFC3339))
908894
}
909-
return queryAfter(fmt.Sprintf("%s %s", query, prowTargetLabel), stop), nil
895+
return fmt.Sprintf(`%s %s`, query, timeAtom)
910896
}
911897

912898
func search(ctx context.Context, log logrus.FieldLogger, client *DownloadClient, rsConfig *configpb.ResultStoreConfig, stop time.Time) ([]string, error) {
913899
if client == nil {
914900
return nil, fmt.Errorf("no ResultStore client provided")
915901
}
916-
query, err := queryProw(rsConfig.GetQuery(), stop)
902+
translatedQuery, err := query.TranslateQuery(rsConfig.GetQuery())
917903
if err != nil {
918-
return nil, fmt.Errorf("queryProw() failed to create query: %v", err)
904+
return nil, fmt.Errorf("query.TranslateQuery(%s): %v", rsConfig.GetQuery(), err)
919905
}
906+
query := queryAfter(translatedQuery, stop)
920907
log.WithField("query", query).Debug("Searching ResultStore.")
921908
// Quit if search goes over 5 minutes.
922909
ctx, cancel := context.WithTimeout(ctx, 5*time.Minute)

0 commit comments

Comments
 (0)