Skip to content

fix: Use BaseURL relative path for rate limit category #4644

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

Open
auraseb wants to merge 1 commit into
base: master
Choose a base branch
from
+126 −31
Open

fix: Use BaseURL relative path for rate limit category #4644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion github/apps_manifest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func TestAppsService_CompleteAppManifest(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, IntegrationManifestCategory, func() (*Response, error) {
got, resp, err := client.Apps.CompleteAppManifest(ctx, "code")
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down
2 changes: 1 addition & 1 deletion github/dependency_graph_snapshots_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func TestDependencyGraphService_CreateSnapshot(t *testing.T) {
}

const methodName = "CreateSnapshot"
testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, DependencySnapshotsCategory, func() (*Response, error) {
got, resp, err := client.DependencyGraph.CreateSnapshot(ctx, "o", "r", snapshot)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down
4 changes: 2 additions & 2 deletions github/dependency_graph_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func TestDependencyGraphService_GetSBOM(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, DependencySBOMCategory, func() (*Response, error) {
got, resp, err := client.DependencyGraph.GetSBOM(ctx, "owner", "repo")
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -111,7 +111,7 @@ func TestDependencyGraphService_GenerateSBOM(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, DependencySBOMCategory, func() (*Response, error) {
got, resp, err := client.DependencyGraph.GenerateSBOM(ctx, "owner", "repo")
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down
24 changes: 12 additions & 12 deletions github/enterprise_scim_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ func TestEnterpriseService_ListProvisionedSCIMGroups(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
got, resp, err := client.Enterprise.ListProvisionedSCIMGroups(ctx, "ee", opts)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -206,7 +206,7 @@ func TestEnterpriseService_ListProvisionedSCIMUsers(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
got, resp, err := client.Enterprise.ListProvisionedSCIMUsers(ctx, "ee", opts)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -271,7 +271,7 @@ func TestEnterpriseService_SetProvisionedSCIMGroup(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
got, resp, err := client.Enterprise.SetProvisionedSCIMGroup(ctx, "ee", "abcd", input)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -357,7 +357,7 @@ func TestEnterpriseService_SetProvisionedSCIMUser(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
got, resp, err := client.Enterprise.SetProvisionedSCIMUser(ctx, "ee", "7fce", input)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -435,7 +435,7 @@ func TestEnterpriseService_UpdateSCIMGroupAttribute(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
got, resp, err := client.Enterprise.UpdateSCIMGroupAttribute(ctx, "ee", "abcd", input)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -541,7 +541,7 @@ func TestEnterpriseService_UpdateSCIMUserAttribute(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
got, resp, err := client.Enterprise.UpdateSCIMUserAttribute(ctx, "ee", "7fce", input)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -634,7 +634,7 @@ func TestEnterpriseService_ProvisionSCIMGroup(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
got, resp, err := client.Enterprise.ProvisionSCIMGroup(ctx, "ee", input)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -747,7 +747,7 @@ func TestEnterpriseService_ProvisionSCIMUser(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
got, resp, err := client.Enterprise.ProvisionSCIMUser(ctx, "ee", input)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -819,7 +819,7 @@ func TestEnterpriseService_GetProvisionedSCIMGroup(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
got, resp, err := client.Enterprise.GetProvisionedSCIMGroup(ctx, "ee", "914a", opts)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -903,7 +903,7 @@ func TestEnterpriseService_GetProvisionedSCIMUser(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
got, resp, err := client.Enterprise.GetProvisionedSCIMUser(ctx, "ee", "5fc0")
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -934,7 +934,7 @@ func TestEnterpriseService_DeleteSCIMGroup(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
return client.Enterprise.DeleteSCIMGroup(ctx, "ee", "abcd")
})
}
Expand All @@ -961,7 +961,7 @@ func TestEnterpriseService_DeleteSCIMUser(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
return client.Enterprise.DeleteSCIMUser(ctx, "ee", "7fce")
})
}
16 changes: 15 additions & 1 deletion github/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1441,7 +1441,7 @@ func (c *Client) bareDo(caller *http.Client, req *http.Request) (*Response, erro
rateLimitCategory := CoreCategory

if !c.disableRateLimitCheck {
rateLimitCategory = GetRateLimitCategory(req.Method, req.URL.Path)
rateLimitCategory = GetRateLimitCategory(req.Method, c.apiPath(req.URL))

if bypass := ctx.Value(BypassRateLimitCheck); bypass == nil {
// If we've hit rate limit, don't make further requests before Reset time.
Expand Down Expand Up @@ -1544,6 +1544,20 @@ func (c *Client) bareDo(caller *http.Client, req *http.Request) (*Response, erro
return response, err
}

// apiPath returns the path of u relative to the client's BaseURL, keeping the
// leading slash, so that "/api/v3/search/code" on a GitHub Enterprise Server
// client becomes "/search/code". Paths outside BaseURL are returned unchanged.
func (c *Client) apiPath(u *url.URL) string {
if c.baseURL == nil {
return u.Path
}
prefix := strings.TrimSuffix(c.baseURL.Path, "/")
if path, ok := strings.CutPrefix(u.Path, prefix); ok && strings.HasPrefix(path, "/") {
return path
}
return u.Path
}

// BareDo sends an API request and lets you handle the api response. If an error
// or API Error occurs, the error will contain more information. Otherwise, you
// are supposed to read and close the response's Body. If rate limit is exceeded
Expand Down
81 changes: 81 additions & 0 deletions github/github_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2751,6 +2751,54 @@ func TestDo_rateLimitCategory(t *testing.T) {
}
}

// Ensure the rate limit category is determined relative to BaseURL, so that a
// client with a path prefix (GitHub Enterprise Server's "/api/v3/") does not
// file search rate limits under the core category.
func TestDo_rateLimitCategory_baseURLPath(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)

reset := time.Now().UTC().Add(time.Minute).Round(time.Second)

mux.HandleFunc("/search/code", func(w http.ResponseWriter, _ *http.Request) {
w.Header().Set(HeaderRateLimit, "10")
w.Header().Set(HeaderRateRemaining, "0")
w.Header().Set(HeaderRateUsed, "10")
w.Header().Set(HeaderRateReset, fmt.Sprint(reset.Unix()))
w.Header().Set(HeaderRateResource, "code_search")
w.WriteHeader(http.StatusForbidden)
fmt.Fprintln(w, `{"message": "API rate limit exceeded"}`)
})

madeNetworkCall := false
mux.HandleFunc("/user", func(http.ResponseWriter, *http.Request) {
madeNetworkCall = true
})

req, _ := client.NewRequest(t.Context(), "GET", "search/code", nil)
if _, err := client.Do(req, nil); err == nil {
t.Fatal("Expected error to be returned.")
}

client.rateMu.Lock()
codeSearch, core := client.rateLimits[CodeSearchCategory], client.rateLimits[CoreCategory]
client.rateMu.Unlock()
if got, want := codeSearch.Remaining, 0; got != want || codeSearch.Limit != 10 {
t.Errorf("rateLimits[CodeSearchCategory] = %+v, want Limit 10 and Remaining %v", codeSearch, want)
}
if core != (Rate{}) {
t.Errorf("rateLimits[CoreCategory] = %+v, want zero value", core)
}

req, _ = client.NewRequest(t.Context(), "GET", "user", nil)
if _, err := client.Do(req, nil); err != nil {
t.Fatalf("Do returned unexpected error: %v", err)
}
if !madeNetworkCall {
t.Error("Core request was blocked by an exhausted code search rate limit.")
}
}

// Ensure rate limit is still parsed, even for error responses.
func TestDo_rateLimit_errorResponse(t *testing.T) {
t.Parallel()
Expand Down Expand Up @@ -5489,3 +5537,36 @@ func TestDeploymentProtectionRuleEvent_GetRunID(t *testing.T) {
t.Errorf("want %#v, got %#v", want, got)
}
}

func TestClient_apiPath(t *testing.T) {
t.Parallel()

u, err := url.Parse("https://ghes.example.com/api/v3/search/code")
if err != nil {
t.Fatal(err)
}
if got, want := (&Client{}).apiPath(u), "/api/v3/search/code"; got != want {
t.Errorf("apiPath with nil BaseURL = %q, want %q", got, want)
}

client, _, _ := setup(t)
base, err := url.Parse("https://ghes.example.com/api/v3/")
if err != nil {
t.Fatal(err)
}
client.baseURL = base
tests := []struct{ in, want string }{
{"https://ghes.example.com/api/v3/search/code", "/search/code"},
{"https://ghes.example.com/api/v3x/search/code", "/api/v3x/search/code"},
{"https://ghes.example.com/uploads/repos/o/r", "/uploads/repos/o/r"},
}
for _, tt := range tests {
u, err := url.Parse(tt.in)
if err != nil {
t.Fatal(err)
}
if got := client.apiPath(u); got != tt.want {
t.Errorf("apiPath(%q) = %q, want %q", tt.in, got, tt.want)
}
}
}
2 changes: 1 addition & 1 deletion github/migrations_source_import_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func TestMigrationService_StartImport(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, SourceImportCategory, func() (*Response, error) {
got, resp, err := client.Migrations.StartImport(ctx, "o", "r", input)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down
12 changes: 6 additions & 6 deletions github/scim_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func TestSCIMService_ListSCIMProvisionedIdentities(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
_, r, err := client.SCIM.ListSCIMProvisionedIdentities(ctx, "o", nil)
return r, err
})
Expand Down Expand Up @@ -166,7 +166,7 @@ func TestSCIMService_ProvisionAndInviteSCIMUser(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
got, resp, err := client.SCIM.ProvisionAndInviteSCIMUser(ctx, "o", opts)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -261,7 +261,7 @@ func TestSCIMService_GetSCIMProvisioningInfoForUser(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
_, r, err := client.SCIM.GetSCIMProvisioningInfoForUser(ctx, "o", "123")
return r, err
})
Expand Down Expand Up @@ -311,7 +311,7 @@ func TestSCIMService_UpdateProvisionedOrgMembership(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
got, resp, err := client.SCIM.UpdateProvisionedOrgMembership(ctx, "o", "123", body)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -376,7 +376,7 @@ func TestSCIMService_UpdateAttributeForSCIMUser(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
got, resp, err := client.SCIM.UpdateAttributeForSCIMUser(ctx, "o", "123", body)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -406,7 +406,7 @@ func TestSCIMService_DeleteSCIMUserFromOrg(t *testing.T) {
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, ScimCategory, func() (*Response, error) {
return client.SCIM.DeleteSCIMUserFromOrg(ctx, "o", "123")
})
}
14 changes: 7 additions & 7 deletions github/search_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func TestSearchService_Repositories(t *testing.T) {
t.Errorf("Search.Repositories returned %+v, want %+v", result, want)
}
const methodName = "Repositories"
testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, SearchCategory, func() (*Response, error) {
got, resp, err := client.Search.Repositories(ctx, "blah", opts)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -180,7 +180,7 @@ func TestSearchService_Topics(t *testing.T) {
t.Errorf("Search.Topics returned %+v, want %+v", result, want)
}
const methodName = "Topics"
testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, SearchCategory, func() (*Response, error) {
got, resp, err := client.Search.Topics(ctx, "blah", opts)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -233,7 +233,7 @@ func TestSearchService_Commits(t *testing.T) {
t.Errorf("Search.Commits returned %+v, want %+v", result, want)
}
const methodName = "Commits"
testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, SearchCategory, func() (*Response, error) {
got, resp, err := client.Search.Commits(ctx, "blah", opts)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -288,7 +288,7 @@ func TestSearchService_Issues(t *testing.T) {
t.Errorf("Search.Issues returned %+v, want %+v", result, want)
}
const methodName = "Issues"
testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, SearchCategory, func() (*Response, error) {
got, resp, err := client.Search.Issues(ctx, "blah", opts)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -491,7 +491,7 @@ func TestSearchService_Users(t *testing.T) {
t.Errorf("Search.Users returned %+v, want %+v", result, want)
}
const methodName = "Users"
testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, SearchCategory, func() (*Response, error) {
got, resp, err := client.Search.Users(ctx, "blah", opts)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -546,7 +546,7 @@ func TestSearchService_Code(t *testing.T) {
t.Errorf("Search.Code returned %+v, want %+v", result, want)
}
const methodName = "Code"
testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, CodeSearchCategory, func() (*Response, error) {
got, resp, err := client.Search.Code(ctx, "blah", opts)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down Expand Up @@ -668,7 +668,7 @@ func TestSearchService_Labels(t *testing.T) {
t.Errorf("Search.Labels returned %+v, want %+v", result, want)
}
const methodName = "Labels"
testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
testNewRequestAndDoFailureCategory(t, methodName, client, SearchCategory, func() (*Response, error) {
got, resp, err := client.Search.Labels(ctx, 1234, "blah", opts)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
Expand Down
Loading