Skip to content

Commit 2f95120

Browse files
mattdhollowaymattdholloway
committed
initial pagination for ListDiscussions 
1 parent d15026b commit 2f95120

File tree

3 files changed

+193
-57
lines changed

3 files changed

+193
-57
lines changed

‎pkg/github/discussions.go‎

Lines changed: 61 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp
3131
mcp.WithString("category",
3232
mcp.Description("Optional filter by discussion category ID. If provided, only discussions with this category are listed."),
3333
),
34+
WithGraphQLPagination(),
3435
),
3536
func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
3637
// Required params
@@ -49,6 +50,12 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp
4950
return mcp.NewToolResultError(err.Error()), nil
5051
}
5152

53+
// Get pagination parameters
54+
pagination, err := OptionalGraphQLPaginationParams(request)
55+
if err != nil {
56+
return mcp.NewToolResultError(err.Error()), nil
57+
}
58+
5259
client, err := getGQLClient(ctx)
5360
if err != nil {
5461
return mcp.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil
@@ -61,7 +68,13 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp
6168
categoryID = &id
6269
}
6370

64-
// Now execute the discussions query
71+
// Build GraphQL query arguments
72+
// Use default of 30 if neither first nor last is specified
73+
if pagination.First == nil && pagination.Last == nil {
74+
defaultFirst := int32(30)
75+
pagination.First = &defaultFirst
76+
}
77+
6578
var discussions []*github.Discussion
6679
if categoryID != nil {
6780
// Query with category filter (server-side filtering)
@@ -77,13 +90,21 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp
7790
} `graphql:"category"`
7891
URL githubv4.String `graphql:"url"`
7992
}
80-
} `graphql:"discussions(first: 100, categoryId: $categoryId)"`
93+
PageInfo struct {
94+
HasNextPage bool
95+
EndCursor string
96+
}
97+
} `graphql:"discussions(first: $first, last: $last, after: $after, before: $before, categoryId: $categoryId)"`
8198
} `graphql:"repository(owner: $owner, name: $repo)"`
8299
}
83100
vars := map[string]interface{}{
84101
"owner": githubv4.String(owner),
85102
"repo": githubv4.String(repo),
86103
"categoryId": *categoryID,
104+
"first": (*githubv4.Int)(pagination.First),
105+
"last": (*githubv4.Int)(pagination.Last),
106+
"after": (*githubv4.String)(pagination.After),
107+
"before": (*githubv4.String)(pagination.Before),
87108
}
88109
if err := client.Query(ctx, &query, vars); err != nil {
89110
return mcp.NewToolResultError(err.Error()), nil
@@ -102,6 +123,20 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp
102123
}
103124
discussions = append(discussions, di)
104125
}
126+
127+
// Include pagination info in response
128+
response := map[string]interface{}{
129+
"discussions": discussions,
130+
"pageInfo": map[string]interface{}{
131+
"hasNextPage": query.Repository.Discussions.PageInfo.HasNextPage,
132+
"endCursor": query.Repository.Discussions.PageInfo.EndCursor,
133+
},
134+
}
135+
out, err := json.Marshal(response)
136+
if err != nil {
137+
return nil, fmt.Errorf("failed to marshal discussions: %w", err)
138+
}
139+
return mcp.NewToolResultText(string(out)), nil
105140
} else {
106141
// Query without category filter
107142
var query struct {
@@ -116,12 +151,20 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp
116151
} `graphql:"category"`
117152
URL githubv4.String `graphql:"url"`
118153
}
119-
} `graphql:"discussions(first: 100)"`
154+
PageInfo struct {
155+
HasNextPage bool
156+
EndCursor string
157+
}
158+
} `graphql:"discussions(first: $first, last: $last, after: $after, before: $before)"`
120159
} `graphql:"repository(owner: $owner, name: $repo)"`
121160
}
122161
vars := map[string]interface{}{
123-
"owner": githubv4.String(owner),
124-
"repo": githubv4.String(repo),
162+
"owner": githubv4.String(owner),
163+
"repo": githubv4.String(repo),
164+
"first": (*githubv4.Int)(pagination.First),
165+
"last": (*githubv4.Int)(pagination.Last),
166+
"after": (*githubv4.String)(pagination.After),
167+
"before": (*githubv4.String)(pagination.Before),
125168
}
126169
if err := client.Query(ctx, &query, vars); err != nil {
127170
return mcp.NewToolResultError(err.Error()), nil
@@ -140,14 +183,21 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp
140183
}
141184
discussions = append(discussions, di)
142185
}
143-
}
144186

145-
// Marshal and return
146-
out, err := json.Marshal(discussions)
147-
if err != nil {
148-
return nil, fmt.Errorf("failed to marshal discussions: %w", err)
187+
// Include pagination info in response
188+
response := map[string]interface{}{
189+
"discussions": discussions,
190+
"pageInfo": map[string]interface{}{
191+
"hasNextPage": query.Repository.Discussions.PageInfo.HasNextPage,
192+
"endCursor": query.Repository.Discussions.PageInfo.EndCursor,
193+
},
194+
}
195+
out, err := json.Marshal(response)
196+
if err != nil {
197+
return nil, fmt.Errorf("failed to marshal discussions: %w", err)
198+
}
199+
return mcp.NewToolResultText(string(out)), nil
149200
}
150-
return mcp.NewToolResultText(string(out)), nil
151201
}
152202
}
153203

‎pkg/github/discussions_test.go‎

Lines changed: 50 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,24 @@ var (
2727
}
2828
mockResponseListAll = githubv4mock.DataResponse(map[string]any{
2929
"repository": map[string]any{
30-
"discussions": map[string]any{"nodes": discussionsAll},
30+
"discussions": map[string]any{
31+
"nodes": discussionsAll,
32+
"pageInfo": map[string]any{
33+
"hasNextPage": false,
34+
"endCursor": "",
35+
},
36+
},
3137
},
3238
})
3339
mockResponseListGeneral = githubv4mock.DataResponse(map[string]any{
3440
"repository": map[string]any{
35-
"discussions": map[string]any{"nodes": discussionsGeneral},
41+
"discussions": map[string]any{
42+
"nodes": discussionsGeneral,
43+
"pageInfo": map[string]any{
44+
"hasNextPage": false,
45+
"endCursor": "",
46+
},
47+
},
3648
},
3749
})
3850
mockErrorRepoNotFound = githubv4mock.ErrorResponse("repository not found")
@@ -48,54 +60,38 @@ func Test_ListDiscussions(t *testing.T) {
4860
assert.Contains(t, toolDef.InputSchema.Properties, "repo")
4961
assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo"})
5062

51-
// mock for the call to ListDiscussions without category filter
52-
var qDiscussions struct {
53-
Repository struct {
54-
Discussions struct {
55-
Nodes []struct {
56-
Number githubv4.Int
57-
Title githubv4.String
58-
CreatedAt githubv4.DateTime
59-
Category struct {
60-
Name githubv4.String
61-
} `graphql:"category"`
62-
URL githubv4.String `graphql:"url"`
63-
}
64-
} `graphql:"discussions(first: 100)"`
65-
} `graphql:"repository(owner: $owner, name: $repo)"`
66-
}
63+
// Use exact string queries that match implementation output (from error messages)
64+
qDiscussions := "query($after:String$before:String$first:Int$last:Int$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, last: $last, after: $after, before: $before){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}"
6765

68-
// mock for the call to get discussions with category filter
69-
var qDiscussionsFiltered struct {
70-
Repository struct {
71-
Discussions struct {
72-
Nodes []struct {
73-
Number githubv4.Int
74-
Title githubv4.String
75-
CreatedAt githubv4.DateTime
76-
Category struct {
77-
Name githubv4.String
78-
} `graphql:"category"`
79-
URL githubv4.String `graphql:"url"`
80-
}
81-
} `graphql:"discussions(first: 100, categoryId: $categoryId)"`
82-
} `graphql:"repository(owner: $owner, name: $repo)"`
83-
}
66+
qDiscussionsFiltered := "query($after:String$before:String$categoryId:ID!$first:Int$last:Int$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, last: $last, after: $after, before: $before, categoryId: $categoryId){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}"
8467

68+
// Variables matching what GraphQL receives after JSON marshaling/unmarshaling
8569
varsListAll := map[string]interface{}{
86-
"owner": githubv4.String("owner"),
87-
"repo": githubv4.String("repo"),
70+
"owner": "owner",
71+
"repo": "repo",
72+
"first": float64(30),
73+
"last": nil,
74+
"after": nil,
75+
"before": nil,
8876
}
8977

9078
varsRepoNotFound := map[string]interface{}{
91-
"owner": githubv4.String("owner"),
92-
"repo": githubv4.String("nonexistent-repo"),
79+
"owner": "owner",
80+
"repo": "nonexistent-repo",
81+
"first": float64(30),
82+
"last": nil,
83+
"after": nil,
84+
"before": nil,
9385
}
9486

9587
varsDiscussionsFiltered := map[string]interface{}{
96-
"owner": githubv4.String("owner"),
97-
"repo": githubv4.String("repo"),
98-
"categoryId": githubv4.ID("DIC_kwDOABC123"),
88+
"owner": "owner",
89+
"repo": "repo",
90+
"categoryId": "DIC_kwDOABC123",
91+
"first": float64(30),
92+
"last": nil,
93+
"after": nil,
94+
"before": nil,
9995
}
10096

10197
tests := []struct {
@@ -167,10 +163,21 @@ func Test_ListDiscussions(t *testing.T) {
167163
}
168164
require.NoError(t, err)
169165

170-
var returnedDiscussions []*github.Discussion
171-
err = json.Unmarshal([]byte(text), &returnedDiscussions)
166+
// Debug: print the actual response
167+
t.Logf("Actual response text: %q", text)
168+
169+
// Parse the structured response with pagination info
170+
var response struct {
171+
Discussions []*github.Discussion `json:"discussions"`
172+
PageInfo struct {
173+
HasNextPage bool `json:"hasNextPage"`
174+
EndCursor string `json:"endCursor"`
175+
} `json:"pageInfo"`
176+
}
177+
err = json.Unmarshal([]byte(text), &response)
172178
require.NoError(t, err)
173179

180+
returnedDiscussions := response.Discussions
174181
assert.Len(t, returnedDiscussions, tc.expectedCount, "Expected %d discussions, got %d", tc.expectedCount, len(returnedDiscussions))
175182

176183
// Verify that all returned discussions have a category if filtered

‎pkg/github/server.go‎

Lines changed: 82 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -174,9 +174,7 @@ func OptionalStringArrayParam(r mcp.CallToolRequest, p string) ([]string, error)
174174
}
175175
}
176176

