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.

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

reinhapa

some parts could be converted to switch expressions

reinhapa · 2025-05-14T17:06:16Z

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

+			if ("not".equalsIgnoreCase(type) ) {
+				st = new NotTerm( parseChildSearchTerm( terms ) );
+			} else if ("and".equalsIgnoreCase(type) ) {
+				st = new AndTerm( parseChildSearchTerms( terms ) );
+			} else if ("or".equalsIgnoreCase(type) ) {
+				st = new OrTerm( parseChildSearchTerms( terms ) );
+			} else if ("from".equalsIgnoreCase(type) ) {
+				st = parseFromTerm( terms );
+			} else if ("subject".equalsIgnoreCase(type) ) {
+				st = parseSubjectTerm( terms );
+			} else if ("body".equalsIgnoreCase(type) ) {
+				st = parseBodyTerm( terms );
+			} else if( "to".equalsIgnoreCase(type) || "recipient".equalsIgnoreCase(type) ) {
+				st = parseRecipientTerm( terms );
+			} else if( "header".equalsIgnoreCase(type) ) {
+				st = parseHeaderTerm( terms );
+			} else if( "flag".equalsIgnoreCase(type) ) {
+				st = parseFlagTerm( terms );
+			} else if( "sent".equalsIgnoreCase(type) ) {
+				st = parseSentDateTerm( terms );
+			} else if( "received".equalsIgnoreCase(type) ) {
+				st = parseReceivedDateTerm( terms );
+			} else {
+				throw new XPathException(this, "Invalid Search Term type specified: " + type );
+			}
+		}

-		if( st == null ) {
-			throw( new XPathException(this, "Invalid Search Terms specified" ) );
+		if (st == null) {
+			throw new XPathException(this, "Invalid Search Terms specified" );
 		}


Maybe turn into a an switch statement. Also the null check seems to be obsolete here as the getAttribute should not return null

@reinhapa yes, there is still a lot of code that can be optimised. But given the amount of code that is changed in this PR I would propose to do it in a separate PR

reinhapa · 2025-05-14T17:07:13Z

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

+		Message.RecipientType rtype = null;
+		if ("to".equalsIgnoreCase(type) ) {
+			rtype = Message.RecipientType.TO;
+		} else if( "cc".equalsIgnoreCase(type) ) {
+			rtype = Message.RecipientType.CC;
+		} else if( "bcc".equalsIgnoreCase(type) ) {
+			rtype = Message.RecipientType.BCC;
 		} else {
-			throw( new XPathException(this, "Pattern attribute must be specified for term with type: " + ((Element)terms).getAttribute( "type" ) ) );
+			throw new XPathException(this, "Invalid recipientType: " + type + ", for term with type: " + ((Element)terms).getAttribute("type"));
 		}


Here also a switch as function could be used to simplify the logic...

I propose to change this in a followup PR

reinhapa · 2025-05-14T17:07:57Z

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

+		final Flags flags;
+
+		if ("answered".equalsIgnoreCase(flag) ) {
+			flags = new Flags( Flags.Flag.ANSWERED );
+		} else if( "deleted".equalsIgnoreCase(flag) ) {
+			flags = new Flags( Flags.Flag.DELETED );
+		} else if( "draft".equalsIgnoreCase(flag) ) {
+			flags = new Flags( Flags.Flag.DRAFT );
+		} else if( "recent".equalsIgnoreCase(flag) ) {
+			flags = new Flags( Flags.Flag.RECENT );
+		} else if( "seen".equalsIgnoreCase(flag) ) {
+			flags = new Flags( Flags.Flag.SEEN );
 		} else {
-			throw( new XPathException(this, "flag attribute must be specified for term with type: " + ((Element)terms).getAttribute( "type" ) ) );
+			throw new XPathException(this, "Invalid flag: " + flag + ", for term with type: " + ((Element)terms).getAttribute("type"));
 		}


Here switch as function would make sense

I propose to change this in a followup PR

reinhapa · 2025-05-14T17:08:28Z

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

+		int  cp = ComparisonTerm.EQ;
+		if ("eq".equalsIgnoreCase(comp) ) {
+			cp = ComparisonTerm.EQ;
+		} else if( "ge".equalsIgnoreCase(comp) ) {
+			cp = ComparisonTerm.GE;
+		} else if( "gt".equalsIgnoreCase(comp) ) {
+			cp = ComparisonTerm.GT;
+		} else if( "le".equalsIgnoreCase(comp) ) {
+			cp = ComparisonTerm.LE;
+		} else if( "lt".equalsIgnoreCase(comp) ) {
+			cp = ComparisonTerm.LT;
+		} else if( "ne".equalsIgnoreCase(comp) ) {
+			cp = ComparisonTerm.NE;
 		} else {
-			throw( new XPathException(this, "comparison attribute must be specified for term with type: " + ((Element)terms).getAttribute( "type" ) ) );
+			throw( new XPathException(this, "Invalid comparison: " + comp + ", for term with type: " + ((Element)terms).getAttribute( "type" ) ) );
 		}
-
-		return( cp );
+		return cp;


.. switch as function

I propose to change this in a followup PR

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?

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

line-o added 6 commits May 14, 2025 14:43

[refactor] Lucene module

9884a87

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

[refactor] remove commons-lang3 dependency from last extensions

55a4692

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

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

4b0efd8

- Eval.java and AppRestoreUtils

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

3596982

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Align DOM implementations with org.w3c.dom #5740

Align DOM implementations with org.w3c.dom #5740

Uh oh!

line-o commented May 14, 2025

Uh oh!

reinhapa left a comment

Uh oh!

reinhapa May 14, 2025

Uh oh!

line-o May 15, 2025

Uh oh!

reinhapa May 14, 2025

Uh oh!

line-o May 15, 2025

Uh oh!

reinhapa May 14, 2025

Uh oh!

line-o May 15, 2025

Uh oh!

reinhapa May 14, 2025

Uh oh!

line-o May 15, 2025

Uh oh!

adamretter commented May 15, 2025

Uh oh!

line-o commented May 15, 2025 •

edited

Loading

Uh oh!

reinhapa commented May 15, 2025

Uh oh!

adamretter commented May 15, 2025 •

edited

Loading

Uh oh!

adamretter commented May 15, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Align DOM implementations with org.w3c.dom #5740

Are you sure you want to change the base?

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

line-o commented May 15, 2025 •

edited

Loading

adamretter commented May 15, 2025 •

edited

Loading

adamretter commented May 15, 2025 •

edited

Loading