Optimistic concurrency control using ETags

[pkainz]

API resources: Optimistic concurrency control using ETags

The Problem

As @mbox already mentioned, the default implementation in drf-extensions does not allow a correct optimistic locking using ETags and conditional requests. The ETag ties each request to a particular HTTP method, view, and response. While this works as expected for server-side caching (correctly returns status 304, and 200), it does not work to identify the requested resources when retrieving an object, altering it and putting it back to the server:

# Retrieve Request
GET /books/1/ HTTP/1.1
Accept: "application/json"

# Response
HTTP/1.1 200 OK
Content-Type: "application/json"
ETag: "591fa57af468db45942e5bf8ebe59378"

{'issn': '9780345531988', 'name': 'The Summons', 'id': 1, 'author': 'Stephen King'}


# Update Request
PUT /books/1/ HTTP/1.1
Accept: "application/json"
If-Match: "591fa57af468db45942e5bf8ebe59378"

{'issn': '9780345531988', 'name': 'The Summons', 'id': 1, 'author': 'John Grisham'}

# Response
HTTP/1.1 412 PRECONDITION FAILED 			# <- THIS SHOULD BE '200 OK'!
Content-Type: "application/json"
ETag: "<another_value_that_hashed_the_PUT_view>"

Related discussion in the DRF issues: encode/django-rest-framework#32

Proposed Changes

I have implemented the following changes/additions to enable optimistic locking using conditional requests using ETag, If-Match, If-None-Match HTTP headers in DRF-extensions. It is based on the requirement that we need an ETag value to represent the current semantic version of individual model instances rather than using it as caching function for a dynamic page.

• New KeyBits for retrieving and listing model instances: decoupled from the views and methods, the entries in the QuerySet are tuples represented by their mere field values.
• New KeyConstructors for API views that use the new KeyBits.
• New default functions to compute ETags for API views (in settings and utils)
• New API{method}ETAGMixins: List, Retrieve, Update, Destroy, as well as combined mixins for ReadOnly and APIETAGMixin.

The new features have been tested in unit tests where applicable and there exists a new functional test app in tests_app/tests/functional/concurrency. It tests a standard APIClient from the rest_framework to query Book models.

Additional Changes

Some other changes have been performed to ensure that all tests pass in django 1.10 and 1.9, DRF 3.5.3:

• added missing fields = '__all__' to serializers (ensures DRF>3.3 compatibility)
• removed double entry in DRF features dict rest_framework_extensions/utils.py

I documented the source code. The usage of the new mixins and decorators is identical to the current implementation that exists for @etag.

[auvipy]

I really appreciate your pr. allow me to review it

[auvipy]

also docs with some release notes are needed

[kevin-brown]

You should probably add .DS_Store to your global .gitignore file, so you don't always have to ignore it for every repo.

http://devoh.com/blog/2013/01/global-gitignore

[pkainz]

Thx for the hint, was missing that on this machine.

[pkainz]

@auvipy sure, go ahead. i will work on some docs and include release notes.

[pkainz]

@auvipy
I ran into some issues when using @api_etag with the DefaultAPIKeyConstructor. The constructor was malfunctioning and disallowed conditional updates, once the Etag was computed for the first call of the view. This locked us out and always returned 304 responses. Hence, I dropped the class and cleaned the code. Further, I fixed some issues with my unit/integration tests to pass with CI.

Docs in the development version has also been updated according to the guidelines.

[auvipy]

docs looks good. you could make the development version to 0.3.2 👍

[auvipy]

can this removal go through deprecation cycle?

[pkainz]

Sorry, but could you be more specific on that?

[kevin-brown]

@auvipy I'm pretty sure this only existed in the context of this pull request.

[kevin-brown]

The difference to the base class ETAGProcessor is that it cannot be used as @api_etag() decorator.

I can't quite figure out what this is trying to say, mostly because this is being aliased to api_etag later in the file.

The rest of the docstring makes sense, as this is basically the ETAGProcessor with the requirement that the function for building the etag must be specified.

[pkainz]

Yes you are right, this is a relict of the development phase and misleading. I removed that in a new commit.

[kevin-brown]

I'm definitely missing something when looking over the changes for this pull request. The follow changes make sense.

• It adds RetrieveModelKeyBit and ListModelKeyBit for making the key unique based on the representation of the individual model instance or list of model instances. This looks like it can be used independent of most of the other changes, as it's just a new way to calculate the key.
• It adds documentation about these new bits and the benefits that they bring.
• It adds new KeyConstructor instances that use these new bits to properly calculate ETag based on model states.

All of these should in one way or another fix the current issue that we have with ETags, where they aren't being generated properly. But what I can't figure out is why there is a whole new set of settings, mixins, and views being added on top of these changes? They allow for the new changes to be rolled out quickly, but they don't add much on top of the existing ETag settings, mixins, and views as it appears that you could realistically swap them both without much issue.

