Skip to content

Feat/extra resource loading from npm #784

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

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

jkiddo
Copy link
Collaborator

@jkiddo jkiddo commented Feb 23, 2025

This feature is backwards compatible and introduces the option to load e.g. example packages from IG's based on configuration

@jamesagnew
Copy link
Contributor

This functionality would probably make sense to just roll into the base PackageInstallationSpec. I'd rather not have this project implementing novel features not actually found in the base project, that's hard for support.

# Conflicts:
#	src/main/java/ca/uhn/fhir/jpa/starter/AppProperties.java
…dition

Added try-load of HAPI optimized dialect
@jkiddo
Copy link
Collaborator Author

jkiddo commented Feb 24, 2025

Sure ... I can make a PR to the base getting the additionalResourceFolders added the PackageInstallationSpec.

@jkiddo
Copy link
Collaborator Author

jkiddo commented Feb 24, 2025

@jamesagnew whats your take on the refactored version of the PostConstruct I've done here in https://github.com/hapifhir/hapi-fhir-jpaserver-starter/pull/784/files#diff-0f42a12718125be49d417ad9c402f5f2cecddfb7cb0ecfde364ad09132fa2a99R21 as mentioned in #758

@jkiddo
Copy link
Collaborator Author

jkiddo commented Feb 24, 2025

and btw - the same thing I did on https://github.com/hapifhir/hapi-fhir-jpaserver-starter/pull/784/files#diff-cca286d85a1c14aea8b529430ecd47aa4a803b53f4c7816deb7d602bd2af3268R11 can be done for a loooooong range of interceptor logic currently handled in the already way-too-big

method (I'm not pointing fingers here as I most likely authored a lot of it myself).

@jkiddo jkiddo marked this pull request as ready for review February 25, 2025 07:56
@jkiddo jkiddo requested a review from dotasek February 25, 2025 07:56
@jkiddo
Copy link
Collaborator Author

jkiddo commented Feb 27, 2025

@jamesagnew I'll move the additional sources logic once its been merged in here - acceptable for you? Then we can merge this into #785 and remove it from the starter as soon as it can be resolved from a snapshot in core.

# Conflicts:
#	src/main/java/ca/uhn/fhir/jpa/starter/AppProperties.java
# Conflicts:
#	src/main/java/ca/uhn/fhir/jpa/starter/AppProperties.java
jkiddo added a commit to jkiddo/hapi-fhir that referenced this pull request Mar 14, 2025
@jkiddo
Copy link
Collaborator Author

jkiddo commented Mar 14, 2025

@jamesagnew / @dotasek - I've isolated the feature set now into hapifhir/hapi-fhir#6802. Let me know if thats the feature set you had in mind, before I start writing tests and all.

@jkiddo jkiddo requested a review from Copilot April 29, 2025 07:38
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements the capability to load additional resources from npm packages through configuration while maintaining backwards compatibility. The key changes include:

  • Updating the YAML configuration to support additional resource folders for integration guides.
  • Introducing ExtendedPackageInstallationSpec and AdditionalResourceInstaller to process and install extra resources.
  • Integrating the new resource installation flow in StarterJpaConfig and updating AppProperties accordingly.

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/main/resources/application.yaml Updated IG configuration comments to include additionalResourceFolders support.
src/main/java/ca/uhn/fhir/jpa/starter/util/JpaHibernatePropertiesProvider.java Added logger and refactored dialect resolution for an optimized HAPI dialect.
src/main/java/ca/uhn/fhir/jpa/starter/ig/ExtendedPackageInstallationSpec.java Added additionalResourceFolders support in IG package installation spec.
src/main/java/ca/uhn/fhir/jpa/starter/ig/AdditionalResourceInstaller.java Introduced a new service to collect and install extra resources from npm packages.
src/main/java/ca/uhn/fhir/jpa/starter/common/StarterJpaConfig.java Integrated additional resource installation in the package installer flow.
src/main/java/ca/uhn/fhir/jpa/starter/common/PartitionModeConfigurer.java Refactored dependency injection implementation.
src/main/java/ca/uhn/fhir/jpa/starter/common/OnPartitionModeEnabled.java Added conditional configuration for partition mode support.
src/main/java/ca/uhn/fhir/jpa/starter/common/FhirServerConfigCommon.java Removed redundant PartitionModeConfigurer bean registration.
src/main/java/ca/uhn/fhir/jpa/starter/AppProperties.java Updated properties to support extended IG installation specs and additional resource configuration.
Files not reviewed (1)
  • Dockerfile: Language not supported

.map(fileName -> {
try {
return new String(folder.fetchFile(fileName));
} catch (IOException e) {
Copy link
Preview

Copilot AI Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider logging the complete exception details in the catch block when fetching file contents fails, to improve debugging of resource loading issues.

Suggested change
} catch (IOException e) {
} catch (IOException e) {
logger.error("Failed to fetch file '{}' from folder '{}'.", fileName, folder.getPath(), e);

Copilot uses AI. Check for mistakes.

tadgh added a commit to hapifhir/hapi-fhir that referenced this pull request May 2, 2025
* Added reusable feature set from hapifhir/hapi-fhir-jpaserver-starter#784

* Added a few sanity tests

* Update hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/packages/PackageInstallationSpec.java

Co-authored-by: tadgh <[email protected]>

* Addressed most comments

* Reduced package size and added exception

* Added changelog

---------

Co-authored-by: tadgh <[email protected]>
# Conflicts:
#	src/main/resources/application.yaml
@robogary
Copy link

robogary commented May 7, 2025

Formatting check succeeded!

@robogary
Copy link

Formatting check succeeded!

@jkiddo jkiddo marked this pull request as draft May 26, 2025 06:57
Addling the binary content as its needed for example resource inspection
@robogary
Copy link

Formatting check succeeded!

@robogary
Copy link

robogary commented Jun 5, 2025

Formatting check succeeded!

@robogary
Copy link

robogary commented Jun 5, 2025

Formatting check succeeded!

@jkiddo
Copy link
Collaborator Author

jkiddo commented Jun 5, 2025

@dotasek once 8.4 is out the door this should be good to go.

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.

3 participants