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

Allow module imports in one-off xqueries #5529

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Allow module imports in one-off xqueries #5529

wants to merge 1 commit into from

Conversation

line-o
Copy link
Member

@line-o line-o commented Oct 31, 2024 •
edited
Loading

Description:

  • add modified conf.xml ModuleImport tests
  • add tests for one-off queries with module imports
    • using a modified conf.xml that loads functx into package repo via auto deploy
    • of a registered module without location hint
    • of a module with location hint
  • change XQueryContext to allow imports again
  • change SourceFactory to work with contextPath set to "."

Reference:

fixes #5525
fixes #5530

Type of tests:

Java tests added in src/test/java/org/exist/xquery/ModuleImportTest.java

@line-o line-o requested a review from a team as a code owner October 31, 2024 10:40
adamretter
adamretter reviewed Oct 31, 2024
View reviewed changes
exist-core/src/main/java/org/exist/source/SourceFactory.java
@@ -111,7 +111,7 @@ public class SourceFactory {
&& ((location.startsWith("/db") && !Files.exists(Paths.get(firstPathSegment(location))))
|| (contextPath != null && contextPath.startsWith("/db") && !Files.exists(Paths.get(firstPathSegment(contextPath)))))) {
final XmldbURI pathUri;
if (contextPath == null) {
if (contextPath == null || ".".equals(contextPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to upset anyone, but I just wanted to point out that this is the wrong approach to fixing this issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is fixing an additional bug I found while creating tests:

see importLibraryFromDbLocation and importLibraryFromXMLDBLocation

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I understood that, but it still is the wrong approach.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I understood that, but it still is the wrong approach.

@adamretter Could you explain why you think this is the wrong approach and how do you think it should be solved correctly?

@line-o
Copy link
Member Author

line-o commented Oct 31, 2024

Adding the functx library to auto deploy in the conf.xml used for all tests leads to test failures.
There are, however, other configurations that do have functx loaded, see src/test/resources-filtered/org/exist/xquery/functions/transform/conf.xml.

Should I, either

  1. limit the application of the conf.xml with functx to only xquery tests
  2. limit the application of the conf.xml with functx to only moduleImport tests
  3. try to import other modules that are present without adding functx

@joewiz
Copy link
Member

joewiz commented Oct 31, 2024

If the issue isn't functx-specific, then choice 3 sounds like it would address the issue.

@adamretter
Copy link
Contributor

You should not have to modify conf.xml for this.

@line-o
Copy link
Member Author

line-o commented Nov 1, 2024 •
edited
Loading

@joewiz module imports are certainly not functx specific.

@line-o
[bugfix] allow module imports in one-off xqueries
a083775 
fixes eXist-db#5525

- add functx to autodeploy for xquery tests
- add tests for one-off queries with module imports
  - of a registered module without location hint
  - of a module with location hint
- change XQueryContext to allow imports again
- change SourceFactory to work with contextPath set to "."
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 1, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
66.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@line-o
Copy link
Member Author

line-o commented Nov 1, 2024 •
edited
Loading

I ended up using option 2, only use conf.xml with functx loaded via autodeploy for ModuleImportTest.

For the tests to work we need do need to add a pure library package to exist. It just happens that the functx-1.0.1.xar is already part of the test fixtures in
src/test/resources/functx/functx-1.0.1.xar.
With that we have a library module that can be imported with and without location hint.

So far as I can see there is no other way than to add another configuration with functx added to autodeploy.
We could change both src/test/java/org/exist/xquery/functions/transform/TransformFromPkgTest.java and src/test/java/org/exist/xquery/ModuleImportTest.java test classes to both use a conf-with-functx.xml to make its use-case more obvious and also re-usable.

@line-o line-o changed the title [bugfix] allow module imports in one-off xqueries Allow module imports in one-off xqueries Nov 5, 2024
reinhapa
reinhapa requested changes Nov 11, 2024
View reviewed changes
exist-core/src/test/java/org/exist/xquery/ModuleImportTest.java
}

@ClassRule
public static ExistEmbeddedServer server = new ExistEmbeddedServer(null, getConfigFile(), null, false, true);
Copy link
Member

Choose a reason for hiding this comment

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

Please move the field declaration to the top of the file to resolve this Codacy issue...

exist-core/src/test/resources-filtered/conf.xml
@@ -768,7 +777,7 @@
Set the parameter to "yes" to enable this feature.

-->
<serializer add-exist-id="none" compress-output="no" output-doctype="yes" enable-xinclude="yes"
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for the attribute output-doctype was being removed? Maybe by accident?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dizzzz dizzzz dizzzz left review comments

@adamretter adamretter adamretter left review comments

@reinhapa reinhapa reinhapa requested changes

Assignees
No one assigned
Labels
needs 6.x.x backport
Projects
v6.3.1 release
Status: In review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] one-off queries cannot import modules with location hint [BUG] 'other' is different type of Path
5 participants
@line-o @joewiz @adamretter @dizzzz @reinhapa