From 31bc3014de0c9765bc91867e9e64f45aeb492087 Mon Sep 17 00:00:00 2001 From: Anro Date: Wed, 15 Apr 2026 17:24:43 +0200 Subject: [PATCH] feat: var checks --- src/lib/convert-forms/handle-var-checks.js | 202 +++++++++++++ src/lib/convert-forms/index.js | 38 ++- src/lib/forms-utils.js | 3 + test/fn/convert-forms.utils.js | 2 + .../convert-forms/handle-var-checks.spec.js | 284 ++++++++++++++++++ test/lib/convert-forms/index.spec.js | 66 +++- 6 files changed, 580 insertions(+), 15 deletions(-) create mode 100644 src/lib/convert-forms/handle-var-checks.js create mode 100644 test/lib/convert-forms/handle-var-checks.spec.js diff --git a/src/lib/convert-forms/handle-var-checks.js b/src/lib/convert-forms/handle-var-checks.js new file mode 100644 index 00000000..d1504103 --- /dev/null +++ b/src/lib/convert-forms/handle-var-checks.js @@ -0,0 +1,202 @@ +const { getNodes, XPATH_MODEL, XML_ATT_NODESET } = require('../forms-utils'); +const { info, warn} = require('../log'); + +const DEFAULT = { + warn_length: 100, + error_length: 138 +}; + +function processLengthInput(n) { + if(typeof n !== 'number' || !Number.isFinite(n) || !Number.isInteger(n) || n < 0 ){ + throw new Error('Please ensure that the warn/error length value is a positive integer'); + } + + return n; +} + +function processListInput(e) { + const set = new Set([]); + const invalidPaths = []; + + if(!Array.isArray(e)){ + return [new Set(), []]; + } + + for(const entry of e){ + if(/[`'"]/.test(entry)){ + invalidPaths.push(entry); + } + else { + set.add(entry); + } + } + + return [set, invalidPaths]; +} + +function formatFeedbackMsg(title, items, footer){ + return `${title}\n${items.join('\n')}\n${footer}`; +} + +function checkLengthEntries(warnLength, errorLength){ + if(errorLength && warnLength >= errorLength){ + throw new Error('The error length needs to be larger than the warn length.'); + } +} + +function checkInvalidListEntries(entries, label){ + if(entries.length > 0){ + throw new Error(formatFeedbackMsg( + `The following ${label} entries are invalid:`, + entries, + 'Please fix or remove where appropriate.' + )); + } +} + +function checkListOverlap(ignoreSet, reservedSet){ + if(!ignoreSet.size || !reservedSet.size){ + return; + } + + const overlap = []; + for(const ignore of ignoreSet){ + if(reservedSet.has(ignore)){ + overlap.push(ignore); + } + } + + if(overlap.length > 0){ + throw new Error(formatFeedbackMsg( + 'Overlap between reserved and ignore lists:', + overlap, + 'Please remove where appropriate.' + )); + } +} + +function processPropData(props){ + const warnLength = 'warn_length' in props ? processLengthInput(props.warn_length) : null; + const errorLength = 'error_length' in props ? processLengthInput(props.error_length) : null; + const [ignoreSet, invalidIgnoreEntries] = processListInput(props.ignore_list); + const [reservedSet, invalidReservedEntries] = processListInput(props.reserved_list); + + if(!warnLength && !errorLength && reservedSet.size === 0){ + info('Warn and error lengths and reserved list not provided. Skipping var checks.'); + return; + } + + checkLengthEntries(warnLength, errorLength); + checkInvalidListEntries(invalidIgnoreEntries, 'ignored'); + checkInvalidListEntries(invalidReservedEntries, 'reserved'); + checkListOverlap(ignoreSet, reservedSet); + + return { warnLength, errorLength, ignoreSet, reservedSet }; +} + +function buildExclusionPath(set){ + if(!set.size){ + return ''; + } + const conditions = Array.from(set).map(v => `@${XML_ATT_NODESET} = "${v}"`).join(' or '); + return `and not(${conditions})`; +} + +function getBindNodes(xmlDoc, ignoreSet){ + try { + return getNodes( + xmlDoc, + `${XPATH_MODEL}/bind[starts-with(@${XML_ATT_NODESET}, "/data/") ${buildExclusionPath(ignoreSet)}]` + ); + } + catch (e){ + const key = 'Unterminated string literal: "'; + if(e.message?.includes(key)){ + const problemPath = e.message.substring(e.message.indexOf(key) + key.length, e.message.indexOf(')')); + throw new Error(`Unable to find path: ${problemPath}`); + } + throw e; + } +} + +function processBindNodes(bindNodes, warnLength, errorLength, reservedSet){ + const reserved = []; + const errorNodes = []; + const warnNodes = []; + + function classifyNode(nodeset) { + const length = nodeset.length; + if (reservedSet.has(nodeset)){ + return 'reserved'; + } + if (errorLength > 0 && length >= errorLength) { + return 'error'; + } + if (warnLength > 0 && length >= warnLength) { + return 'warn'; + } + return null; + } + + for (const bind of bindNodes) { + const nodeset = bind.getAttribute(XML_ATT_NODESET); + switch (classifyNode(nodeset)) { + case 'reserved': + reserved.push(nodeset); + break; + case 'error': + errorNodes.push(nodeset); + break; + case 'warn': + warnNodes.push(nodeset); + break; + } + } + + return { reserved, errorNodes, warnNodes }; +} + +function handleFormVarResults(reserved, warnObj, errorObj){ + if(reserved.length > 0){ + throw new Error(formatFeedbackMsg( + 'The following reserved entries were found in the form:', + reserved, + 'Please remove or rename as appropriate.' + )); + } + if(errorObj.errorNodes.length > 0){ + throw new Error(formatFeedbackMsg( + `The following vars are longer than the acceptable var length (${errorObj.errorLength}):`, + errorObj.errorNodes, + 'Please simplify nesting or remove verbosity.' + )); + } + else if(warnObj.warnNodes.length > 0){ + warn(formatFeedbackMsg( + `The following vars are longer than the acceptable var length (${warnObj.warnLength}):`, + warnObj.warnNodes, + 'Please consider simplifying nesting or removing verbosity.' + )); + } +} + +function checkVars(xmlDoc, props) { + const varConfig = processPropData(props ?? DEFAULT); + if(!varConfig){ + return; + } + const { warnLength, errorLength, ignoreSet, reservedSet } = varConfig; + + const bindNodes = getBindNodes(xmlDoc, ignoreSet); + if(!bindNodes || bindNodes.length === 0){ + info('Form did not contain any bind nodes'); + return; + } + + const { reserved, errorNodes, warnNodes } = processBindNodes(bindNodes, warnLength, errorLength, reservedSet); + handleFormVarResults(reserved, { warnNodes, warnLength }, { errorNodes, errorLength } ); +} + +module.exports = { + checkVars +}; diff --git a/src/lib/convert-forms/index.js b/src/lib/convert-forms/index.js index f9d9b87e..b7a52c17 100644 --- a/src/lib/convert-forms/index.js +++ b/src/lib/convert-forms/index.js @@ -2,10 +2,7 @@ const argsFormFilter = require('../args-form-filter'); const exec = require('../exec-promise'); const fs = require('../sync-fs'); const nodeFs = require('node:fs'); -const { - getFormDir, - escapeWhitespacesInPath, -} = require('../forms-utils'); +const { getFormDir, escapeWhitespacesInPath, } = require('../forms-utils'); const { info, trace, warn, LEVEL_NONE } = require('../log'); const path = require('node:path'); const { DOMParser, XMLSerializer } = require('@xmldom/xmldom'); @@ -15,6 +12,7 @@ const { removeNoLabelNodes } = require('./handle-no-label-placeholders'); const { removeExtraRepeatInstance, addRepeatCount } = require('./handle-repeat'); const { handleDbDocRefs } = require('./handle-db-doc-ref'); const { handleFormId } = require('./handle-form-id'); +const { checkVars } = require('./handle-var-checks'); const domParser = new DOMParser(); const serializer = new XMLSerializer(); @@ -52,8 +50,8 @@ const execute = async (projectDir, subDirectory, options = {}) => { try { await xls2xform(escapeWhitespacesInPath(sourcePath), escapeWhitespacesInPath(xmlSwpPath), xls); - const hiddenFields = await getHiddenFields(`${fs.withoutExtension(sourcePath)}.properties.json`); - fixXml(xmlSwpPath, hiddenFields, options.transformer, options.enketo); + const propsData = getPropsData(`${fs.withoutExtension(sourcePath)}.properties.json`); + fixXml(xmlSwpPath, propsData, options.transformer, options.enketo); } catch (e) { nodeFs.rmSync(xmlSwpPath, { force: true }); throw e; @@ -123,7 +121,7 @@ const xls2xform = async (sourcePath, targetPath, xlsxFileName) => { // here we fix the form content in arcane ways. Seeing as we have out own fork // of pyxform, we should probably be doing this fixing there. -const fixXml = (path, hiddenFields, transformer, enketo) => { +const fixXml = (path, propsData, transformer, enketo) => { // This is not how you should modify XML, but we have reasonable control over // the input and so far this works OK. Keep an eye on the tests, and any // future changes to the output of xls2xform. @@ -140,8 +138,8 @@ const fixXml = (path, hiddenFields, transformer, enketo) => { xml = xml.replaceAll('default="true()"', ''); } - if (hiddenFields) { - const r = new RegExp(`<(${hiddenFields.join('|')})(/?)>`, 'g'); + if (propsData[FORM_PROPERTIES_HIDDEN_FIELDS]) { + const r = new RegExp(`<(${propsData[FORM_PROPERTIES_HIDDEN_FIELDS].join('|')})(/?)>`, 'g'); xml = xml.replace(r, '<$1 tag="hidden"$2>'); } @@ -170,6 +168,10 @@ const fixXml = (path, hiddenFields, transformer, enketo) => { lineSeparator: '\n' }).replaceAll(/\s+<\/value>/g, ''); // Ignoring the 'value' path results in extra trailing whitespace + if(propsData[FORM_PROPERTIES_VAR_RESTRICTIONS]){ + checkVars(xmlDoc, propsData[FORM_PROPERTIES_VAR_RESTRICTIONS]); + } + if (transformer) { xml = transformer(xml, path); } @@ -177,12 +179,20 @@ const fixXml = (path, hiddenFields, transformer, enketo) => { fs.write(path, xml); }; -function getHiddenFields(propsJson) { - if (fs.exists(propsJson)) { - return fs.readJson(propsJson).hidden_fields; +const FORM_PROPERTIES_HIDDEN_FIELDS = 'hidden_fields'; +const FORM_PROPERTIES_VAR_RESTRICTIONS = 'var_restrictions'; +function getPropsData(propsJson) { + if(fs.exists(propsJson)){ + const json = fs.readJson(propsJson); + return { + [FORM_PROPERTIES_HIDDEN_FIELDS]: json[FORM_PROPERTIES_HIDDEN_FIELDS], + [FORM_PROPERTIES_VAR_RESTRICTIONS]: json[FORM_PROPERTIES_VAR_RESTRICTIONS] + }; } - - return []; + return { + [FORM_PROPERTIES_HIDDEN_FIELDS]: [], + [FORM_PROPERTIES_VAR_RESTRICTIONS]: {} + }; } const META_XML_SECTION = ` diff --git a/src/lib/forms-utils.js b/src/lib/forms-utils.js index c79af9fa..1bc2e46e 100644 --- a/src/lib/forms-utils.js +++ b/src/lib/forms-utils.js @@ -3,6 +3,7 @@ const fs = require('./sync-fs'); const XPATH_MODEL = '/h:html/h:head/model'; const XPATH_BODY = '/h:html/h:body'; +const XML_ATT_NODESET = 'nodeset'; const getNode = (currentNode, path) => xpath.parse(path).select1({ node: currentNode, allowAnyNamespaceForNoPrefix: true }); @@ -22,6 +23,8 @@ module.exports = { XPATH_MODEL, XPATH_BODY, + XML_ATT_NODESET, + /** * Matches XPath expressions that are only paths to a node (either absolute or relative) without any * predicates, functions, operators, etc. diff --git a/test/fn/convert-forms.utils.js b/test/fn/convert-forms.utils.js index da17b042..1def4d95 100644 --- a/test/fn/convert-forms.utils.js +++ b/test/fn/convert-forms.utils.js @@ -12,6 +12,7 @@ const serializer = new XMLSerializer(); const createXformString = ({ itext = '', primaryInstance = '', + bindNodes = [], model = ` ${itext} @@ -21,6 +22,7 @@ const createXformString = ({ ${primaryInstance} + ${bindNodes.join('\n')} `, body = '' }) => ` diff --git a/test/lib/convert-forms/handle-var-checks.spec.js b/test/lib/convert-forms/handle-var-checks.spec.js new file mode 100644 index 00000000..838b3a0b --- /dev/null +++ b/test/lib/convert-forms/handle-var-checks.spec.js @@ -0,0 +1,284 @@ +const { expect } = require('chai'); +const sinon = require('sinon'); +const rewire = require('rewire'); +const checks = rewire('../../../src/lib/convert-forms/handle-var-checks'); +const { createXformDoc, FORM_ID } = require('../../fn/convert-forms.utils'); + +describe('Handle var checks', () => { + const getXmlString = bindNodes => ({ + model: ` + + + + + + + + + + + + + + + + + + + + ${bindNodes.join('\n')} + `, + body: ` + + + + + + + + + + + + + ` + }); + + let bindNodes; + let xml; + let props; + let info; + let warn; + beforeEach(() => { + bindNodes = [ + '' + ]; + xml = createXformDoc(getXmlString(bindNodes)); + + props = { 'some_prop': 'some_value' }; + + info = sinon.spy(checks.__get__('info')); + checks.__set__('info', info); + warn = sinon.spy(checks.__get__('warn')); + checks.__set__('warn', warn); + }); + afterEach(sinon.restore); + + it('should use DEFAULT checkVars when no "var_restriction" config is being supplied', () => { + const processPropData = sinon.spy(checks.__get__('processPropData')); + checks.__set__('processPropData', processPropData); + props = null; + expect(() => checks.checkVars(xml, props)).to.not.throw(); + expect(info.callCount).to.be.equal(0); + expect(warn.callCount).to.be.equal(0); + expect(processPropData.calledOnce).to.be.true; + expect(processPropData.calledWith({ warn_length: 100, error_length: 138 })).to.be.true; + expect(processPropData.returned({ + warnLength: 100, + errorLength: 138, + ignoreSet: new Set(), + reservedSet: new Set() + })).to.be.true; + }); + + it('should pick up var config & skip if no warn or error is being supplied', () => { + expect(() => checks.checkVars(xml, props)).to.not.throw(); + expect(info.calledOnce).to.be.true; + expect(info.args[0][0]).to.be.equal('Warn and error lengths and reserved list not provided. Skipping var checks.'); + }); + + it('should throw if warn and error has the same value', () => { + props = { + warn_length: 1, + error_length: 1, + }; + expect(() => checks.checkVars(xml, props)).to.throw( + 'The error length needs to be larger than the warn length.' + ); + }); + + it('should throw if the warn/error lengths contain invalid values', () => { + props = { + warn_length: 0.3, + error_length: '123', + }; + expect(() => checks.checkVars(xml, props)).to.throw( + 'Please ensure that the warn/error length value is a positive integer' + ); + }); + + it('should correctly warn when warn value is <= var length', () => { + props = { + warn_length: 1 + }; + + expect(() => checks.checkVars(xml, props)).to.not.throw(); + expect(info.callCount).to.be.equal(0); + expect(warn.args[0][0]).to.be.equal('The following vars are longer than the acceptable var length (1):\n' + + '/data/inputs/user/contact_id\n' + + 'Please consider simplifying nesting or removing verbosity.'); + }); + + it('should correctly throw when error value is <= var length', () => { + props = { + error_length: 1 + }; + + expect(() => checks.checkVars(xml, props)).to.throw( + 'The following vars are longer than the acceptable var length (1):\n' + + '/data/inputs/user/contact_id\n' + + 'Please simplify nesting or remove verbosity.' + ); + }); + + it('should pass when error value is larger than variable length', () => { + props = { + warn_length: 28, + error_length: 29 + }; + + expect(() => checks.checkVars(xml, props)).to.not.throw(); + expect(info.callCount).to.be.equal(0); + expect(warn.args[0][0]).to.be.equal( + 'The following vars are longer than the acceptable var length (28):\n' + + '/data/inputs/user/contact_id\n' + + 'Please consider simplifying nesting or removing verbosity.' + ); + }); + + it('should pass when warn value is larger than variable length', () => { + props = { + warn_length: 29, + error_length: 30 + }; + + expect(() => checks.checkVars(xml, props)).to.not.throw(); + expect(info.callCount).to.be.equal(0); + expect(warn.callCount).to.be.equal(0); + }); + + it('should pass when var is ignored despite provided error value', () => { + props = { + error_length: 23, + ignore_list: ['/data/inputs/user/contact_id'] + }; + + bindNodes.push( + '' + ); + xml = createXformDoc(getXmlString(bindNodes)); + + expect(() => checks.checkVars(xml, props)).to.not.throw(); + expect(info.callCount).to.be.equal(0); + expect(warn.callCount).to.be.equal(0); + }); + + it('should not throw when ignored item does not exist in xml', () => { + props = { + error_length: 30, + ignore_list: [123] + }; + + expect(() => checks.checkVars(xml, props)).to.not.throw(); + expect(info.callCount).to.be.equal(0); + expect(warn.callCount).to.be.equal(0); + }); + + it('should throw when reserved item exists in xml', () => { + props = { + error_length: 30, + reserved_list: ['/data/inputs/user/name'] + }; + + bindNodes.push( + '' + ); + xml = createXformDoc(getXmlString(bindNodes)); + + expect(() => checks.checkVars(xml, props)).to.throw( + 'The following reserved entries were found in the form:\n' + + '/data/inputs/user/name\n' + + 'Please remove or rename as appropriate.' + ); + expect(info.callCount).to.be.equal(0); + expect(warn.callCount).to.be.equal(0); + }); + + it('should throw when ignore list contains invalid entry', () => { + props = { + error_length: 30, + ignore_list: ['/data/inputs/user/"name`'], + }; + + bindNodes.push( + '' + ); + xml = createXformDoc(getXmlString(bindNodes)); + + expect(() => checks.checkVars(xml, props)).to.throw( + 'The following ignored entries are invalid:\n' + + '/data/inputs/user/"name`\n' + + 'Please fix or remove where appropriate.' + ); + expect(info.callCount).to.be.equal(0); + expect(warn.callCount).to.be.equal(0); + }); + + it('should throw when reserved list contains invalid entry', () => { + props = { + error_length: 30, + reserved_list: ['/data/inputs/user/"name`'], + }; + + bindNodes.push( + '' + ); + xml = createXformDoc(getXmlString(bindNodes)); + + expect(() => checks.checkVars(xml, props)).to.throw( + 'The following reserved entries are invalid:\n' + + '/data/inputs/user/"name`\n' + + 'Please fix or remove where appropriate.' + ); + expect(info.callCount).to.be.equal(0); + expect(warn.callCount).to.be.equal(0); + }); + + it('should throw when reserved item is also in the ignore list', () => { + props = { + error_length: 30, + ignore_list: ['/data/inputs/user/name'], + reserved_list: ['/data/inputs/user/name'] + }; + + bindNodes.push( + '' + ); + xml = createXformDoc(getXmlString(bindNodes)); + + expect(() => checks.checkVars(xml, props)).to.throw( + 'Overlap between reserved and ignore lists:\n' + + '/data/inputs/user/name\n' + + 'Please remove where appropriate.' + ); + expect(info.callCount).to.be.equal(0); + expect(warn.callCount).to.be.equal(0); + }); + + it('should notify when a form has no bind entries to process', () => { + props = { + warn_length: 30, + error_length: 31, + ignore_list: ['/data/inputs/user/contact_id'], + reserved_list: ['/data/inputs/user/name'] + }; + + bindNodes = []; + xml = createXformDoc(getXmlString(bindNodes)); + + expect(() => checks.checkVars(xml, props)).to.not.throw(); + expect(info.calledOnce).to.be.true; + expect(info.args[0][0]).to.be.equal('Form did not contain any bind nodes'); + expect(warn.callCount).to.be.equal(0); + }); +}); diff --git a/test/lib/convert-forms/index.spec.js b/test/lib/convert-forms/index.spec.js index e7ba65bd..0af2324a 100644 --- a/test/lib/convert-forms/index.spec.js +++ b/test/lib/convert-forms/index.spec.js @@ -18,7 +18,7 @@ describe('convert-forms', () => { convertForms.__set__('warn', sinon.stub(log, 'warn')); convertForms.__set__('exec', mockExec); convertForms.__set__('fixXml', sinon.stub()); - convertForms.__set__('getHiddenFields', sinon.stub()); + convertForms.__set__('getPropsData', sinon.stub()); sinon.stub(fs, 'readdir').returns(['a.xml', 'b.xlsx', 'c.xlsx']); sinon.stub(fs, 'exists').returns(true); @@ -218,4 +218,68 @@ describe('convert-forms', () => { ]); }); }); + + describe('var checks', () => { + const convertForms = rewire('./../../../src/lib/convert-forms'); + const { createXformString } = require('../../fn/convert-forms.utils'); + const FORM_ID = 'c'; + const getXmlString = () => ({ + model: ` + + + + + ` + }); + let getPropsData; + let fixXml; + let checkVars; + + beforeEach(() => { + mockExec.resolves(JSON.stringify({ code: 100 })); + + getPropsData = sinon.stub().returns({ + var_restrictions: { some_prop: 'some_value' } + }); + convertForms.__set__('getPropsData', getPropsData); + + const realCheckVars = convertForms.__get__('checkVars'); + checkVars = sinon.spy(realCheckVars); + convertForms.__set__('checkVars', checkVars); + + const realFixXml = convertForms.__get__('fixXml'); + fixXml = sinon.spy(realFixXml); + convertForms.__set__('fixXml', fixXml); + + convertForms.__set__('exec', mockExec); + + sinon.stub(fs, 'read').returns(createXformString(getXmlString())); + sinon.stub(fs, 'write').returns(true); + }); + afterEach(sinon.restore); + + it('should skip checkVars when no "var_restriction" config is being supplied', async () => { + getPropsData = sinon.stub().returns({}); + convertForms.__set__('getPropsData', getPropsData); + await expect(convertForms.execute('./path', 'app', { forms: [FORM_ID] })).to.be.fulfilled; + + expect(getPropsData.calledOnce).to.be.true; + expect(getPropsData.args[0][0]).to.be.equal('./path/forms/app/c.properties.json'); + expect(fixXml.calledOnce).to.be.true; + expect(fixXml.args[0][1]).to.be.deep.equal({}); + expect(checkVars.callCount).to.be.equal(0); + expect(fs.write.calledOnce).to.be.true; + }); + + it('should pick up var config & call checkVars', async () => { + await expect(convertForms.execute('./path', 'app', { forms: ['c'] })).to.be.fulfilled; + + expect(getPropsData.calledOnce).to.be.true; + expect(getPropsData.args[0][0]).to.be.equal('./path/forms/app/c.properties.json'); + expect(fixXml.calledOnce).to.be.true; + expect(fixXml.args[0][1]).to.be.deep.equal({ 'var_restrictions': { 'some_prop': 'some_value' } }); + expect(checkVars.calledOnce).to.be.true; + expect(checkVars.args[0][1]).to.be.deep.equal({ 'some_prop': 'some_value' }); + }); + }); });