NEB-898 Report Xero's rate limits on every response, not only on refusal

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    xpm_ruby (0.5.0)
+    xpm_ruby (0.6.0)
       activesupport
       builder
       dry-types

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...

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"

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)
 

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

lib/xpm_ruby/version.rb

@@ -1,3 +1,3 @@
 module XpmRuby
-  VERSION = "0.5.0".freeze
+  VERSION = "0.6.0".freeze
 end

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|

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) { "<Response><Status>OK</Status></Response>" }
+
+      # 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
         "<Job><Name>Brochure Design</Name><Description>Detailed description of the job</Description><ClientID>24097642</ClientID><StartDate>20291023</StartDate><DueDate>20291028</DueDate></Job>"