Skip to content

Commit 2cb0618

Browse files
sd2kclaude
sd2k
and
claude
authored
fix(tools): small tool bugs from #1305 (#1313)
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 6055dfc commit 2cb0618

22 files changed

Lines changed: 386 additions & 48 deletions

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -303,7 +303,7 @@ Scopes define the specific resources that permissions apply to. Each action requ
303303
| Tool | Category | Description | Required RBAC Permissions | Required Scopes |
304304
| --------------------------------- | ------------------------- | ------------------------------------------------------------------------------------------------------------ | ------------------------------------------------------ | --------------------------------------------------- |
305305
| `list_teams` | Admin | List all teams | `teams:read` | `teams:*` or `teams:id:1` |
306-
| `list_users_by_org` | Admin | List all users in an organization | `users:read` | `global.users:*` or `global.users:id:123` |
306+
| `list_users_by_org` | Admin | List users in an organization, with search and pagination | `users:read` | `global.users:*` or `global.users:id:123` |
307307
| `list_all_roles` | Admin | List all Grafana roles | `roles:read` | `roles:*` |
308308
| `get_role_details` | Admin | Get details for a Grafana role | `roles:read` | `roles:uid:editor` |
309309
| `get_role_assignments` | Admin | List assignments for a role | `roles:read` | `roles:uid:editor` |

‎docs/sources/reference/mcp-tools-table.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ The following table lists MCP tools, required RBAC permissions, and typical scop
3636
| Tool | Category | Description | Required RBAC Permissions | Required Scopes |
3737
| --------------------------------- | -------------- | ------------------------------------------------------------------------------------------------------------ | ------------------------------------------------------ | --------------------------------------------------- |
3838
| `list_teams` | Admin* | List all teams | `teams:read` | `teams:*` or `teams:id:1` |
39-
| `list_users_by_org` | Admin* | List all users in an organization | `users:read` | `global.users:*` or `global.users:id:123` |
39+
| `list_users_by_org` | Admin* | List users in an organization, with search and pagination | `users:read` | `global.users:*` or `global.users:id:123` |
4040
| `list_all_roles` | Admin* | List all Grafana roles | `roles:read` | `roles:*` |
4141
| `get_role_details` | Admin* | Get details for a Grafana role | `roles:read` | `roles:uid:editor` |
4242
| `get_role_assignments` | Admin* | List assignments for a role | `roles:read` | `roles:uid:editor` |

‎tools/admin.go‎

