search

mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
127 stars 41 forks source link

Remove extra parsing of manifest at validation time #1848

Closed diox closed 3 weeks ago

diox commented 3 years ago

We parse the manifest a few times when we validate an upload. This one at least seems wasteful: https://github.com/mozilla/addons-server/blob/2ba2dcfc7170b0e8afb941d08946dda6eb61986b/src/olympia/devhub/tasks.py#L281-L291

Back when we wanted special treatment of legacy add-ons before they reached the linter, this was useful (we were checking the results of parse_addon() to annotate the validation results), but we no longer do that, so this code is useless.

┆Issue is synchronized with this Jira Task

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

diox commented 2 years ago

This has come up recently again during a discussion of an error we saw where a developer made a mistake in their manifest, making applications (or browser_specific_settings) an array instead of an object, resulting in https://sentry.prod.mozaws.net/operations/olympia-prod/issues/19461814/?query=is%3Aunresolved. This is handled in the linter by validating the manifest against the schema, but because of that parse_addon() call that happens earlier, an exception is raised by addons-server and the developer doesn't get to see a nice error message.

We can't just remove the parse_addon() call now though: not long after this issue was filed, 526ff75cd3b made use of it to annotate results. However, this could be done after linting, and only on valid results, or perhaps refactored slightly by having the linter returns what the annotations code needs in metadata and remove that extra parsing entirely.

KevinMind commented 6 months ago

Old Jira Ticket: https://mozilla-hub.atlassian.net/browse/ADDSRV-62

github-actions[bot] commented 1 month ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.