From 533152a04ac94a2c866ed6b21c1f6842f1e2a75b Mon Sep 17 00:00:00 2001 From: Vector316I71A <203425201+Vector316I71A@users.noreply.github.com> Date: Tue, 20 Oct 2026 19:00:06 +0530 Subject: [PATCH 01/10] fix: reject fields without profile and remove duplicate fields and count field filtering by set --- app/models/request.py | 13 +++++- app/pesu.py | 4 +- pyproject.toml | 2 +- tests/integration/test_app_integration.py | 21 +++++++++ tests/unit/test_authenticate_flow.py | 10 +++-- tests/unit/test_pesu.py | 23 ++++++++++ tests/unit/test_request_model.py | 52 +++++++++++++++++++++-- uv.lock | 2 +- 8 files changed, 114 insertions(+), 13 deletions(-) diff --git a/app/models/request.py b/app/models/request.py index 3ef6bfe..1cad130 100644 --- a/app/models/request.py +++ b/app/models/request.py @@ -2,7 +2,7 @@ from typing import ClassVar -from pydantic import BaseModel, ConfigDict, Field, ValidationInfo, field_validator +from pydantic import BaseModel, ConfigDict, Field, ValidationInfo, field_validator, model_validator from pydantic.alias_generators import to_camel from app.models.profile import ProfileField @@ -103,11 +103,20 @@ def validate_password(cls, v: str) -> str: @field_validator("fields") @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.""" + """Validate that fields is either None or non-empty list containing allowed field names without duplicates.""" if v is not None and not v: raise ValueError("Fields must be a non-empty list or None.") + if v is not None: + return list(dict.fromkeys(v)) return v + @model_validator(mode="after") + def validate_fields_returned_only_when_profile_is_true(self) -> RequestModel: + """Validate that fields are only requested when profile is True.""" + if self.profile is not True and self.fields is not None: + raise ValueError("Fields can only be requested when profile is True.") + return self + @field_validator("username", "password", mode="before") @classmethod def validate_text(cls, v: object, info: ValidationInfo) -> object: 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..33978bf 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" diff --git a/tests/integration/test_app_integration.py b/tests/integration/test_app_integration.py index 8a448ab..12fd383 100644 --- a/tests/integration/test_app_integration.py +++ b/tests/integration/test_app_integration.py @@ -603,3 +603,24 @@ 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 "profile" not in response.json() + 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/test_authenticate_flow.py b/tests/unit/test_authenticate_flow.py index 998e8db..2768907 100644 --- a/tests/unit/test_authenticate_flow.py +++ b/tests/unit/test_authenticate_flow.py @@ -123,10 +123,12 @@ def test_requested_fields_only_and_null_when_empty(client, pesu_up, profile_payl def test_fields_without_profile_return_no_profile(client, pesu_up): - body = _authenticate(client, profile=False, fields=["name", "gender", "mobile"]).json() - - assert "profile" not in body - assert len(pesu_up.requests) == 1 + body = _authenticate( + client, profile=False, fields=["name", "gender", "mobile"] + ) + assert body.status_code == 400 + assert "Fields can only be requested when profile is True." in body.text + assert len(pesu_up.requests) == 0 def test_the_username_is_sent_without_surrounding_whitespace(client, pesu_up): diff --git a/tests/unit/test_pesu.py b/tests/unit/test_pesu.py index 33096df..bca9225 100644 --- a/tests/unit/test_pesu.py +++ b/tests/unit/test_pesu.py @@ -18,6 +18,7 @@ UPSTREAM_REQUESTS, UPSTREAM_RESPONSES, MetricsCollector, + PROFILE_FIELD_FILTERING ) from app.models import ProfileModel from app.pesu import _CLOSE_TASKS, DISPATCHER_URL, LOGIN_URL, PESUAcademy, _upstream_call @@ -1218,3 +1219,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..b37e0f4 100644 --- a/tests/unit/test_request_model.py +++ b/tests/unit/test_request_model.py @@ -1,3 +1,5 @@ +from idlelib.iomenu import errors + import pytest from pydantic import ValidationError @@ -105,7 +107,7 @@ 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,9 @@ 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(): @@ -249,3 +251,47 @@ 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(): + """Test that the profile key is omitted.""" + 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(): + """Test that the profile is set as false.""" + 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"] + + +def test_validate_fields_with_profile_true_accepted(): + """Test that fields with profile=True are accepted.""" + field_list = ["name", "email", "srn", "program"] + request_model = RequestModel(username="testuser", password="testpass", profile=True, fields=field_list) + assert request_model.profile == True + assert request_model.fields == ["name", "email", "srn", "program"] + + +def test_validate_profile_false_without_fields(): + """Test that profile=False without fields is accepted.""" + field_list = ["name", "email", "srn", "program"] + request_model = RequestModel(username="testuser", password="testpass", profile=False, fields=None) + assert request_model.profile == False + assert request_model.fields == None 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" }, From fc1ac4738b17afea2caf4b1ce6e9069583c09d49 Mon Sep 17 00:00:00 2001 From: aditeyabaral Date: Tue, 20 Oct 2026 20:06:20 -0500 Subject: [PATCH 02/10] chore: lint and format tests/ with ruff tests/ was excluded from ruff, so import order, unused imports, long lines and comparisons to True or None in tests were left to review to catch. Lint and format it like the rest of the repo, except for the annotation (ANN) and missing-docstring (D1) rules: a test's name says what it checks, and pytest fixture arguments need no annotations. Ruff now reports syntax errors in tests/, so the compileall hook that existed only for that is removed, along with its CONTRIBUTING entry. The fixes are the ones ruff asked for: unused imports, including a stray idlelib import that broke collecting test_request_model.py wherever idlelib is not installed; unsorted imports; lines over 120 characters; == comparisons to True, False and None; an unused variable; three docstrings without a summary line; and fixture annotations in test_sync_agents.py that needed Path imported only for them. --- .github/CONTRIBUTING.md | 1 - .pre-commit-config.yaml | 9 +----- pyproject.toml | 5 ++- tests/integration/test_app_integration.py | 8 +++-- tests/unit/conftest.py | 35 ++++++++++++++++++--- tests/unit/test_app_unit.py | 20 ++++++++---- tests/unit/test_authenticate_flow.py | 4 +-- tests/unit/test_check_version_bump.py | 4 ++- tests/unit/test_metrics_endpoints.py | 8 +++-- tests/unit/test_metrics_instrumentation.py | 2 +- tests/unit/test_metrics_middleware.py | 14 ++++++--- tests/unit/test_openapi_contract.py | 19 ++++++++---- tests/unit/test_openapi_docs.py | 2 +- tests/unit/test_pesu.py | 36 +++++++++++++++++----- tests/unit/test_request_model.py | 25 ++++++++------- tests/unit/test_sync_agents.py | 6 ++-- 16 files changed, 132 insertions(+), 66 deletions(-) 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/pyproject.toml b/pyproject.toml index 33978bf..003d298 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -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 12fd383..4b736fa 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) 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 2768907..200e67b 100644 --- a/tests/unit/test_authenticate_flow.py +++ b/tests/unit/test_authenticate_flow.py @@ -123,9 +123,7 @@ def test_requested_fields_only_and_null_when_empty(client, pesu_up, profile_payl def test_fields_without_profile_return_no_profile(client, pesu_up): - body = _authenticate( - client, profile=False, fields=["name", "gender", "mobile"] - ) + body = _authenticate(client, profile=False, fields=["name", "gender", "mobile"]) assert body.status_code == 400 assert "Fields can only be requested when profile is True." in body.text assert len(pesu_up.requests) == 0 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..3140a0c 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 @@ -264,7 +266,7 @@ def test_failures_are_attributed_to_client_or_server(mock_authenticate, client): 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 bca9225..f4eb722 100644 --- a/tests/unit/test_pesu.py +++ b/tests/unit/test_pesu.py @@ -14,11 +14,11 @@ ) from app.metrics.collector import ( HTTP_CLIENTS, + PROFILE_FIELD_FILTERING, PROFILE_PARSE_ERRORS, UPSTREAM_REQUESTS, UPSTREAM_RESPONSES, MetricsCollector, - PROFILE_FIELD_FILTERING ) from app.models import ProfileModel from app.pesu import _CLOSE_TASKS, DISPATCHER_URL, LOGIN_URL, PESUAcademy, _upstream_call @@ -287,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 @@ -299,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 @@ -764,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") @@ -785,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) @@ -1193,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} diff --git a/tests/unit/test_request_model.py b/tests/unit/test_request_model.py index b37e0f4..78bfbb0 100644 --- a/tests/unit/test_request_model.py +++ b/tests/unit/test_request_model.py @@ -1,5 +1,3 @@ -from idlelib.iomenu import errors - import pytest from pydantic import ValidationError @@ -107,7 +105,9 @@ def test_validate_fields_multiple_invalid_fields(): def test_validate_fields_valid_fields(): - model = RequestModel(username="testuser", password="testpass", profile=True, 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"] @@ -117,7 +117,9 @@ 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", profile=True, 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", profile=True, fields=[field]).fields == [field] @@ -231,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. @@ -257,14 +261,14 @@ def test_validate_profile_key_omitted(): """Test that the profile key is omitted.""" 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)) + assert "Fields can only be requested when profile is True" in str(exc_info.value) def test_validate_profile_is_set_as_false(): """Test that the profile is set as false.""" 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)) + assert "Fields can only be requested when profile is True" in str(exc_info.value) def test_validate_duplicates_are_removed(): @@ -285,13 +289,12 @@ def test_validate_fields_with_profile_true_accepted(): """Test that fields with profile=True are accepted.""" field_list = ["name", "email", "srn", "program"] request_model = RequestModel(username="testuser", password="testpass", profile=True, fields=field_list) - assert request_model.profile == True + assert request_model.profile is True assert request_model.fields == ["name", "email", "srn", "program"] def test_validate_profile_false_without_fields(): """Test that profile=False without fields is accepted.""" - field_list = ["name", "email", "srn", "program"] request_model = RequestModel(username="testuser", password="testpass", profile=False, fields=None) - assert request_model.profile == False - assert request_model.fields == None + assert request_model.profile is False + assert request_model.fields is None 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" From d8ecb35f011d94ac056a281d1dbab8f0ce35c6b0 Mon Sep 17 00:00:00 2001 From: Vector316I71A <203425201+Vector316I71A@users.noreply.github.com> Date: Wed, 21 Oct 2026 13:31:35 +0530 Subject: [PATCH 03/10] Apply batched suggestions from code review Co-authored-by: Aditeya Baral --- app/models/request.py | 10 +++++----- tests/integration/test_app_integration.py | 1 - tests/unit/test_request_model.py | 4 ++-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/app/models/request.py b/app/models/request.py index 1cad130..03e5cdb 100644 --- a/app/models/request.py +++ b/app/models/request.py @@ -103,7 +103,7 @@ def validate_password(cls, v: str) -> str: @field_validator("fields") @classmethod def validate_fields(cls, v: list[str] | None) -> list[str] | None: - """Validate that fields is either None or non-empty list containing allowed field names without duplicates.""" + """Validate that fields is either None or a non-empty list of allowed field names, and remove duplicates.""" if v is not None and not v: raise ValueError("Fields must be a non-empty list or None.") if v is not None: @@ -111,10 +111,10 @@ def validate_fields(cls, v: list[str] | None) -> list[str] | None: return v @model_validator(mode="after") - def validate_fields_returned_only_when_profile_is_true(self) -> RequestModel: - """Validate that fields are only requested when profile is True.""" - if self.profile is not True and self.fields is not None: - raise ValueError("Fields can only be requested when profile is True.") + def validate_fields_need_profile(self) -> RequestModel: + """Reject fields unless profile is true, since they would otherwise be silently ignored.""" + if not self.profile and self.fields is not None: + raise ValueError("Fields can only be requested when profile is true.") return self @field_validator("username", "password", mode="before") diff --git a/tests/integration/test_app_integration.py b/tests/integration/test_app_integration.py index 4b736fa..bff5ed0 100644 --- a/tests/integration/test_app_integration.py +++ b/tests/integration/test_app_integration.py @@ -614,7 +614,6 @@ def test_integration_profile_key_omitted(client): response = client.post("/authenticate", json=payload) data = response.json() assert response.status_code == 400 - assert "profile" not in response.json() assert "Fields can only be requested when profile is True" in data["message"] diff --git a/tests/unit/test_request_model.py b/tests/unit/test_request_model.py index 78bfbb0..65ac638 100644 --- a/tests/unit/test_request_model.py +++ b/tests/unit/test_request_model.py @@ -258,14 +258,14 @@ def test_validate_removed_kycas_fields_rejected(): def test_validate_profile_key_omitted(): - """Test that the profile key is omitted.""" + """Test that fields are rejected when profile is omitted.""" 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(): - """Test that the profile is set as false.""" + """Test that fields are rejected when profile is false.""" 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) From 21c62aad0f70ca895ae56583405bbeb30aff6f6d Mon Sep 17 00:00:00 2001 From: Vector316I71A <203425201+Vector316I71A@users.noreply.github.com> Date: Wed, 21 Oct 2026 13:33:44 +0530 Subject: [PATCH 04/10] Update tests/unit/test_authenticate_flow.py Co-authored-by: Aditeya Baral --- tests/unit/test_authenticate_flow.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/tests/unit/test_authenticate_flow.py b/tests/unit/test_authenticate_flow.py index 200e67b..1cd663d 100644 --- a/tests/unit/test_authenticate_flow.py +++ b/tests/unit/test_authenticate_flow.py @@ -116,17 +116,12 @@ def test_requested_fields_only_and_null_when_empty(client, pesu_up, profile_payl "mobile": "9876543210", "middleName": None, "branchShortCode": "CSE", - "dateOfBirth": "2005-01-01", - } - # On the wire, in the documented order - assert list(profile) == [field for field in full_profile if field in fields] - +def test_fields_without_profile_are_rejected(client, pesu_up): + response = _authenticate(client, profile=False, fields=["name", "gender", "mobile"]) -def test_fields_without_profile_return_no_profile(client, pesu_up): - body = _authenticate(client, profile=False, fields=["name", "gender", "mobile"]) - assert body.status_code == 400 - assert "Fields can only be requested when profile is True." in body.text - assert len(pesu_up.requests) == 0 + 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): From 3de2e6c4affd9fe56d69c6228319c5c8569f1d83 Mon Sep 17 00:00:00 2001 From: Vector316I71A <203425201+Vector316I71A@users.noreply.github.com> Date: Wed, 21 Oct 2026 13:34:10 +0530 Subject: [PATCH 05/10] Update app/models/request.py Co-authored-by: Aditeya Baral --- app/models/request.py | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/app/models/request.py b/app/models/request.py index 03e5cdb..853c051 100644 --- a/app/models/request.py +++ b/app/models/request.py @@ -99,23 +99,18 @@ def validate_password(cls, v: str) -> str: if not v.strip(): raise ValueError("Password cannot be empty.") return v - @field_validator("fields") @classmethod - def validate_fields(cls, v: list[str] | None) -> list[str] | None: - """Validate that fields is either None or a non-empty list of allowed field names, and remove duplicates.""" - 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.") - if v is not None: - return list(dict.fromkeys(v)) - return v - - @model_validator(mode="after") - def validate_fields_need_profile(self) -> RequestModel: - """Reject fields unless profile is true, since they would otherwise be silently ignored.""" - if not self.profile and self.fields is not None: + # 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 self + return list(dict.fromkeys(v)) @field_validator("username", "password", mode="before") @classmethod From f008e273e9773337dcc8d881759c25eda2be15bb Mon Sep 17 00:00:00 2001 From: Vector316I71A <203425201+Vector316I71A@users.noreply.github.com> Date: Wed, 21 Oct 2026 14:44:52 +0530 Subject: [PATCH 06/10] chore: fixed issues and updated readme.md --- README.md | 16 ++++++++-------- app/docs/authenticate.py | 14 ++++++++++++-- app/models/request.py | 10 ++++++---- tests/integration/test_app_integration.py | 15 +++++++++++++-- tests/unit/test_authenticate_flow.py | 4 ++++ tests/unit/test_request_model.py | 12 ++---------- 6 files changed, 45 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index de187e5..ebcb7c1 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. Can only be given when `profile` is `true`, otherwise the request is a 400. If not provided, 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..4e62772 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, or 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,15 @@ "timestamp": "2024-07-28T22:30:10.103368+05:30", }, }, + "fields_without_profile": { + "summary": "Fields without profile set to true", + "value": { + "status": False, + "timestamp": 1700000000, + "message": "Could not validate request data - body.fields: Value error, " + "Fields can only be requested when profile is true.", + }, + }, "username_too_long": { "summary": "Username too long", "value": { diff --git a/app/models/request.py b/app/models/request.py index 853c051..4d18e70 100644 --- a/app/models/request.py +++ b/app/models/request.py @@ -2,7 +2,7 @@ from typing import ClassVar -from pydantic import BaseModel, ConfigDict, Field, ValidationInfo, field_validator, model_validator +from pydantic import BaseModel, ConfigDict, Field, ValidationInfo, field_validator from pydantic.alias_generators import to_camel from app.models.profile import ProfileField @@ -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. 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"]}, ) @@ -99,7 +100,8 @@ def validate_password(cls, v: str) -> str: if not v.strip(): 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, info: ValidationInfo) -> list[str] | None: """Validate fields: a non-empty list, only with profile set to true, with duplicates removed.""" diff --git a/tests/integration/test_app_integration.py b/tests/integration/test_app_integration.py index bff5ed0..451b1e9 100644 --- a/tests/integration/test_app_integration.py +++ b/tests/integration/test_app_integration.py @@ -614,7 +614,7 @@ def test_integration_profile_key_omitted(client): 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"] + assert "Fields can only be requested when profile is true" in data["message"] def test_integration_profile_is_false(client): @@ -624,4 +624,15 @@ def test_integration_profile_is_false(client): 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"] + assert "Fields can only be requested when profile is true" in data["message"] + + +def test_validation_error_for_fields(client): + payload = {"username": "test", "password": "password", "profile": False, "fields": "invalid_value"} + response = client.post("/authenticate", json=payload) + assert response.status_code == 400 + + metrics_response = client.get("/metrics") + assert metrics_response.status_code == 200 + + assert 'validation_errors_total{field="fields"}' in metrics_response.text diff --git a/tests/unit/test_authenticate_flow.py b/tests/unit/test_authenticate_flow.py index 1cd663d..599b926 100644 --- a/tests/unit/test_authenticate_flow.py +++ b/tests/unit/test_authenticate_flow.py @@ -116,6 +116,10 @@ def test_requested_fields_only_and_null_when_empty(client, pesu_up, profile_payl "mobile": "9876543210", "middleName": None, "branchShortCode": "CSE", + "dateOfBirth": "2005-01-01", + } + + def test_fields_without_profile_are_rejected(client, pesu_up): response = _authenticate(client, profile=False, fields=["name", "gender", "mobile"]) diff --git a/tests/unit/test_request_model.py b/tests/unit/test_request_model.py index 65ac638..91cac91 100644 --- a/tests/unit/test_request_model.py +++ b/tests/unit/test_request_model.py @@ -261,14 +261,14 @@ def test_validate_profile_key_omitted(): """Test that fields are rejected when profile is omitted.""" 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) + assert "Fields can only be requested when profile is true" in str(exc_info.value) def test_validate_profile_is_set_as_false(): """Test that fields are rejected when profile is false.""" 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) + assert "Fields can only be requested when profile is true" in str(exc_info.value) def test_validate_duplicates_are_removed(): @@ -285,14 +285,6 @@ def test_validate_with_no_duplicates(): assert request_model.fields == ["name", "email", "srn", "program"] -def test_validate_fields_with_profile_true_accepted(): - """Test that fields with profile=True are accepted.""" - field_list = ["name", "email", "srn", "program"] - request_model = RequestModel(username="testuser", password="testpass", profile=True, fields=field_list) - assert request_model.profile is True - assert request_model.fields == ["name", "email", "srn", "program"] - - def test_validate_profile_false_without_fields(): """Test that profile=False without fields is accepted.""" request_model = RequestModel(username="testuser", password="testpass", profile=False, fields=None) From 23bd3e4e67fbf2db2df2dc43bf903ee86e1575ac Mon Sep 17 00:00:00 2001 From: Vector316I71A <203425201+Vector316I71A@users.noreply.github.com> Date: Thu, 22 Oct 2026 00:18:35 +0530 Subject: [PATCH 07/10] chore: update docstrings and functions --- app/docs/authenticate.py | 2 +- tests/unit/test_authenticate_flow.py | 2 ++ tests/unit/test_request_model.py | 11 ++--------- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/app/docs/authenticate.py b/app/docs/authenticate.py index 4e62772..38d2432 100644 --- a/app/docs/authenticate.py +++ b/app/docs/authenticate.py @@ -230,7 +230,7 @@ "summary": "Fields without profile set to true", "value": { "status": False, - "timestamp": 1700000000, + "timestamp": "2024-07-28T22:30:10.103368+05:30", "message": "Could not validate request data - body.fields: Value error, " "Fields can only be requested when profile is true.", }, diff --git a/tests/unit/test_authenticate_flow.py b/tests/unit/test_authenticate_flow.py index 599b926..10cf17a 100644 --- a/tests/unit/test_authenticate_flow.py +++ b/tests/unit/test_authenticate_flow.py @@ -118,6 +118,8 @@ def test_requested_fields_only_and_null_when_empty(client, pesu_up, profile_payl "branchShortCode": "CSE", "dateOfBirth": "2005-01-01", } + # 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_are_rejected(client, pesu_up): diff --git a/tests/unit/test_request_model.py b/tests/unit/test_request_model.py index 91cac91..91e5912 100644 --- a/tests/unit/test_request_model.py +++ b/tests/unit/test_request_model.py @@ -258,14 +258,14 @@ def test_validate_removed_kycas_fields_rejected(): def test_validate_profile_key_omitted(): - """Test that fields are rejected when profile is 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(): - """Test that fields are rejected when profile is 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) @@ -283,10 +283,3 @@ def test_validate_with_no_duplicates(): 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"] - - -def test_validate_profile_false_without_fields(): - """Test that profile=False without fields is accepted.""" - request_model = RequestModel(username="testuser", password="testpass", profile=False, fields=None) - assert request_model.profile is False - assert request_model.fields is None From 094d0687c681c835308ccc1c2aca7bff0b466bfe Mon Sep 17 00:00:00 2001 From: Vector316I71A <203425201+Vector316I71A@users.noreply.github.com> Date: Thu, 22 Oct 2026 11:08:28 +0530 Subject: [PATCH 08/10] Apply batched suggestions from code review Co-authored-by: Aditeya Baral --- README.md | 14 +++++++------- app/docs/authenticate.py | 12 +++++++----- app/models/request.py | 6 +++--- tests/integration/test_app_integration.py | 9 --------- 4 files changed, 17 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index ebcb7c1..7012d4b 100644 --- a/README.md +++ b/README.md @@ -172,12 +172,12 @@ 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. Can only be given when `profile` is `true`, otherwise the request is a 400. If not provided, all fields are returned. Repeated names are ignored. 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 @@ -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, in any order 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 38d2432..078b6bd 100644 --- a/app/docs/authenticate.py +++ b/app/docs/authenticate.py @@ -156,9 +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 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." + "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, @@ -230,9 +230,11 @@ "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", - "message": "Could not validate request data - body.fields: Value error, " - "Fields can only be requested when profile is true.", }, }, "username_too_long": { diff --git a/app/models/request.py b/app/models/request.py index 4d18e70..f278ee4 100644 --- a/app/models/request.py +++ b/app/models/request.py @@ -73,9 +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. 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." + "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"]}, ) diff --git a/tests/integration/test_app_integration.py b/tests/integration/test_app_integration.py index 451b1e9..795c8ba 100644 --- a/tests/integration/test_app_integration.py +++ b/tests/integration/test_app_integration.py @@ -627,12 +627,3 @@ def test_integration_profile_is_false(client): assert "Fields can only be requested when profile is true" in data["message"] -def test_validation_error_for_fields(client): - payload = {"username": "test", "password": "password", "profile": False, "fields": "invalid_value"} - response = client.post("/authenticate", json=payload) - assert response.status_code == 400 - - metrics_response = client.get("/metrics") - assert metrics_response.status_code == 200 - - assert 'validation_errors_total{field="fields"}' in metrics_response.text From 75baab73b3dc008cff27047335e647e3125084f5 Mon Sep 17 00:00:00 2001 From: Vector316I71A <203425201+Vector316I71A@users.noreply.github.com> Date: Thu, 22 Oct 2026 11:18:38 +0530 Subject: [PATCH 09/10] Apply suggestion from @aditeyabaral Co-authored-by: Aditeya Baral --- tests/integration/test_app_integration.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/integration/test_app_integration.py b/tests/integration/test_app_integration.py index 795c8ba..964d6b5 100644 --- a/tests/integration/test_app_integration.py +++ b/tests/integration/test_app_integration.py @@ -625,5 +625,3 @@ def test_integration_profile_is_false(client): data = response.json() assert response.status_code == 400 assert "Fields can only be requested when profile is true" in data["message"] - - From f7d0b92638297e21db3689a4b80a679275e4d4e0 Mon Sep 17 00:00:00 2001 From: Vector316I71A <203425201+Vector316I71A@users.noreply.github.com> Date: Thu, 22 Oct 2026 11:18:57 +0530 Subject: [PATCH 10/10] Apply suggestion from @aditeyabaral Co-authored-by: Aditeya Baral --- tests/unit/test_metrics_endpoints.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/unit/test_metrics_endpoints.py b/tests/unit/test_metrics_endpoints.py index 3140a0c..9570f07 100644 --- a/tests/unit/test_metrics_endpoints.py +++ b/tests/unit/test_metrics_endpoints.py @@ -265,6 +265,12 @@ 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.""" body = client.get("/metrics?fmt=json").json()