From 687ee8f1b297564d361e2e679a6e1d801687ba83 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Mon, 14 Jul 2025 23:03:59 +0100 Subject: [PATCH 01/17] added updatedAt and Author (aka User) login to query and payload --- pkg/github/discussions.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 3e53a633b..3e5bcca0c 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -72,6 +72,10 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp Number githubv4.Int Title githubv4.String CreatedAt githubv4.DateTime + UpdatedAt githubv4.DateTime + Author struct { + Login githubv4.String + } Category struct { Name githubv4.String } `graphql:"category"` @@ -96,6 +100,10 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp Title: github.Ptr(string(n.Title)), HTMLURL: github.Ptr(string(n.URL)), CreatedAt: &github.Timestamp{Time: n.CreatedAt.Time}, + UpdatedAt: &github.Timestamp{Time: n.UpdatedAt.Time}, + User: &github.User{ + Login: github.Ptr(string(n.Author.Login)), + }, DiscussionCategory: &github.DiscussionCategory{ Name: github.Ptr(string(n.Category.Name)), }, @@ -111,6 +119,10 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp Number githubv4.Int Title githubv4.String CreatedAt githubv4.DateTime + UpdatedAt githubv4.DateTime + Author struct { + Login githubv4.String + } Category struct { Name githubv4.String } `graphql:"category"` @@ -134,6 +146,10 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp Title: github.Ptr(string(n.Title)), HTMLURL: github.Ptr(string(n.URL)), CreatedAt: &github.Timestamp{Time: n.CreatedAt.Time}, + UpdatedAt: &github.Timestamp{Time: n.UpdatedAt.Time}, + User: &github.User{ + Login: github.Ptr(string(n.Author.Login)), + }, DiscussionCategory: &github.DiscussionCategory{ Name: github.Ptr(string(n.Category.Name)), }, From dbbf7240f54b944736069a43168d607fc6003486 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Tue, 15 Jul 2025 10:46:56 +0100 Subject: [PATCH 02/17] added initial support for orderby and direction --- pkg/github/discussions.go | 48 +++++++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 3e5bcca0c..5aa1a834d 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -31,6 +31,14 @@ 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"), + 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 @@ -61,6 +69,27 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp categoryID = &id } + orderBy, err := OptionalParam[string](request, "orderBy") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + if orderBy == "" { + orderBy = "UPDATED_AT" // or "CREATED_AT" + } + direction, err := OptionalParam[string](request, "direction") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + if direction == "" { + direction = "DESC" + } + + /* orderByInput := map[string]interface{}{ + "field": orderBy, + "direction": direction, + } */ + + // Now execute the discussions query var discussions []*github.Discussion if categoryID != nil { @@ -81,14 +110,20 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } `graphql:"category"` URL githubv4.String `graphql:"url"` } - } `graphql:"discussions(first: 100, categoryId: $categoryId)"` + } `graphql:"discussions(first: 100, categoryId: $categoryId, orderBy: {field: $orderByField, direction: $direction})"` } `graphql:"repository(owner: $owner, name: $repo)"` } + + vars := map[string]interface{}{ "owner": githubv4.String(owner), "repo": githubv4.String(repo), "categoryId": *categoryID, + "orderByField": githubv4.DiscussionOrderField(orderBy), + "direction": githubv4.OrderDirection(direction), } + + if err := client.Query(ctx, &query, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -128,13 +163,18 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } `graphql:"category"` URL githubv4.String `graphql:"url"` } - } `graphql:"discussions(first: 100)"` + } `graphql:"discussions(first: 100, orderBy: {field: $orderByField, direction: $direction})"` } `graphql:"repository(owner: $owner, name: $repo)"` } + + vars := map[string]interface{}{ - "owner": githubv4.String(owner), - "repo": githubv4.String(repo), + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "orderByField": githubv4.DiscussionOrderField(orderBy), + "direction": githubv4.OrderDirection(direction), } + if err := client.Query(ctx, &query, vars); err != nil { return mcp.NewToolResultError(err.Error()), nil } From 34a413deece00331bdee43b6157c402abfb5789a Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Tue, 15 Jul 2025 10:48:30 +0100 Subject: [PATCH 03/17] sort by created at instead of updated at by default --- pkg/github/discussions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 5aa1a834d..5bf58f5f8 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -74,7 +74,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp return mcp.NewToolResultError(err.Error()), nil } if orderBy == "" { - orderBy = "UPDATED_AT" // or "CREATED_AT" + orderBy = "CREATED_AT" } direction, err := OptionalParam[string](request, "direction") if err != nil { From 5d7230d7f065738d51ccb77ba384d6e0f014ddae Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Tue, 15 Jul 2025 14:51:26 +0100 Subject: [PATCH 04/17] remove unused code --- pkg/github/discussions.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 5bf58f5f8..57fa0392f 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -84,12 +84,6 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp direction = "DESC" } - /* orderByInput := map[string]interface{}{ - "field": orderBy, - "direction": direction, - } */ - - // Now execute the discussions query var discussions []*github.Discussion if categoryID != nil { From c1afb883d6a63873a0943e9a3ee269c2da43e448 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Wed, 16 Jul 2025 17:49:26 +0100 Subject: [PATCH 05/17] refactor to map to most suitable query based on user inputs at runtime --- pkg/github/discussions.go | 269 ++++++++++++++++++++------------------ 1 file changed, 144 insertions(+), 125 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 57fa0392f..1ddadbcbc 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "log" "github.com/github/github-mcp-server/pkg/translations" "github.com/go-viper/mapstructure/v2" @@ -13,6 +14,71 @@ import ( "github.com/shurcooL/githubv4" ) +// Define reusable fragments for discussions +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 discussionQueries struct { + BasicNoOrder struct { + Repository struct { + Discussions struct { + Nodes []DiscussionFragment + } `graphql:"discussions(first: 100)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + BasicWithOrder struct { + Repository struct { + Discussions struct { + Nodes []DiscussionFragment + } `graphql:"discussions(first: 100, orderBy: $orderBy)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + WithCategoryAndOrder struct { + Repository struct { + Discussions struct { + Nodes []DiscussionFragment + } `graphql:"discussions(first: 100, categoryId: $categoryId, orderBy: $orderBy)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + 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 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")), @@ -32,16 +98,15 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp 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"), + 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.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 @@ -51,146 +116,100 @@ 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 } - 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 - } - orderBy, err := OptionalParam[string](request, "orderBy") if err != nil { return mcp.NewToolResultError(err.Error()), nil } - if orderBy == "" { - orderBy = "CREATED_AT" - } + direction, err := OptionalParam[string](request, "direction") if err != nil { return mcp.NewToolResultError(err.Error()), nil } - if direction == "" { - direction = "DESC" - } - - // Now execute the discussions query - var discussions []*github.Discussion - 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 - UpdatedAt githubv4.DateTime - Author struct { - Login githubv4.String - } - Category struct { - Name githubv4.String - } `graphql:"category"` - URL githubv4.String `graphql:"url"` - } - } `graphql:"discussions(first: 100, categoryId: $categoryId, orderBy: {field: $orderByField, direction: $direction})"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - - - vars := map[string]interface{}{ - "owner": githubv4.String(owner), - "repo": githubv4.String(repo), - "categoryId": *categoryID, - "orderByField": githubv4.DiscussionOrderField(orderBy), - "direction": githubv4.OrderDirection(direction), - } - - - if err := client.Query(ctx, &query, vars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - // 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}, - UpdatedAt: &github.Timestamp{Time: n.UpdatedAt.Time}, - User: &github.User{ - Login: github.Ptr(string(n.Author.Login)), - }, - 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 - UpdatedAt githubv4.DateTime - Author struct { - Login githubv4.String - } - Category struct { - Name githubv4.String - } `graphql:"category"` - URL githubv4.String `graphql:"url"` - } - } `graphql:"discussions(first: 100, orderBy: {field: $orderByField, direction: $direction})"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } + client, err := getGQLClient(ctx) + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil + } + baseVars := map[string]interface{}{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + } - vars := map[string]interface{}{ - "owner": githubv4.String(owner), - "repo": githubv4.String(repo), - "orderByField": githubv4.DiscussionOrderField(orderBy), - "direction": githubv4.OrderDirection(direction), - } + // 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 { + orderObject := githubv4.DiscussionOrder{ + Field: githubv4.DiscussionOrderField(orderBy), + Direction: githubv4.OrderDirection(direction), + } + baseVars["orderBy"] = orderObject + } - if err := client.Query(ctx, &query, vars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } + var discussions []*github.Discussion + queries := &discussionQueries{} - // 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}, - UpdatedAt: &github.Timestamp{Time: n.UpdatedAt.Time}, - User: &github.User{ - Login: github.Ptr(string(n.Author.Login)), - }, - DiscussionCategory: &github.DiscussionCategory{ - Name: github.Ptr(string(n.Category.Name)), - }, - } - discussions = append(discussions, di) - } - } + if category != "" { + vars := make(map[string]interface{}) + for k, v := range baseVars { + vars[k] = v + } + vars["categoryId"] = githubv4.ID(category) + + if useOrdering { + log.Printf("GraphQL Query with category and order: %+v", queries.WithCategoryAndOrder) + log.Printf("GraphQL Variables: %+v", vars) + + if err := client.Query(ctx, &queries.WithCategoryAndOrder, vars); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + for _, node := range queries.WithCategoryAndOrder.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) + } + } else { + log.Printf("GraphQL Query with category no order: %+v", queries.WithCategoryNoOrder) + log.Printf("GraphQL Variables: %+v", vars) + + if err := client.Query(ctx, &queries.WithCategoryNoOrder, vars); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + for _, node := range queries.WithCategoryNoOrder.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) + } + } + } else { + if useOrdering { + log.Printf("GraphQL Query basic with order: %+v", queries.BasicWithOrder) + log.Printf("GraphQL Variables: %+v", baseVars) + + if err := client.Query(ctx, &queries.BasicWithOrder, baseVars); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + for _, node := range queries.BasicWithOrder.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) + } + } else { + log.Printf("GraphQL Query basic no order: %+v", queries.BasicNoOrder) + log.Printf("GraphQL Variables: %+v", baseVars) + + if err := client.Query(ctx, &queries.BasicNoOrder, baseVars); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + for _, node := range queries.BasicNoOrder.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) + } + } + } // Marshal and return out, err := json.Marshal(discussions) From 7eeb87cbd44fe2fb034ee6aecaae83d19ee64d78 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Thu, 17 Jul 2025 09:27:31 +0100 Subject: [PATCH 06/17] updated readme with new description --- README.md | 2 ++ 1 file changed, 2 insertions(+) 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) From 44f8f355d52240e3e97dbdfb3926cf5a688a591b Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Thu, 17 Jul 2025 09:42:02 +0100 Subject: [PATCH 07/17] restore original categoryID code, simplify vars management --- pkg/github/discussions.go | 209 +++++++++++++++++++------------------- 1 file changed, 106 insertions(+), 103 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 1ddadbcbc..6c6a2c775 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -31,36 +31,36 @@ type DiscussionFragment struct { type discussionQueries struct { BasicNoOrder struct { - Repository struct { - Discussions struct { - Nodes []DiscussionFragment - } `graphql:"discussions(first: 100)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - - BasicWithOrder struct { - Repository struct { - Discussions struct { - Nodes []DiscussionFragment - } `graphql:"discussions(first: 100, orderBy: $orderBy)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - - WithCategoryAndOrder struct { - Repository struct { - Discussions struct { - Nodes []DiscussionFragment - } `graphql:"discussions(first: 100, categoryId: $categoryId, orderBy: $orderBy)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - - WithCategoryNoOrder struct { - Repository struct { - Discussions struct { - Nodes []DiscussionFragment - } `graphql:"discussions(first: 100, categoryId: $categoryId)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } + Repository struct { + Discussions struct { + Nodes []DiscussionFragment + } `graphql:"discussions(first: 100)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + BasicWithOrder struct { + Repository struct { + Discussions struct { + Nodes []DiscussionFragment + } `graphql:"discussions(first: 100, orderBy: $orderBy)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + WithCategoryAndOrder struct { + Repository struct { + Discussions struct { + Nodes []DiscussionFragment + } `graphql:"discussions(first: 100, categoryId: $categoryId, orderBy: $orderBy)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + 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 { @@ -101,9 +101,9 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp 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"), + mcp.WithString("direction", + mcp.Description("Order direction."), + mcp.Enum("ASC", "DESC"), ), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { @@ -125,8 +125,8 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp if err != nil { return mcp.NewToolResultError(err.Error()), nil } - - direction, err := OptionalParam[string](request, "direction") + + direction, err := OptionalParam[string](request, "direction") if err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -136,82 +136,85 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp return mcp.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil } - baseVars := map[string]interface{}{ - "owner": githubv4.String(owner), - "repo": githubv4.String(repo), - } + var categoryID *githubv4.ID + if category != "" { + id := githubv4.ID(category) + categoryID = &id + } + + 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 != "" + useOrdering := orderBy != "" && direction != "" if useOrdering { - orderObject := githubv4.DiscussionOrder{ - Field: githubv4.DiscussionOrderField(orderBy), - Direction: githubv4.OrderDirection(direction), - } - baseVars["orderBy"] = orderObject - } + orderObject := githubv4.DiscussionOrder{ + Field: githubv4.DiscussionOrderField(orderBy), + Direction: githubv4.OrderDirection(direction), + } + vars["orderBy"] = orderObject + } + + if categoryID != nil { + vars["categoryId"] = githubv4.ID(categoryID) + } var discussions []*github.Discussion queries := &discussionQueries{} - if category != "" { - vars := make(map[string]interface{}) - for k, v := range baseVars { - vars[k] = v - } - vars["categoryId"] = githubv4.ID(category) - - if useOrdering { - log.Printf("GraphQL Query with category and order: %+v", queries.WithCategoryAndOrder) - log.Printf("GraphQL Variables: %+v", vars) - - if err := client.Query(ctx, &queries.WithCategoryAndOrder, vars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - - for _, node := range queries.WithCategoryAndOrder.Repository.Discussions.Nodes { - discussions = append(discussions, fragmentToDiscussion(node)) - } - } else { - log.Printf("GraphQL Query with category no order: %+v", queries.WithCategoryNoOrder) - log.Printf("GraphQL Variables: %+v", vars) - - if err := client.Query(ctx, &queries.WithCategoryNoOrder, vars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - - for _, node := range queries.WithCategoryNoOrder.Repository.Discussions.Nodes { - discussions = append(discussions, fragmentToDiscussion(node)) - } - } - } else { - if useOrdering { - log.Printf("GraphQL Query basic with order: %+v", queries.BasicWithOrder) - log.Printf("GraphQL Variables: %+v", baseVars) - - if err := client.Query(ctx, &queries.BasicWithOrder, baseVars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - - for _, node := range queries.BasicWithOrder.Repository.Discussions.Nodes { - discussions = append(discussions, fragmentToDiscussion(node)) - } - } else { - log.Printf("GraphQL Query basic no order: %+v", queries.BasicNoOrder) - log.Printf("GraphQL Variables: %+v", baseVars) - - if err := client.Query(ctx, &queries.BasicNoOrder, baseVars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - - for _, node := range queries.BasicNoOrder.Repository.Discussions.Nodes { - discussions = append(discussions, fragmentToDiscussion(node)) - } - } - } - - // Marshal and return + if categoryID != nil { + if useOrdering { + log.Printf("GraphQL Query with category and order: %+v", queries.WithCategoryAndOrder) + log.Printf("GraphQL Variables: %+v", vars) + + if err := client.Query(ctx, &queries.WithCategoryAndOrder, vars); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + for _, node := range queries.WithCategoryAndOrder.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) + } + } else { + log.Printf("GraphQL Query with category no order: %+v", queries.WithCategoryNoOrder) + log.Printf("GraphQL Variables: %+v", vars) + + if err := client.Query(ctx, &queries.WithCategoryNoOrder, vars); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + for _, node := range queries.WithCategoryNoOrder.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) + } + } + } else { + if useOrdering { + log.Printf("GraphQL Query basic with order: %+v", queries.BasicWithOrder) + log.Printf("GraphQL Variables: %+v", vars) + + if err := client.Query(ctx, &queries.BasicWithOrder, vars); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + for _, node := range queries.BasicWithOrder.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) + } + } else { + log.Printf("GraphQL Query basic no order: %+v", queries.BasicNoOrder) + log.Printf("GraphQL Variables: %+v", vars) + + if err := client.Query(ctx, &queries.BasicNoOrder, vars); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + for _, node := range queries.BasicNoOrder.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) + } + } + } + out, err := json.Marshal(discussions) if err != nil { return nil, fmt.Errorf("failed to marshal discussions: %w", err) From bcb82c81992ded877522593dd30c664ae3ff7a82 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Thu, 17 Jul 2025 09:54:09 +0100 Subject: [PATCH 08/17] quick fix --- pkg/github/discussions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 6c6a2c775..5a40be66e 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -159,7 +159,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } if categoryID != nil { - vars["categoryId"] = githubv4.ID(categoryID) + vars["categoryId"] = *categoryID } var discussions []*github.Discussion From 751dfa593375171bb546c055728616e3ea865d27 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Thu, 17 Jul 2025 10:12:13 +0100 Subject: [PATCH 09/17] update tests to account for recent changes (author login, updated at date) --- pkg/github/discussions_test.go | 65 +++++++++++++++++++++++++--------- 1 file changed, 49 insertions(+), 16 deletions(-) diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 5132c6ce0..0d5040d12 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -17,13 +17,37 @@ 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"}, + }, } mockResponseListAll = githubv4mock.DataResponse(map[string]any{ "repository": map[string]any{ @@ -48,15 +72,19 @@ func Test_ListDiscussions(t *testing.T) { assert.Contains(t, toolDef.InputSchema.Properties, "repo") assert.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo"}) - // mock for the call to ListDiscussions without category filter - var qDiscussions struct { + // Mock for BasicNoOrder query + var qBasicNoOrder struct { Repository struct { Discussions struct { Nodes []struct { Number githubv4.Int Title githubv4.String CreatedAt githubv4.DateTime - Category struct { + UpdatedAt githubv4.DateTime + Author struct { + Login githubv4.String + } + Category struct { Name githubv4.String } `graphql:"category"` URL githubv4.String `graphql:"url"` @@ -65,15 +93,19 @@ func Test_ListDiscussions(t *testing.T) { } `graphql:"repository(owner: $owner, name: $repo)"` } - // mock for the call to get discussions with category filter - var qDiscussionsFiltered struct { + // Mock for WithCategoryNoOrder query + var qWithCategoryNoOrder struct { Repository struct { Discussions struct { Nodes []struct { Number githubv4.Int Title githubv4.String CreatedAt githubv4.DateTime - Category struct { + UpdatedAt githubv4.DateTime // Added + Author struct { // Added + Login githubv4.String + } + Category struct { Name githubv4.String } `graphql:"category"` URL githubv4.String `graphql:"url"` @@ -141,15 +173,16 @@ 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(qBasicNoOrder, 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(qWithCategoryNoOrder, varsDiscussionsFiltered, mockResponseListGeneral) httpClient = githubv4mock.NewMockedHTTPClient(matcher) + // BasicNoOrder case "repository not found error": - matcher := githubv4mock.NewQueryMatcher(qDiscussions, varsRepoNotFound, mockErrorRepoNotFound) + matcher := githubv4mock.NewQueryMatcher(qBasicNoOrder, varsRepoNotFound, mockErrorRepoNotFound) httpClient = githubv4mock.NewMockedHTTPClient(matcher) } From b064f7acc24a3cb559a83eec72c50cffb780e8db Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Thu, 17 Jul 2025 12:40:06 +0100 Subject: [PATCH 10/17] use switch statement for better readability --- pkg/github/discussions.go | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 5a40be66e..823f7aaf1 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -163,10 +163,13 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } var discussions []*github.Discussion - queries := &discussionQueries{} + queries := &discussionQueries{} - if categoryID != nil { - if useOrdering { + // we need to check what user inputs we received at runtime, and use the + // most appropriate query + switch { + // use query WithCategoryAndOrder + case categoryID != nil && useOrdering: log.Printf("GraphQL Query with category and order: %+v", queries.WithCategoryAndOrder) log.Printf("GraphQL Variables: %+v", vars) @@ -177,7 +180,9 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp for _, node := range queries.WithCategoryAndOrder.Repository.Discussions.Nodes { discussions = append(discussions, fragmentToDiscussion(node)) } - } else { + + // use query WithCategoryNoOrder + case categoryID != nil && !useOrdering: log.Printf("GraphQL Query with category no order: %+v", queries.WithCategoryNoOrder) log.Printf("GraphQL Variables: %+v", vars) @@ -188,9 +193,9 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp for _, node := range queries.WithCategoryNoOrder.Repository.Discussions.Nodes { discussions = append(discussions, fragmentToDiscussion(node)) } - } - } else { - if useOrdering { + + // use query BasicWithOrder + case categoryID == nil && useOrdering: log.Printf("GraphQL Query basic with order: %+v", queries.BasicWithOrder) log.Printf("GraphQL Variables: %+v", vars) @@ -201,7 +206,9 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp for _, node := range queries.BasicWithOrder.Repository.Discussions.Nodes { discussions = append(discussions, fragmentToDiscussion(node)) } - } else { + + // use query BasicNoOrder + case categoryID == nil && !useOrdering: log.Printf("GraphQL Query basic no order: %+v", queries.BasicNoOrder) log.Printf("GraphQL Variables: %+v", vars) @@ -212,8 +219,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp for _, node := range queries.BasicNoOrder.Repository.Discussions.Nodes { discussions = append(discussions, fragmentToDiscussion(node)) } - } - } + } out, err := json.Marshal(discussions) if err != nil { From 6ab51379d0ef487a162874a1a42936305de8866b Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Thu, 17 Jul 2025 12:44:13 +0100 Subject: [PATCH 11/17] remove comment --- pkg/github/discussions_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 0d5040d12..23f2a22c4 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -101,8 +101,8 @@ func Test_ListDiscussions(t *testing.T) { Number githubv4.Int Title githubv4.String CreatedAt githubv4.DateTime - UpdatedAt githubv4.DateTime // Added - Author struct { // Added + UpdatedAt githubv4.DateTime + Author struct { Login githubv4.String } Category struct { From 25d39bec1187db199e65d1c3905b3953cbf802f4 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Thu, 17 Jul 2025 12:44:32 +0100 Subject: [PATCH 12/17] linting --- pkg/github/discussions.go | 114 ++++++++++++++++----------------- pkg/github/discussions_test.go | 4 +- 2 files changed, 59 insertions(+), 59 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 823f7aaf1..b177298e3 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -163,63 +163,63 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } var discussions []*github.Discussion - queries := &discussionQueries{} - - // we need to check what user inputs we received at runtime, and use the - // most appropriate query - switch { - // use query WithCategoryAndOrder - case categoryID != nil && useOrdering: - log.Printf("GraphQL Query with category and order: %+v", queries.WithCategoryAndOrder) - log.Printf("GraphQL Variables: %+v", vars) - - if err := client.Query(ctx, &queries.WithCategoryAndOrder, vars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - - for _, node := range queries.WithCategoryAndOrder.Repository.Discussions.Nodes { - discussions = append(discussions, fragmentToDiscussion(node)) - } - - // use query WithCategoryNoOrder - case categoryID != nil && !useOrdering: - log.Printf("GraphQL Query with category no order: %+v", queries.WithCategoryNoOrder) - log.Printf("GraphQL Variables: %+v", vars) - - if err := client.Query(ctx, &queries.WithCategoryNoOrder, vars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - - for _, node := range queries.WithCategoryNoOrder.Repository.Discussions.Nodes { - discussions = append(discussions, fragmentToDiscussion(node)) - } - - // use query BasicWithOrder - case categoryID == nil && useOrdering: - log.Printf("GraphQL Query basic with order: %+v", queries.BasicWithOrder) - log.Printf("GraphQL Variables: %+v", vars) - - if err := client.Query(ctx, &queries.BasicWithOrder, vars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - - for _, node := range queries.BasicWithOrder.Repository.Discussions.Nodes { - discussions = append(discussions, fragmentToDiscussion(node)) - } - - // use query BasicNoOrder - case categoryID == nil && !useOrdering: - log.Printf("GraphQL Query basic no order: %+v", queries.BasicNoOrder) - log.Printf("GraphQL Variables: %+v", vars) - - if err := client.Query(ctx, &queries.BasicNoOrder, vars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - - for _, node := range queries.BasicNoOrder.Repository.Discussions.Nodes { - discussions = append(discussions, fragmentToDiscussion(node)) - } - } + queries := &discussionQueries{} + + // we need to check what user inputs we received at runtime, and use the + // most appropriate query + switch { + // use query WithCategoryAndOrder + case categoryID != nil && useOrdering: + log.Printf("GraphQL Query with category and order: %+v", queries.WithCategoryAndOrder) + log.Printf("GraphQL Variables: %+v", vars) + + if err := client.Query(ctx, &queries.WithCategoryAndOrder, vars); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + for _, node := range queries.WithCategoryAndOrder.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) + } + + // use query WithCategoryNoOrder + case categoryID != nil && !useOrdering: + log.Printf("GraphQL Query with category no order: %+v", queries.WithCategoryNoOrder) + log.Printf("GraphQL Variables: %+v", vars) + + if err := client.Query(ctx, &queries.WithCategoryNoOrder, vars); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + for _, node := range queries.WithCategoryNoOrder.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) + } + + // use query BasicWithOrder + case categoryID == nil && useOrdering: + log.Printf("GraphQL Query basic with order: %+v", queries.BasicWithOrder) + log.Printf("GraphQL Variables: %+v", vars) + + if err := client.Query(ctx, &queries.BasicWithOrder, vars); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + for _, node := range queries.BasicWithOrder.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) + } + + // use query BasicNoOrder + case categoryID == nil && !useOrdering: + log.Printf("GraphQL Query basic no order: %+v", queries.BasicNoOrder) + log.Printf("GraphQL Variables: %+v", vars) + + if err := client.Query(ctx, &queries.BasicNoOrder, vars); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + for _, node := range queries.BasicNoOrder.Repository.Discussions.Nodes { + discussions = append(discussions, fragmentToDiscussion(node)) + } + } out, err := json.Marshal(discussions) if err != nil { diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 23f2a22c4..8d56f6727 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -101,8 +101,8 @@ func Test_ListDiscussions(t *testing.T) { Number githubv4.Int Title githubv4.String CreatedAt githubv4.DateTime - UpdatedAt githubv4.DateTime - Author struct { + UpdatedAt githubv4.DateTime + Author struct { Login githubv4.String } Category struct { From 14dcf32c183e5d2d733b6e1a70e963a9466886b7 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Thu, 17 Jul 2025 16:18:22 +0100 Subject: [PATCH 13/17] refactored logic, simplified switch statement --- pkg/github/discussions.go | 139 +++++++++++++++++++------------------- 1 file changed, 68 insertions(+), 71 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index b177298e3..2afba523c 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -14,7 +14,6 @@ import ( "github.com/shurcooL/githubv4" ) -// Define reusable fragments for discussions type DiscussionFragment struct { Number githubv4.Int Title githubv4.String @@ -29,40 +28,43 @@ type DiscussionFragment struct { URL githubv4.String `graphql:"url"` } -type discussionQueries struct { - BasicNoOrder struct { - Repository struct { - Discussions struct { - Nodes []DiscussionFragment - } `graphql:"discussions(first: 100)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } +type BasicNoOrder struct { + Repository struct { + Discussions struct { + Nodes []DiscussionFragment + } `graphql:"discussions(first: 100)"` + } `graphql:"repository(owner: $owner, name: $repo)"` +} - BasicWithOrder struct { - Repository struct { - Discussions struct { - Nodes []DiscussionFragment - } `graphql:"discussions(first: 100, orderBy: $orderBy)"` - } `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)"` +} - WithCategoryAndOrder struct { - Repository struct { - Discussions struct { - Nodes []DiscussionFragment - } `graphql:"discussions(first: 100, categoryId: $categoryId, orderBy: $orderBy)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - WithCategoryNoOrder struct { - Repository struct { - Discussions struct { - Nodes []DiscussionFragment - } `graphql:"discussions(first: 100, categoryId: $categoryId)"` - } `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)), @@ -79,6 +81,19 @@ func fragmentToDiscussion(fragment DiscussionFragment) *github.Discussion { } } +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")), @@ -151,11 +166,8 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp // we shouldn't use ordering unless both a 'field' and 'direction' are provided useOrdering := orderBy != "" && direction != "" if useOrdering { - orderObject := githubv4.DiscussionOrder{ - Field: githubv4.DiscussionOrderField(orderBy), - Direction: githubv4.OrderDirection(direction), - } - vars["orderBy"] = orderObject + vars["orderByField"] = githubv4.DiscussionOrderField(orderBy) + vars["orderByDirection"] = githubv4.OrderDirection(direction) } if categoryID != nil { @@ -163,60 +175,45 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp } var discussions []*github.Discussion - queries := &discussionQueries{} + 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 - switch { - // use query WithCategoryAndOrder - case categoryID != nil && useOrdering: - log.Printf("GraphQL Query with category and order: %+v", queries.WithCategoryAndOrder) + // most appropriate query based on that + switch queryType := discussionQuery.(type) { + case *WithCategoryAndOrder: + log.Printf("GraphQL Query with category and order: %+v", queryType) log.Printf("GraphQL Variables: %+v", vars) - - if err := client.Query(ctx, &queries.WithCategoryAndOrder, vars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - - for _, node := range queries.WithCategoryAndOrder.Repository.Discussions.Nodes { + + for _, node := range queryType.Repository.Discussions.Nodes { discussions = append(discussions, fragmentToDiscussion(node)) } - // use query WithCategoryNoOrder - case categoryID != nil && !useOrdering: - log.Printf("GraphQL Query with category no order: %+v", queries.WithCategoryNoOrder) + case *WithCategoryNoOrder: + log.Printf("GraphQL Query with category no order: %+v", queryType) log.Printf("GraphQL Variables: %+v", vars) - if err := client.Query(ctx, &queries.WithCategoryNoOrder, vars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - - for _, node := range queries.WithCategoryNoOrder.Repository.Discussions.Nodes { + for _, node := range queryType.Repository.Discussions.Nodes { discussions = append(discussions, fragmentToDiscussion(node)) } - // use query BasicWithOrder - case categoryID == nil && useOrdering: - log.Printf("GraphQL Query basic with order: %+v", queries.BasicWithOrder) + case *BasicWithOrder: + log.Printf("GraphQL Query basic with order: %+v", queryType) log.Printf("GraphQL Variables: %+v", vars) - if err := client.Query(ctx, &queries.BasicWithOrder, vars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - - for _, node := range queries.BasicWithOrder.Repository.Discussions.Nodes { + for _, node := range queryType.Repository.Discussions.Nodes { discussions = append(discussions, fragmentToDiscussion(node)) } - // use query BasicNoOrder - case categoryID == nil && !useOrdering: - log.Printf("GraphQL Query basic no order: %+v", queries.BasicNoOrder) - log.Printf("GraphQL Variables: %+v", vars) - if err := client.Query(ctx, &queries.BasicNoOrder, vars); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } + case *BasicNoOrder: + log.Printf("GraphQL Query basic no order: %+v", queryType) + log.Printf("GraphQL Variables: %+v", vars) - for _, node := range queries.BasicNoOrder.Repository.Discussions.Nodes { + for _, node := range queryType.Repository.Discussions.Nodes { discussions = append(discussions, fragmentToDiscussion(node)) } } From 49d0deaca96690c28c662da78b72ace39041e184 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Thu, 17 Jul 2025 16:41:50 +0100 Subject: [PATCH 14/17] linting --- pkg/github/discussions.go | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 2afba523c..cb6ab0359 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -44,8 +44,6 @@ type BasicWithOrder struct { } `graphql:"repository(owner: $owner, name: $repo)"` } - - type WithCategoryAndOrder struct { Repository struct { Discussions struct { @@ -54,7 +52,6 @@ type WithCategoryAndOrder struct { } `graphql:"repository(owner: $owner, name: $repo)"` } - type WithCategoryNoOrder struct { Repository struct { Discussions struct { @@ -63,8 +60,6 @@ type WithCategoryNoOrder struct { } `graphql:"repository(owner: $owner, name: $repo)"` } - - func fragmentToDiscussion(fragment DiscussionFragment) *github.Discussion { return &github.Discussion{ Number: github.Ptr(int(fragment.Number)), @@ -81,17 +76,17 @@ func fragmentToDiscussion(fragment DiscussionFragment) *github.Discussion { } } -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 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) { @@ -187,7 +182,7 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp case *WithCategoryAndOrder: log.Printf("GraphQL Query with category and order: %+v", queryType) log.Printf("GraphQL Variables: %+v", vars) - + for _, node := range queryType.Repository.Discussions.Nodes { discussions = append(discussions, fragmentToDiscussion(node)) } @@ -208,7 +203,6 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp discussions = append(discussions, fragmentToDiscussion(node)) } - case *BasicNoOrder: log.Printf("GraphQL Query basic no order: %+v", queryType) log.Printf("GraphQL Variables: %+v", vars) From eb78fe9682e425b16fdac2b2257be7e83ae47e2d Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Thu, 17 Jul 2025 17:31:19 +0100 Subject: [PATCH 15/17] use original queries from discussions list tool for testing --- pkg/github/discussions_test.go | 507 ++++++++++++++++++++------------- 1 file changed, 311 insertions(+), 196 deletions(-) diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 8d56f6727..26455c902 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -16,205 +16,320 @@ import ( ) var ( - discussionsGeneral = []map[string]any{ - {"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", - "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"}, - }, - } - mockResponseListAll = githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "discussions": map[string]any{"nodes": discussionsAll}, - }, - }) - mockResponseListGeneral = githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "discussions": map[string]any{"nodes": discussionsGeneral}, - }, - }) - mockErrorRepoNotFound = githubv4mock.ErrorResponse("repository not found") + discussionsGeneral = []map[string]any{ + {"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", + "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}, + }, + }) + mockResponseListGeneral = githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "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.ElementsMatch(t, toolDef.InputSchema.Required, []string{"owner", "repo"}) - - // Mock for BasicNoOrder query - var qBasicNoOrder struct { - Repository struct { - Discussions struct { - Nodes []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"` - } - } `graphql:"discussions(first: 100)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - - // Mock for WithCategoryNoOrder query - var qWithCategoryNoOrder struct { - Repository struct { - Discussions struct { - Nodes []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"` - } - } `graphql:"discussions(first: 100, categoryId: $categoryId)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - - varsListAll := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - } - - varsRepoNotFound := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("nonexistent-repo"), - } - - varsDiscussionsFiltered := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - "categoryId": githubv4.ID("DIC_kwDOABC123"), - } - - tests := []struct { - name string - reqParams map[string]interface{} - expectError bool - errContains string - expectedCount int - }{ - { - name: "list all discussions without category filter", - reqParams: map[string]interface{}{ - "owner": "owner", - "repo": "repo", - }, - expectError: false, - expectedCount: 3, // All discussions - }, - { - name: "filter by category ID", - reqParams: map[string]interface{}{ - "owner": "owner", - "repo": "repo", - "category": "DIC_kwDOABC123", - }, - expectError: false, - expectedCount: 2, // Only General discussions (matching the category ID) - }, - { - name: "repository not found error", - reqParams: map[string]interface{}{ - "owner": "owner", - "repo": "nonexistent-repo", - }, - expectError: true, - errContains: "repository not found", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - var httpClient *http.Client - - switch tc.name { - case "list all discussions without category filter": - // Simple case - BasicNoOrder query structure (i.e. no order, no category) - matcher := githubv4mock.NewQueryMatcher(qBasicNoOrder, varsListAll, mockResponseListAll) - httpClient = githubv4mock.NewMockedHTTPClient(matcher) - case "filter by category ID": - // WithCategoryNoOrder - matcher := githubv4mock.NewQueryMatcher(qWithCategoryNoOrder, varsDiscussionsFiltered, mockResponseListGeneral) - httpClient = githubv4mock.NewMockedHTTPClient(matcher) - // BasicNoOrder - case "repository not found error": - matcher := githubv4mock.NewQueryMatcher(qBasicNoOrder, varsRepoNotFound, mockErrorRepoNotFound) - httpClient = githubv4mock.NewMockedHTTPClient(matcher) - } - - gqlClient := githubv4.NewClient(httpClient) - _, handler := ListDiscussions(stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper) - - req := createMCPRequest(tc.reqParams) - res, err := handler(context.Background(), req) - text := getTextResult(t, res).Text - - if tc.expectError { - require.True(t, res.IsError) - assert.Contains(t, text, tc.errContains) - return - } - require.NoError(t, err) - - var returnedDiscussions []*github.Discussion - err = json.Unmarshal([]byte(text), &returnedDiscussions) - require.NoError(t, err) - - assert.Len(t, returnedDiscussions, tc.expectedCount, "Expected %d discussions, got %d", tc.expectedCount, len(returnedDiscussions)) - - // Verify that all returned discussions have a category if filtered - if _, hasCategory := tc.reqParams["category"]; hasCategory { - for _, discussion := range returnedDiscussions { - require.NotNil(t, discussion.DiscussionCategory, "Discussion should have category") - assert.NotEmpty(t, *discussion.DiscussionCategory.Name, "Discussion should have category name") - } - } - }) - } + mockClient := githubv4.NewClient(nil) + 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"}) + + varsListAll := map[string]interface{}{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + } + + varsRepoNotFound := map[string]interface{}{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("nonexistent-repo"), + } + + varsDiscussionsFiltered := map[string]interface{}{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "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", + reqParams: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + }, + expectError: false, + expectedCount: 3, // All discussions + }, + { + name: "filter by category ID", + reqParams: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "category": "DIC_kwDOABC123", + }, + 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{}{ + "owner": "owner", + "repo": "nonexistent-repo", + }, + expectError: true, + errContains: "repository not found", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var httpClient *http.Client + + switch tc.name { + case "list all discussions without category filter": + // 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": + // 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(&BasicNoOrder{}, varsRepoNotFound, mockErrorRepoNotFound) + httpClient = githubv4mock.NewMockedHTTPClient(matcher) + } + + gqlClient := githubv4.NewClient(httpClient) + _, handler := ListDiscussions(stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper) + + req := createMCPRequest(tc.reqParams) + res, err := handler(context.Background(), req) + text := getTextResult(t, res).Text + + if tc.expectError { + require.True(t, res.IsError) + assert.Contains(t, text, tc.errContains) + return + } + require.NoError(t, err) + + var returnedDiscussions []*github.Discussion + err = json.Unmarshal([]byte(text), &returnedDiscussions) + require.NoError(t, err) + + 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 { + require.NotNil(t, discussion.DiscussionCategory, "Discussion should have category") + assert.NotEmpty(t, *discussion.DiscussionCategory.Name, "Discussion should have category name") + } + } + }) + } } func Test_GetDiscussion(t *testing.T) { From d49966841f347a54545265f1367c6dc05b5c2f69 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Thu, 17 Jul 2025 17:31:37 +0100 Subject: [PATCH 16/17] linting --- pkg/github/discussions_test.go | 620 ++++++++++++++++----------------- 1 file changed, 310 insertions(+), 310 deletions(-) diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 26455c902..997944572 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -16,320 +16,320 @@ import ( ) var ( - discussionsGeneral = []map[string]any{ - {"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", - "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) - } - + discussionsGeneral = []map[string]any{ + {"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", + "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}, - }, - }) - mockResponseListGeneral = githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "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") + 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}, + }, + }) + mockResponseListGeneral = githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "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) - 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"}) - - varsListAll := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - } - - varsRepoNotFound := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("nonexistent-repo"), - } - - varsDiscussionsFiltered := map[string]interface{}{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - "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", - reqParams: map[string]interface{}{ - "owner": "owner", - "repo": "repo", - }, - expectError: false, - expectedCount: 3, // All discussions - }, - { - name: "filter by category ID", - reqParams: map[string]interface{}{ - "owner": "owner", - "repo": "repo", - "category": "DIC_kwDOABC123", - }, - 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{}{ - "owner": "owner", - "repo": "nonexistent-repo", - }, - expectError: true, - errContains: "repository not found", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - var httpClient *http.Client - - switch tc.name { - case "list all discussions without category filter": - // 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": - // 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(&BasicNoOrder{}, varsRepoNotFound, mockErrorRepoNotFound) - httpClient = githubv4mock.NewMockedHTTPClient(matcher) - } - - gqlClient := githubv4.NewClient(httpClient) - _, handler := ListDiscussions(stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper) - - req := createMCPRequest(tc.reqParams) - res, err := handler(context.Background(), req) - text := getTextResult(t, res).Text - - if tc.expectError { - require.True(t, res.IsError) - assert.Contains(t, text, tc.errContains) - return - } - require.NoError(t, err) - - var returnedDiscussions []*github.Discussion - err = json.Unmarshal([]byte(text), &returnedDiscussions) - require.NoError(t, err) - - 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 { - require.NotNil(t, discussion.DiscussionCategory, "Discussion should have category") - assert.NotEmpty(t, *discussion.DiscussionCategory.Name, "Discussion should have category name") - } - } - }) - } + mockClient := githubv4.NewClient(nil) + 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"}) + + varsListAll := map[string]interface{}{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + } + + varsRepoNotFound := map[string]interface{}{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("nonexistent-repo"), + } + + varsDiscussionsFiltered := map[string]interface{}{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "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", + reqParams: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + }, + expectError: false, + expectedCount: 3, // All discussions + }, + { + name: "filter by category ID", + reqParams: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "category": "DIC_kwDOABC123", + }, + 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{}{ + "owner": "owner", + "repo": "nonexistent-repo", + }, + expectError: true, + errContains: "repository not found", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var httpClient *http.Client + + switch tc.name { + case "list all discussions without category filter": + // 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": + // 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(&BasicNoOrder{}, varsRepoNotFound, mockErrorRepoNotFound) + httpClient = githubv4mock.NewMockedHTTPClient(matcher) + } + + gqlClient := githubv4.NewClient(httpClient) + _, handler := ListDiscussions(stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper) + + req := createMCPRequest(tc.reqParams) + res, err := handler(context.Background(), req) + text := getTextResult(t, res).Text + + if tc.expectError { + require.True(t, res.IsError) + assert.Contains(t, text, tc.errContains) + return + } + require.NoError(t, err) + + var returnedDiscussions []*github.Discussion + err = json.Unmarshal([]byte(text), &returnedDiscussions) + require.NoError(t, err) + + 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 { + require.NotNil(t, discussion.DiscussionCategory, "Discussion should have category") + assert.NotEmpty(t, *discussion.DiscussionCategory.Name, "Discussion should have category name") + } + } + }) + } } func Test_GetDiscussion(t *testing.T) { From de8583ccfefee7bd536cdc51e3bfcdde56ec3fae Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Thu, 17 Jul 2025 18:10:55 +0100 Subject: [PATCH 17/17] remove logging --- pkg/github/discussions.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index cb6ab0359..14c59bd29 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "fmt" - "log" "github.com/github/github-mcp-server/pkg/translations" "github.com/go-viper/mapstructure/v2" @@ -180,33 +179,21 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp // most appropriate query based on that switch queryType := discussionQuery.(type) { case *WithCategoryAndOrder: - log.Printf("GraphQL Query with category and order: %+v", queryType) - log.Printf("GraphQL Variables: %+v", vars) - for _, node := range queryType.Repository.Discussions.Nodes { discussions = append(discussions, fragmentToDiscussion(node)) } case *WithCategoryNoOrder: - log.Printf("GraphQL Query with category no order: %+v", queryType) - log.Printf("GraphQL Variables: %+v", vars) - for _, node := range queryType.Repository.Discussions.Nodes { discussions = append(discussions, fragmentToDiscussion(node)) } case *BasicWithOrder: - log.Printf("GraphQL Query basic with order: %+v", queryType) - log.Printf("GraphQL Variables: %+v", vars) - for _, node := range queryType.Repository.Discussions.Nodes { discussions = append(discussions, fragmentToDiscussion(node)) } case *BasicNoOrder: - log.Printf("GraphQL Query basic no order: %+v", queryType) - log.Printf("GraphQL Variables: %+v", vars) - for _, node := range queryType.Repository.Discussions.Nodes { discussions = append(discussions, fragmentToDiscussion(node)) }