Skip to content

Commit 3c97c6b

Browse files
gokhanarkangokhanarkan
committed
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.
1 parent a9c00e1 commit 3c97c6b

File tree

5 files changed

+215
-1
lines changed

5 files changed

+215
-1
lines changed

‎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: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ func hasRepoFilter(query string) bool {
2828
return hasFilter(query, "repo")
2929
}
3030

31+
func hasTypeFilter(query string) bool {
32+
return hasFilter(query, "type")
33+
}
34+
3135
func searchHandler(
3236
ctx context.Context,
3337
getClient GetClientFn,

‎pkg/github/search_utils_test.go

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -268,3 +268,59 @@ func Test_hasSpecificFilter(t *testing.T) {
268268
})
269269
}
270270
}
271+
272+
func Test_hasTypeFilter(t *testing.T) {
273+
tests := []struct {
274+
name string
275+
query string
276+
expected bool
277+
}{
278+
{
279+
name: "query with type:user filter at beginning",
280+
query: "type:user location:seattle",
281+
expected: true,
282+
},
283+
{
284+
name: "query with type:org filter in middle",
285+
query: "location:california type:org followers:>100",
286+
expected: true,
287+
},
288+
{
289+
name: "query with type:user filter at end",
290+
query: "location:seattle followers:>50 type:user",
291+
expected: true,
292+
},
293+
{
294+
name: "query without type: filter",
295+
query: "location:seattle followers:>50",
296+
expected: false,
297+
},
298+
{
299+
name: "empty query",
300+
query: "",
301+
expected: false,
302+
},
303+
{
304+
name: "query with type: in text but not as filter",
305+
query: "this type: is important",
306+
expected: false,
307+
},
308+
{
309+
name: "query with multiple type: filters",
310+
query: "type:user type:org",
311+
expected: true,
312+
},
313+
{
314+
name: "complex query with OR expression",
315+
query: "type:user (location:seattle OR location:california)",
316+
expected: true,
317+
},
318+
}
319+
320+
for _, tt := range tests {
321+
t.Run(tt.name, func(t *testing.T) {
322+
result := hasTypeFilter(tt.query)
323+
assert.Equal(t, tt.expected, result, "hasTypeFilter(%q) = %v, expected %v", tt.query, result, tt.expected)
324+
})
325+
}
326+
}

0 commit comments

Comments
 (0)