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 2023/11/01 00:51:07 UTC
[PR] Bytecode support for HTTP requests [tinkerpop]
vkagamlyk opened a new pull request, #2318:
URL: https://github.com/apache/tinkerpop/pull/2318
What works:
old requests like `{ "gremlin":"g.V()" }` with Content-Type `application/json`
new requests with gremlin Content-Type like `application/vnd.gremlin-v3.0+json`
driver integration tests: `shouldSubmitBytecodeWithGraphSON()` and `shouldSubmitBytecodeWithGraphSON()`
Known issues:
client sends binary requests to server
server responds with text
a bit mess with serialization/deserialization (see `obtainHeader()`)
not implemented/verified request option handling
--
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] Bytecode support for HTTP requests [tinkerpop]
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk closed pull request #2318: Bytecode support for HTTP requests
URL: https://github.com/apache/tinkerpop/pull/2318
--
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] Bytecode support for HTTP requests [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2318:
URL: https://github.com/apache/tinkerpop/pull/2318#discussion_r1378654955
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpHandlerUtil.java:
##########
@@ -74,6 +78,39 @@ public class HttpHandlerUtil {
public static RequestMessage getRequestMessageFromHttpRequest(final FullHttpRequest request) {
final String contentType = Optional.ofNullable(request.headers().get(HttpHeaderNames.CONTENT_TYPE)).orElse("application/json");
+ if (contentType.equals(SerTokens.MIME_GRAPHSON_V3)) {
+ final GraphSONMessageSerializerV3 serializer = new GraphSONMessageSerializerV3();
+ try {
+ final ByteBuf buffer = request.content();
+
+ final int first = buffer.readByte();
+ // !!! skip header. Why we write this to json?
Review Comment:
like every odd looking design choice, i think it goes back to an old concept. the gremlin server protocol was written such that the request message included the content type in the serialized message itself:
https://tinkerpop.apache.org/docs/3.7.0/dev/provider/#_graph_driver_provider_requirements
in this way the server could detect the serializer used for the message and choose the right one to decode it. For HTTP it is a bit redundant to do this because we supply `Content-Type` explicitly, but when we had a pure NIO based protocol to support we used that as the means for supplying this information. It also stuck for websockets where it is still in use, though in retrospect we probably could have passed that information once in the handshake.
Interestingly, the fact that we encode the serializer here offers a possible solution to some issues faced here with this change. The `Content-Type` for the payload on the `POST` could always stay `application/json` and preserve the old functionality because the serialized `RequestMessage` itself contains the serializer to use to decode it. There would be no need to add an custom header to tell the server what serializer to use. Serializing a whole `RequestMessage` into the JSON request structure for HTTP seems like a fairly clean solution doesn't it?
--
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] Bytecode support for HTTP requests [tinkerpop]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2318:
URL: https://github.com/apache/tinkerpop/pull/2318#issuecomment-1788256278
## [Codecov](https://app.codecov.io/gh/apache/tinkerpop/pull/2318?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#2318](https://app.codecov.io/gh/apache/tinkerpop/pull/2318?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (675364f) into [3.7-dev](https://app.codecov.io/gh/apache/tinkerpop/commit/7dc9df78e02ad96a56d2ce1f323bf2805ca3c3a2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (7dc9df7) will **decrease** coverage by `0.22%`.
> The diff coverage is `16.21%`.
```diff
@@ Coverage Diff @@
## 3.7-dev #2318 +/- ##
=============================================
- Coverage 76.19% 75.98% -0.22%
- Complexity 12932 12953 +21
=============================================
Files 1046 1071 +25
Lines 61005 64959 +3954
Branches 7206 7211 +5
=============================================
+ Hits 46485 49357 +2872
- Misses 12056 12938 +882
- Partials 2464 2664 +200
```
| [Files](https://app.codecov.io/gh/apache/tinkerpop/pull/2318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...lin/driver/handler/HttpGremlinResponseDecoder.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1kcml2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3RpbmtlcnBvcC9ncmVtbGluL2RyaXZlci9oYW5kbGVyL0h0dHBHcmVtbGluUmVzcG9uc2VEZWNvZGVyLmphdmE=) | `0.00% <ø> (ø)` | |
| [...lin/server/handler/HttpGremlinEndpointHandler.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3RpbmtlcnBvcC9ncmVtbGluL3NlcnZlci9oYW5kbGVyL0h0dHBHcmVtbGluRW5kcG9pbnRIYW5kbGVyLmphdmE=) | `42.16% <50.00%> (+0.09%)` | :arrow_up: |
| [...mlin/driver/handler/HttpGremlinRequestEncoder.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1kcml2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3RpbmtlcnBvcC9ncmVtbGluL2RyaXZlci9oYW5kbGVyL0h0dHBHcmVtbGluUmVxdWVzdEVuY29kZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...nkerpop/gremlin/groovy/engine/GremlinExecutor.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1ncm9vdnkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3RpbmtlcnBvcC9ncmVtbGluL2dyb292eS9lbmdpbmUvR3JlbWxpbkV4ZWN1dG9yLmphdmE=) | `84.92% <36.36%> (-2.74%)` | :arrow_down: |
| [...kerpop/gremlin/server/handler/HttpHandlerUtil.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3RpbmtlcnBvcC9ncmVtbGluL3NlcnZlci9oYW5kbGVyL0h0dHBIYW5kbGVyVXRpbC5qYXZh) | `18.48% <0.00%> (-3.30%)` | :arrow_down: |
... and [35 files with indirect coverage changes](https://app.codecov.io/gh/apache/tinkerpop/pull/2318/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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] Bytecode support for HTTP requests [tinkerpop]
Posted by "rdtr (via GitHub)" <gi...@apache.org>.
rdtr commented on code in PR #2318:
URL: https://github.com/apache/tinkerpop/pull/2318#discussion_r1388537997
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpHandlerUtil.java:
##########
@@ -70,10 +76,53 @@ public class HttpHandlerUtil {
/**
* Convert a http request into a {@link RequestMessage}.
+ * There are 2 payload types options here.
+ * 1.
+ * existing https://tinkerpop.apache.org/docs/current/reference/#connecting-via-http
+ * intended to use with curl, postman, etc. by users
+ * both GET and POST
+ * Content-Type header can be empty or application/json
+ * Accept header can be any, most useful can be application/json, text/plain, application/vnd.gremlin-v3.0+json and application/vnd.gremlin-v3.0+json;types=false
+ * Request body example: { "gremlin": "g.V()" }
+ * 2.
+ * experimental payload with serialized RequestMessage
+ * intended for drivers/GLV's. Support both gremlin and bytecode queries.
+ * only POST
+ * Content-Type is defined by used serializer, expected type GraphSON application/vnd.gremlin-v3.0+json or GraphBinary application/vnd.graphbinary-v1.0. Untyped GraphSON is not supported, it can't deserialize bytecode
+ * Accept header can be any.
+ * Request body contains serialized RequestMessage
*/
- public static RequestMessage getRequestMessageFromHttpRequest(final FullHttpRequest request) {
- final String contentType = Optional.ofNullable(request.headers().get(HttpHeaderNames.CONTENT_TYPE)).orElse("application/json");
+ public static RequestMessage getRequestMessageFromHttpRequest(final FullHttpRequest request,
+ Map<String, MessageSerializer<?>> serializers) throws SerializationException {
+ final String contentType = request.headers().get(HttpHeaderNames.CONTENT_TYPE);
+
+ if (request.method() == POST && contentType != null && !contentType.equals("application/json") && serializers.containsKey(contentType)) {
+ final MessageSerializer<?> serializer = serializers.get(contentType);
+
+ final ByteBuf buffer = request.content();
+
+ // additional validation for header
+ final int first = buffer.readByte();
+ if (first != 0x7b) {
Review Comment:
can you add a comment that we are checking `'{'` ?
--
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] Bytecode support for HTTP requests [tinkerpop]
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2318:
URL: https://github.com/apache/tinkerpop/pull/2318#discussion_r1388480265
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java:
##########
@@ -262,7 +269,7 @@ public void channelRead(final ChannelHandlerContext ctx, final Object msg) {
attemptCommit(requestMessage.getArg(Tokens.ARGS_ALIASES), graphManager, settings.strictTransactionManagement);
try {
- return Unpooled.wrappedBuffer(serializer.getValue1().serializeResponseAsString(responseMessage, ctx.alloc()).getBytes(UTF8));
+ return Unpooled.wrappedBuffer(serializer.getValue1().serializeResponseAsBinary(responseMessage, ctx.alloc()));
Review Comment:
$ curl -v -H Accept:application/vnd.graphbinary-v1.0 http://localhost:45940?gremlin=gmodern.V%28%29
* processing: http://localhost:45940?gremlin=gmodern.V%28%29
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0* Trying [::1]:45940...
* Connected to localhost (::1) port 45940
> GET /?gremlin=gmodern.V%28%29 HTTP/1.1
> Host: localhost:45940
> User-Agent: curl/8.2.1
> Accept:application/vnd.graphbinary-v1.0
>
< HTTP/1.1 200 OK
< content-type: application/vnd.graphbinary-v1.0
< content-length: 629
<
{ [629 bytes data]
--
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] Bytecode support for HTTP requests [tinkerpop]
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2318:
URL: https://github.com/apache/tinkerpop/pull/2318#discussion_r1388480265
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java:
##########
@@ -262,7 +269,7 @@ public void channelRead(final ChannelHandlerContext ctx, final Object msg) {
attemptCommit(requestMessage.getArg(Tokens.ARGS_ALIASES), graphManager, settings.strictTransactionManagement);
try {
- return Unpooled.wrappedBuffer(serializer.getValue1().serializeResponseAsString(responseMessage, ctx.alloc()).getBytes(UTF8));
+ return Unpooled.wrappedBuffer(serializer.getValue1().serializeResponseAsBinary(responseMessage, ctx.alloc()));
Review Comment:
```
$ curl -v -H Accept:application/vnd.graphbinary-v1.0 http://localhost:45940?gremlin=gmodern.V%28%29
* processing: http://localhost:45940?gremlin=gmodern.V%28%29
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0* Trying [::1]:45940...
* Connected to localhost (::1) port 45940
> GET /?gremlin=gmodern.V%28%29 HTTP/1.1
> Host: localhost:45940
> User-Agent: curl/8.2.1
> Accept:application/vnd.graphbinary-v1.0
>
< HTTP/1.1 200 OK
< content-type: application/vnd.graphbinary-v1.0
< content-length: 629
<
{ [629 bytes data]
```
--
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] Bytecode support for HTTP requests [tinkerpop]
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2318:
URL: https://github.com/apache/tinkerpop/pull/2318#discussion_r1381999718
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpHandlerUtil.java:
##########
@@ -71,9 +74,32 @@ public class HttpHandlerUtil {
/**
* Convert a http request into a {@link RequestMessage}.
*/
- public static RequestMessage getRequestMessageFromHttpRequest(final FullHttpRequest request) {
- final String contentType = Optional.ofNullable(request.headers().get(HttpHeaderNames.CONTENT_TYPE)).orElse("application/json");
+ public static RequestMessage getRequestMessageFromHttpRequest(final FullHttpRequest request,
+ Map<String, MessageSerializer<?>> serializers) throws SerializationException {
+ final String contentType = request.headers().get(HttpHeaderNames.CONTENT_TYPE);
+
+ if (!contentType.equals("application/json") && serializers.containsKey(contentType)) {
Review Comment:
what types of requests I think to support:
1. existing https://tinkerpop.apache.org/docs/current/reference/#connecting-via-http
intended to use with curl, postman, etc by users
both GET and POST
`Content-Type` header can be empty or `application/json`
`Accept` header can be any, but I think most useful can be `application/json`, `text/plain`, `application/vnd.gremlin-v3.0+json` and `application/vnd.gremlin-v3.0+json;types=false`
Request body example: `{ "gremlin": "g.V()" }`
We can introduce some model to deserialize with object mapper or continue to use [existing code](https://github.com/apache/tinkerpop/blob/35ac4963f72699dd123fbc99022824df37988f94/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpHandlerUtil.java#L74)
2. newly introduced payload with serialized `RequestMessage`
intended for drivers/GLV's. Support both gremlin and bytecode queries.
only POST
`Content-Type` is defined by used serializer, expected type GraphSON `application/vnd.gremlin-v3.0+json` or GraphBinary `application/vnd.graphbinary-v1.0`. Untyped GraphSON is not supported, it can't deserialize bytecode
`Accept` header can be any. I may add validation that used same serializer for request and response.
Request body contains serialized `RequestMessage`
To distinguish this types of requests I propose to use [Content-Type HTTP header](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type). I think that using a payload to determine the data type is an unnecessary complication and not HTTP way.
`GraphSONUntypedMessageSerializerV3` can't deserialize bytecode request, it knows nothing about steps or parameters, everything turns into HashMap of HashMap.
--
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] Bytecode support for HTTP requests [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2318:
URL: https://github.com/apache/tinkerpop/pull/2318#discussion_r1385682929
##########
docs/src/dev/provider/index.asciidoc:
##########
@@ -1033,6 +1036,21 @@ WebSocket, Gremlin Server can accept a packet frame using a "text" (1) or a "bin
is a bit more limited in that Gremlin Server will always process the body of that request as JSON. Generally speaking
"text" is just for testing purposes.
+For HTTP can be used following serializers distinguished by `Content-Type` HTTP header
Review Comment:
```suggestion
For HTTP the following serializers can be used, distinguished by `Content-Type` HTTP header
```
##########
docs/src/dev/provider/index.asciidoc:
##########
@@ -1033,6 +1036,21 @@ WebSocket, Gremlin Server can accept a packet frame using a "text" (1) or a "bin
is a bit more limited in that Gremlin Server will always process the body of that request as JSON. Generally speaking
"text" is just for testing purposes.
+For HTTP can be used following serializers distinguished by `Content-Type` HTTP header
+[width="100%",cols="3,10, 10",options="header"]
+|=========================================================
+|Serializer | Content-Type| Description
+|GraphBinaryMessageSerializerV1 | application/vnd.graphbinary-v1.0 | Efficient binary format recommended for default use.
+|GraphSONMessageSerializerV3 | application/vnd.gremlin-v3.0+json | Json-compatible format containing types of all data.
+|GraphSONUntypedMessageSerializerV3 | application/vnd.gremlin-v3.0+json;types=false | Json-compatible format without metadata. For this reason it cannot be used for transmission of ByteCode requests.
+|=========================================================
+
+Same options can be used for `Accept` Header. By default, same serializer will handle both request and response.
+Only HTTP Post requests are supported.
+Both Gremlin strings and Gremlin bytecode requests may be supported.
+
+NOTE: All this HTTP options designed for GLV's. For usage with browsers, curl, Postman and other tools see link:https://tinkerpop.apache.org/docs/x.y.z/reference/#connecting-via-http[Connecting via HTTP section].
Review Comment:
```suggestion
NOTE: All of the above HTTP options are designed for use with GLV's. For usage with browsers, cURL, Postman and other tools see link:https://tinkerpop.apache.org/docs/x.y.z/reference/#connecting-via-http[Connecting via HTTP section].
```
##########
docs/src/dev/provider/index.asciidoc:
##########
@@ -1033,6 +1036,21 @@ WebSocket, Gremlin Server can accept a packet frame using a "text" (1) or a "bin
is a bit more limited in that Gremlin Server will always process the body of that request as JSON. Generally speaking
"text" is just for testing purposes.
+For HTTP can be used following serializers distinguished by `Content-Type` HTTP header
+[width="100%",cols="3,10, 10",options="header"]
+|=========================================================
+|Serializer | Content-Type| Description
+|GraphBinaryMessageSerializerV1 | application/vnd.graphbinary-v1.0 | Efficient binary format recommended for default use.
+|GraphSONMessageSerializerV3 | application/vnd.gremlin-v3.0+json | Json-compatible format containing types of all data.
+|GraphSONUntypedMessageSerializerV3 | application/vnd.gremlin-v3.0+json;types=false | Json-compatible format without metadata. For this reason it cannot be used for transmission of ByteCode requests.
+|=========================================================
+
+Same options can be used for `Accept` Header. By default, same serializer will handle both request and response.
+Only HTTP Post requests are supported.
+Both Gremlin strings and Gremlin bytecode requests may be supported.
Review Comment:
```suggestion
Both Gremlin scripts and Gremlin `Bytecode` requests are supported.
```
##########
docs/src/dev/provider/index.asciidoc:
##########
@@ -1033,6 +1036,21 @@ WebSocket, Gremlin Server can accept a packet frame using a "text" (1) or a "bin
is a bit more limited in that Gremlin Server will always process the body of that request as JSON. Generally speaking
"text" is just for testing purposes.
+For HTTP can be used following serializers distinguished by `Content-Type` HTTP header
+[width="100%",cols="3,10, 10",options="header"]
+|=========================================================
+|Serializer | Content-Type| Description
+|GraphBinaryMessageSerializerV1 | application/vnd.graphbinary-v1.0 | Efficient binary format recommended for default use.
+|GraphSONMessageSerializerV3 | application/vnd.gremlin-v3.0+json | Json-compatible format containing types of all data.
+|GraphSONUntypedMessageSerializerV3 | application/vnd.gremlin-v3.0+json;types=false | Json-compatible format without metadata. For this reason it cannot be used for transmission of ByteCode requests.
+|=========================================================
Review Comment:
I wonder if we want to give a bit more details on the design? E.g. how the request is handled when it comes in? Why we are using Content-Type, what should the payload looks like, and some notes on what happens if you used different serializers for request/response? Also some potential issue people may encounter with this (especially if spec isn't followed)?
Also I wonder if this needs to be in a separate section, it feels a bit odd being sticked into the middle of this doc, but I don't have any good recommendations right now either.
--
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] Bytecode support for HTTP requests [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2318:
URL: https://github.com/apache/tinkerpop/pull/2318#discussion_r1387939249
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java:
##########
@@ -262,7 +269,7 @@ public void channelRead(final ChannelHandlerContext ctx, final Object msg) {
attemptCommit(requestMessage.getArg(Tokens.ARGS_ALIASES), graphManager, settings.strictTransactionManagement);
try {
- return Unpooled.wrappedBuffer(serializer.getValue1().serializeResponseAsString(responseMessage, ctx.alloc()).getBytes(UTF8));
+ return Unpooled.wrappedBuffer(serializer.getValue1().serializeResponseAsBinary(responseMessage, ctx.alloc()));
Review Comment:
i don't remember why it was chosen to encode this way (and perhaps it was wrong to do) but this seems like a breaking change. The following is from 3.7.0 and would no longer return the same response, right?:
```
$ curl -v -H Accept:application/vnd.graphbinary-v1.0 http://localhost:8182?gremlin=g.V%28%29
* Trying 127.0.0.1:8182...
* Connected to localhost (127.0.0.1) port 8182 (#0)
> GET /?gremlin=g.V%28%29 HTTP/1.1
> Host: localhost:8182
> User-Agent: curl/7.81.0
> Accept:application/vnd.graphbinary-v1.0
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< content-type: application/vnd.graphbinary-v1.0
< content-length: 872
<
* Connection #0 to host localhost left intact
gQDX0dKWp4pPrJQSEq6fq21cAAAAyAAAAAAAAAAAAAAAAAAJAAAAAAYRAAIAAAAAAAAAAAEAAAAGcGVyc29uCQAAAAACEgACAAAAAAAAAAAAAAAABG5hbWUDAAAAAAVtYXJrb/4BCQAAAAAAEgACAAAAAAAAAAABAAAAA2FnZQEAAAAAHf4BCQAAAAAAEQACAAAAAAAAAAACAAAABnBlcnNvbgkAAAAAAhIAAgAAAAAAAAAAAgAAAARuYW1lAwAAAAAFdmFkYXP+AQkAAAAAABIAAgAAAAAAAAAAAwAAAANhZ2UBAAAAABv+AQkAAAAAABEAAgAAAAAAAAAAAwAAAAhzb2Z0d2FyZQkAAAAAAhIAAgAAAAAAAAAABAAAAARuYW1lAwAAAAADbG9w/gEJAAAAAAASAAIAAAAAAAAAAAUAAAAEbGFuZwMAAAAABGphdmH+AQkAAAAAABEAAgAAAAAAAAAABAAAAAZwZXJzb24JAAAAAAISAAIAAAAAAAAAAAYAAAAEbmFtZQMAAAAABGpvc2j+AQkAAAAAABIAAgAAAAAAAAAABwAAAANhZ2UBAAAAACD+AQkAAAAAABEAAgAAAAAAAAAABQAAAAhzb2Z0d2FyZQkAAAAAAhIAAgAAAAAAAAAACAAAAARuYW1lAwAAAAAGcmlwcGxl/gEJAAAAAAASAAIAAAAAAAAAAAkAAAAEbGFuZwMAAAAABGphdmH+AQkAAAAAABEAAgAAAAAAAAAABgAAAAZwZXJzb24JAAAAAAISAAIAAAAAAAAAAAoAAAAEbmFtZQMAAAAABXBldGVy/gEJAAAAAAASAAIAAAAAAAAAAAsAAAADYWdlAQAAAAAj/gEJAAAAAAA=
```
--
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] Bytecode support for HTTP requests [tinkerpop]
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on PR #2318:
URL: https://github.com/apache/tinkerpop/pull/2318#issuecomment-1806180700
re-targeted to 3.6 in https://github.com/apache/tinkerpop/pull/2336
--
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] Bytecode support for HTTP requests [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2318:
URL: https://github.com/apache/tinkerpop/pull/2318#discussion_r1382106035
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpHandlerUtil.java:
##########
@@ -71,9 +74,32 @@ public class HttpHandlerUtil {
/**
* Convert a http request into a {@link RequestMessage}.
*/
- public static RequestMessage getRequestMessageFromHttpRequest(final FullHttpRequest request) {
- final String contentType = Optional.ofNullable(request.headers().get(HttpHeaderNames.CONTENT_TYPE)).orElse("application/json");
+ public static RequestMessage getRequestMessageFromHttpRequest(final FullHttpRequest request,
+ Map<String, MessageSerializer<?>> serializers) throws SerializationException {
+ final String contentType = request.headers().get(HttpHeaderNames.CONTENT_TYPE);
+
+ if (!contentType.equals("application/json") && serializers.containsKey(contentType)) {
Review Comment:
It seems strange to me to use the `Content-Type` for anything more than a serialization rule. If I understand correctly, by your definition of what you want to support, you can only access the old HTTP API with `application/json` but if you make it `application/vnd.gremlin-v3.0+json` then the payload must be a `RequestMessage` or else it won't work. I suppose we didn't support `application/vnd.gremlin-v3.0+json` as a `Content-Type` there before so maybe that's ok, but now that there is a path that allows you to POST that format, it wouldn't surprise me if folks tried it, especially since from the perspective `Accept` you can request `application/json` and assuming you've configured the server that way you could get back a `application/vnd.gremlin-v3.0+json` formatted payload.
well, please think through the validation on requests here carefully and write good documentation to clarify how this works. we need to make sure folks understand the API they are calling and what `Content-Type` and `Accept` headers the need to use to get the results that they want.
--
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] Bytecode support for HTTP requests [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2318:
URL: https://github.com/apache/tinkerpop/pull/2318#discussion_r1381530244
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpHandlerUtil.java:
##########
@@ -71,9 +74,32 @@ public class HttpHandlerUtil {
/**
* Convert a http request into a {@link RequestMessage}.
*/
- public static RequestMessage getRequestMessageFromHttpRequest(final FullHttpRequest request) {
- final String contentType = Optional.ofNullable(request.headers().get(HttpHeaderNames.CONTENT_TYPE)).orElse("application/json");
+ public static RequestMessage getRequestMessageFromHttpRequest(final FullHttpRequest request,
+ Map<String, MessageSerializer<?>> serializers) throws SerializationException {
+ final String contentType = request.headers().get(HttpHeaderNames.CONTENT_TYPE);
+
+ if (!contentType.equals("application/json") && serializers.containsKey(contentType)) {
Review Comment:
the `!contentType.equals("application/json")` condition means that you use a naked JSON `ObjectMapper` to process the request. with the `Accept` header a user could specify "application/json" to mean Typed GraphSON 3 (for example) if the server was not configured that way:
https://github.com/apache/tinkerpop/blob/master/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/ser/GraphSONMessageSerializerV3.java#L85
I guess for the `Content-Type` and the HTTP specification it's expected that the client be explicit in terms of what the payload is and there is no form of content negotiation that goes on there. I suppose that having it work this way spares the user from having to properly configure the server, such that `application/json` ties to a Untyped GraphSON serializer (which i can only presume would work as well as a naked `ObjectMapper`).
could you test to see if that is the case? if the `GraphSONUntypedMessageSerializerV3` decodes the request in the same fashion that the naked `ObjectMapper` does, then I think it would remove doubts about any inconsistencies we'd need to warn users about. Moreover, we could technically remove the naked` ObjectMapper` and this condition and just try to decode with the serializers.
Perhaps said another way, instead of using `!contentType.equals("application/json")` to control flow here, use the presence/absence of a header in the payload to do that, then if it is there you just use the `MessageSerializer` and if it is not you pull the `Mapper` object from the serializer:
https://github.com/apache/tinkerpop/blob/master/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/ser/AbstractGraphSONMessageSerializerV2.java#L162-L165
and use that to decode the payload. I think one neat thing about this approach is that you could conceivable then send your payload with or without the header potentially (though the underlying `Mapper` object between GraphSON and GraphBinary don't have the same interfaces but theoretically it seems like it could work).
one other point here perhaps it would be good to check the verb earlier to avoid this path: `request.method() == POST`?
--
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] Bytecode support for HTTP requests [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2318:
URL: https://github.com/apache/tinkerpop/pull/2318#discussion_r1387872119
##########
docs/src/dev/provider/index.asciidoc:
##########
@@ -1033,6 +1036,21 @@ WebSocket, Gremlin Server can accept a packet frame using a "text" (1) or a "bin
is a bit more limited in that Gremlin Server will always process the body of that request as JSON. Generally speaking
"text" is just for testing purposes.
+For HTTP can be used following serializers distinguished by `Content-Type` HTTP header
+[width="100%",cols="3,10, 10",options="header"]
+|=========================================================
+|Serializer | Content-Type| Description
+|GraphBinaryMessageSerializerV1 | application/vnd.graphbinary-v1.0 | Efficient binary format recommended for default use.
+|GraphSONMessageSerializerV3 | application/vnd.gremlin-v3.0+json | Json-compatible format containing types of all data.
+|GraphSONUntypedMessageSerializerV3 | application/vnd.gremlin-v3.0+json;types=false | Json-compatible format without metadata. For this reason it cannot be used for transmission of ByteCode requests.
+|=========================================================
Review Comment:
I'd echo @xiazcy concerns here. I sense that it's possible that some of the docs have even fallen behind here as I read the content around what's been added. This feature remains experimental so when I was thinking about documentation here I was mostly thinking about more extensive javadoc rather than official changes to asciidoc. When the experimental label is dropped we can add this sort of content.
--
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