0
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2026-08-10 23:59:31 +08:00
discourse/plugins/discourse-github/spec/lib/commits_populator_spec.rb
Régis Hanol 36a8a51ef0
FEATURE: Route all GitHub API requests through one rate-limited client (#40637)
GitHub oneboxes and the discourse-github plugin talked to GitHub's REST
and
GraphQL API with no rate-limit awareness. On busy instances this
exhausted
GitHub's limits (60 requests/hour unauthenticated, 5000 authenticated),
and
because there was no backoff every render kept hitting GitHub and
re-failing
-- which GitHub's docs warn can get an integration banned. The recently
added PR-status onebox multiplied the number of calls and made it far
worse.

GitHub access was also fragmented: the core onebox engines used OpenURI,
the
discourse-github plugin used Octokit, and the discourse-ai bot tools
used
FinalDestination::HTTP -- three HTTP stacks, three tokens, and
inconsistent
(or entirely missing) error and rate-limit handling.

This introduces a single client, Discourse::GithubApi, that every GitHub
data-API request now flows through. It is built on Faraday with the
SSRF-safe
FinalDestination adapter and:

- authenticates per token (Bearer) and returns plain string-keyed Hashes
(get/post) or raw bodies (raw_get) -- one response shape, no
Octokit/Sawyer
- only ever sends the access token to api.github.com and
  raw.githubusercontent.com, rejecting any other absolute URL, so a
  user-derived path can never leak a token to an arbitrary host
- backs off on rate limits both reactively (403/429) and proactively
(when
X-RateLimit-Remaining hits 0), honouring Retry-After /
X-RateLimit-Reset,
via a shared Redis flag (GithubRateLimit) keyed per token so each
token's
  budget and the shared unauthenticated/IP budget back off independently
- short-circuits while backing off without ever sleeping, so onebox
rendering
  and post baking degrade to a plain link instead of blocking a request
- caches ETags and sends If-None-Match, so unchanged resources return
304s
  that do not count against the rate limit

Every caller was moved onto it:

- the 6 core GitHub onebox engines, via a slimmed
Onebox::Mixins::GithubApi
adapter that keeps their public methods and translates client errors
back
to the OpenURI::HTTPError vocabulary they already rescue (engines
unchanged)
- the github_blob raw.githubusercontent.com fetch
- the discourse-github plugin (badges, linkback, permalinks, token
validator),
which no longer uses the octokit and sawyer gems (they stay in the
Gemfile for
the discourse-code-review official plugin, which still depends on them)
- the discourse-ai bot's GitHub tools (search code, diff, file content,
  search files)

Also adds a GithubOneboxBackoff admin problem check that surfaces while
one of
the onebox token identities is backing off -- scoped to the tokens
resolved by
Onebox::GithubAccess (each configured github_onebox_access_tokens entry
plus the
unauthenticated client) so a backoff on the AI bot or linkback token is
not
misattributed to onebox. Its message points admins at the relevant
setting with
the {{setting:...}} link marker, which problem-check messages now expand
too.
Onebox token resolution is centralised in Onebox::GithubAccess, and the
onebox
cache TTL for transient GitHub failures is shortened so they recover
quickly.

GitHub OAuth login, theme git-clone, the inbound webhook, and the
Oneboxer
FinalDestination URL-resolution special-cases for github.com are
intentionally
out of scope -- they are different concerns, not the rate-limited data
API.
2026-06-15 10:59:10 +02:00

103 lines
3.3 KiB
Ruby
Vendored

# frozen_string_literal: true
describe DiscourseGithubPlugin::CommitsPopulator do
subject(:populator) { described_class.new(repo) }
let(:repo) { DiscourseGithubPlugin::GithubRepo.new(name: "discourse/discourse") }
let!(:site_admin1) { Fabricate(:admin) }
let!(:site_admin2) { Fabricate(:admin) }
let(:branches_url) { "https://api.github.com/repos/discourse/discourse/branches" }
before do
enable_current_plugin
SiteSetting.github_badges_enabled = true
end
def last_pm
Post
.joins(:topic)
.includes(:topic)
.where("topics.archetype = ?", Archetype.private_message)
.last
end
context "when invalid credentials have been provided (401)" do
before { stub_request(:get, branches_url).to_return(status: 401) }
it "disables github badges and sends a PM to the admin of the site to inform them" do
populator.populate!
expect(SiteSetting.github_badges_enabled).to eq(false)
expect(last_pm.topic.allowed_users).to include(site_admin1, site_admin2)
expect(last_pm.topic.title).to eq(
I18n.t("github_commits_populator.errors.invalid_octokit_credentials_pm_title"),
)
expect(last_pm.raw).to eq(
I18n.t(
"github_commits_populator.errors.invalid_octokit_credentials_pm",
base_path: Discourse.base_path,
).strip,
)
end
end
context "when the repository is not found (404)" do
before { stub_request(:get, branches_url).to_return(status: 404) }
it "disables github badges and sends a PM to the admin of the site to inform them" do
populator.populate!
expect(SiteSetting.github_badges_enabled).to eq(false)
expect(last_pm.topic.allowed_users).to include(site_admin1, site_admin2)
expect(last_pm.topic.title).to eq(
I18n.t("github_commits_populator.errors.repository_not_found_pm_title"),
)
expect(last_pm.raw).to eq(
I18n.t(
"github_commits_populator.errors.repository_not_found_pm",
repo_name: repo.name,
base_path: Discourse.base_path,
).strip,
)
end
end
context "if some other GitHub error is raised (500)" do
before { stub_request(:get, branches_url).to_return(status: 500) }
it "simply logs the error and does nothing else" do
populator.populate!
expect(SiteSetting.github_badges_enabled).to eq(true)
end
end
context "if GraphQL returns no data" do
before do
stub_request(:get, branches_url).to_return(
status: 200,
body: [{ "name" => "main", "commit" => { "sha" => "abc" } }].to_json,
headers: {
"Content-Type" => "application/json",
},
)
stub_request(:post, "https://api.github.com/graphql").to_return(
status: 200,
body: { "message" => "Bad credentials" }.to_json,
headers: {
"Content-Type" => "application/json",
},
)
end
it "raises a GraphQLError" do
expect { populator.populate! }.to raise_error(described_class::GraphQLError)
end
end
context "if github_badges_enabled is false" do
before { SiteSetting.github_badges_enabled = false }
it "early returns before making any GitHub request" do
populator.populate!
expect(a_request(:get, branches_url)).not_to have_been_made
end
end
end