From f5161c580fbf26195b555d3fcc2a340e15e8b944 Mon Sep 17 00:00:00 2001 From: Jan Bernitt Date: Tue, 21 Nov 2023 14:02:40 +0100 Subject: [PATCH] feat: save file resource content synchronously for jobs [DHIS2-15276] (#15747) --- .../org/hisp/dhis/feedback/ErrorCode.java | 2 + .../fileresource/FileResourceService.java | 31 ++++++++++--- .../DefaultFileResourceService.java | 43 ++++++++----------- .../DefaultJobConfigurationService.java | 2 +- .../fileresource/FileResourceServiceTest.java | 12 +++--- .../DefaultPushAnalysisService.java | 2 +- .../tracker/TrackedEntityServiceTest.java | 2 +- .../FileResourceCleanUpJobTest.java | 8 ++-- .../java/org/hisp/dhis/icon/IconTest.java | 2 +- .../OrganisationUnitServiceTest.java | 2 +- ...rackedEntityAttributeValueServiceTest.java | 4 +- ...ntityProgramAttributeFileResourceTest.java | 2 +- .../validation/TeTaValidationTest.java | 4 +- .../dhis/webapi/utils/FileResourceUtils.java | 2 +- 14 files changed, 67 insertions(+), 51 deletions(-) diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/feedback/ErrorCode.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/feedback/ErrorCode.java index c14f451b9423..c745f629a9f3 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/feedback/ErrorCode.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/feedback/ErrorCode.java @@ -336,6 +336,8 @@ public enum ErrorCode { /* File resource */ E6100("Filename not present"), E6101("File type not allowed"), + E6102("File content could not be stored"), + E6103("File resource appears to have no content"), /* Users */ E6200("Feedback message recipients user group not defined"), diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/fileresource/FileResourceService.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/fileresource/FileResourceService.java index 92e43b63b40c..90f7915a2b2c 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/fileresource/FileResourceService.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/fileresource/FileResourceService.java @@ -32,7 +32,6 @@ import java.io.InputStream; import java.io.OutputStream; import java.net.URI; -import java.time.Duration; import java.util.List; import java.util.NoSuchElementException; import java.util.Optional; @@ -90,9 +89,31 @@ public interface FileResourceService { */ List findOwnersByStorageKey(@CheckForNull String storageKey); - void saveFileResource(FileResource fileResource, File file); + /** + * Creates the provided file resource and stores the file content asynchronously. + * + * @param fileResource the resource to create + * @param file the content stored asynchronously + */ + void asyncSaveFileResource(FileResource fileResource, File file); + + /** + * Creates the provided file resource and stores the content asynchronously. + * + * @param fileResource the resource to create + * @param bytes the content stored asynchronously + * @return the UID of the created file resource + */ + String asyncSaveFileResource(FileResource fileResource, byte[] bytes); - String saveFileResource(FileResource fileResource, byte[] bytes); + /** + * Creates the provided file resource and stores the content synchronously. + * + * @param fileResource the resource to create + * @param bytes the content stored asynchronously + * @return the UID of the created file resource + */ + String syncSaveFileResource(FileResource fileResource, byte[] bytes) throws ConflictException; void deleteFileResource(String uid); @@ -101,10 +122,6 @@ public interface FileResourceService { @Nonnull InputStream getFileResourceContent(FileResource fileResource) throws ConflictException; - @Nonnull - InputStream getFileResourceContent(FileResource fileResource, Duration timeout) - throws ConflictException; - /** Copy fileResource content to outputStream and Return File content length */ void copyFileResourceContent(FileResource fileResource, OutputStream outputStream) throws IOException, NoSuchElementException; 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 dac3f37812ff..ce5c5f61cd0e 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 @@ -27,10 +27,6 @@ */ package org.hisp.dhis.fileresource; -import static java.lang.System.currentTimeMillis; -import static java.time.Duration.ofMillis; -import static java.time.Duration.ofSeconds; - import java.io.File; import java.io.IOException; import java.io.InputStream; @@ -173,7 +169,7 @@ public List findOwnersByStorageKey(@CheckForNull String stora @Override @Transactional - public void saveFileResource(FileResource fileResource, File file) { + public void asyncSaveFileResource(FileResource fileResource, File file) { validateFileResource(fileResource); fileResource.setStorageStatus(FileResourceStorageStatus.PENDING); @@ -194,7 +190,7 @@ public void saveFileResource(FileResource fileResource, File file) { @Override @Transactional - public String saveFileResource(FileResource fileResource, byte[] bytes) { + public String asyncSaveFileResource(FileResource fileResource, byte[] bytes) { fileResource.setStorageStatus(FileResourceStorageStatus.PENDING); fileResourceStore.save(fileResource); entityManager.flush(); @@ -206,6 +202,22 @@ public String saveFileResource(FileResource fileResource, byte[] bytes) { return uid; } + @Override + @Transactional + public String syncSaveFileResource(FileResource fileResource, byte[] bytes) + throws ConflictException { + fileResource.setStorageStatus(FileResourceStorageStatus.PENDING); + fileResourceStore.save(fileResource); + entityManager.flush(); + + final String uid = fileResource.getUid(); + + String storageId = fileResourceContentStore.saveFileResourceContent(fileResource, bytes); + if (storageId == null) throw new ConflictException(ErrorCode.E6102); + + return uid; + } + @Override @Transactional public void deleteFileResource(String uid) { @@ -245,26 +257,9 @@ public void deleteFileResource(FileResource fileResource) { @Override @Nonnull public InputStream getFileResourceContent(FileResource fileResource) throws ConflictException { - return getFileResourceContent(fileResource, ofSeconds(10)); - } - - @Nonnull - @Override - public InputStream getFileResourceContent(FileResource fileResource, java.time.Duration timeout) - throws ConflictException { String key = fileResource.getStorageKey(); InputStream content = fileResourceContentStore.getFileResourceContent(key); - long since = currentTimeMillis(); - while (content == null && !timeout.minus(ofMillis(currentTimeMillis() - since)).isNegative()) { - try { - Thread.sleep(50); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } - content = fileResourceContentStore.getFileResourceContent(key); - } - if (content == null) - throw new ConflictException("File resource exists but content input stream was null"); + if (content == null) throw new ConflictException(ErrorCode.E6103); return content; } diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/DefaultJobConfigurationService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/DefaultJobConfigurationService.java index 25ce6a73e476..8f46b2a4eb4d 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/DefaultJobConfigurationService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/DefaultJobConfigurationService.java @@ -129,7 +129,7 @@ private void saveJobData(String uid, MimeType contentType, InputStream content) FileResourceDomain.JOB_DATA); fr.setUid(uid); fr.setAssigned(true); - fileResourceService.saveFileResource(fr, data); + fileResourceService.syncSaveFileResource(fr, data); } catch (IOException ex) { throw new ConflictException("Failed to create job data file resource: " + ex.getMessage()); } diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/fileresource/FileResourceServiceTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/fileresource/FileResourceServiceTest.java index ddb65176e77d..f211fdbdac70 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/fileresource/FileResourceServiceTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/fileresource/FileResourceServiceTest.java @@ -102,7 +102,7 @@ void verifySaveFile() { File file = new File(""); - subject.saveFileResource(fileResource, file); + subject.asyncSaveFileResource(fileResource, file); verify(fileResourceStore).save(fileResource); verify(entityManager).flush(); @@ -122,7 +122,8 @@ void verifySaveIllegalFileTypeResourceA() { "very_evil_script.html", "text/html", 1024, "md5", FileResourceDomain.USER_AVATAR); File file = new File("very_evil_script.html"); - assertThrows(IllegalQueryException.class, () -> subject.saveFileResource(fileResource, file)); + assertThrows( + IllegalQueryException.class, () -> subject.asyncSaveFileResource(fileResource, file)); } @Test @@ -136,7 +137,8 @@ void verifySaveIllegalFileTypeResourceB() { FileResourceDomain.MESSAGE_ATTACHMENT); File file = new File("suspicious_program.rpm"); - assertThrows(IllegalQueryException.class, () -> subject.saveFileResource(fileResource, file)); + assertThrows( + IllegalQueryException.class, () -> subject.asyncSaveFileResource(fileResource, file)); } @Test @@ -157,7 +159,7 @@ void verifySaveImageFile() { fileResource.setUid("imageUid1"); - subject.saveFileResource(fileResource, file); + subject.asyncSaveFileResource(fileResource, file); verify(fileResourceStore).save(fileResource); verify(entityManager).flush(); @@ -211,7 +213,7 @@ void verifySaveOrgUnitImageFile() { fileResource.setUid("imageUid1"); - subject.saveFileResource(fileResource, file); + subject.asyncSaveFileResource(fileResource, file); verify(fileResourceStore).save(fileResource); verify(entityManager).flush(); diff --git a/dhis-2/dhis-services/dhis-service-reporting/src/main/java/org/hisp/dhis/pushanalysis/DefaultPushAnalysisService.java b/dhis-2/dhis-services/dhis-service-reporting/src/main/java/org/hisp/dhis/pushanalysis/DefaultPushAnalysisService.java index e2b1d7912270..36ca62b4539a 100644 --- a/dhis-2/dhis-services/dhis-service-reporting/src/main/java/org/hisp/dhis/pushanalysis/DefaultPushAnalysisService.java +++ b/dhis-2/dhis-services/dhis-service-reporting/src/main/java/org/hisp/dhis/pushanalysis/DefaultPushAnalysisService.java @@ -441,7 +441,7 @@ private String saveFileResource(FileResource fileResource, byte[] bytes) { fileResource.setAssigned(true); - String fileResourceUid = fileResourceService.saveFileResource(fileResource, bytes); + String fileResourceUid = fileResourceService.asyncSaveFileResource(fileResource, bytes); externalFileResource.setFileResource(fileResourceService.getFileResource(fileResourceUid)); diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/dxf2/deprecated/tracker/TrackedEntityServiceTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/dxf2/deprecated/tracker/TrackedEntityServiceTest.java index c51ab031fd3f..87d375c7c42b 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/dxf2/deprecated/tracker/TrackedEntityServiceTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/dxf2/deprecated/tracker/TrackedEntityServiceTest.java @@ -162,7 +162,7 @@ class TrackedEntityServiceTest extends TransactionalIntegrationTest { protected void setUpTest() throws Exception { fileResource = createFileResource('F', "fileResource".getBytes()); - fileResourceService.saveFileResource(fileResource, "fileResource".getBytes()); + fileResourceService.asyncSaveFileResource(fileResource, "fileResource".getBytes()); userService = _userService; user = createAndAddAdminUser(AUTHORITY_ALL); diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/fileresource/FileResourceCleanUpJobTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/fileresource/FileResourceCleanUpJobTest.java index ae35abac4c22..207a2a506760 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/fileresource/FileResourceCleanUpJobTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/fileresource/FileResourceCleanUpJobTest.java @@ -182,12 +182,12 @@ void testOrphan() { content = "filecontentA".getBytes(StandardCharsets.UTF_8); FileResource fileResourceA = createFileResource('A', content); fileResourceA.setCreated(DateTime.now().minus(Days.ONE).toDate()); - String uidA = fileResourceService.saveFileResource(fileResourceA, content); + String uidA = fileResourceService.asyncSaveFileResource(fileResourceA, content); content = "filecontentB".getBytes(StandardCharsets.UTF_8); FileResource fileResourceB = createFileResource('A', content); fileResourceB.setCreated(DateTime.now().minus(Days.ONE).toDate()); - String uidB = fileResourceService.saveFileResource(fileResourceB, content); + String uidB = fileResourceService.asyncSaveFileResource(fileResourceB, content); User userB = makeUser("B"); userB.setAvatar(fileResourceB); @@ -259,7 +259,7 @@ private DataValue createFileResourceDataValue(char uniqueChar, byte[] content) { organisationUnitService.addOrganisationUnit(orgUnit); FileResource fileResource = createFileResource(uniqueChar, content); - String uid = fileResourceService.saveFileResource(fileResource, content); + String uid = fileResourceService.asyncSaveFileResource(fileResource, content); DataValue dataValue = createDataValue(fileElement, period, orgUnit, uid, null); fileResource.setAssigned(true); @@ -275,7 +275,7 @@ private DataValue createFileResourceDataValue(char uniqueChar, byte[] content) { private ExternalFileResource createExternal(char uniqueChar, byte[] content) { ExternalFileResource externalFileResource = createExternalFileResource(uniqueChar, content); - fileResourceService.saveFileResource(externalFileResource.getFileResource(), content); + fileResourceService.asyncSaveFileResource(externalFileResource.getFileResource(), content); externalFileResourceService.saveExternalFileResource(externalFileResource); FileResource fileResource = externalFileResource.getFileResource(); diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/icon/IconTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/icon/IconTest.java index 02433f00e707..b6c00dd795bd 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/icon/IconTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/icon/IconTest.java @@ -226,7 +226,7 @@ public FileResource createAndPersistFileResource(char uniqueChar) { fileResource.setCreated(new Date()); fileResource.setAutoFields(); - String fileResourceUid = fileResourceService.saveFileResource(fileResource, content); + String fileResourceUid = fileResourceService.asyncSaveFileResource(fileResource, content); return fileResourceService.getFileResource(fileResourceUid); } diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/organisationunit/OrganisationUnitServiceTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/organisationunit/OrganisationUnitServiceTest.java index 48ee0f19ee43..10b3ac47edba 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/organisationunit/OrganisationUnitServiceTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/organisationunit/OrganisationUnitServiceTest.java @@ -1020,7 +1020,7 @@ void testSaveImage() { fileResource.setAssigned(false); fileResource.setCreated(new Date()); fileResource.setAutoFields(); - fileResourceService.saveFileResource(fileResource, content); + fileResourceService.asyncSaveFileResource(fileResource, content); OrganisationUnit orgUnit = createOrganisationUnit('A'); orgUnit.setImage(fileResource); organisationUnitService.addOrganisationUnit(orgUnit); diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/trackedentityattributevalue/TrackedEntityAttributeValueServiceTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/trackedentityattributevalue/TrackedEntityAttributeValueServiceTest.java index a3505e52fd6d..707b92765f73 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/trackedentityattributevalue/TrackedEntityAttributeValueServiceTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/trackedentityattributevalue/TrackedEntityAttributeValueServiceTest.java @@ -198,10 +198,10 @@ void testFileAttributeValues() { content = "filecontentA".getBytes(); fileResourceA = createFileResource('A', content); fileResourceA.setContentType("image/jpg"); - fileResourceService.saveFileResource(fileResourceA, content); + fileResourceService.asyncSaveFileResource(fileResourceA, content); content = "filecontentB".getBytes(); fileResourceB = createFileResource('B', content); - fileResourceService.saveFileResource(fileResourceB, content); + fileResourceService.asyncSaveFileResource(fileResourceB, content); attributeValueA = createTrackedEntityAttributeValue('A', entityInstanceA, attributeA); attributeValueB = createTrackedEntityAttributeValue('B', entityInstanceB, attributeB); attributeValueA.setValue(fileResourceA.getUid()); diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/imports/bundle/TrackedEntityProgramAttributeFileResourceTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/imports/bundle/TrackedEntityProgramAttributeFileResourceTest.java index 5338a1ddaee6..efda3371695b 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/imports/bundle/TrackedEntityProgramAttributeFileResourceTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/imports/bundle/TrackedEntityProgramAttributeFileResourceTest.java @@ -79,7 +79,7 @@ void testTrackedEntityProgramAttributeFileResourceValue() throws IOException { FileResourceDomain.DOCUMENT); fileResource.setUid("Jzf6hHNP7jx"); File file = File.createTempFile("file-resource", "test"); - fileResourceService.saveFileResource(fileResource, file); + fileResourceService.asyncSaveFileResource(fileResource, file); assertFalse(fileResource.isAssigned()); ImportReport importReport = trackerImportService.importTracker( diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/imports/validation/TeTaValidationTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/imports/validation/TeTaValidationTest.java index f13c725ba1a7..171ceb52e277 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/imports/validation/TeTaValidationTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/imports/validation/TeTaValidationTest.java @@ -79,7 +79,7 @@ void testTrackedEntityProgramAttributeFileResourceValue() throws IOException { FileResourceDomain.DOCUMENT); fileResource.setUid("Jzf6hHNP7jx"); File file = File.createTempFile("file-resource", "test"); - fileResourceService.saveFileResource(fileResource, file); + fileResourceService.asyncSaveFileResource(fileResource, file); assertFalse(fileResource.isAssigned()); TrackerObjects trackerObjects = fromJson("tracker/validations/te-program_with_tea_fileresource_data.json"); @@ -105,7 +105,7 @@ void testFileAlreadyAssign() throws IOException { FileResourceDomain.DOCUMENT); fileResource.setUid("Jzf6hHNP7jx"); File file = File.createTempFile("file-resource", "test"); - fileResourceService.saveFileResource(fileResource, file); + fileResourceService.asyncSaveFileResource(fileResource, file); assertFalse(fileResource.isAssigned()); TrackerObjects trackerObjects = fromJson("tracker/validations/te-program_with_tea_fileresource_data.json"); diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/utils/FileResourceUtils.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/utils/FileResourceUtils.java index 7f55a8707517..91e03eafc38b 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/utils/FileResourceUtils.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/utils/FileResourceUtils.java @@ -210,7 +210,7 @@ public FileResource saveFileResource(String uid, MultipartFile file, FileResourc throw new WebMessageException( conflict(ErrorCode.E1119, FileResource.class.getSimpleName(), uid)); } - fileResourceService.saveFileResource(fileResource, tmpFile); + fileResourceService.asyncSaveFileResource(fileResource, tmpFile); return fileResource; }