From a286efdd88a303534109b26f0b1b82453d9df183 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Tue, 5 Jun 2018 15:25:19 +0200 Subject: [PATCH] WhoisSyncJob : better error handling --- app/jobs/whois_sync_job.rb | 31 ++++++++++++++++++------ app/services/check_logger.rb | 2 +- app/services/whois/errors.rb | 10 ++++---- app/services/whois/parser/base.rb | 2 +- test/jobs/whois_sync_job_test.rb | 38 ++++++++++++++++++++++++++---- test/services/check_logger_test.rb | 21 ++++++++++++++++- 6 files changed, 85 insertions(+), 19 deletions(-) diff --git a/app/jobs/whois_sync_job.rb b/app/jobs/whois_sync_job.rb index 32cf69c..c0e1d17 100644 --- a/app/jobs/whois_sync_job.rb +++ b/app/jobs/whois_sync_job.rb @@ -1,25 +1,28 @@ class WhoisSyncJob < ApplicationJob queue_as :default + rescue_from StandardError do |exception| + check_logger.log(:standard_error, exception) if check.present? + raise # rubocop:disable Style/SignalException + end + rescue_from ActiveRecord::RecordNotFound do; end + # parser error are already logged + rescue_from Whois::Error do; end + attr_reader :check def perform(check_id) - @check = Check.find(check_id) - check.update_attribute(:last_run_at, Time.now) + prepare_check(check_id) - response = Whois.ask(check.domain) + response = Whois.ask(check.domain, logger: check_logger) return unless response.valid? update_from_response(response) - - check.save! rescue Whois::DomainNotFoundError check.active = false check.save! - rescue Whois::ParserError # rubocop:disable Lint/HandleExceptions - # already logged end def update_from_response(response) @@ -27,5 +30,19 @@ class WhoisSyncJob < ApplicationJob check.domain_updated_at = response.updated_at check.domain_expires_at = response.expire_at check.last_success_at = Time.now + + check.save! + end + + private + + def prepare_check(check_id) + @check = Check.find(check_id) + check.update_attribute(:last_run_at, Time.now) + end + + # logger is a reserved ActiveJob method + def check_logger + @check_logger ||= CheckLogger.new(check) end end diff --git a/app/services/check_logger.rb b/app/services/check_logger.rb index 829165d..bd0124e 100644 --- a/app/services/check_logger.rb +++ b/app/services/check_logger.rb @@ -11,7 +11,7 @@ class CheckLogger log_command_result(message) when :parsed_response log_parsed_response(message) - when :parser_error, :service_error + when :parser_error, :service_error, :standard_error log_error(message) end end diff --git a/app/services/whois/errors.rb b/app/services/whois/errors.rb index 219ed84..5813977 100644 --- a/app/services/whois/errors.rb +++ b/app/services/whois/errors.rb @@ -1,10 +1,10 @@ module Whois - class WhoisError < StandardError; end + class Error < StandardError; end - class WhoisCommandError < WhoisError; end - class UnsupportedDomainError < WhoisError; end - class DomainNotFoundError < WhoisError; end - class ParserError < WhoisError; end + class WhoisCommandError < Error; end + class UnsupportedDomainError < Error; end + class DomainNotFoundError < Error; end + class ParserError < Error; end class FieldNotFoundError < ParserError; end class MissingDateFormatError < ParserError; end diff --git a/app/services/whois/parser/base.rb b/app/services/whois/parser/base.rb index 64e132d..5d00f82 100644 --- a/app/services/whois/parser/base.rb +++ b/app/services/whois/parser/base.rb @@ -29,7 +29,7 @@ module Whois logger.log :parsed_response, response response - rescue StandardError => ex + rescue ParserError => ex logger.log :parser_error, ex raise end diff --git a/test/jobs/whois_sync_job_test.rb b/test/jobs/whois_sync_job_test.rb index a20089e..e96755d 100644 --- a/test/jobs/whois_sync_job_test.rb +++ b/test/jobs/whois_sync_job_test.rb @@ -6,7 +6,7 @@ class WhoisSyncJobTest < ActiveJob::TestCase check = create(:check, :nil_dates, domain: domain) mock_system_command("whois", domain, stdout: whois_response(domain)) do - WhoisSyncJob.new.perform(check.id) + WhoisSyncJob.perform_now(check.id) end check.reload @@ -24,7 +24,7 @@ class WhoisSyncJobTest < ActiveJob::TestCase original_updated_at = check.updated_at mock_system_command("whois", "domain.fr", stdout: "not a response") do - WhoisSyncJob.new.perform(check.id) + WhoisSyncJob.perform_now(check.id) end check.reload @@ -35,12 +35,32 @@ class WhoisSyncJobTest < ActiveJob::TestCase assert check.active? end - test "Disable check when whois responds domain not found" do + test "should ignore not found (removed) checks" do + assert_nothing_raised do + WhoisSyncJob.perform_now("9999999") + end + end + + test "should log and re-raise StandardError" do + check = create(:check) + + assert_raise StandardError do + Whois.stub :ask, nil do + WhoisSyncJob.perform_now(check.id) + end + end + + assert_equal 1, check.logs.count + assert_match(/undefined method \W+valid\?/, check.logs.last.error) + assert check.logs.last.failed? + end + + test "disable check when whois responds domain not found" do domain = "willneverexist.fr" check = create(:check, :nil_dates, domain: domain) mock_system_command("whois", domain, stdout: whois_response(domain)) do - WhoisSyncJob.new.perform(check.id) + WhoisSyncJob.perform_now(check.id) end check.reload @@ -50,6 +70,16 @@ class WhoisSyncJobTest < ActiveJob::TestCase assert_nil check.last_success_at end + test "default logger is CheckLogger" do + check = create(:check) + + mock_system_command("whois", check.domain) do + WhoisSyncJob.perform_now(check.id) + end + + assert_equal 1, check.logs.count + end + private def whois_response(domain) diff --git a/test/services/check_logger_test.rb b/test/services/check_logger_test.rb index 8b3b0e1..cbd1044 100644 --- a/test/services/check_logger_test.rb +++ b/test/services/check_logger_test.rb @@ -39,7 +39,7 @@ class CheckLoggerTest < ActiveSupport::TestCase assert @logger.check_log.failed? end - test "should log a successful parsed command" do + test "should log a successful parsed response" do response = OpenStruct.new( domain: "example.fr", extracted: "some data", @@ -52,6 +52,17 @@ class CheckLoggerTest < ActiveSupport::TestCase assert @logger.check_log.succeed? end + test "should log as failed a empty/error parsed response" do + response = OpenStruct.new( + domain: "example.fr", + valid?: false, + ) + @logger.log :parsed_response, response + + assert_equal response.to_json, @logger.check_log.parsed_response + assert @logger.check_log.failed? + end + test "should log parser error with a backtrace" do @logger.log :parser_error, mock_exception @@ -60,6 +71,14 @@ class CheckLoggerTest < ActiveSupport::TestCase assert @logger.check_log.failed? end + test "should log standard error with a backtrace" do + @logger.log :standard_error, mock_exception + + assert_includes @logger.check_log.error, "my error occured" + assert_includes @logger.check_log.error, "minitest.rb" + assert @logger.check_log.failed? + end + test "should log service error" do @logger.log :service_error, mock_exception