use to_representation in favor of get_attribute

[B4rtware]

The rest_framework offers the ability to use SerializerMethodFields which I also wanted to use in my project together with graphene-django but this field gets the wrong return value.
A detailed description can be found here: #760.

---

This is my copied comment from #760 which explains the change:

---

After further investigation, I found the origin of the underlying problem. Digging to the method
graphene_django>rest_framework>mutation.py>SerializerMutation>perform_mutate and applying this modification

    from rest_framework import serializers                             # modified
    ...
    @classmethod
    def perform_mutate(cls, serializer, info):
        obj = serializer.save()

        kwargs = {}
        for f, field in serializer.fields.items():
            if not field.write_only:
                if isinstance(field, serializers.SerializerMethodField): # modified
                    kwargs[f] = field.to_representation(obj)             # modified
                else:                                                    # modified
                    kwargs[f] = field.get_attribute(obj)

        return cls(errors=None, **kwargs)

it worked.

---

The issue is at field.get_attribute(ob). Inspecting the SerializerMethodField inside of the rest_framework you can see that there is no get_attribute function instead it will use the get_attribute from the base class Field which I think (not really traced it completely) just returns the representation of the obj. In my case this is the user object and it will return the users username because __str__ is overridden to do so.

---

So I was interested in how the django_rest_framework handles the get_attribute issue. And I have found out that there is a method called to_representation ON the Serializer class which indeed returns all field names and also the correct values for each field:

OrderedDict([('password', 'f'), ('username', 'john_doe'), ('email', 'john_doe@f.com'), ('token', 'eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VybmFtZSI6ImdkZGRkZHNzZHNkc3Nkc3Nzc2Rhc3Nkc2RkZGRkcyIsImV4cCI6MTU2NzUxNjc2Miwib3JpZ0lhdCI6MTU2NzUxNjQ2Mn0.fdiAgROOyqlV99S5R2mbV0DImp1YqONNVix6i67sCrs')])

And I am curious why the serializer.to_representation was not used and instead the loop over each field. Maybe I will get it after I am using this modification.

Using the mentioned method above, the function could be reduced to:

    @classmethod
    def perform_mutate(cls, serializer, info):
        obj = serializer.save()
        kwargs = serializer.to_representation(obj)
        return cls(errors=None, **kwargs)

Which I will now use as a workaround.
Any feedback is appreciated.

[zbyte64]

This looks like a good improvement, maybe needs a unit test. As for the CI failure, you need to apply black formatting to the file you modified graphene-django/graphene_django/rest_framework/mutation.py

[B4rtware]

This looks like a good improvement, maybe needs a unit test. As for the CI failure, you need to apply black formatting to the file you modified graphene-django/graphene_django/rest_framework/mutation.py

I appreciate that :)
Alright, sorry I will fix the formatting issue. Unfortunately I could need some help with implementing a unit test. Maybe also another test because this is only a read_only field as stated inside the documentation.

I thought about using the example which is provided on the rest_framework website: https://www.django-rest-framework.org/api-guide/fields/#serializermethodfield

For that I have added a new Fake Model because the created field cannot be set to a fixed date.

class MyFakeModelWithDate(models.Model):
    cool_name = models.CharField(max_length=50)
    last_edited = models.DateTimeField()

The serializer would look like that:

class MyModelSerializerWithMethod(serializers.ModelSerializer):
    days_since_last_edit = serializers.SerializerMethodField()

    class Meta:
        model = MyFakeModelWithDate
        fields = "__all__"

    def get_days_since_last_edit(self, obj):
        now = datetime.time.fromisoformat("2020-01-08")
        return (now - obj.last_edited).days

At this point I have currently no clue what to do next. This is the test function in its current state:

@mark.django_db
def test_perform_mutate_success():
    class MyMethodMutation(SerializerMutation):
        class Meta:
            serializer_class = MyModelSerializerWithMethod

    result = MyMethodMutation.mutate_and_get_payload(
        None,
        mock_info(),
        **{"cool_name": "Narf", "edited": datetime.date.fromisoformat("2020-01-04")}
    )

    assert result.cool_name == "Narf"
    assert result.days_since_last_edit == 4

it fails because result.cool_name is None. It would be nice if you or someone else knows what I am overlook or could share some knowledge about it.

[knabben]

Add an

assert result.errors is None

You probably have this filled-in some of the date processing, passing a date for the datetime model field is a good candidate. Push the test on CI, it's easier to follow the stacktrace.

[B4rtware]

Add an

assert result.errors is None

You probably have this filled-in some of the date processing, passing a date for the datetime model field is a good candidate. Push the test on CI, it's easier to follow the stacktrace.

I have now pushed a positiv test. However I get an error for DJANGO=master and TOXENV=black,flake8. My assumtion is that DJANGO=master overall is not compatible with the current version of graphene !? For the TOXENV based issue inside the circleci logs it says that both have failed and black would change rest_framework/tests/test_mutation.py but I can't reproduce this error on my local machine.

make format and make lint both do return no errors.

[knabben]

Check if you have the same black==19.3b0 version as the CI, the mismatch can be producing the fail. The django=master break does not seem related with your PR.

[zbyte64]

Awesome work

[jkimbo]

Looks good. Thanks @B4rtware