diff --git a/swh/web/api/apidoc.py b/swh/web/api/apidoc.py
--- a/swh/web/api/apidoc.py
+++ b/swh/web/api/apidoc.py
@@ -14,11 +14,10 @@
import docutils.nodes
import docutils.parsers.rst
import docutils.utils
-import sentry_sdk
from rest_framework.decorators import api_view
-from swh.web.api.apiresponse import error_response, make_api_response
+from swh.web.api.apiresponse import make_api_response
from swh.web.api.apiurls import APIUrls
from swh.web.common.utils import parse_rst
@@ -287,11 +286,7 @@
def api_doc(
- route: str,
- noargs: bool = False,
- tags: List[str] = [],
- handle_response: bool = False,
- api_version: str = "1",
+ route: str, noargs: bool = False, tags: List[str] = [], api_version: str = "1",
):
"""
Decorator for an API endpoint implementation used to generate a dedicated
@@ -310,10 +305,6 @@
* hidden: remove the entry points from the listing
* upcoming: display the entry point but it is not followable
-
- handle_response: indicate if the decorated function takes
- care of creating the HTTP response or delegates that task to the
- apiresponse module
api_version: api version string
"""
@@ -351,15 +342,10 @@
def documented_view(request, **kwargs):
doc_data = get_doc_data(f, route, noargs)
try:
- response = f(request, **kwargs)
+ return {"data": f(request, **kwargs), "doc_data": doc_data}
except Exception as exc:
- sentry_sdk.capture_exception(exc)
- return error_response(request, exc, doc_data)
-
- if handle_response:
- return response
- else:
- return make_api_response(request, response, doc_data)
+ exc.doc_data = doc_data
+ raise exc
return documented_view
diff --git a/swh/web/api/apiresponse.py b/swh/web/api/apiresponse.py
--- a/swh/web/api/apiresponse.py
+++ b/swh/web/api/apiresponse.py
@@ -7,7 +7,12 @@
import traceback
from typing import Any, Dict, Optional
+import sentry_sdk
+
+from django.http import HttpResponse
+from django.shortcuts import render
from django.utils.html import escape
+from rest_framework.exceptions import APIException
from rest_framework.request import Request
from rest_framework.response import Response
from rest_framework.utils.encoders import JSONEncoder
@@ -96,7 +101,7 @@
data: Dict[str, Any],
doc_data: Optional[Dict[str, Any]] = None,
options: Optional[Dict[str, Any]] = None,
-) -> Response:
+) -> HttpResponse:
"""Generates an API response based on the requested mimetype.
Args:
@@ -123,15 +128,9 @@
# get request status code
doc_data["status_code"] = options.get("status", 200)
- response_args = {
- "status": doc_data["status_code"],
- "headers": headers,
- "content_type": request.accepted_media_type,
- }
-
# when requesting HTML, typically when browsing the API through its
# documented views, we need to enrich the input data with documentation
- # related ones and inform DRF that we request HTML template rendering
+ # and render the apidoc HTML template
if request.accepted_media_type == "text/html":
doc_data["response_data"] = data
if data:
@@ -149,20 +148,24 @@
if not doc_data["noargs"]:
doc_data["endpoint_path"][-1]["path"] += "/doc/"
- response_args["data"] = doc_data
- response_args["template_name"] = "api/apidoc.html"
+ return render(
+ request, "api/apidoc.html", doc_data, status=doc_data["status_code"]
+ )
# otherwise simply return the raw data and let DRF picks
# the correct renderer (JSON or YAML)
else:
- response_args["data"] = data
-
- return Response(**response_args)
+ return Response(
+ data,
+ headers=headers,
+ content_type=request.accepted_media_type,
+ status=doc_data["status_code"],
+ )
def error_response(
- request: Request, error: Exception, doc_data: Dict[str, Any]
-) -> Response:
+ request: Request, exception: Exception, doc_data: Dict[str, Any]
+) -> HttpResponse:
"""Private function to create a custom error response.
Args:
@@ -172,23 +175,25 @@
"""
error_code = 500
- if isinstance(error, BadInputExc):
+ if isinstance(exception, BadInputExc):
error_code = 400
- elif isinstance(error, NotFoundExc):
+ elif isinstance(exception, NotFoundExc):
error_code = 404
- elif isinstance(error, ForbiddenExc):
+ elif isinstance(exception, ForbiddenExc):
error_code = 403
- elif isinstance(error, LargePayloadExc):
+ elif isinstance(exception, LargePayloadExc):
error_code = 413
- elif isinstance(error, StorageDBError):
+ elif isinstance(exception, StorageDBError):
error_code = 503
- elif isinstance(error, StorageAPIError):
+ elif isinstance(exception, StorageAPIError):
error_code = 503
+ elif isinstance(exception, APIException):
+ error_code = exception.status_code
error_opts = {"status": error_code}
error_data = {
- "exception": error.__class__.__name__,
- "reason": str(error),
+ "exception": exception.__class__.__name__,
+ "reason": str(exception),
}
if request.accepted_media_type == "text/html":
@@ -198,3 +203,13 @@
error_data["traceback"] = traceback.format_exc()
return make_api_response(request, error_data, doc_data, options=error_opts)
+
+
+def error_response_handler(
+ exc: Exception, context: Dict[str, Any]
+) -> Optional[HttpResponse]:
+ """Custom DRF exception handler used to generate API error responses.
+ """
+ sentry_sdk.capture_exception(exc)
+ doc_data = getattr(exc, "doc_data", None)
+ return error_response(context["request"], exc, doc_data)
diff --git a/swh/web/api/apiurls.py b/swh/web/api/apiurls.py
--- a/swh/web/api/apiurls.py
+++ b/swh/web/api/apiurls.py
@@ -4,11 +4,13 @@
# See top-level LICENSE file for more information
import functools
-from typing import Dict
+from typing import Dict, List, Optional
+from django.http import HttpResponse
from rest_framework.decorators import api_view
from swh.web.api import throttling
+from swh.web.api.apiresponse import make_api_response
from swh.web.common.urlsindex import UrlsIndex
@@ -26,11 +28,18 @@
scope = "api"
@classmethod
- def get_app_endpoints(cls):
+ def get_app_endpoints(cls) -> Dict[str, Dict[str, str]]:
return cls._apidoc_routes
@classmethod
- def add_doc_route(cls, route, docstring, noargs=False, api_version="1", **kwargs):
+ def add_doc_route(
+ cls,
+ route: str,
+ docstring: str,
+ noargs: bool = False,
+ api_version: str = "1",
+ **kwargs,
+ ) -> None:
"""
Add a route to the self-documenting API reference
"""
@@ -50,12 +59,12 @@
def api_route(
- url_pattern=None,
- view_name=None,
- methods=["GET", "HEAD", "OPTIONS"],
- throttle_scope="swh_api",
- api_version="1",
- checksum_args=None,
+ url_pattern: str,
+ view_name: Optional[str] = None,
+ methods: List[str] = ["GET", "HEAD", "OPTIONS"],
+ throttle_scope: str = "swh_api",
+ api_version: str = "1",
+ checksum_args: Optional[List[str]] = None,
):
"""
Decorator to ease the registration of an API endpoint
@@ -66,6 +75,9 @@
view_name: the name of the API view associated to the route used to
reverse the url
methods: array of HTTP methods supported by the API route
+ throttle_scope: Named scope for rate limiting
+ api_version: web API version
+ checksum_args: list of view argument names holding checksum values
"""
@@ -76,8 +88,18 @@
@api_view(methods)
@throttling.throttle_scope(throttle_scope)
@functools.wraps(f)
- def api_view_f(*args, **kwargs):
- return f(*args, **kwargs)
+ def api_view_f(request, **kwargs):
+ response = f(request, **kwargs)
+ doc_data = None
+ # check if response has been forwarded by api_doc decorator
+ if isinstance(response, dict) and "doc_data" in response:
+ doc_data = response["doc_data"]
+ response = response["data"]
+ # check if HTTP response needs to be created
+ if not isinstance(response, HttpResponse):
+ return make_api_response(request, data=response, doc_data=doc_data)
+ else:
+ return response
# small hacks for correctly generating API endpoints index doc
api_view_f.__name__ = f.__name__
diff --git a/swh/web/api/views/content.py b/swh/web/api/views/content.py
--- a/swh/web/api/views/content.py
+++ b/swh/web/api/views/content.py
@@ -186,7 +186,7 @@
"api-1-content-raw",
checksum_args=["q"],
)
-@api_doc("/content/raw/", handle_response=True)
+@api_doc("/content/raw/")
def api_content_raw(request, q):
"""
.. http:get:: /api/1/content/[(hash_type):](hash)/raw/
diff --git a/swh/web/api/views/origin_save.py b/swh/web/api/views/origin_save.py
--- a/swh/web/api/views/origin_save.py
+++ b/swh/web/api/views/origin_save.py
@@ -13,13 +13,13 @@
)
+@never_cache
@api_route(
r"/origin/save/(?P<visit_type>.+)/url/(?P<origin_url>.+)/",
"api-1-save-origin",
methods=["GET", "POST"],
throttle_scope="swh_save_origin",
)
-@never_cache
@api_doc("/origin/save/")
@format_docstring()
def api_save_origin(request, visit_type, origin_url):
diff --git a/swh/web/api/views/revision.py b/swh/web/api/views/revision.py
--- a/swh/web/api/views/revision.py
+++ b/swh/web/api/views/revision.py
@@ -117,7 +117,7 @@
"api-1-revision-raw-message",
checksum_args=["sha1_git"],
)
-@api_doc("/revision/raw/", tags=["hidden"], handle_response=True)
+@api_doc("/revision/raw/", tags=["hidden"])
def api_revision_raw_message(request, sha1_git):
"""Return the raw data of the message of revision identified by sha1_git
"""
diff --git a/swh/web/api/views/vault.py b/swh/web/api/views/vault.py
--- a/swh/web/api/views/vault.py
+++ b/swh/web/api/views/vault.py
@@ -41,6 +41,7 @@
)
+@never_cache
@api_route(
r"/vault/directory/(?P<dir_id>[0-9a-f]+)/",
"api-1-vault-cook-directory",
@@ -48,7 +49,6 @@
checksum_args=["dir_id"],
throttle_scope="swh_vault_cooking",
)
-@never_cache
@api_doc("/vault/directory/")
@format_docstring()
def api_vault_cook_directory(request, dir_id):
@@ -111,7 +111,7 @@
"api-1-vault-fetch-directory",
checksum_args=["dir_id"],
)
-@api_doc("/vault/directory/raw/", handle_response=True)
+@api_doc("/vault/directory/raw/")
def api_vault_fetch_directory(request, dir_id):
"""
.. http:get:: /api/1/vault/directory/(dir_id)/raw/
@@ -147,6 +147,7 @@
return response
+@never_cache
@api_route(
r"/vault/revision/(?P<rev_id>[0-9a-f]+)/gitfast/",
"api-1-vault-cook-revision_gitfast",
@@ -154,7 +155,6 @@
checksum_args=["rev_id"],
throttle_scope="swh_vault_cooking",
)
-@never_cache
@api_doc("/vault/revision/gitfast/")
@format_docstring()
def api_vault_cook_revision_gitfast(request, rev_id):
@@ -218,7 +218,7 @@
"api-1-vault-fetch-revision_gitfast",
checksum_args=["rev_id"],
)
-@api_doc("/vault/revision/gitfast/raw/", handle_response=True)
+@api_doc("/vault/revision/gitfast/raw/")
def api_vault_fetch_revision_gitfast(request, rev_id):
"""
.. http:get:: /api/1/vault/revision/(rev_id)/gitfast/raw/
@@ -259,7 +259,7 @@
"api-1-vault-revision_gitfast-raw",
checksum_args=["rev_id"],
)
-@api_doc("/vault/revision_gitfast/raw/", tags=["hidden"], handle_response=True)
+@api_doc("/vault/revision_gitfast/raw/", tags=["hidden"])
def _api_vault_revision_gitfast_raw(request, rev_id):
"""
The vault backend sends an email containing an invalid url to fetch a
diff --git a/swh/web/settings/common.py b/swh/web/settings/common.py
--- a/swh/web/settings/common.py
+++ b/swh/web/settings/common.py
@@ -167,6 +167,7 @@
"rest_framework.authentication.SessionAuthentication",
"swh.web.auth.backends.OIDCBearerTokenAuthentication",
],
+ "EXCEPTION_HANDLER": "swh.web.api.apiresponse.error_response_handler",
}
LOGGING = {
diff --git a/swh/web/tests/api/test_apiresponse.py b/swh/web/tests/api/test_apiresponse.py
--- a/swh/web/tests/api/test_apiresponse.py
+++ b/swh/web/tests/api/test_apiresponse.py
@@ -11,6 +11,8 @@
make_api_response,
transform,
)
+from swh.web.common.utils import reverse
+from swh.web.tests.django_asserts import assert_contains
def test_compute_link_header():
@@ -76,6 +78,7 @@
mock_filter.return_value = data
mock_shorten_path.return_value = "my_short_path"
+ mock_json.dumps.return_value = json.dumps(data)
accepted_response_formats = {
"html": "text/html",
@@ -83,34 +86,23 @@
"json": "application/json",
}
- for format in accepted_response_formats:
+ for resp_format in accepted_response_formats:
request = api_request_factory.get("/api/test/path/")
- mime_type = accepted_response_formats[format]
- setattr(request, "accepted_media_type", mime_type)
-
- if mime_type == "text/html":
-
- expected_data = {
- "response_data": json.dumps(data),
- "headers_data": {},
- "heading": "my_short_path",
- "status_code": 200,
- }
-
- mock_json.dumps.return_value = json.dumps(data)
- else:
- expected_data = data
+ content_type = accepted_response_formats[resp_format]
+ setattr(request, "accepted_media_type", content_type)
rv = make_api_response(request, data)
mock_filter.assert_called_with(request, data)
- assert rv.status_code == 200, rv.data
- assert rv.data == expected_data
- if mime_type == "text/html":
- assert rv.template_name == "api/apidoc.html"
+ if resp_format != "html":
+ assert rv.status_code == 200, rv.data
+ assert rv.data == data
+ else:
+ assert rv.status_code == 200, rv.content
+ assert_contains(rv, json.dumps(data))
def test_swh_filter_renderer_do_nothing(api_request_factory):
@@ -138,3 +130,11 @@
assert actual_data == {"a": "some-data"}
mock_ffk.assert_called_once_with(input_data, {"a", "c"})
+
+
+def test_error_response_handler(mocker, api_client):
+ mock_archive = mocker.patch("swh.web.api.views.stat.archive")
+ mock_archive.stat_counters.side_effect = Exception("Something went wrong")
+ url = reverse("api-1-stat-counters")
+ resp = api_client.get(url)
+ assert resp.status_code == 500
diff --git a/swh/web/tests/api/views/__init__.py b/swh/web/tests/api/views/__init__.py
--- a/swh/web/tests/api/views/__init__.py
+++ b/swh/web/tests/api/views/__init__.py
@@ -27,7 +27,7 @@
html_content_type = "text/html"
resp = api_client.get(url, HTTP_ACCEPT=html_content_type)
assert resp.status_code == status_code, resp.content
- assert resp["Content-Type"] == html_content_type
+ assert resp["Content-Type"].startswith(html_content_type)
# check YAML response
yaml_content_type = "application/yaml"