Branch protection status checks returns error for unprotected branches

[jimmidyson]

Response is 404 so throws an error, instead of returning nil slice of required contexts. Sending in a quick PR.

[dmitshur]

This is strange, but I'm getting 404 even for a seemingly protected branch. Are you able to get a 200 OK response still?

I tried it on two repos I know have protected master branches, and I get 404 for everything other than "get branch" endpoint.

For example:

$ curl -H "Accept: application/vnd.github.loki-preview+json" \
'https://api.github.com/repos/gregjones/httpcache/branches/master'
{
  "name": "master",
...
  "protected": true,
  "protection": {
    "enabled": true,
    "required_status_checks": {
      "enforcement_level": "non_admins",
      "contexts": [
        "continuous-integration/travis-ci"
      ]
    }
  },
  "protection_url": "https://api.github.com/repos/gregjones/httpcache/branches/master/protection"
}

$ curl -H "Accept: application/vnd.github.loki-preview+json" \
'https://api.github.com/repos/gregjones/httpcache/branches/master/protection/required_status_checks'
{
  "message": "Not Found",
  "documentation_url": "https://developer.github.com/v3"
}

$ curl -H "Accept: application/vnd.github.loki-preview+json" \
'https://api.github.com/repos/gregjones/httpcache/branches/master/protection/required_status_checks/contexts'
{
  "message": "Not Found",
  "documentation_url": "https://developer.github.com/v3"
}

I also tried authenticating with repo scope, no difference.

Any ideas what I'm doing wrong? /cc @jimmidyson

[jimmidyson]

I'm not sure of the exact scope required to see branch protection but it works fine for me when using a token with all scopes on one of my repos :) Perhaps you don't have enough rights to that particular repo?

[dmitshur]

Ok, let me try again with all scopes. Thanks.

[dmitshur]

Thanks, that was helpful. I was able to reproduce 200 OK responses now.

It turns out the issue was that I needed admin access to the repository, push access isn't enough. For scopes, just public_repo scope is sufficient.

It's strange, given one can access the same information via "Get Branch" endpoint, but I'll write it off as overly-strict-permission-requirement quirks of preview API.

Now, I get responses such as:

$ curl -i -H "Authorization: token <omitted>" -H "Accept: application/vnd.github.loki-preview+json" 'https://api.github.com/repos/shurcooL/home/branches/master/protection/required_status_checks'
HTTP/1.1 200 OK
{
  "url": "https://api.github.com/repos/shurcooL/home/branches/master/protection/required_status_checks",
  "strict": false,
  "include_admins": false,
  "contexts": [
    "ci/gopherci/push",
    "continuous-integration/travis-ci"
  ],
  "contexts_url": "https://api.github.com/repos/shurcooL/home/branches/master/protection/required_status_checks/contexts"
}

$ curl -i -H "Authorization: token <omitted>" -H "Accept: application/vnd.github.loki-preview+json" 'https://api.github.com/repos/shurcooL/home/branches/gorilla-schema-to-parse-query-form/protection/required_status_checks'
HTTP/1.1 404 Not Found
{
  "message": "Branch not protected",
  "documentation_url": "https://developer.github.com/v3"
}

[dmitshur]

I noticed there's a subtle difference between no contexts, and zero contexts. When a branch is protected, you get:

$ curl -i -H "Authorization: token <omitted>" -H "Accept: application/vnd.github.loki-preview+json" 'https://api.github.com/repos/shurcooL/home/branches/master/protection/required_status_checks/contexts'
HTTP/1.1 200 OK
[
  "ci/gopherci/push",
  "continuous-integration/travis-ci"
]

Or, if there are no contexts, you get empty array:

$ curl -i -H "Authorization: token <omitted>" -H "Accept: application/vnd.github.loki-preview+json" 'https://api.github.com/repos/shurcooL/home/branches/master/protection/required_status_checks/contexts'
HTTP/1.1 200 OK
[

]

But if it's not protected, you get 404:

$ curl -i -H "Authorization: token <omitted>" -H "Accept: application/vnd.github.loki-preview+json" 'https://api.github.com/repos/shurcooL/home/branches/master/protection/required_status_checks/contexts'
HTTP/1.1 404 Not Found
{
  "message": "Branch not protected",
  "documentation_url": "https://developer.github.com/v3"
}

With the current implementation of PR #626, one cannot tell the difference between the 2nd and 3rd situations.

Maybe that's okay, and for the best. But an alternative solution we can consider is returning an error such as ErrBranchUnprotected, when the GitHub response is 404 with "message": "Branch not protected".

