Skip to content

fix: Handle missing reviewer type in RequiredReviewer unmarshal#4270

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gmlewis merged 1 commit into from
Jun 4, 2026
Merged

fix: Handle missing reviewer type in RequiredReviewer unmarshal #4270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions github/repos_environments.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -8,6 +8,7 @@ package github
import (
"context"
"encoding/json"
"errors"
"fmt"
"net/http"
)
Expand DownExpand Up@@ -82,6 +83,10 @@ func (r *RequiredReviewer) UnmarshalJSON(data []byte) error {
}

r.Type = reviewer.Type
if reviewer.Type == nil {

@alexandearalexandearJun 4, 2026

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.

Has GitHub's API ever actually returned a null or missing type field in a RequiredReviewer response in the wild, or is this a defensive guard against a theoretically possible but unobserved case?

@gmlewisgmlewisJun 4, 2026•
edited
Loading

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.

Has GitHub's API ever actually returned a null or missing type field in a RequiredReviewer response in the wild, or is this a defensive guard against a theoretically possible but unobserved case?

The same thought occurred to me, and I came to the conclusion that it was most likely the latter, but then thought that this change wouldn't hurt, so I'm OK to move forward with it regardless of the answer. However, if you have objections if this is the former EDIT: latter (whups!), then I'm happy to hear them. 😄

@bulutmufbulutmufJun 4, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Hi @alexandear and @gmlewis, thank you for the review! Yes, this is primarily a defensive guard. While a missing type field might not have been observed in the wild yet, I wanted to ensure the Unmarshal function returns a graceful error rather than risking a nil pointer dereference (panic) for the consumers if the API response ever changes or behaves unexpectedly. Thanks again for your time and maintaining such an awesome project. I'm happy to wait for the second approval, but let me know if you need any adjustments 😄

r.Reviewer = nil
return errors.New("reviewer.Type is nil, not a string of 'User' or 'Team', unable to unmarshal")
}

switch *reviewer.Type {
case "User":
Expand Down
10 changes: 10 additions & 0 deletions github/repos_environments_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -51,6 +51,16 @@ func TestRequiredReviewer_UnmarshalJSON(t *testing.T) {
wantRule: []*RequiredReviewer{{Type: nil, Reviewer: nil}},
wantError: true,
},
"Missing Type in Reviewer Object": {
data: []byte(`[{"reviewer": {"id": 1}}]`),
wantRule: []*RequiredReviewer{{Type: nil, Reviewer: nil}},
wantError: true,
},
"Null Type in Reviewer Object": {
data: []byte(`[{"type": null, "reviewer": {"id": 1}}]`),
wantRule: []*RequiredReviewer{{Type: nil, Reviewer: nil}},
wantError: true,
},
"Wrong ID Type in User Object": {
data: []byte(`[{"type": "User", "reviewer": {"id": "string"}}]`),
wantRule: []*RequiredReviewer{{Type: Ptr("User"), Reviewer: nil}},
Expand Down
Loading