Only map 'allow_blank' keyword for fields that support it

[jannon]

Brief
Started using https://github.com/inonit/drf-haystack and it exposed an issue with the way field.blank gets mapped to allow_blank in ModelSerializer. Specifically, it is added for all fields instead of only the fields that actually support the keyword (CharField and ChoiceField I believe). This causes an error if one were to try to instantiate a different field type with the result of rest_framework.utils.field_mapping.get_field_kwargs (like so: https://github.com/inonit/drf-haystack/blob/master/drf_haystack/serializers.py#L114-L115)

Detail
The stack trace I ran into is at the bottom, but here's why it happens:

There is a model like so:

class MyModel(models.Model):
    problem_field = models.BooleanField(default=False, blank=True)
    ...

It is conceivable that one might have such a model field definition if they were using it to support both a DRF API and regular Django form validation.

The blank=True causes the code in field_mapping.py (https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/utils/field_mapping.py#L105-L106) to add the allow_blank keyword to the field kwargs.

So when drf_haystack gets the field kwargs and tries to instantiate a BooleanField, Field.__init__ throws TypeError: __init__() got an unexpected keyword argument 'allow_blank' because serializers.BooleanField doesn't pop it off the kwargs like serializers.CharField and serializers.ChoiceField do before passing the kwargs up.

Solutions
There are, of course, workarounds. drf-haystack could hack something up. I could not use blank=True and do some custom form handling, but this just exposed something I think should be a part of DRF behavior. If 'allow_blank` is going to be added for all field types, then all field types should accept it (and optionally, ignore it) or it should only be added for field types that support it.

I'd be happy to throw together a PR if you agree. I'm inclined to just check for instance of models.CharField before setting the keyword

stacktrace if you're interested

File "/lib/python3.4/site-packages/rest_framework/viewsets.py", line 85, in view
    return self.dispatch(request, *args, **kwargs)
  File "/lib/python3.4/site-packages/rest_framework/views.py", line 456, in dispatch
    response = self.handle_exception(exc)
  File "/lib/python3.4/site-packages/rest_framework/views.py", line 453, in dispatch
    response = handler(request, *args, **kwargs)
  File "/lib/python3.4/site-packages/rest_framework/mixins.py", line 47, in list
    return Response(serializer.data)
  File "/lib/python3.4/site-packages/rest_framework/serializers.py", line 622, in data
    ret = super(ListSerializer, self).data
  File "/lib/python3.4/site-packages/rest_framework/serializers.py", line 217, in data
    self._data = self.to_representation(self.instance)
  File "/lib/python3.4/site-packages/rest_framework/serializers.py", line 572, in to_representation
    self.child.to_representation(item) for item in iterable
  File "/lib/python3.4/site-packages/rest_framework/serializers.py", line 572, in <listcomp>
    self.child.to_representation(item) for item in iterable
  File "/lib/python3.4/site-packages/drf_haystack/serializers.py", line 137, in to_representation
    ret = super(HaystackSerializer, self).to_representation(instance)
  File "/lib/python3.4/site-packages/rest_framework/serializers.py", line 426, in to_representation
    fields = [field for field in self.fields.values() if not field.write_only]
  File "/lib/python3.4/site-packages/rest_framework/serializers.py", line 317, in fields
    for key, value in self.get_fields().items():
  File "/lib/python3.4/site-packages/drf_haystack/serializers.py", line 118, in get_fields
    field_mapping[field_name] = self._field_mapping[field_type](**kwargs)
  File "/lib/python3.4/site-packages/rest_framework/fields.py", line 506, in __init__
    super(BooleanField, self).__init__(**kwargs)
TypeError: __init__() got an unexpected keyword argument 'allow_blank'

[tomchristie]

What's a good way for us to be sure about or review exactly the full set of built-in fields that support blank=...?
Any sensible way for us to handle custom model fields?

[xordoquy]

Can't we define allow_blank at the class level - in CharField and make it a rule that it'll also be an init arg ?

[jannon]

blank= is defined on Field in django, so any django model field could have it set.

DRF maps that attribute on a django model to an allow_blank kwarg on every DRF serializer field. But as of now, only serializers.CharField and serializers.ChoiceField handle it. A simple grep for 'blank' shows everything involved with this.

The two main options I see are:

1. Only including the allow_blank kwarg when mapping fields that support it (i.e. fields that map to CharField or ChoiceField -- i.e. models.CharField and models.TextField) This is what only include 'allow_blank' on supported fields #3012 does.
2. Include allow_blank in the init kwargs of serializers.Field. I'm not sure if this is what you meant @xordoquy

[rhblind]

Hi,
For what it's worth, I faced the same problem a little while ago, and did actually hack up a "temporary" solution.
(It's in the v1.4 release)
rhblind/drf-haystack@8620c61

I figured, that since drf-haystack is solely used for reading data, some field attributes such as allow_blank is not so crucial for instantiating the fields (since this is used for validation before writing data). So I have this list of attributes which will be ignored before instantiating the serializer field.

It's not ideal though, I'd rather not have this list of fields to ignore, so thumbs up if this is getting fixed =)

[tomchristie]

Closed by #3012.