Skip to content

Commit d52c1d4

Browse files
gokhanarkangokhanarkan
authored
Fix duplicate filter issues across all search tools (#828)
* Enhance search functionality with new query filters and utility functions - Added multiple test cases for searching issues with various query filters in `issues_test.go`. - Introduced utility functions in `search_utils.go` to check for specific filters and extract repository information from queries. - Created comprehensive tests in `search_utils_test.go` to validate the new filtering logic and ensure accurate query parsing. * Refactor repository filter extraction in search utilities - Renamed `extractRepoFilter` to `hasRepoFilter` to simplify the function's purpose. - Updated test cases in `search_utils_test.go` to reflect the new function name and logic. - Adjusted tests to focus on the presence of the `repo:` filter rather than extracting owner and repo details. * Enhance search functionality with additional query filters and utility functions - Added new test cases for searching pull requests and users with various query filters in `pullrequests_test.go` and `search_test.go`. - Implemented a utility function `hasTypeFilter` in `search_utils.go` to check for the presence of `type:` filters in queries. - Updated the search handler to conditionally prepend the `type:` filter based on the presence of existing filters, improving query handling. * Updated both regex patterns to handle this edge case
1 parent 0e7a25e commit d52c1d4

File tree

6 files changed

+630
-3
lines changed

6 files changed

+630
-3
lines changed

‎pkg/github/issues_test.go

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -411,6 +411,100 @@ func Test_SearchIssues(t *testing.T) {
411411
expectError: false,
412412
expectedResult: mockSearchResult,
413413
},
414+
{
415+
name: "query with existing is:issue filter - no duplication",
416+
mockedClient: mock.NewMockedHTTPClient(
417+
mock.WithRequestMatchHandler(
418+
mock.GetSearchIssues,
419+
expectQueryParams(
420+
t,
421+
map[string]string{
422+
"q": "repo:github/github-mcp-server is:issue is:open (label:critical OR label:urgent)",
423+
"page": "1",
424+
"per_page": "30",
425+
},
426+
).andThen(
427+
mockResponse(t, http.StatusOK, mockSearchResult),
428+
),
429+
),
430+
),
431+
requestArgs: map[string]interface{}{
432+
"query": "repo:github/github-mcp-server is:issue is:open (label:critical OR label:urgent)",
433+
},
434+
expectError: false,
435+
expectedResult: mockSearchResult,
436+
},
437+
{
438+
name: "query with existing repo: filter and conflicting owner/repo params - uses query filter",
439+
mockedClient: mock.NewMockedHTTPClient(
440+
mock.WithRequestMatchHandler(
441+
mock.GetSearchIssues,
442+
expectQueryParams(
443+
t,
444+
map[string]string{
445+
"q": "is:issue repo:github/github-mcp-server critical",
446+
"page": "1",
447+
"per_page": "30",
448+
},
449+
).andThen(
450+
mockResponse(t, http.StatusOK, mockSearchResult),
451+
),
452+
),
453+
),
454+
requestArgs: map[string]interface{}{
455+
"query": "repo:github/github-mcp-server critical",
456+
"owner": "different-owner",
457+
"repo": "different-repo",
458+
},
459+
expectError: false,
460+
expectedResult: mockSearchResult,
461+
},
462+
{
463+
name: "query with both is: and repo: filters already present",
464+
mockedClient: mock.NewMockedHTTPClient(
465+
mock.WithRequestMatchHandler(
466+
mock.GetSearchIssues,
467+
expectQueryParams(
468+
t,
469+
map[string]string{
470+
"q": "is:issue repo:octocat/Hello-World bug",
471+
"page": "1",
472+
"per_page": "30",
473+
},
474+
).andThen(
475+
mockResponse(t, http.StatusOK, mockSearchResult),
476+
),
477+
),
478+
),
479+
requestArgs: map[string]interface{}{
480+
"query": "is:issue repo:octocat/Hello-World bug",
481+
},
482+
expectError: false,
483+
expectedResult: mockSearchResult,
484+
},
485+
{
486+
name: "complex query with multiple OR operators and existing filters",
487+
mockedClient: mock.NewMockedHTTPClient(
488+
mock.WithRequestMatchHandler(
489+
mock.GetSearchIssues,
490+
expectQueryParams(
491+
t,
492+
map[string]string{
493+
"q": "repo:github/github-mcp-server is:issue (label:critical OR label:urgent OR label:high-priority OR label:blocker)",
494+
"page": "1",
495+
"per_page": "30",
496+
},
497+
).andThen(
498+
mockResponse(t, http.StatusOK, mockSearchResult),
499+
),
500+
),
501+
),
502+
requestArgs: map[string]interface{}{
503+
"query": "repo:github/github-mcp-server is:issue (label:critical OR label:urgent OR label:high-priority OR label:blocker)",
504+
},
505+
expectError: false,
506+
expectedResult: mockSearchResult,
507+
},
414508
{
415509
name: "search issues fails",
416510
mockedClient: mock.NewMockedHTTPClient(

‎pkg/github/pullrequests_test.go

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1030,6 +1030,77 @@ func Test_SearchPullRequests(t *testing.T) {
10301030
expectError: false,
10311031
expectedResult: mockSearchResult,
10321032
},
1033+
{
1034+
name: "query with existing is:pr filter - no duplication",
1035+
mockedClient: mock.NewMockedHTTPClient(
1036+
mock.WithRequestMatchHandler(
1037+
mock.GetSearchIssues,
1038+
expectQueryParams(
1039+
t,
1040+
map[string]string{
1041+
"q": "is:pr repo:github/github-mcp-server is:open draft:false",
1042+
"page": "1",
1043+
"per_page": "30",
1044+
},
1045+
).andThen(
1046+
mockResponse(t, http.StatusOK, mockSearchResult),
1047+
),
1048+
),
1049+
),
1050+
requestArgs: map[string]interface{}{
1051+
"query": "is:pr repo:github/github-mcp-server is:open draft:false",
1052+
},
1053+
expectError: false,
1054+
expectedResult: mockSearchResult,
1055+
},
1056+
{
1057+
name: "query with existing repo: filter and conflicting owner/repo params - uses query filter",
1058+
mockedClient: mock.NewMockedHTTPClient(
1059+
mock.WithRequestMatchHandler(
1060+
mock.GetSearchIssues,
1061+
expectQueryParams(
1062+
t,
1063+
map[string]string{
1064+
"q": "is:pr repo:github/github-mcp-server author:octocat",
1065+
"page": "1",
1066+
"per_page": "30",
1067+
},
1068+
).andThen(
1069+
mockResponse(t, http.StatusOK, mockSearchResult),
1070+
),
1071+
),
1072+
),
1073+
requestArgs: map[string]interface{}{
1074+
"query": "repo:github/github-mcp-server author:octocat",
1075+
"owner": "different-owner",
1076+
"repo": "different-repo",
1077+
},
1078+
expectError: false,
1079+
expectedResult: mockSearchResult,
1080+
},
1081+
{
1082+
name: "complex query with existing is:pr filter and OR operators",
1083+
mockedClient: mock.NewMockedHTTPClient(
1084+
mock.WithRequestMatchHandler(
1085+
mock.GetSearchIssues,
1086+
expectQueryParams(
1087+
t,
1088+
map[string]string{
1089+
"q": "is:pr repo:github/github-mcp-server (label:bug OR label:enhancement OR label:feature)",
1090+
"page": "1",
1091+
"per_page": "30",
1092+
},
1093+
).andThen(
1094+
mockResponse(t, http.StatusOK, mockSearchResult),
1095+
),
1096+
),
1097+
),
1098+
requestArgs: map[string]interface{}{
1099+
"query": "is:pr repo:github/github-mcp-server (label:bug OR label:enhancement OR label:feature)",
1100+
},
1101+
expectError: false,
1102+
expectedResult: mockSearchResult,
1103+
},
10331104
{
10341105
name: "search pull requests fails",
10351106
mockedClient: mock.NewMockedHTTPClient(

‎pkg/github/search.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,10 @@ func userOrOrgHandler(accountType string, getClient GetClientFn) server.ToolHand
204204
return nil, fmt.Errorf("failed to get GitHub client: %w", err)
205205
}
206206

207-
searchQuery := "type:" + accountType + " " + query
207+
searchQuery := query
208+
if !hasTypeFilter(query) {
209+
searchQuery = "type:" + accountType + " " + query
210+
}
208211
result, resp, err := client.Search.Users(ctx, searchQuery, opts)
209212
if err != nil {
210213
return ghErrors.NewGitHubAPIErrorResponse(ctx,

‎pkg/github/search_test.go

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,46 @@ func Test_SearchUsers(t *testing.T) {
410410
expectError: false,
411411
expectedResult: mockSearchResult,
412412
},
413+
{
414+
name: "query with existing type:user filter - no duplication",
415+
mockedClient: mock.NewMockedHTTPClient(
416+
mock.WithRequestMatchHandler(
417+
mock.GetSearchUsers,
418+
expectQueryParams(t, map[string]string{
419+
"q": "type:user location:seattle followers:>100",
420+
"page": "1",
421+
"per_page": "30",
422+
}).andThen(
423+
mockResponse(t, http.StatusOK, mockSearchResult),
424+
),
425+
),
426+
),
427+
requestArgs: map[string]interface{}{
428+
"query": "type:user location:seattle followers:>100",
429+
},
430+
expectError: false,
431+
expectedResult: mockSearchResult,
432+
},
433+
{
434+
name: "complex query with existing type:user filter and OR operators",
435+
mockedClient: mock.NewMockedHTTPClient(
436+
mock.WithRequestMatchHandler(
437+
mock.GetSearchUsers,
438+
expectQueryParams(t, map[string]string{
439+
"q": "type:user (location:seattle OR location:california) followers:>50",
440+
"page": "1",
441+
"per_page": "30",
442+
}).andThen(
443+
mockResponse(t, http.StatusOK, mockSearchResult),
444+
),
445+
),
446+
),
447+
requestArgs: map[string]interface{}{
448+
"query": "type:user (location:seattle OR location:california) followers:>50",
449+
},
450+
expectError: false,
451+
expectedResult: mockSearchResult,
452+
},
413453
{
414454
name: "search users fails",
415455
mockedClient: mock.NewMockedHTTPClient(
@@ -537,6 +577,46 @@ func Test_SearchOrgs(t *testing.T) {
537577
expectError: false,
538578
expectedResult: mockSearchResult,
539579
},
580+
{
581+
name: "query with existing type:org filter - no duplication",
582+
mockedClient: mock.NewMockedHTTPClient(
583+
mock.WithRequestMatchHandler(
584+
mock.GetSearchUsers,
585+
expectQueryParams(t, map[string]string{
586+
"q": "type:org location:california followers:>1000",
587+
"page": "1",
588+
"per_page": "30",
589+
}).andThen(
590+
mockResponse(t, http.StatusOK, mockSearchResult),
591+
),
592+
),
593+
),
594+
requestArgs: map[string]interface{}{
595+
"query": "type:org location:california followers:>1000",
596+
},
597+
expectError: false,
598+
expectedResult: mockSearchResult,
599+
},
600+
{
601+
name: "complex query with existing type:org filter and OR operators",
602+
mockedClient: mock.NewMockedHTTPClient(
603+
mock.WithRequestMatchHandler(
604+
mock.GetSearchUsers,
605+
expectQueryParams(t, map[string]string{
606+
"q": "type:org (location:seattle OR location:california OR location:newyork) repos:>10",
607+
"page": "1",
608+
"per_page": "30",
609+
}).andThen(
610+
mockResponse(t, http.StatusOK, mockSearchResult),
611+
),
612+
),
613+
),
614+
requestArgs: map[string]interface{}{
615+
"query": "type:org (location:seattle OR location:california OR location:newyork) repos:>10",
616+
},
617+
expectError: false,
618+
expectedResult: mockSearchResult,
619+
},
540620
{
541621
name: "org search fails",
542622
mockedClient: mock.NewMockedHTTPClient(

‎pkg/github/search_utils.go

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,35 @@ import (
66
"fmt"
77
"io"
88
"net/http"
9+
"regexp"
910

1011
"github.com/google/go-github/v74/github"
1112
"github.com/mark3labs/mcp-go/mcp"
1213
)
1314

15+
func hasFilter(query, filterType string) bool {
16+
// Match filter at start of string, after whitespace, or after non-word characters like '('
17+
pattern := fmt.Sprintf(`(^|\s|\W)%s:\S+`, regexp.QuoteMeta(filterType))
18+
matched, _ := regexp.MatchString(pattern, query)
19+
return matched
20+
}
21+
22+
func hasSpecificFilter(query, filterType, filterValue string) bool {
23+
// Match specific filter:value at start, after whitespace, or after non-word characters
24+
// End with word boundary, whitespace, or non-word characters like ')'
25+
pattern := fmt.Sprintf(`(^|\s|\W)%s:%s($|\s|\W)`, regexp.QuoteMeta(filterType), regexp.QuoteMeta(filterValue))
26+
matched, _ := regexp.MatchString(pattern, query)
27+
return matched
28+
}
29+
30+
func hasRepoFilter(query string) bool {
31+
return hasFilter(query, "repo")
32+
}
33+
34+
func hasTypeFilter(query string) bool {
35+
return hasFilter(query, "type")
36+
}
37+
1438
func searchHandler(
1539
ctx context.Context,
1640
getClient GetClientFn,
@@ -22,7 +46,10 @@ func searchHandler(
2246
if err != nil {
2347
return mcp.NewToolResultError(err.Error()), nil
2448
}
25-
query = fmt.Sprintf("is:%s %s", searchType, query)
49+
50+
if !hasSpecificFilter(query, "is", searchType) {
51+
query = fmt.Sprintf("is:%s %s", searchType, query)
52+
}
2653

2754
owner, err := OptionalParam[string](request, "owner")
2855
if err != nil {
@@ -34,7 +61,7 @@ func searchHandler(
3461
return mcp.NewToolResultError(err.Error()), nil
3562
}
3663

37-
if owner != "" && repo != "" {
64+
if owner != "" && repo != "" && !hasRepoFilter(query) {
3865
query = fmt.Sprintf("repo:%s/%s %s", owner, repo, query)
3966
}
4067

0 commit comments

Comments
 (0)