Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix boolean parameter casting #24

Merged
merged 3 commits into from
May 15, 2023
Merged

Fix boolean parameter casting #24

merged 3 commits into from
May 15, 2023

Conversation

joewiz
Copy link
Member

@joewiz joewiz commented May 13, 2023 •
edited
Loading

Before this PR, a function that declared a parameter as xs:boolean with a templating default value for this parameter was not cast to xs:boolean.

In eXist 7.0.0-SNAPSHOT, this led to an error in the function-documentation app. Specifically, app:module() function declares that $details parameter be xs:boolean, and the templating annotation supplies a default value for this parameter, but when the function passes this default value to the app:print-module function, an error is raised because the function's signature requires this parameter to be of type xs:boolean. Loading the landing page of the function documentation app raises the error shown below.

This PR fixes the templates:cast function to ensure that templating defaults are correctly cast to match their function's signature when the signature requires them to be xs:boolean.

The hint that this problem was present was https://github.com/eXist-db/function-documentation/pull/64/files#r1187748791.

err:XPTY0004 xs:string(false) is not a sub-type of xs:boolean [at line 111, column 53, source: /db/apps/fundocs/modules/app.xql]
In function:
	app:print-module(element(xqdoc:xqdoc), element(xqdoc:function)*, xs:boolean) [111:9:/db/apps/fundocs/modules/app.xql]
	app:module(node(), map(*), xs:boolean) [-1:-1:/db/apps/fundocs/modules/app.xql]
	templates:process-output(element(), map(*), item()*, element(function)) [211:9:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:call-by-introspection(element(), map(*), map(*), function(*)) [189:28:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:call(item(), element(), map(*)) [145:37:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:process(node()*, map(*)) [237:13:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:process(node()*, map(*)) [148:81:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:process(node()*, map(*)) [148:81:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:process(node()*, map(*)) [148:81:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:process(node()*, map(*)) [148:81:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:process(node()*, map(*)) [148:81:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:process(node()*, map(*)) [-1:-1:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	lib:parse-params(node(), map(*), xs:string?, xs:string?) [-1:-1:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/lib.xqm]
	templates:process-output(element(), map(*), item()*, element(function)) [211:9:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:call-by-introspection(element(), map(*), map(*), function(*)) [189:28:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:call(item(), element(), map(*)) [137:36:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:process(node()*, map(*)) [237:13:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:process-output(element(), map(*), item()*, element(function)) [211:9:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:call-by-introspection(element(), map(*), map(*), function(*)) [189:28:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:call(item(), element(), map(*)) [137:36:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:process(node()*, map(*)) [435:17:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:process-output(element(), map(*), item()*) [230:9:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:process-output(element(), map(*), item()*, element(function)) [211:9:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:call-by-introspection(element(), map(*), map(*), function(*)) [189:28:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:call(item(), element(), map(*)) [137:36:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:process(node()*, map(*)) [133:51:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:process(node()*, map(*)) [90:9:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]
	templates:apply(node()+, function(*), map(*)?, map(*)?) [24:5:/Users/joe/Library/Application Support/org.exist/expathrepo/templating-1.1.0/content/templates.xqm]

@joewiz joewiz mentioned this pull request May 13, 2023
Merged
@joewiz joewiz force-pushed the fix-boolean branch 2 times, most recently from 66d051b to 07465ff Compare May 13, 2023 19:32
@joewiz
Copy link
Member Author

joewiz commented May 13, 2023

While the CI passes on the tests of eXist 5.2.0, 5.4.1, and (in my local tests) release, the failure on latest is due to the change in error codes in eXist 7.0.0-SNAPSHOT (HEAD), which affects 3 tests here in the templating module.

Here's the patch that enables the tests to pass under 7.0.0-SNAPSHOT:

0001-eXist-7.0.0-SNAPSHOT-compatibility.patch

I do not know how to adjust the tests to be able to detect which matrix environment they're running under and to pass if the error code is 400 under eXist <7 and 500 under eXist >=7.

@joewiz joewiz mentioned this pull request May 13, 2023
Draft
@duncdrum
Copy link
Contributor

@joewiz i think you would need to adjust the test function to execute system:get-version make the comparison inside the function and use assertTrue as Xqs annotation. Something along these lines.

@joewiz
Handle <=6.2.0 vs. >= 7.0.0 error response codes
c988ff3 
eXist 6.2.0 and earlier issues an error 400 when the fn:error function is called. With 7.0.0-SNAPSHOT (eXist-db/exist#4649), eXist issues an error 500. The precise error code is outside the scope of this package - it’s an issue decided by the eXist API - so we can accept either here.
@joewiz
Copy link
Member Author

joewiz commented May 14, 2023

@duncdrum Thanks for the pointer! I did a little more research and settled on a solution that hopefully makes sense. I changed the error code check from to.equal(400) to to.be.oneOf([400, 500]), because, as I noted in the commit message just now:

eXist 6.2.0 and earlier issues an error 400 when the fn:error function is called. With 7.0.0-SNAPSHOT (eXist-db/exist#4649), eXist issues an error 500. The precise error code is outside the scope of this package - it’s an issue decided by the eXist API - so we can accept either here.

All tests now pass.

@joewiz joewiz requested a review from a team May 14, 2023 02:42
@joewiz joewiz added the bug Something isn't working label May 14, 2023
@duncdrum
Copy link
Contributor

This works too,

line-o
line-o reviewed May 15, 2023
View reviewed changes
Copy link
Member

@line-o line-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice fix @joewiz. I would still like to learn why a server error is an expected status code where it was only expecting a bad request before. Is this due to changes in existdb?

@line-o line-o self-requested a review May 15, 2023 10:43
line-o
line-o approved these changes May 15, 2023
View reviewed changes
Copy link
Member

@line-o line-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think this warrants a patch release.

@line-o line-o merged commit c0f6e4b into eXist-db:master May 15, 2023
@line-o line-o mentioned this pull request May 15, 2023
Open
5 tasks
@joewiz joewiz deleted the fix-boolean branch May 15, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@duncdrum duncdrum duncdrum approved these changes

@line-o line-o line-o approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joewiz @duncdrum @line-o