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

Possible bug #20

Open pySilver opened 10 years ago

pySilver commented 10 years ago

Hi!

Here is a possible bug: https://github.com/estebistec/drf-compound-fields/blob/master/drf_compound_fields/fields.py#L105

Shouldn't it be(?):

self.list_field = ListField(item_field, *args, **kwargs)

Otherwise validators may report required field error for some circumstances. Consider example:

myfield = ListOrItemField(IntegerField(), required=False, blank=True)

so, it woud result into:

[] -> Error myfield is required

estebistec commented 10 years ago

so in your example, a list value would be optional but an integer value would be required?

pySilver commented 10 years ago

@estebistec yeah well thats not clear from my example.

here is a serializer:

class MySerializer(Serializer):
    authors = ListOrItemField(
        PrimaryKeyRelatedField(
            queryset=Authors.objects.all(),
            error_messages={
                'does_not_exist': _(u"Invalid Account: %s")
            },
        ),
        write_only=True,
        required=False,
        blank=True
    )

So, since requred=False, authors field expected to accept empty values, such as empty list. Thats how DRF would behave. But in reality it would behave like DRF if empty value won't be passed by api client at all. Otherwise it would result in error:

{
    "authors": ['Field `authors` is required', ]
}

This is happening because undelying ListField did not receive _args and *_kwargs that were defined for original authors field, so it fails while validating field value at DRF side:

rest_framework.fields.WritableField#validate:

def validate(self, value):
        if value in validators.EMPTY_VALUES and self.required:
            raise ValidationError(self.error_messages['required'])

And this problem exists just for ListOrItemField, since ListField normally constructs field using _args and *_kwargs passed from serializer.

estebistec commented 10 years ago

Okay, I see. I'll poke at a fix this evening. Thanks for reporting it!

estebistec commented 10 years ago

Sorry, i haven't forgotten this. Simply passing args and kwargs didn't fix this like I'd hoped, so I have to dig deeper.

pySilver commented 10 years ago

Hm, that worked for me just fine. So passing empty list as value to a ListOrItemField did not result in required error. However passing an empty string would result in some other error – but this is how DRF work (and will work until 3.0)

estebistec commented 9 years ago

@pySilver have you moved to DRF 3.0 yet? I'm thinking of upgrading this lib for it and if you're game I would ensure this bug is fixed or not present in that upgrade.

pySilver commented 9 years ago

@estebistec nop, our app is too big to make an easy upgrade to 3.0 :/

estebistec commented 9 years ago

Please see #27