From dfa08978019423487f4cdbb4fc3d90890e3ca7df Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Thu, 14 May 2026 08:41:19 +0100 Subject: [PATCH 01/16] feat: Replace JClouds with AWS SDK and NIO [DHIS2-20648] --- .../dhis/appmanager/AppStorageSource.java | 2 +- .../hisp/dhis/storage/BlobContainerName.java | 3 +- .../dhis-services/dhis-service-core/pom.xml | 25 +- ...e.java => BlobStoreAppStorageService.java} | 46 +-- .../dhis/appmanager/DefaultAppManager.java | 25 +- .../org/hisp/dhis/jclouds/JCloudsStore.java | 296 -------------- .../hisp/dhis/storage/BlobStoreConfig.java} | 62 ++- .../hisp/dhis/storage/BlobStoreService.java | 14 +- .../FileStoreProvider.java | 10 +- .../storage/FileSystemBlobStoreService.java | 237 +++++++++++ .../hisp/dhis/storage/S3BlobStoreService.java | 387 ++++++++++++++++++ .../storage/TransientBlobStoreService.java | 152 +++++++ ...va => BlobStoreAppStorageServiceTest.java} | 8 +- .../appmanager/DefaultAppManagerTest.java | 4 +- .../hisp/dhis/jclouds/JCloudsStoreTest.java | 76 ---- .../dhis/storage/BlobStoreConfigTest.java | 99 +++++ .../BlobStoreServiceContractTest.java | 13 +- ...leSystemBlobStoreServiceContractTest.java} | 36 +- .../S3BlobStoreServiceContractTest.java | 38 +- ...TransientBlobStoreServiceContractTest.java | 41 +- .../webapi/controller/AppControllerTest.java | 8 +- dhis-2/pom.xml | 48 +-- 22 files changed, 1002 insertions(+), 628 deletions(-) rename dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/{JCloudsAppStorageService.java => BlobStoreAppStorageService.java} (92%) delete mode 100644 dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jclouds/JCloudsStore.java rename dhis-2/dhis-services/dhis-service-core/src/{test/java/org/hisp/dhis/fileresource/JCloudsProviderTest.java => main/java/org/hisp/dhis/storage/BlobStoreConfig.java} (52%) rename dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/{jclouds => storage}/FileStoreProvider.java (87%) create mode 100644 dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java create mode 100644 dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java create mode 100644 dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java rename dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/{JCloudsAppStorageServiceTest.java => BlobStoreAppStorageServiceTest.java} (98%) delete mode 100644 dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/jclouds/JCloudsStoreTest.java create mode 100644 dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/BlobStoreConfigTest.java rename dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/{jclouds => storage}/BlobStoreServiceContractTest.java (97%) rename dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/{jclouds/FilesystemBlobStoreServiceContractTest.java => storage/FileSystemBlobStoreServiceContractTest.java} (66%) rename dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/{jclouds => storage}/S3BlobStoreServiceContractTest.java (68%) rename dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/{jclouds => storage}/TransientBlobStoreServiceContractTest.java (59%) diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/AppStorageSource.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/AppStorageSource.java index f4cbb2a37944..9626c2f45a7a 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/AppStorageSource.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/AppStorageSource.java @@ -33,5 +33,5 @@ * @author Stian Sandvold */ public enum AppStorageSource { - JCLOUDS + BLOB_STORE } diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/storage/BlobContainerName.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/storage/BlobContainerName.java index 0835547e6f51..6c920efb46c0 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/storage/BlobContainerName.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/storage/BlobContainerName.java @@ -38,7 +38,8 @@ * #resolve(BlobKey)} always produces a clean path without any slash-cleaning. * *

