From affd506bc632882492a291cac08fb93886fb42e3 Mon Sep 17 00:00:00 2001
From: Antoine Lambert <antoine.lambert@inria.fr>
Date: Tue, 5 May 2020 16:09:28 +0200
Subject: [PATCH] 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 | 2 +-
swh/web/api/views/vault.py | 14 ++--
swh/web/common/service.py | 18 +++--
swh/web/tests/api/views/test_vault.py | 107 ++++++++++++++++++--------
4 files changed, 96 insertions(+), 45 deletions(-)
diff --git a/requirements-swh.txt b/requirements-swh.txt
index b64ebab8c..c30b5a81b 100644
--- a/requirements-swh.txt
+++ b/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
\ No newline at end of file
+swh.vault >= 0.0.33
\ No newline at end of file
diff --git a/swh/web/api/views/vault.py b/swh/web/api/views/vault.py
index 8701380b6..ff73031dd 100644
--- a/swh/web/api/views/vault.py
+++ b/swh/web/api/views/vault.py
@@ -1,4 +1,4 @@
-# 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)
diff --git a/swh/web/common/service.py b/swh/web/common/service.py
index 5ec880123..edae0c511 100644
--- a/swh/web/common/service.py
+++ b/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):
diff --git a/swh/web/tests/api/views/test_vault.py b/swh/web/tests/api/views/test_vault.py
index d4839ea9d..efb63ddfe 100644
--- a/swh/web/tests/api/views/test_vault.py
+++ b/swh/web/tests/api/views/test_vault.py
@@ -1,28 +1,36 @@
-# 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))
--
GitLab