From 97f7de92a7ea8341cd2c69513aa4ba6a5e6846a3 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Fri, 2 Oct 2026 17:05:06 +0100 Subject: [PATCH 1/3] End the page walk when a route has no next page or returns to a page getFormContext walks the pages from the start page until it gets to the requested page. After the last page the next path is undefined, and findPage read that as the root path '/'. A form with a page at '/' then continued the walk from that page with no end, for each request to a page that was off the route. The walk is synchronous, so the request blocked the process. findPage now finds no page when it gets no path, so the walk ends after the last page. The walk also remembers the pages it has visited and throws a PageRouteCycleError when the route returns to one of them. This covers page paths that are equal after normalisation, such as '/name' and '/name/', and V1 next links that point back to an earlier page. --- src/server/plugins/engine/errors.ts | 15 +++ src/server/plugins/engine/helpers.test.ts | 21 +++ src/server/plugins/engine/helpers.ts | 6 + .../plugins/engine/models/FormModel.test.ts | 124 +++++++++++++++++- src/server/plugins/engine/models/FormModel.ts | 10 ++ 5 files changed, 175 insertions(+), 1 deletion(-) 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..e3720ba45 100644 --- a/src/server/plugins/engine/models/FormModel.test.ts +++ b/src/server/plugins/engine/models/FormModel.test.ts @@ -9,7 +9,10 @@ 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' @@ -434,6 +437,125 @@ 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('makeCondition', () => { diff --git a/src/server/plugins/engine/models/FormModel.ts b/src/server/plugins/engine/models/FormModel.ts index 8ebd877c2..9b225d731 100644 --- a/src/server/plugins/engine/models/FormModel.ts +++ b/src/server/plugins/engine/models/FormModel.ts @@ -43,6 +43,7 @@ import { import { todayAsDateOnly } from '~/src/server/plugins/engine/date-helper.js' import { ConditionBuildError, + PageRouteCycleError, SchemaValidationError } from '~/src/server/plugins/engine/errors.js' import { @@ -428,8 +429,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) From b7c87c2d541c2d9b237f861198cf3769178f2b29 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Mon, 5 Oct 2026 10:24:55 +0100 Subject: [PATCH 2/3] Join the page state schemas in pairs makeFilteredSchema joined the state schema of each relevant page onto one growing schema. Joi copies both schemas on each concat, so the work grew with the square of the page count. For a form with 239 relevant pages the build took about 650 ms of synchronous work, on each request late in the journey and two times on the summary page. The schemas are now joined in pairs, then the results in pairs, until one is left. Each key is copied about log2(pages) times. The same form builds in about 28 ms. It still uses concat, so object-level rules such as the country rule of EastingNorthingField stay in the joined schema, and the keys keep the page order. --- .../plugins/engine/models/FormModel.test.ts | 110 +++++++++++++++++- src/server/plugins/engine/models/FormModel.ts | 21 +++- 2 files changed, 125 insertions(+), 6 deletions(-) diff --git a/src/server/plugins/engine/models/FormModel.test.ts b/src/server/plugins/engine/models/FormModel.test.ts index e3720ba45..eb7d1b8a5 100644 --- a/src/server/plugins/engine/models/FormModel.test.ts +++ b/src/server/plugins/engine/models/FormModel.test.ts @@ -15,7 +15,10 @@ import { } 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' @@ -558,6 +561,111 @@ describe('FormModel', () => { }) }) + 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) { + const model = new FormModel(definition, { 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('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', () => { test('relative date condition', () => { formDefinitionV2Schema.validate = jest diff --git a/src/server/plugins/engine/models/FormModel.ts b/src/server/plugins/engine/models/FormModel.ts index 9b225d731..f5d7e97dd 100644 --- a/src/server/plugins/engine/models/FormModel.ts +++ b/src/server/plugins/engine/models/FormModel.ts @@ -268,13 +268,24 @@ export class FormModel { makeFilteredSchema(relevantPages: PageControllerClass[]) { // Build the entire model schema // from the individual pages/sections - let schema = joi.object().required() + let schemas = relevantPages.map((page) => page.collection.stateSchema) - relevantPages.forEach((page) => { - schema = schema.concat(page.collection.stateSchema) - }) + // Each concat copies both of its schemas, so they are joined in pairs + // rather than one by one, to keep each copy small + while (schemas.length > 1) { + const pairs: typeof schemas = [] + + for (let i = 0; i < schemas.length; i += 2) { + const left = schemas[i] + pairs.push(i + 1 < schemas.length ? left.concat(schemas[i + 1]) : left) + } + + schemas = pairs + } + + const schema = joi.object().required() - return schema + return schemas.length ? schema.concat(schemas[0]) : schema } /** From a740f5907c4e9f67fa40da62953448d307b856b0 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Mon, 5 Oct 2026 11:26:53 +0100 Subject: [PATCH 3/3] Join the page state schemas in batches The pairs loop made the build fast but was hard to read. makeFilteredSchema now joins the page schemas in batches of 32, then joins the batch schemas. Both steps use the same plain helper, joinSchemas, which adds one schema at a time. Inside a batch the growing schema has 32 pages at most, and only the few joins between batches work on a large schema. For a form with 239 relevant pages the build takes about 45 to 65 ms, against about 650 to 870 ms for one join for each page on the full schema. The pages are still joined first to last, so the keys, the object-level rules and the order of errors are the same. A new test covers a form with more pages than one batch. --- .../plugins/engine/models/FormModel.test.ts | 20 +++++++++- src/server/plugins/engine/models/FormModel.ts | 37 +++++++++---------- 2 files changed, 36 insertions(+), 21 deletions(-) diff --git a/src/server/plugins/engine/models/FormModel.test.ts b/src/server/plugins/engine/models/FormModel.test.ts index eb7d1b8a5..2d8978784 100644 --- a/src/server/plugins/engine/models/FormModel.test.ts +++ b/src/server/plugins/engine/models/FormModel.test.ts @@ -615,8 +615,8 @@ describe('FormModel', () => { [`${name}__northing`]: site.northing }) - function validate(state: FormSubmissionState) { - const model = new FormModel(definition, { basePath: 'test' }) + function validate(state: FormSubmissionState, form = definition) { + const model = new FormModel(form, { basePath: 'test' }) const pages = model.pages.filter(({ path }) => path !== '/status') return model @@ -638,6 +638,22 @@ describe('FormModel', () => { ]) }) + 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', diff --git a/src/server/plugins/engine/models/FormModel.ts b/src/server/plugins/engine/models/FormModel.ts index f5d7e97dd..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' @@ -79,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 @@ -266,26 +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 schemas = relevantPages.map((page) => page.collection.stateSchema) - - // Each concat copies both of its schemas, so they are joined in pairs - // rather than one by one, to keep each copy small - while (schemas.length > 1) { - const pairs: typeof schemas = [] - - for (let i = 0; i < schemas.length; i += 2) { - const left = schemas[i] - pairs.push(i + 1 < schemas.length ? left.concat(schemas[i + 1]) : left) - } - - schemas = pairs - } + const pageSchemas = relevantPages.map((page) => page.collection.stateSchema) - const schema = joi.object().required() + const batchesOfPageSchemas = chunk(pageSchemas, PAGES_PER_BATCH) + const schemaForEachBatch = batchesOfPageSchemas.map(joinSchemas) - return schemas.length ? schema.concat(schemas[0]) : schema + return joinSchemas(schemaForEachBatch) } /**