From acf511ac0ef2f3151a0662a1d0437bf70d5463cb Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 2 Feb 2017 20:11:06 +0100 Subject: [PATCH] Polishing --- .../util/concurrent/ListenableFuture.java | 3 +- .../SettableListenableFutureTests.java | 25 +++++----- .../jdbc/datasource/DataSourceUtils.java | 16 +----- .../jms/connection/JmsTransactionManager.java | 5 +- .../support/OpenSessionInViewInterceptor.java | 3 +- .../AbstractPlatformTransactionManager.java | 49 +++---------------- .../web/accept/ContentNegotiationManager.java | 6 +-- .../ResponseEntityExceptionHandler.java | 7 ++- .../servlet/ComplexWebApplicationContext.java | 38 +++++++++----- ...letRequestMethodArgumentResolverTests.java | 4 +- 10 files changed, 63 insertions(+), 93 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/util/concurrent/ListenableFuture.java b/spring-core/src/main/java/org/springframework/util/concurrent/ListenableFuture.java index 5b84297388..9c7a0ec1df 100644 --- a/spring-core/src/main/java/org/springframework/util/concurrent/ListenableFuture.java +++ b/spring-core/src/main/java/org/springframework/util/concurrent/ListenableFuture.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2017 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. @@ -22,6 +22,7 @@ import java.util.concurrent.Future; * Extend {@link Future} with the capability to accept completion callbacks. * If the future has completed when the callback is added, the callback is * triggered immediately. + * *

