From d134d384374957b24ac7065eb7040f4e6c138756 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Mon, 8 Jun 2026 15:58:31 +0100 Subject: [PATCH 1/3] Enhance response detail fetching to include geometries and improve caching mechanism --- src/controllers/resultsController.js | 5 +- src/models/requestData.js | 136 ++++++--------------------- src/models/responseDetails.js | 48 +++++++++- test/unit/requestData.test.js | 91 +++++++++++++----- test/unit/responseDetails.test.js | 14 +++ 5 files changed, 159 insertions(+), 135 deletions(-) diff --git a/src/controllers/resultsController.js b/src/controllers/resultsController.js index dc7115de5..8c32992f0 100644 --- a/src/controllers/resultsController.js +++ b/src/controllers/resultsController.js @@ -147,7 +147,10 @@ export async function fetchResponseDetails (req, res, next) { if (req.locals.template !== failedFileRequestTemplate && req.locals.template !== failedUrlRequestTemplate) { const detailsOpts = req.locals.detailsOptions ?? {} // Original code used a if statement to check template and filter by error severity accordingly, but move to always showing all details in results/results template - const responseDetails = await req.locals.requestData.fetchResponseDetails(pageNumber - 1, 50, { ...detailsOpts }) + const responseDetails = await req.locals.requestData.fetchResponseDetails(pageNumber - 1, 50, { + ...detailsOpts, + includeGeometries: req.locals.datasetTypology === 'geography' + }) req.locals.responseDetails = responseDetails } } catch (e) { diff --git a/src/models/requestData.js b/src/models/requestData.js index ee9afed82..beb9088ed 100644 --- a/src/models/requestData.js +++ b/src/models/requestData.js @@ -1,12 +1,12 @@ import * as v from 'valibot' import logger from '../utils/logger.js' -import { types } from '../utils/logging.js' import axios from 'axios' import config from '../../config/index.js' import ResponseDetails from './responseDetails.js' const ResponseDetailsOptions = v.optional(v.object({ severity: v.optional(v.pipe(v.string(), v.minLength(2))), + includeGeometries: v.optional(v.boolean()), issue: v.optional(v.object({ issueType: v.pipe(v.string(), v.minLength(1)), field: v.pipe(v.string(), v.minLength(1)) @@ -37,7 +37,6 @@ export default class ResultData { */ async fetchResponseDetails (pageOffset = 0, limit = 50, opts = { severity: undefined }) { v.parse(ResponseDetailsOptions, opts) - const url = new URL(`${config.asyncRequestApi.url}/${config.asyncRequestApi.requestsEndpoint}/${this.id}/response-details`) url.searchParams.append('offset', pageOffset * limit) url.searchParams.append('limit', limit) @@ -51,30 +50,20 @@ export default class ResultData { url.searchParams.append('jsonpath', `$.issue_logs[*].severity=="${opts.severity}"`) } - // we do initial request, check how many records there are via 'x-pagination-total-results' header - // and if fetch the rest if needed - const response = await axios.get(url, { timeout: 30000 }) + const geometryUrl = new URL(`${config.asyncRequestApi.url}/${config.asyncRequestApi.requestsEndpoint}/${this.id}/geometries`) + const [response, geometriesResponse] = await Promise.all([ + axios.get(url, { timeout: 30000 }), + opts.includeGeometries ? fetchGeometries(geometryUrl) : Promise.resolve(undefined) + ]) const totalResults = Number.parseInt(response.headers['x-pagination-total-results']) - const responses = [...response.data] - if (Number.isInteger(totalResults) && totalResults > response.data.length) { - const urlTemplate = new URL(url) - urlTemplate.searchParams.delete('offset') - urlTemplate.searchParams.delete('limit') - - const paginationOpts = { limit, offset: response.data.length, maxOffset: Number.isInteger(totalResults) ? totalResults : 100 } - const restResponses = await fetchPaginated(url, paginationOpts) - responses.push(...restResponses.flatMap(resp => resp.data)) - } - // we're not using x-pagination-offset and x-pagination-limit headers, because we fetched - // all the records already, so there's no need for pagination controls on the table const pagination = { totalResults: `${totalResults}`, - offset: '0', - limit: `${totalResults}` + offset: `${pageOffset * limit}`, + limit: `${limit}` } - return new ResponseDetails(this.id, responses, pagination, this.getColumnFieldLog()) + return new ResponseDetails(this.id, response.data, pagination, this.getColumnFieldLog(), geometriesResponse) } isFailed () { @@ -197,99 +186,30 @@ export default class ResultData { } } -/** - * Returns a generator of offset values. - * - * @param {number} limit - * @param {number} offset - * @param {number} maxOffset - */ -function * offsets (limit, offset, maxOffset) { - let currentOffset = offset - while (currentOffset < maxOffset) { - yield currentOffset - currentOffset += limit - } -} +async function fetchGeometries (url) { + const response = await axios.get(url, { timeout: 30000 }) + const totalResults = Number.parseInt(response.headers?.['x-pagination-total-results']) + const limit = Number.parseInt(response.headers?.['x-pagination-limit']) || getGeometryItems(response.data).length || 500 + const geometries = getGeometryItems(response.data) -/** - * - * @param {number} numTasks max number of tasks to run - * @param {Object} gen offset generator - * @param {Function} taskFactory (taskIndex, offset) => Promise<> - * @returns {Promise[]} - */ -function startRequests (numTasks, gen, taskFactory) { - const tasks = [] - for (let i = 0; i < numTasks; ++i) { - const offsetItem = gen.next() - if (!offsetItem.done) { - const p = taskFactory(i, offsetItem.value) - tasks.push(p) - } else { - break - } + if (!Number.isInteger(totalResults) || geometries.length >= totalResults) { + return geometries.length > 0 ? geometries : response.data } - return tasks -} -/** - * Given a task factor function, executes a number of async tasks in parallel, - * but only at most `options.concurrency` tasks are in flight. - * - * Note: the tasks should be IO bound. - * - * If any of the tasks fail, the whole operation fails (in other words: - * no partial results). - * - * @param {Object} options - * @returns {Promise} - */ -async function fetchBatched (options) { - // Note: trying more involved strategy of launching requests by using Promise.any() - // and trying to immedieately replace that one completed promise with a new one - // didn't really behave as expected - work was happening mostly in a single promise. - // This one's simpler and seems to actually do what expected. - const { concurrency, taskFn, offsetInfo } = options - const results = [] - const gen = offsets(offsetInfo.limit, offsetInfo.offset, offsetInfo.maxOffset) - const newTask = async (index, offset) => { - logger.debug('fetchBatched(): starting task', { task: index, offset, type: types.DataFetch }) - const p = taskFn(offset).then((val) => { - logger.debug('fetchBatched(): finishing task', { task: index, offset, type: types.DataFetch }) - return { val, index, offset } - }) - return p + for (let offset = geometries.length; offset < totalResults; offset += limit) { + const pageUrl = new URL(url) + pageUrl.searchParams.set('offset', offset) + pageUrl.searchParams.set('limit', limit) + const page = await axios.get(pageUrl, { timeout: 30000 }) + geometries.push(...getGeometryItems(page.data)) } - let promises = startRequests(concurrency, gen, newTask) - - do { - const completed = await Promise.all(promises) - results.push(...completed) - promises = startRequests(concurrency, gen, newTask) - logger.debug(`fetchBatched(): completed ${completed.length} tasks`, { type: types.DataFetch }) - } while (promises.length > 0) - - logger.info(`fetchBatched(): completed ${results.length} requests`, { type: types.DataFetch }) - results.sort((r1, r2) => r1.offset - r2.offset) - return results.map(r => r.val) + return geometries } -/** - * - * @param {URL} url url - * @param {Object} options - * @returns {Promise} - */ -export const fetchPaginated = async (url, { limit, offset, maxOffset }) => { - const taskFn = async (offset) => { - const thisUrl = new URL(url) - thisUrl.searchParams.set('offset', offset) - thisUrl.searchParams.set('limit', limit) - const result = await axios.get(thisUrl, { timeout: 10000 }) - return result - } - - return await fetchBatched({ concurrency: 4, taskFn, offsetInfo: { limit, offset, maxOffset } }) +function getGeometryItems (data) { + if (Array.isArray(data)) return data + if (Array.isArray(data?.geometries)) return data.geometries + if (Array.isArray(data?.features)) return data.features + return [] } diff --git a/src/models/responseDetails.js b/src/models/responseDetails.js index 7e3550118..56d9c9eb7 100644 --- a/src/models/responseDetails.js +++ b/src/models/responseDetails.js @@ -32,12 +32,14 @@ import { pagination } from '../utils/pagination.js' */ export default class ResponseDetails { #cachedFields + #cachedGeometries - constructor (id, response, pagination, columnFieldLog) { + constructor (id, response, pagination, columnFieldLog, geometries) { this.id = id this.response = response this.pagination = pagination this.columnFieldLog = columnFieldLog + this.geometries = geometries } getRows () { @@ -162,6 +164,16 @@ export default class ResponseDetails { * @returns {any[] | undefined } */ getGeometries () { + if (this.#cachedGeometries) { + return this.#cachedGeometries + } + + const geometries = normaliseGeometries(this.geometries) + if (geometries?.length > 0) { + this.#cachedGeometries = geometries + return this.#cachedGeometries + } + const rows = this.getRows() if (rows.length === 0) { return undefined @@ -177,20 +189,21 @@ export default class ResponseDetails { return undefined } - const geometries = [] + const rowGeometries = [] for (const item of rows) { const geometry = getGeometryValue(item) if (geometry && geometry.trim() !== '') { - geometries.push(geometry) + rowGeometries.push(geometry) } } logger.debug('getGetometries()', { type: types.App, requestId: this.id, - geometryCount: geometries.length, + geometryCount: rowGeometries.length, rowCount: rows.length }) - return geometries + this.#cachedGeometries = rowGeometries + return this.#cachedGeometries } /** @@ -263,3 +276,28 @@ export default class ResponseDetails { return undefined } } + +function normaliseGeometries (data) { + if (!data) return undefined + + const items = Array.isArray(data) + ? data + : data.geometries ?? data.features + + if (!Array.isArray(items)) return undefined + + const geometries = items + .map(item => { + if (typeof item === 'string') return item + if (typeof item?.geo === 'string') return item + if (typeof item?.geometry === 'string') return item.geometry + if (typeof item?.value === 'string') return item.value + if (Array.isArray(item?.transformed_row)) { + return item.transformed_row.find(obj => obj.field === 'geometry' || obj.field === 'point')?.value + } + return undefined + }) + .filter(Boolean) + + return geometries.length > 0 ? geometries : undefined +} diff --git a/test/unit/requestData.test.js b/test/unit/requestData.test.js index 9f4482639..2a8f235ef 100644 --- a/test/unit/requestData.test.js +++ b/test/unit/requestData.test.js @@ -1,4 +1,4 @@ -import RequestData, { fetchPaginated } from '../../src/models/requestData.js' +import RequestData from '../../src/models/requestData.js' import ResponseDetails from '../../src/models/responseDetails.js' import { describe, it, expect, vi } from 'vitest' import axios from 'axios' @@ -19,7 +19,7 @@ vi.spyOn(logger, 'error') // Tech Debt: we should write some more tests around the requestData.js file describe('RequestData', () => { describe('fetchResponseDetails', () => { - it('should return a new ResponseDetails object (paginated)', async () => { + it('should return a new ResponseDetails object with current page data only', async () => { axios.get.mockResolvedValueOnce({ headers: { 'x-pagination-total-results': '2', @@ -28,14 +28,6 @@ describe('RequestData', () => { }, data: [{ 'error-summary': ['error1', 'error2'] }] }) - .mockResolvedValueOnce({ - headers: { - 'x-pagination-total-results': '2', - 'x-pagination-offset': '1', - 'x-pagination-limit': '1' - }, - data: [{ 'error-summary': ['error1', 'error2'] }] - }) const response = { id: 1, @@ -49,11 +41,11 @@ describe('RequestData', () => { expect(responseDetails.pagination.totalResults).toBe('2') expect(responseDetails.pagination.offset).toBe('0') - expect(responseDetails.pagination.limit).toBe('2') + expect(responseDetails.pagination.limit).toBe('1') expect(responseDetails.response).toStrictEqual([ - { 'error-summary': ['error1', 'error2'] }, { 'error-summary': ['error1', 'error2'] } ]) + expect(axios.get).toHaveBeenCalledTimes(1) }) it('should return a new ResponseDetails object', async () => { @@ -78,7 +70,7 @@ describe('RequestData', () => { expect(responseDetails.pagination.totalResults).toBe('1') expect(responseDetails.pagination.offset).toBe('0') - expect(responseDetails.pagination.limit).toBe(responseDetails.pagination.totalResults) + expect(responseDetails.pagination.limit).toBe('50') expect(responseDetails.response).toStrictEqual([{ 'error-summary': ['error1', 'error2'] }]) const url = new URL('http://localhost:8001/requests/1/response-details?offset=0&limit=50') @@ -106,6 +98,71 @@ describe('RequestData', () => { expect(axios.get).toHaveBeenCalledWith(url, { timeout: 30000 }) }) + + it('fetches and caches geometry-only data when requested', async () => { + axios.get + .mockResolvedValueOnce({ + headers: { + 'x-pagination-total-results': 1, + 'x-pagination-offset': 0, + 'x-pagination-limit': 50 + }, + data: [{ 'error-summary': ['error1', 'error2'] }] + }) + .mockResolvedValueOnce({ + data: ['POINT (1 2)', 'POINT (3 4)'] + }) + + const response = { + id: 1, + getColumnFieldLog: () => [] + } + const requestData = new RequestData(response) + + const responseDetails = await requestData.fetchResponseDetails(0, 50, { includeGeometries: true }) + + expect(responseDetails.getGeometries()).toStrictEqual(['POINT (1 2)', 'POINT (3 4)']) + expect(axios.get).toHaveBeenCalledWith( + new URL('http://localhost:8001/requests/1/geometries'), + { timeout: 30000 } + ) + }) + + it('fetches paginated geometry-only data when requested', async () => { + axios.get + .mockResolvedValueOnce({ + headers: { + 'x-pagination-total-results': 1, + 'x-pagination-offset': 0, + 'x-pagination-limit': 50 + }, + data: [{ 'error-summary': ['error1', 'error2'] }] + }) + .mockResolvedValueOnce({ + headers: { + 'x-pagination-total-results': 3, + 'x-pagination-limit': 2 + }, + data: ['POINT (1 2)', 'POINT (3 4)'] + }) + .mockResolvedValueOnce({ + data: ['POINT (5 6)'] + }) + + const response = { + id: 1, + getColumnFieldLog: () => [] + } + const requestData = new RequestData(response) + + const responseDetails = await requestData.fetchResponseDetails(0, 50, { includeGeometries: true }) + + expect(responseDetails.getGeometries()).toStrictEqual(['POINT (1 2)', 'POINT (3 4)', 'POINT (5 6)']) + expect(axios.get).toHaveBeenCalledWith( + new URL('http://localhost:8001/requests/1/geometries?offset=2&limit=2'), + { timeout: 30000 } + ) + }) }) describe('isFailed', () => { @@ -431,11 +488,3 @@ describe('RequestData', () => { }) }) }) - -describe('fetchPaginated', async () => { - it('makes paginated fetch', async ({ expect }) => { - const url = new URL('http://example.com/response-details') - const result = await fetchPaginated(url, { limit: 2, offset: 0, maxOffset: 7 }) - expect(result.length).toBe(4) - }) -}) diff --git a/test/unit/responseDetails.test.js b/test/unit/responseDetails.test.js index a502544fb..296327839 100644 --- a/test/unit/responseDetails.test.js +++ b/test/unit/responseDetails.test.js @@ -261,6 +261,20 @@ describe('ResponseDetails', () => { expect(result).toEqual(expected) }) + it('returns cached geometry-only data before row geometries', () => { + const responseDetails = new ResponseDetails( + undefined, + mockResponse, + undefined, + undefined, + ['POINT (1 2)', { geo: 'POINT (3 4)' }] + ) + + const result = responseDetails.getGeometries() + + expect(result).toEqual(['POINT (1 2)', { geo: 'POINT (3 4)' }]) + }) + it('handles Geox, GeoY columns', () => { const mockColumnFieldLog = [] const responseDetails = new ResponseDetails(undefined, mockResponsWithGeoXGeoY, undefined, mockColumnFieldLog) From 5701273c3d5c1fdee2d632a238912c78ef06d928 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Mon, 8 Jun 2026 16:17:02 +0100 Subject: [PATCH 2/3] Refactor response detail fetching to remove geometry inclusion and improve caching; update tests accordingly --- src/controllers/resultsController.js | 115 ++++++++++++------------ src/models/requestData.js | 37 +------- src/models/responseDetails.js | 100 +++++++++------------ test/unit/requestData.test.js | 65 -------------- test/unit/responseDetails.test.js | 126 +++++++++++++++++---------- test/unit/resultsController.test.js | 8 +- 6 files changed, 188 insertions(+), 263 deletions(-) diff --git a/src/controllers/resultsController.js b/src/controllers/resultsController.js index 8c32992f0..15be56300 100644 --- a/src/controllers/resultsController.js +++ b/src/controllers/resultsController.js @@ -147,10 +147,7 @@ export async function fetchResponseDetails (req, res, next) { if (req.locals.template !== failedFileRequestTemplate && req.locals.template !== failedUrlRequestTemplate) { const detailsOpts = req.locals.detailsOptions ?? {} // Original code used a if statement to check template and filter by error severity accordingly, but move to always showing all details in results/results template - const responseDetails = await req.locals.requestData.fetchResponseDetails(pageNumber - 1, 50, { - ...detailsOpts, - includeGeometries: req.locals.datasetTypology === 'geography' - }) + const responseDetails = await req.locals.requestData.fetchResponseDetails(pageNumber - 1, 50, { ...detailsOpts }) req.locals.responseDetails = responseDetails } } catch (e) { @@ -176,65 +173,69 @@ export const fieldToColumnMapping = ({ columns }) => { * @param {Function} next - Next middleware function * @returns {void} */ -export function setupTableParams (req, res, next) { - if (req.locals.template !== failedFileRequestTemplate && req.locals.template !== failedUrlRequestTemplate) { - const responseDetails = req.locals.responseDetails - // Optionally filter out all non - error rows from dataset - let rows = responseDetails.getRowsWithVerboseColumns(false) - // remove any issues that aren't of severity error - rows = rows.map((row) => { - const { columns, ...rest } = row - - const columnsOnlyErrors = Object.fromEntries(Object.entries(columns).map(([key, value]) => { - let error - if (value.error && value.error.severity === 'error' && value.error.responsibility !== 'internal') { - error = value.error - } - const newValue = { - ...value, - error +export async function setupTableParams (req, res, next) { + try { + if (req.locals.template !== failedFileRequestTemplate && req.locals.template !== failedUrlRequestTemplate) { + const responseDetails = req.locals.responseDetails + // Optionally filter out all non - error rows from dataset + let rows = responseDetails.getRowsWithVerboseColumns(false) + // remove any issues that aren't of severity error + rows = rows.map((row) => { + const { columns, ...rest } = row + + const columnsOnlyErrors = Object.fromEntries(Object.entries(columns).map(([key, value]) => { + let error + if (value.error && value.error.severity === 'error' && value.error.responsibility !== 'internal') { + error = value.error + } + const newValue = { + ...value, + error + } + return [key, newValue] + })) + + return { + ...rest, + columns: columnsOnlyErrors } - return [key, newValue] - })) + }) - return { - ...rest, - columns: columnsOnlyErrors + const fieldToColumn = rows.length > 0 ? fieldToColumnMapping(rows[0]) : new Map() + const columnToField = new Map() + for (const [k, v] of fieldToColumn.entries()) { + columnToField.set(v, k) } - }) - const fieldToColumn = rows.length > 0 ? fieldToColumnMapping(rows[0]) : new Map() - const columnToField = new Map() - for (const [k, v] of fieldToColumn.entries()) { - columnToField.set(v, k) - } - - const { leading: leadingFields, trailing: trailingFields } = splitByLeading({ fields: responseDetails.getFields() }) - // NOTE: the column field log alters the field names (converts '_' -> '-', most of the time 🤷‍♂️), but we want - // the original CSV column names because that's what users expect - const orderedFields = [...leadingFields, ...trailingFields] - const columns = orderedFields - const fields = orderedFields - req.locals.tableParams = { - columns, - fields, - rows, - columnNameProcessing: 'none', - mapping: columnToField + const { leading: leadingFields, trailing: trailingFields } = splitByLeading({ fields: responseDetails.getFields() }) + // NOTE: the column field log alters the field names (converts '_' -> '-', most of the time 🤷‍♂️), but we want + // the original CSV column names because that's what users expect + const orderedFields = [...leadingFields, ...trailingFields] + const columns = orderedFields + const fields = orderedFields + req.locals.tableParams = { + columns, + fields, + rows, + columnNameProcessing: 'none', + mapping: columnToField + } + req.locals.geometries = + req.locals.datasetTypology === 'geography' + ? await responseDetails.getGeometries() + : null + // pagination is on the 'table' tab, so we want to ensure clicking those + // links takes us to a page with the table tab *selected* + const { pageNumber } = req.parsedParams + const pagination = responseDetails.getPagination(pageNumber, { hash: '#table-tab' }) + req.locals.pagination = pagination + req.locals.id = req.params.id + req.locals.lastPage = `/check/status/${req.params.id}` } - req.locals.geometries = - req.locals.datasetTypology === 'geography' - ? responseDetails.getGeometries() - : null - // pagination is on the 'table' tab, so we want to ensure clicking those - // links takes us to a page with the table tab *selected* - const { pageNumber } = req.parsedParams - const pagination = responseDetails.getPagination(pageNumber, { hash: '#table-tab' }) - req.locals.pagination = pagination - req.locals.id = req.params.id - req.locals.lastPage = `/check/status/${req.params.id}` + next() + } catch (error) { + next(error) } - next() } export function setupError (req, res, next) { diff --git a/src/models/requestData.js b/src/models/requestData.js index beb9088ed..ce2e63fb7 100644 --- a/src/models/requestData.js +++ b/src/models/requestData.js @@ -6,7 +6,6 @@ import ResponseDetails from './responseDetails.js' const ResponseDetailsOptions = v.optional(v.object({ severity: v.optional(v.pipe(v.string(), v.minLength(2))), - includeGeometries: v.optional(v.boolean()), issue: v.optional(v.object({ issueType: v.pipe(v.string(), v.minLength(1)), field: v.pipe(v.string(), v.minLength(1)) @@ -50,11 +49,7 @@ export default class ResultData { url.searchParams.append('jsonpath', `$.issue_logs[*].severity=="${opts.severity}"`) } - const geometryUrl = new URL(`${config.asyncRequestApi.url}/${config.asyncRequestApi.requestsEndpoint}/${this.id}/geometries`) - const [response, geometriesResponse] = await Promise.all([ - axios.get(url, { timeout: 30000 }), - opts.includeGeometries ? fetchGeometries(geometryUrl) : Promise.resolve(undefined) - ]) + const response = await axios.get(url, { timeout: 30000 }) const totalResults = Number.parseInt(response.headers['x-pagination-total-results']) const pagination = { @@ -63,7 +58,7 @@ export default class ResultData { limit: `${limit}` } - return new ResponseDetails(this.id, response.data, pagination, this.getColumnFieldLog(), geometriesResponse) + return new ResponseDetails(this.id, response.data, pagination, this.getColumnFieldLog()) } isFailed () { @@ -185,31 +180,3 @@ export default class ResultData { return this.response.data.plugin ?? null } } - -async function fetchGeometries (url) { - const response = await axios.get(url, { timeout: 30000 }) - const totalResults = Number.parseInt(response.headers?.['x-pagination-total-results']) - const limit = Number.parseInt(response.headers?.['x-pagination-limit']) || getGeometryItems(response.data).length || 500 - const geometries = getGeometryItems(response.data) - - if (!Number.isInteger(totalResults) || geometries.length >= totalResults) { - return geometries.length > 0 ? geometries : response.data - } - - for (let offset = geometries.length; offset < totalResults; offset += limit) { - const pageUrl = new URL(url) - pageUrl.searchParams.set('offset', offset) - pageUrl.searchParams.set('limit', limit) - const page = await axios.get(pageUrl, { timeout: 30000 }) - geometries.push(...getGeometryItems(page.data)) - } - - return geometries -} - -function getGeometryItems (data) { - if (Array.isArray(data)) return data - if (Array.isArray(data?.geometries)) return data.geometries - if (Array.isArray(data?.features)) return data.features - return [] -} diff --git a/src/models/responseDetails.js b/src/models/responseDetails.js index 56d9c9eb7..049a869d0 100644 --- a/src/models/responseDetails.js +++ b/src/models/responseDetails.js @@ -2,6 +2,8 @@ import { getVerboseColumns } from '../utils/getVerboseColumns.js' import logger from '../utils/logger.js' import { types } from '../utils/logging.js' import { pagination } from '../utils/pagination.js' +import axios from 'axios' +import config from '../../config/index.js' /** * @typedef {Object} PaginationOptions @@ -33,13 +35,13 @@ import { pagination } from '../utils/pagination.js' export default class ResponseDetails { #cachedFields #cachedGeometries + #hasFetchedGeometries = false - constructor (id, response, pagination, columnFieldLog, geometries) { + constructor (id, response, pagination, columnFieldLog) { this.id = id this.response = response this.pagination = pagination this.columnFieldLog = columnFieldLog - this.geometries = geometries } getRows () { @@ -163,47 +165,50 @@ export default class ResponseDetails { * * @returns {any[] | undefined } */ - getGeometries () { - if (this.#cachedGeometries) { + async getGeometries () { + if (this.#hasFetchedGeometries) { return this.#cachedGeometries } - const geometries = normaliseGeometries(this.geometries) - if (geometries?.length > 0) { - this.#cachedGeometries = geometries - return this.#cachedGeometries - } + this.#cachedGeometries = await this.#fetchGeometries() + this.#hasFetchedGeometries = true + return this.#cachedGeometries + } - const rows = this.getRows() - if (rows.length === 0) { + async #fetchGeometries () { + if (!this.id) { return undefined } - const item = rows[0] - const getGeometryValue = this.#makeGeometryGetter(item) - if (!getGeometryValue) { - logger.debug('could not create geometry getter', { - type: types.App, - requestId: this.id + const url = new URL(`${config.asyncRequestApi.url}/${config.asyncRequestApi.requestsEndpoint}/${this.id}/geometries`) + let response + try { + response = await axios.get(url, { timeout: 30000 }) + } catch (error) { + logger.warn('failed to fetch response geometries', { + type: types.DataFetch, + requestId: this.id, + errorMessage: error.message }) return undefined } + const totalResults = Number.parseInt(response.headers?.['x-pagination-total-results']) + const limit = Number.parseInt(response.headers?.['x-pagination-limit']) || getGeometryItems(response.data).length || 500 + const geometries = normaliseGeometries(response.data) ?? [] - const rowGeometries = [] - for (const item of rows) { - const geometry = getGeometryValue(item) - if (geometry && geometry.trim() !== '') { - rowGeometries.push(geometry) - } + if (!Number.isInteger(totalResults) || geometries.length >= totalResults) { + return geometries.length > 0 ? geometries : normaliseGeometries(response.data) } - logger.debug('getGetometries()', { - type: types.App, - requestId: this.id, - geometryCount: rowGeometries.length, - rowCount: rows.length - }) - this.#cachedGeometries = rowGeometries - return this.#cachedGeometries + + for (let offset = geometries.length; offset < totalResults; offset += limit) { + const pageUrl = new URL(url) + pageUrl.searchParams.set('offset', offset) + pageUrl.searchParams.set('limit', limit) + const page = await axios.get(pageUrl, { timeout: 30000 }) + geometries.push(...(normaliseGeometries(page.data) ?? [])) + } + + return geometries } /** @@ -249,32 +254,6 @@ export default class ResponseDetails { items } } - - /** - * Detects where geometry is stored in the item and returns a function to extract geometry value. - * It's caller's responsibility to handle situations where the getter couldn't be returned. - * For most common use cases, we can omit displaying the map. - * - * @param {Object} item - Data item containing geometry information - * @returns {Function|undefined} Function that takes an item and returns a geometry string, or undefined if no geometry found - */ - #makeGeometryGetter (item) { - /* - The api seems to sometimes respond with weird casing, it can be camal case, all lower or all upper - I'll implement a fix here, but hopefully infa will be addressing it on the backend to - */ - const trow = item.transformed_row - if (trow) { - const key = trow.find(obj => obj.field === 'geometry' || obj.field === 'point')?.field - const getter = (row) => { - const geometry = row.transformed_row?.find(obj => obj.field === key) - return geometry?.value - } - return getter - } - - return undefined - } } function normaliseGeometries (data) { @@ -301,3 +280,10 @@ function normaliseGeometries (data) { return geometries.length > 0 ? geometries : undefined } + +function getGeometryItems (data) { + if (Array.isArray(data)) return data + if (Array.isArray(data?.geometries)) return data.geometries + if (Array.isArray(data?.features)) return data.features + return [] +} diff --git a/test/unit/requestData.test.js b/test/unit/requestData.test.js index 2a8f235ef..e4c8b84f1 100644 --- a/test/unit/requestData.test.js +++ b/test/unit/requestData.test.js @@ -98,71 +98,6 @@ describe('RequestData', () => { expect(axios.get).toHaveBeenCalledWith(url, { timeout: 30000 }) }) - - it('fetches and caches geometry-only data when requested', async () => { - axios.get - .mockResolvedValueOnce({ - headers: { - 'x-pagination-total-results': 1, - 'x-pagination-offset': 0, - 'x-pagination-limit': 50 - }, - data: [{ 'error-summary': ['error1', 'error2'] }] - }) - .mockResolvedValueOnce({ - data: ['POINT (1 2)', 'POINT (3 4)'] - }) - - const response = { - id: 1, - getColumnFieldLog: () => [] - } - const requestData = new RequestData(response) - - const responseDetails = await requestData.fetchResponseDetails(0, 50, { includeGeometries: true }) - - expect(responseDetails.getGeometries()).toStrictEqual(['POINT (1 2)', 'POINT (3 4)']) - expect(axios.get).toHaveBeenCalledWith( - new URL('http://localhost:8001/requests/1/geometries'), - { timeout: 30000 } - ) - }) - - it('fetches paginated geometry-only data when requested', async () => { - axios.get - .mockResolvedValueOnce({ - headers: { - 'x-pagination-total-results': 1, - 'x-pagination-offset': 0, - 'x-pagination-limit': 50 - }, - data: [{ 'error-summary': ['error1', 'error2'] }] - }) - .mockResolvedValueOnce({ - headers: { - 'x-pagination-total-results': 3, - 'x-pagination-limit': 2 - }, - data: ['POINT (1 2)', 'POINT (3 4)'] - }) - .mockResolvedValueOnce({ - data: ['POINT (5 6)'] - }) - - const response = { - id: 1, - getColumnFieldLog: () => [] - } - const requestData = new RequestData(response) - - const responseDetails = await requestData.fetchResponseDetails(0, 50, { includeGeometries: true }) - - expect(responseDetails.getGeometries()).toStrictEqual(['POINT (1 2)', 'POINT (3 4)', 'POINT (5 6)']) - expect(axios.get).toHaveBeenCalledWith( - new URL('http://localhost:8001/requests/1/geometries?offset=2&limit=2'), - { timeout: 30000 } - ) - }) }) describe('isFailed', () => { diff --git a/test/unit/responseDetails.test.js b/test/unit/responseDetails.test.js index 296327839..c40290421 100644 --- a/test/unit/responseDetails.test.js +++ b/test/unit/responseDetails.test.js @@ -1,5 +1,8 @@ import ResponseDetails from '../../src/models/responseDetails.js' -import { describe, it, expect, vi } from 'vitest' +import { beforeEach, describe, it, expect, vi } from 'vitest' +import axios from 'axios' + +vi.mock('axios') vi.mock('../../src/utils/getVerboseColumns.js', () => { return { @@ -10,6 +13,10 @@ vi.mock('../../src/utils/getVerboseColumns.js', () => { }) describe('ResponseDetails', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + const mockResponse = [ { issue_logs: [], @@ -49,17 +56,6 @@ describe('ResponseDetails', () => { } ] - const mockResponsWithGeoXGeoY = mockResponse.map(({ converted_row: row, ...entry }, i) => { - const { geometry, wkt, ...other } = row - return { - ...entry, - converted_row: { ...other, GeoX: `123.4${i}`, GeoY: `123.4${i}` }, - transformed_row: [ - { field: 'geometry', value: `POINT (123.4${i} 123.4${i})` } - ] - } - }) - const mockPagination = { totalResults: 2, offset: 0, @@ -233,57 +229,97 @@ describe('ResponseDetails', () => { }) describe('getGeometries', () => { - it('returns undefined and logs an error if there is no response', () => { + it('returns undefined if there is no request id', async () => { const responseDetails = new ResponseDetails(undefined, undefined, undefined, undefined) - const result = responseDetails.getGeometries() + const result = await responseDetails.getGeometries() + expect(result).toBeUndefined() + expect(axios.get).not.toHaveBeenCalled() }) - it('returns null if there are no geometries', () => { - const responseDetails = new ResponseDetails(undefined, [], undefined, undefined) - const result = responseDetails.getGeometries() + it('returns undefined if there are no geometries', async () => { + axios.get.mockResolvedValueOnce({ + headers: {}, + data: [] + }) + const responseDetails = new ResponseDetails(1, [], undefined, undefined) + const result = await responseDetails.getGeometries() + expect(result).toBeUndefined() }) - it('returns an array of geometries', () => { - const mockColumnFieldLog = [ - { column: 'id', field: 'ID' }, - { column: 'wkt', field: 'WKT' }, - { column: 'geometry', field: 'geometry' }, - { column: 'name', field: 'Name' } - ] - const responseDetails = new ResponseDetails(undefined, mockResponse, undefined, mockColumnFieldLog) - const result = responseDetails.getGeometries() + it('caches empty geometry responses', async () => { + axios.get.mockResolvedValueOnce({ + headers: {}, + data: [] + }) + const responseDetails = new ResponseDetails(1, [], undefined, undefined) + + await responseDetails.getGeometries() + const result = await responseDetails.getGeometries() + + expect(result).toBeUndefined() + expect(axios.get).toHaveBeenCalledTimes(1) + }) + + it('returns geometry-only endpoint data', async () => { + axios.get.mockResolvedValueOnce({ + headers: {}, + data: [ + 'POINT (423432.0000000000000000 564564.0000000000000000)', + 'POINT (423432.0000000000000000 564564.0000000000000000)' + ] + }) + const responseDetails = new ResponseDetails(1, mockResponse, undefined, undefined) + const result = await responseDetails.getGeometries() const expected = [ 'POINT (423432.0000000000000000 564564.0000000000000000)', 'POINT (423432.0000000000000000 564564.0000000000000000)' ] + expect(result).toEqual(expected) + expect(axios.get).toHaveBeenCalledWith( + new URL('http://localhost:8001/requests/1/geometries'), + { timeout: 30000 } + ) }) - it('returns cached geometry-only data before row geometries', () => { - const responseDetails = new ResponseDetails( - undefined, - mockResponse, - undefined, - undefined, - ['POINT (1 2)', { geo: 'POINT (3 4)' }] - ) + it('fetches paginated geometry-only endpoint data', async () => { + axios.get + .mockResolvedValueOnce({ + headers: { + 'x-pagination-total-results': 3, + 'x-pagination-limit': 2 + }, + data: ['POINT (1 2)', 'POINT (3 4)'] + }) + .mockResolvedValueOnce({ + headers: {}, + data: ['POINT (5 6)'] + }) - const result = responseDetails.getGeometries() + const responseDetails = new ResponseDetails(1, mockResponse, undefined, undefined) + const result = await responseDetails.getGeometries() - expect(result).toEqual(['POINT (1 2)', { geo: 'POINT (3 4)' }]) + expect(result).toEqual(['POINT (1 2)', 'POINT (3 4)', 'POINT (5 6)']) + expect(axios.get).toHaveBeenCalledWith( + new URL('http://localhost:8001/requests/1/geometries?offset=2&limit=2'), + { timeout: 30000 } + ) }) - it('handles Geox, GeoY columns', () => { - const mockColumnFieldLog = [] - const responseDetails = new ResponseDetails(undefined, mockResponsWithGeoXGeoY, undefined, mockColumnFieldLog) - const result = responseDetails.getGeometries() - const expected = [ - 'POINT (123.40 123.40)', - 'POINT (123.41 123.41)' - ] - expect(result).toEqual(expected) + it('caches geometry-only endpoint data', async () => { + axios.get.mockResolvedValueOnce({ + headers: {}, + data: ['POINT (1 2)'] + }) + const responseDetails = new ResponseDetails(1, mockResponse, undefined, undefined) + + await responseDetails.getGeometries() + const result = await responseDetails.getGeometries() + + expect(result).toEqual(['POINT (1 2)']) + expect(axios.get).toHaveBeenCalledTimes(1) }) }) diff --git a/test/unit/resultsController.test.js b/test/unit/resultsController.test.js index b6826bae4..b1b259b21 100644 --- a/test/unit/resultsController.test.js +++ b/test/unit/resultsController.test.js @@ -132,11 +132,11 @@ describe('Middleware Tests', () => { getRowsWithVerboseColumns: vi.fn(() => [{ columns: {}, data: 'rowData' }]), getColumns: vi.fn(() => ['column1', 'column2']), getFields: vi.fn(() => ['field1', 'field2']), - getGeometries: vi.fn(() => 'mockGeometries'), + getGeometries: vi.fn().mockResolvedValue('mockGeometries'), getPagination: vi.fn(() => 'mockPagination') } - setupTableParams(req, res, mockNext) + await setupTableParams(req, res, mockNext) expect(req.locals.tableParams).toEqual({ columns: ['field1', 'field2'], @@ -149,7 +149,7 @@ describe('Middleware Tests', () => { expect(req.locals.pagination).toEqual('mockPagination') expect(mockNext).toHaveBeenCalled() }) - it('hide map when typology is not geography', () => { + it('hide map when typology is not geography', async () => { const req = mockRequest() const res = mockResponse() @@ -164,7 +164,7 @@ describe('Middleware Tests', () => { getFields: vi.fn(() => ['field1', 'field2']), getPagination: vi.fn(() => 'mockPagination') } - setupTableParams(req, res, mockNext) + await setupTableParams(req, res, mockNext) expect(req.locals.geometries).toBeNull() }) }) From ebd0eb1967b68124e031d84d1588957c916b7161 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Mon, 8 Jun 2026 16:21:03 +0100 Subject: [PATCH 3/3] Refactor response detail fetching to simplify geometry handling and improve pagination logic --- src/models/responseDetails.js | 48 +++++++++-------------------------- 1 file changed, 12 insertions(+), 36 deletions(-) diff --git a/src/models/responseDetails.js b/src/models/responseDetails.js index 049a869d0..8aac1e7f0 100644 --- a/src/models/responseDetails.js +++ b/src/models/responseDetails.js @@ -193,11 +193,16 @@ export default class ResponseDetails { return undefined } const totalResults = Number.parseInt(response.headers?.['x-pagination-total-results']) - const limit = Number.parseInt(response.headers?.['x-pagination-limit']) || getGeometryItems(response.data).length || 500 - const geometries = normaliseGeometries(response.data) ?? [] + const geometries = response.data + + if (!Array.isArray(geometries)) { + return undefined + } + + const limit = Number.parseInt(response.headers?.['x-pagination-limit']) || geometries.length || 500 if (!Number.isInteger(totalResults) || geometries.length >= totalResults) { - return geometries.length > 0 ? geometries : normaliseGeometries(response.data) + return geometries.length > 0 ? geometries : undefined } for (let offset = geometries.length; offset < totalResults; offset += limit) { @@ -205,7 +210,10 @@ export default class ResponseDetails { pageUrl.searchParams.set('offset', offset) pageUrl.searchParams.set('limit', limit) const page = await axios.get(pageUrl, { timeout: 30000 }) - geometries.push(...(normaliseGeometries(page.data) ?? [])) + if (!Array.isArray(page.data)) { + break + } + geometries.push(...page.data) } return geometries @@ -255,35 +263,3 @@ export default class ResponseDetails { } } } - -function normaliseGeometries (data) { - if (!data) return undefined - - const items = Array.isArray(data) - ? data - : data.geometries ?? data.features - - if (!Array.isArray(items)) return undefined - - const geometries = items - .map(item => { - if (typeof item === 'string') return item - if (typeof item?.geo === 'string') return item - if (typeof item?.geometry === 'string') return item.geometry - if (typeof item?.value === 'string') return item.value - if (Array.isArray(item?.transformed_row)) { - return item.transformed_row.find(obj => obj.field === 'geometry' || obj.field === 'point')?.value - } - return undefined - }) - .filter(Boolean) - - return geometries.length > 0 ? geometries : undefined -} - -function getGeometryItems (data) { - if (Array.isArray(data)) return data - if (Array.isArray(data?.geometries)) return data.geometries - if (Array.isArray(data?.features)) return data.features - return [] -}