Skip to content

Commit 6bb5daf

Browse files
mattdhollowaymattdholloway
committed
refactor pagination handling
1 parent 5c8b4a7 commit 6bb5daf

File tree

3 files changed

+74
-231
lines changed

3 files changed

+74
-231
lines changed

‎pkg/github/discussions.go‎

Lines changed: 27 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp
5050
return mcp.NewToolResultError(err.Error()), nil
5151
}
5252

53-
// Get unified pagination parameters and convert to GraphQL format
53+
// Get pagination parameters and convert to GraphQL format
5454
unifiedPagination, err := OptionalUnifiedPaginationParams(request)
5555
if err != nil {
5656
return mcp.NewToolResultError(err.Error()), nil
@@ -69,13 +69,6 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp
6969
categoryID = &id
7070
}
7171

72-
// Build GraphQL query arguments
73-
// Use default of 30 if neither first nor last is specified
74-
if pagination.First == nil && pagination.Last == nil {
75-
defaultFirst := int32(30)
76-
pagination.First = &defaultFirst
77-
}
78-
7972
var out []byte
8073

8174
var discussions []*github.Discussion
@@ -97,17 +90,14 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp
9790
HasNextPage bool
9891
EndCursor string
9992
}
100-
} `graphql:"discussions(first: $first, last: $last, after: $after, before: $before, categoryId: $categoryId)"`
93+
} `graphql:"discussions(first: $first, categoryId: $categoryId)"`
10194
} `graphql:"repository(owner: $owner, name: $repo)"`
10295
}
10396
vars := map[string]interface{}{
10497
"owner": githubv4.String(owner),
10598
"repo": githubv4.String(repo),
10699
"categoryId": *categoryID,
107-
"first": (*githubv4.Int)(pagination.First),
108-
"last": (*githubv4.Int)(pagination.Last),
109-
"after": (*githubv4.String)(pagination.After),
110-
"before": (*githubv4.String)(pagination.Before),
100+
"first": githubv4.Int(*pagination.First),
111101
}
112102
if err := client.Query(ctx, &query, vars); err != nil {
113103
return mcp.NewToolResultError(err.Error()), nil
@@ -149,16 +139,13 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp
149139
HasNextPage bool
150140
EndCursor string
151141
}
152-
} `graphql:"discussions(first: $first, last: $last, after: $after, before: $before)"`
142+
} `graphql:"discussions(first: $first)"`
153143
} `graphql:"repository(owner: $owner, name: $repo)"`
154144
}
155145
vars := map[string]interface{}{
156-
"owner": githubv4.String(owner),
157-
"repo": githubv4.String(repo),
158-
"first": (*githubv4.Int)(pagination.First),
159-
"last": (*githubv4.Int)(pagination.Last),
160-
"after": (*githubv4.String)(pagination.After),
161-
"before": (*githubv4.String)(pagination.Before),
146+
"owner": githubv4.String(owner),
147+
"repo": githubv4.String(repo),
148+
"first": githubv4.Int(*pagination.First),
162149
}
163150
if err := client.Query(ctx, &query, vars); err != nil {
164151
return mcp.NewToolResultError(err.Error()), nil
@@ -291,10 +278,16 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati
291278
if err != nil {
292279
return mcp.NewToolResultError(err.Error()), nil
293280
}
281+
282+
// Check if pagination parameters were explicitly provided
283+
_, pageProvided := request.Params.Arguments.(map[string]interface{})["page"]
284+
_, perPageProvided := request.Params.Arguments.(map[string]interface{})["perPage"]
285+
paginationExplicit := pageProvided || perPageProvided
286+
294287
paginationParams := unifiedPagination.ToGraphQLParams()
295288

296-
// Use default of 100 if neither first nor last is specified
297-
if paginationParams.First == nil && paginationParams.Last == nil {
289+
// Use default of 100 if pagination was not explicitly provided
290+
if !paginationExplicit {
298291
defaultFirst := int32(100)
299292
paginationParams.First = &defaultFirst
300293
}
@@ -315,16 +308,15 @@ func GetDiscussionComments(getGQLClient GetGQLClientFn, t translations.Translati
315308
HasNextPage githubv4.Boolean
316309
EndCursor githubv4.String
317310
}
318-
} `graphql:"comments(first: $first, after: $after)"`
311+
} `graphql:"comments(first: $first)"`
319312
} `graphql:"discussion(number: $discussionNumber)"`
320313
} `graphql:"repository(owner: $owner, name: $repo)"`
321314
}
322315
vars := map[string]interface{}{
323316
"owner": githubv4.String(params.Owner),
324317
"repo": githubv4.String(params.Repo),
325318
"discussionNumber": githubv4.Int(params.DiscussionNumber),
326-
"first": (*githubv4.Int)(paginationParams.First),
327-
"after": (*githubv4.String)(paginationParams.After),
319+
"first": githubv4.Int(*paginationParams.First),
328320
}
329321
if err := client.Query(ctx, &q, vars); err != nil {
330322
return mcp.NewToolResultError(err.Error()), nil
@@ -376,10 +368,16 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl
376368
if err != nil {
377369
return mcp.NewToolResultError(err.Error()), nil
378370
}
371+
372+
// Check if pagination parameters were explicitly provided
373+
_, pageProvided := request.Params.Arguments.(map[string]interface{})["page"]
374+
_, perPageProvided := request.Params.Arguments.(map[string]interface{})["perPage"]
375+
paginationExplicit := pageProvided || perPageProvided
376+
379377
pagination := unifiedPagination.ToGraphQLParams()
380378

381-
// Use default of 100 if neither first nor last is specified
382-
if pagination.First == nil && pagination.Last == nil {
379+
// Use default of 100 if pagination was not explicitly provided
380+
if !paginationExplicit {
383381
defaultFirst := int32(100)
384382
pagination.First = &defaultFirst
385383
}
@@ -400,14 +398,13 @@ func ListDiscussionCategories(getGQLClient GetGQLClientFn, t translations.Transl
400398
HasNextPage githubv4.Boolean
401399
EndCursor githubv4.String
402400
}
403-
} `graphql:"discussionCategories(first: $first, after: $after)"`
401+
} `graphql:"discussionCategories(first: $first)"`
404402
} `graphql:"repository(owner: $owner, name: $repo)"`
405403
}
406404
vars := map[string]interface{}{
407405
"owner": githubv4.String(params.Owner),
408406
"repo": githubv4.String(params.Repo),
409-
"first": (*githubv4.Int)(pagination.First),
410-
"after": (*githubv4.String)(pagination.After),
407+
"first": githubv4.Int(*pagination.First),
411408
}
412409
if err := client.Query(ctx, &q, vars); err != nil {
413410
return mcp.NewToolResultError(err.Error()), nil

‎pkg/github/discussions_test.go‎

Lines changed: 34 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -61,37 +61,28 @@ func Test_ListDiscussions(t *testing.T) {
6161
assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo"})
6262

6363
// 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}}}}"
64+
qDiscussions := "query($first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}"
6565

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}}}}"
66+
qDiscussionsFiltered := "query($categoryId:ID!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussions(first: $first, categoryId: $categoryId){nodes{number,title,createdAt,category{name},url},pageInfo{hasNextPage,endCursor}}}}"
6767

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

