From bf19ce9e164d3e913acef03288a191330c7aafe8 Mon Sep 17 00:00:00 2001 From: Valentyn Kahamlyk Date: Wed, 6 Dec 2023 12:12:31 -0800 Subject: [PATCH] [TINKERPOP-2913] Ensure that if tx.commit() is called remotely it does not hang for graphs without transactions (#2364) --- CHANGELOG.asciidoc | 1 + .../GraphTraversalTests.cs | 20 +++++++++ gremlin-go/driver/traversal_test.go | 44 +++++++++++++++++++ .../lib/process/transaction.js | 12 ++++- .../test/integration/traversal-test.js | 26 +++++++++++ .../test/unit/traversal-test.js | 23 ++++++++++ .../gremlin_python/process/graph_traversal.py | 2 +- .../driver/test_driver_remote_connection.py | 31 +++++++++++++ 8 files changed, 156 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 1f46886b706..fa640a89dbe 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -24,6 +24,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima === TinkerPop 3.6.7 (NOT OFFICIALLY RELEASED YET) * Improved error message from `JavaTranslator` by including exception source. +* 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-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..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 @@ -59,14 +59,22 @@ class Transaction { * @returns {Promise} */ commit() { - return this._sessionBasedConnection.commit().then(() => this.close()); + if (!this._sessionBasedConnection) { + throw new Error('Cannot commit a transaction that is not started'); + } + + return this._sessionBasedConnection.commit().finally(() => this.close()); } /** * @returns {Promise} */ rollback() { - return this._sessionBasedConnection.rollback().then(() => this.close()); + if (!this._sessionBasedConnection) { + throw new Error('Cannot rollback a transaction that is not started'); + } + + 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 cefd37fcc81..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 @@ -218,6 +218,32 @@ 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(); + const gtx = tx.begin(); + const result = await g.V().count().next(); + 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); + } + }); + 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(); + assert.fail("should throw error"); + } 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..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 @@ -274,6 +274,29 @@ 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(); + assert.fail("should throw error"); + } 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(); + assert.fail("should throw error"); + } 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()