Django-rest-framework: Disabling PATCH.

[rrauenza]

I'd like to be able to disable PATCH. It seems there are two suggestions -- (a), build your own view out of mixins or (b), override the method but return an invalid status:

http://stackoverflow.com/questions/23639113/disable-a-method-in-a-viewset-django-rest-framework

The mixins solution doesn't seem to allow me to include PUT, but exclude PATCH. The override method doesn't affect OPTIONS.

I'd like to propose another solution, but it requires code changes. It looks like abilities are determined here:

def get_method_map(self, viewset, method_map):                              
    """                                                                     
    Given a viewset, and a mapping of http methods to actions,              
    return a new mapping which only includes any mappings that              
    are actually implemented by the viewset.                                
    """                                                                     
    bound_methods = {}                                                      
    for method, action in method_map.items():                               
        if hasattr(viewset, action):                                        
            bound_methods[method] = action                                  
    return bound_methods                                                    

What if this were changed to be:

       if hasattr(viewset, action):
            if viewset.action is not None:  # or check if is callable?                                      
                bound_methods[method] = action                                  

This would allow me to override/disable actions by just setting them to None in inherited classes.

Thoughts?

[tomchristie]

Given that this has come up before I'd be happy to reassess. My preference here would be to limit the requirement specifically to PATCH, and have a setting, that when set to false simply doesn't include the partial_update method on the mixin class.

[tomchristie]

Related as part of this re-assessment: #2945. Tho' may be better that we apply the behavior on the class itself, rather than creating a new mixin. Unsure.

[rrauenza]

I'm curious -- what's the argument for not allowing disabling of _any_ particular action? Seems like it could be a meta field similar to read_only, etc.

Is it to encourage handpicking minimal mixins for a viewset?

[tomchristie]

what's the argument for not allowing disabling of any particular action?

It's just introduces too many ways around to do things. I can see the argument with PATCH, where we're including partial_update on the mixin class alongside update, in a way that makes it harder to disable that the other methods (where you can just include whichever of the methods you want).

I'm not even sure I'm in favor of this for PATCH either, but would consider it.

[rrauenza]

I think I'd be inclined to go with separate PATCH and PUT mixins ... like #2945 suggests. Maybe UpdateModelMixin is a combination of Put and Patch mixins..

[tomchristie]

I'm going to suggest that we stay largely in feature freeze until the admin and other bits of kickstarter work are fully delivered, as we need to keep focus on them. Happy to reconsider this ticket at a later date however.

[tomchristie]

Reopening this based on further review.
I think it might be worth allowing PATCH specifically to be disabled throughout.

I'm not interested in doing this for the other methods, but PATCH is specifically relevant because partial validations may sometimes be awkward to properly support, and because not supporting PATCH operations is a pretty valid API style given their somewhat underspecified nature.

[jpadilla]

@tomchristie any idea on how you want to handle this? I can work on it.

[tomchristie]

I guess simply a global setting and only include the .patch methods on the generic views if it's set to True (the default)

For example:

class UpdateAPIView(mixins.UpdateModelMixin, GenericAPIView):
    def put(self, request, *args, **kwargs):
        return self.update(request, *args, **kwargs)

    if api_settings.SUPPORT_PATCH:
        def patch(self, request, *args, **kwargs):
            return self.partial_update(request, *args, **kwargs)

[tomchristie]

I do still have slightly mixed feelings on this, but doesn't seem unreasonable. Can kinda see myself setting SUPPORT_PATCH = False by default on APIs I create.

[rrauenza]

So if someone wanted some views to have PATCH and some to not have PATCH, set it globally to off and then explicitly add "def patch(...)" to views that need it?

[tomchristie]

Exactly, yes

[cancan101]

I guess I don't quite follow: what is the rationale to do add the ability to disable PATCH on a global level?

[tomchristie]

Closing this, see #3081 for more details.