Merge pull request #768 from digital-land/rosado/189-error-pages

This PR removes all the errorPages/xxx.html templates and replaces them with a single error template. It introduces a Error subclass MiddlewareError that should be use when we need to force the page to display specific error.

The error pages include stack traces and (when relevant) schema validation issues. Both are displayed only on when the environment process.env.NODE_ENV || process.env.ENVIRONMENT is not set to production. Also, no errors details are shown on 404 page - it would be noise.

Also: the templates for URL/file upload errors in the check tool provide /check link to start over (it used to be /url and /upload).

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: [email protected]
 datasetsConfig:
   article-4-direction:
     guidanceUrl: /guidance/specifications/article-4-direction

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.pipe(v.string(), v.email())
+    })
+  })
 })
 
 const readConfig = (config) => {

src/assets/scss/index.scss

@@ -215,3 +215,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}

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

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
+}

src/middleware/common.middleware.js

@@ -6,6 +6,7 @@ import { fetchMany, fetchOne, FetchOneFallbackPolicy, FetchOptions, 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(err.statusCode).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()

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,8 @@ 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 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)
   }
 

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 })
 }
 
 /**

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

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())
@@ -301,9 +303,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],

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(middlewareError.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 })
   })
 }

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()
     }

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 })
   }
 }

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.

src/utils/errors.js

@@ -0,0 +1,31 @@
+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 }
+}
+
+/**
+ * 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
+   * @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'
+  }
+}

src/views/check/results/failedFileRequest.html

@@ -19,7 +19,7 @@ <h1 class="govuk-heading-l">
                 <li>the file is in the right format</li>
                 <li>there is no errors in the file formatting (for example, a missing comma)</li>
             </ul>
-            <a href="/upload" class="govuk-link">Try again</a>
+            <a href="/check" class="govuk-link">Try again</a>
         </div>
     </div>
 {% endblock %}