diff --git a/src/seed_tools/utils/seed_validation.ts b/src/seed_tools/utils/seed_validation.ts index 6a3c4732..19b21d7d 100644 --- a/src/seed_tools/utils/seed_validation.ts +++ b/src/seed_tools/utils/seed_validation.ts @@ -19,7 +19,21 @@ export function validateSeed(seed: VariationsSeed) { // Validate a seed for common errors. Returns an array of error messages. export function validateSeedReturnErrors(seed: VariationsSeed): string[] { - return checkOverlappingStudies(seed); + const errors: string[] = []; + const validators = [checkOverlappingStudies]; + for (const validator of validators) { + try { + errors.push(...validator(seed)); + } catch (e) { + if (e instanceof Error) { + errors.push(e.message); + } else { + // Rethrow non-Error exceptions. + throw e; + } + } + } + return errors; } // Validates a seed by checking for overlapping studies that use the same diff --git a/src/seed_tools/utils/study_json_utils.ts b/src/seed_tools/utils/study_json_utils.ts index 8d523d3b..5817b0e9 100644 --- a/src/seed_tools/utils/study_json_utils.ts +++ b/src/seed_tools/utils/study_json_utils.ts @@ -36,7 +36,7 @@ export async function readStudyFileReturnWithError( e.message += ` (${studyFilePath})`; return Result.error(e); } - // Re-throw non-Error exceptions. + // Rethrow non-Error exceptions. throw e; } } diff --git a/src/seed_tools/utils/study_validation.test.ts b/src/seed_tools/utils/study_validation.test.ts index f695ef42..27c021c7 100644 --- a/src/seed_tools/utils/study_validation.test.ts +++ b/src/seed_tools/utils/study_validation.test.ts @@ -178,6 +178,25 @@ describe('validateStudy', () => { }).toThrowError('Invalid version range'); }); + test('should throw an error if version is invalid', () => { + const study = Study.fromJson({ + name: 'study', + experiment: [ + { + name: 'experiment1', + probability_weight: 100, + }, + ], + filter: { + min_version: '2.a', + }, + }); + + expect(() => { + study_validation.validateStudy(study, studyFilePath); + }).toThrowError('contains non-numeric characters'); + }); + test('should throw an error if os version range is invalid', () => { const study = Study.fromJson({ name: 'study', diff --git a/src/seed_tools/utils/study_validation.ts b/src/seed_tools/utils/study_validation.ts index fdd73396..117ed6cc 100644 --- a/src/seed_tools/utils/study_validation.ts +++ b/src/seed_tools/utils/study_validation.ts @@ -20,14 +20,28 @@ export function validateStudyReturnErrors( study: Study, studyFilePath: string, ): string[] { - return Array.prototype.concat( - checkName(study, studyFilePath), - checkExperiments(study), - checkFilterExcludeFields(study), - checkDateRange(study), - checkVersionRange(study), - checkOsVersionRange(study), - ); + const errors: string[] = []; + const validators = [ + checkName, + checkExperiments, + checkFilterExcludeFields, + checkDateRange, + checkVersionRange, + checkOsVersionRange, + ]; + for (const validator of validators) { + try { + errors.push(...validator(study, studyFilePath)); + } catch (e) { + if (e instanceof Error) { + errors.push(e.message); + } else { + // Rethrow non-Error exceptions. + throw e; + } + } + } + return errors; } // Check that study name matches the file name. diff --git a/src/seed_tools/utils/version.test.ts b/src/seed_tools/utils/version.test.ts index 7f4cfd77..f5aca0a5 100644 --- a/src/seed_tools/utils/version.test.ts +++ b/src/seed_tools/utils/version.test.ts @@ -8,82 +8,83 @@ import { Version } from './version'; describe('Version', () => { describe('parse', () => { const testCases = [ - { input: '', parts: 0, firstpart: 0, success: false }, - { input: ' ', parts: 0, firstpart: 0, success: false }, - { input: '\t', parts: 0, firstpart: 0, success: false }, - { input: '\n', parts: 0, firstpart: 0, success: false }, - { input: ' ', parts: 0, firstpart: 0, success: false }, - { input: '.', parts: 0, firstpart: 0, success: false }, - { input: ' . ', parts: 0, firstpart: 0, success: false }, - { input: '0', parts: 1, firstpart: 0, success: true }, - { input: '0.', parts: 0, firstpart: 0, success: false }, - { input: '0.0', parts: 2, firstpart: 0, success: true }, + { input: '', components: [] }, + { input: ' ', components: [] }, + { input: '\t', components: [] }, + { input: '\n', components: [] }, + { input: ' ', components: [] }, + { input: '.', components: [] }, + { input: ' . ', components: [] }, + { input: '0', components: [0] }, + { input: '0.', components: [] }, + { input: '0.0', components: [0, 0] }, { input: '4294967295.0', - parts: 2, - firstpart: 4294967295, - success: true, + components: [4294967295, 0], }, - { input: '4294967296.0', parts: 0, firstpart: 0, success: false }, - { input: '-1.0', parts: 0, firstpart: 0, success: false }, - { input: '1.-1.0', parts: 0, firstpart: 0, success: false }, - { input: '1,--1.0', parts: 0, firstpart: 0, success: false }, - { input: '+1.0', parts: 0, firstpart: 0, success: false }, - { input: '1.+1.0', parts: 0, firstpart: 0, success: false }, - { input: '1+1.0', parts: 0, firstpart: 0, success: false }, - { input: '++1.0', parts: 0, firstpart: 0, success: false }, - { input: '1.0a', parts: 0, firstpart: 0, success: false }, + { input: '4294967296.0', components: [] }, + { input: '-1.0', components: [] }, + { input: '1.-1.0', components: [] }, + { input: '1,--1.0', components: [] }, + { input: '+1.0', components: [] }, + { input: '1.+1.0', components: [] }, + { input: '1+1.0', components: [] }, + { input: '++1.0', components: [] }, + { input: '1.0a', components: [] }, { input: '1.2.3.4.5.6.7.8.9.0', - parts: 10, - firstpart: 1, - success: true, + components: [1, 2, 3, 4, 5, 6, 7, 8, 9, 0], }, - { input: '02.1', parts: 0, firstpart: 0, success: false }, - { input: '0.01', parts: 2, firstpart: 0, success: true }, - { input: 'f.1', parts: 0, firstpart: 0, success: false }, - { input: '15.007.20011', parts: 3, firstpart: 15, success: true }, - { input: '15.5.28.130162', parts: 4, firstpart: 15, success: true }, + { input: '02.1', components: [] }, + { input: '0.01', components: [0, 1] }, + { input: 'f.1', components: [] }, + { input: '15.007.20011', components: [15, 7, 20011] }, + { input: '15.5.28.130162', components: [15, 5, 28, 130162] }, ]; testCases.forEach((testCase) => { - it(`should ${testCase.success ? 'parse' : 'fail'} "${testCase.input}"`, () => { - if (!testCase.success) { - expect(() => new Version(testCase.input)).toThrowError(); - } else { + it(`should ${testCase.components.length > 0 ? 'parse' : 'fail'} "${testCase.input}"`, () => { + if (testCase.components.length > 0) { const version = new Version(testCase.input); expect( () => new Version(testCase.input, { disallowWildcard: true }), ).not.toThrow(); - expect(version.components.length).toBe(testCase.parts); - expect(version.components[0]).toBe(testCase.firstpart); + expect(version.components).toStrictEqual(testCase.components); expect(version.isWildcard).toBe(false); + } else { + expect(() => new Version(testCase.input)).toThrowError(); } }); }); describe('wildcard', () => { const testCases = [ - { version: '1.2.3.*', expected: true, isWildcard: true }, - { version: '1.2.3.5*', expected: false, isWildcard: false }, - { version: '1.2.3.56*', expected: false, isWildcard: false }, - { version: '1.*.3', expected: false, isWildcard: false }, - { version: '20.*', expected: true, isWildcard: true }, - { version: '+2.*', expected: false, isWildcard: false }, - { version: '*', expected: false, isWildcard: false }, - { version: '*.2', expected: false, isWildcard: true }, + { version: '1.2.3.*', components: [1, 2, 3] }, + { version: '1.2.3.5*', components: [] }, + { version: '1.2.3.56*', components: [] }, + { version: '1.2.3.56.a.*', components: [] }, + { version: '1.2.3.56.*.a', components: [] }, + { version: '1.*.3', components: [] }, + { version: '20.*', components: [20] }, + { version: '+2.*', components: [] }, + { version: '*', components: [] }, + { version: '*.2', components: [] }, ]; testCases.forEach((testCase) => { - it(`should ${testCase.expected ? 'parse' : 'fail'} "${testCase.version}"`, () => { - if (testCase.expected) { + it(`should ${testCase.components.length > 0 ? 'parse' : 'fail'} "${testCase.version}"`, () => { + if (testCase.components.length > 0) { const version = new Version(testCase.version); - expect(version.isWildcard).toBe(testCase.isWildcard); + expect(version.components).toStrictEqual(testCase.components); + expect(version.isWildcard).toBe(true); expect( () => new Version(testCase.version, { disallowWildcard: true }), ).toThrowError(); } else { expect(() => new Version(testCase.version)).toThrowError(); + expect( + () => new Version(testCase.version, { disallowWildcard: true }), + ).toThrowError(); } }); }); diff --git a/src/seed_tools/utils/version.ts b/src/seed_tools/utils/version.ts index 7ddba4ba..f0694ad5 100644 --- a/src/seed_tools/utils/version.ts +++ b/src/seed_tools/utils/version.ts @@ -3,6 +3,8 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this file, // You can obtain one at https://mozilla.org/MPL/2.0/. +import Result from '../../base/result'; + export interface VersionOptions { disallowLeadingZeros?: boolean; disallowWildcard?: boolean; @@ -46,8 +48,12 @@ export class Version { continue; } - const parsedComponent = parseVersionComponent(strComponent); + const parsedComponentResult = parseVersionComponent(strComponent); + if (!parsedComponentResult.ok) { + throw new Error(`${parsedComponentResult.error}: ${versionStr}`); + } + const parsedComponent = parsedComponentResult.value; // base::Version allows leading zeros in subsequent parts (but not the // first) for legacy reasons. This is not necessary for our use case, so // we can forbid them. @@ -121,18 +127,18 @@ export class Version { // Strictly parse a string as a number that can be represented as uint32. This // is required to match the behavior of base::Version in C++. -function parseVersionComponent(str: string): number { +function parseVersionComponent(str: string): Result { if (!/^\d+$/.test(str)) { - throw new Error( - `Version component contains non-numeric characters: ${str}`, + return Result.error( + `Version component "${str}" contains non-numeric characters`, ); } const num = parseInt(str, 10); if (num < 0 || num > 4294967295) { - throw new Error(`Version component does not fit into uint32: ${str}`); + return Result.error(`Version component "${str}" does not fit into uint32`); } - return num; + return Result.ok(num); }