Skip to content

Commit 584ede0

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 cb45d2a commit 584ede0

File tree

4 files changed

+96
-139
lines changed

4 files changed

+96
-139
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: 22 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -22,63 +22,54 @@ 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+
queryTarget bool
28+
want string
29+
wantError bool
2930
}{
3031
{
3132
name: "empty",
32-
atom: "",
33+
atom: keyValue{"", ""},
3334
want: "",
3435
},
3536
{
3637
name: "basic",
37-
atom: `target:"//my-target"`,
38+
atom: keyValue{"target", "//my-target"},
3839
want: `id.target_id="//my-target"`,
3940
},
4041
{
41-
name: "case-sensitive key",
42-
atom: `TARGET:"//MY-TARGET"`,
43-
wantError: true,
42+
name: "case-sensitive key",
43+
atom: keyValue{"TARGET", "//MY-TARGET"},
44+
want: "",
4445
},
4546
{
4647
name: "multiple colons",
47-
atom: `target:"//path/to:my-target"`,
48+
atom: keyValue{"target", "//path/to:my-target"},
4849
want: `id.target_id="//path/to:my-target"`,
4950
},
5051
{
51-
name: "unquoted",
52-
atom: `target://my-target`,
53-
want: `id.target_id="//my-target"`,
54-
},
55-
{
56-
name: "partial quotes",
57-
atom: `target://my-target"`,
58-
want: `id.target_id="//my-target"`,
52+
name: "missing key",
53+
atom: keyValue{"", "//path/to:my-target"},
54+
want: "",
5955
},
6056
{
61-
name: "not enough parts",
62-
atom: "target",
63-
wantError: true,
57+
name: "missing value",
58+
atom: keyValue{"target", ""},
59+
want: "",
6460
},
6561
{
66-
name: "unknown atom",
67-
atom: "label:foo",
68-
wantError: true,
62+
name: "unknown atom",
63+
atom: keyValue{"custom-property", "foo"},
64+
want: "",
6965
},
7066
}
7167

7268
for _, tc := range cases {
7369
t.Run(tc.name, func(t *testing.T) {
74-
got, err := translateAtom(tc.atom)
70+
got := translateAtom(tc.atom, tc.queryTarget)
7571
if tc.want != got {
76-
t.Errorf("translateAtom(%q) differed; got %q, want %q", tc.atom, got, tc.want)
77-
}
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)
72+
t.Errorf("translateAtom(%q, %t) differed; got %q, want %q", tc.atom, tc.queryTarget, got, tc.want)
8273
}
8374
})
8475
}

‎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)

‎pkg/updater/resultstore/resultstore_test.go‎

