From e45d7face317c9ba0f47b58b51b707fc66af5699 Mon Sep 17 00:00:00 2001 From: Zeid Date: Fri, 17 Jan 2025 13:42:51 -0500 Subject: [PATCH] diff_warnings: port functionality (bug 1941342) --- src/lando/api/legacy/api/diff_warnings.py | 67 ----------- src/lando/api/tests/test_diff_warnings.py | 87 +++++++------- src/lando/api/views.py | 112 +++++++++++++++++- ...rename_error_breakdown_diffwarning_data.py | 18 +++ src/lando/main/models/revision.py | 8 +- src/lando/urls.py | 10 ++ 6 files changed, 190 insertions(+), 112 deletions(-) delete mode 100644 src/lando/api/legacy/api/diff_warnings.py create mode 100644 src/lando/main/migrations/0014_rename_error_breakdown_diffwarning_data.py 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..dc22c322 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,88 @@ 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", + "/api/diff_warnings/", 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", + "/api/diff_warnings/", json={"revision_id": 1, "diff_id": 1, "group": "LINT", "data": {}}, - headers=phab_header, + **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/", + { "revision_id": 1, "diff_id": 1, "group": "LINT", "data": diff_warning_data, }, - headers=phab_header, + **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/", + { "revision_id": 1, "diff_id": 1, "group": "LINT", "data": diff_warning_data, }, - headers=phab_header, + **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}/", + **phab_header, ) assert response.status_code == 200 @@ -92,59 +98,60 @@ 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/", + { "revision_id": 1, "diff_id": 1, "group": "LINT", "data": diff_warning_data, }, - headers=phab_header, + **phab_header, ) assert response.status_code == 201 response = client.post( - "/diff_warnings", - json={ + "/api/diff_warnings/", + { "revision_id": 1, "diff_id": 1, "group": "LINT", "data": diff_warning_data, }, - headers=phab_header, + **phab_header, ) assert response.status_code == 201 # Create another diff warning in a different group. response = client.post( - "/diff_warnings", - json={ + "/api/diff_warnings/", + { "revision_id": 1, "diff_id": 1, "group": "GENERAL", "data": diff_warning_data, }, - headers=phab_header, + **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 +159,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..9ae709e0 100644 --- a/src/lando/api/views.py +++ b/src/lando/api/views.py @@ -1 +1,111 @@ -# Create your views here. +from functools import wraps +from typing import Callable + +from django import forms +from django.http import JsonResponse +from django.views import View + +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 + + +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. + """ + + @classmethod + def _get_form(cls, method: str, data: dict): + """Helper method to return validate data.""" + + def data_validator(data): + if not data or "message" not in data: + raise forms.ValidationError( + "Provided data is missing the message value" + ) + + _forms = { + # Used to filter DiffWarning objects. + "GET": { + "revision_id": forms.IntegerField(), + "diff_id": forms.IntegerField(), + "group": forms.CharField(), + }, + # Used to create new DiffWarning objects. + "POST": { + "revision_id": forms.IntegerField(), + "diff_id": forms.IntegerField(), + "group": forms.CharField(), + "data": forms.JSONField(validators=[data_validator]), + }, + } + return type(f"{cls.__name__}_{method}", (forms.Form,), _forms[method])(data) + + @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. + """ + # TODO: validate whether revision/diff exist or not. + form = self._get_form(request.method, request.POST) + if form.is_valid(): + data = form.cleaned_data + warning = DiffWarning(**data) + warning.save() + return JsonResponse(warning.serialize(), status=201) + else: + 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.""" + form = self._get_form(request.method, 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 + ) + else: + 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..314cebef 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, "diff_id": self.diff_id, + "group": self.group, + "id": self.id, "revision_id": self.revision_id, - "status": self.status.value, - "group": self.group.value, + "status": self.status, "data": self.data, } diff --git a/src/lando/urls.py b/src/lando/urls.py index d6c63b26..ea402b7d 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 = [ @@ -33,6 +34,15 @@ path("manage_api_key/", user_settings.manage_api_key, name="user-settings"), ] +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"),