Lines changed: 52 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,14 @@ package tools
33
import (
44
"context"
55
"fmt"
6+
"strconv"
67

8+
"github.com/go-openapi/runtime"
9+
"github.com/go-openapi/strfmt"
710
"github.com/modelcontextprotocol/go-sdk/mcp"
811

912
"github.com/grafana/grafana-openapi-client-go/client/access_control"
10-
"github.com/grafana/grafana-openapi-client-go/client/org"
13+
"github.com/grafana/grafana-openapi-client-go/client/orgs"
1114
"github.com/grafana/grafana-openapi-client-go/client/teams"
1215
"github.com/grafana/grafana-openapi-client-go/models"
1316
mcpgrafana "github.com/grafana/mcp-grafana/v2"
@@ -41,22 +44,64 @@ var ListTeams = mcpgrafana.MustTool(
4144
mcpgrafana.WithOpenWorldHintAnnotation(false),
4245
)
4346

44-
type ListUsersByOrgParams struct{}
47+
const (
48+
defaultListUsersByOrgLimit = 100
49+
maxListUsersByOrgLimit = 1000
50+
)
51+
52+
type ListUsersByOrgParams struct {
53+
Query string `json:"query,omitempty" jsonschema:"description=Optional search string matched against login\\, email and name"`
54+
Limit int `json:"limit,omitempty" jsonschema:"default=100,description=Maximum number of users to return (max 1000)"`
55+
Page int `json:"page,omitempty" jsonschema:"default=1,description=Page number for pagination (1-indexed)"`
56+
}
4557

46-
func listUsersByOrg(ctx context.Context, args ListUsersByOrgParams) ([]*models.OrgUserDTO, error) {
58+
func listUsersByOrg(ctx context.Context, args ListUsersByOrgParams) (*models.SearchOrgUsersQueryResult, error) {
4759
c := mcpgrafana.GrafanaClientFromContext(ctx)
4860

49-
params := org.NewGetOrgUsersForCurrentOrgParamsWithContext(ctx)
50-
search, err := c.Org.GetOrgUsersForCurrentOrg(params)
61+
limit := args.Limit
62+
if limit <= 0 {
63+
limit = defaultListUsersByOrgLimit
64+
}
65+
if limit > maxListUsersByOrgLimit {
66+
limit = maxListUsersByOrgLimit
67+
}
68+
page := args.Page
69+
if page <= 0 {
70+
page = 1
71+
}
72+
73+
// The client has no method for the current-org search endpoint
74+
// (GET /api/org/users/search), so reuse the admin search operation, which
75+
// has the same response shape, and point it at the current-org path.
76+
params := orgs.NewSearchOrgUsersParamsWithContext(ctx)
77+
resp, err := c.Orgs.SearchOrgUsersWithParams(params, func(op *runtime.ClientOperation) {
78+
op.ID = "searchOrgUsersForCurrentOrg"
79+
op.PathPattern = "/org/users/search"
80+
op.Params = runtime.ClientRequestWriterFunc(func(r runtime.ClientRequest, reg strfmt.Registry) error {
81+
// Keeps the default timeout; the unused org_id path param is ignored.
82+
if err := params.WriteToRequest(r, reg); err != nil {
83+
return err
84+
}
85+
if args.Query != "" {
86+
if err := r.SetQueryParam("query", args.Query); err != nil {
87+
return err
88+
}
89+
}
90+
if err := r.SetQueryParam("perpage", strconv.Itoa(limit)); err != nil {
91+
return err
92+
}
93+
return r.SetQueryParam("page", strconv.Itoa(page))
94+
})
95+
})
5196
if err != nil {
5297
return nil, fmt.Errorf("search users: %w", err)
5398
}
54-
return search.Payload, nil
99+
return resp.Payload, nil
55100
}
56101

57102
var ListUsersByOrg = mcpgrafana.MustTool(
58103
"list_users_by_org",
59-
"List users in the Grafana organization. Returns a list of organization users with details like userid, email, role etc.",
104+
"List users in the Grafana organization, optionally filtered by a search query. Results are paginated; the response includes totalCount, page and perPage alongside organization users with details like userid, email, role etc.",
60105
listUsersByOrg,
61106
mcpgrafana.WithTitleAnnotation("List users by org"),
62107
mcpgrafana.WithIdempotentHintAnnotation(true),

‎tools/admin_integration_test.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,10 @@ func TestAdminToolsIntegration(t *testing.T) {
4343

4444
users, err := listUsersByOrg(ctx, ListUsersByOrgParams{})
4545
require.NoError(t, err)
46-
assert.NotEmpty(t, users, "Should return at least one user")
46+
assert.NotEmpty(t, users.OrgUsers, "Should return at least one user")
47+
assert.Positive(t, users.TotalCount)
4748

48-
firstUser := users[0]
49+
firstUser := users.OrgUsers[0]
4950
userRoles, err := listUserRoles(ctx, ListUserRolesParams{UserIDs: []int64{firstUser.UserID}})
5051
require.NoError(t, err)
5152
assert.NotNil(t, userRoles)

‎tools/admin_test.go‎

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@ package tools
55

66
import (
77
"context"
8+
"encoding/json"
9+
"net/http"
10+
"net/http/httptest"
811
"testing"
912

1013
mcpgrafana "github.com/grafana/mcp-grafana/v2"
@@ -58,7 +61,7 @@ func TestAdminToolsUnit(t *testing.T) {
5861
permParams := GetResourcePermissionsParams{Resource: "dashboards", ResourceID: "abc"}
5962
descParams := GetResourceDescriptionParams{ResourceType: "folders"}
6063

61-
// ListUsersByOrgParams should be an empty struct (no parameters required)
64+
// ListUsersByOrgParams has only optional parameters
6265
assert.IsType(t, ListUsersByOrgParams{}, userParams)
6366

6467
// ListTeamsParams should have a Query field
@@ -176,3 +179,43 @@ func TestAdminToolsUnit(t *testing.T) {
176179
})
177180
})
178181
}
182+
183+
func TestListUsersByOrg_Pagination(t *testing.T) {
184+
cases := []struct {
185+
name string
186+
args ListUsersByOrgParams
187+
query, perPage, pageWant string
188+
}{
189+
{"defaults", ListUsersByOrgParams{}, "", "100", "1"},
190+
{"explicit", ListUsersByOrgParams{Query: "alice", Limit: 20, Page: 3}, "alice", "20", "3"},
191+
{"limit capped", ListUsersByOrgParams{Limit: 5000}, "", "1000", "1"},
192+
}
193+
for _, tc := range cases {
194+
t.Run(tc.name, func(t *testing.T) {
195+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
196+
assert.Equal(t, "/api/org/users/search", r.URL.Path)
197+
q := r.URL.Query()
198+
assert.Equal(t, tc.query, q.Get("query"))
199+
assert.Equal(t, tc.perPage, q.Get("perpage"))
200+
assert.Equal(t, tc.pageWant, q.Get("page"))
201+
202+
w.Header().Set("Content-Type", "application/json")
203+
_ = json.NewEncoder(w).Encode(map[string]any{
204+
"totalCount": 250,
205+
"page": 3,
206+
"perPage": 20,
207+
"orgUsers": []map[string]any{{"userId": 7, "login": "alice", "role": "Viewer"}},
208+
})
209+
}))
210+
defer server.Close()
211+
212+
result, err := listUsersByOrg(mockCtxWithClient(server), tc.args)
213+
require.NoError(t, err)
214+
assert.Equal(t, int64(250), result.TotalCount)
215+
assert.Equal(t, int64(3), result.Page)
216+
assert.Equal(t, int64(20), result.PerPage)
217+
require.Len(t, result.OrgUsers, 1)
218+
assert.Equal(t, int64(7), result.OrgUsers[0].UserID)
219+
})
220+
}
221+
}

