refactor(api, robot-server): add error for multiple CSV definitions a…

…nd 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.

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.

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)

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."""

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

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."""

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)

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__}'."
         )
 

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)

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"