Repository: marshmallow-code/marshmallow
Pull request #1770 (2021-03-22 09:16:27)

Remove useless hasattr / gettatr checks in `Field`
Minor code simplification.

Shouldn't break anything unless someone's been naughty deleting those attributes.

 

sloria: One possibly legitimate use case this could break is a wrapper field that doesn't call `super()` in its `__init__`.

```python
class OuterField(Field):
    def __init__(self, inner_field):
        self.inner_field = inner_field

    def _bind_to_schema(...):...
    def _serialize(...):...
```

Not sure if we should support this or not 🤔 
lafrech: > Not sure if we should support this or not thinking

I don't think we should. There is so much stuff happening in `__init__`.

In fact I don't think we do. `__repr__` doesn't make any check and assumes all attributes are defined, so we can argue it was never supported (or `__repr__` was broken).

I force-pushed to also remove `getattr` calls that were used for this purpose.

Patch:
diff --git a/AUTHORS.rst b/AUTHORS.rst
index 9fc0ddbb..6a47e768 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -159,3 +159,4 @@ Contributors (chronological)
 - Stephen Rosen `@sirosen <https://github.com/sirosen>`_
 - Vladimir Mikhaylov `@vemikhaylov <https://github.com/vemikhaylov>`_
 - Stephen Eaton `@madeinoz67 <https://github.com/madeinoz67>`_
+- Antonio Lassandro `@lassandroan <https://github.com/lassandroan>`_
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 079638d2..881fcc33 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -9,6 +9,12 @@ Features:
 - Add ``fields.IPInterface``, ``fields.IPv4Interface``, and
   ``IPv6Interface`` (:issue:`1733`). Thanks :user:`madeinoz67`
   for the suggestion and the PR.
+- Raise ``AttributeError`` for missing methods when using ``fields.Method`` (:pr:`1675`).
+  Thanks :user:`lassandroan`.
+
+Other changes:
+
+- Remove unnecessary ``hasattr`` and ``getattr`` checks in ``Field`` (:pr:`1770`).
 
 3.10.0 (2020-12-19)
 *******************
diff --git a/src/marshmallow/fields.py b/src/marshmallow/fields.py
index 6b7b5e22..6b3699b3 100644
--- a/src/marshmallow/fields.py
+++ b/src/marshmallow/fields.py
@@ -234,11 +234,8 @@ class Field(FieldABC):
         :param callable accessor: A callable used to retrieve the value of `attr` from
             the object `obj`. Defaults to `marshmallow.utils.get_value`.
         """
-        # NOTE: Use getattr instead of direct attribute access here so that
-        # subclasses aren't required to define `attribute` member
-        attribute = getattr(self, "attribute", None)
         accessor_func = accessor or utils.get_value
-        check_key = attr if attribute is None else attribute
+        check_key = attr if self.attribute is None else self.attribute
         return accessor_func(obj, check_key, default)
 
     def _validate(self, value):
@@ -297,12 +294,10 @@ class Field(FieldABC):
         """Validate missing values. Raise a :exc:`ValidationError` if
         `value` should be considered missing.
         """
-        if value is missing_:
-            if hasattr(self, "required") and self.required:
-                raise self.make_error("required")
-        if value is None:
-            if hasattr(self, "allow_none") and self.allow_none is not True:
-                raise self.make_error("null")
+        if value is missing_ and self.required:
+            raise self.make_error("required")
+        if value is None and not self.allow_none:
+            raise self.make_error("null")
 
     def serialize(
         self,
@@ -323,7 +318,7 @@ class Field(FieldABC):
         """
         if self._CHECK_ATTRIBUTE:
             value = self.get_value(obj, attr, accessor=accessor)
-            if value is missing_ and hasattr(self, "default"):
+            if value is missing_:
                 default = self.default
                 value = default() if callable(default) else default
             if value is missing_:
@@ -354,7 +349,7 @@ class Field(FieldABC):
         if value is missing_:
             _miss = self.missing
             return _miss() if callable(_miss) else _miss
-        if getattr(self, "allow_none", False) is True and value is None:
+        if self.allow_none and value is None:
             return None
         output = self._deserialize(value, attr, data, **kwargs)
         self._validate(output)
@@ -1809,22 +1804,30 @@ class Method(Field):
         super().__init__(**kwargs)
         self.serialize_method_name = serialize
         self.deserialize_method_name = deserialize
+        self._serialize_method = None
+        self._deserialize_method = None
 
-    def _serialize(self, value, attr, obj, **kwargs):
-        if not self.serialize_method_name:
-            return missing_
-
-        method = utils.callable_or_raise(
-            getattr(self.parent, self.serialize_method_name, None)
-        )
-        return method(obj)
+    def _bind_to_schema(self, field_name, schema):
+        if self.serialize_method_name:
+            self._serialize_method = utils.callable_or_raise(
+                getattr(schema, self.serialize_method_name)
+            )
 
-    def _deserialize(self, value, attr, data, **kwargs):
         if self.deserialize_method_name:
-            method = utils.callable_or_raise(
-                getattr(self.parent, self.deserialize_method_name, None)
+            self._deserialize_method = utils.callable_or_raise(
+                getattr(schema, self.deserialize_method_name)
             )
-            return method(value)
+
+        super()._bind_to_schema(field_name, schema)
+
+    def _serialize(self, value, attr, obj, **kwargs):
+        if self._serialize_method is not None:
+            return self._serialize_method(obj)
+        return missing_
+
+    def _deserialize(self, value, attr, data, **kwargs):
+        if self._deserialize_method is not None:
+            return self._deserialize_method(value)
         return value
 
 
diff --git a/src/marshmallow/utils.py b/src/marshmallow/utils.py
index 0bc13d7c..cb5e5002 100644
--- a/src/marshmallow/utils.py
+++ b/src/marshmallow/utils.py
@@ -285,9 +285,9 @@ def set_value(dct: typing.Dict[str, typing.Any], key: str, value: typing.Any):
 
 
 def callable_or_raise(obj):
-    """Check that an object is callable, else raise a :exc:`ValueError`."""
+    """Check that an object is callable, else raise a :exc:`TypeError`."""
     if not callable(obj):
-        raise ValueError("Object {!r} is not callable.".format(obj))
+        raise TypeError("Object {!r} is not callable.".format(obj))
     return obj
 
 


Test patch:
diff --git a/tests/test_deserialization.py b/tests/test_deserialization.py
index 8d124b02..6690e429 100644
--- a/tests/test_deserialization.py
+++ b/tests/test_deserialization.py
@@ -1024,7 +1024,7 @@ class TestFieldDeserialization:
         assert excinfo.value.args[0] == "Not a valid IPv6 interface."
 
     def test_deserialization_function_must_be_callable(self):
-        with pytest.raises(ValueError):
+        with pytest.raises(TypeError):
             fields.Function(lambda x: None, deserialize="notvalid")
 
     def test_method_field_deserialization_is_noop_by_default(self):
@@ -1054,19 +1054,17 @@ class TestFieldDeserialization:
         class BadSchema(Schema):
             uppername = fields.Method("uppercase_name", deserialize="lowercase_name")
 
-        s = BadSchema()
-        with pytest.raises(ValueError):
-            s.fields["uppername"].deserialize("STEVE")
+        with pytest.raises(AttributeError):
+            BadSchema()
 
     def test_method_field_deserialize_only(self):
         class MethodDeserializeOnly(Schema):
+            name = fields.Method(deserialize="lowercase_name")
+
             def lowercase_name(self, value):
                 return value.lower()
 
-        m = fields.Method(deserialize="lowercase_name")
-        m.parent = MethodDeserializeOnly()
-
-        assert m.deserialize("ALEC") == "alec"
+        assert MethodDeserializeOnly().load({"name": "ALEC"})["name"] == "alec"
 
     def test_datetime_list_field_deserialization(self):
         dtimes = dt.datetime.now(), dt.datetime.now(), dt.datetime.utcnow()
diff --git a/tests/test_serialization.py b/tests/test_serialization.py
index 7824dc45..a08bbd0a 100644
--- a/tests/test_serialization.py
+++ b/tests/test_serialization.py
@@ -112,7 +112,7 @@ class TestFieldSerialization:
         assert "FOOBAR" == field.serialize("key", user)
 
     def test_function_field_passed_uncallable_object(self):
-        with pytest.raises(ValueError):
+        with pytest.raises(TypeError):
             fields.Function("uncallable")
 
     def test_integer_field(self, user):
@@ -415,10 +415,6 @@ class TestFieldSerialization:
         assert field.serialize("falsy", user) is False
         assert field.serialize("none", user) is None
 
-    def test_function_with_uncallable_param(self):
-        with pytest.raises(ValueError):
-            fields.Function("uncallable")
-
     def test_email_field_serialize_none(self, user):
         user.email = None
         field = fields.Email()
@@ -469,9 +465,8 @@ class TestFieldSerialization:
         class BadSerializer(Schema):
             bad_field = fields.Method("invalid")
 
-        u = User("Foo")
-        with pytest.raises(ValueError):
-            BadSerializer().dump(u)
+        with pytest.raises(AttributeError):
+            BadSerializer()
 
     def test_method_field_passed_serialize_only_is_dump_only(self, user):
         field = fields.Method(serialize="method")
@@ -488,9 +483,8 @@ class TestFieldSerialization:
             foo = "not callable"
             bad_field = fields.Method("foo")
 
-        u = User("Foo")
-        with pytest.raises(ValueError):
-            BadSerializer().dump(u)
+        with pytest.raises(TypeError):
+            BadSerializer()
 
     # https://github.com/marshmallow-code/marshmallow/issues/395
     def test_method_field_does_not_swallow_attribute_error(self):