Align DOM implementations with org.w3c.dom #5740

line-o · 2025-05-14T11:24:34Z

Description:

This PR fixes inconsistencies in

org.exist.dom.memtree.ElementImpl
org.exist.dom.persistent.ElementImpl

with org.w3c.dom.ElementImpl

Because of that a lot of code throughout exist-db core and its extensions needed to be adapted.
So this is definitely a breaking change that developers of Java extensions need to take into account.

On the plus side this also enables us to get rid of a lot of extra checks that were necessary before.
As a result the dependency on apache.commons.lang3.StringUtils could be dropped in several modules.

Reference:

fixes #5672

Type of tests:

None added.

reinhapa

some parts could be converted to switch expressions

extensions/modules/mail/src/main/java/org/exist/xquery/modules/mail/MessageListFunctions.java

adamretter · 2025-05-15T00:04:28Z

Interestingly, this work looks very similar to the previous work done in Elemental on March 17 2025, see here - evolvedbinary/elemental@f8a2781#diff-5b0c3e472fee91090782d0d1535d50f290ac6f450e8fbe68b30c9dcfad1b6505

I have no problem with you (or anyone else) taking code from Elemental, as long as you follow the license terms. That particular code in Elemental is partly under the LGPL 2.1 ONLY license with an 'Evolved Binary' Copyright. This is not the same license used by eXist-db (which is straight LGPL 2.1 (without the ONLY clause)), and it is not the same Copyright.

If you wish to reuse Elemental code in eXist-db, please do, but you must follow the terms of the LGPL, so as I understand it, at a minimum, you would need to include our license header too in each file where you use our code, and a statement from yourselves if you have made any modifications to our code. You may also need to seek guidance on combining 'LGPL 2.1 ONLY' and 'LGPL 2.1' code as I suspect the outcome of that product is 'LGPL 2.1 ONLY' which is not the license that eXist-db currently claims.

line-o · 2025-05-15T06:21:30Z

The license changes in the referenced commit are questionable and I did the changes on my own.
Could you be more specific, @adamretter. Which parts do you allege were copied from the referenced commit?

reinhapa · 2025-05-15T08:32:02Z

@adamretter I would be very careful pointing to different licenses and accusing @line-o to have copied from Elemental here.

Changing the implementation in order to correctly implement an external defined API will naturally result in similar changes that might also exist in Elemental.
Also: How comes that Elemental code contains eXist Source but not the GIT history that lead to it?

adamretter · 2025-05-15T21:40:54Z

The license changes in the referenced commit are questionable

@line-o I don't yet understand what you mean by "questionable". Do you have questions about them? or do you believe there is a problem with our approach to licensing? We have worked hard to make sure we have followed and complied with the terms of the LGPL 2.1 license. If you believe that there is an issue with our licensing, please feel free to open an issue at https://github.com/evolvedbinary/elemental/issues with the specific details of your concerns in reference to the LGPL license terms.

Which parts do you allege were copied from the referenced commit?

I am not 'alleging' anything, I am simply making an observation.

adamretter · 2025-05-15T21:46:51Z

I would be very careful pointing to different licenses

@reinhapa Can you tell me what I need to be 'careful' of please? The code for eXist-db is 'LGPL 2.1', the modifications in Elemental 6.4.0 and 7.0.0 are 'LGPL 2.1 ONLY'. I don't see how clearly pointing that out should upset anyone, in fact quite to the contrary, it is important for both developers and users to understand the licensing of the software they contribute to and/or use; they need to be 'careful' to adhere to the license terms.

and accusing @line-o to have copied from Elemental here.

I have not made any accusations, I have simply made an observation. If you compare the approach taken, and then also the git diffs you will likely find, as I did, that they are almost identical.

Changing the implementation in order to correctly implement an external defined API will naturally result in similar changes that might also exist in Elemental.

I don't disagree... but, there is more to it than just that here.

Also: How comes that Elemental code contains eXist Source but not the GIT history that lead to it?

Elemental does contain the Git History of eXist-db - git log will show you that. What do you believe is missing?

