[jira] [Commented] (TINKERPOP-3035) Add explicit property(IDictionary) for .NET

[ASF GitHub Bot (Jira)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-3035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17840334#comment-17840334
 ] 

ASF GitHub Bot commented on TINKERPOP-3035:
---

FlorianHockmann commented on PR #2565:
URL: https://github.com/apache/tinkerpop/pull/2565#issuecomment-2074375930

   > nit: no test coverage
   
   Yes, I wanted to add a test for this, but the problem is that it's 
manipulating data in the graph. I don't know how to test this with our current 
test setup without impacting other tests. If we could start a container with 
Gremlin Server for just this test, then we could easily test it. But our 
current setup uses the same container for all tests with the same graph. If we 
write to this graph in one test, then it will impact other tests.
   Another option would be to write a unit test which only checks the generated 
Bytecode, but I think such tests have little value, especially when we want to 
get rid of Bytecode altogether in a future release.




> Add explicit property(IDictionary) for .NET
> ---
>
> Key: TINKERPOP-3035
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3035
> Project: TinkerPop
>  Issue Type: Bug
>  Components: dotnet
>Affects Versions: 3.6.6
>Reporter: Stephen Mallette
>Priority: Minor
>
> There is no {{property(IDictionary)}} method for .NET to 
> accompany the {{property(Map)}} step. It works because it probably piggybacks 
> on {{property(object?, object?, objects[]?)}} which has all nullable 
> arguments. The explicit overload with the {{IDictionary}} should be added.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Re: [DISCUSS] Breaking change in Gremlin.Net's traversal enumeration if no element found (TINKERPOP-3072)

[Cole Greer]

Hi Florian,

This seems to be one of those issues where all the choices are somewhat 
undesirable. Doing nothing in 3.6/3.7 leaves us with a break between .NET 7 and 
8. Standardising on nulls requires breaking existing .NET 8 users, and 
standardising on exceptions will break users of .NET 7 and below. I don’t have 
a strong preference on which path we choose for 3.6/3.7 as long as we document 
it as best as we can to try and prevent any surprises to users.

I agree that long term, the correct behaviour is to throw an exception when 
there are no elements left in order to be consistent with the other GLVs.

Thanks,

Cole


From: Florian Hockmann 
Date: Tuesday, April 23, 2024 at 5:43 AM
To: dev@tinkerpop.apache.org 
Subject: [DISCUSS] Breaking change in Gremlin.Net's traversal enumeration if no 
element found (TINKERPOP-3072)
Hi,



Gremlin.Net currently behaves differently if, e.g., Next() is called on a
traversal that has no (more) results for .NET 8 than for earlier .NET
versions. It returns null for earlier .NET versions but throws an exception
on .NET 8. (This is tracked in TINKERPOP-3072
 .)



A minimal example to better illustrate what I'm talking about:



g.V().Has("Name", "doesnotexist").Next();



Result:

On .NET 7 and earlier: null

On .NET 8: System.InvalidOperationException: Enumeration already finished.



So, there is already a breaking change if users upgrade their .NET version
to .NET 8. However, throwing an exception in this case seems to be the
expected behaviour in general for Gremlin as that's what other GLVs do. I at
least checked this for Java and Python. Java throws a NoSuchElementException
and Python a StopIteration error.



That is why I think that the correct thing for Gremlin.Net is to also throw
an exception here as we should have consistent behaviour across GLVs. In
that case, we should of course throw the exception here irrespective of the
.NET version used.



If we want to consistently throw an exception in .NET, then the question is
when we want to make this change: In the next patch releases on 3.6 / 3.7 or
wait until 3.8.0?



I don't have a strong preference here but tend slightly towards adding the
breaking change on 3.6/3.7 because I think that it's confusing for users to
get different behaviour only by updating to .NET 8. If we make this change
in the next release, then we at least have a consistent behaviour across
GLVs and also across .NET versions.

In general, users are only affected by this if they are relying on behaviour
that has only been present in the .NET GLV.



Any opinions on this?



If there are no objections in the next 72 hours, then I'll assume lazy
consensus in favour of a) letting Gremlin.Net throw an exception here (to be
consistent with other GLVs) and b) introducing this change with the next
3.6/3.7 releases.

[jira] [Created] (TINKERPOP-3076) Incorrect handling of large requests in Go GLV

[Valentyn Kahamlyk (Jira)]

Valentyn Kahamlyk created TINKERPOP-3076:


 Summary: Incorrect handling of large requests in Go GLV
 Key: TINKERPOP-3076
 URL: https://issues.apache.org/jira/browse/TINKERPOP-3076
 Project: TinkerPop
  Issue Type: Improvement
  Components: go
Affects Versions: 3.7.2, 3.6.7
Reporter: Valentyn Kahamlyk


When trying to send a request longer than about a megabyte, go GLV does not 
work correctly, possibly truncates data.

Simple reproducer 
{code:go}
func main() {
driverRemoteConnection, err := 
gremlingo.NewDriverRemoteConnection("ws://localhost:8182/gremlin")
if err != nil {
   fmt.Println(err)
   return
}
defer driverRemoteConnection.Close()
g := gremlingo.Traversal_().WithRemote(driverRemoteConnection)

const count = 1100
var ids [count]string
const idLength = 1000

result := make([]byte, idLength)
for i := 0; i < idLength; i++ {
   result[i] = 65
}
id := string(result)

for i := 0; i < count; i++ {
   ids[i] = id
}

r, err := g.V(ids).Count().ToList()
if err != nil {
   fmt.Println("err: ", err)
   return
}
fmt.Println("found: ", r[0].GetString()) 
}{code}
with `count = 1000` it works as expected.

Python GLV works as expected with same request.

Error logged in Gremlin Server 
{code:java}
io.netty.handler.codec.DecoderException: java.lang.IndexOutOfBoundsException: 
readerIndex(1048338) + length(1000) exceeds writerIndex(1048543): 
PooledUnsafeDirectByteBuf(ridx: 1048338, widx: 1048543, cap: 1048576){code}



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Re: [DISCUSS] TinkerPop 4 HTTP API Design

[Ken Hu]

An update regarding the API, the materializeProperties option is being
added back which should be set to either "all" or "tokens". The "all"
option returns all the properties while the "tokens" option returns only
the id and label. So the request syntax currently looks like:

-- Request Syntax --

POST /gremlin HTTP/1.1
Accept: <>
Content-Type: <>
Gremlin-Hints: <>

{
"gremlin": "string",
"timeoutMs": number,
"materializeProperties": "string",
"bindings": object,
"g": "string"
"language" : "string"
}

Additionally, there seems to be a difference between options needed to
change server behavior (timeoutMs, materializeProperties) and options
needed to change the behavior of the Traversal. Currently, we allow these
server options to be set using the with() options. I think it makes sense
for these to only be set in the HTTP request body as it isn't information
needed by the traversal. This leads to a better separation of options that
affect both all traversals (embedded and remote) and options that are
server/remote only. This means that we would drop the GremlinScriptChecker
from the server that is used for parsing through a script for specific
with() options that the server needs. However, a user may need to set
specific options per query so we could add a GLV-only step like request().
The request() step wouldn't be part of the language and would be something
that only the GLVs would recognize and would cause them to add those
options to the HTTP request body. It should be noted that request() is a
working idea, but for now, the most important point is that there will be a
way to set these server options per request from the GLVs.

On Wed, Apr 10, 2024 at 12:45 PM Ken Hu  wrote:

> Hi,
>
> The following is a reference of what the upcoming HTTP API will look like
> for the /gremlin resource. Some smaller items are probably subject to
> change.
>
> There are three differences between the 4.x HTTP API and the 3.x HTTP API
> that should be noted. First, the server always generates the requestId and
> returns it to the client. Second, the format of the request body has
> changed as the OpProcessors have been removed so the request fields have
> been flattened. Third, because the results are now streamed back to the
> user with chunked transfer encoding, there are certain errors that can
> occur after the initial 200 OK has been sent. This means that the user must
> check either the status object in the response body or the trailers to see
> if there are any errors.
>
> The format below uses a bit of pseudo-JSON to help represent request and
> response bodies. The actual format of the request and response bodies will
> be determined by the serializers defined via the "Accept" and
> "Content-Type" headers. As a result, a generic type definition in this
> document like "number" could translate to a "long" for a serializer that
> supports types like GraphBinary.
>
>
> -- Request Syntax --
>
> POST /gremlin HTTP/1.1
> Accept: <>
> Content-Type: <>
> Gremlin-Hints: <>
>
> {
>   "gremlin": "string",
>   "timeoutMs": number,
>   "bindings": object,
>   "g": "string"
>   "language" : "string"
> }
>
>
> -- Headers --
>
> Gremlin-Hints - When provided this header is a semi-colon separated list
> of key/value
> pair metadata that could be helpful to the server in
> processing a
> particular request in some way.
>   Required: No
>
> -- Header Options --
>
>   mimetype - the serialization accepted for the response.
> Required: No. Defaults to
> "application/vnd.gremlin-v4.0+json;types=false"
>
> application/vnd.gremlin-v4.0+json;types=false
>   GraphSON without embedded types that will be easy to parse and work
> with using
>   cURL and similar tools.
>
> application/vnd.gremlin-v4.0+json;types=true
>   GraphSON with embedded types, mostly present for consistency, so
> that both
>   configurations of GraphSON are available. Note that official
> TinkerPop drivers
>   will not support this serializer.
>
> application/vnd.graphbinary-v4.0
>   GraphBinary which has embedded types, mostly of use to Gremlin
> Language Drivers.
>
>   hints
> mutations - Indicates if the Gremlin contains steps that can mutate
> the graph. The
> default would be "unknown". Language drivers would set
> this hint
> automatically at query submission by dynamically detecting
> mutating
> steps in the query being sent.
>   yes | no | unknown
>
>
> -- Request Body --
>
> gremlin - The Gremlin query to execute.
>   Type: String
>   Required: Yes.
>
> timeoutMs - The maximum time a query is allowed to execute in milliseconds.
>   Type: Number
>   Required: No. If not present, then server default is used.
>
> bindings - Any bindings used to execute the query. For example, the
> incoming Gremlin script might
>be g.V(x) where x=1 is a binding tha

[jira] [Commented] (TINKERPOP-3035) Add explicit property(IDictionary) for .NET

[ASF GitHub Bot (Jira)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-3035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17840573#comment-17840573
 ] 

ASF GitHub Bot commented on TINKERPOP-3035:
---

xiazcy commented on PR #2565:
URL: https://github.com/apache/tinkerpop/pull/2565#issuecomment-2075810348

   I believe for Gherkin tests we have the empty graph that can be modified, 
since it gets reset for each test. This does mean adding a Feature test that 
will run for all GLVs.




> Add explicit property(IDictionary) for .NET
> ---
>
> Key: TINKERPOP-3035
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3035
> Project: TinkerPop
>  Issue Type: Bug
>  Components: dotnet
>Affects Versions: 3.6.6
>Reporter: Stephen Mallette
>Priority: Minor
>
> There is no {{property(IDictionary)}} method for .NET to 
> accompany the {{property(Map)}} step. It works because it probably piggybacks 
> on {{property(object?, object?, objects[]?)}} which has all nullable 
> arguments. The explicit overload with the {{IDictionary}} should be added.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)