Wouldn't it make more sense to recommend that these new key bits and constructors are used within the documentation, and eventually (when a version change allows it) make these the default?

[pkainz]

But what I can't figure out is why there is a whole new set of settings, mixins, and views being added on top of these changes?

The main idea was not to break any existing functionality in the package and provide the new behaviour out of the box in drf-extensions. We do not necessarily need the new settings/mixins etc., but it reduces configuration effort in individual views/viewsets if one wants to make use of both the existing and the new functionality.

You can as well use the following code that has the same effect as the APIETAGMixin, but it overrides for all views inheriting the ETAGMixin.
In settings.py:

REST_FRAMEWORK_EXTENSIONS = {
    'DEFAULT_OBJECT_ETAG_FUNC': 'rest_framework_extensions.utils.default_api_object_etag_func',
    'DEFAULT_LIST_ETAG_FUNC': 'rest_framework_extensions.utils.default_api_list_etag_func',
}

In views.py:

from rest_framework import viewsets
from rest_framework_extensions.etag.mixins import ETAGMixin

from my_app.models import Model
from my_app.serializer import ModelSerializer

class SomeView(ETAGMixin, viewset.ModelViewSet):
    queryset = Model.objects.all()
    serializer = ModelSerializer

The alternative option is to keep the default as is and define it on a per-view(set) basis:

from rest_framework import viewsets
from rest_framework_extensions.etag.mixins import ETAGMixin
from rest_framework_extensions.utils import (default_api_object_etag_func, default_api_list_etag_func)

from my_app.models import Model
from my_app.serializer import ModelSerializer

class SomeView(ETAGMixin, viewset.ModelViewSet):
    object_etag_func = default_api_object_etag_func
    list_etag_func = default_api_list_etag_func

    queryset = Model.objects.all()
    serializer = ModelSerializer

Whichever you prefer works without the APIETAGMixin class, even without the APIETAGProcessor, but you would have to make sure that the etag_func parameter is passed to it. Any ideas?

[pkainz]

Wouldn't it make more sense to recommend that these new key bits and constructors are used within the documentation, and eventually (when a version change allows it) make these the default?

We could do that by adding example configurations (for instance as in #171 (comment)) on how to use the new KeyBits and KeyConstructors for the optimistic locking use case, including a hint that manual method decoration using @etag in a view without the etag_func argument will not work for optimistic locking. In my opinion the current solution is cleaner and does not contain this pitfall. But I am open to suggestions :)

[pkainz]

@auvipy @kevin-brown
What's your opinion on how to proceed with this PR?

[auvipy]

I would rather interested to get it in ASAP. but still willing to see others opinion before get merged. I would request you to be patient. and thanks a lot for all this work.

[auvipy]

@pkainz do you think anything could be done for better in this patch? otherwise i will merge it and wait for new pr's

[pkainz]

Yes, I have some additions. Will commit soon.

[pkainz]

@auvipy I added a new commit that adds support for mandatory conditional requests, and returns 428 PRECONDITION REQUIRED if the header is not present in the request. As default, we use If-Match headers for conditional unsafe operations (except POST). I updated the docs accordingly.

[kevin-brown]

Hi, back again to ask more questions.

Is there any reason why the precondition checks were wrapped in with etags instead of being broken out as their own decorator? The docs allude to additional use cases (which do exist, like If-Range and X-BULK-OPERATION) but it requires using etags in order to use it with those use cases.

[auvipy]

@pkainz could you address the issues raised by @kevin-brown

[pkainz]

Fair point, @kevin-brown. No there is not, I simply did not go through all other use cases that may require headers. A dedicated decorator, e.g. @precondition_required, seems reasonable, which can then neatly be mixed into any existing functionality. @auvipy, can you list the operations in drf-extensions that require conditional headers?

[pkainz]

Following the input of @kevin-brown, I removed the APIETAGProcessor and re-used the existing ETAGProcessor functionality to avoid confusion. There is now a generic decorator function @precondition_required for view methods that checks for a set of headers for each HTTP verb.
I added an out-of-the-box working configuration for optimistic concurrency control using a mixin for viewsets/views, that uses the new KeyBits and KeyConstructors to construct an ETag from a resource's attribute values. It returns 428 PRECONDITION REQUIRED, if the required headers are not present. See the updated docs for details.

The principal functionality for checking custom headers is now provided. However, I will not integrate this new pattern into the existing bulk-operations. Maybe someone else will submit a PR for this refactoring. @auvipy, could you merge that PR?

[auvipy]

@kevin-brown could you provide a final feedback as it seems he addressed some of your suggestions?

@pkainz Could you please open one/more relevant issues describing the possible refactors?

[auvipy]

going to reinvestigate this again