Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ModelSerializer dies on IntegerField with a max_length argument #2317

Closed
hakanw opened this issue Dec 18, 2014 · 7 comments
Closed

ModelSerializer dies on IntegerField with a max_length argument #2317

hakanw opened this issue Dec 18, 2014 · 7 comments
Labels
Milestone

Comments

@hakanw
Copy link

hakanw commented Dec 18, 2014

If an IntegerField, or PositiveIntegerField or any other number field has a max_length argument, then any serialization fails.

Here's the stacktrace:

Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/Users/hakan/.virtualenvs/opinion/lib/python2.7/site-packages/rest_framework/serializers.py", line 434, in __repr__
    return unicode_to_repr(representation.serializer_repr(self, indent=1))
  File "/Users/hakan/.virtualenvs/opinion/lib/python2.7/site-packages/rest_framework/utils/representation.py", line 75, in serializer_repr
    fields = serializer.fields
  File "/Users/hakan/.virtualenvs/opinion/lib/python2.7/site-packages/rest_framework/serializers.py", line 312, in fields
    for key, value in self.get_fields().items():
  File "/Users/hakan/.virtualenvs/opinion/lib/python2.7/site-packages/rest_framework/serializers.py", line 1044, in get_fields
    ret[field_name] = field_cls(**kwargs)
  File "/Users/hakan/.virtualenvs/opinion/lib/python2.7/site-packages/rest_framework/fields.py", line 647, in __init__
    super(IntegerField, self).__init__(**kwargs)
TypeError: __init__() got an unexpected keyword argument 'max_length'
@hakanw
Copy link
Author

hakanw commented Dec 18, 2014

Simple way to test this. Make the following test model, which is perfectly valid django:

class TestModel(models.Model):
     hours = models.SmallIntegerField("hours", max_length=2, default=0)

and then try to automatically serialize it with ModelSerializer:

class TestSerializer(serializers.ModelSerializer):
    class Meta:
        model=TestModel

@tomchristie
Copy link
Member

tomchristie commented Dec 18, 2014

max_length isn't valid on IntegerField in Django, at least on master, and although it won't break it is marked as a warning. - https://github.com/django/django/blob/750dbb11335ef7000eae3b58df0111f715ff3f90/django/db/models/fields/__init__.py#L1767-L1772

Having noted that, we should still ensure that REST framework handles this case gracefully.

@tomchristie tomchristie added this to the 3.0.3 Release milestone Dec 18, 2014
@hakanw
Copy link
Author

hakanw commented Dec 18, 2014

Yes, I agree it makes little sense to use it. Maybe it's so form fields will get that argument and validate properly? Anyhow, there seems to be some apps out there in the wild that do use it.

Although I haven't looked at the code yet, maybe a good general solution would be for these serializer fields to ignore arguments they don't recognize? That would also make this code much more future proof against changes in django.

@hakanw
Copy link
Author

hakanw commented Dec 23, 2014

I've tried to fix this in PR #2345. Please give me comments on the fix, as it's my first contribution to this project.

@tomchristie
Copy link
Member

tomchristie commented Dec 23, 2014

Sorry I've not been more helpful with detailing how to fix this the right way around, but pushed for time ATM. Will try to pick up with more advice in due course. Appreciate the work.

@tomchristie
Copy link
Member

tomchristie commented Dec 28, 2014

Thanks for your input on this @hakanw - I've gone ahead and resolved this now as seemed simpler than passing back another review noting how to deal with it.

@hakanw
Copy link
Author

hakanw commented Dec 28, 2014

@tomchristie aah, nice fix. I was messing around with another try but in fields.py, obviously not the right place. Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants