Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

600 ensure issues are gathered from all active resources #603

Closed
wants to merge 8 commits into from
Closed

600 ensure issues are gathered from all active resources #603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6649057
fetchLatestResources should return all active resources, not expired …
GeorgeGoodall Oct 30, 2024
53d4b60
make use of the performance db and change overview page slightly
GeorgeGoodall Oct 30, 2024
78066e7
fix link
GeorgeGoodall Oct 30, 2024
32bcbbb
have endpoint error output if an exception is in the database
GeorgeGoodall Oct 30, 2024
4a94c02
Merge commit 'c8fc6cd1556df293f93983c653a70a585af385e1' into 600-ensu…
GeorgeGoodall Oct 30, 2024
b43c4f0
fetch tasks from all active resources
GeorgeGoodall Oct 30, 2024
348ae5e
get issue details on all resources
GeorgeGoodall Oct 30, 2024
e40ede1
get multiple sources working on issue details page
GeorgeGoodall Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/middleware/common.middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,19 @@ export const fetchLatestResource = fetchOne({
fallbackPolicy: FetchOneFallbackPolicy.continue
})

export const fetchResources = fetchMany({
query: ({ params }) => `select * from endpoint_dataset_resource_summary where REPLACE(organisation, '-eng', '') = "${params.lpa}" AND dataset = "${params.dataset}"`,
dataset: FetchOptions.performanceDb,
result: 'resources'
})
Comment on lines +54 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Address SQL injection vulnerability and improve query robustness

The current implementation has several concerns that need addressing:

  1. The query is vulnerable to SQL injection due to direct string interpolation
  2. The REPLACE operation on organisation might cause unintended matches
  3. No limit on result set size could impact performance with large datasets

Consider this safer implementation:

export const fetchResources = fetchMany({
-  query: ({ params }) => `select * from endpoint_dataset_resource_summary where REPLACE(organisation, '-eng', '') = "${params.lpa}" AND dataset = "${params.dataset}"`,
+  query: ({ params }) => ({
+    text: `SELECT * FROM endpoint_dataset_resource_summary 
+           WHERE REPLACE(organisation, '-eng', '') = $1 
+           AND dataset = $2 
+           LIMIT 1000`,
+    values: [params.lpa, params.dataset]
+  }),
  dataset: FetchOptions.performanceDb,
  result: 'resources'
})
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const fetchResources = fetchMany({
query: ({ params }) => `select * from endpoint_dataset_resource_summary where REPLACE(organisation, '-eng', '') = "${params.lpa}" AND dataset = "${params.dataset}"`,
dataset: FetchOptions.performanceDb,
result: 'resources'
})
export const fetchResources = fetchMany({
query: ({ params }) => ({
text: `SELECT * FROM endpoint_dataset_resource_summary
WHERE REPLACE(organisation, '-eng', '') = $1
AND dataset = $2
LIMIT 1000`,
values: [params.lpa, params.dataset]
}),
dataset: FetchOptions.performanceDb,
result: 'resources'
})


export const takeResourceIdFromParams = (req) => {
logger.debug('skipping resource fetch', { type: types.App, params: req.params })
req.resource = { resource: req.params.resourceId }
}