177-
// WithPagination returns a ToolOption that adds "page" and "perPage" parameters to the tool.
178-
// The "page" parameter is optional, min 1.
179-
// The "perPage" parameter is optional, min 1, max 100. If unset, defaults to 30.
177+
// WithPagination adds REST API pagination parameters to a tool.
180178
// https://docs.github.com/en/rest/using-the-rest-api/using-pagination-in-the-rest-api
181179
func WithPagination() mcp.ToolOption {
182180
return func(tool *mcp.Tool) {
@@ -193,6 +191,32 @@ func WithPagination() mcp.ToolOption {
193191
}
194192
}
195193

194+
// WithGraphQLPagination adds GraphQL cursor-based pagination parameters to a tool.
195+
// https://docs.github.com/en/graphql/reference/objects#connection
196+
func WithGraphQLPagination() mcp.ToolOption {
197+
return func(tool *mcp.Tool) {
198+
mcp.WithNumber("first",
199+
mcp.Description("Number of items to return per page (min 1, max 100)"),
200+
mcp.Min(1),
201+
mcp.Max(100),
202+
)(tool)
203+
204+
mcp.WithNumber("last",
205+
mcp.Description("Number of items to return from the end (min 1, max 100)"),
206+
mcp.Min(1),
207+
mcp.Max(100),
208+
)(tool)
209+
210+
mcp.WithString("after",
211+
mcp.Description("Cursor for pagination, use the 'after' field from the previous response"),
212+
)(tool)
213+
214+
mcp.WithString("before",
215+
mcp.Description("Cursor for pagination, use the 'before' field from the previous response"),
216+
)(tool)
217+
}
218+
}
219+
196220
type PaginationParams struct {
197221
page int
198222
perPage int
@@ -218,6 +242,61 @@ func OptionalPaginationParams(r mcp.CallToolRequest) (PaginationParams, error) {
218242
}, nil
219243
}
220244

245+
type GraphQLPaginationParams struct {
246+
First *int32
247+
Last *int32
248+
After *string
249+
Before *string
250+
}
251+
252+
// OptionalGraphQLPaginationParams returns the GraphQL cursor-based pagination parameters from the request.
253+
// It validates that the parameters are used correctly according to GraphQL connection spec.
254+
func OptionalGraphQLPaginationParams(r mcp.CallToolRequest) (GraphQLPaginationParams, error) {
255+
var params GraphQLPaginationParams
256+
257+
if val, err := OptionalParam[float64](r, "first"); err != nil {
258+
return GraphQLPaginationParams{}, err
259+
} else if val != 0 {
260+
first := int32(val)
261+
params.First = &first
262+
}
263+
264+
if val, err := OptionalParam[float64](r, "last"); err != nil {
265+
return GraphQLPaginationParams{}, err
266+
} else if val != 0 {
267+
last := int32(val)
268+
params.Last = &last
269+
}
270+
271+
if val, err := OptionalParam[string](r, "after"); err != nil {
272+
return GraphQLPaginationParams{}, err
273+
} else if val != "" {
274+
params.After = &val
275+
}
276+
277+
if val, err := OptionalParam[string](r, "before"); err != nil {
278+
return GraphQLPaginationParams{}, err
279+
} else if val != "" {
280+
params.Before = &val
281+
}
282+
283+
// Validate pagination parameters according to GraphQL connection spec
284+
if params.First != nil && params.Last != nil {
285+
return GraphQLPaginationParams{}, fmt.Errorf("only one of 'first' or 'last' may be specified")
286+
}
287+
if params.After != nil && params.Before != nil {
288+
return GraphQLPaginationParams{}, fmt.Errorf("only one of 'after' or 'before' may be specified")
289+
}
290+
if params.After != nil && params.Last != nil {
291+
return GraphQLPaginationParams{}, fmt.Errorf("'after' cannot be used with 'last'. Did you mean to use 'before' instead?")
292+
}
293+
if params.Before != nil && params.First != nil {
294+
return GraphQLPaginationParams{}, fmt.Errorf("'before' cannot be used with 'first'. Did you mean to use 'after' instead?")
295+
}
296+
297+
return params, nil
298+
}
299+
221300
func MarshalledTextResult(v any) *mcp.CallToolResult {
222301
data, err := json.Marshal(v)
223302
if err != nil {

0 commit comments

Comments
 (0)