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.

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

Remove the last small dependency on Apache Xalan #4643

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

adamretter
Copy link
Member

We can use the equivalent classes from Apache Xerces instead

@adamretter adamretter added the enhancement new features, suggestions, etc. label Dec 7, 2022
@adamretter adamretter added this to the eXist-6.0.2 milestone Dec 7, 2022
@dizzzz
Copy link
Member

dizzzz commented Dec 7, 2022

In the unit tests we explicitly test for xalan 😬

@adamretter
Copy link
Member Author

In the unit tests we explicitly test for xalan 😬

@dizzzz Okay no problem. I have updated the PR to address that as well.

@sonarcloud
Copy link

sonarcloud bot commented Dec 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@dizzzz
Copy link
Member

dizzzz commented Dec 8, 2022

interesting... java8 only
image

@dizzzz
Copy link
Member

dizzzz commented Dec 8, 2022

it looks the PR combines 2 PRs ?

@adamretter
Copy link
Member Author

adamretter commented Dec 8, 2022

There is a runtime dependency on Xalan in the GeoTools libraries used by the Spatial Index. I have opened a PR to fix that here - GEOT-7284. Once GEOT-7284 is merged, and they release a version that includes that, we will need to update the Spatial Index to use a newer version of the GeoTools.

@adamretter
Copy link
Member Author

it looks the PR combines 2 PRs ?

@dizzzz now fixed.

@reinhapa
Copy link
Member

@adamretter seems there is no support for "namespace" feature: https://github.com/eXist-db/exist/actions/runs/3649983650/jobs/6165379878#step:4:13854

@adamretter
Copy link
Member Author

@adamretter seems there is no support for "namespace" feature: https://github.com/eXist-db/exist/actions/runs/3649983650/jobs/6165379878#step:4:13854

@reinhapa Yup, that is addressed in my PR geotools/geotools#4141

@adamretter
Copy link
Member Author

@dizzzz @reinhapa Okay so they released GeoTools 28.1 and I just updated this, so I am hopeful that assuming CI goes through okay, then it is now ready.

@dizzzz
Copy link
Member

dizzzz commented Feb 5, 2023

craps
image

@adamretter
Copy link
Member Author

@dizzzz No problem. I have fixed that one now. However I have hit another one in GeoTools, I will need to send them a PR as they are not handling XML Namespaces in a manner that Saxon likes:

Caused by: org.xml.sax.SAXException: Parser configuration problem: namespace reporting is not enabled
	at net.sf.saxon.event.ReceivingContentHandler.getNodeName(ReceivingContentHandler.java:477)
	at net.sf.saxon.event.ReceivingContentHandler.startElement(ReceivingContentHandler.java:366)
	at org.geotools.xml.transform.QNameValidatingHandler.startElement(QNameValidatingHandler.java:61)
	at org.geotools.xml.transform.TransformerBase$ContentHandlerFilter.startElement(TransformerBase.java:367)
	at org.geotools.xml.transform.TransformerBase$TranslatorSupport._start(TransformerBase.java:822)

@adamretter
Copy link
Member Author

Requires this second PR geotools/geotools#4186 to be merged to GeoTools, and then a new release of GeoTools.
After that we will need to update this PR for the new release of GeoTools that includes the above GeoTools PR.

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.

@adamretter despite two approvals the upstream changes still need to get merged and released. And we need to get a ci run here before merging. I’ll convert this to draft in the interim. Changes look good in principal

@adamretter
Copy link
Member Author

The PR geotools/geotools#4186 to GeoTools was merged a couple of weeks ago. We are now just waiting for a new release of GeoTools that includes that fix. Then we can bump the dependency version here and we will be good to go...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new features, suggestions, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants