OSGI metadata of org.hl7.fhir.utilities introduces hard junit dependency

[reckart]

OSGI metadata of org.hl7.fhir.utilities introduces hard junit dependency

[cmark]

+1

this unfortunately prevents us from using this library in our system as the model libraries actually depend on the utilities lib which will pull in the junit packages :(

[cmark]

Thanks @reckart I was about to reply to @grahamegrieve about the scope 😅

@grahamegrieve

Why does moving it to validation make any difference?

Mostly code structuring, if you have a functionality provided by library X which requires a dependency of Y then all dependants of library X will inherently depend on Y even if only just one of them uses the feature that requires Y. In this case the best practice is to split the library into two parts or move the code closer to the actual place where is it being used. That's why I suggested the move to the validation, because it is primarily being used there but not sure if that's the only place. The other option would be module splitting and introducing a utilities-executor or something like that provides the functionality, but that's an even longer route than moving it to the validation, so I omitted it for simplicity.

@reckart

If the test utility code is only used in the validation module, IMHO it would be best to move it to src/test/java there.

That's no good, as that code is being used runtime not just when executing unit tests during the build. Some of the code written with JUnit is actually production code used by the validation module. (If I understood everything correctly when I read the code yesterday)

So it might be sensible to rather copy the code instead of moving it, marking the original code as deprecated and for removal in the next major FHIR release and mark the JUnit dependencies optional in OSGI and provided in Maven - also with deprecation comments and the intention of removing them entirely in the next major version.

Yeah, I agree that for backward compatibility we have to keep the original code in the utilities module as well. These dependencies are already in provided scope so the only thing that needs to happen is marking them optional.

I still think that for the short term I would go with Option 2, no breaking change, no need to move anything, just mark the dependencies optional.

[grahamegrieve]

One reason this has not been actioned is that we are not familiar with OSGi, and do not know how to test this. So a PR would be welcome

[reckart]

javax.annotation - it would be great to move this to the jakarta namespace like hapi-fhir-base.

+1 - that caused us a bunch of headaches as well

[reckart]

@cmark cool. FWIW I'm currently using the library even with its broken OSGI metadata. In order to work around the current problem, we created a few internal empty stub bundles that just claim to provide JUnit, FHIR Rest Server, etc. to ensure that the artifacts resolve. I'm not using fhir-core atm, so I didn't hit the plantuml issue. It would be great if we could get this lib up to speed wrt. OSGi support.

[reckart]

@cmark I had the wrapper bundle before - but IMHO its better to use upstream-provided OSGi artifacts in order to avoid overhead when updating a dependency. That's we I use the original bundles now (with a bit of a hack to work around the bugs) - and why I opened this issue ;)

[reckart]

@grahamegrieve I would assume that in most cases when FHIR is used as a library, downstream users would not want to have JUnit as a transitive dependency. On the Maven level, this is currently handled by marking the JUnit dependency as provided. Does that mean to replace the use of JUnit outside of tests with own code - that would be the default assumption, yes.

I can see though that you have build (somewhat unconventionally I would say) a subsystem on top of JUnit - and for better or worse, that is now part of the code and being used. I have not done a deep analysis of the code, so please take my following comments with a grain of salt.

For most part, library code that is meant for consumption by downstream users should follow some conventions such as not hard-depending on a logging framework (like log4j) but rather depending on a logging API (like slf4j). Likewise, libraries that are typically not only used during testing should not not depend on testing frameworks like JUnit. It is typically better for downstream users if code related to testing (and depending on test frameworks) and code meant for production are maintained in separate modules - it helps the project maintain a lean dependency footprint that allows it to more easily embed into applications of downstream users.

Now application code is typically not meant for downstream consumption. E.g. I would call the validation CLI an application that is meant to be used directly by an end user via the command line. Since such code is not meant for downstream consumption by other applications/libraries, applications do not need to maintain a lean dependency footprint (other than maybe to manage their package size when shipping or their attack surface wrt. security issues). So if the org.hl7.fhir.validation module basically consists only of the CLI application, it is perfectly fine if it depends on JUnit. However, for other modules such as org.hl7.fhir.utilities or org.hl7.fhir.r5 it would be sensible separate concerns, e.g. by factoring code that is primarily being used in tests out into org.hl7.fhir.test-utilities and rg.hl7.fhir.r5-testing modules.

I might have missed something, but it seems to me that the JUnit-dependent code is mainly used in testing and in application code. But it is still packaged along with non-test-only library code and that joint packaging is what is causing a bit of a headache here. If possible, separating packaging offers a cleaner solution than using provided/runtime scopes or optional dependencies (in Maven or OSGi).

[reckart]

I had asked if there was interest in a PR for this:

https://chat.fhir.org/#narrow/stream/179167-hapi/topic/Interest.20in.20PRs.20fixing.20OSGI.20metadata.3F.20.28cross-post.29

However, before investing time here, I'd like to be reassured that a PR would actually be accepted and merged... and there has not been a reassuring reply to my inquiry yet.

@grahamegrieve Are you the maintainer of this code and would you review, merge and release such a PR?

[grahamegrieve]

I'm one of them. You didn't get an answer, true, but no answer is very different from 'we don't care'. Just @jamesagnew who didn't answer - he's pretty busy

We'd accept a PR in principle. Generally, we want to see test cases, but I can't imagine what they'd be in this case, unless we were going to do some kind of OSGi build in the pipelines, and I'm not sure we're interested in that. The other problem with accepting a PR on this is that we have no/little expertise to evaluate it, so I think we'd ask for comments from others on it.

[reckart]

@cmark Would you be able/willing to test a PR prior to it being included in a release?

[cmark]

@reckart @grahamegrieve Sure, I can comment on the PR, check the changes, and help out in testing the fixed libraries. I can also help with the fixes and create the PR (or follow-up PRs, see reason below) if needed. Making libraries OSGi compatible is not the easiest task. 😄

Additional findings
Unfortunately, even after removing the JUnit dependencies the library might require additional tweaks before it can be used in OSGi systems and some further tweaks to make the number of hard dependencies less. Our use case requires the Terminology module model classes and their converters between the various FHIR versions (R4, R4B, R5, and future versions). It would be acceptable to use an all-in-one library as this one but creating a dependency to this library pulls in additional transitive dependencies which we either a) simply don't need because the feature is not going to be used on our side or b) increases tech debt (regular version bumps/security scans needed, jakarta vs javax namespace conflicts, etc.).

