From a6c9fcefed6900a9746778e79c76799b4147b587 Mon Sep 17 00:00:00 2001 From: khartmann Date: Wed, 4 Dec 2019 18:41:07 -0500 Subject: [PATCH 1/3] Enhancement: Remove auto unpublish --- .../service/FileModificationService.java | 32 ++------ .../service/FileModificationServiceTest.java | 76 ++++--------------- .../song/server/service/FileServiceTest.java | 37 +++++---- 3 files changed, 39 insertions(+), 106 deletions(-) diff --git a/song-server/src/main/java/bio/overture/song/server/service/FileModificationService.java b/song-server/src/main/java/bio/overture/song/server/service/FileModificationService.java index 7d860c3a9..7c24d16c8 100644 --- a/song-server/src/main/java/bio/overture/song/server/service/FileModificationService.java +++ b/song-server/src/main/java/bio/overture/song/server/service/FileModificationService.java @@ -21,8 +21,6 @@ import static bio.overture.song.core.exceptions.ServerErrors.INVALID_FILE_UPDATE_REQUEST; import static bio.overture.song.core.exceptions.ServerException.buildServerException; import static bio.overture.song.core.exceptions.ServerException.checkServer; -import static bio.overture.song.core.model.enums.AnalysisStates.PUBLISHED; -import static bio.overture.song.core.model.enums.AnalysisStates.SUPPRESSED; import static bio.overture.song.core.model.enums.AnalysisStates.UNPUBLISHED; import static bio.overture.song.core.model.enums.FileUpdateTypes.CONTENT_UPDATE; import static bio.overture.song.core.model.enums.FileUpdateTypes.METADATA_UPDATE; @@ -93,46 +91,26 @@ public FileUpdateResponse securedFileWithAnalysisUpdate( // analysis val currentState = analysisService.readState(analysisId); checkServer( - currentState != SUPPRESSED, + currentState == UNPUBLISHED, getClass(), ILLEGAL_FILE_UPDATE_REQUEST, "The file with objectId '%s' and analysisId '%s' cannot " + "be updated since its analysisState is '%s'", objectId, analysisId, - SUPPRESSED.toString()); + currentState.toString()); // Update the target file record using the originalFile and the update request val fileUpdateType = updateWithRequest(originalFile, fileUpdateRequest); // Build the response - val response = FileUpdateResponse.builder().unpublishedAnalysis(false); + val response = FileUpdateResponse.builder().unpublishedAnalysis(true); response.originalFile(fileConverter.convertToFileDTO(originalFile)); response.originalAnalysisState(currentState); response.fileUpdateType(fileUpdateType); + response.message( + format("Updated file with objectId '%s' and analysisId '%s'", objectId, analysisId)); - // Can only transition from PUBLISHED to UNPUBLISHED states. - if (currentState == PUBLISHED) { - if (doUnpublish(fileUpdateType)) { - analysisService.securedUpdateState(studyId, analysisId, UNPUBLISHED); - response.unpublishedAnalysis(true); - response.message( - format( - "[WARNING]: Changed analysis from '%s' to '%s'", - PUBLISHED.toString(), UNPUBLISHED.toString())); - } else { - response.message( - format( - "Original analysisState '%s' was not changed since the fileUpdateType was '%s'", - currentState.toString(), fileUpdateType.name())); - } - } else if (currentState == UNPUBLISHED) { // Can still update an unpublished analysis - response.message( - format("Did not change analysisState since it is '%s'", currentState.toString())); - } else { - throw new IllegalStateException( - format("Could not process the analysisState '%s'", currentState.toString())); - } return response.build(); } diff --git a/song-server/src/test/java/bio/overture/song/server/service/FileModificationServiceTest.java b/song-server/src/test/java/bio/overture/song/server/service/FileModificationServiceTest.java index 5e7d6cfd2..bf40a7655 100644 --- a/song-server/src/test/java/bio/overture/song/server/service/FileModificationServiceTest.java +++ b/song-server/src/test/java/bio/overture/song/server/service/FileModificationServiceTest.java @@ -35,11 +35,9 @@ import static bio.overture.song.server.utils.securestudy.impl.SecureFileTester.createSecureFileTester; import static com.google.common.collect.Lists.newArrayList; import static org.icgc.dcc.common.core.json.JsonNodeBuilders.object; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.*; +import bio.overture.song.core.exceptions.ServerException; import bio.overture.song.core.model.FileUpdateRequest; import bio.overture.song.core.model.enums.AccessTypes; import bio.overture.song.core.testing.SongErrorAssertions; @@ -133,55 +131,18 @@ public void testFileUpdateWithPublishedAnalysis() { // No Change val noChangeRequest = new FileUpdateRequest(); - val noChangeResponse = - fileModificationService.securedFileWithAnalysisUpdate( - DEFAULT_STUDY_ID, DEFAULT_FILE_ID, noChangeRequest); - assertFalse(noChangeResponse.isUnpublishedAnalysis()); - assertEquals(noChangeResponse.getFileUpdateType(), NO_UPDATE); - assertEquals(noChangeResponse.getOriginalAnalysisState(), PUBLISHED); - assertEquals(noChangeResponse.getOriginalFile(), originalFile); - assertEquals( - noChangeResponse.getMessage(), - "Original analysisState 'PUBLISHED' was not changed since the fileUpdateType was 'NO_UPDATE'"); - - // Metadata Update - val metadataUpdateRequest = - FileUpdateRequest.builder() - .info( - object() - .with( - randomGenerator.generateRandomUUIDAsString(), - randomGenerator.generateRandomUUIDAsString()) - .end()) - .build(); - val originalFile2 = - fileConverter.convertToFileDTO(fileService.securedRead(DEFAULT_STUDY_ID, DEFAULT_FILE_ID)); - val metadataUpdateResponse = - fileModificationService.securedFileWithAnalysisUpdate( - DEFAULT_STUDY_ID, DEFAULT_FILE_ID, metadataUpdateRequest); - assertFalse(metadataUpdateResponse.isUnpublishedAnalysis()); - assertEquals(metadataUpdateResponse.getFileUpdateType(), METADATA_UPDATE); - assertEquals(metadataUpdateResponse.getOriginalAnalysisState(), PUBLISHED); - assertEquals(metadataUpdateResponse.getOriginalFile(), originalFile2); - assertEquals( - metadataUpdateResponse.getMessage(), - "Original analysisState 'PUBLISHED' was not changed since the fileUpdateType was 'METADATA_UPDATE'"); - - // Content Update - val contentUpdateRequest = - FileUpdateRequest.builder().fileSize(originalFile2.getFileSize() + 77771L).build(); - val originalFile3 = - fileConverter.convertToFileDTO(fileService.securedRead(DEFAULT_STUDY_ID, DEFAULT_FILE_ID)); - val contentUpdateResponse = - fileModificationService.securedFileWithAnalysisUpdate( - DEFAULT_STUDY_ID, DEFAULT_FILE_ID, contentUpdateRequest); - assertTrue(contentUpdateResponse.isUnpublishedAnalysis()); - assertEquals(contentUpdateResponse.getFileUpdateType(), CONTENT_UPDATE); - assertEquals(contentUpdateResponse.getOriginalAnalysisState(), PUBLISHED); - assertEquals(contentUpdateResponse.getOriginalFile(), originalFile3); + Exception exception = null; + try { + fileModificationService.securedFileWithAnalysisUpdate( + DEFAULT_STUDY_ID, DEFAULT_FILE_ID, noChangeRequest); + } catch (ServerException ex) { + exception = ex; + } + assertNotNull(exception); assertEquals( - contentUpdateResponse.getMessage(), - "[WARNING]: Changed analysis from 'PUBLISHED' to 'UNPUBLISHED'"); + "[FileModificationService::illegal.file.update.request] - The file with objectId " + + "'FI1' and analysisId 'AN1' cannot be updated since its analysisState is 'PUBLISHED'", + exception.getMessage()); } @Test @@ -197,11 +158,10 @@ public void testFileUpdateWithUnpublishedAnalysis() { val noChangeResponse = fileModificationService.securedFileWithAnalysisUpdate( DEFAULT_STUDY_ID, DEFAULT_FILE_ID, noChangeRequest); - assertFalse(noChangeResponse.isUnpublishedAnalysis()); + assertTrue(noChangeResponse.isUnpublishedAnalysis()); assertEquals(noChangeResponse.getFileUpdateType(), NO_UPDATE); assertEquals(noChangeResponse.getOriginalAnalysisState(), UNPUBLISHED); assertEquals(noChangeResponse.getOriginalFile(), originalFile); - assertTrue(noChangeResponse.getMessage().contains("Did not change analysisState since it is")); // Metadata Update val metadataUpdateRequest = @@ -218,12 +178,10 @@ public void testFileUpdateWithUnpublishedAnalysis() { val metadataUpdateResponse = fileModificationService.securedFileWithAnalysisUpdate( DEFAULT_STUDY_ID, DEFAULT_FILE_ID, metadataUpdateRequest); - assertFalse(metadataUpdateResponse.isUnpublishedAnalysis()); + assertTrue(metadataUpdateResponse.isUnpublishedAnalysis()); assertEquals(metadataUpdateResponse.getFileUpdateType(), METADATA_UPDATE); assertEquals(metadataUpdateResponse.getOriginalAnalysisState(), UNPUBLISHED); assertEquals(metadataUpdateResponse.getOriginalFile(), originalFile2); - assertTrue( - metadataUpdateResponse.getMessage().contains("Did not change analysisState since it is")); // Content Update val contentUpdateRequest = @@ -233,12 +191,10 @@ public void testFileUpdateWithUnpublishedAnalysis() { val contentUpdateResponse = fileModificationService.securedFileWithAnalysisUpdate( DEFAULT_STUDY_ID, DEFAULT_FILE_ID, contentUpdateRequest); - assertFalse(contentUpdateResponse.isUnpublishedAnalysis()); + assertTrue(contentUpdateResponse.isUnpublishedAnalysis()); assertEquals(contentUpdateResponse.getFileUpdateType(), CONTENT_UPDATE); assertEquals(contentUpdateResponse.getOriginalAnalysisState(), UNPUBLISHED); assertEquals(contentUpdateResponse.getOriginalFile(), originalFile3); - assertTrue( - contentUpdateResponse.getMessage().contains("Did not change analysisState since it is")); } @Test diff --git a/song-server/src/test/java/bio/overture/song/server/service/FileServiceTest.java b/song-server/src/test/java/bio/overture/song/server/service/FileServiceTest.java index f1cc58143..3fceb3348 100644 --- a/song-server/src/test/java/bio/overture/song/server/service/FileServiceTest.java +++ b/song-server/src/test/java/bio/overture/song/server/service/FileServiceTest.java @@ -16,10 +16,28 @@ */ package bio.overture.song.server.service; +import static bio.overture.song.core.exceptions.ServerErrors.FILE_NOT_FOUND; +import static bio.overture.song.core.exceptions.ServerErrors.STUDY_ID_DOES_NOT_EXIST; +import static bio.overture.song.core.model.enums.AccessTypes.CONTROLLED; +import static bio.overture.song.core.model.enums.AccessTypes.OPEN; +import static bio.overture.song.core.model.enums.FileTypes.FAI; +import static bio.overture.song.core.testing.SongErrorAssertions.assertSongError; +import static bio.overture.song.core.utils.RandomGenerator.createRandomGenerator; +import static bio.overture.song.server.utils.TestConstants.DEFAULT_ANALYSIS_ID; +import static bio.overture.song.server.utils.TestConstants.DEFAULT_FILE_ID; +import static bio.overture.song.server.utils.TestConstants.DEFAULT_STUDY_ID; +import static bio.overture.song.server.utils.securestudy.impl.SecureFileTester.createSecureFileTester; +import static com.google.common.collect.Lists.newArrayList; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; + import bio.overture.song.core.testing.SongErrorAssertions; import bio.overture.song.core.utils.JsonUtils; import bio.overture.song.core.utils.RandomGenerator; import bio.overture.song.server.model.entity.FileEntity; +import javax.transaction.Transactional; import lombok.extern.slf4j.Slf4j; import lombok.val; import org.junit.Before; @@ -30,25 +48,6 @@ import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.junit4.SpringRunner; -import javax.transaction.Transactional; - -import static com.google.common.collect.Lists.newArrayList; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotEquals; -import static org.junit.Assert.assertTrue; -import static bio.overture.song.core.exceptions.ServerErrors.FILE_NOT_FOUND; -import static bio.overture.song.core.exceptions.ServerErrors.STUDY_ID_DOES_NOT_EXIST; -import static bio.overture.song.core.model.enums.AccessTypes.CONTROLLED; -import static bio.overture.song.core.model.enums.AccessTypes.OPEN; -import static bio.overture.song.core.model.enums.FileTypes.FAI; -import static bio.overture.song.core.testing.SongErrorAssertions.assertSongError; -import static bio.overture.song.core.utils.RandomGenerator.createRandomGenerator; -import static bio.overture.song.server.utils.TestConstants.DEFAULT_ANALYSIS_ID; -import static bio.overture.song.server.utils.TestConstants.DEFAULT_FILE_ID; -import static bio.overture.song.server.utils.TestConstants.DEFAULT_STUDY_ID; -import static bio.overture.song.server.utils.securestudy.impl.SecureFileTester.createSecureFileTester; - @Slf4j @SpringBootTest @RunWith(SpringRunner.class) From 396e89361ce77a67f41a5683cbf0f37558a6a855 Mon Sep 17 00:00:00 2001 From: khartmann Date: Fri, 6 Dec 2019 14:59:27 -0500 Subject: [PATCH 2/3] Code Cleanup: Deprecated fields as per PR. Bugfix: Now checks the content type of the request rather than just UNPUBLISHED status. Code Cleanup: Fixed order of expected vs actual parameters to assertEquals() tests in FileModificationServiceTest. --- .../song/core/model/FileUpdateResponse.java | 4 +- .../service/FileModificationService.java | 64 ++++--- .../service/FileModificationServiceTest.java | 162 ++++++++++-------- 3 files changed, 138 insertions(+), 92 deletions(-) diff --git a/song-core/src/main/java/bio/overture/song/core/model/FileUpdateResponse.java b/song-core/src/main/java/bio/overture/song/core/model/FileUpdateResponse.java index 098224428..f2177e2b9 100644 --- a/song-core/src/main/java/bio/overture/song/core/model/FileUpdateResponse.java +++ b/song-core/src/main/java/bio/overture/song/core/model/FileUpdateResponse.java @@ -30,8 +30,8 @@ @AllArgsConstructor public class FileUpdateResponse { private FileUpdateTypes fileUpdateType; - private AnalysisStates originalAnalysisState; - private boolean unpublishedAnalysis; + @Deprecated private AnalysisStates originalAnalysisState; + @Deprecated private boolean unpublishedAnalysis; private String message; private FileDTO originalFile; } diff --git a/song-server/src/main/java/bio/overture/song/server/service/FileModificationService.java b/song-server/src/main/java/bio/overture/song/server/service/FileModificationService.java index 7c24d16c8..991eb0de0 100644 --- a/song-server/src/main/java/bio/overture/song/server/service/FileModificationService.java +++ b/song-server/src/main/java/bio/overture/song/server/service/FileModificationService.java @@ -21,6 +21,7 @@ import static bio.overture.song.core.exceptions.ServerErrors.INVALID_FILE_UPDATE_REQUEST; import static bio.overture.song.core.exceptions.ServerException.buildServerException; import static bio.overture.song.core.exceptions.ServerException.checkServer; +import static bio.overture.song.core.model.enums.AnalysisStates.SUPPRESSED; import static bio.overture.song.core.model.enums.AnalysisStates.UNPUBLISHED; import static bio.overture.song.core.model.enums.FileUpdateTypes.CONTENT_UPDATE; import static bio.overture.song.core.model.enums.FileUpdateTypes.METADATA_UPDATE; @@ -62,9 +63,33 @@ public FileModificationService( @Transactional public FileUpdateTypes updateWithRequest( @NonNull FileEntity originalFile, FileData fileUpdateRequest) { - val updatedFile = createUpdateFile(originalFile, fileUpdateRequest); - fileService.unsafeUpdate(updatedFile); - return resolveFileUpdateType(originalFile, fileUpdateRequest); + val analysisId = originalFile.getAnalysisId(); + val objectId = originalFile.getObjectId(); + val currentState = analysisService.readState(analysisId); + val fileUpdateType = resolveFileUpdateType(originalFile, fileUpdateRequest); + + boolean doUpdate = false; + if (fileUpdateType == METADATA_UPDATE) { + doUpdate = true; + } else if (fileUpdateType == CONTENT_UPDATE) { + checkServer( + currentState == UNPUBLISHED, + getClass(), + ILLEGAL_FILE_UPDATE_REQUEST, + "The file with objectId '%s' and analysisId '%s' cannot be updated since its analysisState is '%s' and '%s' is required", + objectId, + analysisId, + currentState, + UNPUBLISHED); + doUpdate = true; + } + + if (doUpdate) { + val updatedFile = createUpdateFile(originalFile, fileUpdateRequest); + fileService.unsafeUpdate(updatedFile); + } + + return fileUpdateType; } /** @@ -91,7 +116,7 @@ public FileUpdateResponse securedFileWithAnalysisUpdate( // analysis val currentState = analysisService.readState(analysisId); checkServer( - currentState == UNPUBLISHED, + currentState != SUPPRESSED, getClass(), ILLEGAL_FILE_UPDATE_REQUEST, "The file with objectId '%s' and analysisId '%s' cannot " @@ -102,9 +127,19 @@ public FileUpdateResponse securedFileWithAnalysisUpdate( // Update the target file record using the originalFile and the update request val fileUpdateType = updateWithRequest(originalFile, fileUpdateRequest); - + val response = FileUpdateResponse.builder().unpublishedAnalysis(false); + if (fileUpdateType == METADATA_UPDATE || fileUpdateType == CONTENT_UPDATE) { + response.message( + format("Updated file with objectId '%s' and analysisId '%s'", objectId, analysisId)); + } else if (fileUpdateType == NO_UPDATE) { + response.message( + format( + "No update for file with objectId '%s' and analysisId '%s'", objectId, analysisId)); + } else { + throw new IllegalStateException("Could not process fileUpdateType: " + fileUpdateType); + } // Build the response - val response = FileUpdateResponse.builder().unpublishedAnalysis(true); + response.originalFile(fileConverter.convertToFileDTO(originalFile)); response.originalAnalysisState(currentState); response.fileUpdateType(fileUpdateType); @@ -137,21 +172,4 @@ private FileEntity createUpdateFile( fileConverter.updateEntityFromData(fileUpdateData, updatedFile); return updatedFile; } - - /** - * Decides whether or not the input {@code fileUpdateType} should unpublish an analysis - * - * @param fileUpdateType - * @return boolean - */ - public static boolean doUnpublish(@NonNull FileUpdateTypes fileUpdateType) { - if (fileUpdateType == CONTENT_UPDATE) { - return true; - } else if (fileUpdateType == METADATA_UPDATE || fileUpdateType == NO_UPDATE) { - return false; - } else { - throw new IllegalStateException( - format("The updateType '%s' is unrecognized", fileUpdateType.name())); - } - } } diff --git a/song-server/src/test/java/bio/overture/song/server/service/FileModificationServiceTest.java b/song-server/src/test/java/bio/overture/song/server/service/FileModificationServiceTest.java index bf40a7655..777620eba 100644 --- a/song-server/src/test/java/bio/overture/song/server/service/FileModificationServiceTest.java +++ b/song-server/src/test/java/bio/overture/song/server/service/FileModificationServiceTest.java @@ -28,7 +28,6 @@ import static bio.overture.song.core.model.enums.FileUpdateTypes.NO_UPDATE; import static bio.overture.song.core.model.enums.FileUpdateTypes.resolveFileUpdateType; import static bio.overture.song.core.utils.RandomGenerator.createRandomGenerator; -import static bio.overture.song.server.service.FileModificationService.doUnpublish; import static bio.overture.song.server.utils.TestConstants.DEFAULT_ANALYSIS_ID; import static bio.overture.song.server.utils.TestConstants.DEFAULT_FILE_ID; import static bio.overture.song.server.utils.TestConstants.DEFAULT_STUDY_ID; @@ -37,7 +36,6 @@ import static org.icgc.dcc.common.core.json.JsonNodeBuilders.object; import static org.junit.Assert.*; -import bio.overture.song.core.exceptions.ServerException; import bio.overture.song.core.model.FileUpdateRequest; import bio.overture.song.core.model.enums.AccessTypes; import bio.overture.song.core.testing.SongErrorAssertions; @@ -84,13 +82,6 @@ public void beforeTest() { this.uniqueMd5 = randomGenerator.generateRandomMD5(); } - @Test - public void testDoPublish() { - assertFalse(doUnpublish(NO_UPDATE)); - assertFalse(doUnpublish(METADATA_UPDATE)); - assertTrue(doUnpublish(CONTENT_UPDATE)); - } - @Test @Transactional public void testCheckFileUnrelatedToStudy() { @@ -125,31 +116,60 @@ public void testFileUpdateWithSuppressedAnalysis() { public void testFileUpdateWithPublishedAnalysis() { analysisService.securedUpdateState(DEFAULT_STUDY_ID, DEFAULT_ANALYSIS_ID, PUBLISHED); val originalAnalysis = analysisService.unsecuredDeepRead(DEFAULT_ANALYSIS_ID); - assertEquals(resolveAnalysisState(originalAnalysis.getAnalysisState()), PUBLISHED); + assertEquals(PUBLISHED, resolveAnalysisState(originalAnalysis.getAnalysisState())); val originalFile = fileConverter.convertToFileDTO(fileService.securedRead(DEFAULT_STUDY_ID, DEFAULT_FILE_ID)); // No Change val noChangeRequest = new FileUpdateRequest(); - Exception exception = null; - try { - fileModificationService.securedFileWithAnalysisUpdate( - DEFAULT_STUDY_ID, DEFAULT_FILE_ID, noChangeRequest); - } catch (ServerException ex) { - exception = ex; - } - assertNotNull(exception); + val noChangeResponse = + fileModificationService.securedFileWithAnalysisUpdate( + DEFAULT_STUDY_ID, DEFAULT_FILE_ID, noChangeRequest); + assertFalse(noChangeResponse.isUnpublishedAnalysis()); + assertEquals(NO_UPDATE, noChangeResponse.getFileUpdateType()); + assertEquals(PUBLISHED, noChangeResponse.getOriginalAnalysisState()); + assertEquals(originalFile, noChangeResponse.getOriginalFile()); assertEquals( - "[FileModificationService::illegal.file.update.request] - The file with objectId " - + "'FI1' and analysisId 'AN1' cannot be updated since its analysisState is 'PUBLISHED'", - exception.getMessage()); + "Updated file with objectId 'FI1' and analysisId 'AN1'", noChangeResponse.getMessage()); + + // Metadata Update + val metadataUpdateRequest = + FileUpdateRequest.builder() + .info( + object() + .with( + randomGenerator.generateRandomUUIDAsString(), + randomGenerator.generateRandomUUIDAsString()) + .end()) + .build(); + val originalFile2 = + fileConverter.convertToFileDTO(fileService.securedRead(DEFAULT_STUDY_ID, DEFAULT_FILE_ID)); + val metadataUpdateResponse = + fileModificationService.securedFileWithAnalysisUpdate( + DEFAULT_STUDY_ID, DEFAULT_FILE_ID, metadataUpdateRequest); + assertFalse(metadataUpdateResponse.isUnpublishedAnalysis()); + assertEquals(METADATA_UPDATE, metadataUpdateResponse.getFileUpdateType()); + assertEquals(PUBLISHED, metadataUpdateResponse.getOriginalAnalysisState()); + assertEquals(originalFile2, metadataUpdateResponse.getOriginalFile()); + assertEquals( + "Updated file with objectId 'FI1' and analysisId 'AN1'", + metadataUpdateResponse.getMessage()); + + // Content Update + val contentUpdateRequest = + FileUpdateRequest.builder().fileSize(originalFile2.getFileSize() + 77771L).build(); + SongErrorAssertions.assertSongError( + () -> + fileModificationService.securedFileWithAnalysisUpdate( + DEFAULT_STUDY_ID, DEFAULT_FILE_ID, contentUpdateRequest), + ILLEGAL_FILE_UPDATE_REQUEST); } @Test @Transactional public void testFileUpdateWithUnpublishedAnalysis() { val originalAnalysis = analysisService.unsecuredDeepRead(DEFAULT_ANALYSIS_ID); - assertEquals(resolveAnalysisState(originalAnalysis.getAnalysisState()), UNPUBLISHED); + assertEquals(UNPUBLISHED, resolveAnalysisState(originalAnalysis.getAnalysisState())); val originalFile = fileConverter.convertToFileDTO(fileService.securedRead(DEFAULT_STUDY_ID, DEFAULT_FILE_ID)); @@ -158,10 +178,12 @@ public void testFileUpdateWithUnpublishedAnalysis() { val noChangeResponse = fileModificationService.securedFileWithAnalysisUpdate( DEFAULT_STUDY_ID, DEFAULT_FILE_ID, noChangeRequest); - assertTrue(noChangeResponse.isUnpublishedAnalysis()); - assertEquals(noChangeResponse.getFileUpdateType(), NO_UPDATE); - assertEquals(noChangeResponse.getOriginalAnalysisState(), UNPUBLISHED); - assertEquals(noChangeResponse.getOriginalFile(), originalFile); + assertFalse(noChangeResponse.isUnpublishedAnalysis()); + assertEquals(NO_UPDATE, noChangeResponse.getFileUpdateType()); + assertEquals(UNPUBLISHED, noChangeResponse.getOriginalAnalysisState()); + assertEquals(originalFile, noChangeResponse.getOriginalFile()); + assertEquals( + "Updated file with objectId 'FI1' and analysisId 'AN1'", noChangeResponse.getMessage()); // Metadata Update val metadataUpdateRequest = @@ -178,10 +200,13 @@ public void testFileUpdateWithUnpublishedAnalysis() { val metadataUpdateResponse = fileModificationService.securedFileWithAnalysisUpdate( DEFAULT_STUDY_ID, DEFAULT_FILE_ID, metadataUpdateRequest); - assertTrue(metadataUpdateResponse.isUnpublishedAnalysis()); - assertEquals(metadataUpdateResponse.getFileUpdateType(), METADATA_UPDATE); - assertEquals(metadataUpdateResponse.getOriginalAnalysisState(), UNPUBLISHED); - assertEquals(metadataUpdateResponse.getOriginalFile(), originalFile2); + assertFalse(metadataUpdateResponse.isUnpublishedAnalysis()); + assertEquals(METADATA_UPDATE, metadataUpdateResponse.getFileUpdateType()); + assertEquals(UNPUBLISHED, metadataUpdateResponse.getOriginalAnalysisState()); + assertEquals(originalFile2, metadataUpdateResponse.getOriginalFile()); + assertEquals( + "Updated file with objectId 'FI1' and analysisId 'AN1'", + metadataUpdateResponse.getMessage()); // Content Update val contentUpdateRequest = @@ -191,10 +216,13 @@ public void testFileUpdateWithUnpublishedAnalysis() { val contentUpdateResponse = fileModificationService.securedFileWithAnalysisUpdate( DEFAULT_STUDY_ID, DEFAULT_FILE_ID, contentUpdateRequest); - assertTrue(contentUpdateResponse.isUnpublishedAnalysis()); - assertEquals(contentUpdateResponse.getFileUpdateType(), CONTENT_UPDATE); - assertEquals(contentUpdateResponse.getOriginalAnalysisState(), UNPUBLISHED); - assertEquals(contentUpdateResponse.getOriginalFile(), originalFile3); + assertFalse(contentUpdateResponse.isUnpublishedAnalysis()); + assertEquals(CONTENT_UPDATE, contentUpdateResponse.getFileUpdateType()); + assertEquals(UNPUBLISHED, contentUpdateResponse.getOriginalAnalysisState()); + assertEquals(originalFile3, contentUpdateResponse.getOriginalFile()); + assertEquals( + "Updated file with objectId 'FI1' and analysisId 'AN1'", + contentUpdateResponse.getMessage()); } @Test @@ -270,9 +298,9 @@ public void testUpdateWithRequests() { val goldenFile = converter.copyFile(referenceFile); val u1 = FileUpdateRequest.builder().fileAccess("controlled").build(); - assertEquals(fileModificationService.updateWithRequest(referenceFile, u1), METADATA_UPDATE); + assertEquals(METADATA_UPDATE, fileModificationService.updateWithRequest(referenceFile, u1)); assertFalse(referenceFile == goldenFile); - assertEquals(referenceFile, goldenFile); + assertEquals(goldenFile, referenceFile); u1.setInfo( object() @@ -280,63 +308,63 @@ public void testUpdateWithRequests() { randomGenerator.generateRandomUUIDAsString(), randomGenerator.generateRandomUUIDAsString()) .end()); - assertEquals(fileModificationService.updateWithRequest(referenceFile, u1), METADATA_UPDATE); + assertEquals(METADATA_UPDATE, fileModificationService.updateWithRequest(referenceFile, u1)); assertFalse(referenceFile == goldenFile); - assertEquals(referenceFile, goldenFile); + assertEquals(goldenFile, referenceFile); u1.setFileAccess("open"); - assertEquals(fileModificationService.updateWithRequest(referenceFile, u1), METADATA_UPDATE); + assertEquals(METADATA_UPDATE, fileModificationService.updateWithRequest(referenceFile, u1)); assertFalse(referenceFile == goldenFile); - assertEquals(referenceFile, goldenFile); + assertEquals(goldenFile, referenceFile); u1.setFileAccess(null); - assertEquals(fileModificationService.updateWithRequest(referenceFile, u1), METADATA_UPDATE); + assertEquals(METADATA_UPDATE, fileModificationService.updateWithRequest(referenceFile, u1)); assertFalse(referenceFile == goldenFile); - assertEquals(referenceFile, goldenFile); + assertEquals(goldenFile, referenceFile); u1.setFileSize(19191L); - assertEquals(fileModificationService.updateWithRequest(referenceFile, u1), CONTENT_UPDATE); + assertEquals(CONTENT_UPDATE, fileModificationService.updateWithRequest(referenceFile, u1)); assertFalse(referenceFile == goldenFile); - assertEquals(referenceFile, goldenFile); + assertEquals(goldenFile, referenceFile); u1.setFileMd5sum(randomGenerator.generateRandomMD5()); - assertEquals(fileModificationService.updateWithRequest(referenceFile, u1), CONTENT_UPDATE); + assertEquals(CONTENT_UPDATE, fileModificationService.updateWithRequest(referenceFile, u1)); assertFalse(referenceFile == goldenFile); - assertEquals(referenceFile, goldenFile); + assertEquals(goldenFile, referenceFile); u1.setInfo(null); - assertEquals(fileModificationService.updateWithRequest(referenceFile, u1), CONTENT_UPDATE); + assertEquals(CONTENT_UPDATE, fileModificationService.updateWithRequest(referenceFile, u1)); assertFalse(referenceFile == goldenFile); - assertEquals(referenceFile, goldenFile); + assertEquals(goldenFile, referenceFile); u1.setFileAccess(null); - assertEquals(fileModificationService.updateWithRequest(referenceFile, u1), CONTENT_UPDATE); + assertEquals(CONTENT_UPDATE, fileModificationService.updateWithRequest(referenceFile, u1)); assertFalse(referenceFile == goldenFile); - assertEquals(referenceFile, goldenFile); + assertEquals(goldenFile, referenceFile); u1.setFileMd5sum(uniqueMd5); - assertEquals(fileModificationService.updateWithRequest(referenceFile, u1), CONTENT_UPDATE); + assertEquals(CONTENT_UPDATE, fileModificationService.updateWithRequest(referenceFile, u1)); assertFalse(referenceFile == goldenFile); - assertEquals(referenceFile, goldenFile); + assertEquals(goldenFile, referenceFile); u1.setFileMd5sum(null); - assertEquals(fileModificationService.updateWithRequest(referenceFile, u1), CONTENT_UPDATE); + assertEquals(CONTENT_UPDATE, fileModificationService.updateWithRequest(referenceFile, u1)); assertFalse(referenceFile == goldenFile); - assertEquals(referenceFile, goldenFile); + assertEquals(goldenFile, referenceFile); u1.setFileSize(referenceFile.getFileSize()); - assertEquals(fileModificationService.updateWithRequest(referenceFile, u1), NO_UPDATE); + assertEquals(NO_UPDATE, fileModificationService.updateWithRequest(referenceFile, u1)); assertFalse(referenceFile == goldenFile); - assertEquals(referenceFile, goldenFile); + assertEquals(goldenFile, referenceFile); u1.setFileSize(null); - assertEquals(fileModificationService.updateWithRequest(referenceFile, u1), NO_UPDATE); + assertEquals(NO_UPDATE, fileModificationService.updateWithRequest(referenceFile, u1)); assertNull(u1.getFileAccess()); assertNull(u1.getFileSize()); assertNull(u1.getFileMd5sum()); assertNull(u1.getInfo()); assertFalse(referenceFile == goldenFile); - assertEquals(referenceFile, goldenFile); + assertEquals(goldenFile, referenceFile); } @Test @@ -347,38 +375,38 @@ public void testFileUpdateTypeResolution() { // update access field val u1 = FileUpdateRequest.builder().fileAccess("controlled").build(); - assertEquals(resolveFileUpdateType(f1, u1), METADATA_UPDATE); + assertEquals(METADATA_UPDATE, resolveFileUpdateType(f1, u1)); // update info field u1.setInfo(object().with("myInfoKey2", "myInfoValue2").end()); - assertEquals(resolveFileUpdateType(f1, u1), METADATA_UPDATE); + assertEquals(METADATA_UPDATE, resolveFileUpdateType(f1, u1)); // update file size val u2 = FileUpdateRequest.builder().fileSize(123123L).build(); u1.setFileSize(123456L); // test request u1 with metadata updates - assertEquals(resolveFileUpdateType(f1, u1), CONTENT_UPDATE); + assertEquals(CONTENT_UPDATE, resolveFileUpdateType(f1, u1)); // test request u2 without any metadata updates - assertEquals(resolveFileUpdateType(f1, u2), CONTENT_UPDATE); + assertEquals(CONTENT_UPDATE, resolveFileUpdateType(f1, u2)); // update file md5 u2.setFileMd5sum(randomGenerator.generateRandomMD5()); u1.setFileMd5sum(randomGenerator.generateRandomMD5()); // test request u1 with metadata updates - assertEquals(resolveFileUpdateType(f1, u1), CONTENT_UPDATE); + assertEquals(CONTENT_UPDATE, resolveFileUpdateType(f1, u1)); // test request u2 without any metadata updates - assertEquals(resolveFileUpdateType(f1, u2), CONTENT_UPDATE); + assertEquals(CONTENT_UPDATE, resolveFileUpdateType(f1, u2)); // test nulls val u3 = FileUpdateRequest.builder().build(); - assertEquals(resolveFileUpdateType(f1, u3), NO_UPDATE); + assertEquals(NO_UPDATE, resolveFileUpdateType(f1, u3)); u3.setFileMd5sum(f1.getFileMd5sum()); u3.setFileSize(f1.getFileSize()); u3.setFileAccess(f1.getFileAccess()); u3.setInfo(f1.getInfo()); - assertEquals(resolveFileUpdateType(f1, u3), NO_UPDATE); + assertEquals(NO_UPDATE, resolveFileUpdateType(f1, u3)); - assertEquals(f1, golden); + assertEquals(golden, f1); } private FileEntity buildReferenceFile() { From 6014211b08aa63c015e62a14131d33a5b300292a Mon Sep 17 00:00:00 2001 From: khartmann Date: Mon, 9 Dec 2019 10:16:43 -0500 Subject: [PATCH 3/3] Code Cleanup: Fixes as per PR --- .../song/server/service/FileModificationService.java | 2 -- .../song/server/service/FileModificationServiceTest.java | 6 ++++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/song-server/src/main/java/bio/overture/song/server/service/FileModificationService.java b/song-server/src/main/java/bio/overture/song/server/service/FileModificationService.java index 991eb0de0..cfd2e0fba 100644 --- a/song-server/src/main/java/bio/overture/song/server/service/FileModificationService.java +++ b/song-server/src/main/java/bio/overture/song/server/service/FileModificationService.java @@ -143,8 +143,6 @@ public FileUpdateResponse securedFileWithAnalysisUpdate( response.originalFile(fileConverter.convertToFileDTO(originalFile)); response.originalAnalysisState(currentState); response.fileUpdateType(fileUpdateType); - response.message( - format("Updated file with objectId '%s' and analysisId '%s'", objectId, analysisId)); return response.build(); } diff --git a/song-server/src/test/java/bio/overture/song/server/service/FileModificationServiceTest.java b/song-server/src/test/java/bio/overture/song/server/service/FileModificationServiceTest.java index 777620eba..51671a4f8 100644 --- a/song-server/src/test/java/bio/overture/song/server/service/FileModificationServiceTest.java +++ b/song-server/src/test/java/bio/overture/song/server/service/FileModificationServiceTest.java @@ -130,7 +130,8 @@ public void testFileUpdateWithPublishedAnalysis() { assertEquals(PUBLISHED, noChangeResponse.getOriginalAnalysisState()); assertEquals(originalFile, noChangeResponse.getOriginalFile()); assertEquals( - "Updated file with objectId 'FI1' and analysisId 'AN1'", noChangeResponse.getMessage()); + "No update for file with objectId 'FI1' and analysisId 'AN1'", + noChangeResponse.getMessage()); // Metadata Update val metadataUpdateRequest = @@ -183,7 +184,8 @@ public void testFileUpdateWithUnpublishedAnalysis() { assertEquals(UNPUBLISHED, noChangeResponse.getOriginalAnalysisState()); assertEquals(originalFile, noChangeResponse.getOriginalFile()); assertEquals( - "Updated file with objectId 'FI1' and analysisId 'AN1'", noChangeResponse.getMessage()); + "No update for file with objectId 'FI1' and analysisId 'AN1'", + noChangeResponse.getMessage()); // Metadata Update val metadataUpdateRequest =