From 9dd728636f6216b9a9dd9f32001764ee3047cdfd Mon Sep 17 00:00:00 2001 From: Sebastian Turner Date: Sun, 18 Oct 2026 06:23:38 -0700 Subject: [PATCH] Use BaseURL relative path for rate limit category Signed-off-by: Sebastian Turner --- github/apps_manifest_test.go | 2 +- github/dependency_graph_snapshots_test.go | 2 +- github/dependency_graph_test.go | 4 +- github/enterprise_scim_test.go | 24 +++---- github/github.go | 16 ++++- github/github_test.go | 81 +++++++++++++++++++++++ github/migrations_source_import_test.go | 2 +- github/scim_test.go | 12 ++-- github/search_test.go | 14 ++-- 9 files changed, 126 insertions(+), 31 deletions(-) diff --git a/github/apps_manifest_test.go b/github/apps_manifest_test.go index 8f2bd77d9a1..207ccd1cf1b 100644 --- a/github/apps_manifest_test.go +++ b/github/apps_manifest_test.go @@ -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) diff --git a/github/dependency_graph_snapshots_test.go b/github/dependency_graph_snapshots_test.go index 0bc7cfbab29..31a3d1c4e1b 100644 --- a/github/dependency_graph_snapshots_test.go +++ b/github/dependency_graph_snapshots_test.go @@ -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) diff --git a/github/dependency_graph_test.go b/github/dependency_graph_test.go index 3c63cca86d3..02031d906b9 100644 --- a/github/dependency_graph_test.go +++ b/github/dependency_graph_test.go @@ -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) @@ -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) diff --git a/github/enterprise_scim_test.go b/github/enterprise_scim_test.go index e037662aba6..26a24102b9b 100644 --- a/github/enterprise_scim_test.go +++ b/github/enterprise_scim_test.go @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) @@ -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") }) } @@ -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") }) } diff --git a/github/github.go b/github/github.go index a13d0a12e4f..83d38e69bdf 100644 --- a/github/github.go +++ b/github/github.go @@ -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. @@ -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 diff --git a/github/github_test.go b/github/github_test.go index dc902f9ed1c..c927a46f0cd 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -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() @@ -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) + } + } +} diff --git a/github/migrations_source_import_test.go b/github/migrations_source_import_test.go index ff57ec40ac2..c8f6e1017b8 100644 --- a/github/migrations_source_import_test.go +++ b/github/migrations_source_import_test.go @@ -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) diff --git a/github/scim_test.go b/github/scim_test.go index 1aacfff16fd..4ba1bdc8516 100644 --- a/github/scim_test.go +++ b/github/scim_test.go @@ -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 }) @@ -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) @@ -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 }) @@ -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) @@ -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) @@ -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") }) } diff --git a/github/search_test.go b/github/search_test.go index 8c9c2571fe8..26fc5c296f6 100644 --- a/github/search_test.go +++ b/github/search_test.go @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) @@ -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)