vintasoftware / drf-rw-serializers

Generic views, viewsets and mixins that extend the Django REST Framework ones adding separated serializers for read and write operations
MIT License
177 stars 28 forks source link

Assertion error when used only with `get_serializer_class()` implementation #86

Closed sshishov closed 3 months ago

sshishov commented 3 months ago

Description

We are currently in the middle of migration to RW serializers, meaning some viewsets has get_read/write_serializer_class implemented and some using old approach (1 serializer) which end-up being served from get_serializer_class. With the latest changes if the class implements get_serializer_class and does not implement serializer_class attribute, then assertion error raised.

What I Did

Below I will try to post the simple code snippet

class MyApiViewSet(drf_rw_serializers_viewsets.GenericViewSet[MyModel]):
    queryset = MyModel.objects.all()

    @typing_extensions.override
    def get_serializer_class(self) -> type[drf_serializers.BaseSerializer[MyModel]:
        serializer_class:type[drf_serializers.BaseSerializer[MyModel] | None = None
        if self.action == 'action_one':
            serializer_class = SomeSerializer
        if serializer_class is None:
            msg = f'Unknown action: {self.action}'
            raise ValueError(msg)
        return serializer_class

This snippet is very good to find out if you are missing any serializer for any action (especially it is done during schema generation).

This code was not producing any errors in 1.1.x version but produces this in 1.2.x+:

AssertionError: 'MyApiViewSet' should either include one of `serializer_class` and
`read_serializer_class` attribute, or override one of the `get_serializer_class()`,
`get_read_serializer_class()` method.

We do override get_serializer_class but apparently in the RW serializer sources it is not checked.

UPDATE:

Okay, the issue is a little bit different.

In our codebase we are calling self.get_wirte_serializer (note that we do not have write_serializer_class attribute, we do not have get_write_serializer_class method, we rely only on get_serializer_class in this case.

Stack trace is the following:

  File "/home/user/app/src/application/view.py", line 123, in some_method
    serializer = self.get_write_serializer(data=data)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/venv/lib/python3.11/site-packages/drf_rw_serializers/generics.py", line 96, in get_write_serializer
    serializer_class = self.get_write_serializer_class()
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/venv/lib/python3.11/site-packages/drf_rw_serializers/generics.py", line 109, in get_write_serializer_class
    return self._get_serializer_class()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/venv/lib/python3.11/site-packages/drf_rw_serializers/generics.py", line 23, in _get_serializer_class
    self.serializer_class is not None

Please note that the error is about read somehow:

'MyApiViewSet' should either include one of `serializer_class` and `read_serializer_class` attribute, or override one of the `get_serializer_class()`, `get_read_serializer_class()` method.
fjsj commented 3 months ago

The error message is probably wrong, but we'll do a git blame and find out why this behavior was introduced. Maybe was asked or contributed by someone. Could you please check both the message and the behavior @pamella ?

pamella commented 3 months ago

Hello, @sshishov! We just released a correction for that in version 1.4.0. Could you try the update and let us know if you are no longer experiencing the problem? Thanks!

sshishov commented 2 months ago

Hi @pamella and @fjsj , we still were not able to test the fix as we stuck in v1.1.1 due to the issues mentioned above.

With the latest release we still getting errors during running our unit-test suite. I will shed a light on the issues we are having later on.