estebistec / drf-compound-fields

Django-REST-framework serializer fields for compound types.
BSD 3-Clause "New" or "Revised" License
92 stars 12 forks source link

Fix behavior of ListField within serializer API #7

Closed estebistec closed 10 years ago

estebistec commented 10 years ago

Working on fixes needed for issue #4, and any other issues we discover.

mwarkentin commented 10 years ago

@estebistec I agree that these tests cover my error case. Thanks!

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling 3f2f4b427f30cf0d40c95630875080bcc5f9f8f5 on list-validate-fix into 6aa85952f6c7350fcf85bda2f75fbc3e8ecf9783 on master.

estebistec commented 10 years ago

I've just about got this fixed up. There's still a shot we could have a 0.1.0 by the end of the weekend.

mwarkentin commented 10 years ago

@estebistec Looking forward to it! We'll try to start using your package as soon as it's ready. :)

estebistec commented 10 years ago

something is going on in handing off the error-dict to the serializer errors. a field error that is a dict is coming through with empty values. suspect something is up in ValidationError messages attribute...

estebistec commented 10 years ago

hmm... okay, I think the errors in the dicts need to stay as lists, but them I'm running up against an error in django.core.exceptions.ValidationError.messages where the code is assuming an embedded ValidationError has a params attribute. Clearly, based on the constructor it may not...

estebistec commented 10 years ago

Yikes, @tomchristie, I wish I'd noticed the existence of NestedValidatorError sooner! Somehow I've been missing it, but it cleans up what I'm trying to do here quite nicely. If you have a moment, let me know what you think of the way I'm returning error for list items and dict values.

Also, @mwarkentin, see what you think of the form of resulting nested errors.

tomchristie commented 10 years ago

If you have a moment, let me know what you think of the way I'm returning error for list items and dict values.

Sure thing, if you include some docs on the format for errors then ping me and I'd be happy to take a browse over.

[ Tho I'm sure you've spent more time thinking about getting that right than I will have done anyways, so your judgement's prob. more valuable than mine in any case :) ]

estebistec commented 10 years ago

@tomchristie I've got another issue logged to do moar/bettar docs, but a sample of errors from the unit tests here:

{'emails': [{1: ValidationError([u'Enter a valid email address.'])}]}

So, what I've decided is that for both lists and dicts the embedded errors will be collected in a dict. For lists it is keyed by list-index (to avoid a sparsely populated list), and for dicts obviously the key for the value that the error applies to.

That dict is being embedded in a list, apparently due to the way way rest_framework collects nested errors.

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling f128f43047376691f3a6c3b547e5e781b4413357 on list-validate-fix into 6aa85952f6c7350fcf85bda2f75fbc3e8ecf9783 on master.

estebistec commented 10 years ago

Okay, we are finally done here :P

mwarkentin commented 10 years ago

Hey @estebistec, just noticed that you're returning the exception as part of the list: {'recipients': [{0: ValidationError([u'Enter a valid email address.'])}]}

Any thoughts on if this whether this should just be the error message, like this: https://github.com/django/django/blob/master/django/forms/forms.py#L371

estebistec commented 10 years ago

@mwarkentin I see it trapping ValidationError there, but where is it making it just the message? Further, the add_error method seems to continue to work with ValidationError objects.

That said, I'm not 100% confident that I'm handling it right. My gut would be to handle it the same way as an embedded serializer with the many=True option (and honestly, I should have looked at that earlier :/). Looking at a simple example, I see it is just the message. Logged #12 to fix this.