From 9b263bb3794cbef1fadf3dbc1735b64750593261 Mon Sep 17 00:00:00 2001 From: Jannis R Date: Tue, 3 May 2022 23:21:44 +0200 Subject: [PATCH] =?UTF-8?q?rework=20error=20handling=20=F0=9F=92=A5?= =?UTF-8?q?=E2=9C=85=F0=9F=93=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/readme.md | 38 +++++ index.js | 25 +-- lib/check-if-res-is-ok.js | 44 +++++ lib/errors.js | 266 +++++++++++++++++++----------- lib/request.js | 44 ++--- package.json | 2 +- test/fixtures/error-h9360.json | 28 ++++ test/fixtures/error-location.json | 28 ++++ test/lib/check-if-res-is-ok.js | 109 ++++++++++++ 9 files changed, 435 insertions(+), 149 deletions(-) create mode 100644 lib/check-if-res-is-ok.js create mode 100644 test/fixtures/error-h9360.json create mode 100644 test/fixtures/error-location.json create mode 100644 test/lib/check-if-res-is-ok.js diff --git a/docs/readme.md b/docs/readme.md index 01516ae7..84fed8b4 100644 --- a/docs/readme.md +++ b/docs/readme.md @@ -135,6 +135,44 @@ const client = createClient({ The default `profile.logRequest` [`console.error`](https://nodejs.org/docs/latest-v10.x/api/console.html#console_console_error_data_args)s the request body, if you have set `$DEBUG` to `hafas-client`. Likewise, `profile.logResponse` `console.error`s the response body. +## Error handling + +Unexpected errors – e.g. due to bugs in `hafas-client` itself – aside, its methods may reject with the following errors: + +- `HafasError` – A generic error to signal that something HAFAS-related went wrong, either in the client, or in the HAFAS endpoint. +- `HafasAccessDeniedError` – The HAFAS endpoint has rejected your request because you're not allowed to access it (or the specific API call). Subclass of `HafasError`. +- `HafasInvalidRequestError` – The HAFAS endpoint reports that an invalid request has been sent. Subclass of `HafasError`. +- `HafasNotFoundError` – The HAFAS endpoint does not know about such stop/trip/etc. Subclass of `HafasError`. +- `HafasServerError` – An error occured within the HAFAS endpoint, so that it can't fulfill the request; For example, this happens when HAFAS' internal backend is unavailable. Subclass of `HafasError`. + +Each error has the following properties: + +- `isHafasError` – Always `true`. Allows you to differente HAFAS-related errors from e.g. network errors. +- `code` – A string representing the error type for all other error classes, e.g. `INVALID_REQUEST` for `HafasInvalidRequestError`. `null` for plain `HafasError`s. +- `isCausedByServer` – Boolean, telling you if the HAFAS endpoint says that it couldn't process your request because *it* is unavailable/broken. +- `hafasCode` – A HAFAS-specific error code, if the HAFAS endpoint returned one; e.g. `H890` when no journeys could be found. `null` otherwise. +- `request` – The [Fetch API `Request`](https://developer.mozilla.org/en-US/docs/Web/API/Request) of the request. +- `url` – The URL of the request. +- `response` – The [Fetch API `Response`](https://developer.mozilla.org/en-US/docs/Web/API/Response). + +To check **if an error from `hafas-client` is HAFAS-specific, use `error instanceof HafasError`**: + +```js +const {HafasError} = require('hafas-client/lib/errors') + +try { + await client.journeys(/* … */) +} catch (err) { + if (err instanceof HafasError) { + // HAFAS-specific error + } else { + // different error, e.g. network (ETIMEDOUT, ENETDOWN) + } +} +``` + +To determine **if you should automatically retry an error, use `!error.causedByServer`**. + ## Using `hafas-client` from another language If you want to use `hafas-client` to access HAFAS APIs but work with non-Node.js environments, you can use [`hafas-client-rpc`](https://github.com/derhuerst/hafas-client-rpc) to create a [JSON-RPC](https://www.jsonrpc.org) interface that you can send commands to. diff --git a/index.js b/index.js index b35c5261..c664c8a8 100644 --- a/index.js +++ b/index.js @@ -8,6 +8,7 @@ const defaultProfile = require('./lib/default-profile') const validateProfile = require('./lib/validate-profile') const {INVALID_REQUEST} = require('./lib/errors') const sliceLeg = require('./lib/slice-leg') +const {HafasError} = require('./lib/errors') const isNonEmptyString = str => 'string' === typeof str && str.length > 0 @@ -248,11 +249,7 @@ const createClient = (profile, userAgent, opt = {}) => { const {res, common} = await profile.request({profile, opt}, userAgent, req) if (!Array.isArray(res.outConL) || !res.outConL[0]) { - const err = new Error('invalid response') - // technically this is not a HAFAS error - // todo: find a different flag with decent DX - err.isHafasError = true - throw err + throw new HafasError('invalid response, expected outConL[0]', null, {}) } const ctx = {profile, opt, common, res} @@ -427,14 +424,10 @@ const createClient = (profile, userAgent, opt = {}) => { const {res, common} = await profile.request({profile, opt}, userAgent, req) if (!res || !Array.isArray(res.locL) || !res.locL[0]) { - // todo: proper stack trace? - // todo: DRY with lib/request.js - const err = new Error('response has no stop') - // technically this is not a HAFAS error - // todo: find a different flag with decent DX - err.isHafasError = true - err.code = INVALID_REQUEST - throw err + throw new HafasError('invalid response, expected locL[0]', null, { + // This problem occurs on invalid input. 🙄 + code: INVALID_REQUEST, + }) } const ctx = {profile, opt, res, common} @@ -622,9 +615,9 @@ const createClient = (profile, userAgent, opt = {}) => { const {res, common} = await profile.request({profile, opt}, userAgent, req) if (!Array.isArray(res.posL)) { - const err = new Error('invalid response') - err.shouldRetry = true - throw err + throw new HafasError('invalid response, expected posL[0]', null, { + shouldRetry: true, + }) } const byDuration = [] diff --git a/lib/check-if-res-is-ok.js b/lib/check-if-res-is-ok.js new file mode 100644 index 00000000..d441c708 --- /dev/null +++ b/lib/check-if-res-is-ok.js @@ -0,0 +1,44 @@ +'use strict' + +const {HafasError, byErrorCode} = require('./errors') + +const checkIfResponseIsOk = (_) => { + const { + body, + errProps: baseErrProps, + } = _ + + const errProps = { + ...baseErrProps, + } + if (body.id) errProps.hafasResponseId = body.id + + // Because we want more accurate stack traces, we don't construct the error here, + // but only return the constructor & error message. + const getError = (_) => { + // mutating here is ugly but pragmatic + if (_.errTxt) errProps.hafasMessage = _.errTxt + if (_.errTxtOut) errProps.hafasDescription = _.errTxtOut + + if (_.err in byErrorCode) return byErrorCode[_.err] + return { + Error: HafasError, + message: body.errTxt || 'unknown error', + props: {}, + } + } + + if (body.err && body.err !== 'OK') { + const {Error: HafasError, message, props} = getError(body) + throw new HafasError(message, body.err, {...errProps, ...props}) + } + if (!body.svcResL || !body.svcResL[0]) { + throw new HafasError('invalid/unsupported response structure', null, errProps) + } + if (body.svcResL[0].err !== 'OK') { + const {Error: HafasError, message, props} = getError(body.svcResL[0]) + throw new HafasError(message, body.svcResL[0].err, {...errProps, ...props}) + } +} + +module.exports = checkIfResponseIsOk diff --git a/lib/errors.js b/lib/errors.js index ca7fc907..68ee7a92 100644 --- a/lib/errors.js +++ b/lib/errors.js @@ -5,205 +5,268 @@ const INVALID_REQUEST = 'INVALID_REQUEST' const NOT_FOUND = 'NOT_FOUND' const SERVER_ERROR = 'SERVER_ERROR' +class HafasError extends Error { + constructor (cleanMessage, hafasCode, props) { + const msg = hafasCode + ? hafasCode + ': ' + cleanMessage + : cleanMessage + super(msg) + + // generic props + this.isHafasError = true + + // error-specific props + this.code = null + // By default, we take the blame, unless we know for sure. + this.isCausedByServer = false + this.hafasCode = hafasCode + Object.assign(this, props) + + return this + } +} + +class HafasAccessDeniedError extends HafasError { + constructor (cleanMessage, hafasCode, props) { + super(cleanMessage, hafasCode, props) + this.code = ACCESS_DENIED + return this + } +} + +class HafasInvalidRequestError extends HafasError { + constructor (cleanMessage, hafasCode, props) { + super(cleanMessage, hafasCode, props) + this.code = INVALID_REQUEST + return this + } +} + +class HafasNotFoundError extends HafasError { + constructor (cleanMessage, hafasCode, props) { + super(cleanMessage, hafasCode, props) + this.code = NOT_FOUND + return this + } +} + +class HafasServerError extends HafasError { + constructor (cleanMessage, hafasCode, props) { + super(cleanMessage, hafasCode, props) + this.code = SERVER_ERROR + this.isCausedByServer = true + return this + } +} + // https://gist.github.com/derhuerst/79d49c0f04c1c192a5d15756e5af575f/edit // todo: // `code: 'METHOD_NA', message: 'HCI Service: service method disabled'` // "err": "PARAMETER", "errTxt": "HCI Service: parameter invalid" +// "err": "PARAMETER", "errTxt": "HCI Service: parameter invalid","errTxtOut":"Während der Suche ist ein interner Fehler aufgetreten" const byErrorCode = Object.assign(Object.create(null), { H_UNKNOWN: { - isServer: false, - code: SERVER_ERROR, + Error: HafasError, message: 'unknown internal error', - statusCode: 500, + props: { + shouldRetry: true, + }, }, AUTH: { - isClient: true, - code: ACCESS_DENIED, + Error: HafasAccessDeniedError, message: 'invalid or missing authentication data', - statusCode: 401 + props: { + }, }, R0001: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasInvalidRequestError, message: 'unknown method', - statusCode: 400 + props: { + }, }, R0002: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasInvalidRequestError, message: 'invalid or missing request parameters', - statusCode: 400 + props: { + }, }, R0007: { - isServer: true, - code: SERVER_ERROR, + Error: HafasServerError, message: 'internal communication error', - statusCode: 500 + props: { + shouldRetry: true, + }, }, R5000: { - isClient: true, - code: ACCESS_DENIED, + Error: HafasAccessDeniedError, message: 'access denied', - statusCode: 401 + props: { + }, }, S1: { - isServer: true, - code: SERVER_ERROR, + Error: HafasServerError, message: 'journeys search: a connection to the backend server couldn\'t be established', - statusCode: 503 + props: { + shouldRetry: true, + }, }, LOCATION: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasNotFoundError, message: 'location/stop not found', - statusCode: 400 + props: { + }, }, H390: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasInvalidRequestError, message: 'journeys search: departure/arrival station replaced', - statusCode: 400 + props: { + }, }, H410: { // todo: or is it a client error? - // todo: statusCode? - isServer: true, - code: SERVER_ERROR, - message: 'journeys search: incomplete response due to timetable change' + Error: HafasServerError, + message: 'journeys search: incomplete response due to timetable change', + props: { + }, }, H455: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasInvalidRequestError, message: 'journeys search: prolonged stop', - statusCode: 400 + props: { + }, }, H460: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasInvalidRequestError, message: 'journeys search: stop(s) passed multiple times', - statusCode: 400 + props: { + }, }, H500: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasInvalidRequestError, message: 'journeys search: too many trains, connection is not complete', - statusCode: 400 + props: { + }, }, H890: { - isClient: true, - code: NOT_FOUND, + Error: HafasNotFoundError, message: 'journeys search unsuccessful', - statusCode: 404 + props: { + shouldRetry: true, + }, }, H891: { - isClient: true, - code: NOT_FOUND, + Error: HafasNotFoundError, message: 'journeys search: no route found, try with an intermediate stations', - statusCode: 404 + props: { + }, }, H892: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasInvalidRequestError, message: 'journeys search: query too complex, try less intermediate stations', - statusCode: 400 + props: { + }, }, H895: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasInvalidRequestError, message: 'journeys search: departure & arrival are too near', - statusCode: 400 + props: { + }, }, H899: { // todo: or is it a client error? - // todo: statusCode? - isServer: true, - code: SERVER_ERROR, - message: 'journeys search unsuccessful or incomplete due to timetable change' + Error: HafasServerError, + message: 'journeys search unsuccessful or incomplete due to timetable change', + props: { + }, }, H900: { // todo: or is it a client error? - // todo: statusCode? - isServer: true, - code: SERVER_ERROR, - message: 'journeys search unsuccessful or incomplete due to timetable change' + Error: HafasServerError, + message: 'journeys search unsuccessful or incomplete due to timetable change', + props: { + }, }, H9220: { - isClient: true, - code: NOT_FOUND, + Error: HafasNotFoundError, message: 'journeys search: no stations found close to the address', - statusCode: 400 + props: { + }, }, H9230: { - isServer: true, - code: SERVER_ERROR, + Error: HafasServerError, message: 'journeys search: an internal error occured', - statusCode: 500 + props: { + shouldRetry: true, + }, }, H9240: { - isClient: true, - code: NOT_FOUND, + Error: HafasNotFoundError, message: 'journeys search unsuccessful', - statusCode: 404 + props: { + shouldRetry: true, + }, }, H9250: { - isServer: true, - code: SERVER_ERROR, + Error: HafasServerError, message: 'journeys search: leg query interrupted', - statusCode: 500 + props: { + shouldRetry: true, + }, }, H9260: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasInvalidRequestError, message: 'journeys search: unknown departure station', - statusCode: 400 + props: { + }, }, H9280: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasInvalidRequestError, message: 'journeys search: unknown intermediate station', - statusCode: 400 + props: { + }, }, H9300: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasInvalidRequestError, message: 'journeys search: unknown arrival station', - statusCode: 400 + props: { + }, }, H9320: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasInvalidRequestError, message: 'journeys search: the input is incorrect or incomplete', - statusCode: 400 + props: { + }, }, H9360: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasInvalidRequestError, message: 'journeys search: invalid date/time', - statusCode: 400 + props: { + }, }, H9380: { - isClient: true, - code: INVALID_REQUEST, + Error: HafasInvalidRequestError, message: 'journeys search: departure/arrival/intermediate station defined more than once', - statusCode: 400 + props: { + }, }, SQ001: { - isServer: true, - code: SERVER_ERROR, + Error: HafasServerError, message: 'no departures/arrivals data available', - statusCode: 503 + props: { + }, }, SQ005: { - isClient: true, - code: NOT_FOUND, + Error: HafasNotFoundError, message: 'no trips found', - statusCode: 404 + props: { + }, }, TI001: { - isServer: true, - code: SERVER_ERROR, + Error: HafasServerError, message: 'no trip info available', - statusCode: 503 + props: { + shouldRetry: true, + }, } }) @@ -212,5 +275,10 @@ module.exports = { INVALID_REQUEST, NOT_FOUND, SERVER_ERROR, + HafasError, + HafasAccessDeniedError, + HafasInvalidRequestError, + HafasNotFoundError, + HafasServerError, byErrorCode, } diff --git a/lib/request.js b/lib/request.js index 04204030..134f0895 100644 --- a/lib/request.js +++ b/lib/request.js @@ -10,7 +10,8 @@ const pick = require('lodash/pick') const {stringify} = require('qs') const {Request, fetch} = require('cross-fetch') const {parse: parseContentType} = require('content-type') -const {byErrorCode} = require('./errors') +const {HafasError} = require('./errors') +const checkIfResponseIsOk = require('./check-if-res-is-ok') const proxyAddress = process.env.HTTPS_PROXY || process.env.HTTP_PROXY || null const localAddresses = process.env.LOCAL_ADDRESS || null @@ -127,15 +128,14 @@ const request = async (ctx, userAgent, reqData) => { const res = await fetch(url, req) - // Async stack traces are not supported everywhere yet, so we create our own. const errProps = { - isHafasError: true, // todo: rename to `isHafasClientError` - statusCode: res.status, - request: req.body, // todo [breaking]: change to fetchReq - url: url, + request: fetchReq, + response: res, + url, } if (!res.ok) { + // todo [breaking]: make this a FetchError or a HafasClientError? const err = new Error(res.statusText) Object.assign(err, errProps) throw err @@ -145,9 +145,7 @@ const request = async (ctx, userAgent, reqData) => { if (cType) { const {type} = parseContentType(cType) if (type !== 'application/json') { - const err = new Error('invalid response content-type: ' + cType) - err.response = res - throw err + throw new HafasError('invalid/unsupported response content-type: ' + cType, null, errProps) } } @@ -155,30 +153,10 @@ const request = async (ctx, userAgent, reqData) => { profile.logResponse(ctx, res, body, reqId) const b = JSON.parse(body) - if (b.err) errProps.hafasErrorCode = b.err - if (b.errTxt) errProps.hafasErrorMessage = b.errTxt - if (b.id) errProps.responseId = b.id - if (b.err && b.err !== 'OK') { - const err = new Error(b.errTxt || b.err) - Object.assign(err, errProps) - if (b.err in byErrorCode) { - Object.assign(err, byErrorCode[b.err]) - } - throw err - } - if (!b.svcResL || !b.svcResL[0]) { - const err = new Error('invalid/unsupported response structure') - Object.assign(err, errProps) - throw err - } - if (b.svcResL[0].err !== 'OK') { - const err = new Error(b.svcResL[0].errTxt || b.svcResL[0].err) - Object.assign(err, errProps) - if (b.svcResL[0].err in byErrorCode) { - Object.assign(err, byErrorCode[b.svcResL[0].err]) - } - throw err - } + checkIfResponseIsOk({ + body: b, + errProps, + }) const svcRes = b.svcResL[0].res return { diff --git a/package.json b/package.json index 1593a787..6c995266 100644 --- a/package.json +++ b/package.json @@ -70,7 +70,7 @@ }, "scripts": { "lint": "eslint .", - "test-unit": "tap test/*.js test/format/*.js test/parse/*.js", + "test-unit": "tap test/lib/*.js test/*.js test/format/*.js test/parse/*.js", "test-integration": "VCR_MODE=playback tap test/e2e/*.js", "test-integration:record": "VCR_MODE=record tap -t60 -j16 test/e2e/*.js", "test-e2e": "VCR_OFF=true tap -t60 -j16 test/e2e/*.js", diff --git a/test/fixtures/error-h9360.json b/test/fixtures/error-h9360.json new file mode 100644 index 00000000..b3622ff0 --- /dev/null +++ b/test/fixtures/error-h9360.json @@ -0,0 +1,28 @@ +{ + "ver": "1.44", + "ext": "BVG.1", + "lang": "deu", + "id": "3xmggksuiukkx6wx", + "err": "OK", + "graph": { + "id": "standard", + "index": 0 + }, + "subGraph": { + "id": "global", + "index": 0 + }, + "view": { + "id": "standard", + "index": 0, + "type": "WGS84" + }, + "svcResL": [ + { + "meth": "StationBoard", + "err": "H9360", + "errTxt": "HAFAS Kernel: Date outside of the timetable period.", + "errTxtOut": "Fehler bei der Datumseingabe oder Datum außerhalb der Fahrplanperiode (01.05.2022 - 10.12.2022)" + } + ] +} diff --git a/test/fixtures/error-location.json b/test/fixtures/error-location.json new file mode 100644 index 00000000..831d9062 --- /dev/null +++ b/test/fixtures/error-location.json @@ -0,0 +1,28 @@ +{ + "ver": "1.44", + "ext": "BVG.1", + "lang": "deu", + "id": "mmmxakseiuk9g6wg", + "err": "OK", + "graph": { + "id": "standard", + "index": 0 + }, + "subGraph": { + "id": "global", + "index": 0 + }, + "view": { + "id": "standard", + "index": 0, + "type": "WGS84" + }, + "svcResL": [ + { + "meth": "StationBoard", + "err": "LOCATION", + "errTxt": "HCI Service: location missing or invalid", + "errTxtOut": "Während der Suche ist ein interner Fehler aufgetreten" + } + ] +} diff --git a/test/lib/check-if-res-is-ok.js b/test/lib/check-if-res-is-ok.js new file mode 100644 index 00000000..2e187297 --- /dev/null +++ b/test/lib/check-if-res-is-ok.js @@ -0,0 +1,109 @@ +'use strict' + +const tap = require('tap') +const checkIfResIsOk = require('../../lib/check-if-res-is-ok') +const { + INVALID_REQUEST, + NOT_FOUND, + HafasError, + HafasInvalidRequestError, + HafasNotFoundError, +} = require('../../lib/errors') + +const resParameter = require('../fixtures/error-parameter.json') +const resNoMatch = require('../fixtures/error-no-match.json') +const resH9360 = require('../fixtures/error-h9360.json') +const resLocation = require('../fixtures/error-location.json') + +const secret = Symbol('secret') + +tap.test('checkIfResponseIsOk properly throws HAFAS "H9360" errors', (t) => { + try { + checkIfResIsOk({ + body: resH9360, + errProps: {secret}, + }) + } catch (err) { + t.ok(err) + + t.ok(err instanceof HafasError) + t.equal(err.isHafasError, true) + t.equal(err.message.slice(0, 7), 'H9360: ') + t.ok(err.message.length > 7) + + t.ok(err instanceof HafasInvalidRequestError) + t.equal(err.isCausedByServer, false) + t.equal(err.code, INVALID_REQUEST) + t.equal(err.hafasCode, 'H9360') + + t.equal(err.hafasResponseId, resH9360.id) + t.equal(err.hafasMessage, 'HAFAS Kernel: Date outside of the timetable period.') + t.equal(err.hafasDescription, 'Fehler bei der Datumseingabe oder Datum außerhalb der Fahrplanperiode (01.05.2022 - 10.12.2022)') + t.equal(err.secret, secret) + + t.end() + } +}) + +tap.test('checkIfResponseIsOk properly throws HAFAS "LOCATION" errors', (t) => { + try { + checkIfResIsOk({ + body: resLocation, + errProps: {secret}, + }) + } catch (err) { + t.ok(err) + + t.ok(err instanceof HafasError) + t.equal(err.isHafasError, true) + t.equal(err.message.slice(0, 10), 'LOCATION: ') + t.ok(err.message.length > 10) + + t.ok(err instanceof HafasNotFoundError) + t.equal(err.isCausedByServer, false) + t.equal(err.code, NOT_FOUND) + t.equal(err.hafasCode, 'LOCATION') + + t.equal(err.hafasResponseId, resLocation.id) + t.equal(err.hafasMessage, 'HCI Service: location missing or invalid') + t.equal(err.hafasDescription, 'Während der Suche ist ein interner Fehler aufgetreten') + t.equal(err.secret, secret) + + t.end() + } +}) + +tap.test('checkIfResponseIsOk properly parses an unknown HAFAS errors', (t) => { + const body = { + ver: '1.42', + id: '1234567890', + err: 'FOO', + errTxt: 'random errTxt', + errTxtOut: 'even more random errTxtOut', + svcResL: [], + } + + try { + checkIfResIsOk({ + body, + errProps: {secret}, + }) + } catch (err) { + t.ok(err) + + t.ok(err instanceof HafasError) + t.equal(err.isHafasError, true) + t.equal(err.message, `${body.err}: ${body.errTxt}`) + + t.equal(err.isCausedByServer, false) + t.equal(err.code, null) + t.equal(err.hafasCode, body.err) + + t.equal(err.hafasResponseId, body.id) + t.equal(err.hafasMessage, body.errTxt) + t.equal(err.hafasDescription, body.errTxtOut) + t.equal(err.secret, secret) + + t.end() + } +})