From 815c38c9cf28145e9df1d3ab8fa57857decee6d7 Mon Sep 17 00:00:00 2001 From: Jeremy Leon Date: Mon, 12 Aug 2024 11:46:40 -0400 Subject: [PATCH] refactor(api, robot-server): add error for multiple CSV definitions and less reliance on file reading/creating (#15956) # Overview This PR follows up both #15855 and #15907 with further refinements and small changes to defining and setting CSV parameters, along with behind the scenes refactors. The two main user/client facing changes of this PR are changing the HTTP status code when invalid data file IDs are sent to the `POST` `/runs`, `/protocols` and `/protocols/{protocolId}/analyses` endpoints from 400 to 422, and raising a new `IncompatibleParameterError` when multiple CSV file parameters are added to a single protocol. The change in status code is to reflect that the error is not because of a malformed request, but because there was something in the request that could not be processed (in this case being the data file ID). The new error was added to align with the original PRD and the current UI. While there's no inherent technical limitation for multiple CSV parameters (other than the theoretical limitations with memory, number of file handlers, etc), there are unexpected UI bugs when multiple ones are defined and this change enforces only one per protocol. The other major change of this PR is a non-user facing refactor of how we set the `CSVParameter` interface. Rather than passing the `Path` of the file to the interface and then using a `TemporaryFile` as the basis for all further access of the CSV file and it's contents, now the contents of the file (passed as `bytes`) is what everything else is based off of, including CSV parsing in `parse_as_csv`. With this change, we only ever make a temporary file handler when the user accesses `.file`. With this change reducing the chance of there being an open file handler, a new `file_opened` property was added to the public interface to reduce needless file opening. This is technically user-facing code meant more for internal usage, but I see no danger in exposing it, though if needed we can tag it is a private non-guaranteed method. ## Changelog - Raise `IncompatibleParameterError` when `add_csv_file` in the `ParameterContext` is used more than once - Change status code of invalid data file IDs posted to runs and protocols endpoints from `400` to `422` - Refactor `CSVParameter` to only open temporary file handler when user requests one ## Review requests Should we label `file_opened` as private/use another way of preventing spurious temporary file creation just to close them? ## Risk assessment Low. --- .../protocol_api/_parameter_context.py | 14 ++- .../opentrons/protocols/execution/execute.py | 10 +- .../parameters/csv_parameter_definition.py | 13 +- .../parameters/csv_parameter_interface.py | 48 +++++--- .../protocols/parameters/exceptions.py | 4 + .../parameters/parameter_file_reader.py | 26 ---- .../opentrons/protocols/parameters/types.py | 3 +- .../protocols/parameters/validation.py | 4 +- .../protocol_api/test_parameter_context.py | 23 +++- .../test_csv_parameter_definition.py | 15 ++- .../test_csv_parameter_interface.py | 115 +++++++----------- .../parameters/test_parameter_file_reader.py | 34 ------ robot-server/robot_server/protocols/router.py | 15 ++- .../robot_server/runs/router/base_router.py | 6 +- 14 files changed, 144 insertions(+), 186 deletions(-) delete mode 100644 api/src/opentrons/protocols/parameters/parameter_file_reader.py delete mode 100644 api/tests/opentrons/protocols/parameters/test_parameter_file_reader.py diff --git a/api/src/opentrons/protocol_api/_parameter_context.py b/api/src/opentrons/protocol_api/_parameter_context.py index 8ca2bdb2c2a..2e0e0096f44 100644 --- a/api/src/opentrons/protocol_api/_parameter_context.py +++ b/api/src/opentrons/protocol_api/_parameter_context.py @@ -15,6 +15,7 @@ from opentrons.protocols.parameters.exceptions import ( ParameterDefinitionError, ParameterValueError, + IncompatibleParameterError, ) from opentrons.protocol_engine.types import ( RunTimeParameter, @@ -183,6 +184,14 @@ def add_csv_file( variable_name: The variable name the CSV parameter will be referred to in the run context. description: A description of the parameter as it will show up on the frontend. """ + if any( + isinstance(parameter, csv_parameter_definition.CSVParameterDefinition) + for parameter in self._parameters.values() + ): + raise IncompatibleParameterError( + "Only one CSV File parameter can be defined per protocol." + ) + validation.validate_variable_name_unique(variable_name, set(self._parameters)) parameter = csv_parameter_definition.create_csv_parameter( display_name=display_name, @@ -246,8 +255,11 @@ def initialize_csv_files( file_id = file_path.parent.name file_name = file_path.name + with file_path.open("rb") as fh: + contents = fh.read() + parameter.file_info = FileInfo(id=file_id, name=file_name) - parameter.value = file_path + parameter.value = contents def export_parameters_for_analysis(self) -> List[RunTimeParameter]: """Exports all parameters into a protocol engine models for reporting in analysis. diff --git a/api/src/opentrons/protocols/execution/execute.py b/api/src/opentrons/protocols/execution/execute.py index ff52b3b1dc5..6dd74bcb778 100644 --- a/api/src/opentrons/protocols/execution/execute.py +++ b/api/src/opentrons/protocols/execution/execute.py @@ -16,7 +16,6 @@ from opentrons.protocols.api_support.types import APIVersion from opentrons.protocols.parameters.csv_parameter_interface import CSVParameter -from opentrons.protocols.parameters.exceptions import RuntimeParameterRequired MODULE_LOG = logging.getLogger(__name__) @@ -51,13 +50,8 @@ def run_protocol( finally: if protocol.api_level >= APIVersion(2, 18): for parameter in context.params.get_all().values(): - if isinstance(parameter, CSVParameter): - try: - parameter.file.close() - # This will be raised if the csv file wasn't set, which means it was never opened, - # so we can safely skip this. - except RuntimeParameterRequired: - pass + if isinstance(parameter, CSVParameter) and parameter.file_opened: + parameter.file.close() else: if protocol.contents["schemaVersion"] == 3: ins = execute_json_v3.load_pipettes_from_json(context, protocol.contents) diff --git a/api/src/opentrons/protocols/parameters/csv_parameter_definition.py b/api/src/opentrons/protocols/parameters/csv_parameter_definition.py index d23b7d70f0b..c48356d01d4 100644 --- a/api/src/opentrons/protocols/parameters/csv_parameter_definition.py +++ b/api/src/opentrons/protocols/parameters/csv_parameter_definition.py @@ -1,5 +1,4 @@ """CSV Parameter definition and associated classes/functions.""" -from pathlib import Path from typing import Optional from opentrons.protocol_engine.types import ( @@ -14,7 +13,7 @@ from .csv_parameter_interface import CSVParameter -class CSVParameterDefinition(AbstractParameterDefinition[Optional[Path]]): +class CSVParameterDefinition(AbstractParameterDefinition[Optional[bytes]]): """The definition for a user defined CSV file parameter.""" def __init__( @@ -30,7 +29,7 @@ def __init__( self._display_name = validation.ensure_display_name(display_name) self._variable_name = validation.ensure_variable_name(variable_name) self._description = validation.ensure_description(description) - self._value: Optional[Path] = None + self._value: Optional[bytes] = None self._file_info: Optional[FileInfo] = None @property @@ -39,13 +38,13 @@ def variable_name(self) -> str: return self._variable_name @property - def value(self) -> Optional[Path]: + def value(self) -> Optional[bytes]: """The current set file for the CSV parameter. Defaults to None on definition creation.""" return self._value @value.setter - def value(self, new_path: Path) -> None: - self._value = new_path + def value(self, contents: bytes) -> None: + self._value = contents @property def file_info(self) -> Optional[FileInfo]: @@ -56,7 +55,7 @@ def file_info(self, file_info: FileInfo) -> None: self._file_info = file_info def as_csv_parameter_interface(self, api_version: APIVersion) -> CSVParameter: - return CSVParameter(csv_path=self._value, api_version=api_version) + return CSVParameter(contents=self._value, api_version=api_version) def as_protocol_engine_type(self) -> RunTimeParameter: """Returns CSV parameter as a Protocol Engine type to send to client.""" diff --git a/api/src/opentrons/protocols/parameters/csv_parameter_interface.py b/api/src/opentrons/protocols/parameters/csv_parameter_interface.py index 40a099558d4..20627322547 100644 --- a/api/src/opentrons/protocols/parameters/csv_parameter_interface.py +++ b/api/src/opentrons/protocols/parameters/csv_parameter_interface.py @@ -1,35 +1,46 @@ import csv -from pathlib import Path +from tempfile import NamedTemporaryFile from typing import Optional, TextIO, Any, List from opentrons.protocols.api_support.types import APIVersion -from . import parameter_file_reader -from .exceptions import ParameterValueError +from .exceptions import ParameterValueError, RuntimeParameterRequired # TODO(jbl 2024-08-02) This is a public facing class and as such should be moved to the protocol_api folder class CSVParameter: - def __init__(self, csv_path: Optional[Path], api_version: APIVersion) -> None: - self._path = csv_path - self._file: Optional[TextIO] = None - self._contents: Optional[str] = None + def __init__(self, contents: Optional[bytes], api_version: APIVersion) -> None: + self._contents = contents self._api_version = api_version + self._file: Optional[TextIO] = None @property def file(self) -> TextIO: """Returns the file handler for the CSV file.""" if self._file is None: - self._file = parameter_file_reader.open_file_path(self._path) + text = self.contents + temporary_file = NamedTemporaryFile("r+") + temporary_file.write(text) + temporary_file.flush() + + # Open a new file handler for the temporary file with read-only permissions and close the other + self._file = open(temporary_file.name, "r") + temporary_file.close() return self._file + @property + def file_opened(self) -> bool: + """Return if a file handler has been opened for the CSV parameter.""" + return self._file is not None + @property def contents(self) -> str: """Returns the full contents of the CSV file as a single string.""" if self._contents is None: - self.file.seek(0) - self._contents = self.file.read() - return self._contents + raise RuntimeParameterRequired( + "CSV parameter needs to be set to a file for full analysis or run." + ) + return self._contents.decode("utf-8") def parse_as_csv( self, detect_dialect: bool = True, **kwargs: Any @@ -42,23 +53,20 @@ def parse_as_csv( rows: List[List[str]] = [] if detect_dialect: try: - self.file.seek(0) - dialect = csv.Sniffer().sniff(self.file.read(1024)) - self.file.seek(0) - reader = csv.reader(self.file, dialect, **kwargs) + dialect = csv.Sniffer().sniff(self.contents[:1024]) + reader = csv.reader(self.contents.split("\n"), dialect, **kwargs) except (UnicodeDecodeError, csv.Error): raise ParameterValueError( - "Cannot parse dialect or contents from provided CSV file." + "Cannot parse dialect or contents from provided CSV contents." ) else: try: - reader = csv.reader(self.file, **kwargs) + reader = csv.reader(self.contents.split("\n"), **kwargs) except (UnicodeDecodeError, csv.Error): - raise ParameterValueError("Cannot parse provided CSV file.") + raise ParameterValueError("Cannot parse provided CSV contents.") try: for row in reader: rows.append(row) except (UnicodeDecodeError, csv.Error): - raise ParameterValueError("Cannot parse provided CSV file.") - self.file.seek(0) + raise ParameterValueError("Cannot parse provided CSV contents.") return rows diff --git a/api/src/opentrons/protocols/parameters/exceptions.py b/api/src/opentrons/protocols/parameters/exceptions.py index 9f7bcee009c..7f1b8a933cb 100644 --- a/api/src/opentrons/protocols/parameters/exceptions.py +++ b/api/src/opentrons/protocols/parameters/exceptions.py @@ -28,3 +28,7 @@ class ParameterDefinitionError(ValueError): class ParameterNameError(ValueError): """An error raised when a parameter name or description is not valid.""" + + +class IncompatibleParameterError(ValueError): + """An error raised when a parameter conflicts with another parameter.""" diff --git a/api/src/opentrons/protocols/parameters/parameter_file_reader.py b/api/src/opentrons/protocols/parameters/parameter_file_reader.py deleted file mode 100644 index 9a39c2fa0dc..00000000000 --- a/api/src/opentrons/protocols/parameters/parameter_file_reader.py +++ /dev/null @@ -1,26 +0,0 @@ -from pathlib import Path -from tempfile import NamedTemporaryFile -from typing import Optional, TextIO - -from .exceptions import RuntimeParameterRequired - - -def open_file_path(file_path: Optional[Path]) -> TextIO: - """Ensure file path is set and open up the file in a safe read-only temporary file.""" - if file_path is None: - raise RuntimeParameterRequired( - "CSV parameter needs to be set to a file for full analysis or run." - ) - # Read the contents of the actual file - with file_path.open() as fh: - contents = fh.read() - - # Open a temporary file with write permissions and write contents to that - temporary_file = NamedTemporaryFile("r+") - temporary_file.write(contents) - temporary_file.flush() - - # Open a new file handler for the temporary file with read-only permissions and close the other - read_only_temp_file = open(temporary_file.name, "r") - temporary_file.close() - return read_only_temp_file diff --git a/api/src/opentrons/protocols/parameters/types.py b/api/src/opentrons/protocols/parameters/types.py index 631d686b7e7..0e248d8e1c0 100644 --- a/api/src/opentrons/protocols/parameters/types.py +++ b/api/src/opentrons/protocols/parameters/types.py @@ -1,11 +1,10 @@ -from pathlib import Path from typing import TypeVar, Union, TypedDict from .csv_parameter_interface import CSVParameter PrimitiveAllowedTypes = Union[str, int, float, bool] -AllAllowedTypes = Union[str, int, float, bool, Path, None] +AllAllowedTypes = Union[str, int, float, bool, bytes, None] UserFacingTypes = Union[str, int, float, bool, CSVParameter] ParamType = TypeVar("ParamType", bound=AllAllowedTypes) diff --git a/api/src/opentrons/protocols/parameters/validation.py b/api/src/opentrons/protocols/parameters/validation.py index 68a1d93ec97..d111e1cc8a8 100644 --- a/api/src/opentrons/protocols/parameters/validation.py +++ b/api/src/opentrons/protocols/parameters/validation.py @@ -237,7 +237,7 @@ def validate_type(value: ParamType, parameter_type: type) -> None: """Validate parameter value is the correct type.""" if not isinstance(value, parameter_type): raise ParameterValueError( - f"Parameter value {value} has type '{type(value).__name__}'," + f"Parameter value {value!r} has type '{type(value).__name__}'," f" but must be of type '{parameter_type.__name__}'." ) @@ -252,7 +252,7 @@ def validate_options( """Validate default values and all possible constraints for a valid parameter definition.""" if not isinstance(default, parameter_type): raise ParameterValueError( - f"Parameter default {default} has type '{type(default).__name__}'," + f"Parameter default {default!r} has type '{type(default).__name__}'," f" but must be of type '{parameter_type.__name__}'." ) diff --git a/api/tests/opentrons/protocol_api/test_parameter_context.py b/api/tests/opentrons/protocol_api/test_parameter_context.py index e2004a4b9b7..f59521c78a9 100644 --- a/api/tests/opentrons/protocol_api/test_parameter_context.py +++ b/api/tests/opentrons/protocol_api/test_parameter_context.py @@ -13,7 +13,10 @@ parameter_definition as mock_parameter_definition, validation as mock_validation, ) -from opentrons.protocols.parameters.exceptions import ParameterDefinitionError +from opentrons.protocols.parameters.exceptions import ( + ParameterDefinitionError, + IncompatibleParameterError, +) from opentrons.protocol_engine.types import BooleanParameter from opentrons.protocol_api._parameter_context import ParameterContext @@ -196,7 +199,7 @@ def test_add_string(decoy: Decoy, subject: ParameterContext) -> None: def test_add_csv(decoy: Decoy, subject: ParameterContext) -> None: """It should create and add a CSV parameter definition.""" subject._parameters["other_param"] = decoy.mock( - cls=mock_csv_parameter_definition.CSVParameterDefinition + cls=mock_parameter_definition.ParameterDefinition ) param_def = decoy.mock(cls=mock_csv_parameter_definition.CSVParameterDefinition) decoy.when(param_def.variable_name).then_return("my potentially cool variable") @@ -220,6 +223,22 @@ def test_add_csv(decoy: Decoy, subject: ParameterContext) -> None: ) +def test_add_csv_raises_for_multiple_csvs( + decoy: Decoy, subject: ParameterContext +) -> None: + """It should raise with a IncompatibleParameterError if there's already a CSV parameter..""" + subject._parameters["other_param"] = decoy.mock( + cls=mock_csv_parameter_definition.CSVParameterDefinition + ) + + with pytest.raises(IncompatibleParameterError): + subject.add_csv_file( + display_name="jkl", + variable_name="qwerty", + description="fee foo fum", + ) + + def test_set_parameters(decoy: Decoy, subject: ParameterContext) -> None: """It should set the parameter values.""" param_def = decoy.mock(cls=mock_parameter_definition.ParameterDefinition) diff --git a/api/tests/opentrons/protocols/parameters/test_csv_parameter_definition.py b/api/tests/opentrons/protocols/parameters/test_csv_parameter_definition.py index 04dee0512b5..ac8da823edb 100644 --- a/api/tests/opentrons/protocols/parameters/test_csv_parameter_definition.py +++ b/api/tests/opentrons/protocols/parameters/test_csv_parameter_definition.py @@ -1,6 +1,5 @@ """Tests for the CSV Parameter Definitions.""" import inspect -from pathlib import Path import pytest from decoy import Decoy @@ -62,9 +61,9 @@ def test_create_csv_parameter(decoy: Decoy) -> None: def test_set_csv_value( decoy: Decoy, csv_parameter_subject: CSVParameterDefinition ) -> None: - """It should set the CSV parameter value to a path.""" - csv_parameter_subject.value = Path("123") - assert csv_parameter_subject.value == Path("123") + """It should set the CSV parameter value to a byte string.""" + csv_parameter_subject.value = b"123" + assert csv_parameter_subject.value == b"123" def test_csv_parameter_as_protocol_engine_type( @@ -79,13 +78,13 @@ def test_csv_parameter_as_protocol_engine_type( file=None, ) - csv_parameter_subject.file_info = FileInfo(id="123abc", name="") + csv_parameter_subject.file_info = FileInfo(id="123", name="abc") result = csv_parameter_subject.as_protocol_engine_type() assert result == CSVParameter( displayName="My cool CSV", variableName="my_cool_csv", description="Comma Separated Value", - file=FileInfo(id="123abc", name=""), + file=FileInfo(id="123", name="abc"), ) @@ -98,6 +97,6 @@ def test_csv_parameter_as_csv_parameter_interface( with pytest.raises(RuntimeParameterRequired): result.file - csv_parameter_subject.value = Path("abc") + csv_parameter_subject.value = b"abc" result = csv_parameter_subject.as_csv_parameter_interface(api_version) - assert result._path == Path("abc") + assert result.contents == "abc" diff --git a/api/tests/opentrons/protocols/parameters/test_csv_parameter_interface.py b/api/tests/opentrons/protocols/parameters/test_csv_parameter_interface.py index 4cd9e649b63..81bffd0028e 100644 --- a/api/tests/opentrons/protocols/parameters/test_csv_parameter_interface.py +++ b/api/tests/opentrons/protocols/parameters/test_csv_parameter_interface.py @@ -1,26 +1,13 @@ import pytest -import inspect +import platform from decoy import Decoy from pytest_lazyfixture import lazy_fixture # type: ignore[import-untyped] -import tempfile -from pathlib import Path -from typing import TextIO, Generator - from opentrons.protocols.api_support.types import APIVersion from opentrons.protocols.api_support.definitions import MAX_SUPPORTED_VERSION -from opentrons.protocols.parameters import ( - parameter_file_reader as mock_param_file_reader, -) from opentrons.protocols.parameters.csv_parameter_interface import CSVParameter -@pytest.fixture(autouse=True) -def _patch_parameter_file_reader(decoy: Decoy, monkeypatch: pytest.MonkeyPatch) -> None: - for name, func in inspect.getmembers(mock_param_file_reader, inspect.isfunction): - monkeypatch.setattr(mock_param_file_reader, name, decoy.mock(func=func)) - - @pytest.fixture def api_version() -> APIVersion: """The API version under test.""" @@ -28,70 +15,54 @@ def api_version() -> APIVersion: @pytest.fixture -def csv_file_basic() -> Generator[TextIO, None, None]: +def csv_file_basic() -> bytes: """A basic CSV file with quotes around strings.""" - with tempfile.TemporaryFile("r+") as temp_file: - contents = '"x","y","z"\n"a",1,2\n"b",3,4\n"c",5,6' - temp_file.write(contents) - temp_file.seek(0) - yield temp_file + return b'"x","y","z"\n"a",1,2\n"b",3,4\n"c",5,6' @pytest.fixture -def csv_file_no_quotes() -> Generator[TextIO, None, None]: +def csv_file_no_quotes() -> bytes: """A basic CSV file with no quotes around strings.""" - with tempfile.TemporaryFile("r+") as temp_file: - contents = "x,y,z\na,1,2\nb,3,4\nc,5,6" - temp_file.write(contents) - temp_file.seek(0) - yield temp_file + return b"x,y,z\na,1,2\nb,3,4\nc,5,6" @pytest.fixture -def csv_file_preceding_spaces() -> Generator[TextIO, None, None]: +def csv_file_preceding_spaces() -> bytes: """A basic CSV file with quotes around strings and spaces preceding non-initial columns.""" - with tempfile.TemporaryFile("r+") as temp_file: - contents = '"x", "y", "z"\n"a", 1, 2\n"b", 3, 4\n"c", 5, 6' - temp_file.write(contents) - temp_file.seek(0) - yield temp_file + return b'"x", "y", "z"\n"a", 1, 2\n"b", 3, 4\n"c", 5, 6' @pytest.fixture -def csv_file_mixed_quotes() -> Generator[TextIO, None, None]: +def csv_file_mixed_quotes() -> bytes: """A basic CSV file with both string quotes and escaped quotes.""" - with tempfile.TemporaryFile("r+") as temp_file: - contents = 'head,er\n"a,b,c",def\n"""ghi""","jkl"' - temp_file.write(contents) - temp_file.seek(0) - yield temp_file + return b'head,er\n"a,b,c",def\n"""ghi""","jkl"' @pytest.fixture -def csv_file_different_delimiter() -> Generator[TextIO, None, None]: +def csv_file_different_delimiter() -> bytes: """A basic CSV file with a non-comma delimiter.""" - with tempfile.TemporaryFile("r+") as temp_file: - contents = "x:y:z\na,:1,:2\nb,:3,:4\nc,:5,:6" - temp_file.write(contents) - temp_file.seek(0) - yield temp_file - - -@pytest.fixture -def subject(api_version: APIVersion) -> CSVParameter: - """Return a CSVParameter interface subject.""" - return CSVParameter(csv_path=Path("abc"), api_version=api_version) + return b"x:y:z\na,:1,:2\nb,:3,:4\nc,:5,:6" def test_csv_parameter( - decoy: Decoy, csv_file_basic: TextIO, subject: CSVParameter + decoy: Decoy, api_version: APIVersion, csv_file_basic: bytes ) -> None: """It should load the CSV parameter and provide access to the file, contents, and rows.""" - decoy.when(mock_param_file_reader.open_file_path(Path("abc"))).then_return( - csv_file_basic - ) - assert subject.file is csv_file_basic + subject = CSVParameter(csv_file_basic, api_version) + + # On Windows, you can't open a NamedTemporaryFile a second time, which breaks the code under test. + # Because of the way CSV analysis works this code will only ever be run on the actual OT-2/Flex hardware, + # so we skip testing and instead assert that we get a PermissionError on Windows (to ensure this + # test gets fixed in case we ever refactor the file opening.) + if platform.system() != "Windows": + assert subject.file.readable() + assert not subject.file.writable() + assert subject.file.read() == '"x","y","z"\n"a",1,2\n"b",3,4\n"c",5,6' + else: + with pytest.raises(PermissionError): + subject.file assert subject.contents == '"x","y","z"\n"a",1,2\n"b",3,4\n"c",5,6' + assert subject.parse_as_csv()[0] == ["x", "y", "z"] @pytest.mark.parametrize( @@ -103,22 +74,26 @@ def test_csv_parameter( ], ) def test_csv_parameter_rows( - decoy: Decoy, csv_file: TextIO, subject: CSVParameter + decoy: Decoy, + api_version: APIVersion, + csv_file: bytes, ) -> None: """It should load the rows as all strings even with no quotes or leading spaces.""" - decoy.when(mock_param_file_reader.open_file_path(Path("abc"))).then_return(csv_file) + subject = CSVParameter(csv_file, api_version) + assert len(subject.parse_as_csv()) == 4 assert subject.parse_as_csv()[0] == ["x", "y", "z"] assert subject.parse_as_csv()[1] == ["a", "1", "2"] def test_csv_parameter_mixed_quotes( - decoy: Decoy, csv_file_mixed_quotes: TextIO, subject: CSVParameter + decoy: Decoy, + api_version: APIVersion, + csv_file_mixed_quotes: bytes, ) -> None: """It should load the rows with no quotes, quotes and escaped quotes with double quotes.""" - decoy.when(mock_param_file_reader.open_file_path(Path("abc"))).then_return( - csv_file_mixed_quotes - ) + subject = CSVParameter(csv_file_mixed_quotes, api_version) + assert len(subject.parse_as_csv()) == 3 assert subject.parse_as_csv()[0] == ["head", "er"] assert subject.parse_as_csv()[1] == ["a,b,c", "def"] @@ -126,25 +101,27 @@ def test_csv_parameter_mixed_quotes( def test_csv_parameter_additional_kwargs( - decoy: Decoy, csv_file_different_delimiter: TextIO, subject: CSVParameter + decoy: Decoy, + api_version: APIVersion, + csv_file_different_delimiter: bytes, ) -> None: """It should load the rows with a different delimiter.""" - decoy.when(mock_param_file_reader.open_file_path(Path("abc"))).then_return( - csv_file_different_delimiter - ) + subject = CSVParameter(csv_file_different_delimiter, api_version) rows = subject.parse_as_csv(delimiter=":") + assert len(rows) == 4 assert rows[0] == ["x", "y", "z"] assert rows[1] == ["a,", "1,", "2"] def test_csv_parameter_dont_detect_dialect( - decoy: Decoy, csv_file_preceding_spaces: TextIO, subject: CSVParameter + decoy: Decoy, + api_version: APIVersion, + csv_file_preceding_spaces: bytes, ) -> None: """It should load the rows without trying to detect the dialect.""" - decoy.when(mock_param_file_reader.open_file_path(Path("abc"))).then_return( - csv_file_preceding_spaces - ) + subject = CSVParameter(csv_file_preceding_spaces, api_version) rows = subject.parse_as_csv(detect_dialect=False) + assert rows[0] == ["x", ' "y"', ' "z"'] assert rows[1] == ["a", " 1", " 2"] diff --git a/api/tests/opentrons/protocols/parameters/test_parameter_file_reader.py b/api/tests/opentrons/protocols/parameters/test_parameter_file_reader.py deleted file mode 100644 index d469c827d08..00000000000 --- a/api/tests/opentrons/protocols/parameters/test_parameter_file_reader.py +++ /dev/null @@ -1,34 +0,0 @@ -import pytest -import platform - -from opentrons_shared_data import get_shared_data_root, load_shared_data - -from opentrons.protocols.parameters.exceptions import RuntimeParameterRequired -from opentrons.protocols.parameters import parameter_file_reader as subject - - -def test_open_file_path() -> None: - """It should open a temporary file handler given a path.""" - contents = load_shared_data("protocol/fixtures/7/simpleV7.json") - shared_data_path = get_shared_data_root() / "protocol/fixtures/7/simpleV7.json" - - # On Windows, you can't open a NamedTemporaryFile a second time, which breaks the code under test. - # Because of the way CSV analysis works this code will only ever be run on the actual OT-2/Flex hardware, - # so we skip testing and instead assert that we get a PermissionError on Windows (to ensure this - # test gets fixed in case we ever refactor the file opening.) - if platform.system() != "Windows": - result = subject.open_file_path(shared_data_path) - - assert result.readable() - assert not result.writable() - assert result.read() == contents.decode("utf-8") - result.close() - else: - with pytest.raises(PermissionError): - subject.open_file_path(shared_data_path) - - -def test_open_file_path_raises() -> None: - """It should raise of no file path is provided.""" - with pytest.raises(RuntimeParameterRequired): - subject.open_file_path(None) diff --git a/robot-server/robot_server/protocols/router.py b/robot-server/robot_server/protocols/router.py index 2ea216f9f29..609b1d8e978 100644 --- a/robot-server/robot_server/protocols/router.py +++ b/robot-server/robot_server/protocols/router.py @@ -196,9 +196,10 @@ class ProtocolLinks(BaseModel): responses={ status.HTTP_200_OK: {"model": SimpleBody[Protocol]}, status.HTTP_201_CREATED: {"model": SimpleBody[Protocol]}, - status.HTTP_400_BAD_REQUEST: {"model": ErrorBody[FileIdNotFound]}, status.HTTP_422_UNPROCESSABLE_ENTITY: { - "model": ErrorBody[Union[ProtocolFilesInvalid, ProtocolRobotTypeMismatch]] + "model": ErrorBody[ + Union[ProtocolFilesInvalid, ProtocolRobotTypeMismatch, FileIdNotFound] + ] }, status.HTTP_503_SERVICE_UNAVAILABLE: {"model": ErrorBody[LastAnalysisPending]}, }, @@ -330,7 +331,9 @@ async def create_protocol( # noqa: C901 for name, file_id in parsed_rtp_files.items() } except FileIdNotFoundError as e: - raise FileIdNotFound(detail=str(e)).as_error(status.HTTP_400_BAD_REQUEST) + raise FileIdNotFound(detail=str(e)).as_error( + status.HTTP_422_UNPROCESSABLE_ENTITY + ) content_hash = await file_hasher.hash(buffered_files) cached_protocol_id = protocol_store.get_id_by_hash(content_hash) @@ -702,8 +705,8 @@ async def delete_protocol_by_id( responses={ status.HTTP_200_OK: {"model": SimpleMultiBody[AnalysisSummary]}, status.HTTP_201_CREATED: {"model": SimpleMultiBody[AnalysisSummary]}, - status.HTTP_400_BAD_REQUEST: {"model": ErrorBody[FileIdNotFound]}, status.HTTP_404_NOT_FOUND: {"model": ErrorBody[ProtocolNotFound]}, + status.HTTP_422_UNPROCESSABLE_ENTITY: {"model": ErrorBody[FileIdNotFound]}, status.HTTP_503_SERVICE_UNAVAILABLE: {"model": ErrorBody[LastAnalysisPending]}, }, ) @@ -743,7 +746,9 @@ async def create_protocol_analysis( for name, file_id in rtp_files.items() } except FileIdNotFoundError as e: - raise FileIdNotFound(detail=str(e)).as_error(status.HTTP_400_BAD_REQUEST) + raise FileIdNotFound(detail=str(e)).as_error( + status.HTTP_422_UNPROCESSABLE_ENTITY + ) try: ( diff --git a/robot-server/robot_server/runs/router/base_router.py b/robot-server/robot_server/runs/router/base_router.py index 7000882b965..1d101c978f9 100644 --- a/robot-server/robot_server/runs/router/base_router.py +++ b/robot-server/robot_server/runs/router/base_router.py @@ -149,8 +149,8 @@ async def get_run_data_from_url( status_code=status.HTTP_201_CREATED, responses={ status.HTTP_201_CREATED: {"model": SimpleBody[Run]}, - status.HTTP_400_BAD_REQUEST: {"model": ErrorBody[FileIdNotFound]}, status.HTTP_404_NOT_FOUND: {"model": ErrorBody[ProtocolNotFound]}, + status.HTTP_422_UNPROCESSABLE_ENTITY: {"model": ErrorBody[FileIdNotFound]}, status.HTTP_409_CONFLICT: {"model": ErrorBody[RunAlreadyActive]}, }, ) @@ -209,7 +209,9 @@ async def create_run( # noqa: C901 for name, file_id in rtp_files.items() } except FileIdNotFoundError as e: - raise FileIdNotFound(detail=str(e)).as_error(status.HTTP_400_BAD_REQUEST) + raise FileIdNotFound(detail=str(e)).as_error( + status.HTTP_422_UNPROCESSABLE_ENTITY + ) protocol_resource = None