From 615bd297e20b14db8bdfa60c300e4f1a2afd3d5a Mon Sep 17 00:00:00 2001 From: Leonid Andreev Date: Mon, 21 Oct 2019 18:52:29 -0400 Subject: [PATCH 1/6] fix for #6278 --- src/main/java/edu/harvard/iq/dataverse/api/Access.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Access.java b/src/main/java/edu/harvard/iq/dataverse/api/Access.java index dafb2a53133..00cc38bed47 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Access.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Access.java @@ -299,7 +299,7 @@ public DownloadInstance datafile(@PathParam("fileId") String fileId, @QueryParam if (df.isTabularData()) { String originalMimeType = df.getDataTable().getOriginalFileFormat(); dInfo.addServiceAvailable(new OptionalAccessService("original", originalMimeType, "format=original","Saved original (" + originalMimeType + ")")); - + dInfo.addServiceAvailable(new OptionalAccessService("tabular", "text/tab-separated-values", "format=tab", "Tabular file in native format")); dInfo.addServiceAvailable(new OptionalAccessService("R", "application/x-rlang-transport", "format=RData", "Data in R format")); dInfo.addServiceAvailable(new OptionalAccessService("preprocessed", "application/json", "format=prep", "Preprocessed data in JSON")); dInfo.addServiceAvailable(new OptionalAccessService("subset", "text/tab-separated-values", "variables=<LIST>", "Column-wise Subsetting")); From 9ef7feca4b3f00aecfcd02a895a248fc3315958c Mon Sep 17 00:00:00 2001 From: Leonid Andreev Date: Mon, 21 Oct 2019 18:53:49 -0400 Subject: [PATCH 2/6] removed a verbose log line (#6278) --- src/main/java/edu/harvard/iq/dataverse/api/Access.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Access.java b/src/main/java/edu/harvard/iq/dataverse/api/Access.java index 00cc38bed47..4ea3c6efa90 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Access.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Access.java @@ -381,7 +381,7 @@ public DownloadInstance datafile(@PathParam("fileId") String fileId, @QueryParam // a NotFoundException. throw new NotFoundException(); } // Else - the file itself was requested or we have the info needed to invoke the service and get the derived info - logger.warning("Returning download instance"); + logger.fine("Returning download instance"); /* * Provide some browser-friendly headers: (?) */ From 879ea341b5628c74599b5d37c3df7f8d3a49721f Mon Sep 17 00:00:00 2001 From: Leonid Andreev Date: Tue, 22 Oct 2019 15:57:18 -0400 Subject: [PATCH 3/6] better http return code; better API status messages (#6278) --- .../edu/harvard/iq/dataverse/api/Access.java | 2 +- .../dataverse/api/DownloadInstanceWriter.java | 11 +++-- .../ServiceUnavailableExceptionHandler.java | 43 +++++++++++++++++++ 3 files changed, 51 insertions(+), 5 deletions(-) create mode 100644 src/main/java/edu/harvard/iq/dataverse/api/errorhandlers/ServiceUnavailableExceptionHandler.java diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Access.java b/src/main/java/edu/harvard/iq/dataverse/api/Access.java index 4ea3c6efa90..1c41a0dfef0 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Access.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Access.java @@ -379,7 +379,7 @@ public DownloadInstance datafile(@PathParam("fileId") String fileId, @QueryParam // a ServiceNotAvailableException. However, since the returns are all files of // some sort, it seems reasonable, and more standard, to just return // a NotFoundException. - throw new NotFoundException(); + throw new NotFoundException("datafile access error: requested optional service (image scaling, format conversion, etc.) is not supported on this datafile."); } // Else - the file itself was requested or we have the info needed to invoke the service and get the derived info logger.fine("Returning download instance"); /* diff --git a/src/main/java/edu/harvard/iq/dataverse/api/DownloadInstanceWriter.java b/src/main/java/edu/harvard/iq/dataverse/api/DownloadInstanceWriter.java index a9c817cc07f..8c480bdcc36 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/DownloadInstanceWriter.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/DownloadInstanceWriter.java @@ -37,10 +37,10 @@ import java.util.List; import java.util.logging.Level; import java.util.logging.Logger; -import javax.faces.context.FacesContext; import javax.inject.Inject; import javax.ws.rs.NotFoundException; import javax.ws.rs.RedirectionException; +import javax.ws.rs.ServiceUnavailableException; /** * @@ -222,7 +222,10 @@ public void writeTo(DownloadInstance di, Class clazz, Type type, Annotation[] if (storageIO == null) { - throw new WebApplicationException(Response.Status.SERVICE_UNAVAILABLE); + //throw new WebApplicationException(Response.Status.SERVICE_UNAVAILABLE); + // 404/not found may be a better return code option here + // (similarly to what the Access API returns when a thumbnail is requested on a text file, etc.) + throw new NotFoundException("datafile access error: requested optional service (image scaling, format conversion, etc.) could not be performed on this datafile."); } } else { if (storageIO instanceof S3AccessIO && !(dataFile.isTabularData()) && isRedirectToS3()) { @@ -241,7 +244,7 @@ public void writeTo(DownloadInstance di, Class clazz, Type type, Annotation[] } if (redirect_url_str == null) { - throw new WebApplicationException(Response.Status.SERVICE_UNAVAILABLE); + throw new WebApplicationException(new ServiceUnavailableException()); } logger.fine("Data Access API: direct S3 url: "+redirect_url_str); @@ -271,7 +274,7 @@ public void writeTo(DownloadInstance di, Class clazz, Type type, Annotation[] logger.fine("Issuing redirect to the file location on S3."); throw new RedirectionException(response); } - throw new WebApplicationException(Response.Status.SERVICE_UNAVAILABLE); + throw new WebApplicationException(new ServiceUnavailableException()); } } diff --git a/src/main/java/edu/harvard/iq/dataverse/api/errorhandlers/ServiceUnavailableExceptionHandler.java b/src/main/java/edu/harvard/iq/dataverse/api/errorhandlers/ServiceUnavailableExceptionHandler.java new file mode 100644 index 00000000000..ebed03a13da --- /dev/null +++ b/src/main/java/edu/harvard/iq/dataverse/api/errorhandlers/ServiceUnavailableExceptionHandler.java @@ -0,0 +1,43 @@ +package edu.harvard.iq.dataverse.api.errorhandlers; + +import javax.json.Json; +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.ServiceUnavailableException; +import javax.ws.rs.core.Context; +import javax.ws.rs.core.Response; +import javax.ws.rs.ext.ExceptionMapper; +import javax.ws.rs.ext.Provider; + +/** + * Produces custom 503 messages for the API. + * @author michael + */ +@Provider +public class ServiceUnavailableExceptionHandler implements ExceptionMapper{ + + @Context + HttpServletRequest request; + + @Override + public Response toResponse(ServiceUnavailableException ex){ + String uri = request.getRequestURI(); + String exMessage = ex.getMessage(); + String outputMessage; + if (exMessage != null && exMessage.toLowerCase().startsWith("datafile")) { + outputMessage = exMessage; + } else { + outputMessage = "Requested service or method not available on the requested object"; + } + return Response.status(503) + .entity( Json.createObjectBuilder() + .add("status", "ERROR") + .add("code", 503) + .add("message", "'" + uri + "' " + outputMessage) + .build()) + .type("application/json").build(); + + + } + +} + From 05040d11dbe1e6fb45c2c69347bf281dba2600fb Mon Sep 17 00:00:00 2001 From: Leonid Andreev Date: Tue, 22 Oct 2019 16:50:48 -0400 Subject: [PATCH 4/6] fixes another thing broken in that PR - "noVarHeader" flag for tab downloads (#6278) --- src/main/java/edu/harvard/iq/dataverse/api/Access.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Access.java b/src/main/java/edu/harvard/iq/dataverse/api/Access.java index 1c41a0dfef0..42328367241 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Access.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Access.java @@ -321,7 +321,7 @@ public DownloadInstance datafile(@PathParam("fileId") String fileId, @QueryParam logger.fine("is download service supported? key=" + key + ", value=" + value); // The loop goes through all query params (e.g. including key, gbrecs, persistentId, etc. ) // So we need to identify when a service is being called and then let checkIfServiceSupportedAndSetConverter see if the required one exists - if (key.equals("imageThumb") || key.equals("format") || key.equals("variables")) { + if (key.equals("imageThumb") || key.equals("format") || key.equals("variables") || key.equals("noVarHeader")) { serviceRequested = true; //Only need to check if this key is associated with a service if (downloadInstance.checkIfServiceSupportedAndSetConverter(key, value)) { @@ -371,6 +371,8 @@ public DownloadInstance datafile(@PathParam("fileId") String fileId, @QueryParam serviceFound = true; break; } + } else { + } } if (serviceRequested && !serviceFound) { From 7f7ae358cea9469a2ac60e3fc358f720a9ecc6f6 Mon Sep 17 00:00:00 2001 From: Leonid Andreev Date: Wed, 23 Oct 2019 13:11:59 -0400 Subject: [PATCH 5/6] added AccessIT to the list of tests run by the automated RestAssured suite (#6278) --- conf/docker-aio/run-test-suite.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conf/docker-aio/run-test-suite.sh b/conf/docker-aio/run-test-suite.sh index c1fca242389..811bc579c6d 100755 --- a/conf/docker-aio/run-test-suite.sh +++ b/conf/docker-aio/run-test-suite.sh @@ -8,4 +8,4 @@ fi # Please note the "dataverse.test.baseurl" is set to run for "all-in-one" Docker environment. # TODO: Rather than hard-coding the list of "IT" classes here, add a profile to pom.xml. -mvn test -Dtest=DataversesIT,DatasetsIT,SwordIT,AdminIT,BuiltinUsersIT,UsersIT,UtilIT,ConfirmEmailIT,FileMetadataIT,FilesIT,SearchIT,InReviewWorkflowIT,HarvestingServerIT,MoveIT,MakeDataCountApiIT,FileTypeDetectionIT,EditDDIIT,ExternalToolsIT -Ddataverse.test.baseurl=$dvurl +mvn test -Dtest=DataversesIT,DatasetsIT,SwordIT,AdminIT,BuiltinUsersIT,UsersIT,UtilIT,ConfirmEmailIT,FileMetadataIT,FilesIT,SearchIT,InReviewWorkflowIT,HarvestingServerIT,MoveIT,MakeDataCountApiIT,FileTypeDetectionIT,EditDDIIT,ExternalToolsIT,AccessIT -Ddataverse.test.baseurl=$dvurl From e4f0c3039c1b3acd124524b32bd301503abcfb94 Mon Sep 17 00:00:00 2001 From: Leonid Andreev Date: Wed, 23 Oct 2019 13:46:16 -0400 Subject: [PATCH 6/6] added a test to AccessIT that would've cought "format=tab" download. (#6278) --- .../edu/harvard/iq/dataverse/api/AccessIT.java | 17 ++++++++++++++--- .../edu/harvard/iq/dataverse/api/UtilIT.java | 6 ++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/test/java/edu/harvard/iq/dataverse/api/AccessIT.java b/src/test/java/edu/harvard/iq/dataverse/api/AccessIT.java index f0ea5ffb0c4..7a69793ebb3 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/AccessIT.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/AccessIT.java @@ -27,6 +27,12 @@ import static org.junit.Assert.assertTrue; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; +import static junit.framework.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; /** * @@ -172,17 +178,22 @@ public void testDownloadSingleFile() { Response anonDownloadConverted = UtilIT.downloadFile(tabFile1Id); // ... and download the same tabular data file, but without the variable name header added: Response anonDownloadTabularNoHeader = UtilIT.downloadTabularFileNoVarHeader(tabFile1Id); + // ... and download the same tabular file, this time requesting the "format=tab" explicitly: + Response anonDownloadTabularWithFormatName = UtilIT.downloadTabularFile(tabFile1Id); assertEquals(OK.getStatusCode(), anonDownloadOriginal.getStatusCode()); assertEquals(OK.getStatusCode(), anonDownloadConverted.getStatusCode()); assertEquals(OK.getStatusCode(), anonDownloadTabularNoHeader.getStatusCode()); + assertEquals(OK.getStatusCode(), anonDownloadTabularWithFormatName.getStatusCode()); int origSizeAnon = anonDownloadOriginal.getBody().asByteArray().length; int convertSizeAnon = anonDownloadConverted.getBody().asByteArray().length; int tabularSizeNoVarHeader = anonDownloadTabularNoHeader.getBody().asByteArray().length; + int tabularSizeWithFormatName = anonDownloadTabularWithFormatName.getBody().asByteArray().length; System.out.println("origSize: "+origSizeAnon + " | convertSize: " + convertSizeAnon + " | convertNoHeaderSize: " + tabularSizeNoVarHeader); - assertEquals(origSizeAnon, tabFile1SizeOriginal); - assertEquals(convertSizeAnon, tabFile1SizeConvertedWithVarHeader); - assertEquals(tabularSizeNoVarHeader, tabFile1SizeConverted); + assertEquals(tabFile1SizeOriginal, origSizeAnon); + assertEquals(tabFile1SizeConvertedWithVarHeader, convertSizeAnon); + assertEquals(tabFile1SizeConverted, tabularSizeNoVarHeader); + assertEquals(tabFile1SizeConvertedWithVarHeader, tabularSizeWithFormatName); //Not logged in restricted Response anonDownloadOriginalRestricted = UtilIT.downloadFileOriginal(tabFile3IdRestricted); diff --git a/src/test/java/edu/harvard/iq/dataverse/api/UtilIT.java b/src/test/java/edu/harvard/iq/dataverse/api/UtilIT.java index 9c9f0a3defd..553be5373e0 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/UtilIT.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/UtilIT.java @@ -655,6 +655,12 @@ static Response downloadFile(Integer fileId, String apiToken) { //.header(API_TOKEN_HTTP_HEADER, apiToken) .get("/api/access/datafile/" + fileId + "?key=" + apiToken); } + + static Response downloadTabularFile(Integer fileId) { + return given() + // this downloads a tabular file, explicitly requesting the format by name + .get("/api/access/datafile/" + fileId + "?format=tab"); + } static Response downloadFileOriginal(Integer fileId) { return given()