From 12fa9de27d59049e980fc434c0837aa49968ad25 Mon Sep 17 00:00:00 2001 From: ch <1938389+netravnen@users.noreply.github.com> Date: Wed, 19 Aug 2026 20:27:14 +0200 Subject: [PATCH] fix(cp): make pdbFetch throw instead of returning null pdbFetch resolved to null for every failure mode -- HTTP error, JSON parse failure, transport error and timeout alike. Because null is also a plausible "nothing here" value, that silently turned the error handling written around it into dead code. fetchRecentNetixlanChanges shows the cost. Its first try block always reached its return, so both its catch and the entire client-side filtering fallback beneath it were unreachable. A rejected updated__gte filter therefore reported "0 rows, no error" -- an admin verifying an IXLAN renumber would read that as "nothing changed" rather than "the check did not run". The code was written expecting pdbFetch to throw; the null return was the deviation, so this restores the surrounding design rather than imposing a new one. Changes: - Added makePdbFetchError(url, detail), producing a PdbFetchError carrying name/status/url/detail so callers can branch on the failure rather than guess at it. - All seven null exits (three same-origin, four cross-origin) now throw or reject. recordFetchFailure diagnostics are unchanged; the same-origin catch rethrows an existing PdbFetchError untouched so status and type survive. - Reviewed all 21 call sites. Eighteen already had an enclosing catch that was dead code and is now live; each was read to confirm it does something sensible (they return error-carrying result objects). - Three callers deliberately want a sentinel, so they catch at their own boundary and say why: requestJson (getBootstrap carries an explicit "@ai Preserve ... null-on-failure" contract for graceful RDAP degradation), and the GraphQL/REST network-name batch fetchers, whose null and [] returns are fallback signals their paging loop depends on. - Guarded the networkixlan toolbar's fire-and-forget `void (async ...)()`, which had no handler at all. A throw there would have become an unhandled rejection; the org/ix links are decorative, so a failed lookup now logs via dbg and omits them, matching prior behavior. - Documented both rules in docs/CONVENTIONS.md. Security: - N/A directly, but it removes a class of silent failure in the flows that verify destructive work. The renumber verification report can no longer claim zero changes when it never successfully queried. Testing: - node --test from user.js/: 500 tests, 499 pass, 1 skipped (live tests are opt-in), up from 493/492. - Added tests/cp-pdbfetch-errors.test.js covering the typed rejection and its status/url/detail, that success still resolves to a parsed payload, and that the revived fallback both runs and reports source "client-filter" -- a path that could not previously execute. - Confirmed not vacuous: restoring the null return on HTTP error fails 6 of the 7 new cases. - Re-audited every call site afterwards; none is left without a handler. Backwards Compatibility: - Behavior on success is identical. - Behavior on failure changes by design: callers that previously saw null now see a rejection. Every site was inspected and either already handled it or was updated, so no caller observes a different outcome than before -- except fetchRecentNetixlanChanges, which now actually falls back. Assisted-by: Claude:claude-opus-5 --- user.js/docs/CONVENTIONS.md | 13 ++ .../peeringdb-cp-consolidated-tools.src.js | 172 +++++++++++++----- .../peeringdb-cp-consolidated-tools.user.js | 172 +++++++++++++----- user.js/tests/cp-pdbfetch-errors.test.js | 97 ++++++++++ 4 files changed, 368 insertions(+), 86 deletions(-) create mode 100644 user.js/tests/cp-pdbfetch-errors.test.js diff --git a/user.js/docs/CONVENTIONS.md b/user.js/docs/CONVENTIONS.md index 68fd004..1cde075 100644 --- a/user.js/docs/CONVENTIONS.md +++ b/user.js/docs/CONVENTIONS.md @@ -41,6 +41,19 @@ bundler/linter without discussing it first (see [AGENTS.md](../AGENTS.md) "Addit no-op unless `isDebugEnabled()` is true. Always-visible failure logs (meant to reach an admin even without debug mode on) use plain `console.error`/`console.warn` directly — this split was deliberately preserved during the debug-logging centralization. +- **Fetch helpers signal failure by throwing, not by returning a sentinel.** CP's `pdbFetch` + rejects with a `PdbFetchError` (`name`, `status`, `url`, `detail`) on HTTP error, parse failure, + transport error or timeout; a resolved value is always a parsed payload. It previously returned + `null` for all four, which silently made every `catch` around it unreachable — + `fetchRecentNetixlanChanges` had two, and its entire client-side fallback was dead code, so a + rejected API filter reported "0 rows, no error" to an admin verifying a renumber. Prefer throwing + for the same reason: a convention that lets correct-looking error handling be inert is worse than + no convention. Where a caller genuinely wants a sentinel — `requestJson` in the RDAP path and the + network-name batch fetchers — it catches at its own boundary and says so in a comment, so the + choice is visible rather than inherited. +- **Fire-and-forget async work must carry its own `try/catch`.** A `void (async () => { ... })()` + has nothing to reject into, so an uncaught error there becomes an unhandled rejection with no + context. See the `networkixlan` toolbar lookup in `peeringdb-cp-consolidated-tools.src.js`. - **Sensitive-data redaction**: no secrets are logged or stored — there are no API tokens/credentials anywhere in the codebase (PeeringDB auth is same-origin session cookies). - **Documentation convention**: functions carry a JSDoc block with `Purpose:`, `Necessity:`, and an diff --git a/user.js/peeringdb-cp-consolidated-tools.src.js b/user.js/peeringdb-cp-consolidated-tools.src.js index 74310cf..2e67891 100644 --- a/user.js/peeringdb-cp-consolidated-tools.src.js +++ b/user.js/peeringdb-cp-consolidated-tools.src.js @@ -4380,7 +4380,10 @@ if (!/^\d+$/.test(id)) return { rows: [], cutoffIso: "", source: "", error: "bad-ixlan-id" }; const cutoffMs = Date.now() - Math.max(1, Number(windowMinutes) || 60) * 60 * 1000; const cutoffIso = new Date(cutoffMs).toISOString().replace(/\.\d{3}Z$/, "Z"); - // Try server-side filter first. + // Try server-side filter first. Both catch blocks below were dead code while + // pdbFetch returned null: the try always reached its return, so a rejected + // updated__gte filter reported "0 rows, no error" and the client-filter + // fallback never ran. They are live now that pdbFetch throws. try { const serverUrl = `${PEERINGDB_API_BASE_URL}/netixlan?ixlan_id=${encodeURIComponent(id)}&updated__gte=${encodeURIComponent(cutoffIso)}&depth=0&limit=250`; const data = await pdbFetch(serverUrl); @@ -5679,6 +5682,27 @@ } } + /** + * Builds the error pdbFetch rejects with. + * Purpose: Give callers a single recognisable failure type carrying enough + * context to report or branch on, instead of an untyped null that is + * indistinguishable from a legitimately empty response. + * @param {string} url - Request URL that failed. + * @param {object} detail - Same shape passed to recordFetchFailure (type, + * mode, status, message, ...), attached for callers that want specifics. + * @returns {Error} Error with name "PdbFetchError" plus url/detail/status. + */ + function makePdbFetchError(url, detail) { + const status = detail && typeof detail.status === "number" ? detail.status : 0; + const suffix = status ? ` (HTTP ${status})` : ""; + const error = new Error(`pdbFetch ${detail?.type || "failed"}${suffix}: ${url}`); + error.name = "PdbFetchError"; + error.url = url; + error.status = status; + error.detail = detail || {}; + return error; + } + /** * Unified JSON fetch helper for all script-initiated HTTP requests. * Purpose: Single network abstraction with timeout, retry, and error normalisation @@ -5686,10 +5710,19 @@ * GM_xmlhttpRequest) call sites. * Necessity: Prevents N ad-hoc GM_xmlhttpRequest patterns from diverging on * timeout handling or header construction. + * + * THROWS on failure -- it does not return null. This previously resolved to + * null for every failure mode, which silently made `catch` blocks around it + * dead code: fetchRecentNetixlanChanges had two, and its entire client-side + * fallback was unreachable because the try block always returned. Callers + * must handle rejection; a resolved value is always a parsed payload. * @ai Preserve request retries/timeouts/error classification and payload assumptions. + * Do NOT reintroduce a null return -- callers rely on rejection to detect failure. * @param {string} url - Absolute URL to fetch. * @param {{ headers?: object, timeout?: number, retries?: number }} [options] - * @returns {Promise} Parsed JSON or null on any failure. + * @returns {Promise} Parsed JSON payload. + * @throws {Error} PdbFetchError (see makePdbFetchError) on HTTP error, parse + * failure, transport error or timeout. */ async function pdbFetch(url, { headers = {}, timeout = PDB_API_TIMEOUT_MS, retries = PDB_API_RETRIES } = {}) { const fullHeaders = buildTampermonkeyRequestHeaders(headers); @@ -5728,7 +5761,7 @@ clearFetchFailure(url); return parsed; } catch (parseError) { - recordFetchFailure(url, { + const detail = { type: "parse", mode: "same-origin", attempt: 1, @@ -5736,12 +5769,13 @@ status: response.status, statusText: response.statusText, message: String(parseError?.message || parseError || "json-parse-failed"), - }); - return null; + }; + recordFetchFailure(url, detail); + throw makePdbFetchError(url, detail); } } - recordFetchFailure(url, { + const httpDetail = { type: "http", mode: "same-origin", attempt: 1, @@ -5749,23 +5783,29 @@ status: response.status, statusText: response.statusText, ok: false, - }); - return null; - } catch (_err) { - recordFetchFailure(url, { + }; + recordFetchFailure(url, httpDetail); + throw makePdbFetchError(url, httpDetail); + } catch (err) { + // A PdbFetchError from the branches above is already recorded; only a + // genuine transport/abort failure needs recording here. Rethrowing + // unchanged keeps the original status and type intact for callers. + if (err?.name === "PdbFetchError") throw err; + const exceptionDetail = { type: "exception", mode: "same-origin", attempt: 1, retries, - message: String(_err?.message || _err || "fetch-exception"), - name: String(_err?.name || "Error"), + message: String(err?.message || err || "fetch-exception"), + name: String(err?.name || "Error"), timeout, - }); - return null; + }; + recordFetchFailure(url, exceptionDetail); + throw makePdbFetchError(url, exceptionDetail); } } - return new Promise((resolve) => { + return new Promise((resolve, reject) => { logExternalRequestUserAgent({ method: "GET", url, @@ -5790,7 +5830,7 @@ resolve(parsed); } catch (_err) { - recordFetchFailure(url, { + const detail = { type: "parse", mode: "cross-origin", attempt: 1, @@ -5798,11 +5838,12 @@ status: response.status, statusText: response.statusText, message: String(_err?.message || _err || "json-parse-failed"), - }); - resolve(null); + }; + recordFetchFailure(url, detail); + reject(makePdbFetchError(url, detail)); } } else { - recordFetchFailure(url, { + const detail = { type: "http", mode: "cross-origin", attempt: 1, @@ -5810,29 +5851,32 @@ status: response.status, statusText: response.statusText, ok: false, - }); - resolve(null); + }; + recordFetchFailure(url, detail); + reject(makePdbFetchError(url, detail)); } }, onerror: () => { - recordFetchFailure(url, { + const detail = { type: "error", mode: "cross-origin", attempt: 1, retries, timeout, - }); - resolve(null); + }; + recordFetchFailure(url, detail); + reject(makePdbFetchError(url, detail)); }, ontimeout: () => { - recordFetchFailure(url, { + const detail = { type: "timeout", mode: "cross-origin", attempt: 1, retries, timeout, - }); - resolve(null); + }; + recordFetchFailure(url, detail); + reject(makePdbFetchError(url, detail)); }, }); }); @@ -6591,13 +6635,22 @@ const variables = { first: limit, offset }; const url = `${endpoint}?query=${encodeURIComponent(query)}&variables=${encodeURIComponent(JSON.stringify(variables))}`; - const payload = await pdbFetch(url, { - headers: { - Accept: "application/json", - }, - timeout: REQUEST_TIMEOUT_MS, - retries: 1, - }); + // null here is a fallback signal, not an error: the caller drops to the REST + // transport when this returns falsy. pdbFetch now throws, so that signal is + // produced explicitly rather than by pdbFetch happening to return null. + let payload = null; + try { + payload = await pdbFetch(url, { + headers: { + Accept: "application/json", + }, + timeout: REQUEST_TIMEOUT_MS, + retries: 1, + }); + } catch (err) { + dbg("names", "GraphQL name batch failed, falling back to REST", { offset, limit, error: err?.message }); + return null; + } if (!payload) return null; if (payload.errors && Array.isArray(payload.errors) && payload.errors.length > 0) return null; @@ -6623,13 +6676,21 @@ }); if (!url) return []; - const payload = await pdbFetch(url, { - headers: { - Accept: "application/json", - }, - timeout: REQUEST_TIMEOUT_MS, - retries: 1, - }); + // An empty batch is how this reports failure to its caller's paging loop, + // which simply stops. Preserved explicitly now that pdbFetch throws. + let payload = null; + try { + payload = await pdbFetch(url, { + headers: { + Accept: "application/json", + }, + timeout: REQUEST_TIMEOUT_MS, + retries: 1, + }); + } catch (err) { + dbg("names", "REST name batch failed", { offset, limit, error: err?.message }); + return []; + } const rows = Array.isArray(payload?.data) ? payload.data : []; return rows @@ -8488,8 +8549,18 @@ * @param {string} url - Absolute RDAP or bootstrap URL. * @returns {Promise} Parsed JSON payload, or null when the request fails. */ - function requestJson(url) { - return pdbFetch(url, { headers: { Accept: RDAP_ACCEPT_HEADER } }); + async function requestJson(url) { + // getBootstrap carries an "@ai Preserve ... null-on-failure behavior so + // RDAP resolution degrades safely" contract, and fetchAutnumRecord relies + // on the same. pdbFetch throws now, so that contract is honoured here + // rather than inherited: RDAP is a best-effort fallback for a name lookup, + // and a failed lookup must not surface as an unhandled rejection. + try { + return await pdbFetch(url, { headers: { Accept: RDAP_ACCEPT_HEADER } }); + } catch (err) { + dbg("rdap", "RDAP request failed", { url, error: err?.message }); + return null; + } } /** @@ -9104,7 +9175,13 @@ const networkId = String(getInputValue("#id_network") || "").trim(); const ixlanId = String(getInputValue("#id_ixlan") || "").trim(); + // Fire-and-forget: nothing awaits this, so an unhandled rejection here + // would surface as a console error with no context and no recovery. + // These org/ix toolbar links are decorative -- a failed lookup means + // the button is simply absent, which is the pre-existing behavior when + // the API returned nothing. void (async () => { + try { let orgId = ""; let ixId = ""; @@ -9159,6 +9236,13 @@ scheduleDomUpdate(`${MODULE_PREFIX}.networkixlan.toolbarOrder`, () => { enforceToolbarButtonOrder(ctx); }); + } catch (err) { + dbg("toolbar", "networkixlan org/ix lookup failed; links omitted", { + networkId, + ixlanId, + error: err?.message, + }); + } })(); } @@ -11107,6 +11191,8 @@ if (typeof window !== "undefined" && window.__PDB_TEST__) { window.__pdbCpTestHooks__ = { SCRIPT_VERSION, + pdbFetch, + fetchRecentNetixlanChanges, getRouteContext, modules, getOrganizationName, diff --git a/user.js/peeringdb-cp-consolidated-tools.user.js b/user.js/peeringdb-cp-consolidated-tools.user.js index fad8ae4..f56cb13 100644 --- a/user.js/peeringdb-cp-consolidated-tools.user.js +++ b/user.js/peeringdb-cp-consolidated-tools.user.js @@ -5718,7 +5718,10 @@ if (!/^\d+$/.test(id)) return { rows: [], cutoffIso: "", source: "", error: "bad-ixlan-id" }; const cutoffMs = Date.now() - Math.max(1, Number(windowMinutes) || 60) * 60 * 1000; const cutoffIso = new Date(cutoffMs).toISOString().replace(/\.\d{3}Z$/, "Z"); - // Try server-side filter first. + // Try server-side filter first. Both catch blocks below were dead code while + // pdbFetch returned null: the try always reached its return, so a rejected + // updated__gte filter reported "0 rows, no error" and the client-filter + // fallback never ran. They are live now that pdbFetch throws. try { const serverUrl = `${PEERINGDB_API_BASE_URL}/netixlan?ixlan_id=${encodeURIComponent(id)}&updated__gte=${encodeURIComponent(cutoffIso)}&depth=0&limit=250`; const data = await pdbFetch(serverUrl); @@ -7017,6 +7020,27 @@ } } + /** + * Builds the error pdbFetch rejects with. + * Purpose: Give callers a single recognisable failure type carrying enough + * context to report or branch on, instead of an untyped null that is + * indistinguishable from a legitimately empty response. + * @param {string} url - Request URL that failed. + * @param {object} detail - Same shape passed to recordFetchFailure (type, + * mode, status, message, ...), attached for callers that want specifics. + * @returns {Error} Error with name "PdbFetchError" plus url/detail/status. + */ + function makePdbFetchError(url, detail) { + const status = detail && typeof detail.status === "number" ? detail.status : 0; + const suffix = status ? ` (HTTP ${status})` : ""; + const error = new Error(`pdbFetch ${detail?.type || "failed"}${suffix}: ${url}`); + error.name = "PdbFetchError"; + error.url = url; + error.status = status; + error.detail = detail || {}; + return error; + } + /** * Unified JSON fetch helper for all script-initiated HTTP requests. * Purpose: Single network abstraction with timeout, retry, and error normalisation @@ -7024,10 +7048,19 @@ * GM_xmlhttpRequest) call sites. * Necessity: Prevents N ad-hoc GM_xmlhttpRequest patterns from diverging on * timeout handling or header construction. + * + * THROWS on failure -- it does not return null. This previously resolved to + * null for every failure mode, which silently made `catch` blocks around it + * dead code: fetchRecentNetixlanChanges had two, and its entire client-side + * fallback was unreachable because the try block always returned. Callers + * must handle rejection; a resolved value is always a parsed payload. * @ai Preserve request retries/timeouts/error classification and payload assumptions. + * Do NOT reintroduce a null return -- callers rely on rejection to detect failure. * @param {string} url - Absolute URL to fetch. * @param {{ headers?: object, timeout?: number, retries?: number }} [options] - * @returns {Promise} Parsed JSON or null on any failure. + * @returns {Promise} Parsed JSON payload. + * @throws {Error} PdbFetchError (see makePdbFetchError) on HTTP error, parse + * failure, transport error or timeout. */ async function pdbFetch(url, { headers = {}, timeout = PDB_API_TIMEOUT_MS, retries = PDB_API_RETRIES } = {}) { const fullHeaders = buildTampermonkeyRequestHeaders(headers); @@ -7066,7 +7099,7 @@ clearFetchFailure(url); return parsed; } catch (parseError) { - recordFetchFailure(url, { + const detail = { type: "parse", mode: "same-origin", attempt: 1, @@ -7074,12 +7107,13 @@ status: response.status, statusText: response.statusText, message: String(parseError?.message || parseError || "json-parse-failed"), - }); - return null; + }; + recordFetchFailure(url, detail); + throw makePdbFetchError(url, detail); } } - recordFetchFailure(url, { + const httpDetail = { type: "http", mode: "same-origin", attempt: 1, @@ -7087,23 +7121,29 @@ status: response.status, statusText: response.statusText, ok: false, - }); - return null; - } catch (_err) { - recordFetchFailure(url, { + }; + recordFetchFailure(url, httpDetail); + throw makePdbFetchError(url, httpDetail); + } catch (err) { + // A PdbFetchError from the branches above is already recorded; only a + // genuine transport/abort failure needs recording here. Rethrowing + // unchanged keeps the original status and type intact for callers. + if (err?.name === "PdbFetchError") throw err; + const exceptionDetail = { type: "exception", mode: "same-origin", attempt: 1, retries, - message: String(_err?.message || _err || "fetch-exception"), - name: String(_err?.name || "Error"), + message: String(err?.message || err || "fetch-exception"), + name: String(err?.name || "Error"), timeout, - }); - return null; + }; + recordFetchFailure(url, exceptionDetail); + throw makePdbFetchError(url, exceptionDetail); } } - return new Promise((resolve) => { + return new Promise((resolve, reject) => { logExternalRequestUserAgent({ method: "GET", url, @@ -7128,7 +7168,7 @@ resolve(parsed); } catch (_err) { - recordFetchFailure(url, { + const detail = { type: "parse", mode: "cross-origin", attempt: 1, @@ -7136,11 +7176,12 @@ status: response.status, statusText: response.statusText, message: String(_err?.message || _err || "json-parse-failed"), - }); - resolve(null); + }; + recordFetchFailure(url, detail); + reject(makePdbFetchError(url, detail)); } } else { - recordFetchFailure(url, { + const detail = { type: "http", mode: "cross-origin", attempt: 1, @@ -7148,29 +7189,32 @@ status: response.status, statusText: response.statusText, ok: false, - }); - resolve(null); + }; + recordFetchFailure(url, detail); + reject(makePdbFetchError(url, detail)); } }, onerror: () => { - recordFetchFailure(url, { + const detail = { type: "error", mode: "cross-origin", attempt: 1, retries, timeout, - }); - resolve(null); + }; + recordFetchFailure(url, detail); + reject(makePdbFetchError(url, detail)); }, ontimeout: () => { - recordFetchFailure(url, { + const detail = { type: "timeout", mode: "cross-origin", attempt: 1, retries, timeout, - }); - resolve(null); + }; + recordFetchFailure(url, detail); + reject(makePdbFetchError(url, detail)); }, }); }); @@ -7929,13 +7973,22 @@ const variables = { first: limit, offset }; const url = `${endpoint}?query=${encodeURIComponent(query)}&variables=${encodeURIComponent(JSON.stringify(variables))}`; - const payload = await pdbFetch(url, { - headers: { - Accept: "application/json", - }, - timeout: REQUEST_TIMEOUT_MS, - retries: 1, - }); + // null here is a fallback signal, not an error: the caller drops to the REST + // transport when this returns falsy. pdbFetch now throws, so that signal is + // produced explicitly rather than by pdbFetch happening to return null. + let payload = null; + try { + payload = await pdbFetch(url, { + headers: { + Accept: "application/json", + }, + timeout: REQUEST_TIMEOUT_MS, + retries: 1, + }); + } catch (err) { + dbg("names", "GraphQL name batch failed, falling back to REST", { offset, limit, error: err?.message }); + return null; + } if (!payload) return null; if (payload.errors && Array.isArray(payload.errors) && payload.errors.length > 0) return null; @@ -7961,13 +8014,21 @@ }); if (!url) return []; - const payload = await pdbFetch(url, { - headers: { - Accept: "application/json", - }, - timeout: REQUEST_TIMEOUT_MS, - retries: 1, - }); + // An empty batch is how this reports failure to its caller's paging loop, + // which simply stops. Preserved explicitly now that pdbFetch throws. + let payload = null; + try { + payload = await pdbFetch(url, { + headers: { + Accept: "application/json", + }, + timeout: REQUEST_TIMEOUT_MS, + retries: 1, + }); + } catch (err) { + dbg("names", "REST name batch failed", { offset, limit, error: err?.message }); + return []; + } const rows = Array.isArray(payload?.data) ? payload.data : []; return rows @@ -9826,8 +9887,18 @@ * @param {string} url - Absolute RDAP or bootstrap URL. * @returns {Promise} Parsed JSON payload, or null when the request fails. */ - function requestJson(url) { - return pdbFetch(url, { headers: { Accept: RDAP_ACCEPT_HEADER } }); + async function requestJson(url) { + // getBootstrap carries an "@ai Preserve ... null-on-failure behavior so + // RDAP resolution degrades safely" contract, and fetchAutnumRecord relies + // on the same. pdbFetch throws now, so that contract is honoured here + // rather than inherited: RDAP is a best-effort fallback for a name lookup, + // and a failed lookup must not surface as an unhandled rejection. + try { + return await pdbFetch(url, { headers: { Accept: RDAP_ACCEPT_HEADER } }); + } catch (err) { + dbg("rdap", "RDAP request failed", { url, error: err?.message }); + return null; + } } /** @@ -10442,7 +10513,13 @@ const networkId = String(getInputValue("#id_network") || "").trim(); const ixlanId = String(getInputValue("#id_ixlan") || "").trim(); + // Fire-and-forget: nothing awaits this, so an unhandled rejection here + // would surface as a console error with no context and no recovery. + // These org/ix toolbar links are decorative -- a failed lookup means + // the button is simply absent, which is the pre-existing behavior when + // the API returned nothing. void (async () => { + try { let orgId = ""; let ixId = ""; @@ -10497,6 +10574,13 @@ scheduleDomUpdate(`${MODULE_PREFIX}.networkixlan.toolbarOrder`, () => { enforceToolbarButtonOrder(ctx); }); + } catch (err) { + dbg("toolbar", "networkixlan org/ix lookup failed; links omitted", { + networkId, + ixlanId, + error: err?.message, + }); + } })(); } @@ -12445,6 +12529,8 @@ if (typeof window !== "undefined" && window.__PDB_TEST__) { window.__pdbCpTestHooks__ = { SCRIPT_VERSION, + pdbFetch, + fetchRecentNetixlanChanges, getRouteContext, modules, getOrganizationName, diff --git a/user.js/tests/cp-pdbfetch-errors.test.js b/user.js/tests/cp-pdbfetch-errors.test.js new file mode 100644 index 0000000..f680eac --- /dev/null +++ b/user.js/tests/cp-pdbfetch-errors.test.js @@ -0,0 +1,97 @@ +'use strict'; + +// pdbFetch's failure contract. +// +// It used to resolve to null for every failure mode -- HTTP error, JSON parse +// failure, transport error, timeout. That silently turned `catch` blocks around +// it into dead code. fetchRecentNetixlanChanges is the clearest casualty: its +// first `try` always reached its return, so a rejected updated__gte filter +// reported "0 rows, no error" and the client-side fallback below it could never +// run. An admin verifying a renumber would conclude nothing had changed. +// +// These cases pin the new contract: a resolved value is always a parsed +// payload, and every failure rejects with a PdbFetchError. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const { loadScript } = require('./helpers/browser-shim'); + +const SCRIPT_PATH = path.join(__dirname, '..', 'peeringdb-cp-consolidated-tools.user.js'); +const API = 'https://www.peeringdb.com/api'; + +function loadCp(fetchMap = {}) { + return loadScript(SCRIPT_PATH, { + hooksKey: '__pdbCpTestHooks__', + pathname: '/cp/peeringdb_server/networkixlan/', + fetchMap, + }); +} + +test('pdbFetch rejects rather than resolving null', async (t) => { + await t.test('an HTTP error rejects with a typed, inspectable error', async () => { + const url = `${API}/net/1`; + const { hooks } = loadCp({ [url]: { __response: true, status: 500, body: {} } }); + + await assert.rejects( + () => hooks.pdbFetch(url), + (err) => { + assert.equal(err.name, 'PdbFetchError', 'callers need to distinguish this from a bug'); + assert.equal(err.status, 500); + assert.equal(err.url, url); + assert.equal(err.detail.type, 'http'); + return true; + }, + ); + }); + + await t.test('an unmapped URL (404) rejects too', async () => { + const { hooks } = loadCp({}); + await assert.rejects(() => hooks.pdbFetch(`${API}/net/999`), { name: 'PdbFetchError' }); + }); + + await t.test('a successful response still resolves to the parsed payload', async () => { + const url = `${API}/net/2`; + const { hooks } = loadCp({ [url]: { data: [{ id: 2, name: 'Example Net' }] } }); + const payload = await hooks.pdbFetch(url); + assert.equal(payload.data[0].name, 'Example Net'); + }); +}); + +test('fetchRecentNetixlanChanges falls back to client-side filtering', async (t) => { + const ixlanId = '42'; + const unfiltered = `${API}/netixlan?ixlan_id=42&depth=0&limit=250`; + + await t.test('the fallback runs when the server-side filter is rejected', async () => { + // The whole point of the fix: this path was unreachable before, because the + // failing pdbFetch returned null instead of throwing and the try returned + // "0 rows, no error" regardless. + const { hooks, fetchCalls } = loadCp({ + [unfiltered]: { + data: [ + { id: 1, updated: new Date().toISOString() }, + { id: 2, updated: '2001-01-01T00:00:00Z' }, + ], + }, + }); + + const result = await hooks.fetchRecentNetixlanChanges(ixlanId, 60); + + assert.equal(result.source, 'client-filter', 'must report which path produced the rows'); + assert.equal(result.error, ''); + assert.equal(result.rows.length, 1, 'the stale row is filtered out by date'); + assert.equal(result.rows[0].id, 1); + assert.equal(fetchCalls.length, 2, 'server-filter attempt, then the fallback'); + assert.ok(fetchCalls[0].url.includes('updated__gte'), 'server-side filter is still tried first'); + }); + + await t.test('both attempts failing reports an error instead of an empty success', async () => { + const { hooks } = loadCp({}); + const result = await hooks.fetchRecentNetixlanChanges(ixlanId, 60); + + assert.equal(result.error, 'fetch-failed', 'silence here is what misled an admin mid-renumber'); + assert.equal(result.rows.length, 0); + assert.equal(result.source, ''); + }); + +});