From 006b1d02ef314842fe46d6394a094e9f586ed55a Mon Sep 17 00:00:00 2001 From: Valentyn Kahamlyk Date: Thu, 23 Nov 2023 11:26:30 -0800 Subject: [PATCH 1/5] added tests for tx operations on remote non-tx graph --- CHANGELOG.asciidoc | 5 +++ .../GraphTraversalTests.cs | 20 +++++++++ .../GraphTraversalTransactionTests.cs | 1 + .../RemoteConnectionFactory.cs | 4 +- gremlin-go/driver/traversal_test.go | 44 +++++++++++++++++++ .../lib/process/transaction.js | 8 ++++ .../test/integration/traversal-test.js | 33 ++++++++++++++ .../test/unit/traversal-test.js | 21 +++++++++ .../gremlin_python/process/graph_traversal.py | 2 +- .../driver/test_driver_remote_connection.py | 31 +++++++++++++ 10 files changed, 166 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 0f8ee06fb8f..b6eb1371ade 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -20,6 +20,11 @@ limitations under the License. image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/images/gremlin-victorian.png[width=185] +[[release-3-6-7]] +=== TinkerPop 3.6.7 (NOT OFFICIALLY RELEASED YET) + +* Added tests for error handling for GLV's if tx.commit() is called remotely for graphs without transactions support. + [[release-3-6-6]] === TinkerPop 3.6.6 (November 20, 2023) diff --git a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/GraphTraversalTests.cs b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/GraphTraversalTests.cs index 262ffeabca2..ef46d586cbd 100644 --- a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/GraphTraversalTests.cs +++ b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/GraphTraversalTests.cs @@ -271,5 +271,25 @@ public async Task ShouldSupportFurtherTraversalsAfterOneWasCancelled() Assert.True(await g.V().Promise(t => t.HasNext(), CancellationToken.None)); } + + [Fact] + public async Task ShouldThrowExceptionOnCommitWhenGraphNotSupportTx() + { + var connection = _connectionFactory.CreateRemoteConnection(); + var g = AnonymousTraversalSource.Traversal().WithRemote(connection); + var tx = g.Tx(); + var exception = await Assert.ThrowsAsync(async () => await tx.CommitAsync()); + Assert.Equal("ServerError: Graph does not support transactions", exception.Message); + } + + [Fact] + public async Task ShouldThrowExceptionOnRollbackWhenGraphNotSupportTx() + { + var connection = _connectionFactory.CreateRemoteConnection(); + var g = AnonymousTraversalSource.Traversal().WithRemote(connection); + var tx = g.Tx(); + var exception = await Assert.ThrowsAsync(async () => await tx.RollbackAsync()); + Assert.Equal("ServerError: Graph does not support transactions", exception.Message); + } } } \ No newline at end of file diff --git a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/GraphTraversalTransactionTests.cs b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/GraphTraversalTransactionTests.cs index 257f20b5308..fc0f80ea7b7 100644 --- a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/GraphTraversalTransactionTests.cs +++ b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/GraphTraversalTransactionTests.cs @@ -23,6 +23,7 @@ using System; using System.Threading.Tasks; +using Gremlin.Net.Driver.Exceptions; using Gremlin.Net.Process.Remote; using Gremlin.Net.Process.Traversal; using Xunit; diff --git a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/RemoteConnectionFactory.cs b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/RemoteConnectionFactory.cs index 8fe6c2e7cde..565e8c8297c 100644 --- a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/RemoteConnectionFactory.cs +++ b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/RemoteConnectionFactory.cs @@ -33,7 +33,7 @@ namespace Gremlin.Net.IntegrationTest.Process.Traversal.DriverRemoteConnection internal class RemoteConnectionFactory : IDisposable { private static readonly string TestHost = ConfigProvider.Configuration["TestServerIpAddress"]; - private static readonly int TestPort = Convert.ToInt32(ConfigProvider.Configuration["TestServerPort"]); + private static readonly int TestPort = 8182; private readonly IList _connections = new List(); private readonly IMessageSerializer _messageSerializer; @@ -46,7 +46,7 @@ public RemoteConnectionFactory(IMessageSerializer messageSerializer = null) public IRemoteConnection CreateRemoteConnection(int connectionPoolSize = 2) { // gmodern is the standard test traversalsource that the main body of test uses - return CreateRemoteConnection("gmodern", connectionPoolSize); + return CreateRemoteConnection("g", connectionPoolSize); } public IRemoteConnection CreateRemoteConnection(string traversalSource, int connectionPoolSize = 2) diff --git a/gremlin-go/driver/traversal_test.go b/gremlin-go/driver/traversal_test.go index f2b3ed07cb6..3f4493ae38c 100644 --- a/gremlin-go/driver/traversal_test.go +++ b/gremlin-go/driver/traversal_test.go @@ -22,6 +22,7 @@ package gremlingo import ( "crypto/tls" "reflect" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -360,6 +361,49 @@ func TestTraversal(t *testing.T) { assert.Equal(t, int32(0), getCount(t, g)) }) + t.Run("Test commit if no transaction started", func(t *testing.T) { + skipTestsIfNotEnabled(t, integrationTestSuiteName, testTransactionEnable) + // Start a traversal. + g := newWithOptionsConnection(t) + + // Create transactions + tx := g.Tx() + + // try to commit + err := tx.Commit() + assert.Equal(t, "E1103: cannot commit a transaction that is not started", err.Error()) + }) + + t.Run("Test rollback if no transaction started", func(t *testing.T) { + skipTestsIfNotEnabled(t, integrationTestSuiteName, testTransactionEnable) + // Start a traversal. + g := newWithOptionsConnection(t) + + // Create transactions + tx := g.Tx() + + // try to rollback + err := tx.Rollback() + assert.Equal(t, "E1102: cannot rollback a transaction that is not started", err.Error()) + }) + + t.Run("Test commit if no transaction support for Graph", func(t *testing.T) { + skipTestsIfNotEnabled(t, integrationTestSuiteName, testTransactionEnable) + // Start a traversal. + g := newWithOptionsConnection(t) + + // Create transactions + tx := g.Tx() + + _, err := tx.Begin() + assert.Nil(t, err) + + // try to commit + err = tx.Commit() + assert.True(t, strings.HasPrefix(err.Error(), + "E0502: error in read loop, error message '{code:244 message:Graph does not support transactions")) + }) + t.Run("Test WithOptions.Tokens WithOptions.None", func(t *testing.T) { skipTestsIfNotEnabled(t, integrationTestSuiteName, getEnvOrDefaultBool("RUN_INTEGRATION_WITH_ALIAS_TESTS", true)) diff --git a/gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/transaction.js b/gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/transaction.js index 8a242831529..b86f2499f56 100644 --- a/gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/transaction.js +++ b/gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/transaction.js @@ -59,6 +59,10 @@ class Transaction { * @returns {Promise} */ commit() { + if (!this._sessionBasedConnection) { + throw new Error('Cannot commit a transaction that is not started'); + } + return this._sessionBasedConnection.commit().then(() => this.close()); } @@ -66,6 +70,10 @@ class Transaction { * @returns {Promise} */ rollback() { + if (!this._sessionBasedConnection) { + throw new Error('Cannot rollback a transaction that is not started'); + } + return this._sessionBasedConnection.rollback().then(() => this.close()); } diff --git a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/traversal-test.js b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/traversal-test.js index cefd37fcc81..6f7d5b12a0f 100644 --- a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/traversal-test.js +++ b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/traversal-test.js @@ -26,6 +26,7 @@ const Mocha = require('mocha'); const assert = require('assert'); const { AssertionError } = require('assert'); const DriverRemoteConnection = require('../../lib/driver/driver-remote-connection'); +const ResponseError = require('../../lib/driver/response-error'); const { Vertex } = require('../../lib/structure/graph'); const { traversal } = require('../../lib/process/anonymous-traversal'); const { GraphTraversalSource, GraphTraversal, statics } = require('../../lib/process/graph-traversal'); @@ -218,6 +219,38 @@ describe('Traversal', function () { }, (err) => assert.fail("tanked: " + err)); }); }); + describe("should handle tx errors if graph not support tx", function() { + it('should throw exception on commit if graph not support tx', async function() { + const g = traversal().withRemote(connection); + const tx = g.tx(); + tx.begin(); + try { + await tx.commit(); + } catch (err) { + assert.strictEqual("Server error: Graph does not support transactions (500)", err.message); + } + }); + it('should throw exception on rollback if graph not support tx', async function() { + const g = traversal().withRemote(connection); + const tx = g.tx(); + tx.begin(); + try { + await tx.rollback(); + } catch (err) { + assert.strictEqual("Server error: Graph does not support transactions (500)", err.message); + } + }); + it('should throw exception on close if graph not support tx', async function() { + const g = traversal().withRemote(connection); + const tx = g.tx(); + tx.begin(); + try { + await tx.close(); + } catch (err) { + assert.strictEqual("Server error: Graph does not support transactions (500)", err.message); + } + }); + }); describe('support remote transactions - commit', function() { before(function () { if (process.env.TEST_TRANSACTIONS !== "true") return this.skip(); diff --git a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/traversal-test.js b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/traversal-test.js index 17f0557866a..66912664269 100644 --- a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/traversal-test.js +++ b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/traversal-test.js @@ -274,6 +274,27 @@ describe('Traversal', function () { }); }); + describe('not opened transactions', function() { + it('should not allow commit for not opened transactions', async function() { + const g = anon.traversal().withRemote(new MockRemoteConnection()); + const tx = g.tx(); + try { + await tx.commit(); + } catch (err) { + assert.strictEqual('Cannot commit a transaction that is not started', err.message); + } + }); + it('should not allow rollback for not opened transactions', async function() { + const g = anon.traversal().withRemote(new MockRemoteConnection()); + const tx = g.tx(); + try { + await tx.rollback(); + } catch (err) { + assert.strictEqual('Cannot rollback a transaction that is not started', err.message); + } + }); + }); + describe('tx#begin()', function() { it("should not allow a transaction to begin more than once", function() { const g = anon.traversal().withRemote(new MockRemoteConnection()); diff --git a/gremlin-python/src/main/python/gremlin_python/process/graph_traversal.py b/gremlin-python/src/main/python/gremlin_python/process/graph_traversal.py index 83d4402ede9..3cae9b17aa3 100644 --- a/gremlin-python/src/main/python/gremlin_python/process/graph_traversal.py +++ b/gremlin-python/src/main/python/gremlin_python/process/graph_traversal.py @@ -1538,7 +1538,7 @@ def begin(self): def rollback(self): with self.__mutex: # Verify transaction is open, close session and return result of transaction's rollback. - self.__verify_transaction_state(True, "Cannot commit a transaction that is not started.") + self.__verify_transaction_state(True, "Cannot rollback a transaction that is not started.") return self.__close_session(self._session_based_connection.rollback()) # Commits the current transaction. diff --git a/gremlin-python/src/main/python/tests/driver/test_driver_remote_connection.py b/gremlin-python/src/main/python/tests/driver/test_driver_remote_connection.py index d57f3732f60..961b1b70897 100644 --- a/gremlin-python/src/main/python/tests/driver/test_driver_remote_connection.py +++ b/gremlin-python/src/main/python/tests/driver/test_driver_remote_connection.py @@ -228,6 +228,37 @@ def test_close_sessions(self, remote_transaction_connection): # after closing transaction we should remove spawned_session assert 0 == len(remote_transaction_connection._DriverRemoteConnection__spawned_sessions) + def test_tx_on_graph_without_tx_support(self, remote_connection): + g = traversal().withRemote(remote_connection) + tx = g.tx() + try: + # tx is just a session, so no exception here + gtx = tx.begin() + # read operation is ok for sessioned connection + assert 6 == gtx.V().count().next() + tx.commit().all().result() + assert False + except GremlinServerError as ex: + assert "500: Graph does not support transactions" == str(ex) + + def test_tx_commit_on_graph_without_tx_support(self, remote_connection): + g = traversal().withRemote(remote_connection) + tx = g.tx() + try: + tx.commit() + assert False + except Exception as ex: + assert "Cannot commit a transaction that is not started." == str(ex) + + def test_tx_rollback_on_graph_without_tx_support(self, remote_connection): + g = traversal().withRemote(remote_connection) + tx = g.tx() + try: + tx.rollback() + assert False + except Exception as ex: + assert "Cannot rollback a transaction that is not started." == str(ex) + def test_clone(self, remote_connection): g = traversal().withRemote(remote_connection) t = g.V().both() From 16fb886ad5bb3d28ec34ae83f620a45123e6954b Mon Sep 17 00:00:00 2001 From: Valentyn Kahamlyk Date: Thu, 23 Nov 2023 14:23:14 -0800 Subject: [PATCH 2/5] fix js --- .../javascript/gremlin-javascript/lib/process/transaction.js | 4 ++-- .../gremlin-javascript/test/integration/traversal-test.js | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/transaction.js b/gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/transaction.js index b86f2499f56..a60e38ff405 100644 --- a/gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/transaction.js +++ b/gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/transaction.js @@ -63,7 +63,7 @@ class Transaction { throw new Error('Cannot commit a transaction that is not started'); } - return this._sessionBasedConnection.commit().then(() => this.close()); + return this._sessionBasedConnection.commit().finally(() => this.close()); } /** @@ -74,7 +74,7 @@ class Transaction { throw new Error('Cannot rollback a transaction that is not started'); } - return this._sessionBasedConnection.rollback().then(() => this.close()); + return this._sessionBasedConnection.rollback().finally(() => this.close()); } /** diff --git a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/traversal-test.js b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/traversal-test.js index 6f7d5b12a0f..9a599d4ddf2 100644 --- a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/traversal-test.js +++ b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/traversal-test.js @@ -26,7 +26,6 @@ const Mocha = require('mocha'); const assert = require('assert'); const { AssertionError } = require('assert'); const DriverRemoteConnection = require('../../lib/driver/driver-remote-connection'); -const ResponseError = require('../../lib/driver/response-error'); const { Vertex } = require('../../lib/structure/graph'); const { traversal } = require('../../lib/process/anonymous-traversal'); const { GraphTraversalSource, GraphTraversal, statics } = require('../../lib/process/graph-traversal'); @@ -223,7 +222,9 @@ describe('Traversal', function () { it('should throw exception on commit if graph not support tx', async function() { const g = traversal().withRemote(connection); const tx = g.tx(); - tx.begin(); + const gtx = tx.begin(); + const result = await g.V().count().next(); + assert.strictEqual(6, result.value); try { await tx.commit(); } catch (err) { From 1dcecf2464a759e50f3ae655de71f44e7d3dad00 Mon Sep 17 00:00:00 2001 From: Valentyn Kahamlyk Date: Thu, 23 Nov 2023 14:49:52 -0800 Subject: [PATCH 3/5] ... --- .../DriverRemoteConnection/RemoteConnectionFactory.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/RemoteConnectionFactory.cs b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/RemoteConnectionFactory.cs index 565e8c8297c..8fe6c2e7cde 100644 --- a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/RemoteConnectionFactory.cs +++ b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/RemoteConnectionFactory.cs @@ -33,7 +33,7 @@ namespace Gremlin.Net.IntegrationTest.Process.Traversal.DriverRemoteConnection internal class RemoteConnectionFactory : IDisposable { private static readonly string TestHost = ConfigProvider.Configuration["TestServerIpAddress"]; - private static readonly int TestPort = 8182; + private static readonly int TestPort = Convert.ToInt32(ConfigProvider.Configuration["TestServerPort"]); private readonly IList _connections = new List(); private readonly IMessageSerializer _messageSerializer; @@ -46,7 +46,7 @@ public RemoteConnectionFactory(IMessageSerializer messageSerializer = null) public IRemoteConnection CreateRemoteConnection(int connectionPoolSize = 2) { // gmodern is the standard test traversalsource that the main body of test uses - return CreateRemoteConnection("g", connectionPoolSize); + return CreateRemoteConnection("gmodern", connectionPoolSize); } public IRemoteConnection CreateRemoteConnection(string traversalSource, int connectionPoolSize = 2) From a7245bc65739814aa32ac5d924ae5d3d2a3c8a25 Mon Sep 17 00:00:00 2001 From: Valentyn Kahamlyk Date: Fri, 24 Nov 2023 11:05:13 -0800 Subject: [PATCH 4/5] cleanup --- .../DriverRemoteConnection/GraphTraversalTransactionTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/GraphTraversalTransactionTests.cs b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/GraphTraversalTransactionTests.cs index fc0f80ea7b7..257f20b5308 100644 --- a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/GraphTraversalTransactionTests.cs +++ b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/GraphTraversalTransactionTests.cs @@ -23,7 +23,6 @@ using System; using System.Threading.Tasks; -using Gremlin.Net.Driver.Exceptions; using Gremlin.Net.Process.Remote; using Gremlin.Net.Process.Traversal; using Xunit; From c88e6518b67a5547cc4480c9bac1b6663d750a5a Mon Sep 17 00:00:00 2001 From: Valentyn Kahamlyk Date: Tue, 5 Dec 2023 09:31:34 -0800 Subject: [PATCH 5/5] code review suggestions --- .../test/integration/traversal-test.js | 12 ++---------- .../gremlin-javascript/test/unit/traversal-test.js | 2 ++ 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/traversal-test.js b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/traversal-test.js index 9a599d4ddf2..64e1bc5a553 100644 --- a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/traversal-test.js +++ b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/traversal-test.js @@ -227,6 +227,7 @@ describe('Traversal', function () { assert.strictEqual(6, result.value); try { await tx.commit(); + assert.fail("should throw error"); } catch (err) { assert.strictEqual("Server error: Graph does not support transactions (500)", err.message); } @@ -237,16 +238,7 @@ describe('Traversal', function () { tx.begin(); try { await tx.rollback(); - } catch (err) { - assert.strictEqual("Server error: Graph does not support transactions (500)", err.message); - } - }); - it('should throw exception on close if graph not support tx', async function() { - const g = traversal().withRemote(connection); - const tx = g.tx(); - tx.begin(); - try { - await tx.close(); + assert.fail("should throw error"); } catch (err) { assert.strictEqual("Server error: Graph does not support transactions (500)", err.message); } diff --git a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/traversal-test.js b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/traversal-test.js index 66912664269..c6b899971c6 100644 --- a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/traversal-test.js +++ b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/traversal-test.js @@ -280,6 +280,7 @@ describe('Traversal', function () { const tx = g.tx(); try { await tx.commit(); + assert.fail("should throw error"); } catch (err) { assert.strictEqual('Cannot commit a transaction that is not started', err.message); } @@ -289,6 +290,7 @@ describe('Traversal', function () { const tx = g.tx(); try { await tx.rollback(); + assert.fail("should throw error"); } catch (err) { assert.strictEqual('Cannot rollback a transaction that is not started', err.message); }