-
Notifications
You must be signed in to change notification settings - Fork 14
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
Builds a ServiceLoader pattern for the XPP Pull Parser #29
Conversation
…is starts the process of allowing any other parser to load the xml-essence.xml
We actually publish that library, but as I read that, it would be a breaking change, yes? So I should create a 2.0.0-SNAPSHOT version of UCUM? |
Which library do you publish? The change breaks due to a new (sane) naming convention for modules. We can also change the new module names to not break. We would just rename the |
@grahamegrieve , thoughts on whether we should keep this backwards compatible, or go the module route and have downstream dependencies switch (i.e. change to modules and change version to 2.0.0) |
I don't know. How many users does the library have? |
Given that there is a real option for people to not upgrade - there are no new functional features here, so users can keep using the current version without any problems - I vote for the breaking changes and merging this PR as is. When they upgrade, they just need to choose which dependency stack they prefer. |
This library is a dependency of the cql-translator/engine and the hapi validation structures, so nearly entire the Java FHIR stack has a dependency directly or indirectly on this library, including the IG publisher: https://github.com/hapifhir/hapi-fhir/blob/c6471547777f07ca44a60362b687468ce56f8d19/hapi-fhir-structures-r4/pom.xml#L44 As @vitorpamplona mentioned this impacts Java 9+ projects. Both HAPI and CQL recently moved to Java 11 and had to do a fair amount of refactoring to avoid the module issues. Here are a few examples: https://github.com/google/android-fhir/search?q=xpp3 Additionally, the license question vitor raised has popped up in several places. Here's an example of the license exception that the Android SDK made: |
And I'd just release a 2.0.0 with these changes and skip the SNAPSHOT. They are technically breaking, but the library otherwise works great. |
@grahamegrieve any decisions here? We need to find a solution. Both HAPI and the CQL evaluator are now creeping up. |
@vitorpamplona I'm confused at why we need all of this complexity in the first place, ie what problem are we trying to solve? Presumably there is some issue or collision with XPP and Android? Can we not just refactor the whole library to use a version of XPP that is ok on android? |
Many people are using different XPP libs. This PR just offers a way for them to use their preferred processor. For people that are still on 8 and some 11 (there are some conflicts, but it mostly works), they can use |
Is Java 8 a use case (we've dropped support for it everywhere in HAPI FHIR at this point)? Could we just standardize on whatever works for Android, so that there is no need for all of this complexity? What would be the downside of just doing that? |
You will need at least two: one for Java and one for Android. Java does not ship with an XPP processor out of the box. Android does. But since this lib is used everywhere, I don't know if users can drop Java 8 support. The change might look complicated, but it's actually quite simple. It's just good dependency management + separation of responsibilities in the lib. BTW: XPP is extremely old at this point. This whole project should be rebuilt with more modern tools in the future. If that is a direction, then this PR will help because it isolates the parsing from the usual UCUM Service API. Any other person can then create a JSON-based Ucum parser and put it here. |
what's a current equivalent of XPP with the same performance features, and that's robust on all java platforms? The problem with a json parser is that UCUM doesn't release json equivalent to ucum-essence.xml |
I don't know about performance, but I don't expect any library in 2023 to have to manually parse an XML like XPP requires you to. And since PullParsing theory hasn't been updated since 2005, I'd say it's not a good idea to stick with it.
Agree. I thought about making a simple converter into a better format (JSON, and even CBOR) before injecting into the app. It could be much faster than what it is today. However, the current design of this lib makes large changes like that very difficult. Frankly, even an XML to direct Java code tool is better than parsing that file. Either way, this PR opens the way for other people to author parsers that just provide this lib the information it needs to do its thing without breaking the past (besides a dependency rename / need to choose). |
This seems a lot easier to me: #30 - removes the problem dependency, and allows for the provider to be overridden if you really don't want the default, but doesn't impact any existing users |
@grahamegrieve that looks like a good approach to me... @vitorpamplona Does the standard |
Yep, #30 is also good. Since this project is been used by so many people, I didn't want to change any of the parsing logic in this PR to avoid unforeseen issues. But if @grahamegrieve is comfortable with the change, killing the pull parser is a good improvement. Android uses a different DocumentBuilderFactory, but you can add the AnimalSniffer plugin to make sure this project doesn't call non-existent methods over there. It would be very similar to what we did with the HAPI-FHIR repo. |
closed - did #30 instead |
Thank you Grahame |
Fixes: #14
The goal here is to allow multiple implementations of the
ucum-essence.xml
parser via ServiceLoaders. Each service loader can make use of a completely separate dependency stack.This PR separates Ucum classes from XML Parsing dependencies and places them in five new modules:
ucum
: base implementationucum.test-resources
: shared test resourcesucum.xmlpull
: Ucum Essense parser using XMLPull API (without linking to any particular implementation)ucum.xmlpull.android
: Ucum Essense parser using XMLPull API with Android (testing on Android SDK directly)ucum.xmlpull.xpp3
: Ucum Essense parser using XMLPull API with XPP3 (the current implementation)ucum.xmlpull.codelibs-xpp3
: Ucum Essense parser using XMLPull API with Codelib's XPP3 (a fork of XPP3 for Java9+)And moves the testing resources to the appropriate modules.
No functional changes were made.
Pros and Cons
Pros:
javax.xml.namespace
package from the jar, fixing the "The package javax.xml is accessible from more than one module: java.xml" error when compiling on recent versions of Java.Cons:
Impact on current users
Current users should move to
ucum.xmlpull.xpp3
in their dependency stack: