Merge pull request #68 from allenxwang/cp

Reopen closed but not merged pull request https://github.com/Netflix/ribbon/pull/64

ribbon-core/src/main/java/com/netflix/client/LoadBalancerContext.java

@@ -19,12 +19,14 @@ package com.netflix.client;
 
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.net.URLEncoder;
 import java.util.Collection;
 import java.util.concurrent.TimeUnit;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import com.google.common.base.Strings;
 import com.netflix.client.config.CommonClientConfigKey;
 import com.netflix.client.config.DefaultClientConfigImpl;
 import com.netflix.client.config.IClientConfig;
@@ -520,12 +522,35 @@ public abstract class LoadBalancerContext<T extends ClientRequest, S extends IRe
             }
             
             newURI = new URI(scheme, theUrl.getUserInfo(), host, port, urlPath, theUrl.getQuery(), theUrl.getFragment());
+            if (isURIEncoded(theUrl)) {
+                StringBuilder sb = new StringBuilder();
+                sb.append(newURI.getScheme())
+                  .append("://")
+                  .append(newURI.getRawAuthority())
+                  .append(theUrl.getRawPath());
+                if (!Strings.isNullOrEmpty(theUrl.getRawQuery())) {
+                    sb.append("?").append(theUrl.getRawQuery());
+                }
+                if (!Strings.isNullOrEmpty(theUrl.getRawFragment())) {
+                    sb.append("#").append(theUrl.getRawFragment());
+                }
+                newURI = new URI(sb.toString());
+            }
             return (T) original.replaceUri(newURI);            
         } catch (URISyntaxException e) {
             throw new ClientException(ClientException.ErrorType.GENERAL, e.getMessage());
         }
     }
 
