Filter reused guid:version strings from not blocked items in mlbf

[KevinMind]

Fixes: mozilla/addons#15132

Description

Ensure filtering of duplicate guid:version strings from not_blocked_items

Context

After merging commit it became clear in stage that we have duplicate hashed version strings in the mlbf data set.

Testing

unit tested to cover the edge case

Checklist

• Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
• Successfully verified the change locally.
• The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
• Add before and after screenshots (Only for changes that impact the UI).
• Add or update relevant docs reflecting the changes made.

[eviljeff]

Something for the next time we touch this code (soon!) - we should have a check that there is no common items between the two sets and raise with an explicit error if it happens - we spent way too long wondering if there was some change in filter_cascade dependencies because the error we got from there was an obscure one about error levels, and not "Don't have duplicates!"

[eviljeff]

being paranoid: can we assert that (addon.guid, version) is in fetch_all_versions_from_db(MLBF._version_excludes) ? i.e. would have been included otherwise?

[KevinMind]

I'd rather not test the underlying function but the final data set. Or am I missing something in your ask?

[eviljeff]

I was looking for a verification the test is set up correctly - that we have a case where there is a guid, version pair that is unblocked, and a pair that is blocked, but they don't both end up in final data set. I suppose closer to a functional test than an integration test.
There are other ways to test that - it was just the first way I thought of.

[KevinMind]

Something for the next time we touch this code (soon!) - we should have a check that there is no common items between the two sets and raise with an explicit error if it happens - we spent way too long wondering if there was some change in filter_cascade dependencies because the error we got from there was an obscure one about error levels, and not "Don't have duplicates!"

It's still not entirely clear if this bug is the cause of the error we saw.. I agree with you though we could have a more explicit check that no entry is in multiple arrays.. not totally clear when or where that check should be though.

[eviljeff]

Something for the next time we touch this code (soon!) - we should have a check that there is no common items between the two sets and raise with an explicit error if it happens - we spent way too long wondering if there was some change in filter_cascade dependencies because the error we got from there was an obscure one about error levels, and not "Don't have duplicates!"

It's still not entirely clear if this bug is the cause of the error we saw..

If I'm right we'll know soon 😄

I agree with you though we could have a more explicit check that no entry is in multiple arrays.. not totally clear when or where that check should be though.

Probably just before cascade.initialize errors in an obscure way, really. If it's gotten that far we've got a bug elsewhere we can't (shouldn't try to) recover from, but at least the error would be comprehensible in sentry.