Skip to content

Commit 9b2c7f5

Browse files
dpwspoon-hiclaude
andcommitted
fix: handle string-typed reviewer.id in ruleset API responses
The GitHub API returns `required_reviewers.reviewer.id` as a JSON string (e.g., `"12345"`), but go-github's `RulesetReviewer.ID` field is `*int64`. This causes `json.Unmarshal` to fail, preventing the provider from recording the resource in state after creation — leading to orphaned rulesets on every retry. Work around this by replacing direct go-github API calls for ruleset CRUD with thin wrappers that use `client.BareDo` to obtain the raw response body, apply a regex fix to convert quoted integer IDs to unquoted integers, and then unmarshal into the go-github types. Fixes #3340 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
1 parent 866a673 commit 9b2c7f5

4 files changed

Lines changed: 241 additions & 8 deletions

File tree

github/resource_github_organization_ruleset.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -768,7 +768,7 @@ func resourceGithubOrganizationRulesetCreate(ctx context.Context, d *schema.Reso
768768

769769
rulesetReq := resourceGithubRulesetObject(d, owner)
770770

771-
ruleset, resp, err := client.Organizations.CreateRepositoryRuleset(ctx, owner, rulesetReq)
771+
ruleset, resp, err := createOrgRuleset(ctx, client, owner, rulesetReq)
772772
if err != nil {
773773
tflog.Error(ctx, fmt.Sprintf("Failed to create organization ruleset: %s/%s", owner, name), map[string]any{
774774
"owner": owner,
@@ -824,7 +824,7 @@ func resourceGithubOrganizationRulesetRead(ctx context.Context, d *schema.Resour
824824
ctx = context.WithValue(ctx, ctxEtag, d.Get("etag").(string))
825825
}
826826

827-
ruleset, resp, err := client.Organizations.GetRepositoryRuleset(ctx, owner, rulesetID)
827+
ruleset, resp, err := getOrgRuleset(ctx, client, owner, rulesetID)
828828
if err != nil {
829829
var ghErr *github.ErrorResponse
830830
if errors.As(err, &ghErr) {
@@ -912,7 +912,7 @@ func resourceGithubOrganizationRulesetUpdate(ctx context.Context, d *schema.Reso
912912

913913
rulesetReq := resourceGithubRulesetObject(d, owner)
914914

915-
ruleset, resp, err := client.Organizations.UpdateRepositoryRuleset(ctx, owner, rulesetID, rulesetReq)
915+
ruleset, resp, err := updateOrgRuleset(ctx, client, owner, rulesetID, rulesetReq)
916916
if err != nil {
917917
tflog.Error(ctx, fmt.Sprintf("Failed to update organization ruleset: %s/%d", owner, rulesetID), map[string]any{
918918
"owner": owner,
@@ -1005,7 +1005,7 @@ func resourceGithubOrganizationRulesetImport(ctx context.Context, d *schema.Reso
10051005
"ruleset_id": rulesetID,
10061006
})
10071007

1008-
ruleset, _, err := client.Organizations.GetRepositoryRuleset(ctx, owner, rulesetID)
1008+
ruleset, _, err := getOrgRuleset(ctx, client, owner, rulesetID)
10091009
if ruleset == nil || err != nil {
10101010
tflog.Error(ctx, fmt.Sprintf("Failed to import organization ruleset: %s/%d", owner, rulesetID), map[string]any{
10111011
"owner": owner,

github/resource_github_repository_ruleset.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -702,7 +702,7 @@ func resourceGithubRepositoryRulesetCreate(ctx context.Context, d *schema.Resour
702702
return diag.Errorf("cannot create ruleset on archived repository %s/%s", owner, repoName)
703703
}
704704

705-
ruleset, resp, err := client.Repositories.CreateRuleset(ctx, owner, repoName, rulesetReq)
705+
ruleset, resp, err := createRepoRuleset(ctx, client, owner, repoName, rulesetReq)
706706
if err != nil {
707707
return diag.FromErr(err)
708708
}
@@ -739,7 +739,7 @@ func resourceGithubRepositoryRulesetRead(ctx context.Context, d *schema.Resource
739739
ctx = context.WithValue(ctx, ctxEtag, d.Get("etag").(string))
740740
}
741741

742-
ruleset, resp, err := client.Repositories.GetRuleset(ctx, owner, repoName, rulesetID, false)
742+
ruleset, resp, err := getRepoRuleset(ctx, client, owner, repoName, rulesetID, false)
743743
if err != nil {
744744
var ghErr *github.ErrorResponse
745745
if errors.As(err, &ghErr) {
@@ -815,7 +815,7 @@ func resourceGithubRepositoryRulesetUpdate(ctx context.Context, d *schema.Resour
815815
return nil
816816
}
817817

818-
ruleset, resp, err := client.Repositories.UpdateRuleset(ctx, owner, repoName, rulesetID, rulesetReq)
818+
ruleset, resp, err := updateRepoRuleset(ctx, client, owner, repoName, rulesetID, rulesetReq)
819819
if err != nil {
820820
return diag.FromErr(err)
821821
}
@@ -874,7 +874,7 @@ func resourceGithubRepositoryRulesetImport(ctx context.Context, d *schema.Resour
874874
return []*schema.ResourceData{d}, err
875875
}
876876

877-
ruleset, _, err := client.Repositories.GetRuleset(ctx, owner, repository.GetName(), rulesetID, false)
877+
ruleset, _, err := getRepoRuleset(ctx, client, owner, repository.GetName(), rulesetID, false)
878878
if ruleset == nil || err != nil {
879879
return []*schema.ResourceData{d}, err
880880
}

github/util_rules.go

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,12 @@ package github
22

33
import (
44
"context"
5+
"encoding/json"
6+
"fmt"
7+
"io"
8+
"net/http"
59
"reflect"
10+
"regexp"
611
"sort"
712

813
"github.com/google/go-github/v84/github"
@@ -880,6 +885,110 @@ func flattenRules(ctx context.Context, rules *github.RepositoryRulesetRules, org
880885
return []any{rulesMap}
881886
}
882887

888+
// reviewerStringIDPattern matches JSON "id" fields with quoted integer values.
889+
// The GitHub API returns reviewer.id as a JSON string (e.g., "id": "12345") but
890+
// go-github's RulesetReviewer.ID expects *int64, causing unmarshal failures.
891+
// See https://github.com/integrations/terraform-provider-github/issues/3340
892+
var reviewerStringIDPattern = regexp.MustCompile(`"id"\s*:\s*"(\d+)"`)
893+
894+
// fixReviewerStringIDs converts quoted integer "id" fields in raw JSON to
895+
// unquoted integers so that go-github can unmarshal them into *int64 fields.
896+
func fixReviewerStringIDs(data []byte) []byte {
897+
return reviewerStringIDPattern.ReplaceAll(data, []byte(`"id":$1`))
898+
}
899+
900+
// doRulesetRequest executes an HTTP request against the GitHub API and
901+
// unmarshals the response into a RepositoryRuleset. It applies
902+
// fixReviewerStringIDs to the response body before unmarshaling to work around
903+
// the GitHub API returning reviewer.id as a string instead of a number.
904+
func doRulesetRequest(ctx context.Context, client *github.Client, req *http.Request) (*github.RepositoryRuleset, *github.Response, error) {
905+
resp, err := client.BareDo(ctx, req)
906+
if err != nil {
907+
return nil, resp, err
908+
}
909+
defer resp.Body.Close()
910+
911+
body, err := io.ReadAll(resp.Body)
912+
if err != nil {
913+
return nil, resp, err
914+
}
915+
916+
body = fixReviewerStringIDs(body)
917+
918+
var rs *github.RepositoryRuleset
919+
if err := json.Unmarshal(body, &rs); err != nil {
920+
return nil, resp, err
921+
}
922+
923+
return rs, resp, nil
924+
}
925+
926+
// createRepoRuleset creates a repository ruleset, working around the
927+
// reviewer.id string marshaling bug in go-github.
928+
func createRepoRuleset(ctx context.Context, client *github.Client, owner, repo string, ruleset github.RepositoryRuleset) (*github.RepositoryRuleset, *github.Response, error) {
929+
u := fmt.Sprintf("repos/%v/%v/rulesets", owner, repo)
930+
req, err := client.NewRequest("POST", u, ruleset)
931+
if err != nil {
932+
return nil, nil, err
933+
}
934+
return doRulesetRequest(ctx, client, req)
935+
}
936+
937+
// getRepoRuleset gets a repository ruleset, working around the
938+
// reviewer.id string marshaling bug in go-github.
939+
func getRepoRuleset(ctx context.Context, client *github.Client, owner, repo string, rulesetID int64, includesParents bool) (*github.RepositoryRuleset, *github.Response, error) {
940+
u := fmt.Sprintf("repos/%v/%v/rulesets/%v?includes_parents=%v", owner, repo, rulesetID, includesParents)
941+
req, err := client.NewRequest("GET", u, nil)
942+
if err != nil {
943+
return nil, nil, err
944+
}
945+
return doRulesetRequest(ctx, client, req)
946+
}
947+
948+
// updateRepoRuleset updates a repository ruleset, working around the
949+
// reviewer.id string marshaling bug in go-github.
950+
func updateRepoRuleset(ctx context.Context, client *github.Client, owner, repo string, rulesetID int64, ruleset github.RepositoryRuleset) (*github.RepositoryRuleset, *github.Response, error) {
951+
u := fmt.Sprintf("repos/%v/%v/rulesets/%v", owner, repo, rulesetID)
952+
req, err := client.NewRequest("PUT", u, ruleset)
953+
if err != nil {
954+
return nil, nil, err
955+
}
956+
return doRulesetRequest(ctx, client, req)
957+
}
958+
959+
// createOrgRuleset creates an organization ruleset, working around the
960+
// reviewer.id string marshaling bug in go-github.
961+
func createOrgRuleset(ctx context.Context, client *github.Client, org string, ruleset github.RepositoryRuleset) (*github.RepositoryRuleset, *github.Response, error) {
962+
u := fmt.Sprintf("orgs/%v/rulesets", org)
963+
req, err := client.NewRequest("POST", u, ruleset)
964+
if err != nil {
965+
return nil, nil, err
966+
}
967+
return doRulesetRequest(ctx, client, req)
968+
}
969+
970+
// getOrgRuleset gets an organization ruleset, working around the
971+
// reviewer.id string marshaling bug in go-github.
972+
func getOrgRuleset(ctx context.Context, client *github.Client, org string, rulesetID int64) (*github.RepositoryRuleset, *github.Response, error) {
973+
u := fmt.Sprintf("orgs/%v/rulesets/%v", org, rulesetID)
974+
req, err := client.NewRequest("GET", u, nil)
975+
if err != nil {
976+
return nil, nil, err
977+
}
978+
return doRulesetRequest(ctx, client, req)
979+
}
980+
981+
// updateOrgRuleset updates an organization ruleset, working around the
982+
// reviewer.id string marshaling bug in go-github.
983+
func updateOrgRuleset(ctx context.Context, client *github.Client, org string, rulesetID int64, ruleset github.RepositoryRuleset) (*github.RepositoryRuleset, *github.Response, error) {
984+
u := fmt.Sprintf("orgs/%v/rulesets/%v", org, rulesetID)
985+
req, err := client.NewRequest("PUT", u, ruleset)
986+
if err != nil {
987+
return nil, nil, err
988+
}
989+
return doRulesetRequest(ctx, client, req)
990+
}
991+
883992
func bypassActorsDiffSuppressFunc(k, o, n string, d *schema.ResourceData) bool {
884993
// If the length has changed, no need to suppress
885994
if k == "bypass_actors.#" {

github/util_rules_test.go

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package github
22

33
import (
4+
"encoding/json"
45
"testing"
56

67
"github.com/google/go-github/v84/github"
@@ -1299,3 +1300,126 @@ func TestFlattenRulesetRepositoryPropertyTargetParameters_NilPropertyValues(t *t
12991300
}
13001301
}
13011302
}
1303+
1304+
func TestFixReviewerStringIDs(t *testing.T) {
1305+
tests := []struct {
1306+
name string
1307+
input string
1308+
expected string
1309+
}{
1310+
{
1311+
name: "converts quoted integer id to unquoted",
1312+
input: `{"id": "12345", "type": "Team"}`,
1313+
expected: `{"id":12345, "type": "Team"}`,
1314+
},
1315+
{
1316+
name: "leaves unquoted integer id unchanged",
1317+
input: `{"id": 12345, "type": "Team"}`,
1318+
expected: `{"id": 12345, "type": "Team"}`,
1319+
},
1320+
{
1321+
name: "converts id without spaces after colon",
1322+
input: `{"id":"99999","type":"Team"}`,
1323+
expected: `{"id":99999,"type":"Team"}`,
1324+
},
1325+
{
1326+
name: "handles multiple reviewer ids",
1327+
input: `[{"id": "111"}, {"id": "222"}]`,
1328+
expected: `[{"id":111}, {"id":222}]`,
1329+
},
1330+
{
1331+
name: "does not convert non-digit string ids",
1332+
input: `{"id": "abc123", "type": "Team"}`,
1333+
expected: `{"id": "abc123", "type": "Team"}`,
1334+
},
1335+
{
1336+
name: "does not convert empty string ids",
1337+
input: `{"id": "", "type": "Team"}`,
1338+
expected: `{"id": "", "type": "Team"}`,
1339+
},
1340+
{
1341+
name: "handles full ruleset response with nested reviewer",
1342+
input: `{"id":1,"name":"test","rules":[{"type":"pull_request","parameters":{"required_reviewers":[{"reviewer":{"id":"12345","type":"Team"},"minimum_approvals":1}]}}]}`,
1343+
expected: `{"id":1,"name":"test","rules":[{"type":"pull_request","parameters":{"required_reviewers":[{"reviewer":{"id":12345,"type":"Team"},"minimum_approvals":1}]}}]}`,
1344+
},
1345+
}
1346+
1347+
for _, tc := range tests {
1348+
t.Run(tc.name, func(t *testing.T) {
1349+
result := fixReviewerStringIDs([]byte(tc.input))
1350+
if string(result) != tc.expected {
1351+
t.Errorf("fixReviewerStringIDs(%q) = %q, want %q", tc.input, string(result), tc.expected)
1352+
}
1353+
})
1354+
}
1355+
}
1356+
1357+
func TestFixReviewerStringIDs_UnmarshalRoundTrip(t *testing.T) {
1358+
// Simulate the actual GitHub API response that causes the bug:
1359+
// reviewer.id is returned as a JSON string instead of a number.
1360+
apiResponse := `{
1361+
"id": 999,
1362+
"name": "test-ruleset",
1363+
"target": "branch",
1364+
"source_type": "Repository",
1365+
"source": "test-repo",
1366+
"enforcement": "active",
1367+
"rules": [
1368+
{
1369+
"type": "pull_request",
1370+
"parameters": {
1371+
"dismiss_stale_reviews_on_push": false,
1372+
"require_code_owner_review": false,
1373+
"require_last_push_approval": false,
1374+
"required_approving_review_count": 1,
1375+
"required_review_thread_resolution": false,
1376+
"required_reviewers": [
1377+
{
1378+
"reviewer": {
1379+
"id": "12345",
1380+
"type": "Team"
1381+
},
1382+
"file_patterns": ["*.go"],
1383+
"minimum_approvals": 1
1384+
}
1385+
]
1386+
}
1387+
}
1388+
]
1389+
}`
1390+
1391+
// Without the fix, this would fail with:
1392+
// json: cannot unmarshal string into Go struct field
1393+
// RulesetReviewer.id of type int64
1394+
fixed := fixReviewerStringIDs([]byte(apiResponse))
1395+
1396+
var rs github.RepositoryRuleset
1397+
if err := json.Unmarshal(fixed, &rs); err != nil {
1398+
t.Fatalf("Failed to unmarshal fixed response: %v", err)
1399+
}
1400+
1401+
if rs.GetID() != 999 {
1402+
t.Errorf("Expected ruleset ID 999, got %d", rs.GetID())
1403+
}
1404+
1405+
if rs.Rules == nil || rs.Rules.PullRequest == nil {
1406+
t.Fatal("Expected pull_request rule to be parsed")
1407+
}
1408+
1409+
reviewers := rs.Rules.PullRequest.RequiredReviewers
1410+
if len(reviewers) != 1 {
1411+
t.Fatalf("Expected 1 required reviewer, got %d", len(reviewers))
1412+
}
1413+
1414+
if reviewers[0].Reviewer == nil {
1415+
t.Fatal("Expected reviewer to be non-nil")
1416+
}
1417+
1418+
if reviewers[0].Reviewer.GetID() != 12345 {
1419+
t.Errorf("Expected reviewer ID 12345, got %d", reviewers[0].Reviewer.GetID())
1420+
}
1421+
1422+
if reviewers[0].Reviewer.GetType() == nil || *reviewers[0].Reviewer.GetType() != github.RulesetReviewerTypeTeam {
1423+
t.Errorf("Expected reviewer type Team, got %v", reviewers[0].Reviewer.GetType())
1424+
}
1425+
}

0 commit comments

Comments
 (0)