diff --git a/Gemfile.lock b/Gemfile.lock index d0ba2c7..336e78e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - xpm_ruby (0.5.0) + xpm_ruby (0.6.0) activesupport builder dry-types diff --git a/README.md b/README.md index 2756183..baa88c4 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,48 @@ For example, a call to `XPMRuby::Staff.list(access_token: access_token, xero_ten As much as possible, we have tried to keep to the same names as documented here: https://developer.xero.com/documentation/practice-manager/overview-practice-manager-api however we have not as yet added the full API (only the endpoints we are currently using in ignitionapp). +## Rate limits + +Xero reports the remaining rate-limit budget on **every** XPM response, not only on the `429` that +refuses one. Set `XpmRuby.on_rate_limits` to be told about each reading: + +```ruby +XpmRuby.on_rate_limits = ->(limits) do + MyApp.record( + tenant: limits.xero_tenant_id, + day_left: limits.daylimit_remaining, + minute_left: limits.minlimit_remaining, + problem: limits.problem + ) +end +``` + +The callback is handed an `XpmRuby::RateLimits`. Which of its fields are populated depends on the +response: + +| field | on a success | on a `429` | +| -- | -- | -- | +| `minlimit_remaining`, `daylimit_remaining`, `appminlimit_remaining` | yes | yes | +| `problem`, `retry_after` | no | yes | + +Xero names a delay and a cause only when it actually refuses a request, so treat `problem` and +`retry_after` as absent on a successful call rather than as "no problem". + +Counts are integers, and `nil` when the response did not report one — a missing header is *not* read +as zero, because zero means the budget is exhausted and is the reading a caller most needs to act +on. Responses that report nothing at all do not invoke the callback. + +Read from a `429` alone the remaining counts can only say the budget is already gone. Read from +every response they let a caller stop short of the limit instead of discovering it. + +Set the callback once, at boot. Every entry point in this gem builds its own `Connection` and never +hands it back, so the callback is what reaches them all. It must not raise: if it does, the error is +warned and the request it was measuring still succeeds. + +`XpmRuby::RateLimitExceeded#details` still carries the raw wire-named header hash from the `429` +that raised it, unchanged for the lowercase headers Xero sends. It is now read case-insensitively, +so a change of casing at Xero's end can no longer empty it. + ## Development TODO set up this gem to release automatically when merged into master... diff --git a/lib/xpm_ruby.rb b/lib/xpm_ruby.rb index c6555b0..b30f2a5 100644 --- a/lib/xpm_ruby.rb +++ b/lib/xpm_ruby.rb @@ -29,6 +29,31 @@ def initialize(message, details:) super(message) end end + + class << self + # Called with a XpmRuby::RateLimits for every response that reports one, refused or not. Set it + # once at boot: + # + # XpmRuby.on_rate_limits = ->(limits) { MyApp.record(limits) } + # + # Every entry point in this gem builds its own Connection and never hands it back, so a reader + # on the connection would be unreachable from a caller that only calls `Client.get`. A callback + # reaches all of them — including the 429s that raise from inside a `rescue` somewhere and would + # otherwise take an exception-carried payload with them. + attr_accessor :on_rate_limits + + def notify_rate_limits(limits) + callback = on_rate_limits + return if callback.nil? + + callback.call(limits) + rescue StandardError => error + # A hook that only measures a budget must not be able to fail the request it was measuring: + # a caller whose store is down would otherwise take every XPM call down with it. Warned + # rather than swallowed, because silent is indistinguishable from a callback nobody wired up. + warn("XpmRuby.on_rate_limits raised #{error.class}: #{error.message}") + end + end end require "active_support" @@ -40,6 +65,7 @@ def initialize(message, details:) require "xpm_ruby/contact" require "xpm_ruby/connection" require "xpm_ruby/job" +require "xpm_ruby/rate_limits" require "xpm_ruby/schema/client/add" require "xpm_ruby/schema/client/update" diff --git a/lib/xpm_ruby/connection.rb b/lib/xpm_ruby/connection.rb index 6af8700..bf5d502 100644 --- a/lib/xpm_ruby/connection.rb +++ b/lib/xpm_ruby/connection.rb @@ -69,6 +69,13 @@ def xpm_url end def handle_response(response) + limits = RateLimits.from_response(response, xero_tenant_id: xero_tenant_id) + + # Reported ahead of the case, so a budget is read off the responses that raise as well as the + # ones that return. Skipped when the response names no limit at all: that is not the same as + # a budget of zero, and a caller should not have to tell the two apart from an object of nils. + XpmRuby.notify_rate_limits(limits) unless limits.empty? + case response.status when 401 detail = error_detail(response) @@ -89,14 +96,7 @@ def handle_response(response) when 503 raise NotAvailable.new(error_detail(response)) when 429 # rate limit exceeded - details = response.headers.slice( - "retry-after", - "x-rate-limit-problem", - "x-minlimit-remaining", - "x-daylimit-remaining", - "x-appminlimit-remaining" - ) - raise RateLimitExceeded.new(response.reason_phrase, details: details) + raise RateLimitExceeded.new(response.reason_phrase, details: limits.headers) when 200 xml = Ox.load(response.body, mode: :hash_no_attrs, symbolize_keys: false) diff --git a/lib/xpm_ruby/rate_limits.rb b/lib/xpm_ruby/rate_limits.rb new file mode 100644 index 0000000..c48b075 --- /dev/null +++ b/lib/xpm_ruby/rate_limits.rb @@ -0,0 +1,103 @@ +module XpmRuby + # Xero returns its rate-limit headers on every XPM response, not only on the 429 that refuses one. + # Read from the 429 alone they can only ever say the budget is already gone, which is too late to + # pace against: the caller learns it is locked out at the moment it is locked out. Read from every + # response they let a caller watch a budget drain while its requests are still succeeding, and + # stop short of the limit instead of discovering it. + class RateLimits + RETRY_AFTER_HEADER = "retry-after".freeze + PROBLEM_HEADER = "x-rate-limit-problem".freeze + MINLIMIT_REMAINING_HEADER = "x-minlimit-remaining".freeze + DAYLIMIT_REMAINING_HEADER = "x-daylimit-remaining".freeze + APPMINLIMIT_REMAINING_HEADER = "x-appminlimit-remaining".freeze + + # Order matters only in that `RateLimitExceeded#details` has always carried this slice under + # these wire names, and callers read it by those names. + HEADERS = [ + RETRY_AFTER_HEADER, + PROBLEM_HEADER, + MINLIMIT_REMAINING_HEADER, + DAYLIMIT_REMAINING_HEADER, + APPMINLIMIT_REMAINING_HEADER + ].freeze + + REPORTED_FIELDS = [ + :problem, + :retry_after, + :minlimit_remaining, + :daylimit_remaining, + :appminlimit_remaining + ].freeze + + attr_reader :status, + :xero_tenant_id, + :headers, + :problem, + :retry_after, + :minlimit_remaining, + :daylimit_remaining, + :appminlimit_remaining + + def self.from_response(response, xero_tenant_id: nil) + headers = response.headers || {} + + new( + status: response.status, + xero_tenant_id: xero_tenant_id, + headers: reported_headers(headers), + problem: headers[PROBLEM_HEADER].presence, + retry_after: integer(headers[RETRY_AFTER_HEADER]), + minlimit_remaining: integer(headers[MINLIMIT_REMAINING_HEADER]), + daylimit_remaining: integer(headers[DAYLIMIT_REMAINING_HEADER]), + appminlimit_remaining: integer(headers[APPMINLIMIT_REMAINING_HEADER]) + ) + end + + # The wire-named hash `RateLimitExceeded#details` has always carried. Built through `[]` rather + # than `slice`, because on a Faraday::Utils::Headers only `[]` is case-insensitive: `slice` + # compares keys exactly, so a `Retry-After` would leave `details` empty and a caller reading it + # would fall back silently, as though Xero had never named a delay at all. Xero sends these + # lowercase — HTTP/2 requires it — so for today's traffic this is the same hash as the slice it + # replaces, down to the key order. + def self.reported_headers(headers) + HEADERS.each_with_object({}) do |name, reported| + value = headers[name] + + reported[name] = value unless value.nil? + end + end + private_class_method(:reported_headers) + + # `to_i` would read a missing or unparseable header as `0`. For a remaining-budget count that is + # not "unknown" but "exhausted", so a caller gating on it would refuse every request off a + # response that never mentioned a budget at all. Absent stays absent. + def self.integer(value) + Integer(value, exception: false) + end + private_class_method(:integer) + + def initialize(status:, xero_tenant_id:, headers:, problem:, retry_after:, minlimit_remaining:, daylimit_remaining:, appminlimit_remaining:) + @status = status + @xero_tenant_id = xero_tenant_id + @headers = headers + @problem = problem + @retry_after = retry_after + @minlimit_remaining = minlimit_remaining + @daylimit_remaining = daylimit_remaining + @appminlimit_remaining = appminlimit_remaining + end + + # A response that names no limit at all. Distinct from a response reporting a budget of zero, + # which is the most important reading there is. + def empty? + REPORTED_FIELDS.all? { |field| public_send(field).nil? } + end + + def to_h + REPORTED_FIELDS + .each_with_object({ status: status, xero_tenant_id: xero_tenant_id }) do |field, fields| + fields[field] = public_send(field) + end + end + end +end diff --git a/lib/xpm_ruby/version.rb b/lib/xpm_ruby/version.rb index e0f8998..e7937b8 100644 --- a/lib/xpm_ruby/version.rb +++ b/lib/xpm_ruby/version.rb @@ -1,3 +1,3 @@ module XpmRuby - VERSION = "0.5.0".freeze + VERSION = "0.6.0".freeze end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index a9d2e98..d3fb77d 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -12,6 +12,12 @@ config.expect_with(:rspec) do |c| c.syntax = :expect end + + # `XpmRuby.on_rate_limits` is module-level state, so one left set would leak into every example + # that ran after it. + config.after(:each) do + XpmRuby.on_rate_limits = nil + end end VCR.configure do |config| diff --git a/spec/xpm_ruby/connection_spec.rb b/spec/xpm_ruby/connection_spec.rb index aa09b1d..bf84a8e 100644 --- a/spec/xpm_ruby/connection_spec.rb +++ b/spec/xpm_ruby/connection_spec.rb @@ -33,7 +33,8 @@ module XpmRuby response = instance_double( Faraday::Response, status: 403, - body: { Detail: "InsufficientPermissions" }.to_json + body: { Detail: "InsufficientPermissions" }.to_json, + headers: {} ) allow_any_instance_of(Faraday::Connection).to receive(:get).and_return(response) end @@ -113,6 +114,126 @@ module XpmRuby end end + describe "rate limit reporting" do + let(:reported) { [] } + + let(:xml_body) { "OK" } + + # What a 200 actually carries, per the recorded traffic in + # spec/vcr_cassettes/xpm_ruby/connection/delete.yml: the three remaining counts and nothing + # else. `retry-after` and `x-rate-limit-problem` arrive only on a 429 — Xero names a delay + # and a cause only when it refuses. + let(:rate_limit_headers) do + { + "x-minlimit-remaining" => "59", + "x-daylimit-remaining" => "4984", + "x-appminlimit-remaining" => "9999" + } + end + + before(:each) do + XpmRuby.on_rate_limits = ->(limits) { reported << limits } + end + + def stub_response(status:, body:, headers:) + response = instance_double(Faraday::Response, status: status, body: body, headers: headers) + allow_any_instance_of(Faraday::Connection).to receive(:get).and_return(response) + end + + # The reason this gem change exists. A 429 can only ever say the budget is already gone; a + # caller pacing itself under the limit needs the count while its requests still work. + context "on a successful response" do + before(:each) do + stub_response(status: 200, body: xml_body, headers: rate_limit_headers) + end + + it "reports the budget Xero returned" do + connection = Connection.new(access_token: access_token, xero_tenant_id: xero_tenant_id) + connection.get(endpoint: "staff.api/list") + + expect(reported.size).to eq(1) + expect(reported.first.to_h).to eq( + status: 200, + xero_tenant_id: xero_tenant_id, + problem: nil, + retry_after: nil, + minlimit_remaining: 59, + daylimit_remaining: 4984, + appminlimit_remaining: 9999 + ) + end + + it "still returns the parsed response to the caller" do + connection = Connection.new(access_token: access_token, xero_tenant_id: xero_tenant_id) + + expect(connection.get(endpoint: "staff.api/list")["Status"]).to eq("OK") + end + end + + context "when the response names no limit" do + before(:each) do + stub_response(status: 200, body: xml_body, headers: { "content-type" => "text/xml" }) + end + + # Reporting an object of nils would make every caller guard against a reading that says + # nothing, and would look identical to a budget that had run out. + it "reports nothing" do + connection = Connection.new(access_token: access_token, xero_tenant_id: xero_tenant_id) + connection.get(endpoint: "staff.api/list") + + expect(reported).to be_empty + end + end + + context "on a refused response" do + it "reports the refusal and still raises with the details it always carried" do + VCR.use_cassette("xpm_ruby/connection/get/rate_limit_exceeded") do + connection = Connection.new(access_token: access_token, xero_tenant_id: xero_tenant_id) + + expect { connection.get(endpoint: "staff.api/list") }.to raise_error(XpmRuby::RateLimitExceeded) + end + + expect(reported.size).to eq(1) + expect(reported.first).to have_attributes( + status: 429, + problem: "minute", + retry_after: 22, + daylimit_remaining: 4539 + ) + end + end + + context "when the callback raises" do + before(:each) do + stub_response(status: 200, body: xml_body, headers: rate_limit_headers) + XpmRuby.on_rate_limits = ->(_limits) { raise("redis is down") } + end + + # A hook that only measures a budget must not be able to fail the request it measured. + it "does not fail the request" do + connection = Connection.new(access_token: access_token, xero_tenant_id: xero_tenant_id) + + expect { connection.get(endpoint: "staff.api/list") } + .to output(/XpmRuby.on_rate_limits raised RuntimeError: redis is down/).to_stderr + + expect(connection.get(endpoint: "staff.api/list")["Status"]).to eq("OK") + end + end + + context "with no callback set" do + before(:each) do + stub_response(status: 200, body: xml_body, headers: rate_limit_headers) + XpmRuby.on_rate_limits = nil + end + + it "returns the response as before" do + connection = Connection.new(access_token: access_token, xero_tenant_id: xero_tenant_id) + + expect(connection.get(endpoint: "staff.api/list")["Status"]).to eq("OK") + end + end + end + describe "#post" do let(:xml_string) do "Brochure DesignDetailed description of the job240976422029102320291028" diff --git a/spec/xpm_ruby/rate_limits_spec.rb b/spec/xpm_ruby/rate_limits_spec.rb new file mode 100644 index 0000000..f185280 --- /dev/null +++ b/spec/xpm_ruby/rate_limits_spec.rb @@ -0,0 +1,120 @@ +require "spec_helper" + +module XpmRuby + RSpec.describe(RateLimits) do + def build(headers, status: 200, xero_tenant_id: "XERO_TENANT_ID") + response = instance_double(Faraday::Response, status: status, headers: headers) + + RateLimits.from_response(response, xero_tenant_id: xero_tenant_id) + end + + describe ".from_response" do + it "parses the reported budgets as integers" do + limits = build({ + "retry-after" => "22", + "x-rate-limit-problem" => "minute", + "x-minlimit-remaining" => "0", + "x-daylimit-remaining" => "4539", + "x-appminlimit-remaining" => "9938" + }) + + expect(limits.retry_after).to eq(22) + expect(limits.problem).to eq("minute") + expect(limits.minlimit_remaining).to eq(0) + expect(limits.daylimit_remaining).to eq(4539) + expect(limits.appminlimit_remaining).to eq(9938) + end + + it "carries the status and tenant the reading came from" do + limits = build({ "x-daylimit-remaining" => "10" }, status: 429, xero_tenant_id: "TENANT") + + expect(limits.status).to eq(429) + expect(limits.xero_tenant_id).to eq("TENANT") + end + + it "keeps the raw wire-named slice for RateLimitExceeded#details" do + limits = build({ + "retry-after" => "22", + "x-daylimit-remaining" => "0", + "content-type" => "text/xml" + }) + + expect(limits.headers).to eq("retry-after" => "22", "x-daylimit-remaining" => "0") + end + + # Faraday stores headers under the casing the server sent, and only `[]` on its Headers is + # case-insensitive — `slice` is not. Xero sends these lowercase, so this is what the old + # `slice` produced; the point of the spec is that a change of casing at Xero's end cannot + # quietly empty the hash that `RateLimitExceeded#details` hands to callers. + it "reads the reported headers whatever casing they arrived under" do + %w[retry-after Retry-After RETRY-AFTER].each do |name| + headers = Faraday::Utils::Headers.new + headers[name] = "22" + + limits = build(headers) + + expect(limits.headers).to eq("retry-after" => "22") + expect(limits.retry_after).to eq(22) + end + end + + # A remaining count of zero is the single most important reading there is: it is the one a + # caller has to stop on. Absent has to stay distinguishable from it, which rules out `to_i`. + it "reads an exhausted budget as zero and a missing one as nil" do + exhausted = build({ "x-daylimit-remaining" => "0" }) + missing = build({}) + blank = build({ "x-daylimit-remaining" => "" }) + + expect(exhausted.daylimit_remaining).to eq(0) + expect(missing.daylimit_remaining).to be_nil + expect(blank.daylimit_remaining).to be_nil + end + + it "reads an unparseable budget as nil rather than zero" do + limits = build({ "x-daylimit-remaining" => "unlimited" }) + + expect(limits.daylimit_remaining).to be_nil + end + + it "reads a blank problem as nil" do + expect(build({ "x-rate-limit-problem" => "" }).problem).to be_nil + end + + it "tolerates a response with no headers at all" do + response = instance_double(Faraday::Response, status: 200, headers: nil) + + expect(RateLimits.from_response(response).headers).to eq({}) + end + end + + describe "#empty?" do + it "is true when the response named no limit" do + expect(build({ "content-type" => "text/xml" })).to be_empty + end + + it "is false when a budget is reported as exhausted" do + expect(build({ "x-daylimit-remaining" => "0" })).not_to be_empty + end + + it "is false when only the problem is named" do + expect(build({ "x-rate-limit-problem" => "concurrent" })).not_to be_empty + end + end + + describe "#to_h" do + it "reports the parsed fields alongside the reading's context" do + limits = build({ "x-daylimit-remaining" => "0", "x-rate-limit-problem" => "day" }, status: 429) + + expect(limits.to_h).to eq( + status: 429, + xero_tenant_id: "XERO_TENANT_ID", + problem: "day", + retry_after: nil, + minlimit_remaining: nil, + daylimit_remaining: 0, + appminlimit_remaining: nil + ) + end + end + end +end