Optimistic concurrency control using ETags (attempt 2)

[chibisov]

I've reverted the merge #171 because I have some questions about it listed below.

Why do we need the yet another mixin classes from the concurrency/mixins.py? For example, we could add a new parameter to a etag decorator:

    @etag(precondition_required=True)
    def put(self, request, *args, **kwargs):
        ...

@precondition_required is not only about concurrency. As I understood it checks for any header provided in the precondition_map. And it's strange to see a map, containing method name when the decorator wraps already wraps the method:

    @precondition_required(precondition_map={'PUT': ['X-mycorp-custom']})
    #                                        ^ Here
    #   v And here
    def put(self, request, *args, **kwargs):
        ...

Please, try to split different parts of the pull request. For example:

• added missing fields = '__all__' should be represented as the another PR
• precondition_required yet another

And so on. It's hard to make the review.

[chibisov]

@auvipy, @kevin-brown, @pkainz

[pkainz]

Sorry for my late reply.

Why do we need the yet another mixin classes from the concurrency/mixins.py? For example, we could add a new parameter to a etag decorator:

@etag(precondition_required=True)
def put(self, request, *args, **kwargs):
    ...

My first implementation did something similar to this. In the discussion https://github.com/chibisov/drf-extensions/pull/171#issuecomment-270801572, https://github.com/chibisov/drf-extensions/pull/171#issuecomment-270867416 with @auvipy and @kevin-brown, they suggested to move it out of the @etag decorator to be more flexible, hence the new set of concurrency/mixins that combine @etag and @precondition_required.

@precondition_required is not only about concurrency. As I understood it checks for any header provided in the precondition_map.

Yes, that's the intended purpose, one point for moving it out of the @etag decorator.

And it's strange to see a map, containing method name when the decorator wraps already wraps the method:

@precondition_required(precondition_map={'PUT': ['X-mycorp-custom']})
#                                        ^ Here
#   v And here
def put(self, request, *args, **kwargs):
    ...

The name in the precondition map is actually supposed to refer to the HTTP verb in the request rather than to the name of the decorated method. For instance, in a viewset you could implement a custom update method, which per default accepts PUT and PATCH requests. So if you would only like to check for a particular header when PUT is used, you would use something like the following:

@precondition_required(precondition_map={'PUT': ['X-mycorp-custom']})
def update(self, request, *args, **kwargs):
    ...

Please, try to split different parts of the pull request. For example:

added missing fields = 'all' should be represented as the another PR precondition_required yet another And so on. It's hard to make the review.

Sorry about that.

[kevin-brown]

Note that part of the reason for the large API changes in #171 was because without them, this would be a backwards-incompatible change, and there was concern about doing it. The primary change in the pull request was adding the new new key bits and constructor.

Why do we need the yet another mixin classes from the concurrency/mixins.py?

This was one of my original questions when I was looking through the pull request.

For example, we could add a new parameter to a etag decorator:

This was changed after my second comment, as I noted that there are more headers than just If-Match that can return a "428 Precondition Required" response.

@precondition_required is not only about concurrency.

Correct, it has applications outside of just working with ETags and handling concurrency.

And it's strange to see a map, containing method name when the decorator wraps already wraps the method:

Part of the reason why I didn't mention this was because of list and detail routes, where you might need to enforce headers on certain requests.

Now that I look at the method, a more straightforward name of headers_required might be something to consider.

[chibisov]

This was changed after my second comment, as I noted that there are more headers than just If-Match that can return a "428 Precondition Required" response.

The same status code doesn't mean we have to use the same mechanism for returning it. I think If-Range and If-Match have a different domain of the application. We can enforce 428 from a different places.

they suggested to move it out of the @etag decorator to be more flexible, hence the new set of concurrency/mixins that combine @etag and @precondition_required.

I still think that @etag(precondition_required=True) looks much simpler. No need to know yet another mixin, decorator, method name mapping, header names, etc...

[auvipy]

I am going to reassess all the open issues with django 2.2 and drf 3.11. will be looking for inputs from all. I will like you to help me design and review the implementation