Skip to content

Add support for deserialization#2

Open
maroux wants to merge 9 commits into
clarkduvall:masterfrom
maroux:deserialization
Open

Add support for deserialization#2
maroux wants to merge 9 commits into
clarkduvall:masterfrom
maroux:deserialization

Conversation

@maroux

@maroux maroux commented Jun 3, 2015

Copy link
Copy Markdown
  • Added support for read-only fields
  • Renamed .data to .representation (and added .internal_value) for better clarity
  • Renamed .to_value to .to_representation (and added to_internal_value) for better clarity

TBD update documentation, fix benchmarks, update benchmarks

Fixes #1

Aniruddha Maru added 2 commits June 3, 2015 11:55
- Added support for read-only fields
- Renamed `.data` to `.representation` (and added `.internal_value`) for better clarity
- Renamed `.to_value` to `.to_representation` (and added `to_internal_value`) for better clarity

TBD update documentation, fix benchmarks, update benchmarks

Fixes clarkduvall#1
@clarkduvall

Copy link
Copy Markdown
Owner

thanks for the pr! will take a look tonight at it

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 6d7fcaf on maroux:deserialization into 3663aaf on clarkduvall:master.

@clarkduvall

Copy link
Copy Markdown
Owner

quick high level comment:
I like the idea of having a similar API to DRF serializers using .to_representation and .to_internal_value now that both serialization and deserialization is supported. Along with that, I would like to keep .data instead of .representation, since it will be an easier drop in replacement for DRF.

Comment thread serpy/fields.py Outdated

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would rather not specify the default to getattr and have these methods fail if method_name doesn't exist

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@clarkduvall

Copy link
Copy Markdown
Owner

It looks like DRF uses update/create/save for deserialization: http://www.django-rest-framework.org/api-guide/serializers/#saving-instances
and Marshmallow uses make_object: http://marshmallow.readthedocs.org/en/latest/quickstart.html#deserializing-to-objects

I'm thinking something like that would be nicer than the _cls class variable. What do you think of a make_object method like Marshmallow?

Comment thread serpy/serializer.py Outdated

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about allowing an instance of a serializer to either serialize or deserialize, but not both. This could have one obj parameter like it was before, and it will either fill out self._representation or self._internal_value, and throw an exception if it tries to do both.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't particularly like overriding parameters that way: Serializer(instance_or_data=foo). But - I agree that by making a serializer do only one of serialization or deserialization would make it faster (eg. don't need to compile "write fields")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I like your proposal below about using instance for serialization and data for deserialization, lets go with that

@clarkduvall

Copy link
Copy Markdown
Owner

would be cool to add some benchmarks comparing the deserialization speed to DRF and Marshmallow, but that doesn't have to be in this PR

@lovelydinosaur

Copy link
Copy Markdown

since it will be an easier drop in replacement for DRF.

Anything that'd make it easy to use as an alternative serializer implementation for REST framework would get a big +1 from me 😄

@maroux

maroux commented Jun 9, 2015

Copy link
Copy Markdown
Author

@clarkduvall @tomchristie Agreed that interface should be a drop-in replacement of DRF.

How about:

Serializer(instance=obj).data
and
Serializer(data=data).instance (this has to be slightly different because DRF 3.0 changed to explicitly using .create(validated_data). To be fair - Serpy could also do the same .create() does deserialization, but I like the property access pattern instead of method call.

Agreed on the _cls class variable. I wasn't sure what's the best way - perhaps Meta inner class? What do you think? I'm entirely sure about the impact of that on performance - I think it's time I set up the benchmarks :)

@clarkduvall

Copy link
Copy Markdown
Owner

@maroux I like the Serializer(instance=obj).data and Serializer(data=data).instance

I think it would be more flexible to have a function instead of just specifying the class for deserialization in case some extra massaging of the data has to be done before returning the instance. So if we had a make_instance function that takes the serialized data you could make the .instance property something like:

def make_instance(self, data):
  return SomeClass(**data)

@property
def instance(self):
  if self._instance is None:
    self._instance = self.make_instance(self.to_internal_value(self._data))
  return self._instance

by default make_instance could just be a noop

@maroux

maroux commented Jun 9, 2015

Copy link
Copy Markdown
Author

@clarkduvall So the problem with make_instance approach is two-fold: I think Serpy should do instance -> dict, and dict -> instance and since the default serializer is capable of taking in an object, I think it should also be able to create an object of arbitrary type.

How about this:

Serializer(instance=obj).data

and

Serializer(data=data).create(klass)

I'm not sure if there is value is setting a default value of klass to None, it would just return data as is? :/

@maroux

maroux commented Jun 9, 2015

Copy link
Copy Markdown
Author

Actually, disregard that suggestion, the Serializer(data=data).create(klass) approach will break nested serializers.

New API - `.data` and `.deserialized_value`

Making `cls` a kwarg in `Serializer` instead of attribute
@maroux

maroux commented Jun 11, 2015

Copy link
Copy Markdown
Author

@clarkduvall What do you think about #3?

I'm now inclined to go back to the Meta inner-class approach that will allow us customize the serializer behavior cleanly.

@clarkduvall

Copy link
Copy Markdown
Owner

Yeah I think as more features are added the Meta inner class will be better. Lets do that, we can also move default_getter and default_setter to the Meta class

Comment thread README.rst Outdated

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change these back to .data

@clarkduvall

Copy link
Copy Markdown
Owner

I still want a make_object function instead of doing cls() and setting attributes on it. I think by default the deserialized output should be a dict, and users shouldn't need to specify any class so we don't assume too much about what the final deserialized output will be. If they want it in class form, they can use the make_object method.

@maroux

maroux commented Jun 12, 2015

Copy link
Copy Markdown
Author

@clarkduvall I'm confused, wouldn't make_object become no-op in default case then since it's input is already a dict? :/

@clarkduvall

Copy link
Copy Markdown
Owner

yep, by default it would be a no-op unless you override it to return a class, see marshmallows implementation: https://github.com/marshmallow-code/marshmallow/blob/c4330c80073811bc175ffead89fe3cdc91797421/marshmallow/schema.py#L587-L595

For example to override it for a Django User model:

def make_object(self, data):
    return User(**data)

then when you do serializer.deserialized_data it will return a User object

@maroux

maroux commented Jun 12, 2015

Copy link
Copy Markdown
Author

I see what you mean. What's your thought on keeping it no-op in the default implementation, but have another serializer that lets you pass in cls?

@clarkduvall

Copy link
Copy Markdown
Owner

@maroux can you give me an example of what you mean?

@squamous

squamous commented Oct 1, 2015

Copy link
Copy Markdown

I just came across this great library. Deserialization would be a big win - I can't really contribute much at this point besides my gratitude.

@ngaurav

ngaurav commented Apr 18, 2016

Copy link
Copy Markdown

I would be really thankful if deserialization compatible with DRF is implemented.

@bikeshedder

Copy link
Copy Markdown

I'm using serpy in one of my projects and now I have to use DRF for deserialization and serpy for serialization. Having to define the same schema twice is rather unconvenient. Are there any plans when deserialization is going to be supported?

+1 for the make_object method rather than calling cls() as it allows for structures like {"type": "Foo", title="BlahBlah"} and {"type": "Bar", "title"="Blurb"} where the first is serialized as Foo and the second as Bar object. Keep it stupid simple and provide hooks to customize object creation. I also think that it is a good idea to pass the deserializer context to make_object. Otherwise it would be impossible to deserializer something like {"payload_type": "Foo", "payload_data": {"title": "BlahBlah"}}. That's a thing DRF does not support but Marshmallow does.

@clarkduvall

Copy link
Copy Markdown
Owner

I don't have a lot of time to work on this now, but if someone wants to implement this (or @maroux wants to continue this PR) using the make_object interface that was discussed earlier, I would definitely take a look at it.

Comment thread serpy/fields.py
# If to_value isn't a method, it must have been overriden.
if not isinstance(to_value, types.MethodType):
def to_internal_value(self, data):
return data

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't you still want to cast it back to int?

Comment thread serpy/serializer.py

:param instance: The object or objects to serialize.
:param data: The data to deserialize.
:param klass: The class for instantiating the deserialized object

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be used at all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Serpy doesn't support deserialization

8 participants