Skip to content

Add opt-in rate limit support on endpoints returning 302s #3411

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
merged 2 commits into from
Jan 2, 2025
Merged

Add opt-in rate limit support on endpoints returning 302s #3411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bce9d21
Add opt-in rate limit support on endpoints returning 302s
wmouchere Dec 31, 2024
5182066
PR Review: make code more idiomatic and improve coverage
wmouchere Dec 31, 2024
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
30 changes: 29 additions & 1 deletion github/actions_artifacts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,14 +142,22 @@ func (s *ActionsService) GetArtifact(ctx context.Context, owner, repo string, ar
func (s *ActionsService) DownloadArtifact(ctx context.Context, owner, repo string, artifactID int64, maxRedirects int) (*url.URL, *Response, error) {
u := fmt.Sprintf("repos/%v/%v/actions/artifacts/%v/zip", owner, repo, artifactID)

if s.client.RateLimitRedirectionalEndpoints {
return s.downloadArtifactWithRateLimit(ctx, u, maxRedirects)
}

return s.downloadArtifactWithoutRateLimit(ctx, u, maxRedirects)
}

func (s *ActionsService) downloadArtifactWithoutRateLimit(ctx context.Context, u string, maxRedirects int) (*url.URL, *Response, error) {
resp, err := s.client.roundTripWithOptionalFollowRedirect(ctx, u, maxRedirects)
if err != nil {
return nil, nil, err
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusFound {
return nil, newResponse(resp), fmt.Errorf("unexpected status code: %s", resp.Status)
return nil, newResponse(resp), fmt.Errorf("unexpected status code: %v", resp.Status)
}

parsedURL, err := url.Parse(resp.Header.Get("Location"))
Expand All @@ -160,6 +168,26 @@ func (s *ActionsService) DownloadArtifact(ctx context.Context, owner, repo strin
return parsedURL, newResponse(resp), nil
}

func (s *ActionsService) downloadArtifactWithRateLimit(ctx context.Context, u string, maxRedirects int) (*url.URL, *Response, error) {
req, err := s.client.NewRequest("GET", u, nil)
if err != nil {
return nil, nil, err
}

url, resp, err := s.client.bareDoUntilFound(ctx, req, maxRedirects)
if err != nil {
return nil, resp, err
}
defer resp.Body.Close()

// If we didn't receive a valid Location in a 302 response
if url == nil {
return nil, resp, fmt.Errorf("unexpected status code: %v", resp.Status)
}

return url, resp, nil
}

// DeleteArtifact deletes a workflow run artifact.
//
// GitHub API docs: https://docs.github.com/rest/actions/artifacts#delete-an-artifact
Expand Down
296 changes: 228 additions & 68 deletions github/actions_artifacts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"fmt"
"net/http"
"net/url"
"strings"
"testing"

"github.com/google/go-cmp/cmp"
Expand Down Expand Up @@ -272,102 +273,261 @@ func TestActionsService_GetArtifact_notFound(t *testing.T) {

func TestActionsService_DownloadArtifact(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)

mux.HandleFunc("/repos/o/r/actions/artifacts/1/zip", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
http.Redirect(w, r, "https://github.com/artifact", http.StatusFound)
})

ctx := context.Background()
url, resp, err := client.Actions.DownloadArtifact(ctx, "o", "r", 1, 1)
if err != nil {
t.Errorf("Actions.DownloadArtifact returned error: %v", err)
}
if resp.StatusCode != http.StatusFound {
t.Errorf("Actions.DownloadArtifact returned status: %d, want %d", resp.StatusCode, http.StatusFound)
tcs := []struct {
name string
respectRateLimits bool
}{
{
name: "withoutRateLimits",
respectRateLimits: false,
},
{
name: "withRateLimits",
respectRateLimits: true,
},
}

want := "https://github.com/artifact"
if url.String() != want {
t.Errorf("Actions.DownloadArtifact returned %+v, want %+v", url.String(), want)
}
for _, tc := range tcs {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)
client.RateLimitRedirectionalEndpoints = tc.respectRateLimits

mux.HandleFunc("/repos/o/r/actions/artifacts/1/zip", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
http.Redirect(w, r, "https://github.com/artifact", http.StatusFound)
})

ctx := context.Background()
url, resp, err := client.Actions.DownloadArtifact(ctx, "o", "r", 1, 1)
if err != nil {
t.Errorf("Actions.DownloadArtifact returned error: %v", err)
}
if resp.StatusCode != http.StatusFound {
t.Errorf("Actions.DownloadArtifact returned status: %d, want %d", resp.StatusCode, http.StatusFound)
}

const methodName = "DownloadArtifact"
testBadOptions(t, methodName, func() (err error) {
_, _, err = client.Actions.DownloadArtifact(ctx, "\n", "\n", -1, 1)
return err
})
want := "https://github.com/artifact"
if url.String() != want {
t.Errorf("Actions.DownloadArtifact returned %+v, want %+v", url.String(), want)
}

// Add custom round tripper
client.client.Transport = roundTripperFunc(func(r *http.Request) (*http.Response, error) {
return nil, errors.New("failed to download artifact")
})
testBadOptions(t, methodName, func() (err error) {
_, _, err = client.Actions.DownloadArtifact(ctx, "o", "r", 1, 1)
return err
})
const methodName = "DownloadArtifact"
testBadOptions(t, methodName, func() (err error) {
_, _, err = client.Actions.DownloadArtifact(ctx, "\n", "\n", -1, 1)
return err
})

// Add custom round tripper
client.client.Transport = roundTripperFunc(func(r *http.Request) (*http.Response, error) {
return nil, errors.New("failed to download artifact")
})
// propagate custom round tripper to client without CheckRedirect
client.initialize()
testBadOptions(t, methodName, func() (err error) {
_, _, err = client.Actions.DownloadArtifact(ctx, "o", "r", 1, 1)
return err
})
})
}
}

