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.

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

Fixes to eXist-db regarding circular imports #4996

Merged

Conversation

adamretter
Copy link
Member

@adamretter adamretter commented Jul 28, 2023

These errors although mostly in eXist-db itself were easy to manifest through using RESTXQ.

Closes #3448
Closes #1010
Closes #4699

  1. The first thing that this PR deals with is a Library Module that attempts to import itself. Whilst BaseX raises an error, and Saxon allows it. With this PR eXist-db now follows Saxon in basically skipping the superfluous import and continues. This is inline with the W3C XQuery 3.1 specification which states:

    The namespace prefix specified in a module declaration must not be xml or xmlns [err:XQST0070], and must not be the same as any namespace prefix bound in the same module by a schema import, by a namespace declaration, or by a module import with a different target namespace [err:XQST0033]. 1

    and:

    A module may import its own target namespace (this is interpreted as importing an implementation-defined set of other modules that share its target namespace.) 2

  2. The second thing that this PR deals with is cyclic imports of Library Modules. i.e. M1 -> M2 -> M1. Before this PR an XQuery executed on eXist-db with a cyclic set of imports in its library modules would crash with a Java StackOverflowError. The expected behaviour has changed here between XQuery 1.0 and XQuery 3.1. In XQuery 1.0 cyclic imports were prohibited:

    It is a static error [err:XQST0093] to import a module M1 if there exists a sequence of modules M1 ... Mi ... M1 such that each module directly depends on the next module in the sequence (informally, if M1 depends on itself through some chain of module dependencies.) 3

    however, in XQuery 3.1 cyclic imports are allowed:

    Implementations must resolve cycles in the import graph, either at the level of target namespace URIs or at the level of location URIs, and ensure that each module is imported only once. 4

    In this PR, the XQuery version is now checked. If the XQuery is 1.0 then XQST0093 is raised, if the XQuery is newer (i.e. 3.0 or 3.1) then the cyclic import is resolved and allowed without issue.


This open source contribution to the eXist-db project was commissioned by the Office of the Historian, U.S. Department of State, https://history.state.gov/.

Footnotes

  1. https://www.w3.org/TR/xquery-31/#id-module-declaration

  2. https://www.w3.org/TR/xquery-31/#dt-module-import

  3. https://www.w3.org/TR/2010/REC-xquery-20101214/#id-module-import

  4. https://www.w3.org/TR/xquery-31/#id-module-handling-cycles

@adamretter adamretter added the bug issue confirmed as bug label Jul 28, 2023
@adamretter adamretter added this to the eXist-7.0.0 milestone Jul 28, 2023
@adamretter adamretter force-pushed the hotfix/restxq-circular-imports branch 2 times, most recently from 585cde5 to bb5c674 Compare July 29, 2023 16:09
@adamretter adamretter force-pushed the hotfix/restxq-circular-imports branch 3 times, most recently from e929dae to e731d3a Compare July 31, 2023 21:22
@adamretter adamretter marked this pull request as ready for review July 31, 2023 21:23
@joewiz
Copy link
Member

joewiz commented Aug 2, 2023

@adamretter To aid folks reviewing the PR, could you share your assessment of the CI failures and whether they are due to the problems in the PR or issues specific to the CI environment?

@adamretter adamretter force-pushed the hotfix/restxq-circular-imports branch 2 times, most recently from fc46628 to 82a58b4 Compare August 3, 2023 14:04
@adamretter
Copy link
Member Author

@joewiz All resolved now.

@duncdrum
Copy link
Contributor

duncdrum commented Aug 13, 2023

@adamretter looks like the license header checker doesn't like one of the license headers
https://github.com/eXist-db/exist/actions/runs/5751766777/job/15611509940?pr=4996#step:4:1871

Error:  Failed to execute goal com.mycila:license-maven-plugin:4.2:check (default-cli) on project exist-restxq: Some files do not have the expected license header -> [Help 1]

Could you please explain the effect of these changes to #4699, is it implementing switching off restxq by default and thereby introducing a breaking change?

@adamretter adamretter force-pushed the hotfix/restxq-circular-imports branch from 82a58b4 to 0d2a271 Compare August 13, 2023 14:49
@adamretter
Copy link
Member Author

looks like the license header checker doesn't like one of the license headers

@duncdrum I have fixed that now.

Could you please explain the effect of these changes to #4699

PR #4699 was never the correct approach and is now redundant due to the fixes in this PR. This PR does not break any backwards compatibility.

@sonarcloud
Copy link

sonarcloud bot commented Aug 13, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 10 Code Smells

75.7% 75.7% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@dizzzz
Copy link
Member

dizzzz commented Aug 30, 2023

image

@reinhapa
Copy link
Member

@adamretter could you resolve the existing conflicts in order to merge your're PR?

@adamretter adamretter force-pushed the hotfix/restxq-circular-imports branch from 0d2a271 to 78c34d2 Compare October 14, 2023 12:43
@adamretter
Copy link
Member Author

I have just rebased this now. @dizzzz @reinhapa I believe this is now ready to merge please

@sonarcloud
Copy link

sonarcloud bot commented Oct 14, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 11 Code Smells

75.7% 75.7% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@reinhapa reinhapa merged commit 6e80c9b into eXist-db:develop Oct 15, 2023
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issue confirmed as bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] RESTXQ infinite loop with circular dependency Recursion RestXQ: deploying web app fails
6 participants