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.

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

Fix incorrect marshalling of DismissalRestrictionsRequest field. #872

Closed
wants to merge 1 commit into from
Closed

Conversation

ethomas2
Copy link

Previously, an empty DismissalRestrictionsRequest was being marshalled
to an empty array [] instead of an empty object {}. This is an invalid
api call according to github's documentation, and caused github to
respond with a 422.

Fixes #664

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Mar 17, 2018
@ethomas2
Copy link
Author

ethomas2 commented Mar 17, 2018

According to github's docs, to disable dismissal_restrictions, you are supposed to pass an empty object {}. go-github was passing an empty array [], which caused github to respond with a 422

It looks like this mistake was made in go-github because there used to be a typo in github's documentation that said to pass in an empty array. That typo has since been fixed in docs. I believe passing an array instead of an object has always resulted in a 422, even when the docs were incorrect.

screen shot 2018-03-17 at 3 47 11 pm

@ethomas2
Copy link
Author

ethomas2 commented Mar 17, 2018

Somewhat related to this pull this pull request:

There are several array-fields in structs for branch protection requests that get marshaled to nil instead of []. Github responds with a 422 when these fields are nil. For example

	_, _, err := ghclient.Repositories.UpdateBranchProtection(
		ctx, "MyOrg", "myrepo", "master",
		&github.ProtectionRequest{
			RequiredPullRequestReviews: &github.PullRequestReviewsEnforcementRequest{},
			Restrictions:               &github.BranchRestrictionsRequest{},
		})

gets this response from github

PUT https://git.dev.box.net/api/v3/repositories/3333/branches/master/protection: 422 Invalid request.

No subschema in "anyOf" matched.
For 'properties/users', nil is not an array.
For 'properties/teams', nil is not an array.
Not all subschemas of "allOf" matched.
For 'anyOf/1', {"users"=>nil, "teams"=>nil} is not a null. []

Instead you must explicitly set Users and Teams to be an empty array instead of a nil array

	_, _, err := ghclient.Repositories.UpdateBranchProtection(
		ctx, "MyOrg", "myrepo", "master",
		&github.ProtectionRequest{
			RequiredPullRequestReviews: &github.PullRequestReviewsEnforcementRequest{},
			Restrictions: &github.BranchRestrictionsRequest{
				Users: []string{},
				Teams: []string{},
			},
		})

There are more structs other than github.BranchRestrictionsRequest which follow this pattern. I've read go-github's docs for these fields and I realize this behavior is intentional, but I think it provides a poor experience. In my own project, i've resorted to writing my own branch protection structs that marshall nil arrays to []. Would you guys be open to me opening another pull request that marshals nil arrays to []?

Previously, an empty DismissalRestrictionsRequest was being marshalled
to an empty array [] instead of an empty object {}. This is an invalid
api call according to github's documentation, and caused github to
respond with a 422.

Fixes #664
@dmitshur
Copy link
Member

This overlaps with #791.

@ethomas2
Copy link
Author

I think #791 makes this pr unnecessary. Closing

@ethomas2 ethomas2 closed this Mar 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants