debef39a18
Wave 3. Six findings, one cause: `int(request.args.get('typeid'))` with nothing
catching the ValueError. The caller sent a bad argument and got
500 INTERNAL_ERROR plus a traceback in the log, which says the server is broken
when the truth is that 'abc' is not a number.
shopdb/utils/queryargs.intarg reads it and raises QueryArgError, which an
app-level handler answers as a 400 naming the argument. App-level on purpose: a
route should not have to remember, and forgetting was the entire defect. The
policy differs from get_pagination_params, which falls back to a default for a
bad page number - a nonsense page is harmless and the caller still wants their
list, but silently ignoring a nonsense FILTER returns rows they believe they
excluded.
The gate matters more than the six fixes. tests/test_core/test_bad_query_args.py
sweeps all 131 registered no-parameter GET routes under /api/ with twelve
argument shapes and fails on any 5xx. It found sites the test run had not
reported - statusid and locationid on /api/assets, the second asset-list route,
limit on the employee lookup and the printer supply history - because the
pattern is how a list filter gets written rather than a property of any one
route. Verified non-vacuous by planting the old `int(...)` back into
/api/machines: four tests fail.
Four usb routes are skipped by exact path, with the reason: they reach the
EXTERNAL cmmc_usb database, so their 500 in a test environment is a missing
database rather than an argument fault. That is the complete set - every usb
GET route calling cmmc_usb_connection - not a convenient subset.
One correction worth recording: the first pass rewrote every
`if x := request.args.get(...)` in the touched files, including text arguments,
so a search for 'ZZNOTHINGMATCHES' briefly became a 400. Caught by
test_list_search_identifiers. Only id-shaped arguments are integers.
Scope: product
65 lines
2.3 KiB
Python
65 lines
2.3 KiB
Python
"""Reading a typed value out of the query string, without a 500.
|
|
|
|
WHY THIS EXISTS
|
|
|
|
List endpoints filtered like this:
|
|
|
|
if type_id := request.args.get('typeid'):
|
|
query = query.filter(Machine.machinetypeid == int(type_id))
|
|
|
|
`int('abc')` raises, nothing catches it, and the caller gets
|
|
500 INTERNAL_ERROR with a traceback in the log. A wrong query argument is the
|
|
caller's mistake and deserves a 400 saying which argument; a 500 says the server
|
|
is broken and sends whoever is reading the log looking for a fault that is not
|
|
there. The feature test run found the same shape in six places across five
|
|
plugins, which makes it a property of how list routes are written rather than of
|
|
any one route.
|
|
|
|
The helpers raise QueryArgError. Routes turn it into a 400 - or, more usually,
|
|
nothing does anything at all, because the app-level handler in
|
|
shopdb/__init__.py answers it. That is the point: a route should not have to
|
|
remember.
|
|
|
|
Deliberately NOT the same policy as get_pagination_params, which falls back to
|
|
a default for a bad page number. A nonsense page is harmless and the caller
|
|
still wants their list; a nonsense FILTER is not, because silently ignoring it
|
|
returns rows the caller believes they excluded.
|
|
"""
|
|
|
|
from flask import request
|
|
|
|
|
|
class QueryArgError(ValueError):
|
|
"""A query argument was present but not of the type the route needs."""
|
|
|
|
def __init__(self, name, value, expected='a whole number'):
|
|
self.name = name
|
|
self.value = value
|
|
super().__init__('%s must be %s (got %r)' % (name, expected, value))
|
|
|
|
|
|
def intarg(name, default=None, request_obj=None):
|
|
"""Read an integer query argument.
|
|
|
|
Absent or empty -> default. Present but not an integer -> QueryArgError,
|
|
which the app answers as a 400 naming the argument.
|
|
"""
|
|
source = request_obj if request_obj is not None else request
|
|
raw = source.args.get(name)
|
|
if raw is None or raw == '':
|
|
return default
|
|
try:
|
|
return int(raw)
|
|
except (TypeError, ValueError):
|
|
raise QueryArgError(name, raw)
|
|
|
|
|
|
def intargs(*names, request_obj=None):
|
|
"""Read several integer arguments at once; returns a dict of those present."""
|
|
found = {}
|
|
for name in names:
|
|
value = intarg(name, request_obj=request_obj)
|
|
if value is not None:
|
|
found[name] = value
|
|
return found
|