Repository: marshmallow-code/marshmallow
Pull request #1675 (2020-10-06 03:19:57)

Raise AttributeError when missing Schema methods
Currently mistyping a schema method name when using `fields.Method` will result in a `ValueError` rather than throwing an `AttributeError` that details what went wrong:

```python
class MySchema(Schema):
    # Method constructor argument is misspelled but actual method is not
    foobar = fields.Method("get_footbar")

    def foobar(self, obj):
         return obj.some_other_data()
```

```
  File ".../lib/python3.8/site-packages/marshmallow/schema.py", line 557, in dump
    result = self._serialize(processed_obj, many=many)
  File ".../lib/python3.8/site-packages/marshmallow/schema.py", line 521, in _serialize
    value = field_obj.serialize(attr_name, obj, accessor=self.get_attribute)
  File ".../lib/python3.8/site-packages/marshmallow/fields.py", line 312, in serialize
    return self._serialize(value, attr, obj, **kwargs)
  File ".../lib/python3.8/site-packages/marshmallow/fields.py", line 1663, in _serialize
    method = utils.callable_or_raise(
  File ".../lib/python3.8/site-packages/marshmallow/utils.py", line 285, in callable_or_raise
    raise ValueError("Object {!r} is not callable.".format(obj))
ValueError: Object None is not callable.
```

With this change the error would yield something like:

```python
AttributeError: 'MySchema' object has no attribute 'get_footbar'
```

https://github.com/marshmallow-code/marshmallow/issues/1674

lafrech: Shouldn't the `callable_or_raise` check be performed at init time?

- Better for performance
- Better for debug, no runtime error

Or do we expect users to patch their schema and add the method after init?
lassandroan: > Shouldn't the `callable_or_raise` check be performed at init time?
> 
> * Better for performance
> * Better for debug, no runtime error
> 
> Or do we expect users to patch their schema and add the method after init?

@lafrech I just pushed a commit 5969cf4 that adds a `Method._bind_to_schema()` override and performs the callable checks there rather than having them be performed at serialization/deserialization time. Let me know your thoughts!

I did notice though that `callable_or_raise()` will raise `ValueError` if the attribute type is incorrect (not callable). Shouldn't this be a [`TypeError` based on the use case?](https://docs.python.org/3/library/exceptions.html#TypeError)

I don't think patching the schema afterwards should be expected behavior, but if they were to swap the attribute after init and make it non-callable they would receive `TypeError` in that situation rather than `ValueError`.
lassandroan: Hi @lafrech 

I've just updated `callable_or_raise()` to raise `TypeError` rather than `ValueError` as to be consistent with python when objects are not callable:

```python
>>> 1()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'int' object is not callable
```
lassandroan: @lafrech thank you! Updated in 124fad7 to address the comments
lafrech: While we're at it, wouldn't it make sense to get the methods in `_bind_to_schema` and cache them in attributes?

```py
    def __init__(self, serialize: str = None, deserialize: str = None, **kwargs):
        # Set dump_only and load_only based on arguments
        kwargs["dump_only"] = bool(serialize) and not bool(deserialize)
        kwargs["load_only"] = bool(deserialize) and not bool(serialize)
        super().__init__(**kwargs)
        self.serialize_method_name = serialize
        self.deserialize_method_name = deserialize
        self._serialize_method = None
        self._deserialize_method = None

    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))

        if self.deserialize_method_name:
            self._deserialize_method = utils.callable_or_raise(getattr(schema, self.deserialize_method_name))

        super()._bind_to_schema(field_name, schema)

    def _serialize(self, value, attr, obj, **kwargs):
        if not self._serialize_method:
            return missing_
        return self._serialize_method(obj)

    def _deserialize(self, value, attr, data, **kwargs):
        if self._deserialize_method:
            return self.deserialize_method(value)
        return value
```
lassandroan: > While we're at it, wouldn't it make sense to get the methods in `_bind_to_schema` and cache them in attributes?

Good call, yeah that's a much better approach and also addresses the attribute availability thing I mentioned before: users shouldn't monkey-patch schema methods. I updated this in b48e7da

There's one issue I encountered with that though is there is this test:

```python
def test_method_field_deserialize_only(self):
        class MethodDeserializeOnly(Schema):
            def lowercase_name(self, value):
                return value.lower()

        m = fields.Method(deserialize="lowercase_name")
        m.parent = MethodDeserializeOnly()

        assert m.deserialize("ALEC") == "alec"
```

From the way this test is written, `m` never has `_bind_to_schema()` called because it's a field completely outside the declaration of the schema itself... is this something that's supported by Marshmallow? 

I'm a bit confused on the use case, but I rewrote the test to utilize the Schema's `load()` instead:

```python
def test_method_field_deserialize_only(self):
        class MethodDeserializeOnly(Schema):
            name = fields.Method(deserialize="lowercase_name")

            def lowercase_name(self, value):
                return value.lower()

        assert MethodDeserializeOnly().load({"name": "ALEC"})["name"] == "alec"
```

Let me know your thoughts
lafrech: This test already relied on

```py
    m.parent = MethodDeserializeOnly()
```

which is basically what `_bind_to_schema` does.

Apart from this PR, it would be a good idea to rewrite the test to use `_bind_to_schema` explicitly IMHO.

There's no way this field can be independent of the schema.

I'm fine with this change.

Note that before your last commit, the use case was broken already because the field didn't go through the callable or raise test, and this wasn't detected due to the test being badly written. Another reason to rewrite it.

Is the independent field use case supported? Well, I've been looking through the fields module, and the other fields actually using bind_to_schema are `DateTime` to get a format from the field class, and container/nested fields to pass `_bind_to_schema`. `DateTime` field doesn't need it to work.

So I suppose it works and could be supported in the general case but it makes no sense, by definition, for the `Method` field so I wouldn't mind making it an exception if the rule was to support the use case.

Regarding your implementation with `hasattr` vs. my proposal, unless there is a reason I'm missing, I tend to apply the general rule of declaring all attributes in `__init__`. pylint, for instance, complains about setting attributes in other methods as it is considered bad practice. Totally opinion-based. My proposal should be rewritten `if self._serialize_method is None`, though, to be more explicit and to skip a boolean evaluation.
lassandroan: @lafrech awesome, thanks for the explanation!

I force pushed the last commit to 6d0edf8 to swap the `hasattr()` for the `None` init + `is not None` checks, I agree it's much better to explicitly create the attribute if it's expected to be used
lassandroan: @sloria forgot that `Authors.rst` got updated recently, so just fixed the conflict. Should be good to review now!

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..95da2ab7 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -9,6 +9,8 @@ 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`.
 
 3.10.0 (2020-12-19)
 *******************
diff --git a/LICENSE b/LICENSE
index cf353a65..dbe64bed 100644
--- a/LICENSE
+++ b/LICENSE
@@ -1,4 +1,4 @@
-Copyright 2020 Steven Loria and contributors
+Copyright 2021 Steven Loria and contributors
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
 of this software and associated documentation files (the "Software"), to deal
diff --git a/setup.py b/setup.py
index a8b4084a..c2f156c6 100644
--- a/setup.py
+++ b/setup.py
@@ -5,8 +5,8 @@ EXTRAS_REQUIRE = {
     "tests": ["pytest", "pytz", "simplejson"],
     "lint": [
         "mypy==0.812",
-        "flake8==3.8.4",
-        "flake8-bugbear==21.3.1",
+        "flake8==3.9.0",
+        "flake8-bugbear==21.3.2",
         "pre-commit~=2.4",
     ],
     "docs": [
diff --git a/src/marshmallow/fields.py b/src/marshmallow/fields.py
index 6b7b5e22..0b3dce99 100644
--- a/src/marshmallow/fields.py
+++ b/src/marshmallow/fields.py
@@ -1809,22 +1809,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):