-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add configurable filesystem binary storage (fix #860) #864
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
base: master
Are you sure you want to change the base?
Changes from 4 commits
1773c89
6f18c79
d5318d2
9624446
dc29187
2caff64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
package ca.uhn.fhir.jpa.starter.common; | ||
|
||
import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; | ||
import ca.uhn.fhir.jpa.binary.api.IBinaryStorageSvc; | ||
import ca.uhn.fhir.jpa.binstore.DatabaseBinaryContentStorageSvcImpl; | ||
import ca.uhn.fhir.jpa.binstore.FilesystemBinaryStorageSvcImpl; | ||
import ca.uhn.fhir.jpa.config.HibernatePropertiesProvider; | ||
import ca.uhn.fhir.jpa.model.config.PartitionSettings; | ||
import ca.uhn.fhir.jpa.model.config.PartitionSettings.CrossPartitionReferenceMode; | ||
|
@@ -19,10 +19,12 @@ | |
import org.hl7.fhir.r4.model.Bundle.BundleType; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; | ||
import org.springframework.boot.env.YamlPropertySourceLoader; | ||
import org.springframework.context.annotation.*; | ||
import org.springframework.orm.jpa.LocalContainerEntityManagerFactoryBean; | ||
import org.springframework.transaction.annotation.EnableTransactionManagement; | ||
import org.springframework.util.Assert; | ||
|
||
import java.util.HashSet; | ||
import java.util.stream.Collectors; | ||
|
@@ -38,6 +40,7 @@ | |
public class FhirServerConfigCommon { | ||
|
||
private static final Logger ourLog = LoggerFactory.getLogger(FhirServerConfigCommon.class); | ||
private static final int DEFAULT_FILESYSTEM_INLINE_THRESHOLD = 102_400; | ||
|
||
public FhirServerConfigCommon(AppProperties appProperties) { | ||
ourLog.info( | ||
|
@@ -222,8 +225,9 @@ public JpaStorageSettings jpaStorageSettings(AppProperties appProperties) { | |
jpaStorageSettings.setLastNEnabled(true); | ||
} | ||
|
||
if (appProperties.getInline_resource_storage_below_size() != 0) { | ||
jpaStorageSettings.setInlineResourceTextBelowSize(appProperties.getInline_resource_storage_below_size()); | ||
Integer inlineResourceThreshold = resolveInlineResourceThreshold(appProperties); | ||
if (inlineResourceThreshold != null && inlineResourceThreshold != 0) { | ||
jpaStorageSettings.setInlineResourceTextBelowSize(inlineResourceThreshold); | ||
} | ||
|
||
jpaStorageSettings.setStoreResourceInHSearchIndex(appProperties.getStore_resource_in_lucene_index_enabled()); | ||
|
@@ -339,16 +343,50 @@ public HibernatePropertiesProvider jpaStarterDialectProvider( | |
return new JpaHibernatePropertiesProvider(myEntityManagerFactory); | ||
} | ||
|
||
@Lazy | ||
@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()); | ||
} | ||
Comment on lines
+355
to
+363
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||
|
||
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; | ||
} | ||
Comment on lines
346
to
+381
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no - that is a deliberate choice. |
||
|
||
return binaryStorageSvc; | ||
private Integer resolveInlineResourceThreshold(AppProperties appProperties) { | ||
Integer inlineResourceThreshold = appProperties.getInline_resource_storage_below_size(); | ||
if (inlineResourceThreshold == null | ||
&& appProperties.getBinary_storage_mode() == AppProperties.BinaryStorageMode.FILESYSTEM) { | ||
return DEFAULT_FILESYSTEM_INLINE_THRESHOLD; | ||
} | ||
return inlineResourceThreshold; | ||
} | ||
|
||
@Bean | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -381,6 +381,14 @@ hapi: | |||||||||||||||||||||||||
# max_page_size: 200 | ||||||||||||||||||||||||||
# retain_cached_searches_mins: 60 | ||||||||||||||||||||||||||
# reuse_cached_search_results_millis: 60000 | ||||||||||||||||||||||||||
# validation: | ||||||||||||||||||||||||||
# requests_enabled: true | ||||||||||||||||||||||||||
# responses_enabled: true | ||||||||||||||||||||||||||
# binary_storage_enabled: true | ||||||||||||||||||||||||||
# binary_storage_mode: FILESYSTEM | ||||||||||||||||||||||||||
# binary_storage_filesystem_base_directory: /binstore | ||||||||||||||||||||||||||
Comment on lines
+384
to
+389
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||||||||
# When binary_storage_mode is FILESYSTEM and this value is not set, | ||||||||||||||||||||||||||
# the starter defaults to 102400 bytes so smaller binaries stay inline. | ||||||||||||||||||||||||||
inline_resource_storage_below_size: 4000 | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
# ------------------------------------------------------------------------------- | ||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
package ca.uhn.fhir.jpa.starter.common; | ||
|
||
import ca.uhn.fhir.jpa.binstore.DatabaseBinaryContentStorageSvcImpl; | ||
import ca.uhn.fhir.jpa.binstore.FilesystemBinaryStorageSvcImpl; | ||
import ca.uhn.fhir.jpa.binary.api.IBinaryStorageSvc; | ||
import ca.uhn.fhir.jpa.starter.AppProperties; | ||
import org.junit.jupiter.api.Test; | ||
import org.junit.jupiter.api.io.TempDir; | ||
|
||
import java.nio.file.Files; | ||
import java.nio.file.Path; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
||
class FhirServerConfigCommonBinaryStorageTest { | ||
|
||
@TempDir | ||
Path tempDir; | ||
|
||
private FhirServerConfigCommon newConfig() { | ||
return new FhirServerConfigCommon(new AppProperties()); | ||
} | ||
|
||
@Test | ||
void defaultsToDatabaseImplementation() { | ||
AppProperties props = new AppProperties(); | ||
|
||
IBinaryStorageSvc svc = binaryStorageSvc(props); | ||
|
||
assertThat(svc).isInstanceOf(DatabaseBinaryContentStorageSvcImpl.class); | ||
} | ||
|
||
@Test | ||
void filesystemModeUsesDefaultMinimumWhenUnspecified() throws Exception { | ||
AppProperties props = new AppProperties(); | ||
props.setBinary_storage_mode(AppProperties.BinaryStorageMode.FILESYSTEM); | ||
Path baseDir = tempDir.resolve("fs-default"); | ||
Files.createDirectories(baseDir); | ||
props.setBinary_storage_filesystem_base_directory(baseDir.toString()); | ||
|
||
FilesystemBinaryStorageSvcImpl svc = filesystemBinaryStorageSvc(props); | ||
|
||
assertThat(svc.getMinimumBinarySize()).isEqualTo(102_400); | ||
} | ||
|
||
@Test | ||
void filesystemModeHonoursExplicitMinimum() throws Exception { | ||
AppProperties props = new AppProperties(); | ||
props.setBinary_storage_mode(AppProperties.BinaryStorageMode.FILESYSTEM); | ||
props.setInline_resource_storage_below_size(4096); | ||
Path baseDir = tempDir.resolve("fs-min-explicit"); | ||
Files.createDirectories(baseDir); | ||
props.setBinary_storage_filesystem_base_directory(baseDir.toString()); | ||
|
||
FilesystemBinaryStorageSvcImpl svc = filesystemBinaryStorageSvc(props); | ||
|
||
assertThat(svc.getMinimumBinarySize()).isEqualTo(4096); | ||
} | ||
|
||
@Test | ||
void filesystemModeSupportsZeroMinimumWhenExplicit() throws Exception { | ||
AppProperties props = new AppProperties(); | ||
props.setBinary_storage_mode(AppProperties.BinaryStorageMode.FILESYSTEM); | ||
props.setInline_resource_storage_below_size(0); | ||
Path baseDir = tempDir.resolve("fs-zero"); | ||
Files.createDirectories(baseDir); | ||
props.setBinary_storage_filesystem_base_directory(baseDir.toString()); | ||
|
||
FilesystemBinaryStorageSvcImpl svc = filesystemBinaryStorageSvc(props); | ||
|
||
assertThat(svc.getMinimumBinarySize()).isZero(); | ||
} | ||
|
||
@Test | ||
void filesystemModeRequiresBaseDirectory() { | ||
AppProperties props = new AppProperties(); | ||
props.setBinary_storage_mode(AppProperties.BinaryStorageMode.FILESYSTEM); | ||
|
||
assertThatThrownBy(() -> filesystemBinaryStorageSvc(props)) | ||
.isInstanceOf(IllegalArgumentException.class) | ||
.hasMessageContaining("binary_storage_filesystem_base_directory"); | ||
} | ||
|
||
private IBinaryStorageSvc binaryStorageSvc(AppProperties props) { | ||
FhirServerConfigCommon config = newConfig(); | ||
if (props.getBinary_storage_mode() == AppProperties.BinaryStorageMode.FILESYSTEM) { | ||
return config.filesystemBinaryStorageSvc(props); | ||
} | ||
return config.databaseBinaryStorageSvc(props); | ||
} | ||
|
||
private FilesystemBinaryStorageSvcImpl filesystemBinaryStorageSvc(AppProperties props) { | ||
return (FilesystemBinaryStorageSvcImpl) binaryStorageSvc(props); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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