From 30d0d4f6d588d5acdeff34fb15b835bd22e9bea0 Mon Sep 17 00:00:00 2001 From: cproudlock Date: Thu, 3 Sep 2026 16:49:55 -0400 Subject: [PATCH] Stop asking for more rows than the server will ever return You are right that this is the one we keep hitting. perpage is capped at 100 server-side, and the cap is applied SILENTLY: a request for 500 returns the first 100 rows with a 200, so a picker built that way shows the first 100 options and drops the rest with nothing anywhere reporting a problem. The missing vendor simply is not in the list. Twelve call sites were still doing it - vendor and business-unit pickers on the machine and measuring-tool forms, and the type lists behind PCs, machines, printers, network devices, measuring tools and configuration policies. Each now either pages through with listAll()/fetchAllPages() or asks for what it can actually have. vendorsApi and businessunitsApi gained listAll(), which they lacked, which is why those two sites reached for 500 in the first place. The gate is the point. tests/test_core/test_perpage_cap.py reads every .vue and .js source under frontend/src and plugins/*/frontend and fails on any perpage over 100. It has to read the SOURCE rather than the running app, because the response is a valid 200 either way - no runtime assertion can see this one. Verified by planting a perpage: 500 back into MachineForm: the gate names that exact line. Separately, /api/auditlogs built its own meta with the pagination fields flat and a 'pages' key, while every other list returns meta.pagination.{page,perpage,total,totalpages,hasnext,hasprev}. A client written against the documented contract found nothing where it looked, on the one list whose whole purpose is being read by somebody checking what happened. It uses paginated_response now. Also carried here, from the previous batch's working tree: Asset.to_dict emits ownlocationid/ownmapx/ownmapy/ownlevelid alongside the resolved values, and the PC form binds to those. The merge of an inherited location is right for a reader and wrong for a form, which could not tell an owned value from a derived one and wrote the inherited one back as the record's own. Scope: product --- frontend/src/api/index.js | 10 +++ .../views/ConfigurationPoliciesList.vue | 4 +- .../computers/frontend/views/PCTypesList.vue | 2 +- .../machines/frontend/views/MachineForm.vue | 4 +- .../frontend/views/MachineTypesList.vue | 2 +- .../frontend/views/MeasuringToolForm.vue | 4 +- .../frontend/views/MeasuringToolTypesList.vue | 2 +- .../frontend/views/MeasuringToolsList.vue | 2 +- .../frontend/views/NetworkTypesList.vue | 2 +- .../frontend/views/PrinterTypesList.vue | 2 +- shopdb/core/api/auditlogs.py | 18 ++--- tests/test_core/test_perpage_cap.py | 79 +++++++++++++++++++ 12 files changed, 109 insertions(+), 22 deletions(-) create mode 100644 tests/test_core/test_perpage_cap.py diff --git a/frontend/src/api/index.js b/frontend/src/api/index.js index 89029cc0..6c7a0da0 100644 --- a/frontend/src/api/index.js +++ b/frontend/src/api/index.js @@ -405,6 +405,12 @@ export const vendorsApi = { list(params = {}) { return api.get('/vendors', { params }) }, + // Every vendor, paged. perpage is capped at 100 server-side, so a picker + // asking for 500 silently showed the first 100 and dropped the rest - with a + // 200, so nothing looked wrong. + listAll(params = {}) { + return fetchAllPages('/vendors', params) + }, get(id) { return api.get(`/vendors/${id}`) }, @@ -667,6 +673,10 @@ export const businessunitsApi = { list(params = {}) { return api.get('/businessunits', { params }) }, + // See vendorsApi.listAll: the cap is 100 whatever the caller asks for. + listAll(params = {}) { + return fetchAllPages('/businessunits', params) + }, get(id) { return api.get(`/businessunits/${id}`) }, diff --git a/plugins/computers/frontend/views/ConfigurationPoliciesList.vue b/plugins/computers/frontend/views/ConfigurationPoliciesList.vue index 24c37ab4..8e014be7 100644 --- a/plugins/computers/frontend/views/ConfigurationPoliciesList.vue +++ b/plugins/computers/frontend/views/ConfigurationPoliciesList.vue @@ -145,8 +145,8 @@ async function loadData() { // 500'd, which looked like data loss. const [pol, types, appList, actions, monitorList] = await Promise.allSettled([ computersApi.policies.configuration.list(), - computersApi.types.list({ perpage: 200 }), - applicationsApi.listAll({ perpage: 200 }), + computersApi.types.listAll(), + applicationsApi.listAll(), computersApi.policies.actions(), endpointsApi.monitors.list() ]) diff --git a/plugins/computers/frontend/views/PCTypesList.vue b/plugins/computers/frontend/views/PCTypesList.vue index 453a011f..5afef7e4 100644 --- a/plugins/computers/frontend/views/PCTypesList.vue +++ b/plugins/computers/frontend/views/PCTypesList.vue @@ -107,7 +107,7 @@ onMounted(() => loadData()) async function loadData() { loading.value = true try { - const response = await computersApi.types.list({ perpage: 200, active: false }) + const response = await computersApi.types.listAll({ active: false }) pcTypes.value = response.data.data || [] } catch (err) { console.error('Error loading PC types:', err) diff --git a/plugins/machines/frontend/views/MachineForm.vue b/plugins/machines/frontend/views/MachineForm.vue index 6350c449..bce1693a 100644 --- a/plugins/machines/frontend/views/MachineForm.vue +++ b/plugins/machines/frontend/views/MachineForm.vue @@ -581,10 +581,10 @@ onMounted(async () => { const [typesRes, statusRes, vendorRes, locRes, allModels, buRes, pcsRes, relTypesRes] = await Promise.all([ machinesApi.types.list(), assetsApi.statuses.list(), - vendorsApi.list({ perpage: 500 }), + vendorsApi.listAll(), locationsApi.listAll(), // backend caps perpage at 100; page through all modelsApi.listAll({ category: 'Equipment' }), // backend caps perpage at 100; page through all - businessunitsApi.list({ perpage: 500 }), + businessunitsApi.listAll(), computersApi.listAll(), // backend caps perpage at 100; page through all assetsApi.types.list() // Used for relationship types, will fix below ]) diff --git a/plugins/machines/frontend/views/MachineTypesList.vue b/plugins/machines/frontend/views/MachineTypesList.vue index bc69909b..d19f5eeb 100644 --- a/plugins/machines/frontend/views/MachineTypesList.vue +++ b/plugins/machines/frontend/views/MachineTypesList.vue @@ -95,7 +95,7 @@ onMounted(loadData) async function loadData() { loading.value = true try { - const response = await machinesApi.types.list({ perpage: 200, active: false }) + const response = await machinesApi.types.list({ perpage: 100, active: false }) items.value = response.data.data || [] } catch (err) { console.error('Error loading machine types:', err) diff --git a/plugins/measuringtools/frontend/views/MeasuringToolForm.vue b/plugins/measuringtools/frontend/views/MeasuringToolForm.vue index bb26a61b..5b18eb76 100644 --- a/plugins/measuringtools/frontend/views/MeasuringToolForm.vue +++ b/plugins/measuringtools/frontend/views/MeasuringToolForm.vue @@ -179,10 +179,10 @@ const businessunits = ref([]) onMounted(async () => { try { const [typesResponse, statusResponse, locResponse, buResponse, assetTypesResponse] = await Promise.all([ - measuringtoolsApi.types.list({ perpage: 200 }), + measuringtoolsApi.types.list({ perpage: 100 }), assetsApi.statuses.list(), locationsApi.listAll(), // backend caps perpage at 100; page through all - businessunitsApi.list({ perpage: 500 }), + businessunitsApi.listAll(), assetsApi.types.list(), ]) types.value = typesResponse.data.data || [] diff --git a/plugins/measuringtools/frontend/views/MeasuringToolTypesList.vue b/plugins/measuringtools/frontend/views/MeasuringToolTypesList.vue index b16919c1..559f7b40 100644 --- a/plugins/measuringtools/frontend/views/MeasuringToolTypesList.vue +++ b/plugins/measuringtools/frontend/views/MeasuringToolTypesList.vue @@ -107,7 +107,7 @@ onMounted(() => loadData()) async function loadData() { loading.value = true try { - const response = await measuringtoolsApi.types.list({ perpage: 200, active: false }) + const response = await measuringtoolsApi.types.list({ perpage: 100, active: false }) types.value = response.data.data || [] } catch (err) { console.error('Error loading measuring tool types:', err) diff --git a/plugins/measuringtools/frontend/views/MeasuringToolsList.vue b/plugins/measuringtools/frontend/views/MeasuringToolsList.vue index 14be7716..52d18bf5 100644 --- a/plugins/measuringtools/frontend/views/MeasuringToolsList.vue +++ b/plugins/measuringtools/frontend/views/MeasuringToolsList.vue @@ -134,7 +134,7 @@ function formatDate(d) { return new Date(d + 'T00:00:00').toLocaleDateString() } onMounted(async () => { try { - const typesResponse = await measuringtoolsApi.types.list({ perpage: 200 }) + const typesResponse = await measuringtoolsApi.types.list({ perpage: 100 }) types.value = typesResponse.data.data || [] } catch (err) { console.error('Error loading types:', err) diff --git a/plugins/network/frontend/views/NetworkTypesList.vue b/plugins/network/frontend/views/NetworkTypesList.vue index cb430bab..85ee1e2c 100644 --- a/plugins/network/frontend/views/NetworkTypesList.vue +++ b/plugins/network/frontend/views/NetworkTypesList.vue @@ -112,7 +112,7 @@ onMounted(loadData) async function loadData() { loading.value = true try { - const response = await networkApi.types.list({ perpage: 200, active: false }) + const response = await networkApi.types.list({ perpage: 100, active: false }) items.value = response.data.data || [] } catch (err) { console.error('Error loading network device types:', err) diff --git a/plugins/printers/frontend/views/PrinterTypesList.vue b/plugins/printers/frontend/views/PrinterTypesList.vue index 40c4241b..aad5b1d7 100644 --- a/plugins/printers/frontend/views/PrinterTypesList.vue +++ b/plugins/printers/frontend/views/PrinterTypesList.vue @@ -95,7 +95,7 @@ onMounted(loadData) async function loadData() { loading.value = true try { - const response = await printersApi.types.list({ perpage: 200, active: false }) + const response = await printersApi.types.list({ perpage: 100, active: false }) items.value = response.data.data || [] } catch (err) { console.error('Error loading printer types:', err) diff --git a/shopdb/core/api/auditlogs.py b/shopdb/core/api/auditlogs.py index 544fa78f..55ccf238 100644 --- a/shopdb/core/api/auditlogs.py +++ b/shopdb/core/api/auditlogs.py @@ -5,7 +5,8 @@ from flask_jwt_extended import jwt_required from shopdb.core.models import AuditLog from shopdb.extensions import db -from shopdb.utils.responses import success_response, error_response, ErrorCodes +from shopdb.utils.responses import (success_response, error_response, + paginated_response, ErrorCodes) from shopdb.utils.authz import require_permission auditlogs_bp = Blueprint('auditlogs', __name__) @@ -123,15 +124,12 @@ def list_auditlogs(): for r in rows: r['userfullname'] = fullnames.get(str(r.get('username'))) - return success_response( - rows, - meta={ - 'page': page, - 'perpage': perpage, - 'total': pagination.total, - 'pages': pagination.pages - } - ) + # paginated_response, like every other list in the product. This one built + # its own meta with the fields FLAT and named 'pages', so a client written + # against the documented meta.pagination.{page,perpage,total,totalpages} + # found nothing where it looked - on the one list whose whole purpose is to + # be read by somebody checking what happened. + return paginated_response(rows, page, perpage, pagination.total) @auditlogs_bp.route('/entity//', methods=['GET']) diff --git a/tests/test_core/test_perpage_cap.py b/tests/test_core/test_perpage_cap.py new file mode 100644 index 00000000..45f63dcf --- /dev/null +++ b/tests/test_core/test_perpage_cap.py @@ -0,0 +1,79 @@ +"""No client may ask for a page bigger than the server will give. + +MAX_PAGE_SIZE caps perpage at 100. The cap is deliberate, and it is applied +SILENTLY: a request for perpage=500 returns the first 100 rows with a 200, so +nothing anywhere reports a problem. A picker built that way shows the first 100 +options and drops the rest, and the person using it has no way to tell - the +missing vendor simply is not in the list. + +That shape has been found and fixed more than once, which is what makes it worth +a gate rather than a comment. Paging through is what fetchAllPages/listAll are +for. + +The check reads the frontend sources rather than the running app, because the +defect is in what the caller ASKS for: the response is a valid 200 either way, +so no runtime assertion can see it. +""" +import re +from pathlib import Path + +REPO = Path(__file__).resolve().parents[2] + +# Where a client is written. The staged copy under frontend/src/.plugins-staged +# is generated from plugins/*/frontend at build time, so checking the source +# avoids reporting the same line twice. +SOURCE_DIRS = [ + REPO / 'frontend' / 'src', + *sorted((REPO / 'plugins').glob('*/frontend')), +] + +SKIP_PARTS = {'.plugins-staged', 'node_modules', 'dist'} + +# perpage: , in an object literal or a query string. +PERPAGE = re.compile(r'perpage[\'"]?\s*[:=]\s*[\'"]?(\d+)', re.IGNORECASE) + +MAX_PAGE_SIZE = 100 + + +def _sources(): + for root in SOURCE_DIRS: + if not root.exists(): + continue + for path in root.rglob('*'): + if path.suffix not in ('.vue', '.js'): + continue + if set(path.parts) & SKIP_PARTS: + continue + if path.name.endswith('.spec.js'): + continue + yield path + + +def test_the_sweep_actually_reads_something(): + """A gate that matches nothing passes forever.""" + files = list(_sources()) + assert len(files) > 100, len(files) + assert any(PERPAGE.search(path.read_text()) for path in files), \ + 'no perpage usage found at all - the pattern is probably wrong' + + +def test_no_client_asks_for_more_rows_than_the_server_returns(): + offenders = [] + for path in _sources(): + for lineno, line in enumerate(path.read_text().splitlines(), start=1): + # A comment explaining the cap is not a caller asking for 1000 + # rows. Crude but sufficient: this is a line-oriented check over + # source that follows one style. + stripped = line.strip() + if stripped.startswith('//') or stripped.startswith('*'): + continue + for match in PERPAGE.finditer(line): + if int(match.group(1)) > MAX_PAGE_SIZE: + offenders.append('%s:%d %s' + % (path.relative_to(REPO), lineno, + line.strip()[:90])) + assert not offenders, ( + 'perpage is capped at %d server-side and the excess is dropped ' + 'silently, so these show a truncated list with no error. Use listAll() ' + '/ fetchAllPages() instead:\n %s' + % (MAX_PAGE_SIZE, '\n '.join(offenders)))