Skip to content

Conversation

rhubner
Copy link
Contributor

@rhubner rhubner commented Jul 13, 2022

This PR fixes #1848 by allowing the user to configure which modules should be loaded lazily.

It adds a new attribute load on the module element of conf.xml. If unspecified, modules are loaded automatically (i.e. eagerly) to maintain backwards compatibility with the previous eXist-db mode of operation.

The exist-distribution config file was updated to take advantage of this new feature. Only those modules that are deemed as required by eXist-db's XQSuite tests and Apps are loaded by default. Other modules can be loaded with import module namespace, for example:

import module namespace validation = "http://exist-db.org/xquery/validation";

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/.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

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

90.2% 90.2% Coverage
0.0% 0.0% Duplication

@adamretter adamretter added the enhancement new features, suggestions, etc. label Jul 19, 2022
@dizzzz
Copy link
Member

dizzzz commented Jul 26, 2022

Great work! Being non-native, I find the term "lazily" quite hard to remember, I think I see "lazy" more often, is shorter and IMO more clear whilst still correct?

String query =
"xquery version \"1.0\";\n" +
"declare namespace transform=\"http://exist-db.org/xquery/transform\";\n" +
"import module namespace transform=\"http://exist-db.org/xquery/transform\";\n" +
Copy link
Member

Choose a reason for hiding this comment

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

interesting change.....

@dizzzz
Copy link
Member

dizzzz commented Jul 26, 2022

Great work! @adamretter are we completely sure that the code is backwards compatible? This line might show something different? (not sure)

image

is this PR a v7 candidate ? will old code still work?

@dizzzz dizzzz requested a review from adamretter July 26, 2022 19:24
@adamretter
Copy link
Contributor

adamretter commented Aug 3, 2022

Great work! Being non-native, I find the term "lazily" quite hard to remember, I think I see "lazy" more often, is shorter and IMO more clear whilst still correct?

@dizzzz "lazily" is the adverb (modifies a verb), whilst "lazy" is the adjective (describes a state).

In this instance load="lazily" - so the verb is "load", and the adverb ("lazily") modifies that.

I think it makes sense, although for sure my English is not perfect either!

We can change it if you still want us to? Perhaps @joewiz has an opinion on this?

@joewiz
Copy link
Member

joewiz commented Aug 4, 2022

I see benefits in standardizing on "lazy". Now the code has instances of both "lazy" and "lazily" (but only "eager" - not "eagerly" - instead "always"). Standardizing on just "lazy" and "eager" (and doing away with "lazily" and "always") would probably prevent people mixing up the different verb forms/words on their code/configurations.

import module namespace test = "http://exist-db.org/xquery/xqsuite"
at "resource:org/exist/xquery/lib/xqsuite/xqsuite.xql";

import module namespace inspect = "http://exist-db.org/xquery/inspection";
Copy link
Member

Choose a reason for hiding this comment

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

This change is a clear hint that this constitutes a breaking change.
Library and main modules that use one of the functions in the inspect namespace will no longer function until they are imported.

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.

If I understand this PR correctly each module with load=lazy needs to imported explicitly in any module that wants to use them. Looking at the list where this is applied - like lucene, http-client, security-manager to name a few - this will create a lot of work on any library and app that is doing a little more than showing static content.
I believe we need to discuss the modules that are lazy-loaded by default.
For this to be merged into 6.x.x my answer would be: none.

@adamretter adamretter force-pushed the feature/lazy-module branch from 3c6c672 to fa3125c Compare October 5, 2022 20:07
Copy link
Contributor

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

LGTM

@adamretter
Copy link
Contributor

Standardizing on just "lazy" and "eager"

@joewiz We have now switched this to lazy and eager

@adamretter adamretter added this to the eXist-7.0.0 milestone Oct 5, 2022
@adamretter
Copy link
Contributor

This change is a clear hint that this constitutes a breaking change.

@line-o Sure, the target for this is eXist-db 7.0.0.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 5, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

91.4% 91.4% Coverage
0.0% 0.0% Duplication

@line-o
Copy link
Member

line-o commented Feb 21, 2023

Tests are failing

@adamretter adamretter force-pushed the feature/lazy-module branch from dba3da3 to 99fb337 Compare March 13, 2023 21:52
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

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

91.4% 91.4% Coverage
0.0% 0.0% Duplication

@duncdrum duncdrum added the needs documentation Signals issues or PRs that will require an update to the documentation repo label Apr 7, 2023
@duncdrum duncdrum self-requested a review April 7, 2023 20:09
Copy link
Contributor

@duncdrum duncdrum left a comment

Choose a reason for hiding this comment

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

We should not lazily omit the documentation that shows the new need for explicit import, that makes this a major release candidate.

Should also be rebased to get some fresh ci results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new features, suggestions, etc. needs documentation Signals issues or PRs that will require an update to the documentation repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt lazy loading model for module imports
6 participants