DS-3651: Correct ControllerAdvice exception handling + tests

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;

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 void handleAuthorizeException(HttpServletRequest request, HttpServletResponse response, Exception ex) throws IOException {
-    protected ResponseEntity<String> handleAuthorizeException(HttpServletRequest request, Exception e) {
+        sendErrorResponse(request, response, ex, ex.getMessage(), HttpServletResponse.SC_UNAUTHORIZED);
-        return new ResponseEntity(e.getMessage(), HttpStatus.UNAUTHORIZED);
     }
 
     @ExceptionHandler(SQLException.class)
-    @ResponseBody
+    protected void handleSQLException(HttpServletRequest request, HttpServletResponse response, Exception ex) throws IOException {
-    protected ResponseEntity<String> handleSQLException(HttpServletRequest request, Exception e) {
+        sendErrorResponse(request, response, ex,
-        String errorMessage = "An internal database error occurred. Please contact the repository administrator. Timestamp: " + new Date().toString();
+                "An internal database error occurred", HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
-        return new ResponseEntity(errorMessage, HttpStatus.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);
+    }
 
 }

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 -------------------------------------------------------------
 

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.*;
-import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header;
-import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
 
 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())
+                    .andExpect(status().isUnauthorized());
-                    //The response should not contain any content.
-                    .andExpect(content().bytes(new byte[0]));
 
             //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())
+                    .andExpect(status().isUnauthorized());
-                    //The response should not contain any content.
-                    .andExpect(content().bytes(new byte[0]));
 
             //An unauthorized request should not log statistics
             checkNumberOfStatsRecords(bitstream, 0);

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();
     }

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
-
 
 
 #------------------------------------------------------------------#