django-rest-framework-bulk icon indicating copy to clipboard operation
django-rest-framework-bulk copied to clipboard

BulkSerializerMixin makes Serializers difficult to test

Open hanssto opened this issue 10 years ago • 4 comments

BulkSerializerMixin.to_internal_values(...) does this:

        request_method = getattr(getattr(self.context.get('view'), 'request'), 'method', '')

So, given the serializer code:

from rest_framework import serializers
from rest_framework_bulk import BulkListSerializer, BulkSerializerMixin
class SimpleSerializer(BulkSerializerMixin, serializers.Serializer):
    simple_field = serializers.CharField()
    class Meta:
        list_serializer_class = BulkListSerializer

And a test case:

from django.test import TestCase
from myproject.serializers import SimpleSerializer
class SimpleSerializerTestCase(TestCase):

    def test_single(self):
        data = {
            "simple_field": "foo",
        }
        serializer = SimpleSerializer(data=data)
        self.assertTrue(serializer.is_valid())

    def test_multiple(self):
        data = [
            {"simple_field": "foo"},
            {"simple_field": "bar"},
        ]
        serializer = SimpleSerializer(data=data, many=True)
        self.assertTrue(serializer.is_valid())

both of these tests will fail with

Traceback (most recent call last): ... File ".../local/lib/python2.7/site-packages/rest_framework_bulk/drf3/serializers.py", line 19, in to_internal_value request_method = getattr(getattr(self.context.get('view'), 'request'), 'method', '') AttributeError: 'NoneType' object has no attribute 'request'

One can initialize the Serializer with a mock, to get rid of the problem:

        import mock
        view = mock.Mock()
        view.request.method = "POST"
        serializer = SimpleSerializer(data=data, context={"view": view})
        self.assertTrue(serializer.is_valid())

but ideally, a Serializer should be testable in isolation, with no notion of a view involved.

I suggest one or more of:

  • Make to_internal_value() look at instance(s) somehow to determine which of the cases is appropriate. (I honestly have no clue if this is possible)
  • Ignore the lookup field if there is no view. Also not ideal for testing.
  • Add a note to the docs.
  • Add test cases to the project which tests against Serializers in isolation to discover other issues.

My apologies if I missed something obvious.

hanssto avatar Jul 31 '15 09:07 hanssto

but ideally, a Serializer should be testable in isolation, with no notion of a view involved.

I agree with that.

Make to_internal_value() look at instance(s) somehow to determine which of the cases is appropriate. (I honestly have no clue if this is possible)

Ill have to think about this. Maybe we can simply check for existence of instance on the serializer. I can see some issues with that as well. Checking for self.instance might not work since to_internal_value does not have direct access to the self.instance in the case of serializer being used as nested serializer in some other serializer. You can probably do self.root.instance but that might not work either since its plausible that in the update of a root serializer, a child serializer resource might need to be created.....

Ignore the lookup field if there is no view. Also not ideal for testing.

You are right and that is not ideal since in this case update will not work.

Add a note to the docs.

I can certainly do that.

Add test cases to the project which tests against Serializers in isolation to discover other issues.

good idea. I am swamped at work so if you are willing, you can submit a PR!

miki725 avatar Jul 31 '15 11:07 miki725

We just ran into this when creating tests for the bulk serializers. Is there any reason why it is looking for view in the context instead of just request? The request is passed in as the serializer context, and it's considerably easier to mock in tests.

kevin-brown avatar May 26 '16 17:05 kevin-brown

This is not just a problem for testing. I am using the serializer to push messages across AMQP. In this case, there is no view. I get the same exception.

sww314 avatar Nov 25 '16 21:11 sww314

Has anyone found a fix for this issue? My DRF post APIView was working fine until i changed it to ListBulkCreateUpdateAPIView.

morufajibike avatar Mar 13 '17 11:03 morufajibike