+    private boolean isURIEncoded(URI uri) {
+        String original = uri.toString();
+        try {
+            return !URLEncoder.encode(original, "UTF-8").equals(original);
+        } catch (Exception e) {
+            return false;
+        }
+    }
+    
     protected boolean isRetriable(T request) {
         if (request.isRetriable()) {
             return true;            

ribbon-core/src/test/java/com/netflix/client/LoadBalancerContextTest.java

+/*
+ *
+ * Copyright 2013 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package com.netflix.client;
+
+import static org.junit.Assert.*;
+
+import java.net.URLEncoder;
+
+import org.junit.Test;
+
+import com.netflix.client.http.HttpRequest;
+import com.netflix.client.http.HttpResponse;
+import com.netflix.loadbalancer.BaseLoadBalancer;
+import com.netflix.loadbalancer.Server;
+
+public class LoadBalancerContextTest {
+
+    static BaseLoadBalancer lb = new BaseLoadBalancer() {
+
+        @Override
+        public Server chooseServer(Object key) {
+            return new Server("www.example.com:8080");
+        }
+    };
+    
+    
+    private MyLoadBalancerContext context;
+    
+    public LoadBalancerContextTest() {
+        context = new MyLoadBalancerContext();
+        context.setLoadBalancer(lb);
+    }
+    
+    @Test
+    public void testComputeFinalUriWithLoadBalancer() throws ClientException {
+        HttpRequest request = HttpRequest.newBuilder().uri("/test?abc=xyz").build();
+        HttpRequest newRequest = context.computeFinalUriWithLoadBalancer(request);
+        assertEquals("http://www.example.com:8080/test?abc=xyz", newRequest.getUri().toString());
+    }
+    
+    @Test
+    public void testEncodedPath() throws ClientException {
+        String uri = "http://localhost:8080/resources/abc%2Fxyz";
+        HttpRequest request = HttpRequest.newBuilder().uri(uri).build();
+        HttpRequest newRequest = context.computeFinalUriWithLoadBalancer(request);
+        assertEquals(uri, newRequest.getUri().toString());
+    }
+    
+    @Test
+    public void testEncodedPathAndHostChange() throws ClientException {
+        String uri = "/abc%2Fxyz";
+        HttpRequest request = HttpRequest.newBuilder().uri(uri).build();
+        HttpRequest newRequest = context.computeFinalUriWithLoadBalancer(request);
+        assertEquals("http://www.example.com:8080" + uri, newRequest.getUri().toString());
+    }
+
+    
+    @Test
+    public void testEncodedQuery() throws Exception {
+        String uri = "http://localhost:8080/resources/abc?";
+        String queryString = "name=" + URLEncoder.encode("éƎ&=*%!@#$%^&*()", "UTF-8");   
+        HttpRequest request = HttpRequest.newBuilder().uri(uri + queryString).build();
+        HttpRequest newRequest = context.computeFinalUriWithLoadBalancer(request);
+        assertEquals(uri + queryString, newRequest.getUri().toString());        
+    }
+}
+
+class MyLoadBalancerContext extends LoadBalancerContext<HttpRequest, HttpResponse> {
+}

ribbon-httpclient/src/main/java/com/netflix/http4/NFHttpClient.java

@@ -66,7 +66,7 @@ public class NFHttpClient extends DefaultHttpClient {
 
 	private static final Logger LOGGER = LoggerFactory.getLogger(NFHttpClient.class);
 
-	protected static final String EXECUTE_TRACER = "NFHttpClient__EXECUTE_REQ";
+	protected static final String EXECUTE_TRACER = "HttpClient-ExecuteTimer";
 
 	private HttpHost httpHost = null;
 	private HttpRoute httpRoute = null;
@@ -89,29 +89,30 @@ public class NFHttpClient extends DefaultHttpClient {
 		this.name = "UNNAMED_" + numNonNamedHttpClients.incrementAndGet();
 		httpHost = new HttpHost(host, port);
 		httpRoute = new HttpRoute(httpHost);
-		init(DefaultClientConfigImpl.getClientConfigWithDefaultValues());
+		init(DefaultClientConfigImpl.getClientConfigWithDefaultValues(), false);
 	}   
 
 	protected NFHttpClient(){
 		super(new ThreadSafeClientConnManager());
 		this.name = "UNNAMED_" + numNonNamedHttpClients.incrementAndGet();
-		init(DefaultClientConfigImpl.getClientConfigWithDefaultValues());
+		init(DefaultClientConfigImpl.getClientConfigWithDefaultValues(), false);
 	}
 
 	protected NFHttpClient(String name) {
-		super(new MonitoredConnectionManager(name));
-		this.name = name;
-		init(DefaultClientConfigImpl.getClientConfigWithDefaultValues());
+	    this(name, DefaultClientConfigImpl.getClientConfigWithDefaultValues(), true);
 	}
 
     protected NFHttpClient(String name, IClientConfig config) {
+        this(name, config, true);
+    }
+
+    protected NFHttpClient(String name, IClientConfig config, boolean registerMonitor) {
         super(new MonitoredConnectionManager(name));
         this.name = name;
-        init(config);
+        init(config, registerMonitor);
     }
-
 	
-	void init(IClientConfig config) {
+	void init(IClientConfig config, boolean registerMonitor) {
 		HttpParams params = getParams();
 
 		HttpProtocolParams.setContentCharset(params, "UTF-8");  
@@ -131,8 +132,10 @@ public class NFHttpClient extends DefaultHttpClient {
 		setHttpRequestRetryHandler(
 				new NFHttpMethodRetryHandler(this.name, this.retriesProperty.get(), false,
 						this.sleepTimeFactorMsProperty.get()));
-	    tracer = Monitors.newTimer(EXECUTE_TRACER, TimeUnit.MILLISECONDS);
-        Monitors.registerObject(name, this);
+	    tracer = Monitors.newTimer(EXECUTE_TRACER + "-" + name, TimeUnit.MILLISECONDS);
+	    if (registerMonitor) {
+            Monitors.registerObject(name, this);
+	    }
 	}
 
 	public void initConnectionCleanerTask(){
@@ -146,12 +149,12 @@ public class NFHttpClient extends DefaultHttpClient {
 
 	}
 
-	@Monitor(name = "connPoolCleaner", type = DataSourceType.INFORMATIONAL)
+	@Monitor(name = "HttpClient-ConnPoolCleaner", type = DataSourceType.INFORMATIONAL)
 	public ConnectionPoolCleaner getConnPoolCleaner() {
 		return connPoolCleaner;
 	}
 
-	@Monitor(name = "connIdleEvictTimeMilliSeconds", type = DataSourceType.INFORMATIONAL)
+	@Monitor(name = "HttpClient-ConnIdleEvictTimeMilliSeconds", type = DataSourceType.INFORMATIONAL)
 	public DynamicIntProperty getConnIdleEvictTimeMilliSeconds() {
 		if (connIdleEvictTimeMilliSeconds == null){
 			connIdleEvictTimeMilliSeconds = DynamicPropertyFactory.getInstance().getIntProperty(name + ".nfhttpclient.connIdleEvictTimeMilliSeconds", 
@@ -160,7 +163,7 @@ public class NFHttpClient extends DefaultHttpClient {
 		return connIdleEvictTimeMilliSeconds;
 	}
 
-	@Monitor(name="connectionsInPool", type = DataSourceType.GAUGE)    
+	@Monitor(name="HttpClient-ConnectionsInPool", type = DataSourceType.GAUGE)    
 	public int getConnectionsInPool() {
 		ClientConnectionManager connectionManager = this.getConnectionManager();
 		if (connectionManager != null) {
@@ -170,7 +173,7 @@ public class NFHttpClient extends DefaultHttpClient {
 		}
 	}
 
-	@Monitor(name = "maxTotalConnections", type = DataSourceType.INFORMATIONAL)
+	@Monitor(name = "HttpClient-MaxTotalConnections", type = DataSourceType.INFORMATIONAL)
 	public int getMaxTotalConnnections() {
 		ClientConnectionManager connectionManager = this.getConnectionManager();
 		if (connectionManager != null) {
@@ -180,7 +183,7 @@ public class NFHttpClient extends DefaultHttpClient {
 		}
 	}
 
-	@Monitor(name = "maxConnectionsPerHost", type = DataSourceType.INFORMATIONAL)
+	@Monitor(name = "HttpClient-MaxConnectionsPerHost", type = DataSourceType.INFORMATIONAL)
 	public int getMaxConnectionsPerHost() {
 		ClientConnectionManager connectionManager = this.getConnectionManager();
 		if (connectionManager != null) {
@@ -193,7 +196,7 @@ public class NFHttpClient extends DefaultHttpClient {
 		}
 	}
 
-	@Monitor(name = "numRetries", type = DataSourceType.INFORMATIONAL)
+	@Monitor(name = "HttpClient-NumRetries", type = DataSourceType.INFORMATIONAL)
 	public int getNumRetries() {
 		return this.retriesProperty.get();
 	}
@@ -202,7 +205,7 @@ public class NFHttpClient extends DefaultHttpClient {
 		this.connIdleEvictTimeMilliSeconds = connIdleEvictTimeMilliSeconds;
 	}
 
-	@Monitor(name = "sleepTimeFactorMs", type = DataSourceType.INFORMATIONAL)
+	@Monitor(name = "HttpClient-SleepTimeFactorMs", type = DataSourceType.INFORMATIONAL)
 	public int getSleepTimeFactorMs() {
 		return this.sleepTimeFactorMsProperty.get();
 	}

ribbon-httpclient/src/main/java/com/netflix/http4/NFHttpClientFactory.java

 /*
-*
-* Copyright 2013 Netflix, Inc.
-*
-* Licensed under the Apache License, Version 2.0 (the "License");
-* you may not use this file except in compliance with the License.
-* You may obtain a copy of the License at
-*
-* http://www.apache.org/licenses/LICENSE-2.0
-*
-* Unless required by applicable law or agreed to in writing, software
-* distributed under the License is distributed on an "AS IS" BASIS,
-* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-* See the License for the specific language governing permissions and
-* limitations under the License.
-*
-*/
+ *
+ * Copyright 2013 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
 package com.netflix.http4;
 
 import java.util.Map;
@@ -71,11 +71,8 @@ public class NFHttpClientFactory {
 		    synchronized (lock) {
 		        client = namedClientMap.get(name);       
 		        if (client == null){
-        			client = new NFHttpClient(name, config);
+        			client = new NFHttpClient(name, config, registerMonitor);
         			namedClientMap.put(name,client);
-        			if (registerMonitor) {
-        			    Monitors.registerObject("HttpClient_" + name, client);
-        			}
 		        }
 		    }
 		}
@@ -96,7 +93,7 @@ public class NFHttpClientFactory {
 	    if(c != null) {
 	        c.getConnectionManager().shutdown();
 	        namedClientMap.remove(name);
-	        Monitors.unregisterObject("HttpClient_" + name, c);
+	        Monitors.unregisterObject(name, c);
 	    }
 	}
 }