Merge pull request #139 from overture-stack/hotfix/score-song-publish…

…-deadlock fix: Deadlock fix with song and score
overture-stack · Feb 13, 2019 · fa495ea · fa495ea
2 parents d35bfd2 + 3ea9927
commit fa495ea
Show file tree

Hide file tree

Showing 6 changed files with 71 additions and 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
@@ -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,14 +75,15 @@ 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) {
 
     val ipAddress = HttpServletRequests.getIpAddress(request);
 
     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
@@ -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/...server/src/main/java/bio/overture/score/server/repository/azure/AzureDownloadService.java b/...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
@@ -17,47 +17,46 @@
  */
 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;
 import com.amazonaws.services.s3.model.S3Object;
 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,24 +144,32 @@ 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();
       val analysisState = metadataService.getAnalysisStateForMetadata(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/...e-server/src/test/java/bio/overture/score/server/repository/s3/S3DownloadServiceTest.java b/...e-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/...r/src/test/java/bio/overture/score/server/service/download/ObjectDownloadServiceTest.java b/...r/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);