You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "vkagamlyk (via GitHub)" <gi...@apache.org> on 2024/05/07 16:32:24 UTC
[PR] driver error handling [tinkerpop]
vkagamlyk opened a new pull request, #2594:
URL: https://github.com/apache/tinkerpop/pull/2594
(no comment)
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] driver error handling [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2594:
URL: https://github.com/apache/tinkerpop/pull/2594#discussion_r1592971797
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpHandlerUtil.java:
##########
@@ -69,6 +71,20 @@ public class HttpHandlerUtil {
* @param message The error message to contain the body.
*/
public static void sendError(final ChannelHandlerContext ctx, final HttpResponseStatus status, final String message) {
+ sendError(ctx, status, message, false);
+ }
+
+ /**
+ * Helper method to send errors back as JSON. Only to be used when the RequestMessage couldn't be parsed, because
+ * a proper serialized ResponseMessage should be sent in that case.
+ *
+ * @param ctx The netty channel context.
+ * @param status The HTTP error status code.
+ * @param message The error message to contain the body.
+ * @param closeChannel Close the netty channel after error message.
+ */
+ public static void sendError(final ChannelHandlerContext ctx, final HttpResponseStatus status, final String message,
Review Comment:
When is closing the channel explicitly needed?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] driver error handling [tinkerpop]
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2594:
URL: https://github.com/apache/tinkerpop/pull/2594#discussion_r1592985079
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpHandlerUtil.java:
##########
@@ -69,6 +71,20 @@ public class HttpHandlerUtil {
* @param message The error message to contain the body.
*/
public static void sendError(final ChannelHandlerContext ctx, final HttpResponseStatus status, final String message) {
+ sendError(ctx, status, message, false);
+ }
+
+ /**
+ * Helper method to send errors back as JSON. Only to be used when the RequestMessage couldn't be parsed, because
+ * a proper serialized ResponseMessage should be sent in that case.
+ *
+ * @param ctx The netty channel context.
+ * @param status The HTTP error status code.
+ * @param message The error message to contain the body.
+ * @param closeChannel Close the netty channel after error message.
+ */
+ public static void sendError(final ChannelHandlerContext ctx, final HttpResponseStatus status, final String message,
Review Comment:
removed.
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpBasicAuthenticationHandler.java:
##########
@@ -112,14 +115,17 @@ public void channelRead(final ChannelHandlerContext ctx, final Object msg) {
credentials.get(PROPERTY_USERNAME), address, authClassParts[authClassParts.length - 1]);
}
} catch (AuthenticationException ae) {
- sendError(ctx, msg);
+ sendError(ctx, msg, ae);
}
}
}
- private void sendError(final ChannelHandlerContext ctx, final Object msg) {
- // Close the connection as soon as the error message is sent.
- ctx.writeAndFlush(new DefaultFullHttpResponse(HTTP_1_1, UNAUTHORIZED)).addListener(ChannelFutureListener.CLOSE);
+ private void sendError(final ChannelHandlerContext ctx, final Object msg, final Throwable t) {
Review Comment:
reused
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] driver error handling [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2594:
URL: https://github.com/apache/tinkerpop/pull/2594#discussion_r1592972739
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpBasicAuthenticationHandler.java:
##########
@@ -64,32 +70,29 @@ public void channelRead(final ChannelHandlerContext ctx, final Object msg) {
if (msg instanceof FullHttpMessage) {
final FullHttpMessage request = (FullHttpMessage) msg;
if (!request.headers().contains("Authorization")) {
- sendError(ctx, msg);
+ sendError(ctx, msg, "Missing or incorrect credentials");
Review Comment:
Nit: make static String for "Missing or...".
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] driver error handling [tinkerpop]
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk merged PR #2594:
URL: https://github.com/apache/tinkerpop/pull/2594
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] driver error handling [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on PR #2594:
URL: https://github.com/apache/tinkerpop/pull/2594#issuecomment-2099279500
LGTM
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] driver error handling [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2594:
URL: https://github.com/apache/tinkerpop/pull/2594#discussion_r1592972248
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpBasicAuthenticationHandler.java:
##########
@@ -112,14 +115,17 @@ public void channelRead(final ChannelHandlerContext ctx, final Object msg) {
credentials.get(PROPERTY_USERNAME), address, authClassParts[authClassParts.length - 1]);
}
} catch (AuthenticationException ae) {
- sendError(ctx, msg);
+ sendError(ctx, msg, ae);
}
}
}
- private void sendError(final ChannelHandlerContext ctx, final Object msg) {
- // Close the connection as soon as the error message is sent.
- ctx.writeAndFlush(new DefaultFullHttpResponse(HTTP_1_1, UNAUTHORIZED)).addListener(ChannelFutureListener.CLOSE);
+ private void sendError(final ChannelHandlerContext ctx, final Object msg, final Throwable t) {
Review Comment:
Could we reuse HttpHandlerUtil's sendError here?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org