Skip to content

Commit 33d5752

Browse files
williammartinwilliammartin
committed
Use arrays rather than comma separated lists
1 parent a14e982 commit 33d5752

File tree

6 files changed

+137
-183
lines changed

6 files changed

+137
-183
lines changed

‎README.md‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,8 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description
129129
- `repo`: Repository name (string, required)
130130
- `title`: Issue title (string, required)
131131
- `body`: Issue body content (string, optional)
132-
- `assignees`: Comma-separated list of usernames to assign to this issue (string, optional)
133-
- `labels`: Comma-separated list of labels to apply to this issue (string, optional)
132+
- `assignees`: Usernames to assign to this issue (string[], optional)
133+
- `labels`: Labels to apply to this issue (string[], optional)
134134

135135
- **add_issue_comment** - Add a comment to an issue
136136

@@ -144,7 +144,7 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description
144144
- `owner`: Repository owner (string, required)
145145
- `repo`: Repository name (string, required)
146146
- `state`: Filter by state ('open', 'closed', 'all') (string, optional)
147-
- `labels`: Comma-separated list of labels to filter by (string, optional)
147+
- `labels`: Labels to filter by (string[], optional)
148148
- `sort`: Sort by ('created', 'updated', 'comments') (string, optional)
149149
- `direction`: Sort direction ('asc', 'desc') (string, optional)
150150
- `since`: Filter by date (ISO 8601 timestamp) (string, optional)
@@ -159,8 +159,8 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description
159159
- `title`: New title (string, optional)
160160
- `body`: New description (string, optional)
161161
- `state`: New state ('open' or 'closed') (string, optional)
162-
- `labels`: Comma-separated list of new labels (string, optional)
163-
- `assignees`: Comma-separated list of new assignees (string, optional)
162+
- `labels`: New labels (string[], optional)
163+
- `assignees`: New assignees (string[], optional)
164164
- `milestone`: New milestone number (number, optional)
165165

166166
- **search_issues** - Search for issues and pull requests

‎pkg/github/helper_test.go‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,52 @@ import (
1010
"github.com/stretchr/testify/require"
1111
)
1212

13+
// expectQueryParams is a helper function to create a partial mock that expects a
14+
// request with the given query parameters, with the ability to chain a response handler.
15+
func expectQueryParams(t *testing.T, expectedQueryParams map[string]string) *partialMock {
16+
return &partialMock{
17+
t: t,
18+
expectedQueryParams: expectedQueryParams,
19+
}
20+
}
21+
22+
// expectRequestBody is a helper function to create a partial mock that expects a
23+
// request with the given body, with the ability to chain a response handler.
24+
func expectRequestBody(t *testing.T, expectedRequestBody any) *partialMock {
25+
return &partialMock{
26+
t: t,
27+
expectedRequestBody: expectedRequestBody,
28+
}
29+
}
30+
31+
type partialMock struct {
32+
t *testing.T
33+
expectedQueryParams map[string]string
34+
expectedRequestBody any
35+
}
36+
37+
func (p *partialMock) andThen(responseHandler http.HandlerFunc) http.HandlerFunc {
38+
p.t.Helper()
39+
return func(w http.ResponseWriter, r *http.Request) {
40+
if p.expectedRequestBody != nil {
41+
var unmarshaledRequestBody any
42+
err := json.NewDecoder(r.Body).Decode(&unmarshaledRequestBody)
43+
require.NoError(p.t, err)
44+
45+
require.Equal(p.t, p.expectedRequestBody, unmarshaledRequestBody)
46+
}
47+
48+
if p.expectedQueryParams != nil {
49+
require.Equal(p.t, len(p.expectedQueryParams), len(r.URL.Query()))
50+
for k, v := range p.expectedQueryParams {
51+
require.Equal(p.t, v, r.URL.Query().Get(k))
52+
}
53+
}
54+
55+
responseHandler(w, r)
56+
}
57+
}
58+
1359
// mockResponse is a helper function to create a mock HTTP response handler
1460
// that returns a specified status code and marshaled body.
1561
func mockResponse(t *testing.T, code int, body interface{}) http.HandlerFunc {

‎pkg/github/issues.go‎

Lines changed: 51 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -228,11 +228,21 @@ func createIssue(client *github.Client, t translations.TranslationHelperFunc) (t
228228
mcp.WithString("body",
229229
mcp.Description("Issue body content"),
230230
),
231-
mcp.WithString("assignees",
232-
mcp.Description("Comma-separate list of usernames to assign to this issue"),
233-
),
234-
mcp.WithString("labels",
235-
mcp.Description("Comma-separate list of labels to apply to this issue"),
231+
mcp.WithArray("assignees",
232+
mcp.Description("Usernames to assign to this issue"),
233+
mcp.Items(
234+
map[string]interface{}{
235+
"type": "string",
236+
},
237+
),
238+
),
239+
mcp.WithArray("labels",
240+
mcp.Description("Labels to apply to this issue"),
241+
mcp.Items(
242+
map[string]interface{}{
243+
"type": "string",
244+
},
245+
),
236246
),
237247
),
238248
func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
@@ -256,12 +266,13 @@ func createIssue(client *github.Client, t translations.TranslationHelperFunc) (t
256266
}
257267

258268
// Get assignees
259-
assignees, err := optionalCommaSeparatedListParam(request, "assignees")
269+
assignees, err := optionalParam[[]string](request, "assignees")
260270
if err != nil {
261271
return mcp.NewToolResultError(err.Error()), nil
262272
}
273+
263274
// Get labels
264-
labels, err := optionalCommaSeparatedListParam(request, "labels")
275+
labels, err := optionalParam[[]string](request, "labels")
265276
if err != nil {
266277
return mcp.NewToolResultError(err.Error()), nil
267278
}
@@ -311,15 +322,23 @@ func listIssues(client *github.Client, t translations.TranslationHelperFunc) (to
311322
),
312323
mcp.WithString("state",
313324
mcp.Description("Filter by state ('open', 'closed', 'all')"),
325+
mcp.Enum("open", "closed", "all"),
314326
),
315-
mcp.WithString("labels",
316-
mcp.Description("Comma-separated list of labels to filter by"),
327+
mcp.WithArray("labels",
328+
mcp.Description("Filter by labels"),
329+
mcp.Items(
330+
map[string]interface{}{
331+
"type": "string",
332+
},
333+
),
317334
),
318335
mcp.WithString("sort",
319336
mcp.Description("Sort by ('created', 'updated', 'comments')"),
337+
mcp.Enum("created", "updated", "comments"),
320338
),
321339
mcp.WithString("direction",
322340
mcp.Description("Sort direction ('asc', 'desc')"),
341+
mcp.Enum("asc", "desc"),
323342
),
324343
mcp.WithString("since",
325344
mcp.Description("Filter by date (ISO 8601 timestamp)"),
@@ -349,7 +368,8 @@ func listIssues(client *github.Client, t translations.TranslationHelperFunc) (to
349368
return mcp.NewToolResultError(err.Error()), nil
350369
}
351370

352-
opts.Labels, err = optionalCommaSeparatedListParam(request, "labels")
371+
// Get labels
372+
opts.Labels, err = optionalParam[[]string](request, "labels")
353373
if err != nil {
354374
return mcp.NewToolResultError(err.Error()), nil
355375
}
@@ -431,12 +451,23 @@ func updateIssue(client *github.Client, t translations.TranslationHelperFunc) (t
431451
),
432452
mcp.WithString("state",
433453
mcp.Description("New state ('open' or 'closed')"),
434-
),
435-
mcp.WithString("labels",
436-
mcp.Description("Comma-separated list of new labels"),
437-
),
438-
mcp.WithString("assignees",
439-
mcp.Description("Comma-separated list of new assignees"),
454+
mcp.Enum("open", "closed"),
455+
),
456+
mcp.WithArray("labels",
457+
mcp.Description("New labels"),
458+
mcp.Items(
459+
map[string]interface{}{
460+
"type": "string",
461+
},
462+
),
463+
),
464+
mcp.WithArray("assignees",
465+
mcp.Description("New assignees"),
466+
mcp.Items(
467+
map[string]interface{}{
468+
"type": "string",
469+
},
470+
),
440471
),
441472
mcp.WithNumber("milestone",
442473
mcp.Description("New milestone number"),
@@ -484,15 +515,17 @@ func updateIssue(client *github.Client, t translations.TranslationHelperFunc) (t
484515
issueRequest.State = github.Ptr(state)
485516
}
486517

487-
labels, err := optionalCommaSeparatedListParam(request, "labels")
518+
// Get labels
519+
labels, err := optionalParam[[]string](request, "labels")
488520
if err != nil {
489521
return mcp.NewToolResultError(err.Error()), nil
490522
}
491523
if len(labels) > 0 {
492524
issueRequest.Labels = &labels
493525
}
494526

495-
assignees, err := optionalCommaSeparatedListParam(request, "assignees")
527+
// Get assignees
528+
assignees, err := optionalParam[[]string](request, "assignees")
496529
if err != nil {
497530
return mcp.NewToolResultError(err.Error()), nil
498531
}

‎pkg/github/issues_test.go‎

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -418,16 +418,23 @@ func Test_CreateIssue(t *testing.T) {
418418
mockedClient: mock.NewMockedHTTPClient(
419419
mock.WithRequestMatchHandler(
420420
mock.PostReposIssuesByOwnerByRepo,
421-
mockResponse(t, http.StatusCreated, mockIssue),
421+
expectRequestBody(t, map[string]any{
422+
"title": "Test Issue",
423+
"body": "This is a test issue",
424+
"labels": []any{"bug", "help wanted"},
425+
"assignees": []any{"user1", "user2"},
426+
}).andThen(
427+
mockResponse(t, http.StatusCreated, mockIssue),
428+
),
422429
),
423430
),
424431
requestArgs: map[string]interface{}{
425432
"owner": "owner",
426433
"repo": "repo",
427434
"title": "Test Issue",
428435
"body": "This is a test issue",
429-
"assignees": "user1, user2",
430-
"labels": "bug, help wanted",
436+
"assignees": []string{"user1", "user2"},
437+
"labels": []string{"bug", "help wanted"},
431438
},
432439
expectError: false,
433440
expectedIssue: mockIssue,
@@ -606,16 +613,26 @@ func Test_ListIssues(t *testing.T) {
606613
{
607614
name: "list issues with all parameters",
608615
mockedClient: mock.NewMockedHTTPClient(
609-
mock.WithRequestMatch(
616+
mock.WithRequestMatchHandler(
610617
mock.GetReposIssuesByOwnerByRepo,
611-
mockIssues,
618+
expectQueryParams(t, map[string]string{
619+
"state": "open",
620+
"labels": "bug,enhancement",
621+
"sort": "created",
622+
"direction": "desc",
623+
"since": "2023-01-01T00:00:00Z",
624+
"page": "1",
625+
"per_page": "30",
626+
}).andThen(
627+
mockResponse(t, http.StatusOK, mockIssues),
628+
),
612629
),
613630
),
614631
requestArgs: map[string]interface{}{
615632
"owner": "owner",
616633
"repo": "repo",
617634
"state": "open",
618-
"labels": "bug,enhancement",
635+
"labels": []string{"bug", "enhancement"},
619636
"sort": "created",
620637
"direction": "desc",
621638
"since": "2023-01-01T00:00:00Z",
@@ -750,7 +767,16 @@ func Test_UpdateIssue(t *testing.T) {
750767
mockedClient: mock.NewMockedHTTPClient(
751768
mock.WithRequestMatchHandler(
752769
mock.PatchReposIssuesByOwnerByRepoByIssueNumber,
753-
mockResponse(t, http.StatusOK, mockIssue),
770+
expectRequestBody(t, map[string]any{
771+
"title": "Updated Issue Title",
772+
"body": "Updated issue description",
773+
"state": "closed",
774+
"labels": []any{"bug", "priority"},
775+
"assignees": []any{"assignee1", "assignee2"},
776+
"milestone": float64(5),
777+
}).andThen(
778+
mockResponse(t, http.StatusOK, mockIssue),
779+
),
754780
),
755781
),
756782
requestArgs: map[string]interface{}{
@@ -760,8 +786,8 @@ func Test_UpdateIssue(t *testing.T) {
760786
"title": "Updated Issue Title",
761787
"body": "Updated issue description",
762788
"state": "closed",
763-
"labels": "bug,priority",
764-
"assignees": "assignee1,assignee2",
789+
"labels": []string{"bug", "priority"},
790+
"assignees": []string{"assignee1", "assignee2"},
765791
"milestone": float64(5),
766792
},
767793
expectError: false,

‎pkg/github/server.go‎

Lines changed: 0 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"fmt"
88
"io"
99
"net/http"
10-
"strings"
1110

1211
"github.com/github/github-mcp-server/pkg/translations"
1312
"github.com/google/go-github/v69/github"
@@ -119,25 +118,6 @@ func isAcceptedError(err error) bool {
119118
return errors.As(err, &acceptedError)
120119
}
121120

122-
// parseCommaSeparatedList is a helper function that parses a comma-separated list of strings from the input string.
123-
func parseCommaSeparatedList(input string) []string {
124-
if input == "" {
125-
return nil
126-
}
127-
128-
parts := strings.Split(input, ",")
129-
result := make([]string, 0, len(parts))
130-
131-
for _, part := range parts {
132-
trimmed := strings.TrimSpace(part)
133-
if trimmed != "" {
134-
result = append(result, trimmed)
135-
}
136-
}
137-
138-
return result
139-
}
140-
141121
// requiredParam is a helper function that can be used to fetch a requested parameter from the request.
142122
// It does the following checks:
143123
// 1. Checks if the parameter is present in the request.
@@ -221,20 +201,3 @@ func optionalIntParamWithDefault(r mcp.CallToolRequest, p string, d int) (int, e
221201
}
222202
return v, nil
223203
}
224-
225-
// optionalCommaSeparatedListParam is a helper function that can be used to fetch a requested parameter from the request.
226-
// It does the following:
227-
// 1. Checks if the parameter is present in the request, if not, it returns an empty list
228-
// 2. If it is present, it checks if the parameter is of the expected type and uses parseCommaSeparatedList to parse it
229-
// and return the list of strings
230-
func optionalCommaSeparatedListParam(r mcp.CallToolRequest, p string) ([]string, error) {
231-
v, err := optionalParam[string](r, p)
232-
if err != nil {
233-
return []string{}, err
234-
}
235-
l := parseCommaSeparatedList(v)
236-
if len(l) == 0 {
237-
return []string{}, nil
238-
}
239-
return l, nil
240-
}

0 commit comments

Comments
 (0)