Fix 500 on delete nonexistent bundle, again (fixes #1918) (#2351)

Author: natanlao

dss-api.yml

@@ -1993,7 +1993,7 @@ paths:
                   code:
                     type: string
                     description: Machine-readable error code.  The types of return values should not be changed lightly.
-                    enum: [unhandled_exception, illegal_arguments, read_only]
+                    enum: [unhandled_exception, illegal_arguments, read_only, not_found]
                 required:
                   - code
   /bundles/{uuid}/checkout:

dss/api/bundles/__init__.py

@@ -1,4 +1,4 @@
-import io
+# -*- coding: utf-8 -*-
 import os
 import json
 import time
@@ -7,19 +7,19 @@
 
 import nestedcontext
 import requests
-from cloud_blobstore import BlobNotFoundError, BlobStore, BlobStoreTimeoutError
+from cloud_blobstore import BlobNotFoundError, BlobStoreTimeoutError
 from flask import jsonify, redirect, request, make_response
 from concurrent.futures import ThreadPoolExecutor, as_completed
 
 from dss import DSSException, dss_handler, DSSForbiddenException
 from dss.config import Config, Replica
-from dss.storage.blobstore import idempotent_save, test_object_exists, ObjectTest
+from dss.storage.blobstore import idempotent_save, ObjectTest, test_object_exists
 from dss.storage.bundles import get_bundle_manifest, save_bundle_manifest
 from dss.storage.checkout import CheckoutError, TokenError
 from dss.storage.checkout.bundle import get_dst_bundle_prefix, verify_checkout
-from dss.storage.identifiers import BundleTombstoneID, BundleFQID, FileFQID
+from dss.storage.identifiers import BundleTombstoneID, FileFQID
 from dss.storage.hcablobstore import BundleFileMetadata, BundleMetadata, FileMetadata
-from dss.util import UrlBuilder, security, hashabledict, multipart_parallel_upload
+from dss.util import UrlBuilder, security, hashabledict
 from dss.util.version import datetime_to_version_format
 
 """The retry-after interval in seconds. Sets up downstream libraries / users to
@@ -221,26 +221,23 @@ def delete(uuid: str, replica: str, json_request_body: dict, version: str = None
     )
 
     handle = Config.get_blobstore_handle(Replica[replica])
-    if test_object_exists(handle, Replica[replica].bucket, bundle_prefix, test_type=ObjectTest.PREFIX):
-        created, idempotent = idempotent_save(
-            handle,
-            Replica[replica].bucket,
-            tombstone_id.to_key(),
-            json.dumps(tombstone_object_data).encode("utf-8")
+    if not test_object_exists(handle, Replica[replica].bucket, bundle_prefix, test_type=ObjectTest.PREFIX):
+        raise DSSException(404, "not_found", "Cannot find bundle!")
+
+    created, idempotent = idempotent_save(
+        handle,
+        Replica[replica].bucket,
+        tombstone_id.to_key(),
+        json.dumps(tombstone_object_data).encode("utf-8")
+    )
+    if not idempotent:
+        raise DSSException(
+            requests.codes.conflict,
+            f"bundle_tombstone_already_exists",
+            f"bundle tombstone with UUID {uuid} and version {version} already exists",
         )
-        if not idempotent:
-            raise DSSException(
-                requests.codes.conflict,
-                f"bundle_tombstone_already_exists",
-                f"bundle tombstone with UUID {uuid} and version {version} already exists",
-            )
-        status_code = requests.codes.ok
-        response_body = dict()  # type: dict
-    else:
-        status_code = requests.codes.not_found
-        response_body = dict(title="bundle not found")
 
-    return jsonify(response_body), status_code
+    return dict(), requests.codes.ok
 
 
 def build_bundle_file_metadata(replica: Replica, user_supplied_files: dict):

tests/test_bundle.py

@@ -714,6 +714,10 @@ def _test_bundle_delete(self, replica: Replica, fixtures_bucket: str, authorized
         tombstone_exists = test_object_exists(handle, bucket, f"bundles/{bundle_uuid}.{bundle_version}.dead")
         self.assertEquals(tombstone_exists, authorized)
 
+    def test_delete_nonexistent(self):
+        nonexistent_uuid = str(uuid.uuid4())
+        self.delete_bundle(Replica.aws, nonexistent_uuid, authorized=True, expected_code=404)
+
     def test_no_replica(self):
         """
         Verify we raise the correct error code when we provide no replica.
@@ -846,7 +850,8 @@ def delete_bundle(
             replica: Replica,
             bundle_uuid: str,
             bundle_version: typing.Optional[str] = None,
-            authorized: bool = True):
+            authorized: bool = True,
+            expected_code: typing.Optional[int] = None):
         # make delete request
         url_builder = UrlBuilder().set(path="/v1/bundles/" + bundle_uuid).add_query('replica', replica.name)
         if bundle_version:
@@ -857,14 +862,15 @@ def delete_bundle(
         if bundle_version:
             json_request_body['version'] = bundle_version
 
-        expected_code = requests.codes.ok if authorized else requests.codes.forbidden
+        if not expected_code:
+            expected_code = requests.codes.ok if authorized else requests.codes.forbidden
 
         # delete and check results
         return self.assertDeleteResponse(
             url,
             expected_code,
             json_request_body=json_request_body,
-            headers=get_auth_header(authorized=authorized),
+            headers=get_auth_header(authorized=authorized)
         )
 
     @lru_cache()