Inspired by {@code com.google.common.util.concurrent.ListenableFuture}. * * @author Arjen Poutsma diff --git a/spring-core/src/test/java/org/springframework/util/concurrent/SettableListenableFutureTests.java b/spring-core/src/test/java/org/springframework/util/concurrent/SettableListenableFutureTests.java index 5e0a68e0c9..20f82a5d39 100644 --- a/spring-core/src/test/java/org/springframework/util/concurrent/SettableListenableFutureTests.java +++ b/spring-core/src/test/java/org/springframework/util/concurrent/SettableListenableFutureTests.java @@ -32,7 +32,6 @@ import static org.mockito.Mockito.*; * @author Mattias Severson * @author Juergen Hoeller */ -@SuppressWarnings({ "rawtypes", "unchecked" }) public class SettableListenableFutureTests { private final SettableListenableFuture settableListenableFuture = new SettableListenableFuture<>(); @@ -266,20 +265,20 @@ public class SettableListenableFutureTests { @Test public void cancelWithMayInterruptIfRunningTrueCallsOverriddenMethod() { - InterruptableSettableListenableFuture tested = new InterruptableSettableListenableFuture(); - assertTrue(tested.cancel(true)); - assertTrue(tested.calledInterruptTask()); - assertTrue(tested.isCancelled()); - assertTrue(tested.isDone()); + InterruptibleSettableListenableFuture interruptibleFuture = new InterruptibleSettableListenableFuture(); + assertTrue(interruptibleFuture.cancel(true)); + assertTrue(interruptibleFuture.calledInterruptTask()); + assertTrue(interruptibleFuture.isCancelled()); + assertTrue(interruptibleFuture.isDone()); } @Test public void cancelWithMayInterruptIfRunningFalseDoesNotCallOverriddenMethod() { - InterruptableSettableListenableFuture tested = new InterruptableSettableListenableFuture(); - assertTrue(tested.cancel(false)); - assertFalse(tested.calledInterruptTask()); - assertTrue(tested.isCancelled()); - assertTrue(tested.isDone()); + InterruptibleSettableListenableFuture interruptibleFuture = new InterruptibleSettableListenableFuture(); + assertTrue(interruptibleFuture.cancel(false)); + assertFalse(interruptibleFuture.calledInterruptTask()); + assertTrue(interruptibleFuture.isCancelled()); + assertTrue(interruptibleFuture.isDone()); } @Test @@ -350,6 +349,7 @@ public class SettableListenableFutureTests { } @Test + @SuppressWarnings({"rawtypes", "unchecked"}) public void cancelDoesNotNotifyCallbacksOnSet() { ListenableFutureCallback callback = mock(ListenableFutureCallback.class); settableListenableFuture.addCallback(callback); @@ -366,6 +366,7 @@ public class SettableListenableFutureTests { } @Test + @SuppressWarnings({"rawtypes", "unchecked"}) public void cancelDoesNotNotifyCallbacksOnSetException() { ListenableFutureCallback callback = mock(ListenableFutureCallback.class); settableListenableFuture.addCallback(callback); @@ -382,7 +383,7 @@ public class SettableListenableFutureTests { } - private static class InterruptableSettableListenableFuture extends SettableListenableFuture { + private static class InterruptibleSettableListenableFuture extends SettableListenableFuture { private boolean interrupted = false; diff --git a/spring-jdbc/src/main/java/org/springframework/jdbc/datasource/DataSourceUtils.java b/spring-jdbc/src/main/java/org/springframework/jdbc/datasource/DataSourceUtils.java index 5cad3aa6f0..eb766badbb 100644 --- a/spring-jdbc/src/main/java/org/springframework/jdbc/datasource/DataSourceUtils.java +++ b/spring-jdbc/src/main/java/org/springframework/jdbc/datasource/DataSourceUtils.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2017 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. @@ -154,7 +154,7 @@ public abstract class DataSourceUtils { } con.setReadOnly(true); } - catch (SQLException ex) { + catch (SQLException | RuntimeException ex) { Throwable exToCheck = ex; while (exToCheck != null) { if (exToCheck.getClass().getSimpleName().contains("Timeout")) { @@ -166,18 +166,6 @@ public abstract class DataSourceUtils { // "read-only not supported" SQLException -> ignore, it's just a hint anyway logger.debug("Could not set JDBC Connection read-only", ex); } - catch (RuntimeException ex) { - Throwable exToCheck = ex; - while (exToCheck != null) { - if (exToCheck.getClass().getSimpleName().contains("Timeout")) { - // Assume it's a connection timeout that would otherwise get lost: e.g. from Hibernate - throw ex; - } - exToCheck = exToCheck.getCause(); - } - // "read-only not supported" UnsupportedOperationException -> ignore, it's just a hint anyway - logger.debug("Could not set JDBC Connection read-only", ex); - } } // Apply specific isolation level, if any. diff --git a/spring-jms/src/main/java/org/springframework/jms/connection/JmsTransactionManager.java b/spring-jms/src/main/java/org/springframework/jms/connection/JmsTransactionManager.java index d098a056b9..bf44f8fa01 100644 --- a/spring-jms/src/main/java/org/springframework/jms/connection/JmsTransactionManager.java +++ b/spring-jms/src/main/java/org/springframework/jms/connection/JmsTransactionManager.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2017 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. @@ -226,8 +226,7 @@ public class JmsTransactionManager extends AbstractPlatformTransactionManager @Override protected void doResume(Object transaction, Object suspendedResources) { - JmsResourceHolder conHolder = (JmsResourceHolder) suspendedResources; - TransactionSynchronizationManager.bindResource(getConnectionFactory(), conHolder); + TransactionSynchronizationManager.bindResource(getConnectionFactory(), suspendedResources); } @Override diff --git a/spring-orm/src/main/java/org/springframework/orm/hibernate5/support/OpenSessionInViewInterceptor.java b/spring-orm/src/main/java/org/springframework/orm/hibernate5/support/OpenSessionInViewInterceptor.java index 5b2ff0bfb5..d44cc19109 100644 --- a/spring-orm/src/main/java/org/springframework/orm/hibernate5/support/OpenSessionInViewInterceptor.java +++ b/spring-orm/src/main/java/org/springframework/orm/hibernate5/support/OpenSessionInViewInterceptor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2017 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. @@ -144,7 +144,6 @@ public class OpenSessionInViewInterceptor implements AsyncWebRequestInterceptor (SessionHolder) TransactionSynchronizationManager.unbindResource(getSessionFactory()); logger.debug("Closing Hibernate Session in OpenSessionInViewInterceptor"); SessionFactoryUtils.closeSession(sessionHolder.getSession()); - } } diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/AbstractPlatformTransactionManager.java b/spring-tx/src/main/java/org/springframework/transaction/support/AbstractPlatformTransactionManager.java index ed3896f3a0..52450c09c1 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/AbstractPlatformTransactionManager.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/AbstractPlatformTransactionManager.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2017 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. @@ -374,14 +374,10 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran prepareSynchronization(status, definition); return status; } - catch (RuntimeException ex) { + catch (RuntimeException | Error ex) { resume(null, suspendedResources); throw ex; } - catch (Error err) { - resume(null, suspendedResources); - throw err; - } } else { // Create "empty" transaction: no actual transaction, but potentially synchronization. @@ -430,14 +426,10 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran prepareSynchronization(status, definition); return status; } - catch (RuntimeException beginEx) { + catch (RuntimeException | Error beginEx) { resumeAfterBeginException(transaction, suspendedResources, beginEx); throw beginEx; } - catch (Error beginErr) { - resumeAfterBeginException(transaction, suspendedResources, beginErr); - throw beginErr; - } } if (definition.getPropagationBehavior() == TransactionDefinition.PROPAGATION_NESTED) { @@ -589,16 +581,11 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran return new SuspendedResourcesHolder( suspendedResources, suspendedSynchronizations, name, readOnly, isolationLevel, wasActive); } - catch (RuntimeException ex) { + catch (RuntimeException | Error ex) { // doSuspend failed - original transaction is still active... doResumeSynchronization(suspendedSynchronizations); throw ex; } - catch (Error err) { - // doSuspend failed - original transaction is still active... - doResumeSynchronization(suspendedSynchronizations); - throw err; - } } else if (transaction != null) { // Transaction active but no synchronization active. @@ -650,14 +637,10 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran try { resume(transaction, suspendedResources); } - catch (RuntimeException resumeEx) { + catch (RuntimeException | Error resumeEx) { logger.error(exMessage, beginEx); throw resumeEx; } - catch (Error resumeErr) { - logger.error(exMessage, beginEx); - throw resumeErr; - } } /** @@ -782,20 +765,13 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran } throw ex; } - catch (RuntimeException ex) { + catch (RuntimeException | Error ex) { if (!beforeCompletionInvoked) { triggerBeforeCompletion(status); } doRollbackOnCommitException(status, ex); throw ex; } - catch (Error err) { - if (!beforeCompletionInvoked) { - triggerBeforeCompletion(status); - } - doRollbackOnCommitException(status, err); - throw err; - } // Trigger afterCommit callbacks, with an exception thrown there // propagated to callers but the transaction still considered as committed. @@ -869,14 +845,10 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran logger.debug("Should roll back transaction but cannot - no transaction available"); } } - catch (RuntimeException ex) { + catch (RuntimeException | Error ex) { triggerAfterCompletion(status, TransactionSynchronization.STATUS_UNKNOWN); throw ex; } - catch (Error err) { - triggerAfterCompletion(status, TransactionSynchronization.STATUS_UNKNOWN); - throw err; - } triggerAfterCompletion(status, TransactionSynchronization.STATUS_ROLLED_BACK); } finally { @@ -906,16 +878,11 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran doSetRollbackOnly(status); } } - catch (RuntimeException rbex) { + catch (RuntimeException | Error rbex) { logger.error("Commit exception overridden by rollback exception", ex); triggerAfterCompletion(status, TransactionSynchronization.STATUS_UNKNOWN); throw rbex; } - catch (Error rberr) { - logger.error("Commit exception overridden by rollback exception", ex); - triggerAfterCompletion(status, TransactionSynchronization.STATUS_UNKNOWN); - throw rberr; - } triggerAfterCompletion(status, TransactionSynchronization.STATUS_ROLLED_BACK); } diff --git a/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManager.java b/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManager.java index a37475e823..2fd4e43de7 100644 --- a/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManager.java +++ b/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManager.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2017 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. @@ -118,9 +118,7 @@ public class ContentNegotiationManager implements ContentNegotiationStrategy, Me } @Override - public List resolveMediaTypes(NativeWebRequest request) - throws HttpMediaTypeNotAcceptableException { - + public List resolveMediaTypes(NativeWebRequest request) throws HttpMediaTypeNotAcceptableException { for (ContentNegotiationStrategy strategy : this.strategies) { List mediaTypes = strategy.resolveMediaTypes(request); if (mediaTypes.isEmpty() || mediaTypes.equals(MEDIA_TYPE_ALL)) { diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java index 7f476b5414..c5eae7dadc 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2017 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. @@ -454,10 +454,13 @@ public abstract class ResponseEntityExceptionHandler { HttpServletRequest request = servletRequest.getNativeRequest(HttpServletRequest.class); HttpServletResponse response = servletRequest.getNativeResponse(HttpServletResponse.class); if (response.isCommitted()) { - logger.error("Async timeout for " + request.getMethod() + " [" + request.getRequestURI() + "]"); + if (logger.isErrorEnabled()) { + logger.error("Async timeout for " + request.getMethod() + " [" + request.getRequestURI() + "]"); + } return null; } } + return handleExceptionInternal(ex, null, headers, status, webRequest); } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/ComplexWebApplicationContext.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/ComplexWebApplicationContext.java index cacb3618ef..8992bf7a01 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/ComplexWebApplicationContext.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/ComplexWebApplicationContext.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2017 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. @@ -87,7 +87,7 @@ public class ComplexWebApplicationContext extends StaticWebApplicationContext { ThemeChangeInterceptor interceptor4 = new ThemeChangeInterceptor(); interceptor4.setParamName("theme2"); UserRoleAuthorizationInterceptor interceptor5 = new UserRoleAuthorizationInterceptor(); - interceptor5.setAuthorizedRoles(new String[] {"role1", "role2"}); + interceptor5.setAuthorizedRoles("role1", "role2"); List interceptors = new ArrayList<>(); interceptors.add(interceptor5); @@ -100,8 +100,7 @@ public class ComplexWebApplicationContext extends StaticWebApplicationContext { interceptors.add(new MyWebRequestInterceptor()); MutablePropertyValues pvs = new MutablePropertyValues(); - pvs.add( - "mappings", "/view.do=viewHandler\n/locale.do=localeHandler\nloc.do=anotherLocaleHandler"); + pvs.add("mappings", "/view.do=viewHandler\n/locale.do=localeHandler\nloc.do=anotherLocaleHandler"); pvs.add("interceptors", interceptors); registerSingleton("myUrlMapping1", SimpleUrlHandlerMapping.class, pvs); @@ -124,7 +123,7 @@ public class ComplexWebApplicationContext extends StaticWebApplicationContext { registerSingleton("noviewController", NoViewController.class); pvs = new MutablePropertyValues(); - pvs.add("order", new Integer(0)); + pvs.add("order", 0); pvs.add("basename", "org.springframework.web.servlet.complexviews"); registerSingleton("viewResolver", ResourceBundleViewResolver.class, pvs); @@ -150,8 +149,8 @@ public class ComplexWebApplicationContext extends StaticWebApplicationContext { pvs = new MutablePropertyValues(); pvs.add("order", "1"); pvs.add("exceptionMappings", - "java.lang.IllegalAccessException=failed2\n" + - "ServletRequestBindingException=failed3"); + "java.lang.IllegalAccessException=failed2\n" + + "ServletRequestBindingException=failed3"); pvs.add("defaultErrorView", "failed0"); registerSingleton("exceptionResolver1", SimpleMappingExceptionResolver.class, pvs); @@ -237,11 +236,11 @@ public class ComplexWebApplicationContext extends StaticWebApplicationContext { } - public static interface MyHandler { + public interface MyHandler { - public void doSomething(HttpServletRequest request) throws ServletException, IllegalAccessException; + void doSomething(HttpServletRequest request) throws ServletException, IllegalAccessException; - public long lastModified(); + long lastModified(); } @@ -259,7 +258,8 @@ public class ComplexWebApplicationContext extends StaticWebApplicationContext { @Override public ModelAndView handle(HttpServletRequest request, HttpServletResponse response, Object delegate) - throws ServletException, IllegalAccessException { + throws ServletException, IllegalAccessException { + ((MyHandler) delegate).doSomething(request); return null; } @@ -295,7 +295,8 @@ public class ComplexWebApplicationContext extends StaticWebApplicationContext { @Override public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) - throws ServletException { + throws ServletException { + if (request.getAttribute("test2") != null) { throw new ServletException("Wrong interceptor order"); } @@ -309,6 +310,7 @@ public class ComplexWebApplicationContext extends StaticWebApplicationContext { public void postHandle( HttpServletRequest request, HttpServletResponse response, Object handler, ModelAndView modelAndView) throws ServletException { + if (request.getAttribute("test2x") != null) { throw new ServletException("Wrong interceptor order"); } @@ -322,9 +324,13 @@ public class ComplexWebApplicationContext extends StaticWebApplicationContext { public void afterCompletion( HttpServletRequest request, HttpServletResponse response, Object handler, Exception ex) throws ServletException { + if (request.getAttribute("test2y") != null) { throw new ServletException("Wrong interceptor order"); } + if (request.getAttribute("test1y") == null) { + throw new ServletException("afterCompletion invoked twice"); + } request.removeAttribute("test1y"); } } @@ -334,7 +340,8 @@ public class ComplexWebApplicationContext extends StaticWebApplicationContext { @Override public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) - throws ServletException { + throws ServletException { + if (request.getAttribute("test1x") == null) { throw new ServletException("Wrong interceptor order"); } @@ -351,6 +358,7 @@ public class ComplexWebApplicationContext extends StaticWebApplicationContext { public void postHandle( HttpServletRequest request, HttpServletResponse response, Object handler, ModelAndView modelAndView) throws ServletException { + if (request.getParameter("noView") != null) { modelAndView.clear(); } @@ -367,9 +375,13 @@ public class ComplexWebApplicationContext extends StaticWebApplicationContext { public void afterCompletion( HttpServletRequest request, HttpServletResponse response, Object handler, Exception ex) throws Exception { + if (request.getAttribute("test1y") == null) { throw new ServletException("Wrong interceptor order"); } + if (request.getAttribute("test2y") == null) { + throw new ServletException("afterCompletion invoked twice"); + } request.removeAttribute("test2y"); } } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java index d96edc5bdb..b8210fa23f 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletRequestMethodArgumentResolverTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2017 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. @@ -60,6 +60,7 @@ public class ServletRequestMethodArgumentResolverTests { private MockHttpServletRequest servletRequest; + @Before public void setUp() throws Exception { method = getClass().getMethod("supportedParams", ServletRequest.class, MultipartRequest.class, @@ -70,6 +71,7 @@ public class ServletRequestMethodArgumentResolverTests { webRequest = new ServletWebRequest(servletRequest, new MockHttpServletResponse()); } + @Test public void servletRequest() throws Exception { MethodParameter servletRequestParameter = new MethodParameter(method, 0);