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

Update javax to jakarta namespaces #1236

Merged

Conversation

jamesagnew
Copy link
Contributor

No description provided.

@vitorpamplona
Copy link
Contributor

Notify @vitorpamplona when this gets released.

@JPercival
Copy link
Contributor

JPercival commented Oct 11, 2023

I think we'll need to more cleanly separate out the JAXB dependencies, in particular with respect to the Cql2ElmVisitor. We have a circular dependency in that the CQL compiler/runtime depends on HAPI fhir structures, and the HAPI fhir server, in turn, depends on CQL. CQL is typically always at HAPI current version - 1, and we won't get a full jakarta HAPI release until February.

There has been some effort to make JAXB optional to support Android, and perhaps we need to push that further. Downstream from CQL neither Android nor HAPI ought to depend on JAXB, and CQL should factored such that that is a possibility. I actually thought we were further along that path. I'm surprised to see how many changes were required to update the jakarta dependencies.

I'm going to check out this branch and play with it a bit to see if we can't further separate it. Will report back!

@JPercival JPercival enabled auto-merge (squash) November 1, 2023 15:05
@JPercival
Copy link
Contributor

JPercival commented Nov 1, 2023

Contrary to what I stated previously, we should take these changes and we can take them now. The rest of this comment explains why I think that.

Two other alternatives were considered:

  1. Dropping JAXB support entirely, in favor of Jackson. Bryn felt that existing users of JAXB do not have enough support to migrate off of JAXB for that to be viable.
  2. Removing the JAXB Annotations from the ELM classes, and use external binding descriptions for both JAXB and Jackson. Exploring this option revealed that it's more work than I anticipated. More on that below.

Why I changed my mind:

  1. My main concern was that the circular dependency between HAPI/CQL which causes CQL to use N-1 would make a direct migration unviable. CQL primarily uses the hapi-fhir-base and hapi-fhir-structures-* projects, which do not use the JAXB annotations. This means that their public APIs remain unchanged during the swap, meaning my concerns were unfounded.
  2. There was a high-priority bug confirmed to be fixed for CDR 2024.11.RC04 so we're no longer needing to hold off merging any potential breaking changes.
  3. I was able to eliminate the need for any downstream changes in cqframework/clinical-reasoning by reworking the usage of annotations there. This change should now be compatible with past / future versions of HAPI because the use of annotations is only part of ELM deserialization/serialization.
  4. I was expecting that we could just strip the JAXB annotations from the ELM classes and use the external bindings. It turns out that some of the auto-generated equals and hashCode implementations also depend on the JAXB annotations. The CQL engine performance is sensitive to these implementations and I'd rather not risk introducing performance regressions at this point.
  5. Beyond the unit tests I validated that the serialized ELM is similar between the implementations.

In summary, the impact of this change is less than I expected while the alternatives were more costly than I expected.

Thoughts or concerns @brynrhodes @jamesagnew @vitorpamplona ?

@vitorpamplona
Copy link
Contributor

Agree. The impact should be small.

Dropping JAXB support entirely, in favor of Jackson.

Keep in mind, Google/Android/IntelliJ have been pushing for Kotlin Serialization instead of Jackson. We might need to have a third option down the road. But that will change everything.

@JPercival JPercival merged commit 1da3590 into cqframework:master Nov 3, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants