From b9ecc1a36e77c29cfc8a7a4804fc3689d830feee Mon Sep 17 00:00:00 2001 From: Bryan Zwicker Date: Tue, 21 Jul 2026 16:00:38 -0400 Subject: [PATCH 1/2] Extract aliased project mutation primitive Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d7dc302d-e6f2-41e9-a2c8-ed598de47067 --- pkg/github/projects_batch_mutation.go | 132 ++++++++++ pkg/github/projects_batch_mutation_test.go | 288 +++++++++++++++++++++ 2 files changed, 420 insertions(+) create mode 100644 pkg/github/projects_batch_mutation.go create mode 100644 pkg/github/projects_batch_mutation_test.go diff --git a/pkg/github/projects_batch_mutation.go b/pkg/github/projects_batch_mutation.go new file mode 100644 index 0000000000..0c478aabf9 --- /dev/null +++ b/pkg/github/projects_batch_mutation.go @@ -0,0 +1,132 @@ +package github + +import ( + "context" + "errors" + "fmt" + "reflect" + "sync" + + "github.com/shurcooL/githubv4" +) + +const batchMutationWireChunkSize = 20 + +type batchMutationKind int + +const ( + batchMutationUpdate batchMutationKind = iota + batchMutationClear +) + +func (k batchMutationKind) fieldName() string { + if k == batchMutationClear { + return "clearProjectV2ItemFieldValue" + } + return "updateProjectV2ItemFieldValue" +} + +type projectV2ItemMutationResult struct { + ProjectV2Item struct { + ID string + FullDatabaseID string `graphql:"fullDatabaseId"` + } `graphql:"projectV2Item"` +} + +type reflectedMutationTypeKey struct { + kind batchMutationKind + size int +} + +var reflectedMutationTypeCache sync.Map + +// Reflected types are cached only by operation and chunk size to bound +// reflect.StructOf's runtime cache; positional names and tags keep request data +// out of type identities. The pinned Client.Mutate binds its third argument to +// $input, so item0 uses $input and later aliases use $input1, $input2, ... +// supplied through the variables map. +func buildAliasedMutationType(kind batchMutationKind, size int) reflect.Type { + key := reflectedMutationTypeKey{kind: kind, size: size} + if cached, ok := reflectedMutationTypeCache.Load(key); ok { + return cached.(reflect.Type) + } + + resultType := reflect.TypeFor[projectV2ItemMutationResult]() + fields := make([]reflect.StructField, size) + for i := range size { + varName := "input" + if i > 0 { + varName = fmt.Sprintf("input%d", i) + } + fields[i] = reflect.StructField{ + Name: fmt.Sprintf("Item%d", i), + Type: resultType, + Tag: reflect.StructTag(fmt.Sprintf(`graphql:"item%d: %s(input: $%s)"`, i, kind.fieldName(), varName)), + } + } + + t := reflect.StructOf(fields) + actual, _ := reflectedMutationTypeCache.LoadOrStore(key, t) + return actual.(reflect.Type) +} + +type mutationAliasOutcome struct { + // Populated confirms this alias returned a project item, even when the + // response also contains GraphQL errors. + Populated bool + NodeID string + FullDatabaseID string +} + +// The pinned client decodes partial data before returning GraphQL errors but +// discards errors[].path. Populated aliases confirm writes; unpopulated aliases +// remain unknown and must not be retried individually. +func executeAliasedMutation(ctx context.Context, gqlClient *githubv4.Client, kind batchMutationKind, inputs []githubv4.Input) ([]mutationAliasOutcome, error) { + if len(inputs) == 0 { + return nil, nil + } + if len(inputs) > batchMutationWireChunkSize { + return nil, fmt.Errorf("internal error: chunk of %d exceeds wire chunk size %d", len(inputs), batchMutationWireChunkSize) + } + + mutationType := buildAliasedMutationType(kind, len(inputs)) + mutationPtr := reflect.New(mutationType) + + var variables map[string]any + if len(inputs) > 1 { + variables = make(map[string]any, len(inputs)-1) + for i := 1; i < len(inputs); i++ { + variables[fmt.Sprintf("input%d", i)] = inputs[i] + } + } + + mutateErr := gqlClient.Mutate(ctx, mutationPtr.Interface(), inputs[0], variables) + + outcomes := make([]mutationAliasOutcome, len(inputs)) + elem := mutationPtr.Elem() + for i := range inputs { + result, ok := elem.Field(i).Interface().(projectV2ItemMutationResult) + if !ok || result.ProjectV2Item.ID == "" { + continue + } + outcomes[i] = mutationAliasOutcome{ + Populated: true, + NodeID: result.ProjectV2Item.ID, + FullDatabaseID: result.ProjectV2Item.FullDatabaseID, + } + } + return outcomes, mutateErr +} + +// The pinned client's GraphQL response error type is unexported; transport and +// decoding failures must remain distinguishable. +func isGraphQLResponseError(err error) bool { + for err != nil { + errType := reflect.TypeOf(err) + if errType.PkgPath() == "github.com/shurcooL/graphql" && errType.Name() == "errors" { + return true + } + err = errors.Unwrap(err) + } + return false +} diff --git a/pkg/github/projects_batch_mutation_test.go b/pkg/github/projects_batch_mutation_test.go new file mode 100644 index 0000000000..c187b9a2ef --- /dev/null +++ b/pkg/github/projects_batch_mutation_test.go @@ -0,0 +1,288 @@ +package github + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "reflect" + "strings" + "testing" + + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// capturedGraphQLRequest is one HTTP request observed by sequencedGraphQLTransport. +type capturedGraphQLRequest struct { + Query string + Variables map[string]any +} + +// sequencedGraphQLTransport is a minimal fake http.RoundTripper for exercising +// executeAliasedMutation without needing to hand-construct +// the exact minified GraphQL query text that reflect.StructOf produces: each call +// is served by the next entry in responses, in order, and the parsed query + +// variables are recorded for assertions. +type sequencedGraphQLTransport struct { + t *testing.T + responses []func(req capturedGraphQLRequest) (status int, body string) + calls []capturedGraphQLRequest +} + +func (s *sequencedGraphQLTransport) RoundTrip(req *http.Request) (*http.Response, error) { + raw, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + var parsed struct { + Query string `json:"query"` + Variables map[string]any `json:"variables"` + } + if err := json.Unmarshal(raw, &parsed); err != nil { + return nil, err + } + captured := capturedGraphQLRequest{Query: parsed.Query, Variables: parsed.Variables} + s.calls = append(s.calls, captured) + + idx := len(s.calls) - 1 + if idx >= len(s.responses) { + s.t.Fatalf("unexpected GraphQL call #%d (query: %s)", idx, parsed.Query) + } + status, body := s.responses[idx](captured) + return &http.Response{ + StatusCode: status, + Body: io.NopCloser(strings.NewReader(body)), + Header: make(http.Header), + }, nil +} + +type errorGraphQLTransport struct { + err error + calls int +} + +func (t *errorGraphQLTransport) RoundTrip(*http.Request) (*http.Response, error) { + t.calls++ + return nil, t.err +} + +// mutationDataResponse builds a `{"data": {...}}` JSON body with one +// "itemN"."projectV2Item" entry per populated index in ids. +func mutationDataResponse(t *testing.T, ids map[int]struct{ NodeID, FullDatabaseID string }) string { + t.Helper() + data := make(map[string]any, len(ids)) + for i, v := range ids { + data[fmt.Sprintf("item%d", i)] = map[string]any{ + "projectV2Item": map[string]any{ + "id": v.NodeID, + "fullDatabaseId": v.FullDatabaseID, + }, + } + } + body, err := json.Marshal(map[string]any{"data": data}) + require.NoError(t, err) + return string(body) +} + +func mutationErrorResponse(t *testing.T, data map[string]any, message string) string { + t.Helper() + payload := map[string]any{ + "errors": []map[string]any{{"message": message}}, + } + if data != nil { + payload["data"] = data + } + body, err := json.Marshal(payload) + require.NoError(t, err) + return string(body) +} + +func newTestGQLClient(transport http.RoundTripper) *githubv4.Client { + return githubv4.NewClient(&http.Client{Transport: transport}) +} + +func inputsOfSize(n int) []githubv4.Input { + inputs := make([]githubv4.Input, n) + for i := range n { + inputs[i] = githubv4.UpdateProjectV2ItemFieldValueInput{ + ProjectID: githubv4.ID("PVT_project"), + ItemID: githubv4.ID(fmt.Sprintf("PVTI_item%d", i)), + FieldID: githubv4.ID("PVTF_field"), + Value: githubv4.ProjectV2FieldValue{Text: githubv4.NewString("v")}, + } + } + return inputs +} + +func Test_BuildAliasedMutationType_FieldNamesAndTags(t *testing.T) { + for _, size := range []int{1, 2, 20} { + t.Run(fmt.Sprintf("size=%d", size), func(t *testing.T) { + typ := buildAliasedMutationType(batchMutationUpdate, size) + require.Equal(t, size, typ.NumField()) + for i := range size { + field := typ.Field(i) + assert.Equal(t, fmt.Sprintf("Item%d", i), field.Name) + + tag, ok := field.Tag.Lookup("graphql") + require.True(t, ok) + + wantVar := "input" + if i > 0 { + wantVar = fmt.Sprintf("input%d", i) + } + wantTag := fmt.Sprintf("item%d: updateProjectV2ItemFieldValue(input: $%s)", i, wantVar) + assert.Equal(t, wantTag, tag) + + // No owner/id/name/value data may ever appear in the tag: only + // positional aliases and variable references. + assert.NotContains(t, tag, "PVT_") + assert.NotContains(t, tag, "octo") + } + }) + } +} + +func Test_BuildAliasedMutationType_ClearKindUsesClearMutation(t *testing.T) { + typ := buildAliasedMutationType(batchMutationClear, 2) + tag0 := typ.Field(0).Tag.Get("graphql") + tag1 := typ.Field(1).Tag.Get("graphql") + assert.Equal(t, "item0: clearProjectV2ItemFieldValue(input: $input)", tag0) + assert.Equal(t, "item1: clearProjectV2ItemFieldValue(input: $input1)", tag1) +} + +func Test_BuildAliasedMutationType_CachedByKindAndSize(t *testing.T) { + a := buildAliasedMutationType(batchMutationUpdate, 3) + b := buildAliasedMutationType(batchMutationUpdate, 3) + assert.True(t, a == b, "expected the same cached reflect.Type for identical (kind, size)") + + c := buildAliasedMutationType(batchMutationClear, 3) + assert.False(t, a == c, "update and clear must not share a cached type") + + d := buildAliasedMutationType(batchMutationUpdate, 4) + assert.False(t, a == d, "different sizes must not share a cached type") +} + +func Test_ExecuteAliasedMutation_OneAlias(t *testing.T) { + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(req capturedGraphQLRequest) (int, string) { + // Single alias: the only input is bound positionally via + // Client.Mutate's third argument, so no extra variables map entries. + assert.Len(t, req.Variables, 1) + assert.Contains(t, req.Variables, "input") + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item0", FullDatabaseID: "1001"}, + }) + }, + }, + } + gqlClient := newTestGQLClient(transport) + + outcomes, err := executeAliasedMutation(context.Background(), gqlClient, batchMutationUpdate, inputsOfSize(1)) + require.NoError(t, err) + require.Len(t, outcomes, 1) + assert.True(t, outcomes[0].Populated) + assert.Equal(t, "PVTI_item0", outcomes[0].NodeID) + assert.Equal(t, "1001", outcomes[0].FullDatabaseID) +} + +func Test_ExecuteAliasedMutation_TwoAliases_FirstInputWorkaround(t *testing.T) { + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(req capturedGraphQLRequest) (int, string) { + // Alias 0's input is always bound to the reserved "input" wire + // variable by Client.Mutate; alias 1's input must be supplied + // separately (as "input1") since a GraphQL variable can only be + // referenced with one value per request. + require.Contains(t, req.Variables, "input1") + require.Contains(t, req.Variables, "input") + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item0", FullDatabaseID: "1001"}, + 1: {NodeID: "PVTI_item1", FullDatabaseID: "1002"}, + }) + }, + }, + } + gqlClient := newTestGQLClient(transport) + + outcomes, err := executeAliasedMutation(context.Background(), gqlClient, batchMutationUpdate, inputsOfSize(2)) + require.NoError(t, err) + require.Len(t, outcomes, 2) + assert.True(t, outcomes[0].Populated) + assert.True(t, outcomes[1].Populated) +} + +func Test_ExecuteAliasedMutation_TwentyAliases(t *testing.T) { + ids := make(map[int]struct{ NodeID, FullDatabaseID string }, 20) + for i := range 20 { + ids[i] = struct{ NodeID, FullDatabaseID string }{ + NodeID: fmt.Sprintf("PVTI_item%d", i), + FullDatabaseID: fmt.Sprintf("%d", 1000+i), + } + } + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(req capturedGraphQLRequest) (int, string) { + assert.Len(t, req.Variables, 20) // "input" (positional) plus input1..input19 + return http.StatusOK, mutationDataResponse(t, ids) + }, + }, + } + gqlClient := newTestGQLClient(transport) + + outcomes, err := executeAliasedMutation(context.Background(), gqlClient, batchMutationUpdate, inputsOfSize(20)) + require.NoError(t, err) + require.Len(t, outcomes, 20) + for i, oc := range outcomes { + assert.Truef(t, oc.Populated, "outcome %d should be populated", i) + } +} + +func Test_ExecuteAliasedMutation_ChunkSizeExceeded(t *testing.T) { + gqlClient := newTestGQLClient(&sequencedGraphQLTransport{t: t}) + _, err := executeAliasedMutation(context.Background(), gqlClient, batchMutationUpdate, inputsOfSize(21)) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds wire chunk size") +} + +func Test_ExecuteAliasedMutation_EmptyInputsIsNoop(t *testing.T) { + gqlClient := newTestGQLClient(&sequencedGraphQLTransport{t: t}) + outcomes, err := executeAliasedMutation(context.Background(), gqlClient, batchMutationUpdate, nil) + require.NoError(t, err) + assert.Nil(t, outcomes) +} + +func Test_ProjectV2ItemMutationResult_ReflectFieldTypeIsConcrete(t *testing.T) { + // executeAliasedMutation type-asserts each reflected field back to + // projectV2ItemMutationResult directly; guard that assumption here. + typ := buildAliasedMutationType(batchMutationUpdate, 1) + assert.Equal(t, reflect.TypeFor[projectV2ItemMutationResult](), typ.Field(0).Type) +} + +func Test_IsGraphQLResponseError(t *testing.T) { + graphqlTransport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(_ capturedGraphQLRequest) (int, string) { + return http.StatusOK, mutationErrorResponse(t, nil, "mutation failed") + }, + }, + } + _, graphqlErr := executeAliasedMutation(t.Context(), newTestGQLClient(graphqlTransport), batchMutationUpdate, inputsOfSize(1)) + require.Error(t, graphqlErr) + assert.True(t, isGraphQLResponseError(graphqlErr)) + + transport := &errorGraphQLTransport{err: context.DeadlineExceeded} + _, transportErr := executeAliasedMutation(t.Context(), newTestGQLClient(transport), batchMutationUpdate, inputsOfSize(1)) + require.Error(t, transportErr) + assert.False(t, isGraphQLResponseError(transportErr)) + assert.False(t, isGraphQLResponseError(errors.New("plain error"))) + assert.False(t, isGraphQLResponseError(nil)) +} From 894cfdcf4fe19d17a9e3adfb41f17f932991fb6e Mon Sep 17 00:00:00 2001 From: Bryan Zwicker Date: Wed, 22 Jul 2026 10:15:52 -0400 Subject: [PATCH 2/2] Test partial GraphQL mutation data Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4d3df48a-5aa0-4cf0-a067-4aa5618c2887 --- pkg/github/projects_batch_mutation_test.go | 30 ++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/pkg/github/projects_batch_mutation_test.go b/pkg/github/projects_batch_mutation_test.go index c187b9a2ef..749776862d 100644 --- a/pkg/github/projects_batch_mutation_test.go +++ b/pkg/github/projects_batch_mutation_test.go @@ -218,6 +218,36 @@ func Test_ExecuteAliasedMutation_TwoAliases_FirstInputWorkaround(t *testing.T) { assert.True(t, outcomes[1].Populated) } +func Test_ExecuteAliasedMutation_PreservesPartialDataWithGraphQLErrors(t *testing.T) { + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(capturedGraphQLRequest) (int, string) { + data := map[string]any{ + "item0": map[string]any{ + "projectV2Item": map[string]any{ + "id": "PVTI_item0", + "fullDatabaseId": "1001", + }, + }, + } + return http.StatusOK, mutationErrorResponse(t, data, "item1 failed") + }, + }, + } + + outcomes, err := executeAliasedMutation(t.Context(), newTestGQLClient(transport), batchMutationUpdate, inputsOfSize(2)) + require.Error(t, err) + assert.True(t, isGraphQLResponseError(err)) + require.Len(t, outcomes, 2) + assert.Equal(t, mutationAliasOutcome{ + Populated: true, + NodeID: "PVTI_item0", + FullDatabaseID: "1001", + }, outcomes[0]) + assert.Equal(t, mutationAliasOutcome{}, outcomes[1]) +} + func Test_ExecuteAliasedMutation_TwentyAliases(t *testing.T) { ids := make(map[int]struct{ NodeID, FullDatabaseID string }, 20) for i := range 20 {