Answer API errors in the envelope, say when supplies are unread, badge each chain
Three more findings from the 2026-09-03 run.
Werkzeug answered 405, 415 and a malformed JSON body itself, in HTML. A client
written against the documented {status,data,meta} contract got a page of markup
with no error code, which is indistinguishable from the server having fallen
over. The 405 had a second edge: on a route the caller cannot otherwise see, a
method-not-allowed CONFIRMS the path exists, so a gated kiosk route meant to
read as absent announced itself by refusing the wrong verb. All three now
answer in the envelope, and only under /api/ - everything else is the SPA and
keeps the framework's behaviour, deep links included.
The Toner Report rendered "0 Printers Checked / 0 Low / 0 Critical" and "No
printers match the selected filter" while the API it had just called said
supply monitoring was unreachable. Zeros read as a clean bill of health for a
fleet nobody has looked at. The page now carries the reason the API gave and
suppresses the counts, because a number nobody measured is not a result. Same
defect shape as the supplies card fixed on 2026-08-29; `unavailable` was
already in the payload and the page ignored it.
Backup revisions badged "current" per (asset, kind). NTLARS settings describe a
MACHINE but are read off the controlling PC, so a machine with two reporting
PCs has two independent chains of the same kind - and only the higher revision
id was badged, so the second PC's newest revision showed as history and lost
its last-verified stamp. The machine looked like one source re-reporting rather
than two. sourcehostname is the chain; a revision without one forms its own,
which keeps the old behaviour for everything that has only ever had one source.
Nine tests. The envelope ones verified by planting: remove the three handlers
and three fail.
Scope: product
This commit is contained in:
@@ -116,13 +116,25 @@ def asset_revisions(assetid):
|
|||||||
|
|
||||||
revisions = query.order_by(BackupRevision.backuprevisionid.desc()).all()
|
revisions = query.order_by(BackupRevision.backuprevisionid.desc()).all()
|
||||||
|
|
||||||
# "Latest" is per kind, not per asset, so an unfiltered listing still marks
|
# "Latest" is per kind AND per source chain. Per kind alone was wrong on
|
||||||
# the current revision of each kind correctly.
|
# the case this exists for: NTLARS settings describe a machine but are read
|
||||||
|
# off the controlling PC, so a machine with two reporting PCs has two
|
||||||
|
# independent chains of the same kind. Marking only the highest revision id
|
||||||
|
# meant the second PC's current revision was badged as history and lost its
|
||||||
|
# "last verified" stamp - the machine looked like it had one source that
|
||||||
|
# kept re-reporting, rather than two.
|
||||||
|
#
|
||||||
|
# sourcehostname is the chain: it records which PC produced the revision.
|
||||||
|
# A revision with none (an older row, or a share-drop with no host) forms
|
||||||
|
# its own chain under the empty key, which keeps the per-kind behaviour for
|
||||||
|
# everything that has only ever had one source.
|
||||||
seen = set()
|
seen = set()
|
||||||
out = []
|
out = []
|
||||||
for revision in revisions:
|
for revision in revisions:
|
||||||
islatest = revision.backupkind not in seen
|
chain = (revision.backupkind,
|
||||||
seen.add(revision.backupkind)
|
(revision.sourcehostname or '').strip().lower())
|
||||||
|
islatest = chain not in seen
|
||||||
|
seen.add(chain)
|
||||||
out.append(_summary(revision, islatest=islatest))
|
out.append(_summary(revision, islatest=islatest))
|
||||||
return success_response(out)
|
return success_response(out)
|
||||||
|
|
||||||
|
|||||||
@@ -17,8 +17,18 @@
|
|||||||
</div>
|
</div>
|
||||||
|
|
||||||
<template v-else>
|
<template v-else>
|
||||||
|
<!-- Cannot see the supplies: say so instead of reporting zeros. -->
|
||||||
|
<div v-if="unavailable" class="card blind-notice">
|
||||||
|
<strong>Supply levels are not being read.</strong>
|
||||||
|
<p>{{ unavailable }}</p>
|
||||||
|
<p class="muted">
|
||||||
|
The counts below are not a result - nothing was measured. Check the
|
||||||
|
supply monitoring configuration in Settings.
|
||||||
|
</p>
|
||||||
|
</div>
|
||||||
|
|
||||||
<!-- Summary Bar -->
|
<!-- Summary Bar -->
|
||||||
<div class="summary-bar">
|
<div v-if="!unavailable" class="summary-bar">
|
||||||
<div class="summary-stat card">
|
<div class="summary-stat card">
|
||||||
<div class="stat-value">{{ summary.total_checked }}</div>
|
<div class="stat-value">{{ summary.total_checked }}</div>
|
||||||
<div class="stat-label">Printers Checked</div>
|
<div class="stat-label">Printers Checked</div>
|
||||||
@@ -122,6 +132,9 @@
|
|||||||
</tr>
|
</tr>
|
||||||
</tbody>
|
</tbody>
|
||||||
</table>
|
</table>
|
||||||
|
<p v-else-if="unavailable" class="empty-state">
|
||||||
|
No supply data was read, so there is nothing to list.
|
||||||
|
</p>
|
||||||
<p v-else class="empty-state">No printers match the selected filter.</p>
|
<p v-else class="empty-state">No printers match the selected filter.</p>
|
||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
@@ -156,6 +169,7 @@ const loading = ref(true)
|
|||||||
const error = ref(null)
|
const error = ref(null)
|
||||||
const printers = ref([])
|
const printers = ref([])
|
||||||
const summary = ref({ total_checked: 0, low: 0, critical: 0 })
|
const summary = ref({ total_checked: 0, low: 0, critical: 0 })
|
||||||
|
const unavailable = ref('')
|
||||||
const filter = ref('all')
|
const filter = ref('all')
|
||||||
|
|
||||||
const filterOptions = [
|
const filterOptions = [
|
||||||
@@ -239,6 +253,11 @@ onMounted(async () => {
|
|||||||
const data = response.data.data
|
const data = response.data.data
|
||||||
printers.value = data.printers || []
|
printers.value = data.printers || []
|
||||||
summary.value = data.summary || { total_checked: 0, low: 0, critical: 0 }
|
summary.value = data.summary || { total_checked: 0, low: 0, critical: 0 }
|
||||||
|
// The API says so explicitly when it cannot see supplies at all. Without
|
||||||
|
// this the page rendered the zeros it was given - "0 Printers Checked,
|
||||||
|
// 0 Low, 0 Critical" and "No printers match the selected filter" - which
|
||||||
|
// reads as a clean bill of health for a fleet nobody has looked at.
|
||||||
|
unavailable.value = data.unavailable || ''
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
console.error('Error loading toner report:', err)
|
console.error('Error loading toner report:', err)
|
||||||
error.value = 'Failed to load supply data. Zabbix may not be configured or reachable.'
|
error.value = 'Failed to load supply data. Zabbix may not be configured or reachable.'
|
||||||
@@ -249,6 +268,13 @@ onMounted(async () => {
|
|||||||
</script>
|
</script>
|
||||||
|
|
||||||
<style scoped>
|
<style scoped>
|
||||||
|
.blind-notice {
|
||||||
|
border-left: 3px solid var(--warning);
|
||||||
|
margin-bottom: 1rem;
|
||||||
|
}
|
||||||
|
.blind-notice p {
|
||||||
|
margin: 0.4rem 0 0 0;
|
||||||
|
}
|
||||||
.summary-bar {
|
.summary-bar {
|
||||||
display: flex;
|
display: flex;
|
||||||
gap: 1.5rem;
|
gap: 1.5rem;
|
||||||
|
|||||||
@@ -435,6 +435,48 @@ def register_error_handlers(app: Flask):
|
|||||||
http_code=404
|
http_code=404
|
||||||
)
|
)
|
||||||
|
|
||||||
|
# Werkzeug answers these three itself, in HTML, so a JSON client asking a
|
||||||
|
# question slightly wrong got a page of markup with no error code and no
|
||||||
|
# envelope - and on a route the caller cannot see, the 405 also confirmed
|
||||||
|
# that the path exists, which is how a gated kiosk route meant to look
|
||||||
|
# absent announced itself.
|
||||||
|
#
|
||||||
|
# Only under /api/. Everything else is the SPA, which needs the framework's
|
||||||
|
# own behaviour (and its own catch-all) left alone.
|
||||||
|
from flask import request as flask_request
|
||||||
|
|
||||||
|
def _api_request():
|
||||||
|
return flask_request.path.startswith('/api/')
|
||||||
|
|
||||||
|
@app.errorhandler(405)
|
||||||
|
def method_not_allowed_error(error):
|
||||||
|
if not _api_request():
|
||||||
|
return error
|
||||||
|
return error_response(
|
||||||
|
ErrorCodes.VALIDATION_ERROR,
|
||||||
|
'That method is not allowed on this endpoint.',
|
||||||
|
http_code=405)
|
||||||
|
|
||||||
|
@app.errorhandler(415)
|
||||||
|
def unsupported_media_type_error(error):
|
||||||
|
if not _api_request():
|
||||||
|
return error
|
||||||
|
return error_response(
|
||||||
|
ErrorCodes.VALIDATION_ERROR,
|
||||||
|
'This endpoint expects a JSON body; send '
|
||||||
|
'Content-Type: application/json.',
|
||||||
|
http_code=415)
|
||||||
|
|
||||||
|
@app.errorhandler(400)
|
||||||
|
def bad_request_error(error):
|
||||||
|
if not _api_request():
|
||||||
|
return error
|
||||||
|
# Werkzeug raises this for malformed JSON before any route sees it.
|
||||||
|
return error_response(
|
||||||
|
ErrorCodes.VALIDATION_ERROR,
|
||||||
|
'The request body could not be read as JSON.',
|
||||||
|
http_code=400)
|
||||||
|
|
||||||
@app.errorhandler(500)
|
@app.errorhandler(500)
|
||||||
def internal_error(error):
|
def internal_error(error):
|
||||||
return error_response(
|
return error_response(
|
||||||
|
|||||||
66
tests/test_core/test_error_envelope.py
Normal file
66
tests/test_core/test_error_envelope.py
Normal file
@@ -0,0 +1,66 @@
|
|||||||
|
"""Every /api/ error answers in the documented envelope.
|
||||||
|
|
||||||
|
docs/API-REFERENCE.md says an error is
|
||||||
|
{"status":"error","data":{"error":{"code","message"}},"meta":{...}}. Three
|
||||||
|
statuses came straight from Werkzeug as HTML instead:
|
||||||
|
|
||||||
|
405 a wrong method - and on a route the caller cannot otherwise see, the
|
||||||
|
405 also CONFIRMED the path exists, so a gated kiosk route meant to
|
||||||
|
read as absent announced itself by refusing the wrong verb
|
||||||
|
415 a body sent without Content-Type: application/json
|
||||||
|
400 a body that is not parseable JSON
|
||||||
|
|
||||||
|
A client written against the contract gets a page of markup with no error code,
|
||||||
|
which is indistinguishable from the server having fallen over.
|
||||||
|
|
||||||
|
Outside /api/ the framework's own behaviour is left alone: that is the SPA, and
|
||||||
|
it has its own catch-all.
|
||||||
|
"""
|
||||||
|
|
||||||
|
|
||||||
|
def _is_envelope(response):
|
||||||
|
body = response.get_json(silent=True)
|
||||||
|
if not body:
|
||||||
|
return False
|
||||||
|
return (body.get('status') == 'error'
|
||||||
|
and 'error' in body.get('data', {})
|
||||||
|
and 'code' in body['data']['error']
|
||||||
|
and 'message' in body['data']['error']
|
||||||
|
and 'meta' in body)
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_wrong_method_answers_in_the_envelope(client):
|
||||||
|
response = client.delete('/api/assets/types')
|
||||||
|
assert response.status_code == 405
|
||||||
|
assert response.content_type.startswith('application/json'), \
|
||||||
|
response.content_type
|
||||||
|
assert _is_envelope(response), response.get_data(as_text=True)[:200]
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_body_with_the_wrong_content_type_answers_in_the_envelope(client):
|
||||||
|
response = client.post('/api/auth/login', data='not json',
|
||||||
|
content_type='text/plain')
|
||||||
|
assert response.status_code == 415
|
||||||
|
assert _is_envelope(response), response.get_data(as_text=True)[:200]
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_malformed_json_body_answers_in_the_envelope(client):
|
||||||
|
response = client.post('/api/auth/login', data='{"username": ',
|
||||||
|
content_type='application/json')
|
||||||
|
assert response.status_code == 400
|
||||||
|
assert _is_envelope(response), response.get_data(as_text=True)[:200]
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_missing_api_route_still_answers_in_the_envelope(client):
|
||||||
|
response = client.get('/api/nothing-here-at-all')
|
||||||
|
assert response.status_code == 404
|
||||||
|
assert _is_envelope(response)
|
||||||
|
|
||||||
|
|
||||||
|
def test_the_spa_catch_all_is_untouched(client):
|
||||||
|
"""A browser asking for an app route must still get the app, not JSON.
|
||||||
|
Enveloping everything would have broken deep links."""
|
||||||
|
response = client.get('/pcs/1')
|
||||||
|
assert response.status_code in (200, 404)
|
||||||
|
if response.status_code == 200:
|
||||||
|
assert not response.content_type.startswith('application/json')
|
||||||
@@ -1525,3 +1525,46 @@ def test_secret_name_matching_covers_the_usual_spellings():
|
|||||||
assert issecretname(name), name
|
assert issecretname(name), name
|
||||||
for name in ('FtpUser', 'FMSHost', 'Retries', 'Passive'):
|
for name in ('FtpUser', 'FMSHost', 'Retries', 'Passive'):
|
||||||
assert not issecretname(name), name
|
assert not issecretname(name), name
|
||||||
|
|
||||||
|
|
||||||
|
# --- "current" is per source chain, not per kind -----------------------------
|
||||||
|
#
|
||||||
|
# NTLARS settings describe a MACHINE but are read off the controlling PC, so a
|
||||||
|
# machine with two reporting PCs has two independent chains of the same kind.
|
||||||
|
# islatest was computed per kind, so only the highest revision id was badged
|
||||||
|
# current: the second PC's newest revision showed as history and lost its "last
|
||||||
|
# verified" stamp, making the machine look like one source re-reporting rather
|
||||||
|
# than two.
|
||||||
|
|
||||||
|
def test_each_reporting_pc_keeps_its_own_current_revision(bk_app, bk_plugin):
|
||||||
|
from plugins.backups.api.routes import _summary
|
||||||
|
from plugins.backups.models import BackupRevision
|
||||||
|
|
||||||
|
with bk_app.app_context():
|
||||||
|
# Two PCs reporting the same machine, each twice.
|
||||||
|
bk_plugin.apply_collector_payload(_payload())
|
||||||
|
changed = CONFIGUREDREG.replace('"Cnc"=""', '"Cnc"="OKUMA"')
|
||||||
|
bk_plugin.apply_collector_payload(_payload(reg=changed))
|
||||||
|
bk_plugin.apply_collector_payload(
|
||||||
|
_payload(sourcehostname='NEWPC001'))
|
||||||
|
again = CONFIGUREDREG.replace('"Cnc"=""', '"Cnc"="MAZAK"')
|
||||||
|
bk_plugin.apply_collector_payload(
|
||||||
|
_payload(reg=again, sourcehostname='NEWPC001'))
|
||||||
|
|
||||||
|
revisions = (_db.session.query(BackupRevision)
|
||||||
|
.order_by(BackupRevision.backuprevisionid.desc()).all())
|
||||||
|
assert len(revisions) >= 4
|
||||||
|
|
||||||
|
# Mirror what the listing route does.
|
||||||
|
seen = set()
|
||||||
|
current = {}
|
||||||
|
for revision in revisions:
|
||||||
|
chain = (revision.backupkind,
|
||||||
|
(revision.sourcehostname or '').strip().lower())
|
||||||
|
if chain not in seen:
|
||||||
|
seen.add(chain)
|
||||||
|
current[revision.sourcehostname] = revision.backuprevisionid
|
||||||
|
|
||||||
|
assert set(current) == {'GGBX0NH3ESF', 'NEWPC001'}, \
|
||||||
|
'each reporting PC has a current revision of its own'
|
||||||
|
assert _summary(revisions[0], islatest=True)['islatest'] is True
|
||||||
|
|||||||
Reference in New Issue
Block a user