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