Rework error handling in File Uploads#19928
Draft
dstufft wants to merge 12 commits into
Draft
Conversation
5626734 to
ae016cb
Compare
ae016cb to
f5f4224
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The file uploads view handler is a gnarly beast and it's really hard to follow what is going on with it.
Some of that is due to the inherent structure of that view currently (lots of nesting, a lot of things happening inline, etc), which this PR doesn't really do anything to resolve.
However, some of that is due to the fact that we have a lot of visual noise being introduced by how we're currently handling errors within that view.
Specifically, it's a common pattern to signal an error in file uploads using something like:
The actual contents of the error message there is not particularly useful for understanding what is happening in the file uploads view, but it takes up a disproportionate amount of visual real estate when reading the view.
We're also trying to have metrics that indicate why uploads are failing, which itself ends up being boilerplate that adds to the visual noise-- but also ends up being something that is very easy to actually forget to add.
We also have a lot of very similar errors which get slightly different error messages, based entirely on whatever the person who implemented that particular error happened to write, which is exasperated by the fact that the error message is being written "in line" with each place an error can occur.
On top of all of this, this pattern makes testing the file uploads handler harder, because we have to test the "rendered" message in our tests to actually see if the error is happening, and there isn't any sort of unique error type for different kinds of errors. Which is made worse by the fact that many of our error messages render help links as part of the message, which means that we have to either stub that out in our tests or setup machinery for handling the help links for us.
So this PR introduces a change where we have a special error type (
ForkliftException) that can be subclassed to create a variety of different errors, and as part of creating that subclass you're required to give an error message and a list of tags. The message/tags can contain Python string formatting markers in them, and they'll be substituted at error time with passed in values.These special error types also understand how to add our various different types of "help" and docs urls that we have, so those can be included declaratively as well and rendered at error time as well.
An example of what some of these errors looks like would be:
All of the "magic" stuff is passed in as a keyword to the class creation, so that we can require things like
messageandtagswhereas it can be easy to forget to add a class var to your subclass that needs to be overriden-- that is impossible here.Then you can raise one of these errors just as you'd normally raise an error in Python:
This gets translated into a
HTTPException(by default it'sHttpBadRequest, but that can be overriden by the subclass) by a decorator (translate_errors) applied to the forklift views that catch those errors and ultimately does the same logic that currently exists within_exc_with_messageas well as emits the metrics.This does make adding a new error a bit more annoying, as you have to go define an error type, but it removes a significant amount of the visual noise in the file upload view:
I've marked this PR as a draft because it's only half done; A bunch of errors in the upload view still need to be translated, I haven't touched the tests at all, etc.
However, I wanted to throw it up to give people a chance to look at the general pattern and see what they thought!
I'm not planning on doing it as part of this PR, but I think we could also make some other improvements here (maybe as a follow up):
Anyways, I'm planning to keep working on this unless people think it's horrible, but I figured I'd let folks see it before I spend too much more time on it.