Skip to content
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

Jump to bottom

Check for nil pointer in update rule parameters #2854

Merged
merged 4 commits into from Aug 3, 2023
Merged

Check for nil pointer in update rule parameters #2854

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
049515d
Check for nil pointer in update rule
o-sama Aug 2, 2023
a1e836a
Add test and create update rule based on passed params
o-sama Aug 2, 2023
e9cfeac
Fix typo
o-sama Aug 2, 2023
8238725
Remove unneeded else statement
o-sama Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 22 additions & 12 deletions github/repos_rules.go
View file
Open in desktop
Expand Up @@ -118,15 +118,20 @@ func (r *RepositoryRule) UnmarshalJSON(data []byte) error {
case "creation", "deletion", "required_linear_history", "required_signatures", "non_fast_forward":
r.Parameters = nil
case "update":
params := UpdateAllowsFetchAndMergeRuleParameters{}
if err := json.Unmarshal(*RepositoryRule.Parameters, &params); err != nil {
return err
if RepositoryRule.Parameters == nil {
r.Parameters = nil
return nil
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

One of the points was to remove the "else" and un-indent the following section, please:

Suggested change
} else {
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You’re totally right, I think I had a brain freeze. Don’t know why I kept an else with a return in there.

params := UpdateAllowsFetchAndMergeRuleParameters{}
if err := json.Unmarshal(*RepositoryRule.Parameters, &params); err != nil {
return err
}

bytes, _ := json.Marshal(params)
rawParams := json.RawMessage(bytes)

r.Parameters = &rawParams
}

bytes, _ := json.Marshal(params)
rawParams := json.RawMessage(bytes)

r.Parameters = &rawParams
case "required_deployments":
params := RequiredDeploymentEnvironmentsRuleParameters{}
if err := json.Unmarshal(*RepositoryRule.Parameters, &params); err != nil {
Expand Down Expand Up @@ -185,13 +190,18 @@ func NewCreationRule() (rule *RepositoryRule) {

// NewUpdateRule creates a rule to only allow users with bypass permission to update matching refs.
func NewUpdateRule(params *UpdateAllowsFetchAndMergeRuleParameters) (rule *RepositoryRule) {
bytes, _ := json.Marshal(params)
if params != nil {
bytes, _ := json.Marshal(params)

rawParams := json.RawMessage(bytes)
rawParams := json.RawMessage(bytes)

return &RepositoryRule{
Type: "update",
Parameters: &rawParams,
}
}
return &RepositoryRule{
Type: "update",
Parameters: &rawParams,
Type: "update",
}
}

Expand Down
39 changes: 39 additions & 0 deletions github/repos_rules_test.go
View file
Open in desktop
Expand Up @@ -292,6 +292,45 @@ func TestRepositoriesService_GetRulesForBranch(t *testing.T) {
})
}

func TestRepositoriesService_GetRulesForBranchEmptyUpdateRule(t *testing.T) {
client, mux, _, teardown := setup()
defer teardown()

mux.HandleFunc("/repos/o/repo/rules/branches/branch", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
fmt.Fprint(w, `[
{
"type": "update"
}
]`)
})

ctx := context.Background()
rules, _, err := client.Repositories.GetRulesForBranch(ctx, "o", "repo", "branch")
if err != nil {
t.Errorf("Repositories.GetRulesForBranch returned error: %v", err)
}

updateRule := NewUpdateRule(nil)

want := []*RepositoryRule{
updateRule,
}
if !cmp.Equal(rules, want) {
t.Errorf("Repositories.GetRulesForBranch returned %+v, want %+v", Stringify(rules), Stringify(want))
}

const methodName = "GetRulesForBranch"

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
got, resp, err := client.Repositories.GetRulesForBranch(ctx, "o", "repo", "branch")
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
}
return resp, err
})
}

func TestRepositoriesService_GetAllRulesets(t *testing.T) {
client, mux, _, teardown := setup()
defer teardown()
Expand Down