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

Assert fields in exclude are model fields #2319

Merged
merged 1 commit into from
Dec 18, 2014

Conversation

maryokhin
Copy link
Contributor

I guess something changed in the way DRF 3 includes default many-to-many fields for ModelSerializer, because as soon as I upgraded I got this unhelpful stack-trace:

Traceback:
File "/Users/maryokhin/.virtualenvs/backend-drf3/lib/python3.4/site-packages/django/core/handlers/base.py" in get_response
  111.                     response = wrapped_callback(request, *callback_args, **callback_kwargs)
File "/Users/maryokhin/.virtualenvs/backend-drf3/lib/python3.4/site-packages/django/views/decorators/csrf.py" in wrapped_view
  57.         return view_func(*args, **kwargs)
File "/Users/maryokhin/.virtualenvs/backend-drf3/lib/python3.4/site-packages/rest_framework/viewsets.py" in view
  85.             return self.dispatch(request, *args, **kwargs)
File "/Users/maryokhin/.virtualenvs/backend-drf3/lib/python3.4/site-packages/rest_framework/views.py" in dispatch
  407.             response = self.handle_exception(exc)
File "/Users/maryokhin/.virtualenvs/backend-drf3/lib/python3.4/site-packages/rest_framework/views.py" in dispatch
  404.             response = handler(request, *args, **kwargs)
File "/Users/maryokhin/.virtualenvs/backend-drf3/lib/python3.4/site-packages/rest_framework/mixins.py" in list
  46.         return Response(serializer.data)
File "/Users/maryokhin/.virtualenvs/backend-drf3/lib/python3.4/site-packages/rest_framework/serializers.py" in data
  422.         ret = super(Serializer, self).data
File "/Users/maryokhin/.virtualenvs/backend-drf3/lib/python3.4/site-packages/rest_framework/serializers.py" in data
  177.                 self._data = self.to_representation(self.instance)
File "/Users/maryokhin/.virtualenvs/backend-drf3/lib/python3.4/site-packages/rest_framework/serializers.py" in to_representation
  391.                 ret[field.field_name] = field.to_representation(attribute)
File "/Users/maryokhin/.virtualenvs/backend-drf3/lib/python3.4/site-packages/rest_framework/serializers.py" in to_representation
  524.             self.child.to_representation(item) for item in iterable
File "/Users/maryokhin/.virtualenvs/backend-drf3/lib/python3.4/site-packages/rest_framework/serializers.py" in <listcomp>
  524.             self.child.to_representation(item) for item in iterable
File "/Users/maryokhin/.virtualenvs/backend-drf3/lib/python3.4/site-packages/rest_framework/serializers.py" in to_representation
  384.         fields = [field for field in self.fields.values() if not field.write_only]
File "/Users/maryokhin/.virtualenvs/backend-drf3/lib/python3.4/site-packages/rest_framework/serializers.py" in fields
  277.             for key, value in self.get_fields().items():
File "/Users/maryokhin/.virtualenvs/backend-drf3/lib/python3.4/site-packages/rest_framework/serializers.py" in get_fields
  846.                     fields.remove(field_name)

Exception Type: ValueError at /channels/
Exception Value: list.remove(x): x not in list

Before I had to exclude this field specifically, now it's not even there.

@tomchristie
Copy link
Member

Before I had to exclude this field specifically, now it's not even there.

Sorry could you give the context for what code is giving you this stacktrace?

What's the most minimal example of a model and serializer that reproduces it?

@maryokhin
Copy link
Contributor Author

class Channel(models.Model):
    name = models.CharField(max_length=128)

class Subscription(models.Model):
    channel = models.ForeignKey(Channel, related_name='subscribers')
class ChannelSerializer(ModelSerializer):
    class Meta():
        model = Channel
        exclude = ('subscribers', )

Here I would have something like the following:

exclude = ('subscribers',)
field_name = 'subscribers'
fields = ['id', 'name']

I think that I had reverse relationships populated on 2.4.4, which is why I excluded it.

In any case, if it's a field name not in the ones that the serializer has (default or declared) it'll spill an unfriendly stack trace when trying to remove it.

@tomchristie
Copy link
Member

Okay, reverse relationships are not (have never been) included by default, so you don't need to do this, but we should clearly be more graceful in this case.

@tomchristie tomchristie added this to the 3.0.3 Release milestone Dec 18, 2014
tomchristie added a commit that referenced this pull request Dec 18, 2014
Assert fields in `exclude` are model fields
@tomchristie tomchristie merged commit e0096fe into encode:master Dec 18, 2014
@tomchristie
Copy link
Member

Thanks!

@maryokhin
Copy link
Contributor Author

I just realized that a long time ago Subscription model was a many-to-many through model, that was probably why it was added to the serializer as a forward relation. Then that changed to a regular many-to-many model, was still excluded, 2.4.4 did't care, 3.0 loudly failed.

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

Successfully merging this pull request may close these issues.

2 participants