diff --git a/src/server/plugins/engine/errors.ts b/src/server/plugins/engine/errors.ts index 493af107c..e4d2b1d65 100644 --- a/src/server/plugins/engine/errors.ts +++ b/src/server/plugins/engine/errors.ts @@ -60,6 +60,21 @@ export class UnknownComponentTypeError extends InvalidFormDefinitionError { } } +/** + * Thrown when the route through the pages returns to a page it has already + * visited (e.g. two page paths that differ only by a trailing slash, or V1 + * `next` links that point back to an earlier page). + */ +export class PageRouteCycleError extends InvalidFormDefinitionError { + public readonly path: string + + constructor(path: string) { + super(`The route through the form returns to page '${path}'`) + this.name = 'PageRouteCycleError' + this.path = path + } +} + /** * Thrown when a form definition fails Joi schema validation. The raw * ValidationError is preserved as `cause` so consumers can reach the diff --git a/src/server/plugins/engine/helpers.test.ts b/src/server/plugins/engine/helpers.test.ts index dee710b8d..9072bb51d 100644 --- a/src/server/plugins/engine/helpers.test.ts +++ b/src/server/plugins/engine/helpers.test.ts @@ -10,6 +10,7 @@ import { encodeUrl, engine, evaluateTemplate, + findPage, getErrors, getExponentialBackoffDelay, getPageHref, @@ -288,6 +289,26 @@ describe('Helpers', () => { }) }) + describe('findPage', () => { + let model: FormModel + + beforeEach(() => { + const [firstPage, ...otherPages] = definition.pages + model = new FormModel( + { ...definition, pages: [{ ...firstPage, path: '/' }, ...otherPages] }, + { basePath: 'test' } + ) + }) + + it('finds the page with the root path when that path is given', () => { + expect(findPage(model, '/')).toBe(model.pages[0]) + }) + + it('finds no page when no path is given', () => { + expect(findPage(model, undefined)).toBeUndefined() + }) + }) + describe('getPageHref', () => { it('should return page href', () => { const returned = getPageHref(page) diff --git a/src/server/plugins/engine/helpers.ts b/src/server/plugins/engine/helpers.ts index 3d752bf29..c5523093e 100644 --- a/src/server/plugins/engine/helpers.ts +++ b/src/server/plugins/engine/helpers.ts @@ -259,6 +259,12 @@ export function getPage( } export function findPage(model: FormModel | undefined, path?: string) { + // A missing path finds no page rather than the page at '/', so that a + // route with no next path ends + if (path === undefined) { + return undefined + } + const findPath = `/${normalisePath(path)}` return model?.pages.find(({ path }) => path === findPath) } diff --git a/src/server/plugins/engine/models/FormModel.test.ts b/src/server/plugins/engine/models/FormModel.test.ts index d3084e2b7..2d8978784 100644 --- a/src/server/plugins/engine/models/FormModel.test.ts +++ b/src/server/plugins/engine/models/FormModel.test.ts @@ -9,10 +9,16 @@ import { } from '@defra/forms-model' import { todayAsDateOnly } from '~/src/server/plugins/engine/date-helper.js' -import { SchemaValidationError } from '~/src/server/plugins/engine/errors.js' +import { + PageRouteCycleError, + SchemaValidationError +} from '~/src/server/plugins/engine/errors.js' import { FormModel } from '~/src/server/plugins/engine/models/FormModel.js' import { buildFormContextRequest } from '~/src/server/plugins/engine/pageControllers/__stubs__/request.js' -import { type FormContextRequest } from '~/src/server/plugins/engine/types.js' +import { + type FormContextRequest, + type FormSubmissionState +} from '~/src/server/plugins/engine/types.js' import { FormAction } from '~/src/server/routes/types.js' import { V2 as definitionV2 } from '~/test/form/definitions/conditions-basic.js' import definition from '~/test/form/definitions/conditions-escaping.js' @@ -434,6 +440,246 @@ describe('FormModel', () => { zeQDES: ['peppers', 'cheese', 'ham'] }) }) + + describe('page walk', () => { + const [namePage, agePage, , summaryPage] = + joinedConditionsDefinition.pages + + const extraPage: PageQuestion = { + title: 'More details', + path: '/more-details', + components: [ + { + type: ComponentType.TextField, + title: 'More details', + name: 'moreDetails', + id: '6f1d9a52-8a3c-4c63-9f8b-1f1f6f0d7a11', + options: { required: false }, + schema: {} + } + ], + id: '0b9b7f0e-51d2-4d0c-a6e4-0a3c3c1c7d21', + next: [] + } + + function getContextFor(definition: FormDefinition, path: string) { + formDefinitionV2Schema.validate = jest + .fn() + .mockReturnValue({ value: definition }) + + const formModel = new FormModel(definition, { basePath: 'test' }) + const pageUrl = new URL(`http://example.com/test/${path}`) + + const request: FormContextRequest = buildFormContextRequest({ + method: 'get', + query: {}, + path: pageUrl.pathname, + params: { path, slug: 'test' }, + url: pageUrl, + app: { model: formModel } + }) + + return formModel.getFormContext(request, { + $$__referenceNumber: 'foobar' + }) + } + + it('ends after the last page when a page has the root path and the requested page is off the route', () => { + const definition: FormDefinition = { + ...joinedConditionsDefinition, + pages: [namePage, agePage, { ...extraPage, path: '/' }, summaryPage] + } + + // The age page has a condition that is false for an empty state + const context = getContextFor(definition, 'age') + + expect(context.relevantPages.map(({ path }) => path)).toEqual([ + '/name', + '/', + '/summary', + '/status' + ]) + }) + + it('throws when a page path resolves to a page earlier in the route', () => { + const definition: FormDefinition = { + ...joinedConditionsDefinition, + pages: [ + namePage, + agePage, + { ...extraPage, path: '/name/' }, + summaryPage + ] + } + + expect(() => getContextFor(definition, 'summary')).toThrow( + new PageRouteCycleError('/name') + ) + }) + + it('throws when the next links of a V1 form make a cycle', () => { + const textField = (name: string) => ({ + type: ComponentType.TextField as const, + name, + title: name, + options: {}, + schema: {} + }) + + const definition: FormDefinition = { + name: 'Next link cycle', + startPage: '/first', + pages: [ + { + title: 'First', + path: '/first', + components: [textField('first')], + next: [{ path: '/second' }] + }, + { + title: 'Second', + path: '/second', + components: [textField('second')], + next: [{ path: '/first' }] + }, + { + title: 'Third', + path: '/third', + components: [textField('third')], + next: [] + } + ], + conditions: [], + lists: [], + sections: [] + } + + expect(() => getContextFor(definition, 'third')).toThrow( + new PageRouteCycleError('/first') + ) + }) + }) + }) + + describe('makeFilteredSchema', () => { + const textPage = (name: string) => ({ + title: name, + path: `/${name}`, + components: [ + { + type: ComponentType.TextField as const, + name, + title: name, + options: {}, + schema: {} + } + ], + next: [] + }) + + const sitePage = (name: string) => ({ + title: name, + path: `/${name}`, + components: [ + { + type: ComponentType.EastingNorthingField as const, + name, + title: name, + options: { countries: ['wales' as const] }, + schema: {} + } + ], + next: [] + }) + + const definition: FormDefinition = { + name: 'Schema of many pages', + startPage: '/first', + pages: [ + textPage('first'), + sitePage('siteOne'), + textPage('second'), + sitePage('siteTwo'), + textPage('third') + ], + conditions: [], + lists: [], + sections: [] + } + + const inWales = { easting: 326781, northing: 269766 } + const outsideWales = { easting: 332218, northing: 283862 } + + const siteState = (name: string, site: typeof inWales) => ({ + [`${name}__easting`]: site.easting, + [`${name}__northing`]: site.northing + }) + + function validate(state: FormSubmissionState, form = definition) { + const model = new FormModel(form, { basePath: 'test' }) + const pages = model.pages.filter(({ path }) => path !== '/status') + + return model + .makeFilteredSchema(pages) + .validate(state, { abortEarly: false }) + } + + it('reports the missing answers of all pages in page order', () => { + const { error } = validate({}) + + expect(error?.details.map(({ path }) => path.join('.'))).toEqual([ + 'first', + 'siteOne__easting', + 'siteOne__northing', + 'second', + 'siteTwo__easting', + 'siteTwo__northing', + 'third' + ]) + }) + + it('keeps the page order for a form with more pages than one batch', () => { + // Component names can only have letters + const names = Array.from( + { length: 70 }, + (_, index) => + `page${'abcdefghij'.charAt(index / 7)}${'abcdefg'.charAt(index % 7)}` + ) + + const { error } = validate( + {}, + { ...definition, startPage: `/${names[0]}`, pages: names.map(textPage) } + ) + + expect(error?.details.map(({ path }) => path.join('.'))).toEqual(names) + }) + + it('accepts a complete and valid state', () => { + const { error } = validate({ + first: 'a', + ...siteState('siteOne', inWales), + second: 'b', + ...siteState('siteTwo', inWales), + third: 'c' + }) + + expect(error).toBeUndefined() + }) + + it('keeps the country rule of each easting and northing field', () => { + const { error } = validate({ + first: 'a', + ...siteState('siteOne', outsideWales), + second: 'b', + ...siteState('siteTwo', outsideWales), + third: 'c' + }) + + expect(error?.details.map(({ message }) => message)).toEqual([ + expect.stringContaining('"siteOne" failed custom validation'), + expect.stringContaining('"siteTwo" failed custom validation') + ]) + }) }) describe('makeCondition', () => { diff --git a/src/server/plugins/engine/models/FormModel.ts b/src/server/plugins/engine/models/FormModel.ts index 8ebd877c2..1b30557c1 100644 --- a/src/server/plugins/engine/models/FormModel.ts +++ b/src/server/plugins/engine/models/FormModel.ts @@ -30,7 +30,8 @@ import { import { add, format } from 'date-fns' import { Parser, type Value } from 'expr-eval-fork' import { type i18n } from 'i18next' -import joi from 'joi' +import joi, { type ObjectSchema } from 'joi' +import chunk from 'lodash/chunk.js' import { logger } from '~/src/server/common/helpers/logging/logger.js' import { EN_GB } from '~/src/server/constants.js' @@ -43,6 +44,7 @@ import { import { todayAsDateOnly } from '~/src/server/plugins/engine/date-helper.js' import { ConditionBuildError, + PageRouteCycleError, SchemaValidationError } from '~/src/server/plugins/engine/errors.js' import { @@ -78,6 +80,18 @@ import { FormAction } from '~/src/server/routes/types.js' import { merge } from '~/src/server/services/cacheService.js' import { type Services } from '~/src/server/types.js' +/** + * Joi copies both schemas on each concat. A small batch keeps each copy + * small, so that the build time stays low for forms with many pages. + */ +const PAGES_PER_BATCH = 32 + +function joinSchemas(schemas: ObjectSchema[]) { + const emptySchema = joi.object().required() + + return schemas.reduce((joined, schema) => joined.concat(schema), emptySchema) +} + export class FormModel { /** The runtime engine that should be used */ engine?: Engine @@ -265,15 +279,12 @@ export class FormModel { * for pages which are no longer accessible due to an answer that has been changed */ makeFilteredSchema(relevantPages: PageControllerClass[]) { - // Build the entire model schema - // from the individual pages/sections - let schema = joi.object().required() + const pageSchemas = relevantPages.map((page) => page.collection.stateSchema) - relevantPages.forEach((page) => { - schema = schema.concat(page.collection.stateSchema) - }) + const batchesOfPageSchemas = chunk(pageSchemas, PAGES_PER_BATCH) + const schemaForEachBatch = batchesOfPageSchemas.map(joinSchemas) - return schema + return joinSchemas(schemaForEachBatch) } /** @@ -428,8 +439,17 @@ export class FormModel { this.initialiseContext(context) + const visitedPages = new Set() + // Walk form pages from start while (nextPage) { + // A route that returns to a visited page has no end + if (visitedPages.has(nextPage)) { + throw new PageRouteCycleError(nextPage.path) + } + + visitedPages.add(nextPage) + // Add page to context context.relevantPages.push(nextPage)