Skip to content

Commit

Permalink
Fix: refine repeat range self relevance logic
Browse files Browse the repository at this point in the history 
Extensive commentary in JSDoc, some of which is probably worth discussing in review.
  • Loading branch information
@eyelidlessness
eyelidlessness committed Jun 19, 2024
1 parent 90e1ad6 commit b44beec
Show file tree
Hide file tree
Showing 4 changed files with 150 additions and 107 deletions.
76 changes: 31 additions & 45 deletions packages/scenario/test/relevant.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -491,53 +491,39 @@ describe('Relevance - TriggerableDagTest.java', () => {
);

/**
* **PORTING NOTES** (supplemental: 2 of 3)
*
* Per discussion of the test above, this test is effectively the same as
* that one, except that it eliminates the ambiguity of 0-arity `position()`
* called with a repeat instance context.
*
* This exercises the other two bugs found porting the above tests:
*
* - Incorrect `relevant` inheritance from any single repeat instance ->
* that single repeat instance's repeat range parent -> all of that parent
* repeat range's repeat instance children.
*
* - Incorrectly preserving form default values on non-relevant nodes.
* **PORTING NOTES** (supplemental: 2 of 3; see commit history for
* additional context and commentary)
*/
it.fails(
'is excluded from producing default values in an evaluation (supplemental to two previous tests)',
async () => {
const scenario = await Scenario.init(
'Some form',
html(
head(
title('Some form'),
model(
mainInstance(
t(
'data id="some-form"',
// position() is one-based
t('node', t('value', '1')), // non-relevant
t('node', t('value', '2')), // non-relevant
t('node', t('value', '3')), // relevant
t('node', t('value', '4')), // relevant
t('node', t('value', '5')), // relevant
t('node-values')
)
),
bind('/data/node').relevant('position(current()) > 2'),
bind('/data/node/value').type('int'),
bind('/data/node-values').calculate('concat(/data/node/value)')
)
),
body(group('/data/node', repeat('/data/node', input('/data/node/value'))))
)
);
it('is excluded from producing default values in an evaluation (supplemental to two previous tests)', async () => {
const scenario = await Scenario.init(
'Some form',
html(
head(
title('Some form'),
model(
mainInstance(
t(
'data id="some-form"',
// position() is one-based
t('node', t('value', '1')), // non-relevant
t('node', t('value', '2')), // non-relevant
t('node', t('value', '3')), // relevant
t('node', t('value', '4')), // relevant
t('node', t('value', '5')), // relevant
t('node-values')
)
),
bind('/data/node').relevant('position(current()) > 2'),
bind('/data/node/value').type('int'),
bind('/data/node-values').calculate('concat(/data/node/value)')
)
),
body(group('/data/node', repeat('/data/node', input('/data/node/value'))))
)
);

expect(scenario.answerOf('/data/node-values')).toEqualAnswer(stringAnswer('345'));
}
);
expect(scenario.answerOf('/data/node-values')).toEqualAnswer(stringAnswer('345'));
});

/**
* **PORTING NOTES** (supplemental: 3 of 3; see commit history for
Expand Down
68 changes: 11 additions & 57 deletions packages/scenario/test/repeat-relevant.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -394,64 +394,18 @@ describe('Interaction between `<repeat>` and `relevant`', () => {
/**
* **PORTING NOTES**
*
* 1. Liberty was taken to omit a position predicate on a non-repeat node
* in one of the ported assertions. While JavaRosa has a notion of
* normalizing expressions (e.g. comparing
* `/data/repeat[1]/non-repeat[1]` as equivalent to
* `/data/repeat[1]/non-repeat`), the web forms engine interface does
* not expose any such notion. This doesn't seem like an issue that
* most clients should be concerned with, so it seems most reasonable
* to port the assertion without requiring such a normalization. If
* that assumption turns out to be wrong, we can revisit in the future.
* The orignal assertion is commented directly above.
*
* 2. Failure anlaysis in some depth follows...
*
* - - -
*
* Fails with observed behavior that the second repeat instance's
* descendants never become relevant. As such, only the assertions before
* the value change fail.
*
* To keep the porting effort's momentum, I've generally tried to defer
* investigating any failure more deeply than gaining an understanding of
* the test case itself and hypothesizing the nature of the failure based
* on my understanding of engine behavior. However, this test was
* confusing enough (on first read) and its behavior surprising enough
* that I did a quick spike. Here's what I found (and some commentary):
*
* - The apparent root cause is that we are incorrectly evaluating repeat
* instance `relevant` expressions in the context of both the repeat
* instance itself (as expected) and its parent element (by mistake).
*
* - In a sense, this is a straightforward bug. It involves a clear (in
* hindsight) logical error. It can be fixed with a relatively
* straightforward special case.
*
* - In another sense, this is a symptom of a design flaw. There are
* likely several very similar issues lurking with essentially the same
* logical error (with at least one other which is clearly present in
* exactly the same method, just for another bind expression).
* Fundamentally, this bug (and others like it) are able to happen
* because of the impedance mismatch between the engine's structural
* model (wherein a "repeat range" is a thing that exists, and is
* treated as a node with the same hierarchical qualities as any other)
* and the engine's use of browser/XML DOM as an implementation of that.
*
* - This is an excellent example of the correctness implications of that
* latter implementation detail, and a good one to consider as we think
* about prioritization of an effort to move away from it.
*
* Note that even addressing the bug found above, this test would fail for
* another reason: we don't currently have a notion of determining
* relevance of a "repeat range" (and thus whether it should present a
* "prompt" event in JavaRosa/Scenario terms) by the relevance of its
* instances. There are other tests addressing this concern more directly,
* so it's likely we'll be able to target that issue with those. This is
* mostly meant as an observation that the two concerns intersect in this
* test even though it's not explicit.
* Liberty was taken to omit a position predicate on a non-repeat node
* in one of the ported assertions. While JavaRosa has a notion of
* normalizing expressions (e.g. comparing
* `/data/repeat[1]/non-repeat[1]` as equivalent to
* `/data/repeat[1]/non-repeat`), the web forms engine interface does
* not expose any such notion. This doesn't seem like an issue that
* most clients should be concerned with, so it seems most reasonable
* to port the assertion without requiring such a normalization. If
* that assumption turns out to be wrong, we can revisit in the future.
* The orignal assertion is commented directly above.
*/
it.fails('updates based on parent position', async () => {
it('updates based on parent position', async () => {
const scenario = await Scenario.init(
'Nested repeat relevance',
html(
Expand Down
25 changes: 25 additions & 0 deletions packages/xforms-engine/src/instance/RepeatInstance.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,31 @@ export class RepeatInstance
protected readonly state: SharedNodeState<RepeatInstanceStateSpec>;
protected override engineState: EngineState<RepeatInstanceStateSpec>;

/**
* @todo Should we special case repeat `readonly` inheritance the same way
* we do for `relevant`?
*
* @see {@link hasNonRelevantAncestor}
*/
declare readonly hasReadonlyAncestor: Accessor<boolean>;

/**
* A repeat instance can inherit non-relevance, just like any other node. That
* inheritance is derived from the repeat instance's parent node in the
* primary instance XML/DOM tree (and would be semantically expected to do so
* even if we move away from that implementation detail).
*
* Since {@link RepeatInstance.parent} is a {@link RepeatRange}, which is a
* runtime data model fiction that does not exist in that hierarchy, we pass
* this call through, allowing the {@link RepeatRange} to check the actual
* primary instance parent node's relevance state.
*
* @todo Should we apply similar reasoning in {@link hasReadonlyAncestor}?
*/
override readonly hasNonRelevantAncestor: Accessor<boolean> = () => {
return this.parent.hasNonRelevantAncestor();
};

// RepeatInstanceNode
readonly nodeType = 'repeat-instance';

Expand Down
88 changes: 83 additions & 5 deletions packages/xforms-engine/src/instance/RepeatRange.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { Accessor } from 'solid-js';
import type { RepeatRangeNode, RepeatRangeNodeAppearances } from '../client/RepeatRangeNode.ts';
import type { ChildrenState } from '../lib/reactivity/createChildrenState.ts';
import { createChildrenState } from '../lib/reactivity/createChildrenState.ts';
import { createComputedExpression } from '../lib/reactivity/createComputedExpression.ts';
import type { MaterializedChildren } from '../lib/reactivity/materializeCurrentStateChildren.ts';
import { materializeCurrentStateChildren } from '../lib/reactivity/materializeCurrentStateChildren.ts';
import type { CurrentState } from '../lib/reactivity/node-state/createCurrentState.ts';
Expand Down Expand Up @@ -64,6 +65,66 @@ export class RepeatRange
protected readonly state: SharedNodeState<RepeatRangeStateSpec>;
protected override engineState: EngineState<RepeatRangeStateSpec>;

/**
* @todo Should we special case repeat `readonly` state the same way
* we do for `relevant`?
*
* @see {@link isSelfRelevant}
*/
declare isSelfReadonly: Accessor<boolean>;

private readonly emptyRangeEvaluationContext: EvaluationContext & {
readonly contextNode: Comment;
};

/**
* @see {@link isSelfRelevant}
*/
private readonly isEmptyRangeSelfRelevant: Accessor<boolean>;

/**
* A repeat range does not exist in the primary instance tree. A `relevant`
* expression applies to each {@link RepeatInstance} child of the repeat
* range. Determining whether a repeat range itself "is relevant" isn't a
* concept the spec addresses, but it may be used by clients to determine
* whether to allow interaction with the range (e.g. by adding a repeat
* instance, or presenting the range's label when empty).
*
* As a naive first pass, it seems like the heuristic for this should be:
*
* 1. Does the repeat range have any repeat instance children?
*
* - If yes, go to 2.
* - If no, go to 3.
*
* 2. Does one or more of those children return `true` for the node's
* `relevant` expression (i.e. is the repeat instance "self relevant")?
*
* 3. Does the relevant expression return `true` for the repeat range itself
* (where, at least for now, the context of that evaluation would be the
* repeat range's {@link anchorNode} to ensure correct relative expressions
* resolve correctly)?
*
* @todo While (3) is proactively implemented, there isn't presently a test
* exercising it. It felt best for now to surface this for discussion in
* review to validate that it's going in the right direction.
*
* @todo While (2) **is actually tested**, the tests currently in place behave
* the same way with only the logic for (3), regardless of whether the repeat
* range actually has any repeat instance children. It's unclear (a) if that's
* a preferable simplification and (b) how that might affect performance (in
* theory it could vary depending on form structure and runtime state).
*/
override readonly isSelfRelevant: Accessor<boolean> = () => {
const instances = this.childrenState.getChildren();

if (instances.length > 0) {
return instances.some((instance) => instance.isSelfRelevant());
}

return this.isEmptyRangeSelfRelevant();
};

// RepeatRangeNode
readonly nodeType = 'repeat-range';

Expand Down Expand Up @@ -113,6 +174,28 @@ export class RepeatRange

this.childrenState = childrenState;

this.anchorNode = this.contextNode.ownerDocument.createComment(
`Begin repeat range: ${definition.nodeset}`
);
this.contextNode.append(this.anchorNode);

this.emptyRangeEvaluationContext = {
scope: this.scope,
evaluator: this.evaluator,
root: this.root,
contextReference: this.contextReference,
contextNode: this.anchorNode,

getSubscribableDependenciesByReference: (reference) => {
return this.getSubscribableDependenciesByReference(reference);
},
};

this.isEmptyRangeSelfRelevant = createComputedExpression(
this.emptyRangeEvaluationContext,
definition.bind.relevant
);

const state = createSharedNodeState(
this.scope,
{
Expand All @@ -132,11 +215,6 @@ export class RepeatRange
}
);

this.anchorNode = this.contextNode.ownerDocument.createComment(
`Begin repeat range: ${definition.nodeset}`
);
this.contextNode.append(this.anchorNode);

this.state = state;
this.engineState = state.engineState;
this.currentState = materializeCurrentStateChildren(
Expand Down

0 comments on commit b44beec

Please sign in to comment.