Repository: gtalarico/pyairtable
Pull request #308 (2023-09-24 04:54:49)

Relax type annotation on batch_upsert (and other batch methods)
This branch addresses two issues with the type hints we rolled out in 2.0:

1. The `batch_upsert` method only accepts records _with_ an ID, not _without_ an ID. (Fixing this got complicated due to how mypy interprets lists of dicts, and I wound up simply relaxing it to `List[Dict[str, Any]]` instead of something like `List[Union[RecordDict, CreateRecordDict, UpdateRecordDict]]`.
2. All of our `batch_*` methods only accept lists, not other iterable types. That means, among other things, that generator expressions can't be passed directly into these functions. This branch fixes that.

Both changes have test coverage, but I may have missed something. Eyes welcome!


Patch:
diff --git a/pyairtable/api/table.py b/pyairtable/api/table.py
index b5cb717..0ac1328 100644
--- a/pyairtable/api/table.py
+++ b/pyairtable/api/table.py
@@ -1,7 +1,7 @@
 import posixpath
 import urllib.parse
 import warnings
-from typing import Any, Iterator, List, Optional, Union, overload
+from typing import Any, Dict, Iterable, Iterator, List, Optional, Union, overload
 
 import pyairtable.models
 from pyairtable.api.retrying import Retry
@@ -260,7 +260,7 @@ class Table:
 
     def batch_create(
         self,
-        records: List[WritableFields],
+        records: Iterable[WritableFields],
         typecast: bool = False,
         return_fields_by_field_id: bool = False,
     ) -> List[RecordDict]:
@@ -282,12 +282,15 @@ class Table:
         ]
 
         Args:
-            records: List of dicts representing records to be created.
+            records: Iterable of dicts representing records to be created.
             typecast: |kwarg_typecast|
             return_fields_by_field_id: |kwarg_return_fields_by_field_id|
         """
         inserted_records = []
 
+        # If we got an iterator, exhaust it and collect it into a list.
+        records = list(records)
+
         for chunk in self.api.chunked(records):
             new_records = [{"fields": fields} for fields in chunk]
             response = self.api.request(
@@ -334,7 +337,7 @@ class Table:
 
     def batch_update(
         self,
-        records: List[UpdateRecordDict],
+        records: Iterable[UpdateRecordDict],
         replace: bool = False,
         typecast: bool = False,
         return_fields_by_field_id: bool = False,
@@ -354,6 +357,9 @@ class Table:
         updated_records = []
         method = "put" if replace else "patch"
 
+        # If we got an iterator, exhaust it and collect it into a list.
+        records = list(records)
+
         for chunk in self.api.chunked(records):
             chunk_records = [{"id": x["id"], "fields": x["fields"]} for x in chunk]
             response = self.api.request(
@@ -371,7 +377,7 @@ class Table:
 
     def batch_upsert(
         self,
-        records: List[UpdateRecordDict],
+        records: Iterable[Dict[str, Any]],
         key_fields: List[FieldName],
         replace: bool = False,
         typecast: bool = False,
@@ -395,6 +401,9 @@ class Table:
         Returns:
             Lists of created/updated record IDs, along with the list of all records affected.
         """
+        # If we got an iterator, exhaust it and collect it into a list.
+        records = list(records)
+
         # The API will reject a request where a record is missing any of fieldsToMergeOn,
         # but we might not reach that error until we've done several batch operations.
         # To spare implementers from having to recover from a partially applied upsert,
@@ -454,7 +463,7 @@ class Table:
             self.api.request("delete", self.record_url(record_id)),
         )
 
-    def batch_delete(self, record_ids: List[RecordId]) -> List[RecordDeletedDict]:
+    def batch_delete(self, record_ids: Iterable[RecordId]) -> List[RecordDeletedDict]:
         """
         Deletes the given records, operating in batches.
 
@@ -472,6 +481,9 @@ class Table:
         """
         deleted_records = []
 
+        # If we got an iterator, exhaust it and collect it into a list.
+        record_ids = list(record_ids)
+
         for chunk in self.api.chunked(record_ids):
             result = self.api.request("delete", self.url, params={"records[]": chunk})
             deleted_records += assert_typed_dicts(RecordDeletedDict, result["records"])
diff --git a/pyairtable/formulas.py b/pyairtable/formulas.py
index 4531bda..3e9e1f5 100644
--- a/pyairtable/formulas.py
+++ b/pyairtable/formulas.py
@@ -170,7 +170,7 @@ def FIND(what: str, where: str, start_position: int = 0) -> str:
     """
     Creates a FIND statement
 
-    >>> FIND(STR_VALUE(2021), FIELD('DatetimeCol'))
+    >>> FIND(STR(2021), FIELD('DatetimeCol'))
     "FIND('2021', {DatetimeCol})"
 
     Args:


Test patch:
diff --git a/tests/test_api_table.py b/tests/test_api_table.py
index 87888e4..dda1632 100644
--- a/tests/test_api_table.py
+++ b/tests/test_api_table.py
@@ -215,7 +215,8 @@ def test_create(table: Table, mock_response_single):
     assert dict_equals(resp, mock_response_single)
 
 
-def test_batch_create(table: Table, mock_records):
+@pytest.mark.parametrize("container", [list, tuple, iter])
+def test_batch_create(table: Table, container, mock_records):
     with Mocker() as mock:
         for chunk in _chunk(mock_records, 10):
             mock.post(
@@ -224,7 +225,7 @@ def test_batch_create(table: Table, mock_records):
                 json={"records": chunk},
             )
         records = [i["fields"] for i in mock_records]
-        resp = table.batch_create(records)
+        resp = table.batch_create(container(records))
     assert seq_equals(resp, mock_records)
 
 
@@ -244,8 +245,9 @@ def test_update(table: Table, mock_response_single, replace, http_method):
     assert dict_equals(resp, mock_response_single)
 
 
+@pytest.mark.parametrize("container", [list, tuple, iter])
 @pytest.mark.parametrize("replace,http_method", [(False, "PATCH"), (True, "PUT")])
-def test_batch_update(table: Table, replace, http_method):
+def test_batch_update(table: Table, container, replace, http_method):
     records = [fake_record(fieldvalue=index) for index in range(50)]
     with Mocker() as mock:
         mock.register_uri(
@@ -255,13 +257,14 @@ def test_batch_update(table: Table, replace, http_method):
                 {"json": {"records": chunk}} for chunk in table.api.chunked(records)
             ],
         )
-        resp = table.batch_update(records, replace=replace)
+        resp = table.batch_update(container(records), replace=replace)
 
     assert resp == records
 
 
+@pytest.mark.parametrize("container", [list, tuple, iter])
 @pytest.mark.parametrize("replace,http_method", [(False, "PATCH"), (True, "PUT")])
-def test_batch_upsert(table: Table, replace, http_method, monkeypatch):
+def test_batch_upsert(table: Table, container, replace, http_method, monkeypatch):
     field_name = "Name"
     exists1 = fake_record({field_name: "Exists 1"})
     exists2 = fake_record({field_name: "Exists 2"})
@@ -283,7 +286,9 @@ def test_batch_upsert(table: Table, replace, http_method, monkeypatch):
             response_list=[{"json": response} for response in responses],
         )
         monkeypatch.setattr(table.api, "MAX_RECORDS_PER_REQUEST", 1)
-        resp = table.batch_upsert(payload, key_fields=[field_name], replace=replace)
+        resp = table.batch_upsert(
+            container(payload), key_fields=[field_name], replace=replace
+        )
 
     assert resp == {
         "createdRecords": [created["id"]],
@@ -311,7 +316,8 @@ def test_delete(table: Table, mock_response_single):
     assert resp == expected
 
 
-def test_batch_delete(table: Table, mock_records):
+@pytest.mark.parametrize("container", [list, tuple, iter])
+def test_batch_delete(table: Table, container, mock_records):
     ids = [i["id"] for i in mock_records]
     with Mocker() as mock:
         for chunk in _chunk(ids, 10):
@@ -325,7 +331,7 @@ def test_batch_delete(table: Table, mock_records):
                 json=json_response,
             )
 
-        resp = table.batch_delete(ids)
+        resp = table.batch_delete(container(ids))
     expected = [{"deleted": True, "id": i} for i in ids]
     assert resp == expected
 
diff --git a/tests/test_typing.py b/tests/test_typing.py
index 549ad81..4c8876d 100644
--- a/tests/test_typing.py
+++ b/tests/test_typing.py
@@ -55,6 +55,16 @@ if TYPE_CHECKING:
     table.update(record_id, {"Field Name": {"email": "alice@example.com"}})
     table.update(record_id, {"Field Name": ["rec1", "rec2", "rec3"]})
 
+    # Ensure batch_upsert takes both records with and without IDs
+    table.batch_upsert(
+        [
+            {"fields": {"Name": "Carol"}},
+            {"id": "recAsdf", "fields": {"Name": "Bob"}},
+            {"id": "recAsdf", "createdTime": "", "fields": {"Name": "Alice"}},
+        ],
+        key_fields=["Name"],
+    )
+
     # Test type annotations for the ORM
     class Actor(orm.Model):
         name = orm.fields.TextField("Name")