Browse Source

feat(api): improve error handling for concurrent requests

Nils Wisiol 5 years ago
parent
commit
cf35f60cf8
4 changed files with 50 additions and 13 deletions
  1. 6 0
      api/desecapi/exceptions.py
  2. 1 0
      api/desecapi/renderers.py
  3. 32 13
      api/desecapi/serializers.py
  4. 11 0
      api/desecapi/views.py

+ 6 - 0
api/desecapi/exceptions.py

@@ -25,3 +25,9 @@ class PDNSValidationError(ValidationError):
 class PDNSException(APIException):
 class PDNSException(APIException):
     def __init__(self, response=None):
     def __init__(self, response=None):
         return super().__init__(f'pdns response code: {response.status_code}, pdns response body: {response.text}')
         return super().__init__(f'pdns response code: {response.status_code}, pdns response body: {response.text}')
+
+
+class ConcurrencyException(APIException):
+    status_code = status.HTTP_429_TOO_MANY_REQUESTS
+    default_detail = 'Too many concurrent requests.'
+    default_code = 'concurrency_conflict'

+ 1 - 0
api/desecapi/renderers.py

@@ -11,6 +11,7 @@ class PlainTextRenderer(renderers.BaseRenderer):
         response = renderer_context.get('response')
         response = renderer_context.get('response')
 
 
         if response and response.exception:
         if response and response.exception:
+            # TODO enable the renderer to handle responses where reponse.data is a list
             if not isinstance(data, dict) or data.get('detail', None) is None:
             if not isinstance(data, dict) or data.get('detail', None) is None:
                 raise ValueError('Expected response.data to be a dict with error details in response.data[\'detail\'], '
                 raise ValueError('Expected response.data to be a dict with error details in response.data[\'detail\'], '
                                  'but got %s:\n\n%s' % (type(response.data), response.data))
                                  'but got %s:\n\n%s' % (type(response.data), response.data))

+ 32 - 13
api/desecapi/serializers.py

@@ -5,13 +5,16 @@ from base64 import urlsafe_b64decode, urlsafe_b64encode, b64encode
 
 
 from captcha.image import ImageCaptcha
 from captcha.image import ImageCaptcha
 from django.core.validators import MinValueValidator
 from django.core.validators import MinValueValidator
+from django.db import IntegrityError, OperationalError
 from django.db.models import Model, Q
 from django.db.models import Model, Q
 from rest_framework import serializers
 from rest_framework import serializers
+from rest_framework.exceptions import ValidationError
 from rest_framework.settings import api_settings
 from rest_framework.settings import api_settings
 from rest_framework.validators import UniqueTogetherValidator, UniqueValidator, qs_filter
 from rest_framework.validators import UniqueTogetherValidator, UniqueValidator, qs_filter
 
 
 from api import settings
 from api import settings
 from desecapi import crypto, models
 from desecapi import crypto, models
+from desecapi.exceptions import ConcurrencyException
 
 
 
 
 class CaptchaSerializer(serializers.ModelSerializer):
 class CaptchaSerializer(serializers.ModelSerializer):
@@ -442,19 +445,35 @@ class RRsetListSerializer(serializers.ListSerializer):
         deleted = known & empty
         deleted = known & empty
 
 
         ret = []
         ret = []
-        for subname, type_ in created:
-            ret.append(self.child.create(
-                validated_data=data_index[(subname, type_)]
-            ))
-
-        for subname, type_ in updated:
-            ret.append(self.child.update(
-                instance=instance_index[(subname, type_)],
-                validated_data=data_index[(subname, type_)]
-            ))
-
-        for subname, type_ in deleted:
-            instance_index[(subname, type_)].delete()
+
+        try:
+            for subname, type_ in created:
+                ret.append(self.child.create(
+                    validated_data=data_index[(subname, type_)]
+                ))
+
+            for subname, type_ in updated:
+                ret.append(self.child.update(
+                    instance=instance_index[(subname, type_)],
+                    validated_data=data_index[(subname, type_)]
+                ))
+
+            for subname, type_ in deleted:
+                instance_index[(subname, type_)].delete()
+
+        # time of check (does it exist?) and time of action (create vs update) are different,
+        # so for parallel requests, we can get integrity errors due to duplicate keys.
+        # This will be considered a 429-error, even though re-sending the request will be successful.
+        except OperationalError as e:
+            try:
+                if e.args[0] == 1213:
+                    # 1213 is mysql for deadlock, other OperationalErrors are treated elsewhere or not treated at all
+                    raise ConcurrencyException from e
+            except (AttributeError, KeyError):
+                pass
+            raise e
+        except (IntegrityError, models.RRset.DoesNotExist) as e:
+            raise ConcurrencyException from e
 
 
         return ret
         return ret
 
 

+ 11 - 0
api/desecapi/views.py

@@ -22,6 +22,7 @@ from rest_framework.viewsets import GenericViewSet
 
 
 import desecapi.authentication as auth
 import desecapi.authentication as auth
 from desecapi import serializers, models
 from desecapi import serializers, models
+from desecapi.exceptions import ConcurrencyException
 from desecapi.pdns_change_tracker import PDNSChangeTracker
 from desecapi.pdns_change_tracker import PDNSChangeTracker
 from desecapi.permissions import IsOwner, IsDomainOwner, WithinDomainLimitOnPOST
 from desecapi.permissions import IsOwner, IsDomainOwner, WithinDomainLimitOnPOST
 from desecapi.renderers import PlainTextRenderer
 from desecapi.renderers import PlainTextRenderer
@@ -334,6 +335,16 @@ class DynDNS12Update(APIView):
         except ValidationError as e:
         except ValidationError as e:
             if any('ttl' in error for error in e.detail):
             if any('ttl' in error for error in e.detail):
                 raise PermissionDenied({'detail': 'Domain not eligible for dynamic updates, please contact support.'})
                 raise PermissionDenied({'detail': 'Domain not eligible for dynamic updates, please contact support.'})
+
+            if any(
+                    any(
+                        getattr(non_field_error, 'code', '') == 'unique'
+                        for non_field_error
+                        in err.get('non_field_errors', [])
+                    )
+                    for err in e.detail
+            ):
+                raise ConcurrencyException from e
             raise e
             raise e
 
 
         with PDNSChangeTracker():
         with PDNSChangeTracker():