Skip to content

Commit 24d015f

Browse files
owenniblockCopilot
andcommitted
Address review feedback on multi-select issue fields
- Wrap the deleteIssueFieldValue mutation with WithGraphQLFeatures ("issue_fields", "repo_issue_fields") to match every sibling issue-field GraphQL op; without it the clear can silently no-op when gated. - Reuse parseStringSlice for field_option_names (optionalIssueWriteFields) and multi_select_option_ids (set_issue_fields) instead of hand-rolling []string/[]any coercion in three places. - Trim whitespace in matchOption so list_issues field_filters matches options consistently with issue_write (e.g. " Auth "). Adds a regression test. - Trim two verbose comments per review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 0e5b593 commit 24d015f

3 files changed

Lines changed: 28 additions & 40 deletions

File tree

pkg/github/issues.go

Lines changed: 8 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -297,21 +297,11 @@ func optionalIssueWriteFields(args map[string]any) ([]issueWriteFieldInput, erro
297297
var fieldOptionNames []string
298298
_, hasNamesKey := itemMap["field_option_names"]
299299
if rawNames := itemMap["field_option_names"]; hasNamesKey && rawNames != nil {
300-
switch v := rawNames.(type) {
301-
case []string:
302-
fieldOptionNames = v
303-
case []any:
304-
fieldOptionNames = make([]string, 0, len(v))
305-
for _, item := range v {
306-
s, ok := item.(string)
307-
if !ok {
308-
return nil, fmt.Errorf("field_option_names for field %q must be an array of strings", fieldName)
309-
}
310-
fieldOptionNames = append(fieldOptionNames, s)
311-
}
312-
default:
300+
parsed, err := parseStringSlice(rawNames)
301+
if err != nil {
313302
return nil, fmt.Errorf("field_option_names for field %q must be an array of strings", fieldName)
314303
}
304+
fieldOptionNames = parsed
315305
}
316306

317307
deleteField, _ := OptionalParam[bool](itemMap, "delete")
@@ -491,10 +481,8 @@ func resolveIssueRequestFieldValues(ctx context.Context, gqlClient *githubv4.Cli
491481
// REST IssueField#build_value_attributes for multi_select expects an array of option names.
492482
resolvedValue = resolvedNames
493483
default:
494-
// Raw value path. Reject it for multi_select so callers get a clear error
495-
// instead of an opaque REST 422 — they should use field_option_names which
496-
// validates options up front. Single_select keeps the existing pass-through
497-
// behaviour for backwards compatibility (field_option_name is the validated path).
484+
// multi_select must go through field_option_names (which validates options);
485+
// reject a raw value here so callers get a clear error instead of a REST 422.
498486
if strings.EqualFold(dataType, "multi_select") {
499487
return nil, nil, fmt.Errorf("issue field %q is multi_select, use field_option_names to set its value", fieldInput.FieldName)
500488
}
@@ -564,9 +552,6 @@ func fetchExistingIssueFieldValues(ctx context.Context, gqlClient *githubv4.Clie
564552
value = string(node.SingleSelectValue.Value)
565553
case "IssueFieldMultiSelectValue":
566554
fieldIDStr = node.MultiSelectValue.Field.FullDatabaseIDStr()
567-
// REST IssueField#build_value_attributes for multi_select expects an array
568-
// of option names. Pass the selected option names through so the merged
569-
// update round-trips faithfully.
570555
names := make([]string, 0, len(node.MultiSelectValue.Options))
571556
for _, opt := range node.MultiSelectValue.Options {
572557
names = append(names, string(opt.Name))
@@ -2595,6 +2580,7 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4
25952580
if err != nil {
25962581
return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to look up issue for field deletion", err), nil
25972582
}
2583+
ctxWithFeatures := ghcontext.WithGraphQLFeatures(ctx, "issue_fields", "repo_issue_fields")
25982584
for _, deletion := range fieldsToDelete {
25992585
var mutation struct {
26002586
DeleteIssueFieldValue struct {
@@ -2607,7 +2593,7 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4
26072593
IssueID: issueID,
26082594
FieldID: deletion.NodeID,
26092595
}
2610-
if err := gqlClient.Mutate(ctx, &mutation, input, nil); err != nil {
2596+
if err := gqlClient.Mutate(ctxWithFeatures, &mutation, input, nil); err != nil {
26112597
return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to delete issue field value", err), nil
26122598
}
26132599
}
@@ -3351,7 +3337,7 @@ func resolveFieldFilters(rawFilters []rawFieldFilter, fields []IssueField) ([]Is
33513337
// or an error listing the valid options.
33523338
func matchOption(field IssueField, value string) (string, error) {
33533339
for _, o := range field.Options {
3354-
if strings.EqualFold(o.Name, value) {
3340+
if strings.EqualFold(strings.TrimSpace(o.Name), strings.TrimSpace(value)) {
33553341
return o.Name, nil
33563342
}
33573343
}

pkg/github/issues_granular.go

Lines changed: 7 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1093,26 +1093,15 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv
10931093
valueCount++
10941094
}
10951095
if rawIDs, exists := fieldMap["multi_select_option_ids"]; exists && rawIDs != nil {
1096-
var optionIDs []githubv4.ID
1097-
switch v := rawIDs.(type) {
1098-
case []string:
1099-
optionIDs = make([]githubv4.ID, 0, len(v))
1100-
for _, s := range v {
1101-
optionIDs = append(optionIDs, githubv4.ID(s))
1102-
}
1103-
case []any:
1104-
optionIDs = make([]githubv4.ID, 0, len(v))
1105-
for _, item := range v {
1106-
s, ok := item.(string)
1107-
if !ok {
1108-
return utils.NewToolResultError("multi_select_option_ids must be an array of strings"), nil, nil
1109-
}
1110-
optionIDs = append(optionIDs, githubv4.ID(s))
1111-
}
1112-
default:
1096+
ids, err := parseStringSlice(rawIDs)
1097+
if err != nil {
11131098
return utils.NewToolResultError("multi_select_option_ids must be an array of strings"), nil, nil
11141099
}
1115-
if len(optionIDs) > 0 {
1100+
if len(ids) > 0 {
1101+
optionIDs := make([]githubv4.ID, 0, len(ids))
1102+
for _, s := range ids {
1103+
optionIDs = append(optionIDs, githubv4.ID(s))
1104+
}
11161105
input.MultiSelectOptionIDs = &optionIDs
11171106
valueCount++
11181107
}

pkg/github/issues_multiselect_test.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,19 @@ func Test_resolveFieldFilters_MultiSelect(t *testing.T) {
115115
assert.Nil(t, out[0].SingleSelectOptionValue)
116116
})
117117

118+
t.Run("trims surrounding whitespace when matching options", func(t *testing.T) {
119+
raw := []rawFieldFilter{{
120+
Name: "Components",
121+
Values: []string{" auth ", " Billing"},
122+
HasValues: true,
123+
}}
124+
out, err := resolveFieldFilters(raw, fields)
125+
require.NoError(t, err)
126+
require.Len(t, out, 1)
127+
require.NotNil(t, out[0].MultiSelectOptionValues)
128+
assert.Equal(t, []githubv4.String{"Auth", "Billing"}, *out[0].MultiSelectOptionValues)
129+
})
130+
118131
t.Run("rejects unknown option and lists valid ones", func(t *testing.T) {
119132
raw := []rawFieldFilter{{
120133
Name: "Components",

0 commit comments

Comments
 (0)