From 165eeb0aaecdd43b19c77285d62a63b2b54c1c88 Mon Sep 17 00:00:00 2001 From: Fenne Date: Thu, 5 Dec 2024 14:45:44 +0100 Subject: [PATCH 01/10] Added range based filtering to run number --- lib/usecases/run/GetAllRunsUseCase.js | 33 ++++++++++++++++++++------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/lib/usecases/run/GetAllRunsUseCase.js b/lib/usecases/run/GetAllRunsUseCase.js index fcf67c329c..2447a11be7 100644 --- a/lib/usecases/run/GetAllRunsUseCase.js +++ b/lib/usecases/run/GetAllRunsUseCase.js @@ -78,16 +78,33 @@ class GetAllRunsUseCase { if (runNumbers) { const runNumberList = runNumbers.split(SEARCH_ITEMS_SEPARATOR) - .map((runNumber) => parseInt(runNumber, 10)) - .filter((runNumber) => !Number.isNaN(runNumber)); + .map((runNumber) => runNumber.trim()) + .filter(Boolean); - if (runNumberList.length) { - if (runNumberList.length > 1) { - // Multiple run numbers. - filteringQueryBuilder.where('runNumber').oneOf(...runNumberList); + const allRuns = new Set(); + + runNumberList.forEach((runNumber) => { + if (runNumber.includes('-')) { + const [start, end] = runNumber.split('-').map((n) => parseInt(n, 10)); + if (!Number.isNaN(start) && !Number.isNaN(end) && start <= end) { + for (let i = start; i <= end; i++) { + allRuns.add(i); + } + } + } else { + const parsedRunNumber = parseInt(runNumber, 10); + if (!Number.isNaN(parsedRunNumber)) { + allRuns.add(parsedRunNumber); + } + } + }); + + const finalRunList = Array.from(allRuns); + if (finalRunList.length) { + if (finalRunList.length > 1) { + filteringQueryBuilder.where('runNumber').oneOf(...finalRunList); } else { - // One run number. - const [runNumber] = runNumberList; + const [runNumber] = finalRunList; filteringQueryBuilder.where('runNumber').substring(`${runNumber}`); } } From efbfe5fe3855a41df302b63377e6f7a905eb396b Mon Sep 17 00:00:00 2001 From: Fenne Date: Thu, 5 Dec 2024 14:58:50 +0100 Subject: [PATCH 02/10] added test that checks the runnumber range filter --- test/api/runs.test.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/api/runs.test.js b/test/api/runs.test.js index 8b1ccbc36e..948ebda5b7 100644 --- a/test/api/runs.test.js +++ b/test/api/runs.test.js @@ -149,6 +149,14 @@ module.exports = () => { expect(runs).to.lengthOf(2); }); + it('should successfully filter on run number range', async () => { + const response = await request(server).get('/api/runs?filter[runNumbers]=1-5,8,12,20-30'); + + expect(response.status).to.equal(200); + const { data: runs } = response.body; + expect(runs).to.lengthOf(18); + }); + it('should return 400 if the calibration status filter is invalid', async () => { { const response = await request(server).get('/api/runs?filter[calibrationStatuses]=invalid'); From 28ff5b201f7957989894f861c5e3caa679ce1528 Mon Sep 17 00:00:00 2001 From: Fenne Date: Mon, 9 Dec 2024 12:20:47 +0100 Subject: [PATCH 03/10] changed variable names to be more descriptive and remove redundant check --- lib/usecases/run/GetAllRunsUseCase.js | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/usecases/run/GetAllRunsUseCase.js b/lib/usecases/run/GetAllRunsUseCase.js index 2447a11be7..8453d15d2f 100644 --- a/lib/usecases/run/GetAllRunsUseCase.js +++ b/lib/usecases/run/GetAllRunsUseCase.js @@ -77,34 +77,34 @@ class GetAllRunsUseCase { } = filter; if (runNumbers) { - const runNumberList = runNumbers.split(SEARCH_ITEMS_SEPARATOR) - .map((runNumber) => runNumber.trim()) + const runNumberCriteria = runNumbers.split(SEARCH_ITEMS_SEPARATOR) + .map((runNumbers) => runNumbers.trim()) .filter(Boolean); - const allRuns = new Set(); + const runNumberSet = new Set(); - runNumberList.forEach((runNumber) => { + runNumberCriteria.forEach((runNumber) => { if (runNumber.includes('-')) { const [start, end] = runNumber.split('-').map((n) => parseInt(n, 10)); - if (!Number.isNaN(start) && !Number.isNaN(end) && start <= end) { + if (!Number.isNaN(start) && !Number.isNaN(end)) { for (let i = start; i <= end; i++) { - allRuns.add(i); + runNumberSet.add(i); } } } else { const parsedRunNumber = parseInt(runNumber, 10); if (!Number.isNaN(parsedRunNumber)) { - allRuns.add(parsedRunNumber); + runNumberSet.add(parsedRunNumber); } } }); - const finalRunList = Array.from(allRuns); - if (finalRunList.length) { - if (finalRunList.length > 1) { - filteringQueryBuilder.where('runNumber').oneOf(...finalRunList); + const finalRunNumberList = Array.from(runNumberSet); + if (finalRunNumberList.length) { + if (finalRunNumberList.length > 1) { + filteringQueryBuilder.where('runNumber').oneOf(...finalRunNumberList); } else { - const [runNumber] = finalRunList; + const [runNumber] = finalRunNumberList; filteringQueryBuilder.where('runNumber').substring(`${runNumber}`); } } From 930cb29d27b10f159f3b2ea1a82653ec6916f729 Mon Sep 17 00:00:00 2001 From: Fenne Date: Tue, 17 Dec 2024 13:21:07 +0100 Subject: [PATCH 04/10] Ranges now cant exceed 100 rows --- lib/usecases/run/GetAllRunsUseCase.js | 5 ++++- test/lib/usecases/run/GetAllRunsUseCase.test.js | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/lib/usecases/run/GetAllRunsUseCase.js b/lib/usecases/run/GetAllRunsUseCase.js index 8453d15d2f..982d3a5a12 100644 --- a/lib/usecases/run/GetAllRunsUseCase.js +++ b/lib/usecases/run/GetAllRunsUseCase.js @@ -77,6 +77,7 @@ class GetAllRunsUseCase { } = filter; if (runNumbers) { + const MAX_RANGE_SIZE = 100; const runNumberCriteria = runNumbers.split(SEARCH_ITEMS_SEPARATOR) .map((runNumbers) => runNumbers.trim()) .filter(Boolean); @@ -87,7 +88,9 @@ class GetAllRunsUseCase { if (runNumber.includes('-')) { const [start, end] = runNumber.split('-').map((n) => parseInt(n, 10)); if (!Number.isNaN(start) && !Number.isNaN(end)) { - for (let i = start; i <= end; i++) { + const safeEnd = Math.min(start + MAX_RANGE_SIZE - 1, end); + + for (let i = start; i <= safeEnd; i++) { runNumberSet.add(i); } } diff --git a/test/lib/usecases/run/GetAllRunsUseCase.test.js b/test/lib/usecases/run/GetAllRunsUseCase.test.js index 652691499b..dd42f141b3 100644 --- a/test/lib/usecases/run/GetAllRunsUseCase.test.js +++ b/test/lib/usecases/run/GetAllRunsUseCase.test.js @@ -51,6 +51,22 @@ module.exports = () => { expect(runs[1].runNumber).to.equal(17); }); + it('should return an array, only containing runs with specified run number and ranges', async () => { + getAllRunsDto.query = { filter: { runNumbers: '1-5,8,12,20-30' } }; + const { runs } = await new GetAllRunsUseCase().execute(getAllRunsDto); + + expect(runs).to.be.an('array'); + expect(runs).to.have.lengthOf(18); + }); + + it('should limit range to 100 if range exceeds the maximum', async () => { + getAllRunsDto.query = { filter: { runNumbers: '1-108' } }; + const { runs } = await new GetAllRunsUseCase().execute(getAllRunsDto); + + expect(runs).to.be.an('array'); + expect(runs).to.have.lengthOf(100); + }); + it('should return runs sorted by runNumber', async () => { { const { runs } = await new GetAllRunsUseCase().execute({ query: { sort: { runNumber: 'ASC' } } }); From a0037ae0f97a515938aa6be8dcad7979919055e6 Mon Sep 17 00:00:00 2001 From: Fenne Date: Tue, 17 Dec 2024 16:00:57 +0100 Subject: [PATCH 05/10] Range validation now happens in DTO for correct error handling --- lib/domain/dtos/filters/RunFilterDto.js | 33 ++++++++++++++++++- lib/usecases/run/GetAllRunsUseCase.js | 5 +-- test/api/runs.test.js | 10 ++++++ .../usecases/run/GetAllRunsUseCase.test.js | 8 ----- 4 files changed, 43 insertions(+), 13 deletions(-) diff --git a/lib/domain/dtos/filters/RunFilterDto.js b/lib/domain/dtos/filters/RunFilterDto.js index 88d9e95b65..439bb44ba3 100644 --- a/lib/domain/dtos/filters/RunFilterDto.js +++ b/lib/domain/dtos/filters/RunFilterDto.js @@ -29,8 +29,39 @@ const EorReasonFilterDto = Joi.object({ description: Joi.string(), }); +/** + * Validates run numbers ranges to not exceed 100 runs + * + * @param {*} value The value to validate + * @param {*} helpers The helpers object + * @returns {Object} The value if validation passes + */ +const validateRange = (value, helpers) => { + const MAX_RANGE_SIZE = 100; + + const runNumbers = value.split(',').map((runNumber) => runNumber.trim()); + + for (const runNumber of runNumbers) { + if (runNumber.includes('-')) { + const [start, end] = runNumber.split('-').map((n) => parseInt(n, 10)); + if (Number.isNaN(start) || Number.isNaN(end) || start > end) { + return helpers.error('any.invalid', { message: `Invalid range: ${runNumber}` }); + } + const rangeSize = end - start + 1; + + if (rangeSize > MAX_RANGE_SIZE) { + return helpers.error('any.invalid', { message: `Range exceeds max size of 100 runs: ${runNumber}` }); + } + } + } + + return value; +}; + exports.RunFilterDto = Joi.object({ - runNumbers: Joi.string().trim(), + runNumbers: Joi.string().trim().custom(validateRange).messages({ + 'any.invalid': '{{#message}}', + }), calibrationStatuses: Joi.array().items(...RUN_CALIBRATION_STATUS), definitions: CustomJoi.stringArray().items(Joi.string().uppercase().trim().valid(...RUN_DEFINITIONS)), eorReason: EorReasonFilterDto, diff --git a/lib/usecases/run/GetAllRunsUseCase.js b/lib/usecases/run/GetAllRunsUseCase.js index 982d3a5a12..8453d15d2f 100644 --- a/lib/usecases/run/GetAllRunsUseCase.js +++ b/lib/usecases/run/GetAllRunsUseCase.js @@ -77,7 +77,6 @@ class GetAllRunsUseCase { } = filter; if (runNumbers) { - const MAX_RANGE_SIZE = 100; const runNumberCriteria = runNumbers.split(SEARCH_ITEMS_SEPARATOR) .map((runNumbers) => runNumbers.trim()) .filter(Boolean); @@ -88,9 +87,7 @@ class GetAllRunsUseCase { if (runNumber.includes('-')) { const [start, end] = runNumber.split('-').map((n) => parseInt(n, 10)); if (!Number.isNaN(start) && !Number.isNaN(end)) { - const safeEnd = Math.min(start + MAX_RANGE_SIZE - 1, end); - - for (let i = start; i <= safeEnd; i++) { + for (let i = start; i <= end; i++) { runNumberSet.add(i); } } diff --git a/test/api/runs.test.js b/test/api/runs.test.js index 948ebda5b7..d9c41c417d 100644 --- a/test/api/runs.test.js +++ b/test/api/runs.test.js @@ -157,6 +157,16 @@ module.exports = () => { expect(runs).to.lengthOf(18); }); + it('should return 400 if range exceeds maximum of 100', async () => { + const runNumberRange = '1-108'; + const response = await request(server).get(`/api/runs?filter[runNumbers]=${runNumberRange}`); + + expect(response.status).to.equal(400); + const { errors: [error] } = response.body; + expect(error.title).to.equal('Invalid Attribute'); + expect(error.detail).to.equal(`Range exceeds max size of 100 runs: ${runNumberRange}`); + }); + it('should return 400 if the calibration status filter is invalid', async () => { { const response = await request(server).get('/api/runs?filter[calibrationStatuses]=invalid'); diff --git a/test/lib/usecases/run/GetAllRunsUseCase.test.js b/test/lib/usecases/run/GetAllRunsUseCase.test.js index dd42f141b3..1d1799fecc 100644 --- a/test/lib/usecases/run/GetAllRunsUseCase.test.js +++ b/test/lib/usecases/run/GetAllRunsUseCase.test.js @@ -59,14 +59,6 @@ module.exports = () => { expect(runs).to.have.lengthOf(18); }); - it('should limit range to 100 if range exceeds the maximum', async () => { - getAllRunsDto.query = { filter: { runNumbers: '1-108' } }; - const { runs } = await new GetAllRunsUseCase().execute(getAllRunsDto); - - expect(runs).to.be.an('array'); - expect(runs).to.have.lengthOf(100); - }); - it('should return runs sorted by runNumber', async () => { { const { runs } = await new GetAllRunsUseCase().execute({ query: { sort: { runNumber: 'ASC' } } }); From 4fd194b3e4b7c795aad10d8f757be61783465c3d Mon Sep 17 00:00:00 2001 From: Fenne Date: Tue, 7 Jan 2025 09:33:23 +0100 Subject: [PATCH 06/10] Clearer error message --- lib/domain/dtos/filters/RunFilterDto.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/domain/dtos/filters/RunFilterDto.js b/lib/domain/dtos/filters/RunFilterDto.js index 439bb44ba3..0035b148ca 100644 --- a/lib/domain/dtos/filters/RunFilterDto.js +++ b/lib/domain/dtos/filters/RunFilterDto.js @@ -50,7 +50,7 @@ const validateRange = (value, helpers) => { const rangeSize = end - start + 1; if (rangeSize > MAX_RANGE_SIZE) { - return helpers.error('any.invalid', { message: `Range exceeds max size of 100 runs: ${runNumber}` }); + return helpers.error('any.invalid', { message: `Given range(${rangeSize}), exceeds max size of 100 runs: ${runNumber}` }); } } } From 69697c2c862fcbbbebd07fe2f4e967ada97f25cc Mon Sep 17 00:00:00 2001 From: Fenne Date: Tue, 7 Jan 2025 10:13:40 +0100 Subject: [PATCH 07/10] Changed test to match new error message --- test/api/runs.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/api/runs.test.js b/test/api/runs.test.js index d180301c87..b038d225c1 100644 --- a/test/api/runs.test.js +++ b/test/api/runs.test.js @@ -159,12 +159,13 @@ module.exports = () => { it('should return 400 if range exceeds maximum of 100', async () => { const runNumberRange = '1-108'; + const rangeSize = '108'; const response = await request(server).get(`/api/runs?filter[runNumbers]=${runNumberRange}`); expect(response.status).to.equal(400); const { errors: [error] } = response.body; expect(error.title).to.equal('Invalid Attribute'); - expect(error.detail).to.equal(`Range exceeds max size of 100 runs: ${runNumberRange}`); + expect(error.detail).to.equal(`Given range(${rangeSize}), exceeds max size of 100 runs: ${runNumberRange}`); }); it('should return 400 if the calibration status filter is invalid', async () => { From 6308f07895025b046852338ef0dd7c4ffc0b2504 Mon Sep 17 00:00:00 2001 From: Fenne Date: Mon, 13 Jan 2025 15:27:45 +0100 Subject: [PATCH 08/10] Changed error message to display the right const this time --- lib/domain/dtos/filters/RunFilterDto.js | 2 +- test/api/runs.test.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/domain/dtos/filters/RunFilterDto.js b/lib/domain/dtos/filters/RunFilterDto.js index 0035b148ca..f1dbadc789 100644 --- a/lib/domain/dtos/filters/RunFilterDto.js +++ b/lib/domain/dtos/filters/RunFilterDto.js @@ -50,7 +50,7 @@ const validateRange = (value, helpers) => { const rangeSize = end - start + 1; if (rangeSize > MAX_RANGE_SIZE) { - return helpers.error('any.invalid', { message: `Given range(${rangeSize}), exceeds max size of 100 runs: ${runNumber}` }); + return helpers.error('any.invalid', { message: `Given range exceeds max size of ${MAX_RANGE_SIZE} runs: ${runNumber}` }); } } } diff --git a/test/api/runs.test.js b/test/api/runs.test.js index b038d225c1..4d639bb158 100644 --- a/test/api/runs.test.js +++ b/test/api/runs.test.js @@ -159,13 +159,13 @@ module.exports = () => { it('should return 400 if range exceeds maximum of 100', async () => { const runNumberRange = '1-108'; - const rangeSize = '108'; + const MAX_RANGE_SIZE = 100; const response = await request(server).get(`/api/runs?filter[runNumbers]=${runNumberRange}`); expect(response.status).to.equal(400); const { errors: [error] } = response.body; expect(error.title).to.equal('Invalid Attribute'); - expect(error.detail).to.equal(`Given range(${rangeSize}), exceeds max size of 100 runs: ${runNumberRange}`); + expect(error.detail).to.equal(`Given range exceeds max size of ${MAX_RANGE_SIZE} runs: ${runNumberRange}`); }); it('should return 400 if the calibration status filter is invalid', async () => { From 0c5bfa912863be19001636ae95614994b4c3a5cd Mon Sep 17 00:00:00 2001 From: martinboulais <31805063+martinboulais@users.noreply.github.com> Date: Wed, 22 Jan 2025 14:07:33 +0100 Subject: [PATCH 09/10] Fix trick to filter on a single run number --- lib/usecases/run/GetAllRunsUseCase.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/usecases/run/GetAllRunsUseCase.js b/lib/usecases/run/GetAllRunsUseCase.js index 8453d15d2f..a8e56838a4 100644 --- a/lib/usecases/run/GetAllRunsUseCase.js +++ b/lib/usecases/run/GetAllRunsUseCase.js @@ -100,8 +100,11 @@ class GetAllRunsUseCase { }); const finalRunNumberList = Array.from(runNumberSet); - if (finalRunNumberList.length) { - if (finalRunNumberList.length > 1) { + + // Check that the final run numbers list contains at least one valid run number + if (finalRunNumberList.length > 0) { + // Check if user provided more than 1 run number initially, it might be twice the same to disable the `LIKE` filtering + if (finalRunNumberList.length > 1 || runNumberCriteria.length > 1) { filteringQueryBuilder.where('runNumber').oneOf(...finalRunNumberList); } else { const [runNumber] = finalRunNumberList; @@ -177,7 +180,7 @@ class GetAllRunsUseCase { * @param {function} literal function to create an object representing a database literal expression * @return {Object} the object representing the column */ - const computedColumn = ({ literal }) => literal("ROUND(alice_l3_current * IF(`alice_l3_polarity` = 'NEGATIVE', -1, 1) / 1000)"); + const computedColumn = ({ literal }) => literal('ROUND(alice_l3_current * IF(`alice_l3_polarity` = \'NEGATIVE\', -1, 1) / 1000)'); filteringQueryBuilder.where(computedColumn).is(aliceL3Current); } if (aliceDipoleCurrent !== undefined) { @@ -187,7 +190,7 @@ class GetAllRunsUseCase { * @return {Object} the object representing the column */ const computedColumn = ({ literal }) => - literal("ROUND(alice_dipole_current * IF(`alice_dipole_polarity` = 'NEGATIVE', -1, 1) / 1000)"); + literal('ROUND(alice_dipole_current * IF(`alice_dipole_polarity` = \'NEGATIVE\', -1, 1) / 1000)'); filteringQueryBuilder.where(computedColumn).is(aliceDipoleCurrent); } From 2500ad5717a54c6850baf77cbe01f903179dc8a6 Mon Sep 17 00:00:00 2001 From: martinboulais <31805063+martinboulais@users.noreply.github.com> Date: Wed, 22 Jan 2025 14:24:30 +0100 Subject: [PATCH 10/10] Fix linter --- lib/usecases/run/GetAllRunsUseCase.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/usecases/run/GetAllRunsUseCase.js b/lib/usecases/run/GetAllRunsUseCase.js index a8e56838a4..d0d4877404 100644 --- a/lib/usecases/run/GetAllRunsUseCase.js +++ b/lib/usecases/run/GetAllRunsUseCase.js @@ -180,7 +180,8 @@ class GetAllRunsUseCase { * @param {function} literal function to create an object representing a database literal expression * @return {Object} the object representing the column */ - const computedColumn = ({ literal }) => literal('ROUND(alice_l3_current * IF(`alice_l3_polarity` = \'NEGATIVE\', -1, 1) / 1000)'); + const computedColumn = ({ literal }) => + literal('ROUND(alice_l3_current * IF(`alice_l3_polarity` = \'NEGATIVE\', -1, 1) / 1000)'); filteringQueryBuilder.where(computedColumn).is(aliceL3Current); } if (aliceDipoleCurrent !== undefined) {