Configured via {@link org.hisp.dhis.external.conf.ConfigurationKey#FILESTORE_CONTAINER} and - * resolved once at startup by {@link org.hisp.dhis.jclouds.JCloudsStore}. + * resolved once at startup by the {@link org.hisp.dhis.storage.BlobStoreService} implementation + * selected for the configured {@code filestore.provider}. */ public record BlobContainerName(String value) { diff --git a/dhis-2/dhis-services/dhis-service-core/pom.xml b/dhis-2/dhis-services/dhis-service-core/pom.xml index 60c8a8acd429..80c7ff6f9832 100644 --- a/dhis-2/dhis-services/dhis-service-core/pom.xml +++ b/dhis-2/dhis-services/dhis-service-core/pom.xml @@ -251,32 +251,11 @@ annotations - - - - org.apache.jclouds - jclouds-core - + - org.apache.jclouds - jclouds-blobstore - - - org.apache.jclouds.api - filesystem - - - org.apache.jclouds.api + software.amazon.awssdk s3 - - org.apache.jclouds.provider - aws-s3 - - - org.apache.jclouds.driver - jclouds-slf4j - diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/JCloudsAppStorageService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java similarity index 92% rename from dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/JCloudsAppStorageService.java rename to dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java index 357be4f96eab..249baee90091 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/JCloudsAppStorageService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java @@ -37,7 +37,6 @@ import java.io.File; import java.io.IOException; import java.io.InputStream; -import java.net.MalformedURLException; import java.net.URI; import java.util.Collections; import java.util.Enumeration; @@ -69,6 +68,7 @@ import org.hisp.dhis.util.ZipBombException; import org.hisp.dhis.util.ZipFileUtils; import org.hisp.dhis.util.ZipSlipException; +import org.springframework.core.io.ByteArrayResource; import org.springframework.core.io.FileSystemResource; import org.springframework.core.io.Resource; import org.springframework.core.io.UrlResource; @@ -79,8 +79,8 @@ */ @Slf4j @RequiredArgsConstructor -@Service("org.hisp.dhis.appmanager.JCloudsAppStorageService") -public class JCloudsAppStorageService implements AppStorageService { +@Service("org.hisp.dhis.appmanager.BlobStoreAppStorageService") +public class BlobStoreAppStorageService implements AppStorageService { private static final String BUNDLED_APP_INFO_FILENAME = "bundled-app-info.json"; public static final String MANIFEST_WEBAPP_FILENAME = "manifest.webapp"; @@ -118,7 +118,7 @@ private void handleAppManifest( BiConsumer handler, BlobKeyPrefix folder, InputStream manifestStream) { try (InputStream inputStream = manifestStream) { App app = App.MAPPER.readValue(inputStream, App.class); - app.setAppStorageSource(AppStorageSource.JCLOUDS); + app.setAppStorageSource(AppStorageSource.BLOB_STORE); app.setFolderName(folder.value()); app.setManifestTranslations( readAppManifestTranslations( @@ -234,7 +234,7 @@ public App installApp( app = AppManager.readAppManifest(file, this.jsonMapper, topLevelFolder); folder = AppFolderName.ofKey(app.getKey()); app.setFolderName(folder.path()); - app.setAppStorageSource(AppStorageSource.JCLOUDS); + app.setAppStorageSource(AppStorageSource.BLOB_STORE); } catch (IOException e) { log.error("Failed to install app: Failure during reading manifest from zip file", e); app = new App(); @@ -291,6 +291,7 @@ private void unzipFile(File file, AppFolderName folder, String topLevelFolder) Enumeration entries = zipFile.entries(); while (entries.hasMoreElements()) { ZipEntry zipEntry = entries.nextElement(); + if (zipEntry.isDirectory()) continue; String filePath = getFilePath(folder.path(), topLevelFolder, zipEntry); // If it's the root folder, skip if (filePath == null) continue; @@ -338,19 +339,7 @@ public void deleteApp(@Nonnull App app) { AppFolderName folder = app.appFolder(); blobStore.deleteBlob(folder.resolve(MANIFEST_WEBAPP_FILENAME)); - // TODO(DHIS2-20648) Once the replacement BlobStoreService implementation does recursive - // deleteDirectory on every backend (see contract test), this branch can collapse to a - // single blobStore.deleteDirectory(folder.asPrefix()) call. - if (blobStore.isFilesystem()) { - // Delete all files related to app (works for local filestore) - blobStore.deleteDirectory(folder.asPrefix()); - } else { - // S3: deleteDirectory is not recursive, so enumerate and delete individually - for (BlobKey key : blobStore.listKeys(folder.asPrefix())) { - log.debug("Deleting app file: {}", key); - blobStore.deleteBlob(key); - } - } + blobStore.deleteDirectory(folder.asPrefix()); log.info("Deleted app {}", app.getName()); } @@ -358,9 +347,9 @@ public void deleteApp(@Nonnull App app) { @Nonnull public ResourceResult getAppResource(@CheckForNull App app, @Nonnull String resource) throws IOException { - if (app == null || !app.getAppStorageSource().equals(AppStorageSource.JCLOUDS)) { + if (app == null || !app.getAppStorageSource().equals(AppStorageSource.BLOB_STORE)) { log.warn( - "Can't look up resource {}. The specified app was not found in JClouds storage.", + "Can't look up resource {}. The specified app was not found in blob storage.", resource); return new ResourceNotFound(resource); } @@ -385,14 +374,21 @@ private boolean keyExistsAsDirectory(BlobKey key) { return blobStore.listKeys(BlobKeyPrefix.of(key.value())).iterator().hasNext(); } - private Resource getResource(@Nonnull BlobKey key) throws MalformedURLException { + private Resource getResource(@Nonnull BlobKey key) throws IOException { if (blobStore.isFilesystem()) { return new FileSystemResource( locationManager.getFileForReading(blobStore.container().resolve(key))); - } else { - URI uri = fileResourceContentStore.getSignedGetContentUri(key); + } + URI uri = fileResourceContentStore.getSignedGetContentUri(key); + if (uri != null) { return new UrlResource(uri); } + // Backend supports neither filesystem access nor signed URLs (e.g. transient): buffer + // the blob bytes so Spring can serve them directly. + try (InputStream in = blobStore.openStream(key)) { + if (in == null) throw new IOException("Blob not found: " + key); + return new ByteArrayResource(in.readAllBytes()); + } } /** @@ -429,12 +425,12 @@ private static void logInstallSuccess(App app, String appFolder) { private static void logDiscoveredApps(Map> apps) { if (apps.isEmpty()) { - log.info("No apps found during JClouds discovery."); + log.info("No apps found during app discovery."); } else { apps.values() .forEach( pair -> - log.info("Discovered app '{}' from JClouds storage ", pair.getLeft().getName())); + log.info("Discovered app '{}' from blob storage ", pair.getLeft().getName())); } } } diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/DefaultAppManager.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/DefaultAppManager.java index 2df67b40e6a4..a91e25b5c7bc 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/DefaultAppManager.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/DefaultAppManager.java @@ -106,7 +106,7 @@ public class DefaultAppManager implements AppManager { private final DhisConfigurationProvider dhisConfigurationProvider; private final AppHubService appHubService; - private final AppStorageService jCloudsAppStorageService; + private final AppStorageService blobStoreAppStorageService; private final DatastoreService datastoreService; private final BundledAppManager bundledAppManager; private final I18nManager i18nManager; @@ -120,8 +120,8 @@ public class DefaultAppManager implements AppManager { public DefaultAppManager( DhisConfigurationProvider dhisConfigurationProvider, AppHubService appHubService, - @Qualifier("org.hisp.dhis.appmanager.JCloudsAppStorageService") - AppStorageService jCloudsAppStorageService, + @Qualifier("org.hisp.dhis.appmanager.BlobStoreAppStorageService") + AppStorageService blobStoreAppStorageService, DatastoreService datastoreService, CacheBuilderProvider cacheBuilderProvider, I18nManager i18nManager, @@ -129,7 +129,7 @@ public DefaultAppManager( BundledAppManager bundledAppManager) { checkNotNull(dhisConfigurationProvider); - checkNotNull(jCloudsAppStorageService); + checkNotNull(blobStoreAppStorageService); checkNotNull(datastoreService); checkNotNull(cacheBuilderProvider); checkNotNull(i18nManager); @@ -138,7 +138,7 @@ public DefaultAppManager( this.dhisConfigurationProvider = dhisConfigurationProvider; this.appHubService = appHubService; - this.jCloudsAppStorageService = jCloudsAppStorageService; + this.blobStoreAppStorageService = blobStoreAppStorageService; this.datastoreService = datastoreService; this.appCache = cacheBuilderProvider.newCacheBuilder().forRegion("appCache").build(); this.i18nManager = i18nManager; @@ -154,7 +154,7 @@ public DefaultAppManager( @PostConstruct public void reloadApps() { Map> installedApps = - jCloudsAppStorageService.discoverInstalledApps(); + blobStoreAppStorageService.discoverInstalledApps(); installBundledApps(installedApps); // Invalidate the previous app cache @@ -441,7 +441,7 @@ public App installApp(@Nonnull File file) { @Nonnull private App installAppZipFile(@Nonnull File file, @CheckForNull BundledAppInfo bundledAppInfo) { - App app = jCloudsAppStorageService.installApp(file, appCache, bundledAppInfo); + App app = blobStoreAppStorageService.installApp(file, appCache, bundledAppInfo); log.debug( String.format( "Installed App with AppHub ID %s (status: %s)", app.getAppHubId(), app.getAppState())); @@ -526,7 +526,7 @@ public boolean deleteApp(@Nonnull App app, boolean deleteAppData) { App appFromCache = appOpt.get(); appCache.put(app.getKey(), appFromCache); - jCloudsAppStorageService.deleteApp(app); + blobStoreAppStorageService.deleteApp(app); reloadApps(); // If a bundled version exists it will replace the deleted override. @@ -565,7 +565,7 @@ public boolean isAccessible(App app) { @Override public ResourceResult getRawAppResource(App app, String pageName) throws IOException { - return jCloudsAppStorageService.getAppResource(app, pageName); + return blobStoreAppStorageService.getAppResource(app, pageName); } @Override @@ -630,12 +630,7 @@ public ResourceResult getAppResource(App app, String pageName, String contextPat @Override public int getUriContentLength(@Nonnull Resource resource) { try { - if (resource.isFile()) { - return (int) resource.contentLength(); - } else { - URLConnection urlConnection = resource.getURL().openConnection(); - return urlConnection.getContentLength(); - } + return (int) resource.contentLength(); } catch (IOException e) { log.error("Error trying to retrieve content length of Resource: {}", e.getMessage()); return -1; diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jclouds/JCloudsStore.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jclouds/JCloudsStore.java deleted file mode 100644 index 6f33b7ca9ca7..000000000000 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jclouds/JCloudsStore.java +++ /dev/null @@ -1,296 +0,0 @@ -/* - * Copyright (c) 2004-2024, University of Oslo - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are met: - * - * 1. Redistributions of source code must retain the above copyright notice, this - * list of conditions and the following disclaimer. - * - * 2. Redistributions in binary form must reproduce the above copyright notice, - * this list of conditions and the following disclaimer in the documentation - * and/or other materials provided with the distribution. - * - * 3. Neither the name of the copyright holder nor the names of its contributors - * may be used to endorse or promote products derived from this software without - * specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED - * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE - * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR - * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES - * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; - * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON - * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS - * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - */ -package org.hisp.dhis.jclouds; - -import static org.jclouds.Constants.PROPERTY_ENDPOINT; -import static org.jclouds.blobstore.options.ListContainerOptions.Builder.prefix; - -import com.google.common.hash.HashCode; -import java.io.IOException; -import java.io.InputStream; -import java.net.URI; -import java.util.Properties; -import java.util.Set; -import javax.annotation.CheckForNull; -import javax.annotation.PostConstruct; -import javax.annotation.PreDestroy; -import lombok.extern.slf4j.Slf4j; -import org.apache.commons.lang3.StringUtils; -import org.hisp.dhis.external.conf.ConfigurationKey; -import org.hisp.dhis.external.conf.DhisConfigurationProvider; -import org.hisp.dhis.external.location.LocationManager; -import org.hisp.dhis.storage.BlobContainerName; -import org.hisp.dhis.storage.BlobKey; -import org.hisp.dhis.storage.BlobKeyPrefix; -import org.hisp.dhis.storage.BlobStoreService; -import org.hisp.dhis.storage.BlobStoreService.ContentDisposition; -import org.hisp.dhis.storage.ContentHash; -import org.jclouds.ContextBuilder; -import org.jclouds.blobstore.BlobRequestSigner; -import org.jclouds.blobstore.BlobStore; -import org.jclouds.blobstore.BlobStoreContext; -import org.jclouds.blobstore.LocalBlobRequestSigner; -import org.jclouds.blobstore.domain.Blob; -import org.jclouds.blobstore.domain.StorageMetadata; -import org.jclouds.blobstore.internal.RequestSigningUnsupported; -import org.jclouds.domain.Location; -import org.jclouds.domain.LocationBuilder; -import org.jclouds.domain.LocationScope; -import org.jclouds.filesystem.reference.FilesystemConstants; -import org.jclouds.http.HttpRequest; -import org.jclouds.logging.slf4j.config.SLF4JLoggingModule; -import org.jclouds.s3.reference.S3Constants; -import org.springframework.stereotype.Component; - -/** - * JClouds-backed implementation of {@link BlobStoreService}. Manages the JClouds {@link - * BlobStoreContext} and initializes the container {@link ConfigurationKey#FILESTORE_CONTAINER} in - * which DHIS2 stores files/images/icons/apps. - */ -@Slf4j -@Component -public class JCloudsStore implements BlobStoreService { - - private final LocationManager locationManager; - private final FileStoreConfig fileStoreConfig; - private final BlobStoreContext blobStoreContext; - - JCloudsStore(DhisConfigurationProvider configurationProvider, LocationManager locationManager) { - this.locationManager = locationManager; - - FileStoreProvider provider = - FileStoreProvider.of( - configurationProvider.getProperty(ConfigurationKey.FILESTORE_PROVIDER)); - String location = configurationProvider.getProperty(ConfigurationKey.FILESTORE_LOCATION); - String endpoint = configurationProvider.getProperty(ConfigurationKey.FILESTORE_ENDPOINT); - String container = configurationProvider.getProperty(ConfigurationKey.FILESTORE_CONTAINER); - - validateFilesystemProvider(provider); - - fileStoreConfig = new FileStoreConfig(provider, location, container); - - String identity = configurationProvider.getProperty(ConfigurationKey.FILESTORE_IDENTITY); - String secret = configurationProvider.getProperty(ConfigurationKey.FILESTORE_SECRET); - - blobStoreContext = - ContextBuilder.newBuilder(provider.key()) - .credentials(identity, secret) - .overrides(configureOverrides(provider, endpoint)) - .modules(Set.of(new SLF4JLoggingModule())) - .build(BlobStoreContext.class); - } - - @PostConstruct - public void init() { - Location location = createLocation(fileStoreConfig.provider.key(), fileStoreConfig.location); - blobStoreContext.getBlobStore().createContainerInLocation(location, fileStoreConfig.container); - - log.info( - "File store configured with provider: '{}', container: '{}' and location: '{}'.", - fileStoreConfig.provider.key(), - fileStoreConfig.container, - fileStoreConfig.location); - } - - @PreDestroy - public void cleanUp() { - blobStoreContext.close(); - } - - @Override - public boolean blobExists(BlobKey key) { - return key != null && getBlobStore().blobExists(fileStoreConfig.container, key.value()); - } - - @Override - @CheckForNull - public InputStream openStream(BlobKey key) { - Blob blob = getBlobStore().getBlob(fileStoreConfig.container, key.value()); - if (blob == null) { - return null; - } - try { - return blob.getPayload().openStream(); - } catch (IOException e) { - log.warn("Unable to open stream for key: {}. {}", key, e.getMessage()); - return null; - } - } - - @Override - public long contentLength(BlobKey key) { - Blob blob = getBlobStore().getBlob(fileStoreConfig.container, key.value()); - if (blob == null) { - return 0; - } - return blob.getMetadata().getContentMetadata().getContentLength(); - } - - @Override - public void putBlob( - BlobKey key, - InputStream content, - long contentLength, - @CheckForNull String contentType, - @CheckForNull ContentDisposition contentDisposition, - @CheckForNull ContentHash contentHash) { - BlobStore bs = getBlobStore(); - var builder = bs.blobBuilder(key.value()).payload(content).contentLength(contentLength); - if (StringUtils.isNotEmpty(contentType)) { - builder.contentType(contentType); - } - if (contentDisposition != null) { - builder.contentDisposition(contentDisposition.value()); - } - if (contentHash != null) { - builder.contentMD5(HashCode.fromString(contentHash.hex())); - } - bs.putBlob(fileStoreConfig.container, builder.build()); - } - - @Override - public void deleteBlob(BlobKey key) { - getBlobStore().removeBlob(fileStoreConfig.container, key.value()); - } - - @Override - public void deleteDirectory(BlobKeyPrefix prefix) { - getBlobStore().deleteDirectory(fileStoreConfig.container, prefix.value()); - } - - @Override - public Iterable listFolders(BlobKeyPrefix prefix) { - // JClouds directory listing requires a trailing "/" on the prefix - String jcloudsPrefix = prefix.value() + "/"; - return getBlobStore() - .list(fileStoreConfig.container, prefix(jcloudsPrefix).delimiter("/")) - .stream() - .map(m -> BlobKeyPrefix.of(m.getName())) - .toList(); - } - - @Override - public Iterable listKeys(BlobKeyPrefix prefix) { - return getBlobStore() - .list(fileStoreConfig.container, prefix(prefix.value()).recursive()) - .stream() - .map(StorageMetadata::getName) - .map(BlobKey::new) - .toList(); - } - - @Override - @CheckForNull - public URI signedGetUri(BlobKey key, long expirationSeconds) { - BlobRequestSigner signer = blobStoreContext.getSigner(); - if (signer instanceof RequestSigningUnsupported || signer instanceof LocalBlobRequestSigner) { - return null; - } - try { - HttpRequest httpRequest = - signer.signGetBlob(fileStoreConfig.container, key.value(), expirationSeconds); - return httpRequest.getEndpoint(); - } catch (UnsupportedOperationException e) { - return null; - } - } - - @Override - public BlobContainerName container() { - return new BlobContainerName(fileStoreConfig.container); - } - - @Override - public boolean isFilesystem() { - return fileStoreConfig.provider == FileStoreProvider.FILESYSTEM; - } - - private void validateFilesystemProvider(FileStoreProvider provider) { - if (provider == FileStoreProvider.FILESYSTEM && !locationManager.externalDirectorySet()) { - throw new IllegalArgumentException( - "File system file store provider could not be configured; external directory is not set. "); - } - } - - private Properties configureOverrides(FileStoreProvider provider, String endpoint) { - if (provider == FileStoreProvider.FILESYSTEM && locationManager.externalDirectorySet()) { - Properties overrides = new Properties(); - overrides.setProperty( - FilesystemConstants.PROPERTY_BASEDIR, locationManager.getExternalDirectoryPath()); - return overrides; - } - - if (provider == FileStoreProvider.AWS_S3) { - Properties overrides = new Properties(); - overrides.setProperty(S3Constants.PROPERTY_S3_VIRTUAL_HOST_BUCKETS, "false"); - return overrides; - } - - if (provider == FileStoreProvider.S3) { - Properties overrides = new Properties(); - overrides.setProperty(S3Constants.PROPERTY_S3_VIRTUAL_HOST_BUCKETS, "false"); - - if (StringUtils.isNotEmpty(endpoint)) { - overrides.setProperty(PROPERTY_ENDPOINT, endpoint); - } - return overrides; - } - - return new Properties(); - } - - private static Location createLocation(String provider, String location) { - if (location == null) { - // some BlobStores allow specifying a location, such as US-EAST, where containers will exist. - // null will choose a default location. - return null; - } - - Location parent = - new LocationBuilder() - .scope(LocationScope.PROVIDER) - .id(provider) - .description(provider) - .build(); - - return new LocationBuilder() - .scope(LocationScope.REGION) - .id(location) - .description(location) - .parent(parent) - .build(); - } - - private record FileStoreConfig(FileStoreProvider provider, String location, String container) {} - - private BlobStore getBlobStore() { - return blobStoreContext.getBlobStore(); - } -} diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/fileresource/JCloudsProviderTest.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreConfig.java similarity index 52% rename from dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/fileresource/JCloudsProviderTest.java rename to dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreConfig.java index ab60c7644ffd..15ad6b5c1790 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/fileresource/JCloudsProviderTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreConfig.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-2022, University of Oslo + * Copyright (c) 2004-2026, University of Oslo * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -12,7 +12,7 @@ * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. * - * 3. Neither the name of the copyright holder nor the names of its contributors + * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software without * specific prior written permission. * @@ -27,43 +27,37 @@ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -package org.hisp.dhis.fileresource; +package org.hisp.dhis.storage; -import static org.jclouds.ContextBuilder.newBuilder; -import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; -import static org.junit.jupiter.api.Assertions.assertThrows; - -import java.util.NoSuchElementException; -import org.junit.jupiter.api.Test; +import org.hisp.dhis.external.conf.ConfigurationKey; +import org.hisp.dhis.external.conf.DhisConfigurationProvider; +import org.hisp.dhis.external.location.LocationManager; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; /** - * Verify that the supported jclouds providers can be selected. + * Selects the {@link BlobStoreService} implementation at startup based on {@link + * ConfigurationKey#FILESTORE_PROVIDER}: * - * @author Jim Grace + *

*/ -class JCloudsProviderTest { - @Test - void verifyFilesystem() { - assertDoesNotThrow(() -> newBuilder("filesystem")); - } - - @Test - void verifyAwsS3() { - assertDoesNotThrow(() -> newBuilder("aws-s3")); - } - - @Test - void verifyS3() { - assertDoesNotThrow(() -> newBuilder("s3")); - } - - @Test - void verifyInvalidProvider() { - assertThrows(NoSuchElementException.class, () -> newBuilder("s4")); - } +@Configuration +public class BlobStoreConfig { - @Test - void verifyTransient() { - assertDoesNotThrow(() -> newBuilder("transient")); + @Bean + public BlobStoreService blobStoreService( + DhisConfigurationProvider configurationProvider, LocationManager locationManager) { + FileStoreProvider provider = + FileStoreProvider.of( + configurationProvider.getProperty(ConfigurationKey.FILESTORE_PROVIDER)); + return switch (provider) { + case S3, AWS_S3 -> new S3BlobStoreService(configurationProvider); + case FILESYSTEM -> new FileSystemBlobStoreService(configurationProvider, locationManager); + case TRANSIENT -> new TransientBlobStoreService(configurationProvider); + }; } } diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreService.java index 84882b0ce56d..3ad6295355a4 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreService.java @@ -35,9 +35,10 @@ import javax.annotation.Nonnull; /** - * Provider-agnostic abstraction over a blob/object store. Implementations exist for JClouds - * (current) and will be added for other backends (e.g. MinIO SDK + NIO filesystem) when JClouds is - * replaced. + * Provider-agnostic abstraction over a blob/object store. Implementations: {@link + * S3BlobStoreService} (AWS SDK v2, for {@code s3} / {@code aws-s3}), {@link + * FileSystemBlobStoreService} (NIO, for {@code filesystem}), and {@link TransientBlobStoreService} + * (in-memory, for {@code transient}). Selection is performed at startup by {@link BlobStoreConfig}. * *

All methods operate against a single container/bucket configured at startup. Keys are * path-like strings (e.g. {@code apps/my-app/index.html}). @@ -111,10 +112,6 @@ void putBlob( * Recursively deletes all blobs whose key starts with {@code prefix}. On filesystem backends this * maps to a directory delete; on object-store backends it performs per-key deletion. */ - // TODO(DHIS2-20648) The replacement implementation must honour this recursively for ALL - // backends. The current jclouds-transient and jclouds-S3 paths are non-recursive (callers - // work around it in JCloudsAppStorageService#deleteApp); see BlobStoreServiceContractTest's - // supportsRecursiveDirectoryDelete capability hook — drop the false overrides once fixed. void deleteDirectory(BlobKeyPrefix prefix); /** @@ -128,9 +125,6 @@ void putBlob( * iterable if no matching blobs exist. Returned keys identify real blobs only — synthetic * directory marker entries (values ending with {@code /}) must not be returned. */ - // TODO(DHIS2-20648) The replacement implementation must filter directory markers. The current - // jclouds-filesystem provider emits them; see BlobStoreServiceContractTest's - // listKeysIncludesDirectoryMarkers capability hook — drop the true override once fixed. Iterable listKeys(BlobKeyPrefix prefix); /** diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jclouds/FileStoreProvider.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileStoreProvider.java similarity index 87% rename from dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jclouds/FileStoreProvider.java rename to dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileStoreProvider.java index 9674681f11a6..bcd69c21d687 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jclouds/FileStoreProvider.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileStoreProvider.java @@ -12,7 +12,7 @@ * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. * - * 3. Neither the name of the copyright holder nor the names of its contributors + * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software without * specific prior written permission. * @@ -27,11 +27,12 @@ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -package org.hisp.dhis.jclouds; +package org.hisp.dhis.storage; import java.util.Arrays; +import java.util.stream.Collectors; -/** The set of JClouds blob-store providers supported by DHIS2. */ +/** The set of blob-store backends DHIS2 supports for {@code filestore.provider}. */ public enum FileStoreProvider { FILESYSTEM("filesystem"), AWS_S3("aws-s3"), @@ -62,6 +63,7 @@ public static FileStoreProvider of(String key) { new IllegalArgumentException( "Unsupported file store provider '" + key - + "'. Supported values are: filesystem, aws-s3, s3, transient.")); + + "'. Supported values are: " + + Arrays.stream(values()).map(FileStoreProvider::key).collect(Collectors.joining(", ")))); } } diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java new file mode 100644 index 000000000000..589523d55419 --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java @@ -0,0 +1,237 @@ +/* + * Copyright (c) 2004-2026, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.storage; + +import java.io.IOException; +import java.io.InputStream; +import java.io.UncheckedIOException; +import java.net.URI; +import java.nio.file.DirectoryStream; +import java.nio.file.FileVisitResult; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.SimpleFileVisitor; +import java.nio.file.StandardCopyOption; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; +import javax.annotation.PostConstruct; +import lombok.extern.slf4j.Slf4j; +import org.hisp.dhis.external.conf.ConfigurationKey; +import org.hisp.dhis.external.conf.DhisConfigurationProvider; +import org.hisp.dhis.external.location.LocationManager; + +/** + * NIO-based implementation of {@link BlobStoreService} replacing the JClouds {@code filesystem} + * path. Blobs are stored as plain files under {@code //}. + * + *

The backend is unsigned and unmetadata-aware: {@code contentType}, {@code contentDisposition}, + * and {@code contentHash} are accepted but not persisted (no native filesystem support, and the + * contract only requires they not throw). {@link #signedGetUri} returns {@code null}. + */ +@Slf4j +public class FileSystemBlobStoreService implements BlobStoreService { + + private final BlobContainerName container; + private final Path baseDir; + + public FileSystemBlobStoreService( + DhisConfigurationProvider configurationProvider, LocationManager locationManager) { + if (!locationManager.externalDirectorySet()) { + throw new IllegalStateException( + "File system blob store cannot start: external directory is not set."); + } + String containerName = + configurationProvider.getProperty(ConfigurationKey.FILESTORE_CONTAINER); + this.container = new BlobContainerName(containerName); + this.baseDir = Paths.get(locationManager.getExternalDirectoryPath(), containerName); + } + + @PostConstruct + public void init() { + try { + Files.createDirectories(baseDir); + } catch (IOException e) { + throw new UncheckedIOException( + "Unable to create file store directory at " + baseDir, e); + } + log.info("Filesystem blob store rooted at: '{}'", baseDir); + } + + @Override + public boolean blobExists(BlobKey key) { + if (key == null) return false; + Path p = resolve(key); + return Files.isRegularFile(p); + } + + @Override + @CheckForNull + public InputStream openStream(BlobKey key) { + Path p = resolve(key); + if (!Files.isRegularFile(p)) return null; + try { + return Files.newInputStream(p); + } catch (IOException e) { + log.warn("Unable to open stream for key: {}. {}", key, e.getMessage()); + return null; + } + } + + @Override + public long contentLength(BlobKey key) { + Path p = resolve(key); + if (!Files.isRegularFile(p)) return 0L; + try { + return Files.size(p); + } catch (IOException e) { + return 0L; + } + } + + @Override + public void putBlob( + BlobKey key, + InputStream content, + long contentLength, + @CheckForNull String contentType, + @CheckForNull ContentDisposition contentDisposition, + @CheckForNull ContentHash contentHash) { + Path target = resolve(key); + try { + Files.createDirectories(target.getParent()); + Files.copy(content, target, StandardCopyOption.REPLACE_EXISTING); + } catch (IOException e) { + throw new UncheckedIOException("Unable to write blob to " + target, e); + } + } + + @Override + public void deleteBlob(BlobKey key) { + try { + Files.deleteIfExists(resolve(key)); + } catch (IOException e) { + throw new UncheckedIOException("Unable to delete blob at " + resolve(key), e); + } + } + + @Override + public void deleteDirectory(BlobKeyPrefix prefix) { + Path dir = baseDir.resolve(prefix.value()); + if (!Files.isDirectory(dir)) return; + try (var paths = Files.walk(dir)) { + paths + .sorted(Comparator.reverseOrder()) + .forEach( + p -> { + try { + Files.deleteIfExists(p); + } catch (IOException e) { + throw new UncheckedIOException("Unable to delete " + p, e); + } + }); + } catch (IOException e) { + throw new UncheckedIOException("Unable to walk directory " + dir, e); + } + } + + @Override + public Iterable listFolders(BlobKeyPrefix prefix) { + Path dir = baseDir.resolve(prefix.value()); + if (!Files.isDirectory(dir)) return List.of(); + List result = new ArrayList<>(); + try (DirectoryStream stream = Files.newDirectoryStream(dir, Files::isDirectory)) { + for (Path child : stream) { + Path relative = baseDir.relativize(child); + result.add(BlobKeyPrefix.of(toBlobPath(relative))); + } + } catch (IOException e) { + throw new UncheckedIOException("Unable to list folders under " + dir, e); + } + return result; + } + + @Override + public Iterable listKeys(BlobKeyPrefix prefix) { + Path dir = baseDir.resolve(prefix.value()); + if (!Files.isDirectory(dir)) return List.of(); + List result = new ArrayList<>(); + try { + Files.walkFileTree( + dir, + new SimpleFileVisitor<>() { + @Nonnull + @Override + public FileVisitResult visitFile(@Nonnull Path file, @Nonnull BasicFileAttributes attrs) { + Path relative = baseDir.relativize(file); + result.add(new BlobKey(toBlobPath(relative))); + return FileVisitResult.CONTINUE; + } + }); + } catch (IOException e) { + throw new UncheckedIOException("Unable to walk directory " + dir, e); + } + return result; + } + + @Override + @CheckForNull + public URI signedGetUri(BlobKey key, long expirationSeconds) { + return null; + } + + @Override + public BlobContainerName container() { + return container; + } + + @Override + public boolean isFilesystem() { + return true; + } + + private Path resolve(BlobKey key) { + return baseDir.resolve(key.value()); + } + + /** Converts a relative {@link Path} to a blob-key path using {@code /} regardless of platform. */ + private static String toBlobPath(Path relative) { + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < relative.getNameCount(); i++) { + if (i > 0) sb.append('/'); + sb.append(relative.getName(i)); + } + return sb.toString(); + } +} diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java new file mode 100644 index 000000000000..ac5005b24430 --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java @@ -0,0 +1,387 @@ +/* + * Copyright (c) 2004-2026, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.storage; + +import java.io.IOException; +import java.io.InputStream; +import java.net.URI; +import java.net.URISyntaxException; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.time.Duration; +import java.util.ArrayList; +import java.util.Base64; +import java.util.HexFormat; +import java.util.List; +import javax.annotation.CheckForNull; +import javax.annotation.PostConstruct; +import javax.annotation.PreDestroy; +import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.StringUtils; +import org.hisp.dhis.external.conf.ConfigurationKey; +import org.hisp.dhis.external.conf.DhisConfigurationProvider; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; +import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.core.interceptor.Context; +import software.amazon.awssdk.core.interceptor.ExecutionAttributes; +import software.amazon.awssdk.core.interceptor.ExecutionInterceptor; +import software.amazon.awssdk.core.interceptor.SdkExecutionAttribute; +import software.amazon.awssdk.core.sync.RequestBody; +import software.amazon.awssdk.http.SdkHttpRequest; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.S3ClientBuilder; +import software.amazon.awssdk.services.s3.S3Configuration; +import software.amazon.awssdk.services.s3.model.BucketAlreadyOwnedByYouException; +import software.amazon.awssdk.services.s3.model.CommonPrefix; +import software.amazon.awssdk.services.s3.model.DeleteObjectsRequest; +import software.amazon.awssdk.services.s3.model.DeleteObjectsResponse; +import software.amazon.awssdk.services.s3.model.HeadObjectResponse; +import software.amazon.awssdk.services.s3.model.ListObjectsV2Request; +import software.amazon.awssdk.services.s3.model.ListObjectsV2Response; +import software.amazon.awssdk.services.s3.model.NoSuchBucketException; +import software.amazon.awssdk.services.s3.model.NoSuchKeyException; +import software.amazon.awssdk.services.s3.model.ObjectIdentifier; +import software.amazon.awssdk.services.s3.model.PutObjectRequest; +import software.amazon.awssdk.services.s3.model.S3Object; +import software.amazon.awssdk.services.s3.presigner.S3Presigner; +import software.amazon.awssdk.services.s3.presigner.S3Presigner.Builder; +import software.amazon.awssdk.services.s3.presigner.model.GetObjectPresignRequest; + +/** + * AWS SDK v2 implementation of {@link BlobStoreService} for the {@code s3} and {@code aws-s3} file + * store providers. + * + *

Targets both real AWS S3 and S3-compatible endpoints (e.g. MinIO, Ceph). When {@link + * ConfigurationKey#FILESTORE_ENDPOINT} is set, the client is pointed at that endpoint with + * path-style addressing enabled (the only mode supported by most S3-compatible servers). + */ +@Slf4j +public class S3BlobStoreService implements BlobStoreService { + + /** Hard cap on objects per S3 {@code DeleteObjects} request; enforced by the service. */ + private static final int DELETE_BATCH_SIZE = 1000; + + private final BlobContainerName container; + private final S3Client s3; + private final S3Presigner presigner; + + public S3BlobStoreService(DhisConfigurationProvider configurationProvider) { + String containerName = + configurationProvider.getProperty(ConfigurationKey.FILESTORE_CONTAINER); + String location = configurationProvider.getProperty(ConfigurationKey.FILESTORE_LOCATION); + String endpoint = configurationProvider.getProperty(ConfigurationKey.FILESTORE_ENDPOINT); + String identity = configurationProvider.getProperty(ConfigurationKey.FILESTORE_IDENTITY); + String secret = configurationProvider.getProperty(ConfigurationKey.FILESTORE_SECRET); + + this.container = new BlobContainerName(containerName); + + // SDK v2 requires a region even when an endpoint override is set; MinIO ≥ 2025-04 rejects a + // region that disagrees with the bucket's LocationConstraint, so fall back to us-east-1 (the + // S3 default that translates to no LocationConstraint on bucket creation) when unset. + Region region = StringUtils.isNotBlank(location) ? Region.of(location) : Region.US_EAST_1; + + // Fall back to the SDK's default credential chain (env vars, system props, IAM role, EC2 + // metadata, ...) when identity/secret are not explicitly configured — typical for AWS-hosted + // deployments that rely on IAM. Static credentials are used otherwise. + AwsCredentialsProvider credentials = + StringUtils.isNotBlank(identity) && StringUtils.isNotBlank(secret) + ? StaticCredentialsProvider.create(AwsBasicCredentials.create(identity, secret)) + : DefaultCredentialsProvider.builder().build(); + + // Path-style addressing is required for custom endpoints (MinIO/Ceph) and applied uniformly + // here for aws-s3 as well, matching the addressing mode used historically. + S3Configuration s3Config = S3Configuration.builder().pathStyleAccessEnabled(true).build(); + + // SDK ≥ 2.30 dropped legacy Content-MD5 in favour of x-amz-checksum-* headers for + // httpChecksumRequired operations. Older MinIO/Ceph builds reject DeleteObjects without + // Content-MD5, so we re-add it via an interceptor below. + S3ClientBuilder clientBuilder = + S3Client.builder() + .region(region) + .credentialsProvider(credentials) + .serviceConfiguration(s3Config) + .overrideConfiguration( + o -> o.addExecutionInterceptor(new DeleteObjectsContentMd5Interceptor())); + Builder presignerBuilder = + S3Presigner.builder() + .region(region) + .credentialsProvider(credentials) + .serviceConfiguration(s3Config); + + if (StringUtils.isNotBlank(endpoint)) { + URI endpointUri = URI.create(endpoint); + clientBuilder.endpointOverride(endpointUri); + presignerBuilder.endpointOverride(endpointUri); + } + + this.s3 = clientBuilder.build(); + this.presigner = presignerBuilder.build(); + } + + @PostConstruct + public void init() { + String bucket = container.value(); + try { + s3.headBucket(b -> b.bucket(bucket)); + } catch (NoSuchBucketException e) { + try { + s3.createBucket(b -> b.bucket(bucket)); + } catch (BucketAlreadyOwnedByYouException ignored) { + // Concurrent startup or external creation — bucket exists, nothing to do. + } + } + log.info("S3 file store configured with bucket: '{}'.", bucket); + } + + @PreDestroy + public void cleanUp() { + s3.close(); + presigner.close(); + } + + @Override + public boolean blobExists(BlobKey key) { + if (key == null) return false; + try { + s3.headObject(b -> b.bucket(container.value()).key(key.value())); + return true; + } catch (NoSuchKeyException e) { + return false; + } + } + + @Override + @CheckForNull + public InputStream openStream(BlobKey key) { + try { + return s3.getObject(b -> b.bucket(container.value()).key(key.value())); + } catch (NoSuchKeyException e) { + return null; + } + } + + @Override + public long contentLength(BlobKey key) { + try { + HeadObjectResponse head = s3.headObject(b -> b.bucket(container.value()).key(key.value())); + return head.contentLength(); + } catch (NoSuchKeyException e) { + return 0L; + } + } + + @Override + public void putBlob( + BlobKey key, + InputStream content, + long contentLength, + @CheckForNull String contentType, + @CheckForNull ContentDisposition contentDisposition, + @CheckForNull ContentHash contentHash) { + PutObjectRequest.Builder put = + PutObjectRequest.builder() + .bucket(container.value()) + .key(key.value()) + .contentLength(contentLength); + if (StringUtils.isNotEmpty(contentType)) { + put.contentType(contentType); + } + if (contentDisposition != null) { + put.contentDisposition(contentDisposition.value()); + } + if (contentHash != null) { + // S3 expects the Content-MD5 header as base64-encoded bytes; ContentHash stores hex. + put.contentMD5(hexToBase64(contentHash.hex())); + } + s3.putObject(put.build(), RequestBody.fromInputStream(content, contentLength)); + } + + @Override + public void deleteBlob(BlobKey key) { + s3.deleteObject(b -> b.bucket(container.value()).key(key.value())); + } + + @Override + public void deleteDirectory(BlobKeyPrefix prefix) { + String bucket = container.value(); + String listPrefix = prefix.value() + "/"; + String continuationToken = null; + do { + ListObjectsV2Request.Builder list = + ListObjectsV2Request.builder().bucket(bucket).prefix(listPrefix); + if (continuationToken != null) { + list.continuationToken(continuationToken); + } + ListObjectsV2Response resp = s3.listObjectsV2(list.build()); + + List ids = new ArrayList<>(resp.contents().size()); + for (S3Object obj : resp.contents()) { + ids.add(ObjectIdentifier.builder().key(obj.key()).build()); + if (ids.size() == DELETE_BATCH_SIZE) { + bulkDelete(bucket, ids); + ids.clear(); + } + } + if (!ids.isEmpty()) { + bulkDelete(bucket, ids); + } + + continuationToken = Boolean.TRUE.equals(resp.isTruncated()) ? resp.nextContinuationToken() : null; + } while (continuationToken != null); + } + + @Override + public Iterable listFolders(BlobKeyPrefix prefix) { + String listPrefix = prefix.value() + "/"; + ListObjectsV2Response resp = + s3.listObjectsV2( + b -> b.bucket(container.value()).prefix(listPrefix).delimiter("/")); + return resp.commonPrefixes().stream() + .map(CommonPrefix::prefix) + .map(BlobKeyPrefix::of) + .toList(); + } + + @Override + public Iterable listKeys(BlobKeyPrefix prefix) { + List result = new ArrayList<>(); + String bucket = container.value(); + String continuationToken = null; + do { + ListObjectsV2Request.Builder list = + ListObjectsV2Request.builder().bucket(bucket).prefix(prefix.value()); + if (continuationToken != null) { + list.continuationToken(continuationToken); + } + ListObjectsV2Response resp = s3.listObjectsV2(list.build()); + for (S3Object obj : resp.contents()) { + // Skip synthetic directory markers — some S3-compatible backends emit them. + if (obj.key().endsWith("/")) continue; + result.add(new BlobKey(obj.key())); + } + continuationToken = Boolean.TRUE.equals(resp.isTruncated()) ? resp.nextContinuationToken() : null; + } while (continuationToken != null); + return result; + } + + @Override + @CheckForNull + public URI signedGetUri(BlobKey key, long expirationSeconds) { + GetObjectPresignRequest presignRequest = + GetObjectPresignRequest.builder() + .signatureDuration(Duration.ofSeconds(expirationSeconds)) + .getObjectRequest(r -> r.bucket(container.value()).key(key.value()).build()) + .build(); + try { + return presigner.presignGetObject(presignRequest).url().toURI(); + } catch (URISyntaxException e) { + throw new IllegalStateException( + "Presigner produced a malformed URL for key: " + key.value(), e); + } + } + + @Override + public BlobContainerName container() { + return container; + } + + @Override + public boolean isFilesystem() { + return false; + } + + private void bulkDelete(String bucket, List ids) { + DeleteObjectsResponse resp = + s3.deleteObjects( + DeleteObjectsRequest.builder() + .bucket(bucket) + .delete(d -> d.objects(ids).build()) + .build()); + log.debug( + "bulkDelete: requested {} keys, deleted {}, errors {}", + ids.size(), + resp.deleted().size(), + resp.errors().size()); + resp.errors() + .forEach( + err -> + log.warn( + "bulkDelete error: key='{}' code='{}' message='{}'", + err.key(), + err.code(), + err.message())); + } + + private static String hexToBase64(String hex) { + return Base64.getEncoder().encodeToString(HexFormat.of().parseHex(hex)); + } + + /** + * Adds a Content-MD5 header to S3 {@code DeleteObjects} requests for compatibility with + * S3-compatible servers (e.g. older MinIO) that reject the new {@code x-amz-checksum-*} mechanism + * introduced by AWS SDK v2 ≥ 2.30. Real AWS S3 accepts either header, so this is a no-op for + * vanilla AWS. + */ + private static final class DeleteObjectsContentMd5Interceptor implements ExecutionInterceptor { + private static final String OPERATION = "DeleteObjects"; + private static final String HEADER = "Content-MD5"; + + @Override + public SdkHttpRequest modifyHttpRequest( + Context.ModifyHttpRequest context, ExecutionAttributes attrs) { + SdkHttpRequest request = context.httpRequest(); + if (!OPERATION.equals(attrs.getAttribute(SdkExecutionAttribute.OPERATION_NAME))) { + return request; + } + if (request.firstMatchingHeader(HEADER).isPresent()) { + return request; + } + return context + .requestBody() + .map(body -> request.toBuilder().putHeader(HEADER, md5Base64(body)).build()) + .orElse(request); + } + + private static String md5Base64(RequestBody body) { + try (InputStream in = body.contentStreamProvider().newStream()) { + MessageDigest md = MessageDigest.getInstance("MD5"); + byte[] buf = new byte[8192]; + int n; + while ((n = in.read(buf)) != -1) md.update(buf, 0, n); + return Base64.getEncoder().encodeToString(md.digest()); + } catch (IOException | NoSuchAlgorithmException e) { + throw new IllegalStateException("Failed to compute Content-MD5 for DeleteObjects body", e); + } + } + } +} diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java new file mode 100644 index 000000000000..a7ef47fe0a91 --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java @@ -0,0 +1,152 @@ +/* + * Copyright (c) 2004-2026, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.storage; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.UncheckedIOException; +import java.net.URI; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import javax.annotation.CheckForNull; +import org.hisp.dhis.external.conf.ConfigurationKey; +import org.hisp.dhis.external.conf.DhisConfigurationProvider; + +/** + * In-memory {@link BlobStoreService} used by tests and any deployment with {@code + * filestore.provider=transient}. Replaces the JClouds {@code transient} provider. + * + *

State is held in a single {@link ConcurrentHashMap} keyed by the full blob key; nothing is + * persisted across JVM restarts. {@link #signedGetUri} returns {@code null}. + */ +public class TransientBlobStoreService implements BlobStoreService { + + private final BlobContainerName container; + private final Map blobs = new ConcurrentHashMap<>(); + + public TransientBlobStoreService(DhisConfigurationProvider configurationProvider) { + this.container = + new BlobContainerName( + configurationProvider.getProperty(ConfigurationKey.FILESTORE_CONTAINER)); + } + + @Override + public boolean blobExists(BlobKey key) { + return key != null && blobs.containsKey(key.value()); + } + + @Override + @CheckForNull + public InputStream openStream(BlobKey key) { + byte[] payload = blobs.get(key.value()); + return payload == null ? null : new ByteArrayInputStream(payload); + } + + @Override + public long contentLength(BlobKey key) { + byte[] payload = blobs.get(key.value()); + return payload == null ? 0L : payload.length; + } + + @Override + public void putBlob( + BlobKey key, + InputStream content, + long contentLength, + @CheckForNull String contentType, + @CheckForNull ContentDisposition contentDisposition, + @CheckForNull ContentHash contentHash) { + try { + blobs.put(key.value(), content.readAllBytes()); + } catch (IOException e) { + throw new UncheckedIOException("Unable to read blob payload for " + key, e); + } + } + + @Override + public void deleteBlob(BlobKey key) { + blobs.remove(key.value()); + } + + @Override + public void deleteDirectory(BlobKeyPrefix prefix) { + String pfx = prefix.value() + "/"; + blobs.keySet().removeIf(k -> k.startsWith(pfx)); + } + + @Override + public Iterable listFolders(BlobKeyPrefix prefix) { + String pfx = prefix.value() + "/"; + Set immediate = new HashSet<>(); + for (String k : blobs.keySet()) { + if (!k.startsWith(pfx)) continue; + String remainder = k.substring(pfx.length()); + int slash = remainder.indexOf('/'); + if (slash < 0) continue; // file at this level, not a folder + immediate.add(pfx + remainder.substring(0, slash)); + } + List result = new ArrayList<>(immediate.size()); + for (String p : immediate) result.add(BlobKeyPrefix.of(p)); + return result; + } + + @Override + public Iterable listKeys(BlobKeyPrefix prefix) { + String pfx = prefix.value(); + List result = new ArrayList<>(); + for (String k : blobs.keySet()) { + if (k.equals(pfx) || k.startsWith(pfx + "/")) { + result.add(new BlobKey(k)); + } + } + return result; + } + + @Override + @CheckForNull + public URI signedGetUri(BlobKey key, long expirationSeconds) { + return null; + } + + @Override + public BlobContainerName container() { + return container; + } + + @Override + public boolean isFilesystem() { + return false; + } +} diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/JCloudsAppStorageServiceTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/BlobStoreAppStorageServiceTest.java similarity index 98% rename from dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/JCloudsAppStorageServiceTest.java rename to dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/BlobStoreAppStorageServiceTest.java index ec785d3dcfd1..63371b13eb3e 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/JCloudsAppStorageServiceTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/BlobStoreAppStorageServiceTest.java @@ -56,7 +56,7 @@ import org.mockito.junit.jupiter.MockitoExtension; @ExtendWith(MockitoExtension.class) -class JCloudsAppStorageServiceTest { +class BlobStoreAppStorageServiceTest { private static final String FOLDER = "apps/test-app_abc123"; private static final BlobContainerName CONTAINER = new BlobContainerName("dhis2-store"); @@ -67,17 +67,17 @@ class JCloudsAppStorageServiceTest { @TempDir File tempDir; - private JCloudsAppStorageService service; + private BlobStoreAppStorageService service; private App app; @BeforeEach void setUp() { service = - new JCloudsAppStorageService( + new BlobStoreAppStorageService( blobStore, locationManager, new ObjectMapper(), fileResourceContentStore); app = new App(); - app.setAppStorageSource(AppStorageSource.JCLOUDS); + app.setAppStorageSource(AppStorageSource.BLOB_STORE); app.setFolderName(FOLDER); } diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/DefaultAppManagerTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/DefaultAppManagerTest.java index f1ed2f4b5a85..1313cc560119 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/DefaultAppManagerTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/DefaultAppManagerTest.java @@ -62,7 +62,7 @@ class DefaultAppManagerTest { @Mock private DhisConfigurationProvider dhisConfigurationProvider; @Mock private AppHubService appHubService; - @Mock private AppStorageService jCloudsAppStorageService; + @Mock private AppStorageService blobStoreAppStorageService; @Mock private DatastoreService datastoreService; @Mock private Cache appCache; @Mock private DefaultCacheBuilderProvider cacheBuilderProvider; @@ -125,7 +125,7 @@ private void requiredByAllTests() { new DefaultAppManager( dhisConfigurationProvider, appHubService, - jCloudsAppStorageService, + blobStoreAppStorageService, datastoreService, cacheBuilderProvider, i18nManager, diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/jclouds/JCloudsStoreTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/jclouds/JCloudsStoreTest.java deleted file mode 100644 index de96534b7a7a..000000000000 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/jclouds/JCloudsStoreTest.java +++ /dev/null @@ -1,76 +0,0 @@ -/* - * Copyright (c) 2004-2024, University of Oslo - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are met: - * - * 1. Redistributions of source code must retain the above copyright notice, this - * list of conditions and the following disclaimer. - * - * 2. Redistributions in binary form must reproduce the above copyright notice, - * this list of conditions and the following disclaimer in the documentation - * and/or other materials provided with the distribution. - * - * 3. Neither the name of the copyright holder nor the names of its contributors - * may be used to endorse or promote products derived from this software without - * specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED - * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE - * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR - * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES - * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; - * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON - * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS - * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - */ -package org.hisp.dhis.jclouds; - -import static org.hisp.dhis.test.utils.Assertions.assertContains; -import static org.junit.jupiter.api.Assertions.*; -import static org.mockito.Mockito.when; - -import org.hisp.dhis.external.conf.ConfigurationKey; -import org.hisp.dhis.external.conf.DhisConfigurationProvider; -import org.hisp.dhis.external.location.LocationManager; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; - -@ExtendWith(MockitoExtension.class) -class JCloudsStoreTest { - @Mock private DhisConfigurationProvider configurationProvider; - @Mock private LocationManager locationManager; - - @Test - void failIfUnsupportedProviderIsConfigured() { - when(configurationProvider.getProperty(ConfigurationKey.FILESTORE_PROVIDER)) - .thenReturn("rackspace-cloudfiles-us"); - - IllegalArgumentException exception = - assertThrows( - IllegalArgumentException.class, - () -> new JCloudsStore(configurationProvider, locationManager)); - - assertContains( - "Unsupported file store provider 'rackspace-cloudfiles-us'", exception.getMessage()); - } - - @Test - void failIfFilesystemProviderIsConfiguredButNoExternalDirectoryIsSet() { - when(configurationProvider.getProperty(ConfigurationKey.FILESTORE_PROVIDER)) - .thenReturn("filesystem"); - when(locationManager.externalDirectorySet()).thenReturn(false); - - IllegalArgumentException exception = - assertThrows( - IllegalArgumentException.class, - () -> new JCloudsStore(configurationProvider, locationManager)); - - assertContains("external directory is not set", exception.getMessage()); - } -} diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/BlobStoreConfigTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/BlobStoreConfigTest.java new file mode 100644 index 000000000000..45cd65f118d4 --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/BlobStoreConfigTest.java @@ -0,0 +1,99 @@ +/* + * Copyright (c) 2004-2026, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.storage; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import org.hisp.dhis.external.conf.ConfigurationKey; +import org.hisp.dhis.external.conf.DhisConfigurationProvider; +import org.hisp.dhis.external.location.LocationManager; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.nio.file.Path; + +/** Verifies {@link BlobStoreConfig} selects the right {@link BlobStoreService} implementation. */ +class BlobStoreConfigTest { + + @Test + void selectsS3ForS3Provider() { + BlobStoreService svc = new BlobStoreConfig().blobStoreService(config("s3"), mock(LocationManager.class)); + assertInstanceOf(S3BlobStoreService.class, svc); + ((S3BlobStoreService) svc).cleanUp(); + } + + @Test + void selectsS3ForAwsS3Provider() { + BlobStoreService svc = new BlobStoreConfig().blobStoreService(config("aws-s3"), mock(LocationManager.class)); + assertInstanceOf(S3BlobStoreService.class, svc); + ((S3BlobStoreService) svc).cleanUp(); + } + + @Test + void selectsFileSystemForFilesystemProvider(@TempDir Path tempDir) { + LocationManager lm = mock(LocationManager.class); + when(lm.externalDirectorySet()).thenReturn(true); + when(lm.getExternalDirectoryPath()).thenReturn(tempDir.toString()); + + BlobStoreService svc = new BlobStoreConfig().blobStoreService(config("filesystem"), lm); + assertInstanceOf(FileSystemBlobStoreService.class, svc); + } + + @Test + void selectsTransientForTransientProvider() { + BlobStoreService svc = new BlobStoreConfig().blobStoreService(config("transient"), mock(LocationManager.class)); + assertInstanceOf(TransientBlobStoreService.class, svc); + } + + @Test + void rejectsUnknownProvider() { + IllegalArgumentException e = + assertThrows( + IllegalArgumentException.class, + () -> new BlobStoreConfig().blobStoreService(config("s4"), mock(LocationManager.class))); + assertEquals(true, e.getMessage().contains("s4")); + } + + private static DhisConfigurationProvider config(String provider) { + DhisConfigurationProvider config = mock(DhisConfigurationProvider.class); + lenient().when(config.getProperty(ConfigurationKey.FILESTORE_PROVIDER)).thenReturn(provider); + lenient().when(config.getProperty(ConfigurationKey.FILESTORE_CONTAINER)).thenReturn("dhis2"); + lenient().when(config.getProperty(ConfigurationKey.FILESTORE_LOCATION)).thenReturn(""); + lenient().when(config.getProperty(ConfigurationKey.FILESTORE_ENDPOINT)).thenReturn(""); + lenient().when(config.getProperty(ConfigurationKey.FILESTORE_IDENTITY)).thenReturn(""); + lenient().when(config.getProperty(ConfigurationKey.FILESTORE_SECRET)).thenReturn(""); + return config; + } +} diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jclouds/BlobStoreServiceContractTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java similarity index 97% rename from dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jclouds/BlobStoreServiceContractTest.java rename to dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java index 2dd2221a18cf..76cab82117da 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jclouds/BlobStoreServiceContractTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java @@ -27,7 +27,7 @@ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -package org.hisp.dhis.jclouds; +package org.hisp.dhis.storage; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; @@ -55,12 +55,7 @@ import java.util.List; import java.util.Set; import org.awaitility.Awaitility; -import org.hisp.dhis.storage.BlobContainerName; -import org.hisp.dhis.storage.BlobKey; -import org.hisp.dhis.storage.BlobKeyPrefix; -import org.hisp.dhis.storage.BlobStoreService; import org.hisp.dhis.storage.BlobStoreService.ContentDisposition; -import org.hisp.dhis.storage.ContentHash; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; @@ -82,8 +77,8 @@ *

  • {@link #validatesContentMd5()} — backend rejects an upload whose payload disagrees with the * supplied {@link ContentHash} (true on real S3, false on local backends). *
  • {@link #supportsRecursiveDirectoryDelete()} — backend implements {@link - * BlobStoreService#deleteDirectory} as a true recursive delete (false on the current jclouds - * S3 path, which is non-recursive — see {@code JCloudsAppStorageService#deleteApp}). + * BlobStoreService#deleteDirectory} as a true recursive delete (false on the current S3 + * path, which is non-recursive — see {@code BlobStoreAppStorageService#deleteApp}). * * *

    Lifecycle: subclasses use {@code @BeforeAll}/{@code @AfterAll} to start and stop the backend @@ -91,7 +86,7 @@ * {@link #cleanUpTestData()}. */ @TestInstance(TestInstance.Lifecycle.PER_CLASS) -abstract class BlobStoreServiceContractTest { +public abstract class BlobStoreServiceContractTest { /** Implementation under test. Lifecycle is owned by the subclass. */ protected abstract BlobStoreService service(); diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jclouds/FilesystemBlobStoreServiceContractTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceContractTest.java similarity index 66% rename from dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jclouds/FilesystemBlobStoreServiceContractTest.java rename to dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceContractTest.java index 62c746d3e417..2c12e0c4f820 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jclouds/FilesystemBlobStoreServiceContractTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceContractTest.java @@ -12,7 +12,7 @@ * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. * - * 3. Neither the name of the copyright holder nor the names of its contributors + * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software without * specific prior written permission. * @@ -27,7 +27,7 @@ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -package org.hisp.dhis.jclouds; +package org.hisp.dhis.storage; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; @@ -40,53 +40,39 @@ import org.hisp.dhis.external.conf.ConfigurationKey; import org.hisp.dhis.external.conf.DhisConfigurationProvider; import org.hisp.dhis.external.location.LocationManager; -import org.hisp.dhis.storage.BlobStoreService; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Tag; /** - * Runs the {@link BlobStoreServiceContractTest} suite against the JClouds {@code filesystem} - * provider, rooted at a temporary directory created in {@code @BeforeAll}. + * Runs the {@link BlobStoreServiceContractTest} suite against {@link FileSystemBlobStoreService}, + * rooted at a temporary directory created in {@code @BeforeAll}. * *

    Tagged {@code integration}: writes real files to disk on every test method. - * - *

    The temp dir is created manually rather than via JUnit's {@code @TempDir} because non-static - * {@code @TempDir} fields are injected too late to be visible inside {@code @BeforeAll}, which left - * {@code tempDir} null at startup and corrupted Mockito's stubbing state for downstream tests in - * the same suite. */ @Tag("integration") -class FilesystemBlobStoreServiceContractTest extends BlobStoreServiceContractTest { +class FileSystemBlobStoreServiceContractTest extends BlobStoreServiceContractTest { private Path tempDir; - private JCloudsStore store; + private FileSystemBlobStoreService store; @BeforeAll void start() throws IOException { tempDir = Files.createTempDirectory("dhis-fs-contract-"); DhisConfigurationProvider config = mock(DhisConfigurationProvider.class); - lenient() - .when(config.getProperty(ConfigurationKey.FILESTORE_PROVIDER)) - .thenReturn("filesystem"); lenient().when(config.getProperty(ConfigurationKey.FILESTORE_CONTAINER)).thenReturn("contract"); - lenient().when(config.getProperty(ConfigurationKey.FILESTORE_LOCATION)).thenReturn(""); - lenient().when(config.getProperty(ConfigurationKey.FILESTORE_ENDPOINT)).thenReturn(""); - lenient().when(config.getProperty(ConfigurationKey.FILESTORE_IDENTITY)).thenReturn(""); - lenient().when(config.getProperty(ConfigurationKey.FILESTORE_SECRET)).thenReturn(""); LocationManager locationManager = mock(LocationManager.class); when(locationManager.externalDirectorySet()).thenReturn(true); when(locationManager.getExternalDirectoryPath()).thenReturn(tempDir.toString()); - store = new JCloudsStore(config, locationManager); + store = new FileSystemBlobStoreService(config, locationManager); store.init(); } @AfterAll void stop() throws IOException { - if (store != null) store.cleanUp(); if (tempDir != null && Files.exists(tempDir)) { try (var paths = Files.walk(tempDir)) { paths.sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(java.io.File::delete); @@ -103,12 +89,4 @@ protected BlobStoreService service() { protected boolean supportsRequestSigning() { return false; } - - @Override - protected boolean listKeysIncludesDirectoryMarkers() { - // TODO(DHIS2-20648) jclouds-filesystem returns synthetic entries for parent directories - // alongside real blobs. Drop this override once the replacement implementation - // filters these out so listKeys returns blob keys only. - return true; - } } diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jclouds/S3BlobStoreServiceContractTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceContractTest.java similarity index 68% rename from dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jclouds/S3BlobStoreServiceContractTest.java rename to dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceContractTest.java index e7ff3cebefaa..ceaf992bb22e 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jclouds/S3BlobStoreServiceContractTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceContractTest.java @@ -12,7 +12,7 @@ * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. * - * 3. Neither the name of the copyright holder nor the names of its contributors + * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software without * specific prior written permission. * @@ -27,15 +27,13 @@ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -package org.hisp.dhis.jclouds; +package org.hisp.dhis.storage; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import org.hisp.dhis.external.conf.ConfigurationKey; import org.hisp.dhis.external.conf.DhisConfigurationProvider; -import org.hisp.dhis.external.location.LocationManager; -import org.hisp.dhis.storage.BlobStoreService; import org.hisp.dhis.test.junit.MinIOTestExtension; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; @@ -43,30 +41,26 @@ import org.junit.jupiter.api.extension.ExtendWith; /** - * Runs the {@link BlobStoreServiceContractTest} suite against the JClouds {@code s3} provider, - * pointed at the MinIO container managed by {@link MinIOTestExtension}. Validates the S3-shaped - * behaviours (presigned URLs, server-side MD5 validation, custom endpoint + path-style addressing) - * that have no other automated coverage today. + * Runs the {@link BlobStoreServiceContractTest} suite against the AWS SDK v2 {@link + * S3BlobStoreService}, pointed at the MinIO container managed by {@link MinIOTestExtension}. + * Validates the S3-shaped behaviours (presigned URLs, server-side MD5 validation, custom endpoint + + * path-style addressing, recursive deleteDirectory) on the JClouds replacement. * - *

    Lives in {@code dhis-test-integration} because it requires Docker. The {@code - * BlobStoreServiceContractTest} base class is consumed from the same module; the {@code - * org.hisp.dhis.jclouds} package is shared with {@link JCloudsStore} so its package-private - * constructor remains reachable. + *

    Tagged {@code integration} because it requires Docker. */ @Tag("integration") @ExtendWith(MinIOTestExtension.class) class S3BlobStoreServiceContractTest extends BlobStoreServiceContractTest { - private JCloudsStore store; + private S3BlobStoreService store; @BeforeAll void start() { DhisConfigurationProvider config = mock(DhisConfigurationProvider.class); - lenient().when(config.getProperty(ConfigurationKey.FILESTORE_PROVIDER)).thenReturn("s3"); lenient().when(config.getProperty(ConfigurationKey.FILESTORE_CONTAINER)).thenReturn("contract"); - // FILESTORE_LOCATION intentionally not stubbed (mock returns null). JCloudsStore.createLocation - // returns null for a null location, so no LocationConstraint is sent on bucket creation — - // newer MinIO releases (>= RELEASE.2025-04-22) reject mismatched regions. + // FILESTORE_LOCATION intentionally not stubbed (mock returns null). S3BlobStoreService falls + // back to us-east-1, which sends no LocationConstraint on bucket creation — newer MinIO + // releases (>= RELEASE.2025-04-22) reject mismatched regions. lenient() .when(config.getProperty(ConfigurationKey.FILESTORE_ENDPOINT)) .thenReturn(MinIOTestExtension.s3Url()); @@ -77,7 +71,7 @@ void start() { .when(config.getProperty(ConfigurationKey.FILESTORE_SECRET)) .thenReturn(MinIOTestExtension.MINIO_PASSWORD); - store = new JCloudsStore(config, mock(LocationManager.class)); + store = new S3BlobStoreService(config); store.init(); } @@ -100,12 +94,4 @@ protected boolean supportsRequestSigning() { protected boolean validatesContentMd5() { return true; } - - @Override - protected boolean supportsRecursiveDirectoryDelete() { - // TODO(DHIS2-20648) jclouds-S3 deleteDirectory is non-recursive — JCloudsAppStorageService - // works around it by enumerating + deleting individually. Drop this override once the - // replacement implementation does recursive deleteDirectory on S3. - return false; - } } diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jclouds/TransientBlobStoreServiceContractTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceContractTest.java similarity index 59% rename from dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jclouds/TransientBlobStoreServiceContractTest.java rename to dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceContractTest.java index 2f1ccd5a792c..620c7b8c545e 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jclouds/TransientBlobStoreServiceContractTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceContractTest.java @@ -12,7 +12,7 @@ * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. * - * 3. Neither the name of the copyright holder nor the names of its contributors + * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software without * specific prior written permission. * @@ -27,51 +27,30 @@ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -package org.hisp.dhis.jclouds; +package org.hisp.dhis.storage; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import org.hisp.dhis.external.conf.ConfigurationKey; import org.hisp.dhis.external.conf.DhisConfigurationProvider; -import org.hisp.dhis.external.location.LocationManager; -import org.hisp.dhis.storage.BlobStoreService; -import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Tag; /** - * Runs the {@link BlobStoreServiceContractTest} suite against the JClouds {@code transient} - * (in-memory) provider — the backend used by H2 and Postgres integration tests today. - * - *

    Tagged {@code integration}: although it uses an in-memory backend, it exercises full jclouds - * machinery and runs ~25 real I/O round-trips per class — too heavy for the unit-test run. Belongs - * alongside the other contract subclasses that validate cross-backend conformance. + * Runs the {@link BlobStoreServiceContractTest} suite against the in-memory {@link + * TransientBlobStoreService} — the backend used by H2 and Postgres integration tests today. */ @Tag("integration") class TransientBlobStoreServiceContractTest extends BlobStoreServiceContractTest { - private JCloudsStore store; + private TransientBlobStoreService store; @BeforeAll void start() { DhisConfigurationProvider config = mock(DhisConfigurationProvider.class); - lenient().when(config.getProperty(ConfigurationKey.FILESTORE_PROVIDER)).thenReturn("transient"); lenient().when(config.getProperty(ConfigurationKey.FILESTORE_CONTAINER)).thenReturn("contract"); - lenient().when(config.getProperty(ConfigurationKey.FILESTORE_LOCATION)).thenReturn(""); - lenient().when(config.getProperty(ConfigurationKey.FILESTORE_ENDPOINT)).thenReturn(""); - lenient().when(config.getProperty(ConfigurationKey.FILESTORE_IDENTITY)).thenReturn(""); - lenient().when(config.getProperty(ConfigurationKey.FILESTORE_SECRET)).thenReturn(""); - - LocationManager locationManager = mock(LocationManager.class); - - store = new JCloudsStore(config, locationManager); - store.init(); - } - - @AfterAll - void stop() { - if (store != null) store.cleanUp(); + store = new TransientBlobStoreService(config); } @Override @@ -83,12 +62,4 @@ protected BlobStoreService service() { protected boolean supportsRequestSigning() { return false; } - - @Override - protected boolean supportsRecursiveDirectoryDelete() { - // TODO(DHIS2-20648) jclouds-transient deleteDirectory is non-recursive in practice; same - // limitation as jclouds-S3. Drop this override once the replacement BlobStoreService - // implementation does recursive deleteDirectory on every backend. - return false; - } } diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/AppControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/AppControllerTest.java index bfc7b9804830..b0d3bcc302be 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/AppControllerTest.java +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/AppControllerTest.java @@ -55,7 +55,7 @@ import org.hisp.dhis.appmanager.AppManager; import org.hisp.dhis.appmanager.AppShortcut; import org.hisp.dhis.appmanager.AppStatus; -import org.hisp.dhis.appmanager.JCloudsAppStorageService; +import org.hisp.dhis.appmanager.BlobStoreAppStorageService; import org.hisp.dhis.external.conf.DhisConfigurationProvider; import org.hisp.dhis.http.HttpStatus; import org.hisp.dhis.jsontree.JsonArray; @@ -93,7 +93,7 @@ public DhisConfigurationProvider dhisConfigurationProvider() { } @Autowired private AppManager appManager; - @Autowired private JCloudsAppStorageService jCloudsAppStorageService; + @Autowired private BlobStoreAppStorageService blobStoreAppStorageService; static { try { @@ -145,14 +145,14 @@ void testInstallReturnsAppInfo() throws IOException { void testInstallMultipleSameKey() throws IOException { // Clean up first Map> installedApps = - jCloudsAppStorageService.discoverInstalledApps(); + blobStoreAppStorageService.discoverInstalledApps(); installedApps.values().forEach(p -> appManager.deleteApp(p.getLeft(), true)); appManager.installApp(new ClassPathResource("app/app_ver1.zip").getFile()); appManager.installApp(new ClassPathResource("app/app_ver3.zip").getFile()); appManager.installApp(new ClassPathResource("app/app_ver2.zip").getFile()); - installedApps = jCloudsAppStorageService.discoverInstalledApps(); + installedApps = blobStoreAppStorageService.discoverInstalledApps(); assertEquals(1, installedApps.size()); assertEquals("2.0.0", installedApps.values().stream().findFirst().get().getLeft().getVersion()); diff --git a/dhis-2/pom.xml b/dhis-2/pom.xml index 3511013cbdbb..2bc463b33ba9 100644 --- a/dhis-2/pom.xml +++ b/dhis-2/pom.xml @@ -153,8 +153,8 @@ 1.3.2 2.12.2 - - 2.7.0 + + 2.44.4 6.20.0 @@ -1080,36 +1080,13 @@ ${gson.version} - + - org.apache.jclouds - jclouds-core - ${jclouds.version} - - - org.apache.jclouds - jclouds-blobstore - ${jclouds.version} - - - org.apache.jclouds.api - filesystem - ${jclouds.version} - - - org.apache.jclouds.api - s3 - ${jclouds.version} - - - org.apache.jclouds.provider - aws-s3 - ${jclouds.version} - - - org.apache.jclouds.driver - jclouds-slf4j - ${jclouds.version} + software.amazon.awssdk + bom + ${aws-sdk.version} + pom + import @@ -2106,7 +2083,6 @@ jasperreports.version=${jasperreports.version} org.apache.logging.log4j:log4j-slf4j-impl org.apache.logging.log4j:log4j-layout-template-json jakarta.servlet:jakarta.servlet-api - org.apache.jclouds.provider:aws-s3 io.micrometer:micrometer-jakarta9 org.hibernate:hibernate-micrometer io.micrometer:micrometer-spring-legacy @@ -2122,8 +2098,6 @@ jasperreports.version=${jasperreports.version} io.netty:netty-all org.cache2k:cache2k-core - org.apache.jclouds.api:filesystem - org.apache.jclouds.api:s3 org.hibernate:hibernate-ehcache jakarta.xml.bind:jakarta.xml.bind-api @@ -2162,6 +2136,12 @@ jasperreports.version=${jasperreports.version} jakarta.jms:jakarta.jms-api org.apache.activemq:artemis-server org.mock-server:mockserver-core:jar + + software.amazon.awssdk:sdk-core + software.amazon.awssdk:aws-core + software.amazon.awssdk:auth + software.amazon.awssdk:regions From 571536354953930f6e6a95cf610244b8ad0f8297 Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Thu, 14 May 2026 09:24:51 +0100 Subject: [PATCH 02/16] feat: Align impl contracts for consistency. Add comments [DHIS2-20648] --- .../BlobStoreAppStorageService.java | 6 +- .../hisp/dhis/storage/BlobStoreConfig.java | 5 +- .../hisp/dhis/storage/FileStoreProvider.java | 6 +- .../storage/FileSystemBlobStoreService.java | 11 ++- .../hisp/dhis/storage/S3BlobStoreService.java | 70 ++++++++++++------- .../storage/TransientBlobStoreService.java | 22 +++++- .../dhis/storage/BlobStoreConfigTest.java | 17 +++-- .../storage/BlobStoreServiceContractTest.java | 4 +- ...ileSystemBlobStoreServiceContractTest.java | 2 +- .../S3BlobStoreServiceContractTest.java | 2 +- ...TransientBlobStoreServiceContractTest.java | 2 +- dhis-2/pom.xml | 7 +- 12 files changed, 98 insertions(+), 56 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java index 249baee90091..52121cc486b4 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java @@ -349,8 +349,7 @@ public ResourceResult getAppResource(@CheckForNull App app, @Nonnull String reso throws IOException { if (app == null || !app.getAppStorageSource().equals(AppStorageSource.BLOB_STORE)) { log.warn( - "Can't look up resource {}. The specified app was not found in blob storage.", - resource); + "Can't look up resource {}. The specified app was not found in blob storage.", resource); return new ResourceNotFound(resource); } if (resource.isBlank()) { @@ -429,8 +428,7 @@ private static void logDiscoveredApps(Map> app } else { apps.values() .forEach( - pair -> - log.info("Discovered app '{}' from blob storage ", pair.getLeft().getName())); + pair -> log.info("Discovered app '{}' from blob storage ", pair.getLeft().getName())); } } } diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreConfig.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreConfig.java index 15ad6b5c1790..c5816337f499 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreConfig.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreConfig.java @@ -12,7 +12,7 @@ * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. * - * 3. Neither the name of the copyright holder nor the names of its contributors + * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software without * specific prior written permission. * @@ -40,7 +40,8 @@ * ConfigurationKey#FILESTORE_PROVIDER}: * *

    diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileStoreProvider.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileStoreProvider.java index bcd69c21d687..b176d1c03e44 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileStoreProvider.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileStoreProvider.java @@ -12,7 +12,7 @@ * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. * - * 3. Neither the name of the copyright holder nor the names of its contributors + * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software without * specific prior written permission. * @@ -64,6 +64,8 @@ public static FileStoreProvider of(String key) { "Unsupported file store provider '" + key + "'. Supported values are: " - + Arrays.stream(values()).map(FileStoreProvider::key).collect(Collectors.joining(", ")))); + + Arrays.stream(values()) + .map(FileStoreProvider::key) + .collect(Collectors.joining(", ")))); } } diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java index 589523d55419..feff980244df 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java @@ -12,7 +12,7 @@ * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. * - * 3. Neither the name of the copyright holder nor the names of its contributors + * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software without * specific prior written permission. * @@ -72,8 +72,7 @@ public FileSystemBlobStoreService( throw new IllegalStateException( "File system blob store cannot start: external directory is not set."); } - String containerName = - configurationProvider.getProperty(ConfigurationKey.FILESTORE_CONTAINER); + String containerName = configurationProvider.getProperty(ConfigurationKey.FILESTORE_CONTAINER); this.container = new BlobContainerName(containerName); this.baseDir = Paths.get(locationManager.getExternalDirectoryPath(), containerName); } @@ -83,8 +82,7 @@ public void init() { try { Files.createDirectories(baseDir); } catch (IOException e) { - throw new UncheckedIOException( - "Unable to create file store directory at " + baseDir, e); + throw new UncheckedIOException("Unable to create file store directory at " + baseDir, e); } log.info("Filesystem blob store rooted at: '{}'", baseDir); } @@ -193,7 +191,8 @@ public Iterable listKeys(BlobKeyPrefix prefix) { new SimpleFileVisitor<>() { @Nonnull @Override - public FileVisitResult visitFile(@Nonnull Path file, @Nonnull BasicFileAttributes attrs) { + public FileVisitResult visitFile( + @Nonnull Path file, @Nonnull BasicFileAttributes attrs) { Path relative = baseDir.relativize(file); result.add(new BlobKey(toBlobPath(relative))); return FileVisitResult.CONTINUE; diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java index ac5005b24430..23c3ef22dd17 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java @@ -12,7 +12,7 @@ * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. * - * 3. Neither the name of the copyright holder nor the names of its contributors + * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software without * specific prior written permission. * @@ -72,6 +72,7 @@ import software.amazon.awssdk.services.s3.model.NoSuchKeyException; import software.amazon.awssdk.services.s3.model.ObjectIdentifier; import software.amazon.awssdk.services.s3.model.PutObjectRequest; +import software.amazon.awssdk.services.s3.model.S3Error; import software.amazon.awssdk.services.s3.model.S3Object; import software.amazon.awssdk.services.s3.presigner.S3Presigner; import software.amazon.awssdk.services.s3.presigner.S3Presigner.Builder; @@ -96,8 +97,7 @@ public class S3BlobStoreService implements BlobStoreService { private final S3Presigner presigner; public S3BlobStoreService(DhisConfigurationProvider configurationProvider) { - String containerName = - configurationProvider.getProperty(ConfigurationKey.FILESTORE_CONTAINER); + String containerName = configurationProvider.getProperty(ConfigurationKey.FILESTORE_CONTAINER); String location = configurationProvider.getProperty(ConfigurationKey.FILESTORE_LOCATION); String endpoint = configurationProvider.getProperty(ConfigurationKey.FILESTORE_ENDPOINT); String identity = configurationProvider.getProperty(ConfigurationKey.FILESTORE_IDENTITY); @@ -256,20 +256,33 @@ public void deleteDirectory(BlobKeyPrefix prefix) { bulkDelete(bucket, ids); } - continuationToken = Boolean.TRUE.equals(resp.isTruncated()) ? resp.nextContinuationToken() : null; + continuationToken = + Boolean.TRUE.equals(resp.isTruncated()) ? resp.nextContinuationToken() : null; } while (continuationToken != null); } @Override public Iterable listFolders(BlobKeyPrefix prefix) { + // S3 caps a single ListObjectsV2 response at 1000 entries; paginate via continuationToken + // so prefixes with more than 1000 immediate child folders are returned exhaustively. + String bucket = container.value(); String listPrefix = prefix.value() + "/"; - ListObjectsV2Response resp = - s3.listObjectsV2( - b -> b.bucket(container.value()).prefix(listPrefix).delimiter("/")); - return resp.commonPrefixes().stream() - .map(CommonPrefix::prefix) - .map(BlobKeyPrefix::of) - .toList(); + List result = new ArrayList<>(); + String continuationToken = null; + do { + ListObjectsV2Request.Builder list = + ListObjectsV2Request.builder().bucket(bucket).prefix(listPrefix).delimiter("/"); + if (continuationToken != null) { + list.continuationToken(continuationToken); + } + ListObjectsV2Response resp = s3.listObjectsV2(list.build()); + for (CommonPrefix cp : resp.commonPrefixes()) { + result.add(BlobKeyPrefix.of(cp.prefix())); + } + continuationToken = + Boolean.TRUE.equals(resp.isTruncated()) ? resp.nextContinuationToken() : null; + } while (continuationToken != null); + return result; } @Override @@ -289,7 +302,8 @@ public Iterable listKeys(BlobKeyPrefix prefix) { if (obj.key().endsWith("/")) continue; result.add(new BlobKey(obj.key())); } - continuationToken = Boolean.TRUE.equals(resp.isTruncated()) ? resp.nextContinuationToken() : null; + continuationToken = + Boolean.TRUE.equals(resp.isTruncated()) ? resp.nextContinuationToken() : null; } while (continuationToken != null); return result; } @@ -320,6 +334,14 @@ public boolean isFilesystem() { return false; } + /** + * Issues a single S3 {@code DeleteObjects} call for up to {@value #DELETE_BATCH_SIZE} keys. + * + *

    Per-key failures inside the response are logged as a single WARN summary (count + first + * error's details) and otherwise swallowed: deletion is best-effort and a partial failure should + * not abort an in-progress recursive {@code deleteDirectory} mid-batch. Callers that need + * stricter guarantees can re-run the operation — successfully deleted keys won't reappear. + */ private void bulkDelete(String bucket, List ids) { DeleteObjectsResponse resp = s3.deleteObjects( @@ -327,19 +349,17 @@ private void bulkDelete(String bucket, List ids) { .bucket(bucket) .delete(d -> d.objects(ids).build()) .build()); - log.debug( - "bulkDelete: requested {} keys, deleted {}, errors {}", - ids.size(), - resp.deleted().size(), - resp.errors().size()); - resp.errors() - .forEach( - err -> - log.warn( - "bulkDelete error: key='{}' code='{}' message='{}'", - err.key(), - err.code(), - err.message())); + if (!resp.errors().isEmpty()) { + S3Error first = resp.errors().get(0); + log.warn( + "bulkDelete: {} of {} keys failed (first error: key='{}' code='{}' message='{}')", + resp.errors().size(), + ids.size(), + first.key(), + first.code(), + first.message()); + } + log.debug("bulkDelete: requested {} keys, deleted {}", ids.size(), resp.deleted().size()); } private static String hexToBase64(String hex) { diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java index a7ef47fe0a91..acc098c3b5c5 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java @@ -12,7 +12,7 @@ * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. * - * 3. Neither the name of the copyright holder nor the names of its contributors + * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software without * specific prior written permission. * @@ -88,8 +88,19 @@ public void putBlob( @CheckForNull String contentType, @CheckForNull ContentDisposition contentDisposition, @CheckForNull ContentHash contentHash) { + // Read exactly contentLength bytes per BlobStoreService.putBlob's contract (the SDK-backed + // impls rely on the same guarantee for Content-Length). Math.toIntExact rejects anything + // larger than Integer.MAX_VALUE up-front — Transient is in-memory, so capping at int is the + // right limit anyway. + int length = Math.toIntExact(contentLength); try { - blobs.put(key.value(), content.readAllBytes()); + byte[] payload = content.readNBytes(length); + if (payload.length != length) { + throw new IOException( + "Expected %d bytes for %s but stream produced %d" + .formatted(length, key, payload.length)); + } + blobs.put(key.value(), payload); } catch (IOException e) { throw new UncheckedIOException("Unable to read blob payload for " + key, e); } @@ -124,10 +135,15 @@ public Iterable listFolders(BlobKeyPrefix prefix) { @Override public Iterable listKeys(BlobKeyPrefix prefix) { + // Raw startsWith matches S3's native ListObjectsV2 behaviour and the BlobStoreService + // javadoc ("key starts with prefix"). FileSystemBlobStoreService diverges by directory + // boundary because filesystems can't list by raw prefix without iterating the parent — + // in practice DHIS2's UUID-shaped keys never produce ambiguous prefixes (e.g. "trash" vs + // "trashbag"), so the impls agree on every input that callers actually produce. String pfx = prefix.value(); List result = new ArrayList<>(); for (String k : blobs.keySet()) { - if (k.equals(pfx) || k.startsWith(pfx + "/")) { + if (k.startsWith(pfx)) { result.add(new BlobKey(k)); } } diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/BlobStoreConfigTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/BlobStoreConfigTest.java index 45cd65f118d4..332d5f59a854 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/BlobStoreConfigTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/BlobStoreConfigTest.java @@ -12,7 +12,7 @@ * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. * - * 3. Neither the name of the copyright holder nor the names of its contributors + * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software without * specific prior written permission. * @@ -36,27 +36,28 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import java.nio.file.Path; import org.hisp.dhis.external.conf.ConfigurationKey; import org.hisp.dhis.external.conf.DhisConfigurationProvider; import org.hisp.dhis.external.location.LocationManager; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; -import java.nio.file.Path; - /** Verifies {@link BlobStoreConfig} selects the right {@link BlobStoreService} implementation. */ class BlobStoreConfigTest { @Test void selectsS3ForS3Provider() { - BlobStoreService svc = new BlobStoreConfig().blobStoreService(config("s3"), mock(LocationManager.class)); + BlobStoreService svc = + new BlobStoreConfig().blobStoreService(config("s3"), mock(LocationManager.class)); assertInstanceOf(S3BlobStoreService.class, svc); ((S3BlobStoreService) svc).cleanUp(); } @Test void selectsS3ForAwsS3Provider() { - BlobStoreService svc = new BlobStoreConfig().blobStoreService(config("aws-s3"), mock(LocationManager.class)); + BlobStoreService svc = + new BlobStoreConfig().blobStoreService(config("aws-s3"), mock(LocationManager.class)); assertInstanceOf(S3BlobStoreService.class, svc); ((S3BlobStoreService) svc).cleanUp(); } @@ -73,7 +74,8 @@ void selectsFileSystemForFilesystemProvider(@TempDir Path tempDir) { @Test void selectsTransientForTransientProvider() { - BlobStoreService svc = new BlobStoreConfig().blobStoreService(config("transient"), mock(LocationManager.class)); + BlobStoreService svc = + new BlobStoreConfig().blobStoreService(config("transient"), mock(LocationManager.class)); assertInstanceOf(TransientBlobStoreService.class, svc); } @@ -82,7 +84,8 @@ void rejectsUnknownProvider() { IllegalArgumentException e = assertThrows( IllegalArgumentException.class, - () -> new BlobStoreConfig().blobStoreService(config("s4"), mock(LocationManager.class))); + () -> + new BlobStoreConfig().blobStoreService(config("s4"), mock(LocationManager.class))); assertEquals(true, e.getMessage().contains("s4")); } diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java index 76cab82117da..792375c0e69e 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java @@ -77,8 +77,8 @@ *

  • {@link #validatesContentMd5()} — backend rejects an upload whose payload disagrees with the * supplied {@link ContentHash} (true on real S3, false on local backends). *
  • {@link #supportsRecursiveDirectoryDelete()} — backend implements {@link - * BlobStoreService#deleteDirectory} as a true recursive delete (false on the current S3 - * path, which is non-recursive — see {@code BlobStoreAppStorageService#deleteApp}). + * BlobStoreService#deleteDirectory} as a true recursive delete (false on the current S3 path, + * which is non-recursive — see {@code BlobStoreAppStorageService#deleteApp}). * * *

    Lifecycle: subclasses use {@code @BeforeAll}/{@code @AfterAll} to start and stop the backend diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceContractTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceContractTest.java index 2c12e0c4f820..6ab58809cee7 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceContractTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceContractTest.java @@ -12,7 +12,7 @@ * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. * - * 3. Neither the name of the copyright holder nor the names of its contributors + * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software without * specific prior written permission. * diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceContractTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceContractTest.java index ceaf992bb22e..84f8193563a2 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceContractTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceContractTest.java @@ -12,7 +12,7 @@ * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. * - * 3. Neither the name of the copyright holder nor the names of its contributors + * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software without * specific prior written permission. * diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceContractTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceContractTest.java index 620c7b8c545e..4fb03bd628e9 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceContractTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceContractTest.java @@ -12,7 +12,7 @@ * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. * - * 3. Neither the name of the copyright holder nor the names of its contributors + * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software without * specific prior written permission. * diff --git a/dhis-2/pom.xml b/dhis-2/pom.xml index 2bc463b33ba9..43be9b75f840 100644 --- a/dhis-2/pom.xml +++ b/dhis-2/pom.xml @@ -2136,12 +2136,15 @@ jasperreports.version=${jasperreports.version} jakarta.jms:jakarta.jms-api org.apache.activemq:artemis-server org.mock-server:mockserver-core:jar - + software.amazon.awssdk:sdk-core software.amazon.awssdk:aws-core software.amazon.awssdk:auth software.amazon.awssdk:regions + software.amazon.awssdk:http-client-spi + software.amazon.awssdk:utils From 28c97bf18b68c5e0fce0057e2034bcd4fabdeb44 Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Thu, 14 May 2026 09:39:06 +0100 Subject: [PATCH 03/16] feat: Remove remaining JClouds refs [DHIS2-20648] --- .../org/hisp/dhis/storage/BlobKeyPrefix.java | 3 +- ...=> BlobStoreFileResourceContentStore.java} | 2 +- .../storage/FileSystemBlobStoreService.java | 4 +- .../storage/TransientBlobStoreService.java | 4 +- .../storage/BlobStoreServiceContractTest.java | 71 +++---------------- .../S3BlobStoreServiceContractTest.java | 8 +-- .../StaticContentControllerTest.java | 4 +- 7 files changed, 21 insertions(+), 75 deletions(-) rename dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/{JCloudsFileResourceContentStore.java => BlobStoreFileResourceContentStore.java} (98%) diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/storage/BlobKeyPrefix.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/storage/BlobKeyPrefix.java index 2d5f64aed40e..11ba1895b710 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/storage/BlobKeyPrefix.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/storage/BlobKeyPrefix.java @@ -61,7 +61,8 @@ public record BlobKeyPrefix(@Nonnull String value) { /** * Creates a {@link BlobKeyPrefix} from {@code value}, stripping any leading or trailing {@code /} * before construction. Prefer this over the canonical constructor when the input may carry - * store-native separators (e.g. JClouds appends a trailing {@code /} to folder names). + * store-native separators (e.g. S3's {@code CommonPrefix} entries arrive with a trailing {@code + * /}). * *

    Examples: * diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/JCloudsFileResourceContentStore.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/BlobStoreFileResourceContentStore.java similarity index 98% rename from dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/JCloudsFileResourceContentStore.java rename to dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/BlobStoreFileResourceContentStore.java index c58a9ae4060e..f0572dbe9807 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/JCloudsFileResourceContentStore.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/BlobStoreFileResourceContentStore.java @@ -57,7 +57,7 @@ @Slf4j @RequiredArgsConstructor @Service("org.hisp.dhis.fileresource.FileResourceContentStore") -public class JCloudsFileResourceContentStore implements FileResourceContentStore { +public class BlobStoreFileResourceContentStore implements FileResourceContentStore { private static final long FIVE_MINUTES_IN_SECONDS = 300L; private final BlobStoreService blobStore; diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java index feff980244df..8099e3672e2c 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java @@ -53,8 +53,8 @@ import org.hisp.dhis.external.location.LocationManager; /** - * NIO-based implementation of {@link BlobStoreService} replacing the JClouds {@code filesystem} - * path. Blobs are stored as plain files under {@code //}. + * NIO-based implementation of {@link BlobStoreService} for the {@code filesystem} file store + * provider. Blobs are stored as plain files under {@code //}. * *

    The backend is unsigned and unmetadata-aware: {@code contentType}, {@code contentDisposition}, * and {@code contentHash} are accepted but not persisted (no native filesystem support, and the diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java index acc098c3b5c5..43818c722d3a 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java @@ -45,8 +45,8 @@ import org.hisp.dhis.external.conf.DhisConfigurationProvider; /** - * In-memory {@link BlobStoreService} used by tests and any deployment with {@code - * filestore.provider=transient}. Replaces the JClouds {@code transient} provider. + * In-memory {@link BlobStoreService} for the {@code transient} file store provider — used by tests + * and any deployment with {@code filestore.provider=transient}. * *

    State is held in a single {@link ConcurrentHashMap} keyed by the full blob key; nothing is * persisted across JVM restarts. {@link #signedGetUri} returns {@code null}. diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java index 792375c0e69e..725a35790795 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java @@ -63,22 +63,18 @@ /** * Behavioural contract every {@link BlobStoreService} implementation must satisfy. * - *

    Subclasses provide a wired {@link BlobStoreService} (e.g. JClouds + transient, JClouds + - * filesystem, JClouds + S3) and the contract tests run unchanged. Intent: lock the behavioural - * surface in place before replacing JClouds, so that the new implementation can be validated - * against the same suite. + *

    Subclasses wire a concrete {@link BlobStoreService} (e.g. {@code S3BlobStoreService}, {@code + * FileSystemBlobStoreService}, {@code TransientBlobStoreService}) and the contract tests run + * unchanged. * *

    Tests that don't apply to a given backend are skipped via {@link * org.junit.jupiter.api.Assumptions} on capability hooks subclasses override: * *

    * *

    Lifecycle: subclasses use {@code @BeforeAll}/{@code @AfterAll} to start and stop the backend @@ -97,33 +93,13 @@ protected boolean validatesContentMd5() { return false; } - protected boolean supportsRecursiveDirectoryDelete() { - return true; - } - - /** - * {@code true} if {@link BlobStoreService#listKeys} may return synthetic "directory marker" keys - * (values ending with {@code /}) alongside real blobs. The current jclouds-filesystem provider - * does this; S3 does not. Backends that expose markers see assertions augmented to tolerate them. - */ - protected boolean listKeysIncludesDirectoryMarkers() { - return false; - } - private final BlobKeyPrefix testPrefix = BlobKeyPrefix.of("blobStoreContract"); @AfterEach void cleanUpTestData() { List keys = new ArrayList<>(); service().listKeys(testPrefix).forEach(keys::add); - // Skip directory-marker entries (values ending with "/"). jclouds-filesystem emits these - // alongside real blobs; calling deleteBlob on them maps to a directory delete which is - // slow and may fail when the directory still holds children — and the markers are - // harmless between tests because contract assertions filter them via - // withoutDirectoryMarkers(). - keys.stream() - .filter(k -> !k.value().endsWith("/")) - .forEach(k -> assertDoesNotThrow(() -> service().deleteBlob(k))); + keys.forEach(k -> assertDoesNotThrow(() -> service().deleteBlob(k))); } // -------------------------------------------------------------------- existence + read @@ -237,10 +213,6 @@ void deleteBlob_unknown_isNoOp() { @Test void deleteDirectory_removesEverythingUnderPrefix_butLeavesSiblings() { - assumeTrue( - supportsRecursiveDirectoryDelete(), - "backend does not implement recursive deleteDirectory (callers are expected to enumerate" - + " and delete individually)"); putString(key("trash/a"), "a"); putString(key("trash/sub/b"), "b"); putString(key("trash/sub/c"), "c"); @@ -292,9 +264,7 @@ void listKeys_returnsAllKeysRecursively() { Set keys = new HashSet<>(); service().listKeys(BlobKeyPrefix.of(key("a").value())).forEach(k -> keys.add(k.value())); - Set blobKeys = withoutDirectoryMarkers(keys); - assertEquals( - Set.of(key("a/1").value(), key("a/b/2").value(), key("a/b/c/3").value()), blobKeys); + assertEquals(Set.of(key("a/1").value(), key("a/b/2").value(), key("a/b/c/3").value()), keys); } @Test @@ -305,8 +275,7 @@ void listKeys_filtersByPrefix() { Set keys = new HashSet<>(); service().listKeys(BlobKeyPrefix.of(key("p1").value())).forEach(k -> keys.add(k.value())); - Set blobKeys = withoutDirectoryMarkers(keys); - assertEquals(Set.of(key("p1/x").value()), blobKeys); + assertEquals(Set.of(key("p1/x").value()), keys); } // -------------------------------------------------------------------- presigning @@ -372,30 +341,6 @@ private BlobKey key(String tail) { return BlobKey.of(testPrefix.value(), tail); } - /** - * Strips synthetic directory-marker keys (values ending in {@code /}) when the backend is known - * to emit them, asserting along the way that markers don't appear on backends that shouldn't emit - * them. This keeps the strict contract for new implementations while tolerating the - * jclouds-filesystem quirk. - */ - private Set withoutDirectoryMarkers(Set keys) { - Set markers = new HashSet<>(); - Set blobs = new HashSet<>(); - for (String k : keys) { - if (k.endsWith("/")) { - markers.add(k); - } else { - blobs.add(k); - } - } - if (!listKeysIncludesDirectoryMarkers()) { - assertTrue( - markers.isEmpty(), - "backend reports it does not emit directory markers, but listKeys returned: " + markers); - } - return blobs; - } - private void putString(BlobKey key, String value) { putBytes(key, value.getBytes(UTF_8)); } diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceContractTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceContractTest.java index 84f8193563a2..3c8dd4fef2ec 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceContractTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceContractTest.java @@ -41,10 +41,10 @@ import org.junit.jupiter.api.extension.ExtendWith; /** - * Runs the {@link BlobStoreServiceContractTest} suite against the AWS SDK v2 {@link - * S3BlobStoreService}, pointed at the MinIO container managed by {@link MinIOTestExtension}. - * Validates the S3-shaped behaviours (presigned URLs, server-side MD5 validation, custom endpoint + - * path-style addressing, recursive deleteDirectory) on the JClouds replacement. + * Runs the {@link BlobStoreServiceContractTest} suite against {@link S3BlobStoreService}, pointed + * at the MinIO container managed by {@link MinIOTestExtension}. Validates the S3-shaped behaviours + * (presigned URLs, server-side MD5 validation, custom endpoint + path-style addressing, recursive + * deleteDirectory). * *

    Tagged {@code integration} because it requires Docker. */ diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/StaticContentControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/StaticContentControllerTest.java index 707f76a7a385..49dd7f7f639c 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/StaticContentControllerTest.java +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/StaticContentControllerTest.java @@ -47,7 +47,7 @@ import static org.springframework.util.MimeTypeUtils.IMAGE_PNG; import com.google.gson.JsonObject; -import org.hisp.dhis.fileresource.JCloudsFileResourceContentStore; +import org.hisp.dhis.fileresource.BlobStoreFileResourceContentStore; import org.hisp.dhis.setting.SystemSettingsService; import org.hisp.dhis.test.webapi.WebSpringTestBase; import org.junit.jupiter.api.BeforeEach; @@ -72,7 +72,7 @@ class StaticContentControllerTest extends WebSpringTestBase { @Autowired private SystemSettingsService settingsService; - @Autowired private JCloudsFileResourceContentStore fileResourceContentStore; + @Autowired private BlobStoreFileResourceContentStore fileResourceContentStore; @BeforeEach void setUp() { From 4d6bcb79fb4c8877ae6e72dee400e43640e31ad6 Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Thu, 14 May 2026 12:57:58 +0100 Subject: [PATCH 04/16] feat: Harden security and add more tests [DHIS2-20648] --- .../java/org/hisp/dhis/storage/BlobKey.java | 10 + .../org/hisp/dhis/storage/BlobKeyTest.java | 22 +++ .../storage/FileSystemBlobStoreService.java | 1 + .../hisp/dhis/storage/S3BlobStoreService.java | 87 ++++++--- .../FileSystemBlobStoreServiceTest.java | 92 +++++++++ .../dhis/storage/S3BlobStoreServiceTest.java | 181 ++++++++++++++++++ .../TransientBlobStoreServiceTest.java | 81 ++++++++ 7 files changed, 443 insertions(+), 31 deletions(-) create mode 100644 dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceTest.java create mode 100644 dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceTest.java create mode 100644 dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/storage/BlobKey.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/storage/BlobKey.java index 78cc72c71f7a..0e99283b83bb 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/storage/BlobKey.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/storage/BlobKey.java @@ -59,6 +59,16 @@ public record BlobKey(String value) { throw new IllegalArgumentException( "BlobKey value must not exceed " + MAX_KEY_LENGTH + " characters: " + value); } + // Reject `..` as a path segment so filesystem-backed stores can't be tricked into writing + // outside the container directory. Substrings like "a..b" are fine — only segments split by + // `/` count. No DHIS2 caller produces `..` segments today (zip uploads sanitise via + // ZipFileUtils, file-resource storage keys are UUID-shaped) so this is defense-in-depth. + for (String segment : value.split("/", -1)) { + if ("..".equals(segment)) { + throw new IllegalArgumentException( + "BlobKey value must not contain '..' as a path segment: " + value); + } + } } /** diff --git a/dhis-2/dhis-api/src/test/java/org/hisp/dhis/storage/BlobKeyTest.java b/dhis-2/dhis-api/src/test/java/org/hisp/dhis/storage/BlobKeyTest.java index 1524c838ca3d..560c3ad01964 100644 --- a/dhis-2/dhis-api/src/test/java/org/hisp/dhis/storage/BlobKeyTest.java +++ b/dhis-2/dhis-api/src/test/java/org/hisp/dhis/storage/BlobKeyTest.java @@ -66,4 +66,26 @@ void ofSingleSegment() { void ofJoinsSegmentsWithSlash() { assertEquals("apps/my-app/index.html", BlobKey.of("apps", "my-app", "index.html").value()); } + + @Test + void leadingDotDotSegmentIsRejected() { + assertThrows(IllegalArgumentException.class, () -> new BlobKey("../escape.txt")); + } + + @Test + void interiorDotDotSegmentIsRejected() { + assertThrows(IllegalArgumentException.class, () -> new BlobKey("a/../b/c")); + } + + @Test + void trailingDotDotSegmentIsRejected() { + assertThrows(IllegalArgumentException.class, () -> new BlobKey("a/b/..")); + } + + @Test + void dotDotAsSubstringWithinSegmentIsAccepted() { + // "a..b" is a perfectly legitimate filename — only `..` as a *segment* is rejected. + BlobKey key = new BlobKey("apps/my..app/index.html"); + assertEquals("apps/my..app/index.html", key.value()); + } } diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java index 8099e3672e2c..fe61e4e0214e 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java @@ -221,6 +221,7 @@ public boolean isFilesystem() { } private Path resolve(BlobKey key) { + // Path-traversal is prevented at construction time by BlobKey (rejects `..` segments). return baseDir.resolve(key.value()); } diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java index 23c3ef22dd17..68478ecf524a 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java @@ -97,55 +97,80 @@ public class S3BlobStoreService implements BlobStoreService { private final S3Presigner presigner; public S3BlobStoreService(DhisConfigurationProvider configurationProvider) { - String containerName = configurationProvider.getProperty(ConfigurationKey.FILESTORE_CONTAINER); - String location = configurationProvider.getProperty(ConfigurationKey.FILESTORE_LOCATION); - String endpoint = configurationProvider.getProperty(ConfigurationKey.FILESTORE_ENDPOINT); - String identity = configurationProvider.getProperty(ConfigurationKey.FILESTORE_IDENTITY); - String secret = configurationProvider.getProperty(ConfigurationKey.FILESTORE_SECRET); - - this.container = new BlobContainerName(containerName); - - // SDK v2 requires a region even when an endpoint override is set; MinIO ≥ 2025-04 rejects a - // region that disagrees with the bucket's LocationConstraint, so fall back to us-east-1 (the - // S3 default that translates to no LocationConstraint on bucket creation) when unset. - Region region = StringUtils.isNotBlank(location) ? Region.of(location) : Region.US_EAST_1; + this( + new BlobContainerName( + configurationProvider.getProperty(ConfigurationKey.FILESTORE_CONTAINER)), + buildClient(configurationProvider), + buildPresigner(configurationProvider)); + } - // Fall back to the SDK's default credential chain (env vars, system props, IAM role, EC2 - // metadata, ...) when identity/secret are not explicitly configured — typical for AWS-hosted - // deployments that rely on IAM. Static credentials are used otherwise. - AwsCredentialsProvider credentials = - StringUtils.isNotBlank(identity) && StringUtils.isNotBlank(secret) - ? StaticCredentialsProvider.create(AwsBasicCredentials.create(identity, secret)) - : DefaultCredentialsProvider.builder().build(); + /** + * Package-private constructor for unit tests — lets the test supply mocked {@link S3Client} and + * {@link S3Presigner} without spinning up MinIO. The public constructor builds real clients from + * the {@link DhisConfigurationProvider}. + */ + S3BlobStoreService(BlobContainerName container, S3Client s3, S3Presigner presigner) { + this.container = container; + this.s3 = s3; + this.presigner = presigner; + } - // Path-style addressing is required for custom endpoints (MinIO/Ceph) and applied uniformly - // here for aws-s3 as well, matching the addressing mode used historically. + private static S3Client buildClient(DhisConfigurationProvider config) { + Region region = region(config); + AwsCredentialsProvider credentials = credentials(config); S3Configuration s3Config = S3Configuration.builder().pathStyleAccessEnabled(true).build(); - // SDK ≥ 2.30 dropped legacy Content-MD5 in favour of x-amz-checksum-* headers for // httpChecksumRequired operations. Older MinIO/Ceph builds reject DeleteObjects without - // Content-MD5, so we re-add it via an interceptor below. - S3ClientBuilder clientBuilder = + // Content-MD5, so we re-add it via the interceptor. + S3ClientBuilder builder = S3Client.builder() .region(region) .credentialsProvider(credentials) .serviceConfiguration(s3Config) .overrideConfiguration( o -> o.addExecutionInterceptor(new DeleteObjectsContentMd5Interceptor())); - Builder presignerBuilder = + String endpoint = config.getProperty(ConfigurationKey.FILESTORE_ENDPOINT); + if (StringUtils.isNotBlank(endpoint)) { + builder.endpointOverride(URI.create(endpoint)); + } + return builder.build(); + } + + private static S3Presigner buildPresigner(DhisConfigurationProvider config) { + Region region = region(config); + AwsCredentialsProvider credentials = credentials(config); + // Path-style addressing is required for custom endpoints (MinIO/Ceph) and applied uniformly + // here for aws-s3 as well, matching the addressing mode used historically. + S3Configuration s3Config = S3Configuration.builder().pathStyleAccessEnabled(true).build(); + Builder builder = S3Presigner.builder() .region(region) .credentialsProvider(credentials) .serviceConfiguration(s3Config); - + String endpoint = config.getProperty(ConfigurationKey.FILESTORE_ENDPOINT); if (StringUtils.isNotBlank(endpoint)) { - URI endpointUri = URI.create(endpoint); - clientBuilder.endpointOverride(endpointUri); - presignerBuilder.endpointOverride(endpointUri); + builder.endpointOverride(URI.create(endpoint)); } + return builder.build(); + } - this.s3 = clientBuilder.build(); - this.presigner = presignerBuilder.build(); + private static Region region(DhisConfigurationProvider config) { + // SDK v2 requires a region even when an endpoint override is set; MinIO ≥ 2025-04 rejects a + // region that disagrees with the bucket's LocationConstraint, so fall back to us-east-1 (the + // S3 default that translates to no LocationConstraint on bucket creation) when unset. + String location = config.getProperty(ConfigurationKey.FILESTORE_LOCATION); + return StringUtils.isNotBlank(location) ? Region.of(location) : Region.US_EAST_1; + } + + private static AwsCredentialsProvider credentials(DhisConfigurationProvider config) { + // Fall back to the SDK's default credential chain (env vars, system props, IAM role, EC2 + // metadata, ...) when identity/secret are not explicitly configured — typical for AWS-hosted + // deployments that rely on IAM. Static credentials are used otherwise. + String identity = config.getProperty(ConfigurationKey.FILESTORE_IDENTITY); + String secret = config.getProperty(ConfigurationKey.FILESTORE_SECRET); + return StringUtils.isNotBlank(identity) && StringUtils.isNotBlank(secret) + ? StaticCredentialsProvider.create(AwsBasicCredentials.create(identity, secret)) + : DefaultCredentialsProvider.builder().build(); } @PostConstruct diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceTest.java new file mode 100644 index 000000000000..df8e433c4562 --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceTest.java @@ -0,0 +1,92 @@ +/* + * Copyright (c) 2004-2026, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.storage; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.io.ByteArrayInputStream; +import java.nio.file.Path; +import org.hisp.dhis.external.conf.ConfigurationKey; +import org.hisp.dhis.external.conf.DhisConfigurationProvider; +import org.hisp.dhis.external.location.LocationManager; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Unit tests for {@link FileSystemBlobStoreService} init-time preconditions and the path-traversal + * guard. The end-to-end behaviour is covered by {@code FileSystemBlobStoreServiceContractTest} in + * dhis-test-integration. + */ +class FileSystemBlobStoreServiceTest { + + @Test + void constructor_externalDirectoryUnset_throws() { + LocationManager lm = mock(LocationManager.class); + when(lm.externalDirectorySet()).thenReturn(false); + + assertThrows(IllegalStateException.class, () -> new FileSystemBlobStoreService(config(), lm)); + } + + @Test + void putBlob_benignKey_resolvesInsideContainer(@TempDir Path tempDir) { + FileSystemBlobStoreService svc = newService(tempDir); + svc.init(); + byte[] payload = "ok".getBytes(); + + svc.putBlob( + new BlobKey("nested/file.txt"), + new ByteArrayInputStream(payload), + payload.length, + null, + null, + null); + + assertTrue(svc.blobExists(new BlobKey("nested/file.txt"))); + assertEquals(payload.length, svc.contentLength(new BlobKey("nested/file.txt"))); + } + + private static FileSystemBlobStoreService newService(Path tempDir) { + LocationManager lm = mock(LocationManager.class); + when(lm.externalDirectorySet()).thenReturn(true); + when(lm.getExternalDirectoryPath()).thenReturn(tempDir.toString()); + return new FileSystemBlobStoreService(config(), lm); + } + + private static DhisConfigurationProvider config() { + DhisConfigurationProvider config = mock(DhisConfigurationProvider.class); + lenient().when(config.getProperty(ConfigurationKey.FILESTORE_CONTAINER)).thenReturn("dhis2"); + return config; + } +} diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceTest.java new file mode 100644 index 000000000000..bf619b3397da --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceTest.java @@ -0,0 +1,181 @@ +/* + * Copyright (c) 2004-2026, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.storage; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.model.DeleteObjectsRequest; +import software.amazon.awssdk.services.s3.model.DeleteObjectsResponse; +import software.amazon.awssdk.services.s3.model.ListObjectsV2Request; +import software.amazon.awssdk.services.s3.model.ListObjectsV2Response; +import software.amazon.awssdk.services.s3.model.S3Error; +import software.amazon.awssdk.services.s3.model.S3Object; +import software.amazon.awssdk.services.s3.presigner.S3Presigner; + +/** + * Unit tests for {@link S3BlobStoreService} behaviours that the MinIO-backed contract test can't + * easily exercise — pagination across multiple {@code ListObjectsV2} pages, and {@code + * DeleteObjects} responses that report per-key errors. Uses Mockito-mocked {@link S3Client} via the + * package-private constructor. + */ +class S3BlobStoreServiceTest { + + private final BlobContainerName container = new BlobContainerName("dhis2"); + + @Test + void listKeys_paginatesAcrossMultiplePages() { + S3Client s3 = mock(S3Client.class); + when(s3.listObjectsV2(any(ListObjectsV2Request.class))) + .thenReturn(page(List.of("apps/a", "apps/b"), true, "tok-1")) + .thenReturn(page(List.of("apps/c"), false, null)); + + S3BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class)); + + List keys = new ArrayList<>(); + svc.listKeys(BlobKeyPrefix.of("apps")).forEach(k -> keys.add(k.value())); + + assertEquals(List.of("apps/a", "apps/b", "apps/c"), keys); + + ArgumentCaptor captor = + ArgumentCaptor.forClass(ListObjectsV2Request.class); + verify(s3, times(2)).listObjectsV2(captor.capture()); + List calls = captor.getAllValues(); + assertNull(calls.get(0).continuationToken(), "first call carries no continuation token"); + assertEquals( + "tok-1", + calls.get(1).continuationToken(), + "second call must pass the token returned by the first response"); + } + + @Test + void listFolders_paginatesAcrossMultiplePages() { + S3Client s3 = mock(S3Client.class); + when(s3.listObjectsV2(any(ListObjectsV2Request.class))) + .thenReturn(commonPrefixPage(List.of("apps/foo/", "apps/bar/"), true, "tok-1")) + .thenReturn(commonPrefixPage(List.of("apps/baz/"), false, null)); + + S3BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class)); + + List folders = new ArrayList<>(); + svc.listFolders(BlobKeyPrefix.of("apps")).forEach(p -> folders.add(p.value())); + + assertEquals(List.of("apps/foo", "apps/bar", "apps/baz"), folders); + verify(s3, times(2)).listObjectsV2(any(ListObjectsV2Request.class)); + } + + @Test + void deleteDirectory_paginatesAcrossMultiplePages() { + S3Client s3 = mock(S3Client.class); + when(s3.listObjectsV2(any(ListObjectsV2Request.class))) + .thenReturn(page(List.of("apps/a", "apps/b"), true, "tok-1")) + .thenReturn(page(List.of("apps/c"), false, null)); + when(s3.deleteObjects(any(DeleteObjectsRequest.class))) + .thenReturn(DeleteObjectsResponse.builder().build()); + + S3BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class)); + svc.deleteDirectory(BlobKeyPrefix.of("apps")); + + // Two list calls (paginated), two delete calls (one per page since each page had svc.deleteDirectory(BlobKeyPrefix.of("apps"))); + verify(s3, times(1)).deleteObjects(any(DeleteObjectsRequest.class)); + } + + @Test + void listKeys_filtersDirectoryMarkers() { + S3Client s3 = mock(S3Client.class); + when(s3.listObjectsV2(any(ListObjectsV2Request.class))) + .thenReturn(page(List.of("apps/", "apps/a", "apps/b/", "apps/b/c"), false, null)); + + S3BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class)); + + List keys = new ArrayList<>(); + svc.listKeys(BlobKeyPrefix.of("apps")).forEach(k -> keys.add(k.value())); + + assertEquals(List.of("apps/a", "apps/b/c"), keys); + assertTrue(keys.stream().noneMatch(k -> k.endsWith("/"))); + } + + private static ListObjectsV2Response page( + List keys, boolean truncated, String nextToken) { + return ListObjectsV2Response.builder() + .contents(keys.stream().map(k -> S3Object.builder().key(k).build()).toList()) + .isTruncated(truncated) + .nextContinuationToken(nextToken) + .build(); + } + + private static ListObjectsV2Response commonPrefixPage( + List prefixes, boolean truncated, String nextToken) { + return ListObjectsV2Response.builder() + .commonPrefixes( + prefixes.stream() + .map( + p -> + software.amazon.awssdk.services.s3.model.CommonPrefix.builder() + .prefix(p) + .build()) + .toList()) + .isTruncated(truncated) + .nextContinuationToken(nextToken) + .build(); + } +} diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java new file mode 100644 index 000000000000..6f3574559b5b --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java @@ -0,0 +1,81 @@ +/* + * Copyright (c) 2004-2026, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.storage; + +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; + +import java.io.ByteArrayInputStream; +import java.io.UncheckedIOException; +import org.hisp.dhis.external.conf.ConfigurationKey; +import org.hisp.dhis.external.conf.DhisConfigurationProvider; +import org.junit.jupiter.api.Test; + +/** + * Unit tests for {@link TransientBlobStoreService} edge cases that the contract test doesn't probe. + */ +class TransientBlobStoreServiceTest { + + @Test + void putBlob_streamShorterThanContentLength_throws() { + TransientBlobStoreService svc = newService(); + byte[] payload = "abc".getBytes(); + // Declare 10 bytes but only provide 3 — readNBytes returns the 3 it could read. + UncheckedIOException ex = + assertThrows( + UncheckedIOException.class, + () -> + svc.putBlob( + new BlobKey("k"), new ByteArrayInputStream(payload), 10L, null, null, null)); + // Sanity-check the wrapped IOException message references the mismatch. + org.junit.jupiter.api.Assertions.assertTrue( + ex.getCause().getMessage().contains("Expected 10"), + "expected wrapped IOException to mention declared length, got: " + + ex.getCause().getMessage()); + } + + @Test + void putBlob_contentLengthAboveInt_throws() { + TransientBlobStoreService svc = newService(); + long tooBig = (long) Integer.MAX_VALUE + 1; + assertThrows( + ArithmeticException.class, + () -> + svc.putBlob( + new BlobKey("k"), new ByteArrayInputStream(new byte[0]), tooBig, null, null, null)); + } + + private static TransientBlobStoreService newService() { + DhisConfigurationProvider config = mock(DhisConfigurationProvider.class); + lenient().when(config.getProperty(ConfigurationKey.FILESTORE_CONTAINER)).thenReturn("dhis2"); + return new TransientBlobStoreService(config); + } +} From 2603559da4304bf5b57fd7d30b98948377debd09 Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Thu, 14 May 2026 13:40:55 +0100 Subject: [PATCH 05/16] feat: Clean logs [DHIS2-20648] --- .../org/hisp/dhis/appmanager/BlobStoreAppStorageService.java | 2 +- .../src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java index 52121cc486b4..f08d7aa935d3 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java @@ -424,7 +424,7 @@ private static void logInstallSuccess(App app, String appFolder) { private static void logDiscoveredApps(Map> apps) { if (apps.isEmpty()) { - log.info("No apps found during app discovery."); + log.info("No apps found during app discovery"); } else { apps.values() .forEach( diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java index 68478ecf524a..c4b44f346e13 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java @@ -185,7 +185,7 @@ public void init() { // Concurrent startup or external creation — bucket exists, nothing to do. } } - log.info("S3 file store configured with bucket: '{}'.", bucket); + log.info("S3 file store configured with bucket: '{}'", bucket); } @PreDestroy From 9ccf9ad714ab723ad116349b9ac737d438121c07 Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Mon, 18 May 2026 07:14:50 +0100 Subject: [PATCH 06/16] Revert get content length --- .../hisp/dhis/appmanager/BlobStoreAppStorageService.java | 4 ++-- .../java/org/hisp/dhis/appmanager/DefaultAppManager.java | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java index f08d7aa935d3..2f68ae42768f 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java @@ -382,7 +382,7 @@ private Resource getResource(@Nonnull BlobKey key) throws IOException { if (uri != null) { return new UrlResource(uri); } - // Backend supports neither filesystem access nor signed URLs (e.g. transient): buffer + // When backend supports neither filesystem access nor signed URLs (e.g. transient): buffer // the blob bytes so Spring can serve them directly. try (InputStream in = blobStore.openStream(key)) { if (in == null) throw new IOException("Blob not found: " + key); @@ -424,7 +424,7 @@ private static void logInstallSuccess(App app, String appFolder) { private static void logDiscoveredApps(Map> apps) { if (apps.isEmpty()) { - log.info("No apps found during app discovery"); + log.info("No apps found during blob store discovery"); } else { apps.values() .forEach( diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/DefaultAppManager.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/DefaultAppManager.java index a91e25b5c7bc..69c85a421a51 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/DefaultAppManager.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/DefaultAppManager.java @@ -630,7 +630,11 @@ public ResourceResult getAppResource(App app, String pageName, String contextPat @Override public int getUriContentLength(@Nonnull Resource resource) { try { - return (int) resource.contentLength(); + if (resource.isFile()) { + return (int) resource.contentLength(); + } + URLConnection urlConnection = resource.getURL().openConnection(); + return urlConnection.getContentLength(); } catch (IOException e) { log.error("Error trying to retrieve content length of Resource: {}", e.getMessage()); return -1; From 6db21f15fc24badb08b6aeff64195ab2ee0c001e Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Mon, 18 May 2026 09:32:12 +0100 Subject: [PATCH 07/16] feat: Enable dir create/exists to keep existing behaviour --- .../BlobStoreAppStorageService.java | 13 ++--- .../hisp/dhis/storage/BlobStoreService.java | 24 +++++++++ .../storage/FileSystemBlobStoreService.java | 15 ++++++ .../hisp/dhis/storage/S3BlobStoreService.java | 28 +++++++++++ .../storage/TransientBlobStoreService.java | 18 ++++++- .../BlobStoreAppStorageServiceTest.java | 9 ++-- .../storage/BlobStoreServiceContractTest.java | 49 +++++++++++++++++-- 7 files changed, 139 insertions(+), 17 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java index 2f68ae42768f..3db0f79e8a88 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java @@ -291,10 +291,15 @@ private void unzipFile(File file, AppFolderName folder, String topLevelFolder) Enumeration entries = zipFile.entries(); while (entries.hasMoreElements()) { ZipEntry zipEntry = entries.nextElement(); - if (zipEntry.isDirectory()) continue; String filePath = getFilePath(folder.path(), topLevelFolder, zipEntry); // If it's the root folder, skip if (filePath == null) continue; + if (zipEntry.isDirectory()) { + // Materialise empty directories so the /api/apps//

    → // redirect + // contract holds even when the zip contains a directory with no files inside. + blobStore.createDirectory(BlobKeyPrefix.of(filePath)); + continue; + } try (InputStream zipInputStream = zipFile.getInputStream(zipEntry)) { blobStore.putBlob( new BlobKey(filePath), zipInputStream, zipEntry.getSize(), null, null, null); @@ -362,17 +367,13 @@ public ResourceResult getAppResource(@CheckForNull App app, @Nonnull String reso if (blobStore.blobExists(key)) { return new ResourceFound(getResource(key)); } - if (keyExistsAsDirectory(key)) { + if (blobStore.directoryExists(BlobKeyPrefix.of(key.value()))) { return new Redirect(resource + "/"); } log.debug("ResourceNotFound {} for App {}", key, app.getName()); return new ResourceNotFound(resource); } - private boolean keyExistsAsDirectory(BlobKey key) { - return blobStore.listKeys(BlobKeyPrefix.of(key.value())).iterator().hasNext(); - } - private Resource getResource(@Nonnull BlobKey key) throws IOException { if (blobStore.isFilesystem()) { return new FileSystemResource( diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreService.java index 3ad6295355a4..2af104c1a9f7 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreService.java @@ -127,6 +127,30 @@ void putBlob( */ Iterable listKeys(BlobKeyPrefix prefix); + /** + * Returns {@code true} if a directory exists at {@code prefix} — either because a real blob is + * stored under it, or because it was explicitly materialised via {@link + * #createDirectory(BlobKeyPrefix)}. Used by the app serving path to decide whether a request for + * {@code /someDir} should redirect to {@code /someDir/} or fall through to a 404. + * + *

    Filesystem backends report directory existence directly. Object-store backends emulate + * directories: any object whose key starts with {@code prefix + "/"} causes this method to return + * {@code true}. + */ + boolean directoryExists(BlobKeyPrefix prefix); + + /** + * Records the existence of a (possibly empty) directory at {@code prefix}. Idempotent — calling + * this for an already-existing or non-empty directory is a no-op. + * + *

    Filesystem backends create a real directory on disk. Object-store backends, which have no + * native directory concept, write a zero-byte placeholder object at {@code prefix + "/"} (the + * de-facto S3 convention) so that subsequent {@link #directoryExists(BlobKeyPrefix)} calls return + * {@code true} even when no real blobs have been stored beneath the prefix. The placeholder is + * never returned from {@link #listKeys(BlobKeyPrefix)}. + */ + void createDirectory(BlobKeyPrefix prefix); + /** * Returns a pre-signed GET URI valid for {@code expirationSeconds} seconds, or {@code null} if * the backend does not support request signing (e.g. local filesystem). diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java index fe61e4e0214e..eb42b1a8204a 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java @@ -204,6 +204,21 @@ public FileVisitResult visitFile( return result; } + @Override + public boolean directoryExists(BlobKeyPrefix prefix) { + return Files.isDirectory(baseDir.resolve(prefix.value())); + } + + @Override + public void createDirectory(BlobKeyPrefix prefix) { + Path dir = baseDir.resolve(prefix.value()); + try { + Files.createDirectories(dir); + } catch (IOException e) { + throw new UncheckedIOException("Unable to create directory " + dir, e); + } + } + @Override @CheckForNull public URI signedGetUri(BlobKey key, long expirationSeconds) { diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java index c4b44f346e13..e351065002fa 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java @@ -333,6 +333,34 @@ public Iterable listKeys(BlobKeyPrefix prefix) { return result; } + @Override + public boolean directoryExists(BlobKeyPrefix prefix) { + ListObjectsV2Response resp = + s3.listObjectsV2( + ListObjectsV2Request.builder() + .bucket(container.value()) + .prefix(prefix.value() + "/") + .maxKeys(1) + .build()); + return !resp.contents().isEmpty(); + } + + @Override + public void createDirectory(BlobKeyPrefix prefix) { + // S3 has no native concept of an empty directory — write a zero-byte placeholder at + // `/` so that `directoryExists` can answer true even before any real blobs are + // stored beneath the prefix. `listKeys` filters trailing-slash entries out, so the + // placeholder never surfaces through the listing contract. + String markerKey = prefix.value() + "/"; + s3.putObject( + PutObjectRequest.builder() + .bucket(container.value()) + .key(markerKey) + .contentLength(0L) + .build(), + RequestBody.fromBytes(new byte[0])); + } + @Override @CheckForNull public URI signedGetUri(BlobKey key, long expirationSeconds) { diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java index 43818c722d3a..33ea0a07a052 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java @@ -143,13 +143,29 @@ public Iterable listKeys(BlobKeyPrefix prefix) { String pfx = prefix.value(); List result = new ArrayList<>(); for (String k : blobs.keySet()) { - if (k.startsWith(pfx)) { + // Filter out synthetic directory markers (keys ending with '/') so the listing + // contract matches the other backends. + if (k.startsWith(pfx) && !k.endsWith("/")) { result.add(new BlobKey(k)); } } return result; } + @Override + public boolean directoryExists(BlobKeyPrefix prefix) { + String pfx = prefix.value() + "/"; + for (String k : blobs.keySet()) { + if (k.startsWith(pfx)) return true; + } + return false; + } + + @Override + public void createDirectory(BlobKeyPrefix prefix) { + blobs.put(prefix.value() + "/", new byte[0]); + } + @Override @CheckForNull public URI signedGetUri(BlobKey key, long expirationSeconds) { diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/BlobStoreAppStorageServiceTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/BlobStoreAppStorageServiceTest.java index 63371b13eb3e..18ad5082ccc9 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/BlobStoreAppStorageServiceTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/BlobStoreAppStorageServiceTest.java @@ -38,7 +38,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import java.io.File; import java.io.IOException; -import java.util.List; import org.hisp.dhis.appmanager.ResourceResult.Redirect; import org.hisp.dhis.appmanager.ResourceResult.ResourceFound; import org.hisp.dhis.appmanager.ResourceResult.ResourceNotFound; @@ -193,10 +192,10 @@ void getAppResource_trailingSlashSubDir_withLeadingSlash_servesIndexHtml() throw @Test void getAppResource_bareSubDirName_redirectsToTrailingSlash() throws IOException { - // blob "subDir" does not exist as a file, but keys exist under that prefix + // blob "subDir" does not exist as a file, but the directory does when(blobStore.blobExists(BlobKey.of(FOLDER, "subDir"))).thenReturn(false); - when(blobStore.listKeys(BlobKeyPrefix.of(BlobKey.of(FOLDER, "subDir").value()))) - .thenReturn(List.of(BlobKey.of(FOLDER, "subDir", "index.html"))); + when(blobStore.directoryExists(BlobKeyPrefix.of(BlobKey.of(FOLDER, "subDir").value()))) + .thenReturn(true); ResourceResult result = service.getAppResource(app, "subDir"); @@ -207,7 +206,7 @@ void getAppResource_bareSubDirName_redirectsToTrailingSlash() throws IOException @Test void getAppResource_missingResource_returnsNotFound() throws IOException { when(blobStore.blobExists(any())).thenReturn(false); - when(blobStore.listKeys(any())).thenReturn(List.of()); + when(blobStore.directoryExists(any())).thenReturn(false); ResourceResult result = service.getAppResource(app, "missing.js"); diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java index 725a35790795..4826217bfbe8 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java @@ -50,9 +50,7 @@ import java.net.http.HttpRequest; import java.net.http.HttpResponse; import java.time.Duration; -import java.util.ArrayList; import java.util.HashSet; -import java.util.List; import java.util.Set; import org.awaitility.Awaitility; import org.hisp.dhis.storage.BlobStoreService.ContentDisposition; @@ -97,9 +95,10 @@ protected boolean validatesContentMd5() { @AfterEach void cleanUpTestData() { - List keys = new ArrayList<>(); - service().listKeys(testPrefix).forEach(keys::add); - keys.forEach(k -> assertDoesNotThrow(() -> service().deleteBlob(k))); + // deleteDirectory removes both real blobs and any synthetic directory markers (which + // listKeys deliberately excludes) so tests that exercise createDirectory leave no + // residue for subsequent tests. + assertDoesNotThrow(() -> service().deleteDirectory(testPrefix)); } // -------------------------------------------------------------------- existence + read @@ -278,6 +277,46 @@ void listKeys_filtersByPrefix() { assertEquals(Set.of(key("p1/x").value()), keys); } + // -------------------------------------------------------------------- directories + + @Test + void directoryExists_unknown_returnsFalse() { + assertFalse(service().directoryExists(BlobKeyPrefix.of(key("never-existed").value()))); + } + + @Test + void createDirectory_then_directoryExists_returnsTrue() { + BlobKeyPrefix dir = BlobKeyPrefix.of(key("empty-dir").value()); + service().createDirectory(dir); + assertTrue(service().directoryExists(dir)); + } + + @Test + void directoryExists_returnsTrue_whenBlobStoredUnderPrefix() { + putString(key("populated/inside"), "x"); + assertTrue(service().directoryExists(BlobKeyPrefix.of(key("populated").value()))); + } + + @Test + void createDirectory_isIdempotent() { + BlobKeyPrefix dir = BlobKeyPrefix.of(key("repeat").value()); + service().createDirectory(dir); + assertDoesNotThrow(() -> service().createDirectory(dir)); + assertTrue(service().directoryExists(dir)); + } + + @Test + void listKeys_excludesDirectoryMarkers() { + BlobKeyPrefix dir = BlobKeyPrefix.of(key("filtered").value()); + service().createDirectory(dir); + putString(key("filtered/file"), "x"); + + Set keys = new HashSet<>(); + service().listKeys(dir).forEach(k -> keys.add(k.value())); + + assertEquals(Set.of(key("filtered/file").value()), keys); + } + // -------------------------------------------------------------------- presigning @Test From 5abf6f11671d99bf5b52a94d64c2e6a660f685c2 Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Mon, 18 May 2026 11:19:10 +0100 Subject: [PATCH 08/16] clean up tests --- .../hisp/dhis/storage/BlobStoreConfigTest.java | 10 ++++++---- .../FileSystemBlobStoreServiceTest.java | 3 ++- .../storage/TransientBlobStoreServiceTest.java | 18 ++++++++---------- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/BlobStoreConfigTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/BlobStoreConfigTest.java index 332d5f59a854..e4464f91ebca 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/BlobStoreConfigTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/BlobStoreConfigTest.java @@ -29,9 +29,9 @@ */ package org.hisp.dhis.storage; -import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -81,12 +81,14 @@ void selectsTransientForTransientProvider() { @Test void rejectsUnknownProvider() { + LocationManager locationManager = mock(LocationManager.class); + BlobStoreConfig blobStoreConfig = new BlobStoreConfig(); + DhisConfigurationProvider dhisConfigWithS4 = config("s4"); IllegalArgumentException e = assertThrows( IllegalArgumentException.class, - () -> - new BlobStoreConfig().blobStoreService(config("s4"), mock(LocationManager.class))); - assertEquals(true, e.getMessage().contains("s4")); + () -> blobStoreConfig.blobStoreService(dhisConfigWithS4, locationManager)); + assertTrue(e.getMessage().contains("s4")); } private static DhisConfigurationProvider config(String provider) { diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceTest.java index df8e433c4562..bd2c4f2a18f0 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceTest.java @@ -55,8 +55,9 @@ class FileSystemBlobStoreServiceTest { void constructor_externalDirectoryUnset_throws() { LocationManager lm = mock(LocationManager.class); when(lm.externalDirectorySet()).thenReturn(false); + DhisConfigurationProvider config = config(); - assertThrows(IllegalStateException.class, () -> new FileSystemBlobStoreService(config(), lm)); + assertThrows(IllegalStateException.class, () -> new FileSystemBlobStoreService(config, lm)); } @Test diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java index 6f3574559b5b..9494d6e0b619 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java @@ -30,6 +30,7 @@ package org.hisp.dhis.storage; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; @@ -48,15 +49,14 @@ class TransientBlobStoreServiceTest { void putBlob_streamShorterThanContentLength_throws() { TransientBlobStoreService svc = newService(); byte[] payload = "abc".getBytes(); + ByteArrayInputStream bais = new ByteArrayInputStream(payload); + BlobKey key = new BlobKey("k"); // Declare 10 bytes but only provide 3 — readNBytes returns the 3 it could read. UncheckedIOException ex = assertThrows( - UncheckedIOException.class, - () -> - svc.putBlob( - new BlobKey("k"), new ByteArrayInputStream(payload), 10L, null, null, null)); + UncheckedIOException.class, () -> svc.putBlob(key, bais, 10L, null, null, null)); // Sanity-check the wrapped IOException message references the mismatch. - org.junit.jupiter.api.Assertions.assertTrue( + assertTrue( ex.getCause().getMessage().contains("Expected 10"), "expected wrapped IOException to mention declared length, got: " + ex.getCause().getMessage()); @@ -66,11 +66,9 @@ void putBlob_streamShorterThanContentLength_throws() { void putBlob_contentLengthAboveInt_throws() { TransientBlobStoreService svc = newService(); long tooBig = (long) Integer.MAX_VALUE + 1; - assertThrows( - ArithmeticException.class, - () -> - svc.putBlob( - new BlobKey("k"), new ByteArrayInputStream(new byte[0]), tooBig, null, null, null)); + BlobKey key = new BlobKey("k"); + ByteArrayInputStream bais = new ByteArrayInputStream(new byte[0]); + assertThrows(ArithmeticException.class, () -> svc.putBlob(key, bais, tooBig, null, null, null)); } private static TransientBlobStoreService newService() { From de63752f204fe0eca908634042ee057ed2ede4a7 Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Tue, 19 May 2026 08:55:08 +0100 Subject: [PATCH 09/16] feat: Use static of --- .../main/java/org/hisp/dhis/appmanager/AppFolderName.java | 2 +- .../main/java/org/hisp/dhis/fileresource/FileResource.java | 2 +- .../hisp/dhis/appmanager/BlobStoreAppStorageService.java | 2 +- .../fileresource/BlobStoreFileResourceContentStore.java | 2 +- .../hisp/dhis/fileresource/DefaultFileResourceService.java | 2 +- .../hisp/dhis/fileresource/FileResourceEventListener.java | 2 +- .../org/hisp/dhis/storage/FileSystemBlobStoreService.java | 2 +- .../main/java/org/hisp/dhis/storage/S3BlobStoreService.java | 2 +- .../org/hisp/dhis/storage/TransientBlobStoreService.java | 2 +- .../hisp/dhis/storage/FileSystemBlobStoreServiceTest.java | 6 +++--- .../hisp/dhis/storage/TransientBlobStoreServiceTest.java | 4 ++-- 11 files changed, 14 insertions(+), 14 deletions(-) diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/AppFolderName.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/AppFolderName.java index 43d45b44d6f6..56e033096a79 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/AppFolderName.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/AppFolderName.java @@ -100,7 +100,7 @@ public BlobKey resolve(String resource) { * org.hisp.dhis.storage.BlobStoreService#deleteDirectory}. */ public BlobKeyPrefix asPrefix() { - return new BlobKeyPrefix(path); + return BlobKeyPrefix.of(path); } @Nonnull diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/fileresource/FileResource.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/fileresource/FileResource.java index fae9c30c3180..ef9548b6ff11 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/fileresource/FileResource.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/fileresource/FileResource.java @@ -201,7 +201,7 @@ public String getStorageKey() { * rather than {@link String}. */ public BlobKey asBlobKey() { - return new BlobKey(storageKey); + return BlobKey.of(storageKey); } public void setStorageKey(String storageKey) { diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java index 3db0f79e8a88..e3830a549593 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java @@ -302,7 +302,7 @@ private void unzipFile(File file, AppFolderName folder, String topLevelFolder) } try (InputStream zipInputStream = zipFile.getInputStream(zipEntry)) { blobStore.putBlob( - new BlobKey(filePath), zipInputStream, zipEntry.getSize(), null, null, null); + BlobKey.of(filePath), zipInputStream, zipEntry.getSize(), null, null, null); } } } diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/BlobStoreFileResourceContentStore.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/BlobStoreFileResourceContentStore.java index f0572dbe9807..a6753a0d9199 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/BlobStoreFileResourceContentStore.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/BlobStoreFileResourceContentStore.java @@ -141,7 +141,7 @@ public String saveFileResourceContent( try (InputStream is = new FileInputStream(file)) { blobStore.putBlob( - new BlobKey(fr.getStorageKey() + dimension), + BlobKey.of(fr.getStorageKey() + dimension), is, file.length(), fr.getContentType(), diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/DefaultFileResourceService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/DefaultFileResourceService.java index df8836507701..ff866ee54962 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/DefaultFileResourceService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/DefaultFileResourceService.java @@ -374,7 +374,7 @@ private static boolean hasMultiDimensionImageSupport(FileResource fileResource) } private static BlobKey imageKey(FileResource fileResource, ImageFileDimension imageDimension) { - return new BlobKey(fileResource.getStorageKey() + imageDimension.getDimension()); + return BlobKey.of(fileResource.getStorageKey() + imageDimension.getDimension()); } @Override diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/FileResourceEventListener.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/FileResourceEventListener.java index 12bbf7373a01..aecfaca4356a 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/FileResourceEventListener.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/FileResourceEventListener.java @@ -140,7 +140,7 @@ public void deleteFile(FileDeletedEvent deleteFileEvent) { .forEach( d -> fileResourceContentStore.deleteFileResourceContent( - new BlobKey(baseKey + d.getDimension()))); + BlobKey.of(baseKey + d.getDimension()))); } else { fileResourceContentStore.deleteFileResourceContent(deleteFileEvent.storageKey()); } diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java index eb42b1a8204a..3ad2e3158df0 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/FileSystemBlobStoreService.java @@ -194,7 +194,7 @@ public Iterable listKeys(BlobKeyPrefix prefix) { public FileVisitResult visitFile( @Nonnull Path file, @Nonnull BasicFileAttributes attrs) { Path relative = baseDir.relativize(file); - result.add(new BlobKey(toBlobPath(relative))); + result.add(BlobKey.of(toBlobPath(relative))); return FileVisitResult.CONTINUE; } }); diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java index e351065002fa..b8e950ed3432 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java @@ -325,7 +325,7 @@ public Iterable listKeys(BlobKeyPrefix prefix) { for (S3Object obj : resp.contents()) { // Skip synthetic directory markers — some S3-compatible backends emit them. if (obj.key().endsWith("/")) continue; - result.add(new BlobKey(obj.key())); + result.add(BlobKey.of(obj.key())); } continuationToken = Boolean.TRUE.equals(resp.isTruncated()) ? resp.nextContinuationToken() : null; diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java index 33ea0a07a052..4b9667c21dd8 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java @@ -146,7 +146,7 @@ public Iterable listKeys(BlobKeyPrefix prefix) { // Filter out synthetic directory markers (keys ending with '/') so the listing // contract matches the other backends. if (k.startsWith(pfx) && !k.endsWith("/")) { - result.add(new BlobKey(k)); + result.add(BlobKey.of(k)); } } return result; diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceTest.java index bd2c4f2a18f0..4af2b79ead40 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/FileSystemBlobStoreServiceTest.java @@ -67,15 +67,15 @@ void putBlob_benignKey_resolvesInsideContainer(@TempDir Path tempDir) { byte[] payload = "ok".getBytes(); svc.putBlob( - new BlobKey("nested/file.txt"), + BlobKey.of("nested/file.txt"), new ByteArrayInputStream(payload), payload.length, null, null, null); - assertTrue(svc.blobExists(new BlobKey("nested/file.txt"))); - assertEquals(payload.length, svc.contentLength(new BlobKey("nested/file.txt"))); + assertTrue(svc.blobExists(BlobKey.of("nested/file.txt"))); + assertEquals(payload.length, svc.contentLength(BlobKey.of("nested/file.txt"))); } private static FileSystemBlobStoreService newService(Path tempDir) { diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java index 9494d6e0b619..d71f89e74208 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java @@ -50,7 +50,7 @@ void putBlob_streamShorterThanContentLength_throws() { TransientBlobStoreService svc = newService(); byte[] payload = "abc".getBytes(); ByteArrayInputStream bais = new ByteArrayInputStream(payload); - BlobKey key = new BlobKey("k"); + BlobKey key = BlobKey.of("k"); // Declare 10 bytes but only provide 3 — readNBytes returns the 3 it could read. UncheckedIOException ex = assertThrows( @@ -66,7 +66,7 @@ void putBlob_streamShorterThanContentLength_throws() { void putBlob_contentLengthAboveInt_throws() { TransientBlobStoreService svc = newService(); long tooBig = (long) Integer.MAX_VALUE + 1; - BlobKey key = new BlobKey("k"); + BlobKey key = BlobKey.of("k"); ByteArrayInputStream bais = new ByteArrayInputStream(new byte[0]); assertThrows(ArithmeticException.class, () -> svc.putBlob(key, bais, tooBig, null, null, null)); } From 6a8905e7f474c885578e4ddf6d410b6d75280ea5 Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Tue, 19 May 2026 11:31:35 +0100 Subject: [PATCH 10/16] feat: Craete bucket first without HEAD call --- .../hisp/dhis/storage/S3BlobStoreService.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java index b8e950ed3432..d4e9ba0dab6e 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java @@ -61,6 +61,7 @@ import software.amazon.awssdk.services.s3.S3Client; import software.amazon.awssdk.services.s3.S3ClientBuilder; import software.amazon.awssdk.services.s3.S3Configuration; +import software.amazon.awssdk.services.s3.model.BucketAlreadyExistsException; import software.amazon.awssdk.services.s3.model.BucketAlreadyOwnedByYouException; import software.amazon.awssdk.services.s3.model.CommonPrefix; import software.amazon.awssdk.services.s3.model.DeleteObjectsRequest; @@ -68,7 +69,6 @@ import software.amazon.awssdk.services.s3.model.HeadObjectResponse; import software.amazon.awssdk.services.s3.model.ListObjectsV2Request; import software.amazon.awssdk.services.s3.model.ListObjectsV2Response; -import software.amazon.awssdk.services.s3.model.NoSuchBucketException; import software.amazon.awssdk.services.s3.model.NoSuchKeyException; import software.amazon.awssdk.services.s3.model.ObjectIdentifier; import software.amazon.awssdk.services.s3.model.PutObjectRequest; @@ -175,15 +175,16 @@ private static AwsCredentialsProvider credentials(DhisConfigurationProvider conf @PostConstruct public void init() { + // Create-first idempotent pattern: try to create the bucket and swallow the "already exists" + // responses. Avoids HeadBucket, which is documented to return either 404 NoSuchBucket or 403 + // Forbidden when the bucket doesn't exist (depending on caller permissions) — empty MinIO + // servers consistently return 403, which a strict NoSuchBucketException catch misses. String bucket = container.value(); try { - s3.headBucket(b -> b.bucket(bucket)); - } catch (NoSuchBucketException e) { - try { - s3.createBucket(b -> b.bucket(bucket)); - } catch (BucketAlreadyOwnedByYouException ignored) { - // Concurrent startup or external creation — bucket exists, nothing to do. - } + s3.createBucket(b -> b.bucket(bucket)); + } catch (BucketAlreadyOwnedByYouException | BucketAlreadyExistsException ignored) { + // Bucket already exists (either from a previous startup, a concurrent startup, or external + // creation). Nothing to do. } log.info("S3 file store configured with bucket: '{}'", bucket); } From c64f5fdc97811ceab8e1a3cee291ba095434b5f1 Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Tue, 19 May 2026 13:40:32 +0100 Subject: [PATCH 11/16] feat: Address PR comments --- .../BlobStoreAppStorageService.java | 2 +- .../dhis/appmanager/DefaultAppManager.java | 4 +- .../storage/TransientBlobStoreService.java | 18 +++++- .../dhis/storage/S3BlobStoreServiceTest.java | 58 +++++++++++++++++-- .../TransientBlobStoreServiceTest.java | 4 +- 5 files changed, 73 insertions(+), 13 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java index e3830a549593..bf7c5d72b002 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java @@ -79,7 +79,7 @@ */ @Slf4j @RequiredArgsConstructor -@Service("org.hisp.dhis.appmanager.BlobStoreAppStorageService") +@Service public class BlobStoreAppStorageService implements AppStorageService { private static final String BUNDLED_APP_INFO_FILENAME = "bundled-app-info.json"; diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/DefaultAppManager.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/DefaultAppManager.java index 69c85a421a51..83a68ff8cf74 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/DefaultAppManager.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/DefaultAppManager.java @@ -86,7 +86,6 @@ import org.hisp.dhis.user.User; import org.hisp.dhis.user.UserService; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.core.io.ByteArrayResource; import org.springframework.core.io.Resource; import org.springframework.stereotype.Component; @@ -120,8 +119,7 @@ public class DefaultAppManager implements AppManager { public DefaultAppManager( DhisConfigurationProvider dhisConfigurationProvider, AppHubService appHubService, - @Qualifier("org.hisp.dhis.appmanager.BlobStoreAppStorageService") - AppStorageService blobStoreAppStorageService, + AppStorageService blobStoreAppStorageService, DatastoreService datastoreService, CacheBuilderProvider cacheBuilderProvider, I18nManager i18nManager, diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java index 4b9667c21dd8..53d4c656ebfd 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/TransientBlobStoreService.java @@ -35,10 +35,10 @@ import java.io.UncheckedIOException; import java.net.URI; import java.util.ArrayList; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.TreeSet; import java.util.concurrent.ConcurrentHashMap; import javax.annotation.CheckForNull; import org.hisp.dhis.external.conf.ConfigurationKey; @@ -80,6 +80,20 @@ public long contentLength(BlobKey key) { return payload == null ? 0L : payload.length; } + /** + * @param key identifies the blob within the container; acts as a path-like object-store key (e.g. + * {@code apps/my-app/index.html}) + * @param content the data to store; must be open and positioned at the start when passed in; + * exactly {@code contentLength} bytes will be read + * @param contentLength the number of bytes in {@code content}; rejects anything larger than + * Integer.MAX_VALUE up-front + * @param contentType MIME type of the blob (e.g. {@code "image/png"}); unused in this + * implementation. + * @param contentDisposition how the blob should be presented when downloaded (e.g. {@code + * attachment; filename="report.pdf"}); unused in this implementation. + * @param contentHash MD5 hash of the blob content used for integrity verification; unused in this + * implementation. + */ @Override public void putBlob( BlobKey key, @@ -120,7 +134,7 @@ public void deleteDirectory(BlobKeyPrefix prefix) { @Override public Iterable listFolders(BlobKeyPrefix prefix) { String pfx = prefix.value() + "/"; - Set immediate = new HashSet<>(); + Set immediate = new TreeSet<>(); for (String k : blobs.keySet()) { if (!k.startsWith(pfx)) continue; String remainder = k.substring(pfx.length()); diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceTest.java index bf619b3397da..b60cc3a79f01 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceTest.java @@ -48,6 +48,7 @@ import software.amazon.awssdk.services.s3.model.DeleteObjectsResponse; import software.amazon.awssdk.services.s3.model.ListObjectsV2Request; import software.amazon.awssdk.services.s3.model.ListObjectsV2Response; +import software.amazon.awssdk.services.s3.model.PutObjectRequest; import software.amazon.awssdk.services.s3.model.S3Error; import software.amazon.awssdk.services.s3.model.S3Object; import software.amazon.awssdk.services.s3.presigner.S3Presigner; @@ -69,7 +70,7 @@ void listKeys_paginatesAcrossMultiplePages() { .thenReturn(page(List.of("apps/a", "apps/b"), true, "tok-1")) .thenReturn(page(List.of("apps/c"), false, null)); - S3BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class)); + BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class)); List keys = new ArrayList<>(); svc.listKeys(BlobKeyPrefix.of("apps")).forEach(k -> keys.add(k.value())); @@ -94,7 +95,7 @@ void listFolders_paginatesAcrossMultiplePages() { .thenReturn(commonPrefixPage(List.of("apps/foo/", "apps/bar/"), true, "tok-1")) .thenReturn(commonPrefixPage(List.of("apps/baz/"), false, null)); - S3BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class)); + BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class)); List folders = new ArrayList<>(); svc.listFolders(BlobKeyPrefix.of("apps")).forEach(p -> folders.add(p.value())); @@ -112,7 +113,7 @@ void deleteDirectory_paginatesAcrossMultiplePages() { when(s3.deleteObjects(any(DeleteObjectsRequest.class))) .thenReturn(DeleteObjectsResponse.builder().build()); - S3BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class)); + BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class)); svc.deleteDirectory(BlobKeyPrefix.of("apps")); // Two list calls (paginated), two delete calls (one per page since each page had svc.deleteDirectory(BlobKeyPrefix.of("apps"))); @@ -145,7 +146,7 @@ void listKeys_filtersDirectoryMarkers() { when(s3.listObjectsV2(any(ListObjectsV2Request.class))) .thenReturn(page(List.of("apps/", "apps/a", "apps/b/", "apps/b/c"), false, null)); - S3BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class)); + BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class)); List keys = new ArrayList<>(); svc.listKeys(BlobKeyPrefix.of("apps")).forEach(k -> keys.add(k.value())); @@ -154,6 +155,53 @@ void listKeys_filtersDirectoryMarkers() { assertTrue(keys.stream().noneMatch(k -> k.endsWith("/"))); } + @Test + void keyPrefix_prependedOnListAndStrippedFromReturnedKeys() { + // With keyPrefix="dev", listObjectsV2 is issued with prefix "dev/apps" and any keys returned + // from the store come back stripped to the caller-visible "apps/..." form. + S3Client s3 = mock(S3Client.class); + when(s3.listObjectsV2(any(ListObjectsV2Request.class))) + .thenReturn(page(List.of("dev/apps/a", "dev/apps/b"), false, null)); + + BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class), "dev"); + + List keys = new ArrayList<>(); + svc.listKeys(BlobKeyPrefix.of("apps")).forEach(k -> keys.add(k.value())); + assertEquals(List.of("apps/a", "apps/b"), keys); + + ArgumentCaptor listCaptor = + ArgumentCaptor.forClass(ListObjectsV2Request.class); + verify(s3).listObjectsV2(listCaptor.capture()); + assertEquals("dev/apps", listCaptor.getValue().prefix()); + } + + @Test + void createDirectory_writesMarkerUnderPrefix() { + // The synthetic directory marker must also live under the configured prefix so it's covered + // by the IAM scoped to that prefix. + S3Client s3 = mock(S3Client.class); + BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class), "dev"); + + svc.createDirectory(BlobKeyPrefix.of("apps/my-app")); + + ArgumentCaptor putCaptor = ArgumentCaptor.forClass(PutObjectRequest.class); + verify(s3) + .putObject(putCaptor.capture(), any(software.amazon.awssdk.core.sync.RequestBody.class)); + assertEquals("dev/apps/my-app/", putCaptor.getValue().key()); + } + + @Test + void normalizeKeyPrefix_stripsLeadingAndTrailingSlashes() { + assertEquals("", S3BlobStoreService.normalizeKeyPrefix(null)); + assertEquals("", S3BlobStoreService.normalizeKeyPrefix("")); + assertEquals("", S3BlobStoreService.normalizeKeyPrefix(" ")); + assertEquals("dev", S3BlobStoreService.normalizeKeyPrefix("dev")); + assertEquals("dev", S3BlobStoreService.normalizeKeyPrefix("/dev")); + assertEquals("dev", S3BlobStoreService.normalizeKeyPrefix("dev/")); + assertEquals("dev", S3BlobStoreService.normalizeKeyPrefix("/dev/")); + assertEquals("a/b", S3BlobStoreService.normalizeKeyPrefix("/a/b/")); + } + private static ListObjectsV2Response page( List keys, boolean truncated, String nextToken) { return ListObjectsV2Response.builder() diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java index d71f89e74208..8ea3a19e1f83 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/TransientBlobStoreServiceTest.java @@ -47,7 +47,7 @@ class TransientBlobStoreServiceTest { @Test void putBlob_streamShorterThanContentLength_throws() { - TransientBlobStoreService svc = newService(); + BlobStoreService svc = newService(); byte[] payload = "abc".getBytes(); ByteArrayInputStream bais = new ByteArrayInputStream(payload); BlobKey key = BlobKey.of("k"); @@ -64,7 +64,7 @@ void putBlob_streamShorterThanContentLength_throws() { @Test void putBlob_contentLengthAboveInt_throws() { - TransientBlobStoreService svc = newService(); + BlobStoreService svc = newService(); long tooBig = (long) Integer.MAX_VALUE + 1; BlobKey key = BlobKey.of("k"); ByteArrayInputStream bais = new ByteArrayInputStream(new byte[0]); From 7b9e7b89af3b62de5ed34987287ba38b3f13126d Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Tue, 19 May 2026 14:06:13 +0100 Subject: [PATCH 12/16] remove redundant tests --- .../dhis/storage/S3BlobStoreServiceTest.java | 48 ------------------- 1 file changed, 48 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceTest.java index b60cc3a79f01..1b76fac260b8 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/storage/S3BlobStoreServiceTest.java @@ -48,7 +48,6 @@ import software.amazon.awssdk.services.s3.model.DeleteObjectsResponse; import software.amazon.awssdk.services.s3.model.ListObjectsV2Request; import software.amazon.awssdk.services.s3.model.ListObjectsV2Response; -import software.amazon.awssdk.services.s3.model.PutObjectRequest; import software.amazon.awssdk.services.s3.model.S3Error; import software.amazon.awssdk.services.s3.model.S3Object; import software.amazon.awssdk.services.s3.presigner.S3Presigner; @@ -155,53 +154,6 @@ void listKeys_filtersDirectoryMarkers() { assertTrue(keys.stream().noneMatch(k -> k.endsWith("/"))); } - @Test - void keyPrefix_prependedOnListAndStrippedFromReturnedKeys() { - // With keyPrefix="dev", listObjectsV2 is issued with prefix "dev/apps" and any keys returned - // from the store come back stripped to the caller-visible "apps/..." form. - S3Client s3 = mock(S3Client.class); - when(s3.listObjectsV2(any(ListObjectsV2Request.class))) - .thenReturn(page(List.of("dev/apps/a", "dev/apps/b"), false, null)); - - BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class), "dev"); - - List keys = new ArrayList<>(); - svc.listKeys(BlobKeyPrefix.of("apps")).forEach(k -> keys.add(k.value())); - assertEquals(List.of("apps/a", "apps/b"), keys); - - ArgumentCaptor listCaptor = - ArgumentCaptor.forClass(ListObjectsV2Request.class); - verify(s3).listObjectsV2(listCaptor.capture()); - assertEquals("dev/apps", listCaptor.getValue().prefix()); - } - - @Test - void createDirectory_writesMarkerUnderPrefix() { - // The synthetic directory marker must also live under the configured prefix so it's covered - // by the IAM scoped to that prefix. - S3Client s3 = mock(S3Client.class); - BlobStoreService svc = new S3BlobStoreService(container, s3, mock(S3Presigner.class), "dev"); - - svc.createDirectory(BlobKeyPrefix.of("apps/my-app")); - - ArgumentCaptor putCaptor = ArgumentCaptor.forClass(PutObjectRequest.class); - verify(s3) - .putObject(putCaptor.capture(), any(software.amazon.awssdk.core.sync.RequestBody.class)); - assertEquals("dev/apps/my-app/", putCaptor.getValue().key()); - } - - @Test - void normalizeKeyPrefix_stripsLeadingAndTrailingSlashes() { - assertEquals("", S3BlobStoreService.normalizeKeyPrefix(null)); - assertEquals("", S3BlobStoreService.normalizeKeyPrefix("")); - assertEquals("", S3BlobStoreService.normalizeKeyPrefix(" ")); - assertEquals("dev", S3BlobStoreService.normalizeKeyPrefix("dev")); - assertEquals("dev", S3BlobStoreService.normalizeKeyPrefix("/dev")); - assertEquals("dev", S3BlobStoreService.normalizeKeyPrefix("dev/")); - assertEquals("dev", S3BlobStoreService.normalizeKeyPrefix("/dev/")); - assertEquals("a/b", S3BlobStoreService.normalizeKeyPrefix("/a/b/")); - } - private static ListObjectsV2Response page( List keys, boolean truncated, String nextToken) { return ListObjectsV2Response.builder() From 0794e47d248957d167f84d46a3464f1de1f17165 Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Tue, 19 May 2026 14:51:17 +0100 Subject: [PATCH 13/16] feat: Remove AppStorageSource, no longer required --- .../java/org/hisp/dhis/appmanager/App.java | 12 ------ .../dhis/appmanager/AppStorageSource.java | 37 ------------------- .../BlobStoreAppStorageService.java | 4 +- .../BlobStoreAppStorageServiceTest.java | 1 - 4 files changed, 1 insertion(+), 53 deletions(-) delete mode 100644 dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/AppStorageSource.java diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/App.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/App.java index d679de593e8b..caac63357b80 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/App.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/App.java @@ -82,8 +82,6 @@ public class App implements Serializable { private String defaultLocale; - private AppStorageSource appStorageSource; - private String folderName; /** Optional. */ @@ -414,22 +412,12 @@ public void setBaseUrl(String baseUrl) { this.baseUrl = baseUrl; } - @JsonProperty - @JacksonXmlProperty(namespace = DxfNamespaces.DXF_2_0) - public AppStorageSource getAppStorageSource() { - return appStorageSource; - } - @JsonProperty @JacksonXmlProperty(namespace = DxfNamespaces.DXF_2_0) public List getShortcuts() { return shortcuts; } - public void setAppStorageSource(AppStorageSource appStorageSource) { - this.appStorageSource = appStorageSource; - } - @JsonProperty @JacksonXmlProperty(namespace = DxfNamespaces.DXF_2_0) public Set getAuthorities() { diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/AppStorageSource.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/AppStorageSource.java deleted file mode 100644 index 9626c2f45a7a..000000000000 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/AppStorageSource.java +++ /dev/null @@ -1,37 +0,0 @@ -/* - * Copyright (c) 2004-2022, University of Oslo - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are met: - * - * 1. Redistributions of source code must retain the above copyright notice, this - * list of conditions and the following disclaimer. - * - * 2. Redistributions in binary form must reproduce the above copyright notice, - * this list of conditions and the following disclaimer in the documentation - * and/or other materials provided with the distribution. - * - * 3. Neither the name of the copyright holder nor the names of its contributors - * may be used to endorse or promote products derived from this software without - * specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED - * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE - * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR - * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES - * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; - * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON - * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS - * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - */ -package org.hisp.dhis.appmanager; - -/** - * @author Stian Sandvold - */ -public enum AppStorageSource { - BLOB_STORE -} diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java index bf7c5d72b002..622d2b8aa481 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/BlobStoreAppStorageService.java @@ -118,7 +118,6 @@ private void handleAppManifest( BiConsumer handler, BlobKeyPrefix folder, InputStream manifestStream) { try (InputStream inputStream = manifestStream) { App app = App.MAPPER.readValue(inputStream, App.class); - app.setAppStorageSource(AppStorageSource.BLOB_STORE); app.setFolderName(folder.value()); app.setManifestTranslations( readAppManifestTranslations( @@ -234,7 +233,6 @@ public App installApp( app = AppManager.readAppManifest(file, this.jsonMapper, topLevelFolder); folder = AppFolderName.ofKey(app.getKey()); app.setFolderName(folder.path()); - app.setAppStorageSource(AppStorageSource.BLOB_STORE); } catch (IOException e) { log.error("Failed to install app: Failure during reading manifest from zip file", e); app = new App(); @@ -352,7 +350,7 @@ public void deleteApp(@Nonnull App app) { @Nonnull public ResourceResult getAppResource(@CheckForNull App app, @Nonnull String resource) throws IOException { - if (app == null || !app.getAppStorageSource().equals(AppStorageSource.BLOB_STORE)) { + if (app == null) { log.warn( "Can't look up resource {}. The specified app was not found in blob storage.", resource); return new ResourceNotFound(resource); diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/BlobStoreAppStorageServiceTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/BlobStoreAppStorageServiceTest.java index 18ad5082ccc9..e44af5ca3670 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/BlobStoreAppStorageServiceTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/appmanager/BlobStoreAppStorageServiceTest.java @@ -76,7 +76,6 @@ void setUp() { blobStore, locationManager, new ObjectMapper(), fileResourceContentStore); app = new App(); - app.setAppStorageSource(AppStorageSource.BLOB_STORE); app.setFolderName(FOLDER); } From cc27c0cc005027c7718bf5da3ad0e8b5068f250f Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Thu, 21 May 2026 12:18:59 +0100 Subject: [PATCH 14/16] feat: Ensure streams are resetable when passing to s3 [DHIS2-20648] --- .../hisp/dhis/storage/BlobStoreService.java | 5 +- .../hisp/dhis/storage/S3BlobStoreService.java | 34 +++- dhis-2/dhis-test-integration/pom.xml | 14 ++ .../S3BlobStoreNonResetableStreamTest.java | 191 ++++++++++++++++++ 4 files changed, 242 insertions(+), 2 deletions(-) create mode 100644 dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreNonResetableStreamTest.java diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreService.java index 2af104c1a9f7..dab152a212c3 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/BlobStoreService.java @@ -81,7 +81,10 @@ public String toString() { /** * Stores a streaming payload under the given key. * - *

    The caller is responsible for closing {@code content} after this method returns. + *

    The caller is responsible for closing {@code content} after this method returns. The + * supplied stream is consumed exactly once and need not support {@code mark/reset}; + * implementations buffer internally if their backend requires re-reads (e.g. SDK retries on the + * S3 backend). * * @param key identifies the blob within the container; acts as a path-like object-store key (e.g. * {@code apps/my-app/index.html}) diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java index d4e9ba0dab6e..8bf2b4ce7b06 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/storage/S3BlobStoreService.java @@ -31,6 +31,7 @@ import java.io.IOException; import java.io.InputStream; +import java.io.UncheckedIOException; import java.net.URI; import java.net.URISyntaxException; import java.security.MessageDigest; @@ -249,7 +250,38 @@ public void putBlob( // S3 expects the Content-MD5 header as base64-encoded bytes; ContentHash stores hex. put.contentMD5(hexToBase64(contentHash.hex())); } - s3.putObject(put.build(), RequestBody.fromInputStream(content, contentLength)); + s3.putObject(put.build(), requestBodyFor(content, contentLength)); + } + + /** + * Builds a {@link RequestBody} that the SDK can safely re-read across retries. + * + *

    The SDK's SigV4 chunked-payload signing pipeline runs inside {@code RetryableStage}: when a + * request is retried (transient 5xx / socket reset / etc.), the whole pipeline re-runs and {@code + * AwsChunkedV4PayloadSigner.sign} calls {@code ContentStreamProvider.newStream()} again on each + * attempt. The wrapper from {@link RequestBody#fromInputStream} can only honour the second call + * when the underlying stream supports {@code mark/reset} — for streams that don't (e.g. {@code + * ZipFile.getInputStream(zipEntry)} used by app install) the wrapper throws. + * + *

    If the caller's stream is non-resetable we pre-buffer it into a byte array and use {@link + * RequestBody#fromBytes}, which is safely re-readable. Resetable streams (file streams, byte + * arrays, anything {@code BufferedInputStream}-wrapped) pass through unchanged so we don't + * regress streaming behaviour where it already works. + */ + private static RequestBody requestBodyFor(InputStream content, long contentLength) { + if (content.markSupported()) { + return RequestBody.fromInputStream(content, contentLength); + } + try { + byte[] buffered = content.readNBytes(Math.toIntExact(contentLength)); + if (buffered.length != contentLength) { + throw new IOException( + "Expected %d bytes but stream produced %d".formatted(contentLength, buffered.length)); + } + return RequestBody.fromBytes(buffered); + } catch (IOException e) { + throw new UncheckedIOException("Unable to buffer non-resetable blob payload", e); + } } @Override diff --git a/dhis-2/dhis-test-integration/pom.xml b/dhis-2/dhis-test-integration/pom.xml index 5cea0a0faf64..018eb64680fe 100644 --- a/dhis-2/dhis-test-integration/pom.xml +++ b/dhis-2/dhis-test-integration/pom.xml @@ -392,7 +392,21 @@ org.hisp.dhis:dhis-support-jdbc org.hisp.dhis:dhis-support-system org.hisp.dhis:json-tree + + software.amazon.awssdk:s3 + software.amazon.awssdk:sdk-core + software.amazon.awssdk:aws-core + software.amazon.awssdk:auth + software.amazon.awssdk:regions + + software.amazon.awssdk:s3 + software.amazon.awssdk:sdk-core + software.amazon.awssdk:aws-core + software.amazon.awssdk:auth + software.amazon.awssdk:regions + org.hisp.dhis:dhis-service-setting diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreNonResetableStreamTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreNonResetableStreamTest.java new file mode 100644 index 000000000000..13f726b8bf4f --- /dev/null +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/S3BlobStoreNonResetableStreamTest.java @@ -0,0 +1,191 @@ +/* + * Copyright (c) 2004-2026, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.storage; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.ByteArrayInputStream; +import java.io.FilterInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.net.URI; +import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import org.hisp.dhis.test.junit.MinIOTestExtension; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; +import org.junit.jupiter.api.extension.ExtendWith; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.core.exception.RetryableException; +import software.amazon.awssdk.core.interceptor.Context; +import software.amazon.awssdk.core.interceptor.ExecutionAttributes; +import software.amazon.awssdk.core.interceptor.ExecutionInterceptor; +import software.amazon.awssdk.core.interceptor.SdkExecutionAttribute; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.S3Configuration; +import software.amazon.awssdk.services.s3.presigner.S3Presigner; + +/** + * {@link software.amazon.awssdk.http.auth.aws.internal.signer.AwsChunkedV4PayloadSigner#sign} (line + * 91 in SDK 2.44.4) calls {@code payload.newStream()} exactly once per invocation. The whole + * signing pipeline runs inside the SDK's {@code RetryableStage} — when a request is retried, the + * pipeline re-runs and {@code sign()} is called again. The second call asks the wrapper from {@code + * RequestBody.fromInputStream} for another stream; because the underlying {@link InputStream} + * doesn't support {@code mark/reset} (e.g. {@code ZipFile.getInputStream(zipEntry)} used by app + * install), the wrapper throws IllegalStateException("...mark/reset..."). + * + *

    Plain MinIO doesn't expose the bug because no retry happens. This test injects an {@link + * ExecutionInterceptor} that throws {@link RetryableException} on the first {@code PutObject} + * attempt, forcing the SDK to retry exactly like real AWS S3 does when the first attempt hits a + * transient 5xx / socket error. + */ +@Tag("integration") +@ExtendWith(MinIOTestExtension.class) +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +class S3BlobStoreNonResetableStreamTest { + + private static final int PAYLOAD_BYTES = 512 * 1024; + + private S3BlobStoreService store; + private FailFirstPutObjectInterceptor failFirstPutObject; + + @BeforeAll + void start() { + failFirstPutObject = new FailFirstPutObjectInterceptor(); + + StaticCredentialsProvider credentials = + StaticCredentialsProvider.create( + AwsBasicCredentials.create( + MinIOTestExtension.MINIO_USER, MinIOTestExtension.MINIO_PASSWORD)); + S3Configuration s3Config = S3Configuration.builder().pathStyleAccessEnabled(true).build(); + URI endpoint = URI.create(MinIOTestExtension.s3Url()); + + S3Client s3 = + S3Client.builder() + .region(Region.US_EAST_1) + .endpointOverride(endpoint) + .credentialsProvider(credentials) + .serviceConfiguration(s3Config) + .overrideConfiguration(o -> o.addExecutionInterceptor(failFirstPutObject)) + .build(); + S3Presigner presigner = + S3Presigner.builder() + .region(Region.US_EAST_1) + .endpointOverride(endpoint) + .credentialsProvider(credentials) + .serviceConfiguration(s3Config) + .build(); + + store = new S3BlobStoreService(new BlobContainerName("repro"), s3, presigner); + store.init(); + // Arm only after init() so the bucket-create call isn't the one we force-retry. + failFirstPutObject.armed.set(true); + } + + @AfterAll + void stop() { + if (store != null) store.cleanUp(); + } + + @Test + void putBlob_nonResetableStream_survivesRetry() throws IOException { + byte[] payload = new byte[PAYLOAD_BYTES]; + ThreadLocalRandom.current().nextBytes(payload); + InputStream nonResetable = nonResetableWrapper(payload); + + BlobKey key = BlobKey.of("forced-retry-test.bin"); + try { + store.putBlob(key, nonResetable, payload.length, "application/octet-stream", null, null); + + assertTrue( + failFirstPutObject.attempts.get() >= 2, + "the SDK must have retried at least once — otherwise this test isn't actually" + + " exercising the retry path it's meant to cover. attempts=" + + failFirstPutObject.attempts.get()); + assertTrue(store.blobExists(key), "blob must exist after a successful retry"); + assertEquals(payload.length, store.contentLength(key)); + try (InputStream in = store.openStream(key)) { + assertNotNull(in); + assertArrayEquals(payload, in.readAllBytes(), "uploaded bytes must round-trip intact"); + } + } finally { + // clean up + if (store.blobExists(key)) store.deleteBlob(key); + } + } + + private static InputStream nonResetableWrapper(byte[] payload) { + return new FilterInputStream(new ByteArrayInputStream(payload)) { + @Override + public boolean markSupported() { + return false; + } + + @Override + public synchronized void mark(int readlimit) { + // no-op + } + + @Override + public synchronized void reset() throws IOException { + throw new IOException("mark/reset not supported"); + } + }; + } + + /** + * Throws a {@link RetryableException} from {@code afterTransmission} on the first {@code + * PutObject} call, then disarms. Mimics the transient-error path that production hits against + * real AWS S3 (5xx / socket reset / etc.) and forces the SDK's {@code RetryableStage} to + * re-execute the signing pipeline. + */ + private static final class FailFirstPutObjectInterceptor implements ExecutionInterceptor { + final AtomicInteger attempts = new AtomicInteger(); + final AtomicBoolean armed = new AtomicBoolean(false); + + @Override + public void afterTransmission(Context.AfterTransmission context, ExecutionAttributes attrs) { + if (!armed.get()) return; + if (!"PutObject".equals(attrs.getAttribute(SdkExecutionAttribute.OPERATION_NAME))) return; + if (attempts.incrementAndGet() == 1) { + throw RetryableException.create("forced retry for non-resetable stream reproduction"); + } + } + } +} From 476755014298bb4561e6894efe636fe44b31ac3a Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Thu, 28 May 2026 09:19:10 +0100 Subject: [PATCH 15/16] resolve conflicts --- .../src/main/java/org/hisp/dhis/jclouds/JCloudsStore.java | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jclouds/JCloudsStore.java diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jclouds/JCloudsStore.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jclouds/JCloudsStore.java deleted file mode 100644 index e69de29bb2d1..000000000000 From 4cd222d75d96882c493c2f8e7604be2f46f661a7 Mon Sep 17 00:00:00 2001 From: David Mackessy Date: Thu, 28 May 2026 10:08:11 +0100 Subject: [PATCH 16/16] remove test util method --- .../org/hisp/dhis/storage/BlobStoreServiceContractTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java index cbd00d90d388..44e1d1efe494 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/storage/BlobStoreServiceContractTest.java @@ -334,7 +334,7 @@ void listKeys_paginatesPastDefaultPageSize() { assertEquals( total, - withoutDirectoryMarkers(keys).size(), + keys.size(), "listKeys must return every blob across all pages, not just the first page"); }