From ff4fac4131819862fcb88c4e3f7597270e0889c1 Mon Sep 17 00:00:00 2001 From: Alex Levy Date: Wed, 13 Mar 2024 12:10:03 -0700 Subject: [PATCH 1/4] Return `""` and `False` from TextField, CheckboxField --- pyairtable/orm/fields.py | 75 +++++++++++++++-------- tests/integration/test_integration_orm.py | 2 +- tests/test_orm.py | 30 ++++----- tests/test_orm_fields.py | 48 +++++++++++++++ tests/test_typing.py | 18 +++--- 5 files changed, 121 insertions(+), 52 deletions(-) diff --git a/pyairtable/orm/fields.py b/pyairtable/orm/fields.py index 3bf18054..a5df6c35 100644 --- a/pyairtable/orm/fields.py +++ b/pyairtable/orm/fields.py @@ -64,19 +64,22 @@ _ClassInfo: TypeAlias = Union[type, Tuple["_ClassInfo", ...]] T = TypeVar("T") -T_Linked = TypeVar("T_Linked", bound="Model") +T_Linked = TypeVar("T_Linked", bound="Model") # used by LinkField T_API = TypeVar("T_API") # type used to exchange values w/ Airtable API T_ORM = TypeVar("T_ORM") # type used to store values internally +T_Missing = TypeVar("T_Missing") # type returned when Airtable has no value -class Field(Generic[T_API, T_ORM], metaclass=abc.ABCMeta): +class Field(Generic[T_API, T_ORM, T_Missing], metaclass=abc.ABCMeta): """ A generic class for an Airtable field descriptor that will be included in an ORM model. - Type-checked subclasses should provide two type parameters, - ``T_API`` and ``T_ORM``, which indicate the type returned - by the API and the type used to store values internally. + Type-checked subclasses should provide three type parameters: + + * ``T_API``, indicating the JSON-serializable type returned by the API + * ``T_ORM``, indicating the type used to store values internally + * ``T_Missing``, indicating the type of value returned if the field is empty Subclasses should also define ``valid_types`` as a type or tuple of types, which will be used to validate the type @@ -149,11 +152,13 @@ def __get__(self, instance: None, owner: Type[Any]) -> SelfType: ... # obj.field will call __get__(instance=obj, owner=Model) @overload - def __get__(self, instance: "Model", owner: Type[Any]) -> Optional[T_ORM]: ... + def __get__( + self, instance: "Model", owner: Type[Any] + ) -> Union[T_ORM, T_Missing]: ... def __get__( self, instance: Optional["Model"], owner: Type[Any] - ) -> Union[SelfType, Optional[T_ORM]]: + ) -> Union[SelfType, T_ORM, T_Missing]: # allow calling Model.field to get the field object instead of a value if not instance: return self @@ -173,8 +178,12 @@ def __set__(self, instance: "Model", value: Optional[T_ORM]) -> None: def __delete__(self, instance: "Model") -> None: raise AttributeError(f"cannot delete {self._description}") - def _missing_value(self) -> Optional[T_ORM]: - return None + @classmethod + def _missing_value(cls) -> T_Missing: + # This assumes Field[T_API, T_ORM, None]. If a subclass defines T_Missing as + # something different, it needs to override _missing_value. + # This can be tidied in 3.13 with T_Missing(default=None). See PEP-696. + return cast(T_Missing, None) def to_record_value(self, value: Any) -> Any: """ @@ -249,17 +258,36 @@ def lte(self, value: Any) -> "formulas.Comparison": return formulas.LTE(self, value) -#: A generic Field whose internal and API representations are the same type. -_BasicField: TypeAlias = Field[T, T] +class _FieldWithTypedDefaultValue(Generic[T], Field[T, T, T]): + """ + A generic Field with default value of the same type as internal and API representations. + + For now this is used for TextField and CheckboxField, because Airtable stores the empty + values for those types ("" and False) internally as None. + """ + + @classmethod + def _missing_value(cls) -> T: + first_type = cls.valid_types + while isinstance(first_type, tuple): + if not first_type: + raise RuntimeError(f"{cls.__qualname__}.valid_types is malformed") + first_type = first_type[0] + return cast(T, first_type()) + + +#: A generic Field with internal and API representations that are the same type. +_BasicField: TypeAlias = Field[T, T, None] #: An alias for any type of Field. -AnyField: TypeAlias = _BasicField[Any] +AnyField: TypeAlias = Field[Any, Any, Any] -class TextField(_BasicField[str]): +class TextField(_FieldWithTypedDefaultValue[str]): """ - Used for all Airtable text fields. Accepts ``str``. + Accepts ``str``. + Returns ``""`` instead of ``None`` if the field is empty on the Airtable base. See `Single line text `__ and `Long text `__. @@ -329,8 +357,9 @@ def valid_or_raise(self, value: int) -> None: raise ValueError("rating cannot be below 1") -class CheckboxField(_BasicField[bool]): +class CheckboxField(_FieldWithTypedDefaultValue[bool]): """ + Accepts ``bool``. Returns ``False`` instead of ``None`` if the field is empty on the Airtable base. See `Checkbox `__. @@ -338,11 +367,8 @@ class CheckboxField(_BasicField[bool]): valid_types = bool - def _missing_value(self) -> bool: - return False - -class DatetimeField(Field[str, datetime]): +class DatetimeField(Field[str, datetime, None]): """ DateTime field. Accepts only `datetime `_ values. @@ -364,7 +390,7 @@ def to_internal_value(self, value: str) -> datetime: return utils.datetime_from_iso_str(value) -class DateField(Field[str, date]): +class DateField(Field[str, date, None]): """ Date field. Accepts only `date `_ values. @@ -386,7 +412,7 @@ def to_internal_value(self, value: str) -> date: return utils.date_from_iso_str(value) -class DurationField(Field[int, timedelta]): +class DurationField(Field[int, timedelta, None]): """ Duration field. Accepts only `timedelta `_ values. @@ -418,7 +444,7 @@ class _DictField(Generic[T], _BasicField[T]): valid_types = dict -class _ListField(Generic[T_API, T_ORM], Field[List[T_API], List[T_ORM]]): +class _ListField(Generic[T_API, T_ORM], Field[List[T_API], List[T_ORM], List[T_ORM]]): """ Generic type for a field that stores a list of values. Can be used to refer to a lookup field that might return more than one value. @@ -455,11 +481,6 @@ def _get_list_value(self, instance: "Model") -> List[T_ORM]: instance._fields[self.field_name] = value return value - def to_internal_value(self, value: Optional[List[T_ORM]]) -> List[T_ORM]: - if value is None: - value = [] - return value - class _ValidatingListField(Generic[T], _ListField[T, T]): contains_type: Type[T] diff --git a/tests/integration/test_integration_orm.py b/tests/integration/test_integration_orm.py index 6300f34e..49455927 100644 --- a/tests/integration/test_integration_orm.py +++ b/tests/integration/test_integration_orm.py @@ -219,7 +219,7 @@ def test_every_field(Everything): # The ORM won't refresh the model's field values after save() assert record.formula_integer is None - assert record.formula_nan is None + assert record.formula_nan == "" assert record.link_count is None assert record.lookup_error == [] assert record.lookup_integer == [] diff --git a/tests/test_orm.py b/tests/test_orm.py index ac90f142..4826d4f3 100644 --- a/tests/test_orm.py +++ b/tests/test_orm.py @@ -15,7 +15,7 @@ class Address(Model): Meta = fake_meta(table_name="Address") street = f.TextField("Street") - number = f.TextField("Number") + number = f.IntegerField("Number") class Contact(Model): @@ -73,7 +73,7 @@ def test_unsupplied_fields(): """ a = Address() assert a.number is None - assert a.street is None + assert a.street == "" def test_null_fields(): @@ -188,7 +188,7 @@ def test_linked_record_can_be_saved(requests_mock, access_linked_records): record IDs into instances of the model. This could interfere with save(), so this test ensures we don't regress the capability. """ - address_json = fake_record(Number="123", Street="Fake St") + address_json = fake_record(Number=123, Street="Fake St") address_id = address_json["id"] address_url_re = re.escape(Address.get_table().url + "?filterByFormula=") contact_json = fake_record(Email="alice@example.com", Link=[address_id]) @@ -246,7 +246,7 @@ def test_undeclared_field(requests_mock, test_case): """ record = fake_record( - Number="123", + Number=123, Street="Fake St", City="Springfield", State="IL", @@ -265,7 +265,7 @@ def test_undeclared_field(requests_mock, test_case): _, get_model_instance = test_case instance = get_model_instance(Address, record["id"]) - assert instance.to_record()["fields"] == {"Number": "123", "Street": "Fake St"} + assert instance.to_record()["fields"] == {"Number": 123, "Street": "Fake St"} @mock.patch("pyairtable.Table.batch_create") @@ -275,19 +275,19 @@ def test_batch_save(mock_update, mock_create): Test that we can pass multiple unsaved Model instances (or dicts) to batch_save and it will create or update them all in as few requests as possible. """ - addr1 = Address(number="123", street="Fake St") - addr2 = Address(number="456", street="Fake St") + addr1 = Address(number=123, street="Fake St") + addr2 = Address(number=456, street="Fake St") addr3 = Address.from_record( { "id": "recExistingRecord", "createdTime": datetime.utcnow().isoformat(), - "fields": {"Number": "789", "Street": "Fake St"}, + "fields": {"Number": 789, "Street": "Fake St"}, } ) mock_create.return_value = [ - fake_record(id="abc", Number="123", Street="Fake St"), - fake_record(id="def", Number="456", Street="Fake St"), + fake_record(id="abc", Number=123, Street="Fake St"), + fake_record(id="def", Number=456, Street="Fake St"), ] # Just like model.save(), Model.batch_save() will set IDs on new records. @@ -298,8 +298,8 @@ def test_batch_save(mock_update, mock_create): mock_create.assert_called_once_with( [ - {"Number": "123", "Street": "Fake St"}, - {"Number": "456", "Street": "Fake St"}, + {"Number": 123, "Street": "Fake St"}, + {"Number": 456, "Street": "Fake St"}, ], typecast=True, ) @@ -307,7 +307,7 @@ def test_batch_save(mock_update, mock_create): [ { "id": "recExistingRecord", - "fields": {"Number": "789", "Street": "Fake St"}, + "fields": {"Number": 789, "Street": "Fake St"}, }, ], typecast=True, @@ -366,8 +366,8 @@ def test_batch_delete__unsaved_record(mock_delete): receives any models which have not been created yet. """ addresses = [ - Address.from_record(fake_record(Number="1", Street="Fake St")), - Address(number="2", street="Fake St"), + Address.from_record(fake_record(Number=1, Street="Fake St")), + Address(number=2, street="Fake St"), ] with pytest.raises(ValueError): Address.batch_delete(addresses) diff --git a/tests/test_orm_fields.py b/tests/test_orm_fields.py index fd14d6f4..96c27128 100644 --- a/tests/test_orm_fields.py +++ b/tests/test_orm_fields.py @@ -679,3 +679,51 @@ def patch_callback(request, context): obj.dt = datetime.datetime(2024, 3, 1, 11, 22, 33) obj.save() assert m.last_request.json()["fields"]["dt"] == "2024-03-01T11:22:33.000" + + +@pytest.mark.parametrize( + "classinfo,expected", + [ + (str, ""), + ((str, bool), ""), + ((((str,),),), ""), + (bool, False), + ], +) +def test_missing_value(classinfo, expected): + """ + Test that _FieldWithTypedDefaultValue._missing_value finds the first + valid type and calls it to create the "missing from Airtable" value. + """ + + class F(f._FieldWithTypedDefaultValue): + valid_types = classinfo + + class T: + the_field = F("Field Name") + + assert T().the_field == expected + + +@pytest.mark.parametrize( + "classinfo,exc_class", + [ + ((), RuntimeError), + ((((), str), bool), RuntimeError), + ], +) +def test_missing_value__invalid_classinfo(classinfo, exc_class): + """ + Test that _FieldWithTypedDefaultValue._missing_value raises an exception + if the class's valid_types is set to an invalid value. + """ + + class F(f._FieldWithTypedDefaultValue): + valid_types = classinfo + + class T: + the_field = F("Field Name") + + obj = T() + with pytest.raises(exc_class): + obj.the_field diff --git a/tests/test_typing.py b/tests/test_typing.py index f8f0f2cb..c5a76ff8 100644 --- a/tests/test_typing.py +++ b/tests/test_typing.py @@ -71,7 +71,7 @@ class Actor(orm.Model): name = orm.fields.TextField("Name") logins = orm.fields.MultipleCollaboratorsField("Logins") - assert_type(Actor().name, Optional[str]) + assert_type(Actor().name, str) assert_type(Actor().logins, List[T.CollaboratorDict]) class Movie(orm.Model): @@ -81,11 +81,11 @@ class Movie(orm.Model): actors = orm.fields.LinkField("Actors", Actor) movie = Movie() - assert_type(movie.name, Optional[str]) + assert_type(movie.name, str) assert_type(movie.rating, Optional[int]) assert_type(movie.actors, List[Actor]) assert_type(movie.prequels, List[Movie]) - assert_type(movie.actors[0].name, Optional[str]) + assert_type(movie.actors[0].name, str) class EveryField(orm.Model): aitext = orm.fields.AITextField("AI Generated Text") @@ -123,7 +123,7 @@ class EveryField(orm.Model): assert_type(record.autonumber, Optional[int]) assert_type(record.barcode, Optional[T.BarcodeDict]) assert_type(record.button, Optional[T.ButtonDict]) - assert_type(record.checkbox, Optional[bool]) + assert_type(record.checkbox, bool) assert_type(record.collaborator, Optional[T.CollaboratorDict]) assert_type(record.count, Optional[int]) assert_type(record.created_by, Optional[T.CollaboratorDict]) @@ -132,7 +132,7 @@ class EveryField(orm.Model): assert_type(record.date, Optional[datetime.date]) assert_type(record.datetime, Optional[datetime.datetime]) assert_type(record.duration, Optional[datetime.timedelta]) - assert_type(record.email, Optional[str]) + assert_type(record.email, str) assert_type(record.float, Optional[float]) assert_type(record.integer, Optional[int]) assert_type(record.last_modified_by, Optional[T.CollaboratorDict]) @@ -141,8 +141,8 @@ class EveryField(orm.Model): assert_type(record.multi_select, List[str]) assert_type(record.number, Optional[Union[int, float]]) assert_type(record.percent, Optional[Union[int, float]]) - assert_type(record.phone, Optional[str]) + assert_type(record.phone, str) assert_type(record.rating, Optional[int]) - assert_type(record.rich_text, Optional[str]) - assert_type(record.select, Optional[str]) - assert_type(record.url, Optional[str]) + assert_type(record.rich_text, str) + assert_type(record.select, str) + assert_type(record.url, str) From 701ea70812d6965dcd29892a8996b52c330707c4 Mon Sep 17 00:00:00 2001 From: Alex Levy Date: Wed, 13 Mar 2024 17:27:15 -0700 Subject: [PATCH 2/4] Fix __all__ in pyairtable.orm.fields --- pyairtable/orm/fields.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/pyairtable/orm/fields.py b/pyairtable/orm/fields.py index a5df6c35..e975a865 100644 --- a/pyairtable/orm/fields.py +++ b/pyairtable/orm/fields.py @@ -944,10 +944,10 @@ class UrlField(TextField): with open(cog.inFile) as fp: src = fp.read() -classes = re.findall(r"class ([A-Z]\w+Field)", src) -constants = re.findall(r"^([A-Z][A-Z_+]) = ", src) +classes = re.findall(r"class ((?:[A-Z]\w+)?Field)", src) +constants = re.findall(r"^(?!T_)([A-Z][A-Z_]+) = ", src, re.MULTILINE) extras = ["LinkSelf"] -names = sorted(classes + constants + extras) +names = sorted(classes) + constants + extras cog.outl("\n\n__all__ = [") for name in ["Field", *names]: @@ -974,12 +974,12 @@ class UrlField(TextField): "DurationField", "EmailField", "ExternalSyncSourceField", + "Field", "FloatField", "IntegerField", "LastModifiedByField", "LastModifiedTimeField", "LinkField", - "LinkSelf", "LookupField", "MultipleCollaboratorsField", "MultipleSelectField", @@ -991,8 +991,13 @@ class UrlField(TextField): "SelectField", "TextField", "UrlField", + "ALL_FIELDS", + "READONLY_FIELDS", + "FIELD_TYPES_TO_CLASSES", + "FIELD_CLASSES_TO_TYPES", + "LinkSelf", ] -# [[[end]]] (checksum: 3fa8c12315457baf170f9766fd8c9f8e) +# [[[end]]] (checksum: 2aa36f4e76db73f3d0b741b6be6c9e9e) # Delayed import to avoid circular dependency From 033a36ed05a6f2b034cefba2802b8a53c871d895 Mon Sep 17 00:00:00 2001 From: Alex Levy Date: Fri, 15 Mar 2024 12:02:55 -0700 Subject: [PATCH 3/4] SelectField should distinguish `""` and `None` --- pyairtable/orm/fields.py | 7 +++++-- pyairtable/orm/model.py | 8 ++++++-- tests/test_orm_fields.py | 27 +++++++++++++++++++++++++++ tests/test_typing.py | 2 +- 4 files changed, 39 insertions(+), 5 deletions(-) diff --git a/pyairtable/orm/fields.py b/pyairtable/orm/fields.py index e975a865..2dbbffcd 100644 --- a/pyairtable/orm/fields.py +++ b/pyairtable/orm/fields.py @@ -846,13 +846,16 @@ class RichTextField(TextField): """ -class SelectField(TextField): +class SelectField(Field[str, str, None]): """ - Equivalent to :class:`~TextField`. + Represents a single select dropdown field. This will return ``None`` if no value is set, + and will only return ``""`` if an empty dropdown option is available and selected. See `Single select `__. """ + valid_types = str + class UrlField(TextField): """ diff --git a/pyairtable/orm/model.py b/pyairtable/orm/model.py index 21038cd1..acfcdaab 100644 --- a/pyairtable/orm/model.py +++ b/pyairtable/orm/model.py @@ -286,10 +286,14 @@ def from_record(cls, record: RecordDict) -> SelfType: # Convert Column Names into model field names field_values = { # Use field's to_internal_value to cast into model fields - field: name_field_map[field].to_internal_value(value) + field: ( + name_field_map[field].to_internal_value(value) + if value is not None + else None + ) for (field, value) in record["fields"].items() # Silently proceed if Airtable returns fields we don't recognize - if field in name_field_map and value is not None + if field in name_field_map } # Since instance(**field_values) will perform validation and fail on # any readonly fields, instead we directly set instance._fields. diff --git a/tests/test_orm_fields.py b/tests/test_orm_fields.py index 96c27128..c93c2842 100644 --- a/tests/test_orm_fields.py +++ b/tests/test_orm_fields.py @@ -1,6 +1,7 @@ import datetime import operator import re +from unittest import mock import pytest @@ -727,3 +728,29 @@ class T: obj = T() with pytest.raises(exc_class): obj.the_field + + +@pytest.mark.parametrize( + "fields,expected", + [ + ({}, None), + ({"Field": None}, None), + ({"Field": ""}, ""), + ({"Field": "xyz"}, "xyz"), + ], +) +def test_select_field(fields, expected): + """ + Test that select field distinguishes between empty string and None. + """ + + class T(Model): + Meta = fake_meta() + the_field = f.SelectField("Field") + + obj = T.from_record(fake_record(**fields)) + assert obj.the_field == expected + + with mock.patch("pyairtable.Table.update", return_value=obj.to_record()) as m: + obj.save() + m.assert_called_once_with(obj.id, fields, typecast=True) diff --git a/tests/test_typing.py b/tests/test_typing.py index c5a76ff8..60019447 100644 --- a/tests/test_typing.py +++ b/tests/test_typing.py @@ -144,5 +144,5 @@ class EveryField(orm.Model): assert_type(record.phone, str) assert_type(record.rating, Optional[int]) assert_type(record.rich_text, str) - assert_type(record.select, str) + assert_type(record.select, Optional[str]) assert_type(record.url, str) From ab13ce5a5e52d3928c5679d2aa4c8218743cbc9e Mon Sep 17 00:00:00 2001 From: Alex Levy Date: Tue, 19 Mar 2024 00:14:06 -0700 Subject: [PATCH 4/4] orm_null_values: Changelog --- docs/source/changelog.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/source/changelog.rst b/docs/source/changelog.rst index 08eceb5f..8022d4de 100644 --- a/docs/source/changelog.rst +++ b/docs/source/changelog.rst @@ -5,6 +5,10 @@ Changelog 3.0 (TBD) ------------------------ +* ORM fields :class:`~pyairtable.orm.fields.TextField` and + :class:`~pyairtable.orm.fields.CheckboxField` will no longer + return ``None`` when the field is empty. + - `PR #347 `_. * Rewrite of :mod:`pyairtable.formulas` module. - `PR #329 `_.