diff --git a/README.md b/README.md index 6f37d6a..9254bb8 100644 --- a/README.md +++ b/README.md @@ -178,7 +178,10 @@ validates against this subset, rejecting: - **Out-of-subset names** — identifiers must be `[a-z][a-z0-9_]*` under 17 chars, codes under 6, groups under 21 - **Deep nesting** — max 3 levels (`group/group/question`) -- **Missing `list_name`** — selects require explicit lists +- **Unresolvable answer options** — a `select_one`/`select_multiple` needs a + list name with rows on the choices sheet; a `select_*_from_file` needs a + registered vocabulary (e.g. `iso_3166_1.csv`) or a CSV passed as + `fileChoices` (the CLI reads CSVs beside the form) - **Reserved words** — `relevance`, `validation`, `text`, etc. (LimeSurvey internals) LimeSurvey's reverse-subset check (`lstsv2xlsform`) is narrower — no arrays, diff --git a/src/cli.ts b/src/cli.ts index 2e80b01..49e7ee2 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -170,9 +170,13 @@ function cmdValidate(argv: string[]): void { const bytes = readInput(positionals, validateHelp); // Parse without the built-in strict gate so we can report all findings. const data = loadXlsform(bytes, true); + // CSVs beside the workbook count, as they do for xlsform2lstsv. const violations = XLSValidator.validateSubset( data.surveyData, data.choicesData, + { + fileChoices: resolveFileChoices(data.surveyData, dirname(positionals[0])), + }, ); if (violations.length === 0) { diff --git a/src/index.ts b/src/index.ts index 4f2ba07..c5fd4ee 100644 --- a/src/index.ts +++ b/src/index.ts @@ -2,7 +2,7 @@ export { XLSLoader } from './xlsform/loader.js'; export { XLSFormParser } from './xlsform/parser.js'; export { XLSValidator } from './xlsform/validate.js'; -export type { SubsetViolation } from './xlsform/validate.js'; +export type { SubsetViolation, SubsetOptions } from './xlsform/validate.js'; export { FieldSanitizer } from './xlsform/sanitize.js'; export { parseLstsv } from './lstsv/parser.js'; diff --git a/src/pipelines/xlsform2lstsv/index.ts b/src/pipelines/xlsform2lstsv/index.ts index 6272372..2ba6dd0 100644 --- a/src/pipelines/xlsform2lstsv/index.ts +++ b/src/pipelines/xlsform2lstsv/index.ts @@ -24,7 +24,7 @@ import { AnswerEmitter, AnswerHelpers } from './answerEmitter.js'; import { TranspilerHelper } from './transpilerHelper.js'; import { FieldNameHandler } from './fieldNameHandler.js'; import { AppearanceHandler } from './appearanceHandler.js'; -import { registeredFileChoices } from '../../vocab.js'; +import { registeredFileChoices, registeredVocabFiles } from '../../vocab.js'; import { parameterAttributes } from './parameters.js'; // Registry appearances are an allowlist: only 'handled' entries are @@ -267,7 +267,7 @@ export class XLSFormToTSVConverter { // Validate the type is registered and emittable. Two failure modes: // 1. registered but unsupported by LimeSurvey TSV (no native slot) // 2. not registered at all (convention:unregisteredRows) - this.validateRowType(xfType, baseType); + this.validateRowType(xfType, baseType, row.name); if (xfType === 'begin_group' || xfType === 'begin group') { await this.handleBeginGroup(row); @@ -288,22 +288,24 @@ export class XLSFormToTSVConverter { } /** - * Throws if the row's type is not emittable. Two checks: registered-but- - * unsupported (with an exception for select_*_from_file when the - * referenced CSV is supplied), and not-registered-at-all. + * Throws if the row's type is not emittable: registered but unsupported, + * not registered at all, or a select whose options don't resolve. */ - private validateRowType(xfType: string, baseType: string): void { - if (UNIMPLEMENTED_TYPES.includes(baseType)) { - const filename = xfType.split(/\s+/)[1]; - const canInline = - baseType in FROM_FILE_BASE && - !!filename && - (this.fileChoices[filename]?.length ?? 0) > 0; - if (!canInline) { - throw new Error( - `Unimplemented XLSForm type: '${baseType}'. This type is not currently supported.`, - ); - } + private validateRowType( + xfType: string, + baseType: string, + name: string | undefined, + ): void { + const where = name ? ` (question "${name}")` : ''; + const target = xfType.split(/\s+/)[1]; + if (baseType in FROM_FILE_BASE) { + this.assertFileChoices(xfType, baseType, target, where); + } else if (UNIMPLEMENTED_TYPES.includes(baseType)) { + throw new Error( + `Unimplemented XLSForm type: '${baseType}'. This type is not currently supported.`, + ); + } else if (TYPE_MAPPINGS[baseType]?.requiresListName) { + this.assertChoiceList(xfType, baseType, target, where); } if ( @@ -318,6 +320,46 @@ export class XLSFormToTSVConverter { } } + /** `select_*_from_file` is supported whenever its options resolve; say which part is missing. */ + private assertFileChoices( + xfType: string, + baseType: string, + file: string | undefined, + where: string, + ): void { + if (!file) { + throw new Error( + `'${baseType}'${where} needs a vocabulary file: '${baseType} .csv'`, + ); + } + if ((this.fileChoices[file]?.length ?? 0) === 0) { + throw new Error( + `'${xfType}'${where}: '${file}' is not a registered vocabulary ` + + `(registered: ${registeredVocabFiles().join(', ')}) and no ` + + `fileChoices were supplied for it`, + ); + } + } + + /** A select without options would import as a question nobody can answer. */ + private assertChoiceList( + xfType: string, + baseType: string, + list: string | undefined, + where: string, + ): void { + if (!list || list === 'or_other') { + throw new Error( + `'${baseType}'${where} needs a choice list: '${baseType} '`, + ); + } + if ((this.choiceManager.getChoices(list)?.length ?? 0) === 0) { + throw new Error( + `'${xfType}'${where}: list '${list}' has no rows on the choices sheet`, + ); + } + } + private async handleBeginGroup(row: SurveyRow): Promise { this.matrixHandler.flushMatrix(this.matrixHelpers()); const originalName = (row.name || '').trim(); diff --git a/src/vocab.ts b/src/vocab.ts index 6287e9b..d0e253e 100644 --- a/src/vocab.ts +++ b/src/vocab.ts @@ -52,6 +52,11 @@ export function parseVocabCsv(csvText: string, listName: string): ChoiceRow[] { return rows; } +/** Filenames of the registered vocabularies (`registry/vocab/`). */ +export function registeredVocabFiles(): string[] { + return Object.keys(VOCABULARY_OPTIONS); +} + /** * Choices for every registered vocabulary the survey references, keyed by * filename. Unregistered filenames are left out; the caller supplies those. diff --git a/src/xlsform/validate.ts b/src/xlsform/validate.ts index 37f3fce..d104fe5 100644 --- a/src/xlsform/validate.ts +++ b/src/xlsform/validate.ts @@ -3,6 +3,7 @@ import { APPEARANCES } from '../generated/Appearances.js'; import { TYPE_MAPPINGS } from '../generated/TypeMappings.js'; import { SurveyRow, ChoiceRow } from '../config/types.js'; +import { registeredVocabFiles } from '../vocab.js'; const NAME_RULES = conventions.conventions.sanitization.name; const CHOICE_RULES = conventions.conventions.sanitization.choiceCode; @@ -31,6 +32,16 @@ export interface SubsetViolation { message: string; } +/** Options for {@link XLSValidator.validateSubset}. */ +export interface SubsetOptions { + /** + * Choices for `select_*_from_file` CSVs the caller will pass to `convert()`, + * keyed by filename. These count as resolvable next to the registered + * vocabularies. + */ + fileChoices?: Record; +} + /** Inputs for {@link XLSValidator.validateAll}. */ export interface ValidateAllOpts { surveyData: SurveyRow[]; @@ -343,6 +354,7 @@ export class XLSValidator { static validateSubset( surveyData: SurveyRow[], choicesData: ChoiceRow[], + options: SubsetOptions = {}, ): SubsetViolation[] { const violations: SubsetViolation[] = []; @@ -350,6 +362,10 @@ export class XLSValidator { violations.push({ severity: 'error', message: msg }); } + const listNames = new Set( + choicesData.map((c) => String(c.list_name ?? '').trim()), + ); + for (const row of surveyData) { const rawType = (row.type || '').trim(); if (!rawType) continue; @@ -358,24 +374,18 @@ export class XLSValidator { const mapping = TYPE_MAPPINGS[baseType]; const where = row.name ? ` (question "${row.name}")` : ''; - if (!mapping) { - violations.push({ - severity: 'error', - message: `type "${baseType}"${where} is not in the registry — not part of the supported XLSForm subset`, - }); - } else if ( - mapping.supported === false && - mapping.limeSurveyType === null - ) { - // select_*_from_file is registered-but-not-natively-expressible; it is - // still supported (inlined from the CSV), so only flag other such types. - if (!baseType.endsWith('_from_file')) { - violations.push({ - severity: 'error', - message: `type "${baseType}"${where} is registered but not expressible in LimeSurvey TSV`, - }); - } - } + const problem = + this.typeProblem(baseType, where) ?? + (mapping?.requiresListName + ? this.choiceListProblem( + rawType, + baseType, + where, + listNames, + options.fileChoices ?? {}, + ) + : null); + if (problem) violations.push({ severity: 'error', message: problem }); this.collectAppearanceViolations(row, baseType, violations); } @@ -383,6 +393,56 @@ export class XLSValidator { return violations; } + /** Why a type is outside the subset, or `null` if it's in it. */ + private static typeProblem(baseType: string, where: string): string | null { + const mapping = TYPE_MAPPINGS[baseType]; + if (!mapping) { + return `type "${baseType}"${where} is not in the registry — not part of the supported XLSForm subset`; + } + // select_*_from_file is registered-but-not-natively-expressible; it is + // still supported (inlined from the CSV), so only flag other such types. + if ( + mapping.supported === false && + mapping.limeSurveyType === null && + !baseType.endsWith('_from_file') + ) { + return `type "${baseType}"${where} is registered but not expressible in LimeSurvey TSV`; + } + return null; + } + + /** + * Why a select's answer options can't be resolved, or `null` if they can. + * `select_one`/`select_multiple` need a list name with rows on the choices + * sheet; `select_*_from_file` needs a registered vocabulary or a file in + * `fileChoices`. Without options the converter emits a question with no + * answers, or fails. + */ + private static choiceListProblem( + rawType: string, + baseType: string, + where: string, + listNames: Set, + fileChoices: Record, + ): string | null { + const target = rawType.split(/\s+/)[1]; + if (baseType.endsWith('_from_file')) { + if (!target) { + return `"${baseType}"${where} needs a vocabulary file: "${baseType} .csv"`; + } + if (registeredVocabFiles().includes(target)) return null; + if ((fileChoices[target]?.length ?? 0) > 0) return null; + return `"${rawType}"${where}: "${target}" is not a registered vocabulary (registered: ${registeredVocabFiles().join(', ')})`; + } + if (!target || target === 'or_other') { + return `"${baseType}"${where} needs a choice list: "${baseType} "`; + } + if (!listNames.has(target)) { + return `"${rawType}"${where}: list "${target}" has no rows on the choices sheet`; + } + return null; + } + /** Flag appearances outside the registry allowlist or wrong for the type. */ private static collectAppearanceViolations( row: SurveyRow, diff --git a/tests/fixtures/surveys/validation_relevance_survey/ddi.xml b/tests/fixtures/surveys/validation_relevance_survey/ddi.xml index c4686bb..6fa6803 100644 --- a/tests/fixtures/surveys/validation_relevance_survey/ddi.xml +++ b/tests/fixtures/surveys/validation_relevance_survey/ddi.xml @@ -47,6 +47,14 @@ Do you consent? + + yes + Yes + + + no + No + Do you consent? diff --git a/tests/fixtures/surveys/validation_relevance_survey/tsv.tsv b/tests/fixtures/surveys/validation_relevance_survey/tsv.tsv index cd2abcf..b03990a 100644 --- a/tests/fixtures/surveys/validation_relevance_survey/tsv.tsv +++ b/tests/fixtures/surveys/validation_relevance_survey/tsv.tsv @@ -9,6 +9,8 @@ Q S username 1 Username en self != '' 1 Q N age 1 Age en self >= 18 and self <= 120 1 Q N price 1 Price en self > 0 and self < 1000 1 Q L consent 1 Do you consent? en 1 +A yes Yes en +A no No en Q S adultinfo age >= 18 Adult Information en self != '' 1 Q S seniordiscount age >= 65 Senior Discount Code en 1 Q S consentrequiredinfo consent == 'yes' Additional Information en 1 diff --git a/tests/fixtures/surveys/validation_relevance_survey/xlsform.json b/tests/fixtures/surveys/validation_relevance_survey/xlsform.json index 683339b..4288bac 100644 --- a/tests/fixtures/surveys/validation_relevance_survey/xlsform.json +++ b/tests/fixtures/surveys/validation_relevance_survey/xlsform.json @@ -27,10 +27,9 @@ "constraint_message": "Price must be between 0 and 1000" }, { - "type": "select_one", + "type": "select_one yes_no", "name": "consent", - "label": "Do you consent?", - "list_name": "yes_no" + "label": "Do you consent?" }, { "type": "text", @@ -96,4 +95,4 @@ "default_language": "en" } ] -} \ No newline at end of file +} diff --git a/tests/ts/unit/questionTypes/select_one.test.ts b/tests/ts/unit/questionTypes/select_one.test.ts index cf34f17..dc01890 100644 --- a/tests/ts/unit/questionTypes/select_one.test.ts +++ b/tests/ts/unit/questionTypes/select_one.test.ts @@ -81,17 +81,15 @@ describe('Select One Question Type', () => { expect(question?.mandatory).toBe('Y'); }); - test('handles missing choice list gracefully', async () => { + test('rejects a choice list with no rows (#41)', async () => { const survey = [ { type: 'select_one missing_list', name: 'q1', label: 'Question' }, ]; - // Don't provide the choice list - const rows = await convertAndParse(survey, []); - const question = findRowByName(rows, 'q1'); - - expect(question).toBeDefined(); - expect(question?.['type/scale']).toBe('L'); + // Without choices LimeSurvey would get a question nobody can answer. + await expect(convertAndParse(survey, [])).rejects.toThrow( + /list 'missing_list' has no rows on the choices sheet/, + ); }); test('converts select_one with relevance', async () => { @@ -160,17 +158,15 @@ describe('Select One Question Type', () => { expect(question?.mandatory).toBe('Y'); }); - test('handles missing choice list gracefully', async () => { + test('rejects a choice list with no rows (#41)', async () => { const survey = [ { type: 'select_one missing_list', name: 'q1', label: 'Question' }, ]; - // Don't provide the choice list - const rows = await convertAndParse(survey, []); - const question = findRowByName(rows, 'q1'); - - expect(question).toBeDefined(); - expect(question?.['type/scale']).toBe('L'); + // Without choices LimeSurvey would get a question nobody can answer. + await expect(convertAndParse(survey, [])).rejects.toThrow( + /list 'missing_list' has no rows on the choices sheet/, + ); }); test('converts select_one with relevance', async () => { diff --git a/tests/ts/unit/subsetValidation.test.ts b/tests/ts/unit/subsetValidation.test.ts index 0b3fe21..fd6b36c 100644 --- a/tests/ts/unit/subsetValidation.test.ts +++ b/tests/ts/unit/subsetValidation.test.ts @@ -69,3 +69,68 @@ describe('validateSubset', () => { ).toEqual([]); }); }); + +describe('validateSubset — unresolvable answer options (#41)', () => { + const errors = ( + type: string, + choices: Record[] = [], + fileChoices?: Record< + string, + { list_name: string; name: string; label: string }[] + >, + ) => + XLSValidator.validateSubset([{ type, name: 'q', label: 'Q' }], choices, { + fileChoices, + }) + .filter((v) => v.severity === 'error') + .map((v) => v.message); + + const skala = [{ list_name: 'skala', name: 'a', label: 'A' }]; + + test.each(['select_one', 'select_multiple'])( + '%s without a list name', + (t) => { + expect(errors(t)).toEqual([ + `"${t}" (question "q") needs a choice list: "${t} "`, + ]); + expect(errors(`${t} or_other`)).toHaveLength(1); + }, + ); + + test.each(['select_one', 'select_multiple'])( + '%s with a list that has no rows', + (t) => { + expect(errors(`${t} skala`)).toEqual([ + `"${t} skala" (question "q"): list "skala" has no rows on the choices sheet`, + ]); + expect(errors(`${t} skala`, skala)).toEqual([]); + }, + ); + + test.each(['select_one_from_file', 'select_multiple_from_file'])( + '%s without a file', + (t) => { + expect(errors(t)).toEqual([ + `"${t}" (question "q") needs a vocabulary file: "${t} .csv"`, + ]); + }, + ); + + test.each(['select_one_from_file', 'select_multiple_from_file'])( + '%s with an unregistered file, unless supplied', + (t) => { + expect(errors(`${t} foo.csv`)).toEqual([ + `"${t} foo.csv" (question "q"): "foo.csv" is not a registered vocabulary (registered: iso_3166_1.csv)`, + ]); + expect( + errors(`${t} foo.csv`, [], { + 'foo.csv': [{ list_name: 'foo.csv', name: 'x', label: 'X' }], + }), + ).toEqual([]); + }, + ); + + test('a registered vocabulary resolves', () => { + expect(errors('select_one_from_file iso_3166_1.csv')).toEqual([]); + }); +}); diff --git a/tests/ts/unit/vocab.test.ts b/tests/ts/unit/vocab.test.ts index 9f6f6ea..cba3b01 100644 --- a/tests/ts/unit/vocab.test.ts +++ b/tests/ts/unit/vocab.test.ts @@ -93,6 +93,8 @@ describe('convert with select_*_from_file', () => { [], [], ), - ).rejects.toThrow(/Unimplemented XLSForm type: 'select_one_from_file'/); + ).rejects.toThrow( + /'own.csv' is not a registered vocabulary \(registered: iso_3166_1.csv\)/, + ); }); });