fix(robot-server): Fix None ambiguity in labware offset filtering (#…

…17218)

robot-server/robot_server/labware_offsets/router.py

@@ -3,9 +3,11 @@
 
 from datetime import datetime
 import textwrap
-from typing import Annotated, Literal
+from typing import Annotated, Literal, Type
 
 import fastapi
+from pydantic import Json
+from pydantic.json_schema import SkipJsonSchema
 from server_utils.fastapi_utils.light_router import LightRouter
 
 from opentrons.protocol_engine import LabwareOffset, LabwareOffsetCreate, ModuleModel
@@ -22,7 +24,7 @@
     SimpleMultiBody,
 )
 
-from .store import LabwareOffsetNotFoundError, LabwareOffsetStore
+from .store import DO_NOT_FILTER, LabwareOffsetNotFoundError, LabwareOffsetStore
 from .fastapi_dependencies import get_labware_offset_store
 
 
@@ -76,45 +78,45 @@ async def post_labware_offset(  # noqa: D103
 async def get_labware_offsets(  # noqa: D103
     store: Annotated[LabwareOffsetStore, fastapi.Depends(get_labware_offset_store)],
     id: Annotated[
-        str | None,
+        Json[str] | SkipJsonSchema[Type[DO_NOT_FILTER]],
         fastapi.Query(description="Filter for exact matches on the `id` field."),
-    ] = None,
+    ] = DO_NOT_FILTER,
     definition_uri: Annotated[
-        str | None,
+        Json[str] | SkipJsonSchema[Type[DO_NOT_FILTER]],
         fastapi.Query(
             alias="definitionUri",
             description=(
                 "Filter for exact matches on the `definitionUri` field."
                 " (Not to be confused with `location.definitionUri`.)"
             ),
         ),
-    ] = None,
+    ] = DO_NOT_FILTER,
     location_slot_name: Annotated[
-        DeckSlotName | None,
+        Json[DeckSlotName] | SkipJsonSchema[Type[DO_NOT_FILTER]],
         fastapi.Query(
             alias="locationSlotName",
             description="Filter for exact matches on the `location.slotName` field.",
         ),
-    ] = None,
+    ] = DO_NOT_FILTER,
     location_module_model: Annotated[
-        ModuleModel | None,
+        Json[ModuleModel | None] | SkipJsonSchema[Type[DO_NOT_FILTER]],
         fastapi.Query(
             alias="locationModuleModel",
             description="Filter for exact matches on the `location.moduleModel` field.",
         ),
-    ] = None,
+    ] = DO_NOT_FILTER,
     location_definition_uri: Annotated[
-        str | None,
+        Json[str | None] | SkipJsonSchema[Type[DO_NOT_FILTER]],
         fastapi.Query(
             alias="locationDefinitionUri",
             description=(
                 "Filter for exact matches on the `location.definitionUri` field."
                 " (Not to be confused with just `definitionUri`.)"
             ),
         ),
-    ] = None,
+    ] = DO_NOT_FILTER,
     cursor: Annotated[
-        int | None,
+        int | SkipJsonSchema[None],
         fastapi.Query(
             description=(
                 "The first index to return out of the overall filtered result list."

robot-server/robot_server/labware_offsets/store.py

@@ -1,9 +1,21 @@
 # noqa: D100
 
+from typing import Type
+
 from opentrons.protocol_engine import LabwareOffset, ModuleModel
 from opentrons.types import DeckSlotName
 
 
+class DO_NOT_FILTER:
+    """A sentinel value for when a filter should not be applied.
+
+    This is different from filtering on `None`, which returns only entries where the
+    value is equal to `None`.
+    """
+
+    pass
+
+
 # todo(mm, 2024-12-06): Convert to be SQL-based and persistent instead of in-memory.
 # https://opentrons.atlassian.net/browse/EXEC-1015
 class LabwareOffsetStore:
@@ -19,11 +31,15 @@ def add(self, offset: LabwareOffset) -> None:
 
     def search(
         self,
-        id_filter: str | None,
-        definition_uri_filter: str | None,
-        location_slot_name_filter: DeckSlotName | None,
-        location_module_model_filter: ModuleModel | None,
-        location_definition_uri_filter: str | None,
+        id_filter: str | Type[DO_NOT_FILTER] = DO_NOT_FILTER,
+        definition_uri_filter: str | Type[DO_NOT_FILTER] = DO_NOT_FILTER,
+        location_slot_name_filter: DeckSlotName | Type[DO_NOT_FILTER] = DO_NOT_FILTER,
+        location_module_model_filter: ModuleModel
+        | None
+        | Type[DO_NOT_FILTER] = DO_NOT_FILTER,
+        location_definition_uri_filter: str
+        | None
+        | Type[DO_NOT_FILTER] = DO_NOT_FILTER,
         # todo(mm, 2024-12-06): Support pagination (cursor & pageLength query params).
         # The logic for that is currently duplicated across several places in
         # robot-server and api. We should try to clean that up, or at least avoid
@@ -33,13 +49,14 @@ def search(
 
         def is_match(candidate: LabwareOffset) -> bool:
             return (
-                id_filter in (None, candidate.id)
-                and definition_uri_filter in (None, candidate.definitionUri)
-                and location_slot_name_filter in (None, candidate.location.slotName)
+                id_filter in (DO_NOT_FILTER, candidate.id)
+                and definition_uri_filter in (DO_NOT_FILTER, candidate.definitionUri)
+                and location_slot_name_filter
+                in (DO_NOT_FILTER, candidate.location.slotName)
                 and location_module_model_filter
-                in (None, candidate.location.moduleModel)
+                in (DO_NOT_FILTER, candidate.location.moduleModel)
                 and location_definition_uri_filter
-                in (None, candidate.location.definitionUri)
+                in (DO_NOT_FILTER, candidate.location.definitionUri)
             )
 
         return [

robot-server/tests/integration/http_api/test_labware_offsets.tavern.yaml

@@ -83,7 +83,7 @@ stages:
   # Just a basic test here. More complicated tests for the filters belong in the unit tests.
   - name: Test getting labware offsets with a filter
     request:
-      url: '{ot3_server_base_url}/labwareOffsets?locationSlotName=A2'
+      url: '{ot3_server_base_url}/labwareOffsets?locationSlotName="A2"'
       method: GET
     response:
       json:
@@ -129,3 +129,122 @@ stages:
         meta:
           cursor: 0
           totalLength: 0
+
+---
+# Some of the filter query parameters can have `null` values or be omitted,
+# with different semantics between the two. That distinction takes a bit of care to
+# preserve across our code, so here we test it specifically.
+test_name: Test null vs. omitted filter query parameters
+marks:
+  - usefixtures:
+      - ot3_server_base_url
+stages:
+  - name: POST test offset 1
+    request:
+      method: POST
+      url: '{ot3_server_base_url}/labwareOffsets'
+      json:
+        data:
+          definitionUri: testNamespace/loadName1/1
+          location:
+            slotName: A1
+            # No moduleModel
+            # No definitionUri
+          vector:
+            x: 1
+            y: 2
+            z: 3
+    response:
+      status_code: 201
+      save:
+        json:
+          offset_1_data: data
+  - name: POST test offset 2
+    request:
+      method: POST
+      url: '{ot3_server_base_url}/labwareOffsets'
+      json:
+        data:
+          definitionUri: testNamespace/loadName2/1
+          location:
+            slotName: A1
+            moduleModel: temperatureModuleV2
+            # No definitionUri
+          vector:
+            x: 1
+            y: 2
+            z: 3
+    response:
+      status_code: 201
+      save:
+        json:
+          offset_2_data: data
+  - name: POST test offset 3
+    request:
+      method: POST
+      url: '{ot3_server_base_url}/labwareOffsets'
+      json:
+        data:
+          definitionUri: testNamespace/loadName2/1
+          location:
+            slotName: A1
+            # no moduleModel
+            definitionUri: testNamespace/adapterLoadName/1
+          vector:
+            x: 1
+            y: 2
+            z: 3
+    response:
+      status_code: 201
+      save:
+        json:
+          offset_3_data: data
+  - name: POST test offset 4
+    request:
+      method: POST
+      url: '{ot3_server_base_url}/labwareOffsets'
+      json:
+        data:
+          definitionUri: testNamespace/loadName3/1
+          location:
+            slotName: A1
+            moduleModel: temperatureModuleV2
+            definitionUri: testNamespace/adapterLoadName/1
+          vector:
+            x: 1
+            y: 2
+            z: 3
+    response:
+      status_code: 201
+      save:
+        json:
+          offset_4_data: data
+  - name: Test no filters
+    request:
+      url: '{ot3_server_base_url}/labwareOffsets'
+    response:
+      json:
+        data:
+          - !force_format_include '{offset_1_data}'
+          - !force_format_include '{offset_2_data}'
+          - !force_format_include '{offset_3_data}'
+          - !force_format_include '{offset_4_data}'
+        meta: !anydict
+  - name: Test filtering on locationModuleModel=null
+    request:
+      url: '{ot3_server_base_url}/labwareOffsets?locationModuleModel=null'
+    response:
+      json:
+        data:
+          - !force_format_include '{offset_1_data}'
+          - !force_format_include '{offset_3_data}'
+        meta: !anydict
+  - name: Test filtering on locationDefinitionUri=null
+    request:
+      url: '{ot3_server_base_url}/labwareOffsets?locationDefinitionUri=null'
+    response:
+      json:
+        data:
+          - !force_format_include '{offset_1_data}'
+          - !force_format_include '{offset_2_data}'
+        meta: !anydict

robot-server/tests/labware_offsets/test_store.py

@@ -16,13 +16,7 @@
 
 
 def _get_all(store: LabwareOffsetStore) -> list[LabwareOffset]:
-    return store.search(
-        id_filter=None,
-        definition_uri_filter=None,
-        location_definition_uri_filter=None,
-        location_module_model_filter=None,
-        location_slot_name_filter=None,
-    )
+    return store.search()
 
 
 def test_filters() -> None:
@@ -52,25 +46,10 @@ def test_filters() -> None:
         subject.add(labware_offset)
 
     # No filters:
-    assert (
-        subject.search(
-            id_filter=None,
-            definition_uri_filter=None,
-            location_definition_uri_filter=None,
-            location_module_model_filter=None,
-            location_slot_name_filter=None,
-        )
-        == labware_offsets
-    )
+    assert subject.search() == labware_offsets
 
     # Filter on one thing:
-    result = subject.search(
-        id_filter=None,
-        definition_uri_filter="definition-uri-b",
-        location_definition_uri_filter=None,
-        location_module_model_filter=None,
-        location_slot_name_filter=None,
-    )
+    result = subject.search(definition_uri_filter="definition-uri-b")
     assert len(result) == 3
     assert result == [
         entry for entry in labware_offsets if entry.definitionUri == "definition-uri-b"
@@ -80,19 +59,13 @@ def test_filters() -> None:
     result = subject.search(
         id_filter="id-2",
         definition_uri_filter="definition-uri-b",
-        location_definition_uri_filter=None,
-        location_module_model_filter=None,
-        location_slot_name_filter=None,
     )
     assert result == [labware_offsets[1]]
 
     # Filters should be ANDed, not ORed, together:
     result = subject.search(
         id_filter="id-1",
         definition_uri_filter="definition-uri-b",
-        location_definition_uri_filter=None,
-        location_module_model_filter=None,
-        location_slot_name_filter=None,
     )
     assert result == []