javarosa
javarosa copied to clipboard
Explore and document the null output scenario of TreeReference.parent()
Given the following code example:
TreeReference someRef = (TreeReference) new XPathReference("../foo").getReference();
TreeReference baseRef = (TreeReference) new XPathReference("bar/baz").getReference();
TreeReference output = someRef.parent(baseRef);
Looking at the code, it's hard to understand why output would be null in this scenario. One could argue that bar/foo could be a reasonable output.
So, TreeReference.parent(TreeReference baseReference) has a doc block saying:
* @return a new TreeReference that represents this reference anchored to the parent or null if this ref has parent
* steps and the parent ref is not a relative reference consisting only of other 'parent' steps.
We should improve code comments and test cases to make this behavior clearer, or consider lifting this limitation.
There's something that I can't understand what purpose it serves in the TreeReference.parent() implementation:
public TreeReference parent(TreeReference baseReference) {
if (isAbsolute()) {
return this.clone();
} else {
TreeReference newRef = baseReference.clone();
if (refLevel > 0) {
if (!baseReference.isAbsolute() && baseReference.size() == 0) {
newRef.refLevel += refLevel;
} else {
return null;
}
}
for (TreeReferenceLevel level : data) {
newRef.add(level.shallowCopy());
}
return newRef;
}
}
(the if (refLevel > 0) block)
I'd want to understand this, because by removing it and fixing the levels like in anchor() we get the behavior we want:
public TreeReference parent(TreeReference baseReference) {
if (isAbsolute())
return this.clone();
TreeReference newRef = baseReference.clone();
for (int i = 0; i < refLevel; i++) {
newRef.removeLastLevel();
}
for (TreeReferenceLevel level : data) {
newRef.add(level.shallowCopy());
}
return newRef;
}
Also notice how this is almost the same as anchor's implementation:
public TreeReference anchor(TreeReference baseReference) throws XPathException {
if (isAbsolute()) {
return this.clone();
} else if (!baseReference.isAbsolute()) {
throw new XPathException(baseReference.toString(true) + " is not an absolute reference");
} else if (refLevel > baseReference.size()) {
throw new XPathException("Attempt to parent past the root node " + this.toString(true));
}
TreeReference newRef = baseReference.clone();
for (int i = 0; i < refLevel; i++) {
newRef.removeLastLevel();
}
for (TreeReferenceLevel level : data) {
newRef.add(level.shallowCopy());
}
return newRef;
}
With a small tweak we could '../../../foo'.parent('bar/baz') into '../foo', which is weird but feels like something that should be valid.
Copied from https://github.com/opendatakit/javarosa/pull/350#issuecomment-415514714
For
parent, we discussed that thenullis because of the limitation described in "However, if this ref has 'parent' steps (..), it can only be anchored if the base ref is a relative ref consisting only of other 'parent' steps. For example, '../../a'.parent('..') is valid and yields '../../../a'.". We can't figure out if that limitation is important but that can be investigated some other time. So #362 can either be closed or changed to make it about investigating whether the limitation can be lifted (which would makeparentandanchoreven more similar).