Undeprecate using instance list get method with external_id (#1969)

CHANGELOG.md

@@ -17,6 +17,12 @@ Changes are grouped as follows
 - `Fixed` for any bug fixes.
 - `Security` in case of vulnerabilities.
 
+## [7.63.3] - 2024-10-13
+### Fixed
+- NodeList and EdgeList (and subclasses) now support using `.get` with an `external_id` as a shortcut over
+  using `instance_id`. When the `external_id` is ambiguous (non-unique, multiple spaces), a ValueError
+  is raised. Thus, using `external_id` is no longer deprecated.
+
 ## [7.63.2] - 2024-10-11
 ### Fixed
 - Setting up interactive `OAuthInteractive` sessions no longer raises `TypeError` as the lower bound for the `msal`
@@ -29,7 +35,7 @@ Changes are grouped as follows
 
 ## [7.63.0] - 2024-10-10
 ### Removed
-- Removed support for Python 3.8 and 3.9.
+- Dropped support for Python 3.8 and 3.9.
 
 ## [7.62.8] - 2024-10-07
 ### Added

cognite/client/_version.py

@@ -1,4 +1,4 @@
 from __future__ import annotations
 
-__version__ = "7.63.2"
+__version__ = "7.63.3"
 __api_subversion__ = "20230101"

cognite/client/data_classes/data_modeling/instances.py

@@ -61,7 +61,7 @@
     ViewId,
     ViewIdentifier,
 )
-from cognite.client.utils._auxiliary import flatten_dict
+from cognite.client.utils._auxiliary import exactly_one_is_not_none, find_duplicates, flatten_dict
 from cognite.client.utils._identifier import InstanceId
 from cognite.client.utils._importing import local_import
 from cognite.client.utils._text import convert_all_keys_to_snake_case, to_camel_case
@@ -935,41 +935,55 @@ def as_ids(self) -> list[NodeId]:
 class DataModelingInstancesList(WriteableCogniteResourceList[T_WriteClass, T_Instance], ABC):
     def _build_id_mappings(self) -> None:
         self._instance_id_to_item = {(inst.space, inst.external_id): inst for inst in self.data}
-        # TODO: Remove when we ditch PY3.8 (Oct, 2024), reason: ambiguous without space:
+        # We allow fast lookup via external ID when not ambiguous:
+        self._ambiguous_xids = find_duplicates(inst.external_id for inst in self.data)
         self._ext_id_to_item = {inst.external_id: inst for inst in self.data}
 
-    def get(
+    def get(  # type: ignore [override]
         self,
-        id: InstanceId | tuple[str, str] | None = None,  # type: ignore [override]
+        instance_id: InstanceId | tuple[str, str] | None = None,
         external_id: str | None = None,
+        *,
+        id: InstanceId | tuple[str, str] | None = None,
     ) -> T_Instance | None:
         """Get an instance from this list by instance ID.
 
         Args:
-            id (InstanceId | tuple[str, str] | None): The instance ID to get. A tuple on the form (space, external_id) is also accepted.
-            external_id (str | None): DEPRECATED (reason: ambiguous). The external ID of the instance to return.
+            instance_id (InstanceId | tuple[str, str] | None): The instance ID to get. A tuple on the form (space, external_id) is also accepted.
+            external_id (str | None): The external ID of the instance to return. Will raise ValueError when ambiguous (in presence of multiple spaces).
+            id (InstanceId | tuple[str, str] | None): (DEPRECATED) Backwards-compatible alias for instance_id. Will be removed in the next major version.
 
         Returns:
             T_Instance | None: The requested instance if present, else None
         """
-        # TODO: Remove when we ditch PY3.8
-        if external_id is not None:
+        if not exactly_one_is_not_none(instance_id, external_id, id):
+            raise ValueError(
+                "Pass exactly one of 'instance_id' or 'external_id' ('id' is a deprecated alias for 'instance_id'). "
+                "Using an external ID requires all instances to be from the same space."
+            )
+        if id is not None:
+            instance_id = id
             warnings.warn(
-                "Calling .get with an external ID is deprecated due to being ambiguous in the absense of 'space', and "
-                "will be removed as of Oct, 2024. Pass an instance ID instead (or a tuple of (space, external_id)).",
+                "Calling .get using `id` is deprecated and will be removed in the next major version. "
+                "Use 'instance_id' instead",
                 UserWarning,
             )
+        elif external_id is not None:
+            if external_id in self._ambiguous_xids:
+                raise ValueError(
+                    f"{external_id=} is ambiguous (multiple spaces are present). Pass 'instance_id' instead."
+                )
             return self._ext_id_to_item.get(external_id)
 
-        if isinstance(id, InstanceId):
-            id = id.as_tuple()
-        return self._instance_id_to_item.get(id)  # type: ignore [arg-type]
+        if isinstance(instance_id, InstanceId):
+            instance_id = instance_id.as_tuple()
+        return self._instance_id_to_item.get(instance_id)  # type: ignore [arg-type]
 
     def extend(self, other: Iterable[Any]) -> None:
         other_res_list = type(self)(other)  # See if we can accept the types
         if self._instance_id_to_item.keys().isdisjoint(other_res_list._instance_id_to_item):
             self.data.extend(other_res_list.data)
-            self._instance_id_to_item.update(other_res_list._instance_id_to_item)
+            self._build_id_mappings()
         else:
             raise ValueError("Unable to extend as this would introduce duplicates")
 
@@ -1088,7 +1102,7 @@ def extend(self, other: NodeListWithCursor) -> None:  # type: ignore[override]
         other_res_list = type(self)(other, other.cursor)  # See if we can accept the types
         if self._instance_id_to_item.keys().isdisjoint(other_res_list._instance_id_to_item):
             self.data.extend(other.data)
-            self._instance_id_to_item.update(other_res_list._instance_id_to_item)
+            self._build_id_mappings()
             self.cursor = other.cursor
         else:
             raise ValueError("Unable to extend as this would introduce duplicates")
@@ -1184,7 +1198,7 @@ def extend(self, other: EdgeListWithCursor) -> None:  # type: ignore[override]
         other_res_list = type(self)(other, other.cursor)  # See if we can accept the types
         if self._instance_id_to_item.keys().isdisjoint(other_res_list._instance_id_to_item):
             self.data.extend(other.data)
-            self._instance_id_to_item.update(other_res_list._instance_id_to_item)
+            self._build_id_mappings()
             self.cursor = other.cursor
         else:
             raise ValueError("Unable to extend as this would introduce duplicates")

pyproject.toml

@@ -1,7 +1,7 @@
 [tool.poetry]
 name = "cognite-sdk"
 
-version = "7.63.2"
+version = "7.63.3"
 description = "Cognite Python SDK"
 readme = "README.md"
 documentation = "https://cognite-sdk-python.readthedocs-hosted.com"

tests/tests_integration/test_api/test_data_modeling/test_instances.py

@@ -813,8 +813,8 @@ def test_aggregate_invalid_view_id(self, cognite_client: CogniteClient) -> None:
         assert "One or more views do not exist: " in error.value.message
         assert view_id.as_source_identifier() in error.value.message
 
-    def test_dump_json_serialize_load_node(self, movie_nodes: NodeList) -> None:
-        node = movie_nodes.get(external_id="movie:pulp_fiction")
+    def test_dump_json_serialize_load_node(self, movie_nodes: NodeList, integration_test_space: Space) -> None:
+        node = movie_nodes.get((integration_test_space.space, "movie:pulp_fiction"))
         assert node is not None, "Pulp fiction movie not found, please recreate it"
 
         node_dumped = node.dump(camel_case=True)
@@ -823,8 +823,8 @@ def test_dump_json_serialize_load_node(self, movie_nodes: NodeList) -> None:
 
         assert node == node_loaded
 
-    def test_dump_json_serialize_load_edge(self, movie_edges: EdgeList) -> None:
-        edge = movie_edges.get(external_id="person:quentin_tarantino:director:quentin_tarantino")
+    def test_dump_json_serialize_load_edge(self, movie_edges: EdgeList, integration_test_space: Space) -> None:
+        edge = movie_edges.get((integration_test_space.space, "person:quentin_tarantino:director:quentin_tarantino"))
         assert edge is not None, "Relation between Quentin Tarantino person and director not found, please recreate it"
 
         edge_dumped = edge.dump(camel_case=True)

tests/tests_unit/test_data_classes/test_data_models/test_resource_class_methods.py

@@ -5,6 +5,7 @@
 
 from __future__ import annotations
 
+from copy import deepcopy
 from typing import Any
 
 import pytest
@@ -68,9 +69,22 @@ def test_get_instance_lists(node_lst: NodeList, edge_lst: EdgeList, space: str,
     inst = inst_lst.get((space, "constant"))  # pass by tuple
     assert inst.space == space  # type: ignore [union-attr]
 
-    # Since ext.id is ambiguous, we always get the last (deprecated):
-    inst = inst_lst.get(external_id="constant")
-    assert inst.space == "foo2"  # type: ignore [union-attr]
+    # Since ext.id is ambiguous, lookup should raise:
+    with pytest.raises(ValueError, match=r"^external_id='constant' is ambiguous.*Pass 'instance_id' instead\.$"):
+        inst = inst_lst.get(external_id="constant")
+
+    lst1 = node_lst[:1]
+    lst2, lst3 = deepcopy(lst1), deepcopy(lst1)
+    lst2[0].external_id = "different"
+    lst3[0].space = "different"
+    assert lst1.get(external_id="constant") is not None
+
+    lst1.extend(lst2)
+    assert lst1.get(external_id="constant") is not None
+
+    lst1.extend(lst3)  # there are now nodes in two spaces with the same external_id:
+    with pytest.raises(ValueError, match=r"^external_id='constant' is ambiguous.*Pass 'instance_id' instead\.$"):
+        lst1.get(external_id="constant")
 
 
 @pytest.mark.parametrize("space", ["foo0", "foo1", "foo2"])