Major issue:

• net.sourceforge.plantuml - used by R5 only, in Example rendering, there is no OSGi compatible MANIFEST present in that library. This is the next biggest problem and there are multiple ways to resolve it. We can discuss them once the JUnit dependencies have been removed.

Minor issues:

• javax.annotation - it would be great to move this to the jakarta namespace like hapi-fhir-base.
• jose - used by R5, only needed for smart health web signatures decryption, validation, etc. which I think is not something everyone needs, so making these optional would be beneficial.
• commons-net - used by R5 and utilities, probably something that could be also made optional but not the biggest issue if someone would need to depend on this.

The minor ones are not necessarily OSGi only dependency issues, especially if someone only requires a subset of the entire FHIR spec. I wanted to mention them here because they came up during my investigation. The plantuml one unfortunately something that needs to be handled otherwise the fhir core jars won't be able to start in an OSGi context.

[cmark]

@reckart fyi, I'm going to spend time working on this because I need a working jar in the coming days to be able to tell whether this library is going to be the one or we have to come up with alternatives

[cmark]

@reckart yeah, we usually create a wrapper bundle that embeds the dependencies as jars and uses only the functionality needed😅 I'll try to come up with a PR as soon as possible that resolves the JUnit dependencies first.

[cmark]

So, the main problem is that JUnit dependencies are required runtime because there is a JUnit4 and JUnit5 Executor logic provided by the utilities module. The validation module uses this to run tests written in JUnit during runtime (if we only consider this repository).
The JUnit 4 and 5 dependencies in the utilities/pom.xml are marked as provided so it is the dependant module's responsibility to pull them in if they'd like to use these classes (which happens in the validation.cli module eventually). Unfortunately, the default behavior of the maven-bundle-plugin still considers these as dependencies and adds them to the MANIFEST.MF.

Option 1:
I think the best fix would be to move all the execution classes from the utilities module to the validation module and change the JUnit dependencies to test scope. (not sure if there is any other module out in the wild that depends on the utilities module and uses these classes, though)

Option 2:
An easier workaround for the short term without modifying any of the classes would be to mark the JUnit 4 and 5 dependencies optional in the utilities/pom.xml. I made the necessary changes for this and built the modules locally without any issues and the resulting OSGi MANIFEST looks okay.

@grahamegrieve @reckart Any thoughts? Go with Option 2 with the smallest impact for now or refactor the code a bit according to Option 1?

[grahamegrieve]

I don't really understand option 1. Why does moving it to validation make any difference? And what does 'test scope' mean?

[reckart]

@grahamegrieve Maven defines various scopes for dependencies. E.g. when building (and running) tests from src/test/java, dependencies from the test scope are considered - but they are not considered when building code from src/main/java that is meant for downstream consumption. These scopes are important to ensure that downstream users do only get transitive dependencies they need to build their code and run it against FHIR, but not the dependencies that FHIR uses only to test itself.

It is a good practice to use the Maven Dependency Plugin to automatically check if pom.xml files contain too many or too few dependencies and if they have the right scope.

Test code (that depends on test libraries like JUnit) should either only be located in src/test/java or (if it is to be reusable across multiple Maven modules) in src/main/java of a dedicated test-code module which is then only used as a "test" scope dependency by other modules.

@cmark If the test utility code is only used in the validation module, IMHO it would be best to move it to src/test/java there. However, doing so in principle would be a breaking change according to semantic versioning. So it might be sensible to rather copy the code instead of moving it, marking the original code as deprecated and for removal in the next major FHIR release and mark the JUnit dependencies optional in OSGI and provided in Maven - also with deprecation comments and the intention of removing them entirely in the next major version.

[grahamegrieve]

Well, one issue is that the testing code can be run by the validator, which is a primary downstream use. So I don't think we can mark it as a test dependency

[reckart]

Runtime code should not depend on JUnit or other test libraries. If JUnit is used during production, that's probably something that should be addressed separately.

[grahamegrieve]

If JUnit is used during production, that's probably something that should be addressed separately.

we should write our own JUnit equivalent? why?

[grahamegrieve]

application code. But it is still packaged along with production code

Application code is very much production code, but I think that's just a quibble about words. @dotasek, thoughts?

[reckart]

Application code is production code yes - that's why I tried above to rephrase it differently making a distinction between application code and library code and also distinguish between libraries mainly used for testing (and depending on test frameworks). Does that argumentation and the motivation for it fit better into your understanding of how things work?

[cmark]

PR is open about marking the JUnit dependencies optional in the utilities/pom.xml.