[feature] Allow Saxon licensed editions (EE and PE) #4854

alanpaxton · 2023-04-06T10:03:14Z

Description:

Add support for loading a Saxon configuration file from a location defined in eXist-db's conf.xml.
Refactor current code in eXist-db which uses Saxon, to use Saxon configuration and Saxon processor provided by the broker pool (single configuration and processor per broker pool)
Introduces a SaxonConfigurationHolder class used by the broker pool to search for and fetch the Saxon configuration referenced by conf.xml and load it into Saxon. The holder holds the saxon configuration lazily, loads it on first request, and retains the same configuration to respond to subsequent requests with.
Provide instructions for replacing the SaxonHE bundled in eXist with a licensed Saxon (EE or PE). The changes to configuration loading allow the user to configure their use of a licensed version of Saxon so that xquery functions such as fn:transform() (which uses Saxon as its transformer) can make use of EE and/or PE features.

Reference:

Type of tests:

Basic unit tests to verify the loading of Saxon configuration via the broker pool.
The manual procedure to use EE has been validated by installing a clean eXist from an installer Jar built from the feature branch, walking through the instructions and executing a fn:transform() script in eXide. Logging by the configuration holder of the result of querying capabilities of the resulting Saxon processor verified that the running version of Saxon provided all licensed features.

line-o

Nice Feature! Please have a look at the Codacy output and the exported names.
I also do wonder how this change affects the runtime of fn:transform.
I hope the constructor of this function does only run once and then reuses the static context and configuration within on each call. Is that the case?

exist-core/src/main/java/org/exist/util/SaxonConfigurationHolder.java

exist-core/src/main/java/org/exist/util/Configuration.java

line-o · 2023-04-06T11:29:02Z

@alanpaxton The commit messages do not conform to our commit message format. They must start with either [enhancement], [bugfix], [refactor] or [ignore].

line-o

Thanks for going the extra-mile @alanpaxton !

line-o · 2023-04-06T15:57:04Z

@alanpaxton Can you rename your commits to match the commit message format?

line-o · 2023-04-07T13:46:01Z

Having learned that it was possible to use Saxon PE and EE up until 6.0.1 I see this as a fix of a regression. I believe this warrants a backport to develop-6.x.x

adamretter · 2023-04-09T09:44:34Z

@line-o EB won't be backporting this at this time, but anyone else is free to do so

line-o · 2023-04-11T12:51:53Z

@alanpaxton I believe, @adamretter would like the work in progress commit bc8d9ce to be cleaned from the history before it gets merged.

adamretter

Just some minor issues, and one question about the design of the config holder class.

exist-core/src/test/resources-filtered/conf.xml

exist-distribution/Saxon-EE.md

exist-distribution/src/main/config/saxon-config.xml

exist-distribution/src/main/config/conf.xml

exist-core/src/main/java/org/exist/storage/BrokerPool.java

kosek · 2023-04-13T11:49:53Z

Because of a bug in Saxon 9.9 that has been fixed only for version 10+ it is not possible to integrate Saxon-PE, only Saxon-EE.

See corresponding Saxon bug: https://saxonica.plan.io/issues/4849

I'm not sure whether this bug will be ever fixed in 9.9. Are there any plans/estimates to use Saxon 10/11 in exist-db?

Set up to configure Saxon with a license for PE/EE if we find such a license, assume at that point that PE/EE has been manually installed. WIP - not fully tested, UT or manually, and we need manual testing after installing EE to confirm that we can get the features we want. [fix] Remove licence file stuff, comment code There is no point in us pushing a licence file into Saxon from a different place, because as soon as we ask Saxon to load a configuration it looks for a licence file. We will just ask to dump a licence file where Saxon expects it by default. Comment the methods in the configuration holder.

The saxon-config.xml file we supply does nothing, it just acts as a placeholder for a fully-realised saxon-config.xml when a licensed Saxon edition is installed.

Instructions in markdown for how to achieve this.

Saxon config tests that we find the saxon config file; the part about looking for a license file is not implemented.

De-genericize static names Don’t capitalize static method Remove unused imports Unpick method for complexity metric Re-order some qualifiers Make markdown conform to checker rules

We don’t need a map of broker pool to saxon config, I’m not sure why we made it like that in the first place. Broker pool just needs a (lazy) reference to its own saxon configuration (wrapper).

alanpaxton · 2023-04-25T10:12:18Z

Because of a bug in Saxon 9.9 that has been fixed only for version 10+ it is not possible to integrate Saxon-PE, only Saxon-EE.

See corresponding Saxon bug: https://saxonica.plan.io/issues/4849

I'm not sure whether this bug will be ever fixed in 9.9. Are there any plans/estimates to use Saxon 10/11 in exist-db?

I had a look at upgrading to 11 when I started this, but saxon has changed the regex code which we fork as a separate repo and include in exist, so there's some quantity more work involved to update and resolve that conflict.

adamretter

Thanks for addressing the review comments - LGTM :-)

sonarcloud · 2023-04-28T14:57:28Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
10 Code Smells

66.4% Coverage
0.0% Duplication

adamretter · 2023-05-04T21:34:42Z

@line-o Can this be merged now then?

line-o requested changes Apr 6, 2023

View reviewed changes

line-o added enhancement new features, suggestions, etc. needs documentation Signals issues or PRs that will require an update to the documentation repo labels Apr 6, 2023

duncdrum added this to the eXist-7.0.0 milestone Apr 6, 2023

line-o approved these changes Apr 6, 2023

View reviewed changes

line-o self-requested a review April 6, 2023 15:57

alanpaxton changed the title ~~Feature/eb 4813 allow Saxon licensed editions (EE and PE)~~ [feature] Allow Saxon licensed editions (EE and PE) Apr 6, 2023

alanpaxton force-pushed the feature/eb-4813-allow-saxon-licensed-editions branch from 2824238 to 71955d4 Compare April 6, 2023 16:40

line-o approved these changes Apr 6, 2023

View reviewed changes

adamretter self-requested a review April 7, 2023 14:08

adamretter requested changes Apr 11, 2023

View reviewed changes

alanpaxton added 8 commits April 24, 2023 15:53

[fix] Link to and polish the placeholder saxon config.

cb05194

The saxon-config.xml file we supply does nothing, it just acts as a placeholder for a fully-realised saxon-config.xml when a licensed Saxon edition is installed.

[test] Document and validate HE to EE upgrade procedure.

d1ea717

Instructions in markdown for how to achieve this.

[refactor] Rename, add licence header, remove license testing

77b0c1a

Saxon config tests that we find the saxon config file; the part about looking for a license file is not implemented.

[refactor] Review feedback (human and automatic)

866b63d

De-genericize static names Don’t capitalize static method Remove unused imports Unpick method for complexity metric Re-order some qualifiers Make markdown conform to checker rules

[refactor] how broker pool holds saxon config

c3c82de

We don’t need a map of broker pool to saxon config, I’m not sure why we made it like that in the first place. Broker pool just needs a (lazy) reference to its own saxon configuration (wrapper).

[ignore] add some comment text, fix eXist-DB case

4170f09

[ignore] documentation should go to doc repo

9772b00

alanpaxton force-pushed the feature/eb-4813-allow-saxon-licensed-editions branch from 71955d4 to 9772b00 Compare April 24, 2023 14:56

[ignore] further clarifying comments

fd89760

adamretter self-requested a review April 28, 2023 13:45

[refactor] Minor cleanup

3a4b2fe

adamretter added 4 commits April 28, 2023 16:07

[bugfix] Use the correct Logger implementation

09f9ec3

[refactor] Use Java NIO instead of IO

22524de

[refactor] Use isEmpty rather than size() == 0

28e1d47

[refactor] Make variables and parameters where possible

8da4669

adamretter approved these changes Apr 28, 2023

View reviewed changes

adamretter merged commit ba68294 into eXist-db:develop May 10, 2023
7 of 12 checks passed

adamretter mentioned this pull request May 12, 2023

[feature] Allow usage of Saxon PE/EE #4813

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] Allow Saxon licensed editions (EE and PE) #4854

[feature] Allow Saxon licensed editions (EE and PE) #4854

alanpaxton commented Apr 6, 2023 •

edited by adamretter

Loading

line-o left a comment

line-o commented Apr 6, 2023

line-o left a comment

line-o commented Apr 6, 2023

line-o commented Apr 7, 2023

adamretter commented Apr 9, 2023 •

edited

Loading

line-o commented Apr 11, 2023

adamretter left a comment

kosek commented Apr 13, 2023

alanpaxton commented Apr 25, 2023

adamretter left a comment

sonarcloud bot commented Apr 28, 2023

adamretter commented May 4, 2023

[feature] Allow Saxon licensed editions (EE and PE) #4854

[feature] Allow Saxon licensed editions (EE and PE) #4854

Conversation

alanpaxton commented Apr 6, 2023 • edited by adamretter Loading

Description:

Reference:

Type of tests:

line-o left a comment

Choose a reason for hiding this comment

line-o commented Apr 6, 2023

line-o left a comment

Choose a reason for hiding this comment

line-o commented Apr 6, 2023

line-o commented Apr 7, 2023

adamretter commented Apr 9, 2023 • edited Loading

line-o commented Apr 11, 2023

adamretter left a comment

Choose a reason for hiding this comment

kosek commented Apr 13, 2023

alanpaxton commented Apr 25, 2023

adamretter left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Apr 28, 2023

adamretter commented May 4, 2023

alanpaxton commented Apr 6, 2023 •

edited by adamretter

Loading

adamretter commented Apr 9, 2023 •

edited

Loading