[4.x.x] Improve out-of-scope detection for binary variables

[adamretter]

This PR fixes some cases where xs:base64Binary value variables within an XQuery are cleared before they should be.

Diagnosis of the problem

If you have a function which returns a Map containing an xs:base64Binary (specifically the result of an EXPath HTTP Client response), and then you try and use that value in a calling function you will encounter an error where the temporary file backing the xs:base64Binary value has incorrectly already been deleted.

What is happening is:

1. The EXPath HTTP Client response constructs an xs:base64Binary value using org.exist.xquery.value.BinaryValueFromFile.
2. BinaryValueFromFile stores the binary data in a temporary file.
3. When the xs:base64BinaryValue is returned from the function inside a Map, eXist-db does not recognise it is inside a Map, and so incorrectly assumes it is no longer required and calls destroy on the variable; this is done to clean up variables that are no longer needed.
4. The destroy call on the variable, causes BinaryValueFromFile to delete the temporary file.
5. When we try and use the value in the Map returned by the function, the binary value cannot be read from the temporary file because it was deleted in the previous step.

Further work in Future

Ultimately, how eXist-db determines whether a Variable is still "in-scope" or "out-of-scope" within an XQuery is far too limited and does not match the semantics of the XQuery language specifications.
The current implementation of this in eXist-db is mostly within XQueryContext#markLocalVariables and XQueryContext#popLocalVariables.

Fixing this completely would require substantiate changes to both eXist-db's XQuery Parser and Interpreter.

Post-script

It is not possible to write a test for this with the way that eXist-db is currently structured. A test for this should be in the Core module. However, the only way I have found to easily trigger it is via the EXPath HTTP Client which is itself in Extension Modules. To introduce a dependency between Core and Extension Modules, when there is already a dependency between Extension Modules and Core would cause a cycle in the build graph (and the build would be rejected by Maven).

[reinhapa]

I have some minor suggestions... ;-)

[reinhapa]

I could imagine using a single boolean would remove the total amount of additional calls eventually:

boolean firstMapping = true;
for (final Mapping mapping : this.mappings) {
    mapping.key.dump(dumper);
    dumper.display(" := ");
    dumper.display(" : ");
    mapping.value.dump(dumper);
    if (firstMapping) {
        firstMapping = false;
    } else {
        dumper.display(", ");
    }
}

[adamretter]

If I am not mistaken you code will end up with a trailing , as it postfixes with the comma character, whereas my code prefixes with the comma character to ensure no "dangling" comma.

[reinhapa]

Sorry @adamretter , the if/else block should of course go prior the key/value rendering:

boolean firstMapping = true;
for (final Mapping mapping : this.mappings) {
    if (firstMapping) {
        firstMapping = false;
    } else {
        dumper.display(", ");
    }
    mapping.key.dump(dumper);
    dumper.display(" := ");
    dumper.display(" : ");
    mapping.value.dump(dumper);
}

[reinhapa]

I found some spots, that may need a second look....

[adamretter]

Thanks @reinhapa I have pushed an additional commit which I think addresses the issues that you found.