I'm not sure if that's better and worth it, though. What do you think? /cc @jimmidyson @gmlewis

[dmitshur]

My thoughts on the above... I think such a ErrBranchUnprotected may have some positive benefit, but it's too early to say.

I propose we go ahead and merge #626 as is, which returns nil error and nil pointer/empty slices for the getter methods when a branch is unprotected.

If there's demand or evidence in the future that it'd be better for those methods to report unprotected branch as an error, we can make a breaking API change and introduce it then. It's hard to introduce such breaking changes, but I think we reserve the right as long as this API is still in preview on GitHub side.

[dmitshur]

Actually, after thinking more about it, I'm becoming more convinced that returning an error for unprotected branches is a better thing to do.

It might also be very easy to implement by adding detection of the 404 response in CheckResponse.

Here's my rationale. If you look at names of relevant endpoints at https://developer.github.com/v3/repos/branches/, they all have "of protected branch" as a common suffix. That tells me their behavior is defined for protected branches only, and it's very reasonable to return a "this is an unprotected branch" error if the branch is not protected.

To give an analogy, suppose you make an API call to list all branches of a repository. If the repository doesn't exist, it makes more sense to get a "repo doesn't exist" error, rather than an empty list of branches and nil error, right?

Thoughts?

[gmlewis]

I agree. I like exposing the extra detail as it helps developers and users of the API distinguish between the two whereas if we hide the distinction, we cause a great deal more frustration and people would have to instrument the code to figure out what is going on underneath.

So I am all in-favor of ErrBranchUnprotected.

[dmitshur]

Ok, so we're in agreement that querying protection details on an unprotected branch should return a non-nil error.

I think this issue is about making that error detectable and different from other types of unexpected errors (such as, some network error or GitHub API being down).

Whatever the case, we'll need to document what the error is when a branch is unprotected.

Returning a specific ErrBranchUnprotected is one possible solution.

Another solution I'm thinking about is documenting that a "not found" error is returned when the branch is unprotected.

Since GitHub API already returns 404 Not Found responses, I feel that's both a good mapping, and also one that's easier to implement generally.

We can create a helper (inspired by os.IsNotExist) that reports whether an error is of "not found" category:

// IsNotFound reports whether the error is known to report that a resource was not found.
// It is satisfied by a response from GitHub API with 404 Not Found status code.
func IsNotFound(err error) bool {
	err, ok := err.(*ErrorResponse)
	return ok && err.Response.StatusCode == http.StatusNotFound
}

Then we can document methods such as GetRequiredStatusChecks like this:

// GetRequiredStatusChecks gets the required status checks for a given protected branch.
// A not found error, such that github.IsNotFound(err) reports true, is returned if the branch
// is not protected.
//
// GitHub API docs: https://developer.github.com/v3/repos/branches/#get-required-status-checks-of-protected-branch
func (s *RepositoriesService) GetRequiredStatusChecks(ctx context.Context, owner, repo, branch string) (*RequiredStatusChecks, *Response, error) {

If we really think it's worth it to return more specific ErrBranchUnprotected errors for these methods, then I would suggest we implement that in the relevant methods themselves, something like:

func (s *RepositoriesService) GetRequiredStatusChecks(ctx context.Context, owner, repo, branch string) (*RequiredStatusChecks, *Response, error) {
	...

	resp, err := s.client.Do(ctx, req, p)
	if err != nil {
		if er, ok := err.(*ErrorResponse); ok &&
			er.Response.StatusCode == http.StatusNotFound &&
			er.Message == "Branch not protected" {
				return ErrBranchUnprotected
		}
		return nil, resp, err
	}

	return p, resp, nil
}

(That logic can be moved into a private helper.)

[gmlewis]

I like IsNotFound... that keeps things simple and still understandable.

[dmitshur]

Bikeshed question, is it better to call it IsNotFound or IsNotExist, to mirror language used by os.IsNotExist?

[jimmidyson]

I'm in two minds about this. While it sounds fine, GitHub returns 404s for unauthorized so a 404 doesn't necessarily mean the branch is unprotected.

[dmitshur]

True. To be really sure it's unprotected, we need this check to be done:

er.Message == "Branch not protected"

Either by the user, or by us. This doesn't sound like something that's reasonable for us to ask the user to do, so I think we should.

[jimmidyson]

So that points more to the ErrBranchUnprotected or github.IsBranchNotProtected (or whatever) rather than github.IsNotFound which is more generic.