diff --git a/.github/workflows/django-tests.yml b/.github/workflows/django-tests.yml new file mode 100644 index 00000000..6bca8c20 --- /dev/null +++ b/.github/workflows/django-tests.yml @@ -0,0 +1,46 @@ +name: Django Tests + +# The SDK workflows run a handful of tests with -p no:django, so nothing ever +# exercised the Django side: views, schema, permissions. This runs that suite. +on: + push: + branches: [dev, main] + pull_request: + branches: [dev, main] + workflow_call: + +permissions: + contents: read + +jobs: + django-tests: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v4 + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.11" + cache: pip + + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install -r requirements.txt + # Not in requirements.txt, which covers the running service only. + pip install pytest pytest-django psycopg2-binary setuptools + + - name: Prepare environment + run: | + cp .env.example .env + mkdir -p logs + + # tests/object_types/charts and the dataset metadata tests were left + # behind by the chart consolidation and fail on main: see #189. + - name: Run tests + run: | + pytest tests/ \ + --ignore=tests/object_types/charts \ + --deselect tests/schema/test_dataset_schema.py::TestAddUpdateDatasetMetadata diff --git a/.github/workflows/pr-gate.yml b/.github/workflows/pr-gate.yml index 332189c5..3a804c10 100644 --- a/.github/workflows/pr-gate.yml +++ b/.github/workflows/pr-gate.yml @@ -17,6 +17,11 @@ concurrency: cancel-in-progress: true jobs: + # Unit tests for this repo's own code, alongside the browser/API suite. + django-tests: + name: Django Tests + uses: ./.github/workflows/django-tests.yml + full-suite: name: Full Suite (dev) uses: CivicDataLab/CivicDataSpace-test/.github/workflows/run-smoke.yml@CI diff --git a/api/views/auth.py b/api/views/auth.py index 98b10295..ff72a869 100644 --- a/api/views/auth.py +++ b/api/views/auth.py @@ -1,5 +1,5 @@ from rest_framework import status, views -from rest_framework.permissions import AllowAny +from rest_framework.permissions import AllowAny, IsAuthenticated from rest_framework.request import Request from rest_framework.response import Response from rest_framework_simplejwt.tokens import RefreshToken @@ -83,6 +83,11 @@ class UserInfoView(views.APIView): View for getting the current user's information. """ + # Without this the project-wide AllowAny default lets an anonymous request + # through, and reading .email off AnonymousUser raises a 500 instead of + # returning 401. + permission_classes = [IsAuthenticated] + def get(self, request: Request) -> Response: user = request.user return Response( diff --git a/api/views/paginated_elastic_view.py b/api/views/paginated_elastic_view.py index 7f892bb5..c8b77a88 100644 --- a/api/views/paginated_elastic_view.py +++ b/api/views/paginated_elastic_view.py @@ -4,6 +4,7 @@ from django.core.cache import cache from django.http import HttpRequest, HttpResponse from elasticsearch_dsl import Search +from elasticsearch_dsl.utils import AttrDict, AttrList from rest_framework.permissions import AllowAny from rest_framework.response import Response from rest_framework.serializers import Serializer @@ -11,6 +12,25 @@ from api.signals.dataset_signals import SEARCH_CACHE_VERSION_KEY +def as_plain_data(value: Any) -> Any: + """Convert Elasticsearch wrapper objects into plain Python containers. + + Serialized hits keep AttrList/AttrDict/InnerDoc values for nested fields. + Those classes are rebuilt per document type, so pickling one raises + "it's not the same object as elasticsearch_dsl.document.InnerDoc" and the + cache write fails, turning the whole response into a 500. + """ + if isinstance(value, AttrList): + return [as_plain_data(item) for item in value] + if isinstance(value, AttrDict): + return as_plain_data(value.to_dict()) + if isinstance(value, dict): + return {key: as_plain_data(item) for key, item in value.items()} + if isinstance(value, (list, tuple)): + return [as_plain_data(item) for item in value] + return value + + T = TypeVar("T") SearchType = TypeVar("SearchType", bound=Search) SerializerType = TypeVar("SerializerType", bound=Serializer) @@ -173,11 +193,13 @@ def get(self, request: HttpRequest) -> Response: for agg in is_individual_usecase_agg: aggregations["is_individual_usecase"][agg["key"]] = agg["doc_count"] - result: Dict[str, Any] = { - "results": serializer.data, - "total": response.hits.total.value, # type: ignore - "aggregations": aggregations, - } + result: Dict[str, Any] = as_plain_data( + { + "results": serializer.data, + "total": response.hits.total.value, # type: ignore + "aggregations": aggregations, + } + ) # Cache the result cache.set(cache_key, result, timeout=3600) # Cache for 1 hour diff --git a/tests/object_types/charts/test_grouped_bar_chart.py b/tests/object_types/charts/test_grouped_bar_chart.py index 87cdaa7c..555bf5c9 100644 --- a/tests/object_types/charts/test_grouped_bar_chart.py +++ b/tests/object_types/charts/test_grouped_bar_chart.py @@ -65,8 +65,7 @@ def test_vertical_grouped_bar_chart(self): } chart_details = MockResourceChartDetails( chart_type="BAR", - options={**options, "allow_multi_series": true}, - options=options, + options={**options, "allow_multi_series": True}, ) chart = UnifiedChart(chart_details, self.test_data) result = chart.create_chart() @@ -104,8 +103,7 @@ def test_styling_options(self): } chart_details = MockResourceChartDetails( chart_type="BAR", - options={**options, "allow_multi_series": true}, - options=options, + options={**options, "allow_multi_series": True}, ) chart = UnifiedChart(chart_details, self.test_data) result = chart.create_chart() @@ -136,8 +134,7 @@ def test_value_aggregation(self): } chart_details = MockResourceChartDetails( chart_type="BAR", - options={**options, "allow_multi_series": true}, - options=options, + options={**options, "allow_multi_series": True}, ) chart = UnifiedChart(chart_details, test_data) result = chart.create_chart() @@ -156,8 +153,7 @@ def test_time_based_grouped_bar_chart(self): } chart_details = MockResourceChartDetails( chart_type="BAR", - options={**options, "allow_multi_series": true}, - options=options, + options={**options, "allow_multi_series": True}, ) chart = UnifiedChart(chart_details, self.test_data) result = chart.create_chart() @@ -190,8 +186,7 @@ def test_value_mapping(self): } chart_details = MockResourceChartDetails( chart_type="BAR", - options={**options, "allow_multi_series": true}, - options=options, + options={**options, "allow_multi_series": True}, ) chart = UnifiedChart(chart_details, test_data) result = chart.create_chart() diff --git a/tests/test_search_cache_and_user_info.py b/tests/test_search_cache_and_user_info.py new file mode 100644 index 00000000..e689ebb9 --- /dev/null +++ b/tests/test_search_cache_and_user_info.py @@ -0,0 +1,67 @@ +"""Regression tests for the two 500s on /api/auth/user/info/ and /api/search/aimodel/.""" + +import pickle + +import pytest +from elasticsearch_dsl import InnerDoc +from elasticsearch_dsl.utils import AttrDict, AttrList +from rest_framework.test import APIRequestFactory + +from api.views.auth import UserInfoView +from api.views.paginated_elastic_view import as_plain_data + + +def nested_doc(**fields: object) -> InnerDoc: + """Build a nested doc the way elasticsearch_dsl does for a nested field. + + The class is rebuilt per document type rather than being the module-level + InnerDoc, which is exactly what pickle refuses to serialize. + """ + cls = type("InnerDoc", (InnerDoc,), {}) + cls.__module__ = "elasticsearch_dsl.document" + doc = cls() + for name, value in fields.items(): + setattr(doc, name, value) + return doc + + +@pytest.mark.django_db +def test_user_info_rejects_anonymous_request() -> None: + """Anonymous callers get 401, not a 500 from reading .email off AnonymousUser. + + The view is called directly: routing it through the test client hides the + bug, because the test settings drop the Keycloak middleware. + """ + request = APIRequestFactory().get("/api/auth/user/info/") + response = UserInfoView.as_view()(request) + assert response.status_code == 401 + + +def test_as_plain_data_makes_search_results_picklable() -> None: + """Nested hits must survive cache.set, which pickles the cached value.""" + result = { + "results": [ + { + "all_providers": AttrList([nested_doc(provider="GPT")]), + "name": AttrDict({"raw": "x"}), + } + ], + "total": 1, + } + + plain = as_plain_data(result) + assert pickle.loads(pickle.dumps(plain)) == plain + assert plain["results"][0]["all_providers"] == [{"provider": "GPT"}] + assert plain["results"][0]["name"] == {"raw": "x"} + assert not _holds_elastic_objects(plain) + + +def _holds_elastic_objects(value: object) -> bool: + """The cache pickles what it is given, so no wrapper may survive anywhere.""" + if isinstance(value, (AttrDict, AttrList, InnerDoc)): + return True + if isinstance(value, dict): + return any(_holds_elastic_objects(item) for item in value.values()) + if isinstance(value, (list, tuple)): + return any(_holds_elastic_objects(item) for item in value) + return False