func TestActionsService_DownloadArtifact_invalidOwner(t *testing.T) {
t.Parallel()
client, _, _ := setup(t)
tcs := []struct {
name string
respectRateLimits bool
}{
{
name: "withoutRateLimits",
respectRateLimits: false,
},
{
name: "withRateLimits",
respectRateLimits: true,
},
}

ctx := context.Background()
_, _, err := client.Actions.DownloadArtifact(ctx, "%", "r", 1, 1)
testURLParseError(t, err)
for _, tc := range tcs {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
client, _, _ := setup(t)
client.RateLimitRedirectionalEndpoints = tc.respectRateLimits

ctx := context.Background()
_, _, err := client.Actions.DownloadArtifact(ctx, "%", "r", 1, 1)
testURLParseError(t, err)
})
}
}

func TestActionsService_DownloadArtifact_invalidRepo(t *testing.T) {
t.Parallel()
client, _, _ := setup(t)
tcs := []struct {
name string
respectRateLimits bool
}{
{
name: "withoutRateLimits",
respectRateLimits: false,
},
{
name: "withRateLimits",
respectRateLimits: true,
},
}

ctx := context.Background()
_, _, err := client.Actions.DownloadArtifact(ctx, "o", "%", 1, 1)
testURLParseError(t, err)
for _, tc := range tcs {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
client, _, _ := setup(t)
client.RateLimitRedirectionalEndpoints = tc.respectRateLimits

ctx := context.Background()
_, _, err := client.Actions.DownloadArtifact(ctx, "o", "%", 1, 1)
testURLParseError(t, err)
})
}
}

func TestActionsService_DownloadArtifact_StatusMovedPermanently_dontFollowRedirects(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)

mux.HandleFunc("/repos/o/r/actions/artifacts/1/zip", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
http.Redirect(w, r, "https://github.com/artifact", http.StatusMovedPermanently)
})
tcs := []struct {
name string
respectRateLimits bool
}{
{
name: "withoutRateLimits",
respectRateLimits: false,
},
{
name: "withRateLimits",
respectRateLimits: true,
},
}

ctx := context.Background()
_, resp, _ := client.Actions.DownloadArtifact(ctx, "o", "r", 1, 0)
if resp.StatusCode != http.StatusMovedPermanently {
t.Errorf("Actions.DownloadArtifact return status %d, want %d", resp.StatusCode, http.StatusMovedPermanently)
for _, tc := range tcs {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)
client.RateLimitRedirectionalEndpoints = tc.respectRateLimits

mux.HandleFunc("/repos/o/r/actions/artifacts/1/zip", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
http.Redirect(w, r, "https://github.com/artifact", http.StatusMovedPermanently)
})

ctx := context.Background()
_, resp, _ := client.Actions.DownloadArtifact(ctx, "o", "r", 1, 0)
if resp.StatusCode != http.StatusMovedPermanently {
t.Errorf("Actions.DownloadArtifact return status %d, want %d", resp.StatusCode, http.StatusMovedPermanently)
}
})
}
}

func TestActionsService_DownloadArtifact_StatusMovedPermanently_followRedirects(t *testing.T) {
t.Parallel()
client, mux, serverURL := setup(t)

mux.HandleFunc("/repos/o/r/actions/artifacts/1/zip", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
redirectURL, _ := url.Parse(serverURL + baseURLPath + "/redirect")
http.Redirect(w, r, redirectURL.String(), http.StatusMovedPermanently)
})
mux.HandleFunc("/redirect", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
http.Redirect(w, r, "http://github.com/artifact", http.StatusFound)
})
tcs := []struct {
name string
respectRateLimits bool
}{
{
name: "withoutRateLimits",
respectRateLimits: false,
},
{
name: "withRateLimits",
respectRateLimits: true,
},
}

ctx := context.Background()
url, resp, err := client.Actions.DownloadArtifact(ctx, "o", "r", 1, 1)
if err != nil {
t.Errorf("Actions.DownloadArtifact return error: %v", err)
for _, tc := range tcs {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
client, mux, serverURL := setup(t)
client.RateLimitRedirectionalEndpoints = tc.respectRateLimits

mux.HandleFunc("/repos/o/r/actions/artifacts/1/zip", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
redirectURL, _ := url.Parse(serverURL + baseURLPath + "/redirect")
http.Redirect(w, r, redirectURL.String(), http.StatusMovedPermanently)
})
mux.HandleFunc("/redirect", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
http.Redirect(w, r, "http://github.com/artifact", http.StatusFound)
})

ctx := context.Background()
url, resp, err := client.Actions.DownloadArtifact(ctx, "o", "r", 1, 1)
if err != nil {
t.Errorf("Actions.DownloadArtifact return error: %v", err)
}
if resp.StatusCode != http.StatusFound {
t.Errorf("Actions.DownloadArtifact return status %d, want %d", resp.StatusCode, http.StatusFound)
}
want := "http://github.com/artifact"
if url.String() != want {
t.Errorf("Actions.DownloadArtifact returned %+v, want %+v", url.String(), want)
}
})
}
if resp.StatusCode != http.StatusFound {
t.Errorf("Actions.DownloadArtifact return status %d, want %d", resp.StatusCode, http.StatusFound)
}

func TestActionsService_DownloadArtifact_unexpectedCode(t *testing.T) {
t.Parallel()
tcs := []struct {
name string
respectRateLimits bool
}{
{
name: "withoutRateLimits",
respectRateLimits: false,
},
{
name: "withRateLimits",
respectRateLimits: true,
},
}
want := "http://github.com/artifact"
if url.String() != want {
t.Errorf("Actions.DownloadArtifact returned %+v, want %+v", url.String(), want)

for _, tc := range tcs {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
client, mux, serverURL := setup(t)
client.RateLimitRedirectionalEndpoints = tc.respectRateLimits

mux.HandleFunc("/repos/o/r/actions/artifacts/1/zip", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
redirectURL, _ := url.Parse(serverURL + baseURLPath + "/redirect")
http.Redirect(w, r, redirectURL.String(), http.StatusMovedPermanently)
})
mux.HandleFunc("/redirect", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
w.WriteHeader(http.StatusNoContent)
})

ctx := context.Background()
url, resp, err := client.Actions.DownloadArtifact(ctx, "o", "r", 1, 1)
if err == nil {
t.Fatalf("Actions.DownloadArtifact should return error on unexpected code")
}
if !strings.Contains(err.Error(), "unexpected status code") {
t.Error("Actions.DownloadArtifact should return unexpected status code")
}
if got, want := resp.Response.StatusCode, http.StatusNoContent; got != want {
t.Errorf("Actions.DownloadArtifact return status %d, want %d", got, want)
}
if url != nil {
t.Errorf("Actions.DownloadArtifact return %+v, want nil", url)
}
})
}
}

Expand Down
Loading
Loading