You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by mattallenuk <gi...@git.apache.org> on 2018/08/25 00:08:48 UTC
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Ability to send a script to the...
GitHub user mattallenuk opened a pull request:
https://github.com/apache/tinkerpop/pull/922
TINKERPOP-1959: Ability to send a script to the Gremlin Server.
Hiya,
Submitting a PR for consideration re sending a script to a Gremlin server.
I've added an eval() function to GraphTraversal and TraveralSource classes. This function takes an optional script string and bindings map. If a script is provided it will be sent directly to the server, discarding any previous glv steps, if no script is provided one will be built from the previously set glv steps.
RemoteStrategy class has been changed to handle submission of eval operations to the DriverRemoteConnection.
Bytecode class has had a toScript() function added that takes the glv instructions and produces a script with bindings.
Traversal class has been changed to handle responses from eval op execution.
I wanted to have the option to build from glv instructions as I'll be using the library to access an Azure CosmoDB which currently doesn't support bytecode but when it does it will make it much easier to update projects to using bytecode by removing the eval function and replacing with one of the other terminal actions.
I do have a couple of queries regarding the proposed changes:
1) Is it safe to assume that a returned value not containing a bulk property was executed by an eval op?
When parsing the returned data from the server in the TraversalSource class it appears that any data returned from a bytecode op contains a bulk property. Can it be assumed then that any return value that doesn't contain a bulk property is a valid return value for eval op, or should there be some sort of checking in place?
```
_getNext() {
while (this.traversers && this._traversersIteratorIndex < this.traversers.length) {
let traverser = this.traversers[this._traversersIteratorIndex];
if (traverser.bulk && traverser.bulk > 0) {
traverser.bulk--;
return { value: traverser.object, done: false };
} else if (traverser.bulk === undefined) {
return { value: traverser, done: true }
}
this._traversersIteratorIndex++;
}
```
2) Should every parameter be bound when building a dynamic script?
The Bytecode::toScript() function converts any glv parameters into bindings to be sent to the server. Should this make consideration for parameters that are likely not set by users. For example g.addV('flowers', 'Lilac'), can it be assumed that flowers was set by the implementer and Lilac by a user? If so should the function be changed to only bind parameters we can assume are set by users?
3) Does the unit test need to be more thorough and check more varied instructions and more complex glv queries/instructions?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mattallenuk/tinkerpop TINKERPOP-1959
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/922.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #922
----
commit b6478d666407656bc1cfc749a81d3e1ad6f06692
Author: Matthew Allen <ma...@...>
Date: 2018-08-23T14:48:36Z
Initial Commit for TINKERPOP-1959 solution.
commit c04ddae8a5793177d7515629358346d91be73e43
Author: Matthew Allen <ma...@...>
Date: 2018-08-24T15:14:29Z
Refactor of RemoteStrategy apply()
commit 090f1394a82be9bfd8fede85fa75655ab1f0f249
Author: Matthew Allen <ma...@...>
Date: 2018-08-24T23:02:27Z
Fix on eval() incorrect documentation
----
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/922
It works with Java and GraphSON 2.0:
```text
gremlin> cluster = Cluster.build().serializer("GRAPHSON_V2D0").create()
==>localhost/127.0.0.1:8182
gremlin> client = cluster.connect()
==>org.apache.tinkerpop.gremlin.driver.Client$ClusteredClient@24a298a6
gremlin> client.submit("g.addV().property(c,k,v)", [c:set,k:'name',v:123]).all().get()
==>result{object=v[4] class=org.apache.tinkerpop.gremlin.structure.util.detached.DetachedVertex}
gremlin> client.submit("g.V().has('name',123).count()").all().get()
==>result{object=1 class=java.lang.Long}
```
So, i'm not sure what's wrong. If you want to commit/push what you have i can try to test on my own to see what I (or maybe someone else) can figure out.
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/922
hi @mattallenuk - Judging from the comments it seems like the only thing missing here code-wise is an added test. Administratively, there's some documentation (reference docs, upgrade docs, changelog entry) to add for this feature as well. I don't mean to pressure or rush you but do you happen to have any idea when you might come back to this to finish up the final bits?
I mainly ask because we are in the midst of planning the release of 3.2.10 and 3.3.4 and we'd like to see this work of yours included. If your schedule doesn't allow for you to come back to this right now, perhaps I could assist in polishing up the final odds/ends for you?
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/922
`addProperty()` looks right. On `tp32` I guess the default is GraphSON 2.0 which has this form for `Cardinality` values:
http://tinkerpop.apache.org/docs/3.2.9/dev/io/#_cardinality
I see that the `accept` is set to "application/json". On `tp32` I think that defaults to GraphSON 1.0 which wouldn't be able to deal with complex objects. You can only pass primitives that are compatible with JSON data types. Does it work if you don't use `Cardinality`? maybe, just do something more simple like:
````js
connection.submit('x+y', { x: 1, y: 1 } );
```
If that works then change to GraphSON 2.0 - "application/vnd.gremlin-v2.0+json" - and see what happens for complex stuff like:
````js
connection.submit(card.toString()', { card: t.cardinality.set } );
```
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/922
Excellent - thanks for doing this. @jorgebay is the better person to handle this review in full, but I'd like to offer a few points:
> Is it safe to assume that a returned value not containing a bulk property was executed by an eval op?
I'm not sure I would call it "safe", but that is typically what happens. I would say that it is not completely "safe" to do so because an `eval()` could return anything and thus return a `BulkSet` and the user would expect that and I guess it would unroll to individual items when you iterated which the user wouldn't expect.
> Should every parameter be bound when building a dynamic script?
We refer to what you've done there as a `Translator` (i.e. something that converts bytecode to the string representation of the Gremlin) and that's neat that you have that. We don't implement that as a `toString()` on `Bytecode` but perhaps that's ok though. FYI, here's what a `Translator` looks like in Java:
```groovy
String gremlin = GroovyTranslator.of("g").translate(bytecode)
```
I don't believe that our translators convert arguments to bindings. It assumes them to be literals. We only construct bindings if the user constructs them as first order `Bindings` in the Gremlin itself, but I don't know if we have that concept here in javascript. For consistency sake, we might want to. You can see the approach taken by Python to handle that as an example:
http://tinkerpop.apache.org/docs/current/reference/#_bindings
> Does the unit test need to be more thorough and check more varied instructions and more complex glv queries/instructions?
On the JVM side we test the translators through the GLV suite (essentially we use the JVM process test suite which is basically the same thing). Maybe the same should be done here.
Separate from your questions, I'd offer that `g.eval()` isn't really in line with how other GLV drivers submit scripts. While I'd agree it to be convenient to have it work that way, I think it would be better to see that working in a way more consistent with other GLVs.
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on the issue:
https://github.com/apache/tinkerpop/pull/922
I've just tried it not parsing the bindings to strings and get the same issue. So I'm wondering now if I do need to parse the parameters or if I'm just calling the addProperty function wrong?
`[RequestMessage{, requestId=27ad0188-8b5a-40e0-bf52-b158a1e92507, op='eval', processor='', args={gremlin=g.V(1).property(card, nm, val), aliases={g=gmodern}, bindings={card={typeName=Cardinality, elementName=set}, nm=test, val=12}, language=gremlin-groovy, accept=application/json}}].`
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on the issue:
https://github.com/apache/tinkerpop/pull/922
@spmallette I can confirm that this test works for bytecode submission so possibly an issue in the way the bindings are being submitted? Should the language parameter be gremlin-groovy or something else?
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r220812375
--- Diff: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/client.js ---
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+'use strict';
+
+const DriverRemoteConnection = require('./driver-remote-connection');
+const Bytecode = require('../process/bytecode');
+
+class Client {
+ /**
+ * Creates a new instance of DriverRemoteConnection.
+ * @param {String} url The resource uri.
+ * @param {Object} [options] The connection options.
+ * @param {Array} [options.ca] Trusted certificates.
+ * @param {String|Array|Buffer} [options.cert] The certificate key.
+ * @param {String} [options.mimeType] The mime type to use.
+ * @param {String|Buffer} [options.pfx] The private key, certificate, and CA certs.
+ * @param {GraphSONReader} [options.reader] The reader to use.
+ * @param {Boolean} [options.rejectUnauthorized] Determines whether to verify or not the server certificate.
+ * @param {String} [options.traversalSource] The traversal source. Defaults to: 'g'.
+ * @param {GraphSONWriter} [options.writer] The writer to use.
+ * @param {Authenticator} [options.authenticator] The authentication handler to use.
+ * @constructor
+ */
+ constructor(url, options) {
+ this._options = options;
+ this._connection = new DriverRemoteConnection(url, options);
+ }
+
+ /**
+ * Opens the underlying connection to the Gremlin Server, if it's not already opened.
+ * @returns {Promise}
+ */
+ open() {
+ return this._connection.open();
+ }
+
+ /**
+ * Send a request to the Gremlin Server, can send a script or bytecode steps.
+ * @param {Bytecode|string} message The bytecode or script to send
+ * @param {Object} bindings The script bindings, if any.
+ * @returns {Promise}
+ */
+ submit(message, bindings) {
+ if (typeof message === 'string' || message instanceof String) {
+ const args = {
+ 'gremlin': message,
+ 'bindings': bindings,
--- End diff --
@jorgebay yes, I'll get that done.
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/922
PR for this is now on #952
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/922
@mattallenuk I figured out what was going on. The `EnumValue` was being interpreted as a `Map` when serializing to GraphSON. The change is pretty trivial, but I have a bunch of other little changes here/there. I'm going to merge this PR to a TINKERPOP-1959 branch for review (hope that's ok with you) and hopefully we can get this merged up today.
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r214831866
--- Diff: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/translator.js ---
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+'use strict';
+
+/**
+ * Class to translate glv bytecode steps into executable Gremlin-Groovy script
+ */
+class Translator {
--- End diff --
Hi @jorgebay,
The purpose of the translator is to allow building of a script from bytecode for users that don't yet have bytecode implementation on their Gremlin Server but want to use GLV instructions rather than writing scripts in text.
Case in point, I'll be using the project on Azure CosmoDB but I cannot send bytecode as it's not yet supported, they're working on it. So I don't want to have to write all my queries as strings and then have to go back at a later date and convert to glv steps. I'd rather use the glv steps and go back later and implement terminal commands. This might just be my own personal preferences, but I'm sure others would appreciate that flexibility?
This is how it would be used:
```javascript
const g = new graph.Graph().traversal();
g.V().order().by('age', t.order.decr);
const script = new Translator('g').translate(g.getBytecode());
```
That script could then be submitted via the client.
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on the issue:
https://github.com/apache/tinkerpop/pull/922
@spmallette Ok so your above code runs fine and returns a value. It appears that my call to g.V(1).property() is not liked.
My request looks like this now:
`[RequestMessage{, requestId=7baf2b77-ca79-4679-9812-f3e0d8d3dbb5, op='eval', processor='', args={gremlin=g.V(1).property(card, nm, val), aliases={g=gmodern}, bindings={card={typeName=Cardinality, elementName=set}, nm=test, val=12}, language=gremlin-groovy, accept=application/vnd.gremlin-v2.0+json}}]`
The full error I get is as follows:
`java.lang.IllegalArgumentException: The provided key/value array length must be a multiple of two
at org.apache.tinkerpop.gremlin.structure.Element$Exceptions.providedKeyValuesMustBeAMultipleOfTwo (Element.java:125)
at org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters.set (Parameters.java:175)
at org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.AddPropertyStep.addPropertyMutations (AddPropertyStep.java:80)
at org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal.property (GraphTraversal.java:2066)
at org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal.property (GraphTraversal.java:2094)
at sun.reflect.NativeMethodAccessorImpl.invoke0 (Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke (NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke (DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke (Method.java:498)
at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite$PojoCachedMethodSite.invoke (PojoMetaMethodSite.java:192)
at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite.call (PojoMetaMethodSite.java:56)
at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall (CallSiteArray.java:47)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call (AbstractCallSite.java:116)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call (AbstractCallSite.java:144)
at Script7.run (Script7.groovy:1)
at org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine.eval (GremlinGroovyScriptEngine.java:856)
at org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine.eval (GremlinGroovyScriptEngine.java:561)
at javax.script.AbstractScriptEngine.eval (AbstractScriptEngine.java:233)
at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.eval (ScriptEngines.java:135)
at org.apache.tinkerpop.gremlin.groovy.engine.GremlinExecutor.lambda$eval$0 (GremlinExecutor.java:291)
at java.util.concurrent.FutureTask.run (FutureTask.java:266)
at java.util.concurrent.Executors$RunnableAdapter.call (Executors.java:511)
at java.util.concurrent.FutureTask.run (FutureTask.java:266)
at java.util.concurrent.ThreadPoolExecutor.runWorker (ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run (ThreadPoolExecutor.java:617)
at java.lang.Thread.run (Thread.java:748)`
What is the best way forward, your test proves that non native JS types are being sent correctly, but my test should also work...
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on the issue:
https://github.com/apache/tinkerpop/pull/922
@spmallette thanks for the feedback. I'll wait on further input from @jorgebay and implement it to be more closely aligned with the other GLVs based on this and further feedback.
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/922
"gremlin-groovy" is fine. The server should default to that if the argument isn't present.
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by jorgebay <gi...@git.apache.org>.
Github user jorgebay commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r219469511
--- Diff: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/client.js ---
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+'use strict';
+
+const DriverRemoteConnection = require('./driver-remote-connection');
+const Bytecode = require('../process/bytecode');
+
+class Client {
+ /**
+ * Creates a new instance of DriverRemoteConnection.
+ * @param {String} url The resource uri.
+ * @param {Object} [options] The connection options.
+ * @param {Array} [options.ca] Trusted certificates.
+ * @param {String|Array|Buffer} [options.cert] The certificate key.
+ * @param {String} [options.mimeType] The mime type to use.
+ * @param {String|Buffer} [options.pfx] The private key, certificate, and CA certs.
+ * @param {GraphSONReader} [options.reader] The reader to use.
+ * @param {Boolean} [options.rejectUnauthorized] Determines whether to verify or not the server certificate.
+ * @param {String} [options.traversalSource] The traversal source. Defaults to: 'g'.
+ * @param {GraphSONWriter} [options.writer] The writer to use.
+ * @param {Authenticator} [options.authenticator] The authentication handler to use.
+ * @constructor
+ */
+ constructor(url, options) {
+ this._options = options;
+ this._connection = new DriverRemoteConnection(url, options);
+ }
+
+ /**
+ * Opens the underlying connection to the Gremlin Server, if it's not already opened.
+ * @returns {Promise}
+ */
+ open() {
+ return this._connection.open();
+ }
+
+ /**
+ * Send a request to the Gremlin Server, can send a script or bytecode steps.
+ * @param {Bytecode|string} message The bytecode or script to send
+ * @param {Object} bindings The script bindings, if any.
+ * @returns {Promise}
+ */
+ submit(message, bindings) {
+ if (typeof message === 'string' || message instanceof String) {
+ const args = {
+ 'gremlin': message,
+ 'bindings': bindings,
--- End diff --
I think bindings need to be serialized.
Currently, I think it will only work with json supported types (boolean, number, strings), but it should work with any type supported by GraphSON2.
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r218858884
--- Diff: gremlin-server/src/test/resources/org/apache/tinkerpop/gremlin/server/gremlin-server-integration.yaml ---
@@ -41,6 +41,7 @@ serializers:
- { className: org.apache.tinkerpop.gremlin.driver.ser.GraphSONMessageSerializerV1d0, config: { ioRegistries: [org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerIoRegistryV1d0] }}
processors:
- { className: org.apache.tinkerpop.gremlin.server.op.session.SessionOpProcessor, config: { sessionTimeout: 28800000 }}
+ - { className: org.apache.tinkerpop.gremlin.server.op.standard.StandardOpProcessor, config: { maxParameters: 64 }}
--- End diff --
No we don't I'd just read somewhere that it was the recommended. I'll remove that option so the default is used.
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/922
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by jorgebay <gi...@git.apache.org>.
Github user jorgebay commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r214823388
--- Diff: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/driver-remote-connection.js ---
@@ -114,24 +114,26 @@ class DriverRemoteConnection extends RemoteConnection {
result: null
};
}
- const message = bufferFromString(this._header + JSON.stringify(this._getRequest(requestId, bytecode, op, args)));
+
+ const message = bufferFromString(this._header + JSON.stringify(this._getRequest(requestId, bytecode, op, args, processor)));
this._ws.send(message);
}));
}
- _getRequest(id, bytecode, op, args) {
+ _getRequest(id, bytecode, op, args, processor) {
if (args) {
args = this._adaptArgs(args);
}
-
+
return ({
'requestId': { '@type': 'g:UUID', '@value': id },
'op': op || 'bytecode',
- 'processor': 'traversal',
+ // if using op eval need to ensure processor stays unset if caller didn't set it.
+ 'processor': (!processor && op !== 'eval') ? 'traversal' : processor,
'args': args || {
- 'gremlin': this._writer.adaptObject(bytecode),
- 'aliases': { 'g': this.traversalSource }
- }
+ 'gremlin': this._writer.adaptObject(bytecode),
--- End diff --
NIT: Use 2 spaces instead of 4 in the following lines.
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on the issue:
https://github.com/apache/tinkerpop/pull/922
Hiya guys,
I have moved the code out of the existing classes and have created a Client class that can handle script sending and a Translator class that can convert bytecode into a string.
The Translator class accepts bindings in the form of a javascript object passed into the step function and converts it into Gremlin Groovy Tuple in the translated script. I haven't considered the impact of this upon Bytecode so this may need to be changed or addressed on the Bytecode side, or Bytecode may already handle parameter maps?
In terms of consumption is there a way we could make the Translator more transparent to the user, I did have an eval function added to Traversal which did the conversion and sending, I removed this but would that be an acceptable solution? It's not inline with other GLV's unfortunately.
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r217501779
--- Diff: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/translator.js ---
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+'use strict';
+
+/**
+ * Class to translate glv bytecode steps into executable Gremlin-Groovy script
+ */
+class Translator {
--- End diff --
I think translators are an established part of TinkerPop at this point. We have them for Java, Groovy, and Python and I've personally found them to be insanely useful on so many internal odds/ends. We don't over-publicize them because we don't want to promote "scripts" but I understand the current need for CosmosDB.
Anyway, including a Javascript translator is an acceptable addition imo.
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by bterlson <gi...@git.apache.org>.
Github user bterlson commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r214988968
--- Diff: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/translator.js ---
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+'use strict';
+
+/**
+ * Class to translate glv bytecode steps into executable Gremlin-Groovy script
+ */
+class Translator {
--- End diff --
Just want to say, as another CosmosDB user, that I very much would love to be able to use the fluent step API even though cosmos doesn't yet support bytecode submissions. I hadn't considered it was possible to convert the glv steps to a script, but now that I see it working, I want it badly 😊
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r214833558
--- Diff: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/translator.js ---
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+'use strict';
+
+/**
+ * Class to translate glv bytecode steps into executable Gremlin-Groovy script
+ */
+class Translator {
--- End diff --
And that is why I was querying bindings in the bytecode as you'll use the glv steps to build a query before converting it to a script. If you are happy for the translator class to stay then I'll need to know how parameters are implemented in bytecode to ensure the translation is done correctly.
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/922
sorry to hear about the sickness. i hope your doing well now. you're right about the documentation. i think we wanted to focus folks on bytecode based traversals. we do have a section for gremlin-python for submitting scripts, but i think i discouraged the one for .NET. I'd forgotten that. I'd had the idea that i was going to massively rework the documentation on 3.4.0 to better organize all this GLV stuff and that hasn't fully happened yet.
ok, so, i guess you just need the test/serialization fixups then, a changelog entry and a note in upgrade docs to say that this is now there...perhaps just add some example usage there. thanks
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r217498294
--- Diff: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/translator.js ---
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+'use strict';
+
+/**
+ * Class to translate glv bytecode steps into executable Gremlin-Groovy script
+ */
+class Translator {
--- End diff --
@jorgebay I'm more than happy to move the Translator functionality into a support project if it doesn't fit within the gremlin-javascript project. I realise it might be too specific a case and hope I haven't overstepped a mark by including it with this PR, it just made total sense to me.
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on the issue:
https://github.com/apache/tinkerpop/pull/922
@spmallette, my apologies I've been unwell the last week. I have done the test but it showed that I need to parse the bindings so that the standard Op processor can read them, it doesn't appear to like bytecode bindings. I'm hoping to have that done and tested today.
On the documentation side I noticed that there doesn't appear to be any documentation in the other glv sections about submitting scripts, am I okay to add this documentation to the javascript glv section?
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by jorgebay <gi...@git.apache.org>.
Github user jorgebay commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r219471784
--- Diff: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/client.js ---
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+'use strict';
+
+const DriverRemoteConnection = require('./driver-remote-connection');
+const Bytecode = require('../process/bytecode');
+
+class Client {
+ /**
+ * Creates a new instance of DriverRemoteConnection.
+ * @param {String} url The resource uri.
+ * @param {Object} [options] The connection options.
+ * @param {Array} [options.ca] Trusted certificates.
+ * @param {String|Array|Buffer} [options.cert] The certificate key.
+ * @param {String} [options.mimeType] The mime type to use.
+ * @param {String|Buffer} [options.pfx] The private key, certificate, and CA certs.
+ * @param {GraphSONReader} [options.reader] The reader to use.
+ * @param {Boolean} [options.rejectUnauthorized] Determines whether to verify or not the server certificate.
+ * @param {String} [options.traversalSource] The traversal source. Defaults to: 'g'.
+ * @param {GraphSONWriter} [options.writer] The writer to use.
+ * @param {Authenticator} [options.authenticator] The authentication handler to use.
+ * @constructor
+ */
+ constructor(url, options) {
+ this._options = options;
+ this._connection = new DriverRemoteConnection(url, options);
+ }
+
+ /**
+ * Opens the underlying connection to the Gremlin Server, if it's not already opened.
+ * @returns {Promise}
+ */
+ open() {
+ return this._connection.open();
+ }
+
+ /**
+ * Send a request to the Gremlin Server, can send a script or bytecode steps.
+ * @param {Bytecode|string} message The bytecode or script to send
+ * @param {Object} bindings The script bindings, if any.
+ * @returns {Promise}
+ */
+ submit(message, bindings) {
+ if (typeof message === 'string' || message instanceof String) {
+ const args = {
+ 'gremlin': message,
+ 'bindings': bindings,
--- End diff --
oh, I see that `args` are adapted by the `Connection` afterwards, never mind my previous comment.
@mattallenuk: Can we include a test containing a binding like a `Long` or `Set` or any type that is not a native js type?
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by jorgebay <gi...@git.apache.org>.
Github user jorgebay commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r214822151
--- Diff: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/client.js ---
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+'use strict';
+
+const DriverRemoteConnection = require('./driver-remote-connection');
+const Bytecode = require('../process/bytecode');
+
+class Client extends DriverRemoteConnection {
--- End diff --
`Client` should not extend `DriverRemoteConnection`, as in the other GLVs `Client` should contain and manage the lifetime of `DriverRemoteConnection` instances, ie: expose `open()` and `shutdown()` methods that open and close the connections that belong to the `DriverRemoteConnection`.
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on the issue:
https://github.com/apache/tinkerpop/pull/922
@spmallette Doing better now thank you!
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r213212310
--- Diff: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/remote-connection.js ---
@@ -66,7 +67,27 @@ class RemoteStrategy extends TraversalStrategy {
if (traversal.traversers) {
return Promise.resolve();
}
- return this.connection.submit(traversal.getBytecode()).then(function (remoteTraversal) {
+
+ let instructions = traversal.getBytecode();
+ let op = 'bytecode';
+ let processor = 'traversal';
+ let args = null;
+
+ // check if the last instruction is an eval statement
+ if (instructions.stepInstructions.length && instructions.stepInstructions[instructions.stepInstructions.length-1][0] === 'eval') {
--- End diff --
Yep valid point I'll certainly make those changes.
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by jorgebay <gi...@git.apache.org>.
Github user jorgebay commented on the issue:
https://github.com/apache/tinkerpop/pull/922
Glad to see you working on this @mattallenuk !
In line with what @spmallette was mentioning, I think we need expose a similar API as the rest of GLVs. Python GLV is a good example (because its a scripting language as JavaScript) but if you have some understanding of C#, you can check out gremlin-dotnet as well.
Besides the `RemoteConnection` the other GLVs expose a `Client` (one abstraction above), that is able to submit traversals in bytecode or in script to evaluate, you can reading here:
- https://github.com/apache/tinkerpop/blob/tp32/gremlin-python/src/main/jython/gremlin_python/driver/client.py
- https://github.com/apache/tinkerpop/blob/tp32/gremlin-python/src/main/jython/tests/driver/test_client.py#L39-L48
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r214832381
--- Diff: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/client.js ---
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+'use strict';
+
+const DriverRemoteConnection = require('./driver-remote-connection');
+const Bytecode = require('../process/bytecode');
+
+class Client extends DriverRemoteConnection {
--- End diff --
np, I'll get that implemented :)
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by jorgebay <gi...@git.apache.org>.
Github user jorgebay commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r214825070
--- Diff: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/translator.js ---
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+'use strict';
+
+/**
+ * Class to translate glv bytecode steps into executable Gremlin-Groovy script
+ */
+class Translator {
--- End diff --
I don't understand the purpose of the translator for this patch. Maybe you were looking to implement bindings?
https://github.com/apache/tinkerpop/blob/fcabd01c4425061334901082a81e62151adaeb64/gremlin-python/src/main/jython/gremlin_python/process/traversal.py#L504
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by jorgebay <gi...@git.apache.org>.
Github user jorgebay commented on the issue:
https://github.com/apache/tinkerpop/pull/922
> I haven't considered the impact of this upon Bytecode so this may need to be changed or addressed on the Bytecode side, or Bytecode may already handle parameter maps?
The Bytecode generation already has a way to denote parameters embedded in the query, so it's ok to implement this independently.
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by bterlson <gi...@git.apache.org>.
Github user bterlson commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r213130596
--- Diff: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/remote-connection.js ---
@@ -66,7 +67,27 @@ class RemoteStrategy extends TraversalStrategy {
if (traversal.traversers) {
return Promise.resolve();
}
- return this.connection.submit(traversal.getBytecode()).then(function (remoteTraversal) {
+
+ let instructions = traversal.getBytecode();
+ let op = 'bytecode';
+ let processor = 'traversal';
+ let args = null;
+
+ // check if the last instruction is an eval statement
+ if (instructions.stepInstructions.length && instructions.stepInstructions[instructions.stepInstructions.length-1][0] === 'eval') {
--- End diff --
Aliasing `instructions.stepInstructions` will make this line easier to read.
Could also consider relying on the falsiness of empty arrays and simplify to `if (sis && sis[sis.length-1][0] === 'eval') {`
---
[GitHub] tinkerpop pull request #922: TINKERPOP-1959: Gremlin Javascript ability to s...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/922#discussion_r218854373
--- Diff: gremlin-server/src/test/resources/org/apache/tinkerpop/gremlin/server/gremlin-server-integration.yaml ---
@@ -41,6 +41,7 @@ serializers:
- { className: org.apache.tinkerpop.gremlin.driver.ser.GraphSONMessageSerializerV1d0, config: { ioRegistries: [org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerIoRegistryV1d0] }}
processors:
- { className: org.apache.tinkerpop.gremlin.server.op.session.SessionOpProcessor, config: { sessionTimeout: 28800000 }}
+ - { className: org.apache.tinkerpop.gremlin.server.op.standard.StandardOpProcessor, config: { maxParameters: 64 }}
--- End diff --
Do we have to test for more than the default parameters (which is 16)?
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on the issue:
https://github.com/apache/tinkerpop/pull/922
@jorgebay @spmallette So I've written the following test, however I'm getting a Provided key/value array length must be a multiple of two when submitting. I am guessing this is due to submitting the cardinality as a string value. How should non-native JS types be submitted via script?
`connection.submit('g.V(vert).property(card, nm, val)', { vert: 1, card: t.cardinality.set, nm: 'test', val: 12 } )`
this produces:
[RequestMessage{, requestId='reqid', op='eval', processor='', args={gremlin=g.V(1).property(card, nm, val), aliases={g=gmodern}, bindings={vert=1, card=set, nm=test, val=12}, language=gremlin-groovy, accept=application/json}}]
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Posted by mattallenuk <gi...@git.apache.org>.
Github user mattallenuk commented on the issue:
https://github.com/apache/tinkerpop/pull/922
@spmallette Ok, have pushed the changes along with the documentation changes we'd discussed. Hopefully it's an easy fix.
---