[O2B-1503] Processed feedback, added tests

Author: Houwie7000

lib/usecases/lhcFill/GetAllLhcFillsUseCase.js

@@ -58,7 +58,8 @@ class GetAllLhcFillsUseCase {
 
                 // Check that the final fill numbers list contains at least one valid fill number
                 if (finalFillnumberList.length > 0) {
-                    queryBuilder.where('fillNumber').oneOf(...finalFillnumberList);
+                    finalFillnumberList.length === 1 ? queryBuilder.where('fillNumber').is(finalFillnumberList[0])
+                        : queryBuilder.where('fillNumber').oneOf(...finalFillnumberList);
                 }
             }
         }

lib/utilities/rangeUtils.js

@@ -26,7 +26,11 @@ export const validateRange = (value, helpers) => {
 
     for (const number of numbers) {
         if (number.includes('-')) {
-            const [start, end] = number.split('-').map((n) => parseInt(n, 10));
+            // Check if '-' occurs more than once in this part of the range
+            if (number.lastIndexOf('-') !== number.indexOf('-')) {
+                return helpers.error('any.invalid', { message: `Invalid range: ${number}` });
+            }
+            const [start, end] = number.split('-').map((n) => Number(n));
             if (Number.isNaN(start) || Number.isNaN(end) || start > end) {
                 return helpers.error('any.invalid', { message: `Invalid range: ${number}` });
             }
@@ -35,6 +39,11 @@ export const validateRange = (value, helpers) => {
             if (rangeSize > MAX_RANGE_SIZE) {
                 return helpers.error('any.invalid', { message: `Given range exceeds max size of ${MAX_RANGE_SIZE} range: ${number}` });
             }
+        } else {
+            // Prevent non-numeric input.
+            if (isNaN(number)) {
+                return helpers.error('any.invalid', { message: `Invalid number: ${number}` });
+            }
         }
     }
 
@@ -44,15 +53,15 @@ export const validateRange = (value, helpers) => {
 /**
  * Unpacks a given string containing number ranges.
  * E.G. input: 5,7-9 => output: 5,7,8,9
- * @param {string} numbersRange numbers that may or may not contain ranges.
+ * @param {string[]} numbersRanges numbers that may or may not contain ranges.
  * @param {string} rangeSplitter string used to indicate and unpack a range.
  * @returns {Set<Number>} set containing the unpacked range.
  */
-export function unpackNumberRange(numbersRange, rangeSplitter = '-') {
+export function unpackNumberRange(numbersRanges, rangeSplitter = '-') {
     // Set to prevent duplicate values.
     const resultNumbers = new Set();
 
-    numbersRange.forEach((number) => {
+    numbersRanges.forEach((number) => {
         if (number.includes(rangeSplitter)) {
             const [start, end] = number.split(rangeSplitter).map((n) => parseInt(n, 10));
             if (!Number.isNaN(start) && !Number.isNaN(end)) {
@@ -61,7 +70,7 @@ export function unpackNumberRange(numbersRange, rangeSplitter = '-') {
                 }
             }
         } else {
-            if (!Number.isNaN(number)) {
+            if (!isNaN(number)) {
                 resultNumbers.add(Number(number));
             }
         }

test/lib/utilities/index.js

@@ -14,11 +14,13 @@
 const cacheAsyncFunctionTest = require('./cacheAsyncFunction.test.js');
 const deepmerge = require('./deepmerge.test.js');
 const isPromise = require('./isPromise.test.js');
+const rangeUtilsTest = require('./rangeUtils.test.js');
 const stringUtilsTest = require('./stringUtils.test.js');
 
 module.exports = () => {
     describe('cacheFunction', cacheAsyncFunctionTest);
     describe('deepmerge', deepmerge);
     describe('isPromise', isPromise);
     describe('stringUtils', stringUtilsTest);
+    describe('rangeUtils', rangeUtilsTest)
 };

test/lib/utilities/rangeUtils.test.js

@@ -0,0 +1,159 @@
+/**
+ * @license
+ * Copyright CERN and copyright holders of ALICE O2. This software is
+ * distributed under the terms of the GNU General Public License v3 (GPL
+ * Version 3), copied verbatim in the file "COPYING".
+ *
+ * See http://alice-o2.web.cern.ch/license for full licensing information.
+ *
+ * In applying this license CERN does not waive the privileges and immunities
+ * granted to it by virtue of its status as an Intergovernmental Organization
+ * or submit itself to any jurisdiction.
+ */
+
+const Sinon = require('sinon');
+const { validateRange, unpackNumberRange } = require('../../../lib/utilities/rangeUtils.js');
+const { expect } = require('chai');
+
+module.exports = () => {
+    describe('validateRange()', () => {
+        let helpers;
+
+        beforeEach(() => {
+            helpers = {
+                error: Sinon.stub()
+            };
+        });
+
+        it('returns the original value for a single valid number', () => {
+            const input = '5';
+            const result = validateRange(input, helpers);
+            expect(result).to.equal(input);
+        });
+
+        it('returns the original value, accepts 0', () => {
+            const input = '0,1';
+            const result = validateRange(input, helpers);
+            expect(result).to.equal(input);
+        });
+
+        it('returns the original value for multiple valid numbers', () => {
+            const input = '1, 2,3, 10 ';
+            const result = validateRange(input, helpers);
+            expect(result).to.equal(input);
+        });
+
+        it('accepts a valid range', () => {
+            const input = '7-9';
+            const result = validateRange(input, helpers);
+            expect(result).to.equal(input);
+        });
+
+        it('accepts numbers and ranges together', () => {
+            const input = '5,7-9,12';
+            const result = validateRange(input, helpers);
+            expect(result).to.equal(input);
+        });
+
+        it('accepts numbers and ranges overlap', () => {
+            const input = '1-6,2,3,4,5,6';
+            const result = validateRange(input, helpers);
+            expect(result).to.equal(input);
+        });
+
+        it('rejects non-numeric input', () => {
+            const input = '5,a,7';
+            validateRange(input, helpers);
+            expect(helpers.error.calledOnce).to.be.true;
+            expect(helpers.error.firstCall.args[0]).to.equal('any.invalid');
+            expect(helpers.error.firstCall.args[1]).to.deep.equal({ message: 'Invalid number: a' });
+        });
+
+        it('rejects range with non-numeric input', () => {
+            const input = '3-a';
+            validateRange(input, helpers);
+            expect(helpers.error.calledOnce).to.be.true;
+            expect(helpers.error.firstCall.args[1]).to.deep.equal({ message: 'Invalid range: 3-a' });
+        });
+
+        it('rejects range where Start > End', () => {
+            const input = '6-5';
+            validateRange(input, helpers);
+            expect(helpers.error.calledOnce).to.be.true;
+            expect(helpers.error.firstCall.args[1]).to.deep.equal({ message: 'Invalid range: 6-5' });
+        });
+
+        // Allowed, technically a valid range
+        it('accepts range where Start === End', () => {
+            const input = '5-5';
+            const result = validateRange(input, helpers);
+            expect(result).to.equal(input);
+        });
+
+        it('rejects range containing more than one `-`', () => {
+            const input = '1-2-3';
+            validateRange(input, helpers);
+            expect(helpers.error.calledOnce).to.be.true;
+            expect(helpers.error.firstCall.args[1]).to.deep.equal({ message: 'Invalid range: 1-2-3' });
+        });
+
+        it('rejects range containing more than one `-`, at end', () => {
+            const input = '1-2-';
+            validateRange(input, helpers);
+            expect(helpers.error.calledOnce).to.be.true;
+            expect(helpers.error.firstCall.args[1]).to.deep.equal({ message: 'Invalid range: 1-2-' });
+        });
+
+        // MAX_RANGE_SIZE = 100, should this change, also change this test...
+        it('rejects a range that exceeds MAX_RANGE_SIZE', () => {
+            const input = '1-101';
+            validateRange(input, helpers);
+            expect(helpers.error.calledOnce).to.be.true;
+            expect(helpers.error.firstCall.args[1]).to.deep.equal({ message: 'Given range exceeds max size of 100 range: 1-101' });
+        });
+
+        it('handles whitespace around inputs', () => {
+            const input = '  2 , 4-6 ,  9 ';
+            const result = validateRange(input, helpers);
+            expect(result).to.equal(input);
+        });
+    });
+
+    describe('unpackNumberRange()', () => {
+        it('unpacks single numbers, duplicate', () => {
+            const input = ['5', '10', '5'];
+            const result = unpackNumberRange(input);
+            expect(Array.from(result)).to.deep.equal([5, 10]);
+        });
+
+        it('unpacks range', () => {
+            const input = ['7-9'];
+            const result = unpackNumberRange(input);
+            expect(Array.from(result)).to.deep.equal([7, 8, 9]);
+        });
+
+        it('unpacks mixed numbers and ranges', () => {
+            const input = ['5', '7-9', '9', '3-4'];
+            const result = unpackNumberRange(input);
+            expect(Array.from(result)).to.deep.equal([5, 7, 8, 9, 3, 4]);
+        });
+
+        it('ignores any non-numeric inputs', () => {
+            const input = ['5', 'x', '2-3', 'a-b', '4-a'];
+            const result = unpackNumberRange(input);
+            expect(Array.from(result)).to.deep.equal([5, 2, 3]);
+        });
+
+        it('accepts/uses a range splitter', () => {
+            const input = ['8..10', '12'];
+            const result = unpackNumberRange(input, '..');
+            expect(Array.from(result)).to.deep.equal([8, 9, 10, 12]);
+        });
+        
+        // Also allowed right now...
+        it('returns empty set if nothing is given', () => {
+            const result = unpackNumberRange([]);
+            expect(result.size).to.equal(0);
+        });
+    });
+};