Upgrade Gremlin to version 3.4.0 or higher

[tec-goblin]

Gremlin version 3 and higher has a very different API. Creating Clients, submitting messages, binding to events and handling result sets are different. Some things your were doing (like overriding handling of messages for
jbmusso/gremlin-javascript#93 and overriding handling of errors might not be needed any more, but this would require testing.

The task of upgrading becomes harder, because there aren't up to date typescript definitions for that dependency. Still, from what I saw, the code will probably look cleaner and will surface new possibilities. It seems changes are concentrated in _executeQueryCoreForEndpoint, queryAndShowResults.

Unfortunately, the upgrade is needed for microsoft/vscode-cosmosdb#814
@Microsoft/vscodeazuretools how do you suggest to proceed? I was thinking of contributing to an updated d.ts first (typed-typings/npm-gremlin#9) which will help us understand the new API, particularly the parts used in the two methods above. Then I can try to change those two methods, but I am not sure behaviour will be 100% the same as before.

Server side it's probably fine, as the .NET Cosmos client seems to be using the new features already.
PS: Enjoy the holidays :)

[PrashanthCorp]

Hi tec-goblin,
I have been looking into upgrading Gremlin to v3.4 as part of a security update. However, it looks like Cosmos was not compliant with the newest version of gremlin. From what I've learnt, this commit in the tinkerpop repository should add cosmos support. I shall test this post-holidays and get back to you about that.

Answering your questions,

It seems changes are concentrated in _executeQueryCoreForEndpoint, queryAndShowResults.

I noticed we needed to make changes in _executeQueryCoreForEndpoint. AFAICT, queryAndShowResults mainly sets up telemetry handlers and the state of the client. Would you like to change the error handling behavior?

how do you suggest to proceed?

If the above commit would allow support, you might have to check whether that merged commit was released in the latest version of gremlin (I could not verify this quickly).
Also, feel free to use code I added to the work-in-progress branch for the update : https://github.com/Microsoft/vscode-cosmosdb/tree/ps/gremlin

In terms of updating the d.ts package, the package you linked to hasn't been touched for a year, so I don't know how promptly you would be able to push changes. You could also add the types in a new package under DefinitelyTyped, but there is a higher standard involved in contributing there. Check out the testing instructions. :)
I think adding types is more of a stretch goal - it's definitely useful, but it could take a lot more time than expected.

Then I can try to change those two methods, but I am not sure behaviour will be 100% the same as before.

This is a valid concern given the extent how many user scenarios are affected by these functions. We have a few tests we run with some test accounts of ours (all graph accounts are on cosmos though). If you're interested in testing that we'll try to share that with you (they're not publicly available at this point). If you have tests for gremlin database accounts outside of Azure, feel free to let us know.

[tec-goblin]

Super! The commit you mentioned is tagged 3.3.4, which is the latest released version (I still need to confirm in master). I'm already working with 3.4.0, in order to be ready for the new ResultSet.

On QueryAndShowResults I thought I needed it to adapt it to the new ResultSet and secondary (later) to implement microsoft/vscode-cosmosdb#814, but I'm definitely not sure. I had just started working on _executeQueryCoreForEndpoint.

On d.ts: Indeed I doubt I'll manage to get my changes accepted. Regardless, I almost finished a first version of the new d.ts (totally different than the old one), and discovered many issues in tinkerpop's javadoc-like comments (the comments on options objects were very wrong). I'd like to share it with them and gently suggest them to switch one day to typescript. For the moment, I have a decent d.ts that can help me during development. It's not the full API, but the most reasonably useful surface, and largely more than what we need. I will upload a first version on my fork of the repository later today. If I don't manage to convince Tinkerpop to plan a switch to Typescript, I might go later all the way to finish with the rest of the API and send it to DefinitelyTyped, but I'm afraid of maintenance workload. Tinkerpop make breaking changes to the API even in minor versions and I'm not sure I'll be able to maintain it.

The code you shared will help me a lot! I will still have issues with testing: I'm reasonably comfortable by now with Typescript and this plugin, but I know gremlin way less than documentDb. Once I have something basic working, I can ask for your tests.

[tec-goblin]

On the definitions, it seems the suggestion has been already discussed and I just added my contribution.
I will wait for them to decide on whether they'll switch to typescript, or include a first-class d.ts in their project, and contribute to whatever they decide. For the moment I have what I need to proceed with upgrading gremlin to 3.4.0 (I target directly 3.4.0, by the time we're finished it will be published).

I will most probably send an update in a few days :).

[tec-goblin]

Just an update: the current state of the code is here. I want to verify whether the old overrides of the internal behaviour are needed - I wouldn't like to expose and use private members (_ws and _connection) if not needed, in order to avoid complications in future updates.
Right now I'm stuck in the basics: opening the connection. I wait for some members of the Tinkerpop team to come back from holidays and have a look.

[tec-goblin]

I advanced a little bit, but I'm still stuck. Connection is opened, but Ι don't seem to receive any messages from the web socket. As nothing is received, we don't get any error messages either. Analytics on the server side (cosmosdb) are very poor.

Probably the custom behaviour for socket errors and handling unicode characters are not needed any more. I removed them for the moment. I'm afraid somebody will need to take it from here :(.