[Vector316I71A]

📌 Description

This PR fixes how profile fields are handled by removing duplicate names, making sure reordered full lists don't falsely trigger the filtering metric.

ℹ️ Fixes / Related Issues
Closes #178

🧱 Type of Change

Please indicate the type of changes introduced in your PR. Anything left unchecked will be assumed to be non-relevant

• 🐛 Bug fix – Non-breaking fix for a functional/logic error
• ✨ New feature – Adds functionality without breaking existing APIs
• ⚠️ Breaking change – Introduces backward-incompatible changes (API, schema, etc.)
• 📝 Documentation update – README, docstrings, OpenAPI tags, etc.
• 🧪 Test suite change – Adds/updates unit, functional, or integration tests
• ⚙️ CI/CD pipeline update – Modifies GitHub Actions, pre-commit, or Docker build
• 🧹 Code quality / Refactor – Improves structure, readability, or style (no functional changes)
• 🐢 Performance improvement – Speeds up auth, profile retrieval, or reduces I/O
• 🕵️ Debug/logging enhancement – Adds or improves logging/debug support
• 🔧 Developer tooling – Scripts, benchmarks, local testing improvements
• 🔒 Security fix – Addresses auth/session/data validation vulnerabilities
• 🧰 Dependency update – Updates libraries in pyproject.toml, uv.lock

🧪 How Has This Been Tested?

Please indicate how you tested your changes. Completing all the relevant items on this list is mandatory

• Unit Tests (tests/unit/)
• Functional Tests (tests/functional/)
• Integration Tests (tests/integration/)
• Manual Testing

⚙️ Test Configuration:

• OS: (Linux)
• Python: (3.14.8 via uv)
• Docker build tested

✅ Checklist

Please indicate the work items you have carried out. Completing all the relevant items on this list is mandatory. Anything left unchecked will be assumed to be non-relevant

• My code follows the CONTRIBUTING.md guidelines
• I've performed a self-review of my changes
• I've added/updated necessary comments and docstrings
• I've updated relevant docs (README or endpoint docs)
• No new warnings introduced
• I've added tests to cover my changes
• All tests pass locally (scripts/run_tests.py)
• I've run linting and formatting (pre-commit run --all-files)
• Docker image builds and runs correctly
• Changes are backwards compatible (if applicable)
• Feature flags or .env vars updated (if applicable)
• I've tested across multiple environments (if applicable)
• Benchmarks still meet expected performance (scripts/benchmark/benchmark_requests.py)

🛠️ Affected API Behaviour

Please indicate the areas affected by changes introduced in your PR

• app/app.py – Modified /authenticate route logic
• app/pesu.py – Updated PESU Academy login or profile handling
• app/exceptions/ – Error types, their status codes or messages
• app/metrics/ – Metrics collection, exposition or the /metrics token
• app/docs/ – OpenAPI (Swagger) examples and descriptions

🧩 Models

• app/models/request.py – Input validation or request schema changes
• app/models/response.py – Authentication response formatting
• app/models/profile.py – Profile response schema
• app/models/upstream.py – Parsing of PESU Academy's login and profile responses

🐳 DevOps & Config