export const fetchEntityCount = fetchOne({
query: ({ req }) => performanceDbApi.entityCountQuery(req.resource.resource),
query: ({ req }) => performanceDbApi.entityCountQuery(req.orgInfo.entity),
result: 'entityCount',
dataset: FetchOptions.fromParams,
fallbackPolicy: FetchOneFallbackPolicy.continue
Expand Down
7 changes: 2 additions & 5 deletions src/middleware/datasetOverview.middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,8 @@ export const prepareDatasetOverviewTemplateParams = (req, res, next) => {
}).map((source, index) => {
let error

if (parseInt(source.status) < 200 || parseInt(source.status) >= 300) {
error = {
code: parseInt(source.status),
exception: source.exception
}
if (parseInt(source.status) < 200 || parseInt(source.status) >= 300 || source.status === '' || source.exception) {
error = 'There was a ' + (source.exception || source.status) + ' error accessing the endpoint'
Comment on lines +132 to +133
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Enhance error handling robustness and security

The error handling logic needs improvement in several areas:

  1. The parseInt() call should include a radix parameter
  2. The code should handle potential undefined/null values
  3. Exception messages might expose sensitive information

Consider applying these improvements:

-    if (parseInt(source.status) < 200 || parseInt(source.status) >= 300 || source.status === '' || source.exception) {
-      error = 'There was a ' + (source.exception || source.status) + ' error accessing the endpoint'
+    if (!source?.status || 
+        Number.parseInt(source.status, 10) < 200 || 
+        Number.parseInt(source.status, 10) >= 300 || 
+        source.exception) {
+      error = `Error accessing the endpoint: ${source.status ? `Status ${source.status}` : 'Unknown status'}`
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (parseInt(source.status) < 200 || parseInt(source.status) >= 300 || source.status === '' || source.exception) {
error = 'There was a ' + (source.exception || source.status) + ' error accessing the endpoint'
if (!source?.status ||
Number.parseInt(source.status, 10) < 200 ||
Number.parseInt(source.status, 10) >= 300 ||
source.exception) {
error = `Error accessing the endpoint: ${source.status ? `Status ${source.status}` : 'Unknown status'}`

}

return {
Expand Down
61 changes: 45 additions & 16 deletions src/middleware/datasetTaskList.middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { fetchDatasetInfo, isResourceAccessible, isResourceNotAccessible, fetchLatestResource, fetchEntityCount, logPageError, fetchLpaDatasetIssues, validateQueryParams, getDatasetTaskListError } from './common.middleware.js'
import { fetchOne, fetchIf, onlyIf, renderTemplate } from './middleware.builders.js'
import { fetchDatasetInfo, fetchEntityCount, logPageError, validateQueryParams, fetchResources } from './common.middleware.js'
import { fetchOne, renderTemplate, fetchMany } from './middleware.builders.js'
import performanceDbApi from '../services/performanceDbApi.js'
import { statusToTagClass } from '../filters/filters.js'
import * as v from 'valibot'
Expand All @@ -14,7 +14,7 @@

const fetchOrgInfoWithStatGeo = fetchOne({
query: ({ params }) => {
return /* sql */ `SELECT name, organisation, statistical_geography FROM organisation WHERE organisation = '${params.lpa}'`
return /* sql */ `SELECT name, organisation, statistical_geography, entity FROM organisation WHERE organisation = '${params.lpa}'`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security vulnerability: SQL injection risk

The query is vulnerable to SQL injection as it directly interpolates the params.lpa value into the SQL string. This could allow malicious input to manipulate the query.

Use parameterised queries instead:

-    return /* sql */ `SELECT name, organisation, statistical_geography, entity FROM organisation WHERE organisation = '${params.lpa}'`
+    return {
+      text: /* sql */ `SELECT name, organisation, statistical_geography, entity FROM organisation WHERE organisation = $1`,
+      values: [params.lpa]
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return /* sql */ `SELECT name, organisation, statistical_geography, entity FROM organisation WHERE organisation = '${params.lpa}'`
return {
text: /* sql */ `SELECT name, organisation, statistical_geography, entity FROM organisation WHERE organisation = $1`,
values: [params.lpa]
}

},
result: 'orgInfo'
})
Expand Down Expand Up @@ -43,19 +43,19 @@
* @returns { { templateParams: object }}
*/
export const prepareDatasetTaskListTemplateParams = (req, res, next) => {
const { issues, entityCount: entityCountRow, params, dataset, orgInfo: organisation } = req
const { entityCount: entityCountRow, params, dataset, orgInfo: organisation, tasks } = 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')

Comment on lines +46 to +49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add null check for tasks array

The test failure indicates that tasks is undefined. Add a default value to prevent the TypeError.

-  const { entityCount: entityCountRow, params, dataset, orgInfo: organisation, tasks } = req
+  const { entityCount: entityCountRow, params, dataset, orgInfo: organisation, tasks = [] } = req
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const { entityCount: entityCountRow, params, dataset, orgInfo: organisation, tasks } = 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')
const { entityCount: entityCountRow, params, dataset, orgInfo: organisation, tasks = [] } = req
const { entity_count: entityCount } = entityCountRow ?? { entity_count: 0 }
const { lpa, dataset: datasetId } = params

console.assert(typeof entityCount === 'number', 'entityCount should be a number')

const taskList = issues.map((issue) => {
const taskList = tasks.map((task) => {

Check failure on line 52 in src/middleware/datasetTaskList.middleware.js

View workflow job for this annotation

GitHub Actions / run-tests / test

test/unit/middleware/datasetTaskList.middleware.test.js > datasetTaskList.middleware.js > prepareDatasetTaskListParams > sets the correct template params on the request object 

TypeError: Cannot read properties of undefined (reading 'map')
 ❯ Module.prepareDatasetTaskListTemplateParams src/middleware/datasetTaskList.middleware.js:52:26
 ❯ test/unit/middleware/datasetTaskList.middleware.test.js:44:7

Check failure on line 52 in src/middleware/datasetTaskList.middleware.js

View workflow job for this annotation

GitHub Actions / test

test/unit/middleware/datasetTaskList.middleware.test.js > datasetTaskList.middleware.js > prepareDatasetTaskListParams > sets the correct template params on the request object 

TypeError: Cannot read properties of undefined (reading 'map')
 ❯ Module.prepareDatasetTaskListTemplateParams src/middleware/datasetTaskList.middleware.js:52:26
 ❯ test/unit/middleware/datasetTaskList.middleware.test.js:44:7
return {
title: {
text: performanceDbApi.getTaskMessage({ ...issue, entityCount, field: issue.field })
text: performanceDbApi.getTaskMessage({ ...task, field: task.field }) // using the entity count here doesn't make sense, should be using the entry count for each resource
},
href: `/organisations/${lpa}/${datasetId}/${issue.issue_type}/${issue.field}`,
status: getStatusTag(issue.status)
href: `/organisations/${lpa}/${datasetId}/${task.issue_type}/${task.field}`,
status: getStatusTag(task.status)
Comment on lines +52 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add defensive checks for task properties

The task mapping assumes all required properties exist. Consider adding defensive checks:

 const taskList = tasks.map((task) => {
+  if (!task?.issue_type || !task?.field) {
+    console.warn('Task missing required properties:', task)
+    return null
+  }
   return {
     title: {
       text: performanceDbApi.getTaskMessage({ ...task, field: task.field })
     },
     href: `/organisations/${lpa}/${datasetId}/${task.issue_type}/${task.field}`,
     status: getStatusTag(task.status)
   }
-})
+}).filter(Boolean)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const taskList = tasks.map((task) => {
return {
title: {
text: performanceDbApi.getTaskMessage({ ...issue, entityCount, field: issue.field })
text: performanceDbApi.getTaskMessage({ ...task, field: task.field }) // using the entity count here doesn't make sense, should be using the entry count for each resource
},
href: `/organisations/${lpa}/${datasetId}/${issue.issue_type}/${issue.field}`,
status: getStatusTag(issue.status)
href: `/organisations/${lpa}/${datasetId}/${task.issue_type}/${task.field}`,
status: getStatusTag(task.status)
const taskList = tasks.map((task) => {
if (!task?.issue_type || !task?.field) {
console.warn('Task missing required properties:', task)
return null
}
return {
title: {
text: performanceDbApi.getTaskMessage({ ...task, field: task.field }) // using the entity count here doesn't make sense, should be using the entry count for each resource
},
href: `/organisations/${lpa}/${datasetId}/${task.issue_type}/${task.field}`,
status: getStatusTag(task.status)
}
}).filter(Boolean)
🧰 Tools
🪛 GitHub Check: test

[failure] 52-52: test/unit/middleware/datasetTaskList.middleware.test.js > datasetTaskList.middleware.js > prepareDatasetTaskListParams > sets the correct template params on the request object
TypeError: Cannot read properties of undefined (reading 'map')
❯ Module.prepareDatasetTaskListTemplateParams src/middleware/datasetTaskList.middleware.js:52:26
❯ test/unit/middleware/datasetTaskList.middleware.test.js:44:7

}
})

Expand Down Expand Up @@ -113,17 +113,46 @@
})
})

export const fetchLpaDatasetTasks = fetchMany({
query: ({ req, params }) => `
SELECT
i.field,
i.issue_type,
i.line_number,
i.value,
i.message,
CASE
WHEN COUNT(
CASE
WHEN it.severity == 'error' THEN 1
ELSE null
END
) > 0 THEN 'Needs fixing'
ELSE 'Live'
END AS status,
COUNT(i.issue_type) as num_issues
FROM
issue i
LEFT JOIN
issue_type it ON i.issue_type = it.issue_type
WHERE
i.resource in ('${req.resources.map(resource => resource.resource).join("', '")}')
AND i.dataset = '${params.dataset}'
AND (it.severity == 'error')
GROUP BY i.issue_type, i.field
ORDER BY it.severity`,
result: 'tasks'
})
Comment on lines +116 to +145
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Fix SQL injection vulnerabilities and syntax issues

Several issues need addressing in this query:

  1. SQL injection vulnerability with string interpolation
  2. Incorrect SQL syntax using == instead of = for comparisons
  3. Potential performance impact with complex joins
 export const fetchLpaDatasetTasks = fetchMany({
-  query: ({ req, params }) => `
+  query: ({ req, params }) => ({
+    text: `
     SELECT
       i.field,
       i.issue_type,
       i.line_number,
       i.value,
       i.message,
       CASE
         WHEN COUNT(
           CASE
-            WHEN it.severity == 'error' THEN 1
+            WHEN it.severity = 'error' THEN 1
             ELSE null
           END
         ) > 0 THEN 'Needs fixing'
         ELSE 'Live'
       END AS status,
       COUNT(i.issue_type) as num_issues
     FROM
         issue i
     LEFT JOIN
       issue_type it ON i.issue_type = it.issue_type
     WHERE
-        i.resource in ('${req.resources.map(resource => resource.resource).join("', '")}')
-        AND i.dataset = '${params.dataset}'
-        AND (it.severity == 'error')
+        i.resource = ANY($1)
+        AND i.dataset = $2
+        AND (it.severity = 'error')
     GROUP BY i.issue_type, i.field
-    ORDER BY it.severity`,
+    ORDER BY it.severity
+    `,
+    values: [
+      req.resources.map(resource => resource.resource),
+      params.dataset
+    ]
+  }),
   result: 'tasks'
 })

Consider adding an index on (resource, dataset, severity) if not already present.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const fetchLpaDatasetTasks = fetchMany({
query: ({ req, params }) => `
SELECT
i.field,
i.issue_type,
i.line_number,
i.value,
i.message,
CASE
WHEN COUNT(
CASE
WHEN it.severity == 'error' THEN 1
ELSE null
END
) > 0 THEN 'Needs fixing'
ELSE 'Live'
END AS status,
COUNT(i.issue_type) as num_issues
FROM
issue i
LEFT JOIN
issue_type it ON i.issue_type = it.issue_type
WHERE
i.resource in ('${req.resources.map(resource => resource.resource).join("', '")}')
AND i.dataset = '${params.dataset}'
AND (it.severity == 'error')
GROUP BY i.issue_type, i.field
ORDER BY it.severity`,
result: 'tasks'
})
export const fetchLpaDatasetTasks = fetchMany({
query: ({ req, params }) => ({
text: `
SELECT
i.field,
i.issue_type,
i.line_number,
i.value,
i.message,
CASE
WHEN COUNT(
CASE
WHEN it.severity = 'error' THEN 1
ELSE null
END
) > 0 THEN 'Needs fixing'
ELSE 'Live'
END AS status,
COUNT(i.issue_type) as num_issues
FROM
issue i
LEFT JOIN
issue_type it ON i.issue_type = it.issue_type
WHERE
i.resource = ANY($1)
AND i.dataset = $2
AND (it.severity = 'error')
GROUP BY i.issue_type, i.field
ORDER BY it.severity
`,
values: [
req.resources.map(resource => resource.resource),
params.dataset
]
}),
result: 'tasks'
})


export default [
validateParams,
fetchResourceStatus,
fetchOrgInfoWithStatGeo,
fetchDatasetInfo,
fetchIf(isResourceAccessible, fetchLatestResource),
fetchIf(isResourceAccessible, fetchLpaDatasetIssues),
fetchIf(isResourceAccessible, fetchEntityCount),
onlyIf(isResourceAccessible, prepareDatasetTaskListTemplateParams),
onlyIf(isResourceAccessible, getDatasetTaskList),
onlyIf(isResourceNotAccessible, prepareDatasetTaskListErrorTemplateParams),
onlyIf(isResourceNotAccessible, getDatasetTaskListError),
fetchResources,
fetchLpaDatasetTasks,
fetchEntityCount,
prepareDatasetTaskListTemplateParams,
getDatasetTaskList,
logPageError
]
64 changes: 25 additions & 39 deletions src/middleware/issueDetails.middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
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 { fetchIf, renderTemplate } from './middleware.builders.js'
import { fetchDatasetInfo, fetchEntityCount, fetchOrgInfo, fetchResources, logPageError, validateQueryParams } from './common.middleware.js'
import { renderTemplate } from './middleware.builders.js'
import * as v from 'valibot'
import { pagination } from '../utils/pagination.js'

Expand Down Expand Up @@ -31,14 +29,10 @@ 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 { resources } = req

try {
const issues = await performanceDbApi.getIssues({ resource: resourceId, issueType, issueField }, datasetId)
const issues = await performanceDbApi.getIssues({ resources: resources.map(resource => resource.resource), issueType, issueField }, datasetId)
req.issues = issues
next()
} catch (error) {
Expand All @@ -56,11 +50,11 @@ async function fetchIssues (req, res, next) {
* @param {*} res
* @param {*} next
*/
async function reformatIssuesToBeByEntryNumber (req, res, next) {
async function reformatIssuesToBeByResourceEntryNumber (req, res, next) {
const { issues } = req
const issuesByEntryNumber = issues.reduce((acc, current) => {
acc[current.entry_number] = acc[current.entry_number] || []
acc[current.entry_number].push(current)
acc[current.resource + current.entry_number] = acc[current.resource + current.entry_number] || []
acc[current.resource + current.entry_number].push(current)
Comment on lines +53 to +57
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Prevent potential key collisions in issue grouping

The current key concatenation of resource + entry_number could lead to collisions. For instance, resource "r1" with entry "23" would create the same key as resource "r12" with entry "3".

Consider using a separator:

-    acc[current.resource + current.entry_number] = acc[current.resource + current.entry_number] || []
-    acc[current.resource + current.entry_number].push(current)
+    const key = `${current.resource}:${current.entry_number}`
+    acc[key] = acc[key] || []
+    acc[key].push(current)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async function reformatIssuesToBeByResourceEntryNumber (req, res, next) {
const { issues } = req
const issuesByEntryNumber = issues.reduce((acc, current) => {
acc[current.entry_number] = acc[current.entry_number] || []
acc[current.entry_number].push(current)
acc[current.resource + current.entry_number] = acc[current.resource + current.entry_number] || []
acc[current.resource + current.entry_number].push(current)
async function reformatIssuesToBeByResourceEntryNumber (req, res, next) {
const { issues } = req
const issuesByEntryNumber = issues.reduce((acc, current) => {
const key = `${current.resource}:${current.entry_number}`
acc[key] = acc[key] || []
acc[key].push(current)

return acc
}, {})
req.issuesByEntryNumber = issuesByEntryNumber
Expand All @@ -86,10 +80,13 @@ async function fetchEntry (req, res, next) {

// look at issue Entries and get the index of that entry - 1

const entityNum = Object.values(issuesByEntryNumber)[pageNum - 1][0].entry_number
const issue = Object.values(issuesByEntryNumber)[pageNum - 1][0]

const entityNum = issue.entry_number
const resource = issue.resource
Comment on lines +83 to +86
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add bounds checking for array access

The code assumes that Object.values(issuesByEntryNumber)[pageNum - 1] exists and has at least one element. This could throw an error if pageNum is out of bounds or if the array is empty.

Consider adding validation:

- const issue = Object.values(issuesByEntryNumber)[pageNum - 1][0]
+ const issues = Object.values(issuesByEntryNumber)
+ if (pageNum < 1 || pageNum > issues.length) {
+   throw new Error('Page number out of bounds')
+ }
+ const issue = issues[pageNum - 1][0]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const issue = Object.values(issuesByEntryNumber)[pageNum - 1][0]
const entityNum = issue.entry_number
const resource = issue.resource
const issues = Object.values(issuesByEntryNumber)
if (pageNum < 1 || pageNum > issues.length) {
throw new Error('Page number out of bounds')
}
const issue = issues[pageNum - 1][0]
const entityNum = issue.entry_number
const resource = issue.resource


req.entryData = await performanceDbApi.getEntry(
req.resource.resource,
resource,
entityNum,
datasetId
)
Expand All @@ -109,9 +106,8 @@ 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)
const { resources } = req
const issueEntitiesCount = await performanceDbApi.getEntitiesWithIssuesCount({ resources: resources.map(resource => resource.resource), issueType, issueField }, datasetId)
req.issueEntitiesCount = parseInt(issueEntitiesCount)
next()
}
Expand Down Expand Up @@ -182,29 +178,19 @@ 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, entryNumber } = req
const { lpa, dataset: datasetId, issue_type: issueType, issue_field: issueField } = req.params
const { entity_count: entityCount } = entityCountRow ?? { entity_count: 0 }

let errorHeading
let issueItems

const BaseSubpath = `/organisations/${lpa}/${datasetId}/${issueType}/${issueField}/`

if (Object.keys(issuesByEntryNumber).length < 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
return {
html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }) + ` in record ${entryNumber}`,
href: `${BaseSubpath}${pageNum}`
}
})
} else {
issueItems = [{
html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issueEntitiesCount, entityCount, field: issueField }, true)
}]
}
const errorHeading = performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issueEntitiesCount, field: issueField }, true)
const issueItems = Object.entries(issuesByEntryNumber).map(([entryNumber, issues], i) => {
const pageNum = i + 1
return {
html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }) + ` in record ${issues[0].entry_number}`,
href: `${BaseSubpath}${pageNum}`
}
})
Comment on lines +187 to +193
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimise issue items generation

Consider memoising the task message generation to avoid redundant calls to performanceDbApi.getTaskMessage() for identical issue types and fields:

+ const baseTaskMessage = performanceDbApi.getTaskMessage(
+   { issue_type: issueType, num_issues: 1, field: issueField }
+ )
  const issueItems = Object.entries(issuesByEntryNumber).map(([entryNumber, issues], i) => {
    const pageNum = i + 1
    return {
-     html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }) + ` in record ${issues[0].entry_number}`,
+     html: `${baseTaskMessage} in record ${issues[0].entry_number}`,
      href: `${BaseSubpath}${pageNum}`
    }
  })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const issueItems = Object.entries(issuesByEntryNumber).map(([entryNumber, issues], i) => {
const pageNum = i + 1
return {
html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }) + ` in record ${issues[0].entry_number}`,
href: `${BaseSubpath}${pageNum}`
}
})
const baseTaskMessage = performanceDbApi.getTaskMessage(
{ issue_type: issueType, num_issues: 1, field: issueField }
)
const issueItems = Object.entries(issuesByEntryNumber).map(([entryNumber, issues], i) => {
const pageNum = i + 1
return {
html: `${baseTaskMessage} in record ${issues[0].entry_number}`,
href: `${BaseSubpath}${pageNum}`
}
})


const fields = entryData.map((row) => processEntryRow(issueType, issuesByEntryNumber, row))
const entityIssues = Object.values(issuesByEntryNumber)[pageNumber - 1] || []
Expand Down Expand Up @@ -288,9 +274,9 @@ export default [
validateIssueDetailsQueryParams,
fetchOrgInfo,
fetchDatasetInfo,
fetchIf(isResourceIdInParams, fetchLatestResource, takeResourceIdFromParams),
fetchResources,
fetchIssues,
reformatIssuesToBeByEntryNumber,
reformatIssuesToBeByResourceEntryNumber,
fetchEntry,
fetchEntityCount,
fetchIssueEntitiesCount,
Expand Down
Loading
Loading