From 7a48c128aae61a17fb99ceb175bc14d095f638da Mon Sep 17 00:00:00 2001 From: Jeremy Leon Date: Tue, 14 Jan 2025 13:19:33 -0500 Subject: [PATCH] fix(api): prevent protocols from passing with mistyped property setting for liquid classes (#17258) Use slots in liquid class property dataclasses to prevent dynamic attribute assignments and minor touch tip property bugfix. --- .../protocol_api/_liquid_properties.py | 33 ++-- .../test_liquid_class_properties.py | 185 ++++++++++++++++++ 2 files changed, 202 insertions(+), 16 deletions(-) diff --git a/api/src/opentrons/protocol_api/_liquid_properties.py b/api/src/opentrons/protocol_api/_liquid_properties.py index dc848cfb7e2..ebb903591ea 100644 --- a/api/src/opentrons/protocol_api/_liquid_properties.py +++ b/api/src/opentrons/protocol_api/_liquid_properties.py @@ -83,7 +83,10 @@ def _sort_volume_and_values(self) -> None: ) -@dataclass +# We use slots for this dataclass (and the rest of liquid properties) to prevent dynamic creation of attributes +# not defined in the class, not for any performance reasons. This is so that mistyping properties when overriding +# values will cause the protocol to fail analysis, rather than silently passing. +@dataclass(slots=True) class DelayProperties: _enabled: bool @@ -118,7 +121,7 @@ def as_shared_data_model(self) -> SharedDataDelayProperties: ) -@dataclass +@dataclass(slots=True) class TouchTipProperties: _enabled: bool @@ -157,7 +160,7 @@ def mm_to_edge(self) -> Optional[float]: @mm_to_edge.setter def mm_to_edge(self, new_mm: float) -> None: validated_mm = validation.ensure_float(new_mm) - self._z_offset = validated_mm + self._mm_to_edge = validated_mm @property def speed(self) -> Optional[float]: @@ -190,7 +193,7 @@ def as_shared_data_model(self) -> SharedDataTouchTipProperties: ) -@dataclass +@dataclass(slots=True) class MixProperties: _enabled: bool @@ -243,7 +246,7 @@ def as_shared_data_model(self) -> SharedDataMixProperties: ) -@dataclass +@dataclass(slots=True) class BlowoutProperties: _enabled: bool @@ -297,7 +300,7 @@ def as_shared_data_model(self) -> SharedDataBlowoutProperties: ) -@dataclass +@dataclass(slots=True) class SubmergeRetractCommon: _position_reference: PositionReference @@ -336,10 +339,8 @@ def delay(self) -> DelayProperties: return self._delay -@dataclass +@dataclass(slots=True) class Submerge(SubmergeRetractCommon): - ... - def as_shared_data_model(self) -> SharedDataSubmerge: return SharedDataSubmerge( positionReference=self._position_reference, @@ -349,7 +350,7 @@ def as_shared_data_model(self) -> SharedDataSubmerge: ) -@dataclass +@dataclass(slots=True) class RetractAspirate(SubmergeRetractCommon): _air_gap_by_volume: LiquidHandlingPropertyByVolume @@ -374,7 +375,7 @@ def as_shared_data_model(self) -> SharedDataRetractAspirate: ) -@dataclass +@dataclass(slots=True) class RetractDispense(SubmergeRetractCommon): _air_gap_by_volume: LiquidHandlingPropertyByVolume @@ -405,7 +406,7 @@ def as_shared_data_model(self) -> SharedDataRetractDispense: ) -@dataclass +@dataclass(slots=True) class BaseLiquidHandlingProperties: _submerge: Submerge @@ -449,7 +450,7 @@ def delay(self) -> DelayProperties: return self._delay -@dataclass +@dataclass(slots=True) class AspirateProperties(BaseLiquidHandlingProperties): _retract: RetractAspirate @@ -487,7 +488,7 @@ def as_shared_data_model(self) -> SharedDataAspirateProperties: ) -@dataclass +@dataclass(slots=True) class SingleDispenseProperties(BaseLiquidHandlingProperties): _retract: RetractDispense @@ -520,7 +521,7 @@ def as_shared_data_model(self) -> SharedDataSingleDispenseProperties: ) -@dataclass +@dataclass(slots=True) class MultiDispenseProperties(BaseLiquidHandlingProperties): _retract: RetractDispense @@ -553,7 +554,7 @@ def as_shared_data_model(self) -> SharedDataMultiDispenseProperties: ) -@dataclass +@dataclass(slots=True) class TransferProperties: _aspirate: AspirateProperties _dispense: SingleDispenseProperties diff --git a/api/tests/opentrons/protocol_api/test_liquid_class_properties.py b/api/tests/opentrons/protocol_api/test_liquid_class_properties.py index 94e6dd49205..ee269f378d5 100644 --- a/api/tests/opentrons/protocol_api/test_liquid_class_properties.py +++ b/api/tests/opentrons/protocol_api/test_liquid_class_properties.py @@ -58,6 +58,62 @@ def test_build_aspirate_settings() -> None: assert aspirate_properties.as_shared_data_model() == aspirate_data +def test_aspirate_settings_overrides() -> None: + """It should allow aspirate properties to be overridden with new values.""" + fixture_data = load_shared_data("liquid-class/fixtures/1/fixture_glycerol50.json") + liquid_class_model = LiquidClassSchemaV1.model_validate_json(fixture_data) + aspirate_data = liquid_class_model.byPipette[0].byTipType[0].aspirate + + aspirate_properties = build_aspirate_properties(aspirate_data) + + aspirate_properties.submerge.position_reference = "well-bottom" # type: ignore[assignment] + assert aspirate_properties.submerge.position_reference.value == "well-bottom" + aspirate_properties.submerge.offset = 5, 0, 0 # type: ignore[assignment] + assert aspirate_properties.submerge.offset == Coordinate(x=5, y=0, z=0) + aspirate_properties.submerge.speed = 123 + assert aspirate_properties.submerge.speed == 123 + aspirate_properties.submerge.delay.enabled = False + assert aspirate_properties.submerge.delay.enabled is False + aspirate_properties.submerge.delay.duration = 5.1 + assert aspirate_properties.submerge.delay.duration == 5.1 + + aspirate_properties.retract.position_reference = "well-center" # type: ignore[assignment] + assert aspirate_properties.retract.position_reference.value == "well-center" + aspirate_properties.retract.offset = 0, 50, 0 # type: ignore[assignment] + assert aspirate_properties.retract.offset == Coordinate(x=0, y=50, z=0) + aspirate_properties.retract.speed = 987 + assert aspirate_properties.retract.speed == 987 + aspirate_properties.retract.touch_tip.enabled = False + assert aspirate_properties.retract.touch_tip.enabled is False + aspirate_properties.retract.touch_tip.z_offset = 2.34 + assert aspirate_properties.retract.touch_tip.z_offset == 2.34 + aspirate_properties.retract.touch_tip.mm_to_edge = 4.56 + assert aspirate_properties.retract.touch_tip.mm_to_edge == 4.56 + aspirate_properties.retract.touch_tip.speed = 501 + assert aspirate_properties.retract.touch_tip.speed == 501 + aspirate_properties.retract.delay.enabled = False + assert aspirate_properties.retract.delay.enabled is False + aspirate_properties.retract.delay.duration = 0.5 + assert aspirate_properties.retract.delay.duration == 0.5 + + aspirate_properties.position_reference = "liquid-meniscus" # type: ignore[assignment] + assert aspirate_properties.position_reference.value == "liquid-meniscus" + aspirate_properties.offset = -1, -2, -3 # type: ignore[assignment] + assert aspirate_properties.offset == Coordinate(x=-1, y=-2, z=-3) + aspirate_properties.pre_wet = False + assert aspirate_properties.pre_wet is False + aspirate_properties.mix.enabled = False + assert aspirate_properties.mix.enabled is False + aspirate_properties.mix.repetitions = 33 + assert aspirate_properties.mix.repetitions == 33 + aspirate_properties.mix.volume = 51 + assert aspirate_properties.mix.volume == 51 + aspirate_properties.delay.enabled = False + assert aspirate_properties.delay.enabled is False + aspirate_properties.delay.duration = 2.3 + assert aspirate_properties.delay.duration == 2.3 + + def test_build_single_dispense_settings() -> None: """It should convert the shared data single dispense settings to the PAPI type.""" fixture_data = load_shared_data("liquid-class/fixtures/1/fixture_glycerol50.json") @@ -115,6 +171,67 @@ def test_build_single_dispense_settings() -> None: assert single_dispense_properties.as_shared_data_model() == single_dispense_data +def test_single_dispense_settings_override() -> None: + """It should allow single dispense properties to be overridden with new values.""" + fixture_data = load_shared_data("liquid-class/fixtures/1/fixture_glycerol50.json") + liquid_class_model = LiquidClassSchemaV1.model_validate_json(fixture_data) + single_dispense_data = liquid_class_model.byPipette[0].byTipType[0].singleDispense + + single_dispense_properties = build_single_dispense_properties(single_dispense_data) + + single_dispense_properties.submerge.position_reference = "well-bottom" # type: ignore[assignment] + assert single_dispense_properties.submerge.position_reference.value == "well-bottom" + single_dispense_properties.submerge.offset = 3, -2, 1 # type: ignore[assignment] + assert single_dispense_properties.submerge.offset == Coordinate(x=3, y=-2, z=1) + single_dispense_properties.submerge.speed = 111 + assert single_dispense_properties.submerge.speed == 111 + single_dispense_properties.submerge.delay.enabled = False + assert single_dispense_properties.submerge.delay.enabled is False + single_dispense_properties.submerge.delay.duration = 5.1 + assert single_dispense_properties.submerge.delay.duration == 5.1 + + single_dispense_properties.retract.position_reference = "well-center" # type: ignore[assignment] + assert single_dispense_properties.retract.position_reference.value == "well-center" + single_dispense_properties.retract.offset = -9, -8, -7 # type: ignore[assignment] + assert single_dispense_properties.retract.offset == Coordinate(x=-9, y=-8, z=-7) + single_dispense_properties.retract.speed = 222 + assert single_dispense_properties.retract.speed == 222 + single_dispense_properties.retract.touch_tip.enabled = False + assert single_dispense_properties.retract.touch_tip.enabled is False + single_dispense_properties.retract.touch_tip.z_offset = 2.34 + assert single_dispense_properties.retract.touch_tip.z_offset == 2.34 + single_dispense_properties.retract.touch_tip.mm_to_edge = 1.11 + assert single_dispense_properties.retract.touch_tip.mm_to_edge == 1.11 + single_dispense_properties.retract.touch_tip.speed = 543 + assert single_dispense_properties.retract.touch_tip.speed == 543 + single_dispense_properties.retract.blowout.enabled = False + assert single_dispense_properties.retract.blowout.enabled is False + single_dispense_properties.retract.blowout.location = "destination" # type: ignore[assignment] + assert single_dispense_properties.retract.blowout.location + assert single_dispense_properties.retract.blowout.location.value == "destination" + single_dispense_properties.retract.blowout.flow_rate = 3.21 + assert single_dispense_properties.retract.blowout.flow_rate == 3.21 + single_dispense_properties.retract.delay.enabled = False + assert single_dispense_properties.retract.delay.enabled is False + single_dispense_properties.retract.delay.duration = 0.1 + assert single_dispense_properties.retract.delay.duration == 0.1 + + single_dispense_properties.position_reference = "liquid-meniscus" # type: ignore[assignment] + assert single_dispense_properties.position_reference.value == "liquid-meniscus" + single_dispense_properties.offset = 11, 22, -33 # type: ignore[assignment] + assert single_dispense_properties.offset == Coordinate(x=11, y=22, z=-33) + single_dispense_properties.mix.enabled = False + assert single_dispense_properties.mix.enabled is False + single_dispense_properties.mix.repetitions = 15 + assert single_dispense_properties.mix.repetitions == 15 + single_dispense_properties.mix.volume = 3 + assert single_dispense_properties.mix.volume == 3 + single_dispense_properties.delay.enabled = False + assert single_dispense_properties.delay.enabled is False + single_dispense_properties.delay.duration = 25.25 + assert single_dispense_properties.delay.duration == 25.25 + + def test_build_multi_dispense_settings() -> None: """It should convert the shared data multi dispense settings to the PAPI type.""" fixture_data = load_shared_data("liquid-class/fixtures/1/fixture_glycerol50.json") @@ -171,6 +288,62 @@ def test_build_multi_dispense_settings() -> None: assert multi_dispense_properties.as_shared_data_model() == multi_dispense_data +def test_multi_dispense_settings_override() -> None: + """It should allow multi dispense properties to be overridden with new values.""" + fixture_data = load_shared_data("liquid-class/fixtures/1/fixture_glycerol50.json") + liquid_class_model = LiquidClassSchemaV1.model_validate_json(fixture_data) + multi_dispense_data = liquid_class_model.byPipette[0].byTipType[0].multiDispense + assert multi_dispense_data is not None + multi_dispense_properties = build_multi_dispense_properties(multi_dispense_data) + assert multi_dispense_properties is not None + + multi_dispense_properties.submerge.position_reference = "well-bottom" # type: ignore[assignment] + assert multi_dispense_properties.submerge.position_reference.value == "well-bottom" + multi_dispense_properties.submerge.offset = 3, -2, 1 # type: ignore[assignment] + assert multi_dispense_properties.submerge.offset == Coordinate(x=3, y=-2, z=1) + multi_dispense_properties.submerge.speed = 111 + assert multi_dispense_properties.submerge.speed == 111 + multi_dispense_properties.submerge.delay.enabled = False + assert multi_dispense_properties.submerge.delay.enabled is False + multi_dispense_properties.submerge.delay.duration = 5.1 + assert multi_dispense_properties.submerge.delay.duration == 5.1 + + multi_dispense_properties.retract.position_reference = "well-center" # type: ignore[assignment] + assert multi_dispense_properties.retract.position_reference.value == "well-center" + multi_dispense_properties.retract.offset = -9, -8, -7 # type: ignore[assignment] + assert multi_dispense_properties.retract.offset == Coordinate(x=-9, y=-8, z=-7) + multi_dispense_properties.retract.speed = 222 + assert multi_dispense_properties.retract.speed == 222 + multi_dispense_properties.retract.touch_tip.enabled = False + assert multi_dispense_properties.retract.touch_tip.enabled is False + multi_dispense_properties.retract.touch_tip.z_offset = 2.34 + assert multi_dispense_properties.retract.touch_tip.z_offset == 2.34 + multi_dispense_properties.retract.touch_tip.mm_to_edge = 1.11 + assert multi_dispense_properties.retract.touch_tip.mm_to_edge == 1.11 + multi_dispense_properties.retract.touch_tip.speed = 543 + assert multi_dispense_properties.retract.touch_tip.speed == 543 + multi_dispense_properties.retract.blowout.enabled = False + assert multi_dispense_properties.retract.blowout.enabled is False + multi_dispense_properties.retract.blowout.location = "destination" # type: ignore[assignment] + assert multi_dispense_properties.retract.blowout.location + assert multi_dispense_properties.retract.blowout.location.value == "destination" + multi_dispense_properties.retract.blowout.flow_rate = 3.21 + assert multi_dispense_properties.retract.blowout.flow_rate == 3.21 + multi_dispense_properties.retract.delay.enabled = False + assert multi_dispense_properties.retract.delay.enabled is False + multi_dispense_properties.retract.delay.duration = 0.1 + assert multi_dispense_properties.retract.delay.duration == 0.1 + + multi_dispense_properties.position_reference = "liquid-meniscus" # type: ignore[assignment] + assert multi_dispense_properties.position_reference.value == "liquid-meniscus" + multi_dispense_properties.offset = 11, 22, -33 # type: ignore[assignment] + assert multi_dispense_properties.offset == Coordinate(x=11, y=22, z=-33) + multi_dispense_properties.delay.enabled = False + assert multi_dispense_properties.delay.enabled is False + multi_dispense_properties.delay.duration = 25.25 + assert multi_dispense_properties.delay.duration == 25.25 + + def test_build_multi_dispense_settings_none( minimal_liquid_class_def2: LiquidClassSchemaV1, ) -> None: @@ -204,3 +377,15 @@ def test_liquid_handling_property_by_volume() -> None: # Test bounds assert subject.get_for_volume(1) == 50.0 assert subject.get_for_volume(1000) == 250.0 + + +def test_non_existent_property_raises_error() -> None: + """It should raise an attribute error if the set property does not exist.""" + fixture_data = load_shared_data("liquid-class/fixtures/1/fixture_glycerol50.json") + liquid_class_model = LiquidClassSchemaV1.model_validate_json(fixture_data) + aspirate_data = liquid_class_model.byPipette[0].byTipType[0].aspirate + + aspirate_properties = build_aspirate_properties(aspirate_data) + + with pytest.raises(AttributeError): + aspirate_properties.mix.enable = True # type: ignore[attr-defined]