javeme commented on code in PR #133:
URL:
https://github.com/apache/incubator-hugegraph-commons/pull/133#discussion_r1310628996
##########
hugegraph-common/src/test/java/org/apache/hugegraph/unit/util/ReflectionUtilTest.java:
##########
@@ -94,7 +94,7 @@ public void testClasses() throws IOException {
@SuppressWarnings("unchecked")
List<ClassInfo> classes = IteratorUtils.toList(ReflectionUtil.classes(
"org.apache.hugegraph.util"));
- Assert.assertEquals(17, classes.size());
Review Comment:
这个的意图是为了测试ReflectionUtil的方法执行是否符合预期
##########
hugegraph-common/src/test/java/org/apache/hugegraph/unit/util/ReflectionUtilTest.java:
##########
@@ -94,7 +94,7 @@ public void testClasses() throws IOException {
@SuppressWarnings("unchecked")
List<ClassInfo> classes = IteratorUtils.toList(ReflectionUtil.classes(
"org.apache.hugegraph.util"));
- Assert.assertEquals(17, classes.size());
Review Comment:
这个的意图是为了测试ReflectionUtil的方法执行是否符合预期
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/AbstractRestClient.java:
##########
@@ -17,188 +17,175 @@
package org.apache.hugegraph.rest;
+import com.google.common.collect.ImmutableMap;
+import lombok.SneakyThrows;
+import okhttp3.*;
+import okio.BufferedSink;
+import okio.GzipSink;
+import okio.Okio;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hugegraph.util.JsonUtil;
+import org.jetbrains.annotations.NotNull;
+
+import javax.net.ssl.*;
+import java.io.FileInputStream;
+import java.io.IOException;
import java.net.URI;
-import java.security.KeyManagementException;
-import java.security.SecureRandom;
-import java.security.cert.CertificateException;
-import java.security.cert.X509Certificate;
+import java.security.KeyStore;
+import java.util.Arrays;
import java.util.Collection;
-import java.util.List;
import java.util.Map;
import java.util.concurrent.Callable;
-import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
-import javax.net.ssl.HostnameVerifier;
-import javax.net.ssl.HttpsURLConnection;
-import javax.net.ssl.SSLContext;
-import javax.net.ssl.SSLSession;
-import javax.net.ssl.TrustManager;
-import javax.net.ssl.X509TrustManager;
-
-import org.apache.commons.collections.MapUtils;
-import org.apache.commons.lang.StringUtils;
-import org.apache.commons.lang3.tuple.Pair;
-import org.apache.http.HttpHeaders;
-import org.apache.http.config.Registry;
-import org.apache.http.config.RegistryBuilder;
-import org.apache.http.conn.socket.ConnectionSocketFactory;
-import org.apache.http.conn.socket.PlainConnectionSocketFactory;
-import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
-import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
-import org.apache.http.pool.PoolStats;
-import org.apache.hugegraph.util.E;
-import org.apache.hugegraph.util.ExecutorUtil;
-import org.glassfish.jersey.SslConfigurator;
-import org.glassfish.jersey.apache.connector.ApacheClientProperties;
-import org.glassfish.jersey.apache.connector.ApacheConnectorProvider;
-import org.glassfish.jersey.client.ClientConfig;
-import org.glassfish.jersey.client.ClientProperties;
-import org.glassfish.jersey.client.JerseyClientBuilder;
-import org.glassfish.jersey.client.authentication.HttpAuthenticationFeature;
-import org.glassfish.jersey.internal.util.collection.Ref;
-import org.glassfish.jersey.internal.util.collection.Refs;
-import org.glassfish.jersey.message.GZipEncoder;
-import org.glassfish.jersey.uri.UriComponent;
-
-import com.google.common.collect.ImmutableMap;
-
-import jakarta.ws.rs.client.Client;
-import jakarta.ws.rs.client.ClientRequestContext;
-import jakarta.ws.rs.client.ClientRequestFilter;
-import jakarta.ws.rs.client.Entity;
-import jakarta.ws.rs.client.Invocation.Builder;
-import jakarta.ws.rs.client.WebTarget;
-import jakarta.ws.rs.core.MediaType;
-import jakarta.ws.rs.core.MultivaluedMap;
-import jakarta.ws.rs.core.Response;
-import jakarta.ws.rs.core.Variant;
-
public abstract class AbstractRestClient implements RestClient {
- // Time unit: hours
- private static final long TTL = 24L;
- // Time unit: ms
- private static final long IDLE_TIME = 40L * 1000L;
+ private OkHttpClient client;
- private static final String TOKEN_KEY = "tokenKey";
+ private String baseUrl;
- private final Client client;
- private final WebTarget target;
-
- private PoolingHttpClientConnectionManager pool;
- private ScheduledExecutorService cleanExecutor;
+ private Request.Builder requestBuilder = new Request.Builder();
public AbstractRestClient(String url, int timeout) {
- this(url, new ConfigBuilder().configTimeout(timeout).build());
+ this(url, OkhttpConfig.builder()
+ .timeout(timeout)
+ .build());
}
public AbstractRestClient(String url, String user, String password,
- int timeout) {
- this(url, new ConfigBuilder().configTimeout(timeout)
- .configUser(user, password)
- .build());
+ Integer timeout) {
+ this(url, OkhttpConfig.builder()
+ .user(user).password(password)
+ .timeout(timeout)
+ .build());
}
public AbstractRestClient(String url, int timeout,
- int maxTotal, int maxPerRoute) {
- this(url, new ConfigBuilder().configTimeout(timeout)
- .configPool(maxTotal, maxPerRoute)
- .build());
+ int maxTotal, int maxPerRoute) {
+ this(url, null, null, timeout, maxTotal, maxPerRoute);
}
public AbstractRestClient(String url, int timeout, int idleTime,
- int maxTotal, int maxPerRoute) {
- this(url, new ConfigBuilder().configTimeout(timeout)
- .configIdleTime(idleTime)
- .configPool(maxTotal, maxPerRoute)
- .build());
+ int maxTotal, int maxPerRoute) {
+ this(url, OkhttpConfig.builder()
+ .idleTime(idleTime)
+ .timeout(timeout)
+ .maxTotal(maxTotal)
+ .maxPerRoute(maxPerRoute)
+ .build());
}
public AbstractRestClient(String url, String user, String password,
- int timeout, int maxTotal, int maxPerRoute) {
- this(url, new ConfigBuilder().configTimeout(timeout)
- .configUser(user, password)
- .configPool(maxTotal, maxPerRoute)
- .build());
+ int timeout, int maxTotal, int
maxPerRoute) {
+ this(url, OkhttpConfig.builder()
+ .user(user).password(password)
+ .timeout(timeout)
+ .maxTotal(maxTotal)
+ .maxPerRoute(maxPerRoute)
+ .build());
}
public AbstractRestClient(String url, String user, String password,
- int timeout, int maxTotal, int maxPerRoute,
- String trustStoreFile,
- String trustStorePassword) {
- this(url, new ConfigBuilder().configTimeout(timeout)
- .configUser(user, password)
- .configPool(maxTotal, maxPerRoute)
- .configSSL(trustStoreFile,
- trustStorePassword)
- .build());
- }
-
- public AbstractRestClient(String url, String token, int timeout) {
- this(url, new ConfigBuilder().configTimeout(timeout)
- .configToken(token)
- .build());
- }
-
- public AbstractRestClient(String url, String token, int timeout,
- int maxTotal, int maxPerRoute) {
- this(url, new ConfigBuilder().configTimeout(timeout)
- .configToken(token)
- .configPool(maxTotal, maxPerRoute)
- .build());
- }
-
- public AbstractRestClient(String url, String token, int timeout,
- int maxTotal, int maxPerRoute,
- String trustStoreFile,
- String trustStorePassword) {
- this(url, new ConfigBuilder().configTimeout(timeout)
- .configToken(token)
- .configPool(maxTotal, maxPerRoute)
- .configSSL(trustStoreFile,
- trustStorePassword)
- .build());
- }
-
- public AbstractRestClient(String url, ClientConfig config) {
- configConnectionManager(url, config);
-
- this.client = JerseyClientBuilder.newClient(config);
- this.client.register(GZipEncoder.class);
- this.target = this.client.target(url);
- this.pool = (PoolingHttpClientConnectionManager) config.getProperty(
- ApacheClientProperties.CONNECTION_MANAGER);
- if (this.pool != null) {
- this.cleanExecutor = ExecutorUtil.newScheduledThreadPool(
- "conn-clean-worker-%d");
- Number idleTimeProp = (Number) config.getProperty("idleTime");
- final long idleTime = idleTimeProp == null ?
- IDLE_TIME : idleTimeProp.longValue();
- final long checkPeriod = idleTime / 2L;
- this.cleanExecutor.scheduleWithFixedDelay(() -> {
- PoolStats stats = this.pool.getTotalStats();
- int using = stats.getLeased() + stats.getPending();
- if (using > 0) {
- // Do clean only when all connections are idle
- return;
- }
- // Release connections when all clients are inactive
- this.pool.closeIdleConnections(idleTime,
TimeUnit.MILLISECONDS);
- this.pool.closeExpiredConnections();
- }, checkPeriod, checkPeriod, TimeUnit.MILLISECONDS);
+ int timeout, int maxTotal, int maxPerRoute,
+ String trustStoreFile,
+ String trustStorePassword) {
+ this(url, OkhttpConfig.builder()
+ .user(user).password(password)
+ .timeout(timeout)
+ .maxTotal(maxTotal)
+ .maxPerRoute(maxPerRoute)
+ .trustStoreFile(trustStoreFile)
+ .trustStorePassword(trustStorePassword)
+ .build());
+ }
+
+ public AbstractRestClient(String url, String token, Integer timeout) {
+ this(url, OkhttpConfig.builder()
+ .token(token)
+ .timeout(timeout)
+ .build());
+ }
+
+ public AbstractRestClient(String url, String token, Integer timeout,
+ Integer maxTotal, Integer maxPerRoute) {
+ this(url,OkhttpConfig.builder()
+ .token(token)
+ .timeout(timeout)
+ .maxTotal(maxTotal)
+ .maxPerRoute(maxPerRoute)
+ .build());
+ }
+
+ public AbstractRestClient(String url, String token, Integer timeout,
+ Integer maxTotal, Integer maxPerRoute,
+ String trustStoreFile,
+ String trustStorePassword) {
+ this(url,OkhttpConfig.builder()
+ .token(token)
+ .timeout(timeout)
+ .maxTotal(maxTotal)
+ .maxPerRoute(maxPerRoute)
+ .trustStoreFile(trustStoreFile)
+ .trustStorePassword(trustStorePassword)
+ .build());
+ }
+
+ public AbstractRestClient(String url, OkhttpConfig okhttpConfig) {
+ this.baseUrl = url;
+ this.client = getOkhttpClient(okhttpConfig);
+ }
+
+ private OkHttpClient getOkhttpClient(OkhttpConfig okhttpConfig) {
+ OkHttpClient.Builder builder = new OkHttpClient.Builder();
+
+ if(okhttpConfig.getTimeout()!=null) {
+ builder.connectTimeout(okhttpConfig.getTimeout(),
TimeUnit.MILLISECONDS)
+ .readTimeout(okhttpConfig.getTimeout(),
TimeUnit.MILLISECONDS);
}
- }
- protected abstract void checkStatus(Response response,
- Response.Status... statuses);
+ if(okhttpConfig.getIdleTime()!=null) {
+ ConnectionPool connectionPool = new ConnectionPool(5,
okhttpConfig.getIdleTime(), TimeUnit.MILLISECONDS);
+ builder.connectionPool(connectionPool);
+ }
- protected Response request(Callable<Response> method) {
- try {
- return method.call();
- } catch (Exception e) {
- throw new ClientException("Failed to do request", e);
+
+ //auth
Review Comment:
could you add more details for the comment and adjust the comment style?
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/AbstractRestClient.java:
##########
@@ -17,188 +17,175 @@
package org.apache.hugegraph.rest;
+import com.google.common.collect.ImmutableMap;
+import lombok.SneakyThrows;
+import okhttp3.*;
+import okio.BufferedSink;
+import okio.GzipSink;
+import okio.Okio;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hugegraph.util.JsonUtil;
+import org.jetbrains.annotations.NotNull;
+
+import javax.net.ssl.*;
+import java.io.FileInputStream;
+import java.io.IOException;
import java.net.URI;
-import java.security.KeyManagementException;
-import java.security.SecureRandom;
-import java.security.cert.CertificateException;
-import java.security.cert.X509Certificate;
+import java.security.KeyStore;
+import java.util.Arrays;
import java.util.Collection;
-import java.util.List;
import java.util.Map;
import java.util.concurrent.Callable;
-import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
-import javax.net.ssl.HostnameVerifier;
-import javax.net.ssl.HttpsURLConnection;
-import javax.net.ssl.SSLContext;
-import javax.net.ssl.SSLSession;
-import javax.net.ssl.TrustManager;
-import javax.net.ssl.X509TrustManager;
-
-import org.apache.commons.collections.MapUtils;
-import org.apache.commons.lang.StringUtils;
-import org.apache.commons.lang3.tuple.Pair;
-import org.apache.http.HttpHeaders;
-import org.apache.http.config.Registry;
-import org.apache.http.config.RegistryBuilder;
-import org.apache.http.conn.socket.ConnectionSocketFactory;
-import org.apache.http.conn.socket.PlainConnectionSocketFactory;
-import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
-import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
-import org.apache.http.pool.PoolStats;
-import org.apache.hugegraph.util.E;
-import org.apache.hugegraph.util.ExecutorUtil;
-import org.glassfish.jersey.SslConfigurator;
-import org.glassfish.jersey.apache.connector.ApacheClientProperties;
-import org.glassfish.jersey.apache.connector.ApacheConnectorProvider;
-import org.glassfish.jersey.client.ClientConfig;
-import org.glassfish.jersey.client.ClientProperties;
-import org.glassfish.jersey.client.JerseyClientBuilder;
-import org.glassfish.jersey.client.authentication.HttpAuthenticationFeature;
-import org.glassfish.jersey.internal.util.collection.Ref;
-import org.glassfish.jersey.internal.util.collection.Refs;
-import org.glassfish.jersey.message.GZipEncoder;
-import org.glassfish.jersey.uri.UriComponent;
-
-import com.google.common.collect.ImmutableMap;
-
-import jakarta.ws.rs.client.Client;
-import jakarta.ws.rs.client.ClientRequestContext;
-import jakarta.ws.rs.client.ClientRequestFilter;
-import jakarta.ws.rs.client.Entity;
-import jakarta.ws.rs.client.Invocation.Builder;
-import jakarta.ws.rs.client.WebTarget;
-import jakarta.ws.rs.core.MediaType;
-import jakarta.ws.rs.core.MultivaluedMap;
-import jakarta.ws.rs.core.Response;
-import jakarta.ws.rs.core.Variant;
-
public abstract class AbstractRestClient implements RestClient {
- // Time unit: hours
- private static final long TTL = 24L;
- // Time unit: ms
- private static final long IDLE_TIME = 40L * 1000L;
+ private OkHttpClient client;
- private static final String TOKEN_KEY = "tokenKey";
+ private String baseUrl;
- private final Client client;
- private final WebTarget target;
-
- private PoolingHttpClientConnectionManager pool;
- private ScheduledExecutorService cleanExecutor;
+ private Request.Builder requestBuilder = new Request.Builder();
public AbstractRestClient(String url, int timeout) {
- this(url, new ConfigBuilder().configTimeout(timeout).build());
+ this(url, OkhttpConfig.builder()
+ .timeout(timeout)
+ .build());
}
public AbstractRestClient(String url, String user, String password,
- int timeout) {
- this(url, new ConfigBuilder().configTimeout(timeout)
- .configUser(user, password)
- .build());
+ Integer timeout) {
+ this(url, OkhttpConfig.builder()
+ .user(user).password(password)
Review Comment:
ditto
##########
hugegraph-common/src/test/java/org/apache/hugegraph/unit/rest/RestClientTest.java:
##########
@@ -493,137 +514,131 @@ public MockRestClientImpl(String url, int timeout) {
@Override
protected void checkStatus(Response response,
- Response.Status... statuses) {
+ int... statuses) {
// pass
}
}
+ @SneakyThrows
@Test
public void testRequest() {
- MockRestClientImpl client = new MockRestClientImpl("test", 1000);
-
- WebTarget target = Mockito.mock(WebTarget.class);
- Builder builder = Mockito.mock(Builder.class);
-
- Mockito.when(target.path("test")).thenReturn(target);
- Mockito.when(target.path("test")
- .path(AbstractRestClient.encode("id")))
- .thenReturn(target);
- Mockito.when(target.path("test").request()).thenReturn(builder);
- Mockito.when(target.path("test")
- .path(AbstractRestClient.encode("id"))
- .request())
- .thenReturn(builder);
-
- Response response = Mockito.mock(Response.class);
- Mockito.when(response.getStatus()).thenReturn(200);
- Mockito.when(response.getHeaders())
- .thenReturn(new MultivaluedHashMap<>());
- Mockito.when(response.readEntity(String.class)).thenReturn("content");
-
- Mockito.when(builder.delete()).thenReturn(response);
- Mockito.when(builder.get()).thenReturn(response);
- Mockito.when(builder.put(Mockito.any())).thenReturn(response);
- Mockito.when(builder.post(Mockito.any())).thenReturn(response);
-
- Whitebox.setInternalState(client, "target", target);
+ MockRestClientImpl client = new MockRestClientImpl("/test", 1000);
+
+ Response response = Mockito.mock(Response.class,
Mockito.RETURNS_DEEP_STUBS);
+ Mockito.when(response.code()).thenReturn(200);
+ Mockito.when(response.headers())
+ .thenReturn(new Headers.Builder().build());
+ Mockito.when(response.body().string()).thenReturn("content");
+
+ Mockito.when(requestBuilder.delete()).thenReturn(requestBuilder);
+ Mockito.when(requestBuilder.get()).thenReturn(requestBuilder);
+
Mockito.when(requestBuilder.put(Mockito.any())).thenReturn(requestBuilder);
+
Mockito.when(requestBuilder.post(Mockito.any())).thenReturn(requestBuilder);
+
+ OkHttpClient okHttpClient = Mockito.mock(OkHttpClient.class,
Mockito.RETURNS_DEEP_STUBS);
+
Mockito.when(okHttpClient.newCall(Mockito.any()).execute()).thenReturn(response);
+
+ Whitebox.setInternalState(client, "client", okHttpClient);
+ Whitebox.setInternalState(client, "requestBuilder", requestBuilder);
RestResult result;
// Test delete
client.setAuthContext("token1");
result = client.delete("test", ImmutableMap.of());
Assert.assertEquals(200, result.status());
- Mockito.verify(builder).header(HttpHeaders.AUTHORIZATION,
+ Mockito.verify(requestBuilder).addHeader("Authorization",
"token1");
+
client.resetAuthContext();
client.setAuthContext("token2");
result = client.delete("test", "id");
Assert.assertEquals(200, result.status());
- Mockito.verify(builder).header(HttpHeaders.AUTHORIZATION,
+ Mockito.verify(requestBuilder).addHeader(HttpHeaders.AUTHORIZATION,
"token2");
client.resetAuthContext();
// Test get
client.setAuthContext("token3");
result = client.get("test");
Assert.assertEquals(200, result.status());
- Mockito.verify(builder).header(HttpHeaders.AUTHORIZATION,
+ Mockito.verify(requestBuilder).addHeader(HttpHeaders.AUTHORIZATION,
"token3");
client.resetAuthContext();
client.setAuthContext("token4");
result = client.get("test", ImmutableMap.of());
Assert.assertEquals(200, result.status());
- Mockito.verify(builder).header(HttpHeaders.AUTHORIZATION,
+ Mockito.verify(requestBuilder).addHeader(HttpHeaders.AUTHORIZATION,
"token4");
client.resetAuthContext();
client.setAuthContext("token5");
result = client.get("test", "id");
Assert.assertEquals(200, result.status());
- Mockito.verify(builder).header(HttpHeaders.AUTHORIZATION,
+ Mockito.verify(requestBuilder).addHeader(HttpHeaders.AUTHORIZATION,
"token5");
client.resetAuthContext();
// Test put
client.setAuthContext("token6");
- result = client.post("test", new Object());
+// result = client.post("test", new Object()); //why use new Object()
as args here?
+ result = client.post("test", null);
Assert.assertEquals(200, result.status());
- Mockito.verify(builder).header(HttpHeaders.AUTHORIZATION,
+ Mockito.verify(requestBuilder).addHeader(HttpHeaders.AUTHORIZATION,
"token6");
client.resetAuthContext();
client.setAuthContext("token7");
- result = client.post("test", new Object(), new MultivaluedHashMap<>());
+ result = client.post("test", null, new Headers.Builder().build());
Review Comment:
OK
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
