From 3831479aca73bd8fb5f833caa55ca36b8fcdff8a Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Wed, 8 Apr 2015 17:35:21 +0200 Subject: [PATCH] DATAREST-507 - Fixed IllegalArgumentException in RepositoryRestExceptionHandler. A lot of cleanups in RepositoryRestExceptionHandler: prefer static methods where possible. Removed delegates where only one client was calling the method. Consistent argument ordering with optional (nullable) parameters last. Silenced logging in unit tests for RepositoryRestExceptionHandler. --- .../RepositoryRestExceptionHandler.java | 54 ++++++++----------- ...positoryRestExceptionHandlerUnitTests.java | 37 ++++++++++++- 2 files changed, 59 insertions(+), 32 deletions(-) diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/RepositoryRestExceptionHandler.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/RepositoryRestExceptionHandler.java index c37e7092c..6dd4c06ed 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/RepositoryRestExceptionHandler.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/RepositoryRestExceptionHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2014 the original author or authors. + * Copyright 2014-2015 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -69,7 +69,7 @@ public class RepositoryRestExceptionHandler { */ @ExceptionHandler ResponseEntity handleNotFound(ResourceNotFoundException o_O) { - return notFound(); + return notFound(new HttpHeaders()); } /** @@ -80,7 +80,7 @@ public class RepositoryRestExceptionHandler { */ @ExceptionHandler ResponseEntity handleNotReadable(HttpMessageNotReadableException o_O) { - return badRequest(o_O); + return badRequest(new HttpHeaders(), o_O); } /** @@ -93,7 +93,8 @@ public class RepositoryRestExceptionHandler { @ExceptionHandler({ InvocationTargetException.class, IllegalArgumentException.class, ClassCastException.class, ConversionFailedException.class, NullPointerException.class }) ResponseEntity handleMiscFailures(Exception o_O) { - return errorResponse(null, HttpStatus.INTERNAL_SERVER_ERROR); + + return errorResponse(HttpStatus.INTERNAL_SERVER_ERROR, new HttpHeaders(), null); } /** @@ -105,9 +106,8 @@ public class RepositoryRestExceptionHandler { @ExceptionHandler ResponseEntity handleRepositoryConstraintViolationException( RepositoryConstraintViolationException o_O) { - - return response(new HttpHeaders(), new RepositoryConstraintViolationExceptionMessage(o_O, messageSourceAccessor), - HttpStatus.BAD_REQUEST); + return response(HttpStatus.BAD_REQUEST, new HttpHeaders(), new RepositoryConstraintViolationExceptionMessage(o_O, + messageSourceAccessor)); } /** @@ -118,7 +118,7 @@ public class RepositoryRestExceptionHandler { */ @ExceptionHandler({ OptimisticLockingFailureException.class, DataIntegrityViolationException.class }) ResponseEntity handleConflict(Exception o_O) { - return errorResponse(null, o_O, HttpStatus.CONFLICT); + return errorResponse(HttpStatus.CONFLICT, new HttpHeaders(), o_O); } /** @@ -134,7 +134,7 @@ public class RepositoryRestExceptionHandler { HttpHeaders headers = new HttpHeaders(); headers.setAllow(o_O.getSupportedHttpMethods()); - return new ResponseEntity(headers, HttpStatus.METHOD_NOT_ALLOWED); + return response(HttpStatus.METHOD_NOT_ALLOWED, headers); } /** @@ -147,45 +147,37 @@ public class RepositoryRestExceptionHandler { ResponseEntity handle(ETagDoesntMatchException o_O) { HttpHeaders headers = o_O.getExpectedETag().addTo(new HttpHeaders()); - return new ResponseEntity(headers, HttpStatus.PRECONDITION_FAILED); + return response(HttpStatus.PRECONDITION_FAILED, headers); } - private ResponseEntity notFound() { - return notFound(new HttpHeaders(), null); + private static ResponseEntity notFound(HttpHeaders headers) { + return response(HttpStatus.NOT_FOUND, headers, null); } - private ResponseEntity notFound(HttpHeaders headers, T body) { - return response(headers, body, HttpStatus.NOT_FOUND); + private static ResponseEntity badRequest(HttpHeaders headers, Exception throwable) { + return errorResponse(HttpStatus.BAD_REQUEST, headers, throwable); } - private ResponseEntity badRequest(T throwable) { - return badRequest(new HttpHeaders(), throwable); - } - - private ResponseEntity badRequest(HttpHeaders headers, T throwable) { - return errorResponse(headers, throwable, HttpStatus.BAD_REQUEST); - } - - private ResponseEntity errorResponse(T throwable, HttpStatus status) { - return errorResponse(new HttpHeaders(), throwable, status); - } - - private ResponseEntity errorResponse(HttpHeaders headers, - Exception exception, HttpStatus status) { + private static ResponseEntity errorResponse(HttpStatus status, HttpHeaders headers, + Exception exception) { if (null != exception && null != exception.getMessage()) { LOG.error(exception.getMessage(), exception); - return response(headers, new ExceptionMessage(exception), status); + return response(status, headers, new ExceptionMessage(exception)); } else { - return response(headers, null, status); + return response(status, headers, null); } } - public ResponseEntity response(HttpHeaders headers, T body, HttpStatus status) { + private static ResponseEntity response(HttpStatus status, HttpHeaders headers) { + return response(status, headers, null); + } + + private static ResponseEntity response(HttpStatus status, HttpHeaders headers, T body) { Assert.notNull(headers, "Headers must not be null!"); Assert.notNull(status, "HttpStatus must not be null!"); diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/RepositoryRestExceptionHandlerUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/RepositoryRestExceptionHandlerUnitTests.java index b21f2a237..290867837 100644 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/RepositoryRestExceptionHandlerUnitTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/RepositoryRestExceptionHandlerUnitTests.java @@ -18,12 +18,20 @@ package org.springframework.data.rest.webmvc; import static org.hamcrest.CoreMatchers.*; import static org.junit.Assert.*; +import org.junit.AfterClass; +import org.junit.BeforeClass; import org.junit.Test; +import org.slf4j.LoggerFactory; import org.springframework.context.support.StaticMessageSource; +import org.springframework.dao.DataIntegrityViolationException; import org.springframework.data.rest.webmvc.support.ExceptionMessage; +import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.http.converter.HttpMessageNotReadableException; +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; + /** * Unit tests for {@link RepositoryRestExceptionHandler}. * @@ -33,6 +41,22 @@ public class RepositoryRestExceptionHandlerUnitTests { static final RepositoryRestExceptionHandler HANDLER = new RepositoryRestExceptionHandler(new StaticMessageSource()); + static Logger logger; + static Level logLevel; + + @BeforeClass + public static void silenceLog() { + + logger = (Logger) LoggerFactory.getLogger(RepositoryRestExceptionHandler.class); + logLevel = logger.getLevel(); + logger.setLevel(Level.OFF); + } + + @AfterClass + public static void enableLogging() { + logger.setLevel(logLevel); + } + /** * @see DATAREST-427 */ @@ -42,6 +66,17 @@ public class RepositoryRestExceptionHandlerUnitTests { ResponseEntity result = HANDLER .handleNotReadable(new HttpMessageNotReadableException("Message!")); - assertThat(result.getStatusCode(), is(org.springframework.http.HttpStatus.BAD_REQUEST)); + assertThat(result.getStatusCode(), is(HttpStatus.BAD_REQUEST)); + } + + /** + * @see DATAREST-507 + */ + @Test + public void handlesConflictCorrectly() { + + ResponseEntity result = HANDLER.handleConflict(new DataIntegrityViolationException("Message!")); + + assertThat(result.getStatusCode(), is(HttpStatus.CONFLICT)); } }