You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "phanindhra876 (via GitHub)" <gi...@apache.org> on 2023/06/29 14:58:02 UTC
[GitHub] [tinkerpop] phanindhra876 opened a new pull request, #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
phanindhra876 opened a new pull request, #2115:
URL: https://github.com/apache/tinkerpop/pull/2115
Users/Providers who wish to use GraphSON-1.0 in text format should make serializer
`GraphSONMessageSerializerV1d0` default serializer. Alternatively introduce new mimeType `application/vnd.gremlin-v1.0+json;sparse=true` to let users/providers use GraphSON-1.0 in text format without changing defaults.
--
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
[GitHub] [tinkerpop] spmallette commented on pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#issuecomment-1631003037
Merged and here's the follow-on commit that renames "sparse" and does some other things to bring a bit more consistency here: https://github.com/apache/tinkerpop/commit/14f2d31b5ebc7cedd50173f746cf8aaa95dfaa0e
--
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
[GitHub] [tinkerpop] saikiranboga commented on pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "saikiranboga (via GitHub)" <gi...@apache.org>.
saikiranboga commented on PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#issuecomment-1630616327
What does `sparse=true` parameter represent in the mime type?
--
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
[GitHub] [tinkerpop] saikiranboga commented on pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "saikiranboga (via GitHub)" <gi...@apache.org>.
saikiranboga commented on PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#issuecomment-1630673880
> Using `application/vnd.gremlin-v1.0+json` it conflicts with GraphSONMessageSerializerGremlinV1d0. User/provider can never configure server to use both of them(same problem as `application/json`). Adding `sparse=true` distinguishes this from `application/vnd.gremlin-v1.0+json` not so much that it's a different format but very similar. We can change it to something like `application/vnd.gremlin-v1.0+json+text` or similar if sparse feels odd.
Thanks. Something like `type=true` seem more closer to the intention.
`strict/sparse` could support more under the naming than closely linking to just type, but I am unsure if there are any plans to make the serializer support more than this.
--
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
[GitHub] [tinkerpop] codecov-commenter commented on pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#issuecomment-1614409926
## [Codecov](https://app.codecov.io/gh/apache/tinkerpop/pull/2115?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#2115](https://app.codecov.io/gh/apache/tinkerpop/pull/2115?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (4908dd6) into [3.6-dev](https://app.codecov.io/gh/apache/tinkerpop/commit/0903ba7a40028889b41203e89a67e96ff0854e3d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (0903ba7) will **decrease** coverage by `5.12%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## 3.6-dev #2115 +/- ##
=============================================
- Coverage 75.31% 70.20% -5.12%
=============================================
Files 1047 24 -1023
Lines 62721 3500 -59221
Branches 6872 0 -6872
=============================================
- Hits 47239 2457 -44782
+ Misses 12933 876 -12057
+ Partials 2549 167 -2382
```
[see 1023 files with indirect coverage changes](https://app.codecov.io/gh/apache/tinkerpop/pull/2115/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
[GitHub] [tinkerpop] phanindhra876 commented on pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "phanindhra876 (via GitHub)" <gi...@apache.org>.
phanindhra876 commented on PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#issuecomment-1630630977
Using `application/vnd.gremlin-v1.0+json` it conflicts with GraphSONMessageSerializerGremlinV1d0. User/provider can never configure server to use both of them(same problem as `application/json`). Adding `sparse=true` distinguishes this from `application/vnd.gremlin-v1.0+json` not so much that it's a different format but very similar. We can change it to something like `application/vnd.gremlin-v1.0+json+text` or similar if sparse feels odd.
--
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
[GitHub] [tinkerpop] phanindhra876 commented on pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "phanindhra876 (via GitHub)" <gi...@apache.org>.
phanindhra876 commented on PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#issuecomment-1630579269
Please confirm if there are any concerns with this change. Else i will add an entry to capture this in 3.6.5 changelog and will squash the commit to merge the change,
--
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
[GitHub] [tinkerpop] phanindhra876 commented on pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "phanindhra876 (via GitHub)" <gi...@apache.org>.
phanindhra876 commented on PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#issuecomment-1614514779
@vkagamlyk Added UT to verify that change works. Please review now.
--
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
[GitHub] [tinkerpop] phanindhra876 commented on a diff in pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "phanindhra876 (via GitHub)" <gi...@apache.org>.
phanindhra876 commented on code in PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#discussion_r1253450794
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ser/Serializers.java:
##########
@@ -54,6 +55,8 @@ public MessageSerializer<?> simpleInstance() {
return new GraphSONMessageSerializerV3d0();
case SerTokens.MIME_GRAPHSON_V1D0:
return new GraphSONMessageSerializerGremlinV1d0();
+ case SerTokens.MIME_GRAPHSON_V1D0_SPARSE:
Review Comment:
Correction. Seems like on client side `GraphSONMessageSerializerV3d0` will be used as serializer for `MIME_JSON` as you can see on L54(old) in the same class.
--
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
[GitHub] [tinkerpop] spmallette commented on pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#issuecomment-1630731873
working on the merge now. was ok with "sparse" but agree with using "type" instead now that i look more carefully. i'll add a follow-on commit to do that.
--
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
[GitHub] [tinkerpop] phanindhra876 commented on a diff in pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "phanindhra876 (via GitHub)" <gi...@apache.org>.
phanindhra876 commented on code in PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#discussion_r1253447728
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ser/Serializers.java:
##########
@@ -54,6 +55,8 @@ public MessageSerializer<?> simpleInstance() {
return new GraphSONMessageSerializerV3d0();
case SerTokens.MIME_GRAPHSON_V1D0:
return new GraphSONMessageSerializerGremlinV1d0();
+ case SerTokens.MIME_GRAPHSON_V1D0_SPARSE:
Review Comment:
On server side it depends on the order of serializers defined in serializerSettings. Whichever is the first in the serializerSettings field will be picked for application/json.
I believe this snippet of code is used in gremlin driver. Not sure how it works on client side.
--
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
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#discussion_r1253424441
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ser/Serializers.java:
##########
@@ -54,6 +55,8 @@ public MessageSerializer<?> simpleInstance() {
return new GraphSONMessageSerializerV3d0();
case SerTokens.MIME_GRAPHSON_V1D0:
return new GraphSONMessageSerializerGremlinV1d0();
+ case SerTokens.MIME_GRAPHSON_V1D0_SPARSE:
Review Comment:
What serializer will be used for `MIME_JSON`?
--
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
[GitHub] [tinkerpop] spmallette merged pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette merged PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115
--
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
[GitHub] [tinkerpop] xiazcy commented on pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#issuecomment-1624107085
LGTM, VOTE +1
--
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
[GitHub] [tinkerpop] vkagamlyk commented on pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#issuecomment-1613797694
Great example for new mime type can be found in [this PR](https://github.com/apache/tinkerpop/pull/2070/files#diff-8ff383b057866ad668f388dcab65b4e4b26531a82b7b9fc462aff9881ce3c51c)
--
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
[GitHub] [tinkerpop] phanindhra876 commented on a diff in pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "phanindhra876 (via GitHub)" <gi...@apache.org>.
phanindhra876 commented on code in PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#discussion_r1253447728
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ser/Serializers.java:
##########
@@ -54,6 +55,8 @@ public MessageSerializer<?> simpleInstance() {
return new GraphSONMessageSerializerV3d0();
case SerTokens.MIME_GRAPHSON_V1D0:
return new GraphSONMessageSerializerGremlinV1d0();
+ case SerTokens.MIME_GRAPHSON_V1D0_SPARSE:
Review Comment:
On server side it depends on the order of serializers defined in serializerSettings. Which is the first in the serializerSettings field will be picked for application/json.
I believe this snippet of code is used in gremlin driver. Not sure how it works on client side.
--
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
[GitHub] [tinkerpop] vkagamlyk commented on pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#issuecomment-1613540698
Hi @phanindhra876,
Thank you for contribution!
It would be nice to add a test to confirm that this code works
--
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
[GitHub] [tinkerpop] phanindhra876 commented on pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "phanindhra876 (via GitHub)" <gi...@apache.org>.
phanindhra876 commented on PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#issuecomment-1613791534
Sure @vkagamlyk. I didnt find any tests for Graphson-1.0. Maybe we need a new test in `GremlinServerHttpIntegrateTest` for this change?
--
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
[GitHub] [tinkerpop] phanindhra876 commented on pull request #2115: [TINKERPOP-2963] Introduce new mimeType to use Graphson-1.0 text serializer
Posted by "phanindhra876 (via GitHub)" <gi...@apache.org>.
phanindhra876 commented on PR #2115:
URL: https://github.com/apache/tinkerpop/pull/2115#issuecomment-1613798472
Thanks for the reference. Will add necessary tests.
--
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