fix(api): prevent protocols from passing with mistyped property setti…

…ng for liquid classes (#17258)

Use slots in liquid class property dataclasses to prevent dynamic attribute assignments and minor touch tip property bugfix.

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

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]