Add configurable filesystem binary storage (fix #860) #864

shusheer · 2025-09-29T22:10:28Z

Overview

add first-class configuration for filesystem-backed binary storage so starter users can select the storage mode without custom beans (addresses Enable filesystem binary storage configuration in the starter #860).

Key Changes

expose binary_storage_mode and binary_storage_filesystem_base_directory through AppProperties, keeping DATABASE as the default.
update FhirServerConfigCommon to instantiate the appropriate IBinaryStorageSvc, validate the filesystem base path, and apply a 100 KB inline threshold automatically when no minimum is provided.
ensure JpaStorageSettings receives the same threshold so inline text handling matches the storage service.
document the new knobs in README and sample application.yaml.
add FhirServerConfigCommonBinaryStorageTest verifying default/database wiring, filesystem mode, min-size overrides (including zero), and base-directory validation.

Testing

mvn -Dtest=FhirServerConfigCommonBinaryStorageTest test
manual runs:
1. filesystem with default threshold → 200 KB payload stored on disk, 32 KB remains inline.
2. filesystem with inline_resource_storage_below_size: 16384 → 32 KB payload stored on disk, 8 KB remains inline.

Copilot

Pull Request Overview

This PR adds configurable filesystem binary storage to the HAPI FHIR JPA Server Starter, allowing users to store binary data on disk instead of in the database without requiring custom beans.

Introduces binary_storage_mode and binary_storage_filesystem_base_directory configuration properties
Implements automatic 100 KB inline threshold when filesystem mode is enabled without explicit size configuration
Adds comprehensive test coverage for the new binary storage configuration options

Reviewed Changes

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

Show a summary per file

File	Description
AppProperties.java	Adds new enum and properties for binary storage mode and filesystem base directory configuration
FhirServerConfigCommon.java	Implements logic to instantiate appropriate binary storage service based on configuration with validation
application.yaml	Documents the new configuration options with examples and usage notes
README.md	Adds documentation section explaining how to configure filesystem binary storage
FhirServerConfigCommonBinaryStorageTest.java	Comprehensive test suite covering all configuration scenarios and validation

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-30T04:59:31Z

src/main/java/ca/uhn/fhir/jpa/starter/common/FhirServerConfigCommon.java

+			if (appProperties.getMax_binary_size() != null) {
+				filesystemSvc.setMaximumBinarySize(appProperties.getMax_binary_size().longValue());
+			}


Inconsistent type handling for max_binary_size. The filesystem implementation uses .longValue() while the database implementation on line 373 passes the Integer directly. This could cause type compatibility issues.

Copilot · 2025-09-30T04:59:31Z

src/main/java/ca/uhn/fhir/jpa/starter/common/FhirServerConfigCommon.java

+		Integer inlineResourceThreshold = appProperties.getInline_resource_storage_below_size();
+		if (inlineResourceThreshold == null
+				&& appProperties.getBinary_storage_mode() == AppProperties.BinaryStorageMode.FILESYSTEM) {
+			inlineResourceThreshold = resolveFilesystemMinimumBinarySize(appProperties);
+		}


The logic for resolving the inline resource threshold is duplicated between jpaStorageSettings() and binaryStorageSvc() methods. Consider extracting this threshold resolution logic to avoid code duplication.

Suggested change

Integer inlineResourceThreshold = appProperties.getInline_resource_storage_below_size();

if (inlineResourceThreshold == null

&& appProperties.getBinary_storage_mode() == AppProperties.BinaryStorageMode.FILESYSTEM) {

inlineResourceThreshold = resolveFilesystemMinimumBinarySize(appProperties);

}

Integer inlineResourceThreshold = resolveInlineResourceThreshold(appProperties);

jkiddo · 2025-10-02T08:32:37Z

src/main/java/ca/uhn/fhir/jpa/starter/common/FhirServerConfigCommon.java

 	@Bean
 	public IBinaryStorageSvc binaryStorageSvc(AppProperties appProperties) {
-		DatabaseBinaryContentStorageSvcImpl binaryStorageSvc = new DatabaseBinaryContentStorageSvcImpl();
+		IBinaryStorageSvc binaryStorageSvc;


The implementations of IBinaryStorageSvc should be defined as Beans and injected as such.

jkiddo

See comments above

jkiddo · 2025-10-03T13:22:23Z

src/main/java/ca/uhn/fhir/jpa/starter/common/FhirServerConfigCommon.java

-		if (appProperties.getMax_binary_size() != null) {
-			binaryStorageSvc.setMaximumBinarySize(appProperties.getMax_binary_size());
+	@Lazy
+	@Bean


Instead of having these declared as lazy you can follow the approach like in

hapi-fhir-jpaserver-starter/src/main/java/ca/uhn/fhir/jpa/starter/mcp/McpServerConfig.java

Line 30 in a499603

@ConditionalOnProperty(

that will guarantee you that only one of those implementation beans created. Then you dont need the ObjectProvider above as well

shusheer · 2025-10-04T21:41:31Z

Apologies, tried to rebase to avoid the merging issues, but may have added complexity in doing so. Hopefully all is clear

Copilot

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

jkiddo · 2025-10-05T18:05:53Z

Looks good so far. Please add an integration test that proves it works when not using the default values.

jkiddo · 2025-10-06T06:41:57Z

With integration tests I mean an entire passthrough like with the rest of the integration tests - which are more end2end.

Copilot

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-06T06:58:28Z

src/main/resources/application.yaml

+    #    validation:
+    #      requests_enabled: true
+    #      responses_enabled: true
+    #    binary_storage_enabled: true
+    #    binary_storage_mode: FILESYSTEM
+    #    binary_storage_filesystem_base_directory: /binstore


Inconsistent indentation inside commented sample properties (extra spaces after #) may confuse users when uncommenting, potentially producing mis-indented YAML. Align these comment lines with surrounding examples (e.g., ' # validation:' and nested keys indented two spaces after the #) for consistency.

Suggested change

# validation:

# requests_enabled: true

# responses_enabled: true

# binary_storage_enabled: true

# binary_storage_mode: FILESYSTEM

# binary_storage_filesystem_base_directory: /binstore

# validation:

# requests_enabled: true

# responses_enabled: true

# binary_storage_enabled: true

# binary_storage_mode: FILESYSTEM

# binary_storage_filesystem_base_directory: /binstore

Copilot · 2025-10-06T06:58:28Z

src/test/java/ca/uhn/fhir/jpa/starter/BinaryStorageIntegrationTest.java

+	private Path baseDir() throws IOException {
+		Path baseDir = Paths.get(BASE_DIRECTORY).toAbsolutePath();
+		Files.createDirectories(baseDir);
+		return baseDir;
+	}


Duplicated baseDir() implementations in two test classes increase maintenance overhead. Extract this method into the BaseBinaryStorageIntegrationTest (parameterized by a supplied directory constant) or a shared utility to eliminate duplication.

Copilot · 2025-10-06T06:58:29Z

src/test/java/ca/uhn/fhir/jpa/starter/BinaryStorageIntegrationTest.java

+	private Path baseDir() throws IOException {
+		Path baseDir = Paths.get(BASE_DIRECTORY).toAbsolutePath();
+		Files.createDirectories(baseDir);
+		return baseDir;
+	}


Duplicated baseDir() implementations in two test classes increase maintenance overhead. Extract this method into the BaseBinaryStorageIntegrationTest (parameterized by a supplied directory constant) or a shared utility to eliminate duplication.

Copilot

Pull Request Overview

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

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-07T19:17:45Z

src/main/java/ca/uhn/fhir/jpa/starter/common/FhirServerConfigCommon.java

 	@Bean
-	public IBinaryStorageSvc binaryStorageSvc(AppProperties appProperties) {
-		DatabaseBinaryContentStorageSvcImpl binaryStorageSvc = new DatabaseBinaryContentStorageSvcImpl();
+	@ConditionalOnProperty(prefix = "hapi.fhir", name = "binary_storage_mode", havingValue = "FILESYSTEM")
+	public FilesystemBinaryStorageSvcImpl filesystemBinaryStorageSvc(AppProperties appProperties) {
+		String baseDirectory = appProperties.getBinary_storage_filesystem_base_directory();
+		Assert.hasText(
+				baseDirectory,
+				"binary_storage_filesystem_base_directory must be provided when binary_storage_mode=FILESYSTEM");
+
+		FilesystemBinaryStorageSvcImpl filesystemSvc = new FilesystemBinaryStorageSvcImpl(baseDirectory);
+		Integer inlineResourceThreshold = resolveInlineResourceThreshold(appProperties);
+		int minimumBinarySize =
+				inlineResourceThreshold == null ? DEFAULT_FILESYSTEM_INLINE_THRESHOLD : inlineResourceThreshold;
+		filesystemSvc.setMinimumBinarySize(minimumBinarySize);
+
+		Integer maxBinarySize = appProperties.getMax_binary_size();
+		if (maxBinarySize != null) {
+			filesystemSvc.setMaximumBinarySize(maxBinarySize.longValue());
+		}
+
+		return filesystemSvc;
+	}

-		if (appProperties.getMax_binary_size() != null) {
-			binaryStorageSvc.setMaximumBinarySize(appProperties.getMax_binary_size());
+	@Bean
+	@ConditionalOnProperty(
+			prefix = "hapi.fhir",
+			name = "binary_storage_mode",
+			havingValue = "DATABASE",
+			matchIfMissing = true)
+	public DatabaseBinaryContentStorageSvcImpl databaseBinaryStorageSvc(AppProperties appProperties) {
+		DatabaseBinaryContentStorageSvcImpl databaseSvc = new DatabaseBinaryContentStorageSvcImpl();
+		Integer maxBinarySize = appProperties.getMax_binary_size();
+		if (maxBinarySize != null) {
+			databaseSvc.setMaximumBinarySize(maxBinarySize.longValue());
 		}
+		return databaseSvc;
+	}


Previously there was a single bean named binaryStorageSvc (returning IBinaryStorageSvc); replacing it with two differently named concrete beans removes that bean name and can break external customizations relying on @qualifier("binaryStorageSvc") or bean name overrides. Consider adding a unifying @bean(name = "binaryStorageSvc") that returns IBinaryStorageSvc and delegates to the active implementation, or add @primary plus an alias to preserve backward compatibility.

no - that is a deliberate choice.

Copilot · 2025-10-07T19:17:45Z

src/main/java/ca/uhn/fhir/jpa/starter/common/FhirServerConfigCommon.java

+		Integer inlineResourceThreshold = resolveInlineResourceThreshold(appProperties);
+		int minimumBinarySize =
+				inlineResourceThreshold == null ? DEFAULT_FILESYSTEM_INLINE_THRESHOLD : inlineResourceThreshold;
+		filesystemSvc.setMinimumBinarySize(minimumBinarySize);
+
+		Integer maxBinarySize = appProperties.getMax_binary_size();
+		if (maxBinarySize != null) {
+			filesystemSvc.setMaximumBinarySize(maxBinarySize.longValue());
+		}


Negative values for inline_resource_storage_below_size or max_binary_size are not validated and could lead to unexpected behavior. Add explicit non-negative assertions (e.g., Assert.isTrue(value >= 0, "...") ) before applying these values.

shusheer · 2025-10-08T00:56:35Z

I see you disagree with copilot on one point, would you like me to implement the non-negative assertion tests? Anything else to adjust?

jkiddo requested a review from Copilot September 30, 2025 04:58

Copilot AI reviewed Sep 30, 2025

View reviewed changes

jkiddo reviewed Oct 2, 2025

View reviewed changes

jkiddo requested changes Oct 2, 2025

View reviewed changes

jkiddo reviewed Oct 3, 2025

View reviewed changes

Ubuntu and others added 4 commits October 4, 2025 19:05

Add configurable filesystem binary storage (fix hapifhir#860)

1773c89

Refine binary storage configuration handling

6f18c79

Refine binary storage bean wiring

d5318d2

Refine binary storage conditional beans

9624446

shusheer force-pushed the master branch from e5c38ed to 9624446 Compare October 4, 2025 21:30

jkiddo requested a review from Copilot October 5, 2025 14:17

Copilot AI reviewed Oct 5, 2025

View reviewed changes

Add integration coverage for binary storage configs

dc29187

jkiddo requested a review from Copilot October 6, 2025 06:56

Copilot AI reviewed Oct 6, 2025

View reviewed changes

Exercise binary storage via REST integration tests

2caff64

jkiddo requested a review from Copilot October 7, 2025 19:15

Copilot AI reviewed Oct 7, 2025

View reviewed changes

Add configurable filesystem binary storage (fix #860) #864

Are you sure you want to change the base?

Add configurable filesystem binary storage (fix #860) #864

Conversation

shusheer commented Sep 29, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

jkiddo Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

jkiddo left a comment

Choose a reason for hiding this comment

Uh oh!

jkiddo Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

shusheer commented Oct 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

jkiddo commented Oct 5, 2025

Uh oh!

jkiddo commented Oct 6, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

jkiddo Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

shusheer commented Oct 8, 2025

Uh oh!

Uh oh!