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.+)/url/(?P.+)/", "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[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[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"