From b93b3c1f3b800a4d7504f68dfcf046403551da46 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Thu, 24 Oct 2024 17:07:57 +0100 Subject: [PATCH 01/17] fix: safely look up issue in the array Error: Cannot read properties of undefined (reading '0') Trigger: GET /organisations/local-authority:SAL/tree/invalid%20date/entry-date/5000 --- src/middleware/issueDetails.middleware.js | 31 +++++++++++++++-------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/middleware/issueDetails.middleware.js b/src/middleware/issueDetails.middleware.js index a537a3c9..cf5a65ef 100644 --- a/src/middleware/issueDetails.middleware.js +++ b/src/middleware/issueDetails.middleware.js @@ -48,7 +48,7 @@ async function fetchIssues (req, res, next) { /** * - * Middleware. Updates `req` with `issues`. + * Middleware. Updates `req` with `issuesByEntryNumber`. * * Requires `issues` in request. * @@ -69,11 +69,11 @@ async function reformatIssuesToBeByEntryNumber (req, res, next) { /** * - * Middleware. Updates `req` with `entryData` + * Middleware. Updates `req` with `entryData`, `entryNumber` and `pageNumber` * * Requires `pageNumber`, `dataset` and * - * @param {*} req + * @param {{ issuesByEntryNumber: Object, resource: { resource: string }, params: { dataset: string, pageNumber: string }}} req * @param {*} res * @param {*} next * @@ -85,15 +85,24 @@ async function fetchEntry (req, res, next) { req.pageNumber = pageNum // look at issue Entries and get the index of that entry - 1 + let entryData + let entryNum + const issuesByEntry = Object.values(issuesByEntryNumber) + if ((pageNum - 1) < issuesByEntry.length) { + const issues = issuesByEntry[pageNum - 1] + entryNum = issues.length > 0 ? issues[0].entry_number : undefined + entryData = await performanceDbApi.getEntry( + req.resource.resource, + entryNum, + datasetId + ) + } else { + entryNum = undefined + entryData = [] + } - const entityNum = Object.values(issuesByEntryNumber)[pageNum - 1][0].entry_number - - req.entryData = await performanceDbApi.getEntry( - req.resource.resource, - entityNum, - datasetId - ) - req.entryNumber = entityNum + req.entryData = entryData + req.entryNumber = entryNum next() } From c2d713ad67daa2055367e8e5dd0854e480c2eeca Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 25 Oct 2024 11:58:18 +0100 Subject: [PATCH 02/17] fix: default data to zero/[] when resource missing, limit max pages num. Error: Cannot read properties of undefined (reading '0') Trigger: GET /organisations/local-authority:SAL/tree/invalid%20date/entry-date/5000 This is further fix, for the above: Added safe default values, so a page can't be displayed (but with missing data, so it's obviously looks broken). To take care of the "brokn look" we disallow going to pages that are missing data (e.g. no data to display returned from datasette) --- src/middleware/common.middleware.js | 1 + src/middleware/issueDetails.middleware.js | 86 ++++++++++++++++------- 2 files changed, 60 insertions(+), 27 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index aeab97eb..a060057f 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -41,6 +41,7 @@ export const fetchDatasetInfo = fetchOne({ export const isResourceAccessible = (req) => req.resourceStatus.status === '200' export const isResourceNotAccessible = (req) => !isResourceAccessible(req) export const isResourceIdInParams = ({ params }) => !('resourceId' in params) +export const isResourceDataPresent = (req) => 'resource' in req /** * Middleware. Updates req with `resource`. diff --git a/src/middleware/issueDetails.middleware.js b/src/middleware/issueDetails.middleware.js index cf5a65ef..67856ca9 100644 --- a/src/middleware/issueDetails.middleware.js +++ b/src/middleware/issueDetails.middleware.js @@ -1,7 +1,14 @@ import performanceDbApi from '../services/performanceDbApi.js' -import logger from '../utils/logger.js' -import { types } from '../utils/logging.js' -import { fetchDatasetInfo, fetchEntityCount, fetchLatestResource, fetchOrgInfo, isResourceIdInParams, logPageError, takeResourceIdFromParams, validateQueryParams } from './common.middleware.js' +import { + fetchDatasetInfo, + fetchEntityCount, + fetchLatestResource, + fetchOrgInfo, + isResourceIdInParams, isResourceDataPresent, + logPageError, + takeResourceIdFromParams, + validateQueryParams +} from './common.middleware.js' import { fetchIf, renderTemplate } from './middleware.builders.js' import * as v from 'valibot' import { pagination } from '../utils/pagination.js' @@ -23,7 +30,7 @@ const validateIssueDetailsQueryParams = validateQueryParams({ * * Middleware. Updates `req` with `issues`. * - * Requires `resourceId` in request params or request (in that order). + * Requires resource id under `req.resource.resource`. * * @param {*} req * @param {*} res @@ -31,12 +38,7 @@ const validateIssueDetailsQueryParams = validateQueryParams({ */ async function fetchIssues (req, res, next) { const { dataset: datasetId, issue_type: issueType, issue_field: issueField } = req.params - const { resource: resourceId } = req.resource - if (!resourceId) { - logger.debug('fetchIssues(): missing resourceId', { type: types.App, params: req.params, resource: req.resource }) - throw Error('fetchIssues: missing resourceId') - } - + const resourceId = req.resource.resource try { const issues = await performanceDbApi.getIssues({ resource: resourceId, issueType, issueField }, datasetId) req.issues = issues @@ -48,7 +50,7 @@ async function fetchIssues (req, res, next) { /** * - * Middleware. Updates `req` with `issuesByEntryNumber`. + * Middleware. Updates `req` with `issuesByEntryNumber` and `entryNumberCount`. * * Requires `issues` in request. * @@ -58,33 +60,33 @@ async function fetchIssues (req, res, next) { */ async function reformatIssuesToBeByEntryNumber (req, res, next) { const { issues } = req + const count = new Set() const issuesByEntryNumber = issues.reduce((acc, current) => { acc[current.entry_number] = acc[current.entry_number] || [] acc[current.entry_number].push(current) + count.add(current.entry_number) return acc }, {}) req.issuesByEntryNumber = issuesByEntryNumber + req.entryNumberCount = count.size next() } /** * - * Middleware. Updates `req` with `entryData`, `entryNumber` and `pageNumber` + * Middleware. Updates `req` with `entryData`, `entryNumber` * - * Requires `pageNumber`, `dataset` and + * Requires `pageNumber`, `dataset`, `issuesByEntryNumber`, `resource` * - * @param {{ issuesByEntryNumber: Object, resource: { resource: string }, params: { dataset: string, pageNumber: string }}} req + * @param {{ issuesByEntryNumber: Object, resource: { resource: string }, params: { dataset: string }}} req * @param {*} res * @param {*} next * */ async function fetchEntry (req, res, next) { - const { dataset: datasetId, pageNumber } = req.params - const { issuesByEntryNumber } = req - const pageNum = pageNumber ? parseInt(pageNumber) : 1 - req.pageNumber = pageNum + const { dataset: datasetId } = req.params + const { issuesByEntryNumber, pageNumber: pageNum } = req - // look at issue Entries and get the index of that entry - 1 let entryData let entryNum const issuesByEntry = Object.values(issuesByEntryNumber) @@ -119,7 +121,6 @@ async function fetchEntry (req, res, next) { async function fetchIssueEntitiesCount (req, res, next) { const { dataset: datasetId, issue_type: issueType, issue_field: issueField } = req.params const { resource: resourceId } = req.resource - console.assert(resourceId, 'missng resource id') const issueEntitiesCount = await performanceDbApi.getEntitiesWithIssuesCount({ resource: resourceId, issueType, issueField }, datasetId) req.issueEntitiesCount = parseInt(issueEntitiesCount) next() @@ -191,7 +192,7 @@ const processEntryRow = (issueType, issuesByEntryNumber, row) => { * Middleware. Updates req with `templateParams` */ export function prepareIssueDetailsTemplateParams (req, res, next) { - const { entryData, pageNumber, issueEntitiesCount, issuesByEntryNumber, entryNumber, entityCount: entityCountRow } = req + const { entryData, pageNumber, issueEntitiesCount, issuesByEntryNumber, entryNumberCount, entryNumber, entityCount: entityCountRow } = req const { lpa, dataset: datasetId, issue_type: issueType, issue_field: issueField } = req.params const { entity_count: entityCount } = entityCountRow ?? { entity_count: 0 } @@ -200,7 +201,7 @@ export function prepareIssueDetailsTemplateParams (req, res, next) { const BaseSubpath = `/organisations/${lpa}/${datasetId}/${issueType}/${issueField}/` - if (Object.keys(issuesByEntryNumber).length < entityCount) { + if (entryNumberCount < entityCount) { errorHeading = performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issueEntitiesCount, entityCount, field: issueField }, true) issueItems = Object.entries(issuesByEntryNumber).map(([entryNumber, issues], i) => { const pageNum = i + 1 @@ -215,6 +216,10 @@ export function prepareIssueDetailsTemplateParams (req, res, next) { }] } + // for whatever reason `issuesByEntryNumber` is only 1k long max, so wa can't have more pages + // than that, even if there are more entities with issues + const maxPageNumber = Math.min(issueEntitiesCount, entryNumberCount) + const fields = entryData.map((row) => processEntryRow(issueType, issuesByEntryNumber, row)) const entityIssues = Object.values(issuesByEntryNumber)[pageNumber - 1] || [] for (const issue of entityIssues) { @@ -244,13 +249,13 @@ export function prepareIssueDetailsTemplateParams (req, res, next) { } } - if (pageNumber < issueEntitiesCount) { + if (pageNumber < maxPageNumber) { paginationObj.next = { href: `${BaseSubpath}${pageNumber + 1}` } } - paginationObj.items = pagination(issueEntitiesCount, pageNumber).map(item => { + paginationObj.items = pagination(maxPageNumber, pageNumber).map(item => { if (item === '...') { return { type: 'ellipsis', @@ -283,6 +288,27 @@ export function prepareIssueDetailsTemplateParams (req, res, next) { next() } +/** + * Middleware. Short-circuits with 404 error if pageNumber is not in range. + * Updates req with `pageNumber` + * + * @param req + * @param res + * @param next + */ +const isPageNumberInRange = (req, res, next) => { + const { pageNumber } = req.params + const { entryNumberCount } = req + const pageNum = pageNumber ? parseInt(pageNumber) : 1 + req.pageNumber = pageNum + + if (pageNumber < 0 || entryNumberCount < pageNumber) { + res.status(404).render('errorPages/404', {}) + return + } + next() +} + /** * Middleware. Renders the issue details page with the list of issues, entry data, * and organisation and dataset details. @@ -293,16 +319,22 @@ export const getIssueDetails = renderTemplate({ handlerName: 'getIssueDetails' }) +/* eslint-disable no-return-assign */ +const zeroEntityCount = (req) => req.entityCount = 0 +const zeroIssueEntitiesCount = (req) => req.issueEntitiesCount = 0 +const emptyIssuesCollection = (req) => req.issues = [] + export default [ validateIssueDetailsQueryParams, fetchOrgInfo, fetchDatasetInfo, fetchIf(isResourceIdInParams, fetchLatestResource, takeResourceIdFromParams), - fetchIssues, + fetchIf(isResourceDataPresent, fetchIssues, emptyIssuesCollection), reformatIssuesToBeByEntryNumber, + isPageNumberInRange, fetchEntry, - fetchEntityCount, - fetchIssueEntitiesCount, + fetchIf(isResourceDataPresent, fetchEntityCount, zeroEntityCount), + fetchIf(isResourceDataPresent, fetchIssueEntitiesCount, zeroIssueEntitiesCount), prepareIssueDetailsTemplateParams, getIssueDetails, logPageError From 147ffd138fe670c1e477b30ac3131ba0b9fd3a31 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 25 Oct 2024 11:59:07 +0100 Subject: [PATCH 03/17] specify default argument ...to help editor with hints --- src/middleware/middleware.builders.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index c5d648b8..1c0d2d58 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -150,7 +150,7 @@ async function fetchIfFn (req, res, next) { * @param {(req) => void} elseFn * @returns */ -export const fetchIf = (condition, fetchFn, elseFn) => { +export const fetchIf = (condition, fetchFn, elseFn = undefined) => { return fetchIfFn.bind({ condition, fetchFn, else: elseFn }) From faad40ef82ba0d1330eda1a15d3c604434d20a8d Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 25 Oct 2024 12:30:43 +0100 Subject: [PATCH 04/17] fix: safely handle missing resource id Error: Cannot read properties of undefined (reading 'resource') Trigger: GET /organisations/local-authority:MEN/brownfield-land We get an empty string id when fetching resource status and that causes things to fail further down. Upadted the code to skip attempted fetch of entity count and setting it to zero. --- src/middleware/common.middleware.js | 8 ++++++++ src/middleware/datasetTaskList.middleware.js | 19 ++++++++++++++++--- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index a060057f..26535368 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -39,10 +39,18 @@ export const fetchDatasetInfo = fetchOne({ * @returns {boolean} */ export const isResourceAccessible = (req) => req.resourceStatus.status === '200' +export const isResourceIdValid = (req) => req.resourceStatus.resource.trim() !== '' export const isResourceNotAccessible = (req) => !isResourceAccessible(req) export const isResourceIdInParams = ({ params }) => !('resourceId' in params) export const isResourceDataPresent = (req) => 'resource' in req +export const and = (...args) => { + return (req) => args.every(arg => arg(req)) +} +export const or = (...args) => { + return (req) => args.some(arg => arg(req)) +} + /** * Middleware. Updates req with `resource`. */ diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index f77db39e..1a1385ef 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -1,4 +1,15 @@ -import { fetchDatasetInfo, isResourceAccessible, isResourceNotAccessible, fetchLatestResource, fetchEntityCount, logPageError, fetchLpaDatasetIssues, validateQueryParams, getDatasetTaskListError } from './common.middleware.js' +import { + fetchDatasetInfo, + isResourceAccessible, + isResourceNotAccessible, + fetchLatestResource, + fetchEntityCount, + logPageError, + fetchLpaDatasetIssues, + validateQueryParams, + getDatasetTaskListError, + isResourceIdValid, and +} from './common.middleware.js' import { fetchOne, fetchIf, onlyIf, renderTemplate } from './middleware.builders.js' import performanceDbApi from '../services/performanceDbApi.js' import { statusToTagClass } from '../filters/filters.js' @@ -46,7 +57,6 @@ export const prepareDatasetTaskListTemplateParams = (req, res, next) => { const { issues, entityCount: entityCountRow, params, dataset, orgInfo: organisation } = req const { entity_count: entityCount } = entityCountRow ?? { entity_count: 0 } const { lpa, dataset: datasetId } = params - console.assert(req.resourceStatus.resource === req.resource.resource, 'mismatch between resourceStatus and resource data') console.assert(typeof entityCount === 'number', 'entityCount should be a number') const taskList = issues.map((issue) => { @@ -113,6 +123,9 @@ const validateParams = validateQueryParams({ }) }) +/* eslint-disable-next-line no-return-assign */ +const zeroEntityCount = (req) => req.entityCount = 0 + export default [ validateParams, fetchResourceStatus, @@ -120,7 +133,7 @@ export default [ fetchDatasetInfo, fetchIf(isResourceAccessible, fetchLatestResource), fetchIf(isResourceAccessible, fetchLpaDatasetIssues), - fetchIf(isResourceAccessible, fetchEntityCount), + fetchIf(and(isResourceAccessible, isResourceIdValid), fetchEntityCount, zeroEntityCount), onlyIf(isResourceAccessible, prepareDatasetTaskListTemplateParams), onlyIf(isResourceAccessible, getDatasetTaskList), onlyIf(isResourceNotAccessible, prepareDatasetTaskListErrorTemplateParams), From 9363343c51bd8bf1fd8b7388926d750fe0e17dc1 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 25 Oct 2024 13:21:47 +0100 Subject: [PATCH 05/17] fix: entityCount is an object --- src/middleware/datasetTaskList.middleware.js | 2 +- src/middleware/issueDetails.middleware.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index 1a1385ef..284dd649 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -124,7 +124,7 @@ const validateParams = validateQueryParams({ }) /* eslint-disable-next-line no-return-assign */ -const zeroEntityCount = (req) => req.entityCount = 0 +const zeroEntityCount = (req) => req.entityCount = { entity_count: 0 } export default [ validateParams, diff --git a/src/middleware/issueDetails.middleware.js b/src/middleware/issueDetails.middleware.js index 67856ca9..064bdd01 100644 --- a/src/middleware/issueDetails.middleware.js +++ b/src/middleware/issueDetails.middleware.js @@ -320,7 +320,7 @@ export const getIssueDetails = renderTemplate({ }) /* eslint-disable no-return-assign */ -const zeroEntityCount = (req) => req.entityCount = 0 +const zeroEntityCount = (req) => req.entityCount = { entity_count: 0 } const zeroIssueEntitiesCount = (req) => req.issueEntitiesCount = 0 const emptyIssuesCollection = (req) => req.issues = [] From 243fd0c7547f923c0996a708f8719f9de041fe53 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 25 Oct 2024 13:55:45 +0100 Subject: [PATCH 06/17] fix: handle case when dataset specification is missing Error: Cannot read properties of undefined (reading 'fields') Trigger: GET /organisations/local-authority:BUC/listed-building/overview The fetched specification collecton doesn't containa a spec for the dataset ('listed-building' in our example) --- src/middleware/datasetOverview.middleware.js | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/middleware/datasetOverview.middleware.js b/src/middleware/datasetOverview.middleware.js index cbcf67d9..84bfe81f 100644 --- a/src/middleware/datasetOverview.middleware.js +++ b/src/middleware/datasetOverview.middleware.js @@ -36,11 +36,18 @@ const fetchSpecification = fetchOne({ result: 'specification' }) +/** + * Middleware. Updates req with `datasetSpecification` + * + * @param req + * @param res + * @param next + */ export const pullOutDatasetSpecification = (req, res, next) => { const { specification } = req const collectionSpecifications = JSON.parse(specification.json) const datasetSpecification = collectionSpecifications.find((spec) => spec.dataset === req.dataset.dataset) - req.specification = datasetSpecification + req.datasetSpecification = datasetSpecification next() } @@ -105,21 +112,21 @@ const fetchEntityCount = fetchOne({ }) export const prepareDatasetOverviewTemplateParams = (req, res, next) => { - const { orgInfo, specification, columnSummary, entityCount, sources, dataset, issues } = req + const { orgInfo, datasetSpecification, columnSummary, entityCount, sources, dataset, issues } = req const mappingFields = columnSummary[0]?.mapping_field?.split(';') ?? [] const nonMappingFields = columnSummary[0]?.non_mapping_field?.split(';') ?? [] const allFields = [...mappingFields, ...nonMappingFields] - const numberOfFieldsSupplied = specification.fields.map(field => field.field).reduce((acc, current) => { + const specFields = datasetSpecification ? datasetSpecification.fields : [] + const numberOfFieldsSupplied = specFields.map(field => field.field).reduce((acc, current) => { return allFields.includes(current) ? acc + 1 : acc }, 0) - - const numberOfFieldsMatched = specification.fields.map(field => field.field).reduce((acc, current) => { + const numberOfFieldsMatched = specFields.map(field => field.field).reduce((acc, current) => { return mappingFields.includes(current) ? acc + 1 : acc }, 0) - const numberOfExpectedFields = specification.fields.length + const numberOfExpectedFields = specFields.length // I'm pretty sure every endpoint has a separate documentation-url, but this isn't currently represented in the performance db. need to double check this and update if so const endpoints = sources.sort((a, b) => { From 7c215178556206aeef788d906b099545a886322e Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 25 Oct 2024 15:24:35 +0100 Subject: [PATCH 07/17] fix tests --- test/unit/middleware/issueDetails.middleware.test.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/unit/middleware/issueDetails.middleware.test.js b/test/unit/middleware/issueDetails.middleware.test.js index 143d049c..dd09e2f9 100644 --- a/test/unit/middleware/issueDetails.middleware.test.js +++ b/test/unit/middleware/issueDetails.middleware.test.js @@ -38,7 +38,7 @@ describe('issueDetails.middleware.js', () => { params: requestParams, // middleware supplies the below entryNumber: 1, - entityCount: { entity_count: 3 }, + entityCount: { entity_count: 1 }, issueEntitiesCount: 1, pageNumber: 1, orgInfo, @@ -46,6 +46,7 @@ describe('issueDetails.middleware.js', () => { entryData, issues, resource: { resource: requestParams.resourceId }, + entryNumberCount: 1, issuesByEntryNumber: { 1: [ { @@ -65,7 +66,6 @@ describe('issueDetails.middleware.js', () => { issues.forEach(issue => { vi.mocked(performanceDbApi.getTaskMessage).mockReturnValueOnce(`mockMessageFor: ${issue.entry_number}`) }) - vi.mocked(performanceDbApi.getTaskMessage).mockReturnValueOnce('mock task message 1') prepareIssueDetailsTemplateParams(req, {}, () => {}) @@ -79,11 +79,10 @@ describe('issueDetails.middleware.js', () => { dataset: 'mock-dataset', collection: 'mock-collection' }, - errorHeading: 'mockMessageFor: 0', + errorHeading: undefined, issueItems: [ { - html: 'mock task message 1 in record 1', - href: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/1' + html: 'mockMessageFor: 0' } ], entry: { @@ -146,6 +145,7 @@ describe('issueDetails.middleware.js', () => { entryData, issues, resource: { resource: requestParams.resourceId }, + entryNumberCount: 1, issuesByEntryNumber: { 1: [ { From 827c9062b01b1cb824086c1fc1a9cdb2f9c5fb0b Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 25 Oct 2024 15:44:01 +0100 Subject: [PATCH 08/17] fix datasetOverview.middleware tests --- test/unit/middleware/datasetOverview.middleware.test.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/unit/middleware/datasetOverview.middleware.test.js b/test/unit/middleware/datasetOverview.middleware.test.js index 3060d6df..b0762a02 100644 --- a/test/unit/middleware/datasetOverview.middleware.test.js +++ b/test/unit/middleware/datasetOverview.middleware.test.js @@ -16,7 +16,7 @@ describe('Dataset Overview Middleware', () => { const res = {} describe('pullOutDatasetSpecification', () => { - it('', () => { + it('leaves specification unchanged, and extracts the dataset specification', () => { const reqWithSpecification = { ...req, specification: { @@ -26,7 +26,8 @@ describe('Dataset Overview Middleware', () => { } } pullOutDatasetSpecification(reqWithSpecification, res, () => {}) - expect(reqWithSpecification.specification).toEqual({ dataset: 'mock-dataset', foo: 'bar' }) + expect(reqWithSpecification.specification).toEqual(reqWithSpecification.specification) + expect(reqWithSpecification.datasetSpecification).toEqual({ dataset: 'mock-dataset', foo: 'bar' }) }) }) @@ -35,7 +36,7 @@ describe('Dataset Overview Middleware', () => { const reqWithResults = { ...req, orgInfo: { name: 'mock-org' }, - specification: { fields: [{ field: 'field1' }, { field: 'field2' }] }, + datasetSpecification: { fields: [{ field: 'field1' }, { field: 'field2' }] }, columnSummary: [{ mapping_field: 'field1', non_mapping_field: 'field3' }], entityCount: { entity_count: 10 }, sources: [ From 47963d48ad1c53399cbd0cd667be6fde9a3075fc Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Mon, 28 Oct 2024 15:53:42 +0000 Subject: [PATCH 09/17] store validated params in req.parsedParams --- src/middleware/common.middleware.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 26535368..7f6b7b4e 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -81,7 +81,7 @@ export const fetchOrgInfo = fetchOne({ /** * Middleware. Validates query params according to schema. - * Short circuits with 400 error if validation fails + * Short circuits with 400 error if validation fails. Potentially updates req with `parsedParams` * * `this` needs: `{ schema }` * @@ -91,7 +91,7 @@ export const fetchOrgInfo = fetchOne({ */ export function validateQueryParamsFn (req, res, next) { try { - v.parse(this.schema || v.any(), req.params) + req.parsedParams = v.parse(this.schema || v.any(), req.params) next() } catch (error) { res.status(400).render('errorPages/400', {}) From 5ba6808d2effdc89d9b4f0f7b5d0ea38297ec940 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Mon, 28 Oct 2024 16:02:55 +0000 Subject: [PATCH 10/17] pagination on issue details pages issues query needs a limit & offset (returns 1000 records max when not specified). --- src/middleware/issueDetails.middleware.js | 65 ++++++++++--------- src/services/performanceDbApi.js | 16 +++-- .../issueDetails.middleware.test.js | 2 + 3 files changed, 44 insertions(+), 39 deletions(-) diff --git a/src/middleware/issueDetails.middleware.js b/src/middleware/issueDetails.middleware.js index 064bdd01..f6501c93 100644 --- a/src/middleware/issueDetails.middleware.js +++ b/src/middleware/issueDetails.middleware.js @@ -12,13 +12,15 @@ import { import { fetchIf, renderTemplate } from './middleware.builders.js' import * as v from 'valibot' import { pagination } from '../utils/pagination.js' +import logger from '../utils/logger.js' +import { types } from '../utils/logging.js' export const IssueDetailsQueryParams = v.object({ lpa: v.string(), dataset: v.string(), issue_type: v.string(), issue_field: v.string(), - pageNumber: v.optional(v.string()), + pageNumber: v.optional(v.pipe(v.string(), v.transform(parseInt), v.minValue(1)), '1'), resourceId: v.optional(v.string()) }) @@ -26,6 +28,8 @@ const validateIssueDetailsQueryParams = validateQueryParams({ schema: IssueDetailsQueryParams }) +const issuesQueryLimit = 1000 + /** * * Middleware. Updates `req` with `issues`. @@ -38,9 +42,12 @@ const validateIssueDetailsQueryParams = validateQueryParams({ */ async function fetchIssues (req, res, next) { const { dataset: datasetId, issue_type: issueType, issue_field: issueField } = req.params + const { pageNumber } = req.parsedParams const resourceId = req.resource.resource + const offset = Math.floor((pageNumber - 1) / issuesQueryLimit) * issuesQueryLimit + logger.debug('fetchIssues()', { type: types.App, resourceId, issueType, issueField, offset }) try { - const issues = await performanceDbApi.getIssues({ resource: resourceId, issueType, issueField }, datasetId) + const issues = await performanceDbApi.getIssues({ resource: resourceId, issueType, issueField, offset }, datasetId) req.issues = issues next() } catch (error) { @@ -74,37 +81,35 @@ async function reformatIssuesToBeByEntryNumber (req, res, next) { /** * - * Middleware. Updates `req` with `entryData`, `entryNumber` + * Middleware. Updates `req` with `entryData` * * Requires `pageNumber`, `dataset`, `issuesByEntryNumber`, `resource` * - * @param {{ issuesByEntryNumber: Object, resource: { resource: string }, params: { dataset: string }}} req + * @param {{ issuesByEntryNumber: Object, resource: { resource: string }, params: { dataset: string }, parsedParams: { pageNumber: number }}} req * @param {*} res * @param {*} next * */ async function fetchEntry (req, res, next) { const { dataset: datasetId } = req.params - const { issuesByEntryNumber, pageNumber: pageNum } = req + const { issues, issueEntitiesCount, issuesByEntryNumber } = req + const { pageNumber } = req.parsedParams let entryData - let entryNum const issuesByEntry = Object.values(issuesByEntryNumber) - if ((pageNum - 1) < issuesByEntry.length) { - const issues = issuesByEntry[pageNum - 1] - entryNum = issues.length > 0 ? issues[0].entry_number : undefined - entryData = await performanceDbApi.getEntry( - req.resource.resource, - entryNum, - datasetId - ) - } else { - entryNum = undefined - entryData = [] + if (issues.length > 0) { + if (pageNumber <= issueEntitiesCount) { + const entryIssues = issuesByEntry[(pageNumber - 1) % issuesQueryLimit] + const entryNum = entryIssues.length > 0 ? entryIssues[0].entry_number : undefined + entryData = entryNum ? await performanceDbApi.getEntry( + req.resource.resource, + entryNum, + datasetId + ): [] + } } - req.entryData = entryData - req.entryNumber = entryNum + req.entryData = entryData ?? [] next() } @@ -122,7 +127,7 @@ async function fetchIssueEntitiesCount (req, res, next) { const { dataset: datasetId, issue_type: issueType, issue_field: issueField } = req.params const { resource: resourceId } = req.resource const issueEntitiesCount = await performanceDbApi.getEntitiesWithIssuesCount({ resource: resourceId, issueType, issueField }, datasetId) - req.issueEntitiesCount = parseInt(issueEntitiesCount) + req.issueEntitiesCount = issueEntitiesCount next() } @@ -192,8 +197,9 @@ const processEntryRow = (issueType, issuesByEntryNumber, row) => { * Middleware. Updates req with `templateParams` */ export function prepareIssueDetailsTemplateParams (req, res, next) { - const { entryData, pageNumber, issueEntitiesCount, issuesByEntryNumber, entryNumberCount, entryNumber, entityCount: entityCountRow } = req + const { entryData, issueEntitiesCount, issuesByEntryNumber, entryNumberCount, entityCount: entityCountRow } = req const { lpa, dataset: datasetId, issue_type: issueType, issue_field: issueField } = req.params + const { pageNumber } = req.parsedParams const { entity_count: entityCount } = entityCountRow ?? { entity_count: 0 } let errorHeading @@ -216,10 +222,7 @@ export function prepareIssueDetailsTemplateParams (req, res, next) { }] } - // for whatever reason `issuesByEntryNumber` is only 1k long max, so wa can't have more pages - // than that, even if there are more entities with issues - const maxPageNumber = Math.min(issueEntitiesCount, entryNumberCount) - + const maxPageNumber = issueEntitiesCount const fields = entryData.map((row) => processEntryRow(issueType, issuesByEntryNumber, row)) const entityIssues = Object.values(issuesByEntryNumber)[pageNumber - 1] || [] for (const issue of entityIssues) { @@ -237,7 +240,7 @@ export function prepareIssueDetailsTemplateParams (req, res, next) { .filter((row) => row.field === 'geometry') .map((row) => row.value) const entry = { - title: `entry: ${entryNumber}`, + title: `entry: ${entryData.length > 0 ? entryData[0].entry_number : ''}`, fields, geometries } @@ -297,12 +300,10 @@ export function prepareIssueDetailsTemplateParams (req, res, next) { * @param next */ const isPageNumberInRange = (req, res, next) => { - const { pageNumber } = req.params - const { entryNumberCount } = req - const pageNum = pageNumber ? parseInt(pageNumber) : 1 - req.pageNumber = pageNum + const { issueEntitiesCount } = req + const { pageNumber } = req.parsedParams - if (pageNumber < 0 || entryNumberCount < pageNumber) { + if (pageNumber < 0 || issueEntitiesCount < pageNumber) { res.status(404).render('errorPages/404', {}) return } @@ -331,10 +332,10 @@ export default [ fetchIf(isResourceIdInParams, fetchLatestResource, takeResourceIdFromParams), fetchIf(isResourceDataPresent, fetchIssues, emptyIssuesCollection), reformatIssuesToBeByEntryNumber, + fetchIf(isResourceDataPresent, fetchIssueEntitiesCount, zeroIssueEntitiesCount), isPageNumberInRange, fetchEntry, fetchIf(isResourceDataPresent, fetchEntityCount, zeroEntityCount), - fetchIf(isResourceDataPresent, fetchIssueEntitiesCount, zeroIssueEntitiesCount), prepareIssueDetailsTemplateParams, getIssueDetails, logPageError diff --git a/src/services/performanceDbApi.js b/src/services/performanceDbApi.js index 10d29d19..1952eb16 100644 --- a/src/services/performanceDbApi.js +++ b/src/services/performanceDbApi.js @@ -208,11 +208,9 @@ export default { /** * Returns a task message based on the provided issue type, issue count, and entity count. * - * @param {Object} options - Options object - * @param {string} options.issueType - The type of issue - * @param {number} options.num_issues - The number of issues - * @param {number} options.entityCount - The number of entities - * @param {boolean} [entityLevel=false] - Whether to use entity-level or dataset level messaging + * - entityLevel - Whether to use entity-level or dataset level messaging + * + * @param {{issue_type: string, num_issues: number, entityCount: number, field: string, entityLevel: boolean = false }} options * * @returns {string} The task message with the issue count inserted * @@ -367,7 +365,8 @@ export default { async getIssues ({ resource, issueType, - issueField + issueField, + offset = 0 }, database = 'digital-land') { const sql = ` SELECT i.field, i.line_number, entry_number, message, issue_type, value @@ -375,6 +374,8 @@ export default { WHERE resource = '${resource}' AND issue_type = '${issueType}' AND field = '${issueField}' + ORDER BY entry_number ASC + LIMIT 1000 ${offset ? `OFFSET ${offset}` : ''} ` const result = await datasette.runQuery(sql, database) @@ -390,8 +391,9 @@ export default { * @returns {Promise<{field: string, value: string, entry_number: number}[]>} */ async getEntry (resourceId, entryNumber, dataset) { + logger.debug({ message: 'getEntry()', resourceId, entryNumber, dataset, type: types.App }) // TODO: why do we order by rowid? - const sql = ` + const sql = /* sql */ ` select fr.rowid, fr.end_date, diff --git a/test/unit/middleware/issueDetails.middleware.test.js b/test/unit/middleware/issueDetails.middleware.test.js index dd09e2f9..8fe0ea58 100644 --- a/test/unit/middleware/issueDetails.middleware.test.js +++ b/test/unit/middleware/issueDetails.middleware.test.js @@ -36,6 +36,7 @@ describe('issueDetails.middleware.js', () => { } const req = { params: requestParams, + parsedParams: { pageNumber: 1 }, // middleware supplies the below entryNumber: 1, entityCount: { entity_count: 1 }, @@ -135,6 +136,7 @@ describe('issueDetails.middleware.js', () => { } const req = { params: requestParams, + parsedParams: { pageNumber: 1 }, // middleware supplies the below entryNumber: 1, entityCount: { entity_count: 3 }, From 587c4740b0eb5d590a593329120943b70f9ab7c5 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Mon, 28 Oct 2024 16:13:47 +0000 Subject: [PATCH 11/17] linting --- src/middleware/issueDetails.middleware.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/middleware/issueDetails.middleware.js b/src/middleware/issueDetails.middleware.js index f6501c93..d1e2fad8 100644 --- a/src/middleware/issueDetails.middleware.js +++ b/src/middleware/issueDetails.middleware.js @@ -101,11 +101,13 @@ async function fetchEntry (req, res, next) { if (pageNumber <= issueEntitiesCount) { const entryIssues = issuesByEntry[(pageNumber - 1) % issuesQueryLimit] const entryNum = entryIssues.length > 0 ? entryIssues[0].entry_number : undefined - entryData = entryNum ? await performanceDbApi.getEntry( - req.resource.resource, - entryNum, - datasetId - ): [] + entryData = entryNum + ? await performanceDbApi.getEntry( + req.resource.resource, + entryNum, + datasetId + ) + : [] } } From cbd8020298893bb093cc45783ab857c2b4701ae4 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Tue, 29 Oct 2024 09:11:15 +0000 Subject: [PATCH 12/17] minor refactoring --- src/middleware/datasetOverview.middleware.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/middleware/datasetOverview.middleware.js b/src/middleware/datasetOverview.middleware.js index 84bfe81f..b0ca3f78 100644 --- a/src/middleware/datasetOverview.middleware.js +++ b/src/middleware/datasetOverview.middleware.js @@ -119,11 +119,11 @@ export const prepareDatasetOverviewTemplateParams = (req, res, next) => { const allFields = [...mappingFields, ...nonMappingFields] const specFields = datasetSpecification ? datasetSpecification.fields : [] - const numberOfFieldsSupplied = specFields.map(field => field.field).reduce((acc, current) => { - return allFields.includes(current) ? acc + 1 : acc + const numberOfFieldsSupplied = specFields.reduce((acc, field) => { + return allFields.includes(field.field) ? acc + 1 : acc }, 0) - const numberOfFieldsMatched = specFields.map(field => field.field).reduce((acc, current) => { - return mappingFields.includes(current) ? acc + 1 : acc + const numberOfFieldsMatched = specFields.reduce((acc, field) => { + return mappingFields.includes(field.field) ? acc + 1 : acc }, 0) const numberOfExpectedFields = specFields.length From 219ff9cb49ae0cc42078f79a3de3742430207bca Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Tue, 29 Oct 2024 09:23:16 +0000 Subject: [PATCH 13/17] safely parse specification JSON --- src/middleware/datasetOverview.middleware.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/middleware/datasetOverview.middleware.js b/src/middleware/datasetOverview.middleware.js index b0ca3f78..fc70c4fe 100644 --- a/src/middleware/datasetOverview.middleware.js +++ b/src/middleware/datasetOverview.middleware.js @@ -2,6 +2,8 @@ import { fetchDatasetInfo, fetchLatestResource, fetchLpaDatasetIssues, fetchOrgI import { fetchOne, fetchIf, fetchMany, renderTemplate, FetchOptions, onlyIf } from './middleware.builders.js' import { fetchResourceStatus, prepareDatasetTaskListErrorTemplateParams } from './datasetTaskList.middleware.js' import performanceDbApi from '../services/performanceDbApi.js' +import logger from '../utils/logger.js' +import { types } from '../utils/logging.js' const fetchColumnSummary = fetchMany({ query: ({ params }) => ` @@ -45,9 +47,15 @@ const fetchSpecification = fetchOne({ */ export const pullOutDatasetSpecification = (req, res, next) => { const { specification } = req - const collectionSpecifications = JSON.parse(specification.json) - const datasetSpecification = collectionSpecifications.find((spec) => spec.dataset === req.dataset.dataset) - req.datasetSpecification = datasetSpecification + let collectionSpecifications + try { + collectionSpecifications = JSON.parse(specification.json) + } catch (e) { + // we can proceed but we probably should notify the user the displayed data may not be complete + logger.info('failed to parse specification JSON', { type: types.DataValidation, collection: req.dataset.collection }) + collectionSpecifications = [] + } + req.datasetSpecification = collectionSpecifications.find((spec) => spec.dataset === req.dataset.dataset) next() } From 5517dba6ff067d8253a2f1dae0d51e78badcf4a7 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Tue, 29 Oct 2024 09:26:02 +0000 Subject: [PATCH 14/17] exclude 0, pages start with 1 --- src/middleware/issueDetails.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/issueDetails.middleware.js b/src/middleware/issueDetails.middleware.js index d1e2fad8..fd13a096 100644 --- a/src/middleware/issueDetails.middleware.js +++ b/src/middleware/issueDetails.middleware.js @@ -305,7 +305,7 @@ const isPageNumberInRange = (req, res, next) => { const { issueEntitiesCount } = req const { pageNumber } = req.parsedParams - if (pageNumber < 0 || issueEntitiesCount < pageNumber) { + if (pageNumber < 1 || issueEntitiesCount < pageNumber) { res.status(404).render('errorPages/404', {}) return } From e52f6c8b7b08ea35eb60d7e45b2913de63ec833d Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Tue, 29 Oct 2024 09:29:21 +0000 Subject: [PATCH 15/17] specify radix --- src/middleware/issueDetails.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/issueDetails.middleware.js b/src/middleware/issueDetails.middleware.js index fd13a096..a6be1500 100644 --- a/src/middleware/issueDetails.middleware.js +++ b/src/middleware/issueDetails.middleware.js @@ -20,7 +20,7 @@ export const IssueDetailsQueryParams = v.object({ dataset: v.string(), issue_type: v.string(), issue_field: v.string(), - pageNumber: v.optional(v.pipe(v.string(), v.transform(parseInt), v.minValue(1)), '1'), + pageNumber: v.optional(v.pipe(v.string(), v.transform(s => parseInt(s, 10)), v.minValue(1)), '1'), resourceId: v.optional(v.string()) }) From 90aa4505cc56a09765584ab7a4552105c596246e Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Tue, 29 Oct 2024 09:47:43 +0000 Subject: [PATCH 16/17] safely defaults for entry issues --- src/middleware/issueDetails.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/issueDetails.middleware.js b/src/middleware/issueDetails.middleware.js index a6be1500..31e75df9 100644 --- a/src/middleware/issueDetails.middleware.js +++ b/src/middleware/issueDetails.middleware.js @@ -99,7 +99,7 @@ async function fetchEntry (req, res, next) { const issuesByEntry = Object.values(issuesByEntryNumber) if (issues.length > 0) { if (pageNumber <= issueEntitiesCount) { - const entryIssues = issuesByEntry[(pageNumber - 1) % issuesQueryLimit] + const entryIssues = issuesByEntry[(pageNumber - 1) % issuesQueryLimit] ?? [] const entryNum = entryIssues.length > 0 ? entryIssues[0].entry_number : undefined entryData = entryNum ? await performanceDbApi.getEntry( From e5fe630ce1637326021a59ce60890e7e33084dce Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Tue, 29 Oct 2024 09:50:56 +0000 Subject: [PATCH 17/17] fix JSDdoc for getTaskMessage() --- src/services/performanceDbApi.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/services/performanceDbApi.js b/src/services/performanceDbApi.js index 1952eb16..79a2862c 100644 --- a/src/services/performanceDbApi.js +++ b/src/services/performanceDbApi.js @@ -208,9 +208,8 @@ export default { /** * Returns a task message based on the provided issue type, issue count, and entity count. * - * - entityLevel - Whether to use entity-level or dataset level messaging - * - * @param {{issue_type: string, num_issues: number, entityCount: number, field: string, entityLevel: boolean = false }} options + * @param {{issue_type: string, num_issues: number, entityCount: number, field: string }} options + * @param {boolean?} entityLevel Whether to use entity-level or dataset level messaging * * @returns {string} The task message with the issue count inserted *