Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(robot-server): Present error codes in error responses #12969

Merged
merged 8 commits into from
Jun 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion robot-server/robot_server/commands/get_default_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
from opentrons.hardware_control import HardwareControlAPI
from opentrons.protocol_engine import ProtocolEngine

from opentrons_shared_data.errors import ErrorCodes

from robot_server.errors import ErrorDetails
from robot_server.hardware import get_hardware
from robot_server.runs import EngineStore, EngineConflictError, get_engine_store
Expand All @@ -24,6 +26,7 @@ class RunActive(ErrorDetails):
"There is an active run. Close the current run"
" to issue commands via POST /commands."
)
errorCode: str = ErrorCodes.ROBOT_IN_USE.value.code


async def get_default_engine(
Expand All @@ -35,7 +38,7 @@ async def get_default_engine(
try:
engine = await engine_store.get_default_engine()
except EngineConflictError as e:
raise RunActive().as_error(status.HTTP_409_CONFLICT) from e
raise RunActive.from_exc(e).as_error(status.HTTP_409_CONFLICT) from e

attached_modules = hardware_api.attached_modules
attached_module_spec = {
Expand Down
4 changes: 3 additions & 1 deletion robot-server/robot_server/commands/router.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from opentrons.protocol_engine import ProtocolEngine, CommandIntent
from opentrons.protocol_engine.errors import CommandDoesNotExistError
from opentrons_shared_data.errors import ErrorCodes

from robot_server.errors import ErrorDetails, ErrorBody
from robot_server.service.json_api import (
Expand All @@ -31,6 +32,7 @@ class CommandNotFound(ErrorDetails):

id: Literal["StatelessCommandNotFound"] = "StatelessCommandNotFound"
title: str = "Stateless Command Not Found"
errorCode: str = ErrorCodes.GENERAL_ERROR.value.code


@commands_router.post(
Expand Down Expand Up @@ -177,7 +179,7 @@ async def get_command(
command = engine.state_view.commands.get(commandId)

except CommandDoesNotExistError as e:
raise CommandNotFound(detail=str(e)).as_error(status.HTTP_404_NOT_FOUND) from e
raise CommandNotFound.from_exc(e).as_error(status.HTTP_404_NOT_FOUND) from e

return await PydanticResponse.create(
content=SimpleBody.construct(data=cast(StatelessCommand, command)),
Expand Down
56 changes: 54 additions & 2 deletions robot-server/robot_server/errors/error_responses.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
"""JSON API errors and response models."""
from pydantic import BaseModel, Field
from pydantic.generics import GenericModel
from typing import Any, Dict, Generic, Optional, Sequence, Tuple, TypeVar
from typing import Any, Dict, Generic, Optional, Sequence, TypeVar, Type

from robot_server.service.json_api import BaseResponseBody, ResourceLinks
from opentrons_shared_data.errors import EnumeratedError, PythonException, ErrorCodes


class ApiError(Exception):
Expand Down Expand Up @@ -105,6 +106,40 @@ def get_some_model():
"occurrence of the error"
),
)
errorCode: str = Field(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the type be Optional[str] since there's a default of None?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's weird that it allows that. i should change that to default to 4000 anyway

ErrorCodes.GENERAL_ERROR.value.code,
description=("The Opentrons error code associated with the error"),
)

@classmethod
def from_exc(
cls: Type["ErrorDetailsT"], exc: BaseException, **supplemental_kwargs: Any
) -> "ErrorDetailsT":
"""Build an ErrorDetails model from an exception.

To allow for custom child models of the ErrorDetails base setting separate
defaults, if a default is set for a given field it won't be set from the
exception unless override_defaults is True.
"""
values = {k: v for k, v in supplemental_kwargs.items()}
if not isinstance(exc, EnumeratedError):
checked_exc: EnumeratedError = PythonException(exc)
else:
checked_exc = exc
values["detail"] = checked_exc.message.strip()
values["errorCode"] = checked_exc.code.value.code

def _exc_to_meta(exc_val: EnumeratedError) -> Dict[str, Any]:
return {
"type": exc_val.detail.get("class", exc_val.__class__.__name__),
"code": exc_val.code.value.code,
"message": exc_val.message.strip(),
"detail": {k: v for k, v in exc_val.detail.items()},
"wrapping": [_exc_to_meta(wrapped) for wrapped in exc_val.wrapping],
}

values["meta"] = _exc_to_meta(checked_exc)
return cls(**values)

def as_error(self, status_code: int) -> ApiError:
"""Serial this ErrorDetails as an ApiError from an ErrorResponse."""
Expand All @@ -121,12 +156,29 @@ class LegacyErrorResponse(BaseErrorBody):
...,
description="A human-readable message describing the error.",
)
errorCode: str = Field(
..., description="The Opentrons error code associated with the error"
)

@classmethod
def from_exc(
cls: Type["LegacyErrorResponse"], exc: BaseException
) -> "LegacyErrorResponse":
"""Build a response from an exception, preserving some detail."""
if not isinstance(exc, EnumeratedError):
checked_exc: EnumeratedError = PythonException(exc)
else:
checked_exc = exc

return cls(
message=checked_exc.message.strip(), errorCode=checked_exc.code.value.code
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to .strip() the message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for a reason I can't be bothered with it comes with a trailing newline

)


class ErrorBody(BaseErrorBody, GenericModel, Generic[ErrorDetailsT]):
"""A response body for a single error."""

errors: Tuple[ErrorDetailsT] = Field(..., description="Error details.")
errors: Sequence[ErrorDetailsT] = Field(..., description="Error details.")
links: Optional[ResourceLinks] = Field(
None,
description=(
Expand Down
48 changes: 31 additions & 17 deletions robot-server/robot_server/errors/exception_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
from fastapi.responses import JSONResponse
from fastapi.exceptions import RequestValidationError
from starlette.exceptions import HTTPException as StarletteHTTPException
from traceback import format_exception, format_exception_only
from typing import Any, Callable, Coroutine, Dict, Optional, Sequence, Type, Union

from opentrons_shared_data.errors import ErrorCodes, EnumeratedError, PythonException

from robot_server.versioning import (
API_VERSION,
MIN_API_VERSION,
Expand Down Expand Up @@ -38,6 +39,18 @@
log = getLogger(__name__)


def _code_or_default(exc: BaseException) -> str:
if isinstance(exc, EnumeratedError):
# For a reason I cannot fathom, mypy thinks this is an Any.
# reveal_type(exc) # -> opentrons_shared_data.errors.exceptions.EnumeratedError
# reveal_type(exc.code) # -> opentrons_shared_data.errors.codes.ErrorCodes
# reveal_type(exc.code.value) # Any (????????????)
# This doesn't happen anywhere else or indeed in the else side of this clause.
return exc.code.value.code # type: ignore [no-any-return]
else:
return ErrorCodes.GENERAL_ERROR.value.code


def _route_is_legacy(request: Request) -> bool:
"""Check if router handling the request is a legacy v1 endpoint."""
router = request.scope.get("router")
Expand Down Expand Up @@ -95,7 +108,9 @@ async def handle_framework_error(
) -> JSONResponse:
"""Map an HTTP exception from the framework to an API response."""
if _route_is_legacy(request):
response: BaseErrorBody = LegacyErrorResponse(message=error.detail)
response: BaseErrorBody = LegacyErrorResponse(
message=error.detail, errorCode=ErrorCodes.GENERAL_ERROR.value.code
)
else:
response = BadRequest(detail=error.detail)

Expand All @@ -114,7 +129,9 @@ async def handle_validation_error(
f"{'.'.join([str(v) for v in val_error['loc']])}: {val_error['msg']}"
for val_error in validation_errors
)
response: BaseErrorBody = LegacyErrorResponse(message=message)
response: BaseErrorBody = LegacyErrorResponse(
message=message, errorCode=ErrorCodes.GENERAL_ERROR.value.code
)
else:
response = MultiErrorResponse(
errors=[
Expand All @@ -132,17 +149,19 @@ async def handle_validation_error(
)


async def handle_unexpected_error(request: Request, error: Exception) -> JSONResponse:
async def handle_unexpected_error(
request: Request, error: BaseException
) -> JSONResponse:
"""Map an unhandled Exception to an API response."""
detail = "".join(format_exception_only(type(error), error)).strip()
stacktrace = "".join(
format_exception(type(error), error, error.__traceback__, limit=-5)
).strip()
if isinstance(error, EnumeratedError):
enumerated: EnumeratedError = error
else:
enumerated = PythonException(error)

if _route_is_legacy(request):
response: BaseErrorBody = LegacyErrorResponse(message=detail)
response: BaseErrorBody = LegacyErrorResponse.from_exc(enumerated)
else:
response = UnexpectedError(detail=detail, meta={"stacktrace": stacktrace})
response = UnexpectedError.from_exc(enumerated)

return await handle_api_error(
request,
Expand All @@ -154,15 +173,10 @@ async def handle_firmware_upgrade_required_error(
request: Request, error: HWFirmwareUpdateRequired
) -> JSONResponse:
"""Map a FirmwareUpdateRequired error from hardware to an API response."""
detail = "".join(
format_exception(type(error), error, error.__traceback__, limit=0)
).strip()
if _route_is_legacy(request):
response: BaseErrorBody = LegacyErrorResponse(message=detail)
response: BaseErrorBody = LegacyErrorResponse.from_exc(error)
else:
response = FirmwareUpdateRequired(
detail=detail, meta={"status_url": "/subsystems/status"}
)
response = FirmwareUpdateRequired.from_exc(error)
return await handle_api_error(
request, response.as_error(status.HTTP_503_SERVICE_UNAVAILABLE)
)
Expand Down
22 changes: 22 additions & 0 deletions robot-server/robot_server/errors/global_errors.py
Original file line number Diff line number Diff line change
@@ -1,39 +1,61 @@
"""Global error types."""
from typing_extensions import Literal
from typing import Type, Any

from .error_responses import ErrorDetails
from opentrons_shared_data.errors import ErrorCodes


class UnexpectedError(ErrorDetails):
"""An error returned when an unhandled exception occurs."""

id: Literal["UnexpectedError"] = "UnexpectedError"
title: str = "Unexpected Internal Error"
errorCode: str = ErrorCodes.GENERAL_ERROR.value.code


class BadRequest(ErrorDetails):
"""An error returned when the framework rejects the request."""

id: Literal["BadRequest"] = "BadRequest"
title: str = "Bad Request"
errorCode: str = ErrorCodes.GENERAL_ERROR.value.code


class InvalidRequest(ErrorDetails):
"""An error returned when the request fails validation."""

id: Literal["InvalidRequest"] = "InvalidRequest"
title: str = "Invalid Request"
errorCode: str = ErrorCodes.GENERAL_ERROR.value.code


class IDNotFound(ErrorDetails):
"""An error returned when an ID is specified incorrectly."""

id: Literal["IDNotFound"] = "IDNotFound"
title: str = "ID Not Found"
errorCode: str = ErrorCodes.GENERAL_ERROR.value.code


class FirmwareUpdateRequired(ErrorDetails):
"""An error returned when a command requests to interact with hardware that requires an update."""

id: Literal["FirmwareUpdateRequired"] = "FirmwareUpdateRequired"
title: str = "Firmware Update Required"
errorCode: str = ErrorCodes.FIRMWARE_UPDATE_REQUIRED.value.code

@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
Comment on lines +48 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

9 changes: 9 additions & 0 deletions robot-server/robot_server/robot/calibration/errors.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from http import HTTPStatus

from opentrons_shared_data.errors import ErrorCodes
from robot_server.service.errors import ErrorDef, ErrorCreateDef


Expand All @@ -8,40 +9,48 @@ class CalibrationError(ErrorDef):
status_code=HTTPStatus.FORBIDDEN,
title="No Pipette Attached",
format_string="No pipette present on {mount} mount",
error_code=ErrorCodes.PIPETTE_NOT_PRESENT.value.code,
)
NO_PIPETTE_ATTACHED = ErrorCreateDef(
status_code=HTTPStatus.FORBIDDEN,
title="No Pipette Attached",
format_string="Cannot start {flow} with fewer than one pipette",
error_code=ErrorCodes.PIPETTE_NOT_PRESENT.value.code,
)
BAD_LABWARE_DEF = ErrorCreateDef(
status_code=HTTPStatus.UNPROCESSABLE_ENTITY,
title="Bad Labware Definition",
format_string="Bad definition for tip rack under calibration",
error_code=ErrorCodes.GENERAL_ERROR.value.code,
)
BAD_STATE_TRANSITION = ErrorCreateDef(
status_code=HTTPStatus.CONFLICT,
title="Illegal State Transition",
format_string="The action {action} may not occur in the state {state}",
error_code=ErrorCodes.GENERAL_ERROR.value.code,
)
NO_STATE_TRANSITION = ErrorCreateDef(
status_code=HTTPStatus.CONFLICT,
title="No State Transition",
format_string="No transition available for state {state}",
error_code=ErrorCodes.GENERAL_ERROR.value.code,
)
UNMET_STATE_TRANSITION_REQ = ErrorCreateDef(
status_code=HTTPStatus.CONFLICT,
title="Unmet State Transition Requirement",
format_string="The command handler {handler} may not occur in the"
' state {state} when "{condition}" is not true',
error_code=ErrorCodes.GENERAL_ERROR.value.code,
)
UNCALIBRATED_ROBOT = ErrorCreateDef(
status_code=HTTPStatus.CONFLICT,
title="No Calibration Data Found",
format_string="Cannot start {flow} without robot calibration",
error_code=ErrorCodes.GENERAL_ERROR.value.code,
)
ERROR_DURING_TRANSITION = ErrorCreateDef(
status_code=HTTPStatus.INTERNAL_SERVER_ERROR,
title="Error During State Transition",
format_string="Event {action} failed to transition " "from {state}: {error}",
error_code=ErrorCodes.GENERAL_ERROR.value.code,
)
6 changes: 2 additions & 4 deletions robot-server/robot_server/runs/router/actions_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,10 @@ async def create_run_action(
)

except RunActionNotAllowedError as e:
raise RunActionNotAllowed(detail=str(e)).as_error(
status.HTTP_409_CONFLICT
) from e
raise RunActionNotAllowed.from_exc(e).as_error(status.HTTP_409_CONFLICT) from e

except RunNotFoundError as e:
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
Comment on lines -124 to +122
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, and it's worse in some other places like the exception handlers, where we

  1. catch an exception
  2. build a model from it
  3. create an exception from the model with as_error
  4. pass that somewhere else
  5. ultimately put the exception instance's .content in the response

What I'd really prefer is that exceptions get turned into models and models get sent, without going back into ApiErrors.


return await PydanticResponse.create(
content=SimpleBody.construct(data=action),
Expand Down
Loading