diff --git a/src/controllers/resultsController.js b/src/controllers/resultsController.js index dc7115de5..15be56300 100644 --- a/src/controllers/resultsController.js +++ b/src/controllers/resultsController.js @@ -173,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 ee9afed82..ce2e63fb7 100644 --- a/src/models/requestData.js +++ b/src/models/requestData.js @@ -1,6 +1,5 @@ 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' @@ -37,7 +36,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 +49,16 @@ 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 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()) } isFailed () { @@ -196,100 +180,3 @@ export default class ResultData { return this.response.data.plugin ?? null } } - -/** - * 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 - } -} - -/** - * - * @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 - } - } - 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 - } - - 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) -} - -/** - * - * @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 } }) -} diff --git a/src/models/responseDetails.js b/src/models/responseDetails.js index 7e3550118..8aac1e7f0 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 @@ -32,6 +34,8 @@ import { pagination } from '../utils/pagination.js' */ export default class ResponseDetails { #cachedFields + #cachedGeometries + #hasFetchedGeometries = false constructor (id, response, pagination, columnFieldLog) { this.id = id @@ -161,35 +165,57 @@ export default class ResponseDetails { * * @returns {any[] | undefined } */ - getGeometries () { - const rows = this.getRows() - if (rows.length === 0) { + async getGeometries () { + if (this.#hasFetchedGeometries) { + return this.#cachedGeometries + } + + this.#cachedGeometries = await this.#fetchGeometries() + this.#hasFetchedGeometries = true + return this.#cachedGeometries + } + + 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 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 : undefined + } - const geometries = [] - for (const item of rows) { - const geometry = getGeometryValue(item) - if (geometry && geometry.trim() !== '') { - geometries.push(geometry) + 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 }) + if (!Array.isArray(page.data)) { + break } + geometries.push(...page.data) } - logger.debug('getGetometries()', { - type: types.App, - requestId: this.id, - geometryCount: geometries.length, - rowCount: rows.length - }) + return geometries } @@ -236,30 +262,4 @@ 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 - } } diff --git a/test/unit/requestData.test.js b/test/unit/requestData.test.js index 9f4482639..e4c8b84f1 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') @@ -431,11 +423,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..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,43 +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('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('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 responseDetails = new ResponseDetails(1, mockResponse, undefined, undefined) + const result = await responseDetails.getGeometries() + + 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('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() }) })