From 72859237e26439cfc23be9fb2a8144dc441a0826 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 12 May 2026 20:40:02 -0600 Subject: [PATCH] Move --type into the search Qualifiers The --type filter was concatenated into ImmutableKeywords in SearchQueryBuild, which bypassed search.Query's quoting and let values that contained quotes (or extra qualifiers) corrupt the final query. Per babakks's suggestion in the review thread, model it as a regular qualifier instead. Add an IssueType field to Qualifiers tagged `qualifier:"type"` so it shares a key with the existing Type field, and rework Qualifiers.Map to honour the tag and concatenate values when multiple fields share the same key. SearchQueryBuild now drops its bespoke type:X formatting and just sets Qualifiers.IssueType, leaving the keyword/qualifier escaping to pkg/search. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/issue/list/list_test.go | 2 +- pkg/cmd/pr/shared/params.go | 15 ++------------- pkg/cmd/pr/shared/params_test.go | 26 ++++++++++++++++++++++++++ pkg/search/query.go | 30 +++++++++++++++++------------- pkg/search/query_test.go | 10 ++++++++++ 5 files changed, 56 insertions(+), 27 deletions(-) diff --git a/pkg/cmd/issue/list/list_test.go b/pkg/cmd/issue/list/list_test.go index 910e0e9bf..af0879a6b 100644 --- a/pkg/cmd/issue/list/list_test.go +++ b/pkg/cmd/issue/list/list_test.go @@ -561,7 +561,7 @@ func Test_issueList(t *testing.T) { "owner": "OWNER", "repo": "REPO", "limit": float64(30), - "query": "( type:Bug ) repo:OWNER/REPO state:open type:issue", + "query": "repo:OWNER/REPO state:open type:Bug type:issue", "type": "ISSUE_ADVANCED", }, params) })) diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 3891a83fb..54854db8f 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -241,18 +241,6 @@ func SearchQueryBuild(options FilterOptions, advancedIssueSearchSyntax bool) str is = "merged" } - searchTerms := options.Search - if options.IssueType != "" { - if searchTerms != "" { - searchTerms += " " - } - if strings.Contains(options.IssueType, " ") { - searchTerms += fmt.Sprintf(`type:"%s"`, options.IssueType) - } else { - searchTerms += "type:" + options.IssueType - } - } - query := search.Query{ Qualifiers: search.Qualifiers{ Assignee: options.Assignee, @@ -260,6 +248,7 @@ func SearchQueryBuild(options FilterOptions, advancedIssueSearchSyntax bool) str Base: options.BaseBranch, Draft: options.Draft, Head: options.HeadBranch, + IssueType: options.IssueType, Label: options.Labels, Mentions: options.Mention, Milestone: options.Milestone, @@ -268,7 +257,7 @@ func SearchQueryBuild(options FilterOptions, advancedIssueSearchSyntax bool) str Is: []string{is}, Type: options.Entity, }, - ImmutableKeywords: searchTerms, + ImmutableKeywords: options.Search, } if !advancedIssueSearchSyntax { diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index ddb7a1b2f..b777cbc9d 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -173,6 +173,32 @@ func Test_listURLWithQuery(t *testing.T) { want: "https://example.com/path?q=label%3A%22help+wanted%22+label%3Adocs+milestone%3A%22Codename+%5C%22What+Was+Missing%5C%22%22+state%3Aopen+type%3Apr", wantErr: false, }, + { + name: "issue type", + args: args{ + listURL: "https://example.com/path", + options: FilterOptions{ + Entity: "issue", + State: "open", + IssueType: "Bug", + }, + }, + want: "https://example.com/path?q=state%3Aopen+type%3ABug+type%3Aissue", + wantErr: false, + }, + { + name: "issue type with spaces is quoted", + args: args{ + listURL: "https://example.com/path", + options: FilterOptions{ + Entity: "issue", + State: "open", + IssueType: `Hot "Spicy" Bug`, + }, + }, + want: "https://example.com/path?q=state%3Aopen+type%3A%22Hot+%5C%22Spicy%5C%22+Bug%22+type%3Aissue", + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/search/query.go b/pkg/search/query.go index f6e7fc05d..e45a4438a 100644 --- a/pkg/search/query.go +++ b/pkg/search/query.go @@ -96,6 +96,7 @@ type Qualifiers struct { Topics string Tree string Type string + IssueType string `qualifier:"type"` Updated string User []string } @@ -234,39 +235,42 @@ func groupWithOR(qualifier string, vs []string) string { return fmt.Sprintf("(%s)", strings.Join(all, " OR ")) } +// Map turns the qualifiers into a slice-keyed map ready for query +// formatting. Multiple struct fields can share the same key when +// tagged with `qualifier:""`; in that case their values are +// concatenated under the shared key. func (q Qualifiers) Map() map[string][]string { m := map[string][]string{} v := reflect.ValueOf(q) t := reflect.TypeOf(q) for i := 0; i < v.NumField(); i++ { - fieldName := t.Field(i).Name - key := camelToKebab(fieldName) - typ := v.FieldByName(fieldName).Kind() - value := v.FieldByName(fieldName) - switch typ { + field := t.Field(i) + key := field.Tag.Get("qualifier") + if key == "" { + key = camelToKebab(field.Name) + } + value := v.Field(i) + switch value.Kind() { case reflect.Ptr: if value.IsNil() { continue } - v := reflect.Indirect(value) - m[key] = []string{fmt.Sprintf("%v", v)} + m[key] = append(m[key], fmt.Sprintf("%v", reflect.Indirect(value))) case reflect.Slice: if value.IsNil() { continue } - s := []string{} - for i := 0; i < value.Len(); i++ { - if value.Index(i).IsZero() { + for j := 0; j < value.Len(); j++ { + if value.Index(j).IsZero() { continue } - s = append(s, fmt.Sprintf("%v", value.Index(i))) + m[key] = append(m[key], fmt.Sprintf("%v", value.Index(j))) } - m[key] = s default: if value.IsZero() { continue } - m[key] = []string{fmt.Sprintf("%v", value)} + m[key] = append(m[key], fmt.Sprintf("%v", value)) } } return m diff --git a/pkg/search/query_test.go b/pkg/search/query_test.go index db6934ba0..1b9572980 100644 --- a/pkg/search/query_test.go +++ b/pkg/search/query_test.go @@ -308,6 +308,16 @@ func TestQualifiersMap(t *testing.T) { "user": {"user"}, }, }, + { + name: "concatenates fields that share a qualifier key", + qualifiers: Qualifiers{ + Type: "issue", + IssueType: "Bug", + }, + out: map[string][]string{ + "type": {"issue", "Bug"}, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {