feat(robot-server): Present error codes in error responses #12969
Conversation
Codecov Report
@@ Coverage Diff @@
## edge #12969 +/- ##
==========================================
- Coverage 72.87% 72.82% -0.05%
==========================================
Files 2351 2362 +11
Lines 64445 64470 +25
Branches 7190 7199 +9
==========================================
- Hits 46963 46950 -13
- Misses 15799 15837 +38
Partials 1683 1683
Flags with carried forward coverage won't be shown. Click here to find out more.
|
By adding codes to the response models we can make sure they're always expressed by all the random overriding models
We can now express error codes in top level error responses and handle and format them from the eneumerated errors that the rest of the system will soon throw. There's some interesting detail there around the MultiErrorResponse and the ErrorResponse but with multiple errors captured inside of it... I'm not sure what the right play is, but this is working out for me. There are some response format changes for some of the messages; we can fix these by making the exceptions that are being turned into the error responses EnumeratedErrors that are properly formatted.
sfoster1
force-pushed
the
robot-server-error-codes
branch
from
4a0a575 to
ab7290a
Compare
|
Openapi docs and spec now show the error code in error response schemas with a default of 4000! |
| detail: Optional[Dict[str, Any]] = None, | ||
| wrapping: Optional[Sequence[EnumeratedError]] = None, | ||
| ) -> None: | ||
| """Build a CanbusCommunicationError.""" |
| detail: Optional[Dict[str, Any]] = None, | ||
| wrapping: Optional[Sequence[EnumeratedError]] = None, | ||
| ) -> None: | ||
| """Build an GripperNotPresentError.""" |
| @@ -105,6 +106,48 @@ def get_some_model(): | |||
| "occurrence of the error" | |||
| ), | |||
| ) | |||
| errorCode: str = Field( | |||
There was a problem hiding this comment.
Should the type be Optional[str] since there's a default of None?
There was a problem hiding this comment.
That's weird that it allows that. i should change that to default to 4000 anyway
There was a problem hiding this comment.
Does the way we treat
MultipleErrorResponseandErrorResponsemake sense? What's the point of the multi error response anyway, given that error response can hold multiple errors?
Hmm. It looks like this was introduced in #7783. I don't know if this was the intent, but theoretically, it lets us provide a stricter model when we know we're only returning a single error. (Which is almost always.) Like, we can override the OpenAPI spec to include "minItems": 1, "maxItems": 1 or something.
| checked_exc = exc | ||
|
|
||
| return cls( | ||
| message=checked_exc.message.strip(), errorCode=checked_exc.code.value.code |
There was a problem hiding this comment.
Why do we need to .strip() the message?
There was a problem hiding this comment.
for a reason I can't be bothered with it comes with a trailing newline
| @classmethod | ||
| def from_exc( | ||
| cls: Type["FirmwareUpdateRequired"], | ||
| exc: BaseException, | ||
| **supplemental_kwargs: Any | ||
| ) -> "FirmwareUpdateRequired": | ||
| """Build a FirmwareUpdateRequired from a specific exception. Preserves metadata.""" | ||
| parent_inst = ErrorDetails.from_exc(exc, **supplemental_kwargs) | ||
| inst = FirmwareUpdateRequired(**parent_inst.dict()) | ||
| if not inst.meta: | ||
| inst.meta = {"update_url": "/subsystems/update"} | ||
| else: | ||
| inst.meta["update_url"] = "/subsystems/update" | ||
| return inst |
There was a problem hiding this comment.
Let me make sure I understand what's going on here:
meta normally includes attributes automatically taken from the Python exception. But when we're returning a FirmwareUpdateRequired model, we also want to include a helpful update_url pointer. That won't be present in the original Python exception, so we need to add it here.
Is that right?
There was a problem hiding this comment.
Yeah pretty much. It's pretty equally metadata, so it doesn't really do any harm to put it in there next to all the other stuff
| raise RunNotFound(detail=str(e)).as_error(status.HTTP_404_NOT_FOUND) from e | ||
| raise RunNotFound.from_exc(e).as_error(status.HTTP_404_NOT_FOUND) from e |
There was a problem hiding this comment.
Very very very happy that we're making this broken detail=str(e) pattern less pervasive.
This raise SomeModel.from_exc(..).as_error(...) pattern is the confusing back-and-forth thing between models and exceptions that you were talking about before, right?
There was a problem hiding this comment.
Yeah, and it's worse in some other places like the exception handlers, where we
- catch an exception
- build a model from it
- create an exception from the model with
as_error - pass that somewhere else
- ultimately put the exception instance's
.contentin the response
What I'd really prefer is that exceptions get turned into models and models get sent, without going back into ApiErrors.
This PR does the equivalent of #12936 for the non-protocol-engine robot server error responses - both the
LegacyErrorResponse(which now has anerrorCodeentry as well as amessage, but otherwise not a whole lot else) and theErrorResponseandMultiErrorResponse, which now have a top levelerrorCodeand also properly intern the exception chain in their multiple-error-body responses.We also go through and add a couple new useful errors, which is a process that will continue.
Review Requests
MultipleErrorResponseandErrorResponsemake sense? What's the point of the multi error response anyway, given that error response can hold multiple errors?Testing
Next steps
Similarly to #12936 , now that the catchalls and structure is in place we need to go through and make sure that the exceptions that we raise to start any given error response off have error codes and tuned messages.