api/vault: Fix 404 handling and improve error messages

Vault 404 error handling needed to be updated since changes related to
exception handling server side.

Also improve returned error messages, add missing test and slightly
refactor tests implementation (use hypothesis instead of harcoded data).

Closes T2388

requirements-swh.txt

@@ -4,4 +4,4 @@ swh.model >= 0.0.64
 swh.scheduler >= 0.0.72
 swh.search >= 0.0.4
 swh.storage >= 0.0.182
-swh.vault >= 0.0.32
+swh.vault >= 0.0.33
\ No newline at end of file

swh/web/api/views/vault.py

-# Copyright (C) 2015-2019  The Software Heritage developers
+# Copyright (C) 2015-2020  The Software Heritage developers
 # See the AUTHORS file at the top-level directory of this distribution
 # License: GNU Affero General Public License version 3, or any later version
 # See top-level LICENSE file for more information
@@ -18,13 +18,15 @@ from swh.web.api.views.utils import api_lookup
 # XXX: a bit spaghetti. Would be better with class-based views.
 def _dispatch_cook_progress(request, obj_type, obj_id):
     hex_id = hashutil.hash_to_hex(obj_id)
-    object_name = obj_type.split("_")[0].title()
+    object_name = obj_type.split("_")[0]
     if request.method == "GET":
         return api_lookup(
             service.vault_progress,
             obj_type,
             obj_id,
-            notfound_msg=("{} '{}' was never requested.".format(object_name, hex_id)),
+            notfound_msg=(
+                "Cooking of {} '{}' was never requested.".format(object_name, hex_id)
+            ),
             request=request,
         )
     elif request.method == "POST":
@@ -34,7 +36,7 @@ def _dispatch_cook_progress(request, obj_type, obj_id):
             obj_type,
             obj_id,
             email,
-            notfound_msg=("{} '{}' not found.".format(object_name, hex_id)),
+            notfound_msg=("{} '{}' not found.".format(object_name.title(), hex_id)),
             request=request,
         )
 
@@ -136,7 +138,7 @@ def api_vault_fetch_directory(request, dir_id):
         service.vault_fetch,
         "directory",
         obj_id,
-        notfound_msg="Directory with ID '{}' not found.".format(dir_id),
+        notfound_msg="Cooked archive for directory '{}' not found.".format(dir_id),
         request=request,
     )
     fname = "{}.tar.gz".format(dir_id)
@@ -243,7 +245,7 @@ def api_vault_fetch_revision_gitfast(request, rev_id):
         service.vault_fetch,
         "revision_gitfast",
         obj_id,
-        notfound_msg="Revision with ID '{}' not found.".format(rev_id),
+        notfound_msg="Cooked archive for revision '{}' not found.".format(rev_id),
         request=request,
     )
     fname = "{}.gitfast.gz".format(rev_id)

swh/web/common/service.py

@@ -11,10 +11,9 @@ from collections import defaultdict
 from typing import Any, Dict, List, Set, Iterator, Optional, Tuple
 
 from swh.model import hashutil
-
-from swh.storage.algos import diff, revisions_walker
-
 from swh.model.identifiers import CONTENT, DIRECTORY, RELEASE, REVISION, SNAPSHOT
+from swh.storage.algos import diff, revisions_walker
+from swh.vault.exc import NotFoundExc as VaultNotFoundExc
 from swh.web import config
 from swh.web.common import converters
 from swh.web.common import query
@@ -1090,22 +1089,29 @@ def lookup_directory_through_revision(revision, path=None, limit=100, with_data=
     return (rev["id"], lookup_directory_with_revision(rev["id"], path, with_data))
 
 
+def _vault_request(vault_fn, *args, **kwargs):
+    try:
+        return vault_fn(*args, **kwargs)
+    except VaultNotFoundExc:
+        return None
+
+
 def vault_cook(obj_type, obj_id, email=None):
     """Cook a vault bundle.
     """
-    return vault.cook(obj_type, obj_id, email=email)
+    return _vault_request(vault.cook, obj_type, obj_id, email=email)
 
 
 def vault_fetch(obj_type, obj_id):
     """Fetch a vault bundle.
     """
-    return vault.fetch(obj_type, obj_id)
+    return _vault_request(vault.fetch, obj_type, obj_id)
 
 
 def vault_progress(obj_type, obj_id):
     """Get the current progress of a vault bundle.
     """
-    return vault.progress(obj_type, obj_id)
+    return _vault_request(vault.progress, obj_type, obj_id)
 
 
 def diff_revision(rev_id):

swh/web/tests/api/views/test_vault.py

-# Copyright (C) 2017-2019  The Software Heritage developers
+# Copyright (C) 2017-2020  The Software Heritage developers
 # See the AUTHORS file at the top-level directory of this distribution
 # License: GNU Affero General Public License version 3, or any later version
 # See top-level LICENSE file for more information
 
+from hypothesis import given
+
 from swh.model import hashutil
+from swh.vault.exc import NotFoundExc
 from swh.web.common.utils import reverse
-
-TEST_OBJ_ID = "d4905454cc154b492bd6afed48694ae3c579345e"
-
-OBJECT_TYPES = ("directory", "revision_gitfast")
+from swh.web.tests.strategies import (
+    directory,
+    revision,
+    unknown_directory,
+    unknown_revision,
+)
 
 
-def test_api_vault_cook(api_client, mocker):
+@given(directory(), revision())
+def test_api_vault_cook(api_client, mocker, directory, revision):
     mock_service = mocker.patch("swh.web.api.views.vault.service")
 
-    for obj_type in OBJECT_TYPES:
+    for obj_type, obj_id in (
+        ("directory", directory),
+        ("revision_gitfast", revision),
+    ):
 
         fetch_url = reverse(
-            f"api-1-vault-fetch-{obj_type}",
-            url_args={f"{obj_type[:3]}_id": TEST_OBJ_ID},
+            f"api-1-vault-fetch-{obj_type}", url_args={f"{obj_type[:3]}_id": obj_id},
         )
         stub_cook = {
             "fetch_url": fetch_url,
-            "obj_id": TEST_OBJ_ID,
+            "obj_id": obj_id,
             "obj_type": obj_type,
             "progress_message": None,
             "status": "done",
@@ -34,7 +42,7 @@ def test_api_vault_cook(api_client, mocker):
         mock_service.vault_fetch.return_value = stub_fetch
 
         url = reverse(
-            f"api-1-vault-cook-{obj_type}", url_args={f"{obj_type[:3]}_id": TEST_OBJ_ID}
+            f"api-1-vault-cook-{obj_type}", url_args={f"{obj_type[:3]}_id": obj_id}
         )
 
         rv = api_client.post(url, {"email": "test@test.mail"})
@@ -48,7 +56,7 @@ def test_api_vault_cook(api_client, mocker):
 
         assert rv.data == stub_cook
         mock_service.vault_cook.assert_called_with(
-            obj_type, hashutil.hash_to_bytes(TEST_OBJ_ID), "test@test.mail"
+            obj_type, hashutil.hash_to_bytes(obj_id), "test@test.mail"
         )
 
         rv = api_client.get(fetch_url)
@@ -57,31 +65,35 @@ def test_api_vault_cook(api_client, mocker):
         assert rv["Content-Type"] == "application/gzip"
         assert rv.content == stub_fetch
         mock_service.vault_fetch.assert_called_with(
-            obj_type, hashutil.hash_to_bytes(TEST_OBJ_ID)
+            obj_type, hashutil.hash_to_bytes(obj_id)
         )
 
 
-def test_api_vault_cook_uppercase_hash(api_client):
+@given(directory(), revision())
+def test_api_vault_cook_uppercase_hash(api_client, directory, revision):
 
-    for obj_type in OBJECT_TYPES:
+    for obj_type, obj_id in (
+        ("directory", directory),
+        ("revision_gitfast", revision),
+    ):
 
         url = reverse(
             f"api-1-vault-cook-{obj_type}-uppercase-checksum",
-            url_args={f"{obj_type[:3]}_id": TEST_OBJ_ID.upper()},
+            url_args={f"{obj_type[:3]}_id": obj_id.upper()},
         )
         rv = api_client.post(url, {"email": "test@test.mail"})
 
         assert rv.status_code == 302
 
         redirect_url = reverse(
-            f"api-1-vault-cook-{obj_type}", url_args={f"{obj_type[:3]}_id": TEST_OBJ_ID}
+            f"api-1-vault-cook-{obj_type}", url_args={f"{obj_type[:3]}_id": obj_id}
         )
 
         assert rv["location"] == redirect_url
 
         fetch_url = reverse(
             f"api-1-vault-fetch-{obj_type}-uppercase-checksum",
-            url_args={f"{obj_type[:3]}_id": TEST_OBJ_ID.upper()},
+            url_args={f"{obj_type[:3]}_id": obj_id.upper()},
         )
 
         rv = api_client.get(fetch_url)
@@ -89,21 +101,51 @@ def test_api_vault_cook_uppercase_hash(api_client):
         assert rv.status_code == 302
 
         redirect_url = reverse(
-            f"api-1-vault-fetch-{obj_type}",
-            url_args={f"{obj_type[:3]}_id": TEST_OBJ_ID},
+            f"api-1-vault-fetch-{obj_type}", url_args={f"{obj_type[:3]}_id": obj_id},
         )
 
         assert rv["location"] == redirect_url
 
 
-def test_api_vault_cook_notfound(api_client, mocker):
-    mock_service = mocker.patch("swh.web.api.views.vault.service")
-    mock_service.vault_cook.return_value = None
-    mock_service.vault_fetch.return_value = None
+@given(directory(), revision(), unknown_directory(), unknown_revision())
+def test_api_vault_cook_notfound(
+    api_client, mocker, directory, revision, unknown_directory, unknown_revision
+):
+    mock_vault = mocker.patch("swh.web.common.service.vault")
+    mock_vault.cook.side_effect = NotFoundExc("object not found")
+    mock_vault.fetch.side_effect = NotFoundExc("cooked archive not found")
+    mock_vault.progress.side_effect = NotFoundExc("cooking request not found")
+
+    for obj_type, obj_id in (
+        ("directory", directory),
+        ("revision_gitfast", revision),
+    ):
+
+        obj_name = obj_type.split("_")[0]
+
+        url = reverse(
+            f"api-1-vault-cook-{obj_type}", url_args={f"{obj_type[:3]}_id": obj_id},
+        )
+
+        rv = api_client.get(url)
+
+        assert rv.status_code == 404, rv.data
+        assert rv["Content-Type"] == "application/json"
+        assert rv.data["exception"] == "NotFoundExc"
+        assert (
+            rv.data["reason"]
+            == f"Cooking of {obj_name} '{obj_id}' was never requested."
+        )
+        mock_vault.progress.assert_called_with(obj_type, hashutil.hash_to_bytes(obj_id))
+
+    for obj_type, obj_id in (
+        ("directory", unknown_directory),
+        ("revision_gitfast", unknown_revision),
+    ):
+        obj_name = obj_type.split("_")[0]
 
-    for obj_type in OBJECT_TYPES:
         url = reverse(
-            f"api-1-vault-cook-{obj_type}", url_args={f"{obj_type[:3]}_id": TEST_OBJ_ID}
+            f"api-1-vault-cook-{obj_type}", url_args={f"{obj_type[:3]}_id": obj_id}
         )
         rv = api_client.post(url)
 
@@ -111,13 +153,13 @@ def test_api_vault_cook_notfound(api_client, mocker):
         assert rv["Content-Type"] == "application/json"
 
         assert rv.data["exception"] == "NotFoundExc"
-        mock_service.vault_cook.assert_called_with(
-            obj_type, hashutil.hash_to_bytes(TEST_OBJ_ID), None
+        assert rv.data["reason"] == f"{obj_name.title()} '{obj_id}' not found."
+        mock_vault.cook.assert_called_with(
+            obj_type, hashutil.hash_to_bytes(obj_id), email=None
         )
 
         fetch_url = reverse(
-            f"api-1-vault-fetch-{obj_type}",
-            url_args={f"{obj_type[:3]}_id": TEST_OBJ_ID},
+            f"api-1-vault-fetch-{obj_type}", url_args={f"{obj_type[:3]}_id": obj_id},
         )
 
         rv = api_client.get(fetch_url)
@@ -125,6 +167,7 @@ def test_api_vault_cook_notfound(api_client, mocker):
         assert rv.status_code == 404, rv.data
         assert rv["Content-Type"] == "application/json"
         assert rv.data["exception"] == "NotFoundExc"
-        mock_service.vault_fetch.assert_called_with(
-            obj_type, hashutil.hash_to_bytes(TEST_OBJ_ID)
+        assert (
+            rv.data["reason"] == f"Cooked archive for {obj_name} '{obj_id}' not found."
         )
+        mock_vault.fetch.assert_called_with(obj_type, hashutil.hash_to_bytes(obj_id))

[Phabricator Migration user]

Some references in the commit message have been migrated:

• T2388 is now swh/meta#2388 (closed)