javarosa icon indicating copy to clipboard operation
javarosa copied to clipboard

Explore and document the null output scenario of TreeReference.parent()

Open ggalmazor opened this issue 7 years ago • 3 comments

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.

ggalmazor avatar Aug 14 '18 14:08 ggalmazor

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;
}

ggalmazor avatar Aug 17 '18 11:08 ggalmazor

With a small tweak we could '../../../foo'.parent('bar/baz') into '../foo', which is weird but feels like something that should be valid.

ggalmazor avatar Aug 17 '18 11:08 ggalmazor

Copied from https://github.com/opendatakit/javarosa/pull/350#issuecomment-415514714

For parent, we discussed that the null is 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 make parent and anchor even more similar).

ggalmazor avatar Aug 27 '18 11:08 ggalmazor