From 535103cd52b58f36618f0facbe938cca4628d32f Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Tue, 20 Jun 2017 16:50:35 +0200 Subject: [PATCH] Fine-tune HTTP/RMI Invoker exception handling Issue: SPR-15684 --- .../rmi/JndiRmiClientInterceptor.java | 8 ++--- .../remoting/rmi/JndiRmiProxyFactoryBean.java | 10 +++---- .../RemoteInvocationSerializingExporter.java | 8 +++-- .../remoting/rmi/RmiProxyFactoryBean.java | 10 +++---- .../remoting/support/RemoteAccessor.java | 9 +++--- .../remoting/support/RemoteExporter.java | 30 +++++++++---------- .../remoting/JmsInvokerProxyFactoryBean.java | 8 ++--- .../AbstractHttpInvokerRequestExecutor.java | 3 +- ...pComponentsHttpInvokerRequestExecutor.java | 7 ++++- .../HttpInvokerProxyFactoryBean.java | 10 +++---- .../SimpleHttpInvokerRequestExecutor.java | 7 +++-- .../jaxws/JaxWsPortClientInterceptor.java | 6 ++-- 12 files changed, 61 insertions(+), 55 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/remoting/rmi/JndiRmiClientInterceptor.java b/spring-context/src/main/java/org/springframework/remoting/rmi/JndiRmiClientInterceptor.java index fc8c6f8dee..160f53cb26 100644 --- a/spring-context/src/main/java/org/springframework/remoting/rmi/JndiRmiClientInterceptor.java +++ b/spring-context/src/main/java/org/springframework/remoting/rmi/JndiRmiClientInterceptor.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. @@ -35,6 +35,7 @@ import org.springframework.remoting.RemoteLookupFailureException; import org.springframework.remoting.support.DefaultRemoteInvocationFactory; import org.springframework.remoting.support.RemoteInvocation; import org.springframework.remoting.support.RemoteInvocationFactory; +import org.springframework.util.Assert; /** * {@link org.aopalliance.intercept.MethodInterceptor} for accessing RMI services @@ -97,9 +98,8 @@ public class JndiRmiClientInterceptor extends JndiObjectLocator implements Metho * but can also be optional if the lookup returns a typed stub. */ public void setServiceInterface(Class serviceInterface) { - if (serviceInterface != null && !serviceInterface.isInterface()) { - throw new IllegalArgumentException("'serviceInterface' must be an interface"); - } + Assert.notNull(serviceInterface, "'serviceInterface' must not be null"); + Assert.isTrue(serviceInterface.isInterface(), "'serviceInterface' must be an interface"); this.serviceInterface = serviceInterface; } diff --git a/spring-context/src/main/java/org/springframework/remoting/rmi/JndiRmiProxyFactoryBean.java b/spring-context/src/main/java/org/springframework/remoting/rmi/JndiRmiProxyFactoryBean.java index 645251429c..d2bed35957 100644 --- a/spring-context/src/main/java/org/springframework/remoting/rmi/JndiRmiProxyFactoryBean.java +++ b/spring-context/src/main/java/org/springframework/remoting/rmi/JndiRmiProxyFactoryBean.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 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. @@ -21,6 +21,7 @@ import javax.naming.NamingException; import org.springframework.aop.framework.ProxyFactory; import org.springframework.beans.factory.BeanClassLoaderAware; import org.springframework.beans.factory.FactoryBean; +import org.springframework.util.Assert; import org.springframework.util.ClassUtils; /** @@ -77,10 +78,9 @@ public class JndiRmiProxyFactoryBean extends JndiRmiClientInterceptor @Override public void afterPropertiesSet() throws NamingException { super.afterPropertiesSet(); - if (getServiceInterface() == null) { - throw new IllegalArgumentException("Property 'serviceInterface' is required"); - } - this.serviceProxy = new ProxyFactory(getServiceInterface(), this).getProxy(this.beanClassLoader); + Class ifc = getServiceInterface(); + Assert.notNull(ifc, "Property 'serviceInterface' is required"); + this.serviceProxy = new ProxyFactory(ifc, this).getProxy(this.beanClassLoader); } diff --git a/spring-context/src/main/java/org/springframework/remoting/rmi/RemoteInvocationSerializingExporter.java b/spring-context/src/main/java/org/springframework/remoting/rmi/RemoteInvocationSerializingExporter.java index 6e0c08466a..599498fe74 100644 --- a/spring-context/src/main/java/org/springframework/remoting/rmi/RemoteInvocationSerializingExporter.java +++ b/spring-context/src/main/java/org/springframework/remoting/rmi/RemoteInvocationSerializingExporter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 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. @@ -107,7 +107,9 @@ public abstract class RemoteInvocationSerializingExporter extends RemoteInvocati } protected final Object getProxy() { - Assert.notNull(this.proxy, ClassUtils.getShortName(getClass()) + " has not been initialized"); + if (this.proxy == null) { + throw new IllegalStateException(ClassUtils.getShortName(getClass()) + " has not been initialized"); + } return this.proxy; } @@ -142,7 +144,7 @@ public abstract class RemoteInvocationSerializingExporter extends RemoteInvocati Object obj = ois.readObject(); if (!(obj instanceof RemoteInvocation)) { throw new RemoteException("Deserialized object needs to be assignable to type [" + - RemoteInvocation.class.getName() + "]: " + obj); + RemoteInvocation.class.getName() + "]: " + ClassUtils.getDescriptiveType(obj)); } return (RemoteInvocation) obj; } diff --git a/spring-context/src/main/java/org/springframework/remoting/rmi/RmiProxyFactoryBean.java b/spring-context/src/main/java/org/springframework/remoting/rmi/RmiProxyFactoryBean.java index 139c69a3f6..4526ae7a26 100644 --- a/spring-context/src/main/java/org/springframework/remoting/rmi/RmiProxyFactoryBean.java +++ b/spring-context/src/main/java/org/springframework/remoting/rmi/RmiProxyFactoryBean.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. @@ -19,6 +19,7 @@ package org.springframework.remoting.rmi; import org.springframework.aop.framework.ProxyFactory; import org.springframework.beans.factory.BeanClassLoaderAware; import org.springframework.beans.factory.FactoryBean; +import org.springframework.util.Assert; /** * {@link FactoryBean} for RMI proxies, supporting both conventional RMI services @@ -65,10 +66,9 @@ public class RmiProxyFactoryBean extends RmiClientInterceptor implements Factory @Override public void afterPropertiesSet() { super.afterPropertiesSet(); - if (getServiceInterface() == null) { - throw new IllegalArgumentException("Property 'serviceInterface' is required"); - } - this.serviceProxy = new ProxyFactory(getServiceInterface(), this).getProxy(getBeanClassLoader()); + Class ifc = getServiceInterface(); + Assert.notNull(ifc, "Property 'serviceInterface' is required"); + this.serviceProxy = new ProxyFactory(ifc, this).getProxy(getBeanClassLoader()); } diff --git a/spring-context/src/main/java/org/springframework/remoting/support/RemoteAccessor.java b/spring-context/src/main/java/org/springframework/remoting/support/RemoteAccessor.java index 7657a65593..19d98104ec 100644 --- a/spring-context/src/main/java/org/springframework/remoting/support/RemoteAccessor.java +++ b/spring-context/src/main/java/org/springframework/remoting/support/RemoteAccessor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 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. @@ -16,6 +16,8 @@ package org.springframework.remoting.support; +import org.springframework.util.Assert; + /** * Abstract base class for classes that access a remote service. * Provides a "serviceInterface" bean property. @@ -46,9 +48,8 @@ public abstract class RemoteAccessor extends RemotingSupport { * but can also be optional if the lookup returns a typed proxy. */ public void setServiceInterface(Class serviceInterface) { - if (serviceInterface != null && !serviceInterface.isInterface()) { - throw new IllegalArgumentException("'serviceInterface' must be an interface"); - } + Assert.notNull(serviceInterface, "'serviceInterface' must not be null"); + Assert.isTrue(serviceInterface.isInterface(), "'serviceInterface' must be an interface"); this.serviceInterface = serviceInterface; } diff --git a/spring-context/src/main/java/org/springframework/remoting/support/RemoteExporter.java b/spring-context/src/main/java/org/springframework/remoting/support/RemoteExporter.java index ad84641953..e1c2bdfa4f 100644 --- a/spring-context/src/main/java/org/springframework/remoting/support/RemoteExporter.java +++ b/spring-context/src/main/java/org/springframework/remoting/support/RemoteExporter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 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. @@ -19,6 +19,7 @@ package org.springframework.remoting.support; import org.springframework.aop.framework.ProxyFactory; import org.springframework.aop.framework.adapter.AdvisorAdapterRegistry; import org.springframework.aop.framework.adapter.GlobalAdvisorAdapterRegistry; +import org.springframework.util.Assert; import org.springframework.util.ClassUtils; /** @@ -63,9 +64,8 @@ public abstract class RemoteExporter extends RemotingSupport { * The interface must be suitable for the particular service and remoting strategy. */ public void setServiceInterface(Class serviceInterface) { - if (serviceInterface != null && !serviceInterface.isInterface()) { - throw new IllegalArgumentException("'serviceInterface' must be an interface"); - } + Assert.notNull(serviceInterface, "'serviceInterface' must not be null"); + Assert.isTrue(serviceInterface.isInterface(), "'serviceInterface' must be an interface"); this.serviceInterface = serviceInterface; } @@ -89,7 +89,7 @@ public abstract class RemoteExporter extends RemotingSupport { * @see RemoteInvocationTraceInterceptor */ public void setRegisterTraceInterceptor(boolean registerTraceInterceptor) { - this.registerTraceInterceptor = Boolean.valueOf(registerTraceInterceptor); + this.registerTraceInterceptor = registerTraceInterceptor; } /** @@ -110,9 +110,7 @@ public abstract class RemoteExporter extends RemotingSupport { * @see #setService */ protected void checkService() throws IllegalArgumentException { - if (getService() == null) { - throw new IllegalArgumentException("Property 'service' is required"); - } + Assert.notNull(getService(), "Property 'service' is required"); } /** @@ -123,10 +121,9 @@ public abstract class RemoteExporter extends RemotingSupport { */ protected void checkServiceInterface() throws IllegalArgumentException { Class serviceInterface = getServiceInterface(); + Assert.notNull(serviceInterface, "Property 'serviceInterface' is required"); + Object service = getService(); - if (serviceInterface == null) { - throw new IllegalArgumentException("Property 'serviceInterface' is required"); - } if (service instanceof String) { throw new IllegalArgumentException("Service [" + service + "] is a String " + "rather than an actual service reference: Have you accidentally specified " + @@ -153,20 +150,23 @@ public abstract class RemoteExporter extends RemotingSupport { protected Object getProxyForService() { checkService(); checkServiceInterface(); + ProxyFactory proxyFactory = new ProxyFactory(); proxyFactory.addInterface(getServiceInterface()); - if (this.registerTraceInterceptor != null ? - this.registerTraceInterceptor.booleanValue() : this.interceptors == null) { + + if (this.registerTraceInterceptor != null ? this.registerTraceInterceptor : this.interceptors == null) { proxyFactory.addAdvice(new RemoteInvocationTraceInterceptor(getExporterName())); } if (this.interceptors != null) { AdvisorAdapterRegistry adapterRegistry = GlobalAdvisorAdapterRegistry.getInstance(); - for (int i = 0; i < this.interceptors.length; i++) { - proxyFactory.addAdvisor(adapterRegistry.wrap(this.interceptors[i])); + for (Object interceptor : this.interceptors) { + proxyFactory.addAdvisor(adapterRegistry.wrap(interceptor)); } } + proxyFactory.setTarget(getService()); proxyFactory.setOpaque(true); + return proxyFactory.getProxy(getBeanClassLoader()); } diff --git a/spring-jms/src/main/java/org/springframework/jms/remoting/JmsInvokerProxyFactoryBean.java b/spring-jms/src/main/java/org/springframework/jms/remoting/JmsInvokerProxyFactoryBean.java index 5ddb7e74ce..b3177fa4dc 100644 --- a/spring-jms/src/main/java/org/springframework/jms/remoting/JmsInvokerProxyFactoryBean.java +++ b/spring-jms/src/main/java/org/springframework/jms/remoting/JmsInvokerProxyFactoryBean.java @@ -59,9 +59,7 @@ public class JmsInvokerProxyFactoryBean extends JmsInvokerClientInterceptor */ public void setServiceInterface(Class serviceInterface) { Assert.notNull(serviceInterface, "'serviceInterface' must not be null"); - if (!serviceInterface.isInterface()) { - throw new IllegalArgumentException("'serviceInterface' must be an interface"); - } + Assert.isTrue(serviceInterface.isInterface(), "'serviceInterface' must be an interface"); this.serviceInterface = serviceInterface; } @@ -73,9 +71,7 @@ public class JmsInvokerProxyFactoryBean extends JmsInvokerClientInterceptor @Override public void afterPropertiesSet() { super.afterPropertiesSet(); - if (this.serviceInterface == null) { - throw new IllegalArgumentException("Property 'serviceInterface' is required"); - } + Assert.notNull(this.serviceInterface, "Property 'serviceInterface' is required"); this.serviceProxy = new ProxyFactory(this.serviceInterface, this).getProxy(this.beanClassLoader); } diff --git a/spring-web/src/main/java/org/springframework/remoting/httpinvoker/AbstractHttpInvokerRequestExecutor.java b/spring-web/src/main/java/org/springframework/remoting/httpinvoker/AbstractHttpInvokerRequestExecutor.java index 61494aaf83..5cef885997 100644 --- a/spring-web/src/main/java/org/springframework/remoting/httpinvoker/AbstractHttpInvokerRequestExecutor.java +++ b/spring-web/src/main/java/org/springframework/remoting/httpinvoker/AbstractHttpInvokerRequestExecutor.java @@ -33,6 +33,7 @@ import org.springframework.remoting.rmi.CodebaseAwareObjectInputStream; import org.springframework.remoting.support.RemoteInvocation; import org.springframework.remoting.support.RemoteInvocationResult; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; /** * Abstract base implementation of the HttpInvokerRequestExecutor interface. @@ -291,7 +292,7 @@ public abstract class AbstractHttpInvokerRequestExecutor implements HttpInvokerR Object obj = ois.readObject(); if (!(obj instanceof RemoteInvocationResult)) { throw new RemoteException("Deserialized object needs to be assignable to type [" + - RemoteInvocationResult.class.getName() + "]: " + obj); + RemoteInvocationResult.class.getName() + "]: " + ClassUtils.getDescriptiveType(obj)); } return (RemoteInvocationResult) obj; } diff --git a/spring-web/src/main/java/org/springframework/remoting/httpinvoker/HttpComponentsHttpInvokerRequestExecutor.java b/spring-web/src/main/java/org/springframework/remoting/httpinvoker/HttpComponentsHttpInvokerRequestExecutor.java index 2161ca658e..0b9beb9f4e 100644 --- a/spring-web/src/main/java/org/springframework/remoting/httpinvoker/HttpComponentsHttpInvokerRequestExecutor.java +++ b/spring-web/src/main/java/org/springframework/remoting/httpinvoker/HttpComponentsHttpInvokerRequestExecutor.java @@ -212,10 +212,12 @@ public class HttpComponentsHttpInvokerRequestExecutor extends AbstractHttpInvoke */ protected HttpPost createHttpPost(HttpInvokerClientConfiguration config) throws IOException { HttpPost httpPost = new HttpPost(config.getServiceUrl()); + RequestConfig requestConfig = createRequestConfig(config); if (requestConfig != null) { httpPost.setConfig(requestConfig); } + LocaleContext localeContext = LocaleContextHolder.getLocaleContext(); if (localeContext != null) { Locale locale = localeContext.getLocale(); @@ -223,9 +225,11 @@ public class HttpComponentsHttpInvokerRequestExecutor extends AbstractHttpInvoke httpPost.addHeader(HTTP_HEADER_ACCEPT_LANGUAGE, StringUtils.toLanguageTag(locale)); } } + if (isAcceptGzipEncoding()) { httpPost.addHeader(HTTP_HEADER_ACCEPT_ENCODING, ENCODING_GZIP); } + return httpPost; } @@ -250,9 +254,10 @@ public class HttpComponentsHttpInvokerRequestExecutor extends AbstractHttpInvoke } private RequestConfig mergeRequestConfig(RequestConfig defaultRequestConfig) { - if (this.requestConfig == null) { // nothing to merge + if (this.requestConfig == null) { // nothing to merge return defaultRequestConfig; } + RequestConfig.Builder builder = RequestConfig.copy(defaultRequestConfig); int connectTimeout = this.requestConfig.getConnectTimeout(); if (connectTimeout >= 0) { diff --git a/spring-web/src/main/java/org/springframework/remoting/httpinvoker/HttpInvokerProxyFactoryBean.java b/spring-web/src/main/java/org/springframework/remoting/httpinvoker/HttpInvokerProxyFactoryBean.java index 19e6d95513..9e5f61be9e 100644 --- a/spring-web/src/main/java/org/springframework/remoting/httpinvoker/HttpInvokerProxyFactoryBean.java +++ b/spring-web/src/main/java/org/springframework/remoting/httpinvoker/HttpInvokerProxyFactoryBean.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. @@ -18,6 +18,7 @@ package org.springframework.remoting.httpinvoker; import org.springframework.aop.framework.ProxyFactory; import org.springframework.beans.factory.FactoryBean; +import org.springframework.util.Assert; /** * {@link FactoryBean} for HTTP invoker proxies. Exposes the proxied service @@ -60,10 +61,9 @@ public class HttpInvokerProxyFactoryBean extends HttpInvokerClientInterceptor im @Override public void afterPropertiesSet() { super.afterPropertiesSet(); - if (getServiceInterface() == null) { - throw new IllegalArgumentException("Property 'serviceInterface' is required"); - } - this.serviceProxy = new ProxyFactory(getServiceInterface(), this).getProxy(getBeanClassLoader()); + Class ifc = getServiceInterface(); + Assert.notNull(ifc, "Property 'serviceInterface' is required"); + this.serviceProxy = new ProxyFactory(ifc, this).getProxy(getBeanClassLoader()); } diff --git a/spring-web/src/main/java/org/springframework/remoting/httpinvoker/SimpleHttpInvokerRequestExecutor.java b/spring-web/src/main/java/org/springframework/remoting/httpinvoker/SimpleHttpInvokerRequestExecutor.java index a200074506..d07bedd040 100644 --- a/spring-web/src/main/java/org/springframework/remoting/httpinvoker/SimpleHttpInvokerRequestExecutor.java +++ b/spring-web/src/main/java/org/springframework/remoting/httpinvoker/SimpleHttpInvokerRequestExecutor.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. @@ -107,7 +107,8 @@ public class SimpleHttpInvokerRequestExecutor extends AbstractHttpInvokerRequest protected HttpURLConnection openConnection(HttpInvokerClientConfiguration config) throws IOException { URLConnection con = new URL(config.getServiceUrl()).openConnection(); if (!(con instanceof HttpURLConnection)) { - throw new IOException("Service URL [" + config.getServiceUrl() + "] is not an HTTP URL"); + throw new IOException( + "Service URL [" + config.getServiceUrl() + "] does not resolve to an HTTP connection"); } return (HttpURLConnection) con; } @@ -130,6 +131,7 @@ public class SimpleHttpInvokerRequestExecutor extends AbstractHttpInvokerRequest if (this.readTimeout >= 0) { connection.setReadTimeout(this.readTimeout); } + connection.setDoOutput(true); connection.setRequestMethod(HTTP_METHOD_POST); connection.setRequestProperty(HTTP_HEADER_CONTENT_TYPE, getContentType()); @@ -142,6 +144,7 @@ public class SimpleHttpInvokerRequestExecutor extends AbstractHttpInvokerRequest connection.setRequestProperty(HTTP_HEADER_ACCEPT_LANGUAGE, StringUtils.toLanguageTag(locale)); } } + if (isAcceptGzipEncoding()) { connection.setRequestProperty(HTTP_HEADER_ACCEPT_ENCODING, ENCODING_GZIP); } diff --git a/spring-web/src/main/java/org/springframework/remoting/jaxws/JaxWsPortClientInterceptor.java b/spring-web/src/main/java/org/springframework/remoting/jaxws/JaxWsPortClientInterceptor.java index 22ebddece8..be1ffef0a8 100644 --- a/spring-web/src/main/java/org/springframework/remoting/jaxws/JaxWsPortClientInterceptor.java +++ b/spring-web/src/main/java/org/springframework/remoting/jaxws/JaxWsPortClientInterceptor.java @@ -278,16 +278,14 @@ public class JaxWsPortClientInterceptor extends LocalJaxWsServiceFactory * Set the interface of the service that this factory should create a proxy for. */ public void setServiceInterface(Class serviceInterface) { - if (!serviceInterface.isInterface()) { - throw new IllegalArgumentException("'serviceInterface' must be an interface"); - } + Assert.notNull(serviceInterface, "'serviceInterface' must not be null"); + Assert.isTrue(serviceInterface.isInterface(), "'serviceInterface' must be an interface"); this.serviceInterface = serviceInterface; } /** * Return the interface of the service that this factory should create a proxy for. */ - @Nullable public Class getServiceInterface() { return this.serviceInterface; }