7875
varsRepoNotFound := map[string]interface{}{
79-
"owner": githubv4.String("owner"),
80-
"repo": githubv4.String("nonexistent-repo"),
81-
"first": float64(30),
82-
"last": nil,
83-
"after": nil,
84-
"before": nil,
76+
"owner": "owner",
77+
"repo": "nonexistent-repo",
78+
"first": float64(30),
8579
}
8680

8781
varsDiscussionsFiltered := map[string]interface{}{
88-
"owner": githubv4.String("owner"),
89-
"repo": githubv4.String("repo"),
90-
"categoryId": githubv4.ID("DIC_kwDOABC123"),
82+
"owner": "owner",
83+
"repo": "repo",
84+
"categoryId": "DIC_kwDOABC123",
9185
"first": float64(30),
92-
"last": nil,
93-
"after": nil,
94-
"before": nil,
9586
}
9687

9788
tests := []struct {
@@ -191,23 +182,13 @@ func Test_GetDiscussion(t *testing.T) {
191182
assert.Contains(t, toolDef.InputSchema.Properties, "discussionNumber")
192183
assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo", "discussionNumber"})
193184

194-
var q struct {
195-
Repository struct {
196-
Discussion struct {
197-
Number githubv4.Int
198-
Body githubv4.String
199-
CreatedAt githubv4.DateTime
200-
URL githubv4.String `graphql:"url"`
201-
Category struct {
202-
Name githubv4.String
203-
} `graphql:"category"`
204-
} `graphql:"discussion(number: $discussionNumber)"`
205-
} `graphql:"repository(owner: $owner, name: $repo)"`
206-
}
185+
// Use exact string query that matches implementation output
186+
qGetDiscussion := "query($discussionNumber:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){number,body,createdAt,url,category{name}}}}"
187+
207188
vars := map[string]interface{}{
208-
"owner": githubv4.String("owner"),
209-
"repo": githubv4.String("repo"),
210-
"discussionNumber": githubv4.Int(1),
189+
"owner": "owner",
190+
"repo": "repo",
191+
"discussionNumber": float64(1),
211192
}
212193
tests := []struct {
213194
name string
@@ -247,7 +228,7 @@ func Test_GetDiscussion(t *testing.T) {
247228
}
248229
for _, tc := range tests {
249230
t.Run(tc.name, func(t *testing.T) {
250-
matcher := githubv4mock.NewQueryMatcher(q, vars, tc.response)
231+
matcher := githubv4mock.NewQueryMatcher(qGetDiscussion, vars, tc.response)
251232
httpClient := githubv4mock.NewMockedHTTPClient(matcher)
252233
gqlClient := githubv4.NewClient(httpClient)
253234
_, handler := GetDiscussion(stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper)
@@ -285,15 +266,14 @@ func Test_GetDiscussionComments(t *testing.T) {
285266
assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo", "discussionNumber"})
286267

287268
// Use exact string query that matches implementation output
288-
qGetComments := "query($after:String$discussionNumber:Int!$first:Int$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){comments(first: $first, after: $after){nodes{body},pageInfo{hasNextPage,endCursor}}}}}"
269+
qGetComments := "query($discussionNumber:Int!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){comments(first: $first){nodes{body},pageInfo{hasNextPage,endCursor}}}}}"
289270

290271
// Variables matching what GraphQL receives after JSON marshaling/unmarshaling
291272
vars := map[string]interface{}{
292-
"owner": githubv4.String("owner"),
293-
"repo": githubv4.String("repo"),
294-
"discussionNumber": githubv4.Int(1),
295-
"first": float64(100), // Default value when no pagination specified
296-
"after": nil,
273+
"owner": "owner",
274+
"repo": "repo",
275+
"discussionNumber": float64(1),
276+
"first": float64(100),
297277
}
298278

299279
mockResponse := githubv4mock.DataResponse(map[string]any{
@@ -328,6 +308,9 @@ func Test_GetDiscussionComments(t *testing.T) {
328308

329309
textContent := getTextResult(t, result)
330310

311+
// Debug: print the actual JSON response
312+
t.Logf("JSON response: %s", textContent.Text)
313+
331314
var comments []*github.IssueComment
332315
err = json.Unmarshal([]byte(textContent.Text), &comments)
333316
require.NoError(t, err)
@@ -340,14 +323,13 @@ func Test_GetDiscussionComments(t *testing.T) {
340323

341324
func Test_ListDiscussionCategories(t *testing.T) {
342325
// Use exact string query that matches implementation output
343-
qListCategories := "query($after:String$first:Int$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussionCategories(first: $first, after: $after){nodes{id,name},pageInfo{hasNextPage,endCursor}}}}"
326+
qListCategories := "query($first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussionCategories(first: $first){nodes{id,name},pageInfo{hasNextPage,endCursor}}}}"
344327

345328
// Variables matching what GraphQL receives after JSON marshaling/unmarshaling
346329
vars := map[string]interface{}{
347-
"owner": githubv4.String("owner"),
348-
"repo": githubv4.String("repo"),
349-
"first": float64(100), // Default value when no pagination specified
350-
"after": nil,
330+
"owner": "owner",
331+
"repo": "repo",
332+
"first": float64(100),
351333
}
352334

353335
mockResp := githubv4mock.DataResponse(map[string]any{
@@ -380,6 +362,10 @@ func Test_ListDiscussionCategories(t *testing.T) {
380362
require.NoError(t, err)
381363

382364
text := getTextResult(t, result).Text
365+
366+
// Debug: print the actual JSON response
367+
t.Logf("JSON response: %s", text)
368+
383369
var categories []map[string]string
384370
require.NoError(t, json.Unmarshal([]byte(text), &categories))
385371
assert.Len(t, categories, 2)

0 commit comments

Comments
 (0)