From ee06817b7d7de4a63f6b31b9a33c62746c54e778 Mon Sep 17 00:00:00 2001 From: igor-belousov <139464423+igor-belousov@users.noreply.github.com> Date: Sun, 3 May 2026 08:55:10 +0300 Subject: [PATCH 1/2] #447: make off_quota? aware of GitHub Search API quota --- .rubocop.yml | 4 +- lib/fbe/middleware/rate_limit.rb | 100 +++++++++------ lib/fbe/octo.rb | 38 +++++- test/fbe/middleware/test_rate_limit.rb | 171 +++++++++++++++++++++++++ test/fbe/test_octo.rb | 104 +++++++++++++++ 5 files changed, 373 insertions(+), 44 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 7385e29..6948e56 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -34,6 +34,7 @@ Elegant/GoodMethodName: - delete_one - dump_headers - extract_remaining_count + - extract_search_remaining_count - faraday_value - github_graph - handle_rate_limit_request @@ -53,6 +54,8 @@ Elegant/GoodMethodName: - off_quota? - organization_memberships - organization_repositories + - parsed_body + - patched_body - print_trace! - publish_to - pull_request @@ -85,7 +88,6 @@ Elegant/GoodMethodName: - track_request - unmask_repos - update_organization_membership - - update_remaining_count - user_name_by_id - user_repository_invitations - with_disable_auto_paginate diff --git a/lib/fbe/middleware/rate_limit.rb b/lib/fbe/middleware/rate_limit.rb index f880948..700e56c 100644 --- a/lib/fbe/middleware/rate_limit.rb +++ b/lib/fbe/middleware/rate_limit.rb @@ -24,6 +24,8 @@ # Copyright:: Copyright (c) 2024-2026 Zerocracy # License:: MIT class Fbe::Middleware::RateLimit < Faraday::Middleware + # NOT thread-safe: assumes single-threaded use (judges run sequentially in judges-action). + # # Initializes the rate limit middleware. # # @param [Object] app The next middleware in the stack @@ -31,6 +33,7 @@ def initialize(app) super @cached = nil @remaining = nil + @searchleft = nil @counter = 0 end @@ -42,7 +45,7 @@ def call(env) if env.url.path == '/rate_limit' handle_rate_limit_request(env) else - track_request + track_request(env.url.path) @app.call(env) end end @@ -56,22 +59,24 @@ def call(env) def handle_rate_limit_request(env) if @cached.nil? || @counter >= 100 response = @app.call(env) - @cached = response.dup + @cached = response @remaining = extract_remaining_count(response) + @searchleft = extract_search_remaining_count(response) @counter = 0 response else - response = @cached.dup - update_remaining_count(response) - Faraday::Response.new(response_env(env, response)) + Faraday::Response.new(response_env(env, @cached)) end end # Tracks non-rate_limit requests and decrements counter. - def track_request - return if @remaining.nil? - @remaining -= 1 if @remaining.positive? + def track_request(path = nil) @counter += 1 + if path&.start_with?('/search/') + @searchleft -= 1 if @searchleft&.positive? + elsif @remaining + @remaining -= 1 if @remaining.positive? + end end # Extracts the remaining count from the response body. @@ -79,53 +84,70 @@ def track_request # @param [Faraday::Response] response The API response # @return [Integer] The remaining requests count def extract_remaining_count(response) - body = response.body - if body.is_a?(String) - begin - body = JSON.parse(body) - rescue JSON::ParserError - return 0 - end - end + body = parsed_body(response) return 0 unless body.is_a?(Hash) body.dig('rate', 'remaining') || 0 end - # Updates the remaining count in the response body. + # Extracts the search-resource remaining count from the response body. + # + # @param [Faraday::Response] response The API response + # @return [Integer, nil] Remaining search-API requests or nil if absent + def extract_search_remaining_count(response) + body = parsed_body(response) + return nil unless body.is_a?(Hash) + body.dig('resources', 'search', 'remaining') + end + + # Parses the response body as a Hash if it's a JSON string. # - # @param [Faraday::Response] response The cached response to update - def update_remaining_count(response) + # @param [Faraday::Response] response The API response + # @return [Hash, Object] Parsed body or original on parse failure + def parsed_body(response) body = response.body - stringed = body.is_a?(String) + return body unless body.is_a?(String) + JSON.parse(body) + rescue JSON::ParserError + nil + end + + # Builds a fresh body with the current remaining counts written in, + # without mutating the cached response. Uses a JSON round-trip for + # the deep copy so we only handle JSON-shaped data. + # + # @param [Object] original The cached response body (Hash or JSON String) + # @return [Object] A new body of the same type with remaining counts updated + def patched_body(original) + stringed = original.is_a?(String) if stringed begin - body = JSON.parse(body) + body = JSON.parse(original) rescue JSON::ParserError - return + return original end + elsif original.is_a?(Hash) + body = JSON.parse(original.to_json) + else + return original end - return unless body.is_a?(Hash) && body['rate'] - body['rate']['remaining'] = @remaining - return unless stringed - response.instance_variable_set(:@body, body.to_json) + body['rate']['remaining'] = @remaining if body['rate'] + body.dig('resources', 'search')&.[]=('remaining', @searchleft) if @searchleft + stringed ? body.to_json : body end - # Creates a response environment for the cached response. + # Builds a response environment that mirrors the cached response, + # preserving Faraday::Env invariants by dup-ing the original env + # and only overriding body and rate-limit headers. # # @param [Faraday::Env] env The original request environment # @param [Faraday::Response] response The cached response - # @return [Hash] Response environment hash + # @return [Faraday::Env] Response env ready to wrap in Faraday::Response def response_env(env, response) - headers = response.headers.dup - headers['x-ratelimit-remaining'] = @remaining.to_s if @remaining - { - method: env.method, - url: env.url, - request_headers: env.request_headers, - request_body: env.request_body, - status: response.status, - response_headers: headers, - body: response.body - } + served = response.env.dup + served.request_headers = env.request_headers + served.body = patched_body(response.body) + served.response_headers = response.headers.dup + served.response_headers['x-ratelimit-remaining'] = @remaining.to_s if @remaining + served end end diff --git a/lib/fbe/octo.rb b/lib/fbe/octo.rb index 1e789f7..2f666a6 100644 --- a/lib/fbe/octo.rb +++ b/lib/fbe/octo.rb @@ -28,6 +28,10 @@ # When we are off quota. class Fbe::OffQuota < StandardError; end +Fbe::SEARCH_METHODS = %i[ + search_issues search_commits search_repositories search_users search_code search_topics +].freeze + # Makes a call to the GitHub API. # # It is supposed to be used instead of +Octokit::Client+, because it @@ -154,13 +158,34 @@ def print_trace!(all: false, max: 5) @trace.clear end end - def off_quota?(threshold: 50) # rubocop:disable Layout/EmptyLineBetweenDefs + def off_quota?(threshold: nil, resource: :core) # rubocop:disable Layout/EmptyLineBetweenDefs, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + threshold ||= resource == :search ? 5 : 50 + label = resource == :search ? 'GitHub Search API' : 'GitHub API' left = @origin.rate_limit!.remaining + got = false + if resource == :search && @origin.respond_to?(:last_response) + body = @origin.last_response&.body + body = JSON.parse(body) if body.is_a?(String) + if body.is_a?(Hash) + fresh = body.dig('resources', 'search', 'remaining') || body.dig(:resources, :search, :remaining) + if fresh + left = Integer(fresh) + got = true + end + end + end + if resource == :search && !got + klass = @origin.respond_to?(:last_response) ? @origin.last_response&.body&.class : nil + @loog.warn( + "Search-quota check fell back to core remaining (#{left}); " \ + "search count unavailable (last_response body class: #{klass.inspect})" + ) + end if left < threshold - @loog.info("Too much GitHub API quota consumed already (#{left} < #{threshold})") + @loog.info("Too much #{label} quota consumed already (#{left} < #{threshold})") true else - @loog.debug("Still #{left} GitHub API quota left (>#{threshold})") + @loog.debug("Still #{left} #{label} quota left (>#{threshold})") false end end @@ -208,7 +233,12 @@ def with_disable_auto_paginate # rubocop:disable Layout/EmptyLineBetweenDefs end o = intercepted(o) do |e, m, _args, _r| - if e == :before && m != :off_quota? && m != :print_trace! && m != :rate_limit && o.off_quota? + next unless e == :before + next if %i[off_quota? print_trace! rate_limit].include?(m) + if Fbe::SEARCH_METHODS.include?(m) + raise(Fbe::OffQuota, "We are off-quota on the search resource, can't do #{m}()") if + o.off_quota?(resource: :search) + elsif o.off_quota? raise(Fbe::OffQuota, "We are off-quota (remaining: #{o.rate_limit.remaining}), can't do #{m}()") end end diff --git a/test/fbe/middleware/test_rate_limit.rb b/test/fbe/middleware/test_rate_limit.rb index 820efab..582ecb9 100644 --- a/test/fbe/middleware/test_rate_limit.rb +++ b/test/fbe/middleware/test_rate_limit.rb @@ -104,6 +104,177 @@ def test_handles_zero_remaining_count assert_equal(0, response.body['rate']['remaining']) end + def test_decrements_search_remaining_for_search_requests + payload = { + 'rate' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 }, + 'resources' => { + 'core' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 }, + 'search' => { 'limit' => 30, 'remaining' => 30, 'reset' => 1_672_531_200 } + } + } + stub_request(:get, 'https://api.github.com/rate_limit') + .to_return(status: 200, body: payload.to_json, headers: { 'Content-Type' => 'application/json' }) + stub_request(:get, 'https://api.github.com/search/issues?q=foo') + .to_return(status: 200, body: '{"items":[]}', headers: { 'Content-Type' => 'application/json' }) + conn = create_connection + conn.get('/rate_limit') + conn.get('/search/issues?q=foo') + response = conn.get('/rate_limit') + assert_equal(29, response.body['resources']['search']['remaining']) + assert_equal(4999, response.body['rate']['remaining']) + end + + def test_search_request_does_not_decrement_core_remaining + payload = { + 'rate' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 }, + 'resources' => { + 'core' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 }, + 'search' => { 'limit' => 30, 'remaining' => 30, 'reset' => 1_672_531_200 } + } + } + stub_request(:get, 'https://api.github.com/rate_limit') + .to_return(status: 200, body: payload.to_json, headers: { 'Content-Type' => 'application/json' }) + stub_request(:get, 'https://api.github.com/search/code?q=bar') + .to_return(status: 200, body: '{"items":[]}', headers: { 'Content-Type' => 'application/json' }) + conn = create_connection + conn.get('/rate_limit') + conn.get('/search/code?q=bar') + response = conn.get('/rate_limit') + assert_equal(4999, response.body['rate']['remaining']) + end + + def test_non_search_request_does_not_decrement_search_remaining + payload = { + 'rate' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 }, + 'resources' => { + 'core' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 }, + 'search' => { 'limit' => 30, 'remaining' => 30, 'reset' => 1_672_531_200 } + } + } + stub_request(:get, 'https://api.github.com/rate_limit') + .to_return(status: 200, body: payload.to_json, headers: { 'Content-Type' => 'application/json' }) + stub_request(:get, 'https://api.github.com/user') + .to_return(status: 200, body: '{"login":"x"}', headers: { 'Content-Type' => 'application/json' }) + conn = create_connection + conn.get('/rate_limit') + conn.get('/user') + response = conn.get('/rate_limit') + assert_equal(30, response.body['resources']['search']['remaining']) + assert_equal(4998, response.body['rate']['remaining']) + end + + def test_search_remaining_survives_repeated_search_calls_within_cache_window + payload = { + 'rate' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 }, + 'resources' => { + 'core' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 }, + 'search' => { 'limit' => 30, 'remaining' => 30, 'reset' => 1_672_531_200 } + } + } + stub_request(:get, 'https://api.github.com/rate_limit') + .to_return(status: 200, body: payload.to_json, headers: { 'Content-Type' => 'application/json' }) + .times(1) + stub_request(:get, 'https://api.github.com/search/issues?q=z') + .to_return(status: 200, body: '{"items":[]}', headers: { 'Content-Type' => 'application/json' }) + .times(25) + conn = create_connection + conn.get('/rate_limit') + 25.times { conn.get('/search/issues?q=z') } + response = conn.get('/rate_limit') + assert_equal(5, response.body['resources']['search']['remaining']) + assert_requested(:get, 'https://api.github.com/rate_limit', times: 1) + end + + def test_search_remaining_clamps_at_zero + payload = { + 'rate' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 }, + 'resources' => { + 'core' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 }, + 'search' => { 'limit' => 30, 'remaining' => 1, 'reset' => 1_672_531_200 } + } + } + stub_request(:get, 'https://api.github.com/rate_limit') + .to_return(status: 200, body: payload.to_json, headers: { 'Content-Type' => 'application/json' }) + stub_request(:get, 'https://api.github.com/search/issues?q=z') + .to_return(status: 200, body: '{"items":[]}', headers: { 'Content-Type' => 'application/json' }) + .times(3) + conn = create_connection + conn.get('/rate_limit') + 3.times { conn.get('/search/issues?q=z') } + response = conn.get('/rate_limit') + assert_equal(0, response.body['resources']['search']['remaining']) + end + + def test_search_remaining_absent_when_payload_lacks_resources + payload = { 'rate' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 } } + stub_request(:get, 'https://api.github.com/rate_limit') + .to_return(status: 200, body: payload.to_json, headers: { 'Content-Type' => 'application/json' }) + stub_request(:get, 'https://api.github.com/search/issues?q=z') + .to_return(status: 200, body: '{"items":[]}', headers: { 'Content-Type' => 'application/json' }) + conn = create_connection + conn.get('/rate_limit') + conn.get('/search/issues?q=z') + response = conn.get('/rate_limit') + refute(response.body.key?('resources'), 'must not invent a resources key when upstream did not return one') + assert_equal(4999, response.body['rate']['remaining']) + end + + def test_decrement_applies_when_body_is_a_json_string + payload = { + 'rate' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 }, + 'resources' => { + 'core' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 }, + 'search' => { 'limit' => 30, 'remaining' => 30, 'reset' => 1_672_531_200 } + } + } + stub_request(:get, 'https://api.github.com/rate_limit') + .to_return(status: 200, body: payload.to_json, headers: { 'Content-Type' => 'application/json' }) + stub_request(:get, 'https://api.github.com/search/issues?q=z') + .to_return(status: 200, body: '{"items":[]}', headers: { 'Content-Type' => 'application/json' }) + conn = + Faraday.new(url: 'https://api.github.com') do |f| + f.use(Fbe::Middleware::RateLimit) + f.adapter(:net_http) + end + conn.get('/rate_limit') + conn.get('/search/issues?q=z') + response = conn.get('/rate_limit') + assert_kind_of(String, response.body, 'body must remain a String when no JSON parser middleware is installed') + parsed = JSON.parse(response.body) + assert_equal(29, parsed['resources']['search']['remaining']) + assert_equal(4999, parsed['rate']['remaining']) + end + + def test_cached_body_is_not_leaked_to_callers + payload = { + 'rate' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 }, + 'resources' => { + 'core' => { 'limit' => 5000, 'remaining' => 4999, 'reset' => 1_672_531_200 }, + 'search' => { 'limit' => 30, 'remaining' => 30, 'reset' => 1_672_531_200 } + } + } + stub_request(:get, 'https://api.github.com/rate_limit') + .to_return(status: 200, body: payload.to_json, headers: { 'Content-Type' => 'application/json' }) + .times(1) + stub_request(:get, 'https://api.github.com/user') + .to_return(status: 200, body: '{"login":"x"}', headers: { 'Content-Type' => 'application/json' }) + .times(2) + conn = create_connection + conn.get('/rate_limit') + first = conn.get('/rate_limit') + first.body['rate']['remaining'] = 0 + first.body['resources']['search']['remaining'] = 0 + conn.get('/user') + conn.get('/user') + second = conn.get('/rate_limit') + refute_equal( + 0, second.body['rate']['remaining'], + 'mutating the body of an earlier cached response must not corrupt subsequent reads' + ) + assert_equal(4997, second.body['rate']['remaining']) + assert_equal(30, second.body['resources']['search']['remaining']) + end + private def create_connection diff --git a/test/fbe/test_octo.rb b/test/fbe/test_octo.rb index e7f0b3a..d4a50fa 100644 --- a/test/fbe/test_octo.rb +++ b/test/fbe/test_octo.rb @@ -184,6 +184,110 @@ def test_off_quota_twice assert_predicate(o, :off_quota?) end + def test_off_quota_search_when_search_resource_exhausted + WebMock.disable_net_connect! + stub_request(:get, 'https://api.github.com/rate_limit').to_return( + body: { rate: { remaining: 4_999 }, resources: { core: { remaining: 4_999 }, search: { remaining: 1 } } }.to_json, + headers: { 'Content-Type' => 'application/json', 'X-RateLimit-Remaining' => '4999' } + ) + o = Fbe.octo(loog: Loog::NULL, global: {}, options: Judges::Options.new) + refute_predicate(o, :off_quota?, 'core quota is healthy, off_quota? must stay false') + assert( + o.off_quota?(resource: :search), + 'search quota is below threshold, off_quota?(resource: :search) must be true' + ) + end + + def test_search_issues_blocked_when_search_quota_exhausted + WebMock.disable_net_connect! + stub_request(:get, 'https://api.github.com/rate_limit').to_return( + body: { rate: { remaining: 4_999 }, resources: { core: { remaining: 4_999 }, search: { remaining: 1 } } }.to_json, + headers: { 'Content-Type' => 'application/json', 'X-RateLimit-Remaining' => '4999' } + ) + o = Fbe.octo(loog: Loog::NULL, global: {}, options: Judges::Options.new) + assert_raises(Fbe::OffQuota) { o.search_issues('repo:foo/bar type:issue') } + end + + def test_search_issues_allowed_when_search_quota_ok_but_core_low + WebMock.disable_net_connect! + stub_request(:get, 'https://api.github.com/rate_limit').to_return( + body: { rate: { remaining: 1 }, resources: { core: { remaining: 1 }, search: { remaining: 30 } } }.to_json, + headers: { 'Content-Type' => 'application/json', 'X-RateLimit-Remaining' => '1' } + ) + stub_request(:get, %r{https://api.github.com/search/issues}).to_return( + body: { total_count: 0, incomplete_results: false, items: [] }.to_json, + headers: { 'Content-Type' => 'application/json' } + ) + o = Fbe.octo(loog: Loog::NULL, global: {}, options: Judges::Options.new) + assert_raises(Fbe::OffQuota) { o.user(42) } + result = o.search_issues('repo:foo/bar type:issue') + assert_equal(0, result[:total_count]) + end + + def test_off_quota_search_falls_back_to_core_when_no_search_resource + WebMock.disable_net_connect! + stub_request(:get, 'https://api.github.com/rate_limit').to_return( + body: { rate: { remaining: 4_999 } }.to_json, + headers: { 'Content-Type' => 'application/json', 'X-RateLimit-Remaining' => '4999' } + ) + o = Fbe.octo(loog: Loog::NULL, global: {}, options: Judges::Options.new) + refute( + o.off_quota?(resource: :search), + 'when payload lacks resources.search, off_quota? must fall back to the core remaining count' + ) + end + + def test_off_quota_search_uses_default_threshold_of_five + WebMock.disable_net_connect! + stub_request(:get, 'https://api.github.com/rate_limit').to_return( + body: { rate: { remaining: 4_999 }, resources: { core: { remaining: 4_999 }, search: { remaining: 5 } } }.to_json, + headers: { 'Content-Type' => 'application/json', 'X-RateLimit-Remaining' => '4999' } + ) + o = Fbe.octo(loog: Loog::NULL, global: {}, options: Judges::Options.new) + refute(o.off_quota?(resource: :search), 'remaining=5 must NOT trip the default search threshold of 5 (strict <)') + assert( + o.off_quota?(resource: :search, threshold: 6), + 'remaining=5 with explicit threshold=6 must trip off_quota?' + ) + end + + def test_off_quota_core_uses_default_threshold_of_fifty + WebMock.disable_net_connect! + stub_request(:get, 'https://api.github.com/rate_limit').to_return( + body: { rate: { remaining: 49 } }.to_json, + headers: { 'Content-Type' => 'application/json', 'X-RateLimit-Remaining' => '49' } + ) + o = Fbe.octo(loog: Loog::NULL, global: {}, options: Judges::Options.new) + assert_predicate(o, :off_quota?, 'remaining=49 must trip the default core threshold of 50') + end + + def test_off_quota_search_when_last_response_is_nil + WebMock.disable_net_connect! + stub_request(:get, 'https://api.github.com/rate_limit').to_return( + body: { rate: { remaining: 4_999 } }.to_json, + headers: { 'Content-Type' => 'application/json', 'X-RateLimit-Remaining' => '4999' } + ) + o = Fbe.octo(loog: Loog::NULL, global: {}, options: Judges::Options.new) + refute( + o.off_quota?(resource: :search), + 'with no prior request made, last_response is nil and we must fall back to core quota' + ) + end + + def test_search_methods_are_routed_through_search_quota_check + WebMock.disable_net_connect! + stub_request(:get, 'https://api.github.com/rate_limit').to_return( + body: { rate: { remaining: 4_999 }, resources: { core: { remaining: 4_999 }, search: { remaining: 1 } } }.to_json, + headers: { 'Content-Type' => 'application/json', 'X-RateLimit-Remaining' => '4999' } + ) + Fbe::SEARCH_METHODS.each do |m| + o = Fbe.octo(loog: Loog::NULL, global: {}, options: Judges::Options.new) + assert_raises(Fbe::OffQuota, "search method #{m} must be blocked when search quota is exhausted") do + o.__send__(m, 'q') + end + end + end + def test_print_quota_left_while_initialize WebMock.disable_net_connect! stub_request(:get, 'https://api.github.com/rate_limit').to_return( From 82fe71f4761be8fffdf7a9f0f86ca822541157f1 Mon Sep 17 00:00:00 2001 From: igor-belousov <139464423+igor-belousov@users.noreply.github.com> Date: Sun, 3 May 2026 19:18:25 +0300 Subject: [PATCH 2/2] #447: drop parsed_body helper, let JSON::ParserError propagate --- .rubocop.yml | 1 - lib/fbe/middleware/rate_limit.rb | 33 ++++++++------------------ test/fbe/middleware/test_rate_limit.rb | 9 ------- 3 files changed, 10 insertions(+), 33 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 6948e56..1b1cc43 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -54,7 +54,6 @@ Elegant/GoodMethodName: - off_quota? - organization_memberships - organization_repositories - - parsed_body - patched_body - print_trace! - publish_to diff --git a/lib/fbe/middleware/rate_limit.rb b/lib/fbe/middleware/rate_limit.rb index 700e56c..e58ce93 100644 --- a/lib/fbe/middleware/rate_limit.rb +++ b/lib/fbe/middleware/rate_limit.rb @@ -84,7 +84,8 @@ def track_request(path = nil) # @param [Faraday::Response] response The API response # @return [Integer] The remaining requests count def extract_remaining_count(response) - body = parsed_body(response) + body = response.body + body = JSON.parse(body) if body.is_a?(String) return 0 unless body.is_a?(Hash) body.dig('rate', 'remaining') || 0 end @@ -94,23 +95,12 @@ def extract_remaining_count(response) # @param [Faraday::Response] response The API response # @return [Integer, nil] Remaining search-API requests or nil if absent def extract_search_remaining_count(response) - body = parsed_body(response) + body = response.body + body = JSON.parse(body) if body.is_a?(String) return nil unless body.is_a?(Hash) body.dig('resources', 'search', 'remaining') end - # Parses the response body as a Hash if it's a JSON string. - # - # @param [Faraday::Response] response The API response - # @return [Hash, Object] Parsed body or original on parse failure - def parsed_body(response) - body = response.body - return body unless body.is_a?(String) - JSON.parse(body) - rescue JSON::ParserError - nil - end - # Builds a fresh body with the current remaining counts written in, # without mutating the cached response. Uses a JSON round-trip for # the deep copy so we only handle JSON-shaped data. @@ -119,17 +109,14 @@ def parsed_body(response) # @return [Object] A new body of the same type with remaining counts updated def patched_body(original) stringed = original.is_a?(String) - if stringed - begin - body = JSON.parse(original) - rescue JSON::ParserError + body = + if stringed + JSON.parse(original) + elsif original.is_a?(Hash) + JSON.parse(original.to_json) + else return original end - elsif original.is_a?(Hash) - body = JSON.parse(original.to_json) - else - return original - end body['rate']['remaining'] = @remaining if body['rate'] body.dig('resources', 'search')&.[]=('remaining', @searchleft) if @searchleft stringed ? body.to_json : body diff --git a/test/fbe/middleware/test_rate_limit.rb b/test/fbe/middleware/test_rate_limit.rb index 582ecb9..bc7881e 100644 --- a/test/fbe/middleware/test_rate_limit.rb +++ b/test/fbe/middleware/test_rate_limit.rb @@ -80,15 +80,6 @@ def test_handles_response_without_rate_data assert_empty(response.body) end - def test_ignores_non_hash_response_body - stub_request(:get, 'https://api.github.com/rate_limit') - .to_return(status: 200, body: 'invalid json', headers: { 'Content-Type' => 'text/plain' }) - conn = create_connection - response = conn.get('/rate_limit') - assert_equal(200, response.status) - assert_equal('invalid json', response.body) - end - def test_handles_zero_remaining_count payload = { 'rate' => { 'limit' => 5000, 'remaining' => 1, 'reset' => 1_672_531_200 } } stub_request(:get, 'https://api.github.com/rate_limit')