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

fix(api): properly handle non-labware jsons in sim #17198

Merged
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
45 changes: 37 additions & 8 deletions api/src/opentrons/protocols/labware.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import json
import os
from pathlib import Path
from typing import Any, AnyStr, Dict, Optional, Union, List
from typing import Any, AnyStr, Dict, Optional, Union, List, Sequence, Literal

import jsonschema # type: ignore

Expand All @@ -17,10 +17,30 @@
USER_DEFS_PATH,
)
from opentrons_shared_data.labware.types import LabwareDefinition
from opentrons_shared_data.errors.exceptions import InvalidProtocolData


MODULE_LOG = logging.getLogger(__name__)

LabwareProblem = Literal[
"no-schema-id", "bad-schema-id", "schema-mismatch", "invalid-json"
]


class NotALabwareError(InvalidProtocolData):
def __init__(
self, problem: LabwareProblem, wrapping: Sequence[BaseException]
) -> None:
messages: dict[LabwareProblem, str] = {
"no-schema-id": "No schema ID present in file",
"bad-schema-id": "Bad schema ID in file",
"invalid-json": "File does not contain valid JSON",
"schema-mismatch": "File does not match labware schema",
}
super().__init__(
message=messages[problem], detail={"kind": problem}, wrapping=wrapping
)


def get_labware_definition(
load_name: str,
Expand Down Expand Up @@ -126,7 +146,7 @@ def save_definition(
json.dump(labware_def, f)


def verify_definition(
def verify_definition( # noqa: C901
contents: Union[AnyStr, LabwareDefinition, Dict[str, Any]]
) -> LabwareDefinition:
"""Verify that an input string is a labware definition and return it.
Expand All @@ -146,15 +166,24 @@ def verify_definition(
if isinstance(contents, dict):
to_return = contents
else:
to_return = json.loads(contents)
try:
to_return = json.loads(contents)
except json.JSONDecodeError as e:
raise NotALabwareError("invalid-json", [e]) from e
try:
schema_version = to_return["schemaVersion"]
except KeyError as e:
raise NotALabwareError("no-schema-id", [e]) from e

try:
schema = schemata_by_version[schema_version]
except KeyError:
raise RuntimeError(
f'Invalid or unknown labware schema version {to_return.get("schemaVersion", None)}'
)
jsonschema.validate(to_return, schema)
except KeyError as e:
raise NotALabwareError("bad-schema-id", [e]) from e

try:
jsonschema.validate(to_return, schema)
except jsonschema.ValidationError as e:
raise NotALabwareError("schema-mismatch", [e]) from e

# we can type ignore this because if it passes the jsonschema it has
# the correct structure
Expand Down
7 changes: 2 additions & 5 deletions api/src/opentrons/util/entrypoint_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from dataclasses import dataclass
import json
import logging
from json import JSONDecodeError
import pathlib
import subprocess
import sys
Expand All @@ -21,8 +20,6 @@
TYPE_CHECKING,
)

from jsonschema import ValidationError # type: ignore

from opentrons.calibration_storage.deck_configuration import (
deserialize_deck_configuration,
)
Expand All @@ -32,7 +29,7 @@
JUPYTER_NOTEBOOK_LABWARE_DIR,
SystemArchitecture,
)
from opentrons.protocol_api import labware
from opentrons.protocols import labware
from opentrons.calibration_storage import helpers
from opentrons.protocol_engine.errors.error_occurrence import (
ErrorOccurrence as ProtocolEngineErrorOccurrence,
Expand Down Expand Up @@ -79,7 +76,7 @@ def labware_from_paths(
if child.is_file() and child.suffix.endswith("json"):
try:
defn = labware.verify_definition(child.read_bytes())
except (ValidationError, JSONDecodeError):
except labware.NotALabwareError:
Comment on lines -82 to +79
Copy link
Contributor

Choose a reason for hiding this comment

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

Much better, thank you.

log.info(f"{child}: invalid labware, ignoring")
log.debug(
f"{child}: labware invalid because of this exception.",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""Exception hierarchy for error codes."""

from typing import Dict, Any, Optional, List, Iterator, Union, Sequence, overload
from logging import getLogger
from traceback import format_exception_only, format_tb
Expand Down Expand Up @@ -1099,7 +1100,7 @@ def __init__(
self,
message: Optional[str] = None,
detail: Optional[Dict[str, str]] = None,
wrapping: Optional[Sequence[EnumeratedError]] = None,
wrapping: Optional[Sequence[Union[EnumeratedError, BaseException]]] = None,
Comment on lines -1102 to +1103
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

) -> None:
"""Build an InvalidProtocolData."""
super().__init__(ErrorCodes.INVALID_PROTOCOL_DATA, message, detail, wrapping)
Expand Down
Loading