• Dockerfile – Changes to base image or build process
• .github/workflows/*.yml – CI/CD pipeline or deployment updates
• pyproject.toml / uv.lock – Dependency version changes
• .pre-commit-config.yaml – Linting or formatting hook changes
• .github/scripts/check_version_bump.py – The CI version check

📊 Benchmarks & Analysis

• scripts/benchmark/benchmark_requests.py – Performance or latency measurement changes
• scripts/benchmark/analyze_benchmark.py – Benchmark result analysis changes
• scripts/run_tests.py – Custom test runner logic or behavior updates

📸 Screenshots / API Demos (if applicable)

🧠 Additional Notes (if applicable)

Fields sent without profile: true (either omitted or set to false) are now rejected with a 400 Bad Request

[aditeyabaral]

Thanks for the update! This follows up on the review on #255. Most of it is addressed, and the logic is right: I checked that each new test fails when its fix is reverted, and coverage of app/ is still 100%. Two things block merging, plus some smaller items below.

Blocking

1. Stray import in tests/unit/test_request_model.py (inline comment). On a Python without idlelib, the whole module fails to import and none of the request-model tests run.
2. The docs don't describe the new behaviour yet. None of these are in the diff, so I can't comment on the lines:
   • app/models/request.py:75, the fields description. "Only used when profile is true" is no longer accurate: it can only be given when profile is true, otherwise the request is rejected. Also mention that repeated names are ignored. This text is the Swagger description, so it matters.
   • README.md:180, the fields row of the request table: same change.
   • README.md:184, the list of requests rejected with a 400: add fields without profile: true.
   • README.md:371, the profile_field_filtering_total row: "a caller passing exactly the default list counts as false" should now say "in any order".
   • app/docs/authenticate.py: add a 400 example for this case next to unknown_profile_field, with the exact message the API returns: Could not validate request data - body: Value error, Fields can only be requested when profile is true.. Also add "fields without profile set to true" to the 400 description.

PR title and description

The PR is squash-merged, so its title becomes the commit message. Please update it to cover the main change, e.g. fix: reject fields without profile, remove duplicate fields and count field filtering by set. The description still says it "adds a debug log when fields are sent without enabling the profile option" and ticks "Debug/logging enhancement". Please describe the 400 instead, and untick that box.

Checked and fine

• Deduplication, the set comparison and the comment above it, and the metric tests (.name, == 1.0, collector).
• The new validator: a request with fields and profile omitted or false gets a 400 with the standard error body and never reaches PESU Academy. "fields": null without profile is still accepted. A bad field name still reports the field error, not this one.
• The version bump to 5.5.0 against dev's 5.4.0 (minor), and uv.lock matches.
• The commit email is fixed.

[aditeyabaral]

Re-review at fc1ac47.

Note: I pushed a commit to your branch (fc1ac47, "chore: lint and format tests/ with ruff"). It turns ruff on for tests/, so most of the style nits from the last review are now caught by pre-commit and CI. It also fixes the ones ruff found here, including the stray idlelib import. Please git pull before you push again, or your push will be rejected. If you run uv run pre-commit run --all-files before committing, ruff will flag these for you from now on.

I've resolved the threads that commit fully fixed and replied on the ones it only partly fixed.

New in this review

• Should fix: the new check should live in validate_fields, so the error is reported against fields. See the inline comment on app/models/request.py:103. It replaces the earlier comments on lines 106 and 114–117.

Still open from the last review

• Blocking: the docs. These aren't in the diff, so they're listed here:
  • app/models/request.py:75, the fields description: it can only be given when profile is true, otherwise the request is a 400. Repeated names are ignored.
  • README.md:180, the fields row of the request table: same change.
  • README.md:184, the list of requests rejected with a 400: add fields without profile: true.
  • README.md:371, the profile_field_filtering_total row: "exactly the default list, in any order".
  • app/docs/authenticate.py: a 400 example next to unknown_profile_field, plus "fields without profile set to true" in the 400 description. With the suggestion below, the message is Could not validate request data - body.fields: Value error, Fields can only be requested when profile is true..
• The PR title and description. The title should cover the rejection, e.g. fix: reject fields without profile, remove duplicate fields and count field filtering by set. The description still describes a DEBUG log and ticks "Debug/logging enhancement".
• The open threads below: the test rename in test_authenticate_flow.py, the always-true assert in the integration test, the docstrings of the two rejection tests, and the duplicate test.

Checked and fine: deduplication and the set comparison (each test fails when its fix is reverted); requests with fields but no profile: true never reach PESU Academy; the version bump to 5.5.0; uv.lock; all CI checks.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

[achyu-dev]

Re-review at f008e27.

---

What's been fixed since the previous reviews

• Validator check moved into validate_fields — error is now at body.fields, matching the expected message format. ✅
• All five docs items from the blocking list: app/models/request.py:75, README.md:180, README.md:184, README.md:371, app/docs/authenticate.py 400 example. ✅
• PR title updated to match the change. ✅
• Test renamed to test_fields_without_profile_are_rejected. ✅
• assert data["status"] is True in the health-check integration test. ✅
• The idlelib stray import, line-length violations, and trailing-comma issues — all resolved by fc1ac47. ✅

---

New finding — docs: timestamp type in the fields_without_profile example

app/docs/authenticate.py, fields_without_profile example:

"timestamp": 1700000000,

Every other 400 example in this file uses an ISO 8601 datetime string (e.g. "2024-07-28T22:30:10.103368+05:30"). The actual API emits a datetime, never a Unix integer. Please update to match:

"timestamp": "2024-07-28T22:30:10.103368+05:30",

---

New finding — test: ordering assertion removed without replacement

In test_authenticate_flow.py, test_requested_fields_only_and_null_when_empty, the diff removes:

# On the wire, in the documented order
assert list(profile) == [field for field in full_profile if field in fields]

The README states "Fields come back in the table's order, whatever order they are asked for in." This was the only automated check of that ordering guarantee. Nothing replaces it. Please restore the assertion or add an equivalent one so the documented contract stays tested.

---

Still open from @aditeyabaral review

1. Docstrings on test_validate_profile_key_omitted and test_validate_profile_is_set_as_false — the current docstrings describe the input ("Test that fields are rejected when profile is omitted/false"). The repo convention and the reviewer's suggestion is to state what is expected instead (e.g., """fields with profile omitted is a 400."""). The inline threads on these are still open.

2. test_validate_profile_false_without_fields — the reviewer flagged this as duplicating an existing test (test_validate_fields_none or test_validate_with_no_duplicates). The thread is still open. If test_validate_fields_none already covers fields=None with the default profile=False, one of them can be removed.

[aditeyabaral]

Re-review at 23bd3e4. Thanks, this is close!

Fixed since the last review:

• The check now lives in validate_fields, so the error is body.fields: ... and the metric records field="fields".
• The model_validator import is gone.
• The docs are updated: the fields description, the README request table, the list of rejected requests, the metrics row, and a 400 example in app/docs/authenticate.py.
• The PR title, the test rename, the always-true assert, the test docstrings and the duplicate test.

I confirmed each of these on the threads that were resolved without a reply. I've reopened one thread whose fix is only partial (see below).

@achyu-dev's two findings are already fixed at 23bd3e4: the example's timestamp is an ISO string again, and the ordering assertion in test_requested_fields_only_and_null_when_empty is still there with its comment reworded. I've answered the questions about the ruff config on those threads.

I also checked:

• Sending fields before profile in the JSON still gets the 400, because pydantic validates fields in declaration order, not input order.
• "profile": "yes" reports only the profile error.
• Coverage of app/ is 100%.
• All CI checks pass.

Still to do:

1. Should fix: the new metrics test doesn't test the new rule (inline comment).
2. Nits: wording in the docs and the README (inline suggestions, already checked against mdformat and ruff). The fields description and README suggestions also answer @jois-code's question about fields: null.
3. PR description: the Description section should also say that fields without profile: true is now rejected with a 400. That's the main user-visible change.

[aditeyabaral]

Thanks for applying the suggestions. The docs, README and fields description are all done now, and I've confirmed and resolved those threads.

Two things are left, both as one-click suggestions below:

1. Pre-commit is failing because of trailing blank lines at the end of tests/integration/test_app_integration.py.
2. The replacement metrics test is missing. The old test was removed, but the new one, which lived in tests/unit/test_metrics_endpoints.py, wasn't added.

Please also mention the new 400 in the PR description. It currently describes only the duplicate removal and the metric fix, but rejecting fields without profile: true is a behaviour change callers will notice, and the squash-merge title only partly covers it.

[aditeyabaral]

Thanks, everything from the reviews is in, all threads are resolved, and CI is green on f7d0b92.

One thing left before this can be approved: the PR description. It still describes only the duplicate removal and the metric fix. Please add a line saying that fields sent without profile: true (omitted or false) is now rejected with a 400, since that is the change callers will notice.

While you are there, a few boxes are now out of date:

• Docs: "I've updated relevant docs" should be ticked.
• Affected areas: app/docs/ should be ticked.
• Config: pyproject.toml / uv.lock and .pre-commit-config.yaml should be ticked, because the ruff config changed in fc1ac47.

[aditeyabaral]

Make PR description updates, else LGTM

[jois-code]

LGTM

Already covered