fix(api): properly handle non-labware jsons in sim #17198
Conversation
opentrons_simulate tries to see if any json file it can reach is a labware and load it if so. We recently changed the exceptions we raise in verify_labware_definitions if that call fails, so now instead of ignoring the file on exception opentrons_simulate raises the error through and terminates. No good! Change the exception to something a little saner and make sure we can actually ignore invalid labware files in sim. Closes RQA-3820
| try: | ||
| to_return = json.loads(contents) | ||
| except json.JSONDecodeError as e: | ||
| raise NotALabwareError("invalid-json", [e]) |
There was a problem hiding this comment.
Nitpick: from e so the tracebacks reflect that we're deliberately converting exception A to exception B, instead of encountering an unexpected exception B while we're trying to handle exception A.
| raise NotALabwareError("invalid-json", [e]) | |
| raise NotALabwareError("invalid-json", [e]) from e |
from None instead of from e would also make sense here. Just a matter of taste I think.
There was a problem hiding this comment.
oh whoops yeah good call
| try: | ||
| schema_version = to_return["schemaVersion"] | ||
| except KeyError as e: | ||
| raise NotALabwareError("no-schema-id", [e]) |
There was a problem hiding this comment.
| raise NotALabwareError("no-schema-id", [e]) | |
| raise NotALabwareError("no-schema-id", [e]) from e |
| ) | ||
| jsonschema.validate(to_return, schema) | ||
| except KeyError as e: | ||
| raise NotALabwareError("bad-schema-id", [e]) |
There was a problem hiding this comment.
| raise NotALabwareError("bad-schema-id", [e]) | |
| raise NotALabwareError("bad-schema-id", [e]) from e |
| wrapping: Optional[Sequence[EnumeratedError]] = None, | ||
| wrapping: Optional[Sequence[Union[EnumeratedError, BaseException]]] = None, |
There was a problem hiding this comment.
Not for this PR, but I think we're losing the plot on wrapping. If wrapping includes BaseException then I think it is strictly a reinvention of the standard __cause__ and __context__ mechanisms, and we should transition to using those instead.
There was a problem hiding this comment.
it has always included those actually, we've just been uneven about propagating the type signature down to things that inherit from the general exception. and while it does do basically the same thing as cause and context, it also represents a specific point at which we leave native exception land and transition into ours.
| try: | ||
| jsonschema.validate(to_return, schema) | ||
| except jsonschema.ValidationError as e: | ||
| raise NotALabwareError("schema-mismatch", [e]) |
There was a problem hiding this comment.
| raise NotALabwareError("schema-mismatch", [e]) | |
| raise NotALabwareError("schema-mismatch", [e]) from e |
| except (ValidationError, JSONDecodeError): | ||
| except labware.NotALabwareError: |
There was a problem hiding this comment.
Much better, thank you.
opentrons_simulate tries to see if any json file it can reach is a labware and load it if so. We recently changed the exceptions we raise in verify_labware_definitions if that call fails, so now instead of ignoring the file on exception opentrons_simulate raises the error through and terminates. No good!
Change the exception to something a little saner and make sure we can actually ignore invalid labware files in sim.
Closes RQA-3820