‎tools/annotations.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ func createAnnotation(ctx context.Context, args CreateAnnotationInput) (any, err
9292
}
9393
req := &models.PostGraphiteAnnotationsCmd{
9494
What: args.What,
95-
When: args.When,
95+
When: args.When / 1000, // the Graphite endpoint takes epoch seconds
9696
Tags: args.Tags,
9797
Data: args.GraphiteData,
9898
}

‎tools/annotations_unit_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ func TestCreateAnnotation_GraphiteFormat_Minimal(t *testing.T) {
8383
var body models.PostGraphiteAnnotationsCmd
8484
require.NoError(t, json.NewDecoder(r.Body).Decode(&body))
8585
assert.Equal(t, "deploy", body.What)
86-
assert.Equal(t, int64(1710000000000), body.When)
86+
assert.Equal(t, int64(1710000000), body.When, "graphite endpoint takes epoch seconds")
8787
assert.Nil(t, body.Tags)
8888
assert.Empty(t, body.Data)
8989

@@ -112,7 +112,7 @@ func TestCreateAnnotation_GraphiteFormat_WithTagsAndData(t *testing.T) {
112112
require.NoError(t, json.NewDecoder(r.Body).Decode(&body))
113113

114114
assert.Equal(t, "incident", body["what"])
115-
assert.Equal(t, float64(1720000000000), body["when"])
115+
assert.Equal(t, float64(1720000000), body["when"])
116116
assert.ElementsMatch(t, []interface{}{"sev1", "network"}, body["tags"].([]interface{}))
117117
assert.Equal(t, "context", body["data"])
118118

‎tools/dashboard.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,11 @@ func postDashboardOKBodyFromK8s(obj map[string]interface{}, uid string) *models.
408408
UID: &uid,
409409
Status: &status,
410410
}
411+
if spec, ok := obj["spec"].(map[string]interface{}); ok {
412+
if title, ok := spec["title"].(string); ok {
413+
body.Title = &title
414+
}
415+
}
411416
if metadata, ok := obj["metadata"].(map[string]interface{}); ok {
412417
// The legacy dashboard "version" is the k8s metadata.generation: Grafana's
413418
// own unstructured->legacy conversion does exactly this mapping
@@ -489,6 +494,10 @@ func updateDashboardWithFullJSON(ctx context.Context, args UpdateDashboardParams
489494
if err != nil {
490495
return nil, fmt.Errorf("unable to save dashboard: %w", err)
491496
}
497+
// Grafana's save response has no title; report the one that was saved.
498+
if title, ok := args.Dashboard["title"].(string); ok && dashboard.Payload != nil && dashboard.Payload.Title == nil {
499+
dashboard.Payload.Title = &title
500+
}
492501
return dashboard.Payload, nil
493502
}
494503

‎tools/dashboard_deeplink_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,8 @@ func TestUpdateDashboardDeeplink(t *testing.T) {
9393
assert.Equal(t, "success", *result.Status)
9494
assert.Equal(t, int64(2), *result.Version)
9595
assert.Equal(t, "test-folder", result.FolderUID)
96+
require.NotNil(t, result.Title)
97+
assert.Equal(t, "New title", *result.Title)
9698
})
9799
}
98100
}

‎tools/dashboard_k8s_test.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,16 @@ func TestUpdateDashboardV2_SetsFolderAndMessageAnnotations(t *testing.T) {
200200
assert.Equal(t, "tweak title", ann["grafana.app/message"])
201201
}
202202

203+
func TestPostDashboardOKBodyFromK8s_SetsTitle(t *testing.T) {
204+
body := postDashboardOKBodyFromK8s(map[string]interface{}{
205+
"metadata": map[string]interface{}{"name": "u1", "generation": float64(3)},
206+
"spec": map[string]interface{}{"title": "Saved title"},
207+
}, "u1")
208+
require.NotNil(t, body.Title)
209+
assert.Equal(t, "Saved title", *body.Title)
210+
assert.Equal(t, int64(3), *body.Version)
211+
}
212+
203213
// TestCreateOrUpdateDashboardV2_RespectsOverwriteFalse verifies a full-JSON v2
204214
// save refuses to replace an existing dashboard when overwrite is false, matching
205215
// the legacy save path.

0 commit comments

Comments
 (0)