Skip to content

fix: Handle string-typed reviewer ID in Ruleset API responses#4178

Merged
gmlewis merged 5 commits intogoogle:masterfrom
mmelodyRTR:fix-ruleset-reviewer-id
Apr 25, 2026
Merged

fix: Handle string-typed reviewer ID in Ruleset API responses#4178
gmlewis merged 5 commits intogoogle:masterfrom
mmelodyRTR:fix-ruleset-reviewer-id

Conversation

@mmelodyRTR
Copy link
Copy Markdown
Contributor

@mmelodyRTR mmelodyRTR commented Apr 24, 2026

Summary

This PR addresses a JSON unmarshaling error in RulesetReviewer.ID when interacting with the GitHub Repository Ruleset API.

While the GitHub API documentation specifies that the reviewer ID should be an integer, the API currently returns these IDs as quoted strings (e.g., "id": "123456"). This causes the standard Go JSON decoder to fail when unmarshaling into the *int64 field.

Related Issues

This issue was identified via the GitHub Terraform Provider: integrations/terraform-provider-github/issues/3214

Changes

  • Implemented a custom UnmarshalJSON for RulesetReviewer in github/rules.go.
  • The unmarshaler robustly handles both numeric (float64) and string-quoted IDs, ensuring compatibility with both the documented and actual API behavior.
  • Maintained the *int64 type for RulesetReviewer.ID to preserve consistency with the rest of the library and avoid breaking changes for existing users.
  • Added a regression test case to TestRepositoryRule and a new comprehensive unit test suite TestRulesetReviewer_UnmarshalJSON in github/rules_test.go.

Verification

  • Verified the API behavior using gh api against a live repository.
  • Ran all tests in github/rules_test.go, which now pass with the fix.

@google-cla
Copy link
Copy Markdown

google-cla Bot commented Apr 24, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@mmelodyRTR
Copy link
Copy Markdown
Contributor Author

Full disclosure: I'm not a go developer and have used agentic ai/coding to drive this implementation. Will take any feedback on board in an effort to provide a proper go solution in line with repository standards

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2026

Codecov Report

❌ Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.69%. Comparing base (9fbf4be) to head (23f5d8a).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
github/rules.go 89.47% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4178      +/-   ##
==========================================
- Coverage   93.69%   93.69%   -0.01%     
==========================================
  Files         210      210              
  Lines       19741    19760      +19     
==========================================
+ Hits        18497    18514      +17     
- Misses       1048     1049       +1     
- Partials      196      197       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @mmelodyRTR!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.

cc: @stevehipwell - @alexandear - @zyfy29 - @Not-Dhananjay-Mishra - @munlicode

@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented Apr 24, 2026

@mmelodyRTR - if you want to make the PR even better, Codecov is reporting that the following line is not covered by a unit test. You could ask the agent to see if it can come up with a unit test that would cover the failure on line 498:

uncovered

@mmelodyRTR
Copy link
Copy Markdown
Contributor Author

Thanks for the feedback @gmlewis - added a test case as requested 🫡

Copy link
Copy Markdown
Contributor

@Not-Dhananjay-Mishra Not-Dhananjay-Mishra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread github/rules_test.go
Comment on lines +1240 to +1241
},
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
},
}
},
{
name: "invalid_type",
json: `{"id": 123456, "type": 123456}`,
wantErr: true,
},
}

This test case might cover those lines.

Image

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codecov still claims it is not covered, but that's OK. We can proceed anyway.

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Apr 25, 2026
@gmlewis gmlewis changed the title fix: handle string-typed reviewer ID in Ruleset API responses fix: Handle string-typed reviewer ID in Ruleset API responses Apr 25, 2026
@gmlewis gmlewis merged commit f019b7f into google:master Apr 25, 2026
9 of 10 checks passed
@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented Apr 25, 2026

Thank you, @mmelodyRTR and @Not-Dhananjay-Mishra!

@mmelodyRTR mmelodyRTR deleted the fix-ruleset-reviewer-id branch April 25, 2026 22:44
@mmelodyRTR
Copy link
Copy Markdown
Contributor Author

Thanks folks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants