diff --git a/src/lando/api/legacy/api/diff_warnings.py b/src/lando/api/legacy/api/diff_warnings.py deleted file mode 100644 index 62b39f2c..00000000 --- a/src/lando/api/legacy/api/diff_warnings.py +++ /dev/null @@ -1,67 +0,0 @@ -""" -This module provides the API controllers for the `DiffWarning` model. - -These API endpoints can be used by clients (such as Lando UI, Code Review bot, etc.) to -get, create, or archive warnings. -""" - -import logging - -from lando.main.auth import require_phabricator_api_key -from lando.main.models.revision import DiffWarning, DiffWarningStatus -from lando.main.support import problem - -logger = logging.getLogger(__name__) - - -@require_phabricator_api_key(provide_client=False) -def post(data: dict): - """Create a new `DiffWarning` based on provided revision and diff IDs. - - Args: - data (dict): A dictionary containing data to store in the warning. `data` - should contain at least a `message` key that contains the message to - show in the warning. - - Returns: - dict: a dictionary representation of the object that was created. - """ - # TODO: validate whether revision/diff exist or not. - if "message" not in data["data"]: - return problem( - 400, - "Provided data is not in correct format", - "Missing required 'message' key in data", - type="https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/400", - ) - warning = DiffWarning(**data) - warning.save() - return warning.serialize(), 201 - - -@require_phabricator_api_key(provide_client=False) -def delete(pk: str): - """Archive a `DiffWarning` based on provided pk.""" - warning = DiffWarning.objects.get(pk=pk) - if not warning: - return problem( - 400, - "DiffWarning does not exist", - f"DiffWarning with primary key {pk} does not exist", - type="https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/400", - ) - warning.status = DiffWarningStatus.ARCHIVED - warning.save() - return warning.serialize(), 200 - - -@require_phabricator_api_key(provide_client=False) -def get(revision_id: str, diff_id: str, group: str): - """Return a list of active revision diff warnings, if any.""" - warnings = DiffWarning.objects.filter( - revision_id=revision_id, - diff_id=diff_id, - status=DiffWarningStatus.ACTIVE, - group=group, - ) - return [w.serialize() for w in warnings], 200 diff --git a/src/lando/api/tests/test_diff_warnings.py b/src/lando/api/tests/test_diff_warnings.py index c0f874c2..8f99e1aa 100644 --- a/src/lando/api/tests/test_diff_warnings.py +++ b/src/lando/api/tests/test_diff_warnings.py @@ -1,3 +1,5 @@ +import json + import pytest from lando.main.models.revision import ( @@ -6,84 +8,93 @@ DiffWarningStatus, ) -pytest.skip(allow_module_level=True) -# NOTE: this should be ported as an API endpoint. - @pytest.fixture def phab_header(phabdouble): user = phabdouble.user(username="test") - return {"X-Phabricator-API-Key": user["apiKey"]} + return {"HTTP_X_Phabricator_API_Key": user["apiKey"]} @pytest.fixture def diff_warning_data(): - return {"message": "this is a test warning"} + return json.dumps({"message": "this is a test warning"}) -def test_diff_warning_create_bad_request(db, client): +@pytest.mark.django_db(transaction=True) +def test_diff_warning_create_bad_request(client): """Ensure a request that is missing required data returns an error.""" response = client.post( - "/diff_warnings", - json={}, + "/api/diff_warnings/", + data={}, + content_type="application/json", ) assert response.status_code == 400 -def test_diff_warning_create_bad_request_no_message(db, client, phab_header): +@pytest.mark.django_db(transaction=True) +def test_diff_warning_create_bad_request_no_message(client, phab_header): """Ensure a request with incorrect data returns an error.""" response = client.post( - "/diff_warnings", - json={"revision_id": 1, "diff_id": 1, "group": "LINT", "data": {}}, - headers=phab_header, + "/api/diff_warnings/", + data={"revision_id": 1, "diff_id": 1, "group": "LINT", "data": {}}, + content_type="application/json", + **phab_header, ) assert response.status_code == 400 -def test_diff_warning_create(db, client, diff_warning_data, phab_header): +@pytest.mark.django_db(transaction=True) +def test_diff_warning_create(client, diff_warning_data, phab_header): """Ensure that a warning is created correctly according to provided parameters.""" response = client.post( - "/diff_warnings", - json={ + "/api/diff_warnings/", + data={ "revision_id": 1, "diff_id": 1, "group": "LINT", "data": diff_warning_data, }, - headers=phab_header, + content_type="application/json", + **phab_header, ) + assert response.status_code == 201 - assert "id" in response.json - pk = response.json["id"] + json_response = response.json() + assert "id" in json_response + + pk = json_response["id"] warning = DiffWarning.objects.get(pk=pk) assert warning.group == DiffWarningGroup.LINT assert warning.revision_id == 1 assert warning.diff_id == 1 assert warning.status == DiffWarningStatus.ACTIVE - assert warning.data == diff_warning_data + assert warning.data == json.loads(diff_warning_data) -def test_diff_warning_delete(db, client, diff_warning_data, phab_header): +@pytest.mark.django_db(transaction=True) +def test_diff_warning_delete(client, diff_warning_data, phab_header): """Ensure that a DELETE request will archive a warning.""" response = client.post( - "/diff_warnings", - json={ + "/api/diff_warnings/", + data={ "revision_id": 1, "diff_id": 1, "group": "LINT", "data": diff_warning_data, }, - headers=phab_header, + content_type="application/json", + **phab_header, ) assert response.status_code == 201 - pk = response.json["id"] + pk = response.json()["id"] warning = DiffWarning.objects.get(pk=pk) assert warning.status == DiffWarningStatus.ACTIVE response = client.delete( - f"/diff_warnings/{pk}", - headers=phab_header, + f"/api/diff_warnings/{pk}/", + content_type="application/json", + **phab_header, ) assert response.status_code == 200 @@ -92,59 +103,63 @@ def test_diff_warning_delete(db, client, diff_warning_data, phab_header): assert warning.status == DiffWarningStatus.ARCHIVED -def test_diff_warning_get(db, client, diff_warning_data, phab_header): +@pytest.mark.django_db(transaction=True, reset_sequences=True) +def test_diff_warning_get(client, diff_warning_data, phab_header): """Ensure that the API returns a properly serialized list of warnings.""" response = client.post( - "/diff_warnings", - json={ + "/api/diff_warnings/", + data={ "revision_id": 1, "diff_id": 1, "group": "LINT", "data": diff_warning_data, }, - headers=phab_header, + content_type="application/json", + **phab_header, ) assert response.status_code == 201 response = client.post( - "/diff_warnings", - json={ + "/api/diff_warnings/", + data={ "revision_id": 1, "diff_id": 1, "group": "LINT", "data": diff_warning_data, }, - headers=phab_header, + content_type="application/json", + **phab_header, ) assert response.status_code == 201 # Create another diff warning in a different group. response = client.post( - "/diff_warnings", - json={ + "/api/diff_warnings/", + data={ "revision_id": 1, "diff_id": 1, "group": "GENERAL", "data": diff_warning_data, }, - headers=phab_header, + content_type="application/json", + **phab_header, ) assert response.status_code == 201 response = client.get( - "/diff_warnings", - query_string={"revision_id": 1, "diff_id": 1, "group": "LINT"}, - headers=phab_header, + "/api/diff_warnings/", + query_params={"revision_id": 1, "diff_id": 1, "group": "LINT"}, + **phab_header, ) assert response.status_code == 200 - assert response.json == [ + assert response.json() == [ { "diff_id": 1, "group": "LINT", "id": 1, "revision_id": 1, "status": "ACTIVE", - "data": diff_warning_data, + "data": json.loads(diff_warning_data), }, { "diff_id": 1, @@ -152,6 +167,6 @@ def test_diff_warning_get(db, client, diff_warning_data, phab_header): "id": 2, "revision_id": 1, "status": "ACTIVE", - "data": diff_warning_data, + "data": json.loads(diff_warning_data), }, ] diff --git a/src/lando/api/views.py b/src/lando/api/views.py index 60f00ef0..762f3add 100644 --- a/src/lando/api/views.py +++ b/src/lando/api/views.py @@ -1 +1,106 @@ -# Create your views here. +import json +from functools import wraps +from typing import Callable + +from django import forms +from django.http import JsonResponse +from django.utils.decorators import method_decorator +from django.views import View +from django.views.decorators.csrf import csrf_exempt + +from lando.main.models.revision import DiffWarning, DiffWarningStatus +from lando.utils.phabricator import get_phabricator_client + + +def phabricator_api_key_required(func: callable) -> Callable: + """A simple wrapper that checks for a valid Phabricator API token.""" + + @wraps(func) + def _wrapper(self, request, *args, **kwargs): + HEADER = "X-Phabricator-API-Key" + if HEADER not in request.headers: + return JsonResponse({"error": f"{HEADER} missing."}, status=400) + + api_key = request.headers[HEADER] + client = get_phabricator_client(api_key=api_key) + has_valid_token = client.verify_api_token() + + if not has_valid_token: + return JsonResponse({}, 401) + + return func(self, request, *args, **kwargs) + + return _wrapper + + +@method_decorator(csrf_exempt, name="dispatch") +class LegacyDiffWarningView(View): + """ + This class provides the API controllers for the legacy `DiffWarning` model. + + These API endpoints can be used by clients (such as Code Review bot) to + get, create, or archive warnings. + """ + + @phabricator_api_key_required + def post(self, request): + """Create a new `DiffWarning` based on provided revision and diff IDs. + + Args: + data (dict): A dictionary containing data to store in the warning. `data` + should contain at least a `message` key that contains the message to + show in the warning. + + Returns: + dict: a dictionary representation of the object that was created. + """ + + class Form(forms.Form): + def data_validator(data): + if not data or "message" not in data: + raise forms.ValidationError( + "Provided data is missing the message value" + ) + + revision_id = forms.IntegerField() + diff_id = forms.IntegerField() + group = forms.CharField() + data = forms.JSONField(validators=[data_validator]) + + # TODO: validate whether revision/diff exist or not. + form = Form(json.loads(request.body)) + if form.is_valid(): + data = form.cleaned_data + warning = DiffWarning.objects.create(**data) + return JsonResponse(warning.serialize(), status=201) + + return JsonResponse({"errors": dict(form.errors)}, status=400) + + @phabricator_api_key_required + def delete(self, request, diff_warning_id): + """Archive a `DiffWarning` based on provided pk.""" + warning = DiffWarning.objects.get(pk=diff_warning_id) + if not warning: + return JsonResponse({}, status=404) + + warning.status = DiffWarningStatus.ARCHIVED + warning.save() + return JsonResponse(warning.serialize(), status=200) + + @phabricator_api_key_required + def get(self, request, **kwargs): + """Return a list of active revision diff warnings, if any.""" + + class Form(forms.Form): + revision_id = forms.IntegerField() + diff_id = forms.IntegerField() + group = forms.CharField() + + form = Form(request.GET) + if form.is_valid(): + warnings = DiffWarning.objects.filter(**form.cleaned_data).all() + return JsonResponse( + [warning.serialize() for warning in warnings], status=200, safe=False + ) + + return JsonResponse({"errors": dict(form.errors)}, status=400) diff --git a/src/lando/main/migrations/0014_rename_error_breakdown_diffwarning_data.py b/src/lando/main/migrations/0014_rename_error_breakdown_diffwarning_data.py new file mode 100644 index 00000000..1c0cb080 --- /dev/null +++ b/src/lando/main/migrations/0014_rename_error_breakdown_diffwarning_data.py @@ -0,0 +1,18 @@ +# Generated by Django 5.1.4 on 2025-01-17 17:39 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("main", "0013_alter_repo_scm_type_alter_worker_scm"), + ] + + operations = [ + migrations.RenameField( + model_name="diffwarning", + old_name="error_breakdown", + new_name="data", + ), + ] diff --git a/src/lando/main/models/revision.py b/src/lando/main/models/revision.py index b0adafc9..677d36fb 100644 --- a/src/lando/main/models/revision.py +++ b/src/lando/main/models/revision.py @@ -124,7 +124,7 @@ class DiffWarning(BaseModel): # An arbitary dictionary of data that will be determined by the client. # It is up to the UI to interpret this data and show it to the user. - error_breakdown = models.JSONField(null=False, blank=True, default=dict) + data = models.JSONField(null=False, blank=True, default=dict) # Whether the warning is active or archived. This is used in filters. status = models.CharField( @@ -146,10 +146,10 @@ class DiffWarning(BaseModel): def serialize(self): """Return a JSON serializable dictionary.""" return { - "id": self.id, + "data": self.data, "diff_id": self.diff_id, + "group": self.group, + "id": self.id, "revision_id": self.revision_id, - "status": self.status.value, - "group": self.group.value, - "data": self.data, + "status": self.status, } diff --git a/src/lando/urls.py b/src/lando/urls.py index c3ec932d..833d8238 100644 --- a/src/lando/urls.py +++ b/src/lando/urls.py @@ -19,6 +19,7 @@ from django.urls import include, path from lando.api.legacy.api import landing_jobs +from lando.api.views import LegacyDiffWarningView from lando.ui.legacy import pages, revisions, user_settings urlpatterns = [ @@ -34,6 +35,15 @@ path("uplift/", revisions.Uplift.as_view(), name="uplift-page"), ] +urlpatterns += [ + path("api/diff_warnings/", LegacyDiffWarningView.as_view(), name="diff-warnings"), + path( + "api/diff_warnings//", + LegacyDiffWarningView.as_view(), + name="diff-warnings", + ), +] + # "API" endpoints ported from legacy API app. urlpatterns += [ path("landing_jobs//", landing_jobs.put, name="landing-jobs"),