search

encode / django-rest-framework

Web APIs for Django. 🎸
https://www.django-rest-framework.org
Other
28.16k stars 6.81k forks source link

Serializers should use models.Field.value_to_string if it exists #86

Closed quodlibetor closed 12 years ago

quodlibetor commented 12 years ago

As a last resort, if no serializer has been added to the Resource, this should be called, since it's Django standard.

sebpiq commented 12 years ago

@quodlibetor : I don't really understand what you mean ... Could you detail a bit ?

quodlibetor commented 12 years ago

It's been awhile since I encountered this, but I think that in serializers.py in Serializer.serialize_model djangorestframework goes through a whole bunch of attributes of each model field to check it there is a good way to serialize it.

But I think it checks the resource, not the model to which the resource corresponds. Since model fields are, according to the docs, supposed to provide their own serializers, duplicating it here without checking if there is already a valid serializer violates DRY, SSOT, etc.

I understand that it makes sense to provide a customizable hook for overriding serializers, but I do think that DRF should at least fall back to the Django-standard method if nothing else exists.

sebpiq commented 12 years ago

Ok thanks ... makes sense now :) Do you happen to know, how good are the default field serializers ? Because IMHO if you those default serializers can return an object that is not - for example - JSON serializable, maybe it makes more sense not to use them, since the final rendering will fail anyways. But, otherwise, that's a good idea and I'll look into it !

quodlibetor commented 12 years ago

Based on the docs they are what is used to serialize the object to the database. So they should return already serialized strings, and the results shouldn't be re-serialized. (That is, if you serialize the result you'll get a string with quotes on the ends, instead of a string representation of a number. I.E. "\"3\"" instead of "3".) And the string should be safe regarding database types, and I think--but do not know--that valid db strings are a subset of JSON, so it should be okay.

tomchristie commented 12 years ago

I've no idea if this is a sensible thing to do or not, but I'd be more than happy to take a look if anyone wants to submit a test which shows if/why this is important.

If it doesn't get addressed, I'll probably close the ticket off at some point, as it's not obvious to me that we're doing anything wrong with the current serialisation.

tomchristie commented 12 years ago

Should be addressed in 2.0's serialization.

https://github.com/tomchristie/django-rest-framework/blob/restframework2/djangorestframework/fields.py#L151