Re-run scanner actions if results changed when running narc scanner on version #23793
Conversation
…n version To handle the case where a scanner action is run multiple times, the actions now check for a NeedsHumanReview corresponding to a scanner action before doing anything: if there weren't any scanner action NHR, or if one or more active scanner action NHR exist, we proceed with the action, but otherwise we abort (reviewer has acknowledged the NHR and the scanner shouldn't override what the reviewer has done)
|
re:
Do you mean "active"? (an active NHR would mean the reviewer hasn't looked at the addon/version; an inactive would mean they have) |
My sentence was poorly worded, the code is less confusing :) I meant:
|
| if not version.needshumanreview_set.filter( | ||
| reason=NeedsHumanReview.REASONS.SCANNER_ACTION | ||
| ).exists(): | ||
| version.needshumanreview_set.create( | ||
| reason=NeedsHumanReview.REASONS.SCANNER_ACTION | ||
| ) | ||
| return True | ||
| # If it has been flagged already... then return True only if one of the | ||
| # flags is active still. | ||
| return version.needshumanreview_set.filter( | ||
| reason=NeedsHumanReview.REASONS.SCANNER_ACTION, is_active=True | ||
| ).exists() |
There was a problem hiding this comment.
Nit/optimization: This is logically correct, but we're running two database queries to check the NHRs exist - if we run a values or values_list of is_active (and group by is_active) we should be able to know in a single query if there any any active or inactive NHRs
There was a problem hiding this comment.
You're right, I don't think it matters enough to refactor it at this point though so I'm going to leave as-is for now.
| scanner_result.results = [json.loads(result) for result in sorted(results)] | ||
| run_action = False | ||
| new_results = [json.loads(result) for result in sorted(results)] | ||
| if version and new_results != scanner_result.results: |
There was a problem hiding this comment.
The results don't contain any datetimes or other unique values, right?
There was a problem hiding this comment.
They shouldn't, it should all be stable. At some point in the original NARC implementation I had the trigger in there (with value being version or upload) and removed it when I realized it would prevent this comparison from working.
| if scanner_result.has_matches: | ||
| statsd.incr('devhub.narc.has_matches') | ||
| for scanner_rule in scanner_result.matched_rules.all(): | ||
| statsd.incr(f'devhub.narc.rule.{scanner_rule.id}.match') |
There was a problem hiding this comment.
Maybe we don't want to increase the statsd pings if the results haven't changed? (it'd be inflating the match counts)
There was a problem hiding this comment.
I went back and forth on this and I'm still not sure so I kept it as-is. The problem is, if you want to compare the number of matches against the number of runs (devhub.narc.success) then you need to still increase the pings even if the results haven't changed.
I introduced devhub.narc.results_differ ping to at least monitor when results differ but maybe we'd want another to tell us when we ran the scanner on a version and results were the same...
There was a problem hiding this comment.
Filed mozilla/addons#15776 as a follow-up.
Fixes mozilla/addons#15765
(Also addresses follow-ups for mozilla/addons#15763)
Description
Initial implementation of mozilla/addons#15763 already triggered a new scan when developer or add-on name changes outside of the XPI, but we didn't re-trigger the scanner actions. This does.
Context
To handle the case where a scanner action is run multiple times (something that is specific to
NARC, doesn't happen for other scanners), the actions now check for aNeedsHumanReviewcorresponding to a scanner action before doing anything: if there weren't any scanner action NHR, or if one or more active scanner action NHR exist, we proceed with the action, but otherwise we abort (reviewer has acknowledged the NHR and the scanner shouldn't override what the reviewer has done)Testing
Initial setup
API submission while providing a different name
namethrough the parameters that would match the rule aboveChanging the name afterwards
Changing the name afterwards but the add-on was reviewed