Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for serializer's method fields. #56

Open
swistakm opened this issue Apr 20, 2017 · 0 comments
Open

Support for serializer's method fields. #56

swistakm opened this issue Apr 20, 2017 · 0 comments

Comments

@swistakm
Copy link
Owner

swistakm commented Apr 20, 2017

This is very common feature in many similar serialization solutions. I see two possible approaches:

  1. Providing decorator function that can turn serialize'rs method to dynamic field class object.
  2. Providing new field class that is bound to the serializer instance.

Decorator approach

Decorator approach is the simplest to implement because does not affect serializers code and does not introduce any backwards incompatible changes.

def unbound_method_field(method):
    class CallField(fields.BaseField):
        def __init__(self):
            super().__init__(
                method.__doc__,
                label=method.__name__,
                read_only=True,
                source='*',
            )

        def to_representation(instance):
            return method(instance)

    return CallField()

class MySerializer(serializers.BaseSerializer):
    @unbound_method_field
    def my_field_times_10(instance):
        return instance['my_field'] * 10

The problem is that we cannot use real methods but only unbound functions defined in class namespace. This is due to how serializer's metaclass works. This creates a bit counterintuitive definitions within class instance. We could use a bit more complex implementation:

def bound_method_field(method):
    class CallField(fields.BaseField):
        def __init__(self):
            super().__init__(
                method.__doc__,
                label=method.__name__,
                read_only=True,
                source='*',
            )

        def to_representation(self, instance):
            return method(self, instance)

    return CallField()

class MySerializer(serializers.BaseSerializer):
    @bound_method_field
    def my_field_times_10(instance):
        return instance['my_field'] * 10

But this will be even more counter intuitive because such method will be "bound" to the field instance and not serializer instance.

Pros:

  • ✅ Simple implementation without any backwards incompatible changes.

Cons:

  • ❌ Such fields could be "read only" only.
  • ❌ Will not work properly with methods defines with classmethod and staticmethod dictionaries
  • ❌ Counterintuitive method definitions
  • ❌ Overriding other field options like source, name, label and validators etc. would require more complex decorator
  • ❌ Need to use star-like sources in 0.x graceful version
  • ❌ It will make serializers harder to read and understand because some fields will be defined as "methods".

New field class type

We could take an example from Django REST Framework and Marshmallow projects and make all field instances bound to the serializer instance on BaseSerializer.fields property access. In order to ensure proper binding of inherited fields from base serializer classes we would have also to perform deep copy of all that fields. This would not have a negative impact on performance once we cache fields dictionary items (see: #54).

Binding fields to the parent is a bit easier in DRF because their serializers are initialized per-object instance. Serializers in graceful are more static and stateless object translators. Still, I believe it can be implemented in similar way.

Pros:

  • ✅ It allows for more complex handling of custom serializer methods.
  • ✅ Serializer method fields can be both "source aware" or work on whole resources.
  • ✅ Once fields are bound to serializer class we can accept methods both for writes and reads.
  • ✅ Minimal implementation would only consist of obligatory dynamic binding the fields to serializer instance. Once it is done user can implement their own serializer-bound method field implementations.
  • ✅ It will work seamlessly with both staticmethod and classmethod.

Cons:

  • ❌ It requires careful fields property handling on first access to ensure no performance impact.
  • ❌ More complex change that requires changes in serializer metaclass.
  • ❌ It requires a copy.deepcopy() call. Also dynamic field manipulation once serializer is initialized will be limited.
  • ❌ Parent serializer will be added to the field dynamically. This will make code more complex and harder to read.
  • ❌ We will have to take some extra precautions in order to ensure proper method is called in complex inheritance scenarios.

Related work:

Side note: the other advantage of approach based on Marsmallow's code is that we could finally pass fields name in the serializer during binding. Then fields would know their names and serializers would not have to wonder if field has its own source specified (see 1.x code). Still I believe that current API of read_instance makes instance processing a bit more performant and it is worth leaving as it is. We could even try to cache field sources inside of serializers namespace.

Summary

It seems that field binding is currently the best approach even if it results in more complex serializer's code. It should land in 1.0.0 release if we want to implement this. It is not that invasive change but at implementing it in 0.x would later require major changes in actual new field classes. We can optionally introduce this change gradually:

  • implement obligatory field binding in 0.x
  • add new field classes in 1.0.0 or in later minor releases of 1.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant