Fix detection of XQuery errors thrown by XQueryServlet #284
Conversation
A subtle change between eXist 5.2.0 and 5.3.0 meant that the fix in eXist-db#261 caused every eXide query to raise an error for users of eXist 5.2.0 but not for those of 5.3.0. eXist-db#261 introduced a check of request:get-data() to detect errors thrown by the XQueryServlet that lacked descriptions and had silently failed in earlier versions of eXide. In eXist 5.2.0, the call to request:get-data() here returns an empty string when there is no XQueryServlet error. Under eXist 5.3.0-SNAPSHOT, the call to request:get-data here returns an empty sequence. This commit makes eXide’s error reporting introduced in eXist-db#261 impervious to this difference between eXist 5.2.0 and 5.3.0 and adds some comments to aid in maintenance.
| if (string-length($xqueryservlet-error) gt 0) then | ||
| $xqueryservlet-error | ||
| ( | ||
| if ($xqueryservlet-error instance of document-node()) then |
There was a problem hiding this comment.
I would combine the to conditions into one
if (
$xqueryservlet-error instance of document-node() and
$xqueryservlet-error/error => normalize-space() => string-length()
)I also left out the "gt 0" because any integer greater zero evals to true
There was a problem hiding this comment.
Ah, but request:get-data() can return a string, in which case request:get-data()/error would throw an error.
There was a problem hiding this comment.
If the first condition of the two is false the second is not evaluated
There was a problem hiding this comment.
That’s implementation-dependent behavior, which I avoid without a compelling reason. From https://www.w3.org/TR/xquery-31/#id-logical-expressions:
The order in which the operands of a logical expression are evaluated is implementation-dependent. The tables above are defined in such a way that an or-expression can return true if the first expression evaluated is true, and it can raise an error if evaluation of the first expression raises an error. Similarly, an and-expression can return false if the first expression evaluated is false, and it can raise an error if evaluation of the first expression raises an error.
There was a problem hiding this comment.
(If that is an eXist optimization, it’s not documented, is it? If not, it should perhaps be noted in https://exist-db.org/exist/apps/doc/xquery and/or https://exist-db.org/exist/apps/doc/tuning.)
| if ($xqueryservlet-error instance of document-node()) then | ||
| if (string-length(normalize-space($xqueryservlet-error/error)) gt 0) then | ||
| $xqueryservlet-error | ||
| else |
There was a problem hiding this comment.
else if ($xqueryservlet-error instance of document-node()) then
| description is null), so the <error> element is empty. To aid the user | ||
| in such situations, the script reports that an unidentified error was | ||
| raised. It also points the user to exist.log, where a hopefully a stack | ||
| trace shows the full error. | ||
| :) | ||
| session:create(), | ||
| let $xqueryservlet-error := request:get-data() |
There was a problem hiding this comment.
Maybe add a new variable here
let $is-error := $xqueryservlet-error instance of document-node()
| @@ -19,14 +19,14 @@ | |||
| xquery version "3.0"; | |||
There was a problem hiding this comment.
I see and understand your reasons for nested if-statements.
Especially given that it fixes an issue for users of the stable release, I would rather merge it now than to discuss this longer.
One question still is nagging me: Why is the the variable called $xqueryservlet-error ?
Can request:get-data() only contain an error? If not, I would like to avoid future confusion by giving it a neutral name like $data or $data-or-error or so.
What do you think, @joewiz ?
|
@line-o Ok, sounds good. Yes, for session.xql, which is only called by controller.xql to store and retrieve query results, a call to |
A subtle change between eXist 5.2.0 and 5.3.0 meant that the fix in #261 caused eXide to raise an error for all queries for users of eXist 5.2.0 but not for those of 5.3.0.
This commit makes eXide’s error reporting introduced in #261 impervious to this difference between eXist 5.2.0 and 5.3.0 and adds some comments to aid in maintenance.
Background: #261 introduced additional detection of errors thrown by the XQueryServlet. Previously, errors that lacked descriptions slipped through unreported. The additional error detection worked in eXist 5.3.0-SNAPSHOT, but caused a serious regression in eXist 5.2.0. I found a difference in the results returned by the call to
request:get-data()here; in eXist 5.2.0, this function returns an empty string when there is no XQueryServlet error, but in eXist 5.3.0-SNAPSHOT, the same call torequest:get-data()returns an empty sequence.To clarify my assertion that a subtle change between 5.2.0 and 5.3.0 played a role in this issue, I couldn't reproduce any difference in
request:get-data()outside of eXide, so it doesn't appear thatrequest:get-data()itself changed between 5.2.0 and 5.3.0. Rather, something about the particular context this function operates in did change—the context being the particular way that eXide sends queries to XQueryServlet for execution and processes the results. For example,exists(request:get-data())returnstrue()in 5.2.0 andfalse()in 5.3.0—when run from eXide—but this query always returnsfalse()when executed via a saved query (i.e.,http://localhost:8080/exist/rest/db/test.xq). Why would we gettrue()in eXide under 5.2.0 but not 5.3.0? Do we know of any changes that could be responsible? I'm not complaining about 5.3.0; I think its behavior is correct here.Thanks to @line-o for reporting the incompatibility between #261 and eXist 5.2.0.
Aside: To avoid the quirks of XQueryServlet's error handling, we should switch query execution to util:eval wrapped in a try-catch block. For an example of such an approach, see: