From 98bb3753f951f9fba664d37122eeaf49891e3b87 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Wed, 8 Jan 2025 12:12:33 +0000 Subject: [PATCH 01/17] error pages: fold error templates into one, show stack when not in prod --- config/util.js | 7 +- src/assets/scss/index.scss | 8 ++ src/filters/filters.js | 2 + src/filters/schemaIssues.js | 21 +++++ src/middleware/common.middleware.js | 8 +- .../entryIssueDetails.middleware.js | 4 +- src/middleware/middleware.builders.js | 7 +- src/routes/guidance.js | 81 ------------------- src/routes/schemas.js | 16 ++-- src/serverSetup/errorHandlers.js | 17 ++-- src/serverSetup/middlewares.js | 4 +- src/services/datasette.js | 1 + src/utils/errors.js | 26 ++++++ src/views/check/results/failedUrlRequest.html | 2 +- src/views/errorPages/400.html | 19 ----- src/views/errorPages/404.html | 19 ----- src/views/errorPages/500.html | 15 ---- src/views/errorPages/503.html | 17 ---- src/views/errorPages/504.html | 15 ---- src/views/errorPages/error.njk | 76 +++++++++++++++++ test/unit/default-http-errors.test.js | 17 +++- .../unit/middleware/common.middleware.test.js | 5 +- test/unit/publishRequestAPI.test.js | 6 +- 23 files changed, 194 insertions(+), 199 deletions(-) create mode 100644 src/filters/schemaIssues.js create mode 100644 src/utils/errors.js delete mode 100644 src/views/errorPages/400.html delete mode 100644 src/views/errorPages/404.html delete mode 100644 src/views/errorPages/500.html delete mode 100644 src/views/errorPages/503.html delete mode 100644 src/views/errorPages/504.html create mode 100644 src/views/errorPages/error.njk diff --git a/config/util.js b/config/util.js index a1463f664..bb8b57649 100644 --- a/config/util.js +++ b/config/util.js @@ -94,7 +94,12 @@ export const ConfigSchema = v.object({ measurementId: v.string() }) ), - tablePageLength: v.number() + tablePageLength: v.number(), + contact: v.object({ + issues: v.object({ + email: v.string() + }) + }) }) const readConfig = (config) => { diff --git a/src/assets/scss/index.scss b/src/assets/scss/index.scss index cb4e0840b..c9e16d1a6 100644 --- a/src/assets/scss/index.scss +++ b/src/assets/scss/index.scss @@ -214,3 +214,11 @@ code * { .app-content__markdown img { max-width: 100%; } + +.schema-issue { + display: flex; + flex-direction: column; + padding-bottom: 1em; +} + +.schema-issue p { padding: 0; margin: 0} diff --git a/src/filters/filters.js b/src/filters/filters.js index 9bd633ef9..02bd16f5a 100644 --- a/src/filters/filters.js +++ b/src/filters/filters.js @@ -8,6 +8,7 @@ import { checkToolDeepLink } from './checkToolDeepLink.js' import pluralize from 'pluralize' import { datasetSlugToReadableName } from '../utils/datasetSlugToReadableName.js' import { getDatasetGuidanceUrl } from './getDatasetConfig.js' +import { schemaIssues } from './schemaIssues.js' /** maps dataset status (as returned by `fetchLpaOverview` middleware to a * CSS class used by the govuk-tag component @@ -43,6 +44,7 @@ const addFilters = (nunjucksEnv) => { nunjucksEnv.addFilter('pluralise', pluralize) nunjucksEnv.addFilter('checkToolDeepLink', checkToolDeepLink) nunjucksEnv.addFilter('getDatasetGuidanceUrl', getDatasetGuidanceUrl) + nunjucksEnv.addFilter('schemaIssues', schemaIssues) } export default addFilters diff --git a/src/filters/schemaIssues.js b/src/filters/schemaIssues.js new file mode 100644 index 000000000..31d923d48 --- /dev/null +++ b/src/filters/schemaIssues.js @@ -0,0 +1,21 @@ +import * as v from 'valibot' + +/** + * Takes an Error and if it's a ValiError, returns an array of issue summaries (with paths). Empty array otherwise. + * + * Useful to show the schema issues in a consise way. + * + * @param {Error} error + * @returns { { path: string[], message: string}[] } + */ +export function schemaIssues (error) { + const issues = [] + if (v.isValiError(error)) { + for (const issue of error.issues) { + if (issue.path) { + issues.push({ path: issue.path.map(elem => elem.key), message: issue.message }) + } + } + } + return issues +} diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 145172c89..91cd5ba96 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -6,6 +6,7 @@ import { fetchOne, FetchOptions, FetchOneFallbackPolicy, fetchMany, renderTempla import * as v from 'valibot' import { pagination } from '../utils/pagination.js' import datasette from '../services/datasette.js' +import { errorTemplateContext, MiddlewareError } from '../utils/errors.js' /** * Middleware. Set `req.handlerName` to a string that will identify @@ -97,7 +98,8 @@ export function validateQueryParamsFn (req, res, next) { req.parsedParams = v.parse(this.schema || v.any(), req.params) next() } catch (error) { - res.status(400).render('errorPages/400', {}) + const err = new MiddlewareError('Query params validation error', 400, { cause: error }) + res.status(400).render(err.template, { ...errorTemplateContext(), err }) } } @@ -126,9 +128,7 @@ export const show404IfPageNumberNotInRange = (req, res, next) => { } if (pageNumber > dataRange.maxPageNumber || pageNumber < 1) { - const error = new Error('page number not in range') - // @ts-ignore - error.status = 404 + const error = new MiddlewareError('page number not in range', 404) return next(error) } next() diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 8f6cf9fcd..62d4881f5 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -1,5 +1,6 @@ import * as v from 'valibot' import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getErrorSummaryItems, getSetBaseSubPath, getSetDataRange, prepareIssueDetailsTemplateParams, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' +import { MiddlewareError } from '../utils/errors.js' import { fetchMany, fetchOne, FetchOptions, renderTemplate } from './middleware.builders.js' import { issueErrorMessageHtml } from '../utils/utils.js' @@ -77,8 +78,7 @@ export const prepareEntry = (req, res, next) => { const { pageNumber } = req.parsedParams if (!issues[pageNumber - 1] || !resources) { - const error = new Error('Missing required values on request object') - error.status = 404 + const error = new MiddlewareError('Missing required values on request object', 404) return next(error) } diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index 518fea9be..fe0d7639b 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -13,6 +13,7 @@ import { templateSchema } from '../routes/schemas.js' import { render } from '../utils/custom-renderer.js' import datasette from '../services/datasette.js' import * as v from 'valibot' +import { errorTemplateContext, MiddlewareError } from '../utils/errors.js' export const FetchOptions = { /** @@ -30,7 +31,8 @@ const datasetOverride = (val, req) => { return 'digital-land' } if (val === FetchOptions.fromParams) { - console.assert(req.params.dataset, 'no "dataset" in request params') + logger.warn('no "dataset" in request params', + { types: types.App, endpoint: req.originalUrl, params: req.params }) return req.params.dataset } else if (val === FetchOptions.performanceDb) { return 'performance' @@ -40,7 +42,8 @@ const datasetOverride = (val, req) => { } const fetchOneFallbackPolicy = (req, res, next) => { - res.status(404).render('errorPages/404', {}) + const err = new MiddlewareError('Not found', 404) + res.status(err.statusCode).render(err.template, { ...errorTemplateContext(), err }) } /** diff --git a/src/routes/guidance.js b/src/routes/guidance.js index 5a3e79157..b0f9edeb2 100644 --- a/src/routes/guidance.js +++ b/src/routes/guidance.js @@ -1,7 +1,4 @@ import express from 'express' -/* import nunjucks from 'nunjucks' -import config from '../../config/index.js' -import logger from '../utils/logger.js' */ const router = express.Router() @@ -11,82 +8,4 @@ router.get('/*', (req, res) => { return res.redirect(302, `https://www.planning.data.gov.uk/guidance/${path}`) }) -/* function getNavigationStructure () { - if (!config.guidanceNavigation) { - logger.error('Guidance navigation configuration is missing') - return { title: 'Guidance', items: [] } - } - - return config.guidanceNavigation -} */ - -/* function extractUrlsFromItems (items) { - let urls = [] - items.forEach(item => { - if (item.url) { - urls.push(item.url) - } - if (item.items) { - urls = urls.concat(extractUrlsFromItems(item.items)) - } - }) - return urls -} - -function checkPathExists (items, path) { - const fullPath = `/guidance${path}`.replace(/\/$/, '') - const urls = extractUrlsFromItems(items) - - return urls.includes(fullPath) -} - -router.get('/*', (req, res) => { - const path = req.params[0].replace(/[^a-zA-Z0-9/-]/g, '') - return res.redirect(302, `https://www.planning.data.gov.uk/guidance${path}`) - - try { - const navigationStructure = getNavigationStructure() - const path = req.params[0].replace(/[^a-zA-Z0-9/-]/g, '') - - if (path.includes('..') || !checkPathExists(navigationStructure.items, req.path)) { - throw new Error('Invalid path') - } - - let templatePath = '' - - switch (path) { - case '': - case '/': - templatePath = 'guidance/index' - break - case 'specifications': - case 'specifications/': - templatePath = 'guidance/specifications/index' - break - default: - templatePath = `guidance/${path}` - } - - const guidancePage = nunjucks.render(`${templatePath}.md`, { - navigation: navigationStructure - }) - - res.send(guidancePage) - } catch (error) { - if (error?.message === 'template not found' || error?.message === 'Invalid path') { - logger.info('Guidance page not found', { type: 'App', path: req.path }) - - res.status(404).render('errorPages/404', {}) - } else { - logger.error('Error rendering guidance page', { - type: 'App', - path: req.path, - error: error.message - }) - - res.status(500).render('errorPages/500', {}) - } - } -}) */ - export default router diff --git a/src/routes/schemas.js b/src/routes/schemas.js index 774479939..063d525e5 100644 --- a/src/routes/schemas.js +++ b/src/routes/schemas.js @@ -4,14 +4,16 @@ */ import * as v from 'valibot' +import { MiddlewareError } from '../utils/errors.js' export const EmptyParams = v.object({}) -export const UptimeParams = v.object({ - upTime: v.string() -}) -export const ErrorParams = v.strictObject({ - err: v.object({}) +export const ErrorPageParams = v.object({ + err: v.instance(MiddlewareError), + env: v.string(), + supportEmail: v.pipe(v.string(), v.email()), + uptime: v.optional(v.string()), + downtime: v.optional(v.string()) }) export const NonEmptyString = v.pipe(v.string(), v.nonEmpty()) @@ -300,9 +302,7 @@ export const templateSchema = new Map([ ['organisations/issueTable.html', OrgIssueTable], ['organisations/issueDetails.html', OrgIssueDetails], - ['errorPages/503', UptimeParams], - ['errorPages/500', ErrorParams], - ['errorPages/404', EmptyParams], + ['errorPages/error.njk', ErrorPageParams], ['privacy-notice.html', EmptyParams], ['landing.html', EmptyParams], ['cookies.html', EmptyParams], diff --git a/src/serverSetup/errorHandlers.js b/src/serverSetup/errorHandlers.js index a8a6f692e..81322a806 100644 --- a/src/serverSetup/errorHandlers.js +++ b/src/serverSetup/errorHandlers.js @@ -1,7 +1,9 @@ import logger from '../utils/logger.js' import { types } from '../utils/logging.js' +import { MiddlewareError, errorTemplateContext } from '../utils/errors.js' export function setupErrorHandlers (app) { + const errContext = errorTemplateContext() app.use((err, req, res, next) => { logger.error({ type: types.Response, @@ -17,17 +19,19 @@ export function setupErrorHandlers (app) { return next(err) } - err.template = err.template || (err.status && `errorPages/${err.status}`) || 'errorPages/500' - if (err.redirect) { return res.redirect(err.redirect) } - err.status = err.status || 500 + const middlewareError = err instanceof MiddlewareError ? err : new MiddlewareError('Internal server error', 500, { cause: err }) try { - res.status(err.status).render(err.template, { err }) + res.status(middlewareError.statusCode).render(err.template, { ...errContext, err: middlewareError }) } catch (e) { - res.status(err.status).render('errorPages/500', { err }) + logger.error('Failed to render error page.', { + type: types.Response, errorMessage: e.message, errorStack: e.stack, originalError: err.message, endpoint: req.originalUrl + }) + const renderError = new MiddlewareError('Failed to render error page', 500, { cause: e }) + res.status(500).render('errorPages/error.njk', { ...errContext, err: renderError }) } }) @@ -41,6 +45,7 @@ export function setupErrorHandlers (app) { endpoint: req.originalUrl, message: 'not found' }) - res.status(404).render('errorPages/404') + const err = new MiddlewareError('Not found', 404) + res.status(err.statusCode).render(err.template, { ...errContext, err }) }) } diff --git a/src/serverSetup/middlewares.js b/src/serverSetup/middlewares.js index bf2220b67..0ad424640 100644 --- a/src/serverSetup/middlewares.js +++ b/src/serverSetup/middlewares.js @@ -6,6 +6,7 @@ import { types } from '../utils/logging.js' import hash from '../utils/hasher.js' import config from '../../config/index.js' import { preventIndexing } from '../middleware/common.middleware.js' +import { MiddlewareError, errorTemplateContext } from '../utils/errors.js' export function setupMiddlewares (app) { app.use((req, res, next) => { @@ -34,7 +35,8 @@ export function setupMiddlewares (app) { app.use((req, res, next) => { const serviceDown = config.maintenance.serviceUnavailable || false if (serviceDown) { - res.status(503).render('errorPages/503', { upTime: config.maintenance.upTime }) + const err = new MiddlewareError('Service unavailable', 503) + res.status(err.statusCode).render(err.template, { ...errorTemplateContext(), err, uptime: config.maintenance.upTime }) } else { next() } diff --git a/src/services/datasette.js b/src/services/datasette.js index b931c05c4..683d3b03d 100644 --- a/src/services/datasette.js +++ b/src/services/datasette.js @@ -9,6 +9,7 @@ export default { * Executes a SQL query on the Datasette instance and returns the results. * * @param {string} query - The SQL query to execute. + * @param {string} database - The name of the database to query. Defaults to 'digital-land'. * @returns {Promise<{data: object, formattedData: object[]}>} - A promise that resolves to an object with the following properties: * - `data`: The raw data returned by Datasette. * - `formattedData`: The formatted data, with columns and rows parsed into a usable format. diff --git a/src/utils/errors.js b/src/utils/errors.js new file mode 100644 index 000000000..6d49e5254 --- /dev/null +++ b/src/utils/errors.js @@ -0,0 +1,26 @@ +import config from '../../config/index.js' + +const context = { + env: process.env.NODE_ENV || process.env.ENVIRONMENT || 'local', + supportEmail: config.contact.issues.email +} + +export function errorTemplateContext () { + return { ...context } +} + +export class MiddlewareError extends Error { + /** + * @param {string} message + * @param {number} statusCode status to be returned to the client + * @param {{template?: string, cause?: Error}} options template path + */ + constructor (message, statusCode, options = {}) { + super(message, options) + if (typeof statusCode !== 'number') { + throw new Error(`statusCode should be a number: ${statusCode}`) + } + this.statusCode = statusCode + this.template = options?.template ?? 'errorPages/error.njk' + } +} diff --git a/src/views/check/results/failedUrlRequest.html b/src/views/check/results/failedUrlRequest.html index ea93f1ec8..7e91af4a3 100644 --- a/src/views/check/results/failedUrlRequest.html +++ b/src/views/check/results/failedUrlRequest.html @@ -22,7 +22,7 @@

  • Error code: {{ error.errCode}}
  • Error Message: {{ error.errMsg }}
    • - Try again + Try again {% endblock %} \ No newline at end of file diff --git a/src/views/errorPages/400.html b/src/views/errorPages/400.html deleted file mode 100644 index 883d3738f..000000000 --- a/src/views/errorPages/400.html +++ /dev/null @@ -1,19 +0,0 @@ -{% extends "layouts/main.html" %} - -{% set pageName = 'Invalid Parameters' %} - -{% block content %} -
    -
    -

    {{pageName}}

    - -

    If you typed the web address, check it is correct.

    - -

    If you pasted the web address, check you copied the entire address.

    - -

    If the web address is correct or you selected a link or button send an email - to digitalland@communities.gov.uk.

    - -
    -
    -{% endblock %} \ No newline at end of file diff --git a/src/views/errorPages/404.html b/src/views/errorPages/404.html deleted file mode 100644 index dc1580562..000000000 --- a/src/views/errorPages/404.html +++ /dev/null @@ -1,19 +0,0 @@ -{% extends "layouts/main.html" %} - -{% set pageName = 'Page not found' %} - -{% block content %} -
    -
    -

    {{pageName}}

    - -

    If you typed the web address, check it is correct.

    - -

    If you pasted the web address, check you copied the entire address.

    - -

    If the web address is correct or you selected a link or button send an email - to digitalland@communities.gov.uk.

    - -
    -
    -{% endblock %} \ No newline at end of file diff --git a/src/views/errorPages/500.html b/src/views/errorPages/500.html deleted file mode 100644 index 2182ac7f9..000000000 --- a/src/views/errorPages/500.html +++ /dev/null @@ -1,15 +0,0 @@ -{% extends "layouts/main.html" %} - -{% set pageName = 'Sorry, there’s a problem with the service' %} - -{% block content %} -
    -
    -

    {{pageName}}

    -

    Try again later.

    -

    - You'll need to start from the beginning of the service. -

    -
    -
    -{% endblock %} diff --git a/src/views/errorPages/503.html b/src/views/errorPages/503.html deleted file mode 100644 index ae47938b7..000000000 --- a/src/views/errorPages/503.html +++ /dev/null @@ -1,17 +0,0 @@ -{% extends "layouts/main.html" %} - -{% set pageName = 'Sorry, the service is unavailable' %} - -{% block content %} -
    -
    -

    {{pageName}}

    - {% if downtime %} -

    The service will be unavailable from {{downtime}}.

    - {% endif %} - {% if upTime %} -

    You’ll be able to use the service from {{upTime}}.

    - {% endif %} -
    -
    -{% endblock %} diff --git a/src/views/errorPages/504.html b/src/views/errorPages/504.html deleted file mode 100644 index 2182ac7f9..000000000 --- a/src/views/errorPages/504.html +++ /dev/null @@ -1,15 +0,0 @@ -{% extends "layouts/main.html" %} - -{% set pageName = 'Sorry, there’s a problem with the service' %} - -{% block content %} -
    -
    -

    {{pageName}}

    -

    Try again later.

    -

    - You'll need to start from the beginning of the service. -

    -
    -
    -{% endblock %} diff --git a/src/views/errorPages/error.njk b/src/views/errorPages/error.njk new file mode 100644 index 000000000..8fc8adc82 --- /dev/null +++ b/src/views/errorPages/error.njk @@ -0,0 +1,76 @@ +{% extends "layouts/main.html" %} + +{% set pageNames = { + '400': 'Invalid parameters', + '404': 'Page not found', + '412': 'Yo momma too fat', + '500': 'Sorry, there’s a problem with the service', + '503': 'Sorry, the service is unavailable', + '504': 'Sorry, there’s a problem with the service'} %} + +{% set pageName = pageNames[err.statusCode] or 'Sorry, there’s a problem with the service' %} + +{% block content %} +
    +
    +

    {{ pageName }}

    + + {% if err.statusCode === 400 %} +

    If you typed the web address, check it is correct.

    +

    If you pasted the web address, check you copied the entire address.

    +

    If the web address is correct or you selected a link or button send an email to {{supportEmail}}.

    + + {% elif err.statusCode === 404 %} +

    If you typed the address, check it is correct.

    +

    If you copied the web address, check you copied the entire address.

    +

    If the web address is correct or you selected a link or button, send an email to {{supportEmail}} for help.

    + +

    If you used an old address, it's possible it has expired. You can start the process from the beginning

    + + {% elif err.statusCode === 503 %} + {% if downtime %} +

    The service will be unavailable from {{downtime}}.

    + {% endif %} + {% if uptime %} +

    You’ll be able to use the service from {{uptime}}.

    + {% endif %} + + {% else %} +

    Try again later.

    +

    You'll need to start from the beginning of the service.

    + {% endif %} + +
    +
    + +
    + {% if env !== 'production' and err.statusCode !== 404 %} +
    + Error details + 
    +            {{ err.stack | safe }}
+
    + {% if err.cause %} +

    Cause:

    + 
    +            {{ err.cause.stack | safe }}
+
    + {% endif %} +
    + + {% set issues = (err.cause | schemaIssues) %} + {% if issues.length > 0 %} +
    + Schema Issues + {% for issue in issues %} +
    +

    Path: [{{ issue.path.join(', ') }}]

    +

    Message:: {{ issue.message }}

    +
    + {% endfor %} +
    + + {% endif %} + {% endif %} +
    +{% endblock %} diff --git a/test/unit/default-http-errors.test.js b/test/unit/default-http-errors.test.js index 7743bb375..a80de225a 100644 --- a/test/unit/default-http-errors.test.js +++ b/test/unit/default-http-errors.test.js @@ -1,15 +1,28 @@ import { describe, expect, it } from 'vitest' import { setupNunjucks } from '../../src/serverSetup/nunjucks.js' +import { MiddlewareError } from '../../src/utils/errors.js' const nunjucks = setupNunjucks({ datasetNameMapping: new Map() }) describe('render default HTTP error pages', () => { - const statuses = ['400', '404', '500', '503', '504'] + const statuses = [400, 404, 500, 503, 504] + const templateContext = { supportEmail: 'foo@example.com' } for (const status of statuses) { it(`status ${status}`, () => { - const page = nunjucks.render(`errorPages/${status}.html`, {}) + const err = new MiddlewareError(`Failed with ${status}`, status) + const page = nunjucks.render(err.template, { err, env: 'local', ...templateContext }) expect(page).toBeDefined() + if (status !== 404) { + expect(page).to.contain('Error details') + } }) } + + it('does not include error detaiils in production', () => { + const err = new MiddlewareError('Failed in prod', 500) + const page = nunjucks.render(err.template, { err, env: 'production', ...templateContext }) + expect(page).toBeDefined() + expect(page).not.to.contain('Error details') + }) }) diff --git a/test/unit/middleware/common.middleware.test.js b/test/unit/middleware/common.middleware.test.js index 23a146ea8..2daf74cb9 100644 --- a/test/unit/middleware/common.middleware.test.js +++ b/test/unit/middleware/common.middleware.test.js @@ -19,6 +19,7 @@ import { prepareIssueDetailsTemplateParams, preventIndexing } from '../../../src/middleware/common.middleware' +import { MiddlewareError } from '../../../src/utils/errors.js' import logger from '../../../src/utils/logger' import datasette from '../../../src/services/datasette.js' import performanceDbApi from '../../../src/services/performanceDbApi.js' @@ -55,7 +56,7 @@ describe('show404IfPageNumberNotInRange middleware', () => { const res = {} const next = vi.fn((err) => { expect(err instanceof Error).toBe(true) - expect(err.status).toBe(404) + expect(err.statusCode).toBe(404) expect(err.message).toBe('page number not in range') }) show404IfPageNumberNotInRange(req, res, next) @@ -69,7 +70,7 @@ describe('show404IfPageNumberNotInRange middleware', () => { const res = {} const next = vi.fn((err) => { expect(err instanceof Error).toBe(true) - expect(err.status).toBe(404) + expect(err.statusCode).toBe(404) expect(err.message).toBe('page number not in range') }) show404IfPageNumberNotInRange(req, res, next) diff --git a/test/unit/publishRequestAPI.test.js b/test/unit/publishRequestAPI.test.js index b744c8ff5..0c212970f 100644 --- a/test/unit/publishRequestAPI.test.js +++ b/test/unit/publishRequestAPI.test.js @@ -3,6 +3,7 @@ import { postFileRequest, postUrlRequest, getRequestData } from '../../src/servi import axios from 'axios' import RequestData from '../../src/models/requestData.js' import config from '../../config/index.js' +import { MiddlewareError } from '../../src/utils/errors.js' vi.mock('axios') @@ -123,10 +124,7 @@ describe('asyncRequestApi', () => { it('should throw an error if the GET request fails with status 500', async () => { const resultId = '123' - const expectedError = new Error('HTTP error! status: 500') - expectedError.message = 'HTTP error! status: 500' - expectedError.status = 500 - + const expectedError = new MiddlewareError('HTTP error! status: 500', 500) const expectedResponse = { status: 500 } axios.get.mockRejectedValue(expectedResponse) From c61a92af5d7d54c6be96f0ff8b90ba76ed9b3fef Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Wed, 8 Jan 2025 12:14:49 +0000 Subject: [PATCH 02/17] remove unused import --- test/unit/middleware/common.middleware.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/middleware/common.middleware.test.js b/test/unit/middleware/common.middleware.test.js index 2daf74cb9..b4d4122b9 100644 --- a/test/unit/middleware/common.middleware.test.js +++ b/test/unit/middleware/common.middleware.test.js @@ -19,7 +19,6 @@ import { prepareIssueDetailsTemplateParams, preventIndexing } from '../../../src/middleware/common.middleware' -import { MiddlewareError } from '../../../src/utils/errors.js' import logger from '../../../src/utils/logger' import datasette from '../../../src/services/datasette.js' import performanceDbApi from '../../../src/services/performanceDbApi.js' From 9d62ab3a2749b433d0fa6f9f938d599538347f3f Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Wed, 8 Jan 2025 12:28:44 +0000 Subject: [PATCH 03/17] fix: use the middleware error --- src/serverSetup/errorHandlers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/serverSetup/errorHandlers.js b/src/serverSetup/errorHandlers.js index 81322a806..e9fc00a5d 100644 --- a/src/serverSetup/errorHandlers.js +++ b/src/serverSetup/errorHandlers.js @@ -25,7 +25,7 @@ export function setupErrorHandlers (app) { const middlewareError = err instanceof MiddlewareError ? err : new MiddlewareError('Internal server error', 500, { cause: err }) try { - res.status(middlewareError.statusCode).render(err.template, { ...errContext, err: middlewareError }) + res.status(middlewareError.statusCode).render(middlewareError.template, { ...errContext, err: middlewareError }) } catch (e) { logger.error('Failed to render error page.', { type: types.Response, errorMessage: e.message, errorStack: e.stack, originalError: err.message, endpoint: req.originalUrl From 3714dba45d78d76a7030fb5419b256a8311ad19c Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Wed, 8 Jan 2025 12:45:04 +0000 Subject: [PATCH 04/17] update default config with contact email --- config/default.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/config/default.yaml b/config/default.yaml index 2224d7b98..61dcb21ea 100644 --- a/config/default.yaml +++ b/config/default.yaml @@ -42,6 +42,10 @@ email: { validations: { maxFileSize: 100000000 } +contact: + issues: + # description: should be used for general issues, e.g. if user lands on 404 doesn't know how to proceed + email: digitalland@communities.gov.uk datasetsConfig: article-4-direction: guidanceUrl: /guidance/specifications/article-4-direction From f91d6b98ad756df636b1c6747b3b974408056c1c Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Wed, 8 Jan 2025 16:09:32 +0000 Subject: [PATCH 05/17] In case of check tool errors, prompt user to start over. --- src/views/check/results/failedFileRequest.html | 2 +- src/views/check/results/failedUrlRequest.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/views/check/results/failedFileRequest.html b/src/views/check/results/failedFileRequest.html index bd1888177..279e8aa88 100644 --- a/src/views/check/results/failedFileRequest.html +++ b/src/views/check/results/failedFileRequest.html @@ -19,7 +19,7 @@

  • the file is in the right format
  • there is no errors in the file formatting (for example, a missing comma)
    • - Try again + Try again {% endblock %} \ No newline at end of file diff --git a/src/views/check/results/failedUrlRequest.html b/src/views/check/results/failedUrlRequest.html index 7e91af4a3..68ee40470 100644 --- a/src/views/check/results/failedUrlRequest.html +++ b/src/views/check/results/failedUrlRequest.html @@ -22,7 +22,7 @@

  • Error code: {{ error.errCode}}
  • Error Message: {{ error.errMsg }}
    • - Try again + Try again {% endblock %} \ No newline at end of file From d6f7ced841b15e2df3b4dfc9df2f7bf27faef238 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Wed, 8 Jan 2025 16:09:52 +0000 Subject: [PATCH 06/17] minor edits --- src/services/asyncRequestApi.js | 2 +- src/views/errorPages/error.njk | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/services/asyncRequestApi.js b/src/services/asyncRequestApi.js index e96f7ea59..5e4d938de 100644 --- a/src/services/asyncRequestApi.js +++ b/src/services/asyncRequestApi.js @@ -60,6 +60,6 @@ export const getRequestData = async (resultId) => { return new ResultData(response.data) } catch (error) { - throw new Error(`HTTP error! status: ${error.status}`) + throw new Error(`HTTP error! status: ${error.status}: ${error.message}`, { cause: error }) } } diff --git a/src/views/errorPages/error.njk b/src/views/errorPages/error.njk index 8fc8adc82..9f17ee972 100644 --- a/src/views/errorPages/error.njk +++ b/src/views/errorPages/error.njk @@ -25,7 +25,7 @@

    If you copied the web address, check you copied the entire address.

    If the web address is correct or you selected a link or button, send an email to {{supportEmail}} for help.

    -

    If you used an old address, it's possible it has expired. You can start the process from the beginning

    +

    If you used an old address, it's possible it has expired. You can start the process from the beginning.

    {% elif err.statusCode === 503 %} {% if downtime %} From 603605aee202aba652da6ceb6617abacce5f2595 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Wed, 8 Jan 2025 16:40:53 +0000 Subject: [PATCH 07/17] fix tests --- test/unit/publishRequestAPI.test.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/unit/publishRequestAPI.test.js b/test/unit/publishRequestAPI.test.js index 0c212970f..898663e03 100644 --- a/test/unit/publishRequestAPI.test.js +++ b/test/unit/publishRequestAPI.test.js @@ -3,7 +3,6 @@ import { postFileRequest, postUrlRequest, getRequestData } from '../../src/servi import axios from 'axios' import RequestData from '../../src/models/requestData.js' import config from '../../config/index.js' -import { MiddlewareError } from '../../src/utils/errors.js' vi.mock('axios') @@ -112,10 +111,10 @@ describe('asyncRequestApi', () => { const resultId = '123' const expectedError = new Error('HTTP error! status: 404') - expectedError.message = 'HTTP error! status: 404' + expectedError.message = 'HTTP error! status: 404: Failed to do the thing' expectedError.status = 404 - const expectedResponse = { status: 404 } + const expectedResponse = { status: 404, message: 'Failed to do the thing' } axios.get.mockRejectedValue(expectedResponse) await expect(getRequestData(resultId)).rejects.toThrow(expectedError) @@ -124,8 +123,8 @@ describe('asyncRequestApi', () => { it('should throw an error if the GET request fails with status 500', async () => { const resultId = '123' - const expectedError = new MiddlewareError('HTTP error! status: 500', 500) - const expectedResponse = { status: 500 } + const expectedError = new Error('HTTP error! status: 500: Failed to do the thing') + const expectedResponse = { status: 500, message: 'Failed to do the thing' } axios.get.mockRejectedValue(expectedResponse) await expect(getRequestData(resultId)).rejects.toThrow(expectedError) From ceb8819b3b86d71e7e4340681469d13188e42c82 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Thu, 9 Jan 2025 10:50:36 +0000 Subject: [PATCH 08/17] minor edit --- src/middleware/common.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 91cd5ba96..8197ba6aa 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -99,7 +99,7 @@ export function validateQueryParamsFn (req, res, next) { next() } catch (error) { const err = new MiddlewareError('Query params validation error', 400, { cause: error }) - res.status(400).render(err.template, { ...errorTemplateContext(), err }) + res.status(err.statusCode).render(err.template, { ...errorTemplateContext(), err }) } } From 73d58b1ed964ac5a16365046d8a3bd85c36df894 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Thu, 9 Jan 2025 10:57:04 +0000 Subject: [PATCH 09/17] add JSDoc string to MiddlewareError --- src/utils/errors.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/utils/errors.js b/src/utils/errors.js index 6d49e5254..edddd0248 100644 --- a/src/utils/errors.js +++ b/src/utils/errors.js @@ -9,6 +9,11 @@ export function errorTemplateContext () { return { ...context } } +/** + * Use this class if you want to display specific HTTP error page. + * + * Uses the `errorPages/error.njk` template, but it can be overriden via options. + */ export class MiddlewareError extends Error { /** * @param {string} message From 44ffec6b795889ed4c2ec2c8f08f52e726c3645a Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Thu, 9 Jan 2025 11:00:27 +0000 Subject: [PATCH 10/17] fix linting --- src/utils/errors.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/errors.js b/src/utils/errors.js index edddd0248..13d1ff84c 100644 --- a/src/utils/errors.js +++ b/src/utils/errors.js @@ -11,7 +11,7 @@ export function errorTemplateContext () { /** * Use this class if you want to display specific HTTP error page. - * + * * Uses the `errorPages/error.njk` template, but it can be overriden via options. */ export class MiddlewareError extends Error { From a476ad428a41854c515a04f869155b1342a0b260 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Thu, 9 Jan 2025 11:09:53 +0000 Subject: [PATCH 11/17] format the error page template --- src/views/errorPages/error.njk | 54 ++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/src/views/errorPages/error.njk b/src/views/errorPages/error.njk index 9f17ee972..a1048953c 100644 --- a/src/views/errorPages/error.njk +++ b/src/views/errorPages/error.njk @@ -43,34 +43,36 @@ -
    - {% if env !== 'production' and err.statusCode !== 404 %} + {% if env !== 'production' and err.statusCode !== 404 %} +
    + +
    + Error details + 
    +          {{ err.stack | safe }}
+
    + {% if err.cause %} +

    Cause:

    + 
    +          {{ err.cause.stack | safe }}
+
    + {% endif %} +
    + + {% set issues = (err.cause | schemaIssues) %} + {% if issues.length > 0 %}
    - Error details - 
    -            {{ err.stack | safe }}
-
    - {% if err.cause %} -

    Cause:

    - 
    -            {{ err.cause.stack | safe }}
-
    - {% endif %} + Schema Issues + {% for issue in issues %} +
    +

    Path: [{{ issue.path.join(', ') }}]

    +

    Message:: {{ issue.message }}

    +
    + {% endfor %}
    - {% set issues = (err.cause | schemaIssues) %} - {% if issues.length > 0 %} -
    - Schema Issues - {% for issue in issues %} -
    -

    Path: [{{ issue.path.join(', ') }}]

    -

    Message:: {{ issue.message }}

    -
    - {% endfor %} -
    - - {% endif %} {% endif %} -
    +
    + {% endif %} + {% endblock %} From b079e172b319a7f67150a5590bfbf15cd7ab3bea Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Thu, 9 Jan 2025 13:45:40 +0000 Subject: [PATCH 12/17] remove debug code --- src/views/errorPages/error.njk | 1 - 1 file changed, 1 deletion(-) diff --git a/src/views/errorPages/error.njk b/src/views/errorPages/error.njk index a1048953c..984dce96d 100644 --- a/src/views/errorPages/error.njk +++ b/src/views/errorPages/error.njk @@ -3,7 +3,6 @@ {% set pageNames = { '400': 'Invalid parameters', '404': 'Page not found', - '412': 'Yo momma too fat', '500': 'Sorry, there’s a problem with the service', '503': 'Sorry, the service is unavailable', '504': 'Sorry, there’s a problem with the service'} %} From 9760e44f540617398f8d54e4ac2c2fa1397ebb37 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Thu, 9 Jan 2025 17:14:04 +0000 Subject: [PATCH 13/17] better schema Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- config/util.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/util.js b/config/util.js index bb8b57649..2f004de7b 100644 --- a/config/util.js +++ b/config/util.js @@ -97,7 +97,7 @@ export const ConfigSchema = v.object({ tablePageLength: v.number(), contact: v.object({ issues: v.object({ - email: v.string() + email: v.pipe(v.string(), v.email()) }) }) }) From f1077a6f00398baa2ce2753b784e818b3b7cc333 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 10 Jan 2025 10:59:18 +0000 Subject: [PATCH 14/17] more details in error message --- src/middleware/entryIssueDetails.middleware.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 62d4881f5..1645bb2eb 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -78,7 +78,8 @@ export const prepareEntry = (req, res, next) => { const { pageNumber } = req.parsedParams if (!issues[pageNumber - 1] || !resources) { - const error = new MiddlewareError('Missing required values on request object', 404) + const details = `issues[pageNumber-1]=${issues[pageNumber-1] ? 'present' : 'missing'} resources=${resources ? 'present' : 'missing'}` + const error = new MiddlewareError(`Missing required values on request object: ${details}`, 404) return next(error) } From 8c63fd232430ad239895b5f640c01957a89cb8b1 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 10 Jan 2025 11:01:22 +0000 Subject: [PATCH 15/17] linting --- src/middleware/entryIssueDetails.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 1645bb2eb..8301111a6 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -78,7 +78,7 @@ export const prepareEntry = (req, res, next) => { const { pageNumber } = req.parsedParams if (!issues[pageNumber - 1] || !resources) { - const details = `issues[pageNumber-1]=${issues[pageNumber-1] ? 'present' : 'missing'} resources=${resources ? 'present' : 'missing'}` + const details = `issues[pageNumber-1]=${issues[pageNumber - 1] ? 'present' : 'missing'} resources=${resources ? 'present' : 'missing'}` const error = new MiddlewareError(`Missing required values on request object: ${details}`, 404) return next(error) } From eba05e84abfc0c2133e65ec552737c73a78dafc0 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 10 Jan 2025 11:09:30 +0000 Subject: [PATCH 16/17] update unit tests --- test/unit/middleware/entryIssueDetails.middleware.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/middleware/entryIssueDetails.middleware.test.js b/test/unit/middleware/entryIssueDetails.middleware.test.js index 1ac73d14f..fa1aeaec6 100644 --- a/test/unit/middleware/entryIssueDetails.middleware.test.js +++ b/test/unit/middleware/entryIssueDetails.middleware.test.js @@ -128,7 +128,7 @@ describe('entryIssueDetails.middleware.test.js', () => { const next = vi.fn() prepareEntry(req, res, next) - expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object')) + expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object: issues[pageNumber-1]=missing resources=present')) }) it('should throw error if resources is missing', () => { @@ -149,7 +149,7 @@ describe('entryIssueDetails.middleware.test.js', () => { prepareEntry(req, res, next) - expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object')) + expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object: issues[pageNumber-1]=present resources=missing')) }) }) From 6866391f852bfd154ae9d5700ea76ed9eb60ced4 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 10 Jan 2025 11:17:19 +0000 Subject: [PATCH 17/17] fix flaky unit tests --- test/unit/views/organisations/lpaOverviewPage.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/views/organisations/lpaOverviewPage.test.js b/test/unit/views/organisations/lpaOverviewPage.test.js index e4b3157bf..b6552990d 100644 --- a/test/unit/views/organisations/lpaOverviewPage.test.js +++ b/test/unit/views/organisations/lpaOverviewPage.test.js @@ -67,13 +67,13 @@ describe(`LPA Overview Page (seed: ${seed})`, () => { }) it('The correct number of dataset cards are rendered with the correct titles in group "statutory"', () => { - if (params.datasets.statutory) { + if (params.datasets.statutory && params.datasets.statutory.length > 0) { datasetGroup({ expect }, 'statutory', params.datasets.statutory, document) } }) it('The correct number of dataset cards are rendered with the correct titles in group "other"', () => { - if (params.datasets.other) { + if (params.datasets.other && params.datasets.other.length > 0) { datasetGroup({ expect }, 'other', params.datasets.other, document) const infoText = document.querySelector('.org-membership-info').textContent