Allow POST, PATCH, DELETE for custom actions in ReadOnlyModelViewSet

[kolomenkin]

Allow POST, PATCH, DELETE for actions in ReadOnlyModelViewSet (#797)

Currently if you try to use POST for action in ReadOnlyModelViewSet you will get problems:

• with DRF UI you will loose data input forms
• drf_yasg package will not generate OpenAPI specifications for such actions

This behavior was added to django-rest-framework-json-api in version 2.8.0 by mistake:

Commit: 7abd764
Subject: remove disallowed PUT method. (#643)

=========================
@n2ygk, please review my PR because I'm changing your code from commit 7abd764.

[codecov]

Codecov Report

Merging #797 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #797      +/-   ##
==========================================
+ Coverage   97.40%   97.45%   +0.05%     
==========================================
  Files          56       56              
  Lines        3007     3068      +61     
==========================================
+ Hits         2929     2990      +61     
  Misses         78       78              

Impacted Files	Coverage Δ
example/tests/test_views.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 073c45b...ae98a93. Read the comment docs.

[auvipy]

why if its readOnly?

[kolomenkin]

I'm developing the following API:

Read-only CRUD + some changing actions

For example:

/api/items/ - GET
/api/items/<id>/ - GET
/api/items/<id>/myaction - POST

For such API I'm using readonly ViewSet + @action myaction with methods=['post']

My proposed fix does not allow POST, DELETE, PATCH for /api/items/ or /api/items/<id>/
But it makes possible to use them in actions without workarounds.

I can prepare a test project with example if you like.

[kolomenkin]

Here is a draft of API:

from django.db import models
import rest_framework_json_api.serializers
import rest_framework_json_api.views


class Department(models.Model):
    id = models.AutoField(db_column='DepartmentId', primary_key=True)
    name = models.CharField(db_column='DepartmentName', max_length=20, null=False, unique=True)


class DepartmentSerializer(rest_framework_json_api.serializers.HyperlinkedModelSerializer):
    class Meta:
        model = Department
        fields = ('id', 'name',)


class DepartmentViewSet(rest_framework_json_api.views.ReadOnlyModelViewSet):
    serializer_class = DepartmentSerializer
    queryset = Department.objects.all().order_by('id')
    # `test_action_post` action will work fine using HTTP POST.
    # But without next line DRF UI will be broken: no HTML form input elements will be shown for this action.
    # And without next line OpenAPI (swagger) is not generated for `test_action_post` using package `drf_yasg`
    # http_method_names = ['get', 'post', 'patch', 'delete', 'head', 'options']

    @action(
        detail=True,
        methods=['post'],
    )
    def test_action_post(self, _request, pk: str):
        return Response(status=status.HTTP_204_NO_CONTENT)

[kolomenkin]

Broken DRF UI is shown in this screenshot.
This screenshot is done with uncommented line from example code.
Without my change there will be nothing inside of the red rectangle.

[sliverc]

Thanks for your PR. Yes this makes sense. When this change was done we forgot about custom actions.

Could you add a test though with a custom action on a read only viewset? This way it can be ensured that this error doesn't occur again in the future.

Also can you please revert the ordering of the AUTHORS file? I agree that this should be sorted but would like to keep a PR to one change. This can be done in another PR later one.

Thanks.

[kolomenkin]

Also can you please revert the ordering of the AUTHORS file?

This is easy. I made this in a separate commit just in case :)

Could you add a test though with a custom action on a read only viewset?

I will try.

[auvipy]

now got it. cool work man!

[kolomenkin]

I have added 8 tests: for each HTTP method and for 2 types for custom actions (detail=False and detail=True).

I'm not sure if it is the most correct way to test this.
At least all these tests are failing without my fix :)

I did not try, but these tests should pass also on django-rest-framework-json-api version 2.7.0

UPDATED:

I wonder if we need to test OPTIONS, HEAD are working fine for custom actions. We can break them too if http_methods is assigned incorrectly.
But in this case we should test not only ReadOnlyModelViewSet but also ModelViewSet and RelationshipView. They had similar changes in 2.8.0.

UPDATED-2:

by the way. This PR should not affect coverage.

But according to the build bot coverage was improved. It happened because coverage counts tests folder too.
But I do not think we have a goal to cover tests. We are trying to cover the main code of application by writing tests.
So I would propose to exclude tests folder from coverage in a separate PR.
Please tell me if you approve the idea of new PR.

[sliverc]

Thanks for your work. You have added tests for the regressions you have found so I think this is OK and we merge it as is.

In terms of test coverage. Tests are also included in the coverage because if there is a code line which is not covered it indicates that most likely this is dead code which can be removed.