PR fixes

tpellissier · tpellissier · commit 3081914c4bd0 · 2025-10-10T16:16:17.000-07:00
diff --git a/README.md b/README.md
@@ -35,8 +35,8 @@ Auth:
 | `get` | `get(entity_set, id)` | `dict` | One record; supply GUID (with/without parentheses). |
 | `get_multiple` | `get_multiple(entity_set, ..., page_size=None)` | `Iterable[list[dict]]` | Pages yielded (non-empty only). |
 | `update` | `update(entity_set, id, patch)` | `None` | Single update; no representation returned. |
-| `update` | `update(entity_set, list[id], patch)` | `None` | Broadcast; same patch applied to all IDs. |
-| `update` | `update(entity_set, list[id], list[patch])` | `None` | 1:1 patches; lengths must match. |
+| `update` | `update(entity_set, list[id], patch)` | `None` | Broadcast; same patch applied to all IDs. Calls UpdateMultiple web API internally. |
+| `update` | `update(entity_set, list[id], list[patch])` | `None` | 1:1 patches; lengths must match. Calls UpdateMultiple web API internally. |
 | `delete` | `delete(entity_set, id)` | `None` | Delete one record. |
 | `delete` | `delete(entity_set, list[id])` | `None` | Delete many (sequential). |
 | `query_sql` | `query_sql(sql)` | `list[dict]` | Constrained read-only SELECT via `?sql=`. |
@@ -50,9 +50,9 @@ Auth:
 | `PandasODataClient.query_sql_df` | `query_sql_df(sql)` | `DataFrame` | DataFrame for SQL results. |
 
 Guidelines:
-- `create` always returns a list of GUIDs (1 for single, N for multi).
+- `create` always returns a list of GUIDs (1 for single, N for bulk).
 - `update`/`delete` always return `None` (single and multi forms).
-- Multi-update chooses broadcast vs per-record by the type of `changes` (dict vs list).
+- Bulk update chooses broadcast vs per-record by the type of `changes` (dict vs list).
 - Paging and SQL operations never mutate inputs.
 - Metadata lookups for logical name stamping cached per entity set (in-memory).
 
@@ -132,14 +132,14 @@ account = client.get("accounts", account_id)
 # Update (returns None)
 client.update("accounts", account_id, {"telephone1": "555-0199"})
 
-# Multi-update (broadcast) – apply same patch to several IDs
+# Bulk update (broadcast) – apply same patch to several IDs
 ids = client.create("accounts", [
 	{"name": "Contoso"},
 	{"name": "Fabrikam"},
 ])
 client.update("accounts", ids, {"telephone1": "555-0200"})  # broadcast patch
 
-# Multi-update (1:1) – list of patches matches list of IDs
+# Bulk update (1:1) – list of patches matches list of IDs
 client.update("accounts", ids, [
 	{"telephone1": "555-1200"},
 	{"telephone1": "555-1300"},
@@ -170,7 +170,7 @@ assert isinstance(ids, list) and all(isinstance(x, str) for x in ids)
 print({"created_ids": ids})
 ```
 
-## Multi-update (UpdateMultiple under the hood)
+## Bulk update (UpdateMultiple under the hood)
 
 Use the unified `update` method for both single and bulk scenarios:
 
@@ -320,7 +320,7 @@ VS Code Tasks
 - No general-purpose OData batching, upsert, or association operations yet.
 - `DeleteMultiple` not yet exposed.
 - Minimal retry policy in library (network-error only); examples include additional backoff for transient Dataverse consistency.
-- Entity naming conventions in Dataverse: for multi-create the SDK resolves logical names from entity set metadata.
+- Entity naming conventions in Dataverse: for bulk create the SDK resolves logical names from entity set metadata.
 
 ## Contributing
 
diff --git a/src/dataverse_sdk/client.py b/src/dataverse_sdk/client.py
@@ -80,12 +80,15 @@ def create(self, entity: str, records: Union[Dict[str, Any], List[Dict[str, Any]
         """
         od = self._get_odata()
         if isinstance(records, dict):
-            rid = od.create(entity, records)  # low-level guarantees GUID str for single
+            rid = od._create_single(entity, records)
             if not isinstance(rid, str):
-                raise TypeError("Low-level create did not return GUID string for single record")
+                raise TypeError("_create_single did not return GUID string")
             return [rid]
         if isinstance(records, list):
-            return od.create(entity, records)  # multi path returns list[str]
+            ids = od._create_multiple(entity, records)
+            if not isinstance(ids, list) or not all(isinstance(x, str) for x in ids):
+                raise TypeError("_create_multiple did not return list[str]")
+            return ids
         raise TypeError("records must be dict or list[dict]")
 
     def update(self, entity: str, ids: Union[str, List[str]], changes: Union[Dict[str, Any], List[Dict[str, Any]]]) -> None:
@@ -105,22 +108,22 @@ def update(self, entity: str, ids: Union[str, List[str]], changes: Union[Dict[st
         if isinstance(ids, str):
             if not isinstance(changes, dict):
                 raise TypeError("For single id, changes must be a dict")
-            od.update(entity, ids, changes)  # discard representation
+            od._update(entity, ids, changes)  # discard representation
             return None
         if not isinstance(ids, list):
             raise TypeError("ids must be str or list[str]")
-        od.update_by_ids(entity, ids, changes)
+        od._update_by_ids(entity, ids, changes)
         return None
 
     def delete(self, entity: str, ids: Union[str, List[str]]) -> None:
         """Delete one or many records (GUIDs). Returns None."""
         od = self._get_odata()
         if isinstance(ids, str):
-            od.delete(entity, ids)
+            od._delete(entity, ids)
             return None
         if not isinstance(ids, list):
             raise TypeError("ids must be str or list[str]")
-        od.delete_many(entity, ids)
+        od._delete_multiple(entity, ids)
         return None
 
     def get(self, entity: str, record_id: str) -> dict:
@@ -138,7 +141,7 @@ def get(self, entity: str, record_id: str) -> dict:
         dict
             The record JSON payload.
         """
-        return self._get_odata().get(entity, record_id)
+        return self._get_odata()._get(entity, record_id)
 
     def get_multiple(
         self,
@@ -155,7 +158,7 @@ def get_multiple(
         Yields a list of records per page, following @odata.nextLink until exhausted.
         Parameters mirror standard OData query options.
         """
-        return self._get_odata().get_multiple(
+        return self._get_odata()._get_multiple(
             entity,
             select=select,
             filter=filter,
@@ -183,7 +186,7 @@ def query_sql(self, sql: str):
         list[dict]
             Result rows (empty list if none).
         """
-        return self._get_odata().query_sql(sql)
+        return self._get_odata()._query_sql(sql)
 
     # Table metadata helpers
     def get_table_info(self, tablename: str) -> Optional[Dict[str, Any]]:
@@ -201,7 +204,7 @@ def get_table_info(self, tablename: str) -> Optional[Dict[str, Any]]:
             Dict with keys like ``entity_schema``, ``entity_logical_name``,
             ``entity_set_name``, and ``metadata_id``; ``None`` if not found.
         """
-        return self._get_odata().get_table_info(tablename)
+        return self._get_odata()._get_table_info(tablename)
 
     def create_table(self, tablename: str, schema: Dict[str, str]) -> Dict[str, Any]:
         """Create a simple custom table.
@@ -220,7 +223,7 @@ def create_table(self, tablename: str, schema: Dict[str, str]) -> Dict[str, Any]
             Metadata summary including ``entity_schema``, ``entity_set_name``,
             ``entity_logical_name``, ``metadata_id``, and ``columns_created``.
         """
-        return self._get_odata().create_table(tablename, schema)
+        return self._get_odata()._create_table(tablename, schema)
 
     def delete_table(self, tablename: str) -> None:
         """Delete a custom table by name.
@@ -230,7 +233,7 @@ def delete_table(self, tablename: str) -> None:
         tablename : str
             Friendly name (``"SampleItem"``) or a full schema name (``"new_SampleItem"``).
         """
-        self._get_odata().delete_table(tablename)
+        self._get_odata()._delete_table(tablename)
 
     def list_tables(self) -> list[str]:
         """List all custom tables in the Dataverse environment.
@@ -240,8 +243,8 @@ def list_tables(self) -> list[str]:
         list[str]
             A list of table names.
         """
-        return self._get_odata().list_tables()
+        return self._get_odata()._list_tables()
 
 
 __all__ = ["DataverseClient"]
-        
+
diff --git a/src/dataverse_sdk/odata.py b/src/dataverse_sdk/odata.py
@@ -34,6 +34,10 @@ def __init__(self, auth, base_url: str, config=None) -> None:
         self._entityset_logical_cache = {}
         # Cache: logical name -> entity set name (reverse lookup for SQL endpoint)
         self._logical_to_entityset_cache: dict[str, str] = {}
+        # Cache: entity set name -> primary id attribute (metadata PrimaryIdAttribute)
+        self._entityset_primaryid_cache: dict[str, str] = {}
+        # Cache: logical name -> primary id attribute
+        self._logical_primaryid_cache: dict[str, str] = {}
 
     def _headers(self) -> Dict[str, str]:
         """Build standard OData headers with bearer auth."""
@@ -51,7 +55,7 @@ def _request(self, method: str, url: str, **kwargs):
         return self._http.request(method, url, **kwargs)
 
     # ----------------------------- CRUD ---------------------------------
-    def create(self, entity_set: str, data: Union[Dict[str, Any], List[Dict[str, Any]]]) -> Union[str, List[str]]:
+    def _create(self, entity_set: str, data: Union[Dict[str, Any], List[Dict[str, Any]]]) -> Union[str, List[str]]:
         """Create one or many records.
 
         Parameters
@@ -122,7 +126,7 @@ def _logical_from_entity_set(self, entity_set: str) -> str:
         # Escape single quotes in entity set name
         es_escaped = self._escape_odata_quotes(es)
         params = {
-            "$select": "LogicalName,EntitySetName",
+            "$select": "LogicalName,EntitySetName,PrimaryIdAttribute",
             "$filter": f"EntitySetName eq '{es_escaped}'",
         }
         r = self._request("get", url, headers=self._headers(), params=params)
@@ -134,10 +138,15 @@ def _logical_from_entity_set(self, entity_set: str) -> str:
             items = []
         if not items:
             raise RuntimeError(f"Unable to resolve logical name for entity set '{es}'. Provide @odata.type explicitly.")
-        logical = items[0].get("LogicalName")
+        md = items[0]
+        logical = md.get("LogicalName")
         if not logical:
             raise RuntimeError(f"Metadata response missing LogicalName for entity set '{es}'.")
+        primary_id_attr = md.get("PrimaryIdAttribute")
         self._entityset_logical_cache[es] = logical
+        if isinstance(primary_id_attr, str) and primary_id_attr:
+            self._entityset_primaryid_cache[es] = primary_id_attr
+            self._logical_primaryid_cache[logical] = primary_id_attr
         return logical
 
     def _create_multiple(self, entity_set: str, records: List[Dict[str, Any]]) -> List[str]:
@@ -189,15 +198,17 @@ def _create_multiple(self, entity_set: str, records: List[Dict[str, Any]]) -> Li
 
     # --- Derived helpers for high-level client ergonomics ---
     def _primary_id_attr(self, entity_set: str) -> str:
-        """Return the primary id attribute name for an entity set.
-
-        Currently derived as <logicalname>id (e.g. account -> accountid).
-        Centralizing here allows future enhancement (metadata-driven or alternate key support).
-        """
+        """Return primary key attribute using metadata (fallback to <logical>id)."""
+        pid = self._entityset_primaryid_cache.get(entity_set)
+        if pid:
+            return pid
         logical = self._logical_from_entity_set(entity_set)
+        pid = self._entityset_primaryid_cache.get(entity_set) or self._logical_primaryid_cache.get(logical)
+        if pid:
+            return pid
         return f"{logical}id"
 
-    def update_by_ids(self, entity_set: str, ids: List[str], changes: Union[Dict[str, Any], List[Dict[str, Any]]]) -> None:
+    def _update_by_ids(self, entity_set: str, ids: List[str], changes: Union[Dict[str, Any], List[Dict[str, Any]]]) -> None:
         """Update many records by GUID list using UpdateMultiple under the hood.
 
         Parameters
@@ -216,7 +227,7 @@ def update_by_ids(self, entity_set: str, ids: List[str], changes: Union[Dict[str
         pk_attr = self._primary_id_attr(entity_set)
         if isinstance(changes, dict):
             batch = [{pk_attr: rid, **changes} for rid in ids]
-            self.update_multiple(entity_set, batch)
+            self._update_multiple(entity_set, batch)
             return None
         if not isinstance(changes, list):
             raise TypeError("changes must be dict or list[dict]")
@@ -227,10 +238,10 @@ def update_by_ids(self, entity_set: str, ids: List[str], changes: Union[Dict[str
             if not isinstance(patch, dict):
                 raise TypeError("Each patch must be a dict")
             batch.append({pk_attr: rid, **patch})
-        self.update_multiple(entity_set, batch)
+        self._update_multiple(entity_set, batch)
         return None
 
-    def delete_many(self, entity_set: str, ids: List[str]) -> None:
+    def _delete_multiple(self, entity_set: str, ids: List[str]) -> None:
         """Delete many records by GUID list (simple loop; potential future optimization point)."""
         if not isinstance(ids, list):
             raise TypeError("ids must be list[str]")
@@ -253,7 +264,7 @@ def esc(match):
             return f"({k})"
         return f"({k})"
 
-    def update(self, entity_set: str, key: str, data: Dict[str, Any]) -> None:
+    def _update(self, entity_set: str, key: str, data: Dict[str, Any]) -> None:
         """Update an existing record.
 
         Parameters
@@ -275,7 +286,7 @@ def update(self, entity_set: str, key: str, data: Dict[str, Any]) -> None:
         r = self._request("patch", url, headers=headers, json=data)
         r.raise_for_status()
 
-    def update_multiple(self, entity_set: str, records: List[Dict[str, Any]]) -> None:
+    def _update_multiple(self, entity_set: str, records: List[Dict[str, Any]]) -> None:
         """Bulk update existing records via the collection-bound UpdateMultiple action.
 
         Parameters
@@ -325,18 +336,18 @@ def update_multiple(self, entity_set: str, records: List[Dict[str, Any]]) -> Non
         headers = self._headers().copy()
         r = self._request("post", url, headers=headers, json=payload)
         r.raise_for_status()
-    # Intentionally ignore response content: no stable contract for IDs across environments.
+        # Intentionally ignore response content: no stable contract for IDs across environments.
         return None
 
-    def delete(self, entity_set: str, key: str) -> None:
+    def _delete(self, entity_set: str, key: str) -> None:
         """Delete a record by GUID or alternate key."""
         url = f"{self.api}/{entity_set}{self._format_key(key)}"
         headers = self._headers().copy()
         headers["If-Match"] = "*"
         r = self._request("delete", url, headers=headers)
         r.raise_for_status()
 
-    def get(self, entity_set: str, key: str, select: Optional[str] = None) -> Dict[str, Any]:
+    def _get(self, entity_set: str, key: str, select: Optional[str] = None) -> Dict[str, Any]:
         """Retrieve a single record.
 
         Parameters
@@ -356,7 +367,7 @@ def get(self, entity_set: str, key: str, select: Optional[str] = None) -> Dict[s
         r.raise_for_status()
         return r.json()
 
-    def get_multiple(
+    def _get_multiple(
         self,
         entity_set: str,
         select: Optional[List[str]] = None,
@@ -435,7 +446,7 @@ def _do_request(url: str, *, params: Optional[Dict[str, Any]] = None) -> Dict[st
             next_link = data.get("@odata.nextLink") or data.get("odata.nextLink") if isinstance(data, dict) else None
 
     # --------------------------- SQL Custom API -------------------------
-    def query_sql(self, sql: str) -> list[dict[str, Any]]:
+    def _query_sql(self, sql: str) -> list[dict[str, Any]]:
         """Execute a read-only SQL query using the Dataverse Web API `?sql=` capability.
 
         The platform supports a constrained subset of SQL SELECT statements directly on entity set endpoints:
@@ -532,7 +543,7 @@ def _entity_set_from_logical(self, logical: str) -> str:
         url = f"{self.api}/EntityDefinitions"
         logical_escaped = self._escape_odata_quotes(logical)
         params = {
-            "$select": "LogicalName,EntitySetName",
+            "$select": "LogicalName,EntitySetName,PrimaryIdAttribute",
             "$filter": f"LogicalName eq '{logical_escaped}'",
         }
         r = self._request("get", url, headers=self._headers(), params=params)
@@ -544,10 +555,15 @@ def _entity_set_from_logical(self, logical: str) -> str:
             items = []
         if not items:
             raise RuntimeError(f"Unable to resolve entity set for logical name '{logical}'.")
-        es = items[0].get("EntitySetName")
+        md = items[0]
+        es = md.get("EntitySetName")
         if not es:
             raise RuntimeError(f"Metadata response missing EntitySetName for logical '{logical}'.")
         self._logical_to_entityset_cache[logical] = es
+        primary_id_attr = md.get("PrimaryIdAttribute")
+        if isinstance(primary_id_attr, str) and primary_id_attr:
+            self._logical_primaryid_cache[logical] = primary_id_attr
+            self._entityset_primaryid_cache[es] = primary_id_attr
         return es
 
     # ---------------------- Table metadata helpers ----------------------
@@ -690,7 +706,7 @@ def _attribute_payload(self, schema_name: str, dtype: str, *, is_primary_name: b
             }
         return None
 
-    def get_table_info(self, tablename: str) -> Optional[Dict[str, Any]]:
+    def _get_table_info(self, tablename: str) -> Optional[Dict[str, Any]]:
         """Return basic metadata for a custom table if it exists.
 
         Parameters
@@ -714,7 +730,7 @@ def get_table_info(self, tablename: str) -> Optional[Dict[str, Any]]:
             "columns_created": [],
         }
     
-    def list_tables(self) -> List[Dict[str, Any]]:
+    def _list_tables(self) -> List[Dict[str, Any]]:
         """List all tables in the Dataverse, excluding private tables (IsPrivate=true)."""
         url = f"{self.api}/EntityDefinitions"
         params = {
@@ -724,7 +740,7 @@ def list_tables(self) -> List[Dict[str, Any]]:
         r.raise_for_status()
         return r.json().get("value", [])
 
-    def delete_table(self, tablename: str) -> None:
+    def _delete_table(self, tablename: str) -> None:
         schema_name = tablename if "_" in tablename else f"new_{self._to_pascal(tablename)}"
         entity_schema = schema_name
         ent = self._get_entity_by_schema(entity_schema)
@@ -736,7 +752,7 @@ def delete_table(self, tablename: str) -> None:
         r = self._request("delete", url, headers=headers)
         r.raise_for_status()
 
-    def create_table(self, tablename: str, schema: Dict[str, str]) -> Dict[str, Any]:
+    def _create_table(self, tablename: str, schema: Dict[str, str]) -> Dict[str, Any]:
         # Accept a friendly name and construct a default schema under 'new_'.
         # If a full SchemaName is passed (contains '_'), use as-is.
         entity_schema = tablename if "_" in tablename else f"new_{self._to_pascal(tablename)}"
diff --git a/tests/test_create_single_guid.py b/tests/test_create_single_guid.py
