diff --git a/lib/userlist.rb b/lib/userlist.rb index 2e81e71..d7e1086 100644 --- a/lib/userlist.rb +++ b/lib/userlist.rb @@ -9,13 +9,9 @@ module Userlist class Error < StandardError; end - class ArgumentError < Error; end - class ServerError < Error; end - class TooManyRequestsError < Error; end - class TimeoutError < Error; end class ConfigurationError < Error @@ -24,7 +20,7 @@ class ConfigurationError < Error def initialize(key) @key = key.to_sym - super <<~MESSAGE + super(<<~MESSAGE) Missing required configuration value for `#{key}` Please set a value for `#{key}` using an environment variable: diff --git a/lib/userlist/push/client.rb b/lib/userlist/push/client.rb index 8d392e7..53688be 100644 --- a/lib/userlist/push/client.rb +++ b/lib/userlist/push/client.rb @@ -8,7 +8,14 @@ class Push class Client include Userlist::Logging - def initialize(config = {}) + HTTP_STATUS = { + ok: (200..299), + server_error: (500..599), + timeout: 408, + rate_limit: 429 + }.freeze + + def initialize(config = {}) @config = Userlist.config.merge(config) raise Userlist::ConfigurationError, :push_key unless @config.push_key @@ -33,7 +40,7 @@ def delete(endpoint, payload = nil) private - attr_reader :config, :status, :last_error + attr_reader :config, :status def http @http ||= begin @@ -60,7 +67,7 @@ def request(method, path, payload = nil) log_response(response) end - handle_response response + handle_response(response) end def build_request(method, path, payload) @@ -73,68 +80,79 @@ def build_request(method, path, payload) end def handle_response(response) - status = response.code.to_i - - return response if status.between?(200, 299) - - case status - when 500..599 then raise Userlist::ServerError, "Server error: #{status}" - when 408 then raise Userlist::TimeoutError, 'Request timed out' - when 429 then raise Userlist::TooManyRequestsError, 'Rate limited' - else raise Userlist::Error, "HTTP #{status}: #{response.message}" - end + @status = response.code.to_i + return response if ok? + + raise_error_for_status(status) end - def retry?(error) - error.is_a?(Userlist::ServerError) || - error.is_a?(Userlist::TooManyRequestsError) || - error.is_a?(Userlist::TimeoutError) + def raise_error_for_status(status) + error_class = error_class_for_status(status) + message = error_message_for_status(status) + raise error_class, message end - def log_request(request) - logger.debug "Sending #{request.method} to #{URI.join(endpoint, request.path)} with body #{request.body}" + def error_class_for_status(status) + error_mapping[status_type(status)] || Userlist::ServerError end - def log_response(response) - logger.debug "Received #{response.code} #{response.message} with body #{response.body}" - response + def error_message_for_status(status) + message = error_messages[status_type(status)] || 'HTTP error' + "#{message}: #{status}" end - def endpoint - @endpoint ||= URI(config.push_endpoint) + def status_type(status) + HTTP_STATUS.find { |type, range| range === status }&.first end - def token - config.push_key + def error_mapping + { + server_error: Userlist::ServerError, + timeout: Userlist::TimeoutError, + rate_limit: Userlist::TooManyRequestsError + } + end + + def error_messages + { + server_error: 'Server error', + timeout: 'Request timeout', + rate_limit: 'Rate limit exceeded' + } + end + + def ok? + HTTP_STATUS[:ok].include?(status) + end + + def error? + [:server_error, :rate_limit, :timeout].include?(status_type(status)) end def retryable @retryable ||= Userlist::Retryable.new do |response| @status = response.code.to_i - + error? end end - def ok? - status.between?(200, 299) + def log_request(request) + logger.debug "Sending #{request.method} to #{URI.join(endpoint, request.path)} with body #{request.body}" end - def error? - server_error? || rate_limited? || timeout? - end - - def server_error? - status.between?(500, 599) + def log_response(response) + logger.debug "Received #{response.code} #{response.message} with body #{response.body}" + response end - - def rate_limited? - status == 429 + + def endpoint + @endpoint ||= URI(config.push_endpoint) end - def timeout? - status == 408 + def token + config.push_key end end end -end \ No newline at end of file +end diff --git a/lib/userlist/push/strategies/active_job/worker.rb b/lib/userlist/push/strategies/active_job/worker.rb index dce1bda..c1594fb 100644 --- a/lib/userlist/push/strategies/active_job/worker.rb +++ b/lib/userlist/push/strategies/active_job/worker.rb @@ -4,6 +4,11 @@ module Userlist class Push module Strategies class ActiveJob + # Worker processes requests through ActiveJob. + # It forwards requests to the Userlist::Push::Client which handles: + # - HTTP communication + # - Retries for failed requests (500s, timeouts, rate limits) + # - Error handling for HTTP-related issues class Worker < ::ActiveJob::Base def perform(method, *args) client = Userlist::Push::Client.new diff --git a/lib/userlist/push/strategies/sidekiq/worker.rb b/lib/userlist/push/strategies/sidekiq/worker.rb index 25e85ad..1a3e6a3 100644 --- a/lib/userlist/push/strategies/sidekiq/worker.rb +++ b/lib/userlist/push/strategies/sidekiq/worker.rb @@ -4,6 +4,13 @@ module Userlist class Push module Strategies class Sidekiq + # Worker processes requests through Sidekiq. + # It forwards requests to the Userlist::Push::Client which handles: + # - HTTP communication + # - Retries for failed requests (500s, timeouts, rate limits) + # - Error handling for HTTP-related issues + # + # Note: Sidekiq retries (if configured) are separate from HTTP request retries class Worker include ::Sidekiq::Worker diff --git a/lib/userlist/push/strategies/threaded/worker.rb b/lib/userlist/push/strategies/threaded/worker.rb index f0dad23..f64158d 100644 --- a/lib/userlist/push/strategies/threaded/worker.rb +++ b/lib/userlist/push/strategies/threaded/worker.rb @@ -2,6 +2,16 @@ module Userlist class Push module Strategies class Threaded + # Worker processes requests from a queue in a separate thread. + # It forwards requests to the Userlist::Push::Client which handles: + # - HTTP communication + # - Retries for failed requests (500s, timeouts, rate limits) + # - Error handling for HTTP-related issues + # + # The worker itself only handles fatal errors to: + # - Prevent the worker thread from crashing + # - Keep processing the queue + # - Log errors for debugging class Worker include Userlist::Logging @@ -22,7 +32,7 @@ def run method, *args = *queue.pop break if method == :stop - retryable.attempt { client.public_send(method, *args) } + client.public_send(method, *args) rescue StandardError => e logger.error "Failed to deliver payload: [#{e.class.name}] #{e.message}" end @@ -44,14 +54,6 @@ def stop def client @client ||= Userlist::Push::Client.new(config) end - - def retryable - @retryable ||= Userlist::Retryable.new do |response| - status = response.code.to_i - - status >= 500 || status == 429 - end - end end end end diff --git a/spec/userlist/push/client_spec.rb b/spec/userlist/push/client_spec.rb index bcec026..351d4b7 100644 --- a/spec/userlist/push/client_spec.rb +++ b/spec/userlist/push/client_spec.rb @@ -91,7 +91,7 @@ ) expect_any_instance_of(Userlist::Retryable).to receive(:sleep).exactly(3).times - + response = subject.post('/events', { foo: :bar }) expect(response.code).to eq('200') end @@ -101,9 +101,9 @@ .to_return(status: 500).times(11) # Initial attempt + 10 retries expect_any_instance_of(Userlist::Retryable).to receive(:sleep).exactly(10).times - + expect { subject.post('/events', { foo: :bar }) } - .to raise_error(Userlist::ServerError, "Server error: 500") + .to raise_error(Userlist::ServerError, 'Server error: 500') end end diff --git a/spec/userlist/push/strategies/active_job/worker_spec.rb b/spec/userlist/push/strategies/active_job/worker_spec.rb index e3db970..ca1fcd8 100644 --- a/spec/userlist/push/strategies/active_job/worker_spec.rb +++ b/spec/userlist/push/strategies/active_job/worker_spec.rb @@ -29,11 +29,4 @@ described_class.perform_now('post', '/user', payload) described_class.perform_now('delete', '/user/identifier') end - - # it 'should retry the job on failure' do - # allow(client).to receive(:post) - - # expect { described_class.perform_now('post', '/events', payload) } - # .to change(described_class.queue_adapter.enqueued_jobs, :size).by(1) - # end end diff --git a/spec/userlist/push/strategies/threaded/worker_spec.rb b/spec/userlist/push/strategies/threaded/worker_spec.rb index 08a9728..0ac0fcf 100644 --- a/spec/userlist/push/strategies/threaded/worker_spec.rb +++ b/spec/userlist/push/strategies/threaded/worker_spec.rb @@ -42,15 +42,6 @@ queue.push([:delete, '/user/identifier']) end - it 'should retry failed responses' do - payload = { foo: :bar } - - expect(client).to receive(:post) { double(code: '500') }.exactly(11).times - expect_any_instance_of(Userlist::Retryable).to receive(:sleep).exactly(10).times - - queue.push([:post, '/users', payload]) - end - it 'should log failed requests' do allow(client).to receive(:post).and_raise(StandardError) queue.push([:post, '/events', payload])