From 1732c00d8a5a94c7ec53ab22d70421b011164b13 Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Wed, 29 Apr 2020 21:45:45 -0700 Subject: [PATCH] Use new endpoints to get and list references Remove the GetRefs and ListRefs methods in favor of ListMatchingRefs, which performs both functions. The GetRef method remains the same, but now returns a standard 404 error if the reference does not exist. --- github/git_refs.go | 85 ++++-------------------- github/git_refs_test.go | 141 ++++++++++++++++------------------------ update-urls/main.go | 3 - 3 files changed, 71 insertions(+), 158 deletions(-) diff --git a/github/git_refs.go b/github/git_refs.go index a6269e5a9e4..94053aaea1c 100644 --- a/github/git_refs.go +++ b/github/git_refs.go @@ -7,8 +7,6 @@ package github import ( "context" - "encoding/json" - "errors" "fmt" "net/url" "strings" @@ -49,16 +47,12 @@ type updateRefRequest struct { Force *bool `json:"force"` } -// GetRef fetches a single Reference object for a given Git ref. -// If there is no exact match, GetRef will return an error. +// GetRef fetches a single reference in a repository. // -// Note: The GitHub API can return multiple matches. -// If you wish to use this functionality please use the GetRefs() method. -// -// GitHub API docs: https://developer.github.com/v3/git/refs/#get-a-reference +// GitHub API docs: https://developer.github.com/v3/git/refs/#get-a-single-reference func (s *GitService) GetRef(ctx context.Context, owner string, repo string, ref string) (*Reference, *Response, error) { ref = strings.TrimPrefix(ref, "refs/") - u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, refURLEscape(ref)) + u := fmt.Sprintf("repos/%v/%v/git/ref/%v", owner, repo, refURLEscape(ref)) req, err := s.client.NewRequest("GET", u, nil) if err != nil { return nil, nil, err @@ -66,13 +60,7 @@ func (s *GitService) GetRef(ctx context.Context, owner string, repo string, ref r := new(Reference) resp, err := s.client.Do(ctx, req, r) - if _, ok := err.(*json.UnmarshalTypeError); ok { - // Multiple refs, means there wasn't an exact match. - return nil, resp, errors.New("multiple matches found for this ref") - } else if _, ok := err.(*ErrorResponse); ok && resp.StatusCode == 404 { - // No ref, there was no match for the ref - return nil, resp, errors.New("no match found for this ref") - } else if err != nil { + if err != nil { return nil, resp, err } @@ -89,69 +77,24 @@ func refURLEscape(ref string) string { return strings.Join(parts, "/") } -// GetRefs fetches a slice of Reference objects for a given Git ref. -// If there is an exact match, only that ref is returned. -// If there is no exact match, GitHub returns all refs that start with ref. -// If returned error is nil, there will be at least 1 ref returned. -// For example: -// -// "heads/featureA" -> ["refs/heads/featureA"] // Exact match, single ref is returned. -// "heads/feature" -> ["refs/heads/featureA", "refs/heads/featureB"] // All refs that start with ref. -// "heads/notexist" -> [] // Returns an error. -// -// GitHub API docs: https://developer.github.com/v3/git/refs/#get-a-reference -func (s *GitService) GetRefs(ctx context.Context, owner string, repo string, ref string) ([]*Reference, *Response, error) { - ref = strings.TrimPrefix(ref, "refs/") - u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, refURLEscape(ref)) - req, err := s.client.NewRequest("GET", u, nil) - if err != nil { - return nil, nil, err - } - - var rawJSON json.RawMessage - resp, err := s.client.Do(ctx, req, &rawJSON) - if err != nil { - return nil, resp, err - } - - // Prioritize the most common case: a single returned ref. - r := new(Reference) - singleUnmarshalError := json.Unmarshal(rawJSON, r) - if singleUnmarshalError == nil { - return []*Reference{r}, resp, nil - } - - // Attempt to unmarshal multiple refs. - var rs []*Reference - multipleUnmarshalError := json.Unmarshal(rawJSON, &rs) - if multipleUnmarshalError == nil { - if len(rs) == 0 { - return nil, resp, fmt.Errorf("unexpected response from GitHub API: an array of refs with length 0") - } - return rs, resp, nil - } - - return nil, resp, fmt.Errorf("unmarshalling failed for both single and multiple refs: %s and %s", singleUnmarshalError, multipleUnmarshalError) -} - // ReferenceListOptions specifies optional parameters to the -// GitService.ListRefs method. +// GitService.ListMatchingRefs method. type ReferenceListOptions struct { - Type string `url:"-"` + Ref string `url:"-"` ListOptions } -// ListRefs lists all refs in a repository. +// ListMatchingRefs lists references in a repository that match a supplied ref. +// Use an empty ref to list all references. // -// GitHub API docs: https://developer.github.com/v3/git/refs/#get-all-references -func (s *GitService) ListRefs(ctx context.Context, owner, repo string, opts *ReferenceListOptions) ([]*Reference, *Response, error) { - var u string - if opts != nil && opts.Type != "" { - u = fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, opts.Type) - } else { - u = fmt.Sprintf("repos/%v/%v/git/refs", owner, repo) +// GitHub API docs: https://developer.github.com/v3/git/refs/#list-matching-references +func (s *GitService) ListMatchingRefs(ctx context.Context, owner, repo string, opts *ReferenceListOptions) ([]*Reference, *Response, error) { + var ref string + if opts != nil { + ref = strings.TrimPrefix(opts.Ref, "refs/") } + u := fmt.Sprintf("repos/%v/%v/git/matching-refs/%v", owner, repo, refURLEscape(ref)) u, err := addOptions(u, opts) if err != nil { return nil, nil, err diff --git a/github/git_refs_test.go b/github/git_refs_test.go index 22dc8eb6247..73c193b20f1 100644 --- a/github/git_refs_test.go +++ b/github/git_refs_test.go @@ -19,7 +19,7 @@ func TestGitService_GetRef_singleRef(t *testing.T) { client, mux, _, teardown := setup() defer teardown() - mux.HandleFunc("/repos/o/r/git/refs/heads/b", func(w http.ResponseWriter, r *http.Request) { + mux.HandleFunc("/repos/o/r/git/ref/heads/b", func(w http.ResponseWriter, r *http.Request) { testMethod(t, r, "GET") fmt.Fprint(w, ` { @@ -64,75 +64,44 @@ func TestGitService_GetRef_noRefs(t *testing.T) { mux.HandleFunc("/repos/o/r/git/refs/heads/b", func(w http.ResponseWriter, r *http.Request) { testMethod(t, r, "GET") w.WriteHeader(http.StatusNotFound) - fmt.Fprint(w, "[]") }) - _, _, err := client.Git.GetRef(context.Background(), "o", "r", "refs/heads/b") - want := "no match found for this ref" - if err.Error() != want { - t.Errorf("Git.GetRef returned %+v, want %+v", err, want) + ref, resp, err := client.Git.GetRef(context.Background(), "o", "r", "refs/heads/b") + if err == nil { + t.Errorf("Expected HTTP 404 response") + } + if got, want := resp.Response.StatusCode, http.StatusNotFound; got != want { + t.Errorf("Git.GetRef returned status %d, want %d", got, want) + } + if ref != nil { + t.Errorf("Git.GetRef return %+v, want nil", ref) } } -func TestGitService_GetRef_multipleRefs(t *testing.T) { +func TestGitService_ListMatchingRefs_singleRef(t *testing.T) { client, mux, _, teardown := setup() defer teardown() - mux.HandleFunc("/repos/o/r/git/refs/heads/b", func(w http.ResponseWriter, r *http.Request) { + mux.HandleFunc("/repos/o/r/git/matching-refs/heads/b", func(w http.ResponseWriter, r *http.Request) { testMethod(t, r, "GET") fmt.Fprint(w, ` [ { - "ref": "refs/heads/booger", - "url": "https://api.github.com/repos/o/r/git/refs/heads/booger", - "object": { - "type": "commit", - "sha": "aa218f56b14c9653891f9e74264a383fa43fefbd", - "url": "https://api.github.com/repos/o/r/git/commits/aa218f56b14c9653891f9e74264a383fa43fefbd" - } - }, - { - "ref": "refs/heads/bandsaw", - "url": "https://api.github.com/repos/o/r/git/refs/heads/bandsaw", + "ref": "refs/heads/b", + "url": "https://api.github.com/repos/o/r/git/refs/heads/b", "object": { "type": "commit", - "sha": "612077ae6dffb4d2fbd8ce0cccaa58893b07b5ac", - "url": "https://api.github.com/repos/o/r/git/commits/612077ae6dffb4d2fbd8ce0cccaa58893b07b5ac" + "sha": "aa218f56b14c9653891f9e74264a383fa43fefbd", + "url": "https://api.github.com/repos/o/r/git/commits/aa218f56b14c9653891f9e74264a383fa43fefbd" } } - ] - `) - }) - - _, _, err := client.Git.GetRef(context.Background(), "o", "r", "refs/heads/b") - want := "multiple matches found for this ref" - if err.Error() != want { - t.Errorf("Git.GetRef returned %+v, want %+v", err, want) - } - -} - -func TestGitService_GetRefs_singleRef(t *testing.T) { - client, mux, _, teardown := setup() - defer teardown() - - mux.HandleFunc("/repos/o/r/git/refs/heads/b", func(w http.ResponseWriter, r *http.Request) { - testMethod(t, r, "GET") - fmt.Fprint(w, ` - { - "ref": "refs/heads/b", - "url": "https://api.github.com/repos/o/r/git/refs/heads/b", - "object": { - "type": "commit", - "sha": "aa218f56b14c9653891f9e74264a383fa43fefbd", - "url": "https://api.github.com/repos/o/r/git/commits/aa218f56b14c9653891f9e74264a383fa43fefbd" - } - }`) + ]`) }) - refs, _, err := client.Git.GetRefs(context.Background(), "o", "r", "refs/heads/b") + opts := &ReferenceListOptions{Ref: "refs/heads/b"} + refs, _, err := client.Git.ListMatchingRefs(context.Background(), "o", "r", opts) if err != nil { - t.Fatalf("Git.GetRefs returned error: %v", err) + t.Fatalf("Git.ListMatchingRefs returned error: %v", err) } ref := refs[0] @@ -146,20 +115,21 @@ func TestGitService_GetRefs_singleRef(t *testing.T) { }, } if !reflect.DeepEqual(ref, want) { - t.Errorf("Git.GetRefs returned %+v, want %+v", ref, want) + t.Errorf("Git.ListMatchingRefs returned %+v, want %+v", ref, want) } // without 'refs/' prefix - if _, _, err := client.Git.GetRefs(context.Background(), "o", "r", "heads/b"); err != nil { - t.Errorf("Git.GetRefs returned error: %v", err) + opts = &ReferenceListOptions{Ref: "heads/b"} + if _, _, err := client.Git.ListMatchingRefs(context.Background(), "o", "r", opts); err != nil { + t.Errorf("Git.ListMatchingRefs returned error: %v", err) } } -func TestGitService_GetRefs_multipleRefs(t *testing.T) { +func TestGitService_ListMatchingRefs_multipleRefs(t *testing.T) { client, mux, _, teardown := setup() defer teardown() - mux.HandleFunc("/repos/o/r/git/refs/heads/b", func(w http.ResponseWriter, r *http.Request) { + mux.HandleFunc("/repos/o/r/git/matching-refs/heads/b", func(w http.ResponseWriter, r *http.Request) { testMethod(t, r, "GET") fmt.Fprint(w, ` [ @@ -185,9 +155,10 @@ func TestGitService_GetRefs_multipleRefs(t *testing.T) { `) }) - refs, _, err := client.Git.GetRefs(context.Background(), "o", "r", "refs/heads/b") + opts := &ReferenceListOptions{Ref: "refs/heads/b"} + refs, _, err := client.Git.ListMatchingRefs(context.Background(), "o", "r", opts) if err != nil { - t.Errorf("Git.GetRefs returned error: %v", err) + t.Errorf("Git.ListMatchingRefs returned error: %v", err) } want := &Reference{ @@ -200,33 +171,35 @@ func TestGitService_GetRefs_multipleRefs(t *testing.T) { }, } if !reflect.DeepEqual(refs[0], want) { - t.Errorf("Git.GetRefs returned %+v, want %+v", refs[0], want) + t.Errorf("Git.ListMatchingRefs returned %+v, want %+v", refs[0], want) } } -// TestGitService_GetRefs_noRefs tests for behaviour resulting from an unexpected GH response. This should never actually happen. -func TestGitService_GetRefs_noRefs(t *testing.T) { +func TestGitService_ListMatchingRefs_noRefs(t *testing.T) { client, mux, _, teardown := setup() defer teardown() - mux.HandleFunc("/repos/o/r/git/refs/heads/b", func(w http.ResponseWriter, r *http.Request) { + mux.HandleFunc("/repos/o/r/git/matching-refs/heads/b", func(w http.ResponseWriter, r *http.Request) { testMethod(t, r, "GET") fmt.Fprint(w, "[]") }) - _, _, err := client.Git.GetRefs(context.Background(), "o", "r", "refs/heads/b") - want := "unexpected response from GitHub API: an array of refs with length 0" - if err.Error() != want { - t.Errorf("Git.GetRefs returned %+v, want %+v", err, want) + opts := &ReferenceListOptions{Ref: "refs/heads/b"} + refs, _, err := client.Git.ListMatchingRefs(context.Background(), "o", "r", opts) + if err != nil { + t.Errorf("Git.ListMatchingRefs returned error: %v", err) } + if len(refs) != 0 { + t.Errorf("Git.ListMatchingRefs returned %+v, want an empty slice", refs) + } } -func TestGitService_ListRefs(t *testing.T) { +func TestGitService_ListMatchingRefs_allRefs(t *testing.T) { client, mux, _, teardown := setup() defer teardown() - mux.HandleFunc("/repos/o/r/git/refs", func(w http.ResponseWriter, r *http.Request) { + mux.HandleFunc("/repos/o/r/git/matching-refs/", func(w http.ResponseWriter, r *http.Request) { testMethod(t, r, "GET") fmt.Fprint(w, ` [ @@ -234,26 +207,26 @@ func TestGitService_ListRefs(t *testing.T) { "ref": "refs/heads/branchA", "url": "https://api.github.com/repos/o/r/git/refs/heads/branchA", "object": { - "type": "commit", - "sha": "aa218f56b14c9653891f9e74264a383fa43fefbd", - "url": "https://api.github.com/repos/o/r/git/commits/aa218f56b14c9653891f9e74264a383fa43fefbd" + "type": "commit", + "sha": "aa218f56b14c9653891f9e74264a383fa43fefbd", + "url": "https://api.github.com/repos/o/r/git/commits/aa218f56b14c9653891f9e74264a383fa43fefbd" } }, { "ref": "refs/heads/branchB", "url": "https://api.github.com/repos/o/r/git/refs/heads/branchB", "object": { - "type": "commit", - "sha": "aa218f56b14c9653891f9e74264a383fa43fefbd", - "url": "https://api.github.com/repos/o/r/git/commits/aa218f56b14c9653891f9e74264a383fa43fefbd" + "type": "commit", + "sha": "aa218f56b14c9653891f9e74264a383fa43fefbd", + "url": "https://api.github.com/repos/o/r/git/commits/aa218f56b14c9653891f9e74264a383fa43fefbd" } } ]`) }) - refs, _, err := client.Git.ListRefs(context.Background(), "o", "r", nil) + refs, _, err := client.Git.ListMatchingRefs(context.Background(), "o", "r", nil) if err != nil { - t.Errorf("Git.ListRefs returned error: %v", err) + t.Errorf("Git.ListMatchingRefs returned error: %v", err) } want := []*Reference{ @@ -277,29 +250,29 @@ func TestGitService_ListRefs(t *testing.T) { }, } if !reflect.DeepEqual(refs, want) { - t.Errorf("Git.ListRefs returned %+v, want %+v", refs, want) + t.Errorf("Git.ListMatchingRefs returned %+v, want %+v", refs, want) } } -func TestGitService_ListRefs_options(t *testing.T) { +func TestGitService_ListMatchingRefs_options(t *testing.T) { client, mux, _, teardown := setup() defer teardown() - mux.HandleFunc("/repos/o/r/git/refs/t", func(w http.ResponseWriter, r *http.Request) { + mux.HandleFunc("/repos/o/r/git/matching-refs/t", func(w http.ResponseWriter, r *http.Request) { testMethod(t, r, "GET") testFormValues(t, r, values{"page": "2"}) fmt.Fprint(w, `[{"ref": "r"}]`) }) - opts := &ReferenceListOptions{Type: "t", ListOptions: ListOptions{Page: 2}} - refs, _, err := client.Git.ListRefs(context.Background(), "o", "r", opts) + opts := &ReferenceListOptions{Ref: "t", ListOptions: ListOptions{Page: 2}} + refs, _, err := client.Git.ListMatchingRefs(context.Background(), "o", "r", opts) if err != nil { - t.Errorf("Git.ListRefs returned error: %v", err) + t.Errorf("Git.ListMatchingRefs returned error: %v", err) } want := []*Reference{{Ref: String("r")}} if !reflect.DeepEqual(refs, want) { - t.Errorf("Git.ListRefs returned %+v, want %+v", refs, want) + t.Errorf("Git.ListMatchingRefs returned %+v, want %+v", refs, want) } } @@ -450,7 +423,7 @@ func TestGitService_GetRef_pathEscape(t *testing.T) { client, mux, _, teardown := setup() defer teardown() - mux.HandleFunc("/repos/o/r/git/refs/heads/b", func(w http.ResponseWriter, r *http.Request) { + mux.HandleFunc("/repos/o/r/git/ref/heads/b", func(w http.ResponseWriter, r *http.Request) { testMethod(t, r, "GET") if strings.Contains(r.URL.RawPath, "%2F") { t.Errorf("RawPath still contains escaped / as %%2F: %v", r.URL.RawPath) diff --git a/update-urls/main.go b/update-urls/main.go index 73c2e9ca3aa..3c07812467d 100644 --- a/update-urls/main.go +++ b/update-urls/main.go @@ -61,9 +61,6 @@ var ( "AppsService.FindRepositoryInstallationByID": true, "AuthorizationsService.CreateImpersonation": true, "AuthorizationsService.DeleteImpersonation": true, - "GitService.GetRef": true, - "GitService.GetRefs": true, - "GitService.ListRefs": true, "MarketplaceService.marketplaceURI": true, "OrganizationsService.GetByID": true, "RepositoriesService.DeletePreReceiveHook": true,