Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xpath: [incorrect] support for indexed-repeat function #150

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/four-bats-kick.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@getodk/scenario": patch
"@getodk/xforms-engine": patch
"@getodk/xpath": patch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: new functionality feels like minor? Not a big deal either way!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. I was partly basing this on precedent in 1ac6b10, which surprised me but seemed to be oriented around targeting some set of functionality for certain 0.x milestones. Not sure if that was the right inference, or if it's still pertinent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I see! I think I was thinking of the API addition as internal but it’s not, really. So that one should have been minor for the engine and dependents?

This one feels unambiguous to me because it’s introducing user-facing functionality.

---

xpath: support for `indexed-repeat` function
93 changes: 86 additions & 7 deletions packages/scenario/src/jr/Scenario.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ interface CreateNewRepeatAssertedReferenceOptions {
readonly assertCurrentReference: string;
}

interface SetPositionalStateOptions {
readonly createMissingRepeatInstances?: boolean;
}

// prettier-ignore
type GetQuestionAtIndexParameters<
ExpectedQuestionType extends QuestionNodeType
Expand Down Expand Up @@ -140,7 +144,8 @@ export class Scenario {
readonly formName: string;
readonly instanceRoot: RootNode;

private readonly getPositionalEvents: Accessor<PositionalEvents>;
protected readonly getPositionalEvents: Accessor<PositionalEvents>;
protected readonly getEventPosition: Accessor<number>;
private readonly setEventPosition: Setter<number>;

protected readonly getSelectedPositionalEvent: Accessor<AnyPositionalEvent>;
Expand All @@ -151,14 +156,15 @@ export class Scenario {
this.formName = formName;
this.instanceRoot = instanceRoot;

const [eventPosition, setEventPosition] = createSignal(0);
const [getEventPosition, setEventPosition] = createSignal(0);

this.getPositionalEvents = () => getPositionalEvents(instanceRoot);
this.getEventPosition = getEventPosition;
this.setEventPosition = setEventPosition;

this.getSelectedPositionalEvent = createMemo(() => {
const events = getPositionalEvents(instanceRoot);
const position = eventPosition();
const position = getEventPosition();
const event = events[position];

if (event == null) {
Expand Down Expand Up @@ -288,13 +294,73 @@ export class Scenario {
return this.setNonTerminalEventPosition(increment, expectReference);
}

private setPositionalStateToReference(reference: string): AnyPositionalEvent {
private createMissingRepeatInstances(reference: string): void {
let tempReference = reference;
let indexedReference: string | null = null;

const trailingPositionalPredicatePattern = /\[\d+\]$/;

do {
if (trailingPositionalPredicatePattern.test(tempReference)) {
indexedReference = tempReference;
} else {
tempReference = tempReference.replace(/\/[^/]+$/, '');

if (tempReference === '') {
break;
}
}
} while (indexedReference == null);

if (indexedReference == null) {
return;
}

const repeatRangeReference = indexedReference.replace(trailingPositionalPredicatePattern, '');

const positionalPredicate = indexedReference.replace(/^.*\[(\d+)\]$/, '$1');
const count = parseInt(positionalPredicate, 10);

if (count < 1) {
return;
}

const repeatRange = this.getInstanceNode(repeatRangeReference);

if (repeatRange.nodeType !== 'repeat-range') {
throw 'todo';
}

const repeatBodyDefinition = repeatRange.definition.bodyElement;

if (repeatBodyDefinition.countExpression != null || repeatBodyDefinition.isFixedCount) {
return;
}

const instances = repeatRange.currentState.children.length;
const delta = count - instances;

for (let i = 0; i < delta; i += 1) {
this.createNewRepeat(repeatRangeReference);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The algorithm here is something like find the last positional predicate, strip it off, use that reference to find the repeat range, add instances up to the position accessed, right? So it won't work for nested repeats that all have missing instances (e.g. /data/repeat1[2]/repeat2[5]/foo)? I think maybe there are tests that use that in JR but I'm not entirely sure. Fine to leave as-is for now but wanted to check my understanding.

I don't think this requires immediate actions either but I wanted to mention that I expected some slightly higher-level building blocks here! Specifically, I'm surprised to see some regex for getting references without predicates and for getting sub references. I imagine at some point it will be worth having some reusable support for those concepts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment about building blocks is partially addressed at 66aa8c9#diff-f3ac6c9869393022cd23375d383ded81f1d35c2c962125846cb288792ee28d52R15

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been mulling this since I saw these comments yesterday, and my instinct is that we're probably better off not porting this aspect of Scenario.

  • The tests I've encountered which assume this behavior have frequently surprised and confused me. And not in a trivial way: almost every time, I've misinterpreted the nature of a failure, or misunderstood the intent of the test, sending me off on unproductive tangents.

  • Tests which expect this level of logical complexity should probably have their own logic tested as well. For instance, I was already deeply uncomfortable with the level of complexity in child-vaccination. There is significant application-like work happening there, and I find it vastly harder to understand the test flow than any of the tests which more explicitly express what they are doing.

  • I should say that I regret the linked comment. In hindsight, the kind of expression analysis and manipulation that I discuss there, and that you say you were expecting here, is definitely appropriate for engine implementation details. I am quite cautious about using logic of that kind to implement test behavior.

  • On Tuesday we discussed the fact that such logic is in fact incoming, supporting a variety of other engine improvements. And as we discussed then, that logic is downstream from this change, as other affected tests would currently depend on this specific implicit repeat instance creation. But to be clear, as that work took form, it felt pretty clear that the expression analysis/manipulation logic will be entirely encapsulated in the engine. If we do anticipate exposing arbitrarily/increasingly complex expression analysis and manipulation as a public API, I think we should really think hard about what the appropriate design for that would be. And the appropriate package and/or abstraction layer from which we should expose it.

I already wanted to make repeat instance creation more explicit before this discussion, but I hesitated in this PR on the basis that it would increase divergence from JavaRosa. But that divergence is much more appealing to me now, in light of some of the implications this has raised.

Copy link
Member

@lognaturel lognaturel Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought your linked comment meant in the engine. Agreed it’s the engine that eventually should have that functionality so good to see it’s coming. 👍

As for not porting the implicit repeat addition, then I think we should probably change the JR tests. I don’t know off the top of my head how many are affected and will look.

}

private setPositionalStateToReference(
reference: string,
options: SetPositionalStateOptions = {}
): AnyPositionalEvent {
const events = this.getPositionalEvents();
const index = events.findIndex(({ node }) => {
return node?.currentState.reference === reference;
});

if (index === -1) {
if (options?.createMissingRepeatInstances) {
this.createMissingRepeatInstances(reference);

return this.setPositionalStateToReference(reference);
}

throw new Error(
`Setting answer to ${reference} failed: could not locate question/positional event with that reference.`
);
Expand All @@ -304,7 +370,9 @@ export class Scenario {
}

private answerSelect(reference: string, ...selectionValues: string[]): ComparableAnswer {
const event = this.setPositionalStateToReference(reference);
const event = this.setPositionalStateToReference(reference, {
createMissingRepeatInstances: true,
});

if (!isQuestionEventOfType(event, 'select')) {
throw new Error(
Expand All @@ -315,6 +383,15 @@ export class Scenario {
return event.answerQuestion(new SelectValuesAnswer(selectionValues));
}

/**
* **PORTING NOTES**
*
* Per JavaRosa:
*
* > This method has side effects:
* > - It will create all the required middle and end repeat group instances
* > - It changes the current form index
*/
answer(...args: AnswerParameters): unknown {
if (isAnswerSelectParams(args)) {
return this.answerSelect(...args);
Expand All @@ -335,7 +412,9 @@ export class Scenario {
} else if (typeof arg0 === 'string') {
const reference = arg0;

event = this.setPositionalStateToReference(reference);
event = this.setPositionalStateToReference(reference, {
createMissingRepeatInstances: true,
});
value = arg1;
} else {
throw new Error('Unsupported `answer` overload call');
Expand Down Expand Up @@ -371,7 +450,7 @@ export class Scenario {
const node = getNodeForReference(this.instanceRoot, reference);

if (node == null) {
throw new Error(`No "answer" node for reference: ${reference}`);
throw new Error(`No instance node for reference: ${reference}`);
}

return node;
Expand Down
20 changes: 14 additions & 6 deletions packages/scenario/test/repeat.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2262,15 +2262,18 @@ describe('Tests ported from JavaRosa - repeats', () => {
/**
* **PORTING NOTES**
*
* - Fails pending implementation of `indexed-repeat` XPath function.
*
* - Parameters adapted to match values in JavaRosa. Note that the
* parameters are passed as {@link options} rather than destructured. Java
* lets you reference `group` (the class property) and `group` (the
* imported static method) in the same scope. TypeScript/JavaScript don't
* let you do that... which is fine, because doing that is really weird!
*
* - `answer` calls updated to omit superfluous position predicate on
* the non-repeat `some-group` step (we do this lookup by `reference`,
* not evaluating arbitrary XPath expressions to identify the question
* being answered).
*/
it.fails.each<IndexedRepeatRelativeRefsOptions>(parameters)('$testName', async (options) => {
it.each<IndexedRepeatRelativeRefsOptions>(parameters)('$testName', async (options) => {
const scenario = await Scenario.init(
'Some form',
html(
Expand Down Expand Up @@ -2311,9 +2314,14 @@ describe('Tests ported from JavaRosa - repeats', () => {
)
);

scenario.answer('/data/some-group[1]/item[1]/value', 11);
scenario.answer('/data/some-group[1]/item[2]/value', 22);
scenario.answer('/data/some-group[1]/item[3]/value', 33);
// scenario.answer('/data/some-group[1]/item[1]/value', 11);
scenario.answer('/data/some-group/item[1]/value', 11);

// scenario.answer('/data/some-group[1]/item[2]/value', 22);
scenario.answer('/data/some-group/item[2]/value', 22);

// scenario.answer('/data/some-group[1]/item[3]/value', 33);
scenario.answer('/data/some-group/item[3]/value', 33);

expect(scenario.answerOf('/data/total-items')).toEqualAnswer(intAnswer(3));
expect(scenario.answerOf('/data/some-group/last-value')).toEqualAnswer(intAnswer(33));
Expand Down
13 changes: 3 additions & 10 deletions packages/scenario/test/select.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -376,16 +376,8 @@ describe('DynamicSelectUpdateTest.java', () => {
});
});

/**
* **PORTING NOTES**
*
* This currently fails because repeat-based itemsets are broken more
* generally. As with the above sub-suite, the last assertion is a reference
* check and will always pass. Once repeat-based itemsets are fixed, we'll
* want to consider whether this test should be implemented differently too.
*/
describe('select with repeat as trigger', () => {
it.fails('recomputes [the] choice list at every request', async () => {
it('recomputes [the] choice list at every request', async () => {
const scenario = await Scenario.init(
'Select with repeat trigger',
html(
Expand Down Expand Up @@ -417,7 +409,8 @@ describe('DynamicSelectUpdateTest.java', () => {

expect(choices.size()).toBe(2);

// Because of the repeat trigger in the count expression, choices should be recomputed every time they're requested
// JR: Because of the repeat trigger in the count expression, choices
// should be recomputed every time they're requested
expect(scenario.choicesOf('/data/select')).not.toBe(choices);
});
});
Expand Down
Loading