Repository: gtalarico/pyairtable
Pull request #387 (2024-09-05 01:32:46)

Model.save() returns SaveResult instead of bool
There is more context about a save operation that might be useful to a caller than just "was a record created?" especially now that #381 has been merged. This branch introduces a new class `SaveResult` which contains all the context on what happened during the save operation.

For backwards compatibility with the prior behavior of `Model.save()`, instances of `SaveResult` behave truthy if the record was created and falsy if the record was not created (though this will emit a deprecation warning). Applications which perform type checking may still require some changes.


Patch:
diff --git a/docs/source/api.rst b/docs/source/api.rst
index fcfa1aa..18cfe4e 100644
--- a/docs/source/api.rst
+++ b/docs/source/api.rst
@@ -95,6 +95,9 @@ API: pyairtable.orm
 .. autoclass:: pyairtable.orm.Model
     :members:
 
+.. autoclass:: pyairtable.orm.SaveResult
+    :members:
+
 
 API: pyairtable.orm.fields
 *******************************
diff --git a/docs/source/changelog.rst b/docs/source/changelog.rst
index 3b83cf2..c0e4041 100644
--- a/docs/source/changelog.rst
+++ b/docs/source/changelog.rst
@@ -35,6 +35,14 @@ Changelog
   - `PR #373 <https://github.com/gtalarico/pyairtable/pull/373>`_
 * Added command line utility and ORM module generator. See :doc:`cli`.
   - `PR #376 <https://github.com/gtalarico/pyairtable/pull/376>`_
+* Changed the behavior of :meth:`Model.save <pyairtable.orm.Model.save>`
+  to no longer send unmodified field values to the API.
+  - `PR #381 <https://github.com/gtalarico/pyairtable/pull/381>`_
+* Added ``use_field_ids=`` parameter to :class:`~pyairtable.Api`.
+  - `PR #386 <https://github.com/gtalarico/pyairtable/pull/386>`_
+* Changed the return type of :meth:`Model.save <pyairtable.orm.Model.save>`
+  from ``bool`` to :class:`~pyairtable.orm.SaveResult`.
+  - `PR #387 <https://github.com/gtalarico/pyairtable/pull/387>`_
 
 2.3.3 (2024-03-22)
 ------------------------
diff --git a/docs/source/migrations.rst b/docs/source/migrations.rst
index 9575125..fa6e9d9 100644
--- a/docs/source/migrations.rst
+++ b/docs/source/migrations.rst
@@ -68,6 +68,10 @@ Changes to the ORM in 3.0
 :data:`Model.created_time <pyairtable.orm.Model.created_time>` is now a ``datetime`` (or ``None``)
 instead of ``str``. This change also applies to all timestamp fields used in :ref:`API: pyairtable.models`.
 
+:meth:`Model.save <pyairtable.orm.Model.save>` now only saves changed fields to the API, which
+means it will sometimes not perform any network traffic (though this behavior can be overridden).
+It also now returns an instance of :class:`~pyairtable.orm.SaveResult` instead of ``bool``.
+
 The 3.0 release has changed the API for retrieving ORM model configuration:
 
 .. list-table::
diff --git a/pyairtable/orm/__init__.py b/pyairtable/orm/__init__.py
index b85a24f..ab7ad1d 100644
--- a/pyairtable/orm/__init__.py
+++ b/pyairtable/orm/__init__.py
@@ -1,7 +1,8 @@
 from . import fields
-from .model import Model
+from .model import Model, SaveResult
 
 __all__ = [
     "Model",
+    "SaveResult",
     "fields",
 ]
diff --git a/pyairtable/orm/model.py b/pyairtable/orm/model.py
index 88313c1..12ceaec 100644
--- a/pyairtable/orm/model.py
+++ b/pyairtable/orm/model.py
@@ -1,4 +1,6 @@
+import dataclasses
 import datetime
