${} references in predicates not used as choice filters should be expanded using current()

[lognaturel]

Software and hardware versions

All versions

Problem description

It's useful to manually write XPath expressions with predicates and it can be convenient to mix in ${} references. For example, consider this form which looks up items in a repeat. The expression instance('item')/root/item[itemindex=current()/../item-counter]/label would be convenient to write as instance('item')/root/item[itemindex=${item-counter}]/label. However, this would currently be expanded to instance('item')/root/item[itemindex=../item-counter]/label which is incorrect.

Steps to reproduce the problem

Convert a form such as the one linked above. Notice that the expression in the predicate does not use current().

Expected behavior

All ${} expressions in XPath predicates should expand using current(). Alternately we could document that we don't allow using ${} references in XPath predicates.

[MartijnR]

I am aware of one group of users (OpenClinica) that would benefit from the proposed feature. Generally, useful for those people that use complex external data structures.

[pbowen-oc]

@gushil - Can you work on this next?

[pbowen-oc]

@MartijnR @lognaturel - Is it as simple as adding current()/ before these references as they are generated now or does it also need to take into account the hierarchy between the item containing this logic and the item being referenced?

[lognaturel]

Would like @MartijnR to chime in as well but I believe that if we detect a ${} reference in square brackets, we can always do like you said and prepend the current expansion with current()/. The hierarchy should be taken care of by the context. I think the tricky part is figuring out the least hacky/most performant way to identify that we're expanding a reference in a predicate.

[MartijnR]

It looks like it does already do the relative paths correctly, so hopefully prepending current()/ would do the trick. Would need (a) test(s) for more complex steps down and up into groups.

I think the tricky part is figuring out the least hacky/most performant way to identify that we're expanding a reference in a predicate.

and that the context of this predicate is not part of the primary instance (and e.g. not creating a choice list from previously answered repeat questions). That is probably reliably done by looking for instance(..) at the start of the expression.

[lognaturel]

context of this predicate is not part of the primary instance

YES. Very important.

[gushil]

@gushil - Can you work on this next?

I'm on it.

[gushil]

@MartijnR @lognaturel

I tried using this form :

• Changed select one stock into select one item
• Still got instance('item')/root/item[itemindex=current()/../item-counter]/label not instance('item')/root/item[itemindex=../item-counter]/label like described in the description.

Anything I missed?

[gushil]

@MartijnR @lognaturel Bump this, in case the notification missed.

Thanks!

[lognaturel]

Did you change instance('item')/root/item[itemindex=current()/../item-counter]/label to instance('item')/root/item[itemindex=${item-counter}]/label?

[gushil]

Did you change instance('item')/root/item[itemindex=current()/../item-counter]/label to instance('item')/root/item[itemindex=${item-counter}]/label?

I haven't do that. Should I?

[lognaturel]

Yes, sorry that wasn’t clear. The linked form works but only because I explicitly wrote current() expressions. That’s not great because it’s hard to keep track of where we do and don’t need to be explicit. Additionally, being able to use ${} expressions means we don’t have to worry about nesting. The raw expressions are brittle in the sense that if I nest them in an additional group or repeat, they just silently fail. This can be pretty hard to troubleshoot.

[MartijnR]

I haven't do that. Should I?

Yes ;). This bug is about translating ${node} syntax in an XPath expression when used inside square brackets. We're trying to make it easier for people to query complex data so they don't have to think about when to use current() and which path steps down and up to take.

[MartijnR]

Oops. Our messages crossed @lognaturel!

[gushil]

@MartijnR @lognaturel

I've created PR #524 that hopefully can fix this issue.

Open to more suggestion, especially with test(s).

Thanks.

[MartijnR]

Additional tests for this feature:

A

type	name	calculation	label
xml-external	item
begin repeat	rep3
calculate	pos3	position(..)
begin group	grp3
text	txt3		Enter
calculate	item3	instance('item')/root/item[index=${pos3}]/label
end group
end repeat

expected output: instance('item')/root/item[index= current()/../../pos3 ]/label. This passes in PR.

B

type	name	calculation	label
xml-external	item
calculate	pos1	position(..)
begin repeat	rep5
--	--	--	--
calculate	pos5	position(..)
calculate	item5	instance('item')/root/item[index=${pos5} and ${pos1} = 1]/label
end repeat

expected output: calculate="instance('item')/root/item[index= current()/../pos5 and /data/pos1 = 1]/label"

C

I don't know if existing tests already cover this, but if not clear, it won't hurt to add these (with no changed behavior):

type	name	calculation
xml-external	item
calculate	pos1	position(..)
calculate	item1	instance('item')/root/item[index=${pos1}]/label

expected output: calculate="instance('item')/root/item[index= /data/pos1 ]/label". This passes in PR.

D

I don't know if existing tests already cover this, but if not clear, it won't hurt to add these (with no changed behavior):

type	name	calculation
xml-external	item
begin repeat	rep4
calculate	pos4	1
calculate	item4	/data/rep2[number(${pos4})]/item2
end repeat

expected output: calculate="/data/rep2[number( ../pos4 )]/item2". This passes in PR.