Repository: marshmallow-code/apispec
Pull request #498 (2019-09-08 14:26:26)

add_attribute_function: bind function to OpenAPIConverter instance
This achieves the same goal as #497.

It makes things more consistent to have custom attribute functions be bound to the instance like the default ones.

This means all custom functions **must** accept `self` (even if they don't use it). I think consistency is worth this (little) inconvenience.

lafrech: Alternatives:

- Don't do anything (don't bind, don't pass self) and let the user declare the function as a method if needed by subclassing the converter. Doing this plus calling `add_attribute_function` means double work. I don't like that.

- Only declare static functions in `FieldConverterMixin` and always pass the converter as keyword arg (or secondary positional):

```py
class FieldConverterMixin:

    def field2property(self, field):
        ret = {}
        for attr_func in self.attribute_functions:
            ret.update(attr_func(field, self, ret=ret))
        return ret

    @staticmethod
    def field2type_and_format(field, openapi_converter, **kwargs):
        [...]

def my_custom_funct(field, openapi_converter, **kwargs):
    [...]

converter.add_attribute_function(my_custom_funct)
```

This way, all signatures are the same.

I'm not sure which interface is the best. But current interface looks awkward to me.
Bangertm: This works for me.  I agree that the current interface is awkward and it's better to have a consistent interface. Currently three field types, (Nested, List, and Dict) require access to self. I image that at least some of the custom fields would require it as well.
lafrech: Yes, and one could also add custom methods not only for custom fields but for custom properties.

In fact, any method potentially needs access to the converter just to know the OAS version.

I'm still hesitant between this PR and the second alternative above (static methods + self/openapi_converter passed as kwargs).

The latter is slightly clearer to the user creating a custom function because the former (this PR) means the user creating a custom function writes a function having `self` as first positional args while not in a class scope.

```py
def my_custom_func(self, field, **kwargs):  # <-- self, here, while not in class scope
    [...]

ma_plugin.converter.add_attribute_function(my_custom_func)
```

OTOH, the `openapi_converter` kwarg might not make much intuitive sense either, as the converter is an internal class very specific to our implementation. Not much more self-explanatory than `self`. And it's weird to pass the converter instance as a kwarg to the methods declared in the class.

```py
def my_custom_func(field, openapi_converter, **kwargs):  # <-- openapi_converter here
    [...]

ma_plugin.converter.add_attribute_function(my_custom_func)
```

Maybe I'm overthinking it.

I'll probably merge this. Just need to update docs/changelog.

Meanwhile, thoughts welcome. I can rework for the static functions alternative.
Bangertm: One additional alternative would be to make all static methods but instead of passing the `openapi_converter` instance just pass the things that we think the the static methods needs as keyword arguments i.e. `openapi_version`, `resolve_nested_schema` and `field2parameters`. I think that providing `self` is the better way to go.

At the end of the day we are saving users having to create their logic in a subclass. So by requiring them to add a `self` positional argument they they have to write the exact same method but without the need to subclass.

Patch:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 92218b2..e07a147 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -7,7 +7,7 @@ Changelog
 Features:
 
 - Add support for generating user-defined OpenAPI properties for custom field
-  classes via an ``add_attribute_function`` method (:pr:`478`).
+  classes via an ``add_attribute_function`` method (:pr:`478` and :pr:`498`).
 - [apispec.ext.marshmallow]: *Backwards-incompatible* ``fields.Raw`` and ``fields.Field`` are
   now represented by OpenAPI `Any Type <https://swagger.io/docs/specification/data-models/data-types/#any>`_
   (:pr:`495`).
diff --git a/docs/using_plugins.rst b/docs/using_plugins.rst
index 367fa6e..20a2b5c 100644
--- a/docs/using_plugins.rst
+++ b/docs/using_plugins.rst
@@ -279,17 +279,21 @@ method. Continuing from the example above:
 
 .. code-block:: python
 
-    def my_custom_field2properties(field, **kwargs):
+    def my_custom_field2properties(self, field, **kwargs):
         """Add an OpenAPI extension flag to MyCustomField instances
         """
         ret = {}
         if isinstance(field, MyCustomField):
-            ret["x-customField"] = True
+            if self.openapi_version.major > 2:
+                ret["x-customString"] = True
         return ret
 
 
     ma_plugin.converter.add_attribute_function(my_custom_field2properties)
 
+The function passed to `add_attribute_function` will be bound to the converter.
+It must accept the converter instance as first positional argument.
+
 Next Steps
 ----------
 
diff --git a/src/apispec/ext/marshmallow/field_converter.py b/src/apispec/ext/marshmallow/field_converter.py
index 8069eb3..8130ef0 100644
--- a/src/apispec/ext/marshmallow/field_converter.py
+++ b/src/apispec/ext/marshmallow/field_converter.py
@@ -129,10 +129,14 @@ class FieldConverterMixin:
         that will be called on a field to convert it from a field to an OpenAPI
         property.
 
-        :param func func: the attribute function to add - will be called for each
-            field in a schema with a `field <marshmallow.fields.Field>` instance
-            positional argument and `self <apispec.ext.marshmallow.openapi.OpenAPIConverter>`
-            and `ret <dict>` keyword arguments.
+        :param func func: the attribute function to add
+            The attribute function will be bound to the
+            `OpenAPIConverter <apispec.ext.marshmallow.openapi.OpenAPIConverter>`
+            instance.
+            It will be called for each field in a schema with
+            `self <apispec.ext.marshmallow.openapi.OpenAPIConverter>` and a
+            `field <marshmallow.fields.Field>` instance
+            positional arguments and `ret <dict>` keyword argument.
             Must return a dictionary of OpenAPI properties that will be shallow
             merged with the return values of all other attribute functions called on the field.
             User added attribute functions will be called after all built-in attribute
@@ -140,7 +144,9 @@ class FieldConverterMixin:
             previously called attribute functions are accessable via the `ret`
             argument.
         """
-        self.attribute_functions.append(functools.partial(func, self=self))
+        bound_func = func.__get__(self)
+        setattr(self, func.__name__, bound_func)
+        self.attribute_functions.append(bound_func)
 
     def field2property(self, field):
         """Return the JSON Schema property definition given a marshmallow


Test patch:
diff --git a/tests/test_ext_marshmallow_field.py b/tests/test_ext_marshmallow_field.py
index 63d9514..701da30 100644
--- a/tests/test_ext_marshmallow_field.py
+++ b/tests/test_ext_marshmallow_field.py
@@ -4,8 +4,6 @@ import re
 import pytest
 from marshmallow import fields, validate
 
-from apispec import APISpec
-from apispec.ext.marshmallow import MarshmallowPlugin
 from apispec.ext.marshmallow.openapi import MARSHMALLOW_VERSION_INFO
 
 from .schemas import CategorySchema, CustomList, CustomStringField, CustomIntegerField
@@ -276,17 +274,22 @@ def test_nested_field_with_property(spec_fixture):
     }
 
 
-def test_custom_properties_for_custom_fields():
-    def custom_string2properties(field, **kwargs):
+def test_custom_properties_for_custom_fields(spec_fixture):
+    def custom_string2properties(self, field, **kwargs):
         ret = {}
         if isinstance(field, CustomStringField):
-            ret["x-customString"] = True
+            if self.openapi_version.major == 2:
+                ret["x-customString"] = True
+            else:
+                ret["x-customString"] = False
         return ret
 
-    ma_plugin = MarshmallowPlugin()
-    APISpec(
-        title="Validation", version="0.1", openapi_version="3.0.0", plugins=(ma_plugin,)
+    spec_fixture.marshmallow_plugin.converter.add_attribute_function(
+        custom_string2properties
+    )
+    properties = spec_fixture.marshmallow_plugin.converter.field2property(
+        CustomStringField()
+    )
+    assert properties["x-customString"] == (
+        spec_fixture.openapi.openapi_version == "2.0"
     )
-    ma_plugin.converter.add_attribute_function(custom_string2properties)
-    properties = ma_plugin.converter.field2property(CustomStringField())
-    assert properties["x-customString"]