Merge pull request cli#12170 from cli/babakks/isolate-user-query-from-base-qualifiers

Isolate user-provided search query from contextual qualifiers

Author: babakks

pkg/cmd/issue/list/list_test.go

@@ -443,7 +443,7 @@ func Test_issueList(t *testing.T) {
 							"owner": "OWNER",
 							"repo":  "REPO",
 							"limit": float64(30),
-							"query": "auth bug assignee:@me author:@me mentions:@me repo:OWNER/REPO state:open type:issue",
+							"query": "( auth bug ) assignee:@me author:@me mentions:@me repo:OWNER/REPO state:open type:issue",
 							"type":  "ISSUE_ADVANCED",
 						}, params)
 					}))
@@ -618,7 +618,7 @@ func TestIssueList_Search_withProjectItems(t *testing.T) {
 				"repo":  "REPO",
 				"type":  "ISSUE_ADVANCED",
 				"limit": float64(30),
-				"query": "just used to force the search API branch repo:OWNER/REPO type:issue",
+				"query": "( just used to force the search API branch ) repo:OWNER/REPO type:issue",
 			}, params)
 		}))
 

pkg/cmd/pr/list/http_test.go

@@ -149,7 +149,7 @@ func Test_ListPullRequests(t *testing.T) {
 					httpmock.GraphQL(`query PullRequestSearch\b`),
 					httpmock.GraphQLQuery(`{"data":{}}`, func(query string, vars map[string]interface{}) {
 						want := map[string]interface{}{
-							"q":     "one world in:title repo:OWNER/REPO state:open type:pr",
+							"q":     "( one world in:title ) repo:OWNER/REPO state:open type:pr",
 							"type":  "ISSUE_ADVANCED",
 							"limit": float64(30),
 						}

pkg/cmd/pr/list/list_test.go

@@ -441,7 +441,7 @@ func TestPRList_Search_withProjectItems(t *testing.T) {
 		  }`, func(_ string, params map[string]interface{}) {
 			require.Equal(t, map[string]interface{}{
 				"limit": float64(30),
-				"q":     "just used to force the search API branch repo:OWNER/REPO state:open type:pr",
+				"q":     "( just used to force the search API branch ) repo:OWNER/REPO state:open type:pr",
 				"type":  "ISSUE_ADVANCED",
 			}, params)
 		}))

pkg/cmd/pr/shared/params.go

@@ -222,19 +222,13 @@ func SearchQueryBuild(options FilterOptions, advancedIssueSearchSyntax bool) str
 			Is:        []string{is},
 			Type:      options.Entity,
 		},
+		ImmutableKeywords: options.Search,
 	}
 
-	var q string
-	if advancedIssueSearchSyntax {
-		q = query.AdvancedIssueSearchString()
-	} else {
-		q = query.StandardSearchString()
+	if !advancedIssueSearchSyntax {
+		return query.StandardSearchString()
 	}
-
-	if options.Search != "" {
-		return fmt.Sprintf("%s %s", options.Search, q)
-	}
-	return q
+	return query.AdvancedIssueSearchString()
 }
 
 func QueryHasStateClause(searchQuery string) bool {

pkg/search/query.go

@@ -16,7 +16,21 @@ const (
 )
 
 type Query struct {
-	Keywords   []string
+	// Keywords holds the list of keywords to search for. These keywords are
+	// treated as individual components of a search query, and will get quoted
+	// as needed. This is useful when the input can be supplied as a list of
+	// search keywords.
+	//
+	// This field is overridden by ImmutableKeywords.
+	Keywords []string
+
+	// ImmutableKeywords holds the search keywords as a single string, and will
+	// be treated as is (e.g. no additional quoting). This is useful when the
+	// input is meant to be taken verbatim from the user.
+	//
+	// This field takes precedence over Keywords.
+	ImmutableKeywords string
+
 	Kind       string
 	Limit      int
 	Order      string
@@ -103,7 +117,12 @@ type Qualifiers struct {
 // issues, and it's called advanced issue search.
 func (q Query) StandardSearchString() string {
 	qualifiers := formatQualifiers(q.Qualifiers, nil)
-	keywords := formatKeywords(q.Keywords)
+	var keywords []string
+	if q.ImmutableKeywords != "" {
+		keywords = []string{q.ImmutableKeywords}
+	} else if ks := formatKeywords(q.Keywords); len(ks) > 0 {
+		keywords = ks
+	}
 	all := append(keywords, qualifiers...)
 	return strings.TrimSpace(strings.Join(all, " "))
 }
@@ -124,10 +143,25 @@ func (q Query) StandardSearchString() string {
 //
 // The advanced syntax is documented at https://github.blog/changelog/2025-03-06-github-issues-projects-api-support-for-issues-advanced-search-and-more
 func (q Query) AdvancedIssueSearchString() string {
-	qualifiers := formatQualifiers(q.Qualifiers, formatAdvancedIssueSearch)
-	keywords := formatKeywords(q.Keywords)
-	all := append(keywords, qualifiers...)
-	return strings.TrimSpace(strings.Join(all, " "))
+	qualifiers := strings.Join(formatQualifiers(q.Qualifiers, formatAdvancedIssueSearch), " ")
+	keywords := q.ImmutableKeywords
+	if keywords == "" {
+		keywords = strings.Join(formatKeywords(q.Keywords), " ")
+	}
+
+	if qualifiers == "" && keywords == "" {
+		return ""
+	}
+
+	if qualifiers != "" && keywords != "" {
+		// We should surround keywords with brackets to avoid leaking of any operators, especially "OR"s.
+		return fmt.Sprintf("( %s ) %s", keywords, qualifiers)
+	}
+
+	if keywords != "" {
+		return keywords
+	}
+	return qualifiers
 }
 
 func formatAdvancedIssueSearch(qualifier string, vs []string) (s []string, applicable bool) {

pkg/search/query_test.go

@@ -14,6 +14,10 @@ func TestStandardSearchString(t *testing.T) {
 		query Query
 		out   string
 	}{
+		{
+			name: "empty query",
+			out:  "",
+		},
 		{
 			name: "converts query to string",
 			query: Query{
@@ -70,6 +74,31 @@ func TestStandardSearchString(t *testing.T) {
 			},
 			out: `topic:"quote qualifier"`,
 		},
+		{
+			name: "respects immutable keywords",
+			query: Query{
+				ImmutableKeywords: "immutable keyword that should be left as is",
+			},
+			out: `immutable keyword that should be left as is`,
+		},
+		{
+			name: "respects immutable keywords, with qualifiers",
+			query: Query{
+				ImmutableKeywords: "immutable keyword that should be left as is",
+				Qualifiers: Qualifiers{
+					Topic: []string{"quote qualifier"},
+				},
+			},
+			out: `immutable keyword that should be left as is topic:"quote qualifier"`,
+		},
+		{
+			name: "prioritises immutable keywords over keywords slice",
+			query: Query{
+				Keywords:          []string{"foo", "bar"},
+				ImmutableKeywords: "immutable keyword",
+			},
+			out: `immutable keyword`,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -84,6 +113,10 @@ func TestAdvancedIssueSearchString(t *testing.T) {
 		query Query
 		out   string
 	}{
+		{
+			name: "empty query",
+			out:  "",
+		},
 		{
 			name: "quotes keywords",
 			query: Query{
@@ -107,6 +140,31 @@ func TestAdvancedIssueSearchString(t *testing.T) {
 			},
 			out: `label:"quote qualifier"`,
 		},
+		{
+			name: "respects immutable keywords",
+			query: Query{
+				ImmutableKeywords: "immutable keyword that should be left as is",
+			},
+			out: `immutable keyword that should be left as is`,
+		},
+		{
+			name: "respects immutable keywords, with qualifiers",
+			query: Query{
+				ImmutableKeywords: "immutable keyword that should be left as is",
+				Qualifiers: Qualifiers{
+					Topic: []string{"quote qualifier"},
+				},
+			},
+			out: `( immutable keyword that should be left as is ) topic:"quote qualifier"`,
+		},
+		{
+			name: "prioritises immutable keywords over keywords slice",
+			query: Query{
+				Keywords:          []string{"foo", "bar"},
+				ImmutableKeywords: "immutable keyword",
+			},
+			out: `immutable keyword`,
+		},
 		{
 			name: "unused qualifiers should not appear in query",
 			query: Query{
@@ -115,7 +173,7 @@ func TestAdvancedIssueSearchString(t *testing.T) {
 					Label: []string{"foo", "bar"},
 				},
 			},
-			out: `keyword label:bar label:foo`,
+			out: `( keyword ) label:bar label:foo`,
 		},
 		{
 			name: "special qualifiers when used once",
@@ -128,7 +186,7 @@ func TestAdvancedIssueSearchString(t *testing.T) {
 					In:   []string{"title"},
 				},
 			},
-			out: `keyword in:title is:private repo:foo/bar user:johndoe`,
+			out: `( keyword ) in:title is:private repo:foo/bar user:johndoe`,
 		},
 		{
 			name: "special qualifiers are OR-ed when used multiple times",
@@ -141,7 +199,7 @@ func TestAdvancedIssueSearchString(t *testing.T) {
 					In:   []string{"title", "body", "comments", "foo"}, // "foo" is to ensure only "title", "body", and "comments" are grouped
 				},
 			},
-			out: `keyword (in:body OR in:comments OR in:title) in:foo (is:blocked OR is:blocking) (is:closed OR is:open) (is:issue OR is:pr) (is:locked OR is:unlocked) (is:merged OR is:unmerged) (is:private OR is:public) is:foo (repo:foo/bar OR repo:foo/baz) (user:janedoe OR user:johndoe)`,
+			out: `( keyword ) (in:body OR in:comments OR in:title) in:foo (is:blocked OR is:blocking) (is:closed OR is:open) (is:issue OR is:pr) (is:locked OR is:unlocked) (is:merged OR is:unmerged) (is:private OR is:public) is:foo (repo:foo/bar OR repo:foo/baz) (user:janedoe OR user:johndoe)`,
 		},
 		{
 			// Since this is a general purpose package, we can't assume with know all
@@ -155,7 +213,7 @@ func TestAdvancedIssueSearchString(t *testing.T) {
 					In: []string{"foo", "bar"},
 				},
 			},
-			out: `keyword in:bar in:foo is:bar is:foo`,
+			out: `( keyword ) in:bar in:foo is:bar is:foo`,
 		},
 		{
 			name: "non-special qualifiers used multiple times",
@@ -170,7 +228,7 @@ func TestAdvancedIssueSearchString(t *testing.T) {
 					Topic:   []string{"foo", "bar"},
 				},
 			},
-			out: `keyword in:bar in:foo is:bar is:foo label:bar label:foo license:bar license:foo no:bar no:foo topic:bar topic:foo`,
+			out: `( keyword ) in:bar in:foo is:bar is:foo label:bar label:foo license:bar license:foo no:bar no:foo topic:bar topic:foo`,
 		},
 	}
 	for _, tt := range tests {

pkg/search/searcher_test.go

@@ -1178,7 +1178,7 @@ func TestSearcherIssuesAdvancedSyntax(t *testing.T) {
 			detector: fd.AdvancedIssueSearchSupportedAsOptIn(),
 			query:    query,
 			wantValues: url.Values{
-				"q":               []string{"keyword author:johndoe (in:body OR in:comments OR in:title) (is:private OR is:public) label:bar label:foo (repo:foo/bar OR repo:foo/baz) (user:janedoe OR user:johndoe)"},
+				"q":               []string{"( keyword ) author:johndoe (in:body OR in:comments OR in:title) (is:private OR is:public) label:bar label:foo (repo:foo/bar OR repo:foo/baz) (user:janedoe OR user:johndoe)"},
 				"advanced_search": []string{"true"}, // opt-in
 			},
 		},
@@ -1189,7 +1189,7 @@ func TestSearcherIssuesAdvancedSyntax(t *testing.T) {
 			detector: fd.AdvancedIssueSearchSupportedAsOnlyBackend(),
 			query:    query,
 			wantValues: url.Values{
-				"q":               []string{"keyword author:johndoe (in:body OR in:comments OR in:title) (is:private OR is:public) label:bar label:foo (repo:foo/bar OR repo:foo/baz) (user:janedoe OR user:johndoe)"},
+				"q":               []string{"( keyword ) author:johndoe (in:body OR in:comments OR in:title) (is:private OR is:public) label:bar label:foo (repo:foo/bar OR repo:foo/baz) (user:janedoe OR user:johndoe)"},
 				"advanced_search": nil, // assert absence
 			},
 		},