Skip to content

Commit eebd8a3

Browse files
boazreicherboazreicher
committed
use the REST PATCH endpoint if updating rationale or suggestion
1 parent 7e79ae9 commit eebd8a3

2 files changed

Lines changed: 205 additions & 130 deletions

File tree

‎pkg/github/granular_tools_test.go‎

Lines changed: 69 additions & 130 deletions
Original file line numberDiff line numberDiff line change
@@ -1277,75 +1277,46 @@ func TestGranularSetIssueFields(t *testing.T) {
12771277
})
12781278

12791279
t.Run("successful set with text value and rationale", func(t *testing.T) {
1280-
matchers := []githubv4mock.Matcher{
1281-
githubv4mock.NewQueryMatcher(
1282-
struct {
1283-
Repository struct {
1284-
Issue struct {
1285-
ID githubv4.ID
1286-
} `graphql:"issue(number: $issueNumber)"`
1287-
} `graphql:"repository(owner: $owner, name: $repo)"`
1288-
}{},
1289-
map[string]any{
1290-
"owner": githubv4.String("owner"),
1291-
"repo": githubv4.String("repo"),
1292-
"issueNumber": githubv4.Int(5),
1293-
},
1294-
githubv4mock.DataResponse(map[string]any{
1295-
"repository": map[string]any{
1296-
"issue": map[string]any{"id": "ISSUE_123"},
1297-
},
1298-
}),
1299-
),
1300-
githubv4mock.NewMutationMatcher(
1301-
struct {
1302-
SetIssueFieldValue struct {
1303-
Issue struct {
1304-
ID githubv4.ID
1305-
Number githubv4.Int
1306-
URL githubv4.String
1307-
}
1308-
IssueFieldValues []struct {
1309-
TextValue struct {
1310-
Value string
1311-
} `graphql:"... on IssueFieldTextValue"`
1312-
SingleSelectValue struct {
1313-
Name string
1314-
} `graphql:"... on IssueFieldSingleSelectValue"`
1315-
DateValue struct {
1316-
Value string
1317-
} `graphql:"... on IssueFieldDateValue"`
1318-
NumberValue struct {
1319-
Value float64
1320-
} `graphql:"... on IssueFieldNumberValue"`
1321-
}
1322-
} `graphql:"setIssueFieldValue(input: $input)"`
1323-
}{},
1324-
SetIssueFieldValueInput{
1325-
IssueID: githubv4.ID("ISSUE_123"),
1326-
IssueFields: []IssueFieldCreateOrUpdateInput{
1327-
{
1328-
FieldID: githubv4.ID("FIELD_1"),
1329-
TextValue: githubv4.NewString(githubv4.String("hello")),
1330-
Rationale: githubv4.NewString(githubv4.String("Reflects the reported severity")),
1280+
// fetchIssueFields query mock for resolving the field's database ID.
1281+
fieldsQuery := githubv4mock.NewQueryMatcher(
1282+
issueFieldsRepoQuery{},
1283+
map[string]any{
1284+
"owner": githubv4.String("owner"),
1285+
"name": githubv4.String("repo"),
1286+
},
1287+
githubv4mock.DataResponse(map[string]any{
1288+
"repository": map[string]any{
1289+
"issueFields": map[string]any{
1290+
"nodes": []any{
1291+
map[string]any{
1292+
"__typename": "IssueFieldText",
1293+
"id": "FIELD_1",
1294+
"fullDatabaseId": "42",
1295+
"name": "DRI",
1296+
"dataType": "TEXT",
1297+
"visibility": "ALL",
1298+
},
13311299
},
13321300
},
13331301
},
1334-
nil,
1335-
githubv4mock.DataResponse(map[string]any{
1336-
"setIssueFieldValue": map[string]any{
1337-
"issue": map[string]any{
1338-
"id": "ISSUE_123",
1339-
"number": 5,
1340-
"url": "https://github.com/owner/repo/issues/5",
1341-
},
1302+
}),
1303+
)
1304+
gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(fieldsQuery))
1305+
1306+
// REST PATCH mock asserting the issue_field_values wire format.
1307+
restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
1308+
PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{
1309+
"issue_field_values": []any{
1310+
map[string]any{
1311+
"field_id": float64(42),
1312+
"value": "hello",
1313+
"rationale": "Reflects the reported severity",
13421314
},
1343-
}),
1344-
),
1345-
}
1315+
},
1316+
}).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(5)})),
1317+
}))
13461318

1347-
gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matchers...))
1348-
deps := BaseDeps{GQLClient: gqlClient}
1319+
deps := BaseDeps{Client: restClient, GQLClient: gqlClient}
13491320
serverTool := GranularSetIssueFields(translations.NullTranslationHelper)
13501321
handler := serverTool.Handler(deps)
13511322

@@ -1390,77 +1361,45 @@ func TestGranularSetIssueFields(t *testing.T) {
13901361
})
13911362

13921363
t.Run("successful set with suggest flag", func(t *testing.T) {
1393-
suggestTrue := githubv4.Boolean(true)
1394-
matchers := []githubv4mock.Matcher{
1395-
githubv4mock.NewQueryMatcher(
1396-
struct {
1397-
Repository struct {
1398-
Issue struct {
1399-
ID githubv4.ID
1400-
} `graphql:"issue(number: $issueNumber)"`
1401-
} `graphql:"repository(owner: $owner, name: $repo)"`
1402-
}{},
1403-
map[string]any{
1404-
"owner": githubv4.String("owner"),
1405-
"repo": githubv4.String("repo"),
1406-
"issueNumber": githubv4.Int(5),
1407-
},
1408-
githubv4mock.DataResponse(map[string]any{
1409-
"repository": map[string]any{
1410-
"issue": map[string]any{"id": "ISSUE_123"},
1411-
},
1412-
}),
1413-
),
1414-
githubv4mock.NewMutationMatcher(
1415-
struct {
1416-
SetIssueFieldValue struct {
1417-
Issue struct {
1418-
ID githubv4.ID
1419-
Number githubv4.Int
1420-
URL githubv4.String
1421-
}
1422-
IssueFieldValues []struct {
1423-
TextValue struct {
1424-
Value string
1425-
} `graphql:"... on IssueFieldTextValue"`
1426-
SingleSelectValue struct {
1427-
Name string
1428-
} `graphql:"... on IssueFieldSingleSelectValue"`
1429-
DateValue struct {
1430-
Value string
1431-
} `graphql:"... on IssueFieldDateValue"`
1432-
NumberValue struct {
1433-
Value float64
1434-
} `graphql:"... on IssueFieldNumberValue"`
1435-
}
1436-
} `graphql:"setIssueFieldValue(input: $input)"`
1437-
}{},
1438-
SetIssueFieldValueInput{
1439-
IssueID: githubv4.ID("ISSUE_123"),
1440-
IssueFields: []IssueFieldCreateOrUpdateInput{
1441-
{
1442-
FieldID: githubv4.ID("FIELD_1"),
1443-
TextValue: githubv4.NewString(githubv4.String("hello")),
1444-
Rationale: githubv4.NewString(githubv4.String("Reflects the reported severity")),
1445-
Suggest: &suggestTrue,
1364+
fieldsQuery := githubv4mock.NewQueryMatcher(
1365+
issueFieldsRepoQuery{},
1366+
map[string]any{
1367+
"owner": githubv4.String("owner"),
1368+
"name": githubv4.String("repo"),
1369+
},
1370+
githubv4mock.DataResponse(map[string]any{
1371+
"repository": map[string]any{
1372+
"issueFields": map[string]any{
1373+
"nodes": []any{
1374+
map[string]any{
1375+
"__typename": "IssueFieldText",
1376+
"id": "FIELD_1",
1377+
"fullDatabaseId": "42",
1378+
"name": "DRI",
1379+
"dataType": "TEXT",
1380+
"visibility": "ALL",
1381+
},
14461382
},
14471383
},
14481384
},
1449-
nil,
1450-
githubv4mock.DataResponse(map[string]any{
1451-
"setIssueFieldValue": map[string]any{
1452-
"issue": map[string]any{
1453-
"id": "ISSUE_123",
1454-
"number": 5,
1455-
"url": "https://github.com/owner/repo/issues/5",
1456-
},
1385+
}),
1386+
)
1387+
gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(fieldsQuery))
1388+
1389+
restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
1390+
PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{
1391+
"issue_field_values": []any{
1392+
map[string]any{
1393+
"field_id": float64(42),
1394+
"value": "hello",
1395+
"rationale": "Reflects the reported severity",
1396+
"suggest": true,
14571397
},
1458-
}),
1459-
),
1460-
}
1398+
},
1399+
}).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(5)})),
1400+
}))
14611401

1462-
gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matchers...))
1463-
deps := BaseDeps{GQLClient: gqlClient}
1402+
deps := BaseDeps{Client: restClient, GQLClient: gqlClient}
14641403
serverTool := GranularSetIssueFields(translations.NullTranslationHelper)
14651404
handler := serverTool.Handler(deps)
14661405

‎pkg/github/issues_granular.go‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"encoding/json"
66
"fmt"
77
"maps"
8+
"strconv"
89
"strings"
910

1011
ghErrors "github.com/github/github-mcp-server/pkg/errors"
@@ -1009,6 +1010,10 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv
10091010
return utils.NewToolResultError("fields array must not be empty"), nil, nil
10101011
}
10111012

1013+
// needsREST is set when any field has rationale or suggest, since
1014+
// those are not yet supported on the GraphQL setIssueFieldValue
1015+
// mutation. The whole request is then dispatched via REST.
1016+
needsREST := false
10121017
issueFields := make([]IssueFieldCreateOrUpdateInput, 0, len(fieldMaps))
10131018
for _, fieldMap := range fieldMaps {
10141019
fieldID, err := RequiredParam[string](fieldMap, "field_id")
@@ -1070,6 +1075,7 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv
10701075
}
10711076
if rationale != "" {
10721077
input.Rationale = githubv4.NewString(githubv4.String(rationale))
1078+
needsREST = true
10731079
}
10741080
}
10751081

@@ -1080,11 +1086,16 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv
10801086
if isSuggestion {
10811087
suggestVal := githubv4.Boolean(true)
10821088
input.Suggest = &suggestVal
1089+
needsREST = true
10831090
}
10841091

10851092
issueFields = append(issueFields, input)
10861093
}
10871094

1095+
if needsREST {
1096+
return setIssueFieldsViaREST(ctx, deps, owner, repo, issueNumber, issueFields)
1097+
}
1098+
10881099
gqlClient, err := deps.GetGQLClient(ctx)
10891100
if err != nil {
10901101
return utils.NewToolResultErrorFromErr("failed to get GitHub GraphQL client", err), nil, nil
@@ -1143,3 +1154,128 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv
11431154
st.FeatureFlagEnable = FeatureFlagIssuesGranular
11441155
return st
11451156
}
1157+
1158+
// issueFieldValueRESTRequest is the JSON body shape accepted by the REST
1159+
// update-issue endpoint when setting issue field values with rationale or
1160+
// suggestion. The endpoint expects integer database IDs and a single string
1161+
// value per field. Used as a fallback to the GraphQL setIssueFieldValue
1162+
// mutation, which does not (yet) support rationale or suggest.
1163+
type issueFieldValueRESTRequest struct {
1164+
IssueFieldValues []issueFieldValueRESTEntry `json:"issue_field_values"`
1165+
}
1166+
1167+
type issueFieldValueRESTEntry struct {
1168+
FieldID int64 `json:"field_id"`
1169+
Value string `json:"value"`
1170+
Rationale string `json:"rationale,omitempty"`
1171+
Suggest bool `json:"suggest,omitempty"`
1172+
}
1173+
1174+
// setIssueFieldsViaREST dispatches a set_issue_fields request through the
1175+
// REST update-issue endpoint. It resolves each field's integer database ID
1176+
// (and, for single_select fields, each option's name) using the GraphQL
1177+
// issue field definitions, then PATCHes the issue.
1178+
func setIssueFieldsViaREST(
1179+
ctx context.Context,
1180+
deps ToolDependencies,
1181+
owner, repo string,
1182+
issueNumber int,
1183+
issueFields []IssueFieldCreateOrUpdateInput,
1184+
) (*mcp.CallToolResult, any, error) {
1185+
// Delete is not supported on the REST shape we use here.
1186+
for _, f := range issueFields {
1187+
if f.Delete != nil && bool(*f.Delete) {
1188+
return utils.NewToolResultError("delete is not supported when rationale or is_suggestion is set"), nil, nil
1189+
}
1190+
}
1191+
1192+
gqlClient, err := deps.GetGQLClient(ctx)
1193+
if err != nil {
1194+
return utils.NewToolResultErrorFromErr("failed to get GitHub GraphQL client", err), nil, nil
1195+
}
1196+
1197+
defs, err := fetchIssueFields(ctx, gqlClient, owner, repo)
1198+
if err != nil {
1199+
return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to look up issue field definitions", err), nil, nil
1200+
}
1201+
1202+
fieldByNodeID := make(map[string]IssueField, len(defs))
1203+
for _, f := range defs {
1204+
fieldByNodeID[f.ID] = f
1205+
}
1206+
1207+
entries := make([]issueFieldValueRESTEntry, 0, len(issueFields))
1208+
for _, f := range issueFields {
1209+
nodeID := fmt.Sprintf("%v", f.FieldID)
1210+
def, ok := fieldByNodeID[nodeID]
1211+
if !ok {
1212+
return utils.NewToolResultError(fmt.Sprintf("unknown field_id %q for %s/%s", nodeID, owner, repo)), nil, nil
1213+
}
1214+
if def.DatabaseID == 0 {
1215+
return utils.NewToolResultError(fmt.Sprintf("could not resolve database ID for field %q", def.Name)), nil, nil
1216+
}
1217+
1218+
var value string
1219+
switch {
1220+
case f.TextValue != nil:
1221+
value = string(*f.TextValue)
1222+
case f.NumberValue != nil:
1223+
value = strconv.FormatFloat(float64(*f.NumberValue), 'f', -1, 64)
1224+
case f.DateValue != nil:
1225+
value = string(*f.DateValue)
1226+
case f.SingleSelectOptionID != nil:
1227+
optID := fmt.Sprintf("%v", *f.SingleSelectOptionID)
1228+
optName := ""
1229+
for _, opt := range def.Options {
1230+
if opt.ID == optID {
1231+
optName = opt.Name
1232+
break
1233+
}
1234+
}
1235+
if optName == "" {
1236+
return utils.NewToolResultError(fmt.Sprintf("unknown single_select_option_id %q for field %q", optID, def.Name)), nil, nil
1237+
}
1238+
value = optName
1239+
}
1240+
1241+
entry := issueFieldValueRESTEntry{
1242+
FieldID: def.DatabaseID,
1243+
Value: value,
1244+
}
1245+
if f.Rationale != nil {
1246+
entry.Rationale = string(*f.Rationale)
1247+
}
1248+
if f.Suggest != nil {
1249+
entry.Suggest = bool(*f.Suggest)
1250+
}
1251+
entries = append(entries, entry)
1252+
}
1253+
1254+
client, err := deps.GetClient(ctx)
1255+
if err != nil {
1256+
return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
1257+
}
1258+
1259+
body := &issueFieldValueRESTRequest{IssueFieldValues: entries}
1260+
apiURL := fmt.Sprintf("repos/%s/%s/issues/%d", owner, repo, issueNumber)
1261+
req, err := client.NewRequest(ctx, "PATCH", apiURL, body)
1262+
if err != nil {
1263+
return utils.NewToolResultErrorFromErr("failed to create request", err), nil, nil
1264+
}
1265+
1266+
issue := &github.Issue{}
1267+
resp, err := client.Do(req, issue)
1268+
if err != nil {
1269+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to set issue field values", resp, err), nil, nil
1270+
}
1271+
defer func() { _ = resp.Body.Close() }()
1272+
1273+
r, err := json.Marshal(MinimalResponse{
1274+
ID: fmt.Sprintf("%d", issue.GetID()),
1275+
URL: issue.GetHTMLURL(),
1276+
})
1277+
if err != nil {
1278+
return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil
1279+
}
1280+
return utils.NewToolResultText(string(r)), nil, nil
1281+
}

0 commit comments

Comments
 (0)