hapifhir / hapi-fhir

🔥 HAPI FHIR - Java API for HL7 FHIR Clients and Servers
http://hapifhir.io
Apache License 2.0
1.94k stars 1.3k forks source link

Make sure HAPI structures are Android friendly #3690

Closed jingtang10 closed 1 year ago

jingtang10 commented 2 years ago

NOTE: Before filing a ticket, please see the following URL: https://github.com/hapifhir/hapi-fhir/wiki/Getting-Help

Describe the issue A clear and concise description of what the feature request is. Please include details about what problem you are ultimately trying to solve (i.e. what are you building with HAPI FHIR) and how this feature would help.

HAPI structures is currently used by the Android FHIR SDK, but it's possible (and perhaps should be encouraged?) that other Android projects use the HAPI structures as FHIR data models.

However, not all Java libraries can be used on Android automatically, certain dependencies will cause issues. For example JAXB as we discovered recently https://github.com/cqframework/clinical_quality_language/pull/752, https://github.com/google/android-fhir/issues/1423. Using Java9+ features can also cause issues on Android.

It would be useful to have a strategy wrt HAPI structures and Android. Is there a set of minimum tests we can add to ensure things work on Android? Is there some GitHub plugin that can help us with this? What documentation should we add to the HAPI docs to be explicit about this with developers?

@jamesagnew @tadgh @yigit @vitorpamplona @stevenckngaa @kevinmost @aditya-07 @fredhersch

Environment (please complete the following information): NA

Additional context https://developer.android.com/studio/write/java8-support https://jakewharton.com/androids-java-9-10-11-and-12-support/

tadgh commented 2 years ago

I had a gander around for maven plugins that could check android compatibility but came up empty. Happy to take suggestions on maintaining android support via tests/github actions if any exist.

jingtang10 commented 1 year ago

Discussed with Vitor (vitorpamplona@ on GitHub).

I think one of the reasons it's so critical to address this issue on HAPI's side is that it's much more difficult to reverse new dependencies after a release. For example, if HAPI introduces a dependency on some java api or system api that is not available on Android, we will only discover that after the release has been made and when we're testing the new HAPI structures lib version. Once we discover the issue, we'll then need to report to HAPI and see if there's a way to override the dependency and/or reverse the usage in the next version. This would be hugely time consuming.

And if something happens that is irreversible, that means HAPI structures will not be usable on Android from that point on. This is pretty limiting for HAPI's usefulness. For this reason, it's really important we take preventative steps.

So one thing that we might be able to do is, similar to Vitor's PR here https://github.com/FHIR/Ucum-java/pull/29, create a new build target that compiles HAPI structures lib on Android. Please note this PR contains a lot more stuff than adding the new build target. We still need to investigate how to create build targets against specific Android API Levels in Maven. But if this can be done this would be a huge step forward.

@tadgh @jamesagnew what's your view on this?

PS this has come up again due to recent issues we have come across in HAPI6: https://github.com/google/android-fhir/pull/1603 https://github.com/google/android-fhir/issues/1627

jamesagnew commented 1 year ago

Hey @jingtang10 - Certainly the intent with the hapi-fhir-android module was that it would be exactly this (a build of HAPI FHIR with no incompatible deps).

I'm guessing from all of the linked issues (e.g. https://github.com/google/android-fhir/issues/1627 and https://github.com/hapifhir/hapi-fhir/pull/3977 ) that this hasn't worked out.

I'm guessing one issue is that I'm obviously not clear on what parts of the library you are trying to run on device. E.g. I see in https://github.com/google/android-fhir/issues/1627 some discussion around Caffeine 3.0 not being android compatible. Which I would have assumed to be the case, but I don't actually get which parts of the library you are using that would be affected by that. Presumably not the JPA server, so maybe the validator?

Perhaps the right starting place would be to figure out what parts of the library you are using, and then come up with a plan to provide alternative mechanisms for each.

E.g. for the Caffeine thing, the only place in the validator it actually gets used (assuming this is the affected part) we could easily just implement an alternative to CachingValidationSupport that uses another cache. For the parser, perhaps pinning to an older version of Jackson, or even creating a Gson-based parser implementation?

vitorpamplona commented 1 year ago

HAPI could use a maven plugin to test compliance with a given android API. I just tested with the Animal Sniffer's plugin and it really works.

<dependency>
  <groupId>org.codehaus.mojo</groupId>
  <artifactId>animal-sniffer-maven-plugin</artifactId>
  <version>1.22</version>
</dependency>

...

<plugin>
  <groupId>org.codehaus.mojo</groupId>
  <artifactId>animal-sniffer-maven-plugin</artifactId>
  <configuration>
    <signature>
      <groupId>net.sf.androidscents.signature</groupId>
      <artifactId>android-api-level-26</artifactId>
      <version>8.0.0_r2</version>
    </signature>
  </configuration>
</plugin>

Currently, mvn animal-sniffer:check outputs:

... Updated below

jingtang10 commented 1 year ago

Thanks @jamesagnew, here's our dependencies on HAPI: https://github.com/google/android-fhir/blob/d5435a4d3af0a060db7dca8e5fc7ef8f7f09e1a9/buildSrc/src/main/kotlin/Dependencies.kt#L41

We use the R4 structures lib and the validation package.

Good find @vitorpamplona. It seems like again XML is the main issue here...

vitorpamplona commented 1 year ago

The question is which android sdk version should HAPI be compliant with?

jingtang10 commented 1 year ago

The question is which android sdk version should HAPI be compliant with?

I don't think we need to go crazy here. I'd be very happy if we begin with a recent (or simply the latest?) Android SDK version and just maintain it going forward. As new Android SDK versions are released each year, we can just add them to the test rules. At a certain point there will be too many versions, and HAPI might not want to test against all of them. So it's down to the team to either selectively test a few versions or drop older Android versions.

jamesagnew commented 1 year ago

Do you guys actually need the XML parser? I've always kind of gone with the assumption that as long as the JSON one works on android that should be good enough. I'm not aware of any serious servers at this point that don't support JSON.

Definitely agree XML is problematic.. I looked into getting StAX working on Android once, and it seems pretty much like a non starter since it lives in a javax.* namespace.

jingtang10 commented 1 year ago

@vitorpamplona is best to answer this. He did a bunch of work for the new CQL evaluator/engine/translator to work on Android. Do they have XML dependencies?

Besides the CQL dependencies we definitely don't want to touch XML on Android. It's a pain.

vitorpamplona commented 1 year ago

We do, but we use Xerces/Woodstox instead. I am trying to setup the plugin to use the Android API + the dependency stack we have on android so that we can truly see how many failures there are

That output was just the base module. There are other errors. I am trying to filter them before putting them here.

@jingtang10 if we want to keep on minSDK 24, it would be nice to have warnings for the [java.lang.reflect.Method.getParameterCount()](https://developer.android.com/reference/java/lang/reflect/Method#getParameterCount()) which doesn't exist until 26.

vitorpamplona commented 1 year ago

Needless to say, some love to https://github.com/hapifhir/hapi-fhir/pull/3977 would be appreciated.

vitorpamplona commented 1 year ago

I just sent a quick PR #4070 to demonstrate how this would work. It uses the Animal Sniffer plugin to make sure the code being used in Android projects (base + structures + validators) is compliant with SDK 26, which it already is.

vitorpamplona commented 1 year ago

As an update on this, CQL Translator, Engine and Evaluator are now using the same strategy of #4070 to assess compliance with Android 8, SDK 26.

Here are the PRs:

JPercival commented 1 year ago

Resolved by #4070 and #4196