From 6f59092607f8cc1355bf9172f9b7e16d0f5bcd20 Mon Sep 17 00:00:00 2001 From: Bob Lail Date: Mon, 5 Sep 2011 21:32:02 -0500 Subject: [PATCH] refactored notice creation process: separate Errbit concerns (e.g. fingerprint) from notice parsing concerns. --- app/controllers/notices_controller.rb | 8 ++++---- app/models/app.rb | 41 +++++++++++++++++++++++++++++++++++++++++ app/models/error_report.rb | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ app/models/notice.rb | 37 +++++++------------------------------ config/initializers/requirements.rb | 1 + lib/core_ext/hash.rb | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/hoptoad.rb | 73 +++++++++++++++++++++++++++++++------------------------------------------ lib/hoptoad/v2.rb | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ spec/controllers/notices_controller_spec.rb | 18 +++++++++--------- spec/fixtures/hoptoad_test_notice_with_one_line_of_backtrace.xml | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ spec/models/app_spec.rb | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ spec/models/notice_spec.rb | 102 ++++++------------------------------------------------------------------------------------------------ 12 files changed, 494 insertions(+), 181 deletions(-) create mode 100644 app/models/error_report.rb create mode 100644 config/initializers/requirements.rb create mode 100644 lib/core_ext/hash.rb create mode 100644 lib/hoptoad/v2.rb create mode 100644 spec/fixtures/hoptoad_test_notice_with_one_line_of_backtrace.xml diff --git a/app/controllers/notices_controller.rb b/app/controllers/notices_controller.rb index 8303949..f983e43 100644 --- a/app/controllers/notices_controller.rb +++ b/app/controllers/notices_controller.rb @@ -1,12 +1,12 @@ class NoticesController < ApplicationController respond_to :xml - + skip_before_filter :authenticate_user!, :only => :create - + def create # params[:data] if the notice came from a GET request, raw_post if it came via POST - @notice = Notice.from_xml(params[:data] || request.raw_post) + @notice = App.report_error!(params[:data] || request.raw_post) respond_with @notice end - + end diff --git a/app/models/app.rb b/app/models/app.rb index 307fedb..2f3964b 100644 --- a/app/models/app.rb +++ b/app/models/app.rb @@ -42,6 +42,47 @@ class App + # Processes a new error report. + # + # Accepts either XML or a hash with the following attributes: + # + # * :klass - the class of error + # * :message - the error message + # * :backtrace - an array of stack trace lines + # + # * :request - a hash of values describing the request + # * :server_environment - a hash of values describing the server environment + # + # * :api_key - the API key with which the error was reported + # * :notifier - information to identify the source of the error report + # + def self.report_error!(*args) + report = ErrorReport.new(*args) + report.generate_notice! + end + + + + # Processes a new error report. + # + # Accepts a hash with the following attributes: + # + # * :klass - the class of error + # * :message - the error message + # * :backtrace - an array of stack trace lines + # + # * :request - a hash of values describing the request + # * :server_environment - a hash of values describing the server environment + # + # * :notifier - information to identify the source of the error report + # + def report_error!(hash) + report = ErrorReport.new(hash.merge(:api_key => api_key)) + report.generate_notice! + end + + + def find_or_create_err!(attrs) Err.where(attrs).first || problems.create!.errs.create!(attrs) end diff --git a/app/models/error_report.rb b/app/models/error_report.rb new file mode 100644 index 0000000..47f32a3 --- /dev/null +++ b/app/models/error_report.rb @@ -0,0 +1,91 @@ +require 'digest/md5' + + +class ErrorReport + + + + def initialize(xml_or_attributes) + @attributes = (xml_or_attributes.is_a?(String) ? Hoptoad.parse_xml!(xml_or_attributes) : xml_or_attributes).with_indifferent_access + end + + + + def klass + @attributes[:klass] + end + + def message + @attributes[:message] + end + + def backtrace + @attributes[:backtrace] + end + + def request + @attributes[:request] + end + + def server_environment + @attributes[:server_environment] + end + + def api_key + @attributes[:api_key] + end + + def notifier + @attributes[:notifier] + end + + + + def fingerprint + @fingerprint ||= ErrorReport.get_fingerprint(self) + end + + def self.get_fingerprint(report) + Digest::MD5.hexdigest(report.backtrace[0].to_s) + end + + def rails_env + server_environment['environment-name'] || 'development' + end + + def component + request['component'] || 'unknown' + end + + def action + request['action'] + end + + def app + @app ||= App.find_by_api_key!(api_key) + end + + + + def generate_notice! + notice = Notice.new( + :message => message, + :backtrace => backtrace, + :request => request, + :server_environment => server_environment, + :notifier => notifier) + + err = app.find_or_create_err!( + :klass => klass, + :component => component, + :action => action, + :environment => rails_env, + :fingerprint => fingerprint) + + err.notices << notice + notice + end + + + +end diff --git a/app/models/notice.rb b/app/models/notice.rb index af10548..eb0667e 100644 --- a/app/models/notice.rb +++ b/app/models/notice.rb @@ -16,8 +16,8 @@ class Notice index :err_id index :created_at + after_create :increase_counter_cache, :cache_attributes_on_problem, :unresolve_problem after_create :deliver_notification, :if => :should_notify? - after_create :increase_counter_cache, :cache_attributes_on_problem before_save :sanitize before_destroy :decrease_counter_cache @@ -30,34 +30,6 @@ class Notice - def self.from_xml(hoptoad_xml) - hoptoad_notice = Hoptoad::V2.parse_xml(hoptoad_xml) - app = App.find_by_api_key!(hoptoad_notice['api-key']) - - hoptoad_notice['request'] ||= {} - hoptoad_notice['request']['component'] = 'unknown' if hoptoad_notice['request']['component'].blank? - hoptoad_notice['request']['action'] = nil if hoptoad_notice['request']['action'].blank? - - err = app.find_or_create_err!({ - :klass => hoptoad_notice['error']['class'], - :component => hoptoad_notice['request']['component'], - :action => hoptoad_notice['request']['action'], - :environment => hoptoad_notice['server-environment']['environment-name'], - :fingerprint => hoptoad_notice['fingerprint'] - }) - err.problem.update_attributes(:resolved => false) if err.problem.resolved? - err.notices.create!({ - :klass => hoptoad_notice['error']['class'], - :message => hoptoad_notice['error']['message'], - :backtrace => [hoptoad_notice['error']['backtrace']['line']].flatten, - :server_environment => hoptoad_notice['server-environment'], - :request => hoptoad_notice['request'], - :notifier => hoptoad_notice['notifier'] - }) - end - - - def user_agent agent_string = env_vars['HTTP_USER_AGENT'] agent_string.blank? ? nil : UserAgent.parse(agent_string) @@ -127,7 +99,7 @@ protected def should_notify? - app.notify_on_errs? && (Errbit::Config.per_app_email_at_notices && app.email_at_notices || Errbit::Config.email_at_notices).include?(err.notices.count) && app.watchers.any? + app.notify_on_errs? && (Errbit::Config.per_app_email_at_notices && app.email_at_notices || Errbit::Config.email_at_notices).include?(problem.notices_count) && app.watchers.any? end @@ -141,6 +113,11 @@ protected end + def unresolve_problem + problem.update_attribute(:resolved, false) if problem.resolved? + end + + def cache_attributes_on_problem problem.cache_notice_attributes(self) if problem.notices_count == 1 end diff --git a/config/initializers/requirements.rb b/config/initializers/requirements.rb new file mode 100644 index 0000000..f4d14e7 --- /dev/null +++ b/config/initializers/requirements.rb @@ -0,0 +1 @@ +require 'core_ext/hash' diff --git a/lib/core_ext/hash.rb b/lib/core_ext/hash.rb new file mode 100644 index 0000000..3d5562f --- /dev/null +++ b/lib/core_ext/hash.rb @@ -0,0 +1,61 @@ +module CoreExt + module Hash + + + + def pick(*picks) + picks = picks.flatten + picks.inject({}) {|result, key| self.key?(key) ? result.merge(key => self[key]) : result} + end + + + + def pick!(*picks) + picks = picks.flatten + keys.each {|key| self.delete(key) unless picks.member?(key) } + end + + + + def except(*picks) + result = self.dup + result.except!(*picks) + result + end + + + + def except!(*picks) + picks = picks.flatten + keys.each {|key| self.delete(key) if picks.member?(key) } + end + + + + def inspect!(depth=0) + s = "" + self.each do |k,v| + s << (" " * depth) + s << k + s << ": " + if v.is_a?(Hash) + s << "{\n" + s << v.inspect!(depth + 2) + s << (" " * depth) + s << "}" + elsif v.is_a?(Array) + s << v.inspect + else + s << v.to_s + end + s << "\n" + end + s + end + + + + end +end + +Hash.send :include, CoreExt::Hash \ No newline at end of file diff --git a/lib/hoptoad.rb b/lib/hoptoad.rb index 4182670..c25a31c 100644 --- a/lib/hoptoad.rb +++ b/lib/hoptoad.rb @@ -1,47 +1,36 @@ -module Hoptoad - module V2 - require 'digest/md5' +require 'hoptoad/v2' - class ApiVersionError < StandardError - def initialize(version) - super "Wrong API Version: Expecting v2.0, got version: #{version}" - end - end - def self.parse_xml(xml) - xml = xml.unpack('C*').pack('U*') # Repack string into Unicode to fix invalid UTF-8 chars - parsed = ActiveSupport::XmlMini.backend.parse(xml)['notice'] - raise ApiVersionError.new(parsed['version']) unless parsed && parsed['version'].to_s == '2.0' - rekeyed = rekey(parsed) - rekeyed['fingerprint'] = Digest::MD5.hexdigest(rekeyed['error']['backtrace'].to_s) - rekeyed +module Hoptoad + + + def self.parse_xml!(xml) + xml = xml.unpack('C*').pack('U*') # Repack string into Unicode to fix invalid UTF-8 chars + parsed = ActiveSupport::XmlMini.backend.parse(xml)['notice'] || raise(ApiVersionError) + processor = get_version_processor(parsed['version']) + processor.process_notice(parsed) + end + + +private + + + def self.get_version_processor(version) + case version + when '2.0'; Hoptoad::V2 + else; raise ApiVersionError + end + end + + +public + + + class ApiVersionError < StandardError + def initialize + super "Wrong API Version: Expecting v2.0" end - - private - - def self.rekey(node) - if node.is_a?(Hash) && node.has_key?('var') && node.has_key?('key') - {node['key'] => rekey(node['var'])} - elsif node.is_a?(Hash) && node.has_key?('var') - rekey(node['var']) - elsif node.is_a?(Hash) && node.has_key?('__content__') && node.has_key?('key') - {node['key'] => node['__content__']} - elsif node.is_a?(Hash) && node.has_key?('__content__') - node['__content__'] - elsif node.is_a?(Hash) - node.inject({}) {|rekeyed, (key,val)| - rekeyed.merge(key => rekey(val)) - } - elsif node.is_a?(Array) && node.first.has_key?('key') - node.inject({}) {|rekeyed,keypair| - rekeyed.merge(rekey(keypair)) - } - elsif node.is_a?(Array) - node.map {|n| rekey(n)} - else - node - end - end end + + end - diff --git a/lib/hoptoad/v2.rb b/lib/hoptoad/v2.rb new file mode 100644 index 0000000..870ed4e --- /dev/null +++ b/lib/hoptoad/v2.rb @@ -0,0 +1,78 @@ +module Hoptoad + module V2 + + + def self.process_notice(parsed) + for_errbit_api( + normalize( + rekey(parsed))) + end + + + private + + + def self.rekey(node) + case node + when Hash + if node.has_key?('var') && node.has_key?('key') + {normalize_key(node['key']) => rekey(node['var'])} + elsif node.has_key?('var') + rekey(node['var']) + elsif node.has_key?('__content__') && node.has_key?('key') + {normalize_key(node['key']) => rekey(node['__content__'])} + elsif node.has_key?('__content__') + rekey(node['__content__']) + else + node.inject({}) {|rekeyed, (key, val)| rekeyed.merge(normalize_key(key) => rekey(val))} + end + + when Array + if node.first.has_key?('key') + node.inject({}) {|rekeyed, keypair| rekeyed.merge(rekey(keypair))} + else + node.map {|n| rekey(n)} + end + + else + node + + end + end + + + def self.normalize_key(key) + key.gsub('.', '_') + end + + + def self.normalize(notice) + error = notice['error'] + backtrace = error['backtrace'] + backtrace['line'] = [backtrace['line']] unless backtrace['line'].is_a?(Array) + + notice['request'] ||= {} + notice['request']['component'] = 'unknown' if notice['request']['component'].blank? + notice['request']['action'] = nil if notice['request']['action'].blank? + + notice + end + + + def self.for_errbit_api(notice) + { + :klass => notice['error']['class'], + :message => notice['error']['message'], + :backtrace => notice['error']['backtrace']['line'], + + :request => notice['request'], + :server_environment => notice['server-environment'], + + :api_key => notice['api-key'], + :notifier => notice['notifier'] + } + end + + + end +end \ No newline at end of file diff --git a/spec/controllers/notices_controller_spec.rb b/spec/controllers/notices_controller_spec.rb index de509bc..a73a6ff 100644 --- a/spec/controllers/notices_controller_spec.rb +++ b/spec/controllers/notices_controller_spec.rb @@ -1,29 +1,29 @@ require 'spec_helper' describe NoticesController do - + context 'notices API' do before do @xml = Rails.root.join('spec','fixtures','hoptoad_test_notice.xml').read @app = Factory(:app_with_watcher) App.stub(:find_by_api_key!).and_return(@app) - @notice = Notice.from_xml(@xml) - + @notice = App.report_error!(@xml) + request.env['Content-type'] = 'text/xml' request.env['Accept'] = 'text/xml, application/xml' end - + it "generates a notice from xml [POST]" do - Notice.should_receive(:from_xml).with(@xml).and_return(@notice) + App.should_receive(:report_error!).with(@xml).and_return(@notice) request.should_receive(:raw_post).and_return(@xml) post :create end - + it "generates a notice from xml [GET]" do - Notice.should_receive(:from_xml).with(@xml).and_return(@notice) + App.should_receive(:report_error!).with(@xml).and_return(@notice) get :create, {:data => @xml} end - + it "sends a notification email" do request.should_receive(:raw_post).and_return(@xml) post :create @@ -34,5 +34,5 @@ describe NoticesController do email.subject.should include("[#{@notice.environment_name}]") end end - + end diff --git a/spec/fixtures/hoptoad_test_notice_with_one_line_of_backtrace.xml b/spec/fixtures/hoptoad_test_notice_with_one_line_of_backtrace.xml new file mode 100644 index 0000000..fdd45ab --- /dev/null +++ b/spec/fixtures/hoptoad_test_notice_with_one_line_of_backtrace.xml @@ -0,0 +1,75 @@ + + + APIKEY + + Hoptoad Notifier + 2.3.2 + http://hoptoadapp.com + + + HoptoadTestingException + HoptoadTestingException: Testing hoptoad via "rake hoptoad:test". If you can see this, it works. + + + + + + http://example.org/verify + application + verify + + verify + application + + + + text/html + + verify + application + + example.org + http + + 0 + #<StringIO:0x103d9dec0> + + + off + false + /verify + 994f235e3372684bc736dd11842b754d2ddcffc8c2958d33a29527c3217becd6655fa4653a318bc7c34131f9baf2acc0c424ed07e48e0e5e87c6cd34d711e985 + 11 + + + verify + application + + false + password + + + true + + 80 + GET + #<ApplicationController:0x103d2f560> + + false + true + / + + + + + #<StringIO:0x103d9dc90> + + + + + + + /path/to/sample/project + development + + \ No newline at end of file diff --git a/spec/models/app_spec.rb b/spec/models/app_spec.rb index 4e0e397..c8455c9 100644 --- a/spec/models/app_spec.rb +++ b/spec/models/app_spec.rb @@ -145,4 +145,94 @@ describe App do end + context '#report_error!' do + before do + @xml = Rails.root.join('spec','fixtures','hoptoad_test_notice.xml').read + @app = Factory(:app, :api_key => 'APIKEY') + ErrorReport.stub(:get_fingerprint).and_return('fingerprintdigest') + end + + it 'finds the correct app' do + @notice = App.report_error!(@xml) + @notice.err.app.should == @app + end + + it 'finds the correct err for the notice' do + App.should_receive(:find_by_api_key!).and_return(@app) + @app.should_receive(:find_or_create_err!).with({ + :klass => 'HoptoadTestingException', + :component => 'application', + :action => 'verify', + :environment => 'development', + :fingerprint => 'fingerprintdigest' + }).and_return(err = Factory(:err)) + err.notices.stub(:create!) + @notice = App.report_error!(@xml) + end + + it 'marks the err as unresolved if it was previously resolved' do + App.should_receive(:find_by_api_key!).and_return(@app) + @app.should_receive(:find_or_create_err!).with({ + :klass => 'HoptoadTestingException', + :component => 'application', + :action => 'verify', + :environment => 'development', + :fingerprint => 'fingerprintdigest' + }).and_return(err = Factory(:err, :problem => Factory(:problem, :resolved => true))) + err.should be_resolved + @notice = App.report_error!(@xml) + @notice.err.should == err + @notice.err.should_not be_resolved + end + + it 'should create a new notice' do + @notice = App.report_error!(@xml) + @notice.should be_persisted + end + + it 'assigns an err to the notice' do + @notice = App.report_error!(@xml) + @notice.err.should be_a(Err) + end + + it 'captures the err message' do + @notice = App.report_error!(@xml) + @notice.message.should == 'HoptoadTestingException: Testing hoptoad via "rake hoptoad:test". If you can see this, it works.' + end + + it 'captures the backtrace' do + @notice = App.report_error!(@xml) + @notice.backtrace.size.should == 73 + @notice.backtrace.last['file'].should == '[GEM_ROOT]/bin/rake' + end + + it 'captures the server_environment' do + @notice = App.report_error!(@xml) + @notice.server_environment['environment-name'].should == 'development' + end + + it 'captures the request' do + @notice = App.report_error!(@xml) + @notice.request['url'].should == 'http://example.org/verify' + @notice.request['params']['controller'].should == 'application' + end + + it 'captures the notifier' do + @notice = App.report_error!(@xml) + @notice.notifier['name'].should == 'Hoptoad Notifier' + end + + it "should handle params without 'request' section" do + xml = Rails.root.join('spec','fixtures','hoptoad_test_notice_without_request_section.xml').read + lambda { App.report_error!(xml) }.should_not raise_error + end + + it "should handle params with only a single line of backtrace" do + xml = Rails.root.join('spec','fixtures','hoptoad_test_notice_with_one_line_of_backtrace.xml').read + lambda { @notice = App.report_error!(xml) }.should_not raise_error + @notice.backtrace.length.should == 1 + end + end + + end diff --git a/spec/models/notice_spec.rb b/spec/models/notice_spec.rb index 84e461e..28223e5 100644 --- a/spec/models/notice_spec.rb +++ b/spec/models/notice_spec.rb @@ -39,111 +39,21 @@ describe Notice do 'method' => ActiveSupport.methods.shuffle.first }] end - + it "should be false for line not starting with PROJECT_ROOT" do Notice.in_app_backtrace_line?(backtrace[0]).should == false end - + it "should be false for file in vendor dir" do Notice.in_app_backtrace_line?(backtrace[1]).should == false end - + it "should be true for application file" do Notice.in_app_backtrace_line?(backtrace[2]).should == true end end - context '#from_xml' do - before do - @xml = Rails.root.join('spec','fixtures','hoptoad_test_notice.xml').read - @app = Factory(:app, :api_key => 'APIKEY') - Digest::MD5.stub(:hexdigest).and_return('fingerprintdigest') - end - - it 'finds the correct app' do - @notice = Notice.from_xml(@xml) - @notice.err.app.should == @app - end - - it 'finds the correct err for the notice' do - App.should_receive(:find_by_api_key!).and_return(@app) - @app.should_receive(:find_or_create_err!).with({ - :klass => 'HoptoadTestingException', - :component => 'application', - :action => 'verify', - :environment => 'development', - :fingerprint => 'fingerprintdigest' - }).and_return(err = Factory(:err)) - err.notices.stub(:create!) - @notice = Notice.from_xml(@xml) - end - - - it 'marks the err as unresolved if it was previously resolved' do - App.should_receive(:find_by_api_key!).and_return(@app) - @app.should_receive(:find_or_create_err!).with({ - :klass => 'HoptoadTestingException', - :component => 'application', - :action => 'verify', - :environment => 'development', - :fingerprint => 'fingerprintdigest' - }).and_return(err = Factory(:err, :problem => Factory(:problem, :resolved => true))) - err.should be_resolved - @notice = Notice.from_xml(@xml) - @notice.err.should == err - @notice.err.should_not be_resolved - end - - it 'should create a new notice' do - @notice = Notice.from_xml(@xml) - @notice.should be_persisted - end - - it 'assigns an err to the notice' do - @notice = Notice.from_xml(@xml) - @notice.err.should be_a(Err) - end - - it 'captures the err message' do - @notice = Notice.from_xml(@xml) - @notice.message.should == 'HoptoadTestingException: Testing hoptoad via "rake hoptoad:test". If you can see this, it works.' - end - - it 'captures the backtrace' do - @notice = Notice.from_xml(@xml) - @notice.backtrace.size.should == 73 - @notice.backtrace.last['file'].should == '[GEM_ROOT]/bin/rake' - end - - it 'captures the server_environment' do - @notice = Notice.from_xml(@xml) - @notice.server_environment['environment-name'].should == 'development' - end - - it 'captures the request' do - @notice = Notice.from_xml(@xml) - @notice.request['url'].should == 'http://example.org/verify' - @notice.request['params']['controller'].should == 'application' - end - - it 'captures the notifier' do - @notice = Notice.from_xml(@xml) - @notice.notifier['name'].should == 'Hoptoad Notifier' - end - - it "should handle params withour 'request' section" do - @xml = Rails.root.join('spec','fixtures','hoptoad_test_notice_without_request_section.xml').read - lambda { Notice.from_xml(@xml) }.should_not raise_error - end - - it "should raise ApiVersionError" do - @xml = Rails.root.join('spec', 'fixtures', 'hoptoad_test_notice_with_wrong_version.xml').read - expect { Notice.from_xml(@xml) }.to raise_error(Hoptoad::V2::ApiVersionError) - end - end - - describe "key sanitization" do before do @hash = { "some.key" => { "$nested.key" => {"$Path" => "/", "some$key" => "key"}}} @@ -179,7 +89,7 @@ describe Notice do before do Errbit::Config.per_app_email_at_notices = true @app = Factory(:app_with_watcher, :email_at_notices => custom_thresholds) - @problem = Factory(:err, :problem => @app.problems.create!) + @err = Factory(:err, :problem => Factory(:problem, :app => @app)) end after do @@ -188,10 +98,10 @@ describe Notice do custom_thresholds.each do |threshold| it "sends an email notification after #{threshold} notice(s)" do - @problem.notices.stub(:count).and_return(threshold) + @err.problem.stub(:notices_count).and_return(threshold) Mailer.should_receive(:err_notification). and_return(mock('email', :deliver => true)) - Factory(:notice, :err => @problem) + Factory(:notice, :err => @err) end end end -- libgit2 0.21.2