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

Refactor Brakeman::Differ#second_pass #1406

Merged
merged 1 commit into from Oct 16, 2019

Conversation

@Becojo
Copy link
Contributor

Becojo commented Oct 15, 2019

Issue

I had noticed that Brakeman::Differ can sometimes provide inaccurate results (for example: missing new warnings or incorrectly reporting a warning as new) but I haven't really dug into the code until today.

Turns out the issue is in Brakeman::Differ#second_pass when it attempts to delete elements using indices while iterating the lists:

warnings[:new].delete_at(new_warning_id - elements_deleted_offset)
elements_deleted_offset += 1
warnings[:fixed].delete_at(fixed_warning_id)

In my case, the output of both delete_at were not the same warning. This caused the fixed list to become empty when the new list still had warnings in it. This would cause the remaining warnings to be considered as new.

Solution

I've redone the algorithm by building a set of fingerprints from both warning lists which I then intersect to remove in bulk from both lists. It should have a better complexity than the previous algorithm.

Though, I left a similar algorithm afterwards for backward compatibility with old warning keys (I'm not sure if it's really needed).

Also, I renamed DifferTests#diff to DifferTests#run_diff because it shadowed Minitest::Test#diff which was quite confusing while coding this.

lib/brakeman/differ.rb Outdated Show resolved Hide resolved
Copy link
Owner

presidentbeef left a comment

Hi @Becojo - thank you so much for looking at this! I think I may have encountered this behavior, too.

Is it possible to devise a test case for the old, bad behavior?

lib/brakeman/differ.rb Outdated Show resolved Hide resolved
@Becojo Becojo force-pushed the Becojo:differ-second-pass-fix branch from 63036b4 to 83d65d3 Oct 15, 2019
@Becojo

This comment has been minimized.

Copy link
Contributor Author

Becojo commented Oct 15, 2019

🤦‍♂I misidentified the cause of my issue. So more context on my use case: I store the JSON reports and compare them using Brakeman::Differ.

The issue only happens if the keys are not converted to back into symbols because the following check will return false:

if new_warning[:fingerprint] and fixed_warning[:fingerprint]

Which then goes into the old warning key check

OLD_WARNING_KEYS.each do |attr|
return false if new_warning[attr] != fixed_warning[attr]
end

Where the warnings are considered equal because every key is nil (because the keys are string and not symbols).


In any case, we can probably repurpose this PR as a performance refactor of the Differ then. What do you think? I've addressed your comment about the loops and removed code about the old warning keys.

@Becojo Becojo changed the title Fix for Brakeman::Differ missing new/fixed warnings Refactor Brakeman::Differ#second_pass Oct 15, 2019
@presidentbeef presidentbeef merged commit a0e046f into presidentbeef:master Oct 16, 2019
8 of 9 checks passed
8 of 9 checks passed
codeclimate/diff-coverage 94% (98% threshold)
Details
ci/circleci: default Your tests passed on CircleCI!
Details
ci/circleci: test-2-3 Your tests passed on CircleCI!
Details
ci/circleci: test-2-4 Your tests passed on CircleCI!
Details
ci/circleci: test-2-5 Your tests passed on CircleCI!
Details
ci/circleci: test-2-6 Your tests passed on CircleCI!
Details
ci/circleci: upload-coverage Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/total-coverage 95% (0.0% change)
Details
@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Oct 16, 2019

Thanks! 🌞

@Becojo Becojo deleted the Becojo:differ-second-pass-fix branch Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.