diff --git a/README.md b/README.md index 48bf019d..cf53ce2a 100644 --- a/README.md +++ b/README.md @@ -204,7 +204,7 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description - `sort`: Sort field (string, optional) - `order`: Sort order (string, optional) - `page`: Page number (number, optional) - - `per_page`: Results per page (number, optional) + - `perPage`: Results per page (number, optional) ### Pull Requests diff --git a/pkg/github/issues.go b/pkg/github/issues.go index d5aba2e7..1632e9e8 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -162,15 +162,7 @@ func searchIssues(client *github.Client, t translations.TranslationHelperFunc) ( mcp.Description("Sort order ('asc' or 'desc')"), mcp.Enum("asc", "desc"), ), - mcp.WithNumber("per_page", - mcp.Description("Results per page (max 100)"), - mcp.Min(1), - mcp.Max(100), - ), - mcp.WithNumber("page", - mcp.Description("Page number"), - mcp.Min(1), - ), + withPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { query, err := requiredParam[string](request, "q") @@ -185,11 +177,7 @@ func searchIssues(client *github.Client, t translations.TranslationHelperFunc) ( if err != nil { return mcp.NewToolResultError(err.Error()), nil } - perPage, err := optionalIntParamWithDefault(request, "per_page", 30) - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - page, err := optionalIntParamWithDefault(request, "page", 1) + pagination, err := optionalPaginationParams(request) if err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -198,8 +186,8 @@ func searchIssues(client *github.Client, t translations.TranslationHelperFunc) ( Sort: sort, Order: order, ListOptions: github.ListOptions{ - PerPage: perPage, - Page: page, + PerPage: pagination.perPage, + Page: pagination.page, }, } @@ -375,12 +363,7 @@ func listIssues(client *github.Client, t translations.TranslationHelperFunc) (to mcp.WithString("since", mcp.Description("Filter by date (ISO 8601 timestamp)"), ), - mcp.WithNumber("page", - mcp.Description("Page number"), - ), - mcp.WithNumber("per_page", - mcp.Description("Results per page"), - ), + withPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { owner, err := requiredParam[string](request, "owner") @@ -432,7 +415,7 @@ func listIssues(client *github.Client, t translations.TranslationHelperFunc) (to opts.Page = int(page) } - if perPage, ok := request.Params.Arguments["per_page"].(float64); ok { + if perPage, ok := request.Params.Arguments["perPage"].(float64); ok { opts.PerPage = int(perPage) } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 0326b9be..485169fd 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -244,7 +244,7 @@ func Test_SearchIssues(t *testing.T) { assert.Contains(t, tool.InputSchema.Properties, "q") assert.Contains(t, tool.InputSchema.Properties, "sort") assert.Contains(t, tool.InputSchema.Properties, "order") - assert.Contains(t, tool.InputSchema.Properties, "per_page") + assert.Contains(t, tool.InputSchema.Properties, "perPage") assert.Contains(t, tool.InputSchema.Properties, "page") assert.ElementsMatch(t, tool.InputSchema.Required, []string{"q"}) @@ -289,17 +289,28 @@ func Test_SearchIssues(t *testing.T) { { name: "successful issues search with all parameters", mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatch( + mock.WithRequestMatchHandler( mock.GetSearchIssues, - mockSearchResult, + expectQueryParams( + t, + map[string]string{ + "q": "repo:owner/repo is:issue is:open", + "sort": "created", + "order": "desc", + "page": "1", + "per_page": "30", + }, + ).andThen( + mockResponse(t, http.StatusOK, mockSearchResult), + ), ), ), requestArgs: map[string]interface{}{ - "q": "repo:owner/repo is:issue is:open", - "sort": "created", - "order": "desc", - "page": float64(1), - "per_page": float64(30), + "q": "repo:owner/repo is:issue is:open", + "sort": "created", + "order": "desc", + "page": float64(1), + "perPage": float64(30), }, expectError: false, expectedResult: mockSearchResult, @@ -567,7 +578,7 @@ func Test_ListIssues(t *testing.T) { assert.Contains(t, tool.InputSchema.Properties, "direction") assert.Contains(t, tool.InputSchema.Properties, "since") assert.Contains(t, tool.InputSchema.Properties, "page") - assert.Contains(t, tool.InputSchema.Properties, "per_page") + assert.Contains(t, tool.InputSchema.Properties, "perPage") assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo"}) // Setup mock issues for success case @@ -641,7 +652,7 @@ func Test_ListIssues(t *testing.T) { "direction": "desc", "since": "2023-01-01T00:00:00Z", "page": float64(1), - "per_page": float64(30), + "perPage": float64(30), }, expectError: false, expectedIssues: mockIssues, diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index a43d5b88..25090cb7 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -94,12 +94,7 @@ func listPullRequests(client *github.Client, t translations.TranslationHelperFun mcp.WithString("direction", mcp.Description("Sort direction ('asc', 'desc')"), ), - mcp.WithNumber("per_page", - mcp.Description("Results per page (max 100)"), - ), - mcp.WithNumber("page", - mcp.Description("Page number"), - ), + withPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { owner, err := requiredParam[string](request, "owner") @@ -130,11 +125,7 @@ func listPullRequests(client *github.Client, t translations.TranslationHelperFun if err != nil { return mcp.NewToolResultError(err.Error()), nil } - perPage, err := optionalIntParamWithDefault(request, "per_page", 30) - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - page, err := optionalIntParamWithDefault(request, "page", 1) + pagination, err := optionalPaginationParams(request) if err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -146,8 +137,8 @@ func listPullRequests(client *github.Client, t translations.TranslationHelperFun Sort: sort, Direction: direction, ListOptions: github.ListOptions{ - PerPage: perPage, - Page: page, + PerPage: pagination.perPage, + Page: pagination.page, }, } diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 9e2e9f47..cf1afcdc 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -140,7 +140,7 @@ func Test_ListPullRequests(t *testing.T) { assert.Contains(t, tool.InputSchema.Properties, "base") assert.Contains(t, tool.InputSchema.Properties, "sort") assert.Contains(t, tool.InputSchema.Properties, "direction") - assert.Contains(t, tool.InputSchema.Properties, "per_page") + assert.Contains(t, tool.InputSchema.Properties, "perPage") assert.Contains(t, tool.InputSchema.Properties, "page") assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo"}) @@ -190,7 +190,7 @@ func Test_ListPullRequests(t *testing.T) { "state": "all", "sort": "created", "direction": "desc", - "per_page": float64(30), + "perPage": float64(30), "page": float64(1), }, expectError: false, diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 9c2ec3d8..5b8725d1 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -28,12 +28,7 @@ func listCommits(client *github.Client, t translations.TranslationHelperFunc) (t mcp.WithString("sha", mcp.Description("Branch name"), ), - mcp.WithNumber("page", - mcp.Description("Page number"), - ), - mcp.WithNumber("perPage", - mcp.Description("Number of records per page"), - ), + withPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { owner, err := requiredParam[string](request, "owner") @@ -48,11 +43,7 @@ func listCommits(client *github.Client, t translations.TranslationHelperFunc) (t if err != nil { return mcp.NewToolResultError(err.Error()), nil } - page, err := optionalIntParamWithDefault(request, "page", 1) - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - perPage, err := optionalIntParamWithDefault(request, "per_page", 30) + pagination, err := optionalPaginationParams(request) if err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -60,8 +51,8 @@ func listCommits(client *github.Client, t translations.TranslationHelperFunc) (t opts := &github.CommitsListOptions{ SHA: sha, ListOptions: github.ListOptions{ - Page: page, - PerPage: perPage, + Page: pagination.page, + PerPage: pagination.perPage, }, } diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index bb6579f8..f7ed8e71 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -582,10 +582,10 @@ func Test_ListCommits(t *testing.T) { ), ), requestArgs: map[string]interface{}{ - "owner": "owner", - "repo": "repo", - "page": float64(2), - "per_page": float64(10), + "owner": "owner", + "repo": "repo", + "page": float64(2), + "perPage": float64(10), }, expectError: false, expectedCommits: mockCommits, diff --git a/pkg/github/search.go b/pkg/github/search.go index a98c2643..117e8298 100644 --- a/pkg/github/search.go +++ b/pkg/github/search.go @@ -20,31 +20,22 @@ func searchRepositories(client *github.Client, t translations.TranslationHelperF mcp.Required(), mcp.Description("Search query"), ), - mcp.WithNumber("page", - mcp.Description("Page number for pagination"), - ), - mcp.WithNumber("perPage", - mcp.Description("Results per page (max 100)"), - ), + withPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { query, err := requiredParam[string](request, "query") if err != nil { return mcp.NewToolResultError(err.Error()), nil } - page, err := optionalIntParamWithDefault(request, "page", 1) - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - perPage, err := optionalIntParamWithDefault(request, "perPage", 30) + pagination, err := optionalPaginationParams(request) if err != nil { return mcp.NewToolResultError(err.Error()), nil } opts := &github.SearchOptions{ ListOptions: github.ListOptions{ - Page: page, - PerPage: perPage, + Page: pagination.page, + PerPage: pagination.perPage, }, } @@ -86,15 +77,7 @@ func searchCode(client *github.Client, t translations.TranslationHelperFunc) (to mcp.Description("Sort order ('asc' or 'desc')"), mcp.Enum("asc", "desc"), ), - mcp.WithNumber("per_page", - mcp.Description("Results per page (max 100)"), - mcp.Min(1), - mcp.Max(100), - ), - mcp.WithNumber("page", - mcp.Description("Page number"), - mcp.Min(1), - ), + withPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { query, err := requiredParam[string](request, "q") @@ -109,11 +92,7 @@ func searchCode(client *github.Client, t translations.TranslationHelperFunc) (to if err != nil { return mcp.NewToolResultError(err.Error()), nil } - perPage, err := optionalIntParamWithDefault(request, "per_page", 30) - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - page, err := optionalIntParamWithDefault(request, "page", 1) + pagination, err := optionalPaginationParams(request) if err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -122,8 +101,8 @@ func searchCode(client *github.Client, t translations.TranslationHelperFunc) (to Sort: sort, Order: order, ListOptions: github.ListOptions{ - PerPage: perPage, - Page: page, + PerPage: pagination.perPage, + Page: pagination.page, }, } @@ -166,15 +145,7 @@ func searchUsers(client *github.Client, t translations.TranslationHelperFunc) (t mcp.Description("Sort order ('asc' or 'desc')"), mcp.Enum("asc", "desc"), ), - mcp.WithNumber("per_page", - mcp.Description("Results per page (max 100)"), - mcp.Min(1), - mcp.Max(100), - ), - mcp.WithNumber("page", - mcp.Description("Page number"), - mcp.Min(1), - ), + withPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { query, err := requiredParam[string](request, "q") @@ -189,11 +160,7 @@ func searchUsers(client *github.Client, t translations.TranslationHelperFunc) (t if err != nil { return mcp.NewToolResultError(err.Error()), nil } - perPage, err := optionalIntParamWithDefault(request, "per_page", 30) - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - page, err := optionalIntParamWithDefault(request, "page", 1) + pagination, err := optionalPaginationParams(request) if err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -202,8 +169,8 @@ func searchUsers(client *github.Client, t translations.TranslationHelperFunc) (t Sort: sort, Order: order, ListOptions: github.ListOptions{ - PerPage: perPage, - Page: page, + PerPage: pagination.perPage, + Page: pagination.page, }, } diff --git a/pkg/github/search_test.go b/pkg/github/search_test.go index 9d02b3a2..bf1bff45 100644 --- a/pkg/github/search_test.go +++ b/pkg/github/search_test.go @@ -170,7 +170,7 @@ func Test_SearchCode(t *testing.T) { assert.Contains(t, tool.InputSchema.Properties, "q") assert.Contains(t, tool.InputSchema.Properties, "sort") assert.Contains(t, tool.InputSchema.Properties, "order") - assert.Contains(t, tool.InputSchema.Properties, "per_page") + assert.Contains(t, tool.InputSchema.Properties, "perPage") assert.Contains(t, tool.InputSchema.Properties, "page") assert.ElementsMatch(t, tool.InputSchema.Required, []string{"q"}) @@ -221,11 +221,11 @@ func Test_SearchCode(t *testing.T) { ), ), requestArgs: map[string]interface{}{ - "q": "fmt.Println language:go", - "sort": "indexed", - "order": "desc", - "page": float64(1), - "per_page": float64(30), + "q": "fmt.Println language:go", + "sort": "indexed", + "order": "desc", + "page": float64(1), + "perPage": float64(30), }, expectError: false, expectedResult: mockSearchResult, @@ -321,7 +321,7 @@ func Test_SearchUsers(t *testing.T) { assert.Contains(t, tool.InputSchema.Properties, "q") assert.Contains(t, tool.InputSchema.Properties, "sort") assert.Contains(t, tool.InputSchema.Properties, "order") - assert.Contains(t, tool.InputSchema.Properties, "per_page") + assert.Contains(t, tool.InputSchema.Properties, "perPage") assert.Contains(t, tool.InputSchema.Properties, "page") assert.ElementsMatch(t, tool.InputSchema.Required, []string{"q"}) @@ -376,11 +376,11 @@ func Test_SearchUsers(t *testing.T) { ), ), requestArgs: map[string]interface{}{ - "q": "location:finland language:go", - "sort": "followers", - "order": "desc", - "page": float64(1), - "per_page": float64(30), + "q": "location:finland language:go", + "sort": "followers", + "order": "desc", + "page": float64(1), + "perPage": float64(30), }, expectError: false, expectedResult: mockSearchResult, diff --git a/pkg/github/server.go b/pkg/github/server.go index 18e5da09..228be212 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -230,3 +230,45 @@ func optionalStringArrayParam(r mcp.CallToolRequest, p string) ([]string, error) return []string{}, fmt.Errorf("parameter %s could not be coerced to []string, is %T", p, r.Params.Arguments[p]) } } + +// withPagination returns a ToolOption that adds "page" and "perPage" parameters to the tool. +// The "page" parameter is optional, min 1. The "perPage" parameter is optional, min 1, max 100. +func withPagination() mcp.ToolOption { + return func(tool *mcp.Tool) { + mcp.WithNumber("page", + mcp.Description("Page number for pagination (min 1)"), + mcp.Min(1), + )(tool) + + mcp.WithNumber("perPage", + mcp.Description("Results per page for pagination (min 1, max 100)"), + mcp.Min(1), + mcp.Max(100), + )(tool) + } +} + +type paginationParams struct { + page int + perPage int +} + +// optionalPaginationParams returns the "page" and "perPage" parameters from the request, +// or their default values if not present, "page" default is 1, "perPage" default is 30. +// In future, we may want to make the default values configurable, or even have this +// function returned from `withPagination`, where the defaults are provided alongside +// the min/max values. +func optionalPaginationParams(r mcp.CallToolRequest) (paginationParams, error) { + page, err := optionalIntParamWithDefault(r, "page", 1) + if err != nil { + return paginationParams{}, err + } + perPage, err := optionalIntParamWithDefault(r, "perPage", 30) + if err != nil { + return paginationParams{}, err + } + return paginationParams{ + page: page, + perPage: perPage, + }, nil +} diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index beb6ecbb..149fb77a 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -551,3 +551,86 @@ func TestOptionalStringArrayParam(t *testing.T) { }) } } + +func TestOptionalPaginationParams(t *testing.T) { + tests := []struct { + name string + params map[string]any + expected paginationParams + expectError bool + }{ + { + name: "no pagination parameters, default values", + params: map[string]any{}, + expected: paginationParams{ + page: 1, + perPage: 30, + }, + expectError: false, + }, + { + name: "page parameter, default perPage", + params: map[string]any{ + "page": float64(2), + }, + expected: paginationParams{ + page: 2, + perPage: 30, + }, + expectError: false, + }, + { + name: "perPage parameter, default page", + params: map[string]any{ + "perPage": float64(50), + }, + expected: paginationParams{ + page: 1, + perPage: 50, + }, + expectError: false, + }, + { + name: "page and perPage parameters", + params: map[string]any{ + "page": float64(2), + "perPage": float64(50), + }, + expected: paginationParams{ + page: 2, + perPage: 50, + }, + expectError: false, + }, + { + name: "invalid page parameter", + params: map[string]any{ + "page": "not-a-number", + }, + expected: paginationParams{}, + expectError: true, + }, + { + name: "invalid perPage parameter", + params: map[string]any{ + "perPage": "not-a-number", + }, + expected: paginationParams{}, + expectError: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + request := createMCPRequest(tc.params) + result, err := optionalPaginationParams(request) + + if tc.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.Equal(t, tc.expected, result) + } + }) + } +}