Lines changed: 16 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -203,30 +203,32 @@ func TestColumnReader(t *testing.T) {
203203
oneMonthConfig := &configpb.TestGroup{
204204
Name: "a-test-group",
205205
DaysOfResults: 30,
206+
ResultSource: &configpb.TestGroup_ResultSource{
207+
ResultSourceConfig: &configpb.TestGroup_ResultSource_ResultstoreConfig{
208+
ResultstoreConfig: &configpb.ResultStoreConfig{
209+
Query: `label:"foo"`,
210+
},
211+
},
212+
},
206213
}
207214
now := time.Now()
208215
oneDayAgo := now.AddDate(0, 0, -1)
209216
twoDaysAgo := now.AddDate(0, 0, -2)
210217
threeDaysAgo := now.AddDate(0, 0, -3)
211218
oneMonthAgo := now.AddDate(0, 0, -30)
212-
testQueryAfter := queryAfter(prowLabel, oneMonthAgo)
219+
testQueryAfter := queryAfter(`invocation_attributes.labels:"foo"`, oneMonthAgo)
213220
cases := []struct {
214221
name string
215222
client *fakeClient
216-
tg *configpb.TestGroup
217223
want []updater.InflatedColumn
218224
wantErr bool
219225
}{
220226
{
221227
name: "empty",
222-
tg: oneMonthConfig,
223228
wantErr: true,
224229
},
225230
{
226231
name: "basic",
227-
tg: &configpb.TestGroup{
228-
DaysOfResults: 30,
229-
},
230232
client: &fakeClient{
231233
searches: map[string][]string{
232234
testQueryAfter: {"id-1", "id-2"},
@@ -351,7 +353,6 @@ func TestColumnReader(t *testing.T) {
351353
},
352354
{
353355
name: "no results from query",
354-
tg: oneMonthConfig,
355356
client: &fakeClient{
356357
searches: map[string][]string{},
357358
invocations: map[string]FetchResult{
@@ -2595,15 +2596,15 @@ func TestQueryAfter(t *testing.T) {
25952596
},
25962597
{
25972598
name: "zero",
2598-
query: prowLabel,
2599+
query: `invocation_attributes.labels:"foo"`,
25992600
when: time.Time{},
2600-
want: "invocation_attributes.labels:\"prow\" timing.start_time>=\"0001-01-01T00:00:00Z\"",
2601+
want: `invocation_attributes.labels:"foo" timing.start_time>="0001-01-01T00:00:00Z"`,
26012602
},
26022603
{
26032604
name: "basic",
2604-
query: prowLabel,
2605+
query: `invocation_attributes.labels:"foo"`,
26052606
when: now,
2606-
want: fmt.Sprintf("invocation_attributes.labels:\"prow\" timing.start_time>=\"%s\"", now.UTC().Format(time.RFC3339)),
2607+
want: fmt.Sprintf(`invocation_attributes.labels:"foo" timing.start_time>="%s"`, now.UTC().Format(time.RFC3339)),
26072608
},
26082609
}
26092610
for _, tc := range cases {
@@ -2616,63 +2617,10 @@ func TestQueryAfter(t *testing.T) {
26162617
}
26172618
}
26182619

2619-
func TestQueryProw(t *testing.T) {
2620-
now := time.Now()
2621-
cases := []struct {
2622-
name string
2623-
query string
2624-
when time.Time
2625-
want string
2626-
wantError bool
2627-
}{
2628-
{
2629-
name: "empty",
2630-
want: "invocation_attributes.labels:\"prow\" timing.start_time>=\"0001-01-01T00:00:00Z\"",
2631-
},
2632-
{
2633-
name: "zero",
2634-
query: "",
2635-
when: time.Time{},
2636-
want: "invocation_attributes.labels:\"prow\" timing.start_time>=\"0001-01-01T00:00:00Z\"",
2637-
},
2638-
{
2639-
name: "basic",
2640-
query: "",
2641-
when: now,
2642-
want: fmt.Sprintf("invocation_attributes.labels:\"prow\" timing.start_time>=\"%s\"", now.UTC().Format(time.RFC3339)),
2643-
},
2644-
{
2645-
name: "target",
2646-
query: `target:"my-job"`,
2647-
when: now,
2648-
want: fmt.Sprintf("id.target_id=\"my-job\" invocation.invocation_attributes.labels:\"prow\" timing.start_time>=\"%s\"", now.UTC().Format(time.RFC3339)),
2649-
},
2650-
{
2651-
name: "invalid query",
2652-
query: `label:foo bar`,
2653-
when: now,
2654-
wantError: true,
2655-
},
2656-
}
2657-
for _, tc := range cases {
2658-
t.Run(tc.name, func(t *testing.T) {
2659-
got, err := queryProw(tc.query, tc.when)
2660-
if !tc.wantError && err != nil {
2661-
t.Errorf("queryProw(%q, %v) errored: %v", tc.query, tc.when, err)
2662-
} else if tc.wantError && err == nil {
2663-
t.Errorf("queryProw(%q, %v) did not error as expected", tc.query, tc.when)
2664-
}
2665-
if diff := cmp.Diff(tc.want, got); diff != "" {
2666-
t.Errorf("queryProw(%q, %v) differed (-want, +got): %s", tc.query, tc.when, diff)
2667-
}
2668-
})
2669-
}
2670-
}
2671-
26722620
func TestSearch(t *testing.T) {
26732621
twoDaysAgo := time.Now().AddDate(0, 0, -2)
2674-
testQueryAfter := queryAfter(prowLabel, twoDaysAgo)
2675-
testTargetQueryAfter, _ := queryProw(`target:"my-job"`, twoDaysAgo)
2622+
testQueryAfter := queryAfter(`invocation_attributes.labels:"foo"`, twoDaysAgo)
2623+
testTargetQueryAfter := queryAfter(`id.target_id="my-job"`, twoDaysAgo)
26762624
cases := []struct {
26772625
name string
26782626
stop time.Time
@@ -2695,6 +2643,7 @@ func TestSearch(t *testing.T) {
26952643
name: "no results",
26962644
rsConfig: &configpb.ResultStoreConfig{
26972645
Project: "my-project",
2646+
Query: `label:foo`,
26982647
},
26992648
client: &fakeClient{},
27002649
wantErr: true,
@@ -2703,6 +2652,7 @@ func TestSearch(t *testing.T) {
27032652
name: "basic",
27042653
rsConfig: &configpb.ResultStoreConfig{
27052654
Project: "my-project",
2655+
Query: `label:"foo"`,
27062656
},
27072657
client: &fakeClient{
27082658
searches: map[string][]string{

0 commit comments

Comments
 (0)