From ecd3198c966a7d87efb8731004a27983f65e8ece Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Wed, 5 Jul 2023 14:33:09 +0200 Subject: [PATCH 1/3] BGDIINF_SB-2971: Changed the HTTP response to 409 in case of duplicate asset upload Now in case of a duplicate asset upload, it returns a 409 - conflict with the current upload id in the answer. --- app/stac_api/apps.py | 16 ++++++++++-- app/stac_api/exceptions.py | 34 +++++++++++++++++++++++++ app/stac_api/s3_multipart_upload.py | 2 +- app/stac_api/serializers.py | 7 ----- app/stac_api/views.py | 12 ++++++--- app/tests/test_asset_upload_endpoint.py | 25 +++++++++++++----- 6 files changed, 75 insertions(+), 21 deletions(-) create mode 100644 app/stac_api/exceptions.py diff --git a/app/stac_api/apps.py b/app/stac_api/apps.py index 31b4a244..e7b3573b 100644 --- a/app/stac_api/apps.py +++ b/app/stac_api/apps.py @@ -4,7 +4,9 @@ import django.core.exceptions from django.apps import AppConfig from django.conf import settings +from django.http import Http404 +import rest_framework.exceptions from rest_framework import serializers from rest_framework.response import Response from rest_framework.views import exception_handler @@ -25,6 +27,7 @@ def ready(self): def custom_exception_handler(exc, context): # NOTE: this exception handler is only called for REST Framework endpoints. Other endpoints # exception are handled via middleware.exception. + if isinstance(exc, django.core.exceptions.ValidationError): # Translate django ValidationError to Rest Framework ValidationError, # this is required because some validation cannot be done in the Rest @@ -34,6 +37,10 @@ def custom_exception_handler(exc, context): if exc.params: message %= exc.params exc = serializers.ValidationError(exc.message, exc.code) + elif isinstance(exc, Http404): + exc = rest_framework.exceptions.NotFound() + elif isinstance(exc, django.core.exceptions.PermissionDenied): + exc = rest_framework.exceptions.PermissionDenied() # Then call REST framework's default exception handler, to get the standard error response. response = exception_handler(exc, context) @@ -42,7 +49,7 @@ def custom_exception_handler(exc, context): # pylint: disable=protected-access extra = { "request": context['request']._request, - "request.query": context['request']._request.GET.urlencode() + "request.query": context['request']._request.GET.urlencode(), } if ( @@ -53,7 +60,12 @@ def custom_exception_handler(exc, context): extra["request.payload"] = context['request'].data logger.error("Response %s: %s", response.status_code, response.data, extra=extra) - response.data = {'code': response.status_code, 'description': response.data} + + data = {'code': response.status_code, 'description': exc.detail} + # Add any exception data object to the response if any (See StacAPIException) + if hasattr(exc, 'data'): + data.update(exc.data) + response.data = data return response # If we don't have a response that's means that we have an unhandled exception that needs to diff --git a/app/stac_api/exceptions.py b/app/stac_api/exceptions.py new file mode 100644 index 00000000..768e8766 --- /dev/null +++ b/app/stac_api/exceptions.py @@ -0,0 +1,34 @@ +import logging + +from django.utils.translation import gettext_lazy as _ + +import rest_framework.exceptions +from rest_framework import status + +logger = logging.getLogger(__name__) + + +class StacAPIException(rest_framework.exceptions.APIException): + '''STAC API custom exception + + These exception can add additional data to the HTTP response. + ''' + + def __init__(self, detail=None, code=None, data=None): + super().__init__(detail, code) + if isinstance(data, dict): + self.data = data + elif data: + self.data = {'data': data} + + +class UploadNotInProgressError(StacAPIException): + status_code = status.HTTP_409_CONFLICT + default_detail = _('Upload not in progress') + default_code = 'conflict' + + +class UploadInProgressError(StacAPIException): + status_code = status.HTTP_409_CONFLICT + default_detail = _('Upload in progress') + default_code = 'conflict' diff --git a/app/stac_api/s3_multipart_upload.py b/app/stac_api/s3_multipart_upload.py index 2a80c35b..c86beab2 100644 --- a/app/stac_api/s3_multipart_upload.py +++ b/app/stac_api/s3_multipart_upload.py @@ -11,7 +11,7 @@ from rest_framework import serializers -from stac_api.serializers import UploadNotInProgressError +from stac_api.exceptions import UploadNotInProgressError from stac_api.utils import get_s3_cache_control_value from stac_api.utils import get_s3_client from stac_api.utils import isoformat diff --git a/app/stac_api/serializers.py b/app/stac_api/serializers.py index a691e123..15a5cbee 100644 --- a/app/stac_api/serializers.py +++ b/app/stac_api/serializers.py @@ -7,7 +7,6 @@ from django.utils.translation import gettext_lazy as _ from rest_framework import serializers -from rest_framework.exceptions import APIException from rest_framework.utils.serializer_helpers import ReturnDict from rest_framework.validators import UniqueValidator from rest_framework_gis import serializers as gis_serializers @@ -828,9 +827,3 @@ class Meta: parts = serializers.ListField( source='Parts', child=UploadPartSerializer(), default=list, read_only=True ) - - -class UploadNotInProgressError(APIException): - status_code = 409 - default_detail = 'Upload not in progress' - default_code = 'conflict' diff --git a/app/stac_api/views.py b/app/stac_api/views.py index 4c3fe0fa..523f3359 100644 --- a/app/stac_api/views.py +++ b/app/stac_api/views.py @@ -13,12 +13,15 @@ from rest_framework import generics from rest_framework import mixins from rest_framework import serializers +from rest_framework.exceptions import APIException from rest_framework.generics import get_object_or_404 from rest_framework.permissions import AllowAny from rest_framework.response import Response from rest_framework_condition import etag from stac_api import views_mixins +from stac_api.exceptions import UploadInProgressError +from stac_api.exceptions import UploadNotInProgressError from stac_api.models import Asset from stac_api.models import AssetUpload from stac_api.models import Collection @@ -35,7 +38,6 @@ from stac_api.serializers import ConformancePageSerializer from stac_api.serializers import ItemSerializer from stac_api.serializers import LandingPageSerializer -from stac_api.serializers import UploadNotInProgressError from stac_api.serializers_utils import get_relation_links from stac_api.utils import get_asset_path from stac_api.utils import harmonize_post_get_for_search @@ -621,8 +623,10 @@ def _save_asset_upload(self, executor, serializer, key, asset, upload_id, urls): } ) if bool(self.get_in_progress_queryset()): - raise serializers.ValidationError( - code='unique', detail=_('Upload already in progress') + raise UploadInProgressError( + code='unique', + detail=_('Upload already in progress'), + data={"upload_id": self.get_in_progress_queryset()[0].upload_id} ) from None raise @@ -647,7 +651,7 @@ def create_multipart_upload(self, executor, serializer, validated_data, asset): ) self._save_asset_upload(executor, serializer, key, asset, upload_id, urls) - except serializers.ValidationError as err: + except APIException as err: executor.abort_multipart_upload(key, asset, upload_id) raise diff --git a/app/tests/test_asset_upload_endpoint.py b/app/tests/test_asset_upload_endpoint.py index 96798989..aa850399 100644 --- a/app/tests/test_asset_upload_endpoint.py +++ b/app/tests/test_asset_upload_endpoint.py @@ -242,6 +242,7 @@ def test_asset_upload_create_multipart_duplicate(self): json_data = response.json() self.check_created_response(json_data) self.check_urls_response(json_data['urls'], number_parts) + initial_upload_id = json_data['upload_id'] response = self.client.post( self.get_create_multipart_upload_path(), @@ -252,8 +253,18 @@ def test_asset_upload_create_multipart_duplicate(self): }, content_type="application/json" ) - self.assertStatusCode(400, response) - self.assertEqual(response.json()['description'], ["Upload already in progress"]) + self.assertStatusCode(409, response) + self.assertEqual(response.json()['description'], "Upload already in progress") + self.assertIn( + "upload_id", + response.json(), + msg="The upload id of the current upload is missing from response" + ) + self.assertEqual( + initial_upload_id, + response.json()['upload_id'], + msg="Current upload ID not matching the one from the 409 Conflict response" + ) self.assertEqual( self.get_asset_upload_queryset().filter(status=AssetUpload.Status.IN_PROGRESS).count(), @@ -313,13 +324,13 @@ def asset_upload_atomic_create_test(worker): results, errors = self.run_parallel(workers, asset_upload_atomic_create_test) for _, response in results: - self.assertStatusCode([201, 400], response) + self.assertStatusCode([201, 409], response) ok_201 = [r for _, r in results if r.status_code == 201] - bad_400 = [r for _, r in results if r.status_code == 400] + bad_409 = [r for _, r in results if r.status_code == 409] self.assertEqual(len(ok_201), 1, msg="More than 1 parallel request was successful") - for response in bad_400: - self.assertEqual(response.json()['description'], ["Upload already in progress"]) + for response in bad_409: + self.assertEqual(response.json()['description'], "Upload already in progress") class AssetUpload1PartEndpointTestCase(AssetUploadBaseTest): @@ -986,7 +997,7 @@ def test_asset_upload_1_parts_duplicate_complete(self): ) self.assertStatusCode(409, response) self.assertEqual(response.json()['code'], 409) - self.assertEqual(response.json()['description'], {'detail': 'Upload not in progress'}) + self.assertEqual(response.json()['description'], 'Upload not in progress') class AssetUploadDeleteInProgressEndpointTestCase(AssetUploadBaseTest): From 900e8db0973e1c98d5089ed10e44720d843a98bd Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Wed, 5 Jul 2023 14:55:45 +0200 Subject: [PATCH 2/3] BGDIINF_SB-2971: Updated the spec with the new 409 - Conflict response --- .../spec/v0.9/openapitransactional.yaml | 29 +++++++++++++++++-- spec/transaction/components/schemas.yaml | 19 ++++++++++++ spec/transaction/paths.yaml | 9 ++++-- 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/spec/static/spec/v0.9/openapitransactional.yaml b/spec/static/spec/v0.9/openapitransactional.yaml index 7664d29f..cc14422a 100644 --- a/spec/static/spec/v0.9/openapitransactional.yaml +++ b/spec/static/spec/v0.9/openapitransactional.yaml @@ -1040,6 +1040,13 @@ paths: $ref: "#/components/responses/BadRequest" "404": $ref: "#/components/responses/NotFound" + "409": + description: Another upload is already in progress, you need first to complete + or abort it before creating a new upload. + content: + application/json: + schema: + $ref: "#/components/schemas/uploadInProgress" 5XX: $ref: "#/components/responses/ServerError" /collections/{collectionId}/items/{featureId}/assets/{assetId}/uploads/{uploadId}: @@ -1254,8 +1261,7 @@ paths: $ref: "#/components/schemas/exception" example: code: 409 - description: - detail: Upload not in progress + description: Upload not in progress "404": $ref: "#/components/responses/NotFound" 5XX: @@ -3950,6 +3956,25 @@ components: type: string description: The RFC7232 ETag for the specified uploaded part. example: "d01af8b8ebbf899e30095be8754b377ddb0f0ed0f7fddbc33ac23b0d1969736b" + uploadInProgress: + description: Another upload is already in progress + properties: + code: + type: integer + example: 409 + description: + type: string + description: Description of the error + example: Upload already in progress + upload_id: + title: ID + type: string + description: Current Asset upload unique identifier + example: KrFTuglD.N8ireqry_w3.oQqNwrYI7SfSXpVRiusKah0YigDnuM06hfJNIUZg4R_No0MMW9FLU2UG5anTW0boTUYVxKfBZWCFXqnQTpjnQEo1K7la39MYpjSTvIbZgnG + required: + - code + - upload_id + type: object examples: inprogress: summary: In progress upload example diff --git a/spec/transaction/components/schemas.yaml b/spec/transaction/components/schemas.yaml index 6ffef8e6..3be69add 100644 --- a/spec/transaction/components/schemas.yaml +++ b/spec/transaction/components/schemas.yaml @@ -554,3 +554,22 @@ components: type: string description: The RFC7232 ETag for the specified uploaded part. example: "d01af8b8ebbf899e30095be8754b377ddb0f0ed0f7fddbc33ac23b0d1969736b" + uploadInProgress: + description: Another upload is already in progress + properties: + code: + type: integer + example: 409 + description: + type: string + description: Description of the error + example: Upload already in progress + upload_id: + title: ID + type: string + description: Current Asset upload unique identifier + example: KrFTuglD.N8ireqry_w3.oQqNwrYI7SfSXpVRiusKah0YigDnuM06hfJNIUZg4R_No0MMW9FLU2UG5anTW0boTUYVxKfBZWCFXqnQTpjnQEo1K7la39MYpjSTvIbZgnG + required: + - code + - upload_id + type: object \ No newline at end of file diff --git a/spec/transaction/paths.yaml b/spec/transaction/paths.yaml index fb2c8959..f16ffa3e 100644 --- a/spec/transaction/paths.yaml +++ b/spec/transaction/paths.yaml @@ -503,6 +503,12 @@ paths: $ref: "../components/responses.yaml#/components/responses/BadRequest" "404": $ref: "../components/responses.yaml#/components/responses/NotFound" + "409": + description: Another upload is already in progress, you need first to complete or abort it before creating a new upload. + content: + application/json: + schema: + $ref: "./schemas.yaml#/components/schemas/uploadInProgress" "5XX": $ref: "../components/responses.yaml#/components/responses/ServerError" "/collections/{collectionId}/items/{featureId}/assets/{assetId}/uploads/{uploadId}": @@ -714,8 +720,7 @@ paths: $ref: "./schemas.yaml#/components/schemas/exception" example: code: 409 - description: - detail: Upload not in progress + description: Upload not in progress "404": $ref: "../components/responses.yaml#/components/responses/NotFound" "5XX": From f5d7fe9454f920bede998f3808a6d7c8f1fd2009 Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Thu, 6 Jul 2023 14:21:27 +0200 Subject: [PATCH 3/3] BGDIINF_SB-2971: Improved error message Improved error message as mentioned in review. --- app/stac_api/exceptions.py | 4 ++-- app/stac_api/views.py | 2 -- app/tests/test_asset_upload_endpoint.py | 2 +- spec/static/spec/v0.9/openapitransactional.yaml | 2 +- spec/transaction/paths.yaml | 2 +- 5 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/stac_api/exceptions.py b/app/stac_api/exceptions.py index 768e8766..1897110c 100644 --- a/app/stac_api/exceptions.py +++ b/app/stac_api/exceptions.py @@ -24,11 +24,11 @@ def __init__(self, detail=None, code=None, data=None): class UploadNotInProgressError(StacAPIException): status_code = status.HTTP_409_CONFLICT - default_detail = _('Upload not in progress') + default_detail = _('No upload in progress') default_code = 'conflict' class UploadInProgressError(StacAPIException): status_code = status.HTTP_409_CONFLICT - default_detail = _('Upload in progress') + default_detail = _('Upload already in progress') default_code = 'conflict' diff --git a/app/stac_api/views.py b/app/stac_api/views.py index 523f3359..b03c755a 100644 --- a/app/stac_api/views.py +++ b/app/stac_api/views.py @@ -624,8 +624,6 @@ def _save_asset_upload(self, executor, serializer, key, asset, upload_id, urls): ) if bool(self.get_in_progress_queryset()): raise UploadInProgressError( - code='unique', - detail=_('Upload already in progress'), data={"upload_id": self.get_in_progress_queryset()[0].upload_id} ) from None raise diff --git a/app/tests/test_asset_upload_endpoint.py b/app/tests/test_asset_upload_endpoint.py index aa850399..67b2d816 100644 --- a/app/tests/test_asset_upload_endpoint.py +++ b/app/tests/test_asset_upload_endpoint.py @@ -997,7 +997,7 @@ def test_asset_upload_1_parts_duplicate_complete(self): ) self.assertStatusCode(409, response) self.assertEqual(response.json()['code'], 409) - self.assertEqual(response.json()['description'], 'Upload not in progress') + self.assertEqual(response.json()['description'], 'No upload in progress') class AssetUploadDeleteInProgressEndpointTestCase(AssetUploadBaseTest): diff --git a/spec/static/spec/v0.9/openapitransactional.yaml b/spec/static/spec/v0.9/openapitransactional.yaml index cc14422a..286d0808 100644 --- a/spec/static/spec/v0.9/openapitransactional.yaml +++ b/spec/static/spec/v0.9/openapitransactional.yaml @@ -1261,7 +1261,7 @@ paths: $ref: "#/components/schemas/exception" example: code: 409 - description: Upload not in progress + description: No upload in progress "404": $ref: "#/components/responses/NotFound" 5XX: diff --git a/spec/transaction/paths.yaml b/spec/transaction/paths.yaml index f16ffa3e..e3928974 100644 --- a/spec/transaction/paths.yaml +++ b/spec/transaction/paths.yaml @@ -720,7 +720,7 @@ paths: $ref: "./schemas.yaml#/components/schemas/exception" example: code: 409 - description: Upload not in progress + description: No upload in progress "404": $ref: "../components/responses.yaml#/components/responses/NotFound" "5XX":