From 3fb493e709bc6b6e661a1581b622d849ddd1813f Mon Sep 17 00:00:00 2001 From: eyelidlessness Date: Tue, 9 Jul 2024 14:59:09 -0700 Subject: [PATCH] Roll back boolean casting fix As we discussed, this can be handled in a separate scope of work. The original commits are left intact for reference, we can rebase before merge if preferred. --- packages/scenario/test/readonly.test.ts | 150 ++++++++++++------ packages/scenario/test/relevant.test.ts | 52 ++++-- packages/scenario/test/validity-state.test.ts | 114 ++++++++----- .../reactivity/createComputedExpression.ts | 90 +---------- 4 files changed, 215 insertions(+), 191 deletions(-) diff --git a/packages/scenario/test/readonly.test.ts b/packages/scenario/test/readonly.test.ts index b15ee1f1..65c453fe 100644 --- a/packages/scenario/test/readonly.test.ts +++ b/packages/scenario/test/readonly.test.ts @@ -14,65 +14,113 @@ import { describe, expect, it } from 'vitest'; import { Scenario } from '../src/jr/Scenario.ts'; describe('TriggerableDagTest.java', () => { - it('is inherited from ancestors', async () => { - const scenario = await Scenario.init( - 'Some form', - html( - head( - title('Some form'), - model( - mainInstance( - t( - 'data id="some-form"', - t('is-outer-readonly'), - t('is-inner-readonly'), - t('is-field-readonly'), - t('outer', t('inner', t('field'))) + interface CastReadonlyExpressionOptions { + readonly castReadonlyExpressionsAsNumber: boolean; + } + + /** + * **PORTING NOTES** + * + * This test has the same semantic considerations (XPath node-set -> boolean + * casting, potential considerations for value casting on writes, etc) as the + * ported relevance test of a similar name/approach. + * + * The test has been parameterized to demonstrate this concisely, with the + * second run wrapping the `readonly` expressions in a `number()` cast (which + * passes as expected). + * + * JR: + * + * Read-only is inherited from ancestor nodes, as per the W3C XForms specs: + * - https://www.w3.org/TR/xforms11/#model-prop-relevant + */ + describe.each([ + { castReadonlyExpressionsAsNumber: false }, + { castReadonlyExpressionsAsNumber: true }, + ])( + 'readonly (cast readonly expression as number: $castReadonlyExpressionAsNumber)', + ({ castReadonlyExpressionsAsNumber }) => { + let testFn: typeof it | typeof it.fails; + + if (castReadonlyExpressionsAsNumber) { + testFn = it; + } else { + testFn = it.fails; + } + + let castReadonlyExpression: (baseExpression: string) => string; + + if (castReadonlyExpressionsAsNumber) { + castReadonlyExpression = (baseExpression) => `number(${baseExpression})`; + } else { + castReadonlyExpression = (baseExpression) => baseExpression; + } + + testFn('is inherited from ancestors', async () => { + const scenario = await Scenario.init( + 'Some form', + html( + head( + title('Some form'), + model( + mainInstance( + t( + 'data id="some-form"', + t('is-outer-readonly'), + t('is-inner-readonly'), + t('is-field-readonly'), + t('outer', t('inner', t('field'))) + ) + ), + bind('/data/is-outer-readonly').type('boolean'), + bind('/data/is-inner-readonly').type('boolean'), + bind('/data/is-field-readonly').type('boolean'), + bind('/data/outer').readonly(castReadonlyExpression('/data/is-outer-readonly')), + bind('/data/outer/inner').readonly( + castReadonlyExpression('/data/is-inner-readonly') + ), + bind('/data/outer/inner/field') + .type('string') + .readonly(castReadonlyExpression('/data/is-field-readonly')) ) ), - bind('/data/is-outer-readonly').type('boolean'), - bind('/data/is-inner-readonly').type('boolean'), - bind('/data/is-field-readonly').type('boolean'), - bind('/data/outer').readonly('/data/is-outer-readonly'), - bind('/data/outer/inner').readonly('/data/is-inner-readonly'), - bind('/data/outer/inner/field').type('string').readonly('/data/is-field-readonly') + body( + input('/data/is-outer-readonly'), + input('/data/is-inner-readonly'), + input('/data/is-field-readonly'), + group('/data/outer', group('/data/outer/inner', input('/data/outer/inner/field'))) + ) ) - ), - body( - input('/data/is-outer-readonly'), - input('/data/is-inner-readonly'), - input('/data/is-field-readonly'), - group('/data/outer', group('/data/outer/inner', input('/data/outer/inner/field'))) - ) - ) - ); + ); - // Form initialization evaluates all triggerables, which makes the field editable (not read-only) - expect(scenario.getInstanceNode('/data/outer')).toBeEnabled(); - expect(scenario.getInstanceNode('/data/outer/inner')).toBeEnabled(); - expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeEnabled(); + // Form initialization evaluates all triggerables, which makes the field editable (not read-only) + expect(scenario.getInstanceNode('/data/outer')).toBeEnabled(); + expect(scenario.getInstanceNode('/data/outer/inner')).toBeEnabled(); + expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeEnabled(); - // Make the outer group read-only - scenario.answer('/data/is-outer-readonly', true); + // Make the outer group read-only + scenario.answer('/data/is-outer-readonly', true); - expect(scenario.getInstanceNode('/data/outer')).toBeReadonly(); - expect(scenario.getInstanceNode('/data/outer/inner')).toBeReadonly(); - expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeReadonly(); + expect(scenario.getInstanceNode('/data/outer')).toBeReadonly(); + expect(scenario.getInstanceNode('/data/outer/inner')).toBeReadonly(); + expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeReadonly(); - // Make the inner group read-only - scenario.answer('/data/is-outer-readonly', false); - scenario.answer('/data/is-inner-readonly', true); + // Make the inner group read-only + scenario.answer('/data/is-outer-readonly', false); + scenario.answer('/data/is-inner-readonly', true); - expect(scenario.getInstanceNode('/data/outer')).toBeEnabled(); - expect(scenario.getInstanceNode('/data/outer/inner')).toBeReadonly(); - expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeReadonly(); + expect(scenario.getInstanceNode('/data/outer')).toBeEnabled(); + expect(scenario.getInstanceNode('/data/outer/inner')).toBeReadonly(); + expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeReadonly(); - // Make the field read-only - scenario.answer('/data/is-inner-readonly', false); - scenario.answer('/data/is-field-readonly', true); + // Make the field read-only + scenario.answer('/data/is-inner-readonly', false); + scenario.answer('/data/is-field-readonly', true); - expect(scenario.getInstanceNode('/data/outer')).toBeEnabled(); - expect(scenario.getInstanceNode('/data/outer/inner')).toBeEnabled(); - expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeReadonly(); - }); + expect(scenario.getInstanceNode('/data/outer')).toBeEnabled(); + expect(scenario.getInstanceNode('/data/outer/inner')).toBeEnabled(); + expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeReadonly(); + }); + } + ); }); diff --git a/packages/scenario/test/relevant.test.ts b/packages/scenario/test/relevant.test.ts index a665a55b..1f36869a 100644 --- a/packages/scenario/test/relevant.test.ts +++ b/packages/scenario/test/relevant.test.ts @@ -16,6 +16,7 @@ import { import { describe, expect, it } from 'vitest'; import { intAnswer } from '../src/answer/ExpectedIntAnswer.ts'; import { stringAnswer } from '../src/answer/ExpectedStringAnswer.ts'; +import type { UntypedAnswer } from '../src/answer/UntypedAnswer.ts'; import { Scenario, getRef } from '../src/jr/Scenario.ts'; import { XPathPathExpr } from '../src/jr/xpath/XPathPathExpr.ts'; import { XPathPathExprEval } from '../src/jr/xpath/XPathPathExprEval.ts'; @@ -28,7 +29,26 @@ describe('Relevance - TriggerableDagTest.java', () => { */ describe('non-relevance', () => { - it('is inherited from ancestors', async () => { + /** + * **PORTING NOTES** + * + * - This fails because the `relevant` expressions produce node-sets, which + * will always evaluate to `true` when those nodes are present (which they + * always are in this test). + * + * - Those node-sets evaluate to nodes which are bound with ``, which strongly suggests that a bind's data type + * should influence casting semantics in expressions like `relevant`, and + * perhaps more generally. + * + * - There are some unaddressed casting considerations which **might be** + * implied by this, discussed in greater detail in porting notes on + * {@link UntypedAnswer}. + * + * Two additional variants of this test are added immediately following this + * one, both briefly exploring some of the contours of the current failure. + */ + it.fails('is inherited from ancestors', async () => { const scenario = await Scenario.init( 'Some form', html( @@ -82,13 +102,23 @@ describe('Relevance - TriggerableDagTest.java', () => { * **PORTING NOTES** (first variant of ported test above) * * This test is identical to the test above, except that both `relevant` - * expressions are wrapped in a `string()` XPath call. This test was - * originally introduced to demonstrate different behavior of boolean - * casting logic depending on the expression's result type. This distinction - * no longer exists, but the alternate test is preserved to help us catch - * potential regressions in boolean casting logic. + * expressions are wrapped in a `string()` XPath call. The test still fails, + * but notably the failing assertion comes later: + * + * In the original test, the first assertion fails because a `node-set` + * expression which resolves to any node will always cast to `true`. When + * the value is cast to a string, the node's text value is consulted in + * casting, producing `false` when empty. + * + * Ultimately, the test fails when checking restoration of the `false` + * state. This is because the `false` value is presently being persisted to + * the primary instance as the string `"0"` (which, as I recall, is the + * expected serialization of boolean `false`). Since the `relevant` + * expression itself produces a string value, and with the engine still + * following strict XPath casting semantics, the value `"0"` is also cast to + * boolean `true` (again, consistent with XPath semantics). */ - it('is inherited from ancestors (variant #1: node-set semantics -> string)', async () => { + it.fails('is inherited from ancestors (variant #1: node-set semantics -> string)', async () => { const scenario = await Scenario.init( 'Some form', html( @@ -142,10 +172,10 @@ describe('Relevance - TriggerableDagTest.java', () => { * **PORTING NOTES** (second variant) * * This variant of the ported JavaRosa test again casts the `relevant` - * expressions, this time to `number`. Like the previous variant, this test - * was originally introduced to demonstrate differences in casting behavior - * depending on the expression's result type. It is likewise preserved to - * help us avoid casting-related regressions in the future. + * expressions, this time to `number`. Here we see the test passes! This + * variant is included because it demonstrates all of the findings above, by + * showing how strict XPath casting semantics interact with the test form's + * expected XForms semantics. */ it('is inherited from ancestors (variant #2: node-set semantics -> number)', async () => { const scenario = await Scenario.init( diff --git a/packages/scenario/test/validity-state.test.ts b/packages/scenario/test/validity-state.test.ts index bafe594e..7eab39e0 100644 --- a/packages/scenario/test/validity-state.test.ts +++ b/packages/scenario/test/validity-state.test.ts @@ -86,50 +86,78 @@ describe('TriggerableDagTest.java', () => { }); describe('constraint violations and form finalization', () => { - it('[has no clear BDD-ish description equivalent]', async () => { - const scenario = await Scenario.init( - 'Some form', - html( - head( - title('Some form'), - model( - mainInstance(t('data id="some-form"', t('a'), t('b'))), - bind('/data/a').type('string').constraint('/data/b'), - bind('/data/b').type('boolean') - ) - ), - body(input('/data/a'), input('/data/b')) - ) - ); - - // First, ensure we will be able to commit an answer in /data/a by - // making it match its constraint. No values can be committed to the - // instance if constraints aren't satisfied. - scenario.answer('/data/b', true); - - // Then, commit an answer (answers with empty values are always valid) - scenario.answer('/data/a', 'cocotero'); - - // Then, make the constraint defined at /data/a impossible to satisfy - scenario.answer('/data/b', false); - - // At this point, the form has /data/a filled with an answer that's - // invalid according to its constraint expression, but we can't be - // aware of that, unless we validate the whole form. - // - // Clients like Collect will validate the whole form before marking - // a submission as complete and saving it to the filesystem. - // - // FormDef.validate(boolean) will go through all the relevant fields - // re-answering them with their current values in order to detect - // any constraint violations. When this happens, a non-null - // ValidationOutcome object is returned including information about - // the violated constraint. - const validate = scenario.getValidationOutcome(); + interface CastReadonlyExpressionOptions { + readonly castReadonlyExpressionsAsNumber: boolean; + } - expect(validate.failedPrompt).toBe(scenario.indexOf('/data/a')); - expect(validate.outcome).toBe(ANSWER_CONSTRAINT_VIOLATED); - }); + describe.each([ + { castReadonlyExpressionsAsNumber: false }, + { castReadonlyExpressionsAsNumber: true }, + ])( + 'readonly (cast readonly expression as number: $castReadonlyExpressionAsNumber)', + ({ castReadonlyExpressionsAsNumber }) => { + let testFn: typeof it | typeof it.fails; + + if (castReadonlyExpressionsAsNumber) { + testFn = it; + } else { + testFn = it.fails; + } + + let castReadonlyExpression: (baseExpression: string) => string; + + if (castReadonlyExpressionsAsNumber) { + castReadonlyExpression = (baseExpression) => `number(${baseExpression})`; + } else { + castReadonlyExpression = (baseExpression) => baseExpression; + } + + testFn('[has no clear BDD-ish description equivalent]', async () => { + const scenario = await Scenario.init( + 'Some form', + html( + head( + title('Some form'), + model( + mainInstance(t('data id="some-form"', t('a'), t('b'))), + bind('/data/a').type('string').constraint(castReadonlyExpression('/data/b')), + bind('/data/b').type('boolean') + ) + ), + body(input('/data/a'), input('/data/b')) + ) + ); + + // First, ensure we will be able to commit an answer in /data/a by + // making it match its constraint. No values can be committed to the + // instance if constraints aren't satisfied. + scenario.answer('/data/b', true); + + // Then, commit an answer (answers with empty values are always valid) + scenario.answer('/data/a', 'cocotero'); + + // Then, make the constraint defined at /data/a impossible to satisfy + scenario.answer('/data/b', false); + + // At this point, the form has /data/a filled with an answer that's + // invalid according to its constraint expression, but we can't be + // aware of that, unless we validate the whole form. + // + // Clients like Collect will validate the whole form before marking + // a submission as complete and saving it to the filesystem. + // + // FormDef.validate(boolean) will go through all the relevant fields + // re-answering them with their current values in order to detect + // any constraint violations. When this happens, a non-null + // ValidationOutcome object is returned including information about + // the violated constraint. + const validate = scenario.getValidationOutcome(); + + expect(validate.failedPrompt).toBe(scenario.indexOf('/data/a')); + expect(validate.outcome).toBe(ANSWER_CONSTRAINT_VIOLATED); + }); + } + ); }); }); }); diff --git a/packages/xforms-engine/src/lib/reactivity/createComputedExpression.ts b/packages/xforms-engine/src/lib/reactivity/createComputedExpression.ts index 405c65e4..bef3c83e 100644 --- a/packages/xforms-engine/src/lib/reactivity/createComputedExpression.ts +++ b/packages/xforms-engine/src/lib/reactivity/createComputedExpression.ts @@ -23,87 +23,7 @@ type EvaluatedExpression< // prettier-ignore type ExpressionEvaluator< Type extends DependentExpressionResultType -> = () => EvaluatedExpression; - -/** - * Handles boolean casting as expected by XForms, which deviates from standard - * XPath semantics: - * - * - XForms: boolean expressions operate on the **value** of the result, - * potentially casting string and number values to produce a boolean result. - * - * - XPath: boolean evaluation of a node-set produces `true` for any non-empty - * node-set result, even where that node-set resolves to a single node, and - * where that node's value would ultimately be cast to false. - * - * @todo This implementation is more complex than would be ideal. There is - * some care taken to avoid reimplementing some of the more complex casting - * logic which is handled internally by the `xpath` package. Ultimately it - * relies on a heuristic: - * - * 1. Is the result false? If so, there's no need for further casting logic. - * 2. Is the result a node-set with multiple nodes? If so, defer to standard - * `xpath` casting. This is probably not what we'll want for every usage! - * 3. Is the result a node-set with a single node (or any other single-node) - * result type? If so, cast the node's **value**. - * 4. Otherwise, continue to defer to standard `xpath` casting. - */ -const booleanExpressionEvaluator = ( - evaluator: XFormsXPathEvaluator, - contextNode: Node, - expression: string -): ExpressionEvaluator<'boolean'> => { - return () => { - const anyResult = evaluator.evaluate(expression, contextNode, null, XPathResult.ANY_TYPE); - const { booleanValue, numberValue, resultType, stringValue } = anyResult; - - if (booleanValue === false) { - return false; - } - - const castSingleResultValue = (): boolean => { - if (numberValue === 0 || stringValue === '') { - return false; - } - - return booleanValue; - }; - - switch (resultType) { - case XPathResult.ORDERED_NODE_ITERATOR_TYPE: - case XPathResult.UNORDERED_NODE_ITERATOR_TYPE: { - let count = 0; - - while (anyResult.iterateNext() != null && count < 2) { - count += 1; - } - - if (count > 1) { - return booleanValue; - } - - return castSingleResultValue(); - } - - case XPathResult.ORDERED_NODE_SNAPSHOT_TYPE: - case XPathResult.UNORDERED_NODE_SNAPSHOT_TYPE: - if (anyResult.snapshotLength > 1) { - return booleanValue; - } - - return castSingleResultValue(); - - case XPathResult.ANY_UNORDERED_NODE_TYPE: - case XPathResult.FIRST_ORDERED_NODE_TYPE: - case XPathResult.STRING_TYPE: { - return castSingleResultValue(); - } - - default: - return booleanValue; - } - }; -}; +> = () => EvaluatedExpression const expressionEvaluator = ( evaluator: XFormsXPathEvaluator, @@ -115,11 +35,9 @@ const expressionEvaluator = ( switch (type) { case 'boolean': - return booleanExpressionEvaluator( - evaluator, - contextNode, - expression - ) as ExpressionEvaluator; + return (() => { + return evaluator.evaluateBoolean(expression, options); + }) as ExpressionEvaluator; case 'nodes': return (() => {