diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 63da9f9..86575ca 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -196,7 +196,6 @@ suite automatically before every commit. The following checks are enforced: - ✅ `ruff` for linting and formatting (with auto-fix), running from `uv.lock` so it is the same version as CI -- ✅ `compileall` to catch syntax errors in `tests/`, which ruff does not lint - ✅ `uv-lock` to check that `uv.lock` matches `pyproject.toml` - ✅ `mdformat` to format Markdown files (with GFM support) - ✅ `end-of-file-fixer`, `trailing-whitespace`, `check-yaml`, `check-toml`, `check-added-large-files` for formatting diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 967980e..f1fea20 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -28,7 +28,7 @@ repos: hooks: # Ruff runs from uv.lock, so it is the same version CI runs. --frozen leaves a stale lock for # the uv-lock hook to report instead of rewriting it, and --force-exclude applies ruff's - # exclude list (tests/) to the file names pre-commit passes. + # exclude list (AGENTS.md, .agents/) to the file names pre-commit passes. - id: ruff-check name: ruff check entry: uv run --frozen ruff check --force-exclude --fix --exit-non-zero-on-fix @@ -43,13 +43,6 @@ repos: types_or: [ python, pyi ] require_serial: true - # Catches syntax errors in tests/, which ruff does not lint - - id: compileall - name: Compile Python sources - entry: uv run --frozen python -m compileall -q - language: system - types: [ python ] - - id: uv-lock name: Check uv.lock matches pyproject.toml entry: uv lock --check diff --git a/README.md b/README.md index de187e5..7012d4b 100644 --- a/README.md +++ b/README.md @@ -172,16 +172,16 @@ object, with the user's profile information if requested. #### Request Parameters -| **Parameter** | **Optional** | **Type** | **Default** | **Description** | -| ------------- | ------------ | ----------- | ----------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `username` | No | `str` | | The user's SRN, PRN, email address, or phone number. Maximum length is **96 characters**. | -| `password` | No | `str` | | The user's password, up to **128 characters**, counted exactly as typed. It is sent only to PESU Academy, and never stored or logged. | -| `profile` | Yes | `boolean` | `False` | Whether to fetch profile information. This makes a second call to PESU Academy, so it takes longer | -| `fields` | Yes | `list[str]` | `None` | Which [`ProfileObject`](#profileobject) fields to return. Only used when `profile` is `true`. If not provided, all fields are returned. Fields come back in the table's order, whatever order they are asked for in | +| **Parameter** | **Optional** | **Type** | **Default** | **Description** | +| ------------- | ------------ | ----------- | ----------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `username` | No | `str` | | The user's SRN, PRN, email address, or phone number. Maximum length is **96 characters**. | +| `password` | No | `str` | | The user's password, up to **128 characters**, counted exactly as typed. It is sent only to PESU Academy, and never stored or logged. | +| `profile` | Yes | `boolean` | `False` | Whether to fetch profile information. This makes a second call to PESU Academy, so it takes longer | +| `fields` | Yes | `list[str]` | `None` | Which [`ProfileObject`](#profileobject) fields to return. A list can only be given when `profile` is `true`; otherwise the request is a `400`. If not provided or `null`, all fields are returned. Repeated names are ignored. Fields come back in the table's order, whatever order they are asked for in | The request body is validated strictly. A missing or empty `username` or `password`, one that is not valid text, a `username` with a line break or another invisible character, a value of the wrong type (such as -the string `"true"` for `profile`), an unknown key, an empty `fields` list, or an unknown field name is rejected with a +the string `"true"` for `profile`), an unknown key, an empty `fields` list, `fields` without `profile: true`, or an unknown field name is rejected with a `400`. So is a body that cannot be read at all, such as JSON that is not valid UTF-8, with the fixed message `Could not parse request body.` @@ -368,7 +368,7 @@ A few definitions that are easy to assume wrongly: | ---------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `authentication_requests_total{profile}` | Authentication requests, split by whether profile data was asked for | | `authentication_results_total{result}` | Attempts by outcome: `success` or `failure`. Deliberately only those two; `errors_total` already names the exception class, and recording the reason here too would put one fact in two places. This family exists for the login **success rate**, where success and failure share a denominator | -| `profile_field_filtering_total{enabled}` | Profile fetches, split by whether the caller narrowed the returned fields. Recorded where the branch is taken, so a caller passing exactly the default list counts as `false` | +| `profile_field_filtering_total{enabled}` | Profile fetches, split by whether the caller narrowed the returned fields. Recorded where the branch is taken, so a caller passing exactly the default list, in any order, counts as `false` | | `profile_parse_errors_total{reason}` | Profile response problems by what broke: `response_structure` (the response could not be parsed, a `422`), `unknown_campus_code` (a campus name with no known campus code), `missing_field` (a field PESU Academy did not send at all) and `unexpected_value` (a field sent in an unexpected shape). The last two are counted once per field, returned to the caller as `null`, and named in the log. `response_structure`, `missing_field` and `unexpected_value` mean PESU Academy's API changed; `unknown_campus_code` usually means a new campus, or a new way of writing a campus's name. A field PESU Academy sends with no value is not counted: that is a student with no value | **Upstream (PESU Academy)** diff --git a/app/docs/authenticate.py b/app/docs/authenticate.py index fd48eec..078b6bd 100644 --- a/app/docs/authenticate.py +++ b/app/docs/authenticate.py @@ -156,8 +156,9 @@ "description": ( "Bad Request - The request body failed validation: a body that is not valid JSON, a missing, empty, " "over-long or invalid username or password, a value of the wrong type, an unknown key, an unknown " - "profile field, or an empty fields list. The message names each field that failed and why. A body " - "that cannot be read at all, such as JSON that is not valid UTF-8, gets a fixed message instead." + "profile field, an empty fields list, or fields without profile set to true. The message names each " + "field that failed and why. A body that cannot be read at all, such as JSON that is not valid UTF-8, " + "gets a fixed message instead." ), "headers": CACHE_CONTROL_HEADER, "model": ResponseModel, @@ -225,6 +226,17 @@ "timestamp": "2024-07-28T22:30:10.103368+05:30", }, }, + "fields_without_profile": { + "summary": "Fields without profile set to true", + "value": { + "status": False, + "message": ( + "Could not validate request data - body.fields: Value error, Fields can only be " + "requested when profile is true." + ), + "timestamp": "2024-07-28T22:30:10.103368+05:30", + }, + }, "username_too_long": { "summary": "Username too long", "value": { diff --git a/app/models/request.py b/app/models/request.py index 3ef6bfe..f278ee4 100644 --- a/app/models/request.py +++ b/app/models/request.py @@ -73,8 +73,9 @@ class RequestModel(BaseModel): title="Profile Fields", description=( "Which profile fields to return, from those listed in `ProfileModel`. Every field is returned when " - "this is omitted. Only used when profile is `true`. Fields come back in `ProfileModel`'s order, " - "whatever order they are asked for in, and a name that is not in `ProfileModel` is rejected." + "this is omitted or `null`. A list can only be given when `profile` is `true`; otherwise the request is " + "rejected with a 400. Repeated names are ignored. Fields come back in `ProfileModel`'s order, whatever " + "order they are asked for in, and a name that is not in `ProfileModel` is rejected." ), json_schema_extra={"example": ["name", "email", "campus", "branch", "semester", "firstName", "mobile"]}, ) @@ -100,13 +101,18 @@ def validate_password(cls, v: str) -> str: raise ValueError("Password cannot be empty.") return v - @field_validator("fields") + @field_validator("fields", mode="after") @classmethod - def validate_fields(cls, v: list[str] | None) -> list[str] | None: - """Validate that fields is either None or a non-empty list containing only allowed field names.""" - if v is not None and not v: + def validate_fields(cls, v: list[str] | None, info: ValidationInfo) -> list[str] | None: + """Validate fields: a non-empty list, only with profile set to true, with duplicates removed.""" + if v is None: + return v + if not v: raise ValueError("Fields must be a non-empty list or None.") - return v + # profile is declared before fields, so it is in info.data unless it failed its own validation + if info.data.get("profile") is False: + raise ValueError("Fields can only be requested when profile is true.") + return list(dict.fromkeys(v)) @field_validator("username", "password", mode="before") @classmethod diff --git a/app/pesu.py b/app/pesu.py index 6fccdd9..af450aa 100644 --- a/app/pesu.py +++ b/app/pesu.py @@ -453,8 +453,8 @@ async def authenticate( """ # Default fields to fetch if fields is not provided fields = self.DEFAULT_FIELDS if fields is None else fields - # Check if fields is not the default fields and enable field filtering - field_filtering = fields != self.DEFAULT_FIELDS + # Field filtering is on only if the requested set differs from the default set; order and repeats don't count. + field_filtering = set(fields) != set(self.DEFAULT_FIELDS) logging.info( f"Connecting to PESU Academy with user={username}, profile={profile}, fields={fields} ...", diff --git a/pyproject.toml b/pyproject.toml index 09b5114..003d298 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "pesu-auth" -version = "5.4.0" +version = "5.5.0" description = "A simple API to authenticate PESU credentials using PESU Academy." readme = "README.md" requires-python = ">=3.14" @@ -76,6 +76,10 @@ select = [ "RET", # Return type rules ] +[tool.ruff.lint.per-file-ignores] +# A test's name says what it checks, and pytest fixture arguments need no annotations +"tests/**" = ["ANN", "D1"] + [tool.ruff.lint.pydocstyle] convention = "google" @@ -86,7 +90,6 @@ runtime-evaluated-base-classes = ["pydantic.BaseModel"] line-length = 120 target-version = "py314" exclude = [ - "tests/", # Agent instructions and skills from the pesu-dev/skills submodule, which lints them itself. # A local `ruff check .` before the submodule is initialized would otherwise follow dangling links. "AGENTS.md", diff --git a/tests/integration/test_app_integration.py b/tests/integration/test_app_integration.py index 8a448ab..964d6b5 100644 --- a/tests/integration/test_app_integration.py +++ b/tests/integration/test_app_integration.py @@ -311,7 +311,7 @@ def test_integration_health_check(client): response = client.get("/health") assert response.status_code == 200 data = response.json() - assert data["status"] == True + assert data["status"] is True assert data["message"] == "ok" @@ -448,9 +448,11 @@ def test_integration_authenticate_snake_case_fields_rejected(client, field): assert "body.fields.0" in data["message"] -@pytest.mark.parametrize("field", ["bloodGroup", "photo", "profilePicture", "programShortCode", "phone", "institute", "rollNumber"]) +@pytest.mark.parametrize( + "field", ["bloodGroup", "photo", "profilePicture", "programShortCode", "phone", "institute", "rollNumber"] +) def test_integration_authenticate_fields_the_api_does_not_return_rejected(client, field): - """PESU sends some of these and this API returned the others before 5.0.0; none is returned now, so asking is a 400.""" + """PESU sends some of these and this API returned the others before 5.0.0; asking for any of them now is a 400.""" payload = {"username": "username", "password": "password", "profile": True, "fields": [field]} response = client.post("/authenticate", json=payload) @@ -603,3 +605,23 @@ async def fake_crash(*args, **kwargs): assert response.status_code == 500 assert_security_headers(response) assert response.headers["Cache-Control"] == "no-store" + + +def test_integration_profile_key_omitted(client): + """Profile key omitted but fields are still included.""" + payload = {"username": "username", "password": "password", "fields": ["name", "email", "srn", "prn"]} + + response = client.post("/authenticate", json=payload) + data = response.json() + assert response.status_code == 400 + assert "Fields can only be requested when profile is true" in data["message"] + + +def test_integration_profile_is_false(client): + """Profile set to false.""" + payload = {"username": "username", "password": "password", "profile": False, "fields": ["name", "email", "srn"]} + + response = client.post("/authenticate", json=payload) + data = response.json() + assert response.status_code == 400 + assert "Fields can only be requested when profile is true" in data["message"] diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 6679515..f6c6cc6 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -39,7 +39,9 @@ AUTHENTICATION_FAILURES = { "wrong_credentials": ( LOGIN_URL, - lambda request: httpx2.Response(401, json={"statusCode": 401, "statusDescription": "Invalid Login Credentials"}), + lambda request: httpx2.Response( + 401, json={"statusCode": 401, "statusDescription": "Invalid Login Credentials"} + ), 401, ), "login_unavailable": (LOGIN_URL, lambda request: httpx2.Response(503), 502), @@ -73,7 +75,15 @@ # Every route that takes no input, with the status a GET of it answers: the docs pages, which are not in # the OpenAPI schema, and the two that are -PAGES = {"/": 200, "/docs": 200, "/docs/oauth2-redirect": 200, "/redoc": 200, "/openapi.json": 200, "/health": 200, "/readme": 308} +PAGES = { + "/": 200, + "/docs": 200, + "/docs/oauth2-redirect": 200, + "/redoc": 200, + "/openapi.json": 200, + "/health": 200, + "/readme": 308, +} # Shaped like the real responses (key names and types taken from the live API, values invented). # The personal fields this service must never keep or log -- photo, parents, address -- are @@ -154,9 +164,24 @@ }, # Not read: there for realism, and to prove a profile does not depend on it "STUDENT_SEMESTERS": [ - {"studentId": "00000000-0000-0000-0000-000000000000", "studentRollNo": 12, "className": "Sem-3", "batchClassOrder": 2026071499}, - {"studentId": "00000000-0000-0000-0000-000000000000", "studentRollNo": 27, "className": "Sem-4", "batchClassOrder": 2027010199}, - {"studentId": "00000000-0000-0000-0000-000000000000", "studentRollNo": 9, "className": "Sem-2", "batchClassOrder": 2026010199}, + { + "studentId": "00000000-0000-0000-0000-000000000000", + "studentRollNo": 12, + "className": "Sem-3", + "batchClassOrder": 2026071499, + }, + { + "studentId": "00000000-0000-0000-0000-000000000000", + "studentRollNo": 27, + "className": "Sem-4", + "batchClassOrder": 2027010199, + }, + { + "studentId": "00000000-0000-0000-0000-000000000000", + "studentRollNo": 9, + "className": "Sem-2", + "batchClassOrder": 2026010199, + }, ], "STUDENT_CGPA_DETAILS": [{"USN": "PES2UG25CS001", "CGPA": "CGPASECRET"}], } diff --git a/tests/unit/test_app_unit.py b/tests/unit/test_app_unit.py index 45e5f5c..235959b 100644 --- a/tests/unit/test_app_unit.py +++ b/tests/unit/test_app_unit.py @@ -1,13 +1,11 @@ +import logging from unittest.mock import MagicMock, patch import pytest from fastapi.testclient import TestClient -from app.exceptions.authentication import AuthenticationError, UpstreamError - from app.app import _build_arg_parser, app, main - -import logging +from app.exceptions.authentication import AuthenticationError, UpstreamError @pytest.fixture @@ -143,14 +141,16 @@ def test_main_function_debug_mode(mock_run, mock_logging, mock_parse_args): ) mock_run.assert_called_once_with("app.app:app", host="127.0.0.1", port=8000, reload=True) + def test_parser_default_port(monkeypatch): - monkeypatch.delenv("PORT",raising=False) + monkeypatch.delenv("PORT", raising=False) parser = _build_arg_parser() args = parser.parse_args([]) assert args.port == 5000 + def test_parser_port_from_env(monkeypatch): monkeypatch.setenv("PORT", "8080") @@ -159,6 +159,7 @@ def test_parser_port_from_env(monkeypatch): assert args.port == 8080 + def test_parser_cli_port_overrides_env(monkeypatch): monkeypatch.setenv("PORT", "8080") @@ -167,6 +168,7 @@ def test_parser_cli_port_overrides_env(monkeypatch): assert args.port == 9000 + def test_parser_invalid_port(monkeypatch): monkeypatch.setenv("PORT", "abc") @@ -175,6 +177,7 @@ def test_parser_invalid_port(monkeypatch): with pytest.raises(SystemExit): parser.parse_args([]) + def test_client_error_is_logged_without_a_traceback(client, caplog): """A 4xx is an expected outcome, so it must be logged at WARNING with no stack trace.""" with patch("app.app.pesu_academy.authenticate") as mock_authenticate: @@ -303,7 +306,12 @@ def test_an_unknown_route_is_a_404_in_the_standard_shape(client, monkeypatch, ca @pytest.mark.parametrize( "method, path, allow", - [("POST", "/health", "GET, HEAD"), ("DELETE", "/metrics", "GET, HEAD"), ("GET", "/authenticate", "POST"), ("PUT", "/docs", "GET, HEAD")], + [ + ("POST", "/health", "GET, HEAD"), + ("DELETE", "/metrics", "GET, HEAD"), + ("GET", "/authenticate", "POST"), + ("PUT", "/docs", "GET, HEAD"), + ], ) def test_an_unsupported_method_is_a_405_in_the_standard_shape(client, monkeypatch, method, path, allow): from app.metrics.collector import ERRORS_BY_TYPE, MetricsCollector diff --git a/tests/unit/test_authenticate_flow.py b/tests/unit/test_authenticate_flow.py index 998e8db..10cf17a 100644 --- a/tests/unit/test_authenticate_flow.py +++ b/tests/unit/test_authenticate_flow.py @@ -118,15 +118,16 @@ def test_requested_fields_only_and_null_when_empty(client, pesu_up, profile_payl "branchShortCode": "CSE", "dateOfBirth": "2005-01-01", } - # On the wire, in the documented order + # Ensure fields come back in the table's order, regardless of the requested order assert list(profile) == [field for field in full_profile if field in fields] -def test_fields_without_profile_return_no_profile(client, pesu_up): - body = _authenticate(client, profile=False, fields=["name", "gender", "mobile"]).json() +def test_fields_without_profile_are_rejected(client, pesu_up): + response = _authenticate(client, profile=False, fields=["name", "gender", "mobile"]) - assert "profile" not in body - assert len(pesu_up.requests) == 1 + assert response.status_code == 400 + assert "Fields can only be requested when profile is true." in response.json()["message"] + assert not pesu_up.requests def test_the_username_is_sent_without_surrounding_whitespace(client, pesu_up): diff --git a/tests/unit/test_check_version_bump.py b/tests/unit/test_check_version_bump.py index 61d7612..4fd1602 100644 --- a/tests/unit/test_check_version_bump.py +++ b/tests/unit/test_check_version_bump.py @@ -11,7 +11,9 @@ def _pyproject(tmp_path, name, version): def _lock(tmp_path, version): path = tmp_path / "uv.lock" - path.write_text(f'version = 1\n\n[[package]]\nname = "pesu-auth"\nversion = "{version}"\nsource = {{ editable = "." }}\n') + path.write_text( + f'version = 1\n\n[[package]]\nname = "pesu-auth"\nversion = "{version}"\nsource = {{ editable = "." }}\n' + ) return path diff --git a/tests/unit/test_metrics_endpoints.py b/tests/unit/test_metrics_endpoints.py index addbad2..9570f07 100644 --- a/tests/unit/test_metrics_endpoints.py +++ b/tests/unit/test_metrics_endpoints.py @@ -129,8 +129,10 @@ def test_a_validation_error_records_its_type(client): def test_an_unhandled_exception_records_a_500(client): - """ServerErrorMiddleware sits above the middleware, so this path cannot be verified by reading - the code -- only by driving a real unhandled exception through the whole stack.""" + """A real unhandled exception, driven through the whole stack, is recorded as a 500. + + ServerErrorMiddleware sits above the middleware, so this path cannot be verified by reading the code. + """ assert client.get("/raiseUnhandledForMetrics").status_code == 500 body = client.get("/metrics?fmt=json").json() assert body["responsesByStatus"]["500"] == 1 @@ -263,8 +265,14 @@ def test_failures_are_attributed_to_client_or_server(mock_authenticate, client): assert client.get("/metrics?fmt=json").json()["failuresByFault"] == {"client": 1, "server": 1} +def test_fields_without_profile_are_recorded_against_fields(client): + """The error is about fields, so it is counted there, not against the whole body.""" + client.post("/authenticate", json={"username": "u", "password": "p", "fields": ["name"]}) + assert client.get("/metrics?fmt=json").json()["validationErrorsByField"] == {"fields": 1} + + def test_in_flight_accounts_for_the_gap_in_the_totals(client): - """total exceeds success + failed only by what is still being served -- here, this request.""" + """Total exceeds success + failed only by what is still being served -- here, this request.""" body = client.get("/metrics?fmt=json").json() assert body["requestsInFlight"] == 1 assert body["requests"]["total"] == body["requests"]["success"] + body["requests"]["failed"] + 1 diff --git a/tests/unit/test_metrics_instrumentation.py b/tests/unit/test_metrics_instrumentation.py index b310604..81199f4 100644 --- a/tests/unit/test_metrics_instrumentation.py +++ b/tests/unit/test_metrics_instrumentation.py @@ -131,7 +131,7 @@ async def test_closing_a_client_is_recorded(collector): @pytest.mark.asyncio async def test_a_client_that_refuses_to_close_is_recorded(collector, caplog): - """created minus closed is the leak indicator, so a failed close cannot be counted as a close.""" + """Created minus closed is the leak indicator, so a failed close cannot be counted as a close.""" client = AsyncMock() client.aclose.side_effect = RuntimeError("refused") diff --git a/tests/unit/test_metrics_middleware.py b/tests/unit/test_metrics_middleware.py index c5b574b..a657c6b 100644 --- a/tests/unit/test_metrics_middleware.py +++ b/tests/unit/test_metrics_middleware.py @@ -105,8 +105,11 @@ async def test_a_redirect_is_not_counted_as_a_failure(collector): @pytest.mark.asyncio async def test_a_raised_exception_is_counted_and_re_raised(collector): - """ServerErrorMiddleware renders the 500 above us, so we never see that response -- we must - record the status ourselves or requests_total stops matching the sum of responses_total.""" + """A raised exception is recorded as a 500 and re-raised. + + ServerErrorMiddleware renders the 500 above us, so we never see that response -- we must record the status + ourselves or requests_total stops matching the sum of responses_total. + """ with pytest.raises(RuntimeError, match="boom"): await record_request_metrics(collector, FakeRequest(scope()), raising(RuntimeError("boom"))) snapshot = collector.snapshot() @@ -126,8 +129,11 @@ async def test_an_exception_does_not_record_an_error_type(collector): @pytest.mark.asyncio async def test_cancellation_is_not_recorded(collector): - """A client disconnect surfaces as cancellation. Swallowing it to record would be worse than - the small undercount, so it propagates untouched and total exceeds success + failed.""" + """A client disconnect surfaces as cancellation, which is not recorded. + + Swallowing it to record would be worse than the small undercount, so it propagates untouched and total + exceeds success + failed. + """ with pytest.raises(asyncio.CancelledError): await record_request_metrics(collector, FakeRequest(scope()), raising(asyncio.CancelledError())) snapshot = collector.snapshot() diff --git a/tests/unit/test_openapi_contract.py b/tests/unit/test_openapi_contract.py index df0a9fe..27cb3bd 100644 --- a/tests/unit/test_openapi_contract.py +++ b/tests/unit/test_openapi_contract.py @@ -14,14 +14,13 @@ import httpx2 import pytest import schemathesis +from fastapi.testclient import TestClient from hypothesis import HealthCheck, assume, given, settings from hypothesis import strategies as st from schemathesis.checks import not_a_server_error from schemathesis.core.failures import FailureGroup from schemathesis.python.asgi import ASGIClient, shutdown_lifespans -from fastapi.testclient import TestClient - from app.app import app from app.metrics.collector import MetricsCollector from app.models.upstream import StudentInfo, StudentPhoto @@ -29,7 +28,9 @@ # Anything PESU Academy could put in a profile field instead of the value expected there JSON_SCALARS = st.none() | st.booleans() | st.integers() | st.floats(allow_nan=False, allow_infinity=False) | st.text() -ANY_JSON_VALUE = JSON_SCALARS | st.lists(JSON_SCALARS, max_size=2) | st.dictionaries(st.text(max_size=5), JSON_SCALARS, max_size=2) +ANY_JSON_VALUE = ( + JSON_SCALARS | st.lists(JSON_SCALARS, max_size=2) | st.dictionaries(st.text(max_size=5), JSON_SCALARS, max_size=2) +) # Stand for a field holding the value PESU Academy usually sends, and for one it leaves out altogether KEEP, ABSENT = object(), object() # For each block the profile is built from, what each field it reads holds in a generated response @@ -117,7 +118,9 @@ def test_every_generated_authentication_request_gets_a_documented_response( @given(data=st.data()) @settings(max_examples=1000, database=None, deadline=None, **UNSEEDED) -def test_every_generated_profile_gets_a_documented_response(data, api_schema, wire, profile_payload, conformance_checks): +def test_every_generated_profile_gets_a_documented_response( + data, api_schema, wire, profile_payload, conformance_checks +): """Generated profile responses from PESU Academy, each asked for with a generated choice of fields.""" profile_response = _profile_response(data, profile_payload) wire.routes[DISPATCHER_URL] = lambda request: httpx2.Response(200, json=profile_response) @@ -240,7 +243,9 @@ def test_an_unreadable_body_gets_a_documented_response(api_schema, conformance_c @pytest.mark.parametrize("header", [None, "Bearer wrong-token", "Basic dXNlcg==", "Bearer ü"]) -def test_a_rejected_metrics_token_gets_a_documented_response(api_schema, conformance_checks, metrics_token, monkeypatch, header): +def test_a_rejected_metrics_token_gets_a_documented_response( + api_schema, conformance_checks, metrics_token, monkeypatch, header +): monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", metrics_token) headers = {} if header is None else {"Authorization": header} case = api_schema["/metrics"]["GET"].Case(query={"fmt": "json"}, headers=headers) @@ -251,7 +256,9 @@ def test_a_rejected_metrics_token_gets_a_documented_response(api_schema, conform @pytest.mark.parametrize("fmt", ["json", "prometheus"]) -def test_an_accepted_metrics_token_gets_a_documented_response(api_schema, conformance_checks, metrics_token, monkeypatch, fmt): +def test_an_accepted_metrics_token_gets_a_documented_response( + api_schema, conformance_checks, metrics_token, monkeypatch, fmt +): monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", metrics_token) case = api_schema["/metrics"]["GET"].Case(query={"fmt": fmt}, headers={"Authorization": f"Bearer {metrics_token}"}) diff --git a/tests/unit/test_openapi_docs.py b/tests/unit/test_openapi_docs.py index 4bbd041..e1e37f4 100644 --- a/tests/unit/test_openapi_docs.py +++ b/tests/unit/test_openapi_docs.py @@ -18,7 +18,7 @@ ProfileParseError, UpstreamError, ) -from app.models import MetricsModel, ProfileModel, RequestModel, ResponseModel +from app.models import ProfileModel, RequestModel, ResponseModel from app.pesu import PESUAcademy diff --git a/tests/unit/test_pesu.py b/tests/unit/test_pesu.py index 33096df..f4eb722 100644 --- a/tests/unit/test_pesu.py +++ b/tests/unit/test_pesu.py @@ -14,6 +14,7 @@ ) from app.metrics.collector import ( HTTP_CLIENTS, + PROFILE_FIELD_FILTERING, PROFILE_PARSE_ERRORS, UPSTREAM_REQUESTS, UPSTREAM_RESPONSES, @@ -286,8 +287,16 @@ async def test_nothing_stands_in_for_a_value_student_info_lacks( ): """The login response and STUDENT_PHOTO carry many of the same details; none fills a gap.""" for key in ( - "LoginId", "SRN", "NameAsInSSLC", "FirstName", "Email", "Mobile", - "ProgramAbbreviation", "ClassName", "SectionName", "DateOfBirth", + "LoginId", + "SRN", + "NameAsInSSLC", + "FirstName", + "Email", + "Mobile", + "ProgramAbbreviation", + "ClassName", + "SectionName", + "DateOfBirth", ): profile_payload["STUDENT_INFO"][key] = None # Every other source still has a value for each of them @@ -298,8 +307,16 @@ async def test_nothing_stands_in_for_a_value_student_info_lacks( profile = await _profile_for(pesu, upstream, make_response, login_payload, profile_payload) for field in ( - "prn", "srn", "name", "firstName", "email", "mobile", "program", - "semester", "section", "dateOfBirth", + "prn", + "srn", + "name", + "firstName", + "email", + "mobile", + "program", + "semester", + "section", + "dateOfBirth", ): assert profile[field] is None, field # What STUDENT_INFO still has is unaffected @@ -763,7 +780,9 @@ def _route_logins(wire, make_response, login_payload, profile_payload): @pytest.mark.asyncio -async def test_the_login_request_on_the_wire(pesu, wire, make_response, login_payload, profile_payload, multipart_fields): +async def test_the_login_request_on_the_wire( + pesu, wire, make_response, login_payload, profile_payload, multipart_fields +): _route_logins(wire, make_response, login_payload, profile_payload) await pesu.authenticate("PES2UG25CS001", "hunter2") @@ -784,7 +803,9 @@ async def test_the_login_request_on_the_wire(pesu, wire, make_response, login_pa @pytest.mark.asyncio -async def test_the_profile_request_on_the_wire(pesu, wire, make_response, login_payload, profile_payload, multipart_fields): +async def test_the_profile_request_on_the_wire( + pesu, wire, make_response, login_payload, profile_payload, multipart_fields +): _route_logins(wire, make_response, login_payload, profile_payload) await pesu.authenticate("user", "hunter2", profile=True) @@ -1192,7 +1213,7 @@ def test_every_profile_field_has_a_test_variable(profile_variables): def test_each_test_variable_is_named_after_its_field(profile_variables): - """branchShortCode is TEST_BRANCH_SHORT_CODE, so the variable for a field never needs looking up.""" + """The variable for branchShortCode is TEST_BRANCH_SHORT_CODE, so none ever needs looking up.""" assert profile_variables == {field: f"TEST_{to_snake(field).upper()}" for field in profile_variables} @@ -1218,3 +1239,25 @@ def test_an_unset_test_variable_is_an_error(monkeypatch, request, profile_variab with pytest.raises(AssertionError, match="TEST_MIDDLE_NAME environment variable not set"): request.getfixturevalue("expected_profile") + + +@pytest.mark.asyncio +async def test_field_filtering_reordered(pesu, upstream, make_response, login_payload, profile_payload, collector): + upstream.side_effect = [make_response(json=login_payload), make_response(json=profile_payload)] + + fields = list(reversed(PESUAcademy.DEFAULT_FIELDS)) + await pesu.authenticate("user", "pass", profile=True, fields=fields) + + snapshot = collector.snapshot() + assert snapshot.value(PROFILE_FIELD_FILTERING.name, enabled="false") == 1.0 + + +@pytest.mark.asyncio +async def test_real_subset_records(pesu, upstream, make_response, login_payload, profile_payload, collector): + upstream.side_effect = [make_response(json=login_payload), make_response(json=profile_payload)] + + fields = ["name", "email", "srn"] + await pesu.authenticate("user", "pass", profile=True, fields=fields) + + snapshot = collector.snapshot() + assert snapshot.value(PROFILE_FIELD_FILTERING.name, enabled="true") == 1.0 diff --git a/tests/unit/test_request_model.py b/tests/unit/test_request_model.py index a581095..91e5912 100644 --- a/tests/unit/test_request_model.py +++ b/tests/unit/test_request_model.py @@ -105,7 +105,9 @@ def test_validate_fields_multiple_invalid_fields(): def test_validate_fields_valid_fields(): - model = RequestModel(username="testuser", password="testpass", fields=["name", "email", "mobile", "dateOfBirth"]) + model = RequestModel( + username="testuser", password="testpass", profile=True, fields=["name", "email", "mobile", "dateOfBirth"] + ) assert model.fields == ["name", "email", "mobile", "dateOfBirth"] @@ -115,9 +117,11 @@ def test_validate_every_profile_field_is_accepted(): from app.models.profile import ProfileField every_field = list(get_args(ProfileField)) - assert RequestModel(username="testuser", password="testpass", fields=every_field).fields == every_field + assert ( + RequestModel(username="testuser", password="testpass", profile=True, fields=every_field).fields == every_field + ) for field in every_field: - assert RequestModel(username="testuser", password="testpass", fields=[field]).fields == [field] + assert RequestModel(username="testuser", password="testpass", profile=True, fields=[field]).fields == [field] def test_validate_fields_none(): @@ -229,7 +233,9 @@ def test_validate_snake_case_fields_rejected(field): assert "fields.0" in str(exc_info.value) -@pytest.mark.parametrize("field", ["bloodGroup", "photo", "profilePicture", "programShortCode", "phone", "institute", "rollNumber"]) +@pytest.mark.parametrize( + "field", ["bloodGroup", "photo", "profilePicture", "programShortCode", "phone", "institute", "rollNumber"] +) def test_validate_fields_the_api_does_not_return_rejected(field): """Neither what PESU sends but this API does not return, nor a field this API no longer returns, can be requested. @@ -249,3 +255,31 @@ def test_validate_removed_kycas_fields_rejected(): errors = exc_info.value.errors() assert any(e["type"] == "literal_error" for e in errors) assert "fields.0" in str(exc_info.value) + + +def test_validate_profile_key_omitted(): + """Fields with profile omitted is a 400.""" + with pytest.raises(ValidationError) as exc_info: + RequestModel(username="testuser", password="testpass", fields=["name", "srn"]) + assert "Fields can only be requested when profile is true" in str(exc_info.value) + + +def test_validate_profile_is_set_as_false(): + """Fields with profile set as False is a 400.""" + with pytest.raises(ValidationError) as exc_info: + RequestModel(username="testuser", password="testpass", profile=False, fields=["name", "srn"]) + assert "Fields can only be requested when profile is true" in str(exc_info.value) + + +def test_validate_duplicates_are_removed(): + """Test that the duplicates of fields are removed.""" + field_list = ["name", "email", "srn", "name", "email", "program"] + request_model = RequestModel(username="testuser", password="testpass", profile=True, fields=field_list) + assert request_model.fields == ["name", "email", "srn", "program"] + + +def test_validate_with_no_duplicates(): + """Test that a list with no duplicates comes back unchanged.""" + field_list = ["name", "email", "srn", "program"] + request_model = RequestModel(username="testuser", password="testpass", profile=True, fields=field_list) + assert request_model.fields == ["name", "email", "srn", "program"] diff --git a/tests/unit/test_sync_agents.py b/tests/unit/test_sync_agents.py index 2f8ded1..cb7088e 100644 --- a/tests/unit/test_sync_agents.py +++ b/tests/unit/test_sync_agents.py @@ -1,12 +1,10 @@ -from pathlib import Path - import pytest from scripts.sync_agents import AGENTS_PATH, REPO_ROOT, ROLES_PATH, main, read_frontmatter, sync_agents @pytest.fixture -def roles_dir(tmp_path: Path, agent_frontmatter) -> Path: +def roles_dir(tmp_path, agent_frontmatter): roles = tmp_path / "roles" roles.mkdir() (roles / "reviewer.md").write_text(f"{agent_frontmatter}\n# Reviewer\n\nThe full role.\n") @@ -15,7 +13,7 @@ def roles_dir(tmp_path: Path, agent_frontmatter) -> Path: @pytest.fixture -def agents_dir(tmp_path: Path) -> Path: +def agents_dir(tmp_path): return tmp_path / "agents" diff --git a/uv.lock b/uv.lock index 96c43e6..1e0c0ff 100644 --- a/uv.lock +++ b/uv.lock @@ -899,7 +899,7 @@ wheels = [ [[package]] name = "pesu-auth" -version = "5.4.0" +version = "5.5.0" source = { editable = "." } dependencies = [ { name = "fastapi" },