Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

filter_chain: don't try to filter nil files #120

Merged
merged 1 commit into from
Sep 9, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ Airbrake Ruby Changelog
* Started validating the 'environment' config option (a warning will be printed,
if it is misconfigured)
([#115](https://github.com/airbrake/airbrake-ruby/pull/115))
* Fixed error while filtering unparseable backtraces
([#120](https://github.com/airbrake/airbrake-ruby/pull/120))

### [v1.4.6][v1.4.6] (August 18, 2016)

Expand Down
8 changes: 6 additions & 2 deletions lib/airbrake-ruby/filter_chain.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ class FilterChain
notice[:errors].each do |error|
Gem.path.each do |gem_path|
error[:backtrace].each do |frame|
frame[:file].sub!(/\A#{gem_path}/, '[GEM_ROOT]'.freeze)
# If the frame is unparseable, then 'file' is nil, thus nothing to
# filter (all frame's data is in 'function' instead).
next unless (file = frame[:file])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have a comment here repeating what you said in PR description "when airbrake-ruby tries to send an exception with a backtrace, which couldn't be parsed. In that case we put the whole frame into function, leaving line and file nil."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

file.sub!(/\A#{gem_path}/, '[GEM_ROOT]'.freeze)
end
end
end
Expand Down Expand Up @@ -67,7 +70,8 @@ def root_directory_filter(root_directory)
proc do |notice|
notice[:errors].each do |error|
error[:backtrace].each do |frame|
frame[:file].sub!(/\A#{root_directory}/, '[PROJECT_ROOT]'.freeze)
next unless (file = frame[:file])
file.sub!(/\A#{root_directory}/, '[PROJECT_ROOT]'.freeze)
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you probably should combine those 2 filters together to save some CPU time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it would be possible to do that without creating messy code. This particular filter executes only if the root_direction option is defined. This proc doesn't know about the config and I wouldn't want to pass it just for 1 check. They look similar only on the surface.

end
end
Expand Down
36 changes: 36 additions & 0 deletions spec/filter_chain_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,42 @@
to(change { notice.ignored? }.from(false).to(true))
end
end

context "gem root filter" do
let(:ex) do
AirbrakeTestError.new.tap do |error|
error.set_backtrace(['(unparseable/frame.rb:23)'])
end
end

it "does not filter file if it is nil" do
config.logger = Logger.new('/dev/null')
notice = Airbrake::Notice.new(config, ex)

expect(notice[:errors].first[:file]).to be_nil
expect { @chain.refine(notice) }.
not_to change { notice[:errors].first[:file] }
end
end

context "root directory filter" do
let(:ex) do
AirbrakeTestError.new.tap do |error|
error.set_backtrace(['(unparseable/frame.rb:23)'])
end
end

it "does not filter file if it is nil" do
config.logger = Logger.new('/dev/null')
config.root_directory = '/bingo/bango'
notice = Airbrake::Notice.new(config, ex)
filter_chain = described_class.new(config)

expect(notice[:errors].first[:file]).to be_nil
expect { filter_chain.refine(notice) }.
not_to change { notice[:errors].first[:file] }
end
end
end
end
end