fixes #322, fixed incorrect serializer instance usage

[pizzapanther]

No description provided.

[coveralls]

Coverage increased (+0.02%) to 93.021% when pulling e739193 on pizzapanther:master into 2600f0f on graphql-python:master.

[urbandove]

Same Issue here
Was wondering if
serialized = serializer.__class__(obj)
return cls(errors=None, **serialized.data)
may be a better fix - in case someone has overriden the serializing behavior

[pizzapanther]

@urbandove actually tried that fix first and it's incorrect also. serialized.data does not contain all the data of the new instance. So if you have something like an auto-generated DateTime .data will not include that. What I did, I thought was the 'most correct' way to do if after some searching but if you know another, let me know.

[coveralls]

Coverage increased (+0.02%) to 93.021% when pulling e05f41a on pizzapanther:master into 2600f0f on graphql-python:master.

[urbandove]

@pizzapanther Interesting that .data doesn't include everything - where would you be autogenerating something that doesn't get picked up by the serializer? And wouldnt that be an issue with the serializer?

Also, in general the whole DRF integration looks like there could be a bit of patching up which im working on - I still cant seem to find a way to use a ModelSerializer instance to update a previous instance without completely overriding the SerializerMutation Class - I'm in the middle of adding such functionality in a ModelSeiralizerMutation class, but if you know if such an integration exists and i just missed it it would be helpful

[pizzapanther]

The problem is .data doesn't get updated after you do .save(). So if you have a field like a created or modified auto updated timestamp, then the field isn't in there. Yes I would say that is a problem with DRF but DRF has been that way for a long time.

The other way I thought about it is you could make a new serializer.

obj = serializer.save()

new_serializer = SerializerClass(instance=obj)
return cls(errors=None, **new_serializer.data)

However, that is incorrect too. So even if .data was updated it would be wrong. The problem is again if you have a DateTime, .data will be contain a Date String and not a DateTime Object. So the method I coded up preserves the Python objects and lets Graphene do the correct conversion.

@urbandove I'm going to work on an update Mutation next and let you know what I find.

[pizzapanther]

ok @urbandove this is how I could get an update/edit mutation to work:

class PostSerializer(serializers.ModelSerializer):
  # regular DRF stuff

  def update(self, instance, validated_data):
    for key, value in validated_data.items():
      setattr(instance, key, value)
      
    instance.save()
    return instance

class UpdatePost (SerializerMutation):
  @classmethod
  def mutate_and_get_payload(cls, root, info, **input):
    instance = Post.objects.get(id=input['id'])
    del input['id']
    serializer = cls._meta.serializer_class(instance=instance, data=input)

    if serializer.is_valid():
      return cls.perform_mutate(serializer, info)
    else:
      errors = [
        ErrorType(field=key, messages=value)
        for key, value in serializer.errors.items()
      ]

      return cls(errors=errors)
      
  @classmethod
  def perform_mutate(cls, serializer, info):
    obj = serializer.save()
    
    kwargs = {}
    for f, field in serializer.fields.items():
      kwargs[f] = field.get_attribute(obj)
      
    return cls(errors=None, **kwargs)
    
  class Meta:
    serializer_class = PostSerializer

Note it also contains the fix i'm submitting with this PR. So hopefully you won't need the perform_mutate part in the near future.

Also note, the error messages do nothing, so I'll assume that is broke and needs a fix too. But at least it runs correctly if the serializer is valid.

The key line is serializer = cls._meta.serializer_class(instance=instance, data=input) vs serializer = cls._meta.serializer_class(data=input) for Add mutations. I think there could be some way to tell Graphene between an update and an add but that would be separate topic/PR.

[pizzapanther]

Design question would be should Add and Update be separate mutations?

[urbandove]

I was just writing my own version of a modelserializer ;)

It seems from my quick searching that the general way its being done at companies which are currently using graphql is to use 2 mutations - create and update

(However the version i was just working on i was adding a field called pk to the input and then checking if it existed. If you would include a pk then it would update otherwise it would create a new item (this is similar to the way django currently decides if it should run a INSERT or UPDATE in sql - https://docs.djangoproject.com/en/1.11/ref/models/instances/#how-django-knows-to-update-vs-insert )

The problem with that approach is that the user may have already specified a value for pk to refer to something else (though i should think that if we are dealing with a ModelSerializer the user would not override the pk field) )

Even though this is the way django deals with the difference between update and create - i think we should follow consensus on the graphql side for frontend facing apis

Another issue i was dealing with was what if you want more than just the mutation fields back, for example, on creation of a new object, if you wanted the primary key of the new object, or one of many other possible fields, how would you be able to return that. As far as i understand the relay spec doesnt allow for that, but i was working on a method where you could instantiate a modelmutation with a return type which was a subclass of DjangoObjectType

[pizzapanther]

You can get any of the serializer fields back, even the id if it is a serialized field. For example.

mutation{
  addPost(input:{...}) {
    title
    slug
    id
  }
}

[urbandove]

I understand - what i want is the global id more - for knowing which item to update - but i guess it could be worked out another way

[pizzapanther]

You can compute that yourself if you have the database id/pk and the Node name. For example if your node is called PostNode and the ID was 1.

In javascript btoa('PostNode:1') => "UG9zdE5vZGU6MQ=="

In Python base64.b64encode('PostNode:1'.encode('utf-8')) => b'UG9zdE5vZGU6MQ=='

[syrusakbary]

I would love to merge this. A test assuring that is fixed would help to avoid repeating this issue in the future :)

[pizzapanther]

I'll add that shortly.

[urbandove]

for the update mutation, you could make a generic update class and replace
instance = Post.objects.get(id=input['id'])
with
instance = cls._meta.serializer_class.Meta.model.objects.get(id=input['id'])

[urbandove]

also - while you are editing that area - there are certain fields which may make it past the serializer.validate() but wont save - for example if you have unique on a field

here is a small piece of code to include in that possiblity

@classmethod
def perform_mutate(cls, serializer, info):
    try:
        obj = serializer.save()
        kwargs = {}
        for f, field in serializer.fields.items():
            kwargs[f] = field.get_attribute(obj)
        return cls(errors=None, **kwargs)
    except Exception as e:
        errors = [
                   ErrorType(field=type(e), messages=str(e))
        ]
        return cls(errors=errors)

[pizzapanther]

@syrusakbary tests are up but it looks like the build is failing for other issues. Is that a problem getting worked?

[coveralls]

Coverage increased (+0.05%) to 93.064% when pulling 0a660e5 on pizzapanther:master into 568073a on graphql-python:master.

[coveralls]

Coverage increased (+0.05%) to 93.064% when pulling 6cfd5b2 on pizzapanther:master into 568073a on graphql-python:master.

[pizzapanther]

@syrusakbary ready for a merge now

[pizzapanther]

@urbandove I'll submit this update after this PR is merged: https://github.com/pizzapanther/graphene-django/blob/drf-serializer-update/docs/rest-framework.rst#addupdate-operations

I added support for Add and Update operations. With the current code you can only do updates by adding customization. The updates I made with the next one should allow add and updates with less customization needed by the user.

I'll also look into the issue you noted. I've noticed errors not showing up too.