TINKERPOP-1874 P does not appear to be serialized consistently in GraphSON#784
Merged
TINKERPOP-1874 P does not appear to be serialized consistently in GraphSON#784
Conversation
Validates various forms of serialization with focus on GraphSON and GLVs (where there were problems).
GraphSON deserialization of P.within and P.without expects a single collection regardless of the number of values, so even a single argument should serialize to a list.
Merged
jorgebay
reviewed
Jan 24, 2018
| } | ||
| ---- | ||
|
|
||
| ==== P within |
Contributor
There was a problem hiding this comment.
Good idea to add within and without examples. Can we also add a little introductory text saying that P expects a single value or a list of values, except in the case of within and without where its always a list of values?
Contributor
Author
There was a problem hiding this comment.
that's a good idea. i will make a note to do it after i merge if that's ok. i'd rather not have to re-work the PR on master as a result of that little change. i have a bunch of cleanup to do anyway after these merge.
Contributor
|
I've added a small suggestion. |
robertdale
reviewed
Jan 24, 2018
| ==== Authentication Challenge | ||
|
|
||
| When authentication is enabled, an initial request to the server will result in an authentication challenge. The typical response message will appear as follows, but handling it could be different dependending on the SASL implementation (e.g. multiple challenges maybe requested in some cases, but not in the default provided by Gremlin Server). | ||
| When authentication is enabled, an initial request to the server will result in an authentication challenge. The typical response message will appear as follows, but handling it could be different dependending on the SASL implementation (e.g. multiple challenges maybe requested in some cases, but no in the default provided by Gremlin Server). |
Member
There was a problem hiding this comment.
Was this meant to be changed?
/no/not/
/maybe/may be/
Contributor
Author
There was a problem hiding this comment.
that's so weird....i will fix it. i thought i already had. +1 otherwise? 😄
robertdale
requested changes
Jan 24, 2018
| ==== Authentication Challenge | ||
|
|
||
| When authentication is enabled, an initial request to the server will result in an authentication challenge. The typical response message will appear as follows, but handling it could be different dependending on the SASL implementation (e.g. multiple challenges maybe requested in some cases, but not in the default provided by Gremlin Server). | ||
| When authentication is enabled, an initial request to the server will result in an authentication challenge. The typical response message will appear as follows, but handling it could be different dependending on the SASL implementation (e.g. multiple challenges maybe requested in some cases, but no in the default provided by Gremlin Server). |
Member
There was a problem hiding this comment.
And this one?
/no/not/
/maybe/may be/
| writer.write("=== ResponseMessage\n\n") | ||
| msg = ResponseMessage.build(UUID.fromString("41d2e28a-20a4-4ab0-b379-d810dede3786")). | ||
| code(org.apache.tinkerpop.gremlin.driver.message.ResponseStatusCode.AUTHENTICATE).create() | ||
| writer.write(toJson(msg, "Authentication Challenge", "When authentication is enabled, an initial request to the server will result in an authentication challenge. The typical response message will appear as follows, but handling it could be different dependending on the SASL implementation (e.g. multiple challenges maybe requested in some cases, but no in the default provided by Gremlin Server).")) |
Member
There was a problem hiding this comment.
Maybe it was this one that meant to be /no/not/ ?
And /maybe/may be/ ?
| writer.write("=== ResponseMessage\n\n") | ||
| msg = ResponseMessage.build(UUID.fromString("41d2e28a-20a4-4ab0-b379-d810dede3786")). | ||
| code(org.apache.tinkerpop.gremlin.driver.message.ResponseStatusCode.AUTHENTICATE).create() | ||
| writer.write(toJson(msg, "Authentication Challenge", "When authentication is enabled, an initial request to the server will result in an authentication challenge. The typical response message will appear as follows, but handling it could be different dependending on the SASL implementation (e.g. multiple challenges maybe requested in some cases, but no in the default provided by Gremlin Server).")) |
Contributor
Author
|
I will clean up all of @robertdale "no/not" stuff when i merge. |
Member
|
VOTE +1 |
Contributor
Author
|
Here's the follow up fixes given the comments: d5b6ebb |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://issues.apache.org/jira/browse/TINKERPOP-1874
Added bunch of tests for this and found that .NET was still working, but that was just because of how the gherkin processor was generating the traversals (it was always converting
Pargs to lists basically). I modified thePtemplate for .NET and made it so thatwithinandwithoutare handled differently from the other enums -withinandwithoutwill always pass a collection rather than an individual object if there is only one item present in the arg array.I also updated the IO docs to include
withinandwithoutso that it was clear to implementers what was expected for them. I may have to come back and do some cleanup here, but I already went deep on my PR to master for this so I'd rather just CTR in the doc fixes later.All tests pass with
docker/build.sh -t -n -iVOTE +1