javeme commented on code in PR #538:
URL:
https://github.com/apache/incubator-hugegraph-toolchain/pull/538#discussion_r1404720810
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/auth/UserAPI.java:
##########
@@ -50,7 +50,7 @@ public User get(Object id) {
}
public UserRole getUserRole(Object id) {
- String idEncoded = RestClient.encode(formatEntityId(id));
+ String idEncoded = formatEntityId(id);
Review Comment:
can we also update the var name: `String id = formatEntityId(id);`?
but I'm not sure what's the behavior if the id contains a special char like
'&'.
##########
hugegraph-client/pom.xml:
##########
@@ -48,15 +49,18 @@
<groupId>org.lz4</groupId>
<artifactId>lz4-java</artifactId>
</dependency>
- <dependency>
- <groupId>org.glassfish.jersey.containers</groupId>
- <artifactId>jersey-container-servlet</artifactId>
- </dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
+
+ <dependency>
+ <groupId>org.projectlombok</groupId>
+ <artifactId>lombok</artifactId>
+ <version>${lombok.version}</version>
+ <optional>true</optional>
+ </dependency>
Review Comment:
not addressed?
##########
hugegraph-client/src/main/java/org/apache/hugegraph/client/RestClient.java:
##########
@@ -18,18 +18,18 @@
package org.apache.hugegraph.client;
import org.apache.hugegraph.exception.ServerException;
-import org.apache.hugegraph.serializer.PathDeserializer;
-import org.apache.hugegraph.structure.graph.Path;
import org.apache.hugegraph.rest.AbstractRestClient;
import org.apache.hugegraph.rest.ClientException;
import org.apache.hugegraph.rest.RestResult;
+import org.apache.hugegraph.serializer.PathDeserializer;
+import org.apache.hugegraph.structure.graph.Path;
import org.apache.hugegraph.util.E;
import org.apache.hugegraph.util.VersionUtil;
import org.apache.hugegraph.util.VersionUtil.Version;
import com.fasterxml.jackson.databind.module.SimpleModule;
-import jakarta.ws.rs.core.Response;
+import okhttp3.Response;
Review Comment:
just use okhttp3.Response at line 80 so that we don't use it elsewhere
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graph/GraphAPI.java:
##########
@@ -61,25 +58,14 @@ public static String formatVertexId(Object id, boolean
allowNull) {
id = id.toString();
}
E.checkArgument(id instanceof String || id instanceof Number,
- "The vertex id must be either String or " +
- "Number, but got '%s'", id);
+ "The vertex id must be either String or Number, but
got '%s'", id);
return (uuid ? "U" : "") + JsonUtil.toJson(id);
}
public static String formatProperties(Map<String, Object> properties) {
if (properties == null) {
return null;
}
- String json = JsonUtil.toJson(properties);
- /*
- * Don't use UrlEncoder.encode, it encoded the space as `+`,
- * which will invalidate the jersey's automatic decoding
- * because it considers the space to be encoded as `%2F`
- */
- return encode(json);
- }
-
- public static String encode(String raw) {
- return UriComponent.encode(raw, Type.QUERY_PARAM_SPACE_ENCODED);
+ return JsonUtil.toJson(properties);
Review Comment:
can we add a test case for a query param that is needed to encoded
##########
hugegraph-client/src/main/java/org/apache/hugegraph/exception/ServerException.java:
##########
@@ -23,7 +23,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import jakarta.ws.rs.core.Response;
+import okhttp3.Response;
Review Comment:
ditto
##########
hugegraph-client/src/main/java/org/apache/hugegraph/driver/HugeClient.java:
##########
@@ -19,14 +19,14 @@
import java.io.Closeable;
-import org.apache.hugegraph.version.ClientVersion;
import org.apache.hugegraph.client.RestClient;
-
import org.apache.hugegraph.rest.ClientException;
import org.apache.hugegraph.util.VersionUtil;
+import org.apache.hugegraph.version.ClientVersion;
-import jakarta.ws.rs.ProcessingException;
+import lombok.extern.slf4j.Slf4j;
Review Comment:
please use log4j like other places
##########
hugegraph-client/src/main/java/org/apache/hugegraph/driver/HugeClient.java:
##########
@@ -60,7 +60,8 @@ public HugeClient(HugeClientBuilder builder) {
builder.maxConnsPerRoute(),
builder.trustStoreFile(),
builder.trustStorePassword());
- } catch (ProcessingException e) {
+ } catch (Exception e) {
+ log.error("", e);
Review Comment:
can we add some more details to the error message and just LOG.warn level is
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
