From 3685770d072744285f923130951eda91e355c3dd Mon Sep 17 00:00:00 2001 From: Brooke Kuhlmann Date: Wed, 6 Aug 2025 10:42:34 -0600 Subject: [PATCH] Updated requester to log and fail SSL, connection, and timeout errors Necessary to log debug information and gracefully handle failures. Other errors might need to be tracked in the future but, for now, these are the most common. Reek has been updated to ignore the logger `#call` method because keeping exception logic in a single place is nice for quick lookup. The data clump smell is also being ignored because we have to repeat passing in custom headers (at least for now). RuboCop has also been updated to ignore the longer parameter list. This is an unfortunte side effect of the HTTP gem which uses multiple keyword arguments for making requests and so this design carries over into our implementation. Ignoring for now. Milestone: minor --- .config/rubocop/config.yml | 4 ++ .reek.yml | 6 +++ lib/trmnl/api/requester.rb | 28 ++++++++++--- spec/lib/trmnl/api/requester_spec.rb | 63 +++++++++++++++++++++++++++- 4 files changed, 94 insertions(+), 7 deletions(-) diff --git a/.config/rubocop/config.yml b/.config/rubocop/config.yml index bb2750e..39b33dd 100644 --- a/.config/rubocop/config.yml +++ b/.config/rubocop/config.yml @@ -1,2 +1,6 @@ inherit_gem: caliber: config/all.yml + +Metrics/ParameterLists: + Exclude: + - lib/trmnl/api/requester.rb diff --git a/.reek.yml b/.reek.yml index a2736e3..bc20f04 100644 --- a/.reek.yml +++ b/.reek.yml @@ -3,5 +3,11 @@ exclude_paths: - vendor detectors: + DataClump: + exclude: + - TRMNL::API::Requester LongParameterList: enabled: false + TooManyStatements: + exclude: + - TRMNL::API::Requester#call diff --git a/lib/trmnl/api/requester.rb b/lib/trmnl/api/requester.rb index 950a519..62f32ce 100644 --- a/lib/trmnl/api/requester.rb +++ b/lib/trmnl/api/requester.rb @@ -6,9 +6,8 @@ require "http" module TRMNL module API # Provides a low level configurable and monadic API client. - # :reek:DataClump class Requester - include Dependencies[:settings, :http] + include Dependencies[:settings, :http, :logger] include Dry::Monads[:result] HEADERS = {}.freeze @@ -26,13 +25,32 @@ module TRMNL attr_reader :settings, :http - # rubocop:todo Metrics/ParameterLists def call method, path, headers, **options http.headers(settings.headers.merge(headers)) - .public_send(method, "#{settings.uri}/#{path}", options) + .public_send(method, uri(path), options) .then { |response| response.status.success? ? Success(response) : Failure(response) } + rescue HTTP::ConnectionError => error then handle_bad_connection path, error + rescue HTTP::TimeoutError => error then handle_timeout path, error + rescue OpenSSL::SSL::SSLError => error then handle_bad_ssl path, error + end + + def uri(path) = "#{settings.uri}/#{path}" + + def handle_bad_connection path, error + logger.debug { error.message } + Failure "Unable to connect: #{uri(path).inspect}. Is the network intermittent or down?" + end + + def handle_timeout path, error + logger.debug { error.message } + Failure "Connection timed out: #{uri(path).inspect}." + end + + def handle_bad_ssl path, error + logger.debug { error.message } + Failure "Unable to secure connection: #{uri(path).inspect}. " \ + "Is your certificate or SSL valid?" end - # rubocop:enable Metrics/ParameterLists end end end diff --git a/spec/lib/trmnl/api/requester_spec.rb b/spec/lib/trmnl/api/requester_spec.rb index dc953ea..41dc97e 100644 --- a/spec/lib/trmnl/api/requester_spec.rb +++ b/spec/lib/trmnl/api/requester_spec.rb @@ -69,7 +69,7 @@ RSpec.describe TRMNL::API::Requester do end end - context "with failure" do + context "with HTTP error status" do let :http do HTTP::Fake::Client.new do get "/api/current_screen" do @@ -85,13 +85,72 @@ RSpec.describe TRMNL::API::Requester do end end - it "answers failure response" do + it "answers failure" do response = requester.get "current_screen" payload = response.alt_map { |result| result.parse.symbolize_keys! } expect(payload).to be_failure(message: "Danger!") end end + + context "with connection failure" do + let :http do + class_spy(HTTP).tap do |spy| + allow(spy).to receive(:headers).and_raise HTTP::ConnectionError, "Danger!" + end + end + + it "logs debug message" do + requester.get "current_screen" + expect(logger.reread).to match(/🔎.+Danger!/) + end + + it "answers failure" do + expect(requester.get("current_screen")).to be_failure( + %(Unable to connect: "https://trmnl.app/api/current_screen". ) \ + "Is the network intermittent or down?" + ) + end + end + + context "with timeout failure" do + let :http do + class_spy(HTTP).tap do |spy| + allow(spy).to receive(:headers).and_raise HTTP::TimeoutError, "Danger!" + end + end + + it "logs debug message" do + requester.get "current_screen" + expect(logger.reread).to match(/🔎.+Danger!/) + end + + it "answers failure" do + expect(requester.get("current_screen")).to be_failure( + %(Connection timed out: "https://trmnl.app/api/current_screen".) + ) + end + end + + context "with SSL failure" do + let :http do + class_spy(HTTP).tap do |spy| + allow(spy).to receive(:headers).and_raise OpenSSL::SSL::SSLError, "Danger!" + end + end + + it "logs debug message" do + requester.get "current_screen" + expect(logger.reread).to match(/🔎.+Danger!/) + end + + it "answers failure" do + expect(requester.get("current_screen")).to be_failure( + %(Unable to secure connection: "https://trmnl.app/api/current_screen". ) \ + "Is your certificate or SSL valid?" + ) + end + end end describe "#post" do