From 3ea9927c4787988982ed6885ac2d4ecb0089f0a7 Mon Sep 17 00:00:00 2001 From: rtisma Date: Wed, 13 Feb 2019 09:20:23 -0500 Subject: [PATCH] fix: Deadlong fix with song and score When song tries to publish, it calls the download endpoint on score. Since the analysis is currently UNPUBLISHED and score will only allow downloads for PUBLISHED files, therefore song cannot publish because score only allows published files to be download. Fix was to add a new path param, that will return object spec WITHOUT urls. --- .../server/controller/DownloadController.java | 14 ++-- .../server/repository/DownloadService.java | 2 +- .../azure/AzureDownloadService.java | 2 +- .../repository/s3/S3DownloadService.java | 72 +++++++++++-------- .../repository/s3/S3DownloadServiceTest.java | 8 ++- .../download/ObjectDownloadServiceTest.java | 31 ++++---- 6 files changed, 71 insertions(+), 58 deletions(-) diff --git a/score-server/src/main/java/bio/overture/score/server/controller/DownloadController.java b/score-server/src/main/java/bio/overture/score/server/controller/DownloadController.java index abb47223..08111753 100644 --- a/score-server/src/main/java/bio/overture/score/server/controller/DownloadController.java +++ b/score-server/src/main/java/bio/overture/score/server/controller/DownloadController.java @@ -17,12 +17,13 @@ */ package bio.overture.score.server.controller; -import javax.servlet.http.HttpServletRequest; - import bio.overture.score.core.model.ObjectSpecification; -import bio.overture.score.server.util.HttpServletRequests; import bio.overture.score.server.repository.DownloadService; import bio.overture.score.server.security.TokenHasher; +import bio.overture.score.server.util.HttpServletRequests; +import lombok.Setter; +import lombok.extern.slf4j.Slf4j; +import lombok.val; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Profile; import org.springframework.http.HttpHeaders; @@ -39,9 +40,7 @@ import org.springframework.web.bind.annotation.ResponseBody; import org.springframework.web.bind.annotation.RestController; -import lombok.Setter; -import lombok.val; -import lombok.extern.slf4j.Slf4j; +import javax.servlet.http.HttpServletRequest; /** * A controller to expose RESTful API for download @@ -76,6 +75,7 @@ public class DownloadController { @RequestParam(value = "offset", required = true) long offset, @RequestParam(value = "length", required = true) long length, @RequestParam(value = "external", defaultValue = "false") boolean external, + @RequestParam(value = "exclude-urls", defaultValue = "false") boolean excludeUrls, @RequestHeader(value = "User-Agent", defaultValue = "unknown") String userAgent, HttpServletRequest request) { @@ -83,7 +83,7 @@ public class DownloadController { log.info("Requesting download of object id {} with access token {} (MD5) from {} and client version {}", objectId, identifier(accessToken), ipAddress, userAgent); - return downloadService.download(objectId, offset, length, external); + return downloadService.download(objectId, offset, length, external, excludeUrls); } protected String identifier(String accessToken) { diff --git a/score-server/src/main/java/bio/overture/score/server/repository/DownloadService.java b/score-server/src/main/java/bio/overture/score/server/repository/DownloadService.java index 4742f068..656c1ca7 100644 --- a/score-server/src/main/java/bio/overture/score/server/repository/DownloadService.java +++ b/score-server/src/main/java/bio/overture/score/server/repository/DownloadService.java @@ -21,7 +21,7 @@ public interface DownloadService { - ObjectSpecification download(String objectId, long offset, long length, boolean forExternalUse); + ObjectSpecification download(String objectId, long offset, long length, boolean forExternalUse, boolean excludeUrls); /** * Attempts to fetch a pre-defined object id (defined in application.yml) from the object repository. Used to confirm diff --git a/score-server/src/main/java/bio/overture/score/server/repository/azure/AzureDownloadService.java b/score-server/src/main/java/bio/overture/score/server/repository/azure/AzureDownloadService.java index f18de9d5..17e235d0 100644 --- a/score-server/src/main/java/bio/overture/score/server/repository/azure/AzureDownloadService.java +++ b/score-server/src/main/java/bio/overture/score/server/repository/azure/AzureDownloadService.java @@ -68,7 +68,7 @@ public class AzureDownloadService implements DownloadService { private String sentinelObjectId; @Override - public ObjectSpecification download(String objectId, long offset, long length, boolean forExternalUse) { + public ObjectSpecification download(String objectId, long offset, long length, boolean forExternalUse, boolean excludeUrls) { try { checkArgument(offset >= 0L); diff --git a/score-server/src/main/java/bio/overture/score/server/repository/s3/S3DownloadService.java b/score-server/src/main/java/bio/overture/score/server/repository/s3/S3DownloadService.java index 85f25c48..087540fa 100644 --- a/score-server/src/main/java/bio/overture/score/server/repository/s3/S3DownloadService.java +++ b/score-server/src/main/java/bio/overture/score/server/repository/s3/S3DownloadService.java @@ -17,40 +17,20 @@ */ package bio.overture.score.server.repository.s3; -import static bio.overture.score.server.metadata.MetadataService.getAnalysisId; -import static com.google.common.base.Preconditions.checkArgument; - -import bio.overture.score.server.metadata.MetadataEntity; -import bio.overture.score.server.metadata.MetadataService; -import lombok.Cleanup; -import lombok.Setter; -import lombok.val; -import lombok.extern.slf4j.Slf4j; - -import java.io.IOException; -import java.time.LocalDateTime; -import java.time.ZoneId; -import java.util.Date; -import java.util.List; - import bio.overture.score.core.model.ObjectKey; import bio.overture.score.core.model.ObjectSpecification; import bio.overture.score.core.model.Part; import bio.overture.score.core.util.ObjectKeys; +import bio.overture.score.core.util.PartCalculator; import bio.overture.score.server.exception.IdNotFoundException; import bio.overture.score.server.exception.InternalUnrecoverableError; import bio.overture.score.server.exception.NotRetryableException; import bio.overture.score.server.exception.RetryableException; -import bio.overture.score.server.repository.URLGenerator; +import bio.overture.score.server.metadata.MetadataEntity; +import bio.overture.score.server.metadata.MetadataService; import bio.overture.score.server.repository.BucketNamingService; import bio.overture.score.server.repository.DownloadService; -import bio.overture.score.core.util.PartCalculator; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.beans.factory.annotation.Value; -import org.springframework.context.annotation.Profile; -import org.springframework.http.HttpStatus; -import org.springframework.stereotype.Service; - +import bio.overture.score.server.repository.URLGenerator; import com.amazonaws.AmazonServiceException; import com.amazonaws.services.s3.AmazonS3; import com.amazonaws.services.s3.model.GetObjectRequest; @@ -58,6 +38,25 @@ import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.ObjectMapper; +import lombok.Cleanup; +import lombok.NonNull; +import lombok.Setter; +import lombok.extern.slf4j.Slf4j; +import lombok.val; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.context.annotation.Profile; +import org.springframework.http.HttpStatus; +import org.springframework.stereotype.Service; + +import java.io.IOException; +import java.time.LocalDateTime; +import java.time.ZoneId; +import java.util.Date; +import java.util.List; + +import static bio.overture.score.server.metadata.MetadataService.getAnalysisId; +import static com.google.common.base.Preconditions.checkArgument; /** * service responsible for object download (full or partial) @@ -100,10 +99,13 @@ public class S3DownloadService implements DownloadService { @Autowired private MetadataService metadataService; + @Override - public ObjectSpecification download(String objectId, long offset, long length, boolean forExternalUse) { + public ObjectSpecification download(String objectId, long offset, long length, boolean forExternalUse, boolean excludeUrls) { try { - checkPublishedAnalysisState(metadataService.getEntity(objectId)); + if (!excludeUrls){ + checkPublishedAnalysisState(metadataService.getEntity(objectId)); + } checkArgument(offset > -1L); @@ -112,7 +114,7 @@ public ObjectSpecification download(String objectId, long offset, long length, b // Short-circuit in default case if (!forExternalUse && (offset == 0L && length < 0L)) { - return objectSpec; + return excludeUrls ? removeUrls(objectSpec) : objectSpec; } // Calculate range values @@ -142,16 +144,24 @@ public ObjectSpecification download(String objectId, long offset, long length, b fillPartUrls(objectKey, parts, objectSpec.isRelocated(), forExternalUse); - return new ObjectSpecification(objectKey.getKey(), objectId, objectId, parts, length, objectSpec.getObjectMd5(), + val spec = new ObjectSpecification(objectKey.getKey(), objectId, objectId, parts, length, objectSpec.getObjectMd5(), objectSpec.isRelocated()); + + return excludeUrls ? removeUrls(spec) : spec; + } catch (Exception e) { - log.error("Failed to download objectId: {}, offset: {}, length: {}, forExternalUse: {}: {} ", - objectId, offset, length, forExternalUse, e); + log.error("Failed to download objectId: {}, offset: {}, length: {}, forExternalUse: {}, excludeUrls: {} : {} ", + objectId, offset, length, forExternalUse, excludeUrls, e); throw e; } } + private static ObjectSpecification removeUrls(ObjectSpecification spec){ + spec.getParts().forEach(x -> x.setUrl(null)); + return spec; + } + void checkPublishedAnalysisState(MetadataEntity entity){ if(!useLegacyMode){ val objectId = entity.getId(); @@ -159,7 +169,7 @@ void checkPublishedAnalysisState(MetadataEntity entity){ if (!analysisState.equals(PUBLISHED_ANALYSIS_STATE)){ val message = String.format("Critical Error: cannot complete download for objectId '%s' with " + "analysisState '%s' and analysisId '%s'. " - + "Can only download objects that have the analysisState '%s'. Update the file metadata and retry.", + + "Can only download objects that have the analysisState '%s' or when the 'exclude-urls=true' flag is set. Update the file metadata or url parameters and retry.", objectId, analysisState, getAnalysisId(entity), PUBLISHED_ANALYSIS_STATE); log.error(message); // Log to audit log file throw new NotRetryableException(new IllegalStateException(message)); diff --git a/score-server/src/test/java/bio/overture/score/server/repository/s3/S3DownloadServiceTest.java b/score-server/src/test/java/bio/overture/score/server/repository/s3/S3DownloadServiceTest.java index 8b50a1f3..f3cd96d1 100644 --- a/score-server/src/test/java/bio/overture/score/server/repository/s3/S3DownloadServiceTest.java +++ b/score-server/src/test/java/bio/overture/score/server/repository/s3/S3DownloadServiceTest.java @@ -9,8 +9,10 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.runners.MockitoJUnitRunner; + import static java.lang.String.format; -import static org.assertj.core.api.Assertions.*; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.catchThrowable; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -50,11 +52,11 @@ public void test_if_unpublished_file_triggers_error() { } @Test - public void verify_if_download_unpublished_objectId_is_blocked() { + public void verify_if_download_is_blocked_with_unpublished_objectId() { when(mockService.getAnalysisStateForMetadata(metadataEntity)).thenReturn("UNPUBLISHED"); when(mockService.getEntity(objectId)).thenReturn(metadataEntity); - val throwable = catchThrowable(() -> s3DownloadService.download(objectId, 0, -1, false)); + val throwable = catchThrowable(() -> s3DownloadService.download(objectId, 0, -1, false, false)); assertThat(throwable).isExactlyInstanceOf(NotRetryableException.class); } diff --git a/score-server/src/test/java/bio/overture/score/server/service/download/ObjectDownloadServiceTest.java b/score-server/src/test/java/bio/overture/score/server/service/download/ObjectDownloadServiceTest.java index 02905a3b..80155c28 100644 --- a/score-server/src/test/java/bio/overture/score/server/service/download/ObjectDownloadServiceTest.java +++ b/score-server/src/test/java/bio/overture/score/server/service/download/ObjectDownloadServiceTest.java @@ -17,22 +17,19 @@ */ package bio.overture.score.server.service.download; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.*; - -import java.net.URL; -import java.util.regex.Pattern; - import bio.overture.score.core.util.ObjectKeys; +import bio.overture.score.core.util.SimplePartCalculator; import bio.overture.score.server.config.ServerConfig; import bio.overture.score.server.exception.IdNotFoundException; -import bio.overture.score.core.util.SimplePartCalculator; import bio.overture.score.server.metadata.MetadataEntity; import bio.overture.score.server.metadata.MetadataService; import bio.overture.score.server.repository.s3.S3BucketNamingService; import bio.overture.score.server.repository.s3.S3DownloadService; import bio.overture.score.server.repository.s3.S3URLGenerator; +import com.amazonaws.AmazonServiceException; +import com.amazonaws.services.s3.AmazonS3; +import com.google.common.base.Splitter; +import lombok.val; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -43,11 +40,15 @@ import org.springframework.test.context.ContextConfiguration; import org.springframework.test.util.ReflectionTestUtils; -import com.amazonaws.AmazonServiceException; -import com.amazonaws.services.s3.AmazonS3; -import com.google.common.base.Splitter; +import java.net.URL; +import java.util.regex.Pattern; -import lombok.val; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.when; @RunWith(MockitoJUnitRunner.class) @ContextConfiguration(classes = ServerConfig.class) @@ -114,7 +115,7 @@ public void it_takes_two_to_fail_with_not_found() { firstException.setStatusCode(HttpStatus.NOT_FOUND.value()); when(s3Client.getObject(Mockito.any())).thenThrow(firstException, firstException); // stubs first two calls to // s3Client.getObject() - service.download(objectId, 0, 1000, false); + service.download(objectId, 0, 1000, false, false); } @Test @@ -148,7 +149,7 @@ public void verify_fallback_in_download_presigned_urls() throws Exception { val sut = spy(service); doReturn(os).when(sut).getSpecification(objectId); - val objSpec = sut.download(objectId, 0, 104857600, false); + val objSpec = sut.download(objectId, 0, 104857600, false, false); val p = objSpec.getParts().get(0); @@ -194,7 +195,7 @@ public void verify_partitioned_buckets_in_download_presigned_urls() throws Excep val sut = spy(service); doReturn(os).when(sut).getSpecification(objectId); - val objSpec = sut.download(objectId, 0, 104857600, false); + val objSpec = sut.download(objectId, 0, 104857600, false, false); val p = objSpec.getParts().get(0);