duncdrum · 2025-07-08T09:15:52Z

@line-o can you rebase this. The container test shows an error in exist.log we should check that first. If the error stays we can't merge without stopping to push containers

fixes eXist-db#5672 Previously ElementImpl.getAttribute and ElementImpl.getAttributeNodeNS of in memory nodes violated the API contract of org.w3c.dom.element. It was returning null if an attribute was not set instead of an empty String. Fixing this lead to a host of changes throughout the project. The if-conditions are simplified and even flipped in a few locations were this greatly improved readability of thd code. The import of StringUtils was dropped were possible.

- refactor calculateBaseURI to use getAttributeNodeNS to differentiate between unset and empty attribute value - annotate functions with Nonnull that override Nonnull methods from org.w3c.dom.ElementImpl - remove deprecated function _getAttributeNS, - return "" instead on misleading constant in getAttributeNS

- drop dependency on Apache commons-lang3 - refactor AnalyzerConfig.getConstructorParameter and AnalyzerConfig.configureAnalyzer for readability

The last two were the mail module and the LDAP security realm.

- Eval.java and AppRestoreUtils

line-o · 2025-07-09T17:27:33Z

@duncdrum the error in docker startup persists. How can we inspect it locally?

duncdrum · 2025-07-09T17:38:09Z

@line-o the logs aren't uploaded anymore automatically we should bring that back.

You would have to build locally start the container and inspect its 'exist.log' There is an 'ERROR' that wasn't there before.

line-o · 2025-07-09T18:18:50Z

so @duncdrum I think we caught an issue here which might be related to the changes:

09 Jul 2025 18:16:17,612 [main] ERROR (AutoDeploymentTrigger.java [execute]:127) - Exception during deployment of app dashboard-2.0.9.xar: Package requires version  of package [http://www.functx.com.⁠](http://www.functx.com./) Installed version is 1.0.1. Please upgrade! 

org.expath.pkg.repo.PackageException: Package requires version  of package [http://www.functx.com.⁠](http://www.functx.com./) Installed version is 1.0.1. Please upgrade!
	at org.exist.repo.Deployment.installAndDeploy(Deployment.java:232)
	at org.exist.repo.Deployment.installAndDeploy(Deployment.java:156)
	at org.exist.repo.Deployment.installAndDeploy(Deployment.java:248)
	at org.exist.repo.AutoDeploymentTrigger.execute(AutoDeploymentTrigger.java:125)
	at org.exist.storage.StartupTriggersManager.startPreMultiUserSystem(StartupTriggersManager.java:60)
	at org.exist.storage.BrokerPoolServicesManager.startPreMultiUserSystemServices(BrokerPoolServicesManager.java:264)
	at org.exist.storage.BrokerPool._initialize(BrokerPool.java:667)
	at org.exist.storage.BrokerPool.initialize(BrokerPool.java:439)
	at org.exist.storage.BrokerPools.configure(BrokerPools.java:177)
	at org.exist.storage.BrokerPools.configure(BrokerPools.java:112)
	at org.exist.jetty.JettyStart.run(JettyStart.java:244)
	at org.exist.jetty.JettyStart.main(JettyStart.java:126)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(Unknown Source)
	at java.base/java.lang.reflect.Method.invoke(Unknown Source)
	at org.exist.start.Main.invokeMain(Main.java:154)
	at org.exist.start.Main.runEx(Main.java:304)
	at org.exist.start.Main.run(Main.java:159)
	at org.exist.start.Main.main(Main.java:96)

line-o · 2025-07-09T18:26:25Z

dashboard has the speciality that it defines no version of the functx dependency so this feels very much related.

When handling dependency elements in package descriptor files the package version attribute was checked in order to determine if a specific dependency version was pinned for this dependency. Now that the DOM implementation will always return an empty string instead of null for unset attributes this now means that 1. the package version is set (otherwise the package would not install) 2. the version variable for each dependency will be non-null because the package version is set. The type will then vary on the attribute value - this would have been null in the past and is now an empty string which changes the resulting DependemcyVersion subtype.

…ements - move private static class and private enum to the end of the class - remove unnecessary throw declaration from getPackageDir - flip if condition at the beginning of installAndDeploy - fix whitespace and formatting

duncdrum · 2025-07-11T10:13:29Z

@line-o nice catch, and yay for tests. Good to go now from my side.

reinhapa · 2025-07-13T17:38:01Z

exist-core/src/main/java/org/exist/repo/Deployment.java

+    private static class RequestedPerms {
+        final String user;
+        final String password;
+        final Optional<String> group;
+        final Either<Integer, String> permissions;
+
+        private RequestedPerms(final String user, final String password, final Optional<String> group, final Either<Integer, String> permissions) {
+            this.user = user;
+            this.password = password;
+            this.group = group;
+            this.permissions = permissions;
+        }
+    }


Might use a record instead of a class here?

makes sense, but I thought a 'switch' change would be in an upcoming PR... but I am wrong as I don't see a remark here.

Yes let's not do too much in one PR.

line-o requested a review from a team as a code owner May 14, 2025 11:24

line-o force-pushed the fix/dom-impl branch from fc178d1 to 3596982 Compare May 14, 2025 12:47

line-o added this to v7.0.0 May 14, 2025

reinhapa requested changes May 14, 2025

View reviewed changes

line-o requested review from a team and reinhapa May 15, 2025 09:54

line-o marked this pull request as draft May 19, 2025 15:56

line-o marked this pull request as ready for review June 16, 2025 18:31

dizzzz approved these changes Jul 8, 2025

View reviewed changes

reinhapa approved these changes Jul 8, 2025

View reviewed changes

line-o added 6 commits July 9, 2025 15:44

[refactor] Lucene module

ef7a696

- drop dependency on Apache commons-lang3 - refactor AnalyzerConfig.getConstructorParameter and AnalyzerConfig.configureAnalyzer for readability

[refactor] remove commons-lang3 dependency from last extensions

348888e

The last two were the mail module and the LDAP security realm.

[refactor] remove commons-lang3 dependency from 2 core classes

d419c8e

- Eval.java and AppRestoreUtils

[refactor] replace ArrayUtils.contains with Set.contains in FunBaseURI

fe59d2a

line-o force-pushed the fix/dom-impl branch from 3596982 to fe59d2a Compare July 9, 2025 13:44

line-o self-assigned this Jul 10, 2025

line-o marked this pull request as draft July 10, 2025 09:58

duncdrum mentioned this pull request Jul 10, 2025

the logs aren't uploaded anymore automatically #5793

Closed

line-o added 2 commits July 10, 2025 17:59

line-o marked this pull request as ready for review July 11, 2025 10:22

reinhapa reviewed Jul 13, 2025

View reviewed changes

dizzzz merged commit 51fb3b0 into eXist-db:develop Jul 20, 2025
7 of 9 checks passed

github-project-automation bot moved this to Done in v7.0.0 Jul 20, 2025

Uh oh!

Align DOM implementations with org.w3c.dom #5740

Align DOM implementations with org.w3c.dom #5740

Uh oh!

Conversation

line-o commented May 14, 2025

Description:

Reference:

Type of tests:

Uh oh!

reinhapa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamretter commented May 15, 2025

Uh oh!

line-o commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reinhapa commented May 15, 2025

Uh oh!

adamretter commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamretter commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duncdrum commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

line-o commented Jul 9, 2025

Uh oh!

duncdrum commented Jul 9, 2025

Uh oh!

line-o commented Jul 9, 2025

Uh oh!

line-o commented Jul 9, 2025

Uh oh!

duncdrum commented Jul 11, 2025

Uh oh!

reinhapa Jul 13, 2025

Choose a reason for hiding this comment

Uh oh!

dizzzz Jul 13, 2025

Choose a reason for hiding this comment

Uh oh!

line-o Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

line-o commented May 15, 2025 •

edited

Loading

adamretter commented May 15, 2025 •

edited

Loading

adamretter commented May 15, 2025 •

edited

Loading

duncdrum commented Jul 8, 2025 •

edited

Loading