Differentiate authentication/request error messages (#274) - #3633
wakqasahmed wants to merge 3 commits into
Conversation
…com#274) - reject nan/inf/-inf/Infinity/1e400 chapterId values with a clear 400 instead of letting them reach PyMySQL and raise an unformatted 500 - reject limit/page < 1 with a clear 400 instead of falling through to Flask-SQLAlchemy's bare 404 from paginate(error_out=True) - truncate raw query-param values echoed into error messages to 50 chars - document the new 400 responses for the paginated endpoints and /hadiths in spec.v1.yml
|
Addressed the review feedback in 914af9c. Blocking findings — fixed:
Non-blocking — fixed: Nit — fixed: Not changed:
Verification: No formal test suite in this repo (confirmed again). Verified manually by building and running the stack via
|
|
Hi @hasankhan @Suhaibinator — noticed this PR doesn't have a reviewer assigned yet — it's been about 4 days, CI is green and it's mergeable. Would you (or whoever's best placed) be able to take a look when you get a chance, or point me to who should? Thanks! |
|
@wakqasahmed have you been able to reproduce the issue? |
|
Hi @Suhaibinator — yes. I reproduced each of the three cases before changing them, using a standalone Flask harness that mirrors the exact modified code paths (no full app stack available in my environment, so the harness approach instead):
Post-fix, all cases return the expected status and differentiated messages, and the response shape (via |
…or-messages-274 # Conflicts: # main.py
|
Hi @hasankhan @Suhaibinator — circling back on this one — still no reviewer assigned, quiet for a while since the last activity. No pressure, just don't want it to get lost. |
Addresses #274 — scoped down from the full ask. The issue notes any error can collapse to a generic/misleading message; a full sweep of every error path in the API would be out of scope for one PR, so this fixes the 3 highest-value, most misleading cases:
x-aws-secretgate (verify_secret) —abort(401)with no description returned Werkzeug's generic default message ("could not verify... wrong credentials...") for both a missing header and a wrong header value. Now returns:401 "Missing 'x-aws-secret' header."when the header is absent401 "Invalid 'x-aws-secret' header value."when it's present but wrongPagination params (
limit/pageinpaginate_results) — a non-integer?limit=or?page=raised an uncaughtValueError, producing an unformatted 500 instead of a JSON error. Now validated and aborted with400and a clear message naming the offending parameter and value.chapterIdquery param (/v1/hadiths) — a non-numeric?chapterId=raised an uncaughtValueError(500) instead of a controlled400. Now validated the same way as the existingurns/refsparsing already does elsewhere in the file.Left out of scope (explicitly, since #274 is broader than a single fix):
main.py(e.g. infra/gateway-level messages upstream of this Flask app).first_or_404()on collection/book/chapter/hadith lookups — these already report the correct status and aren't misleading, just generic; changing their wording would be a broader UX pass, not a bug fix.Response shape is unchanged (
{"error": {"details": ..., "code": ...}}via the existingjsonify_http_errorhandler) — only message content/differentiation changed.Test plan
limit, invalidpage, invalidchapterId, validchapterId— all returned the expected status code and message.python3 -m py_compile main.pypasses.