+import warnings
 from dataclasses import dataclass
 from functools import cached_property
 from typing import (
@@ -10,6 +12,7 @@ from typing import (
     List,
     Mapping,
     Optional,
+    Set,
     Type,
     Union,
     cast,
@@ -225,7 +228,7 @@ class Model:
         """
         return bool(self.id)
 
-    def save(self, *, force: bool = False) -> bool:
+    def save(self, *, force: bool = False) -> "SaveResult":
         """
         Save the model to the API.
 
@@ -235,10 +238,6 @@ class Model:
 
         Args:
             force: If ``True``, all fields will be saved, even if they have not changed.
-
-        Returns:
-            ``True`` if a record was created;
-            ``False`` if it was updated, or if the model had no changes.
         """
         if self._deleted:
             raise RuntimeError(f"{self.id} was deleted")
@@ -250,11 +249,11 @@ class Model:
             self.id = record["id"]
             self.created_time = datetime_from_iso_str(record["createdTime"])
             self._changed.clear()
-            return True
+            return SaveResult(self.id, created=True, field_names=set(field_values))
 
         if not force:
             if not self._changed:
-                return False
+                return SaveResult(self.id)
             field_values = {
                 field_name: value
                 for field_name, value in field_values.items()
@@ -263,7 +262,9 @@ class Model:
 
         self.meta.table.update(self.id, field_values, typecast=self.meta.typecast)
         self._changed.clear()
-        return False
+        return SaveResult(
+            self.id, forced=force, updated=True, field_names=set(field_values)
+        )
 
     def delete(self) -> bool:
         """
@@ -632,3 +633,65 @@ class _Meta:
             "time_zone": None,
             "use_field_ids": self.use_field_ids,
         }
+
+
+@dataclass(frozen=True)
+class SaveResult:
+    """
+    Represents the result of saving a record to the API. The result's
+    attributes contain more granular information about the save operation:
+
+        >>> result = model.save()
+        >>> result.record_id
+        'recWPqD9izdsNvlE'
+        >>> result.created
+        False
+        >>> result.updated
+        True
+        >>> result.forced
+        False
+        >>> result.field_names
+        {'Name', 'Email'}
+
+    If none of the model's fields have changed, calling :meth:`~pyairtable.orm.Model.save`
+    will not perform any API requests and will return a SaveResult with no changes.
+
+        >>> model = YourModel()
+        >>> result = model.save()
+        >>> result.saved
+        True
+        >>> second_result = model.save()
+        >>> second_result.saved
+        False
+
+    For backwards compatibility, instances of SaveResult will evaluate as truthy
+    if the record was created, and falsy if the record was not created.
+    """
+
+    record_id: RecordId
+    created: bool = False
+    updated: bool = False
+    forced: bool = False
+    field_names: Set[FieldName] = dataclasses.field(default_factory=set)
+
+    def __bool__(self) -> bool:
+        """
+        Returns ``True`` if the record was created. This is for backwards compatibility
+        with the behavior of :meth:`~pyairtable.orm.Model.save` prior to the 3.0 release,
+        which returned a boolean indicating whether a record was created.
+        """
+        warnings.warn(
+            "Model.save() now returns SaveResult instead of bool; switch"
+            " to checking Model.save().created instead before the 4.0 release.",
+            DeprecationWarning,
+        )
+        return self.created
+
+    @property
+    def saved(self) -> bool:
+        """
+        Whether the record was saved to the API. If ``False``, this indicates there
+        were no changes to the model and the :meth:`~pyairtable.orm.Model.save`
+        operation was not forced.
+        """
+        return self.created or self.updated


Test patch:
diff --git a/tests/test_orm.py b/tests/test_orm.py
index 70ff53e..4f39b34 100644
--- a/tests/test_orm.py
+++ b/tests/test_orm.py
@@ -62,7 +62,7 @@ def test_model_basics():
     # save
     with mock.patch.object(Table, "create") as m_save:
         m_save.return_value = {"id": "id", "createdTime": NOW}
-        contact.save()
+        assert contact.save().created
 
     assert m_save.called
     assert contact.id == "id"
@@ -163,7 +163,8 @@ def test_unmodified_field_not_saved(contact_record):
 
     # Do not call update() if the record is unchanged
     with mock_update_contact() as m_update:
-        contact.save()
+        result = contact.save()
+        assert not (result.created or result.updated)
         m_update.assert_not_called()
 
     # By default, only pass fields which were changed to the API
diff --git a/tests/test_orm_model.py b/tests/test_orm_model.py
index 6b25bd5..32d5eee 100644
--- a/tests/test_orm_model.py
+++ b/tests/test_orm_model.py
@@ -1,3 +1,4 @@
+from datetime import datetime, timezone
 from functools import partial
 from unittest import mock
 
@@ -6,9 +7,22 @@ from requests_mock import Mocker
 
 from pyairtable.orm import Model
 from pyairtable.orm import fields as f
+from pyairtable.orm.model import SaveResult
 from pyairtable.testing import fake_id, fake_meta, fake_record
 
 
+class FakeModel(Model):
+    Meta = fake_meta()
+    one = f.TextField("one")
+    two = f.TextField("two")
+
+
+class FakeModelByIds(Model):
+    Meta = fake_meta(use_field_ids=True, table_name="Apartments")
+    Name = f.TextField("fld1VnoyuotSTyxW1")
+    Age = f.NumberField("fld2VnoyuotSTy4g6")
+
+
 @pytest.fixture(autouse=True)
 def no_requests(requests_mock):
     """
@@ -126,18 +140,6 @@ def test_model_overlapping(name):
         )
 
 
-class FakeModel(Model):
-    Meta = fake_meta()
-    one = f.TextField("one")
-    two = f.TextField("two")
-
-
-class FakeModelByIds(Model):
-    Meta = fake_meta(use_field_ids=True, table_name="Apartments")
-    Name = f.TextField("fld1VnoyuotSTyxW1")
-    Age = f.NumberField("fld2VnoyuotSTy4g6")
-
-
 def test_repr():
     record = fake_record()
     assert repr(FakeModel.from_record(record)) == f"<FakeModel id='{record['id']}'>"
@@ -375,3 +377,84 @@ def test_dynamic_model_meta():
     assert f.meta.base_id == data["base_id"]
     assert f.meta.table_name == data["table_name"]
     Fake.Meta.table_name.assert_called_once()
+
+
+@mock.patch("pyairtable.Table.create")
+def test_save__create(mock_create):
+    """
+    Test that we can save a model instance we've created.
+    """
+    mock_create.return_value = {
+        "id": fake_id,
+        "createdTime": datetime.now(timezone.utc).isoformat(),
+        "fields": {"one": "ONE", "two": "TWO"},
+    }
+    obj = FakeModel(one="ONE", two="TWO")
+    result = obj.save()
+    assert result.saved
+    assert result.created
+    assert result.field_names == {"one", "two"}
+    assert not result.updated
+    assert not result.forced
+    mock_create.assert_called_once_with({"one": "ONE", "two": "TWO"}, typecast=True)
+
+
+@mock.patch("pyairtable.Table.update")
+def test_save__update(mock_update):
+    """
+    Test that we can save a model instance that already exists.
+    """
+    obj = FakeModel.from_record(fake_record(one="ONE", two="TWO"))
+    obj.one = "new value"
+    result = obj.save()
+    assert result.saved
+    assert not result.created
+    assert result.updated
+    assert result.field_names == {"one"}
+    assert not result.forced
+    mock_update.assert_called_once_with(obj.id, {"one": "new value"}, typecast=True)
+
+
+@mock.patch("pyairtable.Table.update")
+def test_save__update_force(mock_update):
+    """
+    Test that we can save a model instance that already exists,
+    and we can force saving all values to the API.
+    """
+    obj = FakeModel.from_record(fake_record(one="ONE", two="TWO"))
+    obj.one = "new value"
+    result = obj.save(force=True)
+    assert result.saved
+    assert not result.created
+    assert result.updated
+    assert result.forced
+    assert result.field_names == {"one", "two"}
+    mock_update.assert_called_once_with(
+        obj.id, {"one": "new value", "two": "TWO"}, typecast=True
+    )
+
+
+@mock.patch("pyairtable.Table.update")
+def test_save__noop(mock_update):
+    """
+    Test that if a model is unchanged, we don't try to save it to the API.
+    """
+    obj = FakeModel.from_record(fake_record(one="ONE", two="TWO"))
+    result = obj.save()
+    assert not result.saved
+    assert not result.created
+    assert not result.updated
+    assert not result.field_names
+    assert not result.forced
+    mock_update.assert_not_called()
+
+
+def test_save_bool_deprecated():
+    """
+    Test that SaveResult instances can be used as booleans, but emit a deprecation warning.
+    """
+    with pytest.deprecated_call():
+        assert bool(SaveResult(fake_id(), created=False)) is False
+
+    with pytest.deprecated_call():
+        assert bool(SaveResult(fake_id(), created=True)) is True