diff --git a/README.md b/README.md index c5274ff83..5eec21ea7 100644 --- a/README.md +++ b/README.md @@ -595,6 +595,8 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description - **list_discussions** - List discussions - `category`: Optional filter by discussion category ID. If provided, only discussions with this category are listed. (string, optional) + - `direction`: Order direction. (string, optional) + - `orderBy`: Order discussions by field. If provided, the 'direction' also needs to be provided. (string, optional) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 23e2724d4..176b22a35 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -13,6 +13,81 @@ import ( "github.com/shurcooL/githubv4" ) +type DiscussionFragment struct { + Number githubv4.Int + Title githubv4.String + CreatedAt githubv4.DateTime + UpdatedAt githubv4.DateTime + Author struct { + Login githubv4.String + } + Category struct { + Name githubv4.String + } `graphql:"category"` + URL githubv4.String `graphql:"url"` +} + +type BasicNoOrder struct { + Repository struct { + Discussions struct { + Nodes []DiscussionFragment + } `graphql:"discussions(first: 100)"` + } `graphql:"repository(owner: $owner, name: $repo)"` +} + +type BasicWithOrder struct { + Repository struct { + Discussions struct { + Nodes []DiscussionFragment + } `graphql:"discussions(first: 100, orderBy: { field: $orderByField, direction: $orderByDirection })"` + } `graphql:"repository(owner: $owner, name: $repo)"` +} + +type WithCategoryAndOrder struct { + Repository struct { + Discussions struct { + Nodes []DiscussionFragment + } `graphql:"discussions(first: 100, categoryId: $categoryId, orderBy: { field: $orderByField, direction: $orderByDirection })"` + } `graphql:"repository(owner: $owner, name: $repo)"` +} + +type WithCategoryNoOrder struct { + Repository struct { + Discussions struct { + Nodes []DiscussionFragment + } `graphql:"discussions(first: 100, categoryId: $categoryId)"` + } `graphql:"repository(owner: $owner, name: $repo)"` +} + +func fragmentToDiscussion(fragment DiscussionFragment) *github.Discussion { + return &github.Discussion{ + Number: github.Ptr(int(fragment.Number)), + Title: github.Ptr(string(fragment.Title)), + HTMLURL: github.Ptr(string(fragment.URL)), + CreatedAt: &github.Timestamp{Time: fragment.CreatedAt.Time}, + UpdatedAt: &github.Timestamp{Time: fragment.UpdatedAt.Time}, + User: &github.User{ + Login: github.Ptr(string(fragment.Author.Login)), + }, + DiscussionCategory: &github.DiscussionCategory{ + Name: github.Ptr(string(fragment.Category.Name)), + }, + } +} + +func getQueryType(useOrdering bool, categoryID *githubv4.ID) any { + if categoryID != nil && useOrdering { + return &WithCategoryAndOrder{} + } + if categoryID != nil && !useOrdering { + return &WithCategoryNoOrder{} + } + if categoryID == nil && useOrdering { + return &BasicWithOrder{} + } + return &BasicNoOrder{} +} + func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("list_discussions", mcp.WithDescription(t("TOOL_LIST_DISCUSSIONS_DESCRIPTION", "List discussions for a repository")), @@ -31,9 +106,16 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp mcp.WithString("category", mcp.Description("Optional filter by discussion category ID. If provided, only discussions with this category are listed."), ), + mcp.WithString("orderBy", + mcp.Description("Order discussions by field. If provided, the 'direction' also needs to be provided."), + mcp.Enum("CREATED_AT", "UPDATED_AT"), + ), + mcp.WithString("direction", + mcp.Description("Order direction."), + mcp.Enum("ASC", "DESC"), + ), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - // Required params owner, err := RequiredParam[string](request, "owner") if err != nil { return mcp.NewToolResultError(err.Error()), nil @@ -43,106 +125,80 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp return mcp.NewToolResultError(err.Error()), nil } - // Optional params category, err := OptionalParam[string](request, "category") if err != nil { return mcp.NewToolResultError(err.Error()), nil } + orderBy, err := OptionalParam[string](request, "orderBy") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + direction, err := OptionalParam[string](request, "direction") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + client, err := getGQLClient(ctx) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil } - // If category filter is specified, use it as the category ID for server-side filtering var categoryID *githubv4.ID if category != "" { id := githubv4.ID(category) categoryID = &id } - // Now execute the discussions query - var discussions []*github.Discussion + vars := map[string]interface{}{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + } + + // this is an extra check in case the tool description is misinterpreted, because + // we shouldn't use ordering unless both a 'field' and 'direction' are provided + useOrdering := orderBy != "" && direction != "" + if useOrdering { + vars["orderByField"] = githubv4.DiscussionOrderField(orderBy) + vars["orderByDirection"] = githubv4.OrderDirection(direction) + } + if categoryID != nil { - // Query with category filter (server-side filtering) - var query struct { - Repository struct { - Discussions struct { - Nodes []struct { - Number githubv4.Int - Title githubv4.String - CreatedAt githubv4.DateTime - Category struct { - Name githubv4.String - } `graphql:"category"` - URL githubv4.String `graphql:"url"` - } - } `graphql:"discussions(first: 100, categoryId: $categoryId)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - vars := map[string]interface{}{ - "owner": githubv4.String(owner), - "repo": githubv4.String(repo), - "categoryId": *categoryID, - } - if err := client.Query(ctx, &query, vars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } + vars["categoryId"] = *categoryID + } - // Map nodes to GitHub Discussion objects - for _, n := range query.Repository.Discussions.Nodes { - di := &github.Discussion{ - Number: github.Ptr(int(n.Number)), - Title: github.Ptr(string(n.Title)), - HTMLURL: github.Ptr(string(n.URL)), - CreatedAt: &github.Timestamp{Time: n.CreatedAt.Time}, - DiscussionCategory: &github.DiscussionCategory{ - Name: github.Ptr(string(n.Category.Name)), - }, - } - discussions = append(discussions, di) - } - } else { - // Query without category filter - var query struct { - Repository struct { - Discussions struct { - Nodes []struct { - Number githubv4.Int - Title githubv4.String - CreatedAt githubv4.DateTime - Category struct { - Name githubv4.String - } `graphql:"category"` - URL githubv4.String `graphql:"url"` - } - } `graphql:"discussions(first: 100)"` - } `graphql:"repository(owner: $owner, name: $repo)"` + var discussions []*github.Discussion + discussionQuery := getQueryType(useOrdering, categoryID) + + if err := client.Query(ctx, discussionQuery, vars); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + // we need to check what user inputs we received at runtime, and use the + // most appropriate query based on that + switch queryType := discussionQuery.(type) { + case *WithCategoryAndOrder: + for _, node := range queryType.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) } - vars := map[string]interface{}{ - "owner": githubv4.String(owner), - "repo": githubv4.String(repo), + + case *WithCategoryNoOrder: + for _, node := range queryType.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) } - if err := client.Query(ctx, &query, vars); err != nil { - return mcp.NewToolResultError(err.Error()), nil + + case *BasicWithOrder: + for _, node := range queryType.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) } - // Map nodes to GitHub Discussion objects - for _, n := range query.Repository.Discussions.Nodes { - di := &github.Discussion{ - Number: github.Ptr(int(n.Number)), - Title: github.Ptr(string(n.Title)), - HTMLURL: github.Ptr(string(n.URL)), - CreatedAt: &github.Timestamp{Time: n.CreatedAt.Time}, - DiscussionCategory: &github.DiscussionCategory{ - Name: github.Ptr(string(n.Category.Name)), - }, - } - discussions = append(discussions, di) + case *BasicNoOrder: + for _, node := range queryType.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) } } - // Marshal and return out, err := json.Marshal(discussions) if err != nil { return nil, fmt.Errorf("failed to marshal discussions: %w", err) diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index c6688a519..1b00dcfce 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -17,14 +17,58 @@ import ( var ( discussionsGeneral = []map[string]any{ - {"number": 1, "title": "Discussion 1 title", "createdAt": "2023-01-01T00:00:00Z", "url": "https://github.com/owner/repo/discussions/1", "category": map[string]any{"name": "General"}}, - {"number": 3, "title": "Discussion 3 title", "createdAt": "2023-03-01T00:00:00Z", "url": "https://github.com/owner/repo/discussions/3", "category": map[string]any{"name": "General"}}, + {"number": 1, "title": "Discussion 1 title", "createdAt": "2023-01-01T00:00:00Z", "updatedAt": "2023-01-01T00:00:00Z", "author": map[string]any{"login": "user1"}, "url": "https://github.com/owner/repo/discussions/1", "category": map[string]any{"name": "General"}}, + {"number": 3, "title": "Discussion 3 title", "createdAt": "2023-03-01T00:00:00Z", "updatedAt": "2023-02-01T00:00:00Z", "author": map[string]any{"login": "user1"}, "url": "https://github.com/owner/repo/discussions/3", "category": map[string]any{"name": "General"}}, } discussionsAll = []map[string]any{ - {"number": 1, "title": "Discussion 1 title", "createdAt": "2023-01-01T00:00:00Z", "url": "https://github.com/owner/repo/discussions/1", "category": map[string]any{"name": "General"}}, - {"number": 2, "title": "Discussion 2 title", "createdAt": "2023-02-01T00:00:00Z", "url": "https://github.com/owner/repo/discussions/2", "category": map[string]any{"name": "Questions"}}, - {"number": 3, "title": "Discussion 3 title", "createdAt": "2023-03-01T00:00:00Z", "url": "https://github.com/owner/repo/discussions/3", "category": map[string]any{"name": "General"}}, + { + "number": 1, + "title": "Discussion 1 title", + "createdAt": "2023-01-01T00:00:00Z", + "updatedAt": "2023-01-01T00:00:00Z", + "author": map[string]any{"login": "user1"}, + "url": "https://github.com/owner/repo/discussions/1", + "category": map[string]any{"name": "General"}, + }, + { + "number": 2, + "title": "Discussion 2 title", + "createdAt": "2023-02-01T00:00:00Z", + "updatedAt": "2023-02-01T00:00:00Z", + "author": map[string]any{"login": "user2"}, + "url": "https://github.com/owner/repo/discussions/2", + "category": map[string]any{"name": "Questions"}, + }, + { + "number": 3, + "title": "Discussion 3 title", + "createdAt": "2023-03-01T00:00:00Z", + "updatedAt": "2023-03-01T00:00:00Z", + "author": map[string]any{"login": "user3"}, + "url": "https://github.com/owner/repo/discussions/3", + "category": map[string]any{"name": "General"}, + }, } + + // Ordered mock responses + discussionsOrderedCreatedAsc = []map[string]any{ + discussionsAll[0], // Discussion 1 (created 2023-01-01) + discussionsAll[1], // Discussion 2 (created 2023-02-01) + discussionsAll[2], // Discussion 3 (created 2023-03-01) + } + + discussionsOrderedUpdatedDesc = []map[string]any{ + discussionsAll[2], // Discussion 3 (updated 2023-03-01) + discussionsAll[1], // Discussion 2 (updated 2023-02-01) + discussionsAll[0], // Discussion 1 (updated 2023-01-01) + } + + // only 'General' category discussions ordered by created date descending + discussionsGeneralOrderedDesc = []map[string]any{ + discussionsGeneral[1], // Discussion 3 (created 2023-03-01) + discussionsGeneral[0], // Discussion 1 (created 2023-01-01) + } + mockResponseListAll = githubv4mock.DataResponse(map[string]any{ "repository": map[string]any{ "discussions": map[string]any{"nodes": discussionsAll}, @@ -35,53 +79,35 @@ var ( "discussions": map[string]any{"nodes": discussionsGeneral}, }, }) + mockResponseOrderedCreatedAsc = githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "discussions": map[string]any{"nodes": discussionsOrderedCreatedAsc}, + }, + }) + mockResponseOrderedUpdatedDesc = githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "discussions": map[string]any{"nodes": discussionsOrderedUpdatedDesc}, + }, + }) + mockResponseGeneralOrderedDesc = githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "discussions": map[string]any{"nodes": discussionsGeneralOrderedDesc}, + }, + }) mockErrorRepoNotFound = githubv4mock.ErrorResponse("repository not found") ) func Test_ListDiscussions(t *testing.T) { mockClient := githubv4.NewClient(nil) - // Verify tool definition and schema toolDef, _ := ListDiscussions(stubGetGQLClientFn(mockClient), translations.NullTranslationHelper) assert.Equal(t, "list_discussions", toolDef.Name) assert.NotEmpty(t, toolDef.Description) assert.Contains(t, toolDef.InputSchema.Properties, "owner") assert.Contains(t, toolDef.InputSchema.Properties, "repo") + assert.Contains(t, toolDef.InputSchema.Properties, "orderBy") + assert.Contains(t, toolDef.InputSchema.Properties, "direction") assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo"}) - // mock for the call to ListDiscussions without category filter - var qDiscussions struct { - Repository struct { - Discussions struct { - Nodes []struct { - Number githubv4.Int - Title githubv4.String - CreatedAt githubv4.DateTime - Category struct { - Name githubv4.String - } `graphql:"category"` - URL githubv4.String `graphql:"url"` - } - } `graphql:"discussions(first: 100)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - - // mock for the call to get discussions with category filter - var qDiscussionsFiltered struct { - Repository struct { - Discussions struct { - Nodes []struct { - Number githubv4.Int - Title githubv4.String - CreatedAt githubv4.DateTime - Category struct { - Name githubv4.String - } `graphql:"category"` - URL githubv4.String `graphql:"url"` - } - } `graphql:"discussions(first: 100, categoryId: $categoryId)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - varsListAll := map[string]interface{}{ "owner": githubv4.String("owner"), "repo": githubv4.String("repo"), @@ -98,12 +124,35 @@ func Test_ListDiscussions(t *testing.T) { "categoryId": githubv4.ID("DIC_kwDOABC123"), } + varsOrderByCreatedAsc := map[string]interface{}{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "orderByField": githubv4.DiscussionOrderField("CREATED_AT"), + "orderByDirection": githubv4.OrderDirection("ASC"), + } + + varsOrderByUpdatedDesc := map[string]interface{}{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "orderByField": githubv4.DiscussionOrderField("UPDATED_AT"), + "orderByDirection": githubv4.OrderDirection("DESC"), + } + + varsCategoryWithOrder := map[string]interface{}{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "categoryId": githubv4.ID("DIC_kwDOABC123"), + "orderByField": githubv4.DiscussionOrderField("CREATED_AT"), + "orderByDirection": githubv4.OrderDirection("DESC"), + } + tests := []struct { name string reqParams map[string]interface{} expectError bool errContains string expectedCount int + verifyOrder func(t *testing.T, discussions []*github.Discussion) }{ { name: "list all discussions without category filter", @@ -124,6 +173,80 @@ func Test_ListDiscussions(t *testing.T) { expectError: false, expectedCount: 2, // Only General discussions (matching the category ID) }, + { + name: "order by created at ascending", + reqParams: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "orderBy": "CREATED_AT", + "direction": "ASC", + }, + expectError: false, + expectedCount: 3, + verifyOrder: func(t *testing.T, discussions []*github.Discussion) { + // Verify discussions are ordered by created date ascending + require.Len(t, discussions, 3) + assert.Equal(t, 1, *discussions[0].Number, "First should be discussion 1 (created 2023-01-01)") + assert.Equal(t, 2, *discussions[1].Number, "Second should be discussion 2 (created 2023-02-01)") + assert.Equal(t, 3, *discussions[2].Number, "Third should be discussion 3 (created 2023-03-01)") + }, + }, + { + name: "order by updated at descending", + reqParams: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "orderBy": "UPDATED_AT", + "direction": "DESC", + }, + expectError: false, + expectedCount: 3, + verifyOrder: func(t *testing.T, discussions []*github.Discussion) { + // Verify discussions are ordered by updated date descending + require.Len(t, discussions, 3) + assert.Equal(t, 3, *discussions[0].Number, "First should be discussion 3 (updated 2023-03-01)") + assert.Equal(t, 2, *discussions[1].Number, "Second should be discussion 2 (updated 2023-02-01)") + assert.Equal(t, 1, *discussions[2].Number, "Third should be discussion 1 (updated 2023-01-01)") + }, + }, + { + name: "filter by category with order", + reqParams: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "category": "DIC_kwDOABC123", + "orderBy": "CREATED_AT", + "direction": "DESC", + }, + expectError: false, + expectedCount: 2, + verifyOrder: func(t *testing.T, discussions []*github.Discussion) { + // Verify only General discussions, ordered by created date descending + require.Len(t, discussions, 2) + assert.Equal(t, 3, *discussions[0].Number, "First should be discussion 3 (created 2023-03-01)") + assert.Equal(t, 1, *discussions[1].Number, "Second should be discussion 1 (created 2023-01-01)") + }, + }, + { + name: "order by without direction (should not use ordering)", + reqParams: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "orderBy": "CREATED_AT", + }, + expectError: false, + expectedCount: 3, + }, + { + name: "direction without order by (should not use ordering)", + reqParams: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "direction": "DESC", + }, + expectError: false, + expectedCount: 3, + }, { name: "repository not found error", reqParams: map[string]interface{}{ @@ -141,15 +264,35 @@ func Test_ListDiscussions(t *testing.T) { switch tc.name { case "list all discussions without category filter": - // Simple case - no category filter - matcher := githubv4mock.NewQueryMatcher(qDiscussions, varsListAll, mockResponseListAll) + // Simple case - BasicNoOrder query structure (i.e. no order, no category) + matcher := githubv4mock.NewQueryMatcher(&BasicNoOrder{}, varsListAll, mockResponseListAll) httpClient = githubv4mock.NewMockedHTTPClient(matcher) case "filter by category ID": - // Simple case - category filter using category ID directly - matcher := githubv4mock.NewQueryMatcher(qDiscussionsFiltered, varsDiscussionsFiltered, mockResponseListGeneral) + // WithCategoryNoOrder + matcher := githubv4mock.NewQueryMatcher(&WithCategoryNoOrder{}, varsDiscussionsFiltered, mockResponseListGeneral) + httpClient = githubv4mock.NewMockedHTTPClient(matcher) + case "order by created at ascending": + // BasicWithOrder - use ordered response + matcher := githubv4mock.NewQueryMatcher(&BasicWithOrder{}, varsOrderByCreatedAsc, mockResponseOrderedCreatedAsc) + httpClient = githubv4mock.NewMockedHTTPClient(matcher) + case "order by updated at descending": + // BasicWithOrder - use ordered response + matcher := githubv4mock.NewQueryMatcher(&BasicWithOrder{}, varsOrderByUpdatedDesc, mockResponseOrderedUpdatedDesc) + httpClient = githubv4mock.NewMockedHTTPClient(matcher) + case "filter by category with order": + // WithCategoryAndOrder - use ordered response + matcher := githubv4mock.NewQueryMatcher(&WithCategoryAndOrder{}, varsCategoryWithOrder, mockResponseGeneralOrderedDesc) + httpClient = githubv4mock.NewMockedHTTPClient(matcher) + case "order by without direction (should not use ordering)": + // BasicNoOrder - because useOrdering will be false + matcher := githubv4mock.NewQueryMatcher(&BasicNoOrder{}, varsListAll, mockResponseListAll) + httpClient = githubv4mock.NewMockedHTTPClient(matcher) + case "direction without order by (should not use ordering)": + // BasicNoOrder - because useOrdering will be false + matcher := githubv4mock.NewQueryMatcher(&BasicNoOrder{}, varsListAll, mockResponseListAll) httpClient = githubv4mock.NewMockedHTTPClient(matcher) case "repository not found error": - matcher := githubv4mock.NewQueryMatcher(qDiscussions, varsRepoNotFound, mockErrorRepoNotFound) + matcher := githubv4mock.NewQueryMatcher(&BasicNoOrder{}, varsRepoNotFound, mockErrorRepoNotFound) httpClient = githubv4mock.NewMockedHTTPClient(matcher) } @@ -173,6 +316,11 @@ func Test_ListDiscussions(t *testing.T) { assert.Len(t, returnedDiscussions, tc.expectedCount, "Expected %d discussions, got %d", tc.expectedCount, len(returnedDiscussions)) + // Verify order if verifyOrder function is provided + if tc.verifyOrder != nil { + tc.verifyOrder(t, returnedDiscussions) + } + // Verify that all returned discussions have a category if filtered if _, hasCategory := tc.reqParams["category"]; hasCategory { for _, discussion := range returnedDiscussions {