diff --git a/dspace-spring-rest/src/main/java/org/dspace/app/rest/BitstreamContentRestController.java b/dspace-spring-rest/src/main/java/org/dspace/app/rest/BitstreamContentRestController.java index b0a1ecce15..1d0e034846 100644 --- a/dspace-spring-rest/src/main/java/org/dspace/app/rest/BitstreamContentRestController.java +++ b/dspace-spring-rest/src/main/java/org/dspace/app/rest/BitstreamContentRestController.java @@ -16,7 +16,7 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.ws.rs.core.Response; -import org.apache.http.auth.AUTH; +import org.apache.catalina.connector.ClientAbortException; import org.apache.log4j.Logger; import org.dspace.app.rest.model.BitstreamRest; import org.dspace.app.rest.utils.ContextUtil; @@ -27,10 +27,8 @@ import org.dspace.content.Bitstream; import org.dspace.content.service.BitstreamService; import org.dspace.core.Constants; import org.dspace.core.Context; -import org.dspace.services.ConfigurationService; import org.dspace.services.EventService; import org.dspace.usage.UsageEvent; -import org.springframework.beans.factory.InitializingBean; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestMapping; @@ -46,36 +44,29 @@ import org.springframework.web.bind.annotation.RestController; */ @RestController @RequestMapping("/api/"+BitstreamRest.CATEGORY +"/"+ BitstreamRest.PLURAL_NAME + "/{uuid:[0-9a-fxA-FX]{8}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{12}}/content") -public class BitstreamContentRestController implements InitializingBean { +public class BitstreamContentRestController { private static final Logger log = Logger.getLogger(BitstreamContentRestController.class); + //Most file systems are configured to use block sizes of 4096 or 8192 and our buffer should be a multiple of that. + private static final int BUFFER_SIZE = 4096 * 10; + @Autowired private BitstreamService bitstreamService; @Autowired private EventService eventService; - @Autowired - private ConfigurationService configurationService; - @Autowired private AuthorizeService authorizeService; - private int bufferSize; - - @Override - public void afterPropertiesSet() throws Exception { - bufferSize = configurationService.getIntProperty("bitstream-download.buffer.size", -1); - } - @RequestMapping(method = {RequestMethod.GET, RequestMethod.HEAD}) public void retrieve(@PathVariable UUID uuid, HttpServletResponse response, HttpServletRequest request) throws IOException, SQLException, AuthorizeException { Context context = ContextUtil.obtainContext(request); - Bitstream bit = getBitstreamIfAuthorized(context, uuid, response); + Bitstream bit = getBitstream(context, uuid, response); if (bit == null) { //The bitstream was not found or we're not authorized to read it. return; @@ -89,7 +80,7 @@ public class BitstreamContentRestController implements InitializingBean { MultipartFileSender sender = MultipartFileSender .fromInputStream(is) - .withBufferSize(bufferSize) + .withBufferSize(BUFFER_SIZE) .withFileName(bit.getName()) .withLength(bit.getSize()) .withChecksum(bit.getChecksum()) @@ -113,21 +104,17 @@ public class BitstreamContentRestController implements InitializingBean { bit)); } - } catch(IOException ex) { + } catch(ClientAbortException ex) { log.debug("Client aborted the request before the download was completed. Client is probably switching to a Range request.", ex); - } catch (Exception e) { - log.error(e.getMessage(), e); - response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, e.getMessage()); } } - private Bitstream getBitstreamIfAuthorized(Context context, @PathVariable UUID uuid, HttpServletResponse response) throws SQLException, IOException, AuthorizeException { + private Bitstream getBitstream(Context context, @PathVariable UUID uuid, HttpServletResponse response) throws SQLException, IOException, AuthorizeException { Bitstream bit = bitstreamService.find(context, uuid); if (bit == null) { response.sendError(HttpServletResponse.SC_NOT_FOUND); } else { authorizeService.authorizeAction(context, bit, Constants.READ); - } return bit; diff --git a/dspace-spring-rest/src/main/java/org/dspace/app/rest/exception/DSpaceApiExceptionControllerAdvice.java b/dspace-spring-rest/src/main/java/org/dspace/app/rest/exception/DSpaceApiExceptionControllerAdvice.java index 33f022fee0..e8cd7eab71 100644 --- a/dspace-spring-rest/src/main/java/org/dspace/app/rest/exception/DSpaceApiExceptionControllerAdvice.java +++ b/dspace-spring-rest/src/main/java/org/dspace/app/rest/exception/DSpaceApiExceptionControllerAdvice.java @@ -1,10 +1,21 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ package org.dspace.app.rest.exception; +import static org.springframework.web.servlet.DispatcherServlet.EXCEPTION_ATTRIBUTE; + +import java.io.IOException; import java.sql.SQLException; import java.util.Date; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import org.dspace.authorize.AuthorizeException; import org.springframework.http.HttpStatus; @@ -14,21 +25,39 @@ import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.ResponseBody; import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler; +/** + * This Controller advice will handle all exceptions thrown by the DSpace API module + * + * @author Atmire NV (info at atmire dot com) + * + */ @ControllerAdvice public class DSpaceApiExceptionControllerAdvice extends ResponseEntityExceptionHandler{ @ExceptionHandler(AuthorizeException.class) - @ResponseBody - protected ResponseEntity handleAuthorizeException(HttpServletRequest request, Exception e) { - return new ResponseEntity(e.getMessage(), HttpStatus.UNAUTHORIZED); + protected void handleAuthorizeException(HttpServletRequest request, HttpServletResponse response, Exception ex) throws IOException { + sendErrorResponse(request, response, ex, ex.getMessage(), HttpServletResponse.SC_UNAUTHORIZED); } @ExceptionHandler(SQLException.class) - @ResponseBody - protected ResponseEntity handleSQLException(HttpServletRequest request, Exception e) { - String errorMessage = "An internal database error occurred. Please contact the repository administrator. Timestamp: " + new Date().toString(); - return new ResponseEntity(errorMessage, HttpStatus.INTERNAL_SERVER_ERROR); + protected void handleSQLException(HttpServletRequest request, HttpServletResponse response, Exception ex) throws IOException { + sendErrorResponse(request, response, ex, + "An internal database error occurred", HttpServletResponse.SC_INTERNAL_SERVER_ERROR); } + @ExceptionHandler(IOException.class) + protected void handleIOException(HttpServletRequest request, HttpServletResponse response, Exception ex) throws IOException { + sendErrorResponse(request, response, ex, + "An internal read or write operation failed (IO Exception)", HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + } + + private void sendErrorResponse(final HttpServletRequest request, final HttpServletResponse response, + final Exception ex, final String message, final int statusCode) throws IOException { + //Make sure Spring picks up this exception + request.setAttribute(EXCEPTION_ATTRIBUTE, ex); + + //Exception properties will be set by org.springframework.boot.web.support.ErrorPageFilter + response.sendError(statusCode, message); + } } diff --git a/dspace-spring-rest/src/main/java/org/dspace/app/rest/utils/MultipartFileSender.java b/dspace-spring-rest/src/main/java/org/dspace/app/rest/utils/MultipartFileSender.java index 447a9cab63..329cacf1ae 100644 --- a/dspace-spring-rest/src/main/java/org/dspace/app/rest/utils/MultipartFileSender.java +++ b/dspace-spring-rest/src/main/java/org/dspace/app/rest/utils/MultipartFileSender.java @@ -134,7 +134,7 @@ public class MultipartFileSender { return this; } - public void serveResource() throws Exception { + public void serveResource() throws IOException { // Validate and process range ------------------------------------------------------------- diff --git a/dspace-spring-rest/src/test/java/org/dspace/app/rest/BitstreamContentRestControllerIT.java b/dspace-spring-rest/src/test/java/org/dspace/app/rest/BitstreamContentRestControllerIT.java index 3b3ac86e83..9f84fc0c9f 100644 --- a/dspace-spring-rest/src/test/java/org/dspace/app/rest/BitstreamContentRestControllerIT.java +++ b/dspace-spring-rest/src/test/java/org/dspace/app/rest/BitstreamContentRestControllerIT.java @@ -10,9 +10,7 @@ package org.dspace.app.rest; import static org.junit.Assert.assertEquals; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.head; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; import java.io.IOException; import java.io.InputStream; @@ -238,9 +236,7 @@ public class BitstreamContentRestControllerIT extends AbstractControllerIntegrat getClient().perform(get("/api/core/bitstreams/" + bitstream.getID() + "/content")) //** THEN ** - .andExpect(status().isUnauthorized()) - //The response should not contain any content. - .andExpect(content().bytes(new byte[0])); + .andExpect(status().isUnauthorized()); //An unauthorized request should not log statistics checkNumberOfStatsRecords(bitstream, 0); @@ -286,9 +282,7 @@ public class BitstreamContentRestControllerIT extends AbstractControllerIntegrat getClient().perform(get("/api/core/bitstreams/" + bitstream.getID() + "/content")) //** THEN ** - .andExpect(status().isUnauthorized()) - //The response should not contain any content. - .andExpect(content().bytes(new byte[0])); + .andExpect(status().isUnauthorized()); //An unauthorized request should not log statistics checkNumberOfStatsRecords(bitstream, 0); diff --git a/dspace-spring-rest/src/test/java/org/dspace/app/rest/test/AbstractControllerIntegrationTest.java b/dspace-spring-rest/src/test/java/org/dspace/app/rest/test/AbstractControllerIntegrationTest.java index 65f8301318..256b783f14 100644 --- a/dspace-spring-rest/src/test/java/org/dspace/app/rest/test/AbstractControllerIntegrationTest.java +++ b/dspace-spring-rest/src/test/java/org/dspace/app/rest/test/AbstractControllerIntegrationTest.java @@ -22,6 +22,7 @@ import org.junit.Assert; import org.junit.runner.RunWith; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.web.support.ErrorPageFilter; import org.springframework.hateoas.MediaTypes; import org.springframework.http.MediaType; import org.springframework.http.converter.HttpMessageConverter; @@ -84,6 +85,7 @@ public class AbstractControllerIntegrationTest extends AbstractIntegrationTestWi //Always log the repsonse to debug .alwaysDo(MockMvcResultHandlers.log()) //Add all filter implementations + .addFilters(new ErrorPageFilter()) .addFilters(requestFilters.toArray(new Filter[requestFilters.size()])) .build(); } diff --git a/dspace/config/dspace.cfg b/dspace/config/dspace.cfg index d9d238d017..8c331400e6 100644 --- a/dspace/config/dspace.cfg +++ b/dspace/config/dspace.cfg @@ -1953,9 +1953,6 @@ mail.helpdesk = ${mail.admin} # Should all Request Copy emails go to the helpdesk instead of the item submitter? request.item.helpdesk.override = false -# 2 MB -bitstream-download.buffer.size = 2097152 